refactor(ui): move session discovery and file I/O to app layer - #105
Conversation
The session picker was the last part of the TUI that reached into internal/session directly: it listed sessions, deleted their files and type-asserted on session.SessionInfo. /export and /share likewise read and wrote session JSONL themselves, so knowledge of the on-disk format was spread across the presentation layer. Session discovery now returns app.SessionSummary values, and the two file-producing commands are expressed as app-layer operations that return sentinel errors the UI maps to its own messages. The picker takes a narrow SessionStore port rather than calling the package directly, which also makes its scope, filter and delete flows testable without touching the filesystem. With this, internal/ui no longer imports internal/session at all and the session file format is owned entirely by the app layer. SessionSystemPromptEntry is folded into WriteShareableSession; it was introduced only as a step towards moving the share path, and the UI no longer needs to know that shared files embed a system-prompt entry. Fixes #101
|
Connected to Huly®: KIT-106 |
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe app layer now owns session listing, deletion, export, and shareable-file creation. The UI delegates these operations through ChangesApp session queries and deletion
App export and share generation
UI integration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant User
participant TUI
participant App
participant SessionFile
User->>TUI: Run /export or /share
TUI->>App: ExportSession or WriteShareableSession
App->>SessionFile: Read persisted session JSONL
App->>App: Copy or splice system-prompt entry
App-->>TUI: Return generated path
TUI-->>User: Display session file path
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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: 1
🧹 Nitpick comments (2)
internal/app/session_store.go (1)
110-130: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider atomic write for
ExportSessionto avoid partial files on failure.
os.WriteFile(dstPath, data, 0o644)can leave a truncated/partial file atdstPathif the write fails partway (e.g. disk full). Writing to a temp file in the same directory and renaming into place would make the export atomic.🤖 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/app/session_store.go` around lines 110 - 130, Update ExportSession to write the session data to a temporary file in the destination directory, close and validate the temporary file, then atomically rename it to dstPath; remove the temporary file on failures and preserve the existing error wrapping and return values.internal/ui/session_selector.go (1)
106-135: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider logging swallowed session-listing errors.
store.ListSessions/store.ListAllSessionserrors are discarded silently, so a permissions/disk failure looks identical to "no sessions" with zero operator visibility. As per coding guidelines,**/*.goshould usegithub.com/charmbracelet/logfor structured logging; a one-linelog.Warnwhenerr != nilwould preserve today's graceful-degradation UX while giving debuggability.♻️ Proposed logging addition
if cwd != "" { - ss.cwdSessions, _ = store.ListSessions(cwd) + var err error + ss.cwdSessions, err = store.ListSessions(cwd) + if err != nil { + log.Warn("list sessions for cwd failed", "cwd", cwd, "err", err) + } ss.scope = SessionScopeCwd } - ss.allSessions, _ = store.ListAllSessions() + if all, err := store.ListAllSessions(); err != nil { + log.Warn("list all sessions failed", "err", err) + } else { + ss.allSessions = all + }🤖 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/session_selector.go` around lines 106 - 135, Update NewSessionSelector to capture errors from both store.ListSessions and store.ListAllSessions, logging each non-nil error with a one-line structured charmbracelet/log Warn call while preserving the existing empty-list fallback and scope 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_store.go`:
- Around line 197-240: Preserve the created temp path on all write failures in
writeShareFile so the deferred cleanup can remove it. Replace error returns that
explicitly return an empty path with returns that retain the named tmpPath while
still propagating the wrapped error; keep the successful return and existing
cleanup behavior unchanged. Add a regression test only if the surrounding test
structure supports forcing a write failure and verifying the temporary file is
removed.
---
Nitpick comments:
In `@internal/app/session_store.go`:
- Around line 110-130: Update ExportSession to write the session data to a
temporary file in the destination directory, close and validate the temporary
file, then atomically rename it to dstPath; remove the temporary file on
failures and preserve the existing error wrapping and return values.
In `@internal/ui/session_selector.go`:
- Around line 106-135: Update NewSessionSelector to capture errors from both
store.ListSessions and store.ListAllSessions, logging each non-nil error with a
one-line structured charmbracelet/log Warn call while preserving the existing
empty-list fallback and scope 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: 2986b841-d88a-4b16-a5fd-93220d83399e
📒 Files selected for processing (8)
internal/app/session_store.gointernal/app/session_store_test.gointernal/app/session_view.gointernal/app/session_view_test.gointernal/ui/model.gointernal/ui/model_test.gointernal/ui/session_selector.gointernal/ui/session_selector_test.go
💤 Files with no reviewable changes (2)
- internal/app/session_view.go
- internal/app/session_view_test.go
- writeShareFile leaked its temp file on every write failure. tmpPath was
a named result, so `return "", err` zeroed it before the deferred
cleanup ran and os.Remove("") removed nothing. Split the function so
the open file is passed to finishShareFile and the path is held in a
local: the named-result/defer interaction that caused the leak is now
structurally absent rather than worked around. Close is checked too, so
a write that only fails on flush is no longer reported as success.
- Log session-listing failures in the picker instead of discarding them;
a permissions or disk error was indistinguishable from "no sessions".
Skipped: atomic temp-and-rename for ExportSession. os.WriteFile writes
through a symlinked destination whereas rename replaces it, so this
changes behaviour beyond the refactor and is better decided separately.
Review follow-up (07f0c84)Both actionable findings addressed; one nitpick intentionally skipped. 🟠 Major — temp file leak in 🔵 Nitpick — log swallowed listing errors — applied. Verified this is safe here before doing it: 🔵 Nitpick — atomic write for The concern is legitimate, but temp-and-rename is not behaviour-preserving: The blast radius also argues for patience: unlike the session file itself, an export is a user-initiated copy to a user-chosen path — a partial write is immediately visible and retried with one command. Happy to do it as a follow-up if you want it; it just deserves its own decision about the symlink semantics rather than riding along in a refactor. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/ui/session_selector.go (1)
65-77: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftPropagate
context.Contextthrough session-store I/O.
SessionStore.ListSessions,ListAllSessions, andDeleteSessionultimately call filesystem-backed app/session operations (internal/session/store.go), but the injected API and callers provide no cancellation. Addcontext.Contextas the first parameter at each layer: the selector constructor/interface,NewSessionSelector/reloads/deletes,internal/app/session_store.go, the app controller contract/stubs, and test stubs.🤖 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/session_selector.go` around lines 65 - 77, Propagate context.Context as the first parameter through the session-store I/O chain: update SessionStore methods and NewSessionSelector, reload/delete call sites, internal app session-store functions, the app controller contract and stubs, and all test stubs. Pass the same context into the underlying filesystem-backed operations in internal/session/store.go, preserving existing behavior aside from cancellation support.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_store_test.go`:
- Around line 565-567: Update the doc comment above
TestWriteShareableSessionCleansUpAfterCaller to reference the function’s current
name instead of TestWriteShareableSessionLeavesNoTempFilesOnSuccess, without
changing the test behavior.
---
Outside diff comments:
In `@internal/ui/session_selector.go`:
- Around line 65-77: Propagate context.Context as the first parameter through
the session-store I/O chain: update SessionStore methods and NewSessionSelector,
reload/delete call sites, internal app session-store functions, the app
controller contract and stubs, and all test stubs. Pass the same context into
the underlying filesystem-backed operations in internal/session/store.go,
preserving existing behavior aside from cancellation support.
🪄 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: 7a8cb8ac-6264-489e-b950-68ffdc4a6381
📒 Files selected for processing (3)
internal/app/session_store.gointernal/app/session_store_test.gointernal/ui/session_selector.go
The comment named a function that had been renamed. Renamed the test to match what it asserts instead: a successful share leaves exactly the returned file behind, since removing it is the caller's job.
|
@CodeRabbit review |
✅ Action performedReview finished.
|
Re:
|
1 participant
Final step of the #101 refactor. Builds on #102 (terminal capabilities), #103 (sealed event union) and #104 (session snapshots).
Problem
After #104 the UI stopped reaching into the session tree, but three things still tied it to the persistence layer:
session.ListSessions/ListAllSessions, deleted files withsession.DeleteSession, and type-asserted onsession.SessionInfoin five places. It was the lastinternal/sessionimport left ininternal/ui./exportderived its file name, read the source JSONL and wrote the copy itself./shareread the session file, marshalled a system-prompt entry and spliced it in after the header — i.e. the UI encoded the shared-file layout.Change
internal/app/session_store.gotakes ownership of session discovery and the two file-producing commands:SessionSummarysession.SessionInfoin the UIListSessions/ListAllSessions/DeleteSessioninternal/sessioncallsExportSession(dstPath) (string, int, error)os.ReadFile+os.WriteFileinhandleExportCommandWriteShareableSession(systemPrompt, fallbackModelID)os.ReadFile+SessionSystemPromptEntry+buildShareFileinhandleShareCommandErrSessionNotPersistedjoinsErrNoSessionas a sentinel, so the UI keeps its distinct "no session" vs "in-memory" wording without inspecting a snapshot field to infer it.sanitizeFileNameandshortSessionIDmoved with the code that used them — nothing in the UI needs them now.SessionSystemPromptEntryis folded intoWriteShareableSession. It was added in #104 purely as a stepping stone; now that the whole share path lives in the app layer, the UI no longer needs to know that shared files embed a system-prompt entry. It survives as the unexportedsystemPromptEntry.Result
internal/uino longer importsinternal/sessionin any file, test or otherwise.AppControllergrew 5 methods and lost 1; net, the UI's view of the session layer is now entirely value types and sentinel errors.Design notes
The picker takes a port, not a list.
tree_selectorin #104 was converted to pure values (NewTreeSelector(roots, ...)), and consistency argued for the same here. I went with a narrowSessionStoreinterface instead, because unlike the tree selector this component acts: it reloads across scope toggles and deletes files mid-flow. Threading that through the parent model would have meant a request/response message round-trip for delete, which is more coupling than it removes. The port is three methods and*app.Appsatisfies it incidentally.Message ordering in
/shareis preserved deliberately. The snapshot guards stay in the UI ahead of theghprobes, so an in-memory session still reports "not persisted" rather than "gh is not installed".WriteShareableSessionre-checks independently./exporterror text changed slightly. Previously "Failed to read session file" / "Failed to write export file"; now one "Failed to export session: …" carrying the wrapped cause, which names the offending path:Tests
internal/app/session_store_test.go(28 cases) covers summary projection, listing across working directories, delete guards, export sentinels/name derivation/byte-for-byte copy/unwritable destination, and share splicing — asserting the header stays line 0, the system prompt is line 1, model resolution prefers the session over the fallback, and every original entry survives unchanged.newPersistedAppsandboxesHOMEso nothing touches the real session store.internal/ui/session_selector_test.go(11 cases) exercises the picker against a stub store: load/scope selection, listing-error tolerance, scope toggle, named filter, selection, and the full delete flow including decline and the failure path (entry must survive a failed delete).Validation
gofmt·go build·go vet·go test -race ./...·golangci-lint— all clean.Smoke-tested against a seeded session:
/resume(both scopes, named filter, delete arm + decline), history replay,/session,/export(derived name, explicit path, unwritable path),/namewith a/in it →session_renamed_session_two.jsonl,/tree,/new. Exported file verified byte-identical to source. stderr empty throughout.Fixes #101
Summary by CodeRabbit