feat(tui): FILES sidebar panel with click-to-select and file drill-in - #365
Merged
Conversation
Add a FILES section to the right context sidebar showing the files the session has touched — newest first, with A/M status badges, aggregated +/− diffstats, and a pulsing row for the file being written right now. Clicking a row selects the file (accent marker, edit cards tint in the chat, transcript scrolls to the latest edit); a second click opens a drill-in view: the chat column swaps to the file's stacked edit cards, `f` toggles to the syntax-highlighted on-disk file with markers on session-added lines, `d` back to diff, Esc returns to the chat. The roster is fed from two sources: changedFiles carried by write_file/edit_file/apply_patch results (persisted per session, so it survives /resume), and a git sweep — baseline snapshot at startup, re-check after command tools and at turn end — so files created through bash/exec_command scaffolding or subagents show up too.
Contributor
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
WalkthroughThis PR adds a FILES sidebar backed by transcript and git-derived touched-file tracking, plus a drill-in file view with diff and full-file modes. It also wires selection hover, tinting, scroll handling, and key/navigation behavior into the TUI. ChangesFILES Sidebar and File Drill-in Feature
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant TranscriptSelection
participant Model
participant FileView
User->>TranscriptSelection: click FILES row
TranscriptSelection->>Model: selectFile(path) / openFileView(path)
Model->>FileView: openFileView(path)
FileView->>FileView: render diff or full body
FileView-->>Model: fileViewBodyItems(width)
Model-->>User: show file nav bar and body
sequenceDiagram
participant Model
participant GitSweepCmd
participant Git
participant FILESSidebar
Model->>GitSweepCmd: baseline sweep on Init
GitSweepCmd->>Git: git status --porcelain
Git-->>GitSweepCmd: dirty paths
GitSweepCmd-->>Model: gitSweepMsg(baseline)
Model->>Model: handleGitSweepMsg()
Model->>GitSweepCmd: maybeGitSweep() on turn completion
GitSweepCmd->>Git: git status + git diff --numstat
Git-->>GitSweepCmd: dirty paths + diffstat
GitSweepCmd-->>Model: gitSweepMsg(live)
Model->>FILESSidebar: refresh touched files
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (3)
internal/tui/file_view.go (1)
189-201: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExact trimmed-text matching will over-mark common/duplicate lines.
changed[strings.TrimSpace(lines[i])](built infileViewChangedLines, Line 219) matches purely by line content. Any on-disk line whose trimmed text happens to equal an added line — very common for short lines like},}(),return nil, blank-brace lines, etc. — gets the accent gutter marker even though it was never touched. The doc comment at Line 159-160 frames this as "a stale marker just doesn't highlight," but the more common failure mode is the opposite: marking unrelated, unchanged occurrences of a common short line throughout the file.A cheap mitigation: skip very short/low-signal lines (e.g. length < 4-5 chars, or lines consisting only of punctuation) when populating
fileViewChangedLines, since those are the lines most likely to collide.🤖 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 `@internal/tui/file_view.go` around lines 189 - 201, The gutter marker logic in fileViewChangedLines/file_view.go is over-matching duplicate short lines because it keys only on strings.TrimSpace(lines[i]). Update the changed-line tracking to ignore very short or low-signal lines when building the changed set, so common tokens like braces, blank lines, and simple returns do not get marked across unrelated occurrences. Keep the fix localized to fileViewChangedLines and the marker check used in the display loop.internal/tui/files_git_sweep_test.go (1)
173-201: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStatic analysis: nil
context.Contextpassed togitSweepCmd.golangci-lint flags
gitSweepCmd(nil, dir, true)(Line 173) under SA1012; the same pattern recurs at Lines 182 and 198.gitSweepCmddoes guard againstnilinternally, but passingcontext.TODO()here is idiomatic and silences the linter without changing behavior.🔧 Suggested fix
- baseline := gitSweepCmd(nil, dir, true)().(gitSweepMsg) + baseline := gitSweepCmd(context.TODO(), dir, true)().(gitSweepMsg) ... - sweep := gitSweepCmd(nil, dir, false)().(gitSweepMsg) + sweep := gitSweepCmd(context.TODO(), dir, false)().(gitSweepMsg) ... - if msg := gitSweepCmd(nil, t.TempDir(), false)().(gitSweepMsg); msg.ok { + if msg := gitSweepCmd(context.TODO(), t.TempDir(), false)().(gitSweepMsg); msg.ok {(requires adding
"context"to imports)🤖 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 `@internal/tui/files_git_sweep_test.go` around lines 173 - 201, The test is passing a nil context into gitSweepCmd, which triggers SA1012 in the calls used in files_git_sweep_test.go. Update the test to use a non-nil context such as context.TODO() for each gitSweepCmd invocation, and add the context import so the existing gitSweepMsg assertions and gitSweepCmd behavior remain unchanged.Source: Linters/SAST tools
internal/tui/files_git_sweep.go (1)
148-156: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffTransient timeout permanently disables the sweep, same as a real "not a repo" failure.
A hung/slow
git(hittinggitSweepTimeout) and a genuine non-git workspace both setgitSweepUnavailable = truewith no retry. For the timeout case this is a recoverable condition that will now never resurface for the rest of the session. Worth distinguishing "no git here" (permanent) from "git call failed/timed out" (retryable) so a momentarily slow filesystem/lock doesn't permanently blind the FILES sidebar to shell-created files for the session.🤖 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 `@internal/tui/files_git_sweep.go` around lines 148 - 156, The timeout path in handleGitSweepMsg currently marks gitSweepUnavailable true for every failed sweep, which makes a transient gitSweepTimeout behave like a permanent non-repo failure. Update the git sweep handling in model.handleGitSweepMsg (and any related gitSweepMsg fields/creation sites) to distinguish a real “not a repo” result from a timeout or other retryable git failure, so only the permanent case sets gitSweepUnavailable while timeout/temporary failures leave it eligible to retry later.
🤖 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 `@internal/tui/file_view.go`:
- Around line 161-176: The full-file path in renderFileViewFull still reads the
entire file into memory before applying fileViewMaxLines, which defeats the
render freeze safeguard. Change renderFileViewFull to bound the read up front by
using a streaming or limited-read approach, then stop once fileViewMaxLines
lines are collected before any full strings.Split-like processing; keep the
existing target resolution and error handling intact.
In `@internal/tui/files_git_sweep_test.go`:
- Around line 37-45: The TestParseGitNumstat case is using a fabricated rename
record and not covering the real numstat rename format, so update it to use an
authentic git --numstat rename entry handled by parseGitNumstat and assert the
expected map key for the renamed file. Keep the existing checks for normal and
binary entries, but replace the bogus "R\told.go -> pkg/new.go" line and add an
assertion that the "old.go => pkg/new.go" path is parsed into the correct
destination entry.
In `@internal/tui/files_git_sweep.go`:
- Around line 198-217: The git-only entries returned by gitTouchedFiles are not
ordered by recency, so older sweep results can appear before newer ones when
merged into the FILES tail. Update gitTouchedFiles in model to sort m.gitTouched
newest-first by created before building touchedFile entries, then keep the
existing merge flow with touchedFiles so the combined list stays consistently
ordered.
- Around line 104-142: parseGitNumstat is still relying on cutRename, which only
handles porcelain-style “old -> new” paths and will miss git --numstat rename
output like brace-mangled paths, so renamed files never get matched. Update the
git diff collection in files_git_sweep.go to use --numstat -z and parse the
NUL-delimited rename format, or add rename-aware parsing inside parseGitNumstat
that understands brace-mangled numstat paths. Keep unquoteGitPath and the
files[i].path matching logic aligned with the new parsing so renamed entries
resolve correctly.
In `@internal/tui/files_panel.go`:
- Around line 153-159: The overflow count in the files sidebar is using the full
files slice length even though the live row is skipped in the loop, so the “+N
more” label can be inflated. Update the logic in the files panel rendering flow
around the loop that builds the sidebar rows in the code path using files, live,
shown, and maxSidebarFiles so the count excludes the live entry when f.path
matches live. Adjust the remaining-count calculation to use only the non-live
items that could still be shown, keeping the displayed overflow text consistent
with what is actually rendered.
- Around line 52-68: The file summary aggregation in files_panel.go is using
planDiffStat(row.detail) for every entry in row.changedFiles, so each touched
file gets the full patch totals instead of its own stats. Update the per-file
accumulation logic in the touchedFile/index loop to derive adds and dels for
each individual path before adding them to files[at], using the existing
row.changedFiles handling and related symbols like planDiffStat, touchedFile,
and resultRowCreatedFile as the place to split by path.
- Around line 74-78: The ordering logic in files_panel.go currently reverses
first-seen order, which does not keep the most recently touched file first when
a file is revisited. Update the sorting in the files panel logic around the
files slice to order entries by lastRowIndex descending instead of reversing the
slice, so the newest touch always appears first. Use the existing file item
structure and the code path that builds files in the files panel to locate the
change.
In `@internal/tui/model.go`:
- Around line 1083-1089: The file-view shortcut handling in the main key
dispatch is intercepting modal keys before the permission/spec/MCP/picker
branches. Update the key handling around m.fileView.active, composerValue(), and
keyText(msg) so the d/f and Esc shortcuts only apply when no blocking modal is
open, matching the same modal-state checks used by the permission and picker
cases. Make the same guard change in both affected key-handling branches so
modal deny/Esc behavior is not hijacked by file drill-in mode.
In `@internal/tui/sidebar.go`:
- Around line 100-102: sidebarHasContent currently only checks touchedFiles(),
so it can miss an in-progress live edit and suppress the FILES pulse. Update
sidebarHasContent in sidebar.go to also treat a non-empty liveEditingPath() as
content, alongside the existing touchedFiles() check, so the sidebar shows
content as soon as the first live write starts.
In `@internal/tui/transcript_selection.go`:
- Around line 1271-1281: The FILES click handling in transcript_selection.go
reopens the currently active file and silently resets its view state. Update the
click branch around m.fileRowAtMouse so that when m.fileView.active is true and
the clicked path matches m.fileView.path, it does not call openFileView(path);
instead preserve the current fileView mode and chat scroll state, only switching
views when the clicked file is different. Keep the behavior for a different file
path by still using openFileView, and add coverage for re-clicking the same open
file after switching to full mode.
---
Nitpick comments:
In `@internal/tui/file_view.go`:
- Around line 189-201: The gutter marker logic in
fileViewChangedLines/file_view.go is over-matching duplicate short lines because
it keys only on strings.TrimSpace(lines[i]). Update the changed-line tracking to
ignore very short or low-signal lines when building the changed set, so common
tokens like braces, blank lines, and simple returns do not get marked across
unrelated occurrences. Keep the fix localized to fileViewChangedLines and the
marker check used in the display loop.
In `@internal/tui/files_git_sweep_test.go`:
- Around line 173-201: The test is passing a nil context into gitSweepCmd, which
triggers SA1012 in the calls used in files_git_sweep_test.go. Update the test to
use a non-nil context such as context.TODO() for each gitSweepCmd invocation,
and add the context import so the existing gitSweepMsg assertions and
gitSweepCmd behavior remain unchanged.
In `@internal/tui/files_git_sweep.go`:
- Around line 148-156: The timeout path in handleGitSweepMsg currently marks
gitSweepUnavailable true for every failed sweep, which makes a transient
gitSweepTimeout behave like a permanent non-repo failure. Update the git sweep
handling in model.handleGitSweepMsg (and any related gitSweepMsg fields/creation
sites) to distinguish a real “not a repo” result from a timeout or other
retryable git failure, so only the permanent case sets gitSweepUnavailable while
timeout/temporary failures leave it eligible to retry later.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c778130c-683a-4a39-a5fb-0bbce3bcb231
📒 Files selected for processing (14)
internal/tui/file_view.gointernal/tui/file_view_test.gointernal/tui/files_git_sweep.gointernal/tui/files_git_sweep_test.gointernal/tui/files_panel.gointernal/tui/files_panel_test.gointernal/tui/hover.gointernal/tui/model.gointernal/tui/render_cache.gointernal/tui/rendering.gointernal/tui/session.gointernal/tui/sidebar.gointernal/tui/transcript.gointernal/tui/transcript_selection.go
Review fixes for the FILES panel / drill-in / git sweep:
- touchedFiles: split multi-file diffs per file (perFileDiffStats) so an
apply_patch spanning several files no longer charges the whole patch's
+/- totals to every file it touched; order by lastRowIndex descending
so a re-touched file lists first (reversing first-seen order kept it in
its original slot).
- sidebarFileLines: filter the live-writing row out before computing the
"+N more" trailer so the skipped entry no longer inflates the count.
- git sweep: run `git diff --numstat -z` and parse the NUL-terminated
rename records (counts + preimage + postimage) — plain --numstat
brace-mangles renamed paths ("src/{old => new}"), so their diffstat
never matched a porcelain path and renamed files stayed at 0/0;
gitTouchedFiles now yields newest detection first to match the
roster's ordering.
- file view: stream the full-mode read and stop at fileViewMaxLines
(os.ReadFile loaded a multi-GB file wholesale before truncating — the
exact freeze the cap exists to prevent); re-opening the file already
being viewed is a no-op so a stray re-click can't bounce full mode
back to diff or reset scroll; skip sub-4-char lines in the changed-line
markers so common braces/returns don't mark unrelated occurrences.
- keys: gate the drill-in's d/f and Esc handling on noBlockingModal so a
permission prompt / ask-user / wizard keeps its own key handling.
- sidebar: a live in-flight write counts as content, so the FILES pulse
shows for the session's first mutation.
- tests: numstat test now feeds real -z record shapes and asserts the
rename postimage key; new coverage for per-file stats, re-touch
ordering, live-row overflow, same-file reopen, truncation, modal
gating, live-write content, and a real-repo rename sweep.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/tui/file_view_test.go (1)
267-288: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest only exercises
f, notd, despite the doc comment claiming both.The comment says "with a permission prompt up, Esc and the d/f shortcuts belong to the prompt — the drill-in must not swallow them", but only
fis sent toUpdate. If a regression only affects thedbranch (e.g., separatecasehandlingdbefore the modal-guard check), this test won't catch it.🧪 Add the missing `d` case
updated, _ := m.Update(tea.KeyPressMsg{Code: 'f', Text: "f"}) m = updated.(model) if m.fileView.mode != fileViewDiff { t.Fatal("f with a permission prompt up must not switch file-view modes") } + m = m.setFileViewMode(fileViewFull) + updated, _ = m.Update(tea.KeyPressMsg{Code: 'd', Text: "d"}) + m = updated.(model) + if m.fileView.mode != fileViewFull { + t.Fatal("d with a permission prompt up must not switch file-view modes") + } updated, _ = m.Update(tea.KeyPressMsg{Code: tea.KeyEsc})🤖 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 `@internal/tui/file_view_test.go` around lines 267 - 288, The permission-prompt key-handling test only covers the f shortcut, so it can miss regressions in the d path even though the doc comment says both should be blocked by the modal. Update TestFileViewKeysDeferToBlockingModal in file_view_test.go to also send a d keypress through m.Update and assert it does not change fileView.mode while pendingPermission is set, alongside the existing Esc check, so the guard behavior is verified for both drill-in shortcuts.
🤖 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.
Nitpick comments:
In `@internal/tui/file_view_test.go`:
- Around line 267-288: The permission-prompt key-handling test only covers the f
shortcut, so it can miss regressions in the d path even though the doc comment
says both should be blocked by the modal. Update
TestFileViewKeysDeferToBlockingModal in file_view_test.go to also send a d
keypress through m.Update and assert it does not change fileView.mode while
pendingPermission is set, alongside the existing Esc check, so the guard
behavior is verified for both drill-in shortcuts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 43f6ac84-5c4d-41fd-86b9-7c309264065a
📒 Files selected for processing (8)
internal/tui/file_view.gointernal/tui/file_view_test.gointernal/tui/files_git_sweep.gointernal/tui/files_git_sweep_test.gointernal/tui/files_panel.gointernal/tui/files_panel_test.gointernal/tui/model.gointernal/tui/sidebar.go
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/tui/sidebar.go
- internal/tui/files_git_sweep_test.go
- internal/tui/file_view.go
- internal/tui/model.go
- internal/tui/files_git_sweep.go
- internal/tui/files_panel.go
Collaborator
|
@CodeRabbit review |
✅ Action performedReview finished.
|
Collaborator
|
@coderabbitai approve |
✅ Action performedComments resolved and changes approved. |
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
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a FILES section to the TUI's right context sidebar (between PLAN and ACTIVITY) showing the workspace files the session has touched — newest first, with an A/M/✗ status badge, an aggregated +added −removed diffstat per file, a pulsing row for the file whose write is streaming right now, and a +N more overflow trailer.
Why: while an agent works, the question users actually have is "what is it changing right now, and what has it changed so far?" Today the transcript only answers that by scrolling back through tool cards; this panel answers it at a glance and gives every file a direct path to its diffs.
Interaction
Data sources
Implementation notes
Checklist
Summary by CodeRabbit