Move config to ~/.config/roost/config.conf with externalized keybindings - #7
Merged
Conversation
The hand-edited config file now lives at ~/.config/roost/config.conf
(or $XDG_CONFIG_HOME/roost/config.conf) on both macOS and Linux. The
SQLite database and IPC socket stay in their platform-native locations
(~/Library/Application Support/Roost on macOS, $XDG_DATA_HOME on
Linux).
Keybindings move out of cmd/roost and into the config file using
Ghostty's syntax:
keybind = super+j = new_tab # add a second trigger
keybind = super+t = unbind # disable a default
keybind = super+t = close_tab # reassign
Each keybind line is applied on top of the platform defaults; later
lines override earlier ones for the same trigger (last-wins). Unknown
actions and unparseable triggers are logged and skipped.
Implementation notes
- internal/config/paths.go: ConfigDir is now XDG-aware on darwin too;
ConfigFile() is config.conf instead of config.toml. New
LegacyMacConfigFile() points at the pre-cutover path so main.go can
log a one-shot migration warning when only the legacy file exists.
- internal/config/config.go: adds Keybinds []Keybind to Config and a
keybind arm to the parser. parseKeybind splits on the inner =;
errors include file:line.
- cmd/roost/shortcuts.go (new): triggerToAccel converts Ghostty
trigger syntax to a GTK accelerator string; resolveBindings is a
pure function that layers user keybinds on top of defaults
(unit-tested without a live widget tree).
- cmd/roost/app.go: installShortcuts is rewritten as an action-table
driven loop. The propagate-false path for clipboard actions is
preserved (returning false in the gated callback so an editable
widget keeps its native paste).
- cmd/roost/main.go: drops the package-global font vars; cfg threads
through NewApp -> NewSession.
Breaking change: a pre-existing config.toml is not auto-migrated.
Roost logs a warning at startup with the move command. The hard
cutover is fine because Roost is early-stage.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughPlumbs parsed config (fonts and user keybinds) into App and Session, implements Ghostty-style keybind parsing and canonicalization (including Changes
Sequence Diagram(s)sequenceDiagram
participant Main as Main
participant Loader as Config\ Loader
participant App as App\ Init
participant Resolver as resolveBindings
participant Parser as triggerToAccel
participant GTK as GTK
Main->>Loader: Load config.conf (parse Keybinds)
Loader-->>Main: return Config (incl. Keybinds)
Main->>App: NewApp(cfg)
App->>Resolver: resolveBindings(defaults, cfg.Keybinds)
Resolver-->>App: map[trigger]action
loop each resolved trigger
App->>Parser: trigger string
alt parsed
Parser-->>App: accel string
App->>GTK: install accelerator (sorted deterministic order)
else parse fail
Parser-->>App: ok=false (log warning)
end
end
GTK-->>App: shortcuts registered
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/roost/app.go`:
- Around line 964-980: The loop over resolveBindings(defaultBindings(),
a.cfg.Keybinds) is non-deterministic because Go map iteration order can vary and
different trigger strings can alias to the same GTK accel via triggerToAccel;
change the logic to produce a deterministic, de-duplicated set of accels before
calling addGated/addUnconditional: iterate the resolved bindings, convert each
trigger to accel using triggerToAccel, and populate a temporary
map[string]{trigger,action,sa,gated} keyed by accel, choosing a deterministic
winner when collisions occur (e.g., sort triggers or use a stable precedence
such as user keybinds over defaults or lexicographic order of trigger/action),
then extract the keys, sort them, and register accels in that stable order by
calling addGated/addUnconditional with the chosen handler (use resolveBindings,
triggerToAccel, handlers, addGated, addUnconditional, and a.cfg.Keybinds to
locate code).
In `@cmd/roost/main.go`:
- Around line 66-84: In warnLegacyMacConfig, the migration hint "mv "+legacy+"
"+p.ConfigFile() is not shell-safe; change the hint to produce quoted paths
(e.g. use fmt.Sprintf("mv %q %q", legacy, p.ConfigFile()) or strconv.Quote on
each path) so spaces and special characters are preserved when copy/pasting;
update the slog.Warn call to use the new quoted hint string while keeping the
same keys ("old", "new", "hint").
In `@internal/config/paths_test.go`:
- Around line 21-31: The macOS test branch should not hard-code ".config" —
update the assertion in the runtime.GOOS == "darwin" block to validate ConfigDir
dynamically (based on XDG_CONFIG_HOME when set, falling back to ~/.config)
rather than checking for the literal ".config/"+AppName; use the same logic
Resolve() uses (or compute expectedConfig via XDG_CONFIG_HOME or default
home/.config and filepath.Join with AppName) and assert p.ConfigDir contains
that expectedConfig (keep the existing checks for p.DataDir and p.RuntimeDir
untouched).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4808ce5b-3dde-44b0-b849-e4332b621d24
📒 Files selected for processing (13)
cmd/roost/app.gocmd/roost/main.gocmd/roost/session.gocmd/roost/shortcuts.gocmd/roost/shortcuts_test.godocs/development/spec.mddocs/getting-started/first-run.mddocs/getting-started/keybindings.mddocs/reference/paths.mdinternal/config/config.gointernal/config/config_test.gointernal/config/paths.gointernal/config/paths_test.go
Caught by macOS lint job. The longer "super+bracketleft" entry was already padded to its column; the shorter entries needed to extend to match. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ixes - cmd/roost/app.go: sort the resolved triggers before installing shortcuts. When two distinct triggers in the user's config normalize to the same GTK accel (e.g. `ctrl+t` and `control+t`), Go's random map iteration would otherwise pick the winner non-deterministically. Sorting gives lexicographic last-wins. - cmd/roost/main.go: quote both paths in the legacy-config migration hint with %q so the macOS path (which contains spaces in "Library/Application Support/Roost") is copy-paste safe. - internal/config/paths_test.go: don't hard-code the default ~/.config layout in the macOS branch of TestResolve — a developer with XDG_CONFIG_HOME set would spuriously fail. Compare on basename instead. TestResolveConfigDirRespectsXDG already covers the env-var path explicitly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/roost/app.go`:
- Around line 971-988: resolveBindings(defaultBindings(), a.cfg.Keybinds)
applies user overrides before validation which lets invalid user entries erase
defaults; change the logic so defaults are preserved by validating user entries
first and only applying valid overrides: iterate a.cfg.Keybinds (or modify
resolveBindings) to check that the user-specified action exists in handlers and
that triggerToAccel(trigger) parses successfully, then merge those valid
overrides into defaultBindings(); keep using handlers and triggerToAccel for
validation and ensure triggers (from defaultBindings()) are not removed by
invalid user entries.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: beb4f330-3b65-4d6a-8b3a-da2c65dfb9b1
📒 Files selected for processing (3)
cmd/roost/app.gocmd/roost/main.gointernal/config/paths_test.go
✅ Files skipped from review due to trivial changes (1)
- cmd/roost/main.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/config/paths_test.go
Closes a UX issue CodeRabbit flagged on #7: with the previous flow, a typo in a user keybind action (e.g. `keybind = ctrl+t = typo`) would write the typo into the resolved trigger map, then get dropped in the install loop's handler-lookup, leaving `ctrl+t` unbound. The default `new_tab` binding was silently lost. Now installShortcuts seeds the resolved set with platform defaults only and merges each user keybind entry-by-entry, validating trigger and action before overwrite. Invalid entries log a warning and the default binding is preserved. `unbind` continues to work as before. resolveBindings stays purely structural — its existing test suite is unchanged and a clarified comment on the unknown-action test pins that contract for future callers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/roost/app.go`:
- Around line 965-988: The merge loop uses the literal kb.Trigger as the map key
so alias-equivalent triggers don't override/unbind defaults; use the canonical
form returned by triggerToAccel() as the key instead. Specifically, in the loop
that iterates a.cfg.Keybinds (working with resolved :=
resolveBindings(defaultBindings(), nil)), call triggerToAccel(kb.Trigger) once,
use the canonical accel value as the map key for delete/assign, and log using
the original string for clarity; still validate unknown actions against handlers
and preserve the existing unbind/path checks. After updating resolved with
canonical keys, ensure the later accel installation uses those canonical keys
and add a regression test covering alias pairs like "super+t" vs "cmd+t" and
"ctrl+t" vs "control+t".
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ff917dff-b0cf-4c1f-8389-e6906fa1beba
📒 Files selected for processing (2)
cmd/roost/app.gocmd/roost/shortcuts_test.go
Closes the third (and last) of CodeRabbit's PR #7 review threads: the install pipeline keyed its resolved map by literal trigger text, so `keybind = cmd+t = unbind` did NOT remove the macOS default seeded as `super+t` even though both normalize to `<Meta>t`. Same asymmetry for `cmd+t = close_tab` — it ended up alongside `super+t` instead of replacing it. Fix: extract canonicalizeBindings as a pure function that keys the resolved map by the GTK accel form (the output of triggerToAccel). Aliases collapse on insertion, unbind matches by accel, and the production install loop iterates already-canonical accels. The function bundles three jobs that were previously split across shortcuts.go and the install loop in app.go: (1) alias collapse, (2) action validation, (3) trigger validation. The warn callback keeps it pure (no slog import in tests). resolveBindings stays as a structural-merge primitive for the existing test suite, with its docstring updated to point at canonicalizeBindings as the production path. Tests added for the four canonicalization invariants: - super+t default + cmd+t = unbind → {} (alias collapse) - super+t default + cmd+t = close_tab → {<Meta>t: close_tab} - ctrl+t default + ctrl+t = typo → default preserved + warn - ctrl+t default + hyper+t = close → default preserved + warn - ctrl+t default + control+t = unbind → {} (alias unbind) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/roost/shortcuts.go (1)
62-83: Consider removing or marking resolveBindings as test-only.The docstring notes this function is no longer called in production, kept only for tests. To make this clearer and prevent accidental production use:
Option: Move to test file or add explicit test-only marker
Either move this function to
shortcuts_test.go(if it's only used there), or add a more prominent marker:-// resolveBindings layers user keybinds on top of the platform defaults -// and returns a map from Ghostty trigger → action name. Pure structural -// merge: literal trigger strings as keys, no validation. Production no -// longer calls this — installShortcuts uses canonicalizeBindings, which -// collapses aliases and validates user entries. Kept for the tests -// that pin the structural-merge contract. +// resolveBindings is a test helper that layers user keybinds on top of +// platform defaults. Returns a map from Ghostty trigger → action name. +// Pure structural merge: literal trigger strings as keys, no validation. +// +// NOTE: Production code uses canonicalizeBindings instead. This function +// is retained only for tests that pin the structural-merge contract. func resolveBindings(defaults map[string][]string, user []config.Keybind) map[string]string {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/roost/shortcuts.go` around lines 62 - 83, resolveBindings is flagged as test-only but lives in production code; either move the function into the test package (e.g., shortcuts_test.go) where tests use it, or mark it explicitly as test-only by adding a clear comment and/or a build tag so it cannot be pulled into production builds; update references to resolveBindings (and mention related symbols installShortcuts and canonicalizeBindings) to point to the new test location or keep the current symbol but place it in a _test.go file to ensure it is only compiled for tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/roost/shortcuts.go`:
- Around line 62-83: resolveBindings is flagged as test-only but lives in
production code; either move the function into the test package (e.g.,
shortcuts_test.go) where tests use it, or mark it explicitly as test-only by
adding a clear comment and/or a build tag so it cannot be pulled into production
builds; update references to resolveBindings (and mention related symbols
installShortcuts and canonicalizeBindings) to point to the new test location or
keep the current symbol but place it in a _test.go file to ensure it is only
compiled for tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d4743c08-8448-4dcf-8c36-f549649f4f85
📒 Files selected for processing (3)
cmd/roost/app.gocmd/roost/shortcuts.gocmd/roost/shortcuts_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/roost/shortcuts_test.go
# Conflicts: # docs/getting-started/keybindings.md
This was referenced Apr 29, 2026
3 tasks
charliek
added a commit
that referenced
this pull request
May 23, 2026
CodeRabbit on d2aa609 flagged 7 actionable items. All valid; fixed. 1. **Duplicate dep declarations (CRITICAL)** — `Cargo.toml` had `gtk4`, `libadwaita`, `glib`, `pangocairo`, `bitflags` listed both under `[dependencies]` and again at the file tail where they silently fell under `[dev-dependencies]` (no section header between). Removed the duplicate set. Cargo was silently merging them; removal is a no-op at runtime but eliminates the silently-using-the-last-definition footgun. 2. **`PtySupervisor::close` didn't terminate the child** — the legacy daemon's `close()` only dropped the input/resize senders, so a long-running shell would survive arbitrarily long until it noticed the dropped writer (which only happens on the next read/write inside the shell). Acceptance criterion in the plan: "Clean quit reaps all PTY children (SIGHUP + waitpid)" requires an active signal. Each `Session` now stores a sendable `Box<dyn ChildKiller + Send + Sync>` obtained via `Child::clone_killer()`; `close()` invokes it. The waiter task spawned at `spawn()` time reaps via `child.wait()` once the kill lands. ESRCH ("child already gone") is treated as success. 3. **`spawn()` silently replaced existing sessions for the same tab_id** — second spawn would orphan the first PTY child and broadcast channel. `spawn()` now returns `Err(PtyError::DuplicateTab(tab_id))`; callers must `close()` the prior session first. Workspace contract is unchanged (`tab.open` always allocates a fresh id, so this only matters for test/manual abuse). Bonus: `spawn()` now returns the `broadcast::Receiver<PtyOutputEvent>` subscribed BEFORE the reader task starts producing. Eliminates the race where early bytes/exit could be lost between `spawn()` returning and a `subscribe_output()` call. Late subscribers can still join via `subscribe_output()` — broadcast keeps a per-subscriber buffer. 4. **Missing parent-dir fsync after rename** in `store_json::persist_state`. POSIX requires fsync(parent) after `rename(tmp, path)` for the directory entry update to be durable across crashes. Added a best-effort `OpenOptions::new().read(true).open(parent)?.sync_all()` after the rename. EINVAL (tmpfs, etc.) treated as success — the rename itself is atomic; we just can't force the sync. 5. **`InstanceLock::drop` was racing the unlink** — the previous impl called `remove_file(&path)` from `Drop`, but the file handle hadn't been dropped yet (Drop body runs *before* fields drop). That meant another process could observe the path-less file briefly. Removed the unlink from `Drop` entirely (stale lock files left after clean exit are harmless — the next `acquire()` truncates + rewrites the PID after taking the flock). Added an explicit consuming `release(self)` method for callers that want explicit cleanup; it drops the handle before unlinking. 6. **Flat `tokio::time::sleep(50ms)` before `IpcClient::connect`** in `ipc_dispatch.rs` was flaky on slow CI. Replaced with a bounded retry loop (5ms exponential backoff, 100ms cap, 2s deadline). Applied to both call sites. 7. **`pty_smoke.rs` could miss early output** because `subscribe_output()` was called AFTER `spawn()`. Fixed structurally via change (3) — `spawn()` now returns the subscribed receiver. Both existing tests use the returned receiver. Added `duplicate_spawn_for_same_tab_id_is_rejected` to cover change (3) — including the close-then-respawn-succeeds path. 8. **`state_persist.rs` weak assertion**. The test asserted `next_tab.id > project_id` which was technically true but didn't actually verify the next-id counter advanced past the previous tab. Now captures `first_tab_id` and asserts `next_tab.id > first_tab_id`. (CR-numbered the same as inline #7 — this is the 8th distinct fix.) Verify: - `cargo build --workspace` clean. - `cargo test -p roost-linux --lib --tests`: 15 lib + 34 bin + 2 ipc + 3 pty + 3 persist = **57 tests pass**. - `cargo fmt --all -- --check` clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
charliek
added a commit
that referenced
this pull request
May 23, 2026
* fix(linux): Linux IPC robustness sweep (closes #80) Address the deferred CR items from the inline-core refactor (PR #78). Reconciled against the merged code: #1 (parse-error reply), #10 (pending HashSet), #13 (smoke-test ordering) were already done; the rest are fixed here. Project is early-stage, so the bar is long-term correctness. PTY supervisor (crates/roost-linux/src/daemon/pty.rs, tab_session.rs): - #11: wait task now removes the session on child exit (sessions behind Arc<Mutex>), so writes to a dead PTY return NotFound instead of silently succeeding. - #12: close() escalates SIGHUP -> SIGKILL after a grace period. The cloned portable-pty ChildKiller only sends SIGHUP, so a shell that traps/ignores it used to outlive close(); a reaped flag + pid watchdog force-kill it, mirroring the Mac teardown. - #8: send_input/send_resize feed one per-tab serial channel drained by a single task, so keystrokes can't reorder (the old per-call tokio::spawn could under the multi-thread runtime). Workspace state (crates/roost-linux/src/daemon/state.rs): - #4: new project/tab position uses max(position)+1, not len()/count(), so a delete-then-create can't collide. - #5: every mutator now publishes its event while still holding `inner`, so broadcast order matches commit order; persist runs after the drop. The events-resync Resync path is preserved. Server framing (crates/roost-ipc/src/server.rs): - #2: on an envelope decode failure, peel `id` from the raw JSON (string- or number-encoded) so the parse-error reply lands at the client's id, not 0. Lifecycle (crates/roost-linux/src/main.rs, ipc.rs, app.rs, messages.rs): - #7: IPC bind is now a synchronous startup requirement; bind failure aborts startup instead of leaving the UI socket-less. - #6: a second launch dials the new `app.activate` op; the handler forwards to the GTK thread which raises the running window. - #9: events.subscribe returns not-implemented (Linux + Mac) instead of a false {} ACK, so clients don't wait forever. Real streaming is deferred to its first consumer (roostctl watch). Tests: write-after-exit -> NotFound, SIGHUP-ignoring child force-killed, send_input ordering, position max+1 after delete, id-preserving parse error, bind-failure surfaced, events.subscribe not-implemented. Rust workspace + Mac build/clippy/tests green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(mac): IPC handler dispatch harness (#80 follow-up) The Rust handler has tests/ipc_dispatch.rs; the Mac IPCHandlerImpl had no equivalent, leaving its hand-written cross-cutting logic untested — strict unknown-field rejection (decodeParams), ipcDim u16 validation, mapWorkspace/mapPty error-code mapping, the not-implemented/unknown-op paths, and result encoding. The two handlers must stay behaviorally convergent over the shared wire contract, so this guards that. Calls IPCHandlerImpl.handle(op:params:) directly — no socket. Exercises only non-PTY-spawning ops to avoid the forkpty/swift-testing SIGTRAP that disables the PTY paths elsewhere (tab.open stays on the manual pass; the error-mapping ops reach the supervisor only on the lookup-fails path). Seeds 8 tests: events.subscribe→not-implemented (the #9 path), unknown-op, unknown-field strictness, project.rename→not-found, tab.resize out-of-range→invalid-param, tab.resize missing→not-found, identify profile echo, project.create→tab.list round-trip. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(linux): address CodeRabbit review on #80 sweep Resolves 4 of CodeRabbit's 6 findings on PR #86 (the persistence-ordering finding is pre-existing/orthogonal and tracked separately). pty.rs: - Major: spawn() acquired the master reader/writer AFTER spawn_command, so a try_clone_reader/take_writer failure returned with a live child and no wait task — an orphaned PTY. Acquire reader+writer BEFORE spawning, making spawn_command the last fallible step (no orphan). - Major: the exit wait task removed the session by tab_id alone, which could evict a newer session that reused the same tab_id (close() frees the slot synchronously; pty_smoke reuses id 42). Gate the remove on Arc::ptr_eq of the per-spawn `reaped` identity so only the owning waiter deletes. (My prior "monotonic ids" comment over-assumed caller discipline; the supervisor can't rely on it.) - Major: input and resize were unified at TabSession but re-split into the supervisor's two channels (select!), so mixed input/resize wasn't FIFO end-to-end. Merge them into one WriterCmd channel drained by a single ordered loop — genuine submission-order delivery. ipc.rs + messages.rs: - Minor: app.activate ACK'd any payload. Add an empty strict AppActivateParams and decode it so the op validates its envelope (rejects unknown fields) like every other op. IPCHandlerTests.swift: - Minor: replace `as? T != nil` (SwiftLint prefer_type_checking) with a meaningful empty-array assertion on a fresh project's tabs. Rust clippy --all-targets clean; roost-ipc + roost-linux tests green (A3 ordering + dup-spawn still pass under the new channel/guard). Mac swift build + 140 tests green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(linux): order state.json persistence by commit seq (CR #3) Resolves the last CodeRabbit finding on PR #86. persist_async did synchronous file I/O on the caller thread after dropping `inner`, so two concurrent mutators could write state.json out of commit order — a slow earlier commit could clobber a newer one, regressing restart durability. Keep persistence synchronous (state_persist.rs and restart-reload depend on it; an async writer thread would break that), but make it ordered: - Inner.persist_seq: a monotonic commit counter bumped under the lock when a snapshot is taken, so the seq reflects commit order. - snapshot_for_persist now returns (SnapshotFile, seq). - persist(seq, snapshot) serializes on a new persist_guard mutex and drops any snapshot whose seq is <= the highest already written, so the newest committed snapshot always wins regardless of which thread races first. Newest-wins verified by persist_drops_stale_out_of_order_writes. Rust clippy --all-targets clean; roost-ipc + roost-linux suites green (incl. state_persist). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * style: rustfmt ipc.rs import block The AppActivateParams import addition left the use block un-reflowed; CI's rust-lint (cargo fmt --all -- --check) caught it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
charliek
added a commit
that referenced
this pull request
May 26, 2026
CodeRabbit (#106): `RoostBackend.shared.start()` exposes the socket before `registerUI(self)` ran, so `RoostBackend.shared.ui` was nil for a brief launch window and bridge-backed ops (`tab.dump`/`app.screenshot`) would report a misleading "no UI attached". Register the bridge immediately *before* `start()`. Storing `self` is side-effect-free and `self` is fully initialized as the app delegate, so this is safe. The window + tabs are still built afterward (the socket binds early by design so `identify` works at launch, #7), so those ops surface their own honest, retryable errors until the UI is up ("no window" / "not-found") instead of a nil-bridge internal error. Verified live: screenshot + tab.dump work post-launch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
charliek
added a commit
that referenced
this pull request
May 26, 2026
CodeRabbit (#106): `RoostBackend.shared.start()` exposes the socket before `registerUI(self)` ran, so `RoostBackend.shared.ui` was nil for a brief launch window and bridge-backed ops (`tab.dump`/`app.screenshot`) would report a misleading "no UI attached". Register the bridge immediately *before* `start()`. Storing `self` is side-effect-free and `self` is fully initialized as the app delegate, so this is safe. The window + tabs are still built afterward (the socket binds early by design so `identify` works at launch, #7), so those ops surface their own honest, retryable errors until the UI is up ("no window" / "not-found") instead of a nil-bridge internal error. Verified live: screenshot + tab.dump work post-launch. Co-Authored-By: Claude Opus 4.7 (1M context) <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
1 participant
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
~/.config/roost/config.conf(or$XDG_CONFIG_HOME/roost/config.conf) on both platforms. State files (database, socket) keep their native locations.keybind = trigger=action. Defaults still ship in code; user lines layer on top.cmd/roost/installShortcutsbecomes an action-table driven loop. The propagate-false path for clipboard actions is preserved so paste still works in the sidebar rename entry.Why this shape
The Mac config path divergence (XDG instead of
~/Library/Application Support) is deliberate and matches Ghostty / nvim / fish / most CLI-adjacent tools. The trade-off is documented ininternal/config/paths.goanddocs/reference/paths.md. State files (which the user does not edit) stay in the conventional Mac location.The Ghostty keybind syntax is reused so users coming from Ghostty find the format familiar:
Last-wins per trigger across multiple
keybindlines. Unknown actions and unparseable triggers are logged and skipped.Breaking change
A pre-existing
~/Library/Application Support/Roost/config.tomlis not auto-migrated. Startup logs a warning with the move command:Roost is early-stage; a hard cutover is acceptable.
Out of scope
global:,all:,unconsumed:,performable:)text:/csi:/esc:)gtk_application_set_accels_for_action) — the action-table refactor is a prerequisite, but the menubar wiring is its own PRTest plan
go test ./...— green (config, core, store, app, ipc, osc, pty, ghostty)./build/build.sh— producesroostandroost-clikeybindparsing, whitespace tolerance, multiple lines, malformed input, empty trigger, empty action, leading-#comments, and the trailing-#-NOT-stripped behaviorConfigFile()resolution with and withoutXDG_CONFIG_HOME, plus the macOS regression check thatDataDir/RuntimeDirstill resolve to~/Library/Application Support/RoosttriggerToAccelmatrix covers every modifier alias and case-insensitive parsing; rejects unknown modifiers and empty inputresolveBindingssemantics tests cover: empty user list, additive trigger,unbindremoves default, reassignment, idempotentunbind,unbindof unknown trigger (silent), last-wins, and unknown-action survives to the install loop~/.config/roost/config.conf:keybind = super+j = new_tab→ both Cmd-J and Cmd-T open new tabskeybind = super+t = unbind→ Cmd-T does nothing; Cmd-J still openskeybind = super+t = close_tab→ Cmd-T closes; Cmd-W still closes; Cmd-J still openskeybind = nonsense) → startup fails withconfig: <path>:<line>: keybind: ...Cmd-Vpastes into it (theaddGatedpath)~/Library/Application Support/Roost/config.tomlwith no new file; restart logs the migration hint🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests