bug: resume create request race condition #2817
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Caution Review failedThe pull request is closed. WalkthroughReworks client startup hook to track per-subsystem readiness (canvas, conversations, sandbox), gate resume-create on combined readiness, and expand resumeCreate to complete creation with chat context and retries. Server project creation now inserts a default conversation in the same transaction. DB defaults barrel re-exports conversation defaults. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API as Project API
participant DB
rect rgba(200,235,255,0.25)
note right of API: Single DB transaction for project creation
Client->>API: createProject(request)
API->>DB: Insert project
API->>DB: Insert default frames/canvases
API->>DB: Insert default conversation (createDefaultConversation)
API->>DB: Insert creation request
DB-->>API: Commit
API-->>Client: Project created (with conversation)
end
sequenceDiagram
autonumber
participant Hook as useStartProject
participant State as ProjectReadyState
participant Resume as resumeCreate
Note over Hook,State: projectReadyState = {canvas, conversations, sandbox}
State-->>Hook: updates (canvasWithFrames / conversations / sandbox.session)
opt conversations apply
Hook->>Hook: apply conversations asynchronously
end
alt all subsystems ready && creationRequest exists
Hook->>Resume: resumeCreate(creationRequest)
Resume-->>Hook: mark COMPLETED / error, send toasts, reset processedRequestIdRef on failure
else not ready
Note over Hook: wait for readiness
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx (1)
52-56: Good guard; consider strengthening readiness to data presence + no errors.The isProjectReady gate fixes the race, but it currently flips true solely when loading flags are false. If any query resolves with an error or without required data, resumeCreate could still fire prematurely.
- Suggest: require canvas/conversations data presence and no query errors before allowing resumeCreate.
Diff for readiness computation (Lines 106-115):
- useEffect(() => { - const allQueriesResolved = - !isUserLoading && - !isCanvasLoading && - !isConversationsLoading && - !isCreationRequestLoading; - - setIsProjectReady(allQueriesResolved); - }, [isUserLoading, isCanvasLoading, isConversationsLoading, isCreationRequestLoading]); + useEffect(() => { + const allQueriesResolved = + !isUserLoading && + !isCanvasLoading && + !isConversationsLoading && + !isCreationRequestLoading; + const noErrors = !userError && !canvasError && !conversationsError && !creationRequestError; + const dataPresent = !!canvasWithFrames && Array.isArray(conversations) && conversations.length > 0; + setIsProjectReady(allQueriesResolved && noErrors && dataPresent); + }, [ + isUserLoading, isCanvasLoading, isConversationsLoading, isCreationRequestLoading, + userError, canvasError, conversationsError, creationRequestError, + canvasWithFrames, conversations + ]);
- Nit: to avoid any chance of cross-project bleed, reset processedRequestIdRef when projectId changes:
// outside selected range; add near other effects useEffect(() => { processedRequestIdRef.current = null; }, [editorEngine.projectId]);This aligns with the provided TSX guidelines around effect deps and race avoidance.
Would you like me to push a patch with the above readiness tweak?
apps/web/client/src/server/api/routers/project/project.ts (1)
295-299: Creating a default conversation inside the project creation transaction is a solid fix.
- Ensures the client can safely gate on conversations being loaded before resuming the create flow.
- Order (before creation request) is sensible.
Optional: if duplicate-creation retries are possible, consider ON CONFLICT DO NOTHING on a unique (projectId, some-default-flag) to guarantee idempotency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx(1 hunks)apps/web/client/src/server/api/routers/project/project.ts(2 hunks)packages/db/src/defaults/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.tsx: In React providers managing MobX stores, create the observable store with useState(() => new Store()) to ensure a stable instance
Keep a ref (e.g., storeRef.current) to the MobX store to avoid stale closures in effects
Use setTimeout(() => store.clear(), 0) for delayed cleanup of MobX stores to avoid race conditions
Separate project changes from branch updates by using proper effect dependency arrays
Do not use useMemo to hold MobX observable instances; React may drop memoized values causing data loss
Do not clean up MobX stores synchronously during navigation; perform delayed cleanup instead
Do not include the MobX store instance in effect dependency arrays when it causes infinite loops
Files:
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx
🧠 Learnings (1)
📚 Learning: 2025-09-07T23:36:29.678Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-07T23:36:29.678Z
Learning: Applies to **/*.tsx : Separate project changes from branch updates by using proper effect dependency arrays
Applied to files:
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx
🧬 Code graph analysis (1)
apps/web/client/src/server/api/routers/project/project.ts (1)
packages/db/src/defaults/conversation.ts (1)
createDefaultConversation(4-13)
🔇 Additional comments (2)
packages/db/src/defaults/index.ts (1)
3-3: Barrel export for conversation defaults looks good.This makes the new defaults available to server consumers; aligns with the new import in project router.
apps/web/client/src/server/api/routers/project/project.ts (1)
10-14: Imports updated to include conversations and createDefaultConversation.Matches the new barrel export; no issues.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| if (creationRequest && processedRequestIdRef.current !== creationRequest.id) { | ||
| const isProjectReady = Object.values(projectReadyState).every((value) => value); | ||
| if (creationRequest && processedRequestIdRef.current !== creationRequest.id && isProjectReady) { | ||
| console.error('Resume create', creationRequest, isProjectReady, processedRequestIdRef.current); |
There was a problem hiding this comment.
It appears there's a console.error statement that was likely added for debugging purposes but left in the production code. This will generate error messages in the console during normal project creation flows, which could be confusing for users and developers alike.
Consider either:
- Removing this line entirely if the debugging is no longer needed
- Changing to
console.debug()orconsole.log()if the information is still valuable - Implementing a proper logging system with appropriate log levels
Since this appears in a core project creation flow, keeping the console clean of false error messages would improve the developer experience.
| console.error('Resume create', creationRequest, isProjectReady, processedRequestIdRef.current); | |
| console.debug('Resume create', creationRequest, isProjectReady, processedRequestIdRef.current); |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| const applyConversations = async () => { | ||
| if (conversations) { | ||
| await editorEngine.chat.conversation.applyConversations(conversations); | ||
| updateProjectReadyState({ conversations: true }); | ||
| } | ||
| }; | ||
| applyConversations(); |
There was a problem hiding this comment.
The async applyConversations function lacks error handling, which could lead to inconsistent state. If editorEngine.chat.conversation.applyConversations() fails, the component would still mark conversations as ready through updateProjectReadyState({ conversations: true }).
Consider adding error handling:
const applyConversations = async () => {
if (conversations) {
try {
await editorEngine.chat.conversation.applyConversations(conversations);
updateProjectReadyState({ conversations: true });
} catch (error) {
console.error('Failed to apply conversations:', error);
// Either set an error state or retry logic here
}
}
};This ensures the ready state is only updated when conversations are successfully applied.
| const applyConversations = async () => { | |
| if (conversations) { | |
| await editorEngine.chat.conversation.applyConversations(conversations); | |
| updateProjectReadyState({ conversations: true }); | |
| } | |
| }; | |
| applyConversations(); | |
| const applyConversations = async () => { | |
| if (conversations) { | |
| try { | |
| await editorEngine.chat.conversation.applyConversations(conversations); | |
| updateProjectReadyState({ conversations: true }); | |
| } catch (error) { | |
| console.error('Failed to apply conversations:', error); | |
| // Either set an error state or retry logic here | |
| } | |
| } | |
| }; | |
| applyConversations(); |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
| updateProjectReadyState({ conversations: true }); | ||
| } | ||
| }; | ||
| applyConversations(); |
There was a problem hiding this comment.
Consider wrapping the async function 'applyConversations' in a try/catch block to handle potential errors during conversation application.
| applyConversations(); | |
| applyConversations().catch(console.error); |
| if (creationRequest && processedRequestIdRef.current !== creationRequest.id) { | ||
| const isProjectReady = Object.values(projectReadyState).every((value) => value); | ||
| if (creationRequest && processedRequestIdRef.current !== creationRequest.id && isProjectReady) { | ||
| console.error('Resume create', creationRequest, isProjectReady, processedRequestIdRef.current); |
There was a problem hiding this comment.
Remove or refactor the console.error used for logging 'Resume create' before production deployment to avoid verbose debugging output.
| console.error('Resume create', creationRequest, isProjectReady, processedRequestIdRef.current); |
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Important
Improves project creation reliability by ensuring readiness of components and adds default chat conversation for new projects.
canvas,conversations, andsandboxare ready inuse-start-project.tsx.project.ts.ProjectReadyStateinterface inuse-start-project.tsxto track readiness ofcanvas,conversations, andsandbox.updateProjectReadyState()function to update readiness state.use-start-project.tsx.conversationdefaults inindex.ts.This description was created by
for 5920c93. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Bug Fixes