Skip to content

feat(viewer): add collapse-all and hand a finding to a coding agent - #5

Open
SaahiilM wants to merge 13 commits into
mainfrom
feat/viewer-agent-handoff
Open

SaahiilM wants to merge 13 commits into
mainfrom
feat/viewer-agent-handoff

Conversation

@SaahiilM

@SaahiilM SaahiilM commented Oct 19, 2026 •

Copy link
Copy Markdown
Owner

Description

Two affordances for working through a review from the viewer's session page.

Collapse all folds every per-file finding group down to a one-line summary,
so a page with hundreds of findings becomes a scannable index of files and
their counts. Groups hidden by the current filters are left alone, so what
looks open stays in step with what is visible.

Fix with agent carries one finding - file, line range, severity, category,
description and both versions of the code - into Claude Code, Codex or Cursor,
prefilled. Targets are ordered app then editor extension, using each vendor's
documented scheme. No vendor documents a prefilled-prompt scheme for its CLI,
so the CLI is reached by copying the same text instead of a link that would
silently do nothing.

Every one of these prefills a composer without submitting, so the user always
confirms before an agent runs.

Why this is a separate PR

This branch is based on upstream main and contains only the two features
above. The live-session-status work that shared these commits has moved to its
own PR, because upstream already has an implementation of it that would
conflict at the symbol level. Keeping the two apart means this PR is a clean
delta rather than a duplicate.

Nothing in this PR depends on that work: the handoff and the collapse-all
control read only the session summary and the rendered comment, both of which
are unchanged on main.

What each commit does

Commit Change
feat(viewer) Collapse-all control; per-finding agent handoff
fix(viewer) Build deep links before the click that follows them
fix(viewer) Share the dismiss listener; harden the prompt
fix(viewer) Address the remaining review findings
docs viewer.md in all five locales

Each builds and passes tests standalone.

Two things worth reviewing closely

The deep-link fix is a separate commit on purpose. The first implementation
did not work on any platform: the click handler cancelled the click, assigned
the href, then re-dispatched the click via setTimeout. That deferred click
runs outside the transient user activation browsers require before handing an
external protocol to the desktop, so it was silently dropped. Headless
Chromium showed zero navigations requested before the fix, one after. The
commits are split so the regression and its fix are each reviewable on their
own.

The prompt truncation is not a plain slice. A slice can land inside a
fenced code block and leave the fence unclosed, or land between the halves of a
surrogate pair and emit invalid UTF-16. The cut instead walks back to a line
boundary, never splits a pair, and closes an unbalanced fence.

The dismiss handler is installed once for the page rather than once per
finding. A session with hundreds of findings previously ran hundreds of
document-level listeners on every click anywhere in the viewer; measured at 2
listeners for 40 findings, down from 41.

What the review changed

The branch was reviewed with the project's own tool, and the findings were
verified against a real browser rather than taken on trust. Six of them were
real and are fixed:

  • The dropdown was clipped. The comment group clips its overflow to round
    its corners, and the absolutely positioned menu was cut off at the group's
    bottom edge - 26 pixels of a 126-pixel menu were visible. The group now
    lifts its clip while a menu is open.
  • The prompt was truncated on the wrong measure. The vendors cap the
    URL-encoded value, but the truncation counted raw characters, and encoding
    inflates non-ASCII text several times over. It is now truncated on the
    encoded length.
  • The line-boundary walk-back never fired for a long prompt, so the cut
    landed mid-line.
  • The prompt named no repository, so the copy-to-clipboard route and two
    of the deep-link targets received a relative file path with no root to
    resolve it against. The repository is now named in the prompt.
  • The finding's text was shipped twice - in the card and again in a data
    attribute - which roughly doubled the size of a session page (313 KB over
    200 findings). It is now read from the rendered card.
  • The collapse-all control contradicted itself: its label named the action
    while aria-pressed announced the opposite state, and a group hidden by a
    filter kept its open state, so clearing the filter revealed a group still
    open under a button reading "Expand all". Both are corrected.

The menu also claimed the ARIA menu-button pattern without implementing it,
and the copy confirmation was a live region that was hidden while its text
changed, so it was never announced. Both are fixed.

A later round of the review found three more, each confirmed in a browser:

  • Two menus could be open at once. The toggle's click stopped
    propagating, so the page-level dismiss handler never saw it and the
    previously open menu stayed open.
  • The workspace parameter was not a valid file URI. Concatenating
    file:// with the path leaves a Windows path with backslashes and no
    third slash, and a path containing # or ? would start a fragment or
    a query. The URL parser does both jobs.
  • The dismissal had two copies that could drift apart. There is one
    now.

The collapse-all control also skipped groups the pager had hidden, so a
group on the next page stayed open while the button read "Expand all";
every group is collapsed now.

Known limitation

