ENG-1291: Port discourse node specification by sid597 · Pull Request #765 · DiscourseGraphs/discourse-graph

sid597 · 2026-02-11T18:43:20Z

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 · 2026-02-11T18:43:23Z

ENG-1291 Port discourse node specification

sid597 · 2026-02-11T18:43:42Z

ENG-1281: Port discourse node block panel #784 : 2 dependent PRs (#793 , #795 )
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 · 2026-02-11T18:47:28Z

@coderabbitai full review

coderabbitai · 2026-02-11T18:47:42Z

✅ Actions performed

Full review triggered.

coderabbitai · 2026-02-11T18:53:20Z

📝 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

_{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

supabase · 2026-02-23T11:02:26Z

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 ↗︎.

mdroidian · 2026-02-24T04:56:18Z

apps/roam/src/components/settings/utils/init.ts

      const existingProps = getBlockProps(uid);
-      if (!existingProps || Object.keys(existingProps).length === 0) {
+      // TODO: Overwriting on safeParse failure is a temporary fix for schema shape changes
+      // (e.g. specification: [] -> {enabled, query}). Replace with proper versioned migrations.


Why do we need this already if this is our first version? As in, we don't have any other versions/migrations?

we already have merged code in main that would have created the previous specification structure in graphs that have main version of roam plugin and so we need to migrate for those graphs.

mdroidian · 2026-02-24T05:04:11Z

apps/roam/src/components/QueryEditor.tsx

  hideCustomSwitch,
  showAlias,
  discourseNodeType,
+  settingKey, // eslint-disable-line react/prop-types


Why do we need to resort to this disable? What did you try to properly fix it?

not under scope of this pr every other key also has the same problem and in multiple places in the codebase

fixing it properly would result in more touch points and get out of scope for this pr

These PRs were squash-merged into parent branches instead of main due to a Graphite stack merge race condition. Their changes never landed on main despite showing as merged in Graphite. Includes: - #764 ENG-1280: Port query builder/editor for block props - #765 ENG-1291: Port discourse node specification (dual-write) - #769 ENG-1290: Port discourse node config panel - #793 ENG-1440: Port page groups settings in suggestive mode - #794 ENG-1438: Port keyboard shortcut keys/triggers - #805 ENG-1328: Port small blueprint misc settings

specification, config panel) The initial recovery commit claimed to include these but only contained Chain 2 (#793, #794, #805, #804). Chain 1 lives on separate branches that weren't in the merge source. Applied using `gh pr diff` for each PR in order (#764 → #765 → #769). init.ts required manual merge — #765 and #769 both modified it from the same base. Kept #765's safeParse checks inside #769's restructured initializeSettingsBlockProps. - #764 ENG-1280: Port query builder and editor component for block props - #765 ENG-1291: Port discourse node specification - #769 ENG-1290: Port discourse node config panel

…#826) * Land orphaned Graphite stack PRs (#764, #765, #769, #793, #794, #805) These PRs were squash-merged into parent branches instead of main due to a Graphite stack merge race condition. Their changes never landed on main despite showing as merged in Graphite. Includes: - #764 ENG-1280: Port query builder/editor for block props - #765 ENG-1291: Port discourse node specification (dual-write) - #769 ENG-1290: Port discourse node config panel - #793 ENG-1440: Port page groups settings in suggestive mode - #794 ENG-1438: Port keyboard shortcut keys/triggers - #805 ENG-1328: Port small blueprint misc settings * Add #804 ENG-1217: Port section component in left sidebar * Add #764, #765, #769 (Chain 1 orphaned PRs: query builder, specification, config panel) The initial recovery commit claimed to include these but only contained Chain 2 (#793, #794, #805, #804). Chain 1 lives on separate branches that weren't in the merge source. Applied using `gh pr diff` for each PR in order (#764 → #765 → #769). init.ts required manual merge — #765 and #769 both modified it from the same base. Kept #765's safeParse checks inside #769's restructured initializeSettingsBlockProps. - #764 ENG-1280: Port query builder and editor component for block props - #765 ENG-1291: Port discourse node specification - #769 ENG-1290: Port discourse node config panel * fix lint * prettier

These PRs were squash-merged into parent branches instead of main due to a Graphite stack merge race condition. Their changes never landed on main despite showing as merged in Graphite. Includes: - #764 ENG-1280: Port query builder/editor for block props - #765 ENG-1291: Port discourse node specification (dual-write) - #769 ENG-1290: Port discourse node config panel - #793 ENG-1440: Port page groups settings in suggestive mode - #794 ENG-1438: Port keyboard shortcut keys/triggers - #805 ENG-1328: Port small blueprint misc settings

sid597 mentioned this pull request Feb 11, 2026

ENG-1280: Port Discourse node: Query builder and editor component for block props #764

Merged

sid597 marked this pull request as ready for review February 11, 2026 18:46

This comment was marked as resolved.

Sign in to view

coderabbitai bot reviewed Feb 11, 2026

View reviewed changes

This was referenced Feb 12, 2026

ENG-1290: Port discourse node config panel #769

Merged

ENG-1281: Port discourse node block panel #784

Merged

sid597 requested a review from mdroidian February 15, 2026 17:10

sid597 force-pushed the eng-1280-discourse-node-query-builder-and-specification-setting-v2 branch from 898a0bd to adab8d8 Compare February 15, 2026 18:24

sid597 force-pushed the eng-1291-port-discourse-node-specificationv1 branch from 1c7af7a to 6bed1ba Compare February 15, 2026 18:24

This comment was marked as resolved.

Sign in to view

mdroidian requested changes Feb 16, 2026

View reviewed changes

sid597 force-pushed the eng-1280-discourse-node-query-builder-and-specification-setting-v2 branch from adab8d8 to cc9fd40 Compare February 16, 2026 08:25

sid597 force-pushed the eng-1291-port-discourse-node-specificationv1 branch from 6bed1ba to 39d97bf Compare February 16, 2026 08:25

sid597 force-pushed the eng-1280-discourse-node-query-builder-and-specification-setting-v2 branch from cc9fd40 to 3be01d4 Compare February 17, 2026 05:16

sid597 force-pushed the eng-1291-port-discourse-node-specificationv1 branch from 39d97bf to 09f158c Compare February 17, 2026 05:16

This was referenced Feb 17, 2026

ENG-1440: Port page groups settings in suggestive mode #793

Merged

ENG-1438: Port Keyboard shortcut keys/triggers settings #794

Merged

dual write page group to block props #795

Closed

sid597 force-pushed the eng-1291-port-discourse-node-specificationv1 branch from 09f158c to 7b96a6d Compare February 17, 2026 15:07

sid597 force-pushed the eng-1280-discourse-node-query-builder-and-specification-setting-v2 branch 2 times, most recently from 72de51b to 28d44a4 Compare February 17, 2026 15:37

sid597 force-pushed the eng-1291-port-discourse-node-specificationv1 branch 2 times, most recently from 7a25648 to fde2738 Compare February 17, 2026 16:04

sid597 force-pushed the eng-1280-discourse-node-query-builder-and-specification-setting-v2 branch from 28d44a4 to 5a3bbba Compare February 17, 2026 16:04

sid597 mentioned this pull request Feb 20, 2026

ENG-1328: Port small blueprint based misc settings into props based #805

Merged

sid597 changed the base branch from graphite-base/765 to eng-1280-discourse-node-query-builder-and-specification-setting-v2 February 23, 2026 10:27

sid597 mentioned this pull request Feb 23, 2026

ENG-1217: Port section component in left sidebar #804

Merged

This comment was marked as resolved.

Sign in to view

sid597 deleted the branch eng-1280-discourse-node-query-builder-and-specification-setting-v2 February 23, 2026 10:41

sid597 closed this Feb 23, 2026

sid597 deleted the eng-1291-port-discourse-node-specificationv1 branch February 23, 2026 10:42

sid597 restored the eng-1291-port-discourse-node-specificationv1 branch February 23, 2026 11:00

sid597 reopened this Feb 23, 2026

sid597 force-pushed the eng-1280-discourse-node-query-builder-and-specification-setting-v2 branch from ff23661 to c704afc Compare February 23, 2026 13:51

sid597 force-pushed the eng-1291-port-discourse-node-specificationv1 branch from 7a086b1 to 433277c Compare February 23, 2026 13:51

sid597 requested a review from mdroidian February 23, 2026 13:55

sid597 mentioned this pull request Feb 23, 2026

ENG-1473: Port non-boolean personal setting consumers #818

Closed

sid597 force-pushed the eng-1291-port-discourse-node-specificationv1 branch from 433277c to af25640 Compare February 23, 2026 16:40

mdroidian approved these changes Feb 24, 2026

View reviewed changes

sid597 force-pushed the eng-1280-discourse-node-query-builder-and-specification-setting-v2 branch from 3678893 to 89a82ea Compare February 24, 2026 05:46

sid597 force-pushed the eng-1291-port-discourse-node-specificationv1 branch from af25640 to b08670d Compare February 24, 2026 05:46

sid597 added 4 commits February 24, 2026 11:18

ENG-1291: Port discourse node specification

f8ce950

devin review

7ff8a96

pass initialValue instead of defaultvalue

5a95d6b

fix lint, prettier, address ai reviews

6ad633e

sid597 force-pushed the eng-1291-port-discourse-node-specificationv1 branch from b08670d to 6ad633e Compare February 24, 2026 05:48

sid597 force-pushed the eng-1280-discourse-node-query-builder-and-specification-setting-v2 branch from 89a82ea to 33c45e6 Compare February 24, 2026 05:48

sid597 merged commit d9c0d00 into eng-1280-discourse-node-query-builder-and-specification-setting-v2 Feb 24, 2026
13 checks passed

coderabbitai bot mentioned this pull request Feb 24, 2026

ENG-1468: Port node and relations tree based consumers #825

Open

sid597 mentioned this pull request Feb 24, 2026

Land orphaned Graphite stack PRs (#764, #765, #769, #793, #794, #805) #826

Merged

77 tasks

Conversation

sid597 commented Feb 11, 2026 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

linear bot commented Feb 11, 2026

Uh oh!

sid597 commented Feb 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sid597 commented Feb 11, 2026

Uh oh!

coderabbitai bot commented Feb 11, 2026

Uh oh!

This comment was marked as resolved.

Uh oh!

coderabbitai bot commented Feb 11, 2026

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

mdroidian left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

supabase bot commented Feb 23, 2026

Uh oh!

mdroidian Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

sid597 Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

mdroidian Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

sid597 Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

sid597 Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

sid597 commented Feb 11, 2026 •

edited by coderabbitai bot

Loading

sid597 commented Feb 11, 2026 •

edited

Loading