Repository navigation
feat: add note surface type with cmux note CLI - #4332
Closed
austinywang wants to merge 293 commits into
Closed
austinywang wants to merge 293 commits into
austinywang wants to merge 293 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a project-scoped ChangesProject-Scoped Markdown Notes Surface
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Client
participant TerminalController
participant Workspace
participant NoteSupport
participant Filesystem
CLI->>Client: cmux note new/open/list/path/rm
Client->>TerminalController: v2 RPC (note.*)
TerminalController->>NoteSupport: validateSlug / autoSlug
TerminalController->>Workspace: noteProjectRoot() / newNoteSurface(...)
Workspace->>NoteSupport: resolve or ensure note path
NoteSupport->>Filesystem: create/list/delete note files
Workspace->>TerminalController: MarkdownPanel (created or reused)
TerminalController->>Client: structured response (slug, path, project_root, reused/exists)
Client->>CLI: prints human or JSON output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (6 errors, 1 warning)
✅ Passed checks (10 passed)
✨ 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 |
3 of 8 tasks
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@CLI/cmux.swift`:
- Around line 4009-4026: In the "rm"/"delete" subcommand block (the case
handling in CLI/cmux.swift) add the same unknown-flag validation used elsewhere:
after computing let rest = Array(trailingArgs.dropFirst()) and before checking
for extraArg, detect if rest.first starts with "-" and throw a CLIError like
"cmux note rm: unknown flag '\(rest.first!)'"; keep the existing extra-argument
check and then proceed to buildRoutingParams(), set params["slug"] = slugArg and
call client.sendV2("note.delete", ...).
- Around line 3991-4007: The "path" (and similarly "rm") case is missing the
unknown-flag validation that "new", "open", and "list" perform, causing poor
error messages; update the "path" case to run the same unknown-flag check (the
same validation used by the "new"/"open"/"list" branches) before you compute
rest/extraArg from trailingArgs so unknown flags like `--unknown` produce the
consistent "unknown flag" error, then keep the existing extra-argument check and
the rest of the logic (references: the "path" case, trailingArgs, extraArg, and
the "rm" case).
In `@Sources/CmuxConfig.swift`:
- Around line 3253-3426: Extract the NoteSupport enum (including NoteListEntry
and NoteError) into its own Swift source file: create a new file containing
NoteSupport, NoteListEntry, NoteError and all methods (validateSlug, autoSlug,
projectRoot(forCwd:), notesDirectory(forProjectRoot:),
notePath(forSlug:projectRoot:), slug(forNotePath:),
ensureNoteFile(slug:projectRoot:), listNotes(forProjectRoot:),
deleteNote(slug:projectRoot:)). Ensure the new file imports Foundation and that
the types have the appropriate access level (make them internal or public to
match current usage outside the original file), remove the original enum from
CmuxConfig.swift, and add the new source to the same target so all existing
references to NoteSupport continue to compile. Also run a build to fix any
access-level issues and update tests/imports if this is used by other targets.
- Around line 3336-3346: slug(forNotePath:) currently returns the raw slug when
validateSlug(slug) throws, violating the contract; change the final return to
only return the validated slug (String) and otherwise return nil by replacing
"return (try? validateSlug(slug)) ?? slug" with logic that attempts
validateSlug(slug) and returns nil on failure (e.g., use do/try/catch or if-let
with try? to return the validated value or nil). Ensure you reference the local
variable slug and the validateSlug(_:) call in the updated code.
- Around line 3351-3362: The ensureNoteFile(slug:projectRoot:) currently ignores
FileManager.createFile(...)’s Bool result and returns the path even if creation
failed; update the function to check the Bool returned by
FileManager.default.createFile(atPath: path, contents: Data(), attributes: nil)
and if it returns false throw an appropriate error (or propagate a thrown error)
so callers like note.create/note.open don’t assume the file exists; keep the
existing behavior for directory creation (notesDirectory and createDirectory)
and reference the path, dir, and slug variables when adding the failure check
and throw.
In `@Sources/TerminalController.swift`:
- Around line 9444-9462: The success response built in the new-panel branch (the
dictionary passed to result = .ok([...]) that contains keys like "window_id",
"window_ref", "pane_id", "surface_id", "slug", "path", etc.) must include an
explicit "reused": false entry to match the reused-panel branch which returns
"reused": true; update the dictionary in the code that constructs result =
.ok(...) (references: result, v2OrNull, v2Ref, windowId, targetPaneUUID,
panel.id, slug, notePath, projectRoot) to add "reused": false so the response
schema is consistent and callers can reliably distinguish reused vs new panels.
- Around line 9554-9558: The response currently interpolates the raw error into
the API message (result = .err with code "io_error" and message "Failed to
delete note: \(error)"), which may leak internal details; change the message to
a generic string like "Failed to delete note" and move the error details into
the data payload (e.g., add "error" or "error_description":
error.localizedDescription alongside the existing ["slug": slug]) so diagnostics
remain available without exposing internal error text in the top-level message.
- Around line 9453-9456: The code inconsistently uses optional chaining for
sourceSurfaceId (and sourcePaneUUID) even though a prior guard (guard let
sourceSurfaceId) made them non-optional; update the entries using v2OrNull and
v2Ref so they use the non-optional properties: replace
sourceSurfaceId?.uuidString with sourceSurfaceId.uuidString and
sourcePaneUUID?.uuidString with sourcePaneUUID.uuidString, and ensure
v2Ref(kind: .surface, uuid: sourceSurfaceId) and v2Ref(kind: .pane, uuid:
sourcePaneUUID) continue to receive the non-optional UUIDs (symbols:
sourceSurfaceId, sourcePaneUUID, v2OrNull, v2Ref).
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 60b62169-5d59-407e-b20c-34d6308c59f3
📒 Files selected for processing (6)
CLI/cmux.swiftSources/CmuxConfig.swiftSources/SessionPersistence.swiftSources/TerminalController+MoveTabToNewWorkspace.swiftSources/TerminalController.swiftSources/Workspace.swift
Contributor
Greptile SummaryThis PR introduces
Confidence Score: 5/5Safe to merge; no correctness defects remain in the changed paths after multiple rounds of fixes. Multiple previous review rounds addressed concurrency, ordering, I/O-on-main, and localization; the fixes are all reflected in the current HEAD. The only remaining finding is a style concern: two large new files exceed the project's file-size ceiling and bundle more than one concern. These do not affect correctness or security — they are a maintainability signal for a follow-up split, not a reason to hold the merge. Sources/NotesTreeStore.swift and Sources/NotesTreeStorage.swift — both exceed 800 lines and mix concerns that would benefit from a follow-up split. Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as cmux note CLI
participant Socket as ControlSocket RPC
participant TC as TerminalController+Notes
participant Store as CmuxNoteStore
participant FS as Filesystem (.cmux/notes/)
participant UI as MarkdownPanel / NotesTreeStore
CLI->>Socket: note.create / note.open / note.write
Socket->>TC: v2NoteCreate / v2NoteOpenSplit / v2NoteWrite
TC->>TC: v2MainSync (resolve workspace/surface)
TC-->>Store: createOrOpenAsync / writeContent (off-main)
Store->>Store: withStoreLockAsync (storageQueue.async)
Store->>FS: loadIndex → mutate → writeIndex (atomic)
Store->>FS: ensureBodyFile / write content
Store-->>TC: CmuxNoteStoreResult
TC->>TC: v2MainSync (open MarkdownPanel, focus)
TC-->>Socket: JSON result
Socket-->>CLI: slug / path / output
UI->>Store: CmuxNoteStore.list (via NotesTreeStorage)
Store->>Store: withStoreLock (storageQueue.sync)
Store->>FS: loadIndex + sort by mtime
Store-->>UI: [CmuxNoteRecord]
UI->>UI: reload() → buildChildren → NotesTreeNode tree
Reviews (75): Last reviewed commit: "Merge origin/main into notes surface PR" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@Sources/Workspace.swift`:
- Around line 1210-1215: The helper noteSlugForConfigSurface currently falls
back to NoteSupport.autoSlug() which produces a new random slug each launch;
change it so that when surface.name is missing or invalid you either (A) derive
a deterministic slug from a stable layout seed/state (e.g. use a stable
identifier from the CmuxSurfaceDefinition such as surface.id or a layoutSeed
value) and pass that through NoteSupport.validateSlug, or (B) surface validation
fails and you propagate/reject the malformed config so the caller can handle it;
update noteSlugForConfigSurface to use NoteSupport.validateSlug on the
deterministic fallback (or throw/return an error) instead of calling
NoteSupport.autoSlug().
- Around line 865-875: The saved snapshot's noteSlug must be validated before
using it to construct a filesystem path: in the block that reads
snapshotMarkdown.noteSlug, first verify the slug matches the allowed pattern
(e.g. /^[a-z0-9-]+$/) and only then call NoteSupport.notePath(forSlug:
projectRoot:) (with projectRoot from noteProjectRoot()); if validation fails or
the resulting path doesn't exist, set restorePath to snapshotMarkdown.filePath
instead; keep the existing FileManager.default.fileExists(atPath:) check but
perform it only after the slug validation.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: f0b454f0-c4dc-42e3-acd0-c08676496f32
📒 Files selected for processing (2)
Sources/TerminalController.swiftSources/Workspace.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/TerminalController.swift">
<violation number="1" location="Sources/TerminalController.swift:9385">
P2: Invalid `direction` values are silently coerced to `.right` instead of returning `invalid_params`.</violation>
</file>
<file name="Sources/CmuxConfig.swift">
<violation number="1" location="Sources/CmuxConfig.swift:3360">
P2: `ensureNoteFile` can report success even when file creation fails because `createFile`'s `Bool` result is ignored.</violation>
<violation number="2" location="Sources/CmuxConfig.swift:3411">
P2: `deleteNote` is not fully idempotent under races: a file can disappear after `fileExists` and still throw on `removeItem`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@Resources/Localizable.xcstrings`:
- Around line 113745-113746: The two localization entries cli.note.globalUsage
and cli.note.usage currently contain verbatim English for many high-confidence
locales (de, es, fr, it, ko, nb, pt-BR, ru, uk, zh-Hans, zh-Hant); remove these
copied-English string values and replace them with proper localized strings (or
an explicit non-production placeholder status) for each listed locale so they no
longer present English in production; update the corresponding stringUnit state
metadata for each locale (e.g., set to "needs-translation" or the correct
translated state) and ensure only authentic translations or approved
placeholders exist for cli.note.globalUsage and cli.note.usage rather than
copied English.
In `@Sources/NoteSupport.swift`:
- Around line 116-123: ensureNoteFile currently treats any existing path
(including directories) as a valid note file; update ensureNoteFile, listNotes,
and deleteNote to guard that paths are regular files. Specifically: in
ensureNoteFile, when FileManager.default.fileExists(atPath:) is true, verify the
path is a regular file (e.g., use FileManager.default.attributesOfItem(atPath:)
or URL.resourceValues(forKeys:[.isRegularFileKey/.isDirectoryKey])) and throw a
CocoaError if it’s a directory; when creating the file, proceed only if the path
is absent or a regular file; in listNotes, filter out entries where
isRegularFile is false so directories aren’t returned; in deleteNote, check the
target is a regular file before calling removeItem(atPath:) and throw if it’s a
directory. Apply these checks inside the functions named ensureNoteFile,
listNotes, and deleteNote.
In `@Sources/TerminalController`+Notes.swift:
- Line 14: Replace all uses of String(describing: error) in the Notes-related
error responses with error.localizedDescription to avoid leaking Swift type
names; specifically update the return .err(...) expressions that currently pass
String(describing: error) (the occurrences flagged in the Notes controller) to
instead pass error.localizedDescription so the API returns a user-friendly
message—ensure you change every instance mentioned (the three other occurrences
found alongside the one shown) so they match the existing handlers that already
use error.localizedDescription.
In `@Sources/Workspace.swift`:
- Around line 11136-11138: The noteProjectRoot() helper currently calls
NoteSupport.projectRoot(forCwd: currentDirectory) which can pick up the stale
snapshot value; change it to resolve from the live project location at restore
time instead of the saved snapshot. Update noteProjectRoot() to call
NoteSupport.projectRoot(forCwd: liveProjectDirectory) where liveProjectDirectory
is derived from the runtime workspace/project root (e.g., the workspace's
current project directory or process CWD) and ensure restoreSessionSnapshot(_:)
does not overwrite that live value before note panels are recreated (if
restoreSessionSnapshot assigns currentDirectory from the snapshot, move that
assignment or defer rebuilding note panels until after you recompute note roots
from the live location). Use the symbols noteProjectRoot(),
NoteSupport.projectRoot(forCwd:), and restoreSessionSnapshot(_:) to locate and
implement the change.
- Around line 11145-11164: The helper newNoteSurface currently calls
NoteSupport.notePath(forSlug:projectRoot:) with the raw slug when
createIfMissing is false; ensure slug validation is performed inside this
boundary by validating the slug against the allowed pattern (e.g.
/^[a-z0-9-]+$/) before computing the read-only path, and return nil (or an
appropriate error) for invalid slugs. Update newNoteSurface to validate slug at
the top of the else branch (the path that uses NoteSupport.notePath) so callers
no longer must remember to validate, and keep the existing use of
NoteSupport.ensureNoteFile(...) in the createIfMissing=true branch unchanged.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 592e1d0e-9186-4131-9f6a-99cc2e4ff4ba
📒 Files selected for processing (10)
CLI/cmux.swiftResources/Localizable.xcstringsSources/CmuxConfig.swiftSources/NoteSupport.swiftSources/SessionPersistence.swiftSources/TerminalController+Notes.swiftSources/TerminalController.swiftSources/Workspace.swiftcmux.xcodeproj/project.pbxprojcmuxTests/CmuxConfigTests.swift
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@Sources/NoteSupport.swift`:
- Around line 17-30: Replace the hardcoded validation messages in NoteError and
the slug-checking code with localized strings using
String(localized:defaultValue:) (or your app's localization API) and add
matching keys to the app's string catalogs; specifically, change the messages
thrown in NoteError (e.g., "slug is empty", "slug is longer than %d characters",
"slug may contain only lowercase letters, digits, and hyphens", "slug must not
start with a hyphen") to use localization keys (e.g., "note.slug_empty",
"note.slug_too_long", "note.slug_invalid_chars",
"note.slug_cannot_start_hyphen") and supply defaultValue for each, add those
keys to every supported locale catalog, and update the other occurrence
mentioned (the block around lines 233-240) the same way so all user-facing
validation and NoteError.localizedDescription strings are localized.
- Around line 201-217: In deleteNote(slug:projectRoot:) replace the
FileManager.removeItem(atPath:) call with an atomic Darwin.unlink(2) call:
obtain a C path for the computed path (e.g. via withCString or
fileSystemRepresentation), call unlink, and map results so that unlink returning
0 -> return true, errno == ENOENT -> return false, errno == EISDIR (directory)
-> throw NoteError.notRegularFile, and any other errno -> rethrow/convert to an
error; keep the existing validateSlug(notePath:) and isRegularFile checks but
remove reliance on removeItem’s semantics to avoid the TOCTOU race.
In `@Sources/TerminalController`+Notes.swift:
- Around line 22-33: The RPC responses in v2NoteOpen (and related handlers)
return hardcoded English messages; replace the inline message: string literals
in the V2CallResult.err calls with localized strings using
String(localized:defaultValue:) (or your app's localization helper) and create
matching keys in the Localizable.xcstrings catalog (e.g., keys for
"invalid_params.missing_slug" and the validation error fallback). Update
v2NoteOpen, the error branch around NoteSupport.validateSlug, and all other v2*
handlers called by v2NoteOpenSplit/V2CallResult to use localized keys instead of
literal text, passing error.localizedDescription only where appropriate as a
parameterized defaultValue or variable in the localized string so the API body
is locale-aware.
- Around line 8-18: The create-note path currently treats an explicitly empty
providedSlug as omitted and calls NoteSupport.autoSlug(), but we should reject
explicit empty slugs like other endpoints do: change the logic around
v2String(params, "slug")/providedSlug so that if providedSlug is present but
empty you return .err(code: "invalid_params", message: "slug must not be empty",
data: nil) instead of falling through to NoteSupport.autoSlug(); keep using
NoteSupport.validateSlug when providedSlug is non-empty and preserve
auto-generation only when the slug key is truly absent (nil).
- Around line 62-70: The API currently returns raw filesystem errors by
inserting error.localizedDescription into the top-level "message" and
"error_description" fields when building the response (see the result =
.err(...) blocks in TerminalController+Notes.swift); change these to stable,
generic user-facing messages (e.g., "I/O error while accessing the note" or
"Unable to read/write note") and remove any direct inclusion of
localizedDescription from the response body, and instead send the full error to
internal logging (use the controller's logger or a similar internal log call) so
callers get a consistent, non-platform-specific message; apply the same change
to all occurrences where result = .err(...) populates "message" and
"error_description" (including the other ranges mentioned).
In `@Sources/Workspace.swift`:
- Around line 545-548: Only populate noteSlug when the markdown file is inside
this workspace's note root: compute the slug with NoteSupport.slug(forNotePath:)
only if markdownPanel.filePath is located under this workspace's resolved note
root (e.g. check path.hasPrefix(self.resolvedNoteRoot) or use a
project-root-aware containment helper), otherwise pass nil into
SessionMarkdownPanelSnapshot. Update the code that constructs markdownSnapshot
in the Workspace (the SessionMarkdownPanelSnapshot creation) to perform that
containment test and conditionally assign the slug.
- Around line 11139-11140: The noteProjectRoot() helper currently calls
NoteSupport.projectRoot(forCwd: currentDirectory) which uses the local
filesystem but currentDirectory can be a remote/SSH path; change
noteProjectRoot() to first detect remote workspaces (e.g., check
workspace.isRemote or an equivalent flag on the current Workspace/Context) and
if remote either return an explicit local note root (map to a configured
localNotesRoot via NoteSupport or a LocalNotesDirectory constant) or fail fast
by throwing/returning nil so note create/open/restore are rejected for remote
workspaces; apply the same remote-check and mapping/early-reject change to the
other helper calls that use currentDirectory for notes (the usages around the
note create/open/restore code paths).
🪄 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: ASSERTIVE
Plan: Pro
Run ID: d67ce4ab-c48b-47ef-ae6c-ce8c960ce6ae
📒 Files selected for processing (4)
Resources/Localizable.xcstringsSources/NoteSupport.swiftSources/TerminalController+Notes.swiftSources/Workspace.swift
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)
Sources/Workspace.swift (1)
264-280:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCompute the restore note root after applying
snapshot.currentDirectory.
sessionRestoreNoteProjectRootis cached beforecurrentDirectoryis updated from the snapshot, so note restore can resolve slugs against the stale pre-restore cwd and fall back to the old absolute path after a project move.💡 Minimal fix
- sessionRestoreNoteProjectRoot = noteProjectRoot() - defer { sessionRestoreNoteProjectRoot = nil } - let restoredRemoteConfiguration = snapshot.remote?.workspaceConfiguration() if let restoredRemoteConfiguration { configureRemoteConnection( restoredRemoteConfiguration, autoConnect: !SessionRestorePolicy.isRunningUnderAutomatedTests() @@ let normalizedCurrentDirectory = snapshot.currentDirectory.trimmingCharacters(in: .whitespacesAndNewlines) if !normalizedCurrentDirectory.isEmpty { currentDirectory = normalizedCurrentDirectory } + + sessionRestoreNoteProjectRoot = noteProjectRoot() + defer { sessionRestoreNoteProjectRoot = nil }🤖 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 `@Sources/Workspace.swift` around lines 264 - 280, sessionRestoreNoteProjectRoot is being captured before applying snapshot.currentDirectory, causing note restore to use a stale working directory; move the assignment sessionRestoreNoteProjectRoot = noteProjectRoot() (and its defer cleanup) to after you update currentDirectory from snapshot.currentDirectory (i.e. after computing normalizedCurrentDirectory and assigning currentDirectory) so noteProjectRoot() sees the restored cwd; keep the existing defer { sessionRestoreNoteProjectRoot = nil } and leave configureRemoteConnection / disconnectRemoteConnection logic as-is.
🤖 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 `@Resources/Localizable.xcstrings`:
- Around line 113747-113768: The new localization keys (e.g.,
"note.error.slugEmpty", "note.error.slugTooLong", "note.error.slugInvalidChars",
"note.error.slugStartsWithHyphen", "note.error.invalidSlug", and all
"rpc.note.error.*" keys added) only include en and ja; update
Resources/Localizable.xcstrings by adding translated stringUnit entries for
every locale already present in this catalog (ar, bs, da, de, es, fr, it, ko,
nb, pl, pt-BR, ru, th, tr, uk, zh-Hans, zh-Hant, etc.) for each of those keys so
that each key has matching localized values across all supported locales per the
project guideline.
In `@Sources/Workspace.swift`:
- Around line 1331-1341: The code is treating any non-nil return from
newNoteSurface as a freshly placed surface and closing the placeholder even when
newNoteSurface returned an existing panel in another pane; to fix, change
newNoteSurface so that when reuseExisting is false it calls
newMarkdownSurface(...) instead of openOrFocusMarkdownSurface(...), ensuring the
workspace-wide reuse path is not used for layout materialization, and update the
corresponding duplicate branches (the block using
noteSlugForConfigSurface/newNoteSurface and the similar block at the other
location) so they only close the placeholder panel (closePanel(panelId, force:
true)) when the returned panel actually belongs to the target pane, preserving
title setting (setPanelCustomTitle) and focus assignment (focusPanelId)
behavior.
---
Outside diff comments:
In `@Sources/Workspace.swift`:
- Around line 264-280: sessionRestoreNoteProjectRoot is being captured before
applying snapshot.currentDirectory, causing note restore to use a stale working
directory; move the assignment sessionRestoreNoteProjectRoot = noteProjectRoot()
(and its defer cleanup) to after you update currentDirectory from
snapshot.currentDirectory (i.e. after computing normalizedCurrentDirectory and
assigning currentDirectory) so noteProjectRoot() sees the restored cwd; keep the
existing defer { sessionRestoreNoteProjectRoot = nil } and leave
configureRemoteConnection / disconnectRemoteConnection logic as-is.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 0c7f7431-fc51-4ef9-9694-4ca946983c90
📒 Files selected for processing (5)
Resources/Localizable.xcstringsSources/NoteSupport.swiftSources/TerminalController+Notes.swiftSources/Workspace.swiftcmux.xcodeproj/project.pbxproj
Contributor
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@Sources/Workspace.swift`:
- Around line 11154-11199: newNoteSurface is running NoteSupport.ensureNoteFile
/ noteFileExists / notePath synchronously on the `@MainActor` and needs to perform
that filesystem I/O off the main thread: move the code that calls
NoteSupport.ensureNoteFile, NoteSupport.noteFileExists and NoteSupport.notePath
into an async background task (e.g. Task.detached or a background queue),
capture the resulting filePath or error there, then hop back to the main actor
to call openOrFocusMarkdownSurface or newMarkdownSurface; preserve the existing
slug validation, error logging messages and the reuseExisting branch logic, but
only do UI work (openOrFocusMarkdownSurface / newMarkdownSurface) on the main
actor after awaiting the background result.
- Around line 264-265: sessionRestoreNoteProjectRoot is being captured before
applying snapshot.currentDirectory so noteProjectRoot() resolves against the old
cwd; to fix, move the assignment sessionRestoreNoteProjectRoot =
noteProjectRoot() to after the code that applies snapshot.currentDirectory (the
block that sets snapshot.currentDirectory on the workspace), keeping the
existing defer { sessionRestoreNoteProjectRoot = nil } to clear it; in short,
call noteProjectRoot() only after snapshot.currentDirectory has been applied so
sessionRestoreNoteProjectRoot caches the post-restore root.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: d77148cb-3321-4d8f-ab9d-2045d6334bf9
📒 Files selected for processing (6)
CLI/cmux.swiftResources/Localizable.xcstringsSources/NoteSupport.swiftSources/TerminalController+Notes.swiftSources/Workspace.swiftcmux.xcodeproj/project.pbxproj
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Two note-surface bugs: - The pane focus flash animated through detached asyncAfter closures; any interruption between a fade-in segment and its fade-out left the accent ring stuck faintly visible — the "random vertical line" parked on note pane edges. The pattern now runs in one cancellable task whose every exit path lands on opacity 0. - Renaming a note round-tripped through the index write before any title surface updated, so the header, tab, and derived workspace title flashed old -> new. The rename now applies optimistically: synchronously to the renaming panel, via the shared retitle notification to siblings and the tree, and the store result reconciles (or rolls back to the previous title on failure). Regression test asserts the retitle is visible synchronously. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
waitForPendingReloadForTesting awaited only reloadTask, racing the coalesce scheduler under load — intermittent stale-tree failures across the observation suite (a different victim each run). Await the coalesce task first. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The branch (via main) carries cmux-side #7997 for the tab bar relayout feedback loop but not bonsplit-side #180 — running half of a two-sided layout fix leaves stale split geometry, which draws the split divider at an old boundary inside a visually-single pane: the stationary vertical line cutting through note headers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replace the accreted note header (custom NSTextField, sizing-text overlay, underline rectangles, focus-responder overrides) with a from-scratch structure that mirrors the plain markdown viewer: - a slim trailing-actions strip (find, preview toggle, typography, overflow) with no divider, like Notion's top-right page controls - a large chromeless page title at the top of the content column, aligned to the preview's max-width column, with a faint Untitled placeholder; click to edit in place (plain SwiftUI TextField + FocusState), Enter/click-away commits, Escape cancels The four NoteTitle* files are deleted outright; renames keep the optimistic one-repaint path and external retitles still flow in. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
A note is just a markdown file in a managed local directory, so the note-only chrome goes away entirely (the Notion-style page header included): every markdown surface now shares the same structure — file path header, one compact trailing control set (Save for plain files, find, source/preview toggle, typography, overflow menu), and a new formatting toolbar over the source editor (H1/H2/H3, bold/italic/ strikethrough, bullet/numbered/task lists, quote, link) inserting markdown at the selection with undo support; headings toggle instead of stacking. Formatting edits are pure functions pinned by unit tests; toolbar labels localized in English and Japanese. Dead copy-toolbar and confirmation-flash code removed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ce-type # Conflicts: # .github/swift-file-length-budget.tsv # Packages/macOS/CmuxSettingsUI/Sources/CmuxSettingsUI/Sections/BetaFeaturesSection.swift # Packages/macOS/CmuxSettingsUI/Tests/CmuxSettingsUITests/SettingsRowAnchorResolutionTests.swift # Resources/Localizable.xcstrings # Sources/Panels/FilePreviewTextEditor.swift # Sources/RestorableAgentSession.swift # Sources/RightSidebarPanelView.swift # Sources/TerminalController.swift # cmux.xcodeproj/project.pbxproj # vendor/bonsplit
The merge union pushed it to 507; the theme application moves to its own extension file so the PR's newly-tracked-file gate stays green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The blind hunk-union of this merge's pbxproj conflict duplicated definitions main had moved (and my dedupe pass then orphaned a multi-line body). Reconstruct deterministically: main's pbxproj as the base with every branch-added entry re-inserted verbatim at its anchored position. Project loads; all five targets and the test wiring lint verify. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Main's a630590 exports ghostty_renderer_event_e, required by the merged CmuxTerminalCore profiling code; the merge had kept the branch's older pointer once again. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The pbxproj rebuild re-inserted branch-side entries whose files main removed (background-mount policy, its tests, the old beta settings view); purge references whose file exists nowhere in the tree. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The rebuild sourced insertions from the pre-merge project snapshot, which predates the theme-extension split. Audit confirms no other tracked Swift source is unwired. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
OpenStep plist strings containing '+' must be quoted; the unquoted path made Xcode reject the whole project as damaged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Take main's FilePreviewTextEditor (session-owned view construction) instead of a hand-merged hybrid: the accent caret ports into its theme application, the current-line highlight applies at attachTextView, and the markdown call site matches the seven-argument API. The interim theme-extension split is unnecessary against main's 467-line file and is removed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
onPointerDown, highlightsCurrentLine, and currentLineHighlightColor back the branch's CurrentLineHighlight extension; main's class body replaced ours without them. Theme application colors the highlight again. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The stationary vertical line cutting through pane headers is the tab right-border separator painting below the bar when relayout proposes an oversized item; the separator (and its container) are now hard-capped to the tab height. Submodule branch fix-tab-separator-height-clamp, pushed to the fork. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ce-type # Conflicts: # .github/swift-file-length-budget.tsv # Sources/KeyboardShortcutSettings.swift
The merge reset bonsplit to main's tip, dropping the separator-clamp commit (which descends from that tip); re-pin it. Ghostty advances to main's b4b6d69. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The between-tab right-border separator painted below the bar into the pane header under oversized relayout proposals — the stationary faint gray vertical line. The target visual has no between-tab separators; delete the drawing outright (bonsplit b0778ea). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
SwiftUI Divider takes the proposed height; under the split-relayout disease (#8139 family) 1px chrome in this pane has painted far outside its row. A hard 1x14 rectangle plus a clipped row cannot. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The 1x14 group-divider ticks stack visually with the horizontal row dividers and read as a stray vertical line cutting through the note header. Whitespace separates the groups instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ce-type # Conflicts: # Packages/macOS/CmuxSettingsUI/Tests/CmuxSettingsUITests/SettingsRowAnchorResolutionTests.swift # Sources/TerminalController+Capabilities.swift # cmux.xcodeproj/project.pbxproj # vendor/bonsplit
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Notes and markdown both render main's untouched viewer chrome: the file path header with the original trailing controls (typography, mode, copy, external open). All branch chrome — compact control set, find button, formatting toolbar row — is unwired, leaving a clean baseline to direct changes from. The only difference from main's file is the cancellable focus-flash task, kept because the asyncAfter pattern left the flash ring stuck visible. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The branch had overridden the panel icon to doc.text so notes read as plain documents; with notes now rendering through the untouched markdown viewer, the tab and path-header icon match markdown files (doc.richtext), same as main. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
noteas a project-scoped markdown surface with durable metadata in.cmux/notes/index.json. Notes can attach to a workspace, a specific surface, or a terminal surface, and the existing Markdown panel is used as a writing-first note editor while preserving live file watching and preview mode.Closes #4331.
What landed
CmuxNoteStoreowns indexed note records, stable note IDs, body paths, attachment anchors, legacy.cmux/notes/<slug>.mdcompatibility, read/write/append/delete, and list/path metadatanote.create/open/list/path/read/write/append/delete, with--attach/attachmodes for `noneNoteSupport.swift,CmuxNoteModels.swift, andCmuxNoteStore.swift; all new Swift files are wired intocmux.xcodeprojTHIRD_PARTY_LICENSES.mdTesting
testsrelease-buildremote-daemon-teststests-build-and-lagui-regressionsactivation-sessionworkflow-guard-testsweb-typecheckweb-db-migrationsgit diff --checkjq empty Resources/Localizable.xcstringsDemo Video
Not applicable. This is a feature/refactor PR and did not need the
cloud-macvisual repro path.Review Trigger
CodeRabbit, Cursor Bugbot, and Greptile reviewed the PR. Actionable review threads have been fixed or replied to.
Checklist
notesurface type (project-scoped markdown notes) #4331 note surface behaviorcmux.xcodeprojHQ Debug Build Command
Note
Medium Risk
New automation surface (CLI + socket note CRUD) and control-socket routing affect agents and concurrency; beta-gated UI limits blast radius but note file writes and attachment routing still touch workspace state.
Overview
Adds a full
cmux notecommand surface that talks to new v2 socket methods (note.create,open,list,path,read,write,append,delete) for project-scoped markdown under.cmux/notes/, including attach modes, routing flags, and human/JSON output.CLI structure is refactored: markdown open, workspace list/create/env/close/select/rename, and right-sidebar availability move into dedicated extension files;
noteis registered in command suggestions and top-level dispatch.Right sidebar gains an opt-in Notes beta mode: a new defaults key, Settings toggle, dynamic
notesinright-sidebarmode lists/help (CLI reads app UserDefaults suites so PATH-launchedcmuxmatches the app), and shortcutsswitchRightSidebarToNotes/newNote.Control socket routes all
note.*methods on the worker thread to avoid main-actor deadlocks. Terminals can receiveCMUX_WORKSPACE_NOTES_DIRvia spawn policy when a workspace notes root exists.Also adds a Pane Tab Bar Settings section (docs + global/project
cmux.jsonpointers), new drag UTTypes for notes/session trees, and a small RovoDev helper inlining refactor unrelated to notes.Reviewed by Cursor Bugbot for commit 60d4993. Bugbot is set up for automated code reviews on this repo. Configure here.