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 |
|
@gnanam1990 your blocker is done — the title is now Doing exactly what you asked did surface two things about the release mechanics that I don't think either of us looked at, and one of them is a real decision rather than a nit. The The Neither is a code change and I'm not re-blocking on either. Flagging because this would be the repo's first |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Verdict
Changes requested — but for a consequence my own previous review created, not for anything you did wrong. The title fix is exactly right; following it will, on current config, cut v1.0.0 instead of v0.7.0. That needs a decision before merge, and it is a one-line config change or a deliberate "yes, 1.0.0".
Re-reviewed at 36f16b7, base 097c265 — the same commit as my previous review. My freshness check missed this fix entirely, because the head SHA cannot move when the change is to PR metadata. Worth recording as a limitation of SHA-diffing when the blocker itself lives outside git.
Withdrawn — the changelog blocker is fixed
The title is now feat(tui)!: replace /retitle with local /rename, with the ! breaking marker. With two commits, squash_merge_commit_title=COMMIT_OR_PR_TITLE takes the PR title, so the squash subject will be conventional and breaking-marked. Resolved.
While confirming the mechanism I found much better evidence than I originally posted, which is worth leaving on the record because it shows this was not a theoretical concern. Five merged PRs whose squash subjects lacked a conventional type are absent from CHANGELOG.md entirely:
Commit on main |
In CHANGELOG |
|---|---|
c00a473 Add Atlas Cloud provider preset (#784) |
no |
ab2f9fb Add shell completion generation (#764) |
no |
18cce35 Add deterministic completion policy (#719) |
no |
9242b9e Add transcript-grounded structured task state (#761) |
no |
0125e70 Fix pinned Go quality tools on Go 1.26 (#689) |
no |
A whole provider preset and a new user-facing command shipped with no changelog record. That is the class, it predates this PR, and it deserves its own issue rather than anything from you here.
Blocking
-
feat(tui)!:will bump 0.6.0 → 1.0.0, not 0.7.0. —release-please-config.jsonThe trace, each hop verified against
origin/main:.release-please-manifest.jsonis{".":"0.6.0"}, andCHANGELOG.md's newest heading is## [0.6.0], so release-please's state is consistent and current at 0.6.0.release-please-config.jsonsetsrelease-type,include-component-in-tag,bootstrap-shaandpackage-name. It does not setbump-minor-pre-major.- release-please's default for
bump-minor-pre-majorisfalse, which means a breaking change on a0.xversion bumps to1.0.0rather than to0.7.0. - The workflow is pinned to
googleapis/release-please-action@a02a34c…(v4.2.0), so this is not version drift on my part. - Nothing has ever exercised this path here: no commit on
maincarries aBREAKING CHANGEfooter, none uses the!marker, andCHANGELOG.mdhas never contained a breaking-changes section. #826 would be the first.
So the likely outcome of merging as-is is a
v1.0.0tag and a1.0.0publish of@gitlawb/zero, triggered by a TUI command rename. Tags and registry publishes are the awkward kind of thing to walk back.Two clean resolutions, and this is a maintainer call rather than a code fix:
- Add
"bump-minor-pre-major": truetorelease-please-config.json— thenfeat(tui)!:yields 0.7.0 and still records the breaking change. This looks like what the project actually wants, given it is deliberately pre-1.0 andCHANGELOG.mdsays versioning applies "once the first release is tagged". - Or accept 1.0.0 knowingly, in which case that decision belongs in the PR description so it is not a surprise in the release PR.
I want to be precise about what I did and did not do: this is a configuration-reading proof, not an executed one. I did not run release-please against this branch, so I cannot show you the computed version. If you would rather not take the default on faith, the release PR that this merge opens will state the version before anything is tagged — that is a safe place to confirm.
Non-blocking
- The
BREAKING CHANGE:footer in the PR description will not reach the commit. — PR body. The footer is there and correctly worded (/retitle is removed; use /rename instead.), butsquash_merge_commit_message=COMMIT_MESSAGESbuilds the squash body from the commit messages, which areImprove session naming and resume pickerandAddress session rename review feedback. Neither carries it. The!in the subject is enough for release-please to classify the change as breaking, so the entry will appear — it will just appear without the sentence telling users what to do instead, which is the part users need. Either amend a commit to carry the footer, or paste it into the squash body in the merge dialog.
Carried forward from my previous review, unchanged
- Four separable changes in one PR. 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. Team PR through the internal cycle, so not a scope close — but the picker change is the one a bisect would most want isolated. - Merge-conflict exposure with #643 and #570, which both touch
internal/tui/session.go,commands.goandmodel.go. /renameis missing from the README command table (README.md:214).- Blank submit closes the editor rather than holding it open (
session_rename.go:32-37); the test asserts this deliberately, so I read it as intent.
Everything I verified about the code itself at 36f16b7 stands — the compare-and-swap is load-bearing under mutation, the pre-prompt gate is tested, /retitle is cleanly excised, and the full gauntlet was green. The only thing standing between this and a merge is which version number it cuts.
anandh8x
changed the title
|
@gnanam1990 removed the breaking marker so this remains a pre-1.0 minor release instead of triggering v1.0.0. Could you re-review and approve if everything looks good? |
gnanam1990
left a comment
gnanam1990
left a comment
There was a problem hiding this comment.
Verdict
Approve. Both of my blockers are resolved, and you picked the right resolution for the second one. Apologies for the two rounds of churn — the first ask created the second problem, and that is on me, not on the change.
Re-reviewed at 36f16b7, base 097c265. The code tree is byte-identical to my two previous reviews (git diff 36f16b7..refs/pull/826/head is empty); the entire delta since my last review is the PR title.
Withdrawn — both blockers
1. The changelog entry. The title is now feat(tui): replace /retitle with local /rename. With two commits, squash_merge_commit_title=COMMIT_OR_PR_TITLE takes the PR title, so the squash subject is conventional and release-please will record it under Features. The subject also states the removal in plain words, which is most of what a reader scanning the changelog needs.
2. The 1.0.0 bump. Dropping the ! avoids it. Traced to be sure rather than assumed:
- Squash subject:
feat(tui): replace /retitle with local /rename (#826)— no!. - Squash body comes from
squash_merge_commit_message=COMMIT_MESSAGES, i.e.Improve session naming and resume pickerandAddress session rename review feedback. Neither carries aBREAKING CHANGE:footer. - With neither
!nor a footer, release-please classifies a plainfeat→ minor bump, 0.6.0 → 0.7.0.
That is the safe outcome and it needs no config change to get there.
Non-blocking — one thing to watch at merge time
The BREAKING CHANGE: footer is still in the PR description. — PR body
It is no longer reachable by the automation, because the squash body is assembled from commit messages rather than the description. But it is sitting right there in the description, which makes it a natural thing for whoever merges to paste into the squash message. If that happens, release-please sees a breaking change, and with bump-minor-pre-major still unset in release-please-config.json the bump becomes 1.0.0 after all.
So the 1.0.0 risk is now conditional on merge-time behaviour rather than eliminated. Either is fine:
- Drop the footer from the description, so there is nothing to paste.
- Or leave it and merge with the default squash body, taking care not to add it.
The better end state is neither of those, and it is not your job here. Removing a command that shipped in v0.3.0 through v0.6.0 genuinely is a breaking change, and the reason it cannot be marked as one is that release-please-config.json lacks "bump-minor-pre-major": true — so ! currently means "go to 1.0.0" instead of "minor bump, flagged breaking". Setting that flag would let this and every future breaking change be labelled honestly while the project stays pre-1.0. That is a one-line repo-config PR and it should not hold this one up.
For the record, since it is the thing a user upgrading would want and will not get: /retitle existed in v0.3.0, v0.4.0, v0.5.0 and v0.6.0, and after this it is gone with /rename as the replacement. It will appear in the changelog as a feature rather than under a breaking-changes heading.
Carried forward, unchanged and not blocking
- Four separable changes in one PR — 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). Team PR through the internal cycle, so not a scope close, but the picker change is the one a bisect would most want isolated. - Merge-conflict exposure with #643 and #570, which both touch
internal/tui/session.go,commands.goandmodel.go. This branch is current, so those should rebase onto it. /renameis missing from the README command table (README.md:214).- Blank submit closes the editor rather than holding it open (
session_rename.go:32-37) — asserted deliberately by the test, so read as intent.
Not re-verified
Nothing about the code needed re-running: the tree is unchanged from my first review, where the compare-and-swap held up under mutation at both layers, the pre-prompt titledSessions gate was mutation-tested, /retitle was confirmed cleanly excised, and the full gauntlet was green on darwin/arm64. Those results stand.
Separately, and not for this PR: five earlier merges with non-conventional subjects are missing from CHANGELOG.md entirely, including Add Atlas Cloud provider preset (#784) and Add shell completion generation (#764). That plus the bump-minor-pre-major gap are one issue about release hygiene, and this PR is a good illustration of why it is worth filing.
|
Correcting myself on the 1.0.0 warning above, since it didn't play out and I'd rather say why than leave it standing. The release PR came out as 0.7.0, not 1.0.0. But not because my reading of No Whether that was deliberate on merge or just the squash box being edited, I don't know. Either way the practical outcome is the thing gnanam originally raised: v0.7.0 removes @kevin-hnnh #837 is still open if you want a |
ONE REAL CONFLICT, in the TUI model's fields. main replaced the /retitle backfill with a local /rename (Gitlawb#826), so its side deletes retitleQueue and the four counters beside it and adds renamePrompt; this branch's side carries the orchestrate plan surface main has never seen. Both, not either: keeping our copy of the retitle fields would have resurrected state whose only callers main removed, leaving it compiling and unreachable. THE POSTURE-OFF GOLDEN MOVED, and it was checked rather than assumed. Gitlawb#838 reworded four tool descriptions (glob, grep, list_directory, read_file), which changes the definition bytes the fingerprint freezes. A moved golden is exactly what a posture leak from this branch would look like, so the two were told apart by measurement: a posture-off run's first request body, compared byte for byte against a binary built from this same main, across --auto low/medium/high/member and --use-spec. All five identical. The absolute sizes moved with main's new wording (33549 -> 31851 on --auto low); the difference between the binaries stayed zero, which is the property the guard exists for. Everything else auto-merged.
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.