From daee6c1562a782b93f985b40b33047889c5049fa Mon Sep 17 00:00:00 2001 From: anandh8x Date: Wed, 29 Jul 2026 21:36:30 +0530 Subject: [PATCH 1/2] Improve session naming and resume picker --- internal/sessions/session_title_test.go | 28 ++++- internal/sessions/store.go | 36 +++++- internal/tui/btw.go | 2 +- internal/tui/clipboard.go | 11 +- internal/tui/commands.go | 10 +- internal/tui/composer.go | 2 +- internal/tui/model.go | 40 +++---- internal/tui/model_test.go | 45 ++++++- internal/tui/session.go | 33 +++-- internal/tui/session_rename.go | 107 +++++++++++++++++ internal/tui/session_rename_test.go | 152 ++++++++++++++++++++++++ internal/tui/session_title.go | 106 +++-------------- internal/tui/session_title_test.go | 91 +++++--------- internal/tui/sidebar.go | 4 +- internal/tui/transcript_selection.go | 3 +- internal/tui/view.go | 11 +- 16 files changed, 477 insertions(+), 204 deletions(-) create mode 100644 internal/tui/session_rename.go create mode 100644 internal/tui/session_rename_test.go diff --git a/internal/sessions/session_title_test.go b/internal/sessions/session_title_test.go index 61e54a9a3..b4896eba7 100644 --- a/internal/sessions/session_title_test.go +++ b/internal/sessions/session_title_test.go @@ -43,7 +43,7 @@ func TestUpdateTitle(t *testing.T) { t.Fatalf("title not trimmed/stored: %q", updated.Title) } if updated.UpdatedAt != before.UpdatedAt { - t.Fatalf("UpdatedAt must not change on retitle: before=%q after=%q", before.UpdatedAt, updated.UpdatedAt) + t.Fatalf("UpdatedAt must not change on rename: before=%q after=%q", before.UpdatedAt, updated.UpdatedAt) } if updated.EventCount != before.EventCount { t.Fatalf("EventCount changed: before=%d after=%d", before.EventCount, updated.EventCount) @@ -67,7 +67,7 @@ func TestUpdateTitle(t *testing.T) { // An unchanged title is a no-op (still succeeds). if _, err := store.UpdateTitle(session.SessionID, "Clean Generated Title"); err != nil { - t.Fatalf("no-op retitle should succeed: %v", err) + t.Fatalf("no-op rename should succeed: %v", err) } // An invalid session id is rejected. @@ -76,6 +76,30 @@ func TestUpdateTitle(t *testing.T) { } } +func TestUpdateTitleIfCurrent(t *testing.T) { + store := newTitleTestStore(t) + session, err := store.Create(CreateInput{Title: "Automatic title"}) + if err != nil { + t.Fatalf("create: %v", err) + } + + updated, applied, err := store.UpdateTitleIfCurrent(session.SessionID, "Automatic title", "Generated title") + if err != nil || !applied || updated.Title != "Generated title" { + t.Fatalf("matching update = (%#v, %v, %v)", updated, applied, err) + } + + if _, err := store.UpdateTitle(session.SessionID, "Manual title"); err != nil { + t.Fatalf("manual rename: %v", err) + } + updated, applied, err = store.UpdateTitleIfCurrent(session.SessionID, "Generated title", "Late generated title") + if err != nil { + t.Fatalf("stale update: %v", err) + } + if applied || updated.Title != "Manual title" { + t.Fatalf("stale update should preserve manual title: (%#v, %v)", updated, applied) + } +} + func TestUpdateModel(t *testing.T) { store := newTitleTestStore(t) session, err := store.Create(CreateInput{ModelID: "model-a"}) diff --git a/internal/sessions/store.go b/internal/sessions/store.go index 0c4ac7050..464940081 100644 --- a/internal/sessions/store.go +++ b/internal/sessions/store.go @@ -719,7 +719,7 @@ func (store *Store) appendPreparedEventsLocked(sessionID string, inputs []prepar // serialized under the same per-session lock as AppendEvent and re-reads the // latest metadata under that lock before rewriting, so a concurrent append can't // clobber the new title (nor the title clobber a concurrent append's event -// count/timestamp). UpdatedAt is deliberately left untouched: a retitle is not +// count/timestamp). UpdatedAt is deliberately left untouched: a rename is not // activity, so it must not reorder the session in the resumable list. A blank // title is rejected so a failed model generation can never erase a useful // first-message title, and an unchanged title is a no-op (no rewrite/fsync). @@ -751,6 +751,40 @@ func (store *Store) UpdateTitle(sessionID string, title string) (Metadata, error return session, nil } +// UpdateTitleIfCurrent replaces a session title only when it still matches +// expected. It lets background automatic naming avoid overwriting a newer manual +// rename while keeping the check and write under the same per-session lock. +func (store *Store) UpdateTitleIfCurrent(sessionID string, expected string, title string) (Metadata, bool, error) { + if !ValidSessionID(sessionID) { + return Metadata{}, false, fmt.Errorf("invalid zero session id %q", sessionID) + } + trimmed := strings.TrimSpace(title) + if trimmed == "" { + return Metadata{}, false, fmt.Errorf("zero session title is required") + } + unlock, err := store.lockSession(sessionID) + if err != nil { + return Metadata{}, false, err + } + defer unlock() + + session, err := store.readMetadata(sessionID) + if err != nil { + return Metadata{}, false, err + } + if session.Title != strings.TrimSpace(expected) { + return session, false, nil + } + if session.Title == trimmed { + return session, true, nil + } + session.Title = trimmed + if err := store.writeMetadata(session); err != nil { + return Metadata{}, false, err + } + return session, true, nil +} + // UpdateModel replaces a session's selected model without changing its activity // timestamp or event counters. An empty model clears the session override. func (store *Store) UpdateModel(sessionID string, modelID string) (Metadata, error) { diff --git a/internal/tui/btw.go b/internal/tui/btw.go index 5fcb7a4ba..2836af741 100644 --- a/internal/tui/btw.go +++ b/internal/tui/btw.go @@ -206,7 +206,7 @@ func (m model) leaveBTW() (model, tea.Cmd) { func btwCommandUnavailable(command parsedCommand) bool { arg := strings.ToLower(strings.TrimSpace(command.text)) switch command.kind { - case commandNew, commandResume, commandRetitle, commandSpec, commandLoop, commandGoal, + case commandNew, commandResume, commandRename, commandSpec, commandLoop, commandGoal, commandRewind, commandCompact, commandSTTModel, commandMCP: return true case commandModel: diff --git a/internal/tui/clipboard.go b/internal/tui/clipboard.go index 34b03b456..8e446935e 100644 --- a/internal/tui/clipboard.go +++ b/internal/tui/clipboard.go @@ -58,7 +58,8 @@ func pasteFromClipboardCmd() tea.Cmd { // right-click paste (clipboardReadMsg) so a bracketed paste and a right-click // paste behave identically. Surfaces with no editable text field (a permission/ // spec prompt, the MCP manager, an open picker, the detailed transcript) swallow -// the paste; empty content is a no-op. +// the paste. The session rename editor accepts text only; empty content is a +// no-op there rather than an image probe. func (m model) routePaste(content string) (tea.Model, tea.Cmd) { // A paste is a deliberate action, same as a keypress or click — it means // the user moved on to something else, so it disarms a stale Esc @@ -75,6 +76,14 @@ func (m model) routePaste(content string) (tea.Model, tea.Cmd) { // toggle instead of going solid. m.lastCharTime = m.now() m.composerCursorVisible = true + if m.renamePrompt != nil { + if content == "" { + return m, nil + } + var cmd tea.Cmd + m.input, cmd = m.input.Update(tea.PasteMsg{Content: sanitizeComposerInput(content)}) + return m, cmd + } if content == "" { // Empty text clipboard — the user may have pasted a screenshot. // Probe the OS clipboard for image content asynchronously. diff --git a/internal/tui/commands.go b/internal/tui/commands.go index 3ee9cf7e8..137f7beb4 100644 --- a/internal/tui/commands.go +++ b/internal/tui/commands.go @@ -27,7 +27,7 @@ const ( commandPlan commandSearch commandResume - commandRetitle + commandRename commandSpec commandInit commandCompact @@ -226,11 +226,11 @@ var commandDefinitions = []commandDefinition{ kind: commandResume, }, { - name: "/retitle", - usage: "/retitle", + name: "/rename", + usage: "/rename [title]", group: commandGroupSession, - description: "Generate concise titles for resumable sessions.", - kind: commandRetitle, + description: "Rename the current session (no arg opens an editor).", + kind: commandRename, }, { name: "/spec", diff --git a/internal/tui/composer.go b/internal/tui/composer.go index 510d96ccc..f3d410efa 100644 --- a/internal/tui/composer.go +++ b/internal/tui/composer.go @@ -281,7 +281,7 @@ func (m model) composerPositionAtMouse(msg tea.MouseMsg) (int, bool) { func (m model) composerMouseSelectionBlocked() bool { return m.transcriptDetailed || m.setup.visible || m.providerWizard != nil || m.mcpAddWizard != nil || - m.mcpManager != nil || m.picker != nil || m.suggestionsActive() + m.mcpManager != nil || m.picker != nil || m.renamePrompt != nil || m.suggestionsActive() } func (m model) composerPositionAtVisualCell(x int, y int, width int) (int, bool) { diff --git a/internal/tui/model.go b/internal/tui/model.go index 239a08dd5..700e641bc 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -102,6 +102,7 @@ type model struct { doctorInFlight bool doctorFrame int activeSession sessions.Metadata + pendingSessionTitle string sessionEvents []sessions.Event btw btwState // btwRunIDSeq is the highest run ID issued by any completed or abandoned BTW @@ -112,14 +113,8 @@ type model struct { // already been attempted this process, so a finished turn re-fires the title // generator at most once per session (even before its async result lands). // Lazily initialized. - titledSessions map[string]bool - // retitle* drive the sequential /retitle backfill: queued session ids still - // awaiting a title, whether a backfill is running, and its progress counters. - retitleQueue []string - retitleActive bool - retitleTotal int - retitleDone int - retitleOK int + titledSessions map[string]bool + renamePrompt *sessionRenamePrompt usageTracker *usage.Tracker sessionCompactor SessionCompactor prService *PrService @@ -1069,7 +1064,7 @@ func (m *model) stopPRWatcher() { func (m model) noBlockingModal() bool { return m.pendingPermission == nil && m.pendingAskUser == nil && m.pendingSpecReview == nil && m.providerWizard == nil && m.mcpAddWizard == nil && m.mcpManager == nil && m.picker == nil && - m.sttKeyPrompt == nil + m.sttKeyPrompt == nil && m.renamePrompt == nil } func (m model) quit() (tea.Model, tea.Cmd) { @@ -1352,6 +1347,9 @@ func (m model) updateModel(msg tea.Msg) (tea.Model, tea.Cmd) { if m.sttKeyPrompt != nil { return m.handleSTTKeyPromptKey(msg) } + if m.renamePrompt != nil { + return m.handleSessionRenameKey(msg) + } m.transcriptSelection = transcriptSelectionState{} m.composerSelection = composerSelectionState{} m.clearMouseSelection() @@ -2948,6 +2946,12 @@ func (m model) pinnedTitleBar(width int) string { func (m model) footerView(width int) string { var footer strings.Builder + if m.renamePrompt != nil { + footer.WriteString(m.sessionRenamePromptView(width)) + footer.WriteString("\n") + footer.WriteString(m.statusLine(width)) + return footer.String() + } // While an ask-user questionnaire is active it REPLACES the composer box (the // text box becomes the questionnaire): render the tabbed prompt + status line and // skip the plan panel / idle hints / composer for a focused modal. @@ -4520,21 +4524,11 @@ func (m model) dispatchCommand(command parsedCommand) (tea.Model, tea.Cmd) { m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: text}) } return m, nil - case commandRetitle: - if m.pending { - m.transcript = reduceTranscript(m.transcript, transcriptAction{ - kind: actionAppendError, - text: "Cannot retitle sessions while a run is active.", - }) - return m, nil - } - text := "" - var retitleCmd tea.Cmd - m, retitleCmd, text = m.startSessionRetitle() - if text != "" { - m.transcript = reduceTranscript(m.transcript, transcriptAction{kind: actionAppendSystem, text: text}) + case commandRename: + if title := strings.TrimSpace(command.text); title != "" { + return m.renameActiveSession(title), nil } - return m, retitleCmd + return m.openSessionRenamePrompt(), nil case commandSpec: return m.handleSpecCommand(command.text) case commandInit: diff --git a/internal/tui/model_test.go b/internal/tui/model_test.go index f2be354aa..451d9d79e 100644 --- a/internal/tui/model_test.go +++ b/internal/tui/model_test.go @@ -886,8 +886,9 @@ func TestResumeCommandListsRecentSessions(t *testing.T) { if next.picker == nil || next.picker.kind != pickerSession { t.Fatalf("expected /resume to open the session picker, got picker=%#v", next.picker) } - // Every row carries the session title (in the Label, after the timestamp) and - // resolves to / shows the session id (Value + Meta), for both sessions. + // Every row carries the session title after the timestamp and resolves by its + // hidden session id. Raw ids are implementation detail and must not consume + // the visible title width. findByID := func(id string) (pickerItem, bool) { for _, item := range next.picker.items { if item.Value == id { @@ -904,17 +905,49 @@ func TestResumeCommandListsRecentSessions(t *testing.T) { if !strings.Contains(item.Label, want.title) { t.Fatalf("picker Label %q should contain the title %q", item.Label, want.title) } - if !strings.Contains(item.Meta, want.id) { - t.Fatalf("picker %q Meta should show the id %q, got %q", want.title, want.id, item.Meta) + if item.Meta != "" { + t.Fatalf("picker %q should not expose raw session id metadata, got %q", want.title, item.Meta) } } - // The picker overlay renders the titles and ids. + // The picker overlay renders clean title rows plus a position indicator. view := viewString(next.View()) - for _, want := range []string{"Resume a session", "Newer", "Older", first.SessionID, second.SessionID} { + for _, want := range []string{"Resume a session", "Newer", "Older", "1 / 2"} { if !strings.Contains(view, want) { t.Fatalf("session picker view missing %q:\n%s", want, view) } } + for _, id := range []string{first.SessionID, second.SessionID} { + if strings.Contains(view, id) { + t.Fatalf("session picker view should not expose raw id %q:\n%s", id, view) + } + } + + // IDs remain searchable even though they are hidden from normal rows. + searchByID := *next.picker + searchByID.query = first.SessionID + searchByID.applyQuery() + if len(searchByID.items) != 1 || searchByID.items[0].Value != first.SessionID { + t.Fatalf("hidden session id should remain searchable, got %#v", searchByID.items) + } + + // An empty result set reports an unambiguous zero position. + noResults := next + noResults.picker.query = "__missing_session__" + noResults.picker.applyQuery() + if view := viewString(noResults.View()); !strings.Contains(view, "0 / 0") { + t.Fatalf("empty session search should show 0 / 0:\n%s", view) + } +} + +func TestSessionPickerLabelAlignsTitles(t *testing.T) { + today := sessionPickerLabel("20:47:50", "Today title") + older := sessionPickerLabel("Jul 24 10:47", "Older title") + + todayColumn := strings.Index(today, "Today title") + olderColumn := strings.Index(older, "Older title") + if todayColumn != olderColumn { + t.Fatalf("title columns differ: today=%d (%q), older=%d (%q)", todayColumn, today, olderColumn, older) + } } func TestResumePickerSelectionHydratesSession(t *testing.T) { diff --git a/internal/tui/session.go b/internal/tui/session.go index 6d40e374d..94f887c6d 100644 --- a/internal/tui/session.go +++ b/internal/tui/session.go @@ -29,8 +29,13 @@ func (m model) ensureActiveSession(prompt string) (model, error) { return m, nil } + title := strings.TrimSpace(m.pendingSessionTitle) + manuallyNamed := title != "" + if !manuallyNamed { + title = tuiSessionTitle(prompt) + } session, err := m.sessionStore.Create(sessions.CreateInput{ - Title: tuiSessionTitle(prompt), + Title: title, Cwd: m.cwd, ModelID: m.modelName, Provider: m.providerName, @@ -39,7 +44,14 @@ func (m model) ensureActiveSession(prompt string) (model, error) { return m, err } m.activeSession = session + m.pendingSessionTitle = "" m.sessionEvents = []sessions.Event{} + if manuallyNamed { + if m.titledSessions == nil { + m.titledSessions = map[string]bool{} + } + m.titledSessions[session.SessionID] = true + } return m, nil } @@ -55,6 +67,7 @@ func (m model) startNewSession() model { previousID := m.activeSession.SessionID m.activeSession = sessions.Metadata{} + m.pendingSessionTitle = "" m.sessionEvents = nil // Reset the per-session usage + compaction display so the new session starts @@ -217,6 +230,7 @@ func (m model) handleResumeCommand(args string) (model, string) { // the already-active session, whose loops belong to it, not a "previous" one. previousID := m.activeSession.SessionID m.activeSession = *session + m.pendingSessionTitle = "" m.sessionEvents = append([]sessions.Event{}, events...) if m.providerName == "" { m.providerName = session.Provider @@ -392,17 +406,16 @@ func (m model) newSessionPicker() *commandPicker { if !m.sessionHasResumableContent(meta.SessionID) { continue } - // Lead with the timestamp so same-titled sessions (e.g. the same first - // prompt run several times) are visually distinct; the id (right, faint) - // stays for reference and is what selection actually resolves. + // Lead with a fixed-width timestamp so titles form one scannable column. + // The raw id remains the selection/search value but stays out of the row: + // rendering it consumed half the picker and truncated the useful title. label := displayValue(meta.Title, "untitled") if when := sessionWhen(meta.UpdatedAt, now); when != "" { - label = when + " " + label + label = sessionPickerLabel(when, label) } items = append(items, pickerItem{ Label: label, Value: meta.SessionID, - Meta: meta.SessionID, }) } if len(items) == 0 { @@ -417,6 +430,12 @@ func (m model) newSessionPicker() *commandPicker { } } +const sessionPickerTimeWidth = len("Jan 02 15:04") + +func sessionPickerLabel(when, title string) string { + return fmt.Sprintf("%-*s %s", sessionPickerTimeWidth, when, title) +} + // sessionHasResumableContent reports whether a session has anything worth // resuming: a tool call/result, or a non-user message with real content (not the // no-output guardrail stop). Empty/failed runs return false and are hidden from @@ -479,7 +498,7 @@ func (m model) sessionHasResumableContent(sessionID string) bool { // anything worth resuming: a tool call/result, or a non-user message with real // content (not the no-output guardrail stop). It is the pure core of // sessionHasResumableContent so callers that already hold the events (e.g. the -// /retitle scan) don't re-read them. +// session picker refresh) don't re-read them. func eventsHaveResumableContent(events []sessions.Event) bool { for _, event := range events { switch event.Type { diff --git a/internal/tui/session_rename.go b/internal/tui/session_rename.go new file mode 100644 index 000000000..ad1687c78 --- /dev/null +++ b/internal/tui/session_rename.go @@ -0,0 +1,107 @@ +package tui + +import ( + "fmt" + "strings" + + tea "charm.land/bubbletea/v2" + "charm.land/lipgloss/v2" +) + +type sessionRenamePrompt struct{} + +func (m model) openSessionRenamePrompt() model { + if m.sessionStore == nil { + return m.appendSessionRenameError("session storage is unavailable") + } + m.clearComposer() + title := m.activeSession.Title + if m.activeSession.SessionID == "" { + title = m.pendingSessionTitle + } + m.input.SetValue(title) + m.input.CursorEnd() + m.renamePrompt = &sessionRenamePrompt{} + return m +} + +func (m model) handleSessionRenameKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { + switch { + case keyIs(msg, tea.KeyEsc), keyCtrl(msg, 'c'): + m.renamePrompt = nil + m.clearComposer() + return m, nil + case keyIs(msg, tea.KeyEnter): + title := m.input.Value() + m.renamePrompt = nil + m.clearComposer() + return m.renameActiveSession(title), nil + default: + var cmd tea.Cmd + m.input, cmd = m.input.Update(msg) + return m, cmd + } +} + +func (m model) renameActiveSession(title string) model { + title = strings.TrimSpace(title) + if title == "" { + return m.appendSessionRenameError("session name cannot be empty") + } + if m.sessionStore == nil { + return m.appendSessionRenameError("session storage is unavailable") + } + if m.activeSession.SessionID == "" { + m.pendingSessionTitle = title + m.transcript = reduceTranscript(m.transcript, transcriptAction{ + kind: actionAppendSystem, + text: "Session renamed to " + title + ".", + }) + return m + } + updated, err := m.sessionStore.UpdateTitle(m.activeSession.SessionID, title) + if err != nil { + return m.appendSessionRenameError(err.Error()) + } + m.activeSession.Title = updated.Title + if m.titledSessions == nil { + m.titledSessions = map[string]bool{} + } + m.titledSessions[updated.SessionID] = true + m.transcript = reduceTranscript(m.transcript, transcriptAction{ + kind: actionAppendSystem, + text: "Session renamed to " + updated.Title + ".", + }) + return m +} + +func (m model) appendSessionRenameError(detail string) model { + m.transcript = reduceTranscript(m.transcript, transcriptAction{ + kind: actionAppendError, + text: fmt.Sprintf("Could not rename session: %s.", strings.TrimSuffix(strings.TrimSpace(detail), ".")), + }) + return m +} + +func (m model) sessionRenamePromptView(width int) string { + if width <= 0 { + width = defaultStartupWidth + } + input := m.input + input.Prompt = "> " + input.Placeholder = "Type a name" + innerWidth := maxInt(1, width-4) + line := renderComposerInput( + input, + composerState{text: input.Value(), cursor: input.Position()}, + innerWidth, + m.composerCursorVisible, + composerSelectionState{}, + ) + lines := []string{ + line, + zeroTheme.line.Render(strings.Repeat("─", innerWidth)), + zeroTheme.faint.Render("Enter save Esc cancel"), + } + return styledBlockFillTitle(width, "Rename session", lines, zeroTheme.lineStrong, lipgloss.NewStyle()) +} diff --git a/internal/tui/session_rename_test.go b/internal/tui/session_rename_test.go new file mode 100644 index 000000000..a739781d8 --- /dev/null +++ b/internal/tui/session_rename_test.go @@ -0,0 +1,152 @@ +package tui + +import ( + "context" + "strings" + "testing" + + tea "charm.land/bubbletea/v2" + + "github.com/Gitlawb/zero/internal/sessions" +) + +func renameTestModel(t *testing.T) (model, *sessions.Store, sessions.Metadata) { + t.Helper() + store := testSessionStore(t) + session, err := store.Create(sessions.CreateInput{Title: "Current session name"}) + if err != nil { + t.Fatalf("create session: %v", err) + } + m := newModel(context.Background(), Options{SessionStore: store}) + m.activeSession = session + return m, store, session +} + +func TestRenameCommandRenamesCurrentSessionWithoutAgentRun(t *testing.T) { + m, store, session := renameTestModel(t) + m.input.SetValue("/rename Better session name ") + + updated, cmd := m.Update(testKey(tea.KeyEnter)) + next := updated.(model) + if cmd != nil { + t.Fatal("/rename must not start an agent or background command") + } + if next.activeSession.Title != "Better session name" { + t.Fatalf("active title = %q", next.activeSession.Title) + } + stored, err := store.Get(session.SessionID) + if err != nil || stored == nil { + t.Fatalf("get renamed session: %v", err) + } + if stored.Title != "Better session name" { + t.Fatalf("stored title = %q", stored.Title) + } + if !next.titledSessions[session.SessionID] { + t.Fatal("manual rename should suppress later automatic naming") + } + if !transcriptContains(next.transcript, "Session renamed to Better session name.") { + t.Fatalf("missing rename confirmation: %#v", next.transcript) + } +} + +func TestBareRenameOpensPrefilledEditorAndSaves(t *testing.T) { + m, store, session := renameTestModel(t) + m.input.SetValue("/rename") + + updated, cmd := m.Update(testKey(tea.KeyEnter)) + next := updated.(model) + if cmd != nil || next.renamePrompt == nil { + t.Fatalf("bare /rename should open editor without a command: prompt=%#v cmd=%v", next.renamePrompt, cmd) + } + if next.input.Value() != "Current session name" { + t.Fatalf("editor value = %q", next.input.Value()) + } + view := plainRender(t, next.View()) + for _, want := range []string{"Rename session", "Current session name", "Enter save", "Esc cancel"} { + if !strings.Contains(view, want) { + t.Fatalf("rename editor missing %q:\n%s", want, view) + } + } + + next.input.SetValue("Edited in prompt") + updated, cmd = next.Update(testKey(tea.KeyEnter)) + next = updated.(model) + if cmd != nil || next.renamePrompt != nil { + t.Fatalf("saving should close editor without a command: prompt=%#v cmd=%v", next.renamePrompt, cmd) + } + stored, err := store.Get(session.SessionID) + if err != nil || stored == nil || stored.Title != "Edited in prompt" { + t.Fatalf("stored session after editor save = %#v, err=%v", stored, err) + } +} + +func TestRenameNamesFreshSessionBeforeFirstPrompt(t *testing.T) { + store := testSessionStore(t) + m := newModel(context.Background(), Options{SessionStore: store}) + m.input.SetValue("/rename Fresh session name") + + updated, cmd := m.Update(testKey(tea.KeyEnter)) + next := updated.(model) + if cmd != nil { + t.Fatal("fresh /rename must stay local") + } + if next.activeSession.SessionID != "" || next.pendingSessionTitle != "Fresh session name" { + t.Fatalf("fresh rename state = active:%q pending:%q", next.activeSession.SessionID, next.pendingSessionTitle) + } + + next, err := next.ensureActiveSession("first real prompt") + if err != nil { + t.Fatalf("ensure active session: %v", err) + } + if next.activeSession.Title != "Fresh session name" || next.pendingSessionTitle != "" { + t.Fatalf("created session title = %q, pending=%q", next.activeSession.Title, next.pendingSessionTitle) + } + if !next.titledSessions[next.activeSession.SessionID] { + t.Fatal("a manually named fresh session must skip automatic naming") + } + stored, err := store.Get(next.activeSession.SessionID) + if err != nil || stored == nil || stored.Title != "Fresh session name" { + t.Fatalf("stored fresh session = %#v, err=%v", stored, err) + } +} + +func TestRenameEditorRejectsBlankAndEscCancels(t *testing.T) { + m, store, session := renameTestModel(t) + m = m.openSessionRenamePrompt() + m.input.SetValue(" ") + + updated, _ := m.Update(testKey(tea.KeyEnter)) + next := updated.(model) + if next.renamePrompt != nil { + t.Fatal("blank submit should close the editor") + } + if !transcriptContains(next.transcript, "session name cannot be empty") { + t.Fatalf("missing blank-name error: %#v", next.transcript) + } + stored, _ := store.Get(session.SessionID) + if stored == nil || stored.Title != "Current session name" { + t.Fatalf("blank rename changed title: %#v", stored) + } + + next = next.openSessionRenamePrompt() + next.input.SetValue("Do not save") + updated, _ = next.Update(testKey(tea.KeyEsc)) + next = updated.(model) + if next.renamePrompt != nil || next.input.Value() != "" { + t.Fatalf("Esc should close and clear editor: prompt=%#v input=%q", next.renamePrompt, next.input.Value()) + } + stored, _ = store.Get(session.SessionID) + if stored == nil || stored.Title != "Current session name" { + t.Fatalf("cancelled rename changed title: %#v", stored) + } +} + +func TestRetitleCommandIsRemoved(t *testing.T) { + if command, ok := resolveCommand("/retitle"); ok { + t.Fatalf("/retitle should not resolve, got %#v", command) + } + command, ok := resolveCommand("/rename") + if !ok || command.kind != commandRename { + t.Fatalf("/rename should resolve, got ok=%v command=%#v", ok, command) + } +} diff --git a/internal/tui/session_title.go b/internal/tui/session_title.go index 3705ac1b4..45269dacd 100644 --- a/internal/tui/session_title.go +++ b/internal/tui/session_title.go @@ -3,7 +3,6 @@ package tui import ( "context" "errors" - "fmt" "strings" "time" @@ -42,12 +41,11 @@ const sessionTitleSystemPrompt = "You write a short, specific title for a coding var errSessionTitleNoContent = errors.New("session has no content to title") // sessionTitleGeneratedMsg carries the outcome of a background title generation -// back to the Update loop. backfill distinguishes a /retitle queue step (which -// advances the queue and updates a status row) from a silent auto-title. +// back to the Update loop. type sessionTitleGeneratedMsg struct { sessionID string title string - backfill bool + applied bool err error } @@ -168,32 +166,25 @@ func generateSessionTitle(ctx context.Context, provider zeroruntime.Provider, di } // generateSessionTitleCmd builds the background command that generates and -// persists a title for sessionID. When precomputedDigest is empty the command -// reads the session's events itself (the backfill path), keeping that I/O off the -// Update goroutine; the auto-title path passes the in-memory digest directly. -func (m model) generateSessionTitleCmd(sessionID string, precomputedDigest string, backfill bool) tea.Cmd { +// persists a title for sessionID if its original automatic title is still +// current. The compare-and-update keeps a manual /rename authoritative when it +// races a slow provider response. +func (m model) generateSessionTitleCmd(sessionID string, digest string) tea.Cmd { provider := m.provider store := m.sessionStore + originalTitle := m.activeSession.Title return func() tea.Msg { - digest := precomputedDigest - if strings.TrimSpace(digest) == "" { - events, err := store.ReadEvents(sessionID) - if err != nil { - return sessionTitleGeneratedMsg{sessionID: sessionID, backfill: backfill, err: err} - } - digest = sessionTitleDigest(events) - } ctx, cancel := context.WithTimeout(context.Background(), sessionTitleTimeout) defer cancel() title, err := generateSessionTitle(ctx, provider, digest) if err != nil { - return sessionTitleGeneratedMsg{sessionID: sessionID, backfill: backfill, err: err} + return sessionTitleGeneratedMsg{sessionID: sessionID, err: err} } - updated, err := store.UpdateTitle(sessionID, title) + updated, applied, err := store.UpdateTitleIfCurrent(sessionID, originalTitle, title) if err != nil { - return sessionTitleGeneratedMsg{sessionID: sessionID, title: title, backfill: backfill, err: err} + return sessionTitleGeneratedMsg{sessionID: sessionID, title: title, err: err} } - return sessionTitleGeneratedMsg{sessionID: sessionID, title: updated.Title, backfill: backfill} + return sessionTitleGeneratedMsg{sessionID: sessionID, title: updated.Title, applied: applied} } } @@ -256,87 +247,24 @@ func (m model) maybeAutoTitleActiveSession() (model, tea.Cmd) { m.titledSessions = map[string]bool{} } m.titledSessions[sessionID] = true - return m, m.generateSessionTitleCmd(sessionID, digest, false) -} - -// startSessionRetitle scans resumable sessions for ones still carrying their -// default first-message title and queues a model-generated title for each, -// firing them one at a time. It returns a status line for the transcript. -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.retitleActive { - return m, nil, fmt.Sprintf("Already generating titles (%d/%d). Let it finish first.", m.retitleDone, m.retitleTotal) - } - list, err := m.sessionStore.ListResumable() - if err != nil { - return m, nil, "Sessions\nFailed to list sessions: " + err.Error() - } - candidates := make([]string, 0, len(list)) - for _, session := range list { - events, err := m.sessionStore.ReadEvents(session.SessionID) - if err != nil { - continue - } - if !eventsHaveResumableContent(events) { - continue // empty/failed run — nothing worth titling - } - if !sessionTitleIsAuto(session.Title, events) { - continue // already has a model-generated title - } - candidates = append(candidates, session.SessionID) - } - if len(candidates) == 0 { - return m, nil, "All resumable sessions already have a generated title." - } - if m.titledSessions == nil { - m.titledSessions = map[string]bool{} - } - for _, id := range candidates { - m.titledSessions[id] = true - } - m.retitleQueue = append([]string(nil), candidates[1:]...) - m.retitleActive = true - m.retitleTotal = len(candidates) - m.retitleDone = 0 - m.retitleOK = 0 - cmd := m.generateSessionTitleCmd(candidates[0], "", true) - return m, cmd, fmt.Sprintf("Generating titles for %d session(s)… this runs in the background.", len(candidates)) + return m, m.generateSessionTitleCmd(sessionID, digest) } -// handleSessionTitleGenerated applies a finished title and, for the /retitle -// backfill, advances the sequential queue and reports completion. +// handleSessionTitleGenerated applies a finished automatic title to the active +// model. The store update has already been skipped when a manual rename won. func (m model) handleSessionTitleGenerated(msg sessionTitleGeneratedMsg) (model, tea.Cmd) { - titleOK := msg.err == nil && msg.title != "" + titleOK := msg.err == nil && msg.title != "" && msg.applied if titleOK { if msg.sessionID == m.activeSession.SessionID { m.activeSession.Title = msg.title } - } else { + } else if msg.err != nil || msg.title == "" { // titledSessions is marked optimistically when the cmd is scheduled (so a // second turn can't double-fire a title for the same session while the // first is in flight). A FAILED generation — provider error, empty title, // or store write error — must not leave that gate set forever, so release - // it here; a later turn or /retitle can then retry. Success keeps the gate. + // it here so a later turn can retry. Success keeps the gate. delete(m.titledSessions, msg.sessionID) } - if !msg.backfill { - // Auto-title is silent: on failure the first-message title simply stays - // (and the retry gate above was released). - return m, nil - } - m.retitleDone++ - if titleOK { - m.retitleOK++ - } - if len(m.retitleQueue) > 0 { - next := m.retitleQueue[0] - m.retitleQueue = m.retitleQueue[1:] - return m, m.generateSessionTitleCmd(next, "", true) - } - m.retitleActive = false - summary := fmt.Sprintf("Generated titles for %d of %d session(s). Open /resume to see them.", m.retitleOK, m.retitleTotal) - m.transcript = appendTranscriptRow(m.transcript, transcriptRow{kind: rowSystem, tool: "sessions", text: summary}) return m, nil } diff --git a/internal/tui/session_title_test.go b/internal/tui/session_title_test.go index 02279edc5..1df48e51d 100644 --- a/internal/tui/session_title_test.go +++ b/internal/tui/session_title_test.go @@ -6,8 +6,6 @@ import ( "strings" "testing" - tea "charm.land/bubbletea/v2" - "github.com/Gitlawb/zero/internal/sessions" "github.com/Gitlawb/zero/internal/zeroruntime" ) @@ -32,7 +30,7 @@ func appendSessionMessage(t *testing.T, store *sessions.Store, id, role, content } // createAutoTitledSession creates a real, resumable session whose Title is the -// default first-message title (so it is a retitle/auto-title candidate). +// default first-message title (so it is an auto-title candidate). func createAutoTitledSession(t *testing.T, store *sessions.Store, prompt, answer string) sessions.Metadata { t.Helper() session, err := store.Create(sessions.CreateInput{Title: tuiSessionTitle(prompt)}) @@ -179,7 +177,7 @@ func TestAutoTitleGeneratesTitleForActiveSession(t *testing.T) { if !ok { t.Fatalf("expected sessionTitleGeneratedMsg, got %#v", msg) } - if result.err != nil || result.backfill { + if result.err != nil || !result.applied { t.Fatalf("unexpected auto-title result: %#v", result) } if result.title != "Add Fetch Call To Client" { @@ -268,82 +266,47 @@ func TestAutoTitleFailureReleasesRetryGate(t *testing.T) { // A successful generation keeps the session gated (one-shot, no re-fire). ok := newModel(context.Background(), Options{}) ok.titledSessions = map[string]bool{id: true} - ok, _ = ok.handleSessionTitleGenerated(sessionTitleGeneratedMsg{sessionID: id, title: "Real Title"}) + ok, _ = ok.handleSessionTitleGenerated(sessionTitleGeneratedMsg{sessionID: id, title: "Real Title", applied: true}) if !ok.titledSessions[id] { t.Fatal("a successful title generation must keep the session gated") } } -func TestRetitleBackfillTitlesOnlyAutoTitledSessions(t *testing.T) { +func TestManualRenameWinsOverInFlightAutoTitle(t *testing.T) { store := testSessionStore(t) - first := createAutoTitledSession(t, store, "build the resume picker", "Working on it.") - second := createAutoTitledSession(t, store, "fix the dst bug", "Fixed.") - - // An empty/failed run: user prompt + the no-output guardrail stop. Skipped. - empty, err := store.Create(sessions.CreateInput{Title: tuiSessionTitle("do a thing")}) - if err != nil { - t.Fatalf("create empty: %v", err) - } - appendSessionMessage(t, store, empty.SessionID, "user", "do a thing") - appendSessionMessage(t, store, empty.SessionID, "assistant", - "Agent stopped after 3 turns with no output (no visible text and no tool calls) to avoid consuming tokens without making progress.") - - // A session that already has a distinct (hand/model) title. Skipped. - named, err := store.Create(sessions.CreateInput{Title: "Hand Named Session"}) - if err != nil { - t.Fatalf("create named: %v", err) - } - appendSessionMessage(t, store, named.SessionID, "user", "something") - appendSessionMessage(t, store, named.SessionID, "assistant", "ok") + session := createAutoTitledSession(t, store, "build the resume picker", "Working on it.") m := newModel(context.Background(), Options{ SessionStore: store, - Provider: titleProvider("Generated Backfill Title"), + Provider: titleProvider("Generated Automatic Title"), }) - m.input.SetValue("/retitle") - updated, cmd := m.Update(testKey(tea.KeyEnter)) - m = updated.(model) - if !m.retitleActive { - t.Fatal("expected a backfill to be active") - } - if m.retitleTotal != 2 { - t.Fatalf("retitle total = %d, want 2 (only auto-titled real sessions)", m.retitleTotal) - } - if !transcriptContains(m.transcript, "Generating titles for 2 session") { - t.Fatalf("expected a kickoff status row, got %#v", m.transcript) + m.activeSession = session + events, err := store.ReadEvents(session.SessionID) + if err != nil { + t.Fatalf("read events: %v", err) } + m.sessionEvents = events - // Drain the sequential queue. - guard := 0 - for cmd != nil { - guard++ - if guard > 10 { - t.Fatal("retitle queue did not drain") - } - msg := execCmd(cmd) - updated, cmd = m.Update(msg) - m = updated.(model) - } - if m.retitleActive { - t.Fatal("backfill should be finished after the queue drains") + _, cmd := m.maybeAutoTitleActiveSession() + if cmd == nil { + t.Fatal("expected an automatic title command") } - if !transcriptContains(m.transcript, "Generated titles for 2 of 2") { - t.Fatalf("expected a completion status row, got %#v", m.transcript) + if _, err := store.UpdateTitle(session.SessionID, "Manual Name"); err != nil { + t.Fatalf("manual rename: %v", err) } - for _, id := range []string{first.SessionID, second.SessionID} { - got, err := store.Get(id) - if err != nil || got == nil { - t.Fatalf("get %s: %v", id, err) - } - if got.Title != "Generated Backfill Title" { - t.Fatalf("session %s title = %q, want generated", id, got.Title) - } + result, ok := execCmd(cmd).(sessionTitleGeneratedMsg) + if !ok { + t.Fatal("expected a title result") } - if got, _ := store.Get(named.SessionID); got == nil || got.Title != "Hand Named Session" { - t.Fatalf("a named session must keep its title, got %#v", got) + if result.err != nil || result.applied { + t.Fatalf("late automatic title should be skipped, got %#v", result) + } + got, err := store.Get(session.SessionID) + if err != nil || got == nil { + t.Fatalf("get session: %v", err) } - if got, _ := store.Get(empty.SessionID); got == nil || got.Title != tuiSessionTitle("do a thing") { - t.Fatalf("an empty session must be skipped and keep its title, got %#v", got) + if got.Title != "Manual Name" { + t.Fatalf("late automatic title overwrote manual name: %q", got.Title) } } diff --git a/internal/tui/sidebar.go b/internal/tui/sidebar.go index 13b87a14b..7eb1cd45b 100644 --- a/internal/tui/sidebar.go +++ b/internal/tui/sidebar.go @@ -65,7 +65,7 @@ func (m model) sidebarToggleAllowed() bool { return false } if m.setup.visible || m.providerWizard != nil || m.mcpAddWizard != nil || - m.mcpManager != nil || m.picker != nil || m.suggestionsActive() { + m.mcpManager != nil || m.picker != nil || m.renamePrompt != nil || m.suggestionsActive() { return false } // Home/welcome screen: stay single-column until there's real conversation. @@ -113,7 +113,7 @@ func (m model) sidebarAvailable() bool { // suggestionsActive() guard, so clicks still go to the palette and not to the // sidebar rows underneath it. if m.setup.visible || m.helpOverlay || m.leaderHelpOverlay || m.providerWizard != nil || m.mcpAddWizard != nil || - m.mcpManager != nil || m.picker != nil { + m.mcpManager != nil || m.picker != nil || m.renamePrompt != nil { return false } // Home/welcome screen: stay single-column until there's real conversation, so diff --git a/internal/tui/transcript_selection.go b/internal/tui/transcript_selection.go index 8a70bf08e..297acb8fc 100644 --- a/internal/tui/transcript_selection.go +++ b/internal/tui/transcript_selection.go @@ -1125,7 +1125,8 @@ func (m model) transcriptHitTestSource() (header string, items []transcriptBodyI // transcriptHitTestBlocked reports whether mouse hit-testing must be skipped // outright — a modal/overlay is up, or there's no alt-screen viewport at all. func (m model) transcriptHitTestBlocked() bool { - return !m.altScreen || m.height <= 0 || m.setup.visible || m.providerWizard != nil || m.mcpAddWizard != nil || m.mcpManager != nil || m.picker != nil || m.suggestionsActive() + return !m.altScreen || m.height <= 0 || m.setup.visible || m.providerWizard != nil || m.mcpAddWizard != nil || + m.mcpManager != nil || m.picker != nil || m.renamePrompt != nil || m.suggestionsActive() } // transcriptHitTestLayout computes the frame/window/layout mouse hit-testing needs, diff --git a/internal/tui/view.go b/internal/tui/view.go index 7d8e472c8..44c9e3df1 100644 --- a/internal/tui/view.go +++ b/internal/tui/view.go @@ -813,7 +813,16 @@ func (m model) pickerOverlay(width int) string { // Hints live in the footer (a separator + faint keys), matching the /model // picker and the other bordered boxes. lines = append(lines, zeroTheme.line.Render(strings.Repeat("─", innerWidth))) - lines = append(lines, zeroTheme.faint.Render("↑/↓ move Enter select Esc close")) + footer := zeroTheme.faint.Render("↑/↓ move Enter select Esc close") + if m.picker.kind == pickerSession { + position := 0 + if len(m.picker.items) > 0 { + position = clampInt(m.picker.selected, 0, len(m.picker.items)-1) + 1 + } + count := zeroTheme.faint.Render(fmt.Sprintf("%d / %d", position, len(m.picker.items))) + footer = joinHeaderLine(footer, count, innerWidth) + } + lines = append(lines, footer) return centerRenderedBlock(styledBlockFillTitle(overlayWidth, title, lines, zeroTheme.lineStrong, lipgloss.NewStyle()), width) } From 36f16b7483c0c98e63f5a1a093e18614f6623658 Mon Sep 17 00:00:00 2001 From: anandh8x Date: Wed, 29 Jul 2026 22:25:12 +0530 Subject: [PATCH 2/2] Address session rename review feedback --- internal/tui/model_test.go | 3 +++ internal/tui/session_rename.go | 7 +++--- internal/tui/session_rename_test.go | 38 +++++++++++++++++++++++++++++ internal/tui/session_title_test.go | 7 +++++- 4 files changed, 50 insertions(+), 5 deletions(-) diff --git a/internal/tui/model_test.go b/internal/tui/model_test.go index 451d9d79e..8f84628d8 100644 --- a/internal/tui/model_test.go +++ b/internal/tui/model_test.go @@ -945,6 +945,9 @@ func TestSessionPickerLabelAlignsTitles(t *testing.T) { todayColumn := strings.Index(today, "Today title") olderColumn := strings.Index(older, "Older title") + if todayColumn < 0 || olderColumn < 0 { + t.Fatalf("sessionPickerLabel omitted a title: today=%q older=%q", today, older) + } if todayColumn != olderColumn { t.Fatalf("title columns differ: today=%d (%q), older=%d (%q)", todayColumn, today, olderColumn, older) } diff --git a/internal/tui/session_rename.go b/internal/tui/session_rename.go index ad1687c78..f08b9e90d 100644 --- a/internal/tui/session_rename.go +++ b/internal/tui/session_rename.go @@ -44,7 +44,7 @@ func (m model) handleSessionRenameKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { } func (m model) renameActiveSession(title string) model { - title = strings.TrimSpace(title) + title = cutRunes(strings.TrimSpace(title), tuiSessionTitleLimit) if title == "" { return m.appendSessionRenameError("session name cannot be empty") } @@ -98,10 +98,9 @@ func (m model) sessionRenamePromptView(width int) string { m.composerCursorVisible, composerSelectionState{}, ) - lines := []string{ - line, + lines := append(strings.Split(line, "\n"), zeroTheme.line.Render(strings.Repeat("─", innerWidth)), zeroTheme.faint.Render("Enter save Esc cancel"), - } + ) return styledBlockFillTitle(width, "Rename session", lines, zeroTheme.lineStrong, lipgloss.NewStyle()) } diff --git a/internal/tui/session_rename_test.go b/internal/tui/session_rename_test.go index a739781d8..db16bca77 100644 --- a/internal/tui/session_rename_test.go +++ b/internal/tui/session_rename_test.go @@ -49,6 +49,44 @@ func TestRenameCommandRenamesCurrentSessionWithoutAgentRun(t *testing.T) { } } +func TestRenameCommandCapsSessionTitle(t *testing.T) { + m, store, session := renameTestModel(t) + longTitle := strings.Repeat("界", tuiSessionTitleLimit+20) + m.input.SetValue("/rename " + longTitle) + + updated, cmd := m.Update(testKey(tea.KeyEnter)) + next := updated.(model) + if cmd != nil { + t.Fatal("/rename must not start an agent or background command") + } + want := cutRunes(longTitle, tuiSessionTitleLimit) + if next.activeSession.Title != want { + t.Fatalf("active title has %d runes, want %d", len([]rune(next.activeSession.Title)), tuiSessionTitleLimit) + } + stored, err := store.Get(session.SessionID) + if err != nil || stored == nil || stored.Title != want { + t.Fatalf("stored capped title = %#v, err=%v", stored, err) + } +} + +func TestRenameEditorBordersWrappedTitleLines(t *testing.T) { + m, _, _ := renameTestModel(t) + m.input.SetValue(strings.Repeat("x", tuiSessionTitleLimit)) + m.input.CursorEnd() + m.renamePrompt = &sessionRenamePrompt{} + + view := plainRender(t, m.sessionRenamePromptView(80)) + lines := strings.Split(view, "\n") + if len(lines) < 6 { + t.Fatalf("expected wrapped rename editor, got:\n%s", view) + } + for _, line := range lines[1 : len(lines)-1] { + if !strings.HasPrefix(line, "│ ") || !strings.HasSuffix(line, " │") { + t.Fatalf("wrapped editor row lost its border: %q\n%s", line, view) + } + } +} + func TestBareRenameOpensPrefilledEditorAndSaves(t *testing.T) { m, store, session := renameTestModel(t) m.input.SetValue("/rename") diff --git a/internal/tui/session_title_test.go b/internal/tui/session_title_test.go index 1df48e51d..1790fe2fa 100644 --- a/internal/tui/session_title_test.go +++ b/internal/tui/session_title_test.go @@ -287,7 +287,7 @@ func TestManualRenameWinsOverInFlightAutoTitle(t *testing.T) { } m.sessionEvents = events - _, cmd := m.maybeAutoTitleActiveSession() + m, cmd := m.maybeAutoTitleActiveSession() if cmd == nil { t.Fatal("expected an automatic title command") } @@ -309,4 +309,9 @@ func TestManualRenameWinsOverInFlightAutoTitle(t *testing.T) { 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 automatic title result must keep the retry gate set") + } }