keybindings ux improvments - #90
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (9)
WalkthroughThe changes introduce a keybindings configuration validation and state management system. The keybindings module now exposes Changes
Sequence DiagramsequenceDiagram
participant FileSystem
participant Server as Keybindings Module
participant PubSub
participant WSServer as WebSocket Server
participant Client as Web Client
participant UI as Toast Manager
FileSystem->>Server: File change detected
activate Server
Server->>Server: Validate config<br/>(decode entries,<br/>accumulate issues)
Server->>Server: Update cache<br/>(KeybindingsConfigState)
Server->>PubSub: Emit change event<br/>(issues)
deactivate Server
PubSub->>WSServer: Notify subscribers
activate WSServer
WSServer->>WSServer: Create serverConfigUpdated<br/>push (issues payload)
WSServer->>Client: Broadcast to all clients
deactivate WSServer
activate Client
Client->>Client: Receive update
Client->>Client: Compute signature<br/>from issues
Client->>Client: Deduplicate<br/>(compare with last)
alt Issues detected
Client->>UI: Show warning toast<br/>(issue message +<br/>editor action)
else No issues
Client->>UI: Show success toast<br/>(config reloaded)
end
Client->>Client: Invalidate queries
deactivate Client
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
|
Too many files changed for review. ( |
842321b to
51dbea2
Compare
There was a problem hiding this comment.
🟢 Low src/wsServer.ts:113
When a multi-byte UTF-8 character is split across array chunks, decoding each chunk individually corrupts the data. Consider concatenating all buffers first with Buffer.concat(), then decoding to UTF-8 once.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/server/src/wsServer.ts around line 113:
When a multi-byte UTF-8 character is split across array chunks, decoding each chunk individually corrupts the data. Consider concatenating all buffers first with `Buffer.concat()`, then decoding to UTF-8 once.
Evidence trail:
apps/server/src/wsServer.ts lines 113-131 (commit REVIEWED_COMMIT): The `websocketRawToString` function handles `Array.isArray(raw)` by iterating over chunks and calling `Buffer.from(chunk).toString('utf8')` on each Uint8Array/ArrayBuffer individually (lines 121-127), then joining with `chunks.join('')` (line 130). This decodes each chunk separately before concatenation, which corrupts multi-byte UTF-8 characters split across chunk boundaries.
Merges `upstream/main` (`d937e3075`, 62 commits from base `b17cc3d1b`) into the fork. Conflicts resolved per the `fork-upstream-merge` skill; governance docs updated and folded into the merge commit. ## What landed - Landed `562` files (`HEAD^1..HEAD`) vs `560` in the upstream range — gap of two: fork resolution edits (`features.ts`, `moatless/listSearch.ts`) plus `docs/fork/gaps.md`, against `apps/server/src/cli/pair.ts` (already deleted fork-side). Fork delta `626` files. - 14 textual conflicts resolved. Sweep clean (`duplicate-adds`, `tripwires`); `unsupported-methods` ADD count `0`, only the standing `scripts.run` DROP exception remains. ## Notable convergence Upstream moved thread settlement server-side (dropped client `effectiveSettled`, now renders `thread.settledOverride === "settled"`). Moatless already owns settlement and never settles from PR state, so the fork's `prThreadSettling` gate is now redundant — **retired** the flag, its guard, its convergence entry, and the related gaps.md sections. The only fork settlement delta kept is pin-before-settled ordering in the sidebar. ## New upstream features — classification **Usable as-is** - HTML/PDF/media rendering in the file viewer, and opening markdown/HTML/PDF files outside the workspace. - `serverScoped` settings rows. - Server-side thread settlement via `settledOverride` (Moatless already computes this). **Unsupported in Moatless** - Desktop remote update system (`server.commitDesktopUpdate`) — declared `UnsupportedMethodError` in `packages/contracts/src/rpc.ts`, referencing gaps.md "Desktop and host lifecycle". **Backend behavior to consider reproducing** - None new — settlement is already implemented Moatless-side. ## Verification `verify.mjs --fast` fmt/lint/typecheck clean. Tests pass: web `3348`, desktop `689`, mobile `1082`, relay `209`, server `3125` (`10` skipped). `unsupported-methods` exits 1 only for the documented `scripts.run` exception. Merge and resolution done by Claude Opus 4.8 via Claude Code, following the project-local `fork-upstream-merge` skill. --- Moatless task: https://moatless.soaplabstest.com/tasks/ef3f3a62-fa94-4dc0-9bc3-3612a6a4a8de
…ngdotgg#106) The 2026-09-02 merge (pingdotgg#90) lost work twice to sandbox restarts. Conflicted files came back with their `<<<<<<<` markers and were obvious; files edited as collateral of resolving a conflict elsewhere reverted in silence, and one was caught only because typecheck happened to fail on an export it had removed. That merge got lucky. Both files it lost were fork-only, and fork-only files already have a detector — the inventory's `guard` entries are what turned `features.ts` losing `FEATURES.prThreadSettling` into a failing test. **Nothing covered the other half.** Had the restart reverted the resolution in an upstream-owned file instead — `Sidebar.tsx`, `ws.ts`, any of the fourteen that conflicted — no guard names it, `merge-stats.mjs` only counts files and one file is lost in the noise, and the merge is green. ## What this adds **`resolution-check.mjs`** reads each candidate path against both merge parents and reports where the result contradicts its verdict: a `converged` path byte-identical to upstream has lost its delta; one identical to the fork's pre-merge copy never took upstream's change. It runs mid-merge as well as after, because before the commit exists a finding is still a plain edit. Calibrated against pingdotgg#90's own merge commit, which it passes clean. Reverting a delta there produces: ``` XX apps/web/src/components/Sidebar.tsx landed byte-identical to upstream [mobile-touch-upstream-files, converged] Take upstream, then apply Mobile Touch Delta. […] all must survive. `// Fork:` markers 4 → 0. ``` Three scoping decisions, each of which would otherwise have shipped a permanently-red check: - Rule 3 covers `theirs-verbatim` only, never plain `theirs` — `pnpm-lock.yaml` is the only `theirs` path and is meant to differ from upstream forever. - `decide` only warns, and only where the fork side carried a `// Fork:` marker. `apps/web/src/browser/**` is six files the fork has never touched; landing on upstream there is not a resolution. - The marker census is reported, never enforced. `apps/server/src/bin.ts` carries its whole delta as a *deletion*, so the census reads `0 → 0` either way. A signal blind to half its failure class does not get a vote. It says nothing about fork-only (`ours`) paths and cannot — with no upstream side there is no third reference. Guards cover those. Stated in the file header. **Exceptions as data.** `unsupported-methods.mjs` exited 1 on every merge for `scripts.run`, whose derivation reads backwards because it is upstream's own server that refuses it. Exceptions now live in `unsupportedMethodExceptions` in `inventory.json`, each carrying the condition that retires it, printed under `KNOWN EXCEPTIONS` and not counted. The script also names an exception that has stopped firing, so they get deleted rather than accumulate. On unmodified `main` the check reports 4 DROPs; it now reports the 3 real ones. **Those three are genuine pre-existing backend drift** (`server.getUsageSummary`, `subtasks.list`, `threads.getShell`) that the permanent failure had been sitting next to. Left for a separate PR — they need `rpc.ts` and `gaps.md` edits, which is a different concern. **Procedure.** Commit the merge as soon as the markers are gone, then `--amend` through the fix cycle: in this sandbox only committed history survives a restart, so a gitignored state file is not crash insurance and committing early is. Plus a restart-recovery checklist, never `git add -A` during a merge (the restarts wiped a symlink and a submodule gitlink, which `-A` stages as deletions the merge appears to have made), and re-check `pnpm-lock.yaml` before amending. **Delegation.** A documented section: what splits (convergence validation, feature classification, gaps reconciliation), that conflicts split **by concern, not by file** — pingdotgg#90's settings-search work spanned five files and dropping a re-export in one broke another — and that `verify.mjs` must not be delegated, since its packages already contend for one sandbox's CPU. `preflight.mjs --json` emits the forecast grouped by concern. Sonnet for these agents. ## Verification `resolution-check.mjs` green on pingdotgg#90's merge commit; fails correctly when a delta is reverted (rule 1a) and when an upstream change is dropped (rule 2a); catches a silent revert in a real in-progress merge before any commit exists, skipping the 14 unresolved paths. `preflight --json` parses with empty stderr and keeps the staleness gate. `features.test.ts` 16 passed. lint and typecheck clean. Two `verify.mjs --fast` steps remain red, both pre-existing on `main` and neither touched here: the three real DROPs above, and `fmt:check` on `apps/web/src/fork/mermaidDiagram.ts` and `docs/fork/upstream-merge-inventory.md` (both byte-identical to `main`). Reviewed adversarially by a subagent, which found four real defects since fixed: `--json` skipped the inventory staleness gate and always exited 0; `report.failed` was used as a per-section gate so one failure blanked out later sections' tallies; `--package` dropped OOM attribution and would report a killed suite as a plain test failure; and `autoMerged` repeated the conflict set when `merge-tree` cannot predict. Claude Opus 4.8 via Claude Code. --- Moatless task: https://moatless.soaplabstest.com/tasks/ef3f3a62-fa94-4dc0-9bc3-3612a6a4a8de
Note
Medium Risk
Adds filesystem watching and new WS payload fields for keybindings config validation issues; changes affect server startup/config loading and client push handling, with moderate risk of missed updates or noisy notifications across platforms.
Overview
Keybindings config loading is refactored to return a runtime
KeybindingsConfigStatethat includes non-fatal validation issues (malformed JSON vs invalid entries), while still falling back to defaults and not overwriting a bad file.The server now watches
keybindings.jsonfor changes, revalidates, and pushesserver.configUpdatedevents over WebSocket;server.getConfig/server.upsertKeybindingresponses and contracts are extended to include anissuesarray.The web app subscribes to
server.configUpdated, invalidates cached config queries, and shows success/warning toasts (with an action to openkeybindings.jsonin the user’s editor). Tests are expanded to cover malformed/partial configs, issue reporting, and the new push channel + caching behavior.Written by Cursor Bugbot for commit 51dbea2. This will update automatically on new commits. Configure here.
Note
Rework keybindings UX by adding Effect-based server config and WS schema, exposing
Keybindings.upsertKeybindingRuleover WebSocket, and wiring Settings UI to openkeybindings.jsonRefactor server to Effect services and schema-validated WebSocket protocol; add a
server.configUpdatedpush and request/response schemas; implement aKeybindingsservice with atomic writes, issues reporting, and change stream; routeserverGetConfigandserverUpsertKeybindingin apps/server/src/wsServer.ts; update web to acquire the API lazily, render settings showing the keybindings path, open the config via native editor, and validate project script keybindings; migrate terminal PTY selection and Node/Bun layers; replace Zod with Effect Schema across contracts and tests; introduce orchestration engine, projections, and snapshot syncing to drive UI state.📍Where to Start
Start with the WebSocket server routing and pushes in apps/server/src/wsServer.ts, then review the Keybindings service in apps/server/src/keybindings.ts and the web settings integration in apps/web/src/routes/_chat.settings.tsx.
📊 Macroscope summarized 51dbea2. 5 files reviewed, 4 issues evaluated, 1 issue filtered, 0 comments posted
🗂️ Filtered Issues
apps/server/src/keybindings.ts — 0 comments posted, 2 evaluated, 1 filtered
upsertKeybindingRuleoperation causes data loss by silently permanently deleting any existing invalid or malformed entries in the configuration file. TheloadWritableCustomKeybindingsConfigfunction (used byupsertKeybindingRule) iterates over the raw configuration and filters out any entries that failKeybindingRuleorResolvedKeybindingFromConfigvalidation, returning only the valid subset.upsertKeybindingRulethen takes this filtered subset, adds the new rule, and overwrites the configuration file on disk viawriteConfigAtomically. Consequently, if a user has a syntax error or an invalid property in one rule, adding a new rule via the application will wipe the invalid rule from the file instead of preserving it. [ Out of scope ]Summary by CodeRabbit
Release Notes
New Features