Update 1.1.0rc with latest from 1.0.0rc#833
Open
gsxdsm wants to merge 3 commits intoAutoMaker-Org:v1.1.0rcfrom
Open
Update 1.1.0rc with latest from 1.0.0rc#833gsxdsm wants to merge 3 commits intoAutoMaker-Org:v1.1.0rcfrom
gsxdsm wants to merge 3 commits intoAutoMaker-Org:v1.1.0rcfrom
Conversation
…er-Org#828) * Changes from fix/dev-server-hang * fix: Address PR AutoMaker-Org#828 review feedback - Reset RAF buffer on context changes (worktree switch, dev-server restart) to prevent stale output from flushing into new sessions - Fix high-frequency WebSocket filter to catch auto-mode:event wrapping (auto_mode_progress is wrapped in auto-mode:event) and add feature:progress - Reorder Vite aliases so explicit jsx-runtime entries aren't shadowed by the broad /^react(\/|$)/ regex (Vite uses first-match-wins) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: Batch dev server logs and fix React module resolution order * feat: Add fallback timer for flushing dev server logs in background tabs --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* Changes from fix/board-react-crash * fix: Prevent cascading re-renders and crashes from high-frequency WS events
* Changes from fix/event-hook-endpoint * fix: Allow empty eventHooks/ntfyEndpoints to reconcile from server Remove the `length > 0` guards in fast-hydrate reconciliation that prevented intentional empty-array clears from syncing across clients. Server-side wipe protection (`__allowEmpty*` escape hatches) already ensures empty arrays in the server are intentional. Addresses PR AutoMaker-Org#831 review feedback from CodeRabbit and Gemini. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis pull request optimizes rendering performance and stabilizes state management by replacing direct QueryClient cache mutations with centralized persistFeatureUpdate in board-view hooks, introducing feature transition guards to prevent race conditions with WebSocket events, batching high-frequency logs and activities, refining WebSocket event logging levels for high-frequency messages, adjusting dev-server output throttling, and improving Vite configuration for module resolution and cache isolation. Changes
Sequence DiagramsequenceDiagram
participant UI as User/Component
participant Store as AppStore
participant PersistFn as persistFeatureUpdate
participant QueryCache as React Query
participant Server as Backend API
rect rgba(100, 150, 200, 0.5)
Note over UI,Server: New Optimistic Update Flow
UI->>Store: User action (e.g., move feature)
UI->>PersistFn: Call persistFeatureUpdate(featureId, newStatus)
PersistFn->>Store: markFeatureTransitioning(featureId)
PersistFn->>QueryCache: Optimistic update (setQueryData)
UI->>UI: Re-render with new status
PersistFn->>Server: Send update request
Server-->>QueryCache: Server response/invalidation
QueryCache->>UI: Trigger refetch/invalidation
UI->>UI: Re-render with server data
PersistFn->>Store: unmarkFeatureTransitioning(featureId)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested labels
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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
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 focuses on enhancing the application's performance and stability, particularly in areas involving real-time updates and state management. Key changes include optimizing WebSocket event handling to reduce log spam and UI re-renders, implementing more efficient state updates in the UI to prevent cascading re-renders, and introducing mechanisms to manage feature transition states, thereby mitigating race conditions. Additionally, the build configuration has been refined to ensure a more robust and consistent React environment. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a significant set of performance optimizations and refactorings across the UI and server. The changes focus on reducing log spam, preventing unnecessary UI re-renders by improving state management with Zustand and React Query, and fixing race conditions related to optimistic updates. The refactoring to centralize feature state updates is a great improvement for maintainability. Overall, these are excellent changes that should noticeably improve the application's performance and stability, especially on mobile devices. I have one minor suggestion to improve code clarity.
Note: Security Review did not run due to the size of the PR.
| // invalidateQueries() has time to settle before WS-driven invalidations | ||
| // are allowed through again. Without this, a WS event arriving during | ||
| // the refetch window would trigger a conflicting invalidation. | ||
| setTimeout(() => unmarkFeatureTransitioning(feature.id), 500); |
Contributor
There was a problem hiding this comment.
The 500ms delay is a magic number. To improve readability and maintainability, consider extracting it into a named constant at the top of the file, for example: const UNMARK_TRANSITION_DELAY_MS = 500;. This makes the purpose of the delay clearer and simplifies future adjustments.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ui/src/hooks/use-query-invalidation.ts (1)
182-191:⚠️ Potential issue | 🟠 MajorSkipped invalidations are dropped with no catch-up refresh.
At Line 187, invalidations are skipped during transitions, but there’s no deferred replay once transitions end. If no subsequent qualifying event arrives, the feature list can stay stale.
Suggested fix (defer + drain once transition window ends)
- if (FEATURE_LIST_INVALIDATION_EVENTS.includes(event.type) && !isAnyFeatureTransitioning()) { - queryClient.invalidateQueries({ - queryKey: queryKeys.features.all(currentProjectPath), - }); - } + if (FEATURE_LIST_INVALIDATION_EVENTS.includes(event.type)) { + if (isAnyFeatureTransitioning()) { + pendingFeatureListInvalidationRef.current = true; + scheduleFeatureListDrain(); + } else { + pendingFeatureListInvalidationRef.current = false; + queryClient.invalidateQueries({ + queryKey: queryKeys.features.all(currentProjectPath), + }); + } + }// Add near other refs in the hook: const pendingFeatureListInvalidationRef = useRef(false); const featureListDrainTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null); function scheduleFeatureListDrain() { if (featureListDrainTimerRef.current) return; featureListDrainTimerRef.current = setTimeout(() => { featureListDrainTimerRef.current = null; if (pendingFeatureListInvalidationRef.current && !isAnyFeatureTransitioning()) { pendingFeatureListInvalidationRef.current = false; queryClient.invalidateQueries({ queryKey: queryKeys.features.all(currentProjectPath) }); } }, 650); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/hooks/use-query-invalidation.ts` around lines 182 - 191, FEATURE_LIST_INVALIDATION_EVENTS-based invalidations are currently dropped when isAnyFeatureTransitioning() is true, leaving the feature list stale if no later event arrives; add a deferred replay: introduce pendingFeatureListInvalidationRef and featureListDrainTimerRef in the hook, when an invalidation is skipped set pendingFeatureListInvalidationRef.current = true and call scheduleFeatureListDrain() which starts a single 650ms timer (no-op if already set); when the timer fires clear the timer ref, check !isAnyFeatureTransitioning() and pendingFeatureListInvalidationRef.current, then clear pending and call queryClient.invalidateQueries({ queryKey: queryKeys.features.all(currentProjectPath) }); ensure the timer is cleared on unmount to avoid leaks and place the refs/handler alongside the other hook refs.
🧹 Nitpick comments (4)
apps/server/src/index.ts (1)
601-611: Consider expanding the high-frequency event list.The list currently includes
dev-server:output,test-runner:output, andfeature:progress, but theEventTypedefinition inlibs/types/src/event.tsshows additional streaming/progress events that may also be high-frequency:
agent:stream— streaming agent outputideation:stream— streaming ideation outputworktree:init-output— worktree initialization outputtest-runner:progress— test progress updatesIf these events fire frequently, they'll still log at info level and may contribute to the same log spam issue this change addresses.
♻️ Suggested expansion
const isHighFrequency = - type === 'dev-server:output' || type === 'test-runner:output' || type === 'feature:progress'; + type === 'dev-server:output' || + type === 'test-runner:output' || + type === 'test-runner:progress' || + type === 'feature:progress' || + type === 'agent:stream' || + type === 'ideation:stream' || + type === 'worktree:init-output';Alternatively, consider using a
Setfor O(1) lookup and easier maintenance:♻️ Alternative: Use a Set constant
const HIGH_FREQUENCY_EVENTS = new Set([ 'dev-server:output', 'test-runner:output', 'test-runner:progress', 'feature:progress', 'agent:stream', 'ideation:stream', 'worktree:init-output', ]); // Then in the handler: const isHighFrequency = HIGH_FREQUENCY_EVENTS.has(type);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/src/index.ts` around lines 601 - 611, The current high-frequency check (isHighFrequency) only includes 'dev-server:output', 'test-runner:output', and 'feature:progress' and misses other streaming/progress events; update it to include 'agent:stream', 'ideation:stream', 'worktree:init-output', and 'test-runner:progress' and refactor to use a constant Set (e.g., HIGH_FREQUENCY_EVENTS) for O(1) lookups; replace the inline boolean expression with HIGH_FREQUENCY_EVENTS.has(type) and keep using logger.debug for those types and logger.info otherwise to avoid log spam.apps/ui/vite.config.mts (2)
270-270: Consider centralizing singleton dependency names to avoid config drift.
resolve.dedupeandoptimizeDeps.includenow share several package names. Extracting a shared constant will reduce future mismatch risk.♻️ Proposed refactor
+const SINGLETON_DEPS = [ + 'react', + 'react-dom', + 'zustand', + 'use-sync-external-store', + '@xyflow/react', +] as const; ... - dedupe: ['react', 'react-dom', 'zustand', 'use-sync-external-store', '@xyflow/react'], + dedupe: [...SINGLETON_DEPS], ... include: [ - 'react', - 'react-dom', + ...SINGLETON_DEPS, 'react/jsx-runtime', 'react/jsx-dev-runtime', - 'use-sync-external-store', 'use-sync-external-store/shim', 'use-sync-external-store/shim/with-selector', - 'zustand', - '@xyflow/react', ],Also applies to: 367-376
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/vite.config.mts` at line 270, The dedupe list in vite.config.mts is duplicated across resolve.dedupe and optimizeDeps.include causing potential drift; define a single shared constant (e.g., const SINGLETON_DEPS or SHARED_DEDUPE) containing ['react','react-dom','zustand','use-sync-external-store','@xyflow/react'] and reference that constant in both resolve.dedupe and optimizeDeps.include (also update the other occurrence around the 367-376 block) so both locations read from the same source of truth.
257-264: Consider usingcreateRequire+require.resolve()for jsx-runtime aliases.The jsx-runtime aliases are solving a real CJS/ESM interop problem (as documented at lines 310 and 363–366), and the suggested refactor using
createRequirefrom Node'smodulepackage is the idiomatic ESM approach. This would be more maintainable than hardcoded relative paths. The current approach works, butrequire.resolve()provides better abstraction if the install layout ever changes.♻️ Proposed refactor
+import { createRequire } from 'module'; ... const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const require = createRequire(import.meta.url); ... { find: 'react/jsx-runtime', - replacement: path.resolve(__dirname, '../../node_modules/react/jsx-runtime.js'), + replacement: require.resolve('react/jsx-runtime'), }, { find: 'react/jsx-dev-runtime', - replacement: path.resolve(__dirname, '../../node_modules/react/jsx-dev-runtime.js'), + replacement: require.resolve('react/jsx-dev-runtime'), },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/vite.config.mts` around lines 257 - 264, Replace the hardcoded path.resolve(__dirname, '../../node_modules/react/jsx-runtime.js') and react/jsx-dev-runtime alias entries with Node's createRequire + require.resolve to robustly resolve the package entrypoints; specifically, import createRequire from 'module' (or use a createRequire call near the top of vite.config.mts), create a require for __filename, and use require.resolve('react/jsx-runtime') and require.resolve('react/jsx-dev-runtime') in the replacement fields for the existing alias objects so the aliases reference resolved module paths instead of brittle relative paths.apps/ui/src/lib/feature-transition-state.ts (1)
17-18: Scope transition checks to the affected entity, not globally.At Line 17, a global boolean means one transitioning feature can suppress unrelated feature-list invalidations. Prefer a scoped check (feature/worktree/project key) so only the conflicting transition is gated.
Refactor direction
- export function isAnyFeatureTransitioning(): boolean { - return transitioningFeatures.size > 0; - } + // Keep global helper if needed, but add scoped checks for callers that can key events. + export function isFeatureTransitioning(featureKey: string): boolean { + return transitioningFeatures.has(featureKey); + } + + export function isAnyFeatureTransitioning(): boolean { + return transitioningFeatures.size > 0; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/src/lib/feature-transition-state.ts` around lines 17 - 18, The current global check in isAnyFeatureTransitioning() uses transitioningFeatures.size and blocks unrelated invalidations; change it to a scoped check that accepts a unique key (e.g., featureId, worktreeId or composite project key) and tests only entries for that key in the transitioningFeatures collection. Add a new function (or overload) like isFeatureTransitioning(scopeKey: string): boolean that inspects transitioningFeatures.has(scopeKey) or filters by scope in the map/set, update callers to pass the proper scope (feature/worktree/project) instead of relying on the global isAnyFeatureTransitioning(), and keep the original function only for backward-compatibility if needed by delegating to a scoped check that returns true only when any scope matches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/ui/src/components/views/board-view/hooks/use-board-actions.ts`:
- Around line 949-954: The success toasts in handleCompleteFeature and
handleUnarchiveFeature are shown immediately without confirming persistence;
change these handlers to await the result from persistFeatureUpdate (update
persistFeatureUpdate to return a success boolean or result if it doesn't
already), and only call toast.success (using truncateDescription for the
message) when the awaited result indicates success; if persistence fails, show a
toast.error or appropriate failure feedback instead and handle any rollback
logic. Ensure you update both handleCompleteFeature and handleUnarchiveFeature
to await persistFeatureUpdate and gate toast.success on the returned success
flag.
In
`@apps/ui/src/components/views/board-view/worktree-panel/hooks/use-dev-server-logs.ts`:
- Around line 236-241: The cleanup currently only runs on unmount which allows
buffered logs to flush when worktreePath changes; update the effect in
use-dev-server-logs so resetPendingOutput is also invoked when worktreePath
changes by including worktreePath in the effect dependencies (i.e., useEffect
cleanup should run on worktreePath change as well as unmount), ensuring
resetPendingOutput is called in the cleanup returned by that effect to clear
buffered logs when switching worktrees.
In `@apps/ui/src/hooks/use-auto-mode.ts`:
- Around line 221-253: The batching currently iterates and calls
addAutoModeActivity per item (causing many store updates) and the unmount
cleanup clears the timer without flushing pendingActivitiesRef; fix by adding a
single flush function (e.g., flushPendingActivities) that reads
pendingActivitiesRef, resets it and calls the store once with the whole batch
(either via a new addAutoModeActivities API or by extending addAutoModeActivity
to accept an array / single transaction), replace the for-loop in
batchedAddAutoModeActivity with a single call to that flush, and in the
useEffect cleanup call flushPendingActivities before clearing flushTimerRef so
no queued activities are dropped (refer to pendingActivitiesRef, flushTimerRef,
batchedAddAutoModeActivity, addAutoModeActivity, and the useEffect cleanup).
---
Outside diff comments:
In `@apps/ui/src/hooks/use-query-invalidation.ts`:
- Around line 182-191: FEATURE_LIST_INVALIDATION_EVENTS-based invalidations are
currently dropped when isAnyFeatureTransitioning() is true, leaving the feature
list stale if no later event arrives; add a deferred replay: introduce
pendingFeatureListInvalidationRef and featureListDrainTimerRef in the hook, when
an invalidation is skipped set pendingFeatureListInvalidationRef.current = true
and call scheduleFeatureListDrain() which starts a single 650ms timer (no-op if
already set); when the timer fires clear the timer ref, check
!isAnyFeatureTransitioning() and pendingFeatureListInvalidationRef.current, then
clear pending and call queryClient.invalidateQueries({ queryKey:
queryKeys.features.all(currentProjectPath) }); ensure the timer is cleared on
unmount to avoid leaks and place the refs/handler alongside the other hook refs.
---
Nitpick comments:
In `@apps/server/src/index.ts`:
- Around line 601-611: The current high-frequency check (isHighFrequency) only
includes 'dev-server:output', 'test-runner:output', and 'feature:progress' and
misses other streaming/progress events; update it to include 'agent:stream',
'ideation:stream', 'worktree:init-output', and 'test-runner:progress' and
refactor to use a constant Set (e.g., HIGH_FREQUENCY_EVENTS) for O(1) lookups;
replace the inline boolean expression with HIGH_FREQUENCY_EVENTS.has(type) and
keep using logger.debug for those types and logger.info otherwise to avoid log
spam.
In `@apps/ui/src/lib/feature-transition-state.ts`:
- Around line 17-18: The current global check in isAnyFeatureTransitioning()
uses transitioningFeatures.size and blocks unrelated invalidations; change it to
a scoped check that accepts a unique key (e.g., featureId, worktreeId or
composite project key) and tests only entries for that key in the
transitioningFeatures collection. Add a new function (or overload) like
isFeatureTransitioning(scopeKey: string): boolean that inspects
transitioningFeatures.has(scopeKey) or filters by scope in the map/set, update
callers to pass the proper scope (feature/worktree/project) instead of relying
on the global isAnyFeatureTransitioning(), and keep the original function only
for backward-compatibility if needed by delegating to a scoped check that
returns true only when any scope matches.
In `@apps/ui/vite.config.mts`:
- Line 270: The dedupe list in vite.config.mts is duplicated across
resolve.dedupe and optimizeDeps.include causing potential drift; define a single
shared constant (e.g., const SINGLETON_DEPS or SHARED_DEDUPE) containing
['react','react-dom','zustand','use-sync-external-store','@xyflow/react'] and
reference that constant in both resolve.dedupe and optimizeDeps.include (also
update the other occurrence around the 367-376 block) so both locations read
from the same source of truth.
- Around line 257-264: Replace the hardcoded path.resolve(__dirname,
'../../node_modules/react/jsx-runtime.js') and react/jsx-dev-runtime alias
entries with Node's createRequire + require.resolve to robustly resolve the
package entrypoints; specifically, import createRequire from 'module' (or use a
createRequire call near the top of vite.config.mts), create a require for
__filename, and use require.resolve('react/jsx-runtime') and
require.resolve('react/jsx-dev-runtime') in the replacement fields for the
existing alias objects so the aliases reference resolved module paths instead of
brittle relative paths.
ℹ️ Review info
Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e297001d-fbc7-4345-9bb3-0cdf903e1e26
📒 Files selected for processing (17)
apps/server/src/index.tsapps/server/src/services/dev-server-service.tsapps/server/tests/unit/services/dev-server-event-types.test.tsapps/ui/src/components/views/board-view/hooks/use-board-actions.tsapps/ui/src/components/views/board-view/hooks/use-board-column-features.tsapps/ui/src/components/views/board-view/hooks/use-board-drag-drop.tsapps/ui/src/components/views/board-view/hooks/use-board-features.tsapps/ui/src/components/views/board-view/kanban-board.tsxapps/ui/src/components/views/board-view/worktree-panel/hooks/use-dev-server-logs.tsapps/ui/src/components/views/board-view/worktree-panel/hooks/use-dev-servers.tsapps/ui/src/hooks/use-auto-mode.tsapps/ui/src/hooks/use-query-invalidation.tsapps/ui/src/lib/feature-transition-state.tsapps/ui/src/lib/http-api-client.tsapps/ui/src/routes/__root.tsxapps/ui/src/store/app-store.tsapps/ui/vite.config.mts
Comment on lines
+949
to
+954
| persistFeatureUpdate(feature.id, { status: 'completed' as const }); | ||
| toast.success('Feature completed', { | ||
| description: `Archived: ${truncateDescription(feature.description)}`, | ||
| }); | ||
| }, | ||
| [updateFeature, persistFeatureUpdate] | ||
| [persistFeatureUpdate] |
Contributor
There was a problem hiding this comment.
Success toasts can be incorrect when persistence fails.
handleCompleteFeature and handleUnarchiveFeature show success immediately after calling persistFeatureUpdate, but that persistence path can rollback on API failure. This can leave users seeing a success toast even when the status change did not stick. Gate success notifications on confirmed persistence outcome (e.g., make persistFeatureUpdate return a success flag/result and await it here).
Also applies to: 971-983
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/src/components/views/board-view/hooks/use-board-actions.ts` around
lines 949 - 954, The success toasts in handleCompleteFeature and
handleUnarchiveFeature are shown immediately without confirming persistence;
change these handlers to await the result from persistFeatureUpdate (update
persistFeatureUpdate to return a success boolean or result if it doesn't
already), and only call toast.success (using truncateDescription for the
message) when the awaited result indicates success; if persistence fails, show a
toast.error or appropriate failure feedback instead and handle any rollback
logic. Ensure you update both handleCompleteFeature and handleUnarchiveFeature
to await persistFeatureUpdate and gate toast.success on the returned success
flag.
Comment on lines
+236
to
+241
| // Clean up pending RAF on unmount to prevent state updates after unmount | ||
| useEffect(() => { | ||
| return () => { | ||
| resetPendingOutput(); | ||
| }; | ||
| }, [resetPendingOutput]); |
Contributor
There was a problem hiding this comment.
Reseting only on unmount can leak buffered logs across worktree switches.
Line 236 currently scopes cleanup to unmount, but buffered content can still flush after a worktreePath change and append output from the previous worktree into the new state.
💡 Suggested fix
- // Clean up pending RAF on unmount to prevent state updates after unmount
- useEffect(() => {
- return () => {
- resetPendingOutput();
- };
- }, [resetPendingOutput]);
+ // Reset buffered output whenever subscription scope changes, and on unmount
+ useEffect(() => {
+ resetPendingOutput();
+ return () => {
+ resetPendingOutput();
+ };
+ }, [worktreePath, autoSubscribe, resetPendingOutput]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Clean up pending RAF on unmount to prevent state updates after unmount | |
| useEffect(() => { | |
| return () => { | |
| resetPendingOutput(); | |
| }; | |
| }, [resetPendingOutput]); | |
| // Reset buffered output whenever subscription scope changes, and on unmount | |
| useEffect(() => { | |
| resetPendingOutput(); | |
| return () => { | |
| resetPendingOutput(); | |
| }; | |
| }, [worktreePath, autoSubscribe, resetPendingOutput]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/ui/src/components/views/board-view/worktree-panel/hooks/use-dev-server-logs.ts`
around lines 236 - 241, The cleanup currently only runs on unmount which allows
buffered logs to flush when worktreePath changes; update the effect in
use-dev-server-logs so resetPendingOutput is also invoked when worktreePath
changes by including worktreePath in the effect dependencies (i.e., useEffect
cleanup should run on worktreePath change as well as unmount), ensuring
resetPendingOutput is called in the cleanup returned by that effect to clear
buffered logs when switching worktrees.
Comment on lines
+221
to
+253
| // Batch addAutoModeActivity calls to reduce Zustand set() frequency. | ||
| // Without batching, each WS event (especially auto_mode_progress which fires | ||
| // rapidly during streaming) triggers a separate set() → all subscriber selectors | ||
| // re-evaluate → on mobile this overwhelms React's batching → crash. | ||
| // This batches activities in a ref and flushes them in a single set() call. | ||
| const pendingActivitiesRef = useRef<Parameters<typeof addAutoModeActivity>[0][]>([]); | ||
| const flushTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null); | ||
| const batchedAddAutoModeActivity = useCallback( | ||
| (activity: Parameters<typeof addAutoModeActivity>[0]) => { | ||
| pendingActivitiesRef.current.push(activity); | ||
| if (!flushTimerRef.current) { | ||
| flushTimerRef.current = setTimeout(() => { | ||
| const batch = pendingActivitiesRef.current; | ||
| pendingActivitiesRef.current = []; | ||
| flushTimerRef.current = null; | ||
| // Flush all pending activities in a single store update | ||
| for (const act of batch) { | ||
| addAutoModeActivity(act); | ||
| } | ||
| }, 100); | ||
| } | ||
| }, | ||
| [addAutoModeActivity] | ||
| ); | ||
|
|
||
| // Cleanup flush timer on unmount | ||
| useEffect(() => { | ||
| return () => { | ||
| if (flushTimerRef.current) { | ||
| clearTimeout(flushTimerRef.current); | ||
| } | ||
| }; | ||
| }, []); |
Contributor
There was a problem hiding this comment.
Activity batching currently still emits per-item store updates and can lose queued events.
The flush loop calls addAutoModeActivity once per activity, so high-frequency bursts still trigger many store updates. Also, unmount cleanup clears the timer without flushing pendingActivitiesRef, which can drop recent activities.
💡 Suggested fix (single flush path + unmount flush)
- const pendingActivitiesRef = useRef<Parameters<typeof addAutoModeActivity>[0][]>([]);
+ const pendingActivitiesRef = useRef<Parameters<typeof addAutoModeActivity>[0][]>([]);
const flushTimerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
+ const flushPendingActivities = useCallback(() => {
+ const batch = pendingActivitiesRef.current;
+ if (batch.length === 0) return;
+ pendingActivitiesRef.current = [];
+
+ // One store update for the whole batch (preserves current prepend behavior).
+ useAppStore.setState((state) => {
+ const entries = batch
+ .map((activity) => ({
+ ...activity,
+ id: Math.random().toString(36).slice(2),
+ timestamp: new Date(),
+ }))
+ .reverse();
+ return {
+ autoModeActivityLog: [...entries, ...state.autoModeActivityLog].slice(0, 100),
+ };
+ });
+ }, []);
const batchedAddAutoModeActivity = useCallback(
(activity: Parameters<typeof addAutoModeActivity>[0]) => {
pendingActivitiesRef.current.push(activity);
if (!flushTimerRef.current) {
flushTimerRef.current = setTimeout(() => {
- const batch = pendingActivitiesRef.current;
- pendingActivitiesRef.current = [];
flushTimerRef.current = null;
- // Flush all pending activities in a single store update
- for (const act of batch) {
- addAutoModeActivity(act);
- }
+ flushPendingActivities();
}, 100);
}
},
- [addAutoModeActivity]
+ [flushPendingActivities]
);
useEffect(() => {
return () => {
if (flushTimerRef.current) {
clearTimeout(flushTimerRef.current);
+ flushTimerRef.current = null;
}
+ flushPendingActivities();
};
- }, []);
+ }, [flushPendingActivities]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ui/src/hooks/use-auto-mode.ts` around lines 221 - 253, The batching
currently iterates and calls addAutoModeActivity per item (causing many store
updates) and the unmount cleanup clears the timer without flushing
pendingActivitiesRef; fix by adding a single flush function (e.g.,
flushPendingActivities) that reads pendingActivitiesRef, resets it and calls the
store once with the whole batch (either via a new addAutoModeActivities API or
by extending addAutoModeActivity to accept an array / single transaction),
replace the for-loop in batchedAddAutoModeActivity with a single call to that
flush, and in the useEffect cleanup call flushPendingActivities before clearing
flushTimerRef so no queued activities are dropped (refer to
pendingActivitiesRef, flushTimerRef, batchedAddAutoModeActivity,
addAutoModeActivity, and the useEffect cleanup).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
Performance Improvements
Bug Fixes
Chores