Eng 1672: Settings not reading from block props#983
Open
sid597 wants to merge 8 commits intomigration-block-init-staging-branchfrom
Open
Eng 1672: Settings not reading from block props#983sid597 wants to merge 8 commits intomigration-block-init-staging-branchfrom
sid597 wants to merge 8 commits intomigration-block-init-staging-branchfrom
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…panels PR #784 dropped the defaultValue prop when porting BlocksPanel to DualWriteBlocksPanel, breaking block-props reads when legacy is empty. Restore the hydration path and add flag-gated dev nuke command.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Collaborator
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
📝 WalkthroughWalkthroughThis PR extends settings panel components to accept optional ChangesSettings Initialization & Default Values
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Collaborator
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/roam/src/components/settings/components/EphemeralBlocksPanel.tsx (1)
69-94:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPossible leaked pull-watch and post-unmount work due to async
ensureChildren.
ensureChildrenresolves asynchronously, but the effect cleanup runs synchronously. If the component unmounts (oruidchanges) before the promise resolves:
- The cleanup at lines 87-93 runs first;
pullWatchArgsRef.currentis stillnull, soremovePullWatchis skipped.- The
.thenat line 81 then proceeds toel.innerHTML = "", render into a now-detachedel, and callregisterPullWatch()— leaking a pull watch that nothing will ever remove.Use a cancelled flag (or AbortSignal) to guard the post-await work, and ensure cleanup tears down a watcher registered later.
🛡️ Suggested guard
useEffect(() => { const el = containerRef.current; if (!el || !uid) return; + let cancelled = false; const pattern = "[:block/string :block/order {:block/children ...}]"; const entityId = `[:block/uid "${uid}"]`; const callback = () => handleChange(); @@ void ensureChildren.then(() => { + if (cancelled) return; el.innerHTML = ""; void window.roamAlphaAPI.ui.components.renderBlock({ uid, el }); registerPullWatch(); }); return () => { + cancelled = true; window.clearTimeout(debounceRef.current); if (pullWatchArgsRef.current) { window.roamAlphaAPI.data.removePullWatch(...pullWatchArgsRef.current); pullWatchArgsRef.current = null; } }; }, [uid, handleChange]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/roam/src/components/settings/components/EphemeralBlocksPanel.tsx` around lines 69 - 94, The async ensureChildren promise can complete after unmount and cause a leaked pull watch; modify the effect in EphemeralBlocksPanel.tsx to use a local cancelled flag (or AbortSignal) captured by the then-handler of ensureChildren and check it before running el.innerHTML, window.roamAlphaAPI.ui.components.renderBlock, and registerPullWatch; also update the cleanup to set cancelled=true and, if pullWatchArgsRef.current was set by registerPullWatch after unmount, call window.roamAlphaAPI.data.removePullWatch(...pullWatchArgsRef.current) and null it (in addition to clearing debounceRef.current) so any watcher created after the async resolution is still torn down.apps/roam/src/utils/createDiscourseNode.ts (1)
181-212:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't fall back to raw block creation for SmartBlocks templates.
When
useSmartBlocksis true, Line 193-212 still ends up creating plain blocks if the extension is disabled or if there is no legacytemplateblock to use assrcUid. That writes literal<% ... %>tokens into the new page instead of a usable template.At minimum, this path should warn and stop rather than creating broken content.
Suggested fix
if (useSmartBlocks && !window.roamjs?.extension?.smartblocks) { renderToast({ content: "This template requires SmartBlocks. Enable SmartBlocks in Roam Depot to use this template.", id: "smartblocks-extension-disabled", intent: "warning", }); - await createBlocksFromTemplate(); + handleOpenInSidebar(pageUid); + return pageUid; + } else if (useSmartBlocks && !canUseLegacySmartBlock) { + renderToast({ + content: + "This template requires SmartBlocks, but no legacy template block is available to execute it yet.", + id: "smartblocks-template-source-missing", + intent: "warning", + }); + handleOpenInSidebar(pageUid); + return pageUid; } else if ( useSmartBlocks && canUseLegacySmartBlock && window.roamjs?.extension?.smartblocks ) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/roam/src/utils/createDiscourseNode.ts` around lines 181 - 212, The code currently detects SmartBlocks via hasSmartBlockSyntax/useSmartBlocks but still calls createBlocksFromTemplate when SmartBlocks are required but unavailable, which writes raw "<%...%>" tokens; change the conditional flow so that when useSmartBlocks is true and either window.roamjs?.extension?.smartblocks is falsy OR canUseLegacySmartBlock is false you do not call createBlocksFromTemplate — instead call renderToast with a clear warning (reuse the existing "smartblocks-extension-disabled" pattern or create a new id/message) and abort the template creation (return/throw) to stop further processing; keep the existing branch that calls triggerSmartblock when smartblocks exist and handleImageCreation when appropriate, and ensure createBlocksFromTemplate is only invoked in the final else when useSmartBlocks is false.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/roam/src/components/settings/components/EphemeralBlocksPanel.tsx`:
- Around line 70-79: The current ensureChildren logic races: it checks
getFirstChildUidByBlockUid(uid) synchronously then kicks off async createBlock
calls, allowing duplicate children on rapid remounts; fix by re-checking or
gating before creating blocks — inside the promise chain for ensureChildren (or
in a small helper used by it) call getFirstChildUidByBlockUid(uid) again and
abort creation if a child now exists, or maintain a Set of populated UIDs (e.g.,
populatedUidSet) and check/add uid atomically before invoking createBlock for dv
items; reference ensureChildren, getFirstChildUidByBlockUid, createBlock, dv and
uid when making the change.
In `@apps/roam/src/components/settings/DiscourseNodeAttributes.tsx`:
- Around line 76-85: The current attributes state creation (useState for
attributes) assigns uid: "" for accessor-only rows which causes multiple rows to
share the empty uid and triggers block API calls with empty uids; change the
Attribute shape to include a stable temporary localId (e.g., generated with a
UUID or incrementing counter) and initialize each accessor-only row with a
unique localId in the useState initializer that uses getBasicTreeByParentUid and
defaultValue, use localId as the React key and for local lookups/matching in the
component, and modify any logic that currently matches by uid (the
mapping/lookup code around where you key by label and match rows by uid) to
prefer uid when present but fall back to localId; additionally guard updateBlock
and deleteBlock calls to only invoke the block APIs when the row has a non-empty
real uid (and queue or skip ops for local-only rows until a real uid is
assigned).
- Around line 76-85: The state initializer in DiscourseNodeAttributes currently
ignores an explicit empty object because it checks
Object.keys(defaultValue).length>0; change the check to detect undefined/null
instead so an explicit {} from the settings store is treated as a real value: in
the useState initializer (the arrow function passed to useState) use a presence
check like defaultValue !== undefined && defaultValue !== null to decide whether
to map Object.entries(defaultValue) into Attribute objects (using
getBasicTreeByParentUid and treeByLabel as already written) so that an empty
record results in an empty attributes array rather than falling back to legacy
rehydration.
In `@apps/roam/src/index.ts`:
- Around line 153-170: The dev-only command added via
extensionAPI.ui.commandPalette.addCommand (the "DG: Dev - Nuke personal legacy
settings" entry) is never removed on unload; capture the command registration
return value or generate a unique id when calling
extensionAPI.ui.commandPalette.addCommand, store it (e.g., in a local variable
like devNukeCommand or in an array of disposables), and call the matching
extensionAPI.ui.commandPalette.removeCommand(devNukeCommand) (or iterate and
remove) from the existing unload block so the command is deregistered on
extension reload/unload; update the unload cleanup code to remove this specific
command alongside other teardown logic.
In `@apps/roam/src/utils/getLeftSidebarSettings.ts`:
- Around line 211-221: The current code treats a missing accessor snapshot the
same as an empty global section by using globalValues?.Children ?? [], which
drops legacy config.children for pre-migration users; change the source array
used to build children to use config.children when globalValues?.Children is
undefined (not just empty) — e.g. compute a targetPageUids array =
globalValues?.Children === undefined ? config.children.map(c => c.text) :
globalValues.Children and then keep the existing legacyChildByPageUid lookup and
mapping logic (symbols: legacyChildByPageUid, children, config.children,
globalValues?.Children).
- Around line 251-289: The current mapping uses (personalValues ?? []) so an
undefined personal snapshot gets treated as an explicit empty array and wipes
out legacy sections; change getLeftSidebarSettings to branch on personalValues
being undefined vs an actual array: if personalValues === undefined, iterate the
existing sections (using legacyByName / sections) and convert each legacy
section into the returned shape (preserving uid, text, settings, children,
childrenUid), otherwise (when personalValues is an array, including empty array)
map personalValues as currently done; reference legacyByName, sections,
personalValues, and the inner mapping logic (truncateResult, folded, Alias,
childrenUid) to produce identical output shape for the fallback.
---
Outside diff comments:
In `@apps/roam/src/components/settings/components/EphemeralBlocksPanel.tsx`:
- Around line 69-94: The async ensureChildren promise can complete after unmount
and cause a leaked pull watch; modify the effect in EphemeralBlocksPanel.tsx to
use a local cancelled flag (or AbortSignal) captured by the then-handler of
ensureChildren and check it before running el.innerHTML,
window.roamAlphaAPI.ui.components.renderBlock, and registerPullWatch; also
update the cleanup to set cancelled=true and, if pullWatchArgsRef.current was
set by registerPullWatch after unmount, call
window.roamAlphaAPI.data.removePullWatch(...pullWatchArgsRef.current) and null
it (in addition to clearing debounceRef.current) so any watcher created after
the async resolution is still torn down.
In `@apps/roam/src/utils/createDiscourseNode.ts`:
- Around line 181-212: The code currently detects SmartBlocks via
hasSmartBlockSyntax/useSmartBlocks but still calls createBlocksFromTemplate when
SmartBlocks are required but unavailable, which writes raw "<%...%>" tokens;
change the conditional flow so that when useSmartBlocks is true and either
window.roamjs?.extension?.smartblocks is falsy OR canUseLegacySmartBlock is
false you do not call createBlocksFromTemplate — instead call renderToast with a
clear warning (reuse the existing "smartblocks-extension-disabled" pattern or
create a new id/message) and abort the template creation (return/throw) to stop
further processing; keep the existing branch that calls triggerSmartblock when
smartblocks exist and handleImageCreation when appropriate, and ensure
createBlocksFromTemplate is only invoked in the final else when useSmartBlocks
is false.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bb7bdbf8-c4cb-4b79-a342-b9978c4e5add
📒 Files selected for processing (11)
apps/roam/src/components/LeftSidebarView.tsxapps/roam/src/components/settings/DiscourseNodeAttributes.tsxapps/roam/src/components/settings/DiscourseNodeSuggestiveRules.tsxapps/roam/src/components/settings/LeftSidebarGlobalSettings.tsxapps/roam/src/components/settings/NodeConfig.tsxapps/roam/src/components/settings/components/EphemeralBlocksPanel.tsxapps/roam/src/components/settings/utils/settingKeys.tsapps/roam/src/index.tsapps/roam/src/utils/createDiscourseNode.tsapps/roam/src/utils/getExportSettings.tsapps/roam/src/utils/getLeftSidebarSettings.ts
…ot is undefined" This reverts commit 3b3f197.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://www.loom.com/share/28fd2950a90b45df99625398f251722f
Summary by CodeRabbit
Release Notes
New Features
Improvements
Developer