Improve /resume: interactive session picker + model-generated titles - #224
Conversation
- /resume (no arg) now opens an interactive picker like /model and /provider: one row per resumable session showing the title and session id (+ relative age); arrow to highlight, type to filter, enter to load. /resume <id> and /resume latest still resolve directly. - Resume now loads the rehydrated (compaction-aware) event view instead of the raw log, so a resumed session honors a prior /compact — matching the CLI's `zero exec --resume` and the in-TUI /compact reload. Falls back to raw on a rehydrate error. - The resume summary reports the model/provider the continuation will actually use (the active ones), noting the session's recorded model/provider when it differs, instead of implying a switch that doesn't happen. - Drop the dead, self-contradictory resumeText(arg) branch (it was unreachable and its hint was circular); resumeText is now the no-session/error fallback. Tests: session picker opens with title+id rows and hydrates on selection; resume honors prior compaction (rehydrated, not raw events).
Same-titled sessions (e.g. the same first prompt run several times) looked like duplicates because the row showed only the identical title plus a long id that truncated the age away. Lead each row's label with a precise local timestamp (HH:MM:SS today, "Jan _2 15:04" earlier this year, else the date) so distinct sessions are obviously distinct; the full id stays as the right-aligned meta and is what selection resolves. sessionWhen parses the RFC3339 UpdatedAt.
Most of a heavily-retried prompt's sessions are empty failed runs — just the
prompt plus the no-output guardrail stop ("Agent stopped … with no output …"),
with no assistant text and no tool calls. They have nothing to resume and flood
the picker with identical-looking rows. Skip any session with no resumable
content (no tool call/result and no real non-user message) from the picker; they
stay on disk and are never deleted. Adds agent.IsNoProgressStop to recognize the
stop message from a recorded event, and a test that the picker hides an empty
run while keeping a real one.
Sessions were titled from their first user message, so prompts that started the same way (or empty/failed runs) looked identical in the /resume picker. Generate a concise, specific title with the active provider instead. - sessions.Store.UpdateTitle(id, title): rewrites only Title under the per-session lock, re-reading the latest metadata first; leaves UpdatedAt untouched (a retitle is not activity and must not reorder the resumable list), rejects a blank title, and no-ops an unchanged one. - Title generator: a bounded digest of the conversation (user/assistant text + tool names, per-message and total caps, skipping the no-output guardrail stop) is sent as a one-shot completion; the response is cleaned (first content line, quotes/markup/"Title:" label stripped, word- and rune-capped) before storing. - Auto-title going forward: after a successful turn, a session still carrying its default first-message title gets a title in the background, at most once per session. - /retitle: backfills existing resumable sessions that still have a first-message title, one at a time, skipping empty/failed runs and already-named sessions, with kickoff and completion status lines. Generation runs off the Update goroutine; failures are non-fatal (the first-message title simply stays). Adds store + TUI unit tests (UpdateTitle semantics, digest/cleaning, auto-title one-shot, backfill candidate selection). gofmt/vet/build(host+linux+windows)/test -race/ staticcheck all green.
…ature
Adapts the /resume picker + model-generated titles to Bubble Tea v2:
- session_title.go now imports charm.land/bubbletea/v2.
- Tests drive keys via the v2 testKey() helper instead of
tea.KeyMsg{Type: ...}, and read the picker overlay via
viewString(View()) (View() returns tea.View under v2).
- Resolves the TestResumeCommandListsRecentSessions conflict in favor of
the picker assertions (bare /resume opens the session picker).
gofmt/vet/build(host+linux+windows)/staticcheck clean; sessions + tui
pass under -race.
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. |
WalkthroughAdds automatic background session title generation after each successful agent turn, a ChangesSession Auto-Titling, /retitle Command, and Interactive /resume Picker
Sequence Diagram(s)sequenceDiagram
participant User
participant TUI as TUI model.handleSubmit
participant AgentLoop as agentResponseMsg handler
participant AutoTitle as maybeAutoTitleActiveSession
participant GenCmd as generateSessionTitleCmd
participant Provider as Provider Stream
participant Store as Store.UpdateTitle
participant SessionPicker as newSessionPicker
User->>TUI: /retitle
TUI->>TUI: startSessionRetitle (scan, filter auto-titled)
TUI-->>User: "Retitling N sessions..."
User->>TUI: /resume (bare)
TUI->>SessionPicker: openSessionPicker
SessionPicker->>SessionPicker: filter by resumableContent (exclude no-progress-stop)
SessionPicker-->>TUI: picker overlay
TUI-->>User: "Resume a session" picker
User->>TUI: select session
TUI->>TUI: handleResumeCommand(sessionID)
TUI->>TUI: resumeEvents (rehydrated-first)
TUI-->>User: "Resumed <ID>" summary
AgentLoop->>AutoTitle: maybeAutoTitleActiveSession (post-turn)
AutoTitle->>GenCmd: generateSessionTitleCmd(sessionID, digest)
GenCmd->>Provider: streaming completion (30s)
Provider-->>GenCmd: raw output
GenCmd->>GenCmd: cleanGeneratedTitle
GenCmd->>Store: UpdateTitle
Store-->>GenCmd: persisted
GenCmd-->>AgentLoop: sessionTitleGeneratedMsg
AgentLoop->>AgentLoop: update active session title
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/tui/command_center.go (1)
235-239:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
resumeTextagainst a nilsessionStoreto avoid panic.Line 238 dereferences
m.sessionStoreunconditionally. In fallback paths, this can crash instead of rendering a safe “store unavailable” response.Proposed fix
func (m model) resumeText() string { + if m.sessionStore == nil { + return renderCommandOutput(commandOutput{ + Title: "Sessions", + Status: commandStatusBlocked, + Sections: []commandSection{{ + Title: "Store", + Lines: []string{"error: session store is unavailable"}, + }}, + }) + } // Only standalone conversations — not child/spec sub-runs, which an agent // spawns by the dozen and would otherwise flood the picker (the "… N more"). sessions, err := m.sessionStore.ListResumable()🤖 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/command_center.go` around lines 235 - 239, Add a nil check for m.sessionStore in the resumeText method before calling ListResumable() on it. If m.sessionStore is nil, return an appropriate fallback message indicating the store is unavailable instead of allowing the unconditional dereference that would cause a panic. This guards against cases where sessionStore might not be initialized in fallback paths.
🧹 Nitpick comments (1)
internal/tui/model_test.go (1)
984-986: ⚡ Quick winAssert full rehydrated event equality, not only count.
This can false-pass if resume loads the wrong event slice with the same length. Compare
next.sessionEventsandrehydrateddirectly to lock the compaction contract.Suggested test hardening
import ( "bytes" "context" "errors" "path/filepath" + "reflect" "strings" "testing" "time" @@ if len(next.sessionEvents) != len(rehydrated) { t.Fatalf("resumed sessionEvents = %d, want rehydrated %d (not raw %d): resume must honor prior compaction", len(next.sessionEvents), len(rehydrated), len(raw)) } + if !reflect.DeepEqual(next.sessionEvents, rehydrated) { + t.Fatalf("resumed sessionEvents do not match rehydrated events") + } }🤖 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/model_test.go` around lines 984 - 986, The test assertion in the rehydration check only validates that next.sessionEvents and rehydrated have the same length, which can produce false positives if the wrong events are loaded. Replace the length comparison with a direct equality check that compares next.sessionEvents and rehydrated slices fully to ensure the compaction contract is properly honored and the correct events are restored during resume.
🤖 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/agent/guardrails.go`:
- Around line 105-109: The IsNoProgressStop function currently uses
strings.Contains to match the noOutputStopMarker anywhere in the content, which
is too permissive and can cause false positives when the marker appears as part
of legitimate assistant or user text. Tighten the detection logic by replacing
the substring check with a more precise validation—such as checking for an exact
content match (where the entire content equals the marker) or validating the
marker appears only at a specific position or in a specific format that reliably
identifies actual guardrail stops rather than coincidental substring matches.
In `@internal/tui/session.go`:
- Around line 201-204: In the fallback path where
m.sessionStore.ReadEvents(sessionID) is called in the resumeEvents function, the
error handling block returns the wrong error variable. When rawErr is not nil,
change the return statement to return rawErr instead of err, so that the actual
failure from ReadEvents is propagated rather than masking it with the earlier
rehydration error.
---
Outside diff comments:
In `@internal/tui/command_center.go`:
- Around line 235-239: Add a nil check for m.sessionStore in the resumeText
method before calling ListResumable() on it. If m.sessionStore is nil, return an
appropriate fallback message indicating the store is unavailable instead of
allowing the unconditional dereference that would cause a panic. This guards
against cases where sessionStore might not be initialized in fallback paths.
---
Nitpick comments:
In `@internal/tui/model_test.go`:
- Around line 984-986: The test assertion in the rehydration check only
validates that next.sessionEvents and rehydrated have the same length, which can
produce false positives if the wrong events are loaded. Replace the length
comparison with a direct equality check that compares next.sessionEvents and
rehydrated slices fully to ensure the compaction contract is properly honored
and the correct events are restored during resume.
🪄 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: a78dbe17-b226-4625-a236-f0b507757e62
📒 Files selected for processing (12)
internal/agent/guardrails.gointernal/sessions/session_title_test.gointernal/sessions/store.gointernal/tui/command_center.gointernal/tui/commands.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/picker.gointernal/tui/session.gointernal/tui/session_test.gointernal/tui/session_title.gointernal/tui/session_title_test.go
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Requesting changes based on the current diff.
Findings:
- internal/agent/guardrails.go:108: IsNoProgressStop uses strings.Contains, so any legitimate message that quotes the no-output marker can be treated as a failed empty run. That can hide real sessions from /resume and skip title generation. Please tighten this to an exact or otherwise structured guardrail-stop match.
- internal/tui/session.go:201-204: resumeEvents returns the earlier rehydration err when raw fallback ReadEvents fails. It should return rawErr so callers see the actual fallback failure.
- internal/tui/command_center.go:235-238: resumeText dereferences m.sessionStore without a nil guard. newModel normally initializes it, but this fallback method should still avoid a zero-value/model-test panic.
- internal/tui/model_test.go:984-986: the rehydration test only compares lengths. Please compare next.sessionEvents with the rehydrated event slice directly so the test proves /resume loaded the compacted context, not another same-length slice.
CI is green and the feature direction looks fine, but the first two are correctness issues that should be fixed before merge.
…nil guard Three review findings on the /resume work: - IsNoProgressStop matched any content CONTAINING the no-output marker substring (strings.Contains), so a legitimate assistant/tool message that quoted the marker could be misread as a failed empty run — wrongly hiding a real session from the /resume picker and skipping its title generation. Now it requires the full answer structure (prefix + marker + suffix); a quote no longer matches. - resumeEvents returned the earlier rehydration error when the raw ReadEvents fallback itself failed, masking the actual fallback failure. Returns rawErr. - resumeText dereferenced m.sessionStore unconditionally; a nil store (fallback/ test paths) would panic. Now renders a safe "store unavailable" message. (newSessionPicker already guarded nil.) Tests: IsNoProgressStop accepts the real answer and rejects a quoting message / bare marker / prefix- or suffix-only; resumeText with a nil store returns the safe message instead of panicking. Full gate green.
|
Thanks for the review — all three addressed in
gofmt / vet / build (host+linux+windows) / |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Verdict: Request changes — three substantive issues + one minor test-strength finding
This is a strong, well-architected feature — the file split (separate session_title.go from session.go), the bounded-digest approach, the per-session lock on UpdateTitle, the strict no-progress detection, and the one-shot auto-title gating are all solid choices. The test coverage is good. The issues below are correctness/UX gaps that are worth fixing before merge; the architecture stays as-is.
Findings
1. titledSessions is set optimistically — auto-title failures are silent and permanent (UX correctness)
internal/tui/session_title.go:250 (maybeAutoTitleActiveSession) and :289 (startSessionRetitle) set m.titledSessions[sessionID] = true before the title cmd fires, so a failed title generation (provider error, model returned no usable title, timeout) leaves the session with its first-message title forever for this process. The user has no idea anything happened — no log line, no status row, no retry path, and the only escape is process restart. The PR comment says "on failure the first-message title simply stays" but it stays permanently, not just for this turn.
Suggested fix: move the titledSessions[id] = true into the success path of the returned msg (handleSessionTitleGenerated), not the cmd-scheduling site. The session id is gated only after the title actually persists. Failure leaves the map untouched, so a future turn (or /retitle restart) can retry.
2. IsNoProgressStop is structurally loose — a real message that quotes the marker in passing is misclassified
internal/agent/guardrails.go:117 — the current check is HasPrefix(Prefix) && Contains(Marker) && HasSuffix(Suffix), which only requires the three components in order with arbitrary text in between. A hostile real message like
Agent stopped after 3 turns. The marker is "with no output (no visible text and no tool calls)" so here it is: to avoid consuming tokens without making progress.
is currently classified as a no-progress stop and would hide a real session from /resume and skip its title generation. (I confirmed this in a quick repro outside the repo.) The PR's own test guards against the "marker only" case but not the "marker as a quote in the middle" case.
Suggested fix: after HasPrefix succeeds, find the marker's index, then require the suffix to come immediately after the marker (whitespace allowed but no arbitrary text). Something like:
idx := strings.Index(trimmed, noOutputStopMarker)
if idx < 0 {
return false
}
return strings.HasSuffix(trimmed[idx+len(noOutputStopMarker):], noOutputStopSuffix)Add a test case to no_progress_stop_test.go for the hostile-input sentence above.
3. maybeAutoTitleActiveSession has no sessionStore == nil guard
internal/tui/session_title.go:243 — checks m.provider == nil but not m.sessionStore == nil. The auto-title path is reachable for any model that ran a successful turn, including the fallback model constructed in some tests. If m.sessionStore is nil, the closure inside generateSessionTitleCmd would nil-dereference on the auto-title path (the backfill path happens to guard it because the closure re-reads events). The other nil-store fix in this PR (resumeText, newSessionPicker) set the precedent — this one was missed.
Suggested fix: add if m.sessionStore == nil { return m, nil } after the provider check. Add a small test (TestMaybeAutoTitleSkipsWhenStoreNil) parallel to the existing TestResumeCommandReportsMissingSession style.
4. TestResumeHonorsPriorCompaction only compares slice lengths, not contents
internal/tui/model_test.go:946 (and :984 for the assertion) — already flagged by @anandh8x. The setup checks len(rehydrated) >= len(raw) and the assertion checks len(next.sessionEvents) != len(rehydrated). A regression that produced a same-length but different-content slice (e.g. rehydration returns the raw events in a different order, or replaces the compaction summary event with one of the dropped originals) would pass this test.
Suggested fix: replace the length check with if !reflect.DeepEqual(next.sessionEvents, rehydrated) { t.Fatalf(...) } (or element-by-element comparison for a more readable failure message). Cheap, catches the actual bug class.
Non-blocking notes (skip if you want)
generateSessionTitleCmdclosure capturesm.providerandm.sessionStoreat cmd-build time. If the user runs/providerbetween the successful turn and the title cmd landing, the title is generated by the old provider. Probably fine — same provider, different model — but worth a one-line comment if you want to be explicit.UpdateTitle(internal/sessions/store.go:580) trims the input but doesn't normalize internal whitespace, so a model return of"Some Title"(two spaces) won't no-op against an existing"Some Title"(one space). Minor; would just rewrite the file with the new whitespace.
What's good (no action needed)
- The picker structure mirrors
/modeland/providercleanly. sessionTitleDigestcorrectly skips the no-output stop AND respects the digest char budget.cleanGeneratedTitlecovers the realistic model failure modes (quotes, code fences, "Title:" labels, multi-line responses).resumeEventsrehydration-aware-with-fallback is the right call for the compaction correctness story.sessionWhentimestamp formatting (today →15:04:05, this year →Jan _2 15:04, else date) is exactly the right granularity.- Per-session
titledSessionsgating at the type level (a map, not a counter) is the right shape for "at most once per session." - All tests pass on
-count=1and on CI. The "Zero Review" failure is the pre-existingTestLoadOrCreateSecretConcurrentConvergesflake ininternal/oauth, not this PR.
Happy to pair on any of these — they're all 5-15 line changes.
Plan guide — revisions for
|
…o-title, nil-store guard, deep resume assertion - guardrails: IsNoProgressStop now matches the exact structure noOutputStopAnswer emits — prefix + "<int> turns " + marker + " " + suffix — instead of prefix && contains(marker) && suffix. A genuine message that merely quotes the marker amid other prose is no longer misclassified as a failed empty run (which would wrongly hide a real session from /resume and skip its title generation). Adds the hostile-input regression cases (quoted marker, text between marker/suffix, non-integer turn count). - session_title: release the optimistic titledSessions gate when a generation FAILS (provider error, empty title, store write error) so a later turn or /retitle can retry; success keeps the one-shot gate. Kept the optimistic mark (rather than mark-on-success) to preserve the no-double-fire guarantee while a generation is in flight. - session_title: guard maybeAutoTitleActiveSession against a nil sessionStore (the title cmd calls store.UpdateTitle) — mirrors the resume nil-store guards. - model_test: assert resumed sessionEvents with reflect.DeepEqual against the rehydrated context, not just slice length, so a reordered/substituted-but- same-length regression is caught.
|
Thanks for the thorough review @Vasanthdev2004 — all four addressed in 1. 2. 3. 4. Length-only compaction assertion. Fixed — Non-blocking notes left as-is: the Gate: |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/tui/session_title.go (1)
265-275:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing nil guard for
sessionStorebefore use.
maybeAutoTitleActiveSessioncorrectly checksm.sessionStore == nilat line 241, butstartSessionRetitleonly checksm.providerbefore callingm.sessionStore.ListResumable()at line 272. If/retitleis invoked with a nil store (e.g., in a test or fallback context), this panics.🛡️ Proposed fix to add nil-store guard
func (m model) startSessionRetitle() (model, tea.Cmd, string) { if m.provider == nil { return m, nil, "Cannot retitle sessions: no active provider is configured." } + if m.sessionStore == nil { + return m, nil, "Cannot retitle sessions: session store is unavailable." + } if m.retitleActive { return m, nil, fmt.Sprintf("Already generating titles (%d/%d). Let it finish first.", m.retitleDone, m.retitleTotal) }🤖 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.go` around lines 265 - 275, The startSessionRetitle function checks if m.provider is nil but does not check if m.sessionStore is nil before calling m.sessionStore.ListResumable() at line 272. Add a nil guard for m.sessionStore early in the startSessionRetitle function, similar to the pattern already used in maybeAutoTitleActiveSession (which checks m.sessionStore == nil at line 241), to prevent a panic if the session store is not initialized. Return an appropriate error message when the store is nil.
🤖 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.
Outside diff comments:
In `@internal/tui/session_title.go`:
- Around line 265-275: The startSessionRetitle function checks if m.provider is
nil but does not check if m.sessionStore is nil before calling
m.sessionStore.ListResumable() at line 272. Add a nil guard for m.sessionStore
early in the startSessionRetitle function, similar to the pattern already used
in maybeAutoTitleActiveSession (which checks m.sessionStore == nil at line 241),
to prevent a panic if the session store is not initialized. Return an
appropriate error message when the store is nil.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dc655539-65d5-448b-8d1c-a3988232e674
📒 Files selected for processing (5)
internal/agent/guardrails.gointernal/agent/no_progress_stop_test.gointernal/tui/model_test.gointernal/tui/session_title.gointernal/tui/session_title_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/agent/guardrails.go
- internal/tui/model_test.go
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
LGTM ? no issues found. UpdateTitle correctly serializes under the per-session lock, re-reads latest metadata before writing, leaves UpdatedAt untouched, rejects blank/unchanged titles, and the auto-title path fires at most once per session. The resume picker hydrates via handleResumeCommand and correctly prefers the compaction-aware event view.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
LGTM ? no issues found. UpdateTitle correctly serializes under the per-session lock, re-reads latest metadata before writing, leaves UpdatedAt untouched, rejects blank/unchanged titles, and the auto-title path fires at most once per session. The resume picker hydrates via handleResumeCommand and correctly prefers the compaction-aware event view.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Looked this over carefully ??? it's a solid PR. The interaction with choosePicker for the new pickerSession kind is wired in cleanly, and openSessionPicker does the right thing by short-circuiting to the text fallback when there's nothing resumable. handleResumeCommand -> resumeEvents preferring the rehydrated (compaction-aware) view with a raw-read fallback is the right move, and the summary line showing the active vs recorded model/provider is a small but very useful touch ??? it'll save people a lot of "wait, why did it switch?" moments.
The title work is where I was most skeptical, and it's well-handled:
Store.UpdateTitleis locked under the same per-session mutex asAppendEventand re-reads metadata under that lock, so it can't race with a concurrent append or vice-versa. LeavingUpdatedAtalone on a retitle is the right call ??? a title isn't activity, and a retitle should never reorder the picker.- The
titledSessionsgate is set optimistically when the cmd is scheduled and released on failure, so a hung/failed provider doesn't permanently lock a session out of ever getting a model-generated title. Auto-title is silent on failure (first-message title stays), which is the right UX. cleanGeneratedTitleis defensive in the right ways: first-non-empty-line after markup strip, word cap, rune cap, andsessionTitleTimeoutkeeps a stuck provider from wedging the background cmd.- The
IsNoProgressStopstructural match (prefix +<int> turns+ marker + suffix) is correct ??? a model quoting the marker amid other prose won't be misclassified as an empty run and wrongly hidden from the picker.
session_title_test.go covers digest, cleaning, auto-title one-shot, and /retitle candidate selection; the test in the sessions package covers the lock/no-blank/no-op semantics. resume_nil_store_test.go covers the nil-store paths.
I also confirmed gofmt/go vet clean and the three affected packages (internal/sessions, internal/agent, internal/tui) pass under go test -count=1.
LGTM. Two small suggestions for follow-up, not blockers:
- The title generation runs serially in
/retitle(one at a time viam.retitleQueue). That's the right call to avoid burst-charging, but the per-sessionsessionTitleTimeoutis 30s ??? a run with N sessions could take N*30s in the worst case. Worth surfacing a cancel command in/retitleso a user can bail if it's taking too long. m.titledSessionslives on the model. If the TUI restarts (process restart) the gate resets, so a session that was mid-title at exit can get re-titled on the next turn. Probably fine in practice (the model will just re-generate the same title), but worth noting in case you want a persistenttitledAtmarker on the metadata.
Nice work on this one ??? the picker UX is much better than the old text blob, and the model-generated titles make /resume actually usable for finding past work.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Looked this over carefully ??? it's a solid PR. The interaction with choosePicker for the new pickerSession kind is wired in cleanly, and openSessionPicker does the right thing by short-circuiting to the text fallback when there's nothing resumable. handleResumeCommand -> resumeEvents preferring the rehydrated (compaction-aware) view with a raw-read fallback is the right move, and the summary line showing the active vs recorded model/provider is a small but very useful touch ??? it'll save people a lot of "wait, why did it switch?" moments.
The title work is where I was most skeptical, and it's well-handled:
Store.UpdateTitleis locked under the same per-session mutex asAppendEventand re-reads metadata under that lock, so it can't race with a concurrent append or vice-versa. LeavingUpdatedAtalone on a retitle is the right call ??? a title isn't activity, and a retitle should never reorder the picker.- The
titledSessionsgate is set optimistically when the cmd is scheduled and released on failure, so a hung/failed provider doesn't permanently lock a session out of ever getting a model-generated title. Auto-title is silent on failure (first-message title stays), which is the right UX. cleanGeneratedTitleis defensive in the right ways: first-non-empty-line after markup strip, word cap, rune cap, andsessionTitleTimeoutkeeps a stuck provider from wedging the background cmd.- The
IsNoProgressStopstructural match (prefix +<int> turns+ marker + suffix) is correct ??? a model quoting the marker amid other prose won't be misclassified as an empty run and wrongly hidden from the picker.
session_title_test.go covers digest, cleaning, auto-title one-shot, and /retitle candidate selection; the test in the sessions package covers the lock/no-blank/no-op semantics. resume_nil_store_test.go covers the nil-store paths.
I also confirmed gofmt/go vet clean and the three affected packages (internal/sessions, internal/agent, internal/tui) pass under go test -count=1.
LGTM. Two small suggestions for follow-up, not blockers:
- The title generation runs serially in
/retitle(one at a time viam.retitleQueue). That's the right call to avoid burst-charging, but the per-sessionsessionTitleTimeoutis 30s ??? a run with N sessions could take N*30s in the worst case. Worth surfacing a cancel command in/retitleso a user can bail if it's taking too long. m.titledSessionslives on the model. If the TUI restarts (process restart) the gate resets, so a session that was mid-title at exit can get re-titled on the next turn. Probably fine in practice (the model will just re-generate the same title), but worth noting in case you want a persistenttitledAtmarker on the metadata.
Nice work on this one ??? the picker UX is much better than the old text blob, and the model-generated titles make /resume actually usable for finding past work.
…icker # Conflicts: # internal/tui/model.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/tui/model.go (1)
1489-1499: 💤 Low valueDead code:
scrollableTranscriptViewandscrollableTranscriptLayoutVieware unused.Static analysis confirms
scrollableTranscriptViewis never called. It delegates toscrollableTranscriptLayoutView, which is also only called from here. Both can be removed—the active code path now goes throughscrollableTranscriptItemsView.🧹 Remove dead code
-func (m model) scrollableTranscriptView(header string, body string, footer string, width int, overlay string) string { - return m.scrollableTranscriptLayoutView(header, transcriptBodyLayout{lines: viewLines(body)}, footer, width, overlay) -} - -func (m model) scrollableTranscriptLayoutView(header string, body transcriptBodyLayout, footer string, width int, overlay string) string { - frame := m.scrollableTranscriptFrame(header, footer) - window := transcriptViewportForLayout(body, frame, m.chatScrollOffset).window() - - bodyWindow := body.visibleLines(window) - return m.renderScrollableTranscriptWindow(frame, bodyWindow, window, width, overlay) -}🤖 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/model.go` around lines 1489 - 1499, The functions scrollableTranscriptView and scrollableTranscriptLayoutView are dead code that are no longer used in the codebase. Remove both of these methods entirely from the model.go file. The scrollableTranscriptView method delegates to scrollableTranscriptLayoutView which is only called from scrollableTranscriptView, and static analysis confirms neither function is called elsewhere. The active code path now uses scrollableTranscriptItemsView instead.Source: Linters/SAST tools
🤖 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/model.go`:
- Around line 1489-1499: The functions scrollableTranscriptView and
scrollableTranscriptLayoutView are dead code that are no longer used in the
codebase. Remove both of these methods entirely from the model.go file. The
scrollableTranscriptView method delegates to scrollableTranscriptLayoutView
which is only called from scrollableTranscriptView, and static analysis confirms
neither function is called elsewhere. The active code path now uses
scrollableTranscriptItemsView instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: da2d3e91-272e-40ea-8afb-ce67a4b0e560
📒 Files selected for processing (3)
internal/tui/command_center.gointernal/tui/model.gointernal/tui/model_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/tui/command_center.go
- internal/tui/model_test.go
3 participants
What & why
/resumelisted sessions as a static text blob, and every session was titled by its first user message — so prompts that started the same way (and the many empty/failed runs that produced no output) were indistinguishable in the list. This reworks/resumeinto an interactive picker and gives sessions real, model-generated titles.Changes
Interactive session picker
/resumenow opens a single-select overlay (a newpickerSessionkind) just like/modeland/provider: arrow/type-to-filter/enter./resume <id>and/resume lateststill resolve directly.15:04:05, this year →Jan _2 15:04, else date) so same-titled sessions are still distinguishable; the id shows in the row meta.Hide empty/failed sessions
agent.IsNoProgressStoprecognizes that stop marker.Model-generated titles
sessions.Store.UpdateTitlerewrites only the title under the per-session lock (re-reading the latest metadata first), leavesUpdatedAtuntouched so a retitle never reorders the list, rejects blanks, and no-ops an unchanged title.Title:label stripped, word- and rune-capped)./retitlebackfill: titles existing resumable sessions that still have a first-message title, one at a time, skipping empty/failed and already-named sessions, with kickoff and completion status lines.Resume correctness fixes
zero exec --resumeand the in-TUI/compactreload, so resuming a compacted session honors the prior compaction instead of re-inflating the raw log.Notes
/retitle, so no burst).Testing
UpdateTitlesemantics; digest/cleaning/auto-title one-shot;/retitlecandidate selection; picker open/hydrate; hide-empties; resume honors prior compaction.go vet, build (host + linux + windows), staticcheck all clean;internal/sessionsandinternal/tuipass under-race.Summary by CodeRabbit
Release Notes
New Features
/retitleto generate/update concise session titles./resumenow opens an interactive session picker when used without an ID.Improvements
Bug Fixes
Tests