Repository navigation
Conversation
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.
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.
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.
Open
11 of 15 tasks
Contributor
|
🔍 OpenCodeReview found 1 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Comment on lines
62
to
+64
| js := append(append(append([]byte(nil), a11y...), '\n'), pagerJS...) | ||
| js = append(append(js, '\n'), sessionJS...) | ||
| js = append(append(js, '\n'), agentJS...) |
Contributor
There was a problem hiding this comment.
The script concatenation order here (a11y → pager → session → agent) differs from the live template's load order (a11y → pager → agent → session). While both scripts are currently independent IIFEs, this inconsistency means the exported page executes scripts in a different order than the served page. If session.js ever comes to depend on something initialized by agent.js (or vice versa), the two modes will silently diverge. Match the template's order by appending agentJS before sessionJS.
Suggestion:
Suggested change
| js := append(append(append([]byte(nil), a11y...), '\n'), pagerJS...) | |
| js = append(append(js, '\n'), sessionJS...) | |
| js = append(append(js, '\n'), agentJS...) | |
| js := append(append(append([]byte(nil), a11y...), '\n'), pagerJS...) | |
| js = append(append(js, '\n'), agentJS...) | |
| js = append(append(js, '\n'), sessionJS...) |
Author
There was a problem hiding this comment.
Fixed in b6608b7. The export now concatenates a11y → pager → agent → session, matching the template's load order, and a new test (TestExportSession_ScriptOrderMatchesTheTemplate) reads the order out of templates/session.html and asserts the exported page inlines the scripts in that same sequence — it fails against the previous order.
This was referenced Oct 20, 2026
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.
This branch has not been deployed
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.
Description
Two affordances for working through a review from the viewer's session page.
Collapse all folds every per-file finding group down to its one-line
summary, which turns a session with hundreds of findings into a scannable
index of files and their counts. The button toggles to Expand all.
Fix with agent carries one finding — its file, line range, severity,
category, description, both versions of the code, and the repository path —
into Claude Code, Codex, Cursor, or the VS Code extensions, prefilled. A
Copy prompt for agent action puts the same text on the clipboard instead,
for pasting into a terminal.
Every target prefills a composer and stops: nothing is submitted for you, so
you read the prompt and send it.
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. Nine of them were
real and are fixed:
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.
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.
of the five deep-link targets received a relative file path with no root to
resolve it against. The repository is now named in the prompt.
attribute, which roughly doubled the size of a session page. It is now read
from the rendered card.
propagating, so the page-level dismiss handler never saw it and the
previously open menu stayed open.
file://with the path leaves a Windows path with backslashes and nothird slash, and a path containing
#or?would start a fragment ora query. The URL parser does both jobs.
on the next page stayed open while the button read "Expand all".
and the copy confirmation was a live region that was hidden while its text
changed, so it was never announced.
The dismissal also had two copies that could drift apart. There is one now.
The
code-reviewworkflow reported one more, on the export path: the exportedpage concatenated its scripts in a different order from the served one, so the
same page ran them in a different sequence depending on how it was served.
That is fixed too, and a test now pins the export to the template's order.
Type of Change
How Has This Been Tested?
make testpasses locallymake check,make test(0 failures) andmake coverage(91.4%, threshold90%) all pass.
Manual testing, against the real viewer serving a real session and against
an exported single-file page:
clipboard, and both carry the finding, the repository, the current code
and the suggested change.
the code sections.
file://with no network — producesthe same prompt.
line, to confirm it is not clipped by the group whose corners it inherits.
carries text, so it is announced.
control's state was checked against what the groups actually do.
were open at once.
#, which now producefile:///C:/code/projandfile:///my/pr%23oj/proj.Notes for reviewers
The
code-reviewworkflow was failing onmainfor every PR, includingother contributors', with the runner reporting
Can't find 'action.yml'. Thatwas the workflow's own sparse checkout polluting the shared runner
workspace, not a finding against any branch. Upstream has since fixed it
(fix(ci): prevent PR policy checkout state pollution #1783), and this branch merges that fix. The workflow only triggers on
opened, so a check from this branch's opening commit will not re-run byitself — a maintainer with write access has to re-run it, and it now gets
past the checkout. The same review on feat(viewer): show live sessions as running with a ticking duration #1786 passed after the fix.
The workflow runs on this PR are waiting to be approved. Every one of
them shows as
action_requiredwith no jobs at all, which is GitHub holdinga fork pull request's workflows until a maintainer approves them — the
Checks tab offers "Approve and run workflows". Nothing has failed and
nothing is waiting for a runner: the jobs have not started. The same ground
is covered locally by
make check,make testandmake coverage, whichare what the CI
testjob runs, and the repository's ownAnalyze (go)andwindowsjobs pass on the fork.Checklist
go fmt,go vet)AI disclosure: this PR was written with AI assistance (Claude Code, model
claude-fable-5). I have reviewed the diff myself, and the behavioural claims
above were verified against a real browser rather than assumed. The commits
carry no AI attribution.
🤖 Generated with Claude Code