Release 107 feedback: clean desktop chat previews - #84
Open
alichherawalla wants to merge 416 commits into
Open
Conversation
The desktop MCP server exposes the on-device ACTION tools (calendar, reminders, contacts, messages_send, mail_send, open_url, web_task, computer_task) so a paired phone can list + run them on this Mac. But the gateway binds 0.0.0.0 (LAN-wide), so unauthenticated any device on the network could trigger a mail_send or a computer-use task. The model/ inference tools are safe to leave open; the action tools are not. Gate them: buildMcpServer(actionsAllowed) registers the action tools ONLY when the request carries the desktop's action token, so an unpaired device neither sees them in tools/list nor can call them. Model tools stay open. - mcp-auth-logic.ts: constant-time Bearer check (timingSafeEqual), fail closed on blank/short/wrong/malformed. Electron-free, unit tested. - mcp-auth.ts: token store (32 random bytes, persisted 0600 in userData, generated on first use) + isActionAuthorized(req); dev-only startup log so a device can be paired for testing (never logged in a packaged build). - mcp-server.ts: handleMcpRequest builds the per-request server from isActionAuthorized(req); action tools register only in that branch. - Source-level regression guard for the gate (buildMcpServer can't run in a unit test - it imports electron/native modules), plus the auth unit tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…airing
The desktop needs to hand a phone the details to reach its MCP action tools -
via a scannable QR or a shown code (both interchangeable). These are the pure,
tested pieces:
- lan-address.ts: pick a LAN IPv4 the phone can actually reach (skip loopback/
internal/link-local, prefer 192.168 > 10 > 172.16-31). The gateway binds
0.0.0.0; this just chooses what to advertise.
- pairing-payload.ts: the versioned pairing payload
{ t:'offgrid-mcp-pair', v:1, url, token, name? } + JSON encode. Same data the
manual URL+token flow uses; the OGAM parser mirrors this contract.
IPC + the desktop "Pair a device" panel (QR + show-code) build on these next.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A core Settings section that shows a phone how to run this Mac's MCP action
tools - scan the QR (fills in URL + token, nothing to type) or expand "Enter it
by hand" for the URL + Authorization header to copy. Same data either way, so
QR and code stay interchangeable.
- ipc `pairing:info`: composes lan-address + pairing-payload + the live gateway
port + action token + host name into { mcpUrl, token, deviceName, lanIps, qr }.
The token is the desktop's own secret shown on its own screen.
- preload: `pairing.info()` namespace.
- PairDevicePanel: QR (qrcode.react) + show-code fallback + a "no local network"
guard; self-explanatory copy.
- Shared ui/CopyButton (phosphor), and migrated GatewayScreen's local copy button
to it (DRY; drops its tabler copy icons).
Renderer needs a main restart to pick up the new IPC; verified via tsc + the
lan-address/pairing-payload unit tests. Visual/QR-scan check lands with the
mobile scanner (phase D).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The mesh flow supersedes it: you pair once on the Devices tab and toggle "Allow tools" per device, instead of a separate Settings panel that showed a QR for the phone to scan. Removes PairDevicePanel, the pairing:info IPC handler + preload namespace, and the now-unused qrcode.react dep. The pairing-payload / lan-address helpers stay - desktop-pro's mesh grant sender still imports them.
Dropped with the standalone Pair-a-device panel; the Devices screen now renders the mesh pairing code as a QR (desktop-pro), so the dep returns for that use.
A small, curated set of starter prompts grouped by the capability each shows off (browse the web, drive your Mac, remembers what you've seen, tools from your phone). Tapping a preset will seed a real chat with its prompt so the agent asks its own follow-ups - the demo and the "prompting is a conversation" lesson in one. Data only, no pro logic; presets that need pro / a paired phone / capture history are tagged via `requires` so the surface gates them. Invariants covered by tests.
Renders the preset catalog as capability-grouped cards (browse / drive your Mac / remembers / phone), each with an icon, a blurb, and a plain requirement label when it's gated so a card never dead-ends silently. Clicking a card calls onRun(preset) so the host can seed a chat with the prompt. Placement-agnostic and brand-dense (mono, emerald accent, Phosphor icons). Optional "request a capability" link points at a form. Covered by a render/click test.
Wire ExploreSection into MemoryChat's empty state (non-image chat mode) so a brand-new chat leads with the capability-grouped starter prompts. Tapping a preset fires sendMessage(preset.prompt) directly - the prompt stays hidden and the agent takes over asking its own follow-ups, teaching that prompting is a conversation. Adds REQUEST_FORM_URL (undefined for now) as the single place to point the "Request a capability" link once the Google Form exists; until then the surface hides the link rather than shipping a broken one. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Make the demo presets discoverable to everyone, not just on a fresh chat's empty state: a top-level "Explore" nav item opens ExploreScreen, which reuses the same ExploreSection verbatim (one component, two placements). Add a seed-prompt handoff so any surface can open a fresh chat that auto-sends a preset: openTarget.seedPrompt resets to a new conversation and a small effect below sendMessage fires it once the reset has settled, so the prompt lands in the new empty chat. App's handleRunPreset drives it from the Explore screen. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Match the brand name shown in the sidebar and used across the app, per the strict brand-copy rule, rather than the bare "Off Grid" shorthand. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A prompt like "best-reviewed places" failed with "Sorry, something went wrong":
the rag:chat retrieval built its FTS5 MATCH by joining tokenizeQuery's tokens
bare (`tokens.join(' OR ')`), and tokenizeQuery keeps hyphens - so `best-reviewed`
reached MATCH as invalid syntax and threw `SqliteError: no such column: reviewed`,
failing every FTS query in the turn (memory/message/summary/entity/fact). Common
words trip it too: e-commerce, back-end, co-founder.
The repo already had the safe pattern (search-ranking.ts `ftsExpr` quotes each
term so punctuation can't be a syntax error); rag:chat just didn't use it. Add
`ftsMatchExpression` next to tokenizeQuery in the pure, testable ipc-query-logic
module: quote each token as an FTS5 phrase literal and OR-join (preserving the
any-term recall), with a quoted-phrase fallback when nothing tokenises so that
path can't throw either. Route rag:chat through it.
Regression test runs the built expression against a REAL FTS5 table (node:sqlite):
the old bare form throws `no such column`, the fixed form matches, and hyphen /
colon / quote-heavy inputs never throw.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… clause
The preset "Play something upbeat and set the volume to about 30%" read as
broken in testing: there is no system-volume tool at all, so the model honestly
refused that half; and "play something upbeat" is too vague for the local model
to route (orchestrator logged plan=[none] for the "music" follow-up), so it
disclaimed instead of acting.
The capability itself works - a concrete "play <x> on Spotify" routes straight
to computer_task and queues the native action (verified: orchestrator logged
plan=[computer_task] and queued it awaiting approval). So make the preset match
that proven shape ("Play some jazz on Spotify") and drop the unsupported volume
promise, so the "Drive your Mac" demo actually drives the app.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…pro too The inline approval card (ActionGateDock, mounted in the chat) was only fed when no pro queue was listening - so in a pro build the gate always went to the pro queue (a notification) and the in-chat Approve/Reject card never appeared. That was flagged in the code as "pro queue wins until the migration". Do the migration: gateHost now parks AND emits the inline card whenever a human is needed (pro queue accepted it, or an inline surface is registered). The notification and the in-chat card are two views of the ONE engine gate - whichever the user acts on calls resolveActionGate for the same actionId (idempotent), and the other view settles on the outcome broadcast. A chat-initiated computer-use task is now approvable right where it was asked. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add registerApprovalModeProvider / computerApprovalMode to gate-host so the user's "Auto-approve vs Ask every time" choice governs the gate. gateHost short-circuits to approve when the mode is 'auto' (the task still journals and its outcome shows in chat); 'ask' (the default when no provider is registered) parks for approval as before. Pro owns the persisted setting + the Sync-sharing toggle and registers the provider - core just exposes the seam, so the free build has no computer-use logic. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…d + mesh) The inline gate surface was a single slot, so only one consumer could render a parked gate. Mobile approvals need a second: pro forwarding the gate to paired phones over the mesh, alongside the desktop chat card. Make it a Set of subscribers - registerInlineGateSurface adds/removes, gateHost fans each parked gate to all of them. Each surface resolves the ONE engine gate via resolveActionGate; the first verdict wins, the rest are no-ops. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
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:
📝 WalkthroughWalkthroughThe change updates transcription language handling, adds transcription settings, standardizes side panels, adds Devices subroutes, sanitizes Markdown links, supports provider-owned MCP tools, and updates tests and repository wiring. ChangesApplication feature updates
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🟡 Moderate · up to This PR changes desktop preview behavior alongside connector and navigation flows, but the current head still permits broken focus wrapping, can leave the Electron SQLite dependency unusable after a failed restoration, and can crash route parsing for malformed device URLs. Merge should wait for these bounded correctness and runtime issues to be fixed or explicitly accepted. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
The Explore surface now renders each capability as a bordered panel (icon
tile + teaches line) holding a dense card grid, adapting to its container
(two panel columns on the Explore screen, one in the chat empty state).
Preset titles become short capability labels - the prompt-phrased titles
("Find me a flight to book", "What did I work on today?") no longer leak
the seeded prompt onto the card. Robust ungated presets get a ready
marker; the Explore screen gains a real page header with catalog-computed
meta and hides the section's compact intro via showIntro.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Boots the free build on a fresh profile, walks the Explore screen and the chat empty state, and asserts the capability panels render with card labels while the seeded prompt never appears on the surface. Screenshots captured for evidence. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Every preset now carries its own Phosphor icon in the catalog SSOT (AirplaneTilt, SpotifyLogo, Crop, ...) so both placements show the same mark; the card leads with the icon and warms it emerald on hover. Each panel header gains a small runs-count meta. Tests cover the icon invariant, the per-card rendering, and the panel counts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…app quits The agent WebContentsView runs with backgroundThrottling off (so the agent can work while the user does other things), and it was only ever hidden - bounds set to zero - never muted or torn down. So a web_task that started a video kept its audio playing after the pane closed, the window was hidden, or the app was "quit" (on macOS a window close leaves the process alive). - Mute the view's audio whenever it's hidden, unmute when shown, so closing the browser pane actually silences it. - React to the window too (setRegion only fires while the pane is mounted): mute on window hide, fully dispose (remove + close the WebContents) on window close. - disposeBrowserHost() on before-quit stops media immediately rather than whenever the process finally exits. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Drop the amber "this model can't ground" warning (VisionGrounderNudge) - it added noise on every non-grounder computer-use run. Removed the render + its import. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The card sat on screen while the action ran and the outcome made its way back. resolve() now drops the pending card immediately on approve/reject/edit; the outcome still lands as its own row, and an edit re-gates as a fresh pending event. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ip notch The left-edge resize handle was covered by the native WebContentsView below the header, so it was only grabbable in a thin strip. Inset the web region (ml-4) to give the handle a real full-height gutter, and swap the faint bar for a centered DotsSixVertical grip so it clearly reads as draggable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The live step narration for a web task sat in a feed under the browser pane. Move it into the chat turn: MemoryChat subscribes to browser:onStep + onTaskState and renders the recent steps in the streaming assistant turn (a new task resets them, and they fall away when the turn ends). WatchedBrowserPane drops its step feed + summary and keeps just the goal, live browser view, and takeover prompt. Computer-use task steps are unchanged (they stay in their own small modal). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The card labels/blurbs are unchanged; the actual prompt each preset SENDS is now defined - it names the site or app and states the task plainly, so it routes cleanly and reads sensibly, e.g. find-flight now sends "Go to skyscanner.com and help me find a flight to book. I will give you the route, dates, and budget when you ask." Named targets: Skyscanner, Google Maps, Google Shopping, Spotify, Preview, Mail; the memory/phone prompts point at what was captured on the Mac. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Resolutions: - packages/models: accept main's de-vendoring (now file:../shared/packages/models, where the grounder work already landed). - pro: adopt main's submodule model (.gitmodules); pointer bumped to the synced desktop-pro feature commit in a follow-up once desktop-pro is synced. - MemoryChat.tsx: took main's rewrite, re-grafted the 12 chat-approvals surgical changes (ExploreSection seed handoff, ActionGateDock/VisionSupervisorOverlay, self-contained WebTaskStepFeed for the streaming turn); dropped the MessageNudge cutoff change (superseded by main's ResponseCutoffNotice). - ipc.ts: main's superset chat-stream-state imports. - tools.ts: widen the planning-path return to main's plural imageRequests contract. - vitest.db.config.ts + release.yml: union of both sides' exclusions / build steps. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… by the sync - pro: bump submodule pointer to the synced desktop-pro feat/mesh-tool-grant (4b882d8). - docs: restore the directory name (an earlier commit accidentally renamed docs/ -> docshi/); main carries docs/, and the coverage ledger + release checklist read docs/. - WatchedBrowserPane: guard the browser IPC method calls (onTaskState?./onTakeover?.) so App integration tests (and a partial preload) don't crash on mount when the optional browser API isn't wired. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds the presence seam the floating task card needs. `useTaskWorkspaceOpen` answers "does the user want the workspace open" and rightly survives navigation; its other readers depend on that. "Is a docked surface painting right now" is a different question, and only the surface can answer it, so it registers while mounted. A count, not a boolean: two workspaces exist (the chat panel and the Tasks route) and they can hand over in either order.
There is one native WebContentsView for the window, and more than one HTML surface can host it. Its position was decided by whichever surface called set-region last, so a handover depended on an ordering React does not guarantee: the arriving surface mounts before the departing one cleans up, so the departing surface's release landed last and cleared the arriving one's claim. The floating card was left painting nothing, and since its size never changed afterwards nothing re-reported - blank until a manual resize. A cleared region also hides the native view, and a hidden view captures nothing, so a running task died with "the browser returned an empty screenshot". Surfaces now report under an owner key and main paints the highest priority one present (docked outranks floating). The two messages commute, so no ordering can strand the page. Every PiP failure this week came from that race.
…capture failed Two problems in the capture loop, one of which was silently killing tasks. The viewport check demanded 1920x1200 within a flat 2px tolerance. But the CSS viewport is bounds/zoom, so the smallest expressible change is 1 device pixel = 1/zoom CSS pixels. The floating card runs at the 0.25 zoom floor, where that is FOUR CSS pixels: a 3px layout rounding reported 1908x1192, was rejected forever, and every capture spun to the deadline the moment the card took over. The docked pane sits near zoom 1, where the same rounding stayed under 2px - which is why only the card broke. The tolerance is now a device-pixel budget converted at the current zoom. The loop also has four distinct exit conditions and reported them as one of two sentences, so a failure could not be told apart - and the commonest one never reaches capturePage at all, yet still said "empty screenshot". It now names the blocker and the numbers behind it. That diagnostic is what found the bug above.
…control by view id Enter never submitted. Chromium needs a virtual key code to treat a key as that key, and the two dispatch paths disagreed: pressKey carried a three-entry map with proper codes, while a vision `press` went through dispatchKeyPhase, which merely capitalized whatever it was given and sent no code at all. So the model saying "press return" produced key:'Return' with no keycode - a keystroke the page could not act on, reported as a success. One table now serves both paths, with the aliases the model actually uses (return, esc, up, del), and an unrecognised name fails loudly instead of being title-cased into a key that does not exist. The chat control compared the view against 'chat', but the chat view's id is 'memory-chat' - the route is /chat, the view is not. The comparison never matched, so the control showed on the very screen it exists to hide from. The id is exported now, so a hand-written literal cannot drift from it again.
jsdom implements no scrolling, so Element.scrollIntoView is absent and calling it throws. Real browsers always have it. Stubbed in the shared setup rather than guarded at each call site, so product code is not shaped around a gap in the test environment.
The desktop recorded nothing about generation performance - no rate, no token counts, no time to first token - so there was no way to answer "why did that take so long" except by guessing. Three pieces. parseSseLine used to drop any chunk without choices, which is exactly the final chunk carrying usage and timings, so the numbers were being thrown away before anything could read them. The local request now asks for usage. The accumulator times the stream itself, counting the first reasoning token as the first token - for a thinking model the alternative reports the whole thinking pass as latency. Rates prefer the server's own measurement, which times decode alone; our wall clock includes queueing and prefill and understates the rate on a cold prompt. Every field is optional, and a server that reports nothing produces no numbers rather than a row of zeros - "0 tok/s" is a lie, an absent rate is the truth. Carried to the renderer through the same context blob that already persists reasoning and cutoff, so no schema or IPC change. Display comes next.
Records how long the screen capture took and how long the model call took, and carries both to the step detail. Measured at the graph level, around the two calls, so it covers the whole cost of each phase rather than the part any one layer can see. The capture loop in particular retries against an unsettled page for up to six seconds without any single call looking slow.
Renders the measured numbers - prefill rate, decode rate, time to first token, token count, total - as one muted line under an assistant message, and persists them in the same context blob that already carries reasoning and cutoff, so they survive a reload. Only the fields the run produced. A server that reports no token counts shows a time and nothing else, because a zero rate would be a lie. The metrics module moved from main/llm to shared: the renderer needs the same shape it renders, and the IPC contract cannot import from main. Its inputs are typed structurally so shared stays independent of the SSE layer. Unlike mobile this is not behind a preference. The desktop chat has no display-settings surface to hang a toggle on, and adding one for a single muted line is not worth the drift - say the word and it becomes a setting.
Mobile keeps these numbers behind a switch and so should we - they are for when you go looking, not a permanent fixture under every answer. Settings -> Model -> Generation details. It is a DISPLAY preference, so it rides saveSetting rather than LlmSettings: nothing here reaches the engine, and putting it in the engine's parameter block would fire a reconfigure on every flip. It travels to the message row through MessageRowState, the bag that already carries the other display state, so no new prop drilling. The integration test found a real gap: metrics were persisted but never set on the LIVE message, so the numbers only appeared after a reload. They are on both now. The test drives the real SettingsPanel and the real chat, and covers the third case too - a run that measured nothing prints nothing, never "0 tok/s".
modelInput was the serialized prompt, history included, stored per step and re-read on every tasks:list poll. Measured on a real profile: 10 MB across 50 tasks, 1.27 MB in the worst single task, of which modelInput was 73%. It is a debugging echo of what we just sent. Nothing needs it after the request, and its only reader was a details block that duplicated the tool-call JSON already shown beside it. Removed at every producer, from the stored shape, and from the UI. sanitizeComputerUseStepDetail now takes a LOOSE input rather than the stored shape: it is the boundary that decides what may be persisted, so it has to be able to receive a field and refuse it. Its test now passes modelInput carrying a secret and asserts the whole field is dropped - the redaction guarantee holds by removal instead of by scrubbing.
|
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
Verification
Summary by CodeRabbit
New Features
Bug Fixes
<think>and__LABEL:markers are hidden.