perf(tui): replace reconcile() with path-syntax setStore for streaming updates#2
Closed
coleleavitt wants to merge 104 commits intodevfrom
Closed
perf(tui): replace reconcile() with path-syntax setStore for streaming updates#2coleleavitt wants to merge 104 commits intodevfrom
coleleavitt wants to merge 104 commits intodevfrom
Conversation
…o#14079) Co-authored-by: Aiden Cline <63023139+rekram1-node@users.noreply.github.com>
Co-authored-by: David Hill <iamdavidhill@gmail.com>
…hanges (anomalyco#14773)" This reverts commit a0b3bbf.
Co-authored-by: adamelmore <2363879+adamdottv@users.noreply.github.com> Co-authored-by: David Hill <iamdavidhill@gmail.com>
…g updates Replace all reconcile() calls with direct assignment and path-syntax setStore() to eliminate O(n) deep-diff overhead during streaming. - Hot path: reconcile(info) → direct assignment for message/part/session updates - Hot path: produce() delta flush → setStore path-syntax (O(depth) vs O(n)) - Bootstrap: remove reconcile() from all one-shot API response stores - Remove unused reconcile import from solid-js/store Proven from Solid.js source: reconcile() recursively traverses the entire object tree (applyState in modifiers.ts), while path-syntax navigates directly to the leaf node via updatePath (store.ts).
|
|
Unable to trigger custom agent "Code Reviewer". You have run out of credits 😔 |
|
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. |
|
Hey! Your PR title Please update it to start with one of:
Where See CONTRIBUTING.md for details. |
|
Important Review skippedToo many files! This PR contains 275 files, which is 125 over the limit of 150. ⛔ Files ignored due to path filters (25)
📒 Files selected for processing (275)
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 QodoMulti-feature release: Go subscription tier, TUI config separation, file search, comment system, and cross-platform improvements
WalkthroughsDescription**Major Features:** • Introduced "Go" subscription tier (formerly "Lite") with comprehensive billing, validation, and UI support across all languages • Added file find/search functionality with keyboard shortcuts and dual rendering modes (CSS Highlights API + DOM overlay fallback) • Implemented prompt history with inline comment tracking and metadata persistence • Added code comment utilities with metadata serialization and file selection support • Introduced line selection and diff selection utilities for shadow DOM contexts **Configuration & Infrastructure:** • Separated TUI configuration from main config schema into dedicated tui.json files with migration support for legacy keys • Added ConfigPaths module for file discovery and JSONC parsing with environment variable and file substitution • Refactored schema generation to support multiple config types (Config.Info and TuiConfig.Info) • Enhanced git configuration with support for long paths and symlinks in snapshot management • Added Process utility module for cross-platform command execution with improved error handling **Tool & Agent Improvements:** • Implemented tool execution state tracking and bash output deduplication to prevent duplicate snapshots • Added synthetic pending state emission before first running tool update • Enhanced tool event handling with metadata output extraction **Testing & Quality:** • Added comprehensive TUI configuration test suite covering precedence and migration • Improved snapshot tests cross-platform compatibility with forward slash path normalization • Added subscription monthly usage analysis and date utility function tests • Refactored E2E permission testing with mock-based approach • Extended tool event streaming and deduplication test coverage • Added session request tree and latest root session tests **UI & Styling:** • Implemented line comment component styling with comprehensive CSS and installation utilities • Added file viewer runtime utilities for shadow DOM readiness and color scheme synchronization • Created commented lines marking utilities for diff and file views • Added media type detection and data URL generation utilities • Integrated line comment styles into Pierre diff component **Internationalization:** • Added Go subscription translations across 15+ languages (English, Spanish, French, German, Italian, Portuguese, Russian, Chinese, Japanese, Korean, Thai, Turkish, Polish, Danish, Norwegian, Arabic, Bosnian) • Updated permission-related translations to use "permissions" terminology • Added provider tagline translations for OpenCode and OpenCode Go **Process & Tooling:** • Replaced Bun.spawn() calls with new Process utility for consistent process handling across LSP server, ripgrep, and git operations • Simplified PTY subscriber tracking by removing complex socket tagging logic • Added automatic merge conflict resolution in beta script using OpenCode AI • Updated Stripe product naming from "OpenCode Lite" to "OpenCode Go" Diagramflowchart LR
A["Legacy Config<br/>opencode.json"] -->|"migrate"| B["TUI Config<br/>tui.json"]
C["ConfigPaths<br/>Discovery"] -->|"load"| D["TUI Config<br/>Loader"]
E["File Find<br/>Module"] -->|"render"| F["CSS Highlights<br/>+ DOM Overlay"]
G["Comment<br/>Metadata"] -->|"attach"| H["Prompt History<br/>+ Request Parts"]
I["Tool Events"] -->|"deduplicate"| J["Bash Output<br/>Snapshots"]
K["Process<br/>Utility"] -->|"replace"| L["Bun.spawn<br/>Calls"]
M["Go Subscription<br/>Tier"] -->|"support"| N["Billing + UI<br/>+ i18n"]
File Changes1. packages/opencode/test/config/tui.test.ts
|
Code Review by Qodo
1. cleanup() swallows command errors
|
Owner
Author
|
Closing — fork's dev branch was stale when PR was created, causing 104 commits in diff. Recreating with synced fork. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
15 issues found across 409 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=".github/workflows/compliance-close.yml">
<violation number="1" location=".github/workflows/compliance-close.yml:75">
P2: Empty `catch` block silently swallows all errors, including unexpected ones (network failures, auth errors, rate limits). Consider logging the error with `core.warning` to preserve observability, matching the workflow's existing logging pattern with `core.info`.</violation>
</file>
<file name="packages/app/src/pages/session/review-tab.tsx">
<violation number="1" location="packages/app/src/pages/session/review-tab.tsx:136">
P1: Event listeners are never actually removed: `addEventListener` uses `{ capture: true }` but `removeEventListener` omits it (defaults to `false`). The `capture` flag must match for removal to work (per MDN spec). These four listeners will leak on every cleanup.</violation>
</file>
<file name="packages/app/src/context/permission.tsx">
<violation number="1" location="packages/app/src/context/permission.tsx:67">
P3: Migration leaves stale `autoAcceptEdits` key in persisted data. The spread `...data` copies all old keys, and the `merge()` function in `persist.ts` preserves extra keys not in defaults. Consider destructuring out the old key to keep persisted data clean.</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: Duplicate `<Show when={i.id === "opencode"}>` condition — merge into a single `<Show>` block with a fragment, consistent with the `"opencode-go"` pattern added in this same diff.</violation>
</file>
<file name="packages/app/src/context/layout-scroll.ts">
<violation number="1" location="packages/app/src/context/layout-scroll.ts:36">
P2: Unnecessary `clone()` on the hot path: `clone(opts.getSnapshot(sessionKey))` is called before the early-return check for a non-empty `current`. Move the clone after the early-return to avoid allocating an object that gets immediately discarded on every scroll event.</violation>
</file>
<file name="packages/app/src/context/file/path.ts">
<violation number="1" location="packages/app/src/context/file/path.ts:108">
P2: `normalizeDir` only strips trailing forward slashes (`/\/+$/`), but `normalize` now preserves native backslashes on Windows. A Windows directory path with a trailing backslash (e.g. `C:\\repo\\subdir\\`) will normalize to `"subdir\\"`, and `normalizeDir` won't strip the trailing `\`. Consider updating the regex to strip both separators: `/[\/\\]+$/`.</violation>
</file>
<file name="packages/app/src/components/settings-providers.tsx">
<violation number="1" location="packages/app/src/components/settings-providers.tsx:190">
P2: Two consecutive `<Show when={item.id === "opencode"}>` blocks evaluate the same condition. Merge them into a single `<Show>` with a fragment, consistent with how the `opencode-go` case is handled just below.</violation>
</file>
<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 produces an unhandled promise rejection.</violation>
</file>
<file name=".github/actions/setup-bun/action.yml">
<violation number="1" location=".github/actions/setup-bun/action.yml:20">
P2: The `case` statement has no default (`*`) branch. If `RUNNER_OS` doesn't match any listed value, `$OS` is unset and the URL output will be malformed (e.g. `bun--x64-baseline.zip`), causing a hard-to-debug 404 in the download step. Add a fallback or guard to avoid silently producing a broken URL.</violation>
</file>
<file name="packages/app/src/context/prompt.tsx">
<violation number="1" location="packages/app/src/context/prompt.tsx:120">
P2: `isCommentItem` misses items that have a `commentID` but no `comment` text. This means `replaceComments` can leave stale comment items in the list when they lack a trimmed `comment` string, since it only filters items where `!!item.comment?.trim()` is true. The check should also account for `commentID`.</violation>
</file>
<file name="packages/console/app/src/i18n/fr.ts">
<violation number="1" location="packages/console/app/src/i18n/fr.ts:501">
P2: Missing English source translations: the `workspace.lite.*` keys are added here in `fr.ts` but have no corresponding entries in `en.ts`. This will cause the i18n fallback to show raw key strings (e.g., `workspace.lite.subscription.title`) for English users, or for any locale that falls back to English.</violation>
</file>
<file name="packages/app/src/pages/layout.tsx">
<violation number="1" location="packages/app/src/pages/layout.tsx:1097">
P2: Making `navigateToProject` async without updating callers risks unhandled promise rejections. None of the 6+ call sites `await` or `.catch()` the returned promise. Wrap the body in a top-level `try/catch` so the function never rejects when called fire-and-forget.</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` → `parseReadableConfigInvalidError`. Since this is a new public API, fixing the name now avoids a breaking rename later.</violation>
<violation number="2" location="packages/app/src/utils/server-errors.ts:30">
P2: The empty string `""` in the array is always removed by `filter(Boolean)`, so it has no effect. If a blank line separator between the file and issues was intended, this is a bug — you'd need to filter first, then insert the separator. If no separator was intended, remove the `""` to avoid confusion.</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:240">
P2: Review count badge lost all styling classes (typography, color, padding, rounded pill background). This appears to be an unintentional UI regression — the count will render as plain unstyled text instead of the previous pill-shaped badge.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Comment on lines
+136
to
+139
| scroll.removeEventListener("wheel", handleInteraction) | ||
| scroll.removeEventListener("pointerdown", handleInteraction) | ||
| scroll.removeEventListener("touchstart", handleInteraction) | ||
| scroll.removeEventListener("keydown", handleInteraction) |
There was a problem hiding this comment.
P1: Event listeners are never actually removed: addEventListener uses { capture: true } but removeEventListener omits it (defaults to false). The capture flag must match for removal to work (per MDN spec). These four listeners will leak on every cleanup.
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/review-tab.tsx, line 136:
<comment>Event listeners are never actually removed: `addEventListener` uses `{ capture: true }` but `removeEventListener` omits it (defaults to `false`). The `capture` flag must match for removal to work (per MDN spec). These four listeners will leak on every cleanup.</comment>
<file context>
@@ -85,48 +63,81 @@ export function SessionReviewTab(props: SessionReviewTabProps) {
- cancelAnimationFrame(frame)
+ if (restoreFrame !== undefined) cancelAnimationFrame(restoreFrame)
+ if (scroll) {
+ scroll.removeEventListener("wheel", handleInteraction)
+ scroll.removeEventListener("pointerdown", handleInteraction)
+ scroll.removeEventListener("touchstart", handleInteraction)
</file context>
Suggested change
| scroll.removeEventListener("wheel", handleInteraction) | |
| scroll.removeEventListener("pointerdown", handleInteraction) | |
| scroll.removeEventListener("touchstart", handleInteraction) | |
| scroll.removeEventListener("keydown", handleInteraction) | |
| scroll.removeEventListener("wheel", handleInteraction, { capture: true }) | |
| scroll.removeEventListener("pointerdown", handleInteraction, { capture: true }) | |
| scroll.removeEventListener("touchstart", handleInteraction, { capture: true }) | |
| scroll.removeEventListener("keydown", handleInteraction, { capture: true }) |
| issue_number: item.number, | ||
| name: 'needs:compliance', | ||
| }); | ||
| } catch (e) {} |
There was a problem hiding this comment.
P2: Empty catch block silently swallows all errors, including unexpected ones (network failures, auth errors, rate limits). Consider logging the error with core.warning to preserve observability, matching the workflow's existing logging pattern with core.info.
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` block silently swallows all errors, including unexpected ones (network failures, auth errors, rate limits). Consider logging the error with `core.warning` to preserve observability, matching the workflow's existing logging pattern with `core.info`.</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.warning(`Failed to remove needs:compliance label from #${item.number}: ${e.message}`); | |
| } |
| <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: Duplicate <Show when={i.id === "opencode"}> condition — merge into a single <Show> block with a fragment, consistent with the "opencode-go" pattern added in this same diff.
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>Duplicate `<Show when={i.id === "opencode"}>` condition — merge into a single `<Show>` block with a fragment, consistent with the `"opencode-go"` pattern added in this same diff.</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>
Comment on lines
+36
to
+45
| const next = clone(opts.getSnapshot(sessionKey)) | ||
| const current = cache[sessionKey] | ||
| if (!current) { | ||
| setCache(sessionKey, next) | ||
| return | ||
| } | ||
|
|
||
| if (Object.keys(current).length > 0) return | ||
| if (Object.keys(next).length === 0) return | ||
| setCache(sessionKey, next) |
There was a problem hiding this comment.
P2: Unnecessary clone() on the hot path: clone(opts.getSnapshot(sessionKey)) is called before the early-return check for a non-empty current. Move the clone after the early-return to avoid allocating an object that gets immediately discarded on every scroll event.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/app/src/context/layout-scroll.ts, line 36:
<comment>Unnecessary `clone()` on the hot path: `clone(opts.getSnapshot(sessionKey))` is called before the early-return check for a non-empty `current`. Move the clone after the early-return to avoid allocating an object that gets immediately discarded on every scroll event.</comment>
<file context>
@@ -33,8 +33,16 @@ export function createScrollPersistence(opts: Options) {
function seed(sessionKey: string) {
- if (cache[sessionKey]) return
- setCache(sessionKey, clone(opts.getSnapshot(sessionKey)))
+ const next = clone(opts.getSnapshot(sessionKey))
+ const current = cache[sessionKey]
+ if (!current) {
</file context>
Suggested change
| const next = clone(opts.getSnapshot(sessionKey)) | |
| const current = cache[sessionKey] | |
| if (!current) { | |
| setCache(sessionKey, next) | |
| return | |
| } | |
| if (Object.keys(current).length > 0) return | |
| if (Object.keys(next).length === 0) return | |
| setCache(sessionKey, next) | |
| const current = cache[sessionKey] | |
| if (current && Object.keys(current).length > 0) return | |
| const next = clone(opts.getSnapshot(sessionKey)) | |
| if (!current) { | |
| setCache(sessionKey, next) | |
| return | |
| } | |
| if (Object.keys(next).length === 0) return | |
| setCache(sessionKey, next) |
| const root = scope() | ||
|
|
||
| let path = unquoteGitPath(decodeFilePath(stripQueryAndHash(stripFileProtocol(input)))).replace(/\\/g, "/") | ||
| let path = unquoteGitPath(decodeFilePath(stripQueryAndHash(stripFileProtocol(input)))) |
There was a problem hiding this comment.
P2: normalizeDir only strips trailing forward slashes (/\/+$/), but normalize now preserves native backslashes on Windows. A Windows directory path with a trailing backslash (e.g. C:\\repo\\subdir\\) will normalize to "subdir\\", and normalizeDir won't strip the trailing \. Consider updating the regex to strip both separators: /[\/\\]+$/.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/app/src/context/file/path.ts, line 108:
<comment>`normalizeDir` only strips trailing forward slashes (`/\/+$/`), but `normalize` now preserves native backslashes on Windows. A Windows directory path with a trailing backslash (e.g. `C:\\repo\\subdir\\`) will normalize to `"subdir\\"`, and `normalizeDir` won't strip the trailing `\`. Consider updating the regex to strip both separators: `/[\/\\]+$/`.</comment>
<file context>
@@ -103,32 +103,30 @@ export function encodeFilePath(filepath: string): string {
+ const root = scope()
- let path = unquoteGitPath(decodeFilePath(stripQueryAndHash(stripFileProtocol(input)))).replace(/\\/g, "/")
+ let path = unquoteGitPath(decodeFilePath(stripQueryAndHash(stripFileProtocol(input))))
- // Remove initial root prefix, if it's a complete match or followed by /
</file context>
| } | ||
|
|
||
| function navigateToProject(directory: string | undefined) { | ||
| async function navigateToProject(directory: string | undefined) { |
There was a problem hiding this comment.
P2: Making navigateToProject async without updating callers risks unhandled promise rejections. None of the 6+ call sites await or .catch() the returned promise. Wrap the body in a top-level try/catch so the function never rejects when called fire-and-forget.
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>Making `navigateToProject` async without updating callers risks unhandled promise rejections. None of the 6+ call sites `await` or `.catch()` the returned promise. Wrap the body in a top-level `try/catch` so the function never rejects when called fire-and-forget.</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>
| 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: The empty string "" in the array is always removed by filter(Boolean), so it has no effect. If a blank line separator between the file and issues was intended, this is a bug — you'd need to filter first, then insert the separator. If no separator was intended, remove the "" to avoid confusion.
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>The empty string `""` in the array is always removed by `filter(Boolean)`, so it has no effect. If a blank line separator between the file and issues was intended, this is a bug — you'd need to filter first, then insert the separator. If no separator was intended, remove the `""` to avoid confusion.</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>
| 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 → parseReadableConfigInvalidError. Since this is a new public API, 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` → `parseReadableConfigInvalidError`. Since this is a new public API, 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>
| <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 (typography, color, padding, rounded pill background). This appears to be an unintentional UI regression — the count will render as plain unstyled text instead of the previous pill-shaped badge.
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 240:
<comment>Review count badge lost all styling classes (typography, color, padding, rounded pill background). This appears to be an unintentional UI regression — the count will render as plain unstyled text instead of the previous pill-shaped badge.</comment>
<file context>
@@ -202,133 +216,128 @@ 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>
|
|
||
| return { | ||
| ...data, | ||
| autoAccept: |
There was a problem hiding this comment.
P3: Migration leaves stale autoAcceptEdits key in persisted data. The spread ...data copies all old keys, and the merge() function in persist.ts preserves extra keys not in defaults. Consider destructuring out the old key to keep persisted data clean.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/app/src/context/permission.tsx, line 67:
<comment>Migration leaves stale `autoAcceptEdits` key in persisted data. The spread `...data` copies all old keys, and the `merge()` function in `persist.ts` preserves extra keys not in defaults. Consider destructuring out the old key to keep persisted data clean.</comment>
<file context>
@@ -61,9 +54,25 @@ export const { use: usePermission, provider: PermissionProvider } = createSimple
+
+ return {
+ ...data,
+ autoAccept:
+ typeof data.autoAcceptEdits === "object" && data.autoAcceptEdits && !Array.isArray(data.autoAcceptEdits)
+ ? data.autoAcceptEdits
</file context>
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 introduces significant performance enhancements for the TUI by optimizing state updates during LLM streaming. It also refactors TUI configuration into a dedicated file for better organization and improves file rendering and commenting features across the application. Underlying infrastructure for process execution and Windows compatibility has been made more robust, and a new subscription tier, 'OpenCode Go', has been integrated into the console. 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}
Comment on lines
+33
to
+50
| async function conflicts() { | ||
| const out = await $`git diff --name-only --diff-filter=U`.text().catch(() => "") | ||
| return out | ||
| .split("\n") | ||
| .map((x) => x.trim()) | ||
| .filter(Boolean) | ||
| } | ||
|
|
||
| async function cleanup() { | ||
| try { | ||
| await $`git merge --abort` | ||
| } catch {} | ||
| try { | ||
| await $`git checkout -- .` | ||
| } catch {} | ||
| try { | ||
| await $`git clean -fd` | ||
| } catch {} |
There was a problem hiding this comment.
1. cleanup() swallows command errors 📘 Rule violation ⛯ Reliability
conflicts() and cleanup() intentionally ignore failures via empty catch {} / `.catch(() =>
"")`, which can hide critical merge/cleanup failures and make incident debugging difficult. This
violates the requirement to handle failure points with meaningful context and avoid unnecessary
try/catch usage.
Agent Prompt
## Issue description
`script/beta.ts` currently swallows errors from git commands (empty `catch {}`) and suppresses errors when reading conflicts (`.catch(() => "")`). This can hide real failures and makes diagnosing merge/cleanup issues difficult.
## Issue Context
The compliance checklist requires robust error handling with actionable context, and discourages broad/unnecessary try/catch blocks.
## Fix Focus Areas
- script/beta.ts[33-50]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
307
to
313
| setStore( | ||
| "part", | ||
| event.properties.messageID, | ||
| produce((draft) => { | ||
| const part = draft[result.index] | ||
| const field = event.properties.field as keyof typeof part | ||
| const existing = part[field] as string | undefined | ||
| ;(part[field] as string) = (existing ?? "") + event.properties.delta | ||
| }), | ||
| result.index, | ||
| event.properties.field as any, | ||
| (prev: string) => (prev ?? "") + event.properties.delta, | ||
| ) |
There was a problem hiding this comment.
2. field cast to any 📘 Rule violation ✓ Correctness
The streaming update path uses event.properties.field as any when calling setStore, erasing type safety for store writes and risking runtime updates to invalid/non-string fields. This introduces new any usage in a hot-path update.
Agent Prompt
## Issue description
`event.properties.field` is cast to `any` when calling `setStore(...)`, which defeats type checking for store updates.
## Issue Context
This is on the streaming hot path (`message.part.delta`). The field appears to be intended as a key into a message-part object, and the update function assumes the field holds a string.
## Fix Focus Areas
- packages/opencode/src/cli/cmd/tui/context/sync.tsx[307-313]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+8
to
+10
| parseApiKey: (headers: Headers) => headers.get("x-api-key") ?? undefined, | ||
| parseModel: (url: string, body: any) => body.model, | ||
| parseIsStream: (url: string, body: any) => !!body.stream, |
There was a problem hiding this comment.
3. Post body typed any 📘 Rule violation ✓ Correctness
The new POST handler parses request body as any, which removes compile-time guarantees for external input handling and can mask missing validation/narrowing. This introduces any into request parsing code.
Agent Prompt
## Issue description
The request `body` is typed as `any` in `parseModel`/`parseIsStream`, which erases type safety for external inputs.
## Issue Context
This is a new API route handler; input bodies should be typed safely (e.g., `unknown` + narrowing) rather than `any`.
## Fix Focus Areas
- packages/console/app/src/routes/zen/go/v1/messages.ts[8-10]
ⓘ 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.
4. Diagnostics typed with any 📘 Rule violation ✓ Correctness
Diagnostics introduces any in its diagnostics prop type (`Record<string, Record<string, any>[]>`), undermining type safety for LSP diagnostic rendering and making misuse easy. This violates the no-any requirement in new code.
Agent Prompt
## Issue description
`Diagnostics` uses `any` in its prop typing, which removes type safety for diagnostic rendering.
## Issue Context
The component reads `severity`, `range.start.line`, `range.start.character`, and `message`, so the type can be specified structurally or by importing an existing diagnostic type.
## Fix Focus Areas
- packages/opencode/src/cli/cmd/tui/routes/session/index.tsx[2141-2147]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
24
to
+35
| showAllFiles: () => void | ||
| tabForPath: (path: string) => string | ||
| openTab: (tab: string) => void | ||
| loadFile: (path: string) => void | ||
| loadFile: (path: string) => any | Promise<void> | ||
| }) => { | ||
| return (path: string) => { | ||
| batch(() => { | ||
| input.showAllFiles() | ||
| input.openTab(input.tabForPath(path)) | ||
| input.loadFile(path) | ||
| const maybePromise = input.loadFile(path) | ||
| const openTab = () => input.openTab(input.tabForPath(path)) | ||
| if (maybePromise instanceof Promise) maybePromise.then(openTab) | ||
| else openTab() |
There was a problem hiding this comment.
5. loadfile returns any 📘 Rule violation ✓ Correctness
createOpenReviewFile changes loadFile to return any | Promise<void>, introducing any where void | Promise<void> (or a typed result) would suffice. This reduces type safety and violates the no-any rule.
Agent Prompt
## Issue description
`loadFile` is typed as returning `any | Promise<void>`, introducing `any` unnecessarily.
## Issue Context
The implementation only needs to distinguish between synchronous `void` and async `Promise<void>`.
## Fix Focus Areas
- packages/app/src/pages/session/helpers.ts[23-35]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+78
to
+118
| <button | ||
| type="button" | ||
| aria-label={props.buttonLabel} | ||
| data-slot="line-comment-button" | ||
| on:mousedown={(e) => e.stopPropagation()} | ||
| on:mouseup={(e) => e.stopPropagation()} | ||
| on:click={props.onClick as any} | ||
| on:mouseenter={props.onMouseEnter as any} | ||
| > | ||
| <Show | ||
| when={props.inline} | ||
| fallback={<Icon name={icon() === "plus" ? "plus-small" : "comment"} size="small" />} | ||
| > | ||
| <InlineGlyph icon={icon()} /> | ||
| </Show> | ||
| </button> | ||
| <Show when={props.open}> | ||
| <div | ||
| data-slot="line-comment-popover" | ||
| classList={{ | ||
| [props.popoverClass ?? ""]: !!props.popoverClass, | ||
| }} | ||
| on:mousedown={(e) => e.stopPropagation()} | ||
| on:focusout={props.onPopoverFocusOut as any} | ||
| > | ||
| {props.children} | ||
| </div> | ||
| </Show> | ||
| </> | ||
| } | ||
| > | ||
| <div | ||
| data-slot="line-comment-popover" | ||
| data-inline-body="" | ||
| classList={{ | ||
| [props.popoverClass ?? ""]: !!props.popoverClass, | ||
| }} | ||
| onFocusOut={props.onPopoverFocusOut} | ||
| on:mousedown={(e) => e.stopPropagation()} | ||
| on:click={props.onClick as any} | ||
| on:mouseenter={props.onMouseEnter as any} | ||
| on:focusout={props.onPopoverFocusOut as any} |
There was a problem hiding this comment.
7. Linecomment handlers cast any 📘 Rule violation ✓ Correctness
LineCommentAnchor casts event handlers to any for on:click, on:mouseenter, and on:focusout, hiding potential mismatches between handler signatures and actual events. This introduces any into UI event wiring.
Agent Prompt
## Issue description
`line-comment.tsx` uses `as any` casts for event handlers, removing type safety for UI event wiring.
## Issue Context
The component already defines handler props; they should be passed without `any` by ensuring the prop types match the events used (or by adapting via small wrapper functions).
## Fix Focus Areas
- packages/ui/src/components/line-comment.tsx[78-118]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+334
to
+336
| const setHighlights = (ranges: FileFindHit[], currentIndex: number) => { | ||
| const api = (globalThis as unknown as { CSS?: { highlights?: any }; Highlight?: any }).CSS?.highlights | ||
| const Highlight = (globalThis as unknown as { Highlight?: any }).Highlight |
There was a problem hiding this comment.
8. Highlights types use any 📘 Rule violation ✓ Correctness
setHighlights() uses any in global typing for CSS.highlights and Highlight, which disables type checking for a browser API integration. This introduces avoidable any into new logic.
Agent Prompt
## Issue description
The highlight API integration uses `any` in global type assertions, defeating type checking.
## Issue Context
Only a small surface is needed (`set`, `delete`, and `new Highlight(...)`); define minimal interfaces/types and keep runtime feature detection.
## Fix Focus Areas
- packages/ui/src/pierre/file-find.ts[334-336]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+496
to
+504
| test("streams running bash output snapshots and de-dupes identical snapshots", async () => { | ||
| await using tmp = await tmpdir() | ||
| await Instance.provide({ | ||
| directory: tmp.path, | ||
| fn: async () => { | ||
| 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.
9. Test casts newsession args 📘 Rule violation ✓ Correctness
The new ACP event-subscription tests cast agent.newSession(...) arguments to any, weakening type safety and potentially hiding incorrect test setup. This introduces new any usage in tests.
Agent Prompt
## Issue description
The test calls `agent.newSession(...)` with an argument cast to `any`, which bypasses type checking.
## Issue Context
Tests should still preserve type safety; prefer `satisfies` or importing the correct input type for `newSession`.
## Fix Focus Areas
- packages/opencode/test/acp/event-subscription.test.ts[496-504]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+150
to
168
| await Database.transaction(async (tx) => { | ||
| await tx | ||
| .update(BillingTable) | ||
| .set({ | ||
| customerID, | ||
| liteSubscriptionID: subscriptionID, | ||
| lite: {}, | ||
| paymentMethodID: paymentMethod.id, | ||
| paymentMethodLast4: paymentMethod.card?.last4 ?? null, | ||
| paymentMethodType: paymentMethod.type, | ||
| }) | ||
| .where(eq(BillingTable.workspaceID, workspaceID)) | ||
|
|
||
| await tx.insert(PaymentTable).values({ | ||
| workspaceID, | ||
| id: Identifier.create("payment"), | ||
| amount: centsToMicroCents(amountInCents), | ||
| paymentID, | ||
| invoiceID, | ||
| customerID, | ||
| enrichment: { | ||
| type: "subscription", | ||
| couponID, | ||
| }, | ||
| await tx.insert(LiteTable).values({ | ||
| workspaceID, | ||
| id: Identifier.create("lite"), | ||
| userID: userID, | ||
| }) | ||
| }) |
There was a problem hiding this comment.
10. Lite webhook unique constraint loop 🐞 Bug ⛯ Reliability
customer.subscription.created (lite) unconditionally inserts into LiteTable, which has a unique index on (workspaceID,userID). Any Stripe retry/duplicate delivery will hit a unique constraint error and return 500, causing repeated retries and leaving the workspace in a stuck/partial billing state.
Agent Prompt
### Issue description
`customer.subscription.created` for `type === "lite"` inserts into `LiteTable` unconditionally, but `LiteTable` enforces a unique index on `(workspaceID, userID)`. Stripe webhooks are at-least-once; duplicates/retries will cause a constraint error and repeated 500 responses.
### Issue Context
The handler should be idempotent: if the Lite row already exists for the workspace/user, the webhook should treat it as success.
### Fix Focus Areas
- packages/console/app/src/routes/stripe/webhook.ts[150-168]
- packages/console/core/src/schema/billing.sql.ts[62-76]
### Implementation notes
- Prefer a DB-level upsert (e.g., `onDuplicateKeyUpdate` / equivalent) for `LiteTable` keyed by `(workspaceID, userID)`.
- Alternatively, `select` then `insert` only if absent, but ensure it’s race-safe.
- Consider also persisting a Stripe event id (or subscription id) to dedupe across retries.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+121
to
134
| // get payment id from invoice | ||
| const invoice = await Billing.stripe().invoices.retrieve(invoiceID, { | ||
| expand: ["payments"], | ||
| }) | ||
| const paymentID = invoice.payments?.data[0].payment.payment_intent as string | ||
| if (!paymentID) throw new Error("Payment ID not found") | ||
|
|
||
| // set customer metadata | ||
| if (!billing?.customerID) { | ||
| await Billing.stripe().customers.update(customerID, { | ||
| metadata: { | ||
| workspaceID, | ||
| }, | ||
| }) | ||
| } | ||
| // get payment method for the payment intent | ||
| const paymentIntent = await Billing.stripe().paymentIntents.retrieve(paymentID, { | ||
| expand: ["payment_method"], | ||
| }) | ||
| const paymentMethod = paymentIntent.payment_method | ||
| if (!paymentMethod || typeof paymentMethod === "string") throw new Error("Payment method not expanded") | ||
|
|
There was a problem hiding this comment.
11. Lite webhook requires payment intent 🐞 Bug ✓ Correctness
The Lite customer.subscription.created handler throws if the latest invoice has no payment intent/payment method. The same webhook file explicitly acknowledges payment intents can be undefined when coupons are used, so Lite subscriptions with discounts/trials can fail with 500 and never provision Lite access.
Agent Prompt
### Issue description
The Lite `customer.subscription.created` handler assumes `latest_invoice` always has a payment intent and expanded payment method. For coupon/trial scenarios, Stripe may not create a payment intent, and the code will throw, returning 500.
### Issue Context
The same webhook already documents that `paymentID` can be undefined with coupons in another path; Lite should follow similar robustness.
### Fix Focus Areas
- packages/console/app/src/routes/stripe/webhook.ts[121-134]
- packages/console/app/src/routes/stripe/webhook.ts[218-226]
### Implementation notes
- If `paymentID` is missing, proceed with Lite provisioning and skip updating `paymentMethod*` fields (or fetch default payment method from `customer.invoice_settings.default_payment_method` / subscription defaults).
- Ensure the handler still returns 2xx when state is consistent, to avoid Stripe retry storms.
ⓘ 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 impressive pull request that delivers significant performance improvements and a wide range of refactorings and features. The core change of replacing reconcile() with path-syntax setStore is a crucial optimization for the TUI's responsiveness during LLM streaming. I appreciate the detailed analysis in the PR description that motivated this change.
The broader refactoring efforts are also excellent. Separating TUI-specific configuration into tui.json with an automatic migration script is a great move for maintainability and user experience. The introduction of a new Process utility to centralize command execution is a major architectural improvement, enhancing robustness and consistency across the codebase. Furthermore, the UI enhancements, such as the new comment management features, improved keyboard navigation, and fixes for scroll positioning, all contribute to a more polished and reliable user experience.
I have one suggestion regarding the shell environment loading on desktop, but overall, this is a very high-quality contribution.
|
|
||
| let mut cmd = Command::new(shell); | ||
| cmd.args(["-il", "-c", &line]); | ||
| cmd.args(["-l", "-c", &line]); |
There was a problem hiding this comment.
It seems the command to spawn the user's shell now uses -l (login shell) instead of the previously used -il (interactive login shell). While the new load_shell_env function correctly attempts to use -il first, this spawn_command function, which is on a critical path, does not.
Using only -l might not load the user's full environment (like aliases, functions, and PATH modifications from files like .bashrc or .zshrc), which could lead to commands not being found. An interactive shell (-i) is generally required to source these files. I recommend changing this back to -il to ensure the spawned process inherits the complete user environment, which is crucial for reliability.
Suggested change
| cmd.args(["-l", "-c", &line]); | |
| cmd.args(["-il", "-c", &line]); |
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
reconcile()calls insync.tsxwith direct assignment and path-syntaxsetStore()to eliminate O(n) deep-diffing on every streaming token eventproduce()delta flush on hot path with surgicalsetStore("part", msgID, idx, field, fn)— O(depth) ≈ O(1) instead of O(n)reconcileimport entirely — no remaining usesProblem
During LLM streaming (~100
message.part.deltaevents/sec), each token triggers a Solid.js store update viareconcile(). This function (proven from Solid.js sourcemodifiers.ts:129-140) callsapplyState()which recursively traverses the entire object tree — O(n) per update.Combined with Bun's bmalloc SYSCALL spin loop on
madvise(MADV_DONTNEED)EAGAIN, the excessive object allocations from reconcile trigger GC which triggers bmalloc's zero-delay retry loop, causing 100% CPU and TUI freeze.Root Cause Analysis
Solid.js Store API Complexity (from source analysis)
reconcile()setStore()path-syntaxproduce()Mutation Audit (46 total in sync.tsx)
Part of a 3-layer fix
Verification
bun turbo typecheck— all 3 packages pass)bun turbo build— all packages pass)Summary by cubic
Replaced Solid store reconcile() with path-syntax setStore and direct assignment on streaming paths to cut per-token work and prevent CPU spikes/freezes during LLM streaming. Also simplified review UI and improved comment/session handling.
Refactors
Bug Fixes
Written for commit 56e7057. Summary will update on new commits.