Uh oh!
There was an error while loading. Please reload this page.
feat(review): know what has been sent, and correct a walkthrough - #25
Merged
Conversation
Submitting a review left no trace on the findings that went. They stayed open, looking exactly like the ones held back, so the only way to learn what had already gone was to try sending again. A comment now carries the finding it came from, the result says which ones the forge took, and those are marked submitted - still open, because sending is not resolving. The dialog labels them and leaves them unselected, so resending is a deliberate act rather than an accident. A walkthrough could only be added to. An agent that recorded a wrong step had to add a second one and rely on the newest winning, leaving the bad one in the database - which happened here, and had to be cleaned out by hand. `agent tour-delete` removes one, or all of them for the session, and the review skill now says to check what it recorded and rebuild rather than stack. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018PkYQzbsnMihHesafWvXKs
develop is the default branch and takes squash merges through pull requests only, matching the other repositories here. Nothing is merged into it locally, and the squashed commit rather than the branch is what gets offered upstream - head branches are deleted on merge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018PkYQzbsnMihHesafWvXKs
… the url gh resolves a fork's parent, so an unpinned `gh pr checkout 25` fetched whichever pull request carried that number upstream - it checked out nilbuild's feat/host branch and the diff came out as 5 files against an unrelated commit. `gh pr view` was pinned earlier; these two were missed. Found by comparing the rendered diff against the forge's own counts, which is the check that catches this class of thing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018PkYQzbsnMihHesafWvXKs
fiddur added a commit
that referenced
this pull request
Aug 24, 2026
) Six things about the walkthrough, all from watching it get used on #25. **It opened on stop 1 without having gone there.** So the first press of "next" moved to stop 2 and stop 1 was never shown. It now opens at `0/n` and the down arrow reads "Start the walkthrough"; before it starts, the header shows the walkthrough's topic rather than a stop it hasn't scrolled to. **Every highlighted hunk said "The walkthrough points here."** All of them, at once, regardless of which stop the header was showing — so the copy told you nothing. Each now names its stop (`Walkthrough stop 3 — the marker`), the current one says `(you are here)`, and it gets a stronger highlight than the rest. **Nothing marked the stops in the diff.** A reader who scrolls rather than steps had no way to see which parts the walkthrough had something to say about. A lamp now sits where each stop begins — in the gutter, next to the line numbers, in both unified and split view — and in the file list, with a count when a file is visited more than once. Clicking a lamp jumps to that stop. **Jumping landed on the top of the file** and left the sidebar pointing at the previous one until you scrolled by hand: the active file came from a scroll observer with a threshold the jump didn't cross. Jumps now scroll to the stop's own lines (`data-new-line` anchors the row) and set the active file directly, the way the comment navigation already did. **Jump-to-first**, left of the arrows, for both the walkthrough and the comments. Appears only once you are past the first, since before that it would do nothing. Also declares two types that were already in use and had drifted: the split view's `onMouseDown` carries the shift key it was already being handed at runtime, and `ToolbarProps` has the `reviewInProgress` it already reads. Both were type errors in files this PR touches. ## Tests 51 new, 239 green in `@diffity/ui`, 407 across the workspace. The navigation arithmetic, the stop labels and the mark grouping are pure functions with their own tests; the stepper, the file list, the gutter lamps and the comment toolbar are covered through jsdom renders. Two type errors remain in the UI package and are older than this branch — `dashboard.tsx` reads a `loading` field that its hook does not return, and `toolbar.tsx` passes a partial `GitHubDetails`. Left alone rather than guessed at. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_018PkYQzbsnMihHesafWvXKs --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…w carried `tour-delete` with no argument removed every walkthrough in the session, and that was the easier form to type — "fix the walkthrough" was enough to wipe one a human had recorded. The id is required now and the sweep has to say so with --all. The sweep also spares a walkthrough another agent may still be writing. That guard cannot be unconditional: a walkthrough starts out building, so an agent replacing its own half-written attempt is the ordinary case, and it says so with --include-building. An existing test caught this, having been written for exactly that flow. A sent finding now records the review it went out in and the commit it went out against, not just that it was sent. A timestamp cannot answer the question asked after a push — whether the comment went out against the code that is there now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018PkYQzbsnMihHesafWvXKs
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 freeto 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.
The two gaps the first real submission exposed, plus a README that had fallen a long way behind.
Knowing what has been sent
Submitting left no trace on the findings that went out. They stayed
open, looking identical to theones held back, so the only way to learn what was already on the pull request was to try sending
again and read the "already existed" count.
createReviewreports back which ones the forge actually took — not every one offered, since duplicates and unpostable lines are filtered firstsubmitted_at. Deliberately its own column rather than a status: sending is not resolving, and a submitted finding is still open until someone deals with itCorrecting a walkthrough
A walkthrough could only be added to. An agent that recorded a wrong step had to add a second one and
rely on the newest winning, leaving the bad one in the database — which is exactly what happened
while reviewing #14378, and I had to remove it with SQL.
agent tour-delete [<id>]removes one, or all of them for the session, withDELETE /api/tours/:idfor symmetry with threads. The review skill now says to check what it recorded and rebuild rather
than stack.
README
It documented
[must-fix]/[suggestion]severities that are now configurable, said a PR opens"against its base branch" when it is now pinned to the base commit, and had nothing at all about
the reading order, the attention rules, the review composer, review state,
--repo, or the agentCLI. Five new sections, and the ones that were wrong are corrected.
Tests
threadToPayloadlegitimately gainedthreadId, so the test names it372 pass.
🤖 Generated with Claude Code
https://claude.ai/code/session_018PkYQzbsnMihHesafWvXKs