fix: manual rename reverts to folder default after a reload race (TitleLocked lost on reapply)#1570
Open
pablosiber wants to merge 2 commits into
Open
Conversation
…ad race A manual session rename could silently revert to the cwd-folder default on the next turn. When a rename's save was skipped (isReloading=true), the title was queued in pendingTitleChanges and re-applied after the storage-watcher reload swap — but the queue stored only the title string, dropping its TitleLocked intent. The reapplied title came back unlocked, so the next asheshgoplani#572 Claude-name sync (ReconcileTitleFromClaude) overwrote it with Claude Code 2.1.19x's auto-derived folder name. Carry the lock state alongside the title (new pendingTitle struct). All three writers capture inst.TitleLocked at enqueue time — true for a user rename via SetField, false for a sync-sourced title — and the reapply restores it, so a user rename stays locked while a synced title keeps tracking Claude. Pinned by TestHomeRenamePendingChangeRestoresTitleLock (both directions).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR introduces a ChangesPending title lock persistence
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/ui/home_test.go (1)
729-805: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider documenting RemoteSession non-applicability.
This new test exercises the pending-title reload/lock-reapply path but neither covers a
RemoteSessioncase nor documents why one is unnecessary. SincependingTitleChangesreapply is keyed by session ID and resolved viagetInstanceByID(local*session.Instanceonly),RemoteSession(which has no localInstance) is likely structurally inapplicable here — analogous to the precedent for fork-gating tests. Adding a shortt.Skip(or comment) documenting this would satisfy the project convention for TUI-behavior test changes.📝 Suggested documentation addition
func TestHomeRenamePendingChangeRestoresTitleLock(t *testing.T) { + // Note: RemoteSession has no local *session.Instance and is never keyed + // into pendingTitleChanges/getInstanceByID, so it cannot reach this + // reload-reapply path. No RemoteSession case is needed here. t.Run("user rename is relocked", func(t *testing.T) {Based on coding guidelines, "When a PR changes TUI behavior in internal/ui/ or cmd/agent-deck/, verify it also handles RemoteSession or include an explicit t.Skip documenting why".
🤖 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/ui/home_test.go` around lines 729 - 805, The new TestHomeRenamePendingChangeRestoresTitleLock in home_test.go covers the pending-title reapply path for local instances but does not address RemoteSession applicability. Add a short t.Skip or an explanatory comment in this test to document that pendingTitleChanges is resolved through getInstanceByID on local *session.Instance objects, so RemoteSession is structurally not applicable here. Keep the note near TestHomeRenamePendingChangeRestoresTitleLock so future TUI behavior changes understand why no RemoteSession coverage is needed.Source: Coding guidelines
🤖 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/ui/home.go`:
- Around line 9711-9722: The local rename path in
h.getInstanceByID/session.SetField mutates inst.Title and inst.TitleLocked
without the same h.instancesMu protection used elsewhere, so update the rename
flow in home.go to hold h.instancesMu around this mutation or delegate to the
existing locked helper. Keep the lock consistent with the other instance
mutators and make sure the pendingTitleChanges update still happens after the
protected rename.
---
Nitpick comments:
In `@internal/ui/home_test.go`:
- Around line 729-805: The new TestHomeRenamePendingChangeRestoresTitleLock in
home_test.go covers the pending-title reapply path for local instances but does
not address RemoteSession applicability. Add a short t.Skip or an explanatory
comment in this test to document that pendingTitleChanges is resolved through
getInstanceByID on local *session.Instance objects, so RemoteSession is
structurally not applicable here. Keep the note near
TestHomeRenamePendingChangeRestoresTitleLock so future TUI behavior changes
understand why no RemoteSession coverage is needed.
🪄 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: 6e7ccfa9-cf25-4584-a781-ab9b328595a9
⛔ Files ignored due to path filters (1)
CHANGELOG.mdis excluded by!**/*.md,!CHANGELOG.md
📒 Files selected for processing (3)
internal/ui/home.gointernal/ui/home_test.gointernal/ui/tui_eval_seam_b_test.go
…ession N/A Addresses CodeRabbit review on asheshgoplani#1570: - Hold h.instancesMu around the group-dialog SetField mutation (mirrors the edit-dialog rename path); run the tmux-heavy postCommit after unlocking so a slow subprocess can't stall the status worker / reconciler. - Document why the new lock-reapply test needs no RemoteSession case (pendingTitleChanges resolves via getInstanceByID, local instances only).
Author
|
Thanks @coderabbitai — both points addressed in e5875c2:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
cd /repo 2>/dev/null || true
git log --oneline -10
git show e5875c2 --stat 2>/dev/null || echo "commit not found in shallow clone"Length of output: 1866 🏁 Script executed: #!/bin/bash
cd /repo 2>/dev/null || true
git show e5875c2 -- internal/ui/home.goLength of output: 2398 The diff confirms both points were addressed correctly:
Looks good — thanks for the quick turnaround, [resolve] (＾▽＾) |
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
1 participant
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.
Symptom
A session I renamed by hand keeps snapping back to the default
<folder>-<hash>name — not once, but intermittently across restarts. In the DB the affected rows havetitle_locked = 0, while renames that did stick havetitle_locked = 1.Root cause
The pending-title reapply path drops the lock.
pendingTitleChangesexists so a rename survives a storage-watcher reload that races the save (isReloading=true→saveInstances()is a no-op → the reload replacesh.instanceswith stale disk rows). But the map stored only the title string:session.SetField(FieldTitle, …)correctly setsTitle+SetAutoName(false)+TitleLocked = true, and queues the title inpendingTitleChanges.title_locked = 0(the lock was never persisted).loadSessionsMsgrestoresTitleand clears auto-name, but never re-setsTitleLocked— so the row is now correctly-named but unlocked.ReconcileTitleFromClaudesees!TitleLocked, reads Claude Code 2.1.19x's auto-derived cwd-folder name (nameSource="derived"), and overwrites the title back to the folder default.This is adjacent to #1545 (which stopped the derived name from clobbering the
auto_namehandle) but distinct: here a genuine user rename loses its lock and is reverted. It's a race, which is why it recurs intermittently.Fix
Carry the lock intent through the reload swap.
pendingTitleChangesbecomesmap[string]pendingTitle{title, locked}. All three writers captureinst.TitleLockedat enqueue time:handleEditSessionDialogKey(SetField dispatch)truehandleGroupDialogKey(TUI rename)trueattachSession→ReconcileTitleFromClaudefalseThe reapply restores
TitleLockedfrom the queued value — so a user rename stays locked (survives the #572 sync) while a sync-sourced title stays unlocked (keeps tracking Claude's session name). The unlocked case is why a blanketTitleLocked = truewould be wrong.Tests
TestHomeRenamePendingChangeRestoresTitleLockwith both directions (user rename → relocked; sync title → stays unlocked). Verified it fails onmain(TitleLocked = false after reapply, want true) and passes with the fix.TestHomeRenamePendingChanges*updated to the struct and still green.go build,go vet,gofmt -l, and the full./internal/ui/+./internal/session/suites all pass locally (go 1.25.11).Notes
Found while on v1.9.73; the derived-name interaction is more visible since Claude Code 2.1.19x started auto-deriving folder names. Reproduces on current
main(v1.10.9) via the added test.Summary by CodeRabbit