feat: add runner syncing, simplify generation selection and stream handling #1886
Conversation
|
|
Finished running flow.
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughImplements a two‑phase generation runner (initiate then listen), replaces a single Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI
participant Runner as GenerateContentRunner
participant API as GenerationService
participant Stream as StreamProcessor
participant Store as GenerationStore
UI->>Runner: startGeneration()
activate Runner
Runner->>API: initiateGeneration()
Note right of Runner #f9f0c1: set didPerformingContentGeneration
API-->>Runner: generation (status: pending)
Note right of Runner #e6f7ff: await status === "running"
API-->>Runner: generation (status: running)
Runner->>Stream: processStream(generation)
Note right of Stream #e6f7ff: set didListeningContentGeneration
Stream-->>Runner: chunks/events
Runner->>Store: apply streamed updates
deactivate Runner
sequenceDiagram
autonumber
participant Hook as use-node-generations
participant SWR as useSWR
participant Store as GenerationStore
Hook->>SWR: fetch generations
SWR-->>Hook: { data, isLoading }
alt data available and not loading
Hook->>Store: addGenerationRunner(data)
end
Hook->>Hook: currentGeneration = filter(!cancelled & node/origin) -> sort(createdAt desc) -> pick first
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
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. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
🔍 QA Testing Assistant by Giselle📋 Manual QA ChecklistBased on the changes in this PR, here are the key areas to test manually:
✨ Prompt for AI AgentsUse the following prompts with Cursor or Claude Code to automate E2E testing: 📝 E2E Test Generation Prompt``` You are an expert QA engineer. Your task is to write a comprehensive E2E test suite using Playwright for a Pull Request that introduces significant changes to a content generation feature. The key changes involve:
Your goal is to create a new Playwright test file, 1. Context Summary
2. Test ScenariosCreate tests for the following scenarios within a Key Requirement: You must use Playwright's
3. Playwright Implementation Instructions
4. MCP Integration Guidelines (Optional)
5. CI-Ready Code Requirements
--> ``` --- |
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||||||
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
|
@codex review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/giselle/src/react/generations/generate-content-runner.tsx (1)
64-66: Consider resetting refs when generation.id changes.The refs
didPerformingContentGenerationanddidListeningContentGenerationpersist across renders. If the same component instance is reused for a new generation (differentgeneration.id), the refs won't reset, preventing the new generation from starting or listening.Add an effect to reset the refs when
generation.idchanges:const didPerformingContentGeneration = useRef(false); const didListeningContentGeneration = useRef(false); const reachedStreamEnd = useRef(false); + + useEffect(() => { + didPerformingContentGeneration.current = false; + didListeningContentGeneration.current = false; + reachedStreamEnd.current = false; + }, [generation.id]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/giselle/src/react/generations/generate-content-runner.tsx(2 hunks)packages/giselle/src/react/generations/hooks/use-node-generations.ts(3 hunks)packages/giselle/src/react/generations/store.ts(1 hunks)
🔇 Additional comments (8)
packages/giselle/src/react/generations/store.ts (1)
24-34: LGTM! Deduplication logic is correct and efficient.The Set-based deduplication prevents duplicate generations by ID when adding new generations to the store, making
addGenerationRunneridempotent. The approach is clean and efficient.packages/giselle/src/react/generations/hooks/use-node-generations.ts (5)
2-2: LGTM! useEffect import supports new sync effect.
39-39: LGTM! isLoading extraction guards the sync effect.
54-60: LGTM! Remote generation sync effect is correct.The effect properly waits for data to load and syncs it into the local store via
addGenerationRunner, preventing duplicates thanks to the store's deduplication logic.
62-80: LGTM! Simplified currentGeneration computation is clear and correct.The new filter-and-sort approach is more straightforward than the previous implementation. The filtering logic correctly handles both studio and non-studio origins, and the dependencies are accurate.
58-58: Remove debug console.log statement.This console.log exposes generation data in the browser console and should be removed before production.
Apply this diff:
- console.log(data); addGenerationRunner(data);packages/giselle/src/react/generations/generate-content-runner.tsx (2)
64-65: LGTM! Separate refs for two-phase lifecycle are clear.Using distinct refs for the start and listen phases prevents duplicate execution and makes the intent explicit.
163-177: LGTM! Start generation effect is correct.The effect correctly initiates content generation when the generation is queued, and the ref guard prevents duplicate starts.
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting |
toyamarinyon
force-pushed
the
feat-runner-gen-selection
branch
from
3f8d894 to
745fbbe
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the generation system by adding runner syncing to maintain consistency between local and remote state, simplifying the generation selection logic for better predictability, and separating stream handling into distinct lifecycle phases to improve reliability.
- Add runner syncing to synchronize remote generations with local store using deduplication
- Simplify generation selection by filtering directly from store instead of complex merging logic
- Separate generation lifecycle into distinct start and stream listening phases
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| store.ts | Adds deduplication logic to prevent duplicate generations when syncing |
| use-node-generations.ts | Implements runner syncing and simplifies generation selection logic |
| generate-content-runner.tsx | Separates generation start from stream handling using distinct lifecycle phases |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| return allGenerations[0]; | ||
| }, [generations, data, nodeId, origin]); | ||
| return filteredGenerations[0]; | ||
| }, [generations, nodeId, origin]); |
There was a problem hiding this comment.
The dependency array is missing data which is used indirectly through the syncing effect. Consider adding data to ensure the memo recalculates when remote data changes, or document why it's intentionally excluded.
| }, [generations, nodeId, origin]); | |
| }, [generations, nodeId, origin, data]); |
Copilot uses AI. Check for mistakes.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/giselle/src/react/generations/generate-content-runner.tsx (1)
179-189: Verify processStream idempotency and concurrent execution safety.The listen effect correctly waits for "running" status before calling
processStream. However, ensure that:
processStreamitself is safe if called multiple times (e.g., if React strict mode double-invokes effects).- The
reachedStreamEndref is also reset whengeneration.idchanges, otherwise a new generation won't process its stream.Run the following script to check if
reachedStreamEndis reset anywhere:#!/bin/bash # Description: Check if reachedStreamEnd ref is reset on generation.id change. rg -nP 'reachedStreamEnd\.current\s*=\s*false' packages/giselle/src/react/generations/generate-content-runner.tsx
🧹 Nitpick comments (1)
packages/giselle/src/react/generations/hooks/use-node-generations.ts (1)
61-79: Simplify filter logic to avoid redundant checks.The filter on lines 62-73 checks
generation.context.operationNode.id === nodeIdandgeneration.context.origin.type === origin.type, but thegenerationsarray (line 21) is already filtered bynodeId. This redundantnodeIdcheck adds unnecessary overhead.Apply this diff to remove the redundant
nodeIdcheck:const currentGeneration = useMemo<Generation>(() => { const filteredGenerations = generations .filter( (generation) => generation.status !== "cancelled" && - generation.context.operationNode.id === nodeId && generation.context.origin.type === origin.type && (origin.type === "studio" ? generation.context.origin.type === "studio" && generation.context.origin.workspaceId === origin.workspaceId : generation.context.origin.type !== "studio" && generation.context.origin.actId === origin.actId), ) .sort( (a, b) => new Date(b.createdAt).getTime() - new Date(a.createdAt).getTime(), ); return filteredGenerations[0]; - }, [generations, nodeId, origin]); + }, [generations, origin]);Note: The
nodeIddependency can also be removed from theuseMemodependencies since it's no longer used in the filter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/giselle/src/react/generations/generate-content-runner.tsx(2 hunks)packages/giselle/src/react/generations/hooks/use-node-generations.ts(3 hunks)packages/giselle/src/react/generations/store.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/giselle/src/react/generations/store.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development-guide.mdc)
**/*.{ts,tsx}: Use Biome for formatting with tab indentation and double quotes
Follow organized imports pattern (enabled in biome.json)
Use TypeScript for type safety; avoidanytypes
Use Next.js patterns for web applications
Use async/await for asynchronous code rather than promises
Error handling: use try/catch blocks and propagate errors appropriately
Use kebab-case for all filenames (e.g.,user-profile.ts)
Use camelCase for variables, functions, and methods (e.g.,userEmail)
Use prefixes likeis,has,can,shouldfor boolean variables and functions for clarity
Use verbs or verb phrases that clearly indicate purpose for function naming (e.g.,calculateTotalPrice(), notprocess())If breaking changes are introduced in new AI SDK versions, update code to accommodate those changes
**/*.{ts,tsx}: Use TypeScript and avoid any
Use async/await and proper error handling
Variables and functions should use camelCase
Boolean variables and functions should use is/has/can/should prefixes
Function names should clearly indicate purpose
Files:
packages/giselle/src/react/generations/generate-content-runner.tsxpackages/giselle/src/react/generations/hooks/use-node-generations.ts
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/development-guide.mdc)
**/*.tsx: Use functional components with React hooks
Use PascalCase for React components and classes (e.g.,UserProfile)
**/*.tsx: Components should use React hooks and Next.js patterns
Component identifiers should use PascalCase
Files:
packages/giselle/src/react/generations/generate-content-runner.tsx
**/*
📄 CodeRabbit inference engine (.cursor/rules/naming-guide.mdc)
All filenames should use kebab-case (lowercase with hyphens)
Files:
packages/giselle/src/react/generations/generate-content-runner.tsxpackages/giselle/src/react/generations/hooks/use-node-generations.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/naming-guide.mdc)
**/*.{js,jsx,ts,tsx}: React components and classes should use PascalCase
Variables, functions, and methods should use camelCase
Use verbs or verb phrases for function names; names should clearly indicate what the function does; avoid ambiguous names that could lead to misuse
Use nouns or noun phrases for variable names; names should describe what the variable represents; avoid single-letter variables except in very short scopes
Use prefixes like 'is', 'has', 'can', 'should' for both variables and functions returning boolean values; make the true/false meaning clear
Files:
packages/giselle/src/react/generations/generate-content-runner.tsxpackages/giselle/src/react/generations/hooks/use-node-generations.ts
🧬 Code graph analysis (1)
packages/giselle/src/react/generations/hooks/use-node-generations.ts (3)
packages/giselle/src/react/generations/contexts/generation-runner-system.tsx (1)
useGenerationRunnerSystem(79-87)packages/giselle/src/react/use-giselle-engine.ts (1)
useGiselleEngine(86-168)packages/giselle/src/react/feature-flags/context.ts (1)
useFeatureFlag(16-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (3)
packages/giselle/src/react/generations/generate-content-runner.tsx (1)
64-65: LGTM! Two-phase refs correctly separate start and listen.The separate refs clearly distinguish the start phase (queued → running) from the listen phase (running → stream processing).
packages/giselle/src/react/generations/hooks/use-node-generations.ts (2)
54-59: LGTM! Sync effect correctly registers remote generations.The effect properly waits for data to load and then syncs fetched generations into the store via
addGenerationRunner. The dependencies are correct.
61-79: No changes required—‘stage’ and ‘github-app’ origins both define a requiredactId, so filtering onactIdcorrectly covers all non-studio cases.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/giselle/src/react/generations/generate-content-runner.tsx (1)
171-185: Consider more granular dependencies for the start effect.The effect depends on the entire
generationobject, which may cause re-runs even when only non-status properties change. While thedidPerformingContentGenerationguard prevents duplicate starts, a more granular dependency would be clearer:- }, [generation, client, updateGeneration, onStart]); + }, [generation.id, generation.status, client, updateGeneration, onStart]);This makes the intent explicit: restart the effect only when ID or status changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/giselle/src/react/generations/generate-content-runner.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development-guide.mdc)
**/*.{ts,tsx}: Use Biome for formatting with tab indentation and double quotes
Follow organized imports pattern (enabled in biome.json)
Use TypeScript for type safety; avoidanytypes
Use Next.js patterns for web applications
Use async/await for asynchronous code rather than promises
Error handling: use try/catch blocks and propagate errors appropriately
Use kebab-case for all filenames (e.g.,user-profile.ts)
Use camelCase for variables, functions, and methods (e.g.,userEmail)
Use prefixes likeis,has,can,shouldfor boolean variables and functions for clarity
Use verbs or verb phrases that clearly indicate purpose for function naming (e.g.,calculateTotalPrice(), notprocess())If breaking changes are introduced in new AI SDK versions, update code to accommodate those changes
**/*.{ts,tsx}: Use TypeScript and avoid any
Use async/await and proper error handling
Variables and functions should use camelCase
Boolean variables and functions should use is/has/can/should prefixes
Function names should clearly indicate purpose
Files:
packages/giselle/src/react/generations/generate-content-runner.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/development-guide.mdc)
**/*.tsx: Use functional components with React hooks
Use PascalCase for React components and classes (e.g.,UserProfile)
**/*.tsx: Components should use React hooks and Next.js patterns
Component identifiers should use PascalCase
Files:
packages/giselle/src/react/generations/generate-content-runner.tsx
**/*
📄 CodeRabbit inference engine (.cursor/rules/naming-guide.mdc)
All filenames should use kebab-case (lowercase with hyphens)
Files:
packages/giselle/src/react/generations/generate-content-runner.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/naming-guide.mdc)
**/*.{js,jsx,ts,tsx}: React components and classes should use PascalCase
Variables, functions, and methods should use camelCase
Use verbs or verb phrases for function names; names should clearly indicate what the function does; avoid ambiguous names that could lead to misuse
Use nouns or noun phrases for variable names; names should describe what the variable represents; avoid single-letter variables except in very short scopes
Use prefixes like 'is', 'has', 'can', 'should' for both variables and functions returning boolean values; make the true/false meaning clear
Files:
packages/giselle/src/react/generations/generate-content-runner.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (2)
packages/giselle/src/react/generations/generate-content-runner.tsx (2)
70-76: Lifecycle reset correctly addresses past review concerns.The reset effect properly clears all lifecycle guards when
generation.idchanges, ensuring new generations can start and process streams. The exhaustive dependencies suppression is justified since resetting should only occur on ID changes, not on other generation property updates.
187-197: Parent subscription to generation updates is correct via useGenerationStore selectors.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/giselle/src/react/generations/generate-content-runner.tsx (1)
188-193: Add error handling for startContentGeneration.The promise from
startContentGenerationlacks error handling. If it rejects, this could result in an unhandled promise rejection and the generation would be stuck in "queued" status without user feedback.Apply this diff to handle errors:
- client - .startContentGeneration({ generation }) - .then(({ generation: runningGeneration }) => { - onStart?.(runningGeneration); - updateGeneration(runningGeneration); - }); + client + .startContentGeneration({ generation }) + .then(({ generation: runningGeneration }) => { + onStart?.(runningGeneration); + updateGeneration(runningGeneration); + }) + .catch((error) => { + console.error("Failed to start content generation:", error); + onError?.(error); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/giselle/src/react/generations/generate-content-runner.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development-guide.mdc)
**/*.{ts,tsx}: Use Biome for formatting with tab indentation and double quotes
Follow organized imports pattern (enabled in biome.json)
Use TypeScript for type safety; avoidanytypes
Use Next.js patterns for web applications
Use async/await for asynchronous code rather than promises
Error handling: use try/catch blocks and propagate errors appropriately
Use kebab-case for all filenames (e.g.,user-profile.ts)
Use camelCase for variables, functions, and methods (e.g.,userEmail)
Use prefixes likeis,has,can,shouldfor boolean variables and functions for clarity
Use verbs or verb phrases that clearly indicate purpose for function naming (e.g.,calculateTotalPrice(), notprocess())If breaking changes are introduced in new AI SDK versions, update code to accommodate those changes
**/*.{ts,tsx}: Use TypeScript and avoid any
Use async/await and proper error handling
Variables and functions should use camelCase
Boolean variables and functions should use is/has/can/should prefixes
Function names should clearly indicate purpose
Files:
packages/giselle/src/react/generations/generate-content-runner.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/development-guide.mdc)
**/*.tsx: Use functional components with React hooks
Use PascalCase for React components and classes (e.g.,UserProfile)
**/*.tsx: Components should use React hooks and Next.js patterns
Component identifiers should use PascalCase
Files:
packages/giselle/src/react/generations/generate-content-runner.tsx
**/*
📄 CodeRabbit inference engine (.cursor/rules/naming-guide.mdc)
All filenames should use kebab-case (lowercase with hyphens)
Files:
packages/giselle/src/react/generations/generate-content-runner.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/naming-guide.mdc)
**/*.{js,jsx,ts,tsx}: React components and classes should use PascalCase
Variables, functions, and methods should use camelCase
Use verbs or verb phrases for function names; names should clearly indicate what the function does; avoid ambiguous names that could lead to misuse
Use nouns or noun phrases for variable names; names should describe what the variable represents; avoid single-letter variables except in very short scopes
Use prefixes like 'is', 'has', 'can', 'should' for both variables and functions returning boolean values; make the true/false meaning clear
Files:
packages/giselle/src/react/generations/generate-content-runner.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: codeql / languages (javascript) / Perform CodeQL for javascript
- GitHub Check: Cursor Bugbot
🔇 Additional comments (4)
packages/giselle/src/react/generations/generate-content-runner.tsx (4)
64-65: LGTM! Clear separation of lifecycle phases.The two separate refs effectively guard the start and listen phases independently, making the lifecycle explicit and easier to reason about.
70-85: LGTM! Comprehensive cleanup on generation switch.The reset effect correctly clears all stateful refs and pending operations when the generation ID changes, preventing stale state from interfering with the new generation lifecycle.
180-194: LGTM! Start phase correctly initiates generation.The effect properly guards against duplicate execution and only initiates content generation for queued generations. The dependency on the entire
generationobject ensures the effect reacts to status changes while the ref guard prevents re-execution.
196-206: LGTM! Listen phase correctly starts stream processing.The effect properly waits for "running" status before initiating stream processing. Calling
processStream()without await is correct—it runs asynchronously without blocking the effect, and the guard ensures it's only initiated once per generation.
| cancelAnimationFrame(pendingUpdate.current); | ||
| pendingUpdate.current = null; | ||
| } | ||
| }, [generation.id]); |
There was a problem hiding this comment.
Bug: Stale State Causes Incorrect Reset Logic
The useEffect responsible for resetting generation state doesn't update prevGenerationId.current after a generation change. This causes the reset logic to either run on every subsequent render or incorrectly skip when a generation ID is reused. As a result, lifecycle flags and message queues can remain stale, which may prevent generations from running as expected.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/giselle/src/react/generations/generate-content-runner.tsx (2)
183-197: Consider refining dependency array for efficiency.The effect depends on the entire
generationobject, causing it to re-run on any property change. Since the guard prevents duplicate starts, this is safe but potentially inefficient.Consider this refinement to run only on relevant changes:
- }, [generation, client, updateGeneration, onStart]); + }, [generation.id, generation.status, client, updateGeneration, onStart]);This limits re-runs to when the ID or status changes, reducing unnecessary effect executions.
199-209: Refine dependencies to avoid relying on effect execution order.The effect depends on the entire
generationobject, which may re-run more often than necessary. More importantly, the reset effect (line 72) and this listen effect both depend ongeneration.idchanges, but React doesn't guarantee effect execution order. While the current code order likely works, it's fragile.Simplify the dependency array to rely only on
processStream, which already tracksgeneration.id:- }, [generation, processStream]); + }, [generation.status, processStream]);This makes the dependency more explicit (we only care about status for the guard check) and removes the implicit reliance on effect ordering for the reset logic to work correctly. The
processStreamcallback already depends ongeneration.id, so ID changes will still trigger this effect via the recreated callback.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/giselle/src/react/generations/generate-content-runner.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development-guide.mdc)
**/*.{ts,tsx}: Use Biome for formatting with tab indentation and double quotes
Follow organized imports pattern (enabled in biome.json)
Use TypeScript for type safety; avoidanytypes
Use Next.js patterns for web applications
Use async/await for asynchronous code rather than promises
Error handling: use try/catch blocks and propagate errors appropriately
Use kebab-case for all filenames (e.g.,user-profile.ts)
Use camelCase for variables, functions, and methods (e.g.,userEmail)
Use prefixes likeis,has,can,shouldfor boolean variables and functions for clarity
Use verbs or verb phrases that clearly indicate purpose for function naming (e.g.,calculateTotalPrice(), notprocess())If breaking changes are introduced in new AI SDK versions, update code to accommodate those changes
**/*.{ts,tsx}: Use TypeScript and avoid any
Use async/await and proper error handling
Variables and functions should use camelCase
Boolean variables and functions should use is/has/can/should prefixes
Function names should clearly indicate purpose
Files:
packages/giselle/src/react/generations/generate-content-runner.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/development-guide.mdc)
**/*.tsx: Use functional components with React hooks
Use PascalCase for React components and classes (e.g.,UserProfile)
**/*.tsx: Components should use React hooks and Next.js patterns
Component identifiers should use PascalCase
Files:
packages/giselle/src/react/generations/generate-content-runner.tsx
**/*
📄 CodeRabbit inference engine (.cursor/rules/naming-guide.mdc)
All filenames should use kebab-case (lowercase with hyphens)
Files:
packages/giselle/src/react/generations/generate-content-runner.tsx
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/naming-guide.mdc)
**/*.{js,jsx,ts,tsx}: React components and classes should use PascalCase
Variables, functions, and methods should use camelCase
Use verbs or verb phrases for function names; names should clearly indicate what the function does; avoid ambiguous names that could lead to misuse
Use nouns or noun phrases for variable names; names should describe what the variable represents; avoid single-letter variables except in very short scopes
Use prefixes like 'is', 'has', 'can', 'should' for both variables and functions returning boolean values; make the true/false meaning clear
Files:
packages/giselle/src/react/generations/generate-content-runner.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: codeql / languages (javascript) / Perform CodeQL for javascript
- GitHub Check: Cursor Bugbot
- GitHub Check: check
🔇 Additional comments (1)
packages/giselle/src/react/generations/generate-content-runner.tsx (1)
64-65: LGTM! Clear two-phase lifecycle guards.The separation of
didPerformingContentGenerationanddidListeningContentGenerationmakes the distinct lifecycle phases explicit and easier to reason about.
| const prevGenerationId = useRef(generation.id); | ||
|
|
||
| // Reset lifecycle refs when generation changes | ||
| useEffect(() => { | ||
| if (prevGenerationId.current === generation.id) { | ||
| return; | ||
| } | ||
| didPerformingContentGeneration.current = false; | ||
| didListeningContentGeneration.current = false; | ||
| reachedStreamEnd.current = false; | ||
|
|
||
| // Clear message queue to prevent stale messages from previous generation | ||
| messageUpdateQueue.current.clear(); | ||
|
|
||
| // Cancel pending animation frame to prevent applying stale updates | ||
| if (pendingUpdate.current !== null) { | ||
| cancelAnimationFrame(pendingUpdate.current); | ||
| pendingUpdate.current = null; | ||
| } | ||
| }, [generation.id]); |
There was a problem hiding this comment.
Update prevGenerationId after reset to complete the guard logic.
The effect correctly resets state when generation.id changes, but prevGenerationId.current is never updated to the new ID. This makes the early-return check ineffective and could cause redundant resets if React re-runs the effect.
Apply this diff to update the ref after the reset:
// Reset lifecycle refs when generation changes
useEffect(() => {
if (prevGenerationId.current === generation.id) {
return;
}
+ prevGenerationId.current = generation.id;
+
didPerformingContentGeneration.current = false;
didListeningContentGeneration.current = false;
reachedStreamEnd.current = false;
// Clear message queue to prevent stale messages from previous generation
messageUpdateQueue.current.clear();
// Cancel pending animation frame to prevent applying stale updates
if (pendingUpdate.current !== null) {
cancelAnimationFrame(pendingUpdate.current);
pendingUpdate.current = null;
}
}, [generation.id]);📝 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.
| const prevGenerationId = useRef(generation.id); | |
| // Reset lifecycle refs when generation changes | |
| useEffect(() => { | |
| if (prevGenerationId.current === generation.id) { | |
| return; | |
| } | |
| didPerformingContentGeneration.current = false; | |
| didListeningContentGeneration.current = false; | |
| reachedStreamEnd.current = false; | |
| // Clear message queue to prevent stale messages from previous generation | |
| messageUpdateQueue.current.clear(); | |
| // Cancel pending animation frame to prevent applying stale updates | |
| if (pendingUpdate.current !== null) { | |
| cancelAnimationFrame(pendingUpdate.current); | |
| pendingUpdate.current = null; | |
| } | |
| }, [generation.id]); | |
| const prevGenerationId = useRef(generation.id); | |
| // Reset lifecycle refs when generation changes | |
| useEffect(() => { | |
| if (prevGenerationId.current === generation.id) { | |
| return; | |
| } | |
| // Update the stored previous ID so the guard logic stays in sync | |
| prevGenerationId.current = generation.id; | |
| didPerformingContentGeneration.current = false; | |
| didListeningContentGeneration.current = false; | |
| reachedStreamEnd.current = false; | |
| // Clear message queue to prevent stale messages from previous generation | |
| messageUpdateQueue.current.clear(); | |
| // Cancel pending animation frame to prevent applying stale updates | |
| if (pendingUpdate.current !== null) { | |
| cancelAnimationFrame(pendingUpdate.current); | |
| pendingUpdate.current = null; | |
| } | |
| }, [generation.id]); |
🤖 Prompt for AI Agents
In packages/giselle/src/react/generations/generate-content-runner.tsx around
lines 69 to 88, the effect resets lifecycle refs when generation.id changes but
never updates prevGenerationId.current, so the early-return guard remains
ineffective; fix by setting prevGenerationId.current = generation.id after
performing the resets (and after cancelling any pending animation frame) so
future effect runs correctly detect the current generation and avoid redundant
resets.
User description
This is part 2 of 2 in a stack made with GitButler:
PR Type
Enhancement
Description
Add runner syncing to keep local state with remote
Simplify generation selection logic for better predictability
Separate stream handling into distinct lifecycle phases
Prevent duplicate generations in store with deduplication
Diagram Walkthrough
File Walkthrough
use-node-generations.ts
Sync generations and simplify selection logicpackages/giselle/src/react/generations/hooks/use-node-generations.ts
useEffectto sync remote generations with local storestore.ts
Add generation deduplication in storepackages/giselle/src/react/generations/store.ts
generate-content-runner.tsx
Separate generation start and stream handlingpackages/giselle/src/react/generations/generate-content-runner.tsx
phases
useEffecthooks for starting and listening to generationNote
Sync remote generations into the store with deduplication, simplify current-generation selection, and split generation start vs. stream listening with lifecycle-safe resets.
packages/giselle/src/react/generations/generate-content-runner.tsx)queued, stream only whenrunning.generation.idchanges (clear message queue, cancel pending animation frame, reset flags).packages/giselle/src/react/generations/hooks/use-node-generations.ts)addGenerationRunnerafter SWR load.currentGenerationby filtering/sorting store entries (remove merge/dedup logic with fetched data).packages/giselle/src/react/generations/store.ts)addGenerationRunnernow deduplicates by ID before appending togenerations.Written by Cursor Bugbot for commit 59152a7. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor