Repository navigation
Conversation
|
@cmolinatnt is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
To use Codex here, create a Codex account and connect to github. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (34)
📝 WalkthroughWalkthroughAdds configurable session restore behavior ( ChangesSession Restore, Shell History, and Command History
Sequence Diagram(s)sequenceDiagram
participant User
participant CommandPalette as ContentView / Command Palette
participant AppDelegate
participant Recorder as TerminalCommandHistoryRecorder
participant Panel as CommandHistoryWindowController
User->>CommandPalette: invoke palette.showCommandHistory
CommandPalette->>AppDelegate: showCommandHistoryForFocusedSurface(preferredWindow:)
AppDelegate->>Recorder: load(surfaceID:)
Recorder-->>AppDelegate: [TerminalCommandHistoryEntry]
AppDelegate->>Panel: show(entries:)
Panel-->>User: NSPanel with CommandHistoryView (newest-first list)
sequenceDiagram
participant PTY as PTY byte stream
participant ByteTee as MobileTerminalByteTee
participant Recorder as TerminalCommandHistoryRecorder
participant Parser as OSC133CommandParser
participant Disk as per-surface commands JSON
PTY->>ByteTee: raw bytes + surfaceID
ByteTee->>Recorder: append(surfaceID, bytes) [O(1) when disabled]
Recorder->>Recorder: enqueue on serial queue
Recorder->>Parser: feed text
Recorder->>Parser: takeCompletedBlocks()
Parser-->>Recorder: [TerminalCommandBlock]
Recorder->>Recorder: filter empty, trim, assign ID, enforce limit
Recorder->>Disk: persist atomically (best-effort)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
404d370 to
cee5184
Compare
Adds a "Sessions" settings area (session.* in cmux.json, both default-on) with
two capabilities:
- Per-tab shell history (session.persistShellHistory): each terminal tab gets
its own HISTFILE keyed by the tab's stable surface UUID, injected via the
existing per-shell startup hooks and applied in the zsh/bash prompt hook
(after the user's rc, with incremental writes) so up-arrow / Ctrl-R recall
the tab's commands after a reopen. Also records a per-tab cmux command
history (OSC 133, via the existing PTY tee) viewable from the command palette
("Show Command History"). Keyed by surface UUID — not working directory —
because a tab's cwd is not stable between first open and restore.
- Configurable session restore (session.restoreMode: always | ask | never,
default ask): SessionRestorePolicy honors the mode and, in "ask", AppDelegate
prompts before applying the restore (keeping the snapshot reopenable from
File > Restore Previous Launch).
Wired through the settings catalog, Settings UI, JSON schema, docs messages,
and Localizable.xcstrings (en + ja). Tests: restore-mode policy, HISTFILE
keying + env injection, OSC 133 drain, command-history round-trip, and config
defaults.
cee5184 to
381b9e8
Compare
Greptile SummaryThis PR introduces two session features — per-tab shell history (keyed by surface UUID, injected via the existing PTY tee and shell-integration prompt hooks) and a configurable session restore mode (
Confidence Score: 3/5Two user-visible correctness issues need fixing before shipping: the feature's own UI and web schema tell users the wrong story about how their history is scoped, and a new debug-named property lands unconditionally in shipping source against the project's explicit seam policy. The "grouped by project" / "namespaced by project directory" copy appears in three user-facing surfaces (Settings UI subtitle, JSON schema description, and the web messages file) but the implementation keys history by surface UUID — a first-time user who opens a second tab in the same project will find it starts empty, directly contradicting what they read in Settings. Separately,
Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant AppDelegate
participant SessionRestorePolicy
participant SnapshotStore
participant Alert
AppDelegate->>SessionRestorePolicy: shouldAttemptRestore(restoreMode:)
alt "restoreMode == .never"
SessionRestorePolicy-->>AppDelegate: false (skip)
else "restoreMode == .always or .ask"
SessionRestorePolicy-->>AppDelegate: true
AppDelegate->>SnapshotStore: loadStartupSnapshot()
SnapshotStore-->>AppDelegate: snapshot
alt "restoreMode == .ask"
AppDelegate->>Alert: confirmRestorePreviousSession()
alt user picks Restore
Alert-->>AppDelegate: true - apply snapshot
else user picks Start Fresh
Alert-->>AppDelegate: "false - startupSessionSnapshot = nil"
end
else "restoreMode == .always"
AppDelegate->>AppDelegate: apply snapshot silently
end
end
Note over AppDelegate: Per-tab history (parallel flow)
participant PTYTee as PTY Tee (IO thread)
participant Recorder as TerminalCommandHistoryRecorder
participant Disk
PTYTee->>Recorder: append(surfaceID:bytes:) O(1) if disabled
Recorder->>Recorder: queue.async - ingest / parse OSC 133
Recorder->>Disk: persist entries JSON (atomic write)
AppDelegate->>Recorder: load(surfaceID:) on MainActor - disk read
Recorder-->>AppDelegate: [TerminalCommandHistoryEntry]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant AppDelegate
participant SessionRestorePolicy
participant SnapshotStore
participant Alert
AppDelegate->>SessionRestorePolicy: shouldAttemptRestore(restoreMode:)
alt "restoreMode == .never"
SessionRestorePolicy-->>AppDelegate: false (skip)
else "restoreMode == .always or .ask"
SessionRestorePolicy-->>AppDelegate: true
AppDelegate->>SnapshotStore: loadStartupSnapshot()
SnapshotStore-->>AppDelegate: snapshot
alt "restoreMode == .ask"
AppDelegate->>Alert: confirmRestorePreviousSession()
alt user picks Restore
Alert-->>AppDelegate: true - apply snapshot
else user picks Start Fresh
Alert-->>AppDelegate: "false - startupSessionSnapshot = nil"
end
else "restoreMode == .always"
AppDelegate->>AppDelegate: apply snapshot silently
end
end
Note over AppDelegate: Per-tab history (parallel flow)
participant PTYTee as PTY Tee (IO thread)
participant Recorder as TerminalCommandHistoryRecorder
participant Disk
PTYTee->>Recorder: append(surfaceID:bytes:) O(1) if disabled
Recorder->>Recorder: queue.async - ingest / parse OSC 133
Recorder->>Disk: persist entries JSON (atomic write)
AppDelegate->>Recorder: load(surfaceID:) on MainActor - disk read
Recorder-->>AppDelegate: [TerminalCommandHistoryEntry]
Reviews (1): Last reviewed commit: "feat(session): per-tab shell history and..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /// Test seam: when set, ``confirmRestorePreviousSession()`` returns this | ||
| /// instead of running the modal restore prompt, so launch-restore tests can | ||
| /// drive the Restore/Start-Fresh decision without UI. | ||
| var debugRestoreSessionConfirmationHandler: (() -> Bool)? |
There was a problem hiding this comment.
debugRestoreSessionConfirmationHandler is declared unconditionally in Sources/AppDelegate.swift — not guarded by #if DEBUG at the declaration site — and its name matches the debug… pattern the no-test-debug-seam rule forbids. The analogous debugCloseMainWindowConfirmationHandler pre-exists, but this PR introduces a second one. Because AppDelegate is @testable import-able, the preferred fix is to widen the visibility of the relevant state to internal and inspect it from the test target directly, keeping the property out of shipping source.
Rule Used: Flag Swift files under a production Sources path (... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| configurationReview: .json("session.persistShellHistory"), | ||
| String(localized: "settings.session.persistShellHistory", defaultValue: "Per-Tab Shell History"), | ||
| subtitle: persistShellHistory.current | ||
| ? String(localized: "settings.session.persistShellHistory.subtitleOn", defaultValue: "Each tab keeps its own command history (up-arrow / Ctrl-R), grouped by project, and records a cmux command history.") |
There was a problem hiding this comment.
The subtitle says "grouped by project" but the implementation keys history by surface UUID (not by working directory). The PR description explicitly states: "Keyed by surface UUID — not working directory — because a tab's cwd is not stable between first open and restore." A user who opens a second tab in the same project directory will find it starts with empty history, directly contradicting the "grouped by project" wording. The same incorrect description appears in web/messages/en.json:960 ("namespaced by project directory") and in the SessionCatalogSection doc-comment. All three occurrences should be corrected to describe the actual behavior: each tab retains its own history across session restores via its stable surface ID.
Rule Used: Flag production user-facing text that is not fully... (source)
| }, | ||
| "session": { | ||
| "restoreMode": "When cmux launches with a previous session available: \"always\" restores it silently, \"ask\" prompts before restoring (the default), and \"never\" starts fresh (the previous session stays reopenable via File > Restore Previous Launch).", | ||
| "persistShellHistory": "Give each terminal tab its own shell history (up-arrow / Ctrl-R) namespaced by project directory, and record a cmux command history per tab. When off, tabs use your shell's normal global history (e.g. ~/.zsh_history)." |
There was a problem hiding this comment.
This schema description says "namespaced by project directory" but history is actually keyed by surface UUID. See the matching comment on SessionSection.swift:74 — the same incorrect description needs correction here and in SessionCatalogSection.swift's doc-comment.
Rule Used: Flag production user-facing text that is not fully... (source)
| func showCommandHistoryForFocusedSurface(preferredWindow: NSWindow? = nil) { | ||
| guard | ||
| let target = resolveFocusedNotificationTarget(preferredWindow: preferredWindow), | ||
| let surfaceId = target.surfaceId | ||
| else { | ||
| NSSound.beep() | ||
| return | ||
| } | ||
| let entries = TerminalCommandHistoryRecorder.load(surfaceID: surfaceId) | ||
| CommandHistoryWindowController.shared.show(entries: entries) |
There was a problem hiding this comment.
@MainActor interactive path
TerminalCommandHistoryRecorder.load(surfaceID:) calls Data(contentsOf:url) followed by JSONDecoder().decode synchronously from this @MainActor function, which is triggered by a command-palette action. The recorder already holds the complete in-memory state for open surfaces in statesBySurfaceID on its private serial queue, but that state is not exposed to the main actor — so every palette invocation goes to disk instead. On a slow filesystem or a file that has grown to the 1 000-entry cap the JSON read blocks the main thread. The fix is either to add an async-safe path that drains the in-memory entries off the queue (e.g. queue.sync { self.statesBySurfaceID[id]?.entries } is safe from MainActor in a short-running call), or to make showCommandHistoryForFocusedSurface an async entry-point that awaits the queue.
Rule Used: Flag production Swift that reads, decodes, or scan... (source)
1 participant
Adds a "Sessions" settings area (session.* in cmux.json, both default-on) with two capabilities:
Per-tab shell history (session.persistShellHistory): each terminal tab gets its own HISTFILE keyed by the tab's stable surface UUID, injected via the existing per-shell startup hooks and applied in the zsh/bash prompt hook (after the user's rc, with incremental writes) so up-arrow / Ctrl-R recall the tab's commands after a reopen. Also records a per-tab cmux command history (OSC 133, via the existing PTY tee) viewable from the command palette ("Show Command History"). Keyed by surface UUID — not working directory — because a tab's cwd is not stable between first open and restore.
Configurable session restore (session.restoreMode: always | ask | never, default ask): SessionRestorePolicy honors the mode and, in "ask", AppDelegate prompts before applying the restore (keeping the snapshot reopenable from File > Restore Previous Launch).
Wired through the settings catalog, Settings UI, JSON schema, docs messages, and Localizable.xcstrings (en + ja). Tests: restore-mode policy, HISTFILE keying + env injection, OSC 133 drain, command-history round-trip, and config defaults.
Summary
Testing
Demo Video
For UI or behavior changes, include a short demo video (GitHub upload, Loom, or other direct link).
Review Trigger (Copy/Paste as PR comment)
Checklist
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Adds a Sessions settings area with per-tab shell history and a configurable session restore mode. Tabs keep their own history across reopens, and launch behavior is now controllable.
New Features
session.persistShellHistory, default true)HISTFILEviaCMUX_SHELL_HISTFILE, keyed by the tab’s surface UUID; applied after user rc with incremental writes for safe persistence.session.restoreMode:always|ask|never, defaultask)askshows a launch prompt;neverstarts fresh;alwaysrestores silently. Previous sessions stay available via File → Restore Previous Launch.session.*in~/.config/cmux/cmux.json; schema and en/ja strings added.Migration
session.restoreMode/session.persistShellHistoryin~/.config/cmux/cmux.json.Written for commit 381b9e8. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation