Diff review modal in session menu#78
Merged
Merged
Conversation
Wire up the session menu's "Diff" action to a full-screen modal that renders the uncommitted working-tree diff (tracked + untracked) via @pierre/diffs with a split/unified toggle. Reviewers select line ranges to attach GitHub-style comments, persisted per session in SQLite, and "Submit review" composes them into a chat prompt for pi. Backend adds git.WorkingTreeDiff plus /api/git/diff and /api/diff/reviews handlers. The heavy diff renderer is lazy-loaded in its own chunk and kept out of the export bundle. New diff.* strings translated across all locales.
Address the diff review modal's rough first cut: - Make the sheet fill the viewport instead of a small centered dialog, and move its styles into the embedded session.css (the live page loads global stylesheets, not Svelte-scoped <style>). - Use the bundled pierre-dark/pierre-light themes so the diff matches the app instead of rendering on a white background. - Surface a discoverable gutter "+" button (enableGutterUtility) so users can actually start a review comment on a line or selected range. Add a Chromium e2e spec covering full-page render, the split/unified toggle, the not-a-repo state, and the comment round-trip into the composer.
Three bugs in the diff modal: - Adding a comment did nothing: CodeView.updateItem only re-renders when an item's `version` changes, but makeItem never set one, so the gutter "+" never produced a compose box. Stamp a fresh monotonic version per update. - Comments now persist (they were saved server-side already; the broken add path just meant nothing was ever stored to reload). - The diff ignored app theme changes. Observe data-theme and push the new themeType into CodeView so the diff re-themes live. Rework the e2e to exercise the real flow — gutter "+", type, save, reload, reopen (persisted), submit — plus a theme-switch test.
CodeView.setOptions REPLACES the options object rather than merging, so each setLayout (diffStyle) and theme-change call was silently wiping enableLineSelection, enableGutterUtility, renderAnnotation, and onGutterUtilityClick. After any split/unified toggle or theme switch the diff lost line selection and the comment "+", so multi-line drag-select comments never worked. Retain the full options object and merge patches into it. Add an e2e that drag-selects a line range (after toggling to unified) and asserts the persisted comment spans multiple lines.
WorkingTreeDiff spawned one `git diff --no-index` per untracked file, so a tree with thousands of untracked files took tens of seconds and the modal sat on "Loading diff…". Synthesize untracked-file patches in-process (one read each), cap the combined patch at 2 MiB, skip oversized/binary files, and add a 10s timeout to the git diff command. As a frontend safety net, race the load against a 30s timeout so a stalled endpoint or renderer chunk shows an error instead of spinning forever.
manualChunks forced @pierre/diffs + all of shiki (every language grammar) into a single 'diffs' chunk, producing a ~9.9 MB blob that the diff modal must download before it can render. On a slow or remote (e.g. Tailscale) connection that stalls past the load timeout, surfacing as "Diff timed out". Let the dynamic import split it naturally: shiki lazy-loads only the grammar for each diffed file, so the initial download drops dramatically.
The Go server serves built assets under /static/assets/, but Vite's default base (/) made the chunk loader request dynamically-imported chunks from /assets/… — which 404. The app entry happened to load (its path is built from the manifest with the /static/ prefix), but lazily-loaded chunks (hljs, the rolldown runtime, and the @pierre/diffs shiki grammars) were fetched from the wrong path. Setting base to /static/ aligns the chunk loader with where assets are actually served; verified no more 404s.
The diff renders fine on localhost but a user hits "Diff timed out" remotely, so make the failure self-diagnosing and rule out stale assets: - Load the renderer, diff, and reviews as labelled stages with per-stage timeouts and console timing logs; the timeout message now names the stalled stage (renderer/diff/reviews). Reviews are best-effort and no longer block. - Send no-cache on the app-shell HTML so a content-hashed shell can't pin the browser to an old bundle after a rebuild. - Purge old Cache API entries when the (no-fetch-handler) service worker activates, in case an older worker cached app assets.
Only the diff command was timeout-protected; rev-parse and ls-files used an unbounded exec. On a repo with a large untracked tree, `git ls-files --others` can block for tens of seconds, hanging /api/git/diff and the diff modal. Wrap run() in a 10s context timeout so the endpoint always returns.
CodeView's reconcileHeights stops re-measuring a file once it has zero annotations, so removing the last comment (or canceling a draft) stranded the height the annotation had reserved — leaving a large empty gap above the file. Force a layout-cache reset (via a no-op unsafeCSS bump, the only option with no visual effect that resets per-item layout) when a file drops to zero annotations. Verified: ~104px gap before, ~8px after; covered by an e2e that asserts the file offset stays put across add+delete.
CI ran the diff spec on Mobile Chrome too — the browserName==='chromium' skip lets it through, and the mobile command-menu surface is a different DOM (#mobile-command-panel), so the openDiffModal click never finds a visible item. Gate on testInfo.project.name via beforeEach instead. The multi-line drag also flaked on Desktop Chrome: the unified line-number elements weren't laid out when boundingBox() was read. Wait for both gutter cells to be visible first.
@pierre/diffs CodeView uses the container we hand to setup() as the scroll viewport — it reads root.scrollTop and attaches scroll listeners there. .diff-codeview had overflow: hidden, so tall diffs were clipped at the visible region with no way to scroll past it.
… the sheet header Mirror the diff sheet's open state to a `?diff=open` query param via replaceState, and restore it on mount in <SessionShell>, so refreshing the page keeps the sheet open instead of dropping the user back to the chat. replaceState (not push) keeps the back-button flow identical — FullScreenSheet still owns the mobile history entry. Also: <FullScreenSheet> takes a new optional `headerExtra` snippet that renders between the back button and the close-X. <DiffModal> uses it to move Split/Unified + Submit review into the header, reclaiming the second toolbar row that was eating vertical space on mobile.
After a keyboard refresh (Cmd-R / F5) the browser stays in keyboard focus mode, so the auto-focus that ran when a URL-restored sheet opened landed :focus-visible on the back button and showed a stray blue ring on the "← Diff" label. Focus the panel itself (tabindex=-1, outline:none) instead. The focus trap still works because panelEl contains every focusable, so Tab moves into them — but nothing visibly grabs the ring on open.
… review composer
Diff modal additions:
- Per-file collapse via a chevron rendered into the file header through
renderHeaderPrefix. Toggles via @pierre/diffs' built-in `collapsed` item flag.
- Toolbar "Collapse all" / "Expand all" button derived from a reactive
collapsedCount vs fileCount comparison. New `diff.collapseAll`,
`diff.expandAll`, `diff.collapseFile`, `diff.expandFile` keys
translated into all 14 locales.
Mobile layout:
- On viewports ≤ 900px the toolbar moves out of the sheet header and renders
as a second row in the sheet body. The phone-width header couldn't host
"← Diff" + Split/Unified + Collapse all + Submit review without crushing
the back button.
- The line-comment composer renders into document.body as a viewport-bottom
sheet instead of inline in the diff. The diffs library's
`code { contain: content }` creates a fixed-positioning containing block,
so a `position: fixed` annotation child stayed pinned to its (very
narrow) split-view column. Hosting the composer in light DOM escapes
the containment; cleanup is hooked into save/cancel/teardown.
E2E + tooling:
- New tests for per-file chevron toggle and bulk Collapse all/Expand all.
- The reload-persistence test no longer re-clicks the command menu —
`?diff=open` restores the sheet, so we just assert it's still open.
- knip.config.js ignores @pierre/diffs explicitly: its Svelte parser
stops surfacing the dynamic `import('@pierre/diffs')` once DiffModal
carries enough $state/$derived runes.
Two PWA failure modes after the diff modal moved its toolbar into the sheet header: 1. The back button "← Diff" was hidden behind the macOS traffic lights. 2. Clicking Split / Unified / Collapse all / Submit review did nothing — the OS swallowed the events as window-drag attempts because the sheet header sits inside the title-bar strip in window-controls-overlay mode. Inset the header past the OS controls and mirror the drag / no-drag pattern .session-header-bar already uses (drag on the bar, no-drag on its <a> / <button> children) so the title-bar strip is draggable but the buttons remain clickable. Also force flex-shrink: 0 on the diff sheet's back button — a slightly narrow PWA or split-screen window was otherwise crushing it to zero width before WCO even came into play.
Review comments live in the app DB keyed by session id, outside the append-only JSONL, and had no cleanup path: deleting a session left its comments behind forever. Add reapOrphanedReviewComments, swept lazily on the index load against the full session list, so the delete path stays untouched.
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.
Summary
@pierre/diffs, renders the uncommitted diff (tracked changes plus untracked files synthesized in Go), and supports a Split/Unified toggle that follows the app theme.review_commentsSQLite table (per-session), survive reload, and a Submit review button composes them into a chat prompt for pi.git.WorkingTreeDiff(caps untracked + total size, all git calls have a 10s timeout) and three handlers —GET /api/git/diff,GET/POST/DELETE /api/diff/reviews. Frontend:DiffModal.sveltedriven by an action (nobind:thisDOM ops) with a staged-and-instrumented load so the rare hang names which stage stalled.no-cacheon the app shell, service-worker cache purge, Vitebase: /static/) and ships diff strings in all 14 locales.Test plan
make check— lint, format, knip, 639 frontend tests, build,go vet, all Go tests passe2e/tests/diff.spec.ts(6 Chromium tests):WorkingTreeDifftable-driven, binary untracked, 1000-untracked perf bound;review_commentsCRUD/validationManual smoke after pulling: `make build`, restart pi-web, hard-reload, open session → "…" → Diff.