cursor:// is documented by Cursor but no handler is registered on this
machine
, so that entry does nothing here until Cursor is installed.
claude-cli://, codex:// and vscode:// are all registered and verified
working.

No vendor documents a prefilled-prompt scheme for its CLI, so the CLI is
reached by copying the prompt rather than by a link that would silently do
nothing. Every target prefills without submitting - the user always confirms
before an agent runs.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring (no functional changes)
  • Documentation update
  • CI / Build / Tooling

How Has This Been Tested?

  • make test passes locally
  • Manual testing (describe below)

make check, make test (0 failures) and make coverage (91.4%, threshold
90%) all pass.

Manual testing, against the real viewer serving a real session:

  • The deep-link regression was reproduced and fixed under headless Chromium
    over the DevTools protocol, driving a trusted click and counting the
    navigations the browser actually attempted.
  • The menu was measured before and after the clipping fix by sampling the
    rendered pixels down its own centre line: 26 of 126 pixels visible before,
    the full menu after.
  • The prompt was read back out of a generated deep link and out of the
    clipboard, and both carry the finding, the repository, the current code and
    the suggested change.
  • The copy confirmation was checked to be in the accessibility tree while it
    carries text, so it is announced.
  • Collapse-all was driven through a filtered view: the group hidden by the
    filter now collapses with the rest.
  • Truncation was exercised with prompts long enough to actually hit the clamp,
    including mostly non-ASCII text and a cut that lands inside a code block.
  • Scheme handlers on this machine confirmed with xdg-mime query default.

New tests cover the handoff context, the collapse-all control, the exported
page carrying the script, and regressions that fail if the re-dispatched click
or the per-finding listener are ever reintroduced.

Checklist

  • My code follows the project's coding style (go fmt, go vet)
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or my feature works
  • New and existing unit tests pass locally with my changes
  • I have updated the documentation accordingly (if applicable)
  • I have signed the CLA
  • I did not use AI/LLM to create this PR, or I disclosed the tool/model below and reviewed its output; I did not attribute commits to AI and will answer maintainer questions and review comments myself without AI/LLM.

AI disclosure: this PR was written with AI assistance (Claude Code, model
claude-fable-5). I have reviewed the diff myself, including the deep-link
regression and the prompt-truncation behaviour, both of which were verified
against a real browser rather than assumed. The commits carry no AI
attribution.

🤖 Generated with Claude Code


Opened upstream as alibaba#1782. The CLA is signed and the
PR commit policy passes; the remaining workflows are waiting on a maintainer to
approve them, which is the usual gate for a first-time contributor's fork PR.

Two affordances for working through a review from the session page.

Collapse all folds every per-file finding group to a one-line summary, so
a page with hundreds of findings becomes a scannable index of files and
their counts. Groups hidden by the current filters are left alone, keeping
what looks open in step with what is visible.

Fix with agent carries one finding - file, line range, severity, category,
description and both versions of the code - into Claude Code, Codex or
Cursor, prefilled. Targets are ordered app then editor extension, using
each vendor's documented scheme. No vendor documents a prefilled-prompt
scheme for its CLI, so the CLI is reached by copying the same text instead
of a link that would silently do nothing.

Every one of these prefills a composer without submitting, so the user
always confirms before an agent runs.
The menu item handler cancelled the click, assigned the deep link to the
anchor's href, and re-dispatched the click on a later turn of the event
loop. That deferred click runs outside the transient user activation every
current browser requires before handing an external protocol to the
desktop, so the navigation was dropped and the click did nothing at all.

Verified in headless Chromium: zero navigations requested before this
change, one after.

The href is now built while the menu opens, which is itself a real click,
and the item's own click is left to the browser.

Placeholder hrefs become about:blank rather than "#", so a link followed
before the script rewrites it cannot scroll the page.
agent.js registered one document-level click listener per finding, so a
session with hundreds of findings ran hundreds of listeners on every click
anywhere in the viewer. The dismiss handler is now installed once and only
touches menus that are open: 2 listeners for 40 findings, down from 41.

Prompt truncation used a plain slice, which could cut inside a fenced code
block and leave it unclosed, or split a surrogate pair and emit invalid
UTF-16. It now backs off to a line boundary, never splits a pair, and closes
an unbalanced fence.

Also replaces two `var`s with `const` in code that uses const throughout.
Documents what the collapse-all control does and how the per-finding agent
handoff works, including that no vendor prefills a CLI prompt and that every
target waits for the user to confirm before running.

All five locales updated.
The dropdown was clipped by the comment group's overflow, so the menu was
effectively invisible; the group now lifts its clip while a menu is open.

The prompt was truncated on its raw character count, but the vendors
measure the URL-encoded value, and encoding inflates non-ASCII text
several times over. It is now truncated on the encoded length.

