Fix silent failures of Alt+O / Alt+C shortcuts - #2721
Merged
simon04 merged 6 commits intoSep 14, 2026
Merged
Conversation
Alt+O (open original page) and Alt+C (copy original link) both bail out silently when the rendered entry has no ._attribution ._attribution-link element, which left users of affected documentations with dead shortcuts and no feedback (freeCodeCamp#2634). Alt+C also logged to console on every use and ignored clipboard promise rejections after the navigator.clipboard migration. - Show a transient notice when no original-page link exists - Surface a notice if the clipboard write rejects instead of failing silently; drop the leftover console.log - Notices auto-dismiss after 3s without stacking
There was a problem hiding this comment.
🟡 Changes recommended
Some identified page shapes and browser contexts still fail, and existing notices can obscure the new feedback.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds user-facing feedback when original-page shortcuts cannot locate or copy attribution links.
Changes:
- Adds transient notices for unavailable links and clipboard failures.
- Removes debug logging and handles clipboard promise rejections.
File summaries
| File | Description |
|---|---|
assets/javascripts/views/content/entry_page.js |
Handles shortcut failures and transient notice lifecycle. |
assets/javascripts/templates/notice_tmpl.js |
Defines the new notice messages. |
Review details
Suppressed comments (2)
assets/javascripts/views/content/entry_page.js:235
- The same
:last-childconstraint means this new fallback is shown even when a usable attribution link exists but attribution is not the final child. Alt+O therefore remains broken for a page shape explicitly called out in the investigation; use a selector that does not depend on sibling position.
this.showTransientNotice("noOriginalLink");
assets/javascripts/views/content/entry_page.js:246
- This transient notice can be invisible whenever a persistent
Noticealready exists, such as the single-doc notice (app/app.js:116) or disabled-doc notice (views/pages/hidden.js:10).Notice.show()prepends the new element, and all notices share the same absolute bounds and z-index, so the later persistent sibling paints over it. The transient notice needs an explicit stacking/lifecycle strategy that keeps it above an existing notice.
this.transientNotice = new app.views.Notice(type);
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The `._attribution:last-child` lookup failed whenever the attribution was followed by another element, so Alt+O / Alt+C reported the link as unavailable even though it existed. Match the last attribution link instead, which keeps the previous intent (the appended attribution wins over any inside the page content) without depending on sibling position.
`navigator.clipboard` is undefined in unsupported or non-secure browser contexts, so Alt+C threw before the rejection handler could report the failure. Check for the API up front and show the same notice.
All notices share the same absolute bounds and z-index, and `Notice.show()` prepends, so the transient notice was painted over by an existing single-doc or disabled-doc notice. Raise it with a `_notice-transient` class, and route every teardown through `hideTransientNotice()` so the notice can't outlive the page it was shown on.
There was a problem hiding this comment.
🟡 Changes recommended
A delayed clipboard rejection can display a stale failure notice after navigation.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
Comment on lines
+239
to
+241
| navigator.clipboard | ||
| .writeText(link.href + location.hash) | ||
| .catch(() => this.showTransientNotice("copyFailed")); |
There was a problem hiding this comment.
🟡 Changes recommended
Transient notices can incorrectly persist across entry-to-entry navigation.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Balanced
|
|
||
| deactivate() { | ||
| if (super.deactivate(...arguments)) { | ||
| this.hideTransientNotice(); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2634
What this PR does
Alt+O (open original page) and Alt+C (copy original link) currently fail completely silently when the rendered entry has no
._attribution ._attribution-linkelement, and Alt+C additionally had a leftoverconsole.logand ignored clipboard promise rejections. This PR shows a transient notice when the link is unavailable, surfaces clipboard failures instead of swallowing them, and removes the debug logging.Investigation notes
While debugging why the shortcuts "do nothing" for some users and work for others:
_attributionblock (verified against e.g. git-status.html), so the selector input exists at the data level.return, every one of those cases produced identical "nothing happens" behavior, making the bug impossible to self-diagnose from the UI. Surfacing a notice turns the invisible state into actionable feedback.Changes
entry_page.js: both handlers now show a transient notice ("The original page link is not available for this documentation") instead of silently returning;onAltChandles thenavigator.clipboard.writeTextrejection with its own notice; removed strayconsole.lognotice_tmpl.js: addednoOriginalLinkNoticeandcopyFailedNoticetemplates following the existing patternTesting
DevDocs has no JS unit-test infrastructure yet (#2673), so verification was: syntax check on both modified files plus manual code-path review against the cached-entry structure linked above. The notice path can be exercised by running any entry whose scraped HTML lacks an attribution block.
Happy to iterate if maintainers would prefer different wording/behavior (e.g. also attempting URL reconstruction for docs without attribution).