-
Notifications
You must be signed in to change notification settings - Fork 4
ENG-1290: Port discourse node config panel #769
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: eng-1291-port-discourse-node-specificationv1
Are you sure you want to change the base?
ENG-1290: Port discourse node config panel #769
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughModifies the Discourse relation configuration panel to support asynchronous block updates and saves computed relation data to global settings. Concurrently refactors initialization logic to reconcile placeholder relation keys with actual block UIDs and updates the block property accessor signature. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/roam/src/components/settings/DiscourseRelationConfigPanel.tsx`:
- Around line 572-712: Wrap the async IIFE passed to setTimeout in a
try/catch/finally: inside try keep the existing sequence (setInputSetting,
updateBlock, createBlock, saveCyToElementRef, deleteBlock/createBlock loops,
setGlobalSetting, back()); in catch log/report the error (e.g., console.error or
show a user-facing toast) so failures from deleteBlock/createBlock/etc. are
surfaced; in finally ensure UI state is restored (reset any loading flags like
setLoading(false) if used) and still call back() or otherwise enable navigation
so the user is not stuck; locate the IIFE around symbols setInputSetting,
deleteBlock, createBlock, setGlobalSetting, and back to apply the change.
🧹 Nitpick comments (1)
apps/roam/src/components/settings/DiscourseRelationConfigPanel.tsx (1)
576-596: UnawaitedsetInputSetting/updateBlockcalls in an otherwiseawait-ed flow.These calls are fire-and-forget while the rest of the save flow (lines 667–676) carefully awaits block mutations. If any of these writes fail silently or race with the
createBlockat line 601 (which also targetsrootUid), the saved state could be inconsistent. Consider awaiting them for predictable ordering, especially since you're already in anasynccontext.♻️ Suggested change
- setInputSetting({ + await setInputSetting({ blockUid: rootUid, key: "source", value: source, }); - setInputSetting({ + await setInputSetting({ blockUid: rootUid, key: "destination", value: destination, index: 1, }); - setInputSetting({ + await setInputSetting({ blockUid: rootUid, key: "complement", value: complement, index: 2, }); - updateBlock({ + await updateBlock({ uid: rootUid, text: label, });
42e81c7 to
4e3f7d6
Compare

https://www.loom.com/share/09868acba4424e28b0369c96104dbd3d
Summary by CodeRabbit