qchore: Guard Blueprint.canUpdate and fully reset create wizard state …#284
qchore: Guard Blueprint.canUpdate and fully reset create wizard state …#284wryonik wants to merge 1 commit into
Conversation
…between blueprints
📝 WalkthroughWalkthroughChanges refactor blueprint creation and update logic in the store to check blueprint instance existence directly, integrate a Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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 review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/create/[id]/store.ts (1)
199-239: Remove unreachable code at line 239.The
throw new Error('Unknown error saving blueprint')statement on line 239 is unreachable dead code. All execution paths now return before reaching it:
- Line 214 returns when no blueprint exists
- Lines 225/231 return when
canUpdateis available- Line 237 returns in the fallback path
🔎 Proposed fix
// Fallback: if canUpdate is not available (or not a function), always create a new version console.warn('Blueprint.canUpdate is not available, submitting new version draft instead'); await blueprint.submitNewVersionDraft(data); return blueprint.props.id!; - - throw new Error('Unknown error saving blueprint'); } catch (err) {
🧹 Nitpick comments (2)
src/app/create/[id]/store.ts (1)
219-219: Consider typingcanUpdateproperly when SDK types are updated.The
as anycast is a reasonable workaround ifcanUpdateisn't yet in the SDK's type definitions. The runtimetypeofcheck at line 222 provides adequate safety. Once the SDK types includecanUpdate, this cast can be removed.src/app/components/Navbar.tsx (1)
44-62: Consider awaiting the IndexedDB clear to avoid race with persist middleware.The fire-and-forget pattern works for error resilience, but there's a subtle race condition: calling
reset()triggers state changes that thepersistmiddleware may immediately serialize back to IndexedDB (under the same'create-blueprint'key), potentially racing with or overwriting thesetIdb('create-blueprint', null)call.Consider awaiting the IndexedDB clear before resetting, or making
handleCreateBlueprintasync:🔎 Proposed fix
- const handleCreateBlueprint = () => { - // Clear persisted create-blueprint store in IndexedDB - try { - // Fire and forget – if this fails we still reset in-memory state below - setIdb('create-blueprint', null).catch((err) => { - console.error('Failed to clear persisted create-blueprint store', err); - }); - } catch (err) { - console.error('Error while scheduling create-blueprint store clear', err); - } - - // Reset in-memory create blueprint store so we always start from a clean slate - try { - useCreateBlueprintStore.getState().reset(); - } catch (err) { - console.error('Failed to reset create blueprint store', err); - } - - router.push('/create'); + const handleCreateBlueprint = async () => { + // Clear persisted create-blueprint store in IndexedDB first + try { + await setIdb('create-blueprint', null); + } catch (err) { + console.error('Failed to clear persisted create-blueprint store', err); + } + + // Reset in-memory create blueprint store so we always start from a clean slate + try { + useCreateBlueprintStore.getState().reset(); + } catch (err) { + console.error('Failed to reset create blueprint store', err); + } + + router.push('/create'); };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/components/Navbar.tsxsrc/app/create/[id]/store.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/app/components/Navbar.tsx (1)
src/app/create/[id]/store.ts (1)
useCreateBlueprintStore(71-364)
🔇 Additional comments (2)
src/app/create/[id]/store.ts (1)
186-187: LGTM!Good improvement to derive
blueprintIdfrom the blueprint instance first, which ensures consistency when the blueprint has been created butstate.idmight be stale.src/app/components/Navbar.tsx (1)
10-11: LGTM!Good aliasing of
setassetIdbto avoid naming conflicts and improve clarity about the source of the function.
…between blueprints
Summary by CodeRabbit
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.