Repository navigation
Conversation
A new read-only procedure alongside listCommits, which is untouched. It returns a topologically ordered commit window plus per-ref decorations, so the renderer can draw lanes without spawning git itself. - Tips are seeded from git's own refs, never from workspace records. `refScope` picks the breadth: head, open-workspaces, local, remote, all. `remote` and `all` pass one flag (--remotes, --exclude=refs/stash --all) rather than enumerating refnames, so a tag-heavy repo cannot overflow the command line. Detached worktree heads are still named explicitly — they are not refs. - Local branches are classified open / detached-worktree / prunable (+ reason) / merged / orphan-branch. Containment comes from `git branch --merged <baseRef>`, not from graph topology, so a branch merged far outside the window still reports merged. `prunable` comes from git's own porcelain flag, so classification costs no synchronous fs calls on the host event loop. - "Open workspaces" is read off `git worktree list`, not the workspaces table: the worker has no DB access and every Superset workspace is a worktree. Worktrees git marks prunable are excluded. - Every git spawn happens in the worker task, including base resolution, so stdout draining and parsing stay off the host event loop. Overlapping traversals coalesce, keyed by workspace and scope, so a git:changed storm during a rebase does not fan out into duplicate walks. - Cursors carry the scope they were issued for. --skip counts rows of one traversal, so a foreign cursor restarts at page one rather than serving a window the traversal never produced. - getCommitFiles now also returns the commit's own metadata, so a diff pane opened from the graph renders its header without a second spawn.
A fourth sidebar tab whose primary value is ref classification, not the lanes. Commit history was only reachable as a flat base..HEAD dropdown, with no view of the branches and worktrees that accumulate outside the open workspaces — a branch whose worktree was deleted was invisible until it caused a problem. Read-only: no checkout, cherry-pick, revert or prune. - assignLanes is pure and tested, no React or tRPC. An active-lane sweep over topo-ordered input, with lane identity pinned to the originating branch so a refetch does not reshuffle the graph. - Virtualized list with per-row inline SVG rather than one tall canvas. Row height derives from the commit's own refs, no measurement pass, and the size cache is keyed by commit hash so a cached size travels with its commit when a refetch shifts every index. - Lane colours are additive --graph-lane-1..8 tokens in both themes at equal chroma and lightness. No component hardcodes a colour; the hex in globals.css is only the pre-hydration fallback. - Ref badges encode state in texture as well as colour — fill, dashes, strike — so they still read at 260px and for a colourblind viewer, leaving hue free for lane identity. - Commit clicks open the existing diff pane kind. The graph owns its own persisted selection, separate from the Changes tab's filter, so the two surfaces no longer fight over one piece of state. Shift-click ranges, cmd/ctrl-click forces a new pane, double-click pins. - The control strip carries a scope chooser (This workspace / Open workspaces / All local branches / Remote / Everything) and two toggles: ref badges on their own untruncated line, and dimming of commits carrying no unreferenced ref. It renders even when the window is empty, so a scope returning nothing cannot take away the control that would let you leave it. The strip is not SidebarHeader's actions slot — that slot clips the last tab label instead of shrinking it. - The strip reads through a live query, not .get(): .get() is a non-reactive snapshot, which showed as a multi-second lag on a write that had already completed.
Both documents are shipped, so they go straight to plans/done/ per the plan-placement rule. The follow-up doc records the verification results the implementation was checked against, including the one-off large-repo bench whose script was removed before merge.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a read-only Git graph sidebar with scoped pagination, branch and worktree states, lane-based rendering, responsive layouts, persisted controls, commit selection, and graph-owned diff panes with commit metadata. ChangesGit graph data and traversal
Graph rendering and interaction
Sidebar and diff integration
Implementation and verification records
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
2 of 4 tasks
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/DiffPane.tsx (1)
315-320: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRender commit metadata for empty changesets.
Lines 315-320 return before line 337 renders
CommitMetadataHeader. An empty commit or a range with no file changes therefore loses its commit identity and displays only “No changes”.Keep the metadata header outside the empty-files return. Render the empty state below it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/DiffPane/DiffPane.tsx around lines 315 - 320, Update the DiffPane empty-files branch so CommitMetadataHeader still renders when files.length is zero. Move the empty-state return below the metadata header, preserving the existing loading/“No changes” message while ensuring empty commits and unchanged ranges retain their commit identity.
🧹 Nitpick comments (11)
packages/host-service/src/workers/tasks/git.ts (1)
102-120: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueAlign the field-count guard with the 7-field format.
The format string emits 7 tab-separated fields. The guard rejects fewer than 6. With exactly 6 fields the parser returns a record whose
messageis empty and whosedatemay hold body text. Use< 7so a truncated line is rejected instead of producing a partly wrong record.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/host-service/src/workers/tasks/git.ts` around lines 102 - 120, Update the field-count guard in parseCommitMetadata to reject any input with fewer than 7 tab-separated fields, preserving parsing only for the complete seven-field format.packages/host-service/test/integration/git-graph.integration.test.ts (1)
196-205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve inherited environment when using
git.env().
simple-gitobject form replaces the child-process environment, so these latergit.commit()calls losePATHand any identity configuration from the scenario. Set per-commit date options or spreadprocess.envinto.env()instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/host-service/test/integration/git-graph.integration.test.ts` around lines 196 - 205, Update the commit setup around the git.env calls for m2 and m3 (backdated) so each commit preserves the inherited process environment, either by spreading process.env into the environment object or by passing the dates as per-commit options. Keep the distinct author and committer dates for both commits unchanged.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/RefBadge/RefBadge.tsx (1)
8-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType
laneColorasGraphLaneColor.The doc comment states the range is 1-8, but the type is
number. Lines 74, 75 and 87 interpolate the value into--graph-lane-${laneColor}. An out-of-range value resolves to an undefined CSS variable and the badge loses its tint with no error.GraphLaneColoris already exported from../../types, and the caller passesrow.color, which already has that type.♻️ Proposed fix
-import type { GraphRef, GraphRefState } from "../../types"; +import type { GraphLaneColor, GraphRef, GraphRefState } from "../../types"; @@ /** 1-8; only `open` badges tint themselves with their lane's colour. */ - laneColor: number; + laneColor: GraphLaneColor;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/RefBadge/RefBadge.tsx around lines 8 - 9, Update the RefBadgeProps laneColor property to use the exported GraphLaneColor type from ../../types instead of number, preserving the documented 1–8 constraint and existing CSS variable interpolation.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/utils/assignLanes/assignLanes.test.ts (1)
11-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
fulldoes not produce a 40-character hash.
FULLholds 34 zeros. For a one-character label,full("a")returns 35 characters, so the comment at Line 13 is wrong. The tests still pass, becausepresent.hasandfindRowcompare the same padded strings. The fragile part isfindRowat Line 79, which matches bystartsWith: two labels where one is a prefix of the other (for example"m"and"m1") would resolve to the wrong row.padEndremoves both problems.♻️ Proposed fix
-const FULL = "0000000000000000000000000000000000"; - /** Pads a short label into a 40-char hash so parent refs match commit hashes. */ -const full = (h: string): string => `${h}${FULL}`.slice(0, 40); +const full = (h: string): string => h.padEnd(40, "0").slice(0, 40);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/utils/assignLanes/assignLanes.test.ts around lines 11 - 14, Update the full helper to use padEnd(40, "0") so every label is exactly 40 characters, replacing the current FULL-based concatenation; preserve the existing label-to-hash behavior used by present and findRow.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphLanes/GraphLanes.test.tsx (2)
135-142: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the lane x and the viewBox width from
graphGeometry.The literals
7and18restategeo.pitch / 2and1 * geo.pitch + 4. The file already importsgraphGeometry. If the pitch changes, these assertions fail for a reason unrelated to the behavior under test.♻️ Proposed fix
+ const x0 = geo.pitch / 2; - expect(incoming).toContain(`d="M7,0 V${yc}"`); + expect(incoming).toContain(`d="M${x0},0 V${yc}"`);Also applies to: 180-187
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphLanes/GraphLanes.test.tsx around lines 135 - 142, Update the affected GraphLanes test assertions around the incoming and outgoing path checks, including the similar assertions near the referenced second range, to derive the lane x-coordinate and viewBox width from the imported graphGeometry values instead of hardcoded 7 and 18 literals. Preserve the existing path expectations while making them adapt when geometry pitch changes.
87-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe merge assertion does not test the fill.
Both node branches render exactly one
<circle>, so the count at Line 90 and Line 93 does not separate a donut from a filled node. The lane-color check at Line 94 also passes for the donut, because the donut setsstroketo the same variable. Assert the distinguishing attributes instead.♻️ Proposed stronger assertions
- const plain = render(row({ isMerge: false, isRoot: false })); - expect(count(plain, "<circle")).toBe(1); - // Merge: filled node. - const merge = render(row({ isMerge: true })); - expect(count(merge, "<circle")).toBe(1); - expect(merge).toContain("var(--graph-lane-1)"); + const plain = render(row({ isMerge: false, isRoot: false })); + expect(count(plain, "<circle")).toBe(1); + expect(plain).toContain('stroke="var(--graph-lane-1)"'); + expect(plain).toContain("fill-sidebar"); + // Merge: filled node, no ring. + const merge = render(row({ isMerge: true })); + expect(count(merge, "<circle")).toBe(1); + expect(merge).toContain('fill="var(--graph-lane-1)"'); + expect(merge).not.toContain("fill-sidebar");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphLanes/GraphLanes.test.tsx around lines 87 - 98, Update the merge-case assertions in the test “renders a donut node for a plain commit, a filled node for merges/roots” to verify distinguishing SVG attributes rather than only circle count or lane color. Assert that the plain commit uses the lane color as stroke without a separate fill, while the merge uses the lane color as fill and does not rely on the shared stroke assertion; keep the root circle-count behavior unchanged.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphRow/GraphRow.tsx (2)
124-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpose the selected state to assistive technology.
The row communicates selection only through the tint and the lane-coloured rail. The rail carries
aria-hidden="true". A screen reader therefore reports no difference between a selected row and an unselected row.Add
aria-pressed={selected}(oraria-current) to the button.♿ Proposed change
<button type="button" onClick={onSelect} onDoubleClick={onDoubleClick} + aria-pressed={selected} title={commit.message}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphRow/GraphRow.tsx around lines 124 - 154, Add aria-pressed={selected} to the button rendered by GraphRow so its selected state is exposed to assistive technology, while preserving the existing visual selection behavior.
69-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider sharing the two-line rule with
isTwoLineRow.
ownLinere-implements the rule thatisTwoLineRowinGraphTabContent.tsxalready defines (twoLineRefs && refs.length > 0). The virtualizer estimates row height fromisTwoLineRow, andGraphRowrenders height fromownLine. If one rule changes, the estimated height and the rendered height diverge and rows overlap.Move the predicate into
GraphLanes/constants.tsand import it in both places. That keeps a single rule and avoids a circular import betweenGraphRowandGraphTabContent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphRow/GraphRow.tsx around lines 69 - 78, Extract the shared two-line predicate currently represented by ownLine and isTwoLineRow into a named helper in constants.ts, then import and use it in both GraphRow and GraphTabContent. Replace the local rule without changing the existing row-height estimation or rendering behavior, and avoid introducing imports between those components.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphTabContent/GraphTabContent.tsx (3)
92-99: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueBuild
indexByHashonly for the range case.The map is built at lines 92-93, but the
kind === "commit"branch returns at line 94 without reading it. Move the map construction below that branch.♻️ Proposed change
- const indexByHash = new Map<string, number>(); - for (let i = 0; i < hashes.length; i++) indexByHash.set(hashes[i], i); if (selection.kind === "commit") { return { selectedSet: new Set([selection.hash]), inRangeSet: new Set<string>(), }; } + const indexByHash = new Map<string, number>(); + for (let i = 0; i < hashes.length; i++) indexByHash.set(hashes[i], i);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphTabContent/GraphTabContent.tsx around lines 92 - 99, Move the indexByHash Map construction and population below the selection.kind === "commit" early-return in the surrounding selection-range logic, so the commit branch returns without creating an unused map while range handling retains the existing lookup behavior.
348-400: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe copy actions have no keyboard path.
The menu opens only from
onContextMenu. Keyboard users cannot reach Copy Hash, Copy Short Hash, Copy Subject, or Open in New Tab. Consider opening the same menu onShift+F10or on the context-menu key when a row has focus.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphTabContent/GraphTabContent.tsx around lines 348 - 400, Add keyboard context-menu handling to the graph row focus interaction so Shift+F10 and the Context Menu key open the existing menu state used by onContextMenu, positioned for the focused row. Preserve the current mouse context-menu behavior and ensure the existing Copy Hash, Copy Short Hash, Copy Subject, and Open in New Tab actions remain available through the keyboard.
264-282: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe truncation notice depends on three unrelated callbacks.
Line 264 gates the whole control strip on
onSelectRefScope && onToggleTwoLineRefs && onToggleUnreferencedOnly. All three props are optional. If a caller omits one toggle, the scope chooser and the "Showing first N commits" notice both disappear. The comment above states that the strip must render even with zero rows, so this coupling works against the stated intent.Make the three callbacks required in
GraphTabContentProps, or render the strip unconditionally and gate onlyGraphHeaderActionson the callbacks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphTabContent/GraphTabContent.tsx around lines 264 - 282, Decouple the truncation notice and control strip from the optional callbacks in GraphTabContent: update GraphTabContentProps to make onSelectRefScope, onToggleTwoLineRefs, and onToggleUnreferencedOnly required, or render the strip unconditionally while conditionally rendering GraphHeaderActions only when its callbacks are available. Ensure the strip and “Showing first N commits” notice still render with zero rows or when any callback is omitted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/plans/20260801-1200-git-graph-visual-spec.md`:
- Around line 202-206: Add the text language identifier to the opening fences of
both diagram code blocks in the visual specification, including the
Wide/Standard/Compact diagram and the corresponding block around the second
referenced section, to resolve MD040 warnings.
- Around line 93-155: Align the lane-colour specification with the actual
implementation: either document the theme-derived ANSI palette and
color-mix-based lanes 7–8, including the correct fallback-variable location, or
update the implementation to provide the specified fixed equal-lightness OKLCH
tokens in both themes. Apply the same correction to the corresponding later
section and ensure the documented token source matches the implementation.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphHeaderActions/GraphHeaderActions.tsx:
- Around line 109-131: Update the TooltipContent in the unreferenced toggle
within GraphHeaderActions to switch its text based on unreferencedOnly, matching
the existing state-dependent aria-label and the neighboring toggle behavior:
show the action to stop highlighting when active and the action to highlight
when inactive.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphRow/GraphRow.test.tsx:
- Around line 228-232: Remove the self-comparison assertion in the “changes
nothing when the toggle is off” test, or replace it with a comparison against a
known single-line baseline; retain the existing “+5” and “max-w-[12ch]”
assertions.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphRow/GraphRow.tsx:
- Around line 194-198: Update the showDate rendering in GraphRow to validate the
result of Date.parse(commit.date) before passing it to formatRelativeTime.
Render a reliable fallback when the date is unparseable, while preserving the
existing relative-time output for valid timestamps.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphTabContent/GraphTabContent.test.ts:
- Around line 35-41: Rename the test covering resolveRowSelection(commit("zzz"),
H) to describe that an out-of-window commit remains in selectedSet while
matching no row, keeping its existing assertions unchanged.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/utils/assignLanes/assignLanes.ts:
- Around line 140-169: Update the fork-parent handling in the assignLanes loop
so its free-lane search cannot select mine after the first parent terminated or
cleared that lane. Exclude mine when searching lanes.indexOf(null), allocate or
reuse another lane instead, and preserve the existing edge creation and parent
assignment behavior for other lanes.
In `@packages/host-service/src/workers/tasks/git.ts`:
- Around line 82-96: Validate commitHash and fromHash in the router input schema
before they reach gitCommitFilesTask, rejecting values beginning with “-” and
accepting only valid revision/hash shapes. Ensure both getChangedFilesForDiff
and git show receive validated revisions; apply the same constraint consistently
across other call sites identified by the search.
In `@plans/done/20260801-commit-graph-sidebar.md`:
- Line 147: Add language identifiers to every affected Markdown fence: use bash
at plans/done/20260801-commit-graph-sidebar.md lines 147-147, text at lines
183-183, and bash or text at plans/done/20260803-commit-graph-followups.md lines
43-43.
- Around line 222-226: Update the touched-files list and both palette sections
in the plan to reference apps/desktop/src/renderer/globals.css instead of
packages/ui/src/globals.css, preserving the existing graph token details.
- Around line 169-176: Update the GraphRow edge documentation for the
assignLanes utilities to match the shipped GraphEdge kind union defined in
types.ts, including in-straight, out-straight, pass, out-stub, and in-merge
alongside the existing kinds. Keep the documented edge shape and lane behavior
unchanged.
- Around line 311-317: Update the Phase 1 acceptance criteria to state that all
five ref states are returned correctly, matching the contract and integration
test coverage for open, detached-worktree, orphan-branch, prunable, and merged.
- Around line 15-16: Mark the original phase-5 design in this plan as superseded
by the persisted graphSelection approach and commit metadata rendered inside
DiffPane. Update the affected entries, including the changesFilter-based
selection and CommitDetailPanel references, or add an explicit superseded note
pointing to the follow-up plan; ensure all referenced sections consistently
reflect the replacement design.
In `@plans/done/20260803-commit-graph-followups.md`:
- Around line 1-8: Move the still-pending commit graph follow-up plan out of
plans/done/ and into plans/ while its listed defects, controls, verification
work, and deferred tasks remain open; return it to plans/done/ only after all
documented work ships.
- Around line 14-20: Reconcile the verification status in the current-state
table and Sections 5.1, 5.2, and 5.6 so they consistently reflect that the
profiling script ran and was removed, and that GraphLanes and RefBadge component
tests exist. Remove or update the outdated debt entries rather than leaving
contradictory claims.
---
Outside diff comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/hooks/usePaneRegistry/components/DiffPane/DiffPane.tsx:
- Around line 315-320: Update the DiffPane empty-files branch so
CommitMetadataHeader still renders when files.length is zero. Move the
empty-state return below the metadata header, preserving the existing
loading/“No changes” message while ensuring empty commits and unchanged ranges
retain their commit identity.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphLanes/GraphLanes.test.tsx:
- Around line 135-142: Update the affected GraphLanes test assertions around the
incoming and outgoing path checks, including the similar assertions near the
referenced second range, to derive the lane x-coordinate and viewBox width from
the imported graphGeometry values instead of hardcoded 7 and 18 literals.
Preserve the existing path expectations while making them adapt when geometry
pitch changes.
- Around line 87-98: Update the merge-case assertions in the test “renders a
donut node for a plain commit, a filled node for merges/roots” to verify
distinguishing SVG attributes rather than only circle count or lane color.
Assert that the plain commit uses the lane color as stroke without a separate
fill, while the merge uses the lane color as fill and does not rely on the
shared stroke assertion; keep the root circle-count behavior unchanged.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphRow/GraphRow.tsx:
- Around line 124-154: Add aria-pressed={selected} to the button rendered by
GraphRow so its selected state is exposed to assistive technology, while
preserving the existing visual selection behavior.
- Around line 69-78: Extract the shared two-line predicate currently represented
by ownLine and isTwoLineRow into a named helper in constants.ts, then import and
use it in both GraphRow and GraphTabContent. Replace the local rule without
changing the existing row-height estimation or rendering behavior, and avoid
introducing imports between those components.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphTabContent/GraphTabContent.tsx:
- Around line 92-99: Move the indexByHash Map construction and population below
the selection.kind === "commit" early-return in the surrounding selection-range
logic, so the commit branch returns without creating an unused map while range
handling retains the existing lookup behavior.
- Around line 348-400: Add keyboard context-menu handling to the graph row focus
interaction so Shift+F10 and the Context Menu key open the existing menu state
used by onContextMenu, positioned for the focused row. Preserve the current
mouse context-menu behavior and ensure the existing Copy Hash, Copy Short Hash,
Copy Subject, and Open in New Tab actions remain available through the keyboard.
- Around line 264-282: Decouple the truncation notice and control strip from the
optional callbacks in GraphTabContent: update GraphTabContentProps to make
onSelectRefScope, onToggleTwoLineRefs, and onToggleUnreferencedOnly required, or
render the strip unconditionally while conditionally rendering
GraphHeaderActions only when its callbacks are available. Ensure the strip and
“Showing first N commits” notice still render with zero rows or when any
callback is omitted.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/RefBadge/RefBadge.tsx:
- Around line 8-9: Update the RefBadgeProps laneColor property to use the
exported GraphLaneColor type from ../../types instead of number, preserving the
documented 1–8 constraint and existing CSS variable interpolation.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/utils/assignLanes/assignLanes.test.ts:
- Around line 11-14: Update the full helper to use padEnd(40, "0") so every
label is exactly 40 characters, replacing the current FULL-based concatenation;
preserve the existing label-to-hash behavior used by present and findRow.
In `@packages/host-service/src/workers/tasks/git.ts`:
- Around line 102-120: Update the field-count guard in parseCommitMetadata to
reject any input with fewer than 7 tab-separated fields, preserving parsing only
for the complete seven-field format.
In `@packages/host-service/test/integration/git-graph.integration.test.ts`:
- Around line 196-205: Update the commit setup around the git.env calls for m2
and m3 (backdated) so each commit preserves the inherited process environment,
either by spreading process.env into the environment object or by passing the
dates as per-commit options. Keep the distinct author and committer dates for
both commits unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b7ed9b55-a6c5-4f73-be3c-212d009fc361
📒 Files selected for processing (48)
apps/desktop/plans/20260801-1200-git-graph-visual-spec.mdapps/desktop/src/renderer/globals.cssapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/WorkspaceSidebar.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphHeaderActions/GraphHeaderActions.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphHeaderActions/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphLanes/GraphLanes.test.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphLanes/GraphLanes.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphLanes/constants.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphLanes/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphRow/GraphRow.test.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphRow/GraphRow.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphRow/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphTabContent/GraphTabContent.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphTabContent/GraphTabContent.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphTabContent/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/RefBadge/RefBadge.test.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/RefBadge/RefBadge.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/RefBadge/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/types.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/useGraphTab.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/utils/assignLanes/assignLanes.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/utils/assignLanes/assignLanes.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/utils/assignLanes/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useChangeset/useChangeset.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/DiffPane.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/components/CommitMetadataHeader/CommitMetadataHeader.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/components/CommitMetadataHeader/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/usePaneRegistry.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useWorkspacePaneOpeners/useWorkspacePaneOpeners.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/types.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.tsapps/desktop/src/renderer/stores/theme/store.tsapps/desktop/src/renderer/stores/theme/utils/css-variables.tsapps/desktop/src/renderer/stores/theme/utils/index.tsapps/desktop/src/shared/themes/graph-lanes.test.tsapps/desktop/src/shared/themes/graph-lanes.tspackages/host-service/src/index.tspackages/host-service/src/trpc/router/git/git.tspackages/host-service/src/trpc/router/git/types.tspackages/host-service/src/trpc/router/git/utils/graph-log.test.tspackages/host-service/src/trpc/router/git/utils/graph-log.tspackages/host-service/src/workers/tasks/git.tspackages/host-service/test/integration/git-graph.integration.test.tspackages/host-service/test/integration/git-history.integration.test.tsplans/done/20260801-commit-graph-sidebar.mdplans/done/20260803-commit-graph-followups.md
Comment on lines
+194
to
+198
| {showDate && ( | ||
| <span className="min-w-10 shrink-0 text-right font-mono text-[11px] text-muted-foreground tabular-nums"> | ||
| {formatRelativeTime(Date.parse(commit.date))} | ||
| </span> | ||
| )} |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the graph log format string and the date field it produces.
fd -t f 'graph-log' packages/host-service | xargs rg -n -C4 '%c|%a|pretty|format|date'
# Inspect formatRelativeTime's handling of NaN input.
fd -t f 'formatRelativeTime' apps/desktop/src/renderer/lib --exec cat -n {}Repository: superset-sh/superset
Length of output: 10924
Guard against unparseable commit.date before rendering relative time.
graph-log.ts declares the date as a string but does not emit it; Date.parse(commit.date) can still receive values outside the host parser’s control. Use a validated timestamp before calling formatRelativeTime, and render a reliable fallback if parsing fails.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphRow/GraphRow.tsx
around lines 194 - 198, Update the showDate rendering in GraphRow to validate
the result of Date.parse(commit.date) before passing it to formatRelativeTime.
Render a reliable fallback when the date is unparseable, while preserving the
existing relative-time output for valid timestamps.
Comment on lines
+1
to
+8
| # Commit Graph — Pending Work | ||
|
|
||
| Follow-up to `plans/` sibling work on the sidebar commit graph. The graph ships and renders live; | ||
| this document covers what is left. Every item states the files, the acceptance bar, and why the | ||
| decision went the way it did. | ||
|
|
||
| Predecessor plan: `plans/20260801-commit-graph-sidebar.md` (phases 0–6). Phases 0–4 are landed; | ||
| phase 5 is **superseded** by §2 below; phase 6 is partially landed. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Keep pending plans under plans/ until they ship.
This file is under plans/done/, but it documents open defects, queued controls, verification debt, and deferred work. Move it to plans/ while these items remain open, then move it to plans/done/ after completion.
As per coding guidelines, “Place cross-cutting implementation plans in plans/ and move shipped plans to plans/done/.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plans/done/20260803-commit-graph-followups.md` around lines 1 - 8, Move the
still-pending commit graph follow-up plan out of plans/done/ and into plans/
while its listed defects, controls, verification work, and deferred tasks remain
open; return it to plans/done/ only after all documented work ships.
Source: Coding guidelines
- assignLanes: an extra parent could reclaim the lane the same row had just stubbed, drawing a terminator and a live edge on one lane. Skip the row's own lane when looking for a free slot; regression test added. - git.getCommitFiles / git.getDiff: constrain commitHash and fromHash to a hash shape. They reach git positionally, and git reads a leading dash as an option — 'git show --output=<path>' writes an arbitrary file. - GraphHeaderActions: the unreferenced tooltip stayed 'Highlight unreferenced' while its aria-label flipped with state. - Tighten two tests that asserted less than they read as: the two-line-refs off case compared a render against itself, and the out-of-window selection test contradicted its own name.
Five ref states, not four; the lane tokens live in the desktop renderer's globals.css, not packages/ui; the shipped GraphEdge kinds are the six-way split, not straight/fork/merge. The visual spec gets a superseded note for the lane ramp, which now derives from the active theme's ANSI palette. Fenced blocks get languages.
Author
|
Thanks — went through all of it. Pushed 40a0bc7 (code) and 7837067 (docs). Fixed
Not fixed
|
acseven
marked this pull request as ready for review
August 7, 2026 14:50
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/host-service/src/trpc/router/git/git.ts (1)
364-369: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winInclude all traversal inputs in the coalescing key.
Lines 350-359 pass
baseBranch,limit, andskipto the worker. Lines 364-369 omit them fromdedupeKey.If two requests use the same workspace and scope with different cursors, limits, or base branches, one request can receive the other request's page. The returned commits and
nextCursorthen become incorrect.Proposed fix
- dedupeKey: `${input.workspaceId}:graph-log:${input.refScope}`, + dedupeKey: JSON.stringify({ + workspaceId: input.workspaceId, + baseBranch: input.baseBranch ?? null, + refScope: input.refScope, + limit: input.limit, + skip, + }),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/host-service/src/trpc/router/git/git.ts` around lines 364 - 369, Update the dedupeKey in the git graph-log traversal request to include every traversal input passed to the worker: refScope, baseBranch, limit, and skip, in addition to workspaceId. Preserve coalescing only for requests with identical traversal parameters.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/host-service/src/trpc/router/git/git.ts`:
- Around line 364-369: Update the dedupeKey in the git graph-log traversal
request to include every traversal input passed to the worker: refScope,
baseBranch, limit, and skip, in addition to workspaceId. Preserve coalescing
only for requests with identical traversal parameters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 13a3aedf-be83-4c37-843e-585da3cdc440
📒 Files selected for processing (10)
apps/desktop/plans/20260801-1200-git-graph-visual-spec.mdapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphHeaderActions/GraphHeaderActions.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphRow/GraphRow.test.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphTabContent/GraphTabContent.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/utils/assignLanes/assignLanes.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/utils/assignLanes/assignLanes.tspackages/host-service/src/trpc/router/git/git.tspackages/host-service/test/integration/git-history.integration.test.tsplans/done/20260801-commit-graph-sidebar.mdplans/done/20260803-commit-graph-followups.md
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphRow/GraphRow.test.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphHeaderActions/GraphHeaderActions.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/utils/assignLanes/assignLanes.ts
- plans/done/20260803-commit-graph-followups.md
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/utils/assignLanes/assignLanes.test.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphTabContent/GraphTabContent.test.ts
- plans/done/20260801-commit-graph-sidebar.md
- apps/desktop/plans/20260801-1200-git-graph-visual-spec.md
The dedupe key carried only workspace + refScope, so two overlapping listGraph calls that differ in baseBranch, limit or skip were served one another's rows. baseBranch resolves a tick after the graph mounts, so a cold open reliably issues two such calls.
acseven
force-pushed
the
feat/git-graph-sidebar
branch
from
August 7, 2026 15:01
6a4d297 to
8e8d380
Compare
Author
|
Second review — one finding, confirmed and fixed in 8e8d380. Include all traversal inputs in the coalescing key ( The paginating case CodeRabbit describes is the obvious one, but the reliable one is a cold open: Key now carries scope, base, limit and skip. The two sibling coalescers ( |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/host-service/src/trpc/router/git/git.ts">
<violation number="1" location="packages/host-service/src/trpc/router/git/git.ts:372">
P2: Overlapping requests with `baseBranch: undefined` and `baseBranch: ""` are incorrectly coalesced because both produce the same key, even though base resolution and `merged` ref classification differ; encoding the key as a structured tuple (or otherwise distinguishing `undefined`) would prevent one caller from receiving the other's result.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // `baseBranch` alone decides the `merged` classification, and the | ||
| // base resolves a tick after mount, so the first two calls of a | ||
| // cold open genuinely differ. | ||
| dedupeKey: `${input.workspaceId}:graph-log:${input.refScope}:${input.baseBranch ?? ""}:${input.limit}:${skip}`, |
Contributor
There was a problem hiding this comment.
P2: Overlapping requests with baseBranch: undefined and baseBranch: "" are incorrectly coalesced because both produce the same key, even though base resolution and merged ref classification differ; encoding the key as a structured tuple (or otherwise distinguishing undefined) would prevent one caller from receiving the other's result.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/host-service/src/trpc/router/git/git.ts, line 372:
<comment>Overlapping requests with `baseBranch: undefined` and `baseBranch: ""` are incorrectly coalesced because both produce the same key, even though base resolution and `merged` ref classification differ; encoding the key as a structured tuple (or otherwise distinguishing `undefined`) would prevent one caller from receiving the other's result.</comment>
<file context>
@@ -363,9 +363,13 @@ export const gitRouter = router({
+ // `baseBranch` alone decides the `merged` classification, and the
+ // base resolves a tick after mount, so the first two calls of a
+ // cold open genuinely differ.
+ dedupeKey: `${input.workspaceId}:graph-log:${input.refScope}:${input.baseBranch ?? ""}:${input.limit}:${skip}`,
},
);
</file context>
Suggested change
| dedupeKey: `${input.workspaceId}:graph-log:${input.refScope}:${input.baseBranch ?? ""}:${input.limit}:${skip}`, | |
| dedupeKey: JSON.stringify([ | |
| input.workspaceId, | |
| "graph-log", | |
| input.refScope, | |
| input.baseBranch, | |
| input.limit, | |
| skip, | |
| ]), |
Contributor
There was a problem hiding this comment.
23 issues found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/host-service/src/trpc/router/git/utils/graph-log.ts">
<violation number="1" location="packages/host-service/src/trpc/router/git/utils/graph-log.ts:88">
P2: A valid remote branch named `feature/HEAD` will never appear as a graph ref badge because the symbolic-HEAD filter matches any nested ref ending in `/HEAD`; restrict the filter to exactly `refs/remotes/<remote>/HEAD` so nested branch names remain visible.</violation>
<violation number="2" location="packages/host-service/src/trpc/router/git/utils/graph-log.ts:113">
P2: Graph metadata is corrupted for commits whose author name contains a tab: the field split shifts the email and date because only the subject is treated as tab-safe. Use an unambiguous delimiter or escaping for every structured field in the worker format and parser.</violation>
</file>
<file name="apps/desktop/plans/20260801-1200-git-graph-visual-spec.md">
<violation number="1" location="apps/desktop/plans/20260801-1200-git-graph-visual-spec.md:74">
P3: The edge-geometry table header says `assignLanes` emits five edge kinds, but the signature actually produces six — `out-stub` (the dashed parent-outside-window edge) is documented only in the paragraph below and not counted. The implementation plan lists all six, so the spec and the plan disagree about the row contract. Small, but this file is the frozen seam definition; aligning the count avoids a reader treating `out-stub` as non-contractual.</violation>
<violation number="2" location="apps/desktop/plans/20260801-1200-git-graph-visual-spec.md:147">
P2: The §3 'Assignment' paragraph still describes lane colour as `hash(originatingBranchName) % 8`, but that is precisely the scheme the shipped `assignLanes` replaced (rotating cursor with live-lane skip) after the §1.2 collision defect was confirmed — 15 of 42 live rows painted the same colour. Since this spec is the 'signed off' design contract that owns colour, keeping the old algorithm here will mislead anyone applying the spec later. Update the Assignment paragraph to describe the rotating live-skip cursor (or point at `assignLanes`' `pickColor`) so the spec stops contradicting the implementation and the follow-up.</violation>
<violation number="3" location="apps/desktop/plans/20260801-1200-git-graph-visual-spec.md:290">
P3: The §6 Scope control entry is stale and contradicts the shipped code: it claims the select sits in the header showing "Local refs" with "the three not-yet-implemented scopes, disabled". All five scopes are implemented and enabled, the default scope's label is "All local branches" (not "Local refs"), and the control lives on the graph's own strip rather than the header. Since this spec is the design contract for the tab, a reader following §6 would be misled about the shipped scope surface.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/WorkspaceSidebar.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/WorkspaceSidebar.tsx:139">
P2: Every workspace now performs a 500-commit `git.listGraph` traversal when its sidebar mounts, even if the user stays on Files, Changes, or Review. Passing the active-tab state through to disable this query until Graph is selected, or lazy-mounting the tab content, would avoid adding this host-service/git cost to the normal sidebar path.</violation>
<violation number="2" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/WorkspaceSidebar.tsx:188">
P3: Graph is rendered as the third sidebar tab because it is inserted before Review, despite the feature being specified as the fourth tab. Appending Graph after `reviewTab` preserves the existing tab order and makes Graph the new fourth tab.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts:158">
P3: The new persisted state is untested: the heal-workspace test that asserts each sidebarState default skips the added `graphSelection`, and none of the new fields (`graphRefScope`, `graphTwoLineRefs`, `graphUnreferencedOnly`, and the `graphSelection` range/commit discriminated union) get a parse or heal assertion. Since these add a defaulted enum, a new selection shape, and a new `activeTab` value, a round-trip/heal test for each would catch regressions on read/write healing.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphTabContent/GraphTabContent.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphTabContent/GraphTabContent.tsx:151">
P3: The `limit` prop is discarded as an unused `_limit`, leaving a dead prop contract that can drift from the component's actual pagination behavior; removing it from this component and its caller would keep the API honest.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/components/CommitMetadataHeader/CommitMetadataHeader.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/components/CommitMetadataHeader/CommitMetadataHeader.tsx:23">
P3: Indented commit bodies lose their leading spaces in the metadata pane because `body` is passed through `.trim()`, making formatted commit messages inaccurate. Remove only the separator newline while preserving body indentation.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/RefBadge/RefBadge.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/RefBadge/RefBadge.tsx:93">
P2: Long ref names can be clipped without an ellipsis in the inline layout, making the intended capped badge harder to read; allowing the label flex item to shrink lets its existing `text-ellipsis` work as intended.</violation>
</file>
<file name="packages/host-service/src/workers/tasks/git.ts">
<violation number="1" location="packages/host-service/src/workers/tasks/git.ts:347">
P2: The `open-workspaces` scope includes commits reachable only from deleted detached worktrees, so stale history appears as an open workspace. Excluding `w.prunable` here would match the existing branch-worktree filtering and the documented scope semantics.</violation>
</file>
<file name="apps/desktop/src/renderer/globals.css">
<violation number="1" location="apps/desktop/src/renderer/globals.css:63">
P3: Dark-theme fallback lanes use the generic xterm bright ANSI values (#729fcf/#8ae234/#ef2929/#34e2e2/#ad7fa8/#fce94f), but the default ember theme ships its own bright palette (#7ec0f5/#98d1a8/#e88888/#73c7d3/#d494e6/#ecd08f). The light fallback in this same diff matches light.ts exactly, so the dark set is the only mismatch — before applyGraphLaneColors runs, hydrated graph lanes would flash to colors that never match the actual theme. Consider mirroring ember's bright ANSI values to keep the fallback consistent with the live set.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphRow/GraphRow.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphRow/GraphRow.tsx:73">
P2: Commits carrying several long refs can lose their subject entirely at standard sidebar widths because the fixed three-badge allowance and non-shrinking ref strip consume all space before the subject. Reserve a minimum subject width or make the ref summary adapt to available width by collapsing additional badges.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/useGraphTab.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/useGraphTab.tsx:97">
P2: A ref change in one workspace invalidates all cached graph queries because this call omits the workspace input, so unrelated graph observers or later workspace revisits can trigger costly traversals. Scoping the invalidation to `{ workspaceId }` keeps refreshes local to the event.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/utils/assignLanes/assignLanes.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/utils/assignLanes/assignLanes.ts:34">
P2: Existing branch lanes change color when a newly added earlier tip or a scope/refetch changes traversal order, so row tints and edge colors no longer identify the same branch across updates. Derive the lane color from the originating branch ref and propagate that identity through forks, using the documented lane-index fallback only when no branch name exists.</violation>
</file>
<file name="plans/done/20260803-commit-graph-followups.md">
<violation number="1" location="plans/done/20260803-commit-graph-followups.md:98">
P3: §2.1 says to 'Add a graphSelection field to sidebarState' and points at `schema.ts:132`, but the schema already defines `graphSelectionSchema` (line 117) with a nullable `graphSelection` default (lines 158/202), and the real file path is under renderer/routes/_authenticated/. Update the section to reflect that this wiring already exists rather than presenting it as work to do.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphLanes/GraphLanes.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphLanes/GraphLanes.tsx:42">
P2: Overflowed merge and fork edges can render as backward hooks when both endpoints collapse onto the last visible lane. Using a zero direction when `x0 === x1` would make the collapsed path vertical rather than extending past and back over the same lane.</violation>
<violation number="2" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphLanes/GraphLanes.tsx:91">
P3: Each row's svg is marked `aria-hidden="true"` but still carries an inert `<title>Commit graph lanes</title>`. Since the element is hidden from the accessibility tree, the title never reaches assistive tech, and in this virtualized list the identical text node is duplicated into every rendered row's DOM for no benefit while adding per-row DOM weight. It would be cleaner to drop the `<title>` (the column is purely decorative under aria-hidden), or if a label is genuinely wanted, make it meaningful and move it outside the aria-hidden subtree.</violation>
<violation number="3" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphLanes/GraphLanes.tsx:114">
P2: Plain commit nodes keep the base sidebar color inside their rings even when the row is hovered or selected, creating a visible square/spot mismatch with the row background. The node fill should track the row's effective ground color rather than using a fixed `fill-sidebar` token.</violation>
</file>
<file name="apps/desktop/src/shared/themes/graph-lanes.ts">
<violation number="1" location="apps/desktop/src/shared/themes/graph-lanes.ts:39">
P2: The light-theme lane ramp includes ANSI yellow without contrast normalization, making lane 6 a low-contrast 1.5px stroke on the light sidebar. Deriving the palette through a contrast-checked lightness adjustment (while preserving each hue) would keep all lanes readable.</violation>
</file>
<file name="packages/host-service/src/trpc/router/git/git.ts">
<violation number="1" location="packages/host-service/src/trpc/router/git/git.ts:335">
P2: A workspace whose worktree was deleted is returned as a successful empty graph because this path bypasses `resolveWorktreePath`; using the shared resolver preserves the expected `NOT_FOUND`/`WORKTREE_MISSING` response.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/DiffPane.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/DiffPane.tsx:337">
P2: Empty commits selected from the Graph pane show only “No changes” and never display their commit metadata, even though `getCommitFiles` returns a `commit` for them. Rendering the header before the empty-file early returns would preserve the commit subject, author, and hash for valid zero-file commits.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| if ( | ||
| type === "remote" && | ||
| fullRef.startsWith("refs/remotes/") && | ||
| fullRef.endsWith("/HEAD") |
Contributor
There was a problem hiding this comment.
P2: A valid remote branch named feature/HEAD will never appear as a graph ref badge because the symbolic-HEAD filter matches any nested ref ending in /HEAD; restrict the filter to exactly refs/remotes/<remote>/HEAD so nested branch names remain visible.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/host-service/src/trpc/router/git/utils/graph-log.ts, line 88:
<comment>A valid remote branch named `feature/HEAD` will never appear as a graph ref badge because the symbolic-HEAD filter matches any nested ref ending in `/HEAD`; restrict the filter to exactly `refs/remotes/<remote>/HEAD` so nested branch names remain visible.</comment>
<file context>
@@ -0,0 +1,212 @@
+ if (
+ type === "remote" &&
+ fullRef.startsWith("refs/remotes/") &&
+ fullRef.endsWith("/HEAD")
+ ) {
+ continue;
</file context>
Suggested change
| fullRef.endsWith("/HEAD") | |
| \t\t\t\t/^refs\/remotes\/[^/]+\/HEAD$/.test(fullRef) |
| const commits: RawGraphCommit[] = []; | ||
| for (const line of raw.split("\n")) { | ||
| if (!line) continue; | ||
| const parts = line.split("\t"); |
Contributor
There was a problem hiding this comment.
P2: Graph metadata is corrupted for commits whose author name contains a tab: the field split shifts the email and date because only the subject is treated as tab-safe. Use an unambiguous delimiter or escaping for every structured field in the worker format and parser.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/host-service/src/trpc/router/git/utils/graph-log.ts, line 113:
<comment>Graph metadata is corrupted for commits whose author name contains a tab: the field split shifts the email and date because only the subject is treated as tab-safe. Use an unambiguous delimiter or escaping for every structured field in the worker format and parser.</comment>
<file context>
@@ -0,0 +1,212 @@
+ const commits: RawGraphCommit[] = [];
+ for (const line of raw.split("\n")) {
+ if (!line) continue;
+ const parts = line.split("\t");
+ if (parts.length < 7) continue;
+ const [hash, shortHash, parentsRaw, author, authorEmail, date] = parts;
</file context>
| Index order is deliberately *not* the hue order — adjacent lanes on screen get maximally separated | ||
| hues (262 → 152 → 25 → 197), which matters far more than the ramp reading as a gradient when listed. | ||
|
|
||
| **Assignment.** Lane colour is `hash(originatingBranchName) % 8`, not `laneIndex % 8`. A lane keeps |
Contributor
There was a problem hiding this comment.
P2: The §3 'Assignment' paragraph still describes lane colour as hash(originatingBranchName) % 8, but that is precisely the scheme the shipped assignLanes replaced (rotating cursor with live-lane skip) after the §1.2 collision defect was confirmed — 15 of 42 live rows painted the same colour. Since this spec is the 'signed off' design contract that owns colour, keeping the old algorithm here will mislead anyone applying the spec later. Update the Assignment paragraph to describe the rotating live-skip cursor (or point at assignLanes' pickColor) so the spec stops contradicting the implementation and the follow-up.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/plans/20260801-1200-git-graph-visual-spec.md, line 147:
<comment>The §3 'Assignment' paragraph still describes lane colour as `hash(originatingBranchName) % 8`, but that is precisely the scheme the shipped `assignLanes` replaced (rotating cursor with live-lane skip) after the §1.2 collision defect was confirmed — 15 of 42 live rows painted the same colour. Since this spec is the 'signed off' design contract that owns colour, keeping the old algorithm here will mislead anyone applying the spec later. Update the Assignment paragraph to describe the rotating live-skip cursor (or point at `assignLanes`' `pickColor`) so the spec stops contradicting the implementation and the follow-up.</comment>
<file context>
@@ -0,0 +1,348 @@
+Index order is deliberately *not* the hue order — adjacent lanes on screen get maximally separated
+hues (262 → 152 → 25 → 197), which matters far more than the ramp reading as a gradient when listed.
+
+**Assignment.** Lane colour is `hash(originatingBranchName) % 8`, not `laneIndex % 8`. A lane keeps
+its colour when the graph refetches and lane indices shift. Fallback for a lane with no branch name:
+`laneIndex % 8`.
</file context>
| icon: LuGitCompareArrows, | ||
| }; | ||
|
|
||
| const graphTabDef = useGraphTab({ |
Contributor
There was a problem hiding this comment.
P2: Every workspace now performs a 500-commit git.listGraph traversal when its sidebar mounts, even if the user stays on Files, Changes, or Review. Passing the active-tab state through to disable this query until Graph is selected, or lazy-mounting the tab content, would avoid adding this host-service/git cost to the normal sidebar path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/WorkspaceSidebar.tsx, line 139:
<comment>Every workspace now performs a 500-commit `git.listGraph` traversal when its sidebar mounts, even if the user stays on Files, Changes, or Review. Passing the active-tab state through to disable this query until Graph is selected, or lazy-mounting the tab content, would avoid adding this host-service/git cost to the normal sidebar path.</comment>
<file context>
@@ -112,6 +136,19 @@ export function WorkspaceSidebar({
icon: LuGitCompareArrows,
};
+ const graphTabDef = useGraphTab({
+ workspaceId,
+ compact,
</file context>
| /> | ||
| <span | ||
| className={cn( | ||
| "overflow-hidden text-ellipsis", |
Contributor
There was a problem hiding this comment.
P2: Long ref names can be clipped without an ellipsis in the inline layout, making the intended capped badge harder to read; allowing the label flex item to shrink lets its existing text-ellipsis work as intended.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/RefBadge/RefBadge.tsx, line 93:
<comment>Long ref names can be clipped without an ellipsis in the inline layout, making the intended capped badge harder to read; allowing the label flex item to shrink lets its existing `text-ellipsis` work as intended.</comment>
<file context>
@@ -0,0 +1,101 @@
+ />
+ <span
+ className={cn(
+ "overflow-hidden text-ellipsis",
+ graphRef.state === "prunable" && "line-through",
+ )}
</file context>
| const subject = | ||
| newlineIdx === -1 ? commit.message : commit.message.slice(0, newlineIdx); | ||
| const body = | ||
| newlineIdx === -1 ? "" : commit.message.slice(newlineIdx + 1).trim(); |
Contributor
There was a problem hiding this comment.
P3: Indented commit bodies lose their leading spaces in the metadata pane because body is passed through .trim(), making formatted commit messages inaccurate. Remove only the separator newline while preserving body indentation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/usePaneRegistry/components/DiffPane/components/CommitMetadataHeader/CommitMetadataHeader.tsx, line 23:
<comment>Indented commit bodies lose their leading spaces in the metadata pane because `body` is passed through `.trim()`, making formatted commit messages inaccurate. Remove only the separator newline while preserving body indentation.</comment>
<file context>
@@ -0,0 +1,67 @@
+ const subject =
+ newlineIdx === -1 ? commit.message : commit.message.slice(0, newlineIdx);
+ const body =
+ newlineIdx === -1 ? "" : commit.message.slice(newlineIdx + 1).trim();
+
+ return (
</file context>
Comment on lines
+290
to
+292
| - **Scope control** — the header carries the `refScope` select (`Local refs` / the three | ||
| not-yet-implemented scopes, disabled). Persisted per project, so it does not belong in the row spec | ||
| beyond leaving the header 24px. |
Contributor
There was a problem hiding this comment.
P3: The §6 Scope control entry is stale and contradicts the shipped code: it claims the select sits in the header showing "Local refs" with "the three not-yet-implemented scopes, disabled". All five scopes are implemented and enabled, the default scope's label is "All local branches" (not "Local refs"), and the control lives on the graph's own strip rather than the header. Since this spec is the design contract for the tab, a reader following §6 would be misled about the shipped scope surface.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/plans/20260801-1200-git-graph-visual-spec.md, line 290:
<comment>The §6 Scope control entry is stale and contradicts the shipped code: it claims the select sits in the header showing "Local refs" with "the three not-yet-implemented scopes, disabled". All five scopes are implemented and enabled, the default scope's label is "All local branches" (not "Local refs"), and the control lives on the graph's own strip rather than the header. Since this spec is the design contract for the tab, a reader following §6 would be misled about the shipped scope surface.</comment>
<file context>
@@ -0,0 +1,348 @@
+- **Truncated** — sticky footer row, 24px, `--muted` background, hairline top border:
+ **"Showing 500 of 12,431 commits"** with a **Load more** button on the right. Numbers use
+ `tabular-nums`. Renders only when `nextCursor !== null`.
+- **Scope control** — the header carries the `refScope` select (`Local refs` / the three
+ not-yet-implemented scopes, disabled). Persisted per project, so it does not belong in the row spec
+ beyond leaving the header 24px.
</file context>
Suggested change
| - **Scope control** — the header carries the `refScope` select (`Local refs` / the three | |
| not-yet-implemented scopes, disabled). Persisted per project, so it does not belong in the row spec | |
| beyond leaving the header 24px. | |
| - **Scope control** — the graph's own control strip carries the `refScope` select (This workspace / Open workspaces / All local branches / Remote / Everything; default All local branches), plus the two-line-refs and unreferenced-only toggles. Persisted per project, so it does not belong in the row spec beyond leaving the strip rows at their set heights. |
| --fill-selected: color-mix(in oklab, var(--foreground) 10%, transparent); | ||
| /* Commit-graph lanes: pre-hydration fallback for the bright ANSI set. | ||
| The live values come from shared/themes/graph-lanes via applyGraphLaneColors. */ | ||
| --graph-lane-1: #729fcf; |
Contributor
There was a problem hiding this comment.
P3: Dark-theme fallback lanes use the generic xterm bright ANSI values (#729fcf/#8ae234/#ef2929/#34e2e2/#ad7fa8/#fce94f), but the default ember theme ships its own bright palette (#7ec0f5/#98d1a8/#e88888/#73c7d3/#d494e6/#ecd08f). The light fallback in this same diff matches light.ts exactly, so the dark set is the only mismatch — before applyGraphLaneColors runs, hydrated graph lanes would flash to colors that never match the actual theme. Consider mirroring ember's bright ANSI values to keep the fallback consistent with the live set.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/globals.css, line 63:
<comment>Dark-theme fallback lanes use the generic xterm bright ANSI values (#729fcf/#8ae234/#ef2929/#34e2e2/#ad7fa8/#fce94f), but the default ember theme ships its own bright palette (#7ec0f5/#98d1a8/#e88888/#73c7d3/#d494e6/#ecd08f). The light fallback in this same diff matches light.ts exactly, so the dark set is the only mismatch — before applyGraphLaneColors runs, hydrated graph lanes would flash to colors that never match the actual theme. Consider mirroring ember's bright ANSI values to keep the fallback consistent with the live set.</comment>
<file context>
@@ -58,6 +58,16 @@
--fill-selected: color-mix(in oklab, var(--foreground) 10%, transparent);
+ /* Commit-graph lanes: pre-hydration fallback for the bright ANSI set.
+ The live values come from shared/themes/graph-lanes via applyGraphLaneColors. */
+ --graph-lane-1: #729fcf;
+ --graph-lane-2: #8ae234;
+ --graph-lane-3: #ef2929;
</file context>
| (`useGraphTab.tsx:76-86`), and the shift-click anchor is read out of the same field | ||
| (`useGraphTab.tsx:95-110`). Stop writing `changesFilter` and the graph stops highlighting itself. | ||
|
|
||
| Add a `graphSelection` field to `sidebarState` |
Contributor
There was a problem hiding this comment.
P3: §2.1 says to 'Add a graphSelection field to sidebarState' and points at schema.ts:132, but the schema already defines graphSelectionSchema (line 117) with a nullable graphSelection default (lines 158/202), and the real file path is under renderer/routes/_authenticated/. Update the section to reflect that this wiring already exists rather than presenting it as work to do.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At plans/done/20260803-commit-graph-followups.md, line 98:
<comment>§2.1 says to 'Add a graphSelection field to sidebarState' and points at `schema.ts:132`, but the schema already defines `graphSelectionSchema` (line 117) with a nullable `graphSelection` default (lines 158/202), and the real file path is under renderer/routes/_authenticated/. Update the section to reflect that this wiring already exists rather than presenting it as work to do.</comment>
<file context>
@@ -0,0 +1,359 @@
+(`useGraphTab.tsx:76-86`), and the shift-click anchor is read out of the same field
+(`useGraphTab.tsx:95-110`). Stop writing `changesFilter` and the graph stops highlighting itself.
+
+Add a `graphSelection` field to `sidebarState`
+(`CollectionsProvider/dashboardSidebarLocal/schema.ts:132`), same discriminated shape as
+`changesFilterSchema` minus the `all` case:
</file context>
| height={height} | ||
| viewBox={`0 0 ${width} ${height}`} | ||
| > | ||
| <title>Commit graph lanes</title> |
Contributor
There was a problem hiding this comment.
P3: Each row's svg is marked aria-hidden="true" but still carries an inert <title>Commit graph lanes</title>. Since the element is hidden from the accessibility tree, the title never reaches assistive tech, and in this virtualized list the identical text node is duplicated into every rendered row's DOM for no benefit while adding per-row DOM weight. It would be cleaner to drop the <title> (the column is purely decorative under aria-hidden), or if a label is genuinely wanted, make it meaningful and move it outside the aria-hidden subtree.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useGraphTab/components/GraphLanes/GraphLanes.tsx, line 91:
<comment>Each row's svg is marked `aria-hidden="true"` but still carries an inert `<title>Commit graph lanes</title>`. Since the element is hidden from the accessibility tree, the title never reaches assistive tech, and in this virtualized list the identical text node is duplicated into every rendered row's DOM for no benefit while adding per-row DOM weight. It would be cleaner to drop the `<title>` (the column is purely decorative under aria-hidden), or if a label is genuinely wanted, make it meaningful and move it outside the aria-hidden subtree.</comment>
<file context>
@@ -0,0 +1,132 @@
+ height={height}
+ viewBox={`0 0 ${width} ${height}`}
+ >
+ <title>Commit graph lanes</title>
+ {row.edges.map((edge) => (
+ <path
</file context>
Contributor
|
This PR has had no activity for 53 days and will close in 7 days unless there is new activity. If this contribution is still important or relevant, please leave a comment to keep it open. If it closes, you are welcome to reopen it or ask a maintainer to reopen it. Thank you for contributing! |
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.
What & why
Adds a fourth Graph tab to the v2 workspace sidebar: a lane-based commit graph whose primary value is ref classification, not the lanes. The lanes are the frame; what the tab is actually for is surfacing branches and worktrees that exist in git but have no open workspace behind them.
Commit history was only reachable as a flat
base..HEADdropdown — no topology, and no view at all of the branches and worktrees that accumulate outside the currently-open workspaces. A branch whose worktree was deleted, or a worktree with no workspace row, was invisible until it caused a problem.Read-only. No write actions (checkout, cherry-pick, revert, prune) in this PR.
Relates to #4935 and #5594 (#3849 folded in).
Host service —
git.listGraphNew procedure alongside
listCommits, which is untouched (desktop and mobile both consume its output type).getCommitFilesis the one existing procedure that changed: it now also returns the commit's own metadata (subject, body, author, date, parents), so a diff pane opened from the graph renders its header without a second spawn.Tip set is seeded from git's own refs, never from workspace records.
refScopepicks how wide it goes, and all five values are served:headHEADopen-workspaceslocal(default)remoteHEAD+--remotesall--exclude=refs/stash --allremoteandallpass one flag rather than enumerating refnames, so a tag-heavy repo can't overflow the command line. Detached worktree heads are still named explicitly — they aren't refs. Underopen-workspaces, a worktree git marksprunableis excluded: its directory is gone, so it is not an open workspace.Ref state is classified per local branch:
open·detached-worktree·prunable(+ reason) ·merged·orphan-branch. Containment comes fromgit branch --merged <baseRef>, not from graph topology, so a branch merged far outside the window still reportsmerged.prunablecomes from git's own porcelain flag (gitdir file points to non-existent location) rather than a per-branchexistsSync, so classification costs no synchronous fs calls on the host event loop.Every git spawn happens in the worker (
gitGraphLogTask), including base resolution, so both stdout draining and parsing stay off the host event loop. Decorations are built fromfor-each-refand resolved throughResolvedRef— never from%D, never from a shortname prefix.Overlapping traversals coalesce, so a
git:changedstorm during a rebase doesn't fan out into duplicate walks.The pagination cursor is tagged with the scope it was issued for.
--skipcounts rows of one traversal, so replaying alocaloffset againstheadwould window a list that never had those rows; a cursor from another scope restarts at page one instead of serving a silently wrong page. Scope is also part of the coalescing dedupe key, so two scopes never share one walk.Renderer
assignLanes— pure, tested, no React or tRPC. Active-lane sweep over topo-ordered input; lane identity is pinned to the originating branch so a refetch doesn't reshuffle the graph.--graph-lane-1..8tokens in both themes, equal chroma/lightness so the lanes read as one family and survive a theme switch. No component hardcodes a colour; the live values come fromshared/themes/graph-lanesthroughapplyGraphLaneColors, and the hex inglobals.cssis only the pre-hydration fallback set.diffpane kind; the graph owns its own persisted selection, separate from the Changes tab's filter, so the two surfaces no longer fight over one piece of state. Shift-click ranges, cmd/ctrl-click forces a new pane, double-click pins — matching the file tree's convention exactly.Notable decisions
The "unreferenced" filter dims, it does not remove. Lanes and edges are assigned across the whole commit list; dropping rows would leave the surviving edges pointing at rows that aren't there, and filtering before lane assignment yields a graph of disconnected nodes.
Selected rows and range-interior rows share a tint alpha. Selection also carries
--accent; the range interior doesn't. Same alpha therefore keeps an endpoint strictly brighter than the band it anchors, in both themes, with no per-theme numbers.The row size cache is keyed by commit hash, not by list index. Refs move while the tab is open, so a refetch that prepends a commit shifts every index — under the virtualizer's default index key a row inherits the cached height of whatever previously sat in its slot, which in two-line mode misfits by a whole badge line and shows up as overlapping rows or a blank band. Keying by hash lets a cached size travel with its commit;
rowsis also in the remeasure deps for the case the key can't catch, a ref landing on a commit whose hash doesn't change. Only reproduces on a repo with live churn."Open workspaces" is read off
git worktree list, not the workspaces table. The worker has no DB access, and every Superset workspace is a worktree, so the worktree list already answers the question. The known ceiling: a worktree created outside Superset counts as an open workspace. Joining against DB rows in the coordinator is the upgrade path if that ever matters.The header toggles read through a live query, not
.get(). They write tov2SidebarProjects, and.get()is a non-reactive snapshot — the row changed in localStorage while the header kept rendering the stale flag until some unrelated re-render happened to run, which reads as a multi-second lag on a write that already completed. No spinner: the write is synchronous and local, so a pending indicator would animate over an already-finished state change. The toggles alsoensureProjectInSidebarfirst, matchinguseV2ProjectDefaultApp— a project with no sidebar row yet would otherwise swallow the write entirely.The strip's controls are not in
SidebarHeader'sactionsslot. That slot is ashrink-0island in a fixed 40px row whose four tab buttons are eachflex-1with nomin-w-0; anything rendered there clips the last tab label instead of shrinking it. The chooser and toggles live on the graph's own control strip instead.How I tested it
bun run lintbun run typecheck@superset/ui, which fails on a duplicate@types/hastresolution innode_modules. Nopackages/uifile,package.json, orbun.lockis in this branch's diff, so it is not from herebun run testapps/desktoptestsgetProcessEnvWithShellPath, environment-dependent and untouched by this branchpackages/host-servicetestsrefScopehas its own integration test: a branch with no worktree is absent underheadandopen-workspacesbut present underlocalandall; a worktree checked out on that branch whose directory is then deleted stays excluded fromopen-workspaces, which fails without the prunable filter; andremoteexcludes the local-only branch untilrefs/remotes/origin/sideexists, then includes it. Cursor handling is covered both ways — alocalcursor pages forward, and the same cursor handed toheadrestarts at page one instead of windowing rows that traversal never produced.buildTipSetis unit-tested per scope, including thatallandremoteemit a flag rather than a refname list.Integration test covers all five ref states against a fixture repo (open workspace, worktree with no workspace row, orphan branch, prunable worktree with its path deleted) plus a merge commit, a tag, and a branch merged outside the commit window. Lane assignment is unit-tested for linear / merge / octopus / criss-cross / root / lane reuse / parent outside the window, including determinism when commits are appended.
Rendering was checked against the running app at 260px and 400px sidebar widths, in both toggle states, with the measurements alongside the screenshots (total virtual size, per-row height, badge clipping). The toggle-reactivity fix was verified end-to-end on the running app.
The row-height regression is pinned by a test that prepends a commit to a mixed-height list and asserts every commit present in both lists keeps the same
(key, size)pair, looked up by hash rather than by slot. It was confirmed red against the pre-fix behaviour: restoring the index key fails it. The size and key derivation was extracted into a puregraphRowSizerto make that testable — this repo has no jsdom or testing-library, and adding one for two callbacks wasn't worth the dependency.It was also verified live: with the Graph tab open on a repo of ~1600 commits, a commit landing on
HEADprepended a two-line row and shifted every index by one. Row offsets stayed consistent with rendered heights across the shift and across the subsequent removal — 0 mismatches in both directions, where the pre-fix build produced an overlap at the slot that grew and a blank band at the slot that shrank.Screenshots
Captured from the dev app over CDP (
RENDERER_REMOTE_DEBUG_PORT=9222 bun dev, renderer matched to this workspace'sDESKTOP_VITE_PORT), driving the real controls with dispatched mouse input. The clip is the sidebar column only; the rest of the window is unchanged by this PR. The branches belowmainare a throwaway topology built for these shots and deleted afterwards — the repo's own history is a straight line, which shows none of the cases the tab exists for.Inline refs (default) — four concurrent lanes, two merges and an octopus. The fork elbow leaves the node horizontally then bends down; the merge descends then bends in, so the two read apart at a 3.5px node. Row tint carries lane membership without tracing the line. The dashed, struck-through red badge is a branch whose worktree directory is gone (
prunable); badges truncate to fit 260px.Scope chooser — the five scopes as the user reads them,
All local branchesselected. Plain wording rather than git's, because the audience is people who don't think in refs.Refs on their own line (toggle on) — same commits, badges move above the row they decorate and stop truncating:
origin/mainand the long branch names read in full. That is the trade the toggle exists for, and the reason row height has to come from the commit's own refs.Highlight unreferenced (toggle on) — the point of the tab. Rows carrying a branch or worktree with no open workspace behind it stay bright; everything already accounted for dims. Nothing is removed, so the lanes and edges still connect.
Checklist
type(scope): subject)bun run lintpasses with 0 output (CI fails on warnings too)bun run typecheck— green for every package this PR touches;@superset/uifails on a pre-existing duplicate@types/hastresolution, see the table aboveKnown gaps
--skipoffset, so pages re-anchor if refs move between fetches. Crossing scopes is handled (the cursor is dropped), moving refs are not. The truncation banner surfaces when a repo has more history than one page.Everythingis now largelyAll local branches ∪ Remote ∪ tags, so it may not earn its slot. Left in for one release to see whether anyone picks it.remotereflects the last fetch. Nothing in this PR fetches, so a stale remote reads as a stale graph.All local branchesis the longest label and squeezes the "Showing first N of M" text beside it at the narrowest sidebar widths.Summary by CodeRabbit