Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
This was referenced Feb 3, 2026
Draft
Collaborator
Author
sid597
changed the title
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
sid597
force-pushed
the
eng-1345-modify-existing-roamjs-component-onchange-to-also-change
branch
from
82e3f74 to
c022156
Compare
Collaborator
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
apps/roam/src/components/settings/components/BlockPropSettingPanels.tsx
Outdated
Show resolved
Hide resolved
Contributor
📝 WalkthroughWalkthroughUpdates settings panels in ExportSettings to use Global variants with block synchronization support. Extends all Base*Panel types with RoamBlockSyncProps to enable Roam block syncing across settings panels. Reinstates schema watcher initialization in the onload flow at two points. Changes
Sequence DiagramsequenceDiagram
participant User as User (UI)
participant Panel as Global*Panel<br/>(Base Panel Component)
participant Roam as Roam Block<br/>(Storage)
User->>Panel: Change setting value
Panel->>Panel: Detect change event
alt hasBlockSync is true
Panel->>Panel: Prepare block sync operation
Panel->>Roam: Create/Update/Delete block<br/>(via syncToBlock or syncFlagToBlock)
Roam-->>Panel: Block operation complete
else hasBlockSync is false
Panel->>Panel: No block sync needed
end
Panel->>User: Update UI with new value
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/roam/src/components/settings/components/BlockPropSettingPanels.tsx (1)
96-107:⚠️ Potential issue | 🟡 MinorAdd explicit return types to all panel components.
All Base* and wrapper panels (BaseTextPanel,BaseFlagPanel,BaseNumberPanel,BaseSelectPanel,BaseMultiTextPanel, and their exported Global*/Personal* wrappers) lack explicit return type annotations. Add: JSX.Elementto align with the TypeScript guideline requiring explicit return types for functions.♻️ Example fix (apply to all panels)
-const BaseTextPanel = ({ +const BaseTextPanel = ({ title, description, settingKeys, getter, setter, defaultValue = "", placeholder, parentUid, uid, order, -}: BaseTextPanelProps) => { +}: BaseTextPanelProps): JSX.Element => {
🤖 Fix all issues with AI agents
In `@apps/roam/src/components/settings/components/BlockPropSettingPanels.tsx`:
- Around line 127-130: Remove the temporary debug setTimeout/console.log
statements in BlockPropSettingPanels.tsx (the blocks invoking setTimeout(() => {
console.log(`[TextPanel] "${title}" blockProp:`, getter(settingKeys)); }, 500))
and identical calls at the other locations (lines referencing title, getter, and
settingKeys); either delete these statements or wrap them behind a centralized
debug flag/ENV check (e.g., isDebugLogging) so logs only run when explicitly
enabled.
In `@apps/roam/src/index.ts`:
- Around line 163-166: The TODO comment saying "REMOVE stub call after testing"
is ambiguous and risks accidental removal of required startup logic; either
update the comment to state that initSchema(),
setupPullWatchOnSettingsPage(blockUids) and
setupPullWatchDiscourseNodes(nodePageUids) are required for block-sync or wrap
these calls behind a clear feature flag (e.g., ENABLE_BLOCK_SYNC) checked at
startup so they remain opt‑in during testing; implement the flag by reading an
env/config value and only invoking initSchema and the two setup* functions when
the flag is true, or alternatively change the TODO text to a permanent
explanatory note clarifying their necessity for block-sync.
🧹 Nitpick comments (1)
apps/roam/src/components/settings/components/BlockPropSettingPanels.tsx (1)
335-341: Prevent unnecessary re-evaluation ofgetShallowTreeByParentUidon each render.The initializer expression in
useRef(...)is evaluated every time the component renders, even though React only uses the result once. SincegetShallowTreeByParentUidqueries the Roam API, this causes unnecessary work on each render.Use lazy initialization with
useStateto compute the child UIDs once:♻️ Lazy initialization pattern
- const childUidsRef = useRef<string[]>( - initialBlockUid - ? getShallowTreeByParentUid(initialBlockUid).map( - (c: { uid: string }) => c.uid, - ) - : [], - ); + const [initialChildUids] = useState<string[]>(() => + initialBlockUid + ? getShallowTreeByParentUid(initialBlockUid).map( + (c: { uid: string }) => c.uid, + ) + : [], + ); + const childUidsRef = useRef<string[]>(initialChildUids);
apps/roam/src/components/settings/components/BlockPropSettingPanels.tsx
Outdated
Show resolved
Hide resolved
sid597
force-pushed
the
eng-1345-modify-existing-roamjs-component-onchange-to-also-change
branch
from
c022156 to
87c2e08
Compare
Collaborator
Author
|
To test, update the update your |
sid597
force-pushed
the
eng-1345-modify-existing-roamjs-component-onchange-to-also-change
branch
from
87c2e08 to
1e8fcd2
Compare
apps/roam/src/components/settings/components/BlockPropSettingPanels.tsx
Outdated
Show resolved
Hide resolved
sid597
force-pushed
the
eng-1345-modify-existing-roamjs-component-onchange-to-also-change
branch
from
1e8fcd2 to
cf56730
Compare
sid597
force-pushed
the
eng-1190-replace-existing-roam-js-components-with-blockprop-based
branch
from
9095d0b to
304f5f9
Compare
sid597
force-pushed
the
eng-1345-modify-existing-roamjs-component-onchange-to-also-change
branch
from
cf56730 to
0723252
Compare
mdroidian
approved these changes
Feb 5, 2026
Contributor
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
Generally looks good, a few minor suggestions.
We'll just have to be sure to test this extensively together before it goes affects production.
Make sure to change these before merging:
- use the new api:
data.block.create - use ~ instead of ..
- delete the empty line change
apps/roam/src/components/settings/components/BlockPropSettingPanels.tsx
Outdated
Show resolved
Hide resolved
apps/roam/src/components/settings/components/BlockPropSettingPanels.tsx
Outdated
Show resolved
Hide resolved
apps/roam/src/components/settings/components/BlockPropSettingPanels.tsx
Outdated
Show resolved
Hide resolved
sid597
changed the base branch from
eng-1190-replace-existing-roam-js-components-with-blockprop-based
to
graphite-base/745
sid597
commented
Feb 6, 2026
| const newValue = e.target.value; | ||
| setValue(newValue); | ||
| setter(settingKeys, newValue); | ||
| syncToBlock?.(newValue); |
Collaborator
Author
There was a problem hiding this comment.
to test:
- add the following code to
index.ts(just above the `
const { blockUids, nodePageUids } = await initSchema();
const cleanupSettingsWatchers = setupPullWatchOnSettingsPage(blockUids);
const cleanupNodeWatchers = setupPullWatchDiscourseNodes(nodePageUids);
- add the following timeout here
setTimeout(() => {
console.log(`[TextPanel] "${title}" blockProp:`, getter(settingKeys));
}, 500);
Do same thing to other base panels.
- then goto
ExportSettings.tsx
and update its panels with the new components
<GlobalFlagPanel
title="Append Referenced Node"
description="If a referenced node is defined in a node's format, it will be appended to the discourse context"
settingKeys={["Export", "Append Referenced Node"]}
defaultValue={false}
order={6}
uid={exportSettings.appendRefNodeContext.uid}
parentUid={parentUid}
/>
</div>
<div className="link-type-select-wrapper">
<GlobalSelectPanel
title="Link Type"
description="How to format links that appear in your export."
settingKeys={["Export", "Link Type"]}
options={["alias", "wikilinks", "roam url"]}
defaultValue="alias"
order={5}
uid={exportSettings.linkType.uid}
parentUid={parentUid}
/>
</div>
<GlobalNumberPanel
title="Max Filename Length"
description="Set the maximum name length for markdown file exports"
settingKeys={["Export", "Max Filename Length"]}
defaultValue={64}
order={0}
uid={exportSettings.maxFilenameLength.uid}
parentUid={parentUid}
/>
<GlobalMultiTextPanel
title="Frontmatter"
description="Specify all the lines that should go to the Frontmatter of the markdown file"
settingKeys={["Export", "Frontmatter"]}
defaultValue={[]}
order={2}
uid={exportSettings.frontmatter.uid}
parentUid={parentUid}
/>
0723252 to
d42b423
Compare
sid597
force-pushed
the
graphite-base/745
branch
from
304f5f9 to
7769e02
Compare
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
2 participants
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/fcc05f0564e84a88baf41f535272523a
Summary by CodeRabbit
New Features
Improvements