refactor(app): expose session state as value snapshots (#101) - #104
Conversation
The TUI reached through AppController.GetTreeSession() to grab the live
*session.TreeManager and drove it directly: reading the header, walking
the branch, appending entries, forking, and type-switching on the
concrete *session.MessageEntry / *session.ModelChangeEntry / ... entry
types to decide how to filter and render the tree. That made the session
storage schema part of the UI's contract — any change to how entries are
persisted broke rendering, and the UI could not be exercised or reused
without a real session on disk.
Session state is now projected into immutable value types owned by the
app layer:
- SessionSnapshot carries session metadata (id, name, file path, cwd,
created, leaf, counts, persisted).
- TreeNodeView carries a node's id, parent, kind, role, text payload,
label and children. EntryKind replaces the concrete-type switches,
so an unrecognised entry degrades to a fallback instead of breaking.
- SessionHistory returns the current branch as []message.Message, so
transcript replay no longer decodes entries itself.
Mutations move behind intent-revealing methods (SetSessionName,
NewSession, ForkSession, SessionSystemPromptEntry) instead of the UI
constructing sessions and swapping them in via SwitchTreeSession.
GetTreeSession and SwitchTreeSession are dropped from AppController;
App keeps both for cmd/root.go wiring, which owns session lifecycle.
TreeNodeSelectedMsg now carries ParentID directly rather than the raw
entry, removing the entry lookup and type assertion the fork path did to
find where to branch from.
Behaviour is unchanged: filters, display strings, colours, export/share
filenames and fork semantics all match the previous implementation, now
covered by tests for the projections and for tree filtering/rendering.
The session selector still calls session.ListSessions/DeleteSession
directly; moving session discovery and the export/share file I/O out of
the UI is the next step.
Refs #101
|
Connected to Huly®: KIT-105 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change moves session metadata, tree projection, history decoding, and session mutations into ChangesSession state decoupling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant UIModel
participant AppController
participant TreeManager
participant SessionFile
UIModel->>AppController: request session snapshot or tree
AppController->>TreeManager: read or mutate session state
TreeManager->>SessionFile: persist session changes
AppController-->>UIModel: return projected session data
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@CodeRabbit review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/ui/model.go (1)
104-128: 📐 Maintainability & Code Quality | 🔵 TrivialNew
NewSession/ForkSession/SessionSystemPromptEntryinterface methods lackcontext.Context.Per the app-layer implementations (
session_view.go),NewSessionandForkSessioncallsession.CreateTreeSession/tm.ForkToNewSession, which perform disk I/O to create/write session files. These are brand-new interface methods introduced in this PR, so this is the cheapest point to add acontext.Contextfirst parameter for cancellation/timeout support, per the repo's Go guideline.As per coding guidelines, "Use
Contextas the first parameter for blocking operations in Go."♻️ Proposed signature changes
- NewSession(cwd string) error + NewSession(ctx context.Context, cwd string) error // ForkSession creates a new session in cwd holding the history up to // targetID and switches to it. Used by /fork and tree-node selection. - ForkSession(cwd, targetID string) error + ForkSession(ctx context.Context, cwd, targetID string) error🤖 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/model.go` around lines 104 - 128, Add context.Context as the first parameter to NewSession, ForkSession, and SessionSystemPromptEntry in the app-layer interface and corresponding implementations. Update all callers to pass the active context, and propagate it through session_view.go to the underlying session creation, fork, and serialization operations without changing their existing behavior.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/app/session_view.go`:
- Around line 228-229: Wrap the propagated errors in
internal/app/session_view.go at lines 228-229, 243-245, 259-261, and 283-283:
add contextual fmt.Errorf wrapping for failures from AppendSessionInfo,
CreateTreeSession, ForkToNewSession, and MarshalEntry respectively, preserving
the original errors with %w.
In `@internal/ui/model.go`:
- Around line 5813-5817: Update AppModel.renderSessionHistory so an empty
SessionHistory still clears the current message list and runs refreshContent,
marks layoutDirty, and sets pendingGotoBottom before returning or completing.
Preserve the existing history-rendering behavior for non-empty sessions,
ensuring SessionSelectedMsg and performFork cannot leave the previous transcript
visible.
---
Nitpick comments:
In `@internal/ui/model.go`:
- Around line 104-128: Add context.Context as the first parameter to NewSession,
ForkSession, and SessionSystemPromptEntry in the app-layer interface and
corresponding implementations. Update all callers to pass the active context,
and propagate it through session_view.go to the underlying session creation,
fork, and serialization operations without changing their existing behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 47e33840-0d11-455b-9a57-84c4fd85fea5
📒 Files selected for processing (8)
internal/app/session_view.gointernal/app/session_view_test.gointernal/session/entry.gointernal/ui/core/events.gointernal/ui/model.gointernal/ui/model_test.gointernal/ui/tree_selector.gointernal/ui/tree_selector_test.go
- renderSessionHistory no longer returns early on an empty history, which left the previous conversation on screen. The old code returned when the branch had no *entries*; the refactor returned when it had no *messages*, so a branch holding only non-message entries stopped clearing. This is reachable from /retry and /undo (both pop the last user message and can empty the branch), and from forking or resuming into an empty session. The no-session guard is preserved via SessionSnapshot so an in-memory run's transcript is still left untouched. - Wrap propagated errors in SetSessionName, NewSession, ForkSession and SessionSystemPromptEntry with fmt.Errorf/%w so failures are diagnosable across the app/UI boundary. Skipped: adding context.Context to NewSession/ForkSession/ SessionSystemPromptEntry. Nothing beneath them accepts a context — internal/session performs synchronous file I/O with no cancellation — and no method on AppController takes one, so the parameter would be ignored and would advertise cancellation that does not exist. Threading context through the session layer is its own change. Refs #101
|
Thanks — addressed the two actionable findings in 61a94eb. Fixed 🟠 One deviation from the proposed diff: it clears unconditionally, which would drop the Fixed 🟡 error wrapping at all four sites, applied individually rather than from the concatenated suggestion block. Skipped 🔵 |
1 participant
Step (b) of the #101 refactor. Follows #102 (lazy terminal capability detection) and #103 (sealed
app.Eventunion).Problem
internal/uireached throughAppController.GetTreeSession()to get the live*session.TreeManagerand drove it directly — reading the header, walking the branch, appending entries, forking sessions — and type-switched on the concrete persistence types (*session.MessageEntry,*session.ModelChangeEntry,*session.BranchSummaryEntry, ...) to decide how to filter and render the session tree.That made the on-disk entry schema part of the UI's contract. Changing how entries are persisted broke tree rendering, and none of the tree/session UI could be exercised without a real session on disk.
Change
Session state is projected into immutable value types owned by the app layer (
internal/app/session_view.go):SessionSnapshotTreeNodeViewEntryKind, role, text payload, label, childrenEntryKindPlus
SessionHistory() []message.Messagefor transcript replay, so the UI no longer decodes entries itself.Mutations move behind intent-revealing methods rather than the UI building sessions and swapping them in:
GetTreeSession/SwitchTreeSessionare dropped fromAppController.Appkeeps both —cmd/root.goowns session lifecycle and legitimately needs the manager.TreeNodeSelectedMsgnow carriesParentIDinstead of the rawEntry any, removing the entry lookup + type assertion the fork path used to find its branch point. This also keepsinternal/ui/corefree of anyinternal/appdependency.Notes on scope
EntryKindUnknownmeans a new entry type degrades to the(unknown entry)fallback instead of breaking the build or rendering blank.sanitizeFileName, andsnap.ID[:12]is nowshortSessionID(the oldGetSessionID()[:12]would panic on a short id).MessageEntry.Text()moved the parts-JSON extraction intointernal/session, where the format is defined — it was previously duplicated in the UI asextractTextFromParts.SessionSummarywas originally sketched for this step but moved to step (c): its only producer isListSessions/ListAllSessions, so introducing it here would mean touchingsession_selector.gotwice.Remaining for step (c)
session_selector.gostill callssession.ListSessions/ListAllSessions/DeleteSessiondirectly, and/export+/sharestill do their ownos.ReadFile/os.WriteFile. Moving those out removes theinternal/sessionimport frominternal/uientirely — that PR carriesFixes #101.Testing
internal/app/session_view_test.go— snapshot fields, detachment (a held snapshot/projection is not mutated by later session writes), entry-kind projection, unknown-entry fallback, history filtering to messages only,ErrNoSessionon every mutation without a session, system-prompt entry model resolution + fallback.internal/ui/tree_selector_test.go— display text for every kind, truncation, untruncated user text for the editor, a full filter matrix across all 5 modes × 9 node shapes, tree flattening/depth, and cursor placement for/treeand/fork. No tree-selector tests existed before.go build,go vet,gofmt -l,go test -race ./...all clean;golangci-lint runreports 0 issues./session,/tree(all 5 filter modes, labels, active marker, nesting),/fork(branches at the parent, replays history, repopulates the editor),/name(read + write),/export(filename sanitising),/new. Empty stderr throughout.Refs #101
Summary by CodeRabbit
New Features
Bug Fixes
Tests