fix(desktop): fix renderer freeze while streaming workspace logs#609
Merged
Conversation
The ResizeObserver callback unconditionally called fit(), refresh(), and the terminalResize IPC on every fire. Since fit() mutates the terminal's dimensions, it could re-fire the observer, sustaining a frame-rate loop that pins the renderer main thread at 100% CPU and freezes the window. Track the last propagated cols/rows and early-return when the grid size is unchanged, so the loop can no longer sustain itself.
✅ Deploy Preview for devsydev canceled.
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughLogTable.svelte is rewritten as a virtualized, scroll-tracking log viewer backed by a caching log-parser. WorkspaceWizard and WorkspaceDetailPage adopt buffered, animation-frame-flushed output streaming and render via LogTable in follow mode. Terminal.svelte deduplicates redundant resize handling. A ResizeObserver test polyfill is added. ChangesVirtualized Log Viewer and Streaming Integration
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Backend
participant WorkspaceDetailPage
participant pendingLinesBuffer
participant LogTable
Backend->>WorkspaceDetailPage: progress.message
WorkspaceDetailPage->>pendingLinesBuffer: push message
WorkspaceDetailPage->>WorkspaceDetailPage: schedule requestAnimationFrame(flushLines)
Backend->>WorkspaceDetailPage: progress.done
WorkspaceDetailPage->>WorkspaceDetailPage: cancel scheduled frame
WorkspaceDetailPage->>pendingLinesBuffer: flushLines()
pendingLinesBuffer-->>WorkspaceDetailPage: appended to outputLines
WorkspaceDetailPage->>LogTable: render outputLines with follow
sequenceDiagram
participant User
participant LogTable
participant ResizeObserver
participant parseLogLine
User->>LogTable: scroll event
LogTable->>LogTable: onScroll updates scrollTop, isPinned
ResizeObserver->>LogTable: viewport size change
LogTable->>LogTable: recompute totalHeight, visible range
LogTable->>parseLogLine: parse visible raw lines
parseLogLine-->>LogTable: ParsedLogLine[]
LogTable->>LogTable: render visible rows
alt follow enabled and isPinned
LogTable->>LogTable: auto-scroll to newest entries
end
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
✅ Deploy Preview for images-devsy-sh canceled.
|
The workspace creation wizard (launch tab) and workspace detail page froze the renderer at 100% CPU while streaming container logs, even though the underlying CLI completed successfully. Two compounding causes on the log-streaming path: - Each CommandProgress event triggered a full state flush (outputLines = [...outputLines, msg]) plus a scroll rAF, so a fast stream produced one reactive flush per line. - LogTable re-derived parsed = lines.map(parseLogLine) over the entire buffer on every append, making parse work quadratic in line count. Coalesce incoming lines into a single per-frame flush via requestAnimationFrame, and memoize parseLogLine so each unique line is parsed once. Pending state and the frame handle are cleared on reset, new operation start, and component destroy.
skevetter
changed the title
Opening a large persisted log (observed: 3.4 MB / 12,456 lines for a single workspace launch) rendered every line as a table row at once, producing tens of thousands of DOM nodes in one synchronous pass and freezing the renderer regardless of parse cost. Cap LogTable at the most recent maxLines rows (default 5000) and show a notice for the hidden count. Covers both the live-stream and the persisted-log-file render sites. The Copy actions still operate on the full buffer, so no data is lost.
Replace the fixed 5000-line render cap with a windowed virtual list: LogTable now renders only the rows visible in its own scroll viewport (plus overscan), so a 12k-line log mounts a handful of DOM nodes instead of thousands. Rows are fixed-height and absolutely positioned within a spacer sized to the full line count, giving a native scrollbar over the entire log with no pagination controls and no truncation. The component owns its viewport and an optional follow mode that pins to the tail as new lines stream in, but only while the user is already at the bottom. Consumers drop their outer scroll wrappers and manual scrollIntoView hooks (outputEl / tableEndEl / scrollToBottom).
The virtualized LogTable uses a ResizeObserver, which jsdom does not implement. Rendering LogTable consumers in tests threw an uncaught ReferenceError, failing the suite even though all assertions passed. Add a no-op ResizeObserver polyfill alongside the existing matchMedia one.
skevetter
marked this pull request as ready for review
July 7, 2026 06:36
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
desktop/src/renderer/src/pages/WorkspaceDetailPage.svelte (1)
153-158: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSame per-frame full-array copy as the Wizard.
flushLinesreallocates all ofoutputLineson every flush (~O(n²) over a long stream).outputLinesis a$statearray, so an in-placepushavoids the copy while still updatingLogTable.♻️ Proposed change
function flushLines() { flushHandle = null if (pendingLines.length === 0) return - outputLines = [...outputLines, ...pendingLines] - pendingLines = [] + outputLines.push(...pendingLines) + pendingLines.length = 0 }Same Svelte 5 reactivity confirmation as noted in
WorkspaceWizard.svelteapplies here.
[source_library_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 `@desktop/src/renderer/src/pages/WorkspaceDetailPage.svelte` around lines 153 - 158, The flushLines function in WorkspaceDetailPage.svelte is repeatedly recreating outputLines on every frame, causing unnecessary full-array copies during long streams. Update flushLines to append pendingLines into the existing $state array in place instead of reassignment, keeping the reactive LogTable updates while avoiding the O(n²) behavior; use the existing flushLines, outputLines, and pendingLines symbols to make the change consistent with the Wizard fix.desktop/src/renderer/src/lib/components/workspace/WorkspaceWizard.svelte (1)
415-420: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid rebuilding
outputLineson each flush.outputLinesis a$statearray, sopush(...pendingLines)keepsLogTablereactive throughlines.lengthand avoids copying the full buffer every frame.♻️ Proposed change
function flushLines() { flushHandle = null if (pendingLines.length === 0) return - outputLines = [...outputLines, ...pendingLines] - pendingLines = [] + outputLines.push(...pendingLines) + pendingLines.length = 0 }🤖 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 `@desktop/src/renderer/src/lib/components/workspace/WorkspaceWizard.svelte` around lines 415 - 420, In flushLines in WorkspaceWizard.svelte, avoid replacing the entire outputLines $state array on every flush, since that rebuilds the full buffer and is unnecessary. Update the existing array in place by appending pendingLines to outputLines, then clear pendingLines and keep the flushHandle reset logic unchanged. This preserves reactivity for LogTable via outputLines.length and reduces repeated copying during frequent flushes.desktop/src/renderer/src/lib/components/log/LogTable.svelte (1)
87-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider preserving table semantics for assistive tech.
Replacing the
<table>with absolutely-positioned grid<div>s drops the row/column semantics that screen readers relied on, so the Time/Level/Message relationship is no longer announced. Adding ARIA roles keeps the semantics without affecting the virtualization/layout.♻️ Suggested ARIA roles
<div bind:this={viewport} onscroll={onScroll} + role="table" class="relative overflow-auto rounded-md border {maxHeightClass} {className}" > <div + role="row" class="sticky top-0 z-10 grid grid-cols-[5rem_6rem_1fr] border-b bg-background text-start font-medium" style="height: {rowHeight}px" > - <div class="flex items-center px-2">Time</div> - <div class="flex items-center px-2">Level</div> - <div class="flex items-center px-2">Message</div> + <div role="columnheader" class="flex items-center px-2">Time</div> + <div role="columnheader" class="flex items-center px-2">Level</div> + <div role="columnheader" class="flex items-center px-2">Message</div> </div>🤖 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 `@desktop/src/renderer/src/lib/components/log/LogTable.svelte` around lines 87 - 118, The virtualized log rendering in LogTable currently replaces the table structure with positioned divs, which removes the row/column semantics used by assistive tech. Update the LogTable markup around the visible row loop to preserve table semantics by adding appropriate ARIA roles to the container, row wrapper, and each Time/Level/Message cell so screen readers still announce the relationship correctly. Keep the existing virtualization and styling logic intact, and ensure the semantics map cleanly to the row.index and row.line fields without changing the display behavior.
🤖 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 `@desktop/src/renderer/src/lib/components/log/LogTable.svelte`:
- Around line 73-86: Remove the extra scroll wrapper around LogTable in
ProviderWizard.svelte so the table’s own viewport remains the only scrolling
container. LogTable already handles scrolling and virtualization through its
bind:this={viewport} element with overflow-auto, so the outer max-h-48
overflow-y-auto wrapper is causing a duplicate scrollbar and incorrect height.
Update ProviderWizard to either pass the appropriate maxHeightClass into
LogTable or render LogTable directly without the additional wrapper.
---
Nitpick comments:
In `@desktop/src/renderer/src/lib/components/log/LogTable.svelte`:
- Around line 87-118: The virtualized log rendering in LogTable currently
replaces the table structure with positioned divs, which removes the row/column
semantics used by assistive tech. Update the LogTable markup around the visible
row loop to preserve table semantics by adding appropriate ARIA roles to the
container, row wrapper, and each Time/Level/Message cell so screen readers still
announce the relationship correctly. Keep the existing virtualization and
styling logic intact, and ensure the semantics map cleanly to the row.index and
row.line fields without changing the display behavior.
In `@desktop/src/renderer/src/lib/components/workspace/WorkspaceWizard.svelte`:
- Around line 415-420: In flushLines in WorkspaceWizard.svelte, avoid replacing
the entire outputLines $state array on every flush, since that rebuilds the full
buffer and is unnecessary. Update the existing array in place by appending
pendingLines to outputLines, then clear pendingLines and keep the flushHandle
reset logic unchanged. This preserves reactivity for LogTable via
outputLines.length and reduces repeated copying during frequent flushes.
In `@desktop/src/renderer/src/pages/WorkspaceDetailPage.svelte`:
- Around line 153-158: The flushLines function in WorkspaceDetailPage.svelte is
repeatedly recreating outputLines on every frame, causing unnecessary full-array
copies during long streams. Update flushLines to append pendingLines into the
existing $state array in place instead of reassignment, keeping the reactive
LogTable updates while avoiding the O(n²) behavior; use the existing flushLines,
outputLines, and pendingLines symbols to make the change consistent with the
Wizard fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c2aaf326-34ff-436f-8d29-bd8fda461248
📒 Files selected for processing (6)
desktop/src/renderer/src/lib/__mocks__/setup.tsdesktop/src/renderer/src/lib/components/log/LogTable.sveltedesktop/src/renderer/src/lib/components/terminal/Terminal.sveltedesktop/src/renderer/src/lib/components/workspace/WorkspaceWizard.sveltedesktop/src/renderer/src/lib/utils/log-parser.tsdesktop/src/renderer/src/pages/WorkspaceDetailPage.svelte
- ProviderWizard: drop the outer scroll wrappers around LogTable so the table's own viewport is the only scroll container (was a double scrollbar with wrong height after LogTable started owning its viewport); pass maxHeightClass instead. - WorkspaceWizard / WorkspaceDetailPage: push into the outputLines $state array in place instead of reallocating the full buffer each flush; length reset stays reactive for LogTable via lines.length. - LogTable: restore table semantics for assistive tech with role=table/row/rowgroup/columnheader/cell and aria-rowcount/rowindex, which the virtualized div layout had dropped.
skevetter
marked this pull request as draft
July 7, 2026 07:03
skevetter
marked this pull request as ready for review
July 7, 2026 10:11
There was a problem hiding this comment.
🧹 Nitpick comments (1)
desktop/src/renderer/src/lib/components/provider/ProviderWizard.svelte (1)
532-532: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
rounded-nonelikely won't override LogTable's built-inrounded-md.
LogTable's root rendersclass="... rounded-md border {maxHeightClass} {className}", so the passedclassName="border-x-0 border-b-0 rounded-none"is appended afterrounded-md, not before. Since Tailwind resolves same-property conflicts by stylesheet generation order (not class-attribute order), androunded-mdis emitted afterrounded-nonein the generated CSS,rounded-mdwins — the corners will likely stay rounded despite the intended override. Theborder-x-0/border-b-0overrides target different longhand properties than the basebordershorthand and should apply fine.💅 Suggested fix using a class-merge utility
-<LogTable lines={initLines} maxHeightClass="max-h-48" class="border-x-0 border-b-0 rounded-none" /> +<LogTable lines={initLines} maxHeightClass="max-h-48" class={cn("border-x-0 border-b-0 rounded-none")} />(where
cnusestailwind-merge/twMergeto resolve conflicting utilities deterministically, if such a helper already exists in this codebase)Per Tailwind's own docs, "When you add two classes that target the same CSS property, the class that appears later in the stylesheet wins." and generated utility order places
rounded-mdafterrounded-none.🤖 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 `@desktop/src/renderer/src/lib/components/provider/ProviderWizard.svelte` at line 532, The `LogTable` usage in `ProviderWizard.svelte` is trying to remove corner rounding with `rounded-none`, but `LogTable` already applies `rounded-md` in its root class list, so the override likely won’t win. Update the `LogTable` class handling (or the passed class composition) so conflicting Tailwind utilities are merged deterministically, using the existing `cn`/`twMerge` helper if available, and ensure the `rounded-none` intent is applied after `rounded-md` in the merged result.
🤖 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.
Nitpick comments:
In `@desktop/src/renderer/src/lib/components/provider/ProviderWizard.svelte`:
- Line 532: The `LogTable` usage in `ProviderWizard.svelte` is trying to remove
corner rounding with `rounded-none`, but `LogTable` already applies `rounded-md`
in its root class list, so the override likely won’t win. Update the `LogTable`
class handling (or the passed class composition) so conflicting Tailwind
utilities are merged deterministically, using the existing `cn`/`twMerge` helper
if available, and ensure the `rounded-none` intent is applied after `rounded-md`
in the merged result.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d76210c8-613a-49b2-ad9d-094b176a7b8c
📒 Files selected for processing (4)
desktop/src/renderer/src/lib/components/log/LogTable.sveltedesktop/src/renderer/src/lib/components/provider/ProviderWizard.sveltedesktop/src/renderer/src/lib/components/workspace/WorkspaceWizard.sveltedesktop/src/renderer/src/pages/WorkspaceDetailPage.svelte
🚧 Files skipped from review as they are similar to previous changes (3)
- desktop/src/renderer/src/lib/components/workspace/WorkspaceWizard.svelte
- desktop/src/renderer/src/lib/components/log/LogTable.svelte
- desktop/src/renderer/src/pages/WorkspaceDetailPage.svelte
LogTable built its root class by string interpolation, so a consumer's className was appended after the base rounded-md/max-h-96 rather than overriding them — Tailwind resolves same-property conflicts by generated stylesheet order, letting the base classes win. Route the root class through cn() (clsx + tailwind-merge) so passed classes like rounded-none and max-h-48 deterministically override the defaults.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a renderer freeze where the desktop window pinned the main thread at 100% CPU and became unresponsive. Reproduced during workspace creation (wizard launch tab) while the container streamed logs: the log view froze even though the underlying CLI completed the launch successfully. A representative launch produced a 3.4 MB / 12,456-line log with individual lines up to ~13 KB.
Diagnosed via
sampleon a frozen renderer: the main thread was spinning permanently insidev8::MicrotasksScope::PerformCheckpoint(JIT frames, unsymbolized).Root cause
Three compounding costs on the log-rendering path:
CommandProgressevent didoutputLines = [...outputLines, msg]plus a scrollrequestAnimationFrame, so a fast stream produced one full Svelte state flush per line.LogTablere-derivedparsed = lines.map(parseLogLine)over the entire buffer on every append; each line ran several regexes +JSON.parse. Line N re-parsed all N−1 prior lines.LogTablerendered one table row per line, so a large log built tens of thousands of DOM nodes in one synchronous layout.Changes
log-parser.ts— memoizeparseLogLine(boundedMap, cap 10k). Each unique line is parsed once.WorkspaceWizard.svelte/WorkspaceDetailPage.svelte— coalesce incoming lines intopendingLinesand flush once per animation frame instead of per line. Cleared on reset, new operation start, andonDestroy.LogTable.svelte— virtualized (windowed) rendering. The component owns its scroll viewport and renders only the rows visible in view (plus overscan) as fixed-height absolutely-positioned rows over a full-height spacer. A 12k-line log mounts a handful of DOM nodes; the full log is scrollable with no pagination and no truncation. An opt-infollowmode pins to the tail as lines stream, but only while the user is already at the bottom. Consumers dropped their outer scroll wrappers and manualscrollIntoViewhooks.Terminal.svelteResizeObserver guard was split into fix(desktop): guard terminal ResizeObserver against feedback loop #610 (unrelated defensive hardening).Verification
npx svelte-checkon the renderer: 0 errors (1 pre-existing unrelated warning).npx vitest run: 243/243 tests pass. (One main-process suite,cli.test.ts, fails only because deps were installed with--ignore-scriptsso Electron's binary wasn't fetched — unrelated to these changes; CI installs normally.)Notes
A live symbolized profile was not captured; the O(n²) / unbounded-DOM path is strongly supported by the source audit and the exact repro (freeze while streaming a 3.4 MB log, CLI succeeds).
Summary by CodeRabbit
ResizeObserverfallback when missing.