The line-boundary walk-back compared against the whole text rather than
the cut region, so it never fired for a long prompt and the cut landed
mid-line.

The prompt named no repository, so the copy-to-clipboard route and two
of the deep-link targets received a relative file path with no root to
resolve it against.

The finding's text was shipped twice, in the card and again in a data
attribute, which roughly doubled the size of a session page; it is now
read from the rendered card.

The menu claimed the ARIA menu-button pattern without implementing it,
the live region was hidden while its text changed, and the collapse-all
control's toggle attribute contradicted its label. Each is corrected.
@SaahiilM
SaahiilM force-pushed the feat/viewer-agent-handoff branch from e1a3cd1 to 59f5833 Compare October 19, 2026 18:18
@SaahiilM

Copy link
Copy Markdown
Owner Author

The branch was reviewed with the project's own tool. The findings were verified against a real browser rather than taken on trust, and six were real:

Finding What was wrong Fix
Dropdown clipped (high) The comment group clips its overflow to round its corners, so the absolutely positioned menu was cut off at the group's bottom edge. Measured: 26 of 126 pixels visible. The group lifts its clip while a menu is open.
Truncation measured the wrong thing The vendors cap the URL-encoded value, but the truncation counted raw characters; encoding inflates non-ASCII text several times over. Truncation is now on the encoded length.
Line-boundary walk-back dead The guard compared against the whole text rather than the cut region, so it never fired for a long prompt and the cut landed mid-line. The guard is relative to the cut position.
Prompt named no repository The copy-to-clipboard route and two of the five deep-link targets received a relative file path with no root to resolve it against. The repository is named in the prompt.
Text shipped twice The finding's content, existing code and suggestion code were rendered in the card and repeated in data attributes, roughly doubling the size of a session page (313 KB over 200 findings). The text is read from the rendered card.
Collapse-all contradicted itself The label named the action while aria-pressed announced the opposite state, and a group hidden by a filter kept its open state, so clearing the filter revealed a group still open under a button reading "Expand all". The label carries the state, and every group collapses.

Two more were fixed on the same grounds: the menu claimed the ARIA menu-button pattern without implementing it (now a plain disclosure list), and the copy confirmation was a live region that was hidden while its text changed, so it was never announced (it now stays in the accessibility tree).

Also added static/agent.js to TestEmbeddedAssetsHaveNoTerminators, which guards the export's inlining contract.

Re-verified after the fixes: make check, make test (0 failures) and make coverage (91.4%, threshold 90%) all pass, and the prompt was read back out of a generated deep link and out of the clipboard to confirm it still carries the finding, the repository, the current code and the suggested change.

The prompt is truncated on its URL-encoded length, because that is what the
vendors cap, and encoding inflates non-ASCII text several times over. The
docs still said the prompt was cut to a fixed character count.
The comment still said the context travels as data-* attributes, which was
true before the finding's text moved to being read from the rendered card.
The toggle's click stopped propagating, so the page-level dismiss handler
never saw it: opening a second menu left the first one open, so two
dropdowns were on screen at once. The handler already skips clicks that
land inside its own root, so the toggle's click is not dismissed by it
either way.

The workspace parameter is a file URI, and concatenating "file://" with
the path does not produce one: a Windows path keeps its backslashes and
gains no third slash, and a path containing "#" or "?" would start a
fragment or a query. The URL parser does both jobs.

The dismissal had two copies that could drift apart. There is one now.
@SaahiilM

Copy link
Copy Markdown
Owner Author

A later round of the review found three more defects. Each was confirmed in
a browser before it was fixed, and each has a regression test:

  • Two menus could be open at once. The toggle's click called
    stopPropagation(), so the page-level dismiss handler never saw it and the
    previously open menu stayed open. Verified: opening a second menu left the
    first one open. The handler already skips clicks that land inside its own
    root, so the toggle's click is not dismissed by it either way, and the
    stopPropagation() was doing nothing but breaking the other menu.
  • The workspace parameter was not a valid file URI. "file://" + dir
    leaves a Windows path with backslashes and no third slash
    (file://C:\code\proj), and a path containing # or ? starts a
    fragment or a query. The URL parser does both jobs; it now produces
    file:///C:/code/proj and file:///my/pr%23oj/proj.
  • The dismissal had two copies — one in the per-finding handlers and one
    in the page-level handler — which could drift apart. There is one now.

The collapse-all control also skipped groups the pager had hidden, so a group
on the next page stayed open while the button read "Expand all". Every group
is collapsed now.

The prompt names the repository so an agent that gets no workspace
parameter still knows which tree to work in, and the docs did not say so.
fix(tool): report an empty code_comment batch instead of success (alibaba#1773)
The exported page concatenated the scripts a11y, pager, session, agent, while the served page loads a11y, pager, agent, session. The two pages are the same page, so they must run the same scripts in the same sequence.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant