Roam: ENG-709: Settings panel for suggestive mode #327
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThis update introduces new UI components and configuration structures for managing "suggestive mode" and "suggestive rules" within a Roam Research environment. It adds new React components for configuring suggestive rules and page groups, extends configuration structures and utilities, and integrates these features into existing settings and node configuration panels. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SettingsDialog
participant SuggestiveModeSettings
participant PageGroupsPanel
participant ConfigTreeUtils
User->>SettingsDialog: Select "Suggestive Mode" tab
SettingsDialog->>SuggestiveModeSettings: Render component
SuggestiveModeSettings->>ConfigTreeUtils: Fetch suggestive mode config
SuggestiveModeSettings->>PageGroupsPanel: Render with UID from config
User->>SuggestiveModeSettings: Toggle flags / click "Generate Embeddings"
SuggestiveModeSettings->>ConfigTreeUtils: (On action) Update or log action
User->>PageGroupsPanel: Add/remove groups/pages
PageGroupsPanel->>ConfigTreeUtils: Update config tree
PageGroupsPanel->>SuggestiveModeSettings: Refresh UI
sequenceDiagram
participant User
participant NodeConfig
participant DiscourseNodeSuggestiveRules
participant ConfigTreeUtils
User->>NodeConfig: Select "Node Suggestive Rules" tab
NodeConfig->>DiscourseNodeSuggestiveRules: Render with node and parentUid
DiscourseNodeSuggestiveRules->>ConfigTreeUtils: Fetch node config
User->>DiscourseNodeSuggestiveRules: Edit template, embedding ref, or flags
DiscourseNodeSuggestiveRules->>ConfigTreeUtils: Update config tree
DiscourseNodeSuggestiveRules->>DiscourseNodeSuggestiveRules: Refresh UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (8)
apps/roam/src/utils/discourseConfigRef.ts (1)
49-54: Nit: Align "Page Groups" naming (casing) or remove unused 'name'The returned object sets
name: "page groups"while the key is "Page Groups". Align casing or dropnameif it's not used.- pageGroups: { - uid: pageGroupsNode?.uid || "", - name: "page groups", - pages: [], - }, + pageGroups: { + uid: pageGroupsNode?.uid || "", + name: "Page Groups", + pages: [], + },apps/roam/src/utils/renderNodeConfigPage.ts (1)
151-162: Good addition: Dedicated "Suggestive Rules" panelPanel wiring looks correct;
uidwill refer to the “Suggestive Rules” block created by ConfigPage. This aligns with NodeConfig.Consider adding a brief description to guide users, e.g.,
description: "Configure node-level suggestive rules and templates."for UX consistency with other panels.apps/roam/src/components/settings/Settings.tsx (1)
164-169: Tab added correctly; minor naming consistencyThe tab is "Suggestive Mode" while the panel header says "Discourse Suggestions". Consider aligning naming to reduce confusion.
- title="Suggestive Mode" + title="Suggestive Mode" className="mb-8 overflow-y-auto" - panel={<SuggestiveModeSettings />} + panel={<SuggestiveModeSettings />}If you prefer the "Suggestions" terminology, mirror it in both places.
apps/roam/src/components/settings/SuggestiveModeSettings.tsx (2)
16-18: Consistency: Align header with tab titleTab label is "Suggestive Mode" while header says "Discourse Suggestions". Consider unifying to "Suggestive Mode".
- Discourse Suggestions + Suggestive Mode
20-27: Prefer Tailwind class to inline style for spacingKeeps styling consistent with the rest of the codebase.
- <Button + <Button icon="cloud-upload" text={"Generate & Upload All Node Embeddings"} onClick={() => console.log("Not implemented")} intent={Intent.PRIMARY} - style={{ marginTop: "8px" }} + className="mt-2" />apps/roam/src/utils/configPageTabs.ts (1)
17-17: Nit: naming consistency between file and componentFile is PageGroupPanel.tsx but the component is PageGroupsPanel (plural). Either rename the file to PageGroupsPanel.tsx or alias the import as PageGroupPanel for consistency across the codebase.
apps/roam/src/components/settings/PageGroupPanel.tsx (1)
152-159: Memoize getAllPageNames() to avoid recomputing on every rendergetAllPageNames() is invoked for each render and for each group’s input. Cache once per mount.
- options={getAllPageNames()} + options={allPageNames}Add this near the other hooks (above the return):
const allPageNames = useMemo(() => getAllPageNames(), []);apps/roam/src/components/settings/DiscourseNodeSuggestiveRules.tsx (1)
143-153: Prefer Tailwind class over inline style for min widthAligns with repo convention.
<Button text="Update Embeddings" intent={Intent.NONE} onClick={() => console.log("Not implemented")} - style={{ minWidth: "140px" }} + className="min-w-[140px]" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/roam/src/components/settings/DiscourseNodeSuggestiveRules.tsx(1 hunks)apps/roam/src/components/settings/NodeConfig.tsx(2 hunks)apps/roam/src/components/settings/PageGroupPanel.tsx(1 hunks)apps/roam/src/components/settings/Settings.tsx(2 hunks)apps/roam/src/components/settings/SuggestiveModeSettings.tsx(1 hunks)apps/roam/src/utils/configPageTabs.ts(2 hunks)apps/roam/src/utils/discourseConfigRef.ts(3 hunks)apps/roam/src/utils/renderNodeConfigPage.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use the roamAlphaApi documentation from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when working with the Roam API
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use Roam Depot/Extension API documentation from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when working with the Roam Extension API
📚 Learning: 2025-06-25T22:56:17.522Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#0
File: :0-0
Timestamp: 2025-06-25T22:56:17.522Z
Learning: In the Roam discourse-graph system, the existence of the configuration page (identified by DISCOURSE_CONFIG_PAGE_TITLE) and its corresponding UID is a system invariant. The code can safely assume this page will always exist, so defensive null checks are not needed when using `getPageUidByPageTitle(DISCOURSE_CONFIG_PAGE_TITLE)`.
Applied to files:
apps/roam/src/utils/renderNodeConfigPage.tsapps/roam/src/utils/discourseConfigRef.tsapps/roam/src/utils/configPageTabs.ts
📚 Learning: 2025-06-22T10:40:52.752Z
Learnt from: sid597
PR: DiscourseGraphs/discourse-graph#232
File: apps/roam/src/utils/getAllDiscourseNodesSince.ts:18-31
Timestamp: 2025-06-22T10:40:52.752Z
Learning: In apps/roam/src/utils/getAllDiscourseNodesSince.ts, the user confirmed that querying for `?title` with `:node/title` and mapping it to the `text` field in the DiscourseGraphContent type is the correct implementation for retrieving discourse node content from Roam Research, despite it appearing to query page titles rather than block text content.
Applied to files:
apps/roam/src/utils/renderNodeConfigPage.tsapps/roam/src/utils/discourseConfigRef.tsapps/roam/src/components/settings/NodeConfig.tsxapps/roam/src/components/settings/DiscourseNodeSuggestiveRules.tsx
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension
Applied to files:
apps/roam/src/utils/renderNodeConfigPage.tsapps/roam/src/components/settings/Settings.tsxapps/roam/src/components/settings/SuggestiveModeSettings.tsxapps/roam/src/components/settings/PageGroupPanel.tsxapps/roam/src/components/settings/NodeConfig.tsxapps/roam/src/components/settings/DiscourseNodeSuggestiveRules.tsxapps/roam/src/utils/configPageTabs.ts
📚 Learning: 2025-07-19T22:33:54.521Z
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/api.mdc:0-0
Timestamp: 2025-07-19T22:33:54.521Z
Learning: You are working on the api routes for Discourse Graph which uses NextJS app router.
Applied to files:
apps/roam/src/utils/renderNodeConfigPage.ts
📚 Learning: 2025-06-23T11:49:45.457Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#220
File: apps/roam/src/utils/conceptConversion.ts:11-40
Timestamp: 2025-06-23T11:49:45.457Z
Learning: In the DiscourseGraphs/discourse-graph codebase, direct access to `window.roamAlphaAPI` is the established pattern throughout the codebase. The team prefers to maintain this pattern consistently rather than making piecemeal changes, and plans to address dependency injection as a global refactor when scheduled.
Applied to files:
apps/roam/src/utils/renderNodeConfigPage.ts
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use Roam Depot/Extension API documentation from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when working with the Roam Extension API
Applied to files:
apps/roam/src/components/settings/Settings.tsxapps/roam/src/components/settings/PageGroupPanel.tsxapps/roam/src/components/settings/NodeConfig.tsxapps/roam/src/components/settings/DiscourseNodeSuggestiveRules.tsxapps/roam/src/utils/configPageTabs.ts
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use the roamAlphaApi documentation from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when working with the Roam API
Applied to files:
apps/roam/src/components/settings/Settings.tsxapps/roam/src/components/settings/SuggestiveModeSettings.tsxapps/roam/src/components/settings/PageGroupPanel.tsxapps/roam/src/components/settings/NodeConfig.tsxapps/roam/src/components/settings/DiscourseNodeSuggestiveRules.tsxapps/roam/src/utils/configPageTabs.ts
📚 Learning: 2025-06-19T19:43:43.380Z
Learnt from: sid597
PR: DiscourseGraphs/discourse-graph#226
File: apps/roam/src/components/settings/HomePersonalSettings.tsx:123-149
Timestamp: 2025-06-19T19:43:43.380Z
Learning: The "Fetch Embeddings for nodes" button in HomePersonalSettings.tsx is for testing purposes only, so it doesn't require production-level error handling or user feedback improvements.
Applied to files:
apps/roam/src/components/settings/Settings.tsxapps/roam/src/components/settings/SuggestiveModeSettings.tsxapps/roam/src/components/settings/DiscourseNodeSuggestiveRules.tsx
📚 Learning: 2025-07-19T22:34:08.725Z
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/main.mdc:0-0
Timestamp: 2025-07-19T22:34:08.725Z
Learning: Applies to {apps/website,packages/ui}/**/*.{tsx,jsx} : Use platform-native UI components first with shadcn/ui as a fallback
Applied to files:
apps/roam/src/components/settings/Settings.tsxapps/roam/src/components/settings/SuggestiveModeSettings.tsxapps/roam/src/components/settings/PageGroupPanel.tsxapps/roam/src/components/settings/DiscourseNodeSuggestiveRules.tsx
📚 Learning: 2025-06-17T23:37:45.289Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `node.type` serves as the UID field rather than having a conventional `node.uid` field. This is an unusual naming convention where the type field actually contains the unique identifier.
Applied to files:
apps/roam/src/utils/discourseConfigRef.tsapps/roam/src/components/settings/NodeConfig.tsxapps/roam/src/components/settings/DiscourseNodeSuggestiveRules.tsx
📚 Learning: 2025-06-17T23:37:45.289Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `type` serves as the unique identifier field, not a type classification field. The interface has no `uid` or `id` field, making `node.type` the correct field to use for UID-related operations.
Applied to files:
apps/roam/src/utils/discourseConfigRef.tsapps/roam/src/components/settings/NodeConfig.tsxapps/roam/src/components/settings/DiscourseNodeSuggestiveRules.tsx
📚 Learning: 2025-06-22T10:40:21.679Z
Learnt from: sid597
PR: DiscourseGraphs/discourse-graph#232
File: apps/roam/src/utils/getAllDiscourseNodesSince.ts:15-16
Timestamp: 2025-06-22T10:40:21.679Z
Learning: In the getAllDiscourseNodesSince function in apps/roam/src/utils/getAllDiscourseNodesSince.ts, date validation is performed before the function is called, so additional date validation within the function is not needed.
Applied to files:
apps/roam/src/components/settings/NodeConfig.tsxapps/roam/src/components/settings/DiscourseNodeSuggestiveRules.tsx
📚 Learning: 2025-07-19T22:34:08.725Z
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/main.mdc:0-0
Timestamp: 2025-07-19T22:34:08.725Z
Learning: Applies to {apps/website,packages/ui}/**/*.{tsx,jsx} : When refactoring inline styles, use tailwind classes
Applied to files:
apps/roam/src/components/settings/DiscourseNodeSuggestiveRules.tsx
🔇 Additional comments (9)
apps/roam/src/utils/discourseConfigRef.ts (2)
65-65: LGTM: FormattedConfigTree extension and inclusionThe
suggestiveModeaddition is cleanly typed and correctly integrated into the returned config.Also applies to: 92-92
41-49: Defaults are correctly handled in getUidAndBooleanSetting
getUidAndBooleanSettingreturns
• uid:undefined(blank)
• value:false(via!!node?.uid)
when the matching node is missing.
No changes are needed here.apps/roam/src/utils/renderNodeConfigPage.ts (1)
27-27: LGTM: Import of DiscourseNodeSuggestiveRulesImport is correct and scoped.
apps/roam/src/components/settings/Settings.tsx (1)
28-28: LGTM: Import of SuggestiveModeSettingsNo issues.
apps/roam/src/components/settings/NodeConfig.tsx (1)
18-18: LGTM: Import of DiscourseNodeSuggestiveRulesImport path and usage are correct.
apps/roam/src/components/settings/SuggestiveModeSettings.tsx (2)
29-45: LGTM: Flag panels are correctly wiredUIDs and parentUid are correctly passed; should allow creation if missing (assuming FlagPanel default behavior).
If FlagPanel expects non-empty
uidto pre-exist, confirm it handles emptyuidby creating underparentUid.
47-48: LGTM: PageGroupsPanel receives parent block uidAssuming the panel creates missing children under this uid, this is correct.
apps/roam/src/components/settings/DiscourseNodeSuggestiveRules.tsx (2)
17-22: Ignore incorrect DiscourseNode shape change
TheDiscourseNodealias in DiscourseNodeSuggestiveRules.tsx is a local type for Roam blocks (via getSubTree) and must includeuidfor all parentUid and setting lookups. The review conflates it with the graph’s DiscourseNode (type-as-UID) from utils/getDiscourseNodes.ts. No changes are needed here.Likely an incorrect or invalid review comment.
133-140: Verify FlagPanel value prop type
It looks like you’re passing the entire{ uid, value }object fromnodeConfigTree.isFirstChildinto thevalueprop ofFlagPanel, but based ongetUidAndBooleanSettingthe intention is to pass a boolean. SinceFlagPanelis imported fromroamjs-components, please confirm whether itsvalueprop is typed asboolean(and not the full object). If so, update this block to:<FlagPanel title="First Child" description="If the block is the first child of the template, it will be embedded and ranked." order={1} - uid={nodeConfigTree.isFirstChild.uid} - value={nodeConfigTree.isFirstChild} + uid={nodeConfigTree.isFirstChild.uid} + value={nodeConfigTree.isFirstChild.value} />File:
• apps/roam/src/components/settings/DiscourseNodeSuggestiveRules.tsx (around lines 133–140)
apps/roam/src/components/settings/DiscourseNodeSuggestiveRules.tsx
Outdated
Show resolved
Hide resolved
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (6)
apps/roam/src/utils/configPageTabs.ts (1)
136-145: Redundant ts-ignore: you already cast to FieldYou can drop the ts-ignore above Panel since the object is explicitly cast.
- // @ts-ignore { title: "Page Groups" Panel: CustomPanel, description: "Set page groups to use for discourse suggestions", options: { component: PageGroupsPanel, }, } as Field<CustomField>,apps/roam/src/components/settings/SuggestiveModeSettings.tsx (1)
12-15: Avoid side-effects in state initializer; use useEffect + useStateCalling refreshConfigTree() during initial state creation causes side-effects in render. Initialize from getFormattedConfigTree, then refresh in an effect.
- const [settings, setSettings] = useState(() => { - refreshConfigTree(); - return getFormattedConfigTree(); - }); + const [settings, setSettings] = useState(() => getFormattedConfigTree()); + useEffect(() => { + refreshConfigTree(); + setSettings(getFormattedConfigTree()); + }, []);apps/roam/src/components/settings/PageGroupPanel.tsx (4)
93-101: Remove setTimeout around state updateThe setTimeout indirection is unnecessary and can cause UI lag or stale updates.
- const setPageInput = useCallback((groupUid: string, value: string) => { - setTimeout(() => { - setNewPageInputs((prev) => ({ - ...prev, - [groupUid]: value, - })); - }, 0); - }, []); + const setPageInput = useCallback((groupUid: string, value: string) => { + setNewPageInputs((prev) => ({ ...prev, [groupUid]: value })); + }, []);
135-137: Fix empty state check for arraysObject.keys(pageGroups).length is for objects, not arrays. Use pageGroups.length.
- {Object.keys(pageGroups).length === 0 && ( + {pageGroups.length === 0 && ( <div className="text-sm italic text-gray-500">No groups added.</div> )}
43-52: Normalize and dedupe group names (trim + case-insensitive)Prevents near-duplicate groups like "Research" vs "research" and trailing spaces.
- const addGroup = async (name: string) => { - if (!name || pageGroups.some((g) => g.name === name)) return; + const addGroup = async (name: string) => { + const normalized = name?.trim(); + if ( + !normalized || + pageGroups.some((g) => g.name.trim().toLowerCase() === normalized.toLowerCase()) + ) + return; try { - await createBlock({ parentUid: uid, node: { text: name } }); + await createBlock({ parentUid: uid, node: { text: normalized } }); refreshGroups(); setNewGroupName(""); } catch (e) { console.error("Error adding group", e); } };Also update the button’s disabled condition below to use the same normalization/dedupe rule.
63-82: Normalize page names and add robust dedupe; optional: store as page refsTrim input and dedupe case-insensitively. Optionally wrap in [[...]] if you want explicit page refs.
- const addPageToGroup = async (groupUid: string, page: string) => { + const addPageToGroup = async (groupUid: string, page: string) => { const group = pageGroups.find((g) => g.uid === groupUid); - if (!page || group?.pages.some((p) => p.name === page)) { + const normalized = page?.trim(); + if ( + !normalized || + group?.pages.some((p) => p.name.trim().toLowerCase() === normalized.toLowerCase()) + ) { return; } try { - await createBlock({ parentUid: groupUid, node: { text: page } }); + // For explicit page refs: use text: `[[${normalized}]]` + await createBlock({ parentUid: groupUid, node: { text: normalized } }); refreshGroups(); setNewPageInputs((prev) => ({ ...prev, [groupUid]: "", })); setAutocompleteKeys((prev) => ({ ...prev, [groupUid]: (prev[groupUid] || 0) + 1, })); } catch (e) { console.error("Error adding page to group", e); } };
🧹 Nitpick comments (7)
apps/roam/src/utils/renderNodeConfigPage.ts (1)
151-163: Add a short description for the new “Suggestive Rules” panelHelps users discover intent and aligns with other panels that include descriptions.
{ title: "Suggestive Rules", + description: + "Configure node-level suggestive rules (templates and options).", Panel: CustomPanel, options: {apps/roam/src/components/settings/SuggestiveModeSettings.tsx (1)
55-61: Unify title string with config tabThis uses "Include Parent And Child Blocks" while configPageTabs uses "Include Parent and Child Blocks". Standardize to the latter for consistency.
- title="Include Parent And Child Blocks" + title="Include Parent and Child Blocks"apps/roam/src/components/settings/PageGroupPanel.tsx (3)
128-130: Keep disabled logic in sync with normalizationMirror the normalized checks used in addGroup/addPageToGroup to avoid enabling the button for near-duplicates.
- disabled={ - !newGroupName || pageGroups.some((g) => g.name === newGroupName) - } + disabled={ + !newGroupName?.trim() || + pageGroups.some( + (g) => g.name.trim().toLowerCase() === newGroupName.trim().toLowerCase(), + ) + }- disabled={ - !getPageInput(group.uid) || - group.pages.some((p) => p.name === getPageInput(group.uid)) - } + disabled={ + !getPageInput(group.uid)?.trim() || + group.pages.some( + (p) => + p.name.trim().toLowerCase() === + getPageInput(group.uid).trim().toLowerCase(), + ) + }Also applies to: 178-180
166-166: Avoid recomputing all page names on every renderMemoize getAllPageNames() once to reduce work.
Add above the return (and import useMemo):
const allPageNames = useMemo(() => getAllPageNames(), []);Then change this line:
- options={getAllPageNames()} + options={allPageNames}
16-23: Refresh when uid prop changesIf the parent uid changes, refreshGroups should run to reflect the new subtree.
Add:
useEffect(() => { refreshGroups(); }, [uid, refreshGroups]);Also applies to: 25-33
apps/roam/src/components/settings/DiscourseNodeSuggestiveRules.tsx (2)
79-86: Guard when Template subtree is missinggetSubTree(...).uid may be empty if "Template" doesn’t exist; consider safe default to avoid runtime issues.
- const templateUid = useMemo( - () => - getSubTree({ - parentUid: nodeUid, - key: "Template", - }).uid, - [nodeUid], - ); + const templateUid = useMemo(() => { + const t = getSubTree({ parentUid: nodeUid, key: "Template" }); + return t?.uid || ""; + }, [nodeUid]);Optionally also set a sensible defaultValue only when templateUid is truthy.
99-114: Minor: Make orders unique under the same parentBoth Template and Embedding Block Ref use order=0. Consider incrementing to ensure stable ordering in UI.
- order={0} + order={0} ... - order={0} + order={1} ... - order={1} + order={2}Also applies to: 127-134
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/roam/src/components/settings/DiscourseNodeSuggestiveRules.tsx(1 hunks)apps/roam/src/components/settings/NodeConfig.tsx(2 hunks)apps/roam/src/components/settings/PageGroupPanel.tsx(1 hunks)apps/roam/src/components/settings/Settings.tsx(2 hunks)apps/roam/src/components/settings/SuggestiveModeSettings.tsx(1 hunks)apps/roam/src/utils/configPageTabs.ts(2 hunks)apps/roam/src/utils/discourseConfigRef.ts(3 hunks)apps/roam/src/utils/getSuggestiveModeConfigSettings.ts(1 hunks)apps/roam/src/utils/renderNodeConfigPage.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-17T23:37:45.289Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `node.type` serves as the UID field rather than having a conventional `node.uid` field. This is an unusual naming convention where the type field actually contains the unique identifier.
Applied to files:
apps/roam/src/components/settings/DiscourseNodeSuggestiveRules.tsx
📚 Learning: 2025-06-17T23:37:45.289Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `type` serves as the unique identifier field, not a type classification field. The interface has no `uid` or `id` field, making `node.type` the correct field to use for UID-related operations.
Applied to files:
apps/roam/src/components/settings/DiscourseNodeSuggestiveRules.tsx
🔇 Additional comments (8)
apps/roam/src/utils/getSuggestiveModeConfigSettings.ts (1)
18-21: Config key casing is already consistentI verified that the config tree and lookup both use
"Suggestive mode"(lowercase “mode”)—for example:
- apps/roam/src/utils/configPageTabs.ts:121 → id: "Suggestive mode"
- apps/roam/src/utils/getSuggestiveModeConfigSettings.ts:20 → key: "Suggestive mode"
- apps/roam/src/components/settings/SuggestiveModeSettings.tsx:22 → node: { text: "Suggestive mode" }
The only
"Suggestive Mode"(upper-case “Mode”) appears in UI titles (e.g. Settings.tsx title prop), which is independent of the config-lookup key. No changes are needed.Likely an incorrect or invalid review comment.
apps/roam/src/components/settings/NodeConfig.tsx (2)
18-18: LGTM: import added for Node Suggestive Rules
331-347: Ensure reliable retrieval/creation of the “Suggestive Rules” blockSwap out the inline getSubTree call in NodeConfig.tsx (around lines 331–347) for a precomputed UID and, if needed, create the block before rendering:
• In
apps/roam/src/components/settings/NodeConfig.tsx, after your other UID declarations (for example, right afterconst indexUid = getUid("Index");), add:const suggestiveRulesUid = getUid("Suggestive Rules");• Update the
<DiscourseNodeSuggestiveRules>invocation:- <DiscourseNodeSuggestiveRules - node={node} - parentUid={getSubTree({ - parentUid: node.type, - key: "Suggestive Rules", - }).uid} - /> + <DiscourseNodeSuggestiveRules + node={node} + parentUid={suggestiveRulesUid} + />• Optionally, ensure the block actually exists before you render:
if (!suggestiveRulesUid) { await createBlock({ parentUid: node.type, node: { text: "Suggestive Rules" }, }); // re‐fetch or re‐assign suggestiveRulesUid here }Points to verify manually:
- How getSubTree behaves when the block is missing (does it return
""oruid: ""?), and- Whether creating the parent block (and retrieving its UID) is required to prevent downstream breakage.
apps/roam/src/components/settings/Settings.tsx (2)
28-28: LGTM: import for Suggestive Mode settings
164-169: LGTM: new “Suggestive Mode” tab under Global SettingsIntegration looks consistent with other tabs.
apps/roam/src/utils/discourseConfigRef.ts (1)
28-29: LGTM: typed inclusion of suggestiveMode in FormattedConfigTreeapps/roam/src/utils/configPageTabs.ts (1)
17-17: Import looks correctPageGroupsPanel import path is consistent with project alias usage.
apps/roam/src/components/settings/DiscourseNodeSuggestiveRules.tsx (1)
98-105: BlocksPanel uses correct parentUidUsing nodeUid (node.type) aligns with DiscourseNode’s UID convention and fixes prior issue.
apps/roam/src/components/settings/DiscourseNodeSuggestiveRules.tsx
Outdated
Show resolved
Hide resolved
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
apps/roam/src/utils/getSuggestiveModeConfigSettings.ts (1)
27-41: Don’t discard configured pages; populate from the config tree and harden parentUidReturning an empty
pagesarray drops existing config. Also defaultparentUiddefensively.Apply:
- return { - parentUid: suggestiveModeNode.uid, + return { + parentUid: suggestiveModeNode?.uid || "", grabFromReferencedPages: getUidAndBooleanSetting({ tree: suggestiveModeNode.children, text: "Include Current Page Relations", }), grabParentAndChildren: getUidAndBooleanSetting({ tree: suggestiveModeNode.children, text: "Include Parent And Child Blocks", }), pageGroups: { uid: pageGroupsNode?.uid || "", name: "Page Groups", - pages: [], + pages: + (pageGroupsNode?.children || []).map((c) => ({ + uid: c.uid, + name: c.text, + })) || [], }, };apps/roam/src/utils/configPageTabs.ts (2)
136-145: Remove unnecessary ts-ignore; the cast already provides correct typingThe CustomPanel field is cast to Field, so the ts-ignore is redundant. Remove it for consistency with the rest of the file.
- // @ts-ignore { title: "Page Groups", Panel: CustomPanel, description: "Set page groups to use for discourse suggestions", options: { component: PageGroupsPanel, }, } as Field<CustomField>,
131-135: Standardize title casing to avoid brittle lookupsUse "Include Parent and Child Blocks" (lowercase "and") to match other occurrences and lookup utilities.
- title: "Include Parent And Child Blocks", + title: "Include Parent and Child Blocks",Run to confirm all occurrences are consistent:
#!/bin/bash rg -n -S $'Include Parent and Child Blocks|Include Parent And Child Blocks' apps/roamapps/roam/src/components/settings/SuggestiveModeSettings.tsx (3)
12-15: Move refreshConfigTree out of state initializerAvoid invoking side-effects in the state initializer. Initialize from current tree, then refresh and set via useEffect.
- const [settings, setSettings] = useState(() => { - refreshConfigTree(); - return getFormattedConfigTree(); - }); + const [settings, setSettings] = useState(() => getFormattedConfigTree()); + useEffect(() => { + refreshConfigTree(); + setSettings(getFormattedConfigTree()); + }, []);
17-29: Harden block creation with error handling and avoid unhandled promiseHandle missing parent UID, catch failures, and use void to avoid unhandled promise warnings.
useEffect(() => { - const ensureSettings = async () => { - if (!settings.suggestiveMode.parentUid) { - await createBlock({ - parentUid: getPageUidByPageTitle(DISCOURSE_CONFIG_PAGE_TITLE), - node: { text: "Suggestive mode" }, - }); - refreshConfigTree(); - setSettings(getFormattedConfigTree()); - } - }; - ensureSettings(); + const ensureSettings = async () => { + if (!settings.suggestiveMode.parentUid) { + try { + const parent = getPageUidByPageTitle(DISCOURSE_CONFIG_PAGE_TITLE); + if (!parent) throw new Error("Discourse config page not found"); + await createBlock({ + parentUid: parent, + node: { text: "Suggestive mode" }, + }); + refreshConfigTree(); + setSettings(getFormattedConfigTree()); + } catch (e) { + console.error("Failed to ensure Suggestive mode settings root", e); + } + } + }; + void ensureSettings(); }, [settings.suggestiveMode.parentUid]);
55-61: Standardize title casing to match lookupsChange to "Include Parent and Child Blocks" to align with utils and the config tab.
- <FlagPanel - title="Include Parent And Child Blocks" + <FlagPanel + title="Include Parent and Child Blocks"apps/roam/src/components/settings/PageGroupPanel.tsx (2)
94-101: Remove unnecessary setTimeout wrapper around state updatesetState is already async and batched; the setTimeout(0) indirection can cause UI lag and stale updates.
- const setPageInput = useCallback((groupUid: string, value: string) => { - setTimeout(() => { - setNewPageInputs((prev) => ({ - ...prev, - [groupUid]: value, - })); - }, 0); - }, []); + const setPageInput = useCallback((groupUid: string, value: string) => { + setNewPageInputs((prev) => ({ ...prev, [groupUid]: value })); + }, []);
135-137: Bug: array empty-state check uses Object.keys on an arrayUse pageGroups.length to correctly detect an empty list.
- {Object.keys(pageGroups).length === 0 && ( + {pageGroups.length === 0 && ( <div className="text-sm italic text-gray-500">No groups added.</div> )}
🧹 Nitpick comments (6)
apps/roam/src/utils/renderNodeConfigPage.ts (1)
151-162: Optional: add a short description for the new panel to match surrounding panelsKeeps consistency with other entries and clarifies purpose.
{ title: "Suggestive Rules", + description: "Configure rules that generate suggestions for this node type.", Panel: CustomPanel, options: { component: ({ uid }) => React.createElement(DiscourseNodeSuggestiveRules, { node, parentUid: uid, }), }, } as Field<CustomField>,apps/roam/src/components/settings/NodeConfig.tsx (1)
331-347: Use existing getUid helper for consistency and readabilityYou already have
const getUid = (key: string) => getSubTree({ parentUid: node.type, key }).uid;. Use it here to stay DRY.<DiscourseNodeSuggestiveRules node={node} - parentUid={ - getSubTree({ - parentUid: node.type, - key: "Suggestive Rules", - }).uid - } + parentUid={getUid("Suggestive Rules")} />apps/roam/src/components/settings/PageGroupPanel.tsx (3)
43-52: Normalize inputs and avoid case-sensitive duplicates when adding groupsTrim the name and compare case-insensitively to reduce user friction.
- const addGroup = async (name: string) => { - if (!name || pageGroups.some((g) => g.name === name)) return; + const addGroup = async (name: string) => { + const normalized = name?.trim(); + if ( + !normalized || + pageGroups.some((g) => g.name.toLowerCase() === normalized.toLowerCase()) + ) + return; try { - await createBlock({ parentUid: uid, node: { text: name } }); + await createBlock({ parentUid: uid, node: { text: normalized } }); refreshGroups(); setNewGroupName(""); } catch (e) { console.error("Error adding group", e); } };
63-82: Normalize inputs and avoid case-sensitive duplicates when adding pagesTrim input and compare case-insensitively; consider storing explicit page refs if desired.
- const addPageToGroup = async (groupUid: string, page: string) => { + const addPageToGroup = async (groupUid: string, page: string) => { const group = pageGroups.find((g) => g.uid === groupUid); - if (!page || group?.pages.some((p) => p.name === page)) { + const normalized = page?.trim(); + if ( + !normalized || + group?.pages.some((p) => p.name.toLowerCase() === normalized.toLowerCase()) + ) { return; } try { - await createBlock({ parentUid: groupUid, node: { text: page } }); + // If you prefer explicit page refs in Roam, use: `[[${normalized}]]` + await createBlock({ parentUid: groupUid, node: { text: normalized } }); refreshGroups(); setNewPageInputs((prev) => ({ ...prev, [groupUid]: "", })); setAutocompleteKeys((prev) => ({ ...prev, [groupUid]: (prev[groupUid] || 0) + 1, })); } catch (e) { console.error("Error adding page to group", e); } };
161-168: Avoid recomputing all page names on every renderMemoize getAllPageNames and reuse it in AutocompleteInput.
-import React, { useState, useCallback } from "react"; +import React, { useState, useCallback, useMemo } from "react"; @@ + const allPageNames = useMemo(() => getAllPageNames(), []); @@ - <AutocompleteInput + <AutocompleteInput key={getAutocompleteKey(group.uid)} value={getPageInput(group.uid)} placeholder="Add page…" setValue={(v) => setPageInput(group.uid, v)} - options={getAllPageNames()} + options={allPageNames} maxItemsDisplayed={50} />apps/roam/src/components/settings/DiscourseNodeSuggestiveRules.tsx (1)
38-55: Add cleanup to BlockRenderer to avoid lingering DOM nodes on unmountClear the container on effect cleanup.
useEffect(() => { const container = containerRef.current; if (container) { while (container.firstChild) { container.removeChild(container.firstChild); } if (uid) { window.roamAlphaAPI.ui.components.renderBlock({ uid: uid, el: container, }); } } - }, [uid]); + return () => { + if (containerRef.current) { + containerRef.current.textContent = ""; + } + }; + }, [uid]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/roam/src/components/settings/DiscourseNodeSuggestiveRules.tsx(1 hunks)apps/roam/src/components/settings/NodeConfig.tsx(2 hunks)apps/roam/src/components/settings/PageGroupPanel.tsx(1 hunks)apps/roam/src/components/settings/Settings.tsx(2 hunks)apps/roam/src/components/settings/SuggestiveModeSettings.tsx(1 hunks)apps/roam/src/utils/configPageTabs.ts(2 hunks)apps/roam/src/utils/discourseConfigRef.ts(3 hunks)apps/roam/src/utils/getSuggestiveModeConfigSettings.ts(1 hunks)apps/roam/src/utils/renderNodeConfigPage.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-19T22:34:08.725Z
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/main.mdc:0-0
Timestamp: 2025-07-19T22:34:08.725Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Maintain consistent naming conventions: PascalCase for components and types
Applied to files:
apps/roam/src/utils/configPageTabs.ts
📚 Learning: 2025-06-17T23:37:45.289Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `node.type` serves as the UID field rather than having a conventional `node.uid` field. This is an unusual naming convention where the type field actually contains the unique identifier.
Applied to files:
apps/roam/src/components/settings/DiscourseNodeSuggestiveRules.tsx
📚 Learning: 2025-06-17T23:37:45.289Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#220
File: apps/roam/src/utils/conceptConversion.ts:42-56
Timestamp: 2025-06-17T23:37:45.289Z
Learning: In the DiscourseNode interface from apps/roam/src/utils/getDiscourseNodes.ts, the field `type` serves as the unique identifier field, not a type classification field. The interface has no `uid` or `id` field, making `node.type` the correct field to use for UID-related operations.
Applied to files:
apps/roam/src/components/settings/DiscourseNodeSuggestiveRules.tsx
🧬 Code Graph Analysis (4)
apps/roam/src/utils/getSuggestiveModeConfigSettings.ts (1)
apps/roam/src/utils/getExportSettings.ts (2)
BooleanSetting(12-12)getUidAndBooleanSetting(56-62)
apps/roam/src/utils/discourseConfigRef.ts (1)
apps/roam/src/utils/getSuggestiveModeConfigSettings.ts (2)
SuggestiveModeConfigWithUids(5-14)getSuggestiveModeConfigAndUids(16-43)
apps/roam/src/components/settings/SuggestiveModeSettings.tsx (2)
apps/roam/src/utils/discourseConfigRef.ts (1)
getFormattedConfigTree(31-57)apps/roam/src/utils/renderNodeConfigPage.ts (1)
DISCOURSE_CONFIG_PAGE_TITLE(29-29)
apps/roam/src/components/settings/DiscourseNodeSuggestiveRules.tsx (1)
apps/roam/src/utils/getExportSettings.ts (1)
getUidAndBooleanSetting(56-62)
🔇 Additional comments (10)
apps/roam/src/utils/getSuggestiveModeConfigSettings.ts (1)
16-18: Good refactor: broke the circular dependency by acceptingtreeTaking
tree: RoamBasicNode[]eliminates the runtime cycle withdiscourseConfigRef. This aligns with prior feedback.apps/roam/src/components/settings/NodeConfig.tsx (1)
18-18: Import looks correctImporting
DiscourseNodeSuggestiveRuleshere is expected for the new tab.apps/roam/src/components/settings/Settings.tsx (2)
28-28: Import is correctBrings the new Suggestive Mode settings into the dialog.
164-169: New “Suggestive Mode” tab integration looks goodPlacement under Global Settings matches feature scope. Panel wiring is correct.
apps/roam/src/utils/discourseConfigRef.ts (3)
10-13: Correctly scoped import for suggestive mode helpersImports limited to the helper and type; good separation.
28-29: Type extension is appropriateAdding
suggestiveMode: SuggestiveModeConfigWithUids;to the formatted tree is consistent with other domains (export, canvas).
55-56: Initialization-cycle issue resolvedConfirmed that:
getSuggestiveModeConfigAndUidsno longer importsdiscourseConfigRef- The only call site (
apps/roam/src/utils/discourseConfigRef.ts:55) passesconfigTreeRef.treeInitialization-time cycle is fully addressed.
apps/roam/src/components/settings/DiscourseNodeSuggestiveRules.tsx (3)
64-65: Using node.type as UID is correct for DiscourseNodeThis aligns with the project’s convention where type is the node UID.
106-113: BlocksPanel parentUid set to nodeUidCorrect parent is used for editing the node’s template.
135-142: FlagPanel receives boolean valuePassing isFirstChild.value fixes the prior bug of supplying an object.
apps/roam/src/components/settings/DiscourseNodeSuggestiveRules.tsx
Outdated
Show resolved
Hide resolved
apps/roam/src/components/settings/DiscourseNodeSuggestiveRules.tsx
Outdated
Show resolved
Hide resolved
apps/roam/src/components/settings/DiscourseNodeSuggestiveRules.tsx
Outdated
Show resolved
Hide resolved
apps/roam/src/components/settings/DiscourseNodeSuggestiveRules.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Good start! Quite a few changes requested but the big one is the settings pattern. (noted in a few places)
There are comments before this review, so be sure to check those as well (sorry!)
One more big thing, I'm getting a lot of type/build errors. I tried rm node_modules/package-lock to see if I could get a similar build state as yours, but no luck. Let's try rebasing this main to update to the latest package-lock. That means removing your node_modules and using main's package-lock instead of your current one.
Let me know if you have issues.
| value={settings.suggestiveMode.grabFromReferencedPages.value} | ||
| /> | ||
| <FlagPanel | ||
| title="Include Parent And Child Blocks" |
There was a problem hiding this comment.
Does "Include Current Page Relations" make "Include Parent and Child Blocks" redundant?
EG: if "Include Current Page Relations" is checked should "Include Parent and Child Blocks" be checked and disabled?
If so, maybe we make it into a selection instead. (this can be deferred)
There was a problem hiding this comment.
Yes this is true : if "Include Current Page Relations" is checked "Include Parent and Child Blocks" should be checked and disabled.
|
All the review points addressed now using the settings pattern, waiting on #343 to finish, so i can rebase this off that, fix bugs, polish whatever flow is missing. |
…ase' into eng-709-settings-panel-for-suggestive-mode
…ity, fix functionality
|
https://www.loom.com/share/4988d7f6744641789a41ba605ac0497e?sid=4da947f1-43f0-464e-a517-ba269f391c30 This video shows a toaster that is all white, it happened because of roam studio but if i disable roam studio and then do this again, the toaster and text show up correctly. |
apps/roam/src/components/settings/DiscourseNodeSuggestiveRules.tsx
Outdated
Show resolved
Hide resolved
| <Button | ||
| text="Update Embeddings" | ||
| intent={Intent.NONE} | ||
| onClick={() => console.log("Not implemented")} |
There was a problem hiding this comment.
👀 Is there a linear ticket or reference for when this will be implemented?
There was a problem hiding this comment.
🤦🏽 Yeah this needs a ticket to discuss what is the right action for this, I have a minimal function that can be a right approach and was in alpha branch. Thank you for catching this 🙌🏽
ENG-800: What is the right approach to handle "Update Embeddings" button?
apps/roam/src/components/settings/DiscourseNodeSuggestiveRules.tsx
Outdated
Show resolved
Hide resolved
…-settings-panel-for-suggestive-mode
|
reverted the Update Embeddings code, merging cc @mdroidian |
* settings for context and page groups * node settings * address review * fix node creation if it does not exist * remove unused, refactor * address coderabbit review * unify spelling, handle fallback * sync * address coderabbit code * address lint errors * use async instead of backend * address review * address review * bug fixes * bug fixes * unused import * Add an input type for platform accounts Add functions to upsert platform accounts (individually or in bulk) Keep the old create_account_in_space function as a shim for now. Use the new upsert_account in upsert_documents and upsert_content, allowing for more complete inline information. * Add an input type for platform accounts Add functions to upsert platform accounts (individually or in bulk) Keep the old create_account_in_space function as a shim for now. Use the new upsert_account in upsert_documents and upsert_content, allowing for more complete inline information. * Add an input type for platform accounts Add functions to upsert platform accounts (individually or in bulk) Keep the old create_account_in_space function as a shim for now. Use the new upsert_account in upsert_documents and upsert_content, allowing for more complete inline information. * bulk upsert accounts, use database function imports * add comment for future * use better import method * commonjs-to-esm * fix merge errors, fix tree config setting extraction, test functionality, fix functionality * add progress toaster, use space existence condition for on load sync * defaultvalue * address review, leaving out update embeddings from here * update embeddings * ENG-818: Declare dbDotEnv as mjs explicitly * reverting create a new pr for update embeddings --------- Co-authored-by: Marc-Antoine Parent <[email protected]>
3 participants
Summary by CodeRabbit
New Features
Enhancements
https://www.loom.com/share/7218d88262e142098ab0b5622fa25272?sid=94b47b41-2e76-4874-8849-ce78c8cb4baa