refactor(opencode): replace reconcile() with path-syntax setStore for streaming updates#3
refactor(opencode): replace reconcile() with path-syntax setStore for streaming updates#3coleleavitt wants to merge 4 commits intodevfrom
Conversation
…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).
|
|
Hey! Your PR title Please update it to start with one of:
Where See CONTRIBUTING.md for details. |
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 enhances the performance and responsiveness of the TUI, particularly during high-frequency LLM streaming events. By refactoring Solid.js store update mechanisms from inefficient O(n) deep-diffing Highlights
Changelog
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
|
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
Review Summary by Qodo
WalkthroughsDescription• Replace reconcile() calls with direct assignment in hot path • Convert produce() delta flush to path-syntax setStore() for O(1) updates • Remove unused reconcile import from solid-js/store • Eliminate O(n) deep-diffing overhead during streaming token events Diagramflowchart LR
A["reconcile() calls<br/>O(n) tree walk"] -->|"Replace with<br/>direct assignment"| B["Direct assignment<br/>O(1) hot path"]
C["produce() delta flush<br/>O(n) mutations"] -->|"Replace with<br/>path-syntax setStore"| D["setStore path-syntax<br/>O(depth) ≈ O(1)"]
B --> E["Reduced GC pressure<br/>during streaming"]
D --> E
File Changes1. packages/opencode/src/cli/cmd/tui/context/sync.tsx
|
Code Review by Qodo
1.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughPer-message-part delta coalescing was added with a microtask-flush mechanism; many store writes switched from Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
|
@kusari-inspector re-run |
|
🔄 Run triggered at 01:36:10 UTC. Starting fresh analysis... |
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
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/opencode/src/cli/cmd/tui/context/sync.tsx">
<violation number="1" location="packages/opencode/src/cli/cmd/tui/context/sync.tsx:404">
P1: **Bug: `setStore` merges objects — stale keys persist on re-bootstrap.** In Solid.js, `setStore(key, plainObject)` shallow-merges the new object into the existing store node rather than replacing it. This means keys present in the old store but absent in the new data will **not** be removed. Since `bootstrap()` is re-invoked on `server.instance.disposed`, dictionary stores (`mcp`, `session_status`, `mcp_resource`, `provider_auth`, `provider_default`, `config`) will accumulate stale entries from the previous server state.
`reconcile()` should be retained for these dictionary/object-typed stores where keys can be added or removed between fetches. It's safe to drop `reconcile()` for array-typed stores (arrays are replaced, not merged) and for indexed object updates where the schema is fixed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/opencode/src/cli/cmd/tui/context/sync.tsx`:
- Around line 310-312: Replace the unsafe cast "event.properties.field as any"
with a constrained "event.properties.field as keyof Part" and update the
streaming callback used in the message.part.delta handling so it guards types
before concatenation: inside the updater for the field (the (prev: string) =>
... callback) check that both the existing value and event.properties.delta are
strings (e.g., typeof prev === 'string' && typeof event.properties.delta ===
'string') and only then return prev + event.properties.delta; otherwise return a
safe fallback (prev ?? '' or convert/assign appropriately). Ensure you reference
the Part type, event.properties.field, and the updater callback so only string
fields are concatenated.
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| event.properties.field as any, | ||
| (prev: string) => (prev ?? "") + event.properties.delta, |
There was a problem hiding this comment.
1. event.properties.field cast to any 📘 Rule violation ⛨ Security
The updated setStore() call casts event.properties.field to any, reducing type safety and allowing arbitrary store-path writes. This can mask bugs and weakens validation of externally-sourced event data.
Agent Prompt
## Issue description
The PR introduces `event.properties.field as any` in a `setStore()` path segment, which violates the no-`any` requirement and allows arbitrary property/path updates from event data.
## Issue Context
This code runs on streaming updates (`message.part.delta`). The field name originates from `event.properties.field`, which should be treated as untrusted/variable input unless it is provably constrained by types/runtime checks.
## Fix Focus Areas
- packages/opencode/src/cli/cmd/tui/context/sync.tsx[310-312]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
/gemini review |
![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 pull request provides a significant performance improvement by replacing all uses of Solid.js's reconcile() with more performant setStore updates. The detailed analysis in the description clearly explains the rationale. The changes correctly apply path-syntax setStore for surgical updates on the hot path and direct assignment for bootstrap data, which should effectively reduce CPU and GC pressure during LLM streaming.
I have one minor suggestion regarding type safety where as any is used.
| ;(part[field] as string) = (existing ?? "") + event.properties.delta | ||
| }), | ||
| result.index, | ||
| event.properties.field as any, |
There was a problem hiding this comment.
This is a great performance optimization. However, the use of as any weakens type safety. While it can be difficult to correctly type dynamic paths with Solid's setStore, it's good practice to avoid any if possible.
If a more specific type isn't feasible, I suggest adding a comment to explain why as any is used. This will help future maintainers understand the code.
event.properties.field as any, // Using 'as any' as TypeScript struggles with dynamic paths in `setStore`.
…type safety - Restore reconcile() for 6 dictionary/object stores (provider_default, config, mcp, mcp_resource, session_status, provider_auth) to properly remove stale keys on re-bootstrap — setStore shallow-merges objects - Replace 'as any' with 'as keyof Part' and add runtime type guard for delta handler to satisfy type safety requirements - Keep path-syntax setStore for streaming hot path and array stores
|
coleleavitt
changed the title
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
…per-token setStore calls Accumulate message.part.delta events in a plain Record buffer (zero reactive overhead), then flush to the store via a single batched setStore() call per queueMicrotask. This collapses N deltas per part per SDK flush into 1 setStore() call, reducing proxy trap invocations from 5N to 5 per flush cycle. - Add pending delta accumulator (plain Record, no proxies) - Schedule flushDeltas() via queueMicrotask after sdk.tsx batch completes - Clear stale pending deltas on message.part.updated/removed - No store shape changes, no hook changes, no new timers
… segfault on Ctrl+C
1 participant
Issue for this PR
Closes # N/A — performance improvement discovered during profiling
Type of change
What does this PR do?
Replaces O(n)
reconcile()deep-diff calls with O(1) path-syntaxsetStore()on the streaming hot path insync.tsx, reducing GC pressure and CPU usage during LLM token streaming.Root cause: During LLM streaming (~10 tokens/sec), each
message.part.deltaevent triggeredproduce()which internally walks the store tree. This created excessive object allocations that triggered GC, which in turn hit a bmalloc contention bug causing 100% CPU and TUI freeze.What changed (1 file,
sync.tsx):produce()with path-syntaxsetStore("part", messageID, index, field, updater)— navigates directly to the leaf node in O(depth) ≈ O(1) instead of walking the whole treereconcile()for 6 object/dictionary stores (provider_default,config,mcp,mcp_resource,session_status,provider_auth) becausesetStoreshallow-merges objects — stale keys would persist across re-bootstrap withoutreconcile()reconcile()with direct assignment for array stores (provider,agent,session,command,lsp) since arrays are replaced entirely bysetStore, not mergedas anycast withas keyof Part+ runtime type guard on the delta updaterWhy this works: Proven from Solid.js source analysis —
reconcile()(modifiers.ts:129-140) callsapplyState()which recursively traverses the entire object tree on every call. Path-syntaxsetStore()(store.ts:269-313) navigates directly to the target leaf. For streaming updates that touch a single field, this is the correct API.How did you verify your code works?
bun turbo typecheck— 16/16 packages pass cleansetStorepath-syntax is O(depth) and that objects are shallow-merged (requiringreconcile()for dict stores)Screenshots / recordings
N/A — no UI changes, performance-only refactor
Checklist
Summary by CodeRabbit