ENG-1291: Port discourse node specification

[sid597]

https://www.loom.com/share/ec0987fd82474127ad994237104f9b9e

---

Summary by CodeRabbit

• Refactor
  • Enhanced query settings interface with improved flag-based controls for managing query specifications
  • Reorganized settings data structure to support explicit enable/disable states and clearer configuration options

[linear]

ENG-1291 Port discourse node specification

[supabase]

This pull request has been ignored for the connected project zytfjzqyijgagqxrzbmz because there are no changes detected in packages/database/supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.

---

Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

[sid597]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• ENG-1281: Port discourse node block panel #784 : 3 dependent PRs (#793 , #795 , #805 )
• ENG-1290: Port discourse node config panel #769
• ENG-1291: Port discourse node specification #765 👈 (View in Graphite)
• ENG-1280: Port Discourse node: Query builder and editor component for block props #764
• ENG-1225: Discourse node migration simple settings #696 : 1 other dependent PR (#697 )
• ENG-1273: Migrate small personal settings #691
• ENG-1272 Migrate all small global settings components #688 : 1 other dependent PR (#690 )
• ENG-1345: Modify existing RoamJS component onChange to also change props #745 : 1 other dependent PR (#684 )
• ENG-1190 Replace existing roam-js components with blockProp based ones #689
• ENG-1188: prod: pull watchers for prop based settings #682
• ENG-1187: Prod: Implement get and set for block prop based settings #669
• ENG-1189: Prod: Init block prop based schema #666
• ENG-1330: zod schema for discourse relations #719
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[sid597]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[coderabbitai]

📝 Walkthrough

Walkthrough

A settingKey prop is added to query components (QueryBuilder, QueryEditor) to specify which settings object path to use ("index" or "specification"). The zod schema is restructured so that specification changes from an array to an object with an enabled flag and nested query object. DiscourseNodeSpecification component is refactored to use DiscourseNodeFlagPanel and manages block creation/removal for enable/disable state.

Changes

Cohort / File(s)	Summary
Component Props Threading apps/roam/src/components/QueryBuilder.tsx, apps/roam/src/components/QueryEditor.tsx	Added optional settingKey prop ("index" | "specification") threaded through components. QueryEditor uses settingKey to determine dynamic path for syncing settings via setDiscourseNodeSetting and guards effect with settingKey presence.
Settings Schema Restructure apps/roam/src/components/settings/utils/zodSchema.ts, apps/roam/src/components/settings/utils/zodSchema.example.ts	Changed DiscourseNodeSchema.specification from array to object with enabled flag and nested query structure. Added returnNode field to IndexSchema. Updated default transformations to match new nested shape.
Component Implementation Updates apps/roam/src/components/settings/DiscourseNodeIndex.tsx, apps/roam/src/components/settings/DiscourseNodeSpecification.tsx	DiscourseNodeIndex passes settingKey="index" to QueryBuilder and initializes settings with custom and returnNode. DiscourseNodeSpecification replaces Checkbox-based UI with DiscourseNodeFlagPanel, adds block creation/removal logic for enable/disable, and passes settingKey="specification" to nested components.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• ENG-1187: Prod: Implement get and set for block prop based settings #669: PR #669 introduces block-property accessors (get/set) that this PR directly depends on for calling setDiscourseNodeSetting with dynamic paths in QueryEditor.
• ENG-879 A node can have no specification or format #703: Both PRs modify DiscourseNodeSpecification component, with this PR adding settingKey prop threading and UI component replacement that complements changes in #703.
• ENG-1105: zod schema for all the settings #664: The schema restructuring of DiscourseNodeSettings.specification from array to object with enabled flag aligns with schema evolution work related in #664.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'ENG-1291: Port discourse node specification' directly describes the main change: porting (implementing/migrating) the discourse node specification feature.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

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/QueryEditor.tsx (1)

491-518: ⚠️ Potential issue | 🔴 Critical

Critical bug confirmed: returnNode is excluded from the sync payload, causing it to be reset to "" whenever conditions/selections/custom change.

The stripped object (line 494–498) contains only { conditions, selections, custom }. When IndexSchema.safeParse validates it, the missing returnNode field defaults to "" (per the schema definition at zodSchema.ts:56). This overrides the returnNode initially set by:

• DiscourseNodeIndex.tsx (line 53: returnNode: DEFAULT_RETURN_NODE)
• DiscourseNodeSpecification.tsx (line 77: returnNode: node.text)

The overwrite occurs ~250ms after mount when the sync effect fires on any change to conditions, selections, or custom.

Since returnNode is not stored as state in QueryEditor (only extracted once from parseQuery result but not destructured), there is no way to persist it through subsequent syncs. Include returnNode in the state and sync payload to fix this.

🧹 Nitpick comments (3)

apps/roam/src/components/settings/utils/zodSchema.ts (1)

106-113: Consider extracting the default specification value to avoid duplication with IndexSchema defaults.

Line 113 manually spells out the full default for query ({ conditions: [], selections: [], custom: "", returnNode: "" }), which duplicates the defaults already declared in IndexSchema (lines 53–57). If IndexSchema gains a new field with a default, this transform will silently become stale.

♻️ Suggested refactor

+const DEFAULT_SPECIFICATION = {
+  enabled: false,
+  query: { conditions: [], selections: [], custom: "", returnNode: "" },
+} as const;
+
 export const DiscourseNodeSchema = z.object({
   ...
   specification: z
     .object({
       enabled: z.boolean().default(false),
       query: IndexSchema.default({}),
     })
     .nullable()
     .optional()
-    .transform((val) => val ?? { enabled: false, query: { conditions: [], selections: [], custom: "", returnNode: "" } }),
+    .transform((val) => val ?? DEFAULT_SPECIFICATION),

This keeps the default in one place and shortens the line.

apps/roam/src/components/settings/DiscourseNodeSpecification.tsx (2)

96-111: Potential race: disable path deletes scratch children then resets block props — ensure ordering is safe.

Lines 99–107 delete all scratch children, then in .then() reset the block props. This ordering is fine for the happy path. However, if the user rapidly toggles enabled → disabled → enabled, the effect on line 32 will fire for both states. The cleanup function (lines 112–114) only calls refreshConfigTree() and doesn't cancel the in-flight Promise.all or createBlock chains, so interleaved create/delete operations could leave inconsistent state.

Consider adding a cancelled flag or using an AbortController pattern in the effect to avoid races on rapid toggles.

---

119-121: CSS overrides to hide the query button and restyle the checkbox are fine but fragile.

These selectors (.bp3-button.bp3-intent-primary, .bp3-checkbox, .bp3-control-indicator) are tied to BlueprintJS 3 class names. Consider adding a brief comment explaining why these overrides are needed, so future maintainers don't accidentally remove them.

[mdroidian]