fix(chat): show repo-qualified path in turn changed-files card - #5375
Pulkit7070 wants to merge 5 commits into
Conversation
Each row in a turn's changed-files card showed only the file basename. In a workspace with more than one repository, two files with the same name in different repos both render as the same bare filename, and the label no longer matches the repository-qualified path the Files inspector opens for that row. Show the same workspace-relative path the row already opens (openPath from turnFileOpenPath), which reconstructs the repository qualifier from the turn's absolute file_change paths and cwd. The daemon stores the provider's repo-relative path verbatim, so this qualifier is reconstructed on the client and did not appear in the card before. Fixes Untrivial-ai#5366
i-trytoohard
added
bug
Prasad-D-Ware
left a comment
Prasad-D-Ware
left a comment
There was a problem hiding this comment.
Review of the repo-qualified change-card label.
Scope check first: this diff only swaps the label's source (fileBasename(file.path) -> openPath). openPath already existed and already drove the click target, so click destinations, tooltips, and ordering are unchanged. No behavioral regression — everything below is either a missed case or a label-only issue.
The load-bearing one is the first inline comment: as written this makes the label and click target consistent, but does not fix the duplicate-name disambiguation #5366 describes.
| title="" | ||
| > | ||
| {fileBasename(file.path)} | ||
| {openPath} |
There was a problem hiding this comment.
This misses the case #5366 reports.
When two same-named files change in the same turn, rememberTurnPathHint sets the colliding basename entry to undefined. turnFileOpenPath then falls back to the bare workspace-test.txt for both rows — which is exactly the ambiguity the issue is about.
The new tests only ever pass a single file_change entry, so the collision branch is never exercised. A test with two entries sharing a basename (alpha/workspace-test.txt + beta/workspace-test.txt) would fail today.
The fix belongs in turnFileOpenPath / rememberTurnPathHint — keep all candidates per basename and disambiguate against cwd — rather than in the display layer.
There was a problem hiding this comment.
Hints now keep every distinct candidate per basename instead of collapsing to undefined on the first collision, and matching happens against the whole relative suffix. Two rows that carry a repo subdir (alpha/workspace-test.txt, beta/workspace-test.txt) now each resolve to their own repo, with a component test exercising two file_change entries that share a basename.
One limit worth stating plainly: when both repos store the file as a bare workspace-test.txt, the two diff rows are identical input strings, so no display-layer function can render them differently. In that case the row keeps the honest basename rather than guessing a repo. Truly disambiguating that case needs the daemon to store a repo qualifier on the diff path; that is out of scope for this change.
| title="" | ||
| > | ||
| {fileBasename(file.path)} | ||
| {openPath} |
There was a problem hiding this comment.
Basename-only hint matching can now name the wrong file.
Given a diff row src/a.ts plus an unrelated <cwd>/other/a.ts in the turn's activity, the hint matches on basename alone and the row renders other/a.ts.
To be fair this isn't strictly new — the click target was already wrong in that case, so arguably this just makes the label honest about an existing bug. But the user now sees a wrong path where they previously saw a correct basename, so it's worth handling alongside the collision fix above (same root cause: basename-keyed hints).
There was a problem hiding this comment.
Fixed. Matching is now on the whole relative suffix, not the basename alone: a candidate only binds when its normalized path equals the row or ends with /<row-relative-path>. A row src/a.ts against an unrelated <cwd>/other/a.ts no longer renders other/a.ts; it falls back to src/a.ts.
| title="" | ||
| > | ||
| {fileBasename(file.path)} | ||
| {openPath} |
There was a problem hiding this comment.
No-cwd fallback invents a qualifier that doesn't exist in the workspace.
With no cwd hint available, workspaceRelativeOpenPath's slice(-2) fallback prefixes the label with the worktree directory name — e.g. demo-1/file.txt. demo-1 is a worktree folder, not a workspace path segment, so the label claims a directory the user can't find.
When there's no reliable qualifier, falling back to the plain basename reads better than a fabricated one.
There was a problem hiding this comment.
Fixed. Dropped the slice(-2) fallback in workspaceRelativeOpenPath. With no cwd to anchor against, the leading segments are the worktree directory rather than workspace path, so it now returns the plain basename instead of demo-1/file.txt. Updated the two tests that asserted the fabricated prefix.
| @@ -2520,7 +2520,7 @@ export function TurnChangedFiles({ | |||
| className="min-w-0 flex-1 truncate text-[12px] text-foreground/80" | |||
There was a problem hiding this comment.
truncate now clips the part that identifies the row.
This span still uses truncate (tail ellipsis), which was fine for basenames but cuts the wrong end now that it renders full paths: frontend/src/renderer/comp… drops the filename entirely.
Head-truncation (…/renderer/components/chat/ChatTimelineItems.tsx) or direction: rtl on the span keeps the filename visible, which is the one segment a user scans for.
There was a problem hiding this comment.
Fixed. Both label spans now use direction: rtl with text-left and a bdi wrapper, so the ellipsis lands at the head (…/chat/ChatTimelineItems.tsx) and the filename stays visible. bdi keeps RTL from reordering the path segments.
…iers Address review of the repo-qualified change-card label: - Keep every distinct absolute candidate per basename instead of collapsing to undefined on the first collision, so subdir-qualified rows in a multi-repo workspace each resolve to their own repo. - Match hints on the whole relative suffix, not the basename alone, so a row like src/a.ts no longer binds to an unrelated other/a.ts. - Drop the slice(-2) fallback in workspaceRelativeOpenPath: with no cwd to anchor against, the leading segments are the worktree directory, not workspace path, so fall back to the basename rather than inventing a qualifier such as demo-1/file.txt. - Head-truncate the label spans (direction: rtl + bdi) so full paths keep the filename visible instead of clipping it.
Prasad-D-Ware
left a comment
Prasad-D-Ware
left a comment
There was a problem hiding this comment.
Re-reviewed at ca9d6ce5c. All four earlier findings are addressed — the suffix-matching matchTurnCandidate plus the multi-candidate byBase map genuinely fixes the multi-repo collision from #5366 and the wrong-directory match, and the head-truncation ([direction:rtl] + <bdi>) fixes the clipped filename. Nice rework; the helper is the right place for this.
Two new findings, and they're both the same over-correction: removing the slice(-2) fallback went one step further than needed and now discards qualification that used to survive. In both cases below the PR makes the path less qualified than pre-PR, which is the opposite of the goal.
I verified both by running the old and new implementations side by side in node.
Cleared on inspection, for the record: the <bdi> dir=auto resolves LTR for real filenames so no segment reordering; the unconditional rtl class is harmless at the two basename-only FileLocationLabel call sites (tailwind-merge keeps the caller's text-[12px]); the absolute-path branch matches the old resolveTurnFilePath round-trip; and the ambiguous-row tooltip fabricating ${cwd}/${path} is pre-existing, not introduced here.
| return workspaceRelativeOpenPath(fromBasename, hints.cwd); | ||
| } | ||
| const matched = matchTurnCandidate(normalized, hints); | ||
| if (matched) return workspaceRelativeOpenPath(matched, hints.cwd); |
There was a problem hiding this comment.
A hint match with no cwd now throws away qualification the row already had.
If the turn has only file_change activities (no command, so hints.cwd is undefined), workspaceRelativeOpenPath can't strip a root and returns the basename. So a row that already carried its directory segments loses them:
row: src/a.ts
hint: /w/demo-1/src/a.ts
cwd: undefined
pre-PR: src/a.ts
post-PR: a.ts
The card then shows a.ts and onOpenFile("a.ts") has nothing to resolve against. The hint matched successfully and we ended up worse than the input.
Suggested fix: when there's no cwd anchor, prefer the row's own normalized over a hint-derived basename — the hint only helps once there's a root to strip.
if (matched) {
const open = workspaceRelativeOpenPath(matched, hints.cwd);
return open.includes("/") || !normalized.includes("/") ? open : normalized;
}There was a problem hiding this comment.
Fixed. turnFileOpenPath now keeps the row's own path when a matched hint would collapse to a bare basename with no cwd to strip against: open.includes("/") || !normalized.includes("/") ? open : normalized. So src/a.ts with hint /w/demo-1/src/a.ts and no cwd stays src/a.ts instead of degrading to a.ts. Added a test asserting exactly that.
| if (segments.length >= 2) { | ||
| return segments.slice(-2).join("/"); | ||
| } | ||
| return fileBasename(normalized); |
There was a problem hiding this comment.
Absolute paths outside the recorded cwd degrade to a bare basename.
turnPathHints records the first activity's cwd (line 60), and agents routinely run a command in a subdirectory before editing elsewhere in the worktree:
cwd: /w/demo-1/frontend (from the first command activity)
file: /w/demo-1/backend/x.ts
pre-PR: backend/x.ts
post-PR: x.ts
The startsWith(root + "/") check fails, so we fall straight through to fileBasename and the row both displays and opens an unresolvable name.
The doc comment above justifies this by saying the leading segments are worktree directory rather than workspace path — true for the worktree root, but not for a cwd that's a subdirectory of it. Walking up from cwd to the worktree root (or keeping the deepest common ancestor) would preserve backend/x.ts here without reintroducing the demo-1/ fabrication.
There was a problem hiding this comment.
Fixed. workspaceRelativeOpenPath now anchors on the deepest directory the file and the cwd share (commonDirPrefix) rather than requiring the file to sit under the cwd. With cwd=/w/demo-1/frontend and file=/w/demo-1/backend/x.ts it strips the shared /w/demo-1 and keeps backend/x.ts, and it still never prefixes a segment that is not already in the path, so no demo-1/ fabrication returns. Added a test for the sibling-directory case.
Address follow-up review of the change-card label: - workspaceRelativeOpenPath now strips the deepest directory the file and the cwd share instead of requiring the file to sit under the cwd. When an agent runs a command in frontend/ then edits backend/x.ts, the shared worktree root is stripped and backend/x.ts survives, without ever prefixing a segment that is not already in the path. - turnFileOpenPath keeps the row's own path when a matched hint would collapse to a bare basename with no cwd to strip against, so src/a.ts no longer degrades to a.ts.
Prasad-D-Ware
left a comment
Prasad-D-Ware
left a comment
There was a problem hiding this comment.
Deep review at e9e2b3988. Both findings from the last round are fixed — the :125 guard and the new commonDirPrefix anchor both work, and cwd-in-frontend/ correctly yields backend/x.ts. CI is green on all four checks; everything below I verified by re-executing the helper logic in node or reading the code at head, not by running the suite locally.
The load-bearing pair is #1 and #4: as it stands the PR improves the common case but still renders a bare basename in a scenario #5366 specifically describes, and the data needed to fix it properly is already wired up a few components away.
Smaller cleanup notes, not worth inline threads: fileBasename is now defined identically three times (turn-file-open-path.ts:12, workspace-file-path.ts:7, ChatTimelineItems.tsx:2386) plus a fourth basename in SessionFileTabs.tsx:9; the [direction:rtl] + <bdi> treatment and its rationale comment are duplicated at ChatTimelineItems.tsx:2520 and :2656; and the +N / −N / 0 badge trio is copy-pasted between the two row branches.
| const root = cwd.replace(/\\/g, "/").replace(/\/$/, ""); | ||
| if (normalized === root) return fileBasename(normalized); | ||
| if (normalized.startsWith(`${root}/`)) { | ||
| const root = commonDirPrefix(normalized, cwd.replace(/\\/g, "/").replace(/\/$/, "")); |
There was a problem hiding this comment.
commonDirPrefix re-collapses the repo qualifier when the agent works inside the repo — this is #5366's symptom returning.
cwd: <workspace>/alpha (agent cd'd into the repo — routine)
candidate: <workspace>/alpha/workspace-test.txt
row: workspace-test.txt (bare, as the daemon stores it)
-> workspace-test.txt (expected alpha/workspace-test.txt)
The deepest shared directory is <workspace>/alpha, so the alpha/ segment gets stripped as though it were worktree root — precisely the qualifier the card is supposed to show. The :125 guard can't rescue it, because a bare row has no / to preserve.
The new tests all pass only because every one of them puts cwd at the workspace root or in a sibling directory. A case with cwd set to ${cwd}/alpha and a bare row would fail today.
Anchoring on the shared prefix is the right instinct for the sibling case, but it needs a floor at the workspace root so it can't eat repo segments — otherwise the more precisely the cwd is recorded, the less qualification survives.
There was a problem hiding this comment.
Dropped the heuristic entirely. commonDirPrefix, workspaceRelativeOpenPath, and turnFileOpenPath are gone; the label now resolves against the real session workspace file list through matchWorkspaceFilePath. There is no shared-prefix stripping left to eat the repo segment, so a cwd inside the repo no longer changes the result.
| * (e.g. two bare `x.txt` rows against candidates in two repos), so the caller can | ||
| * fall back to the row's own path rather than guess. | ||
| */ | ||
| function matchTurnCandidate(relPath: string, hints: TurnPathHints): string | undefined { |
There was a problem hiding this comment.
The ambiguity this gives up on is solvable — the real workspace file list is a couple of components away.
Follow the click path: onOpenFile(openPath) -> handleOpenFile -> openResolvedWorkspaceFile -> matchWorkspaceFilePath(rawPath, data.files) at SessionView.tsx:1201. The click already disambiguates against the actual workspace file list. Only the label is stuck guessing from turn hints — which is why it has to bail out on the two-repo case.
Threading data.files into TurnChangedFiles and deriving the label with matchWorkspaceFilePath would resolve workspace-test.txt -> alpha/workspace-test.txt from real data, fixing both the collision case and the inline comment above, and would let this whole heuristic go away.
Worth noting the duplication is already live: matchTurnCandidate re-implements matchWorkspaceFilePath's exact cascade (normalize -> exact -> endsWith('/'+rel) suffix -> unique-basename -> fall back to input), and the string it produces is then handed to the real one to be re-resolved.
There was a problem hiding this comment.
Done as suggested. TurnChangedFiles now takes the session workspace file list (useSessionWorkspaceFileList, sharing the warm summary query) and derives each label with matchWorkspaceFilePath(file.path, files). workspace-test.txt resolves to alpha/workspace-test.txt from real data, the two-repo collision resolves, and the duplicated cascade in matchTurnCandidate is gone. Label and open target are the same string by construction.
| expect(turnFileOpenPath("beta/workspace-test.txt", hints)).toBe("beta/workspace-test.txt"); | ||
| }); | ||
|
|
||
| it("leaves a bare row unqualified when candidates are genuinely ambiguous", () => { |
There was a problem hiding this comment.
This test satisfies the letter of #5366's acceptance criteria while asserting the opposite outcome.
The issue asks for "one repository-qualified path format" and "a multi-repository test covers duplicate filenames in different repositories." This is that test — and it asserts workspace-test.txt, locking in the unqualified result the issue was filed about.
The reasoning in the comment is sound in isolation: two bare rows against two candidates genuinely are indistinguishable from turn hints alone, and guessing would name the wrong file. But that's an argument for getting better input (see my comment on matchTurnCandidate), not for encoding the gap as expected behavior — a future correct fix now has to delete a passing test, which reads like a regression.
If the bail-out stays, consider renaming it to say it's a known limitation rather than desired behavior, and leaving #5366 open.
There was a problem hiding this comment.
The test now seeds the workspace file list and asserts alpha/workspace-test.txt, so it matches the acceptance criteria instead of locking in the bare basename. No known-limitation bail-out remains for this case.
| if (!candidates?.length) return undefined; | ||
| const matches = candidates.filter((candidate) => { | ||
| const normalized = candidate.replace(/\\/g, "/"); | ||
| return normalized === rel || normalized.endsWith(`/${rel}`); |
There was a problem hiding this comment.
normalized === rel is unreachable.
Candidates only enter byBase behind looksAbsolutePath(file.path) (:64), and both callers reach matchTurnCandidate only after an absolute-path early return (resolveTurnFilePath:73, turnFileOpenPath:119) — so rel is always relative and an absolute candidate can never equal it, including the C:\ and ~ forms.
The equality test is inherited from matchWorkspaceFilePath:25-27, where the file list really is relative and it does real work. Here it just implies a case the control flow forbids. normalized.endsWith(/${rel}) alone is sufficient.
There was a problem hiding this comment.
Removed. matchTurnCandidate is suffix-only now (endsWith("/"+rel)); the comment records why the equality was dead code: candidates are always absolute, and callers reach it only after the absolute-path early return.
| } | ||
| return fileBasename(normalized); | ||
| } | ||
|
|
There was a problem hiding this comment.
The doc comment above overclaims — a fabricated worktree-dir qualifier is still reachable when cwd is present.
The comment promises to "never prefix a segment that is not already in the path" and to fall back to the basename "rather than inventing a qualifier," and the test is named falls back to the basename when cwd is missing. Both are true only for the missing-cwd path. With a cwd pointing at a sibling worktree:
workspaceRelativeOpenPath("<...>/worktrees/demo/demo-1/src/a.ts",
"<...>/worktrees/demo/demo-2")
-> demo-1/src/a.ts
demo-1 is a worktree directory, exactly the fabricated segment the previous revision removed slice(-2) to avoid. Lower-likelihood than the cwd-in-repo case above, but the comment currently reads as a guarantee.
There was a problem hiding this comment.
workspaceRelativeOpenPath is removed, so the fabricated worktree-dir segment can no longer reach the label. resolveTurnFilePath still joins the cwd, but only for the tooltip absolute path (meant to show the full path), not the visible label or the open target.
| <span | ||
| className={cn( | ||
| "min-w-0 truncate text-[11.5px] text-foreground/65 outline-none", | ||
| "min-w-0 truncate text-left text-[11.5px] text-foreground/65 outline-none [direction:rtl]", |
There was a problem hiding this comment.
Unconditional [direction:rtl] changes two surfaces this PR isn't about.
FileLocationLabel has three call sites; :896 and :1310 pass no displayPath, so they still render bare basenames — but they now inherit head-truncation too, flipping their ellipsis from ChatTimeline… to …meItems.tsx.
Head-truncation is the right call for the full paths at :2569, and arguably fine for basenames, but it's an unreviewed visual change to unrelated timeline surfaces. Gating it on displayPath being present (or moving the rtl class to the call site that needs it) keeps the blast radius to this PR's rows.
There was a problem hiding this comment.
Gated. The [direction:rtl] + text-left classes and the bdi wrapper apply only when displayPath is set, so the two basename-only call sites keep their default tail truncation.
The turn changed-files card reconstructed each row's repository-qualified path with a client-side heuristic (commonDirPrefix / workspaceRelativeOpenPath / turnFileOpenPath) over the turn's absolute file_change paths and cwd. That collapsed the qualifier whenever the agent worked inside the repo (cwd at the repo root re-stripped the repo segment) and could fabricate a worktree-dir segment for a sibling cwd, and it gave up entirely on two bare same-named rows. Resolve the label against the real session workspace file list instead, via the same matchWorkspaceFilePath the click handler already uses (SessionView). The label and the file a click opens are now the same string by construction, the two-repo collision resolves from real data, and the heuristic is retired for the label. matchWorkspaceFilePath also learns to map an absolute worktree path onto its repo-relative entry (longest-tail match), so absolute diff rows resolve too. matchTurnCandidate/resolveTurnFilePath stay only for the tooltip's absolute path; matchTurnCandidate drops its unreachable exact-equality branch. RTL head truncation on FileLocationLabel is gated on displayPath so the two basename-only call sites keep their tail-truncated ellipsis.
…ed-change-card # Conflicts: # frontend/src/renderer/components/chat/ChatTimelineItems.tsx
Prasad-D-Ware
left a comment
Prasad-D-Ware
left a comment
There was a problem hiding this comment.
Deep review at 88b011e4.
First, a correction on my own earlier advice. Last round I suggested resolving the label against the live workspace file list via matchWorkspaceFilePath, since the click path already did that. This revision takes that direction, and I now think it was the wrong steer — my suggestion assumed the resolver would refuse to guess when a path is ambiguous, and it does the opposite: files.find(...) returns the first match. Please weigh my comment below accordingly rather than treating the previous one as settled.
The concrete consequence is the top finding: for the exact input in #5366, the card now confidently names the wrong repo where it previously showed an honest bare basename. That is a regression in user-visible correctness, not just a missed fix.
Supporting facts I verified rather than assumed — I simulated matchWorkspaceFilePath from both main and this head, and traced the backend:
ListWorkspaceFilesResponse.Filesis the whole tracked tree (mergeWorkspaceFilePaths, capmaxWorkspaceFiles = 5000), carryingstatus: "unmodified"for untouched files — it is not a changed-files list.- Changed paths are prepended in
sort.Stringsorder, sofiles.find(...)deterministically picks the lexicographically-first repo. - ACP providers emit absolute diff paths (
acp/client.go:965), stored verbatim by the daemon (chat/controller.go:2356).
Two findings I could not anchor inline because the lines fall outside the diff hunks:
-
ChatTimelineItems.tsx:2619—key={${file.status}-${file.oldPath ?? ""}-${file.path}}produces identical React keys for two same-named rows from different repos: the precise case this PR exists to disambiguate. That triggers React's duplicate-key warning and lets the two<li>s share reconciliation state. Since both rows now also render the same label, nothing in the DOM tells them apart. Include the index or the resolved path. -
Altitude.
chat/controller.go:2356writesPath: file.Pathverbatim, discarding the repo qualifier at the one point where the session's workspace layout and the provider cwd are both known. Every defect below — bare-row collapse, unmodified-file matches, absolute-path fallback, label/tooltip divergence, historical relabeling — is a consequence of re-deriving that qualifier in the renderer from an unrelated live query. Qualifying at ingestion, or exposing one server-side resolver that the card, Summary and Files all call, removes the whole class and is what actually satisfies "every workspace surface should show the same repository-qualified path."
My honest read: the client-side reconstruction has now been tried twice, and each attempt trades one wrong path for another because the qualifier simply is not present in the input. I'd stop iterating here and move the fix to ingestion.
| // Resolve the label against the real workspace file list, the same | ||
| // list the click handler matches, so the row names exactly the file | ||
| // it opens. Before the list loads this returns the raw row path. | ||
| const openPath = matchWorkspaceFilePath(file.path, workspaceFiles); |
There was a problem hiding this comment.
The #5366 scenario is still unfixed, and the card now mislabels rather than under-labels.
Multi-repo workspace, alpha and beta each change workspace-test.txt. The daemon stores the provider path verbatim, so both diff.files[].path are the bare workspace-test.txt — the two rows are byte-identical input.
Inside matchWorkspaceFilePath the exact and inputTail passes both miss, so resolution falls to files.find(f => f.path.endsWith('/' + normalized)) — first match wins. Changed paths are prepended in sort.Strings order, so alpha always wins:
row 1 (alpha) -> alpha/workspace-test.txt
row 2 (beta) -> alpha/workspace-test.txt <-- wrong repo
Both aria-labels read "Open alpha/workspace-test.txt in Files", and clicking beta's row opens alpha's file. Simulated and confirmed against this head.
This is worse than the previous revision, which showed a bare basename: unhelpful, but not a false statement about which repo changed. A reviewer trusting this card would review the wrong diff.
The new "keeps two same-named files in different repos distinct" test passes only because it pre-qualifies its row paths (alpha/..., beta/...) — the reported input has no qualifier to begin with, so the test never reaches the branch that fails. #5366's acceptance criterion "a multi-repository test covers duplicate filenames in different repositories" is still unmet.
A filter + single-match check (as matchTurnCandidate already does) would at least keep the honest bare fallback instead of guessing.
| ...sessionWorkspaceFilesQueryOptions(sessionId ?? ""), | ||
| enabled: Boolean(sessionId), | ||
| refetchInterval: false, | ||
| select: (data: WorkspaceFilesResponse) => data.files, |
There was a problem hiding this comment.
select: (data) => data.files hands back the entire tracked tree, not the turn's changed files.
The daemon builds files from mergeWorkspaceFilePaths(git ls-files, changedPaths) and stamps WorkspaceFileUnmodified on everything outside the change set (workspace_files.go:902-911, asserted by service_test.go:999), up to maxWorkspaceFiles = 5000. The sibling hook immediately above filters with isChangedWorkspaceFile for exactly this reason; this one doesn't.
So a row can resolve onto a file the turn never touched: a turn that edits /tmp/scratch/config.yaml (outside the worktree) against a repo whose only tracked config.yaml is an unmodified deploy/config.yaml renders the row as deploy/config.yaml and opens that untouched file.
Filtering to changed files would also shrink the list the resolver scans, which helps the perf issue on workspace-file-path.ts.
| // leading segments. Prefer the longest matching entry so a deeper repo-qualified | ||
| // path wins over a bare basename. The longest tail of a fixed string is unique, | ||
| // so this never has to guess between two same-length candidates. | ||
| const inputTail = files |
There was a problem hiding this comment.
The new inputTail branch silently changes behavior for every existing caller of this shared resolver.
Simulated PR vs main:
matchWorkspaceFilePath('packages/web/README.md', [README.md, docs/README.md])
main: packages/web/README.md PR: README.md
matchWorkspaceFilePath('/Users/me/.claude/app/settings.json', [settings.json, app/settings.json])
main: <input unchanged> PR: app/settings.json
This function is the single resolver behind SessionView.revealResolvedWorkspaceFile (SessionView.tsx:1644), used by handleOpenReviewFile and handleOpenFile. So clicking a review finding on a file outside the worktree now silently opens an unrelated workspace file instead of failing visibly. That blast radius is well outside this PR's stated scope and no test covers it.
Separately, this function now runs per row per render inside visible.map with no useMemo: up to four full passes over files plus a filter(...).sort(...) (two allocations), against a list that can hold 5000 entries. A transcript with 50 cards x 4 rows is ~1M string comparisons and 200 allocations on every chat re-render — new blocking work in a tree that just received a perf pass (#4719). A useMemo'd Map keyed by basename/tail, built once per list identity, would fix both.
| /** | ||
| * Session whose workspace file list resolves each row's repository-qualified | ||
| * open path. This is the same list the click handler matches against, so the | ||
| * label and the file it opens are the same string by construction. |
There was a problem hiding this comment.
This documented invariant doesn't hold.
"the label and the file it opens are the same string by construction" — but onOpenFile(openPath) reaches SessionView.revealResolvedWorkspaceFile, which does await fetchWorkspaceFiles() and then a second matchWorkspaceFilePath against the freshly fetched list (SessionView.tsx:1644).
If the tree changed between render and click — likely while an agent is still running — a row labeled alpha/x.txt can miss exact/inputTail/suffix against the new list, fall through to the byBase single-match branch, and open beta/x.txt. Same string by construction only holds if both resolutions see the same list, and they explicitly don't.
Relatedly, the label and the tooltip are now produced by two different resolvers: label = matchWorkspaceFilePath(file.path, workspaceFiles), tooltip = shortenPaths(resolveTurnFilePath(file.path, pathHints)). For the multi-repo bare row, matchTurnCandidate returns undefined (your own turn-file-open-path.test.ts asserts it "does not guess") and resolveTurnFilePath joins the cwd — so the row reads alpha/workspace-test.txt while hovering it shows <worktree>/workspace-test.txt, a path that exists nowhere on disk. That is the opposite of the issue's "one repository-qualified path format."
| }) { | ||
| const [expanded, setExpanded] = useState(false); | ||
| const pathHints = useMemo(() => turnPathHints(items), [items]); | ||
| const workspaceFiles = useSessionWorkspaceFileList(sessionId); |
There was a problem hiding this comment.
Rendering chat now opens the daemon file-watcher SSE for the whole session, including sessions that deliberately opt out.
Every mounted TurnChangedFiles adds a subscribeWorkspaceFileChanges ref, so the stream stays open as long as any turn card is rendered, rather than only while a Files view is mounted.
SessionInspector.tsx:197 passes browserOnly ? undefined : session?.id specifically to keep this query off for browser-only sessions. This call site passes sessionId unconditionally, re-enabling both the fetch and the watcher for exactly those sessions.
Separately: a turn's changed-files card is a historical record, but it's now relabeled against live state. Edit notes.txt in turn 1, later delete it and create docs/notes.txt, and the settled turn-1 card retroactively claims it edited docs/notes.txt. Labels also visibly flip from raw to resolved when the query lands, and churn while a live turn writes files, since the subscription invalidates every 150ms.
| // and open the repository-qualified path (`alpha/workspace-test.txt`) resolved | ||
| // against real workspace data instead of guessing from turn hints. Returns an | ||
| // empty list until the query has data, so callers fall back to the raw row path. | ||
| export function useSessionWorkspaceFileList( |
There was a problem hiding this comment.
No test coverage, and the only consumer mocks it away.
TurnChangedFiles.test.tsx:13-19 replaces the whole module's useSessionWorkspaceFileList with a vi.fn(), so nothing exercises enabled: Boolean(sessionId), the select, the EMPTY_WORKSPACE_FILES fallback, or the subscription's effect cleanup. Dropping the ?? EMPTY_WORKSPACE_FILES would feed undefined into matchWorkspaceFilePath and throw on files.find — and the entire suite would still pass.
This is also a near-verbatim copy of useSessionWorkspaceFilesChangedCount directly above: same query options, same enabled, same refetchInterval: false, same subscribeWorkspaceFileChanges effect, differing only in select. Per AGENTS.md ("Add helpers only when they remove duplication across real call sites"), one useSessionWorkspaceFilesSelect(sessionId, select) with the changed count derived from the list would collapse both.
| // Head-truncation only helps a full `displayPath`, whose filename is at the tail. | ||
| // The basename-only call sites pass no `displayPath`, so leave their default | ||
| // tail-truncation alone rather than flip their ellipsis to the head. | ||
| const label = displayPath ?? fileBasename(path); |
There was a problem hiding this comment.
displayPath mixes nullish-coalescing and truthiness across three adjacent lines.
const label = displayPath ?? fileBasename(path) keeps "", while the two uses below test displayPath ? ... and take the false branch. matchWorkspaceFilePath returns rawPath unchanged when normalizeWorkspacePath yields an empty string, so a diff row with an empty or "./" path renders a blank, unlabeled row instead of falling back to the basename.
Use the same truthiness test in all three places.
| return; | ||
| } | ||
| if (byBase.get(base) !== absolutePath) byBase.set(base, undefined); | ||
| if (!candidates.includes(absolutePath)) candidates.push(absolutePath); |
There was a problem hiding this comment.
Minor: candidates.includes(absolutePath) makes insertion O(n), so turnPathHints is quadratic in the candidate count for a basename. A long turn that repeatedly edits same-named files across repos pays that on every hint collection, and the arrays stay memoized for the whole transcript.
A Set<string> per basename keeps identical dedup semantics at O(1), materialized to an array only at match time.
3 participants
What
Each row in a turn's changed-files card (
TurnChangedFiles) showed only the file basename. In a workspace with more than one repository, two files with the same name in different repos (workspace-test.txtin bothalphaandbeta) both render as the same bare filename, and the row's on-screen label no longer matches the repository-qualified path the Files inspector opens when you click it.This shows the same workspace-relative path the row already opens:
openPath, computed byturnFileOpenPath(file.path, pathHints)and already passed toonOpenFileand theOpen ... in Filesaria-label. Both render branches (the clickable row and the read-only row) now displayopenPathinstead offileBasename(file.path).Fixes #5366.
Why this is the right seam
The daemon stores the provider's repo-relative path verbatim (
backend/internal/service/chat/controller.go,Path: file.Path), so the diff row'sfile.pathcarries no repository qualifier. That qualifier is reconstructed on the client from the turn's absolutefile_changepaths and command cwd (turnFileOpenPath/turnPathHints), which is exactly the value the row already opens. DisplayingopenPathmakes the label and the click target the same string, so a row can never name one file and open another. When there are no hints (single repo, no reconstruction possible),openPathis the unchanged repo-relative path, so single-repo rows are unaffected.Tests
frontend/src/renderer/components/chat/TurnChangedFiles.test.tsx:file.path(workspace-test.txt) plus afile_changeactivity carrying the absolute path and a command cwd rendersalpha/workspace-test.txt, and the bare basename is absent. This fails if the row shows rawfile.path.onOpenFileargument are bothalpha/workspace-test.txt.Ran locally:
npx vitest run src/renderer/components/chat/TurnChangedFiles.test.tsx(32 passed),npm run typecheck,npm run typecheck:e2e.