Review: the diff fits the window, the rail means something, and comments come back - #11
Merged
Conversation
…nts come back Four reported defects, all in the Review destination. The diff column rendered wider than its own viewport. Measured in the running app: a 606px column drawing 1437px cells. Rows bound their prefWidth to the ListCell, and a ListCell's own preferred width is its graphic's plus 24px of padding, which a ListView then uses as the breadth for every cell -- a feedback loop that added 24px a pass until the hunk-card frames hung off the right edge behind a horizontal scrollbar the reader had to use just to see the borders of cards whose content was short. Rows now size to the list's viewport, which is handed down by the skin and cannot feed back, and long lines wrap instead of scrolling. The pin can finally sit at the card's right edge, where the design always wanted it. Clicking the gutter did nothing. The inline annotation composer lived in the old Review sub-tab and was not carried over when Review became a destination, so a gutter click only flickered the list selection. It is back: one composer at a time, anchored under its line, ⌘⏎ to send and Esc to cancel, stored as a NIT authored by "You" in the same store the margin and the ◆n pins already render from. The empty surface rendered the full chrome with the content removed -- an empty queue rail with a live filter field, an intent rail reading 0/0, a margin claiming "Nothing flagged in this intent" with no intent, a verdict bar with dead arrows and a disabled Submit, and the state's title printed twice. It is one centred state now. The rail was disconnected from the code and could not tell its own entries apart. Selecting an intent only scrolled, which on a 45-file diff was indistinguishable from doing nothing; the column narrows to that intent's hunks now, with a "whole scope" chip as the way back out. And the no-reviewer fallback was one intent per file: 45 cards whose titles all clipped to the same app/src/main/java/app/dry… prefix, all tagged "change", all carrying the same rationale and the same flat heat bar. FallbackIntents clusters by directory and kind, infers tests/config/generated from the path, and takes risk from churn. Two things fell out of the regrouping. A finding whose intentId matches no current intent used to disappear from every margin rather than being shown somewhere; it now falls back to matching by file, because a finding is something a human or an agent went to the trouble of writing down. And the fallback addresses hunks by id exactly as a reviewer's grouping does, so the column's filter and the scroll-into-view read the same thing. Verified by running the real app -- maxCell 1437 -> 597 inside a 606px column with the scrollbar gone -- since the FX layer has no headless harness there; diag.diffWidths and the explorerScript "comment" verb are what made that checkable rather than eyeballed. 1142 tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Verdicts are persisted by intent id, and the fallback grouping's ids changed
when it stopped emitting one intent per file (file:<path> -> auto:<kind>:<dir>).
Every approval given before that read as unsettled, so a finished review asked
to be done again.
AnnotationStore.migrateLegacyVerdicts carries them onto the intent that now
contains those files. The merge is asymmetric, because the two directions do
not carry the same risk:
- Any CHANGES among a group's files makes the group CHANGES. "Something in
here needs work" stays true however the group is drawn.
- APPROVED requires EVERY file of the group to have been settled. Approving
a group claims the human read all of it, so a partially-approved group
carries nothing forward and is re-settled by hand. Silently approving code
nobody looked at is the one outcome a migration must never produce.
A partial group keeps its legacy verdicts rather than having them deleted --
they record something the human really did decide, and the grouping may change
again. A human APPROVED outranks an agent's AUTO_APPROVED in the merged result,
so the carried verdict never claims less human attention than was paid. Each
migrated verdict carries a note saying it was carried over, so its provenance
is not misrepresented as a fresh decision.
It runs from the host's intents() call, which is the only moment both the scope
and its grouping are known, and is idempotent and near-free once there is
nothing left to carry. It deliberately does not fire a store change: every
other mutator does, but this one runs inside the render that is about to read
its result, and firing would re-enter that render through the store's own
listener. The write is still persisted.
FakeReviewHost migrates too. A fake that skipped it would agree with the real
host right up until the migration broke, which is the one moment a fake earns
its keep.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ReviewCommentComposerTest failed on CI and passed locally: the comment was stored correctly, but anchored to a line the test had not meant to click. The helper took gutters().get(0). lookup() returns scene-graph order, and a virtualized ListView recycles and reorders its cells, so "the first gutter" is whichever row happens to own the first reused cell. That is line 1 often enough to be green on this machine and red on the runner. Gutters are now selected by their rendered line number, which is what the test meant all along -- the anchor is the thing under test, so choosing the line by position was choosing it by accident. The same latent order-dependence in onlyOneComposerIsEverOpen goes with it. No product change: buildLine installs a fresh handler on every updateItem, so a recycled cell's gutter always reports the row it is currently rendering. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Four usability defects reported against the Review destination, plus the data migration the fourth one made necessary.
The diff column rendered wider than its own viewport
Measured in the running app: a 606px column drawing 1437px cells. Rows bound their
prefWidthto theListCell, but aListCell's own preferred width is its graphic's plus 24px of padding, whichListViewthen applies as the breadth for every cell — a feedback loop adding 24px a pass. The hunk-card frames ended up off the right edge behind a horizontal scrollbar the reader had to use just to see the borders of cards whose content was short.Rows now size to the list's viewport, which is handed down by the skin and cannot feed back, and long lines wrap instead of scrolling. After:
maxCell=597in a 606px column, no scrollbar. The◆npin can finally sit at the card's right edge, where the design always wanted it.Clicking the gutter did nothing
The inline annotation composer lived in the old Review sub-tab and was not carried over when Review became a destination, so a gutter click only flickered the list selection and there was no way at all to write a comment.
It is back: one composer at a time, anchored under its line,
⌘⏎to send andEscto cancel, stored as a NIT authored by "You" in the same store the findings margin and the◆npins already render from.The empty surface was chrome around nothing
It rendered the full layout with the content removed — an empty queue rail with a live filter field, an intent rail reading
0/0, a margin claiming "Nothing flagged in this intent" with no intent, a verdict bar with dead arrows and a disabled Submit, and the state's title printed twice. It is one centred state now.The rail was disconnected from the code, and could not tell its own entries apart
Selecting an intent only scrolled, which on a 45-file diff was indistinguishable from doing nothing. The column now narrows to that intent's hunks, with a
whole scopechip as the way back out.And the no-reviewer fallback was one intent per file: 45 cards whose titles all clipped to the same
app/src/main/java/app/dry…prefix, all taggedchange, all carrying the same rationale and the same flat heat bar. Every card was individually correct and the rail as a whole was unreadable.FallbackIntentsclusters by directory and kind, infers tests/config/generated from the path, and takes risk from churn.Two things that fell out of the regrouping
A finding whose
intentIdmatched no current intent used to disappear from every margin rather than being shown somewhere. It now falls back to matching by file — a finding is something a human or an agent went to the trouble of writing down, and regrouping around it must not lose it.Verdicts are persisted by intent id, so the id change would have reset every approval.
AnnotationStore.migrateLegacyVerdictscarries them over, with a deliberately asymmetric merge:CHANGESamong a group's files makes the groupCHANGES— "something in here needs work" stays true however the group is drawn.APPROVEDrequires every file of the group to have been settled. Approving a group claims the human read all of it, so a partially-approved group carries nothing forward and is re-settled by hand.Silently approving code nobody looked at is the one outcome a migration must never produce. A partial group keeps its legacy verdicts rather than having them deleted; a human
APPROVEDoutranks an agent'sAUTO_APPROVED; and each migrated verdict carries a note recording that it was carried over rather than freshly given.Verification
The FX layer has no headless harness inside the running app, so the width defect was diagnosed and confirmed by running it —
diag.diffWidthsand a newexplorerScriptcommentverb are what made that checkable rather than eyeballed, with a control run against the unfixed code.1161 tests pass. New:
FallbackIntentsTest,ReviewDiffColumnWidthTest(two of its four assertions failed against the old code),ReviewDiffColumnIntentFilterTest,ReviewEmptySurfaceTest,ReviewCommentComposerTest,LegacyVerdictMigrationTest,ReviewCarriedOverVerdictTest. Six existing tests had fixtures that encoded one-intent-per-file; their assertions were rewritten to check what they actually meant (coverage, isolation) rather than the old titles.🤖 Generated with Claude Code