Skip to content

fix(web): browser clients can save project action shortcuts - #18769

Closed
NK-Works wants to merge 2 commits into
pingdotgg:mainfrom
NK-Works:fix/browser-action-shortcuts-18427
Closed

NK-Works wants to merge 2 commits into
pingdotgg:mainfrom
NK-Works:fix/browser-action-shortcuts-18427

Conversation

@NK-Works

Copy link
Copy Markdown
Contributor

Problem

Fixes #18427. In a browser client, recording a shortcut in a project action's Keybinding field (Settings → Project → Actions, and the chat action editor) reported success but the shortcut was never written — reopening showed the old/empty value. Both save paths skipped the keybinding upsert/remove behind isElectron gates while the editor stayed editable, so the write was silently dropped.

Change

Removed both client-side gates so browser clients persist action shortcuts through the existing keybinding RPCs:

  • apps/web/src/components/settings/useProjectScriptSettings.ts: removed if (!isElectron) continue; (plus unused import, doc comment updated).
  • apps/web/src/components/ChatView.tsx: removed if (!isElectron) return updateResult; in persistProjectScripts.

No RPC/contract changes: server.upsertKeybinding/server.removeKeybinding already require orchestration:operate (same scope as server.updateSettings) and write to the environment's server-side keybindings.json. The Keybindings settings page already allows browser writes with only a notice about browser-claimed shortcuts, so gating the editor instead would have been inconsistent. #15394 history shows the gate predates it with no recorded rationale.

Scope and approval

Triaged direction from #18427, which prefers allowing browser writes since the RPCs already check scopes. Focused two-line behavior fix plus a regression test.

Verification

  • New useProjectScriptSettings.test.tsx (3 tests: save shortcut → upsert called, clear shortcut → remove called, settings failure → no keybinding write). Fails pre-fix (2 failed with gate restored), passes post-fix.
  • Related suites: 4 files / 33 tests pass. tsc --noEmit -p apps/web clean; vp lint clean on touched files.
  • Manual browser verification against a worktree dev server: recorded Ctrl+Shift+K on a new action in Settings → Project → Actions and Alt+Shift+J via the chat action editor; both shortcuts persisted to keybindings.json/settings.json, survived reopening the editors, and appear live in the Script actions menu. Screenshots are in the linked thread (could not upload PR-only assets per repo policy).

Built with Muse Spark 1.3 Free via OpenCode.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:XS 0-9 changed lines (additions + deletions). labels Oct 12, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 12, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 3650cd8

Macroscope's review found this PR approvable — This is a small, localized bug fix that removes browser-only skips from two existing save paths and relies on unchanged, scope-protected keybinding RPCs. The added regression tests cover shortcut creation, removal, and settings-write failure without introducing schema, deployment, or static-analysis changes.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 2827fb8b-0344-4811-bfad-18b5b4dd7228
📥 Commits

Reviewing files that changed from the base of the PR and between 3650cd8 and 35c414f.

📒 Files selected for processing (3)
  • apps/web/src/components/ChatView.tsx
  • apps/web/src/components/settings/useProjectScriptSettings.test.tsx
  • apps/web/src/components/settings/useProjectScriptSettings.ts
💤 Files with no reviewable changes (1)
  • apps/web/src/components/ChatView.tsx

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

Project-script keybinding updates now run in browser and other non-Electron clients after settings are saved. Tests cover shortcut creation, shortcut removal, failed settings writes, and denied settings-write scope.

Changes: Project-script shortcut synchronization

Layer / File(s) Summary
Project-script shortcut synchronization
apps/web/src/components/settings/useProjectScriptSettings.ts, apps/web/src/components/ChatView.tsx, apps/web/src/components/settings/useProjectScriptSettings.test.tsx
The settings hook and chat editor no longer skip keybinding processing outside Electron. Tests cover shortcut upserts, removals, failed settings writes, and denied settings-write scope.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 35c41

Browser shortcut saves use the existing settings-write authorization path. No actionable issue remains before merge beyond normal checks.

Security architecture risk: 🔵 Low · up to 35c41

Browser saves use existing permission-protected operations and do not grant new privileges. Settings and shortcuts are saved separately, so interruption or failure can leave them temporarily inconsistent.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The mutation scope is environment-level shortcut configuration, not an isolated project-only preference. The settings hook can reconcile several selected environments, but it checks write permission for every target and routes each operation using that target's identity.

Security Findings and Attack Paths

  • inferred — The removed client gates do not establish a new privilege-escalation path: browser users already had access to the same authorized upsert/remove operations through Keybindings settings. The change adds action-editor reachability, not additional server authority.

Trust Boundaries and Controls

  • observed — Browser-supplied shortcut rules cross an authenticated WebSocket boundary. Authorization middleware attached to the RPC group evaluates the connection's session scopes before allowing the handler effect to run, so client-side checks are not the sole enforcement point.

Resilience and Maintainability Implications

  • observed — The settings hook rejects overlapping saves within that hook instance and releases its saving guard in finally. Chat persistence rechecks shortcut permission after settings success. These controls contain some interruption and permission-change cases but do not make the multi-step update transactional.
Pre-merge checks · Passed 4
Passed Passed checks (4 passed)
Check name Status Explanation
Linked Issues check Passed The PR satisfies #18427's browser-write option. useProjectScriptSettings.ts now runs keybinding synchronization without the isElectron gate. ChatView.tsx now evaluates and writes keybinding chan…
Out of Scope Changes check Passed The changes remain within #18427. The source changes remove the two client-side gates and update related documentation. The added test file verifies the changed behavior and failure paths. No unrelate…
Title check Passed The title clearly and concisely describes the main change: browser clients can save project action shortcuts.
Description check Passed The description covers the problem, implementation, scope approval, and focused verification. It identifies the issue, changed files, RPC behavior, tests, type checking, linting, and manual browser ch…
✨ Finishing Touches 💡 1
🧪 Generate unit tests (beta)
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

Recording a shortcut in a project action's Keybinding field reported success but silently dropped the write outside Electron: both save paths skipped the keybinding upsert/remove behind isElectron gates. The keybinding RPCs already enforce the caller's scopes and persist to the environment's server-side keybindings.json, and the Keybindings settings page already allows browser writes, so the gates are removed instead of gating the editor.
Main introduced AuthSettingsWriteScope, effect 4.0.2 (effect/reactivity), tsc-rs, and NewProjectScriptInput.runOnSettle since this branch was cut. Update the regression test to the new APIs, mock readEnvironmentScope (including a no-scope explicit-failure case), and remove the remaining isElectron term from ChatView's changesKeybinding so scoped browser clients persist shortcuts; scoped-out clients get the explicit keyboard-shortcut failure instead of a silent drop.
@NK-Works
NK-Works force-pushed the fix/browser-action-shortcuts-18427 branch from 3650cd8 to 35c414f Compare October 13, 2026 03:35
@NK-Works NK-Works closed this Oct 13, 2026
@NK-Works NK-Works reopened this Oct 13, 2026
@NK-Works NK-Works closed this Oct 13, 2026
@NK-Works NK-Works reopened this Oct 13, 2026
@t3dotgg

t3dotgg commented Oct 14, 2026

Copy link
Copy Markdown
Member

Note

🤖 Claude Opus 5.5 responding on behalf of Theo

Closing because this PR states that Muse Spark wrote code in it:

Built with Muse Spark 1.3 Free via OpenCode.

We do not accept code written by Gemini models or by Muse models other than Muse Wonder Full. See Models we do not accept.

To have this considered again, open a new PR with the code written by a different model and link this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XS 0-9 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Action shortcuts recorded in the browser are never saved

2 participants