feat(tui)!: replace /retitle with local /rename - #826
Conversation
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. |
WalkthroughThe TUI replaces ChangesSession title flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant TUIModel
participant SessionStore
participant AutoTitle
User->>TUIModel: Enter /rename
TUIModel->>SessionStore: Persist manual title
SessionStore-->>TUIModel: Updated metadata
AutoTitle->>SessionStore: UpdateTitleIfCurrent(original title, generated title)
SessionStore-->>AutoTitle: applied=false when manual title changed
AutoTitle-->>TUIModel: Report title result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/tui/session_title_test.go (1)
275-312: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the missing branch assertion: stale auto-title must keep the retry gate set.
handleSessionTitleGeneratedhas three outcomes (apply / stale-skip-keep-gate / failure-release-gate); this test proves the store-level CAS outcome but doesn't driveresultthroughhandleSessionTitleGeneratedto confirmtitledSessions[session.SessionID]staystrueafter a stale (applied:false, err:nil) result — the one combination not covered byTestAutoTitleFailureReleasesRetryGate.got, err := store.Get(session.SessionID) if err != nil || got == nil { t.Fatalf("get session: %v", err) } if got.Title != "Manual Name" { t.Fatalf("late automatic title overwrote manual name: %q", got.Title) } + + next, _ := m.handleSessionTitleGenerated(result) + if !next.titledSessions[session.SessionID] { + t.Fatal("a stale auto-title result must not release the retry gate (manual rename already won)") + } }🤖 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/session_title_test.go` around lines 275 - 312, Extend TestManualRenameWinsOverInFlightAutoTitle to pass the stale successful result through handleSessionTitleGenerated after execCmd, then assert titledSessions[session.SessionID] remains true. Preserve the existing store-level assertions and verify the applied:false, err:nil branch keeps the retry gate set.
🤖 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/model_test.go`:
- Around line 942-949: Update TestSessionPickerLabelAlignsTitles to assert that
todayColumn and olderColumn are non-negative before comparing them, so missing
titles fail the test rather than matching at -1; retain the existing alignment
assertion for present titles.
In `@internal/tui/session_rename.go`:
- Around line 46-53: Update renameActiveSession to truncate the trimmed title to
tuiSessionTitleLimit before validating and storing it, preserving the existing
empty-title and unavailable-storage handling.
---
Nitpick comments:
In `@internal/tui/session_title_test.go`:
- Around line 275-312: Extend TestManualRenameWinsOverInFlightAutoTitle to pass
the stale successful result through handleSessionTitleGenerated after execCmd,
then assert titledSessions[session.SessionID] remains true. Preserve the
existing store-level assertions and verify the applied:false, err:nil branch
keeps the retry gate set.
🪄 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 Plus
Run ID: fab9a4d9-a0fa-42a8-a3e4-3632c1506cfc
📒 Files selected for processing (16)
internal/sessions/session_title_test.gointernal/sessions/store.gointernal/tui/btw.gointernal/tui/clipboard.gointernal/tui/commands.gointernal/tui/composer.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/session.gointernal/tui/session_rename.gointernal/tui/session_rename_test.gointernal/tui/session_title.gointernal/tui/session_title_test.gointernal/tui/sidebar.gointernal/tui/transcript_selection.gointernal/tui/view.go
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Verdict
Changes requested — for one thing that is not a code change. The engineering here is sound and I could not break it; the problem is that as titled, this PR will silently drop a user-facing breaking change from the changelog.
Reviewed at 36f16b7, base 097c265 (level with origin/main, zero commits behind). Author has write access, so this is the internal cycle rather than the issue-approved gate.
What I verified
-
The compare-and-swap actually does the work it claims. I gutted it — removed the
session.Title != strings.TrimSpace(expected)guard atinternal/sessions/store.go:775so a late automatic title always overwrites — and two independent tests failed, one at each layer:--- FAIL: TestUpdateTitleIfCurrent stale update should preserve manual title: Title:"Late generated title" ... true --- FAIL: TestManualRenameWinsOverInFlightAutoTitle late automatic title should be skipped, got {title:"Generated Automatic Title", applied:true} -
The pre-first-prompt path is guarded, and the guard is tested. Removing the
m.titledSessions[session.SessionID] = trueblock atinternal/tui/session.go:48-53failsTestRenameNamesFreshSessionBeforeFirstPrompt("a manually named fresh session must skip automatic naming"). Belt and braces here —sessionTitleIsAutoatsession_title.go:213would also refuse a manual title — but the explicit gate is the one under test. -
"Hidden but searchable" holds. Dropping
Meta: meta.SessionIDfrom the picker item does not cost searchability:scorePickerItembuilds its haystack fromGroup, Label, Value, Meta(internal/tui/picker.go:168) andValueis still the session ID. -
The
n / totalfooter is consistent with the filtered view.applyPickerFilterreplacesp.itemswholesale (picker.go:158) and resetsselectedto 0, so the counter and the visible list always agree. I went looking for an off-by-one against an unfiltered total and there isn't one. -
/retitleis fully excised — no leftoverretitleQueue/retitleActive/retitleTotal/retitleDone/retitleOKfields, no stale docs. The only surviving reference is the negative test atsession_rename_test.go:182. -
Gauntlet, darwin/arm64, non-
/tmpcheckout:make fmt-checkclean,go vet ./...clean,go test ./...fully green,go test -race ./internal/tui/ ./internal/sessions/ -count=3green,go run ./cmd/zero-release buildandsmokeboth pass,git diff HEAD --checkclean. -
Passes executed: 0–5, 7–12. Pass 6 was run narrowly rather than in full — this diff has no auth, exec, parsing or network surface; I checked the two boundaries it does touch (the new store write path and the paste route) and nothing else applied.
Blocking
-
The PR title is not a conventional commit, and this repo derives its changelog from the squash subject. — PR title /
release-please-config.jsonThe chain, each hop checked:
release-please-config.jsonand.github/workflows/release-please.ymlmean versioning andCHANGELOG.mdare generated from conventional-commit subjects.gh api repos/Gitlawb/zeroreportssquash_merge_commit_title=COMMIT_OR_PR_TITLE. This PR has two commits (daee6c15,36f16b74), so the squash subject will be the PR title, not a commit subject.- That title is
Improve session naming and resume picker— no type, no scope. release-please parses no type, so the change contributes no changelog entry and no version bump. - Every recently merged PR does follow the convention:
fix(sandbox):,test(tools):,fix(config):.
That would be a minor annoyance on an ordinary change. It is not one here, because this PR removes
/retitle, which shipped in #224 and has been present in every release from v0.3.0 through v0.6.0. A command that existed in the last release disappears, and under the current title the changelog will not say so.Retitle to something like
feat(tui)!: replace /retitle with a local /renameand add aBREAKING CHANGE: /retitle is removed; use /renamefooter. No code change required — this is the whole blocker.
Non-blocking
&& msg.appliedcurrently has no observable effect. —internal/tui/session_title.go:255. I mutated it out and the entireinternal/tuisuite stayed green, so I traced why rather than reporting it as untested-and-therefore-broken. When the store rejects the update,UpdateTitleIfCurrentreturns the freshly-read metadata (store.go:777), somsg.titleis already the manual title — assigning it tom.activeSession.Titleis a no-op, and thetitledSessionsgate is retained on both branches either way. The flag is correct defensive code that becomes load-bearing the momentUpdateTitleIfCurrentreturns anything else on rejection. Worth a one-line comment saying that, so a future reader does not delete it as dead.- Four separable changes in one PR. — diffstat. The
/retitle→/renameswap, the rename editor, the pre-prompt naming plus compare-and-swap, and the/resumepicker relayout (session.go:406-436,view.go:816-825) are independent; the picker work does not depend on any of the rename work. Team PR through the internal cycle, so I am not treating this as a scope close — but the picker change is the one a bisect would most want isolated. - Merge-conflict exposure with two long-lived branches. #643 and #570 both touch
internal/tui/session.go,commands.goandmodel.go. Neither is close to merging, and this branch is current, so the right outcome is probably that they rebase onto this — worth saying so on those PRs rather than holding this one. /renameis missing from the README command table. —README.md:214./resume,/rewindare listed as the session commands;/retitlenever was, so nothing is stale, but a new user-facing command belongs on that line.- Blank submit closes the editor instead of holding it open. —
internal/tui/session_rename.go:32-37.m.renamePrompt = nilis set before validation, so Enter on whitespace closes the editor and prints an error.TestRenameEditorRejectsBlankAndEscCancels:157asserts this deliberately, so I read it as intended rather than an oversight — flagging only because the user loses their editing context to recover from a typo.
Questions for the author
/retitlerefused to run mid-turn (Cannot retitle sessions while a run is active);/renamehas nom.pendingguard. I believe that is correct — the old guard existed because/retitlemade provider calls and/renameis pure local metadata, and the in-flight case is exactly what the compare-and-swap covers. Confirming that was deliberate rather than dropped with the surrounding code.
Not covered by this PR
- A migration path for
/retitle. It now resolves to nothing, so a user who typed it last week gets a generic unknown-command error. A one-release alias that prints "renamed to/rename" would be kinder than a changelog line alone, though the changelog line is the part I am blocking on. - Bulk retitling of old sessions is simply gone as a capability. That looks intentional given the "no provider call, no token usage" rationale, but nothing in the PR body says the bulk case was considered and dropped rather than overlooked.
Not verified
- Windows and Linux — everything above is darwin/arm64 only. The diff is platform-neutral TUI and store code, and
internal/sessionslocking is unchanged by this PR, so I did not expect divergence and did not look for it. - The manual TUI walkthrough listed in the PR description. I exercised the rename paths through the model's
Updateloop and the store, not through a real terminal, so the editor's rendering and the picker's new column alignment are unconfirmed by eye. - Cross-process races.
UpdateTitleIfCurrenttakes the same per-session lock asUpdateTitleandAppendEvent, and I read that as correct, but I did not run twozeroprocesses against one session to prove it.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approving, with two small things you can take or leave.
renameActiveSession (session_rename.go:47) does TrimSpace then cutRunes, which strips the ends but keeps interior newlines. /rename foo<newline>bar persists a multi-line title into metadata, and the pickers and listings all assume one line. The other title paths collapse whitespace, so this one is the odd one out.
The empty-paste guard in routePaste (clipboard.go:79-81) looks unreachable: the right-click path already returns earlier, so nothing gets there with an empty payload. The clipboard change is also the one part of this PR with no test.
Neither is worth holding it for. The store change is backward compatible, sessions without the new fields still load, and I could not get the picker to point at a deleted session.
anandh8x
changed the title
|
@gnanam1990 Updated the PR title to the conventional breaking-change format and added the |
3 participants
Summary
/retitlecommand with a local/rename [title]command./renameopen a prefilled editor with Enter-to-save and Esc-to-cancel behavior./resumewith aligned timestamp/title columns, hidden-but-searchable raw session IDs, and a visible position count.Why
Renaming a session should be an immediate local metadata operation rather than a model-backed bulk action. The previous resume rows also gave raw IDs too much visual space, truncating the titles users actually scan for.
Impact
Users can rename the current session without a provider call or token usage, including before the first prompt. Resume rows are cleaner and easier to scan while session IDs remain available through search.
Validation
make fmt-checkgo vet ./...go test ./...go run ./cmd/zero-release buildgo run ./cmd/zero-release smokemake lint-staticmake vulncheckgit diff HEAD --checkSummary by CodeRabbit
/renameto rename the active session or set a title before the first prompt./retitleis no longer available; rename editor state better blocks unrelated UI interactions.BREAKING CHANGE: /retitle is removed; use /rename instead.