Add section shortcut links - #205
Conversation
Amp-Thread-ID: https://ampcode.com/threads/T-01a0449e-4249-720c-b618-ef3d17bd26ad Co-authored-by: Amp <amp@ampcode.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 661be379a0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| permalink.setAttribute("aria-label", `Copy link to ${headingText}`) | ||
| permalink.title = "Copy link to this section" | ||
| permalink.innerHTML = '<svg width="16" height="16" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round" aria-hidden="true"><path d="M10 13a5 5 0 0 0 7.54.54l3-3a5 5 0 0 0-7.07-7.07l-1.72 1.71"/><path d="M14 11a5 5 0 0 0-7.54-.54l-3 3a5 5 0 0 0 7.07 7.07l1.71-1.71"/></svg>' | ||
| permalink.addEventListener("click", event => this.copySectionLink(event)) |
There was a problem hiding this comment.
This direct listener is not a last-resort case: the newly created link is inside the connected controller and can use a data-action binding to invoke copySectionLink. Keeping the listener here violates the repository's explicit requirement to prefer standard Stimulus bindings whenever Stimulus can bind the element.
AGENTS.md reference: AGENTS.md:L90-L90
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Updated by Amp 🤖 — the generated permalink now uses a Stimulus data-action instead of a direct listener.
| const permalink = document.createElement("a") | ||
| permalink.className = "section-permalink" |
There was a problem hiding this comment.
When an open plan receives a live revision, live_update_controller.js replaces the children of #plan-content-body, but this ancestor controller remains connected and buildToc() is not called again. The replacement headings therefore never pass through this permalink creation block, so every section shortcut disappears until a full reload; listen for the in-place content replacement and rebuild the heading state rather than relying solely on connect().
AGENTS.md reference: AGENTS.md:L76-L80
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Updated by Amp 🤖 — live body replacements now dispatch a bubbling content-updated event; the content-nav action rebuilds headings, shortcut links, the outline, and scroll tracking. The checkbox live-update system spec verifies the rebuilt link.
| const link = event.currentTarget | ||
|
|
||
| try { | ||
| await navigator.clipboard.writeText(link.href) |
There was a problem hiding this comment.
When the plan was opened from a notification using ?thread=<id>, the fragment-only href resolves against that full URL, so copying link.href retains the thread parameter. Opening the resulting section link causes text_selection_controller to auto-open and scroll to that comment thread, overriding the section destination; construct the copied URL from the canonical plan URL or remove transient query parameters before writing it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Updated by Amp 🤖 — shortcut hrefs now start from the canonical plan URL and strip query parameters before adding the section fragment. Coverage opens through a transient thread query and verifies it is absent from the shortcut.
There was a problem hiding this comment.
Follow-up by Amp 🤖 — this now derives from the current page URL (so historical versions retain their own path), strips its query, and adds only the section fragment.
Amp-Thread-ID: https://ampcode.com/threads/T-01a0449e-4249-720c-b618-ef3d17bd26ad Co-authored-by: Amp <amp@ampcode.com>
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Amp-Thread-ID: https://ampcode.com/threads/T-01a0449e-4249-720c-b618-ef3d17bd26ad Co-authored-by: Amp <amp@ampcode.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Amp-Thread-ID: https://ampcode.com/threads/T-01a0449e-4249-720c-b618-ef3d17bd26ad Co-authored-by: Amp <amp@ampcode.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0196a12233
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| display: flex; | ||
| align-items: center; |
There was a problem hiding this comment.
For presentation plans with a title slide, buildToc() adds this class to the slide's heading, so display: flex turns the h1::before accent bar defined in deck.css into a horizontal flex item beside the title instead of the block above it. This visibly breaks the title-slide composition—and can reflow it when Stimulus connects—in both the embedded deck and presentation mode; scope the row layout away from deck title headings or explicitly preserve their vertical accent layout.
AGENTS.md reference: AGENTS.md:L87-L89
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Updated by Amp 🤖 — title-slide headings now use a two-column grid with the accent spanning the full first row, preserving the accent above the title while centering the shortcut beside it. Added browser coverage for both the accent placement and title/link centerlines.
Amp-Thread-ID: https://ampcode.com/threads/T-01a0449e-4249-720c-b618-ef3d17bd26ad Co-authored-by: Amp <amp@ampcode.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50ee32c9b2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const sectionUrl = new URL(pageUrl) | ||
| sectionUrl.hash = id | ||
| permalink.className = "section-permalink" | ||
| permalink.href = sectionUrl.href |
There was a problem hiding this comment.
When another element outside the H1–H3 outline already owns this generated ID, the shortcut copies a fragment that resolves to that earlier element rather than this heading. For example, the Markdown renderer assigns section-1 to a numbered H4 (markdown_helper.rb:119-125), but a later H2 titled “Section 1” also receives section-1 because usedIds only tracks the selected H1–H3 headings; its copied shortcut therefore opens the H4. Reserve all existing document IDs before choosing the shortcut target ID.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Updated by Amp 🤖 — generated Commonmarker self-anchors are replaced by the shortcut, and every remaining document ID is reserved before heading IDs are assigned. Browser coverage injects an existing fragment and verifies the heading receives a unique suffixed target.
| .section-permalink[data-copy-state]::after { | ||
| content: attr(data-copy-message); | ||
| position: absolute; | ||
| left: calc(100% + var(--space-xs)); |
There was a problem hiding this comment.
On a narrow viewport with a long heading, the flex layout places the shortcut at the card's right edge, and this absolute pseudo-element always opens to its right. The “Copied!”/failure bubble consequently extends beyond the viewport and creates horizontal overflow or partially hidden feedback; position it inward at phone widths or otherwise clamp it to the viewport.
AGENTS.md reference: AGENTS.md:L87-L89
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Updated by Amp 🤖 — at phone widths the feedback bubble is right-aligned to the shortcut so it opens inward. Browser coverage clicks it at 390px and verifies both the right edge and absence of horizontal overflow.
Amp-Thread-ID: https://ampcode.com/threads/T-01a0449e-4249-720c-b618-ef3d17bd26ad Co-authored-by: Amp <amp@ampcode.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbf6a30a98
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| this._headings.forEach(heading => { | ||
| heading.querySelectorAll("a.anchor[id]").forEach(anchor => anchor.remove()) |
There was a problem hiding this comment.
When a plan has repeated unnumbered headings, Commonmarker assigns fragments such as intro and intro-1, but removing those anchors and regenerating IDs with this controller's -2 suffix convention leaves the second heading as intro-2. Existing bookmarks and authored links to #intro-1 then have no target; preserve the generated anchor ID when promoting the fragment target to the heading, or retain an equivalent alias.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Updated by Amp 🤖 — generated duplicate fragments are now promoted verbatim from each empty self-anchor (for example intro and intro-1) instead of regenerated. A browser spec locks those exact IDs.
| // shortcut replaces it; keeping both would leave two elements claiming | ||
| // the same fragment once the heading itself receives that id. | ||
| this._headings.forEach(heading => { | ||
| heading.querySelectorAll("a.anchor[id]").forEach(anchor => anchor.remove()) |
There was a problem hiding this comment.
For a heading containing allowed raw HTML such as <a class="anchor" id="details" href="/details">Details</a>, this selector also removes the author-supplied link and its visible text. The Markdown sanitizer permits a, class, id, and href, so distinguish Commonmarker's empty self-link from authored descendants before deleting it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Updated by Amp 🤖 — promotion is now restricted to empty anchors whose href is their own fragment. Visible and non-self authored links remain intact, with browser coverage for an authored class=anchor link.
Amp-Thread-ID: https://ampcode.com/threads/T-01a0449e-4249-720c-b618-ef3d17bd26ad Co-authored-by: Amp <amp@ampcode.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Why
Plan sections need durable, shareable URLs without making readers hunt through the browser address bar.
What
Risk Assessment
Low risk. This is isolated client-side presentation and clipboard behavior on rendered plan headings, with browser coverage for visibility and copying.
References
Generated with Amp