From 2ecb1feb1d051809f0a80f470823b61cc0c1a724 Mon Sep 17 00:00:00 2001 From: David Wei Date: Tue, 22 Sep 2026 15:14:31 +1200 Subject: [PATCH] Show the change id on each change target An agent walking a reviewer through a report refers to blocks by their change id ("see change-010"), but the target carrying that id was a zero-sized span, so the reader had no way to tell which hunk was meant short of guessing from the prose. Move the step's actions out of the right gutter to a row above the step text, replace the LINK wordmark with a chain glyph, and render each change id beside it as its own link to that anchor. Co-Authored-By: Claude Opus 5 (1M context) --- src/report/render.ts | 25 ++++++++++++++----------- test/report-dom.test.ts | 7 +++++-- test/report.test.ts | 9 +++++---- 3 files changed, 24 insertions(+), 17 deletions(-) diff --git a/src/report/render.ts b/src/report/render.ts index 0b4ac10..9cba068 100644 --- a/src/report/render.ts +++ b/src/report/render.ts @@ -153,11 +153,11 @@ function renderSection( .filter((change) => change.canonical) .map( (change) => - ``, + `${escapeHtml(change.id)}`, ) .join('') const actions = `
- ${renderPermalink(stepTarget.fragment, `Permalink to step ${stepIndex + 1} in ${section.title}`, 'LINK')}${changeTargets} + ${renderPermalink(stepTarget.fragment, `Permalink to step ${stepIndex + 1} in ${section.title}`, linkIcon)}${changeTargets}
` if (step.diff === undefined && step.binary === undefined) { return `
${actions}${textMarkup}
` @@ -266,8 +266,12 @@ ${links} ` } -function renderPermalink(fragment: string, label: string, text: string): string { - return `` +// The link glyph is markup, not text, so the caller owns the escaping. +const linkIcon = + '' + +function renderPermalink(fragment: string, label: string, content: string): string { + return `` } function pluralize(count: number, noun: string): string { @@ -495,12 +499,11 @@ main { max-width: none; min-width: 0; margin: 0; padding: 22px 28px 72px; } .section-fold[open] > summary { border-bottom-color: var(--border); } .prose { color: #3c4d41; font-size: 14px; } .step { position: relative; scroll-margin-top: 18px; } -.step-actions { position: absolute; top: 8px; right: 12px; z-index: 2; } -.step-text { max-width: 900px; padding: 8px 68px 8px 20px; font-size: 17px; } -.permalink { display: block; padding: 2px 3px; color: #98a29c; font: 600 10px/20px ui-monospace, SFMono-Regular, Menlo, monospace; text-decoration: none; } -.permalink:hover, .permalink:focus-visible { color: var(--accent); } -.change-target { position: absolute; top: 0; left: 0; width: 0; height: 0; overflow: hidden; scroll-margin-top: 18px; } -.step:not(:has(.step-text)) .step-files { padding-right: 68px; } +.step-actions { display: flex; flex-wrap: wrap; align-items: center; gap: 2px; padding: 8px 20px 0; } +.step-text { max-width: 900px; padding: 4px 20px 8px; font-size: 17px; } +.permalink, .change-target { display: inline-flex; align-items: center; border-radius: 5px; padding: 2px 6px; color: #98a29c; font: 600 10px/14px ui-monospace, SFMono-Regular, Menlo, monospace; text-decoration: none; } +.permalink:hover, .permalink:focus-visible, .change-target:hover, .change-target:focus-visible { color: var(--accent); background: #eef5ef; } +.change-target { scroll-margin-top: 18px; } .prose strong { color: #142c1d; } .step-files { padding: 12px; display: grid; gap: 9px; } .step + .step { border-top: 1px solid #e3ebe5; } @@ -627,7 +630,7 @@ main { max-width: none; min-width: 0; margin: 0; padding: 22px 28px 72px; } .section-fold > summary { font-size: 17px; } } @media print { - .layout-form, .permalink { display: none; } + .layout-form, .permalink, .change-target { display: none; } .review-map { display: none; } .review-workspace { display: block; } .report-cover { box-shadow: none; break-inside: avoid; } diff --git a/test/report-dom.test.ts b/test/report-dom.test.ts index ba9dacc..889824e 100644 --- a/test/report-dom.test.ts +++ b/test/report-dom.test.ts @@ -795,10 +795,13 @@ describe('report browser client', () => { expect(title.tagName).toBe('A') expect(title.textContent).toBe('Linked title') expect(title.tabIndex).toBe(0) - expect(stepLink.textContent).toBe('LINK') + expect(stepLink.getAttribute('href')).toBe(`#${stepLink.closest('.step')!.id}`) + expect(stepLink.querySelector('svg')).not.toBeNull() expect(stepLink.tabIndex).toBe(0) expect(doc.querySelector('[data-copy-fragment]')).toBeNull() - expect(doc.querySelector('a[href="#change-001"]')).toBeNull() + expect(doc.querySelector('a[href="#change-001"]')?.textContent).toBe( + 'change-001', + ) button.dispatchEvent(new dom.window.MouseEvent('click', { bubbles: true, cancelable: true }) as unknown as Event) expect(fold.open).toBe(false) diff --git a/test/report.test.ts b/test/report.test.ts index 5b74f4a..6a20aff 100644 --- a/test/report.test.ts +++ b/test/report.test.ts @@ -442,7 +442,7 @@ describe('renderReport shell', () => { expect(html).not.toContain('id="section-0-step-0-file-0"') }) - test('section titles and step markers are links while change targets stay empty', () => { + test('section titles and step markers are links while change targets name themselves', () => { const value = document([{ title: 'Linkable', steps: [{ text: 'A linkable step.', diff: simplePatch(), changes: ['change-001'] }], @@ -452,10 +452,11 @@ describe('renderReport shell', () => { expect(html).toContain(`Linkable`) expect(html).toContain( - ``, + `', ) - expect(html).toContain('id="change-001" data-target-kind="change">') - expect(html).not.toContain('href="#change-001"') expect(html).not.toContain('data-copy-fragment') })