fix(tui): resolve streaming freeze from GC pressure and event flooding#1
Closed
coleleavitt wants to merge 94 commits intodevfrom
Closed
fix(tui): resolve streaming freeze from GC pressure and event flooding#1coleleavitt wants to merge 94 commits intodevfrom
coleleavitt wants to merge 94 commits intodevfrom
Conversation
…o#14079) Co-authored-by: Aiden Cline <63023139+rekram1-node@users.noreply.github.com>
Co-authored-by: adamelmore <2363879+adamdottv@users.noreply.github.com>
Co-authored-by: David Hill <iamdavidhill@gmail.com>
Co-authored-by: David Hill <iamdavidhill@gmail.com>
Batch ALL high-frequency streaming events (delta, status, part, message, todo, diff) into a unified 100ms flush window to reduce store mutations and GC pressure. Previously only delta events were debounced while 8 other event types triggered immediate renders, causing full markdown re-parse per token and triggering Bun's JSC GC madvise spin loop. - sync.tsx: unified batch flush for all streaming events - sdk.tsx: increase event batch window from 16ms to 50ms - processor.ts: fix stream termination (for-await never exited on finish) - db.ts: handle fire-and-forget effect Promise rejections - mcp/index.ts: add timeout to client.listTools() calls
|
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
|
Important Review skippedToo many files! This PR contains 274 files, which is 124 over the limit of 150. ⛔ Files ignored due to path filters (26)
📒 Files selected for processing (274)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Review Summary by QodoRefactor TUI config, add Go subscription support, implement cross-platform process utility, and fix stream termination
WalkthroughsDescription**Major refactoring and feature additions across multiple packages:** • **TUI Configuration Separation**: Extracted TUI-specific settings (theme, keybinds, tui) from main config schema into dedicated tui.json files with automatic migration from legacy opencode.json format • **Cross-Platform Process Utility**: Introduced new Process utility for consistent process spawning across Bun and Node.js, replacing scattered Bun.spawn() calls throughout codebase (LSP server, ripgrep, git, clipboard, bun registry) • **Go Subscription Plan Support**: Added comprehensive "lite" subscription tier alongside existing "black" plans with usage tracking, billing validation, and Stripe webhook integration • **Stream Termination Fix**: Fixed session processor stream hang where for await loop never exited on finish event, preventing proper cleanup • **Bash Output Deduplication**: Implemented snapshot deduplication and synthetic pending state handling in ACP agent to prevent duplicate tool updates • **Windows Path Compatibility**: Fixed cross-platform path handling for Windows backslashes, UNC paths, and symlink support with git config flags • **Comprehensive Test Coverage**: Added 510+ lines of TUI config tests, subscription analysis tests, date utility tests, permission tests, and process utility tests • **i18n Updates**: Added 30+ new translation keys across 15+ languages for Black plan pause messages and Go subscription UI • **Billing Schema Extension**: Extended database schema with LiteTable for Go plan usage tracking and subscription management • **Permission Terminology Fixes**: Corrected permission-related terminology across 20+ i18n files from "edits" to "permissions" Diagramflowchart LR
A["Config System"] -->|"Extract TUI keys"| B["TUI Config<br/>tui.json"]
A -->|"Migrate legacy"| C["Migration Utility"]
C -->|"Create"| B
D["Process Spawning"] -->|"Unify"| E["Process Utility<br/>Node.js child_process"]
E -->|"Replace"| F["Bun.spawn<br/>calls"]
G["Subscription"] -->|"Add tier"| H["Go Plan<br/>lite"]
H -->|"Track usage"| I["LiteTable<br/>DB Schema"]
H -->|"Webhook"| J["Stripe Events"]
K["Session Processor"] -->|"Fix loop"| L["Stream Termination<br/>finish event"]
M["ACP Agent"] -->|"Deduplicate"| N["Bash Output<br/>Snapshots"]
O["Path Helper"] -->|"Support"| P["Windows Paths<br/>UNC + Symlinks"]
File Changes1. packages/opencode/test/config/tui.test.ts
|
Code Review by Qodo
1. scheduleBatchFlush uses any
|
![devloai[bot]](https://avatars.githubusercontent.com/in/886113?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary:
- Batches all high-frequency streaming events (
delta,status,part,message,todo,diff) into a 100ms flush window to reduce GC pressure - Increases SDK event batch window from 16ms → 50ms
- Fixes stream termination bug in
processor.tswherefor awaitnever exited onfinishevent - Adds error handling for fire-and-forget effect Promises in
db.ts - Adds
withTimeout()toclient.listTools()inmcp/index.ts
Review Summary:
The core batching approach in sync.tsx is sound and addresses a real GC pressure problem. The processor.ts stream termination fix is correct — finish signals the provider is done and break safely calls return() on the AsyncGenerator. The db.ts effect error handling and MCP timeout additions are clean improvements.
One architectural issue stands out: all pending event Maps and the batchTimer are declared as module-level globals, meaning they are shared across all SyncProvider instances and persist beyond component lifetime. If the provider unmounts while a flush is pending, the timer fires with a stale setStore/store closure. This is a real, reproducible bug in environments with hot-reload or component remounting. This state should live inside init() with an onCleanup guard.
Follow-up suggestions:
- @devloai fix the module-level global state issue in
sync.tsxby moving pending Maps andbatchTimerinsideinit()with properonCleanupteardown
| const pendingDiffs = new Map<string, import("@/snapshot").Snapshot.FileDiff[]>() | ||
| let batchTimer: Timer | undefined | ||
|
|
||
| function scheduleBatchFlush(setStore: any, store: any) { |
There was a problem hiding this comment.
The scheduleBatchFlush function captures setStore and store from the first call only (subsequent calls early-return when batchTimer is already set). This means if a second SyncProvider instance were mounted (e.g. hot-reload, navigate away and back), the timer would keep flushing stale closures.
More critically: all the pending Maps are module-level globals shared across all SyncProvider instances. If the component unmounts while a timer is pending, the flush fires after unmount with a stale setStore/store reference, potentially writing to a dead reactive scope.
Consider moving all the pending state and the timer inside the init function and returning a cleanup that cancels the timer:
// Inside init():
let batchTimer: Timer | undefined
const pendingDeltas = new Map(...)
// ... etc
onCleanup(() => {
if (batchTimer) clearTimeout(batchTimer)
batchTimer = undefined
pendingDeltas.clear()
// etc.
})actions
Quick Fix: Apply a small focused fix for this specific issue (takes < 1min).
Feedback: Rate this comment to help me improve future code reviews:
- 👍 Good - Helpful and accurate
- 👎 Poor - Wrong, unclear, or unhelpful
- Skip if you don't have any strong opinions either way.
| try { | ||
| let currentText: MessageV2.TextPart | undefined | ||
| let reasoningMap: Record<string, MessageV2.ReasoningPart> = {} | ||
| let finished = false |
There was a problem hiding this comment.
The let finished = false declaration is indented at 4 spaces while the surrounding declarations use 12 spaces (3 levels of nesting: try, while, function body). This appears to be a copy-paste indentation error.
let currentText: MessageV2.TextPart | undefined
let reasoningMap: Record<string, MessageV2.ReasoningPart> = {}
let finished = false
const stream = await LLM.stream(streamInput)actions
Quick Fix: Apply a small focused fix for this specific issue (takes < 1min).
Feedback: Rate this comment to help me improve future code reviews:
- 👍 Good - Helpful and accurate
- 👎 Poor - Wrong, unclear, or unhelpful
- Skip if you don't have any strong opinions either way.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
9 issues found across 348 files
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/app/src/pages/session/helpers.ts">
<violation number="1" location="packages/app/src/pages/session/helpers.ts:34">
P2: Missing `.catch()` on the promise chain — if `loadFile` rejects, this will produce an unhandled promise rejection. Add error handling to be consistent with the PR's stated goal of handling fire-and-forget promise rejections.</violation>
</file>
<file name="packages/app/src/pages/session/file-tabs.tsx">
<violation number="1" location="packages/app/src/pages/session/file-tabs.tsx:377">
P2: `cancelCommenting` bypasses `setCommenting(null)`, skipping the `scheduleComments()` side-effect that clears `draftTop`. Use the existing wrapper to keep behavior consistent.</violation>
</file>
<file name="packages/app/src/pages/layout.tsx">
<violation number="1" location="packages/app/src/pages/layout.tsx:1097">
P2: `navigateToProject` is now `async` but callers (e.g., `openProject`, `closeProject`, the reactive `on()` block, and `chooseProject`) still call it without `await` — creating fire-and-forget promises. Any unexpected throw becomes a silent unhandled rejection, and concurrent calls (e.g., from the deep-link loop via `openProject`) now race non-deterministically. Consider either awaiting/`.catch()`-ing at call sites, or wrapping the entire body in a try/catch to guarantee no rejections escape.</violation>
</file>
<file name="packages/app/src/pages/session/session-side-panel.tsx">
<violation number="1" location="packages/app/src/pages/session/session-side-panel.tsx:226">
P2: Review count badge lost all styling classes — the pill indicator (`rounded-full bg-surface-base`, text/padding/flex classes) was removed, leaving a bare unstyled `<div>`. This looks like an accidental regression from the refactor.</violation>
</file>
<file name="packages/app/src/components/dialog-select-model-unpaid.tsx">
<violation number="1" location="packages/app/src/components/dialog-select-model-unpaid.tsx:100">
P2: Two adjacent `<Show>` blocks share the identical condition `i.id === "opencode"`. Merge them into a single `<Show>` with a fragment, consistent with how the `opencode-go` block is structured just below.</violation>
</file>
<file name="packages/app/src/components/dialog-select-provider.tsx">
<violation number="1" location="packages/app/src/components/dialog-select-provider.tsx:74">
P3: Duplicate `<Show when={i.id === "opencode"}>` — this new block checks the same condition as the existing one a few lines below. Merge them into a single `<Show>` block to avoid redundant condition evaluation and improve readability.</violation>
</file>
<file name="packages/app/src/utils/server-errors.ts">
<violation number="1" location="packages/app/src/utils/server-errors.ts:23">
P2: Typo in exported function name: `parseReabaleConfigInvalidError` should be `parseReadableConfigInvalidError`. Since this is a newly added exported symbol, fixing the name now avoids a breaking rename later.</violation>
<violation number="2" location="packages/app/src/utils/server-errors.ts:30">
P2: Bug: The empty string `""` intended as a blank-line separator is removed by `.filter(Boolean)` (empty string is falsy). The issues will appear directly after the header with no visual break. Consider filtering before inserting the separator.</violation>
</file>
<file name=".github/workflows/compliance-close.yml">
<violation number="1" location=".github/workflows/compliance-close.yml:75">
P3: Empty `catch (e) {}` silently swallows all errors, not just the expected 404. Consider logging the error at debug level so unexpected failures (auth issues, rate limits, network errors) aren't invisible.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| input.loadFile(path) | ||
| const maybePromise = input.loadFile(path) | ||
| const openTab = () => input.openTab(input.tabForPath(path)) | ||
| if (maybePromise instanceof Promise) maybePromise.then(openTab) |
There was a problem hiding this comment.
P2: Missing .catch() on the promise chain — if loadFile rejects, this will produce an unhandled promise rejection. Add error handling to be consistent with the PR's stated goal of handling fire-and-forget promise rejections.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/app/src/pages/session/helpers.ts, line 34:
<comment>Missing `.catch()` on the promise chain — if `loadFile` rejects, this will produce an unhandled promise rejection. Add error handling to be consistent with the PR's stated goal of handling fire-and-forget promise rejections.</comment>
<file context>
@@ -24,13 +24,15 @@ export const createOpenReviewFile = (input: {
- input.loadFile(path)
+ const maybePromise = input.loadFile(path)
+ const openTab = () => input.openTab(input.tabForPath(path))
+ if (maybePromise instanceof Promise) maybePromise.then(openTab)
+ else openTab()
})
</file context>
| const cancelCommenting = () => { | ||
| const p = path() | ||
| if (p) file.setSelectedLines(p, null) | ||
| setNote("commenting", null) |
There was a problem hiding this comment.
P2: cancelCommenting bypasses setCommenting(null), skipping the scheduleComments() side-effect that clears draftTop. Use the existing wrapper to keep behavior consistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/app/src/pages/session/file-tabs.tsx, line 377:
<comment>`cancelCommenting` bypasses `setCommenting(null)`, skipping the `scheduleComments()` side-effect that clears `draftTop`. Use the existing wrapper to keep behavior consistent.</comment>
<file context>
@@ -371,6 +371,12 @@ export function FileTabContent(props: { tab: string }) {
+ const cancelCommenting = () => {
+ const p = path()
+ if (p) file.setSelectedLines(p, null)
+ setNote("commenting", null)
+ }
+
</file context>
Suggested change
| setNote("commenting", null) | |
| setCommenting(null) |
| } | ||
|
|
||
| function navigateToProject(directory: string | undefined) { | ||
| async function navigateToProject(directory: string | undefined) { |
There was a problem hiding this comment.
P2: navigateToProject is now async but callers (e.g., openProject, closeProject, the reactive on() block, and chooseProject) still call it without await — creating fire-and-forget promises. Any unexpected throw becomes a silent unhandled rejection, and concurrent calls (e.g., from the deep-link loop via openProject) now race non-deterministically. Consider either awaiting/.catch()-ing at call sites, or wrapping the entire body in a try/catch to guarantee no rejections escape.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/app/src/pages/layout.tsx, line 1097:
<comment>`navigateToProject` is now `async` but callers (e.g., `openProject`, `closeProject`, the reactive `on()` block, and `chooseProject`) still call it without `await` — creating fire-and-forget promises. Any unexpected throw becomes a silent unhandled rejection, and concurrent calls (e.g., from the deep-link loop via `openProject`) now race non-deterministically. Consider either awaiting/`.catch()`-ing at call sites, or wrapping the entire body in a try/catch to guarantee no rejections escape.</comment>
<file context>
@@ -1093,14 +1094,51 @@ export default function Layout(props: ParentProps) {
}
- function navigateToProject(directory: string | undefined) {
+ async function navigateToProject(directory: string | undefined) {
if (!directory) return
const root = projectRoot(directory)
</file context>
| <div class="flex items-center gap-1.5"> | ||
| <div>{language.t("session.tab.review")}</div> | ||
| <Show when={hasReview()}> | ||
| <div>{reviewCount()}</div> |
There was a problem hiding this comment.
P2: Review count badge lost all styling classes — the pill indicator (rounded-full bg-surface-base, text/padding/flex classes) was removed, leaving a bare unstyled <div>. This looks like an accidental regression from the refactor.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/app/src/pages/session/session-side-panel.tsx, line 226:
<comment>Review count badge lost all styling classes — the pill indicator (`rounded-full bg-surface-base`, text/padding/flex classes) was removed, leaving a bare unstyled `<div>`. This looks like an accidental regression from the refactor.</comment>
<file context>
@@ -202,133 +202,123 @@ export function SessionSidePanel(props: {
+ <div class="flex items-center gap-1.5">
+ <div>{language.t("session.tab.review")}</div>
+ <Show when={hasReview()}>
+ <div>{reviewCount()}</div>
+ </Show>
+ </div>
</file context>
Suggested change
| <div>{reviewCount()}</div> | |
| <div class="text-12-medium text-text-strong h-4 px-2 flex flex-col items-center justify-center rounded-full bg-surface-base"> | |
| {reviewCount()} | |
| </div> |
| <div class="w-full flex items-center gap-x-3"> | ||
| <ProviderIcon data-slot="list-item-extra-icon" id={i.id as IconName} /> | ||
| <span>{i.name}</span> | ||
| <Show when={i.id === "opencode"}> |
There was a problem hiding this comment.
P2: Two adjacent <Show> blocks share the identical condition i.id === "opencode". Merge them into a single <Show> with a fragment, consistent with how the opencode-go block is structured just below.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/app/src/components/dialog-select-model-unpaid.tsx, line 100:
<comment>Two adjacent `<Show>` blocks share the identical condition `i.id === "opencode"`. Merge them into a single `<Show>` with a fragment, consistent with how the `opencode-go` block is structured just below.</comment>
<file context>
@@ -97,9 +97,20 @@ export const DialogSelectModelUnpaid: Component = () => {
<div class="w-full flex items-center gap-x-3">
<ProviderIcon data-slot="list-item-extra-icon" id={i.id as IconName} />
<span>{i.name}</span>
+ <Show when={i.id === "opencode"}>
+ <div class="text-14-regular text-text-weak">{language.t("dialog.provider.opencode.tagline")}</div>
+ </Show>
</file context>
| return o.name === "ConfigInvalidError" && typeof o.data === "object" && o.data !== null | ||
| } | ||
|
|
||
| export function parseReabaleConfigInvalidError(errorInput: ConfigInvalidError) { |
There was a problem hiding this comment.
P2: Typo in exported function name: parseReabaleConfigInvalidError should be parseReadableConfigInvalidError. Since this is a newly added exported symbol, fixing the name now avoids a breaking rename later.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/app/src/utils/server-errors.ts, line 23:
<comment>Typo in exported function name: `parseReabaleConfigInvalidError` should be `parseReadableConfigInvalidError`. Since this is a newly added exported symbol, fixing the name now avoids a breaking rename later.</comment>
<file context>
@@ -0,0 +1,32 @@
+ return o.name === "ConfigInvalidError" && typeof o.data === "object" && o.data !== null
+}
+
+export function parseReabaleConfigInvalidError(errorInput: ConfigInvalidError) {
+ const head = "Invalid configuration"
+ const file = errorInput.data.path && errorInput.data.path !== "config" ? errorInput.data.path : ""
</file context>
| const issues = (errorInput.data.issues ?? []).map((issue) => { | ||
| return `${issue.path.join(".")}: ${issue.message}` | ||
| }) | ||
| if (issues.length) return [head, file, "", ...issues].filter(Boolean).join("\n") |
There was a problem hiding this comment.
P2: Bug: The empty string "" intended as a blank-line separator is removed by .filter(Boolean) (empty string is falsy). The issues will appear directly after the header with no visual break. Consider filtering before inserting the separator.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/app/src/utils/server-errors.ts, line 30:
<comment>Bug: The empty string `""` intended as a blank-line separator is removed by `.filter(Boolean)` (empty string is falsy). The issues will appear directly after the header with no visual break. Consider filtering before inserting the separator.</comment>
<file context>
@@ -0,0 +1,32 @@
+ const issues = (errorInput.data.issues ?? []).map((issue) => {
+ return `${issue.path.join(".")}: ${issue.message}`
+ })
+ if (issues.length) return [head, file, "", ...issues].filter(Boolean).join("\n")
+ return [head, file, detail].filter(Boolean).join("\n")
+}
</file context>
| <div class="px-1.25 w-full flex items-center gap-x-3"> | ||
| <ProviderIcon data-slot="list-item-extra-icon" id={icon(i.id)} /> | ||
| <span>{i.name}</span> | ||
| <Show when={i.id === "opencode"}> |
There was a problem hiding this comment.
P3: Duplicate <Show when={i.id === "opencode"}> — this new block checks the same condition as the existing one a few lines below. Merge them into a single <Show> block to avoid redundant condition evaluation and improve readability.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/app/src/components/dialog-select-provider.tsx, line 74:
<comment>Duplicate `<Show when={i.id === "opencode"}>` — this new block checks the same condition as the existing one a few lines below. Merge them into a single `<Show>` block to avoid redundant condition evaluation and improve readability.</comment>
<file context>
@@ -70,13 +71,19 @@ export const DialogSelectProvider: Component = () => {
<div class="px-1.25 w-full flex items-center gap-x-3">
<ProviderIcon data-slot="list-item-extra-icon" id={icon(i.id)} />
<span>{i.name}</span>
+ <Show when={i.id === "opencode"}>
+ <div class="text-14-regular text-text-weak">{language.t("dialog.provider.opencode.tagline")}</div>
+ </Show>
</file context>
| issue_number: item.number, | ||
| name: 'needs:compliance', | ||
| }); | ||
| } catch (e) {} |
There was a problem hiding this comment.
P3: Empty catch (e) {} silently swallows all errors, not just the expected 404. Consider logging the error at debug level so unexpected failures (auth issues, rate limits, network errors) aren't invisible.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/compliance-close.yml, line 75:
<comment>Empty `catch (e) {}` silently swallows all errors, not just the expected 404. Consider logging the error at debug level so unexpected failures (auth issues, rate limits, network errors) aren't invisible.</comment>
<file context>
@@ -65,6 +65,15 @@ jobs:
+ issue_number: item.number,
+ name: 'needs:compliance',
+ });
+ } catch (e) {}
+
if (isPR) {
</file context>
Suggested change
| } catch (e) {} | |
| } catch (e) { | |
| core.info(`Failed to remove label from #${item.number}: ${e.message}`); | |
| } |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the performance and stability of the TUI by optimizing event handling during LLM streaming, which was previously causing high garbage collection pressure and UI freezes. It also addresses several critical bugs related to stream termination, error propagation in database effects, and unresponsive MCP client calls. Furthermore, it introduces a clearer separation of configuration concerns by migrating TUI-specific settings to a dedicated file and enhances the desktop app's ability to correctly load shell environments on Windows. Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| const pendingDiffs = new Map<string, import("@/snapshot").Snapshot.FileDiff[]>() | ||
| let batchTimer: Timer | undefined | ||
|
|
||
| function scheduleBatchFlush(setStore: any, store: any) { |
There was a problem hiding this comment.
1. schedulebatchflush uses any 📘 Rule violation ✓ Correctness
The new streaming batcher introduces multiple explicit any types (e.g., setStore: any, `store: any, and draft: any[]`), weakening type safety and enabling incorrect store mutations to compile. This violates the requirement to avoid any in new/modified TypeScript code.
Agent Prompt
## Issue description
New code in the streaming batcher uses explicit `any` types (including in `scheduleBatchFlush` and `produce()` drafts), violating the project rule to avoid `any` and reducing compile-time safety.
## Issue Context
The batcher can be typed without `any` by:
- moving `scheduleBatchFlush` inside `init()` so it can capture typed `store`/`setStore` via inference, and/or
- defining a local `type` for the store shape and using it for `createStore<...>()` and `SetStoreFunction<...>`.
Also remove unnecessary `(m: any)` / `(p: any)` annotations and let inference work.
## Fix Focus Areas
- packages/opencode/src/cli/cmd/tui/context/sync.tsx[44-155]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
24
to
28
| showAllFiles: () => void | ||
| tabForPath: (path: string) => string | ||
| openTab: (tab: string) => void | ||
| loadFile: (path: string) => void | ||
| loadFile: (path: string) => any | Promise<void> | ||
| }) => { |
There was a problem hiding this comment.
2. loadfile returns any 📘 Rule violation ✓ Correctness
createOpenReviewFile changes loadFile to return any | Promise<void>, introducing any and erasing the expected contract of the callback. This can mask incorrect return types and breaks the no-any rule.
Agent Prompt
## Issue description
`loadFile` was broadened to `any | Promise<void>`, which introduces `any` and removes type safety.
## Issue Context
The implementation only needs to know whether the result is async; the correct type is `void | Promise<void>` (or `unknown | Promise<void>` with narrowing), not `any`.
## Fix Focus Areas
- packages/app/src/pages/session/helpers.ts[23-36]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| ) | ||
| } | ||
|
|
||
| function Diagnostics(props: { diagnostics?: Record<string, Record<string, any>[]>; filePath: string }) { |
There was a problem hiding this comment.
3. diagnostics props use any 📘 Rule violation ✓ Correctness
The newly added Diagnostics component types diagnostics as `Record<string, Record<string, any>[]>, introducing any` into the TUI route. This prevents compile-time checking of diagnostic shape used in rendering (e.g., severity, range, message).
Agent Prompt
## Issue description
`Diagnostics` introduces `any` in its `diagnostics` prop type, which weakens type safety in new UI code.
## Issue Context
The component reads `severity`, `range.start.line`, `range.start.character`, and `message`. Define/import a type that includes these fields, or accept `unknown` and narrow before rendering.
## Fix Focus Areas
- packages/opencode/src/cli/cmd/tui/routes/session/index.tsx[2141-2157]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+9
to
+10
| parseModel: (url: string, body: any) => body.model, | ||
| parseIsStream: (url: string, body: any) => !!body.stream, |
There was a problem hiding this comment.
4. parsemodel uses body: any 📘 Rule violation ⛨ Security
The new POST handler uses body: any for request parsing, introducing any on external input handling. This removes type safety where validation/narrowing is expected for untrusted request bodies.
Agent Prompt
## Issue description
The new console endpoint parses request bodies using `any`, which violates the no-`any` rule and weakens safety for untrusted inputs.
## Issue Context
Request bodies are external inputs; prefer `unknown` and explicit narrowing (e.g., check for object shape and string/boolean fields) before reading `model`/`stream`.
## Fix Focus Areas
- packages/console/app/src/routes/zen/go/v1/messages.ts[4-11]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+43
to
+75
| async function clearPermissionDock(page: any, label: RegExp) { | ||
| const dock = page.locator(permissionDockSelector) | ||
| for (let i = 0; i < 3; i++) { | ||
| const count = await dock.count() | ||
| if (count === 0) return | ||
| await dock.getByRole("button", { name: label }).click() | ||
| await page.waitForTimeout(150) | ||
| } | ||
| } | ||
|
|
||
| async function withMockPermission<T>( | ||
| page: any, | ||
| request: { | ||
| id: string | ||
| sessionID: string | ||
| permission: string | ||
| patterns: string[] | ||
| metadata?: Record<string, unknown> | ||
| always?: string[] | ||
| }, | ||
| opts: { child?: any } | undefined, | ||
| fn: () => Promise<T>, | ||
| ) { | ||
| let pending = [ | ||
| { | ||
| ...request, | ||
| always: request.always ?? ["*"], | ||
| metadata: request.metadata ?? {}, | ||
| }, | ||
| ] | ||
|
|
||
| const list = async (route: any) => { | ||
| await route.fulfill({ |
There was a problem hiding this comment.
5. E2e mocks use any 📘 Rule violation ✓ Correctness
The updated e2e test introduces multiple any-typed Playwright objects (page: any, route: any,
and opts: { child?: any }), reducing test correctness and violating the no-any rule. This can
hide incorrect API usage and make refactors harder.
Agent Prompt
## Issue description
New e2e helpers type Playwright objects as `any`, violating the no-`any` rule and reducing test type safety.
## Issue Context
Playwright provides types like `Page` and `Route` that can be imported from `@playwright/test` (or the project’s fixture types). Replace `opts.child?: any` with a concrete type matching the mocked session object.
## Fix Focus Areas
- packages/app/e2e/session/session-composer-dock.spec.ts[43-109]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+501
to
+504
| const { agent, controller, sessionUpdates, stop } = createFakeAgent() | ||
| const cwd = "/tmp/opencode-acp-test" | ||
| const sessionId = await agent.newSession({ cwd, mcpServers: [] } as any).then((x) => x.sessionId) | ||
| const input = { command: "echo hello", description: "run command" } |
There was a problem hiding this comment.
6. Acp test casts as any 📘 Rule violation ✓ Correctness
The updated ACP event subscription tests use as any casts when calling agent.newSession(...), introducing any and bypassing type checks on session creation inputs. This violates the no-any rule and can let incorrect test setup compile.
Agent Prompt
## Issue description
The ACP event subscription test suite introduces `as any` casts for session creation, bypassing TypeScript safety.
## Issue Context
Prefer importing the expected input type for `agent.newSession` (or deriving it via `Parameters<typeof agent.newSession>[0]`) and building a valid object without casting.
## Fix Focus Areas
- packages/opencode/test/acp/event-subscription.test.ts[496-541]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is an excellent pull request that addresses critical performance and stability issues in the TUI while also making significant architectural improvements. The batching of streaming events, fixing the stream termination bug, and adding timeouts are all crucial fixes. The refactoring of configuration into separate server and TUI files, along with the robust migration path, is very well-executed. Similarly, replacing direct Bun.spawn calls with a more robust Process utility is a great move for maintainability and stability. The introduction of the "Go" plan is a major feature, and the implementation across the backend and frontend seems thorough. Overall, these are high-quality changes that significantly improve the product.
Comment on lines
+18
to
+21
| expect(path.normalize("C:\\repo\\src\\app.ts")).toBe("src\\app.ts") | ||
| expect(path.normalize("C:/repo/src/app.ts")).toBe("src/app.ts") | ||
| expect(path.normalize("file://C:/repo/src/app.ts")).toBe("src/app.ts") | ||
| expect(path.normalize("c:\\repo\\src\\app.ts")).toBe("src/app.ts") | ||
| expect(path.normalize("c:\\repo\\src\\app.ts")).toBe("src\\app.ts") |
There was a problem hiding this comment.
This test has become platform-dependent. It will pass on Windows but fail on POSIX systems because the expected path contains backslashes. To ensure the test is portable, consider normalizing the path separators to forward slashes before the assertion.
Suggested change
| expect(path.normalize("C:\\repo\\src\\app.ts")).toBe("src\\app.ts") | |
| expect(path.normalize("C:/repo/src/app.ts")).toBe("src/app.ts") | |
| expect(path.normalize("file://C:/repo/src/app.ts")).toBe("src/app.ts") | |
| expect(path.normalize("c:\\repo\\src\\app.ts")).toBe("src/app.ts") | |
| expect(path.normalize("c:\\repo\\src\\app.ts")).toBe("src\\app.ts") | |
| expect(path.normalize("C:\\repo\\src\\app.ts").replace(/\\/g, "/")).toBe("src/app.ts") | |
| expect(path.normalize("C:/repo/src/app.ts")).toBe("src/app.ts") | |
| expect(path.normalize("file://C:/repo/src/app.ts")).toBe("src/app.ts") | |
| expect(path.normalize("c:\\repo\\src\\app.ts").replace(/\\/g, "/")).toBe("src/app.ts") |
Owner
Author
|
Closing to rebase onto current dev. Will reopen with clean diff (sync.tsx changes moved to PR #3). |
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
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
delta,status,part,message,todo,diff) into a unified 100ms flush window insync.tsx— previously onlydeltaevents were debounced while 8 other event types triggered immediate store mutations and re-renderssdk.tsxto further reduce render frequency during streamingprocessor.tswherefor await (stream.fullStream)never exited onfinishevent (caused 0% CPU hangs)db.ts(prevented silent crash propagation)client.listTools()inmcp/index.ts(could hang indefinitely on unresponsive MCP servers)Root Cause
During LLM streaming, each token emits 4-7 events. Only
message.part.deltawas debounced — the other 8 event types (session.status,message.part.updated,message.updated,todo.updated,session.diff, etc.) all calledsetStore()immediately, triggering full markdown re-parse per render. This generated O(n) garbage per render where n = total accumulated text length, which triggered Bun's JSC garbage collector. The GC'svmDeallocatePhysicalPages()callsmadvise(MADV_DONTDUMP)which requires kernelmmap_write_lock— under memory pressure the kernel returnsEAGAIN, and WebKit's bmallocSYSCALLmacro retries in a zero-delay infinite loop, causing 100% CPU and TUI freeze.Files Changed
src/cli/cmd/tui/context/sync.tsxsrc/cli/cmd/tui/context/sdk.tsxsrc/session/processor.tsfor-awaitstream termination onfinisheventsrc/storage/db.tssrc/mcp/index.tswithTimeout()toclient.listTools()Testing
turbo typecheckpasses across all 18 packagesturbo buildsucceedsSummary by cubic
Fix TUI streaming freezes by batching high-frequency events and hardening stream handling. This reduces render churn and GC pressure, improving stability during long streams.
Bug Fixes
Refactors
Written for commit d9a81df. Summary will update on new commits.