feat(web): web-desktop bundle Encore Feature + responsive mobile UX#991
feat(web): web-desktop bundle Encore Feature + responsive mobile UX#991ronaldeddings wants to merge 6 commits into
Conversation
Ships the full Maestro desktop UI through the browser by mirroring every
ipcMain handler over a WebSocket bridge. Renderer code runs unchanged in
the browser via a Vite-aliased electron shim. Gated behind a new
`webDesktopBundle` Encore Feature so the IPC surface stays sealed by
default.
## Bridge architecture (1:1 IPC mirror)
- **Server (`src/main/web-server/handlers/bridgeHandlers.ts`)**: dispatches
`bridge.invoke` messages against Electron's `ipcMain._invokeHandlers`
Map and broadcasts `WebContents.send` calls as `bridge.event` to all
connected web clients. Empty/unknown channels reject loudly; handler
exceptions surface their messages to the caller.
- **Web shim (`src/web-desktop/electron-shim.ts`)**: replaces the
`electron` module at build time with a WebSocket-backed `ipcRenderer`,
`contextBridge`, and reconnect-aware `BridgeClient`. Preload code runs
unchanged in the browser.
- **Bootstrap (`src/web-desktop/bootstrap.ts`)**: polyfills `window.process`,
loads preload first, then the renderer. `@sentry/electron/renderer` is
aliased to a no-op shim.
- **Vite config (`vite.config.web-desktop.mts`)**: outputs `dist/web-desktop/`,
served by `WebServer` at `/<token>/desktop/` when the Encore Feature
is on.
## Encore Feature gate (4 enforcement points)
- `WebServer` constructor takes `getWebDesktopBundleEnabled` callback
- `web-server-factory.ts` wires it to `settingsStore.encoreFeatures.webDesktopBundle`
- Gated: route registration, static asset mount, bridge hook install,
and `bridge.invoke` dispatch in `messageHandlers.ts`
- Type: `EncoreFeatureFlags.webDesktopBundle: boolean` in `renderer/types/index.ts`
## Responsive mobile UX
- **Breakpoints (`useViewportBreakpoint.ts`)**: xs/sm/md/lg/xl tracked via
`data-bp` on `<html>` so CSS can target without re-renders.
- **Tri-state left sidebar (`uiStore.cycleLeftSidebar`)**: open → collapsed
strip → hidden, drawer-style overlay below `md`.
- **Header (`MainPanelHeader.tsx`)**: inline hamburger when sidebar hidden,
context usage shown as `{N}%` (was full-width gauge), secondary buttons
hide below 480px.
- **Input toolbar (`InputArea.tsx`)**: collapses History/Plan/Thinking
pills behind a `…` toggle on narrow viewports; nowrap to prevent wrap.
- **File picker (`FileExplorerPanel.tsx`)**: touch-aware single-tap open,
500ms long-press → context menu, capture-phase mousedown swallower so
iOS Safari's synthetic mousedown doesn't dismiss the menu.
- **Git diff (`GitDiffViewer.tsx`)**: hides counter + side-by-side toggle
below `md`, close button becomes × icon with `aria-label`.
- **Tab popover (`NewTabPopover.tsx`)**: edge-flip so options don't get
clipped at right viewport edge.
- **App shell CSS**: `100dvh` + `env(safe-area-inset-bottom)` for iOS;
`select-none` discipline preserved.
## iOS Safari fixes
- **`generateId` (`utils/ids.ts`)**: feature-detects `crypto.randomUUID`
(Safari requires secure context) and falls back to `generateUUID()`
from `shared/uuid.ts` over plain HTTP/Tailscale.
- **File open resilience (`useAppHandlers.ts`)**: `Promise.allSettled` so
a failed `fs.stat` doesn't drop the file content from preview.
## Tests
- New: `bridgeHandlers.test.ts` — 5 cases covering empty channel rejection,
unknown channel, success path, handler errors, and arg pass-through.
Uses `vi.hoisted` to stub `ipcMain._invokeHandlers` without a real
Electron runtime.
- Updated: GitDiffViewer, MainPanel, MainPanelHeader, SessionList,
SidebarActions tests adjusted for renamed labels/icons.
## Validation
- `npm run lint` (tsc all configs): clean
- `npm run lint:eslint`: clean
- `npm run test`: 27,844 passed / 108 skipped / 0 failed
- `npm run build`: main + renderer + web + web-desktop + cli all built
|
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 web-desktop bundle (Vite build, entry HTML/bootstrap), browser Electron/Sentry shims, a WebSocket↔IPC bridge with server-side handlers and lifecycle hooks gated by a runtime feature flag, responsive breakpoint hook/CSS and app-level narrow-viewport behaviors, tri-state sidebar and touch support, tests, and resilience fixes. ChangesWeb-Desktop Bundle Implementation
IPC Bridge and Server
Responsive UI and Mobile Layout
Error Handling and Resilience Improvements
Sequence Diagram(s)sequenceDiagram
participant WebClient
participant WebServer
participant MessageHandler
participant ipcMain
participant WebContents
WebClient->>WebServer: send "bridge.invoke" (requestId, channel, args)
WebServer->>MessageHandler: handleMessage -> detects "bridge.invoke"
MessageHandler->>ipcMain: dispatch to registered handler (channel, event, ...args)
ipcMain->>MessageHandler: handler result / throws
MessageHandler->>WebClient: send "bridge.response" (ok/result|error)
WebContents->>WebServer: webContents.send(channel,args)
WebServer->>WebClient: broadcast "bridge.event" (channel,args,timestamp)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🧹 Nitpick comments (9)
src/renderer/components/FileExplorerPanel.tsx (1)
1178-1200: 💤 Low valueConsider also gating folder expand/collapse on
isTouchPointer.On a touch device, the user expects a single tap on a folder row to expand it (which already works), but a single tap on a file opens it (new behavior, good). The current
onClickis fine, butonDoubleClickat Line 1201-1205 is still wired up on touch devices where it will essentially never fire reliably — and on iOS, the synthesized second click after a double-tap may also be hijacked by the browser's zoom gesture. Not a defect, just dead code on touch. Worth a quickisTouchPointer ? undefined : onDoubleClickto make intent obvious, but feel free to skip.🤖 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 `@src/renderer/components/FileExplorerPanel.tsx` around lines 1178 - 1200, The double-click handler should be disabled on touch devices: update the JSX where the row's onDoubleClick is set (the same component that uses isTouchPointer and handleFileClick in the onClick block) to conditionally attach the handler only when !isTouchPointer (e.g. onDoubleClick={isTouchPointer ? undefined : (e)=>{ /* existing dblclick logic that calls handleFileClick/toggleFolder... */ }}), so touch devices won't receive a never-fired or synthesized second click while desktop mice still get double-click behavior.src/renderer/hooks/ui/useAppHandlers.ts (1)
322-372: 💤 Low valuePromise.allSettled split looks correct and matches the bridge-failure scenario described in the PR.
The decoupling of
readFilefromstatis the right fix — over the WebSocket bridge astatrejection should not throw away successfully-read content. A couple of small notes you may want to address:
- Line 354-359:
content === nullis reachable whencontentResult.status === 'fulfilled'and the value is null. That's still handled correctly, but the comment "either the file is missing or the SSH read was cancelled" is now slightly misleading because read-failure is no longer routed here (it's therejectedbranch above). Consider tightening the comment to "cancelled or returned null".- Line 361:
statResult.valueis typed asAwaited<ReturnType<typeof window.maestro.fs.stat>>, which depending on its declaration may itself be nullable; thestat?.modifiedAtchain on Line 373 already guards this, so functionally fine.No functional changes required.
🤖 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 `@src/renderer/hooks/ui/useAppHandlers.ts` around lines 322 - 372, The comment above the content-null check is slightly misleading: update the comment near the `content === null` check (in the block handling `contentResult` from `Promise.allSettled` for `window.maestro.fs.readFile`) to say the null case means the read was cancelled or the API returned null (e.g., "cancelled or returned null") instead of implying a read failure; no code logic changes needed—leave the `rejected` branch, `closeLoadingTabIfStillLoading`, and the `statResult` handling (`window.maestro.fs.stat` / `stat?.modifiedAt`) as-is.src/renderer/components/MainPanel/MainPanelHeader.tsx (1)
115-132: 💤 Low valueConsider extracting stable store action references.
The onClick handler directly calls
useUIStore.getState()twice. For consistency with the rest of the component (which extracts stable store actions likesetActiveFocus,setBookmarksCollapsed, etc.), consider defining stable references at the component level:// Near other stable store actions const setLeftSidebarHidden = useUIStore.getState().setLeftSidebarHidden; const setLeftSidebarOpen = useUIStore.getState().setLeftSidebarOpen;Then use them in the handler:
onClick={() => { setLeftSidebarHidden(false); setLeftSidebarOpen(true); }}This avoids creating new closures on each render and matches the established pattern in this file.
🤖 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 `@src/renderer/components/MainPanel/MainPanelHeader.tsx` around lines 115 - 132, The onClick handler for the inline hamburger calls useUIStore.getState() twice instead of using stable action references; extract the actions at component scope by assigning const setLeftSidebarHidden = useUIStore.getState().setLeftSidebarHidden and const setLeftSidebarOpen = useUIStore.getState().setLeftSidebarOpen (near the other stable store actions), then update the button onClick to call setLeftSidebarHidden(false) and setLeftSidebarOpen(true) so you avoid repeated getState() calls and match the file's established pattern (the conditional rendering stays controlled by leftSidebarHidden).scripts/import-vram-sessions.ts (2)
173-173: Arbitrary auto-starring threshold.Files >= 10MB are automatically starred. While this is a utility script and the threshold is reasonable, consider making it configurable or documenting the rationale.
You could add a constant at the top:
const AUTO_STAR_THRESHOLD = 10 * 1024 * 1024; // 10MBThen use
file.size >= AUTO_STAR_THRESHOLDon line 173.🤖 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 `@scripts/import-vram-sessions.ts` at line 173, The auto-starring threshold is hard-coded as a magic number; introduce a named constant (e.g., AUTO_STAR_THRESHOLD) at the top of scripts/import-vram-sessions.ts and replace the literal 10 * 1024 * 1024 used in the starred assignment (file.size >= 10 * 1024 * 1024) with file.size >= AUTO_STAR_THRESHOLD so the threshold is configurable and self-documenting; optionally allow overriding via env/config and add a brief comment explaining the value.
72-74: ⚡ Quick winEmpty catch block silently swallows parsing errors.
When JSONL parsing fails, the error is silently ignored. This makes it difficult to diagnose malformed data or detect if important conversation entries are being skipped.
🔍 Proposed improvement to log parsing failures
} catch (e) { - // Skip invalid lines + // Skip invalid lines but log for debugging + if (process.env.DEBUG) { + console.warn(` Warning: Failed to parse line in ${jsonlPath}: ${e}`); + } }🤖 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 `@scripts/import-vram-sessions.ts` around lines 72 - 74, The empty catch block that swallows JSONL parsing errors (the catch (e) in the JSONL parsing loop) should be changed to log the parsing failure and context instead of silently skipping: capture the exception and log a warning or error including the thrown error message, the offending line content, and its index/line number (then continue to next line). Update the catch (e) block to call the project logger or console (e.g., processLogger.warn or console.warn) with a clear message referencing the failed line and include e.stack or e.message so malformed entries are visible during import.scripts/populate-real-conversations.ts (3)
118-122: Document the reprocessing logic.The condition for detecting placeholder content is specific but lacks explanation. A brief comment would help future maintainers understand when tabs are reprocessed.
+ // Reprocess tabs with corrupted data ([object Object]) or only system placeholders const needsReprocess = tab.logs.some( (log) => log.text.includes('[object Object]') ||🤖 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 `@scripts/populate-real-conversations.ts` around lines 118 - 122, Add a brief inline comment above the needsReprocess calculation explaining why those specific checks trigger reprocessing: that any log whose text contains the literal "[object Object]" indicates a malformed/serialized object placeholder and that a system log with text "Session loaded from Claude Code" denotes an import/resume marker requiring reprocessing; reference the needsReprocess variable and the tab.logs predicate (the includes('[object Object]') check and the (log.source === 'system' && log.text === 'Session loaded from Claude Code') check) so future maintainers understand the intent and can update the conditions safely.
88-90: ⚡ Quick winEmpty catch block silently swallows parsing errors.
Same pattern as the other script. Consider logging parse failures when debugging.
🔍 Proposed improvement
} catch (e) { - // Skip invalid JSON lines + // Skip invalid JSON lines but log for debugging + if (process.env.DEBUG) { + console.warn(` Warning: Failed to parse JSONL line: ${e}`); + } }🤖 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 `@scripts/populate-real-conversations.ts` around lines 88 - 90, The empty catch block after the JSON.parse in scripts/populate-real-conversations.ts is silently swallowing parse errors; update the catch(e) block that currently only comments "Skip invalid JSON lines" to log the error and the raw line (or line index) being parsed (e.g., use console.warn or the module's logger) so you still skip invalid lines but retain useful debug info; keep behavior unchanged otherwise (do not rethrow).
133-140: Hard-coded project paths reduce portability.The script includes six hard-coded project path patterns specific to one developer's directory structure. This makes the script non-portable and brittle if directory names change.
Consider:
- Reading project directories dynamically from
CLAUDE_PROJECTS_PATH- Moving these paths to a configuration file
- Adding a command-line argument to specify additional search paths
Example of dynamic approach:
// Read all subdirectories in Claude projects const projectPaths = readdirSync(CLAUDE_PROJECTS_PATH, { withFileTypes: true }) .filter(dirent => dirent.isDirectory()) .map(dirent => dirent.name);🤖 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 `@scripts/populate-real-conversations.ts` around lines 133 - 140, The hard-coded array projectPaths in scripts/populate-real-conversations.ts makes the script non-portable; replace the static list by dynamically discovering project directories (and/or accepting overrides) using the existing CLAUDE_PROJECTS_PATH environment variable and a CLI option: read subdirectories from CLAUDE_PROJECTS_PATH (e.g., via readdirSync with withFileTypes) to build projectPaths at runtime, fall back to a small config/default list if CLAUDE_PROJECTS_PATH is unset, and add a command-line flag to accept additional paths that are appended to the discovered list so callers can override or extend behavior.src/web-desktop/bootstrap.ts (1)
46-54: ⚡ Quick winPrefer
textContentoverinnerHTMLfor error display.Using
innerHTMLto render error messages can introduce XSS risk if the error text contains special characters. While the risk is low here (errors come from module loading, not user input), usingtextContentis safer and simpler.♻️ Safer error rendering
void boot().catch((err) => { const root = document.getElementById('root'); if (root) { - root.innerHTML = `<div style="padding:24px;font-family:monospace;color:`#f88`;background:`#111`;min-height:100vh"><h2>Web-Desktop bootstrap failed</h2><pre>${ - (err && (err.stack || err.message)) || String(err) - }</pre></div>`; + const container = document.createElement('div'); + container.style.cssText = 'padding:24px;font-family:monospace;color:`#f88`;background:`#111`;min-height:100vh'; + const heading = document.createElement('h2'); + heading.textContent = 'Web-Desktop bootstrap failed'; + const pre = document.createElement('pre'); + pre.textContent = (err && (err.stack || err.message)) || String(err); + container.appendChild(heading); + container.appendChild(pre); + root.innerHTML = ''; + root.appendChild(container); } console.error('[bootstrap] boot failed', err); });🤖 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 `@src/web-desktop/bootstrap.ts` around lines 46 - 54, The error rendering currently sets root.innerHTML with interpolated error text (in the boot().catch handler), which risks XSS; instead create the container elements and set their text via element.textContent (or createTextNode) so the error string is not interpreted as HTML. Update the catch block that finds document.getElementById('root') to build a wrapper div and a pre (or text node) programmatically, apply the same inline styles via element.style or className, and assign the error payload using (err && (err.stack || err.message)) || String(err) to pre.textContent; keep the console.error call unchanged.
🤖 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 `@package.json`:
- Around line 27-30: The root "build" npm script omits the new
"build:web-desktop" target so the web-desktop bundle isn't produced; update the
"build" script to include "npm run build:web-desktop" in the chain (e.g., add &&
npm run build:web-desktop alongside build:main, build:preload, build:renderer,
build:web and build:cli) so dist/web-desktop is generated during the primary
build pipeline.
In `@scripts/import-vram-sessions.ts`:
- Line 131: The two writeFileSync calls (e.g., writeFileSync(GROUPS_PATH,
JSON.stringify(groupsData, ...)) and the final writeFileSync that persists
session data) lack error handling and can cause data loss; wrap these filesystem
writes in try/catch, write to a temp file first (e.g., GROUPS_PATH + '.tmp' /
sessions tmp), fsync/close, then rename to the real path to make the write
atomic, and on catch log the error (include the caught error) and abort/rollback
so the in-memory removal of the VRAM session is not persisted when the disk
write failed; also consider creating a backup copy of the original file before
overwriting so you can restore on failure.
- Line 65: The id fallback uses Date.now() + Math.random(), creating
inconsistent IDs; change the id assignment for the object (where it currently
reads id: entry.uuid || `${Date.now()}-${Math.random().toString(36).slice(2,
11)}`) to use the same randomUUID() approach used earlier (the randomUUID usage
on line 49) so both branches produce the same UUID format and strength; ensure
you call the same randomUUID() symbol (or import it the same way as the earlier
usage) when entry.uuid is falsy.
- Around line 116-119: Wrap the JSON parsing of SESSIONS_PATH and GROUPS_PATH in
try/catch to validate and handle corrupted JSON: when reading sessionsData and
groupsData (JSON.parse of readFileSync for SESSIONS_PATH and conditional parse
for GROUPS_PATH), catch any SyntaxError, log a clear error via console.error or
processLogger including the path (SESSIONS_PATH / GROUPS_PATH) and the parse
error message, and exit the process (process.exit(1)) so the script doesn't
continue with invalid data; optionally recommend creating a backup or skipping
overwrite only after successful validation.
- Around line 89-103: The directory scanning loop using readdirSync(JSONL_DIR)
and statSync(fullPath) can throw on missing directories or permission errors;
wrap the outer readdirSync call in a try-catch to surface a clear error
(including JSONL_DIR) and abort/exit gracefully, and wrap the per-file
statSync(fullPath) in its own try-catch so a single bad file won't crash the
whole import—use the existing files array push logic unchanged when stat
succeeds, and log or rethrow errors with context referencing JSONL_DIR,
fullPath, MIN_SIZE, and cutoff so failures are easy to diagnose.
- Around line 13-16: The code assumes HOME and the derived constants HOME,
SESSIONS_PATH, GROUPS_PATH and JSONL_DIR are valid but doesn't validate them;
add early validation after those constants are defined: check that HOME is
non-empty and that SESSIONS_PATH, GROUPS_PATH and JSONL_DIR exist and are
readable (using fs.existsSync or fs.statSync), and if any check fails throw or
log a clear error that includes which path failed and why so file operations in
functions like the import routine fail fast with actionable messages.
In `@scripts/populate-real-conversations.ts`:
- Line 171: Wrap the writeFileSync(SESSIONS_PATH, JSON.stringify(sessionsData,
null, '\t')) call in a try/catch and handle failures: before overwriting, back
up the current SESSIONS_PATH (e.g., write existing file to SESSIONS_PATH +
'.bak'), then write the new sessionsData atomically by writing to a temp file
and fs.rename to SESSIONS_PATH; if any step throws, log the error (including the
thrown error object) and exit non‑zero so failures are visible. Ensure you
reference the same symbols (writeFileSync / SESSIONS_PATH / sessionsData and
note that tab.logs was mutated earlier) so the change is applied at the correct
spot.
- Line 98: The JSON.parse call that creates sessionsData from
readFileSync(SESSIONS_PATH, 'utf-8') lacks error handling; wrap the read+parse
in a try/catch (around the JSON.parse and/or readFileSync used to populate
sessionsData) and on failure log a clear error including SESSIONS_PATH and the
parse exception (or exit gracefully), so the script reports a helpful message
instead of crashing.
- Line 81: Fallback ID generation for entries uses a Date.now()+Math.random()
string; replace the fallback in the object where you set "id: entry.uuid ||
`${Date.now()}-${Math.random()...}`" with a proper UUID (e.g.,
crypto.randomUUID()) to match other scripts, and add the necessary import or use
the global crypto API so the code compiles (update the line that constructs the
id and ensure crypto.randomUUID is available).
- Around line 10-15: The script assumes process.env.HOME and the constructed
SESSIONS_PATH and CLAUDE_PROJECTS_PATH exist before any file operations; add
early validation that process.env.HOME is defined and that both paths exist and
are readable (use fs.existsSync or fs.statSync/try-catch) before proceeding, and
if any check fails log a clear error via console.error or processLogger and exit
(process.exit(1)) so downstream code that uses SESSIONS_PATH or
CLAUDE_PROJECTS_PATH (the constants defined at top) does not attempt file
operations on missing paths.
In `@src/__tests__/renderer/components/MainPanel.test.tsx`:
- Around line 1108-1109: Update the negative assertions in MainPanel.test.tsx to
check for absence of the context widget by using
queryByTestId('header-context-widget') instead of asserting missing 'Context
Window' text; locate the assertions around the existing expects that reference
getByTestId/getByText and replace them with expectations that
queryByTestId('header-context-widget') returns null (or
toBeNull()/not.toBeInTheDocument()) so the test fails when the widget DOM node
is present even if its label text changes.
In `@src/main/web-server/handlers/bridgeHandlers.ts`:
- Around line 82-86: The try/catch around the bridge fanout (the call to
broadcastSink?.(channel, args) currently logging via logger.warn) is swallowing
errors without reporting them to Sentry; import and call the Sentry utilities
(captureException or captureMessage) from src/utils/sentry.ts in those catch
blocks (the one around broadcastSink and the other block around lines 136-143)
and pass the caught error plus contextual info (channel, args and LOG_CONTEXT or
other relevant identifiers) so exceptions are recorded in production diagnostics
while preserving the existing logger.warn behavior.
In `@src/main/web-server/web-server-factory.ts`:
- Around line 111-117: The code assumes settingsStore.get('encoreFeatures')
returns an object and directly casts to Record<string, unknown>, which can throw
if the stored value is null or a non-object; update the WebServer feature-check
callback to first retrieve the raw value (e.g., const efRaw =
settingsStore.get('encoreFeatures')), then guard its shape with a runtime check
(typeof efRaw === 'object' && efRaw !== null) before casting to Record<string,
unknown> or accessing webDesktopBundle, and finally return (ef.webDesktopBundle
=== true) only when the guard passes (otherwise return false); reference the
WebServer constructor callback, settingsStore.get, ef/efRaw, and
webDesktopBundle.
In `@src/renderer/App.tsx`:
- Around line 577-582: The mutual-exclusion effect currently always closes the
right panel whenever both drawers are open, which causes the right panel to
immediately close when the user attempts to open it; change the logic to act on
the transition that opened a drawer instead of the static both-open state: when
isNarrowViewport and leftSidebarOpen just transitioned to true while
rightPanelOpen is true, call useUIStore.getState().setRightPanelOpen(false);
likewise, when isNarrowViewport and rightPanelOpen just transitioned to true
while leftSidebarOpen is true, call
useUIStore.getState().setLeftSidebarOpen(false); implement this by tracking
previous values with refs (e.g., prevLeftSidebarOpen / prevRightPanelOpen) or by
splitting into two effects that depend on each drawer flag to ensure the newly
opened drawer remains open and the other drawer is closed.
In `@src/renderer/components/FileExplorerPanel.tsx`:
- Around line 1134-1164: The long-press timer and swallow listeners started in
onTouchStart can fire after FileExplorerPanelInner unmounts or after the context
menu is opened elsewhere; cancel the pending timer and remove any attached
document listeners during unmount and whenever setContextMenu opens/changes to
avoid calling setSelectedFileIndex/setContextMenu on an unmounted component and
prevent stray swallow listeners; implement this by tracking the timer id in
longPressTimerRef and the swallow handler, adding a useEffect cleanup in
FileExplorerPanelInner that clears longPressTimerRef.current, resets
longPressFiredRef.current = false, and removes the swallow listeners, and also
ensure any code-path that opens the context menu clears an existing
longPressTimerRef before setting a new menu.
In `@src/renderer/components/SessionList/SidebarActions.tsx`:
- Around line 53-59: The tooltip logic for the sidebar toggle uses
leftSidebarOpen and shows "Hide sidebar" even when leftSidebarHidden is true
(fully hidden) but leftSidebarOpen is false; update the title computation in
SidebarActions to reflect the next action (show/expand) when leftSidebarHidden
=== true by branching on leftSidebarHidden first (in addition to leftSidebarOpen
and hasNoSessions) and use a label like "Show sidebar" or "Expand sidebar"
combined with toggleSidebarShortcutLabel so the title matches the actual toggle
behavior governed by the uiStore cycle logic.
In `@src/renderer/components/TabBar/NewTabPopover.tsx`:
- Line 79: The overflow check uses a hardcoded POPOVER_WIDTH = 200 but the
popover's style sets minWidth: 180, causing incorrect alignment calculations;
update the NewTabPopover component to use a single shared constant for the
popover width (e.g., POPOVER_MIN_WIDTH) and reference that constant both in the
overflow calculation (where POPOVER_WIDTH is used) and in the style minWidth so
the logic and rendered size match (or compute the measured width dynamically
from the popover element for the overflow check if variable sizing is required);
make the change around the POPOVER_WIDTH definition and the style assignment
that sets minWidth.
In `@src/web-desktop/sentry-shim.ts`:
- Around line 7-8: The no-op implementations of captureException and
captureMessage drop all reports; update these functions in sentry-shim.ts to
delegate to the actual Sentry utilities from src/utils/sentry.ts (import the
real captureException/captureMessage there) and forward the incoming parameters
(error/message, level, ctx) so events and context are preserved; keep the
existing signatures (captureException(_err: unknown, _ctx?: unknown): void and
captureMessage(_msg: unknown, _level?: unknown, _ctx?: unknown): void) but call
the corresponding exports from src/utils/sentry.ts and ensure any exceptions
during forwarding are not swallowed silently (let them propagate or log).
---
Nitpick comments:
In `@scripts/import-vram-sessions.ts`:
- Line 173: The auto-starring threshold is hard-coded as a magic number;
introduce a named constant (e.g., AUTO_STAR_THRESHOLD) at the top of
scripts/import-vram-sessions.ts and replace the literal 10 * 1024 * 1024 used in
the starred assignment (file.size >= 10 * 1024 * 1024) with file.size >=
AUTO_STAR_THRESHOLD so the threshold is configurable and self-documenting;
optionally allow overriding via env/config and add a brief comment explaining
the value.
- Around line 72-74: The empty catch block that swallows JSONL parsing errors
(the catch (e) in the JSONL parsing loop) should be changed to log the parsing
failure and context instead of silently skipping: capture the exception and log
a warning or error including the thrown error message, the offending line
content, and its index/line number (then continue to next line). Update the
catch (e) block to call the project logger or console (e.g., processLogger.warn
or console.warn) with a clear message referencing the failed line and include
e.stack or e.message so malformed entries are visible during import.
In `@scripts/populate-real-conversations.ts`:
- Around line 118-122: Add a brief inline comment above the needsReprocess
calculation explaining why those specific checks trigger reprocessing: that any
log whose text contains the literal "[object Object]" indicates a
malformed/serialized object placeholder and that a system log with text "Session
loaded from Claude Code" denotes an import/resume marker requiring reprocessing;
reference the needsReprocess variable and the tab.logs predicate (the
includes('[object Object]') check and the (log.source === 'system' && log.text
=== 'Session loaded from Claude Code') check) so future maintainers understand
the intent and can update the conditions safely.
- Around line 88-90: The empty catch block after the JSON.parse in
scripts/populate-real-conversations.ts is silently swallowing parse errors;
update the catch(e) block that currently only comments "Skip invalid JSON lines"
to log the error and the raw line (or line index) being parsed (e.g., use
console.warn or the module's logger) so you still skip invalid lines but retain
useful debug info; keep behavior unchanged otherwise (do not rethrow).
- Around line 133-140: The hard-coded array projectPaths in
scripts/populate-real-conversations.ts makes the script non-portable; replace
the static list by dynamically discovering project directories (and/or accepting
overrides) using the existing CLAUDE_PROJECTS_PATH environment variable and a
CLI option: read subdirectories from CLAUDE_PROJECTS_PATH (e.g., via readdirSync
with withFileTypes) to build projectPaths at runtime, fall back to a small
config/default list if CLAUDE_PROJECTS_PATH is unset, and add a command-line
flag to accept additional paths that are appended to the discovered list so
callers can override or extend behavior.
In `@src/renderer/components/FileExplorerPanel.tsx`:
- Around line 1178-1200: The double-click handler should be disabled on touch
devices: update the JSX where the row's onDoubleClick is set (the same component
that uses isTouchPointer and handleFileClick in the onClick block) to
conditionally attach the handler only when !isTouchPointer (e.g.
onDoubleClick={isTouchPointer ? undefined : (e)=>{ /* existing dblclick logic
that calls handleFileClick/toggleFolder... */ }}), so touch devices won't
receive a never-fired or synthesized second click while desktop mice still get
double-click behavior.
In `@src/renderer/components/MainPanel/MainPanelHeader.tsx`:
- Around line 115-132: The onClick handler for the inline hamburger calls
useUIStore.getState() twice instead of using stable action references; extract
the actions at component scope by assigning const setLeftSidebarHidden =
useUIStore.getState().setLeftSidebarHidden and const setLeftSidebarOpen =
useUIStore.getState().setLeftSidebarOpen (near the other stable store actions),
then update the button onClick to call setLeftSidebarHidden(false) and
setLeftSidebarOpen(true) so you avoid repeated getState() calls and match the
file's established pattern (the conditional rendering stays controlled by
leftSidebarHidden).
In `@src/renderer/hooks/ui/useAppHandlers.ts`:
- Around line 322-372: The comment above the content-null check is slightly
misleading: update the comment near the `content === null` check (in the block
handling `contentResult` from `Promise.allSettled` for
`window.maestro.fs.readFile`) to say the null case means the read was cancelled
or the API returned null (e.g., "cancelled or returned null") instead of
implying a read failure; no code logic changes needed—leave the `rejected`
branch, `closeLoadingTabIfStillLoading`, and the `statResult` handling
(`window.maestro.fs.stat` / `stat?.modifiedAt`) as-is.
In `@src/web-desktop/bootstrap.ts`:
- Around line 46-54: The error rendering currently sets root.innerHTML with
interpolated error text (in the boot().catch handler), which risks XSS; instead
create the container elements and set their text via element.textContent (or
createTextNode) so the error string is not interpreted as HTML. Update the catch
block that finds document.getElementById('root') to build a wrapper div and a
pre (or text node) programmatically, apply the same inline styles via
element.style or className, and assign the error payload using (err &&
(err.stack || err.message)) || String(err) to pre.textContent; keep the
console.error call 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: CHILL
Plan: Pro
Run ID: c274c450-6611-4a59-adcf-00b0e6980491
📒 Files selected for processing (36)
PRD-CY-CONTEXT-RUNTIME.mdpackage.jsonscripts/import-vram-sessions.tsscripts/populate-real-conversations.tssrc/__tests__/main/web-server/handlers/bridgeHandlers.test.tssrc/__tests__/renderer/components/GitDiffViewer.test.tsxsrc/__tests__/renderer/components/MainPanel.test.tsxsrc/__tests__/renderer/components/MainPanel/MainPanelHeader.test.tsxsrc/__tests__/renderer/components/SessionList.test.tsxsrc/__tests__/renderer/components/SessionList/SidebarActions.test.tsxsrc/main/web-server/WebServer.tssrc/main/web-server/handlers/bridgeHandlers.tssrc/main/web-server/handlers/messageHandlers.tssrc/main/web-server/web-server-factory.tssrc/renderer/App.tsxsrc/renderer/components/FileExplorerPanel.tsxsrc/renderer/components/GitDiffViewer.tsxsrc/renderer/components/InputArea.tsxsrc/renderer/components/MainPanel/MainPanelHeader.tsxsrc/renderer/components/RightPanel.tsxsrc/renderer/components/SessionList/SessionList.tsxsrc/renderer/components/SessionList/SidebarActions.tsxsrc/renderer/components/TabBar/NewTabPopover.tsxsrc/renderer/hooks/ui/index.tssrc/renderer/hooks/ui/useAppHandlers.tssrc/renderer/hooks/ui/useViewportBreakpoint.tssrc/renderer/index.csssrc/renderer/stores/settingsStore.tssrc/renderer/stores/uiStore.tssrc/renderer/types/index.tssrc/renderer/utils/ids.tssrc/web-desktop/bootstrap.tssrc/web-desktop/electron-shim.tssrc/web-desktop/index.htmlsrc/web-desktop/sentry-shim.tsvite.config.web-desktop.mts
| const HOME = process.env.HOME || ''; | ||
| const SESSIONS_PATH = join(HOME, 'Library/Application Support/maestro-dev/maestro-sessions.json'); | ||
| const GROUPS_PATH = join(HOME, 'Library/Application Support/maestro-dev/maestro-groups.json'); | ||
| const JSONL_DIR = join(HOME, '.claude/projects/-Volumes-VRAM'); |
There was a problem hiding this comment.
Add validation for critical paths before use.
The script assumes HOME is set and that all paths exist, but provides no validation. If HOME is unset or paths don't exist, subsequent file operations will fail with unclear errors.
🛡️ Proposed fix to validate paths early
const HOME = process.env.HOME || '';
+if (!HOME) {
+ console.error('ERROR: HOME environment variable is not set');
+ process.exit(1);
+}
+
const SESSIONS_PATH = join(HOME, 'Library/Application Support/maestro-dev/maestro-sessions.json');
const GROUPS_PATH = join(HOME, 'Library/Application Support/maestro-dev/maestro-groups.json');
const JSONL_DIR = join(HOME, '.claude/projects/-Volumes-VRAM');
+
+if (!existsSync(JSONL_DIR)) {
+ console.error(`ERROR: JSONL directory does not exist: ${JSONL_DIR}`);
+ process.exit(1);
+}
+if (!existsSync(SESSIONS_PATH)) {
+ console.error(`ERROR: Sessions file does not exist: ${SESSIONS_PATH}`);
+ process.exit(1);
+}🤖 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 `@scripts/import-vram-sessions.ts` around lines 13 - 16, The code assumes HOME
and the derived constants HOME, SESSIONS_PATH, GROUPS_PATH and JSONL_DIR are
valid but doesn't validate them; add early validation after those constants are
defined: check that HOME is non-empty and that SESSIONS_PATH, GROUPS_PATH and
JSONL_DIR exist and are readable (using fs.existsSync or fs.statSync), and if
any check fails throw or log a clear error that includes which path failed and
why so file operations in functions like the import routine fail fast with
actionable messages.
| onTouchStart={(e) => { | ||
| longPressFiredRef.current = false; | ||
| if (longPressTimerRef.current) { | ||
| window.clearTimeout(longPressTimerRef.current); | ||
| } | ||
| const touch = e.touches[0]; | ||
| const x = touch.clientX; | ||
| const y = touch.clientY; | ||
| longPressTimerRef.current = window.setTimeout(() => { | ||
| longPressFiredRef.current = true; | ||
| setSelectedFileIndex(globalIndex); | ||
| setContextMenu({ x, y, node, path: fullPath }); | ||
| // iOS dispatches a synthetic mousedown after the long-press | ||
| // touchend at the original touch coords (on the file row, | ||
| // NOT the menu). useClickOutside would catch it and close | ||
| // the menu before the user can tap any item. Intercept it | ||
| // in the capture phase so the click-outside handler never | ||
| // sees it. One-shot; auto-cleans after 1s if nothing fires. | ||
| const swallow = (ev: Event) => { | ||
| ev.stopPropagation(); | ||
| document.removeEventListener('mousedown', swallow, true); | ||
| document.removeEventListener('click', swallow, true); | ||
| }; | ||
| document.addEventListener('mousedown', swallow, true); | ||
| document.addEventListener('click', swallow, true); | ||
| window.setTimeout(() => { | ||
| document.removeEventListener('mousedown', swallow, true); | ||
| document.removeEventListener('click', swallow, true); | ||
| }, 1000); | ||
| }, 500); | ||
| }} |
There was a problem hiding this comment.
Cancel pending long-press timer on unmount and on context-menu open.
longPressTimerRef is set inside onTouchStart but never cleared when FileExplorerPanelInner unmounts. If the user starts a touch and the panel unmounts within the 500 ms window (e.g., switching tabs/sessions, closing the panel on a narrow viewport), the timer still fires and calls setSelectedFileIndex / setContextMenu on an unmounted component, plus the two document listeners (swallow) get attached and only self-clean up to 1 s later. Same issue if the context menu opens through some other path — the pending long-press will reopen it.
Also worth noting that longPressFiredRef is left at true after a successful long-press; only the next onTouchStart resets it. If the menu is opened by long-press and the user then clicks (via mouse on a hybrid device) without an intervening touchstart, that click is silently dropped at Line 1180-1183.
Suggested cleanup at the hook level:
🧹 Proposed unmount-safe cleanup
const longPressTimerRef = useRef<number | null>(null);
const longPressFiredRef = useRef<boolean>(false);
+
+ // Clear any pending long-press timer if the panel unmounts mid-hold.
+ useEffect(() => {
+ return () => {
+ if (longPressTimerRef.current) {
+ window.clearTimeout(longPressTimerRef.current);
+ longPressTimerRef.current = null;
+ }
+ };
+ }, []);🤖 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 `@src/renderer/components/FileExplorerPanel.tsx` around lines 1134 - 1164, The
long-press timer and swallow listeners started in onTouchStart can fire after
FileExplorerPanelInner unmounts or after the context menu is opened elsewhere;
cancel the pending timer and remove any attached document listeners during
unmount and whenever setContextMenu opens/changes to avoid calling
setSelectedFileIndex/setContextMenu on an unmounted component and prevent stray
swallow listeners; implement this by tracking the timer id in longPressTimerRef
and the swallow handler, adding a useEffect cleanup in FileExplorerPanelInner
that clears longPressTimerRef.current, resets longPressFiredRef.current = false,
and removes the swallow listeners, and also ensure any code-path that opens the
context menu clears an existing longPressTimerRef before setting a new menu.
| title={ | ||
| hasNoSessions && leftSidebarOpen | ||
| ? 'Add an agent first to collapse sidebar' | ||
| : `${leftSidebarOpen ? 'Collapse' : 'Expand'} Sidebar${toggleSidebarShortcutLabel}` | ||
| : leftSidebarOpen | ||
| ? `Collapse to status strip${toggleSidebarShortcutLabel}` | ||
| : `Hide sidebar${toggleSidebarShortcutLabel}` | ||
| } |
There was a problem hiding this comment.
Title text is incorrect when sidebar is fully hidden.
When leftSidebarHidden=true and leftSidebarOpen=false (fully hidden state), the button is still rendered and the title on line 58 will say "Hide sidebar", but clicking will actually restore the sidebar to full visibility (per the cycle logic in uiStore.ts line 175).
The title should reflect the next state: when hidden, the next action is "Show sidebar" or "Expand sidebar", not "Hide sidebar".
🔧 Proposed fix
+const leftSidebarHidden = useUIStore((s) => s.leftSidebarHidden);
<button
type="button"
disabled={hasNoSessions && leftSidebarOpen}
onClick={() => cycleLeftSidebar()}
className={...}
title={
hasNoSessions && leftSidebarOpen
? 'Add an agent first to collapse sidebar'
- : leftSidebarOpen
- ? `Collapse to status strip${toggleSidebarShortcutLabel}`
- : `Hide sidebar${toggleSidebarShortcutLabel}`
+ : leftSidebarHidden
+ ? `Show sidebar${toggleSidebarShortcutLabel}`
+ : leftSidebarOpen
+ ? `Collapse to status strip${toggleSidebarShortcutLabel}`
+ : `Hide sidebar${toggleSidebarShortcutLabel}`
}
>🤖 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 `@src/renderer/components/SessionList/SidebarActions.tsx` around lines 53 - 59,
The tooltip logic for the sidebar toggle uses leftSidebarOpen and shows "Hide
sidebar" even when leftSidebarHidden is true (fully hidden) but leftSidebarOpen
is false; update the title computation in SidebarActions to reflect the next
action (show/expand) when leftSidebarHidden === true by branching on
leftSidebarHidden first (in addition to leftSidebarOpen and hasNoSessions) and
use a label like "Show sidebar" or "Expand sidebar" combined with
toggleSidebarShortcutLabel so the title matches the actual toggle behavior
governed by the uiStore cycle logic.
| export function captureException(_err: unknown, _ctx?: unknown): void {} | ||
| export function captureMessage(_msg: unknown, _level?: unknown, _ctx?: unknown): void {} |
There was a problem hiding this comment.
No-op error capture drops all explicit reports in web-desktop.
On Line 7 and Line 8, captureException/captureMessage silently discard errors, so web-desktop failures won’t reach telemetry.
Proposed fix
+import { captureException as reportException, captureMessage as reportMessage } from '../utils/sentry';
+
export function init(_options: unknown): void {}
-export function captureException(_err: unknown, _ctx?: unknown): void {}
-export function captureMessage(_msg: unknown, _level?: unknown, _ctx?: unknown): void {}
+export function captureException(err: unknown, ctx?: unknown): void {
+ reportException(err instanceof Error ? err : new Error(String(err)), {
+ extra: { source: 'web-desktop/sentry-shim', ctx },
+ });
+}
+export function captureMessage(msg: unknown, _level?: unknown, ctx?: unknown): void {
+ reportMessage(String(msg), { source: 'web-desktop/sentry-shim', ctx });
+}As per coding guidelines: src/**/*.{ts,tsx}: Do not silently swallow errors... Use Sentry utilities (captureException, captureMessage) from src/utils/sentry.ts for explicit error reporting with context.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function captureException(_err: unknown, _ctx?: unknown): void {} | |
| export function captureMessage(_msg: unknown, _level?: unknown, _ctx?: unknown): void {} | |
| import { captureException as reportException, captureMessage as reportMessage } from '../utils/sentry'; | |
| export function init(_options: unknown): void {} | |
| export function captureException(err: unknown, ctx?: unknown): void { | |
| reportException(err instanceof Error ? err : new Error(String(err)), { | |
| extra: { source: 'web-desktop/sentry-shim', ctx }, | |
| }); | |
| } | |
| export function captureMessage(msg: unknown, _level?: unknown, ctx?: unknown): void { | |
| reportMessage(String(msg), { source: 'web-desktop/sentry-shim', ctx }); | |
| } |
🤖 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 `@src/web-desktop/sentry-shim.ts` around lines 7 - 8, The no-op implementations
of captureException and captureMessage drop all reports; update these functions
in sentry-shim.ts to delegate to the actual Sentry utilities from
src/utils/sentry.ts (import the real captureException/captureMessage there) and
forward the incoming parameters (error/message, level, ctx) so events and
context are preserved; keep the existing signatures (captureException(_err:
unknown, _ctx?: unknown): void and captureMessage(_msg: unknown, _level?:
unknown, _ctx?: unknown): void) but call the corresponding exports from
src/utils/sentry.ts and ensure any exceptions during forwarding are not
swallowed silently (let them propagate or log).
Greptile SummaryShips the web-desktop bundle feature: the full Maestro renderer runs in a browser tab by mirroring every
Confidence Score: 5/5Safe to merge; all four bridge enforcement points are correctly gated and the server install/uninstall lifecycle is well-managed. The feature gate is enforced at four independent layers so the IPC surface cannot be accidentally widened. The WebSocket reconnect hang, HTML blocking, broadcastSink lifecycle, and first-paint layout flash — all previously flagged — have been addressed in this revision. Remaining findings are minor correctness hazards that do not affect correct operation with the current token format or Electron version. No files require special attention for merging; bridgeHandlers.ts and WebServer.ts are worth a second read if the Electron version is bumped or the security-token generation scheme changes. Important Files Changed
Sequence DiagramsequenceDiagram
participant Browser as Browser (Web Desktop)
participant WS as WebSocket (/<token>/ws)
participant MH as MessageHandler
participant BH as bridgeHandlers
participant IPC as ipcMain._invokeHandlers
participant WC as WebContents.send (patched)
participant BS as BroadcastService
Browser->>WS: "{ type: bridge.invoke, requestId, channel, args }"
WS->>MH: handleMessage()
MH->>MH: getWebDesktopBundleEnabled()?
alt Feature disabled
MH-->>Browser: "{ type: bridge.response, ok: false }"
else Feature enabled
MH->>BH: handleBridgeInvoke()
BH->>IPC: handlers.get(channel)(FAKE_EVENT, ...args)
IPC-->>BH: result / throws
BH-->>Browser: "{ type: bridge.response, ok: true/false }"
end
Note over WC,BS: Fanout path
WC->>BS: broadcastSink(channel, args)
BS-->>Browser: "{ type: bridge.event, channel, args }"
Reviews (2): Last reviewed commit: "fix(web-desktop): recover from WS constr..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Thanks for the ambitious push here @ronaldeddings — the IPC mirror behind an Encore Feature flag is exactly the right shape for this, and the responsive UX work + iOS Safari fixes are great quality-of-life wins. PR is clean against One real bug worth fixing before we merge, plus a couple of smaller items: 🔴 The this.ws.addEventListener('close', () => {
const err = new Error('bridge disconnected');
for (const [id, p] of this.pending) p.reject(err);
this.pending.clear();
this.queue.length = 0; // queued frames are also orphaned
setTimeout(() => this.connect(url), 1000);
this.ready = new Promise((r) => (this.resolveReady = r));
});🟡 Smaller items (optional, can be follow-ups):
The reconnect drain is the only blocker from my side — once that lands I'm happy to approve. Thanks again! |
P0 fixes - BridgeClient close: drain `pending` (reject in-flight invokes with "bridge disconnected") and clear the outbound queue before reconnect. Without this, any `ipcRenderer.invoke()` issued during a disconnect window hangs forever — the new server has no memory of those request IDs after restart. (catch: pedramamini, greptile-apps) - App.tsx mutual-exclusion: track previous values via refs and react to the *transition* that opened a drawer instead of the static both-open state. Old logic was biased toward left — opening the right panel while left was open immediately re-closed the right. (catch: coderabbitai) - package.json: include `build:web-desktop` in the root `build` script so CI/release builds produce dist/web-desktop. Without this the Encore Feature is dead in any non-dev build. (catch: coderabbitai) P1 fixes - useViewportBreakpoint: set `data-bp` on <html> synchronously during hook init (in addition to the useEffect). CSS rules using `:root[data-bp='xs']` now apply before first paint on narrow viewports — no more one-frame desktop-layout flash. (catch: pedramamini, greptile-apps) - WebServer: cache the resolved web-desktop asset path and the rendered index.html on first request. Eliminates the existsSync + readFileSync + regex-replace round-trip on every desktop page load. (catch: pedramamini, greptile-apps) - bridgeHandlers: add uninstallWebContentsBridgeHook() that restores the original WebContents.prototype.send and clears broadcastSink. Wired into WebServer.stop() so a defunct BroadcastService isn't called after the Encore Feature is toggled off and the server restarts. (catch: greptile-apps) Minor fixes - NewTabPopover: consolidate POPOVER_WIDTH (overflow math) and the inline `minWidth: 180` into a single shared `POPOVER_MIN_WIDTH = 200` constant. The two had drifted, making the edge-flip trigger slightly earlier than necessary. (catch: coderabbitai) - web-server-factory: guard the `encoreFeatures` cast with `typeof === 'object' && !== null` before reading `webDesktopBundle`. Returns false on corrupted settings instead of throwing. (catch: coderabbitai) Scope cleanup - Remove PRD-CY-CONTEXT-RUNTIME.md, scripts/import-vram-sessions.ts, and scripts/populate-real-conversations.ts. These were personal utilities/PRDs that shouldn't have ridden along with this PR. Validation - npm run lint (tsc all configs): clean - npm run lint:eslint: clean - npm run test: 27,844 passed / 108 skipped / 0 failed - npm run build: main + preload + renderer + web + web-desktop + cli all built
|
Thanks for the catch @pedramamini — got all three! Pushed
While I was in there, also picked up a few of the AI-reviewer items that held up under verification:
Also dropped three files from the diff that snuck in by accident ( Gates re-ran clean: tsc, ESLint, 27,844 tests, full build with web-desktop. Ready for another look whenever you have a minute. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/web-desktop/electron-shim.ts (1)
184-188: ⚡ Quick winConsider failing loudly for unsupported IPC methods if they are ever called.
These stubs (
removeAllListeners,postMessage,sendSync,sendTo,sendToHost) currently succeed silently without doing anything. While no code in the repository currently calls these methods, the coding guidelines require that errors not be silently swallowed. If these methods are ever invoked in the future, they should fail immediately or throw/capture to Sentry so gaps are obvious rather than misbehaving silently. For now, they can remain as defensive stubs, but consider adding comments explaining why they are no-ops or implementing them properly if they become needed.🤖 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 `@src/web-desktop/electron-shim.ts` around lines 184 - 188, The current electron-shim stubs removeAllListeners, postMessage, sendSync, sendTo, and sendToHost silently no-op; change them to fail loudly by throwing a clear, descriptive Error (e.g., "Unsupported in electron shim: <methodName>") or capturing the failure with the existing error reporting mechanism (Sentry/logger) so any future invocation is immediately visible; update the implementations for the functions named removeAllListeners, postMessage, sendSync, sendTo, and sendToHost to either throw or log-and-throw, and add a short comment explaining this defensive behavior for future maintainers.
🤖 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 `@src/web-desktop/electron-shim.ts`:
- Around line 76-80: The JSON.parse inside the message handler (the try/catch
around parsing ev.data) currently swallows parse errors and returns; instead, on
parse failure capture the error, call the socket close/terminate path (e.g.
invoke the WebSocket instance's close() or onClose handler) so any pending
invoke() calls receive a deterministic disconnect error, and log/report the
parse error (or rethrow to let Sentry capture it) rather than silently
returning; update the catch block in the message parsing code to close the
socket and surface the error information.
---
Nitpick comments:
In `@src/web-desktop/electron-shim.ts`:
- Around line 184-188: The current electron-shim stubs removeAllListeners,
postMessage, sendSync, sendTo, and sendToHost silently no-op; change them to
fail loudly by throwing a clear, descriptive Error (e.g., "Unsupported in
electron shim: <methodName>") or capturing the failure with the existing error
reporting mechanism (Sentry/logger) so any future invocation is immediately
visible; update the implementations for the functions named removeAllListeners,
postMessage, sendSync, sendTo, and sendToHost to either throw or log-and-throw,
and add a short comment explaining this defensive behavior for future
maintainers.
🪄 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: CHILL
Plan: Pro
Run ID: 5d8fc505-042b-41c8-b1f2-096e4d3f8b5b
📒 Files selected for processing (8)
package.jsonsrc/main/web-server/WebServer.tssrc/main/web-server/handlers/bridgeHandlers.tssrc/main/web-server/web-server-factory.tssrc/renderer/App.tsxsrc/renderer/components/TabBar/NewTabPopover.tsxsrc/renderer/hooks/ui/useViewportBreakpoint.tssrc/web-desktop/electron-shim.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/renderer/hooks/ui/useViewportBreakpoint.ts
- src/main/web-server/WebServer.ts
- src/main/web-server/handlers/bridgeHandlers.ts
- src/renderer/App.tsx
| try { | ||
| msg = JSON.parse(typeof ev.data === 'string' ? ev.data : String(ev.data)); | ||
| } catch { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Don't silently drop malformed bridge frames.
If the server ever emits bad JSON here, this just returns and any waiting invoke() can sit pending until the socket later closes. Capture the parse failure and close the socket so callers get a deterministic disconnect error instead of a silent hang.
As per coding guidelines, "Do not silently swallow errors. Let unhandled exceptions bubble up to Sentry for error tracking in production."
🤖 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 `@src/web-desktop/electron-shim.ts` around lines 76 - 80, The JSON.parse inside
the message handler (the try/catch around parsing ev.data) currently swallows
parse errors and returns; instead, on parse failure capture the error, call the
socket close/terminate path (e.g. invoke the WebSocket instance's close() or
onClose handler) so any pending invoke() calls receive a deterministic
disconnect error, and log/report the parse error (or rethrow to let Sentry
capture it) rather than silently returning; update the catch block in the
message parsing code to close the socket and surface the error information.
Addresses CodeRabbit follow-up on PR RunMaestro#991. - JSON.parse failure on a WS frame: log the error + raw frame preview via console.warn and drop the frame, instead of silently returning. Deliberately NOT closing the socket — one malformed payload shouldn't reject every in-flight invoke (the drain logic added in the prior commit would fire) or spin a reconnect loop on a flaky network. - removeAllListeners: implement against BridgeClient.listeners (clear one channel's Set when given, clear the whole Map when not) instead of returning ipcRenderer with no effect. - sendSync: throw a clear Error. Synchronous IPC can't be tunneled over a WebSocket; returning undefined would let callers miscompute on a silent null. Real renderer code paths only use invoke/send/on. - postMessage / sendTo / sendToHost: kept as no-ops since they have no callers in this codebase and no meaningful translation to the bridge, but added a comment explaining the intent so the next reviewer doesn't re-flag them. Validation - npm run lint: clean - npm run lint:eslint: clean - npm run test: 27,844 passed / 108 skipped / 0 failed
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 `@src/web-desktop/electron-shim.ts`:
- Around line 177-179: The ipcRenderer.send shim currently swallows all
bridge.invoke errors via .catch(() => {}); update the send implementation (the
send arrow function that calls bridge.invoke) to not silently discard
rejections: at minimum call console.error (or call the existing
captureException/Sentry hook if available in this bundle) with the error in the
.catch handler so failures are visible, and avoid suppressing the rejection
(either rethrow the error or omit the catch so it can bubble to global
handlers/Sentry); reference the send function and bridge.invoke when making this
change.
- Around line 63-69: connect() currently catches WebSocket constructor errors,
logs and returns without scheduling a reconnect, leaving this.ready unresolved
and ipcRenderer.invoke() calls hung; update the connect method (where new
WebSocket(url) is called and this.ws is set) to, on constructor error, schedule
the same 1s reconnect timer used in the close path (ensuring the retry logic
used elsewhere is invoked) so that transient failures trigger a retry and
this.ready can eventually resolve; ensure the reconnect scheduling reuses the
existing retry mechanism or timer variable/name used in this class so behavior
matches the close/error 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: CHILL
Plan: Pro
Run ID: bf4bc623-4408-4c9e-8065-e0d45b47fe7a
📒 Files selected for processing (1)
src/web-desktop/electron-shim.ts
…failures Addresses CodeRabbit 3rd-pass review on PR RunMaestro#991. - connect(): when `new WebSocket(url)` throws synchronously (SyntaxError on a malformed URL, SECURITY_ERR from a CSP-blocked port, etc.), now schedule the same 1s reconnect the close path uses. Without this this.ready never resolves and every subsequent ipcRenderer.invoke() hangs on `await this.ready` — same hang class as the original Pedram finding, different entry point. - ipcRenderer.send: replace `.catch(() => {})` with a `console.error` log so bridge.invoke rejections (unknown channel, server-side handler throws, disconnect) are visible. Still fire-and-forget — no rethrow, since callers don't expect a Promise. Validation - npm run lint: clean - npm run lint:eslint: clean - npm run test: 27,844 passed / 108 skipped / 0 failed
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/web-desktop/electron-shim.ts (1)
81-91:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMalformed bridge frames still leave invokes hanging.
Dropping an unparseable frame here means a corrupted
bridge.responsecan keep itspendingentry alive until some later disconnect. Close the socket and report the parse failure so callers deterministically hit the existingbridge disconnectedpath instead of hanging behind a swallowed protocol error. As per coding guidelines, "Do not silently swallow errors. Let unhandled exceptions bubble up to Sentry for error tracking in production."🤖 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 `@src/web-desktop/electron-shim.ts` around lines 81 - 91, In the onmessage parsing catch block (where ev, msg, raw and preview are used) don't silently drop the frame: after logging the parse failure, immediately close the underlying socket (call socket.close() / ws.close()) and rethrow the error (or call the existing bridge disconnect handler) so the connection takes the normal "bridge disconnected" path and pending entries are cleared and reported to error tracking.
🤖 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 `@src/web-desktop/electron-shim.ts`:
- Around line 108-113: The current loop calling each renderer listener (for ...
of set) swallows exceptions by only console.error; instead import and use the
Sentry utilities from src/utils/sentry.ts (captureException / captureMessage) to
report the error with context (include channel and any relevant args or
fakeEvent) and then re-throw the exception so unexpected renderer/preload errors
propagate to Sentry and preserve Electron event semantics; update the block that
invokes cb(fakeEvent, ...args) to captureException/captureMessage on error with
context including channel and listener identity, then re-throw the caught error
rather than continuing.
---
Duplicate comments:
In `@src/web-desktop/electron-shim.ts`:
- Around line 81-91: In the onmessage parsing catch block (where ev, msg, raw
and preview are used) don't silently drop the frame: after logging the parse
failure, immediately close the underlying socket (call socket.close() /
ws.close()) and rethrow the error (or call the existing bridge disconnect
handler) so the connection takes the normal "bridge disconnected" path and
pending entries are cleared and reported to error tracking.
🪄 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: CHILL
Plan: Pro
Run ID: d7187fdb-0b81-4c0a-a075-ad15d3599256
📒 Files selected for processing (1)
src/web-desktop/electron-shim.ts
| for (const cb of set) { | ||
| try { | ||
| cb(fakeEvent, ...args); | ||
| } catch (err) { | ||
| console.error(`[bridge] listener for ${channel} threw`, err); | ||
| } |
There was a problem hiding this comment.
Don't swallow renderer listener exceptions.
Catching cb(...) and only logging it hides preload/renderer bugs from Sentry and changes Electron-style event semantics. Treat these as unexpected failures: report them with src/utils/sentry.ts context and re-throw instead of continuing after a partially handled event. As per coding guidelines, "For unexpected errors, re-throw them to allow Sentry to capture them. Use Sentry utilities (captureException, captureMessage) from src/utils/sentry.ts for explicit error reporting with context."
🤖 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 `@src/web-desktop/electron-shim.ts` around lines 108 - 113, The current loop
calling each renderer listener (for ... of set) swallows exceptions by only
console.error; instead import and use the Sentry utilities from
src/utils/sentry.ts (captureException / captureMessage) to report the error with
context (include channel and any relevant args or fakeEvent) and then re-throw
the exception so unexpected renderer/preload errors propagate to Sentry and
preserve Electron event semantics; update the block that invokes cb(fakeEvent,
...args) to captureException/captureMessage on error with context including
channel and listener identity, then re-throw the caught error rather than
continuing.
|
@greptile-apps review |
|
✅ Actions performedReview triggered.
|
|
Manual test pass against this PR on a local 0.16.17-RC build (Linux AppImage). Bridge architecture works — Bug 1 — No UI toggle in Encore Features tabThe flag is wired into ```bash …plus a full app restart (the bridge hook installs once at server startup). Likely a one-line follow-up to add the toggle alongside the other Encore feature switches. Bug 2 — Messages sent through the bridge don't reach the agent or echo back
|
Addresses chr1syy's manual-test finding on PR RunMaestro#991 that messages sent from /desktop/ neither reached the agent nor echoed back into the web view. Root cause - Every main→renderer push goes through `safeSend` in `utils/safe-send.ts`, which gates on `mainWindow` existing AND webContents being alive. - The previous bridge implementation monkey-patched `WebContents.prototype.send` to fan out implicitly. That worked only while `safeSend` actually reached `webContents.send(...)`. If the Electron window was closing, destroyed, or mid-launch, `safeSend` short-circuited and the prototype patch never fired — so web-desktop clients silently missed every push (agent output, session updates, state-sync events, the lot). Sending worked (it goes through `ipcMain.handle('process:write')` which the bridge does intercept), but the receive path was dead. Fix - Replace the prototype monkey-patch with an explicit `broadcastBridgeEvent(channel, args)` export from `bridgeHandlers.ts`. - Call it from `safeSend` unconditionally — before the `mainWindow` liveness check — so every IPC push fans out to web-desktop clients even when the Electron renderer isn't reachable. - `broadcastBridgeEvent` is a no-op when the Encore Feature is off or no clients are connected (broadcastSink === null), so cost in the default-off case is one null check per send. - Remove `WebContents.prototype.send` patch entirely. Direct `mainWindow.webContents.send(...)` calls in `web-server-factory.ts` (the `remote:*` events) are mobile→desktop-driver events that target the existing Electron renderer specifically; web-desktop intentionally shouldn't receive them. Validation - npm run lint: clean - npm run lint:eslint: clean - npm run test: 27,847 passed / 108 skipped (3 new tests covering broadcastBridgeEvent install/no-op/uninstall paths) - npm run build:web-desktop: clean - Live end-to-end test via Interceptor in real Chrome: 1. Opened http://localhost:31999/<token>/desktop/ 2. Typed "ping from web-desktop bridge test — please reply with PONG" in the active agent session 3. Confirmed message arrived in main process and was written to the agent's PTY 4. Agent's "thinking PONG" and final "PONG" streamed back into the web-desktop view in real time Tests - bridgeHandlers.test.ts gains three cases for the new public surface: - no-op before install - fanout shape after install (type/channel/args/timestamp) - no-op again after uninstall
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/main/web-server/handlers/bridgeHandlers.ts (2)
98-102:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReport bridge fanout failures to Sentry.
The catch block logs the error but doesn't report it to Sentry. Broadcast failures are unexpected system issues and should be captured for production diagnostics. As per coding guidelines, use
captureExceptionfromsrc/utils/sentry.tsto report unexpected errors with context.🔍 Proposed fix
import { ipcMain } from 'electron'; import { logger } from '../../utils/logger'; +import { captureException } from '../../utils/sentry'; import type { WebClient } from '../types'; import type { BroadcastService } from '../services'; // ... export function broadcastBridgeEvent(channel: string, args: unknown[]): void { if (!broadcastSink) return; try { broadcastSink(channel, args); } catch (err) { + captureException(err instanceof Error ? err : new Error(String(err)), { + extra: { operation: 'bridge:eventFanout', channel }, + }); logger.warn(`bridge fanout failed: ${(err as Error).message}`, LOG_CONTEXT); } }🤖 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 `@src/main/web-server/handlers/bridgeHandlers.ts` around lines 98 - 102, The catch block around broadcastSink currently only logs with logger.warn; update it to also report the exception to Sentry by calling captureException(err, { extra: { channel, args, logContext: LOG_CONTEXT } }) (import captureException from src/utils/sentry.ts) inside the same catch for unexpected bridge fanout failures, keeping the existing logger.warn call for local logs and ensuring you pass the error object and contextual metadata (channel/args/LOG_CONTEXT) to captureException for diagnostics.
148-155:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReport handler exceptions to Sentry.
The catch block converts handler errors to client responses but doesn't report them to Sentry. Handler exceptions are unexpected failures that should be tracked for production diagnostics. As per coding guidelines, use
captureExceptionfromsrc/utils/sentry.tsto report unexpected errors with context.🔍 Proposed fix
import { ipcMain } from 'electron'; import { logger } from '../../utils/logger'; +import { captureException } from '../../utils/sentry'; import type { WebClient } from '../types'; import type { BroadcastService } from '../services'; // ... try { const result = await handler(FAKE_EVENT, ...args); send(client, { type: 'bridge.response', requestId, ok: true, result, }); } catch (err) { + captureException(err instanceof Error ? err : new Error(String(err)), { + extra: { operation: 'bridge:invoke', channel, requestId }, + }); const error = err instanceof Error ? err.message : String(err); send(client, { type: 'bridge.response', requestId, ok: false, error, }); }🤖 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 `@src/main/web-server/handlers/bridgeHandlers.ts` around lines 148 - 155, The catch block in bridgeHandlers.ts that currently converts handler errors into a client response needs to also report the original error to Sentry: import and call captureException (from src/utils/sentry.ts) inside the catch before sending the bridge.response, passing the raw err (or the Error instance) and include contextual metadata such as requestId and the incoming message type so Sentry can correlate it; keep the existing send(client, { type: 'bridge.response', requestId, ok: false, error }) after calling captureException.
🧹 Nitpick comments (1)
src/__tests__/main/web-server/handlers/bridgeHandlers.test.ts (1)
164-164: 💤 Low valueConsider removing unused helper or adding a basic usage test.
The
void lastSend;statement is a workaround to suppress unused variable warnings, but it's unusual. If the helper is intended for future tests, consider either adding a minimal test that uses it now, or removing it and re-adding when needed.🤖 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 `@src/__tests__/main/web-server/handlers/bridgeHandlers.test.ts` at line 164, The declaration "void lastSend;" is an unused workaround to silence TS warnings; either remove the unused helper entirely from the test file or add a minimal assertion that uses the lastSend variable (e.g., initialize lastSend where it’s set in the tests and include a simple expect(lastSend).toBeDefined() or similar) so it is referenced; update the bridgeHandlers.test.ts tests around the existing lastSend variable usage (search for lastSend) to implement one of these two options.
🤖 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.
Duplicate comments:
In `@src/main/web-server/handlers/bridgeHandlers.ts`:
- Around line 98-102: The catch block around broadcastSink currently only logs
with logger.warn; update it to also report the exception to Sentry by calling
captureException(err, { extra: { channel, args, logContext: LOG_CONTEXT } })
(import captureException from src/utils/sentry.ts) inside the same catch for
unexpected bridge fanout failures, keeping the existing logger.warn call for
local logs and ensuring you pass the error object and contextual metadata
(channel/args/LOG_CONTEXT) to captureException for diagnostics.
- Around line 148-155: The catch block in bridgeHandlers.ts that currently
converts handler errors into a client response needs to also report the original
error to Sentry: import and call captureException (from src/utils/sentry.ts)
inside the catch before sending the bridge.response, passing the raw err (or the
Error instance) and include contextual metadata such as requestId and the
incoming message type so Sentry can correlate it; keep the existing send(client,
{ type: 'bridge.response', requestId, ok: false, error }) after calling
captureException.
---
Nitpick comments:
In `@src/__tests__/main/web-server/handlers/bridgeHandlers.test.ts`:
- Line 164: The declaration "void lastSend;" is an unused workaround to silence
TS warnings; either remove the unused helper entirely from the test file or add
a minimal assertion that uses the lastSend variable (e.g., initialize lastSend
where it’s set in the tests and include a simple expect(lastSend).toBeDefined()
or similar) so it is referenced; update the bridgeHandlers.test.ts tests around
the existing lastSend variable usage (search for lastSend) to implement one of
these two options.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f13c78c0-f2ab-41d4-b1a9-9bd6c9cb00b5
📒 Files selected for processing (3)
src/__tests__/main/web-server/handlers/bridgeHandlers.test.tssrc/main/utils/safe-send.tssrc/main/web-server/handlers/bridgeHandlers.ts
…, 4) Addresses the remaining items from chr1syy's manual-test review on PR RunMaestro#991, plus a regression from the round-1 P1 fix. Bug 1 — UI toggle in EncoreTab - Add a Web-Desktop Bundle toggle to EncoreTab.tsx alongside the other Encore Features (Symphony, Cue, Director's Notes). Reuses the same card pattern, Globe icon, Beta badge, and feature-card border. Body copy explains what /<token>/desktop/ does and reminds the user to restart Maestro after toggling (the bridge hook installs once at server startup). - Register the new toggle in searchableSettings.ts so the Settings search bar finds it by "web", "desktop", "bridge", "bundle", etc. Bug 3 — SettingsModal in web-desktop - Reproduced via Interceptor end-to-end (General/Display/SSH/Encore/ Maestro Prompts tabs all open cleanly, prompt editor loads its textbox, no thrown exceptions visible in the renderer console). Crash chr1syy reported on the round-1 code was almost certainly an indirect symptom of Bug 2 — Settings depends on push events through safeSend for its state-sync, and those silently dropped when the bridge fanout was gated on mainWindow liveness. Bug 2's switch to the explicit broadcastBridgeEvent fanout fixed it as a side effect. No additional code change needed; this commit records the verdict. Bug 4 — Electron-host-only UI guards - New helper: src/renderer/utils/runtimeContext.ts exposes isWebDesktop() / isElectronDesktop(). Reads the server-injected __MAESTRO_CONFIG__ (load-bearing) with a /desktop/ path fallback. Memoized after first call. - SessionList.tsx hides the global LIVE / OFFLINE toggle when isWebDesktop() is true. Toggling Live Mode off from inside the bridge would kill the webserver the user's own browser is connected to, which is exactly the disconnect chr1syy hit. - HamburgerMenuContent.tsx hides the Check for Updates entry in web- desktop — it assumes the Electron binary is the runtime host. - App.tsx hides the draggable title bar (WebkitAppRegion: drag) in web-desktop. There's no Electron host to drag the window of, the bar's just visual clutter, and a stale instance had re-appeared since the round-1 cleanup. - App.tsx also drops the 40px (pt-10) top reservation that was there to clear the now-hidden title bar — without this, the renderer left a blank strip at the top of /desktop/. Bonus — revert HTML cache from round-1 P1 fix - WebServer.ts: the round-1 PR1-P1 "cache the served index.html on first read" change served stale Vite asset hashes after every rebuild and left /desktop/ stuck on "Loading…" with 404s in the network panel. Path resolution stays cached (cheap, doesn't change across rebuilds), but the HTML itself is now re-read per request. The readFileSync of a sub-2KB index is negligible compared to the bug. Validation - npm run lint: clean - npm run lint:eslint: clean - npm run test: 27,847 passed / 108 skipped / 0 failed - npm run build:web-desktop: clean - Live verification via Interceptor in real Chrome at http://localhost:31999/<token>/desktop/: • Web-Desktop Bundle toggle visible in Encore Features tab • LIVE/OFFLINE chip absent from header • Check for Updates absent from hamburger menu • Drag-region title bar absent from DOM (0 elements with app-region: drag) • Computed paddingTop on .maestro-app-shell == 0px • Settings modal opens, tabs navigate, prompts editor loads • Bug 2 ping/pong round-trip still intact
|
Thanks @chr1syy — the manual test pass made all the difference here. The level of detail (repro steps, severity ranking, references to specific code surfaces, screenshots in your head) turned what would've been four debugging sessions into four targeted fixes. Especially appreciate the headline call on Bug 2 — that one was hiding behind a passing CI suite and I would not have found it from code review alone. Pushed Bug 2 — messages from
|
|
Two more follow-up findings from the latest manual test pass against PR #991 head ( Bug 5 —
|
|
One more finding from continued testing on the bug-fix build. Bug 2's safeSend fix is correctly merged and the broadcast path is intact — verified Bug 7 — Cross-view messages: user input doesn't fan out and agent responses collapse into one blobRepro:
What works:
Most likely root cause: The renderer splits the streamed This also explains why the user-typed message never appears in the cross-view: it isn't a Fix direction (two equally workable options):
The first option is closer to how the desktop renderer already works (everything fans out via main), and avoids re-shaping the data plane. Diagnostic that misdirected us firstI initially suspected Open offer from the earlier comment still stands: happy to bundle Bug 5 ( |
|
One more finding from continued testing on the bug-fix build. Bug 8 — Copy-to-Clipboard silently fails on AI and Human messages in
|
|
Quick follow-up on Bug 8 — the blast radius is wider than my comment implied. Right-click → Copy Link on a hyperlink in agent chat fails with the same symptom (silent, no clipboard write, no toast). Traced to const handleCopy = useCallback(() => {
safeClipboardWrite(menu.url);
onDismiss();
}, [menu.url, onDismiss]);Same
Effectively every copy action in the renderer is silently broken in |
3 participants
Summary
Ships the full Maestro desktop UI through the browser by mirroring every
ipcMainhandler over a WebSocket bridge. Renderer code runs unchanged in the browser via a Vite-aliasedelectronshim. Gated behind a newwebDesktopBundleEncore Feature so the IPC surface stays sealed by default.Architecture — bridge IPC mirror (1:1)
Server (
src/main/web-server/handlers/bridgeHandlers.ts)bridge.invokemessages againstipcMain._invokeHandlersWebContents.sendcalls asbridge.eventto all web clientsWeb shim (
src/web-desktop/electron-shim.ts)electronmodule at build timeipcRenderer,contextBridge, reconnect-awareBridgeClientBootstrap & build
src/web-desktop/bootstrap.tspolyfillswindow.process, loads preload then renderer@sentry/electron/rendereraliased to a no-op shimvite.config.web-desktop.mtsoutputsdist/web-desktop/, served at/<token>/desktop/Encore Feature gate (4 enforcement points)
WebServerconstructor takesgetWebDesktopBundleEnabledcallbackweb-server-factory.tswires it tosettingsStore.encoreFeatures.webDesktopBundlebridge.invokedispatchmaestro-cli settings set encoreFeatures.webDesktopBundle trueResponsive mobile UX
useViewportBreakpointwritesdata-bp(xs/sm/md/lg/xl) on<html>so CSS targets without re-renders{N}%, secondary buttons hide below 480px…on narrow viewports, nowrap to prevent wraparia-label100dvh+env(safe-area-inset-bottom)for iOS safe areasiOS Safari fixes
generateId— feature-detectscrypto.randomUUID(requires secure context on Safari) and falls back togenerateUUIDfromshared/uuid.tsPromise.allSettledso a failedfs.statdoesn't drop the file content from previewTests
bridgeHandlers.test.ts— 5 cases (empty channel, unknown channel, success, handler error, arg pass-through). Usesvi.hoistedto stubipcMain._invokeHandlerswithout a real Electron runtimeValidation
npm run lint(tsc all configs): cleannpm run lint:eslint: cleannpm run test: 27,844 passed / 108 skipped / 0 failednpm run build: main + renderer + web + web-desktop + cli all builtvalidate:push): passedHow to test
Then open
http://<host>:31999/<token>/desktop/from your browser — Safari, Chrome, mobile over Tailscale all supported.Out-of-scope notes
Three files included at Ron's request (personal workstreams, separate from the bundle):
PRD-CY-CONTEXT-RUNTIME.md— Cy runtime integration PRDscripts/import-vram-sessions.ts— session import utilityscripts/populate-real-conversations.ts— conversation data utilitySummary by CodeRabbit
New Features
Bug Fixes
Tests