ENG-942 Canvas migration bug #493
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
@coderabbitai full review
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughDeferred loading of an initial snapshot until after TLStore creation and centralized store error handling: added handleStoreError to log, update UI state, emit telemetry, and send contextual error emails; removed initialData from createTLStore and replaced duplicated error paths with shared handling and conditional snapshot inclusion in emails. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Hook as useRoamStore Hook
participant Store as TLStore
participant Handler as handleStoreError
participant Email as Email Service
Caller->>Hook: initialize store
rect rgb(210,230,255)
Note over Hook,Store: Create store (no initialData)
Hook->>Store: createTLStore()
alt success
Store-->>Hook: store instance
else failure
Store-->>Hook: creation error
Hook->>Handler: handleStoreError("Failed to create TLStore", error, ctx)
Handler->>Email: send contextual email
Handler-->>Hook: rethrow/error state
end
end
rect rgb(255,245,200)
Note over Hook,Store: Post-creation snapshot loading
alt initialSnapshot exists
Hook->>Store: loadSnapshot(_store, initialSnapshot)
alt success
Store-->>Hook: snapshot loaded
else migration error
Store-->>Hook: migration error
Hook->>Handler: handleStoreError("Failed to migrate snapshot", error, { snapshotSize })
Handler->>Email: send contextual email (attach snapshot if small)
Handler-->>Hook: rethrow/error state
end
else no snapshot
Hook-->>Hook: continue without loading
end
end
Hook->>Caller: return store or error state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Pre-merge checks✅ Passed checks (3 passed)
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 |
…or handler for store creation and snapshot migration failures. This improves error logging and email notifications for better debugging and user support.
|
@coderabbitai full review |
✅ Actions performedFull review 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: 0
🧹 Nitpick comments (1)
apps/roam/src/components/canvas/useRoamStore.ts (1)
158-158: Consider extracting the size threshold as a named constant.The
10000byte threshold for including snapshots in error emails is hardcoded. Extracting it as a named constant (e.g.,MAX_SNAPSHOT_SIZE_FOR_EMAIL) would improve maintainability.Example:
const MAX_SNAPSHOT_SIZE_FOR_EMAIL = 10000; // 10KBThen reference it on line 158:
- ...(snapshotSize < 10000 ? { initialSnapshot } : {}), + ...(snapshotSize < MAX_SNAPSHOT_SIZE_FOR_EMAIL ? { initialSnapshot } : {}),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/roam/src/components/canvas/useRoamStore.ts(1 hunks)
🔇 Additional comments (3)
apps/roam/src/components/canvas/useRoamStore.ts (3)
138-161: Excellent centralized error handling—previous review concerns fully addressed.The
handleStoreErrorhelper correctly consolidates logging, state updates (setError,setLoading), telemetry, and email notifications. This addresses the previous review comment's concerns:
- ✓ Error state is now properly set (line 146)
- ✓ No duplicate emails—callers return
nullinstead of re-throwing- ✓ Consistent error handling across both creation and migration failures
The conditional snapshot inclusion (line 158) based on size is a smart optimization to prevent massive emails while still capturing
snapshotSizefor all errors.
166-178: Correct fix for the migration bug.Removing
initialDatafromcreateTLStoreis the key change that allows tldraw to apply migrations properly. The error handling correctly routes throughhandleStoreError, ensuring state consistency.
180-190: Post-creation snapshot loading completes the migration fix.Loading the snapshot after store creation (rather than during instantiation) allows
loadSnapshotto apply the necessary migrations. The error handling is correct: it callshandleStoreErrorand returnsnullwithout re-throwing, preventing duplicate notifications.
createTlStore doesn't apply migrations
So users were getting errors related to missing
sizeandfontFamilyAt shape(type = Uht8c3nBh).props.size: Expected "s" or "m" or "l" or "xl", got undefinedI think this is fixed/changed in 2.4.0 with a snapshot prop tldraw/tldraw#4233
Summary by CodeRabbit