ENG-1187: Prod: Implement get and set for block prop based settings by sid597 · Pull Request #669 · DiscourseGraphs/discourse-graph

sid597 · 2026-01-05T08:58:33Z

https://www.loom.com/share/3c07a184d5d740a59410ee49d1fead0c

Summary by CodeRabbit

New Features
- Expanded settings management: unified handling for feature flags, global, personal, and node-specific settings with stronger validation and safer nested reads/writes.
Changes
- Relations data moved from index-based lists to keyed maps for more reliable access and iteration.
- Added caching and cache invalidation for discourse nodes/relations to improve performance and robustness.
- Added lightweight stubs/hooks to support left-sidebar personal settings initialization.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

linear · 2026-01-05T08:58:36Z

ENG-1187 Base: Accessors: implement Get and Set setttings as block properties functions

supabase · 2026-01-05T08:58:40Z

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 · 2026-01-05T08:58:53Z

ENG-1345: Modify existing RoamJS component onChange to also change props #745 : 2 dependent PRs (#684 , #688 )
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 👈 (View in Graphite)
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-01-05T08:59:24Z

@CodeRabbit ai full review

coderabbitai · 2026-01-05T08:59:39Z

@sid597: I'll perform a full review of this PR now.

🧠 Learnings used

Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 630
File: apps/roam/src/utils/settingsUsingBlockProps.ts:64-66
Timestamp: 2025-12-22T05:43:09.803Z
Learning: In `apps/roam/src/utils/settingsUsingBlockProps.ts`, the `setBlockPropBasedSettings` function expects callers to ensure that the `value` parameter matches the expected type for the given `keys` path. Type validation is handled at the caller side rather than within the utility function.

✅ Actions performed

Full review triggered.

coderabbitai · 2026-01-05T09:02:52Z

📝 Walkthrough

Walkthrough

Adds a new Roam settings accessors module with typed, nested get/set utilities, schema validation, caching and invalidation; converts Relations from arrays to record objects in schemas and defaults; and wires the new accessors into init with stub calls for Left Sidebar personal sections.

Changes

Cohort / File(s)	Summary
New Settings Accessors `apps/roam/src/components/settings/utils/accessors.ts`	New file exporting ~30 helpers for reading/writing nested Roam block props by UID or settings-page keys: block-level getters/setters, feature-flag/global/personal/discourse-node accessors, caching/invalidation, path-safe deep updates, Zod validation, and small Left Sidebar stub demos.
Init wiring & stubs `apps/roam/src/components/settings/utils/init.ts`	Replaced direct Roam query with `getAllDiscourseNodes()` from the new accessors; added imports for stub helpers and calls to `stubSetLeftSidebarPersonalSections` and deferred `stubGetLeftSidebarPersonalSections`. Minor control-flow refactor.
Relations default shape `apps/roam/src/components/settings/data/defaultRelationsBlockProps.ts`	Changed export from an array of `DiscourseRelationSettings[]` to an object keyed by relation ids (`{ informs: {...}, supports: {...}, opposes: {...} }`), altering iteration/access patterns.
Example schema defaults `apps/roam/src/components/settings/utils/zodSchema.example.ts`	Updated example GlobalSettings.Relations from an array to a keyed object (Record<string, Relation>) and defaultGlobalSettings.Relations from `[]` to `{}`.
Zod schema updates & top-level keys `apps/roam/src/components/settings/utils/zodSchema.ts`	GlobalSettings.Relations changed from `z.array(...)` to `z.record(z.string(), ...)` with default cast to record; added and exported `TOP_LEVEL_BLOCK_PROP_KEYS` constant and updated `getTopLevelBlockPropsConfig` to use those keys.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant Accessors as Accessors Module
    participant Zod as Zod Schema
    participant Roam as Roam Blocks DB
    participant Cache as Local Cache

    Client->>Accessors: getGlobalSetting(keys)
    Accessors->>Cache: check cache for page/block props
    alt cache hit
        Cache-->>Accessors: cached value
    else cache miss
        Accessors->>Roam: read block by page/title/uid
        Roam-->>Accessors: block props (raw)
        Accessors->>Zod: validate/parse props
        Zod-->>Accessors: parsed/typed settings
        Accessors->>Cache: store parsed value
    end
    Accessors-->>Client: return typed setting

    Client->>Accessors: setGlobalSetting(keys, value)
    Accessors->>Zod: validate value against schema
    Zod-->>Accessors: validated payload
    Accessors->>Roam: write/update nested block props
    Roam-->>Accessors: write confirmation
    Accessors->>Cache: invalidate/update cache
    Accessors-->>Client: success

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

ENG-1330: zod schema for discourse relations #719 — Modifies discourse relations schema and arrays; strongly related to Relations → record changes.
ENG-1105: zod schema for all the settings #664 — Also touches GlobalSettings.Relations and top-level settings keys; overlaps with zodSchema updates.
ENG-1047 Repair setBlockProps #542 — Adjusts block prop read/write normalization; likely interacts with the new get/set accessors' behavior.

🚥 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 clearly summarizes the main change: implementing get and set functionality for block property-based settings, which aligns with the comprehensive new accessor module and integration work across the changeset.
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.}

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.}

devin-ai-integration

Devin Review found 1 potential issue.

View issue and 4 additional flags in Devin Review.

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

mdroidian

Good start.

Main items to address:

Key-path semantics: Several functions accept keys: string[], but the meaning of keys differs by function (sometimes it’s a traversal path, sometimes the first element is treated as a lookup key, etc.). Please make this explicit at the call site—either by splitting functions (e.g., setAtPath vs setRoot / getFromSettingsBlock vs getByUid) or by renaming parameters (path vs settingsKeys) and adding docstrings/examples.
Type safety for keys: Callers currently have to manually know the valid keys for each schema. Can we make this more Zod/TypeScript-safe (e.g., typed path helpers, exported key constants, or a small set of typed getters/setters per settings domain) to avoid stringly-typed access?
Type assertions: There are a number of as ... assertions. Please reduce these where possible by tightening types and/or adding type guards. If an assertion is truly necessary, add a short comment explaining why it is required, and why it is safe.
Export surface: It’s unclear which exports are intended as public API vs internal helpers. Please audit exports and keep only the functions intended for callers outside accessors.ts (or move internal helpers to non-exported scope).
Error reporting: Any console.warn that indicates a user-impacting or unexpected state should go through internalError() so we can track it and surface appropriate messaging.

apps/roam/src/components/settings/data/defaultRelationsBlockProps.ts

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

mdroidian · 2026-01-25T00:23:06Z

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

+  }, settings) as T | undefined;
+};
+
+let discourseNodesCache: DiscourseNodeSettings[] | null = null;


What is the thought behind having a cache just for discourse node settings?

you mean we should have the caching for other settings as well? Its for discourse node because we have to run a query and thats expensive so we cache it

Which query are you referring to?

line 466 in function getAllDiscourseNodes()

getAllDiscourseNodes() (just a handlefull of pages based on a simple string match) shouldn't be a heavy query, let's not cache it

mdroidian · 2026-01-25T00:40:33Z

@sid597 Also make sure you format with prettier

sid597 · 2026-01-28T09:51:37Z

Did the following changes:

use _INF-rel, _SUP-rel etc as the key for default relations
reduce usage of as .... to bare minimum
use internalError
Add schema validation on setters, so each call to setGlobalSetting, setPersonalSetting, first validates the schema and then sets the value. We already do this on read but now also on set. Before this review I was going with , do the validation on the component side itself e.g FlagPanel component first checks the value its setting and only then we set the value the cons of that is iff a user calls the underneath function of setGlobalSetting with any wrong value type
removed all the exports that are not sure of if needed later.
Inside geAllDiscourseNodes function use fast.q and get block props in same call instead of mutliple calls

devin-ai-integration

Devin Review found 1 new potential issue.

View issue and 5 additional flags in Devin Review.

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

mdroidian

A few comments to address then I'd like to walk through this with you 1:1. Also check the merge conflict.

apps/roam/src/components/settings/data/defaultRelationsBlockProps.ts

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

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

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

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

devin-ai-integration

Devin Review found 1 potential issue.

View 5 additional findings in Devin Review.

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

mdroidian

Let's just remove the caches for now (shouldn't need them, so let's not make it more complex than we need to). We'll add them back in if we find a need for them.

Then there's a few more comments to address.

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

apps/roam/src/components/settings/data/defaultRelationsBlockProps.ts

apps/roam/src/components/settings/utils/zodSchema.example.ts

mdroidian · 2026-02-06T04:17:49Z

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

+  }, settings) as T | undefined;
+};
+
+let discourseNodesCache: DiscourseNodeSettings[] | null = null;


getAllDiscourseNodes() (just a handlefull of pages based on a simple string match) shouldn't be a heavy query, let's not cache it

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

mdroidian

Actually I'll approve this, but tag me before merging if there are any issues with implementing the requests.

devin-ai-integration

Devin Review found 1 new potential issue.

View 6 additional findings in Devin Review.

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

sid597 marked this pull request as ready for review January 5, 2026 08:58

sid597 mentioned this pull request Jan 5, 2026

ENG-1105: zod schema for all the settings #664

Merged

sid597 mentioned this pull request Jan 5, 2026

ENG-1189: Prod: Init block prop based schema #666

Merged

sid597 force-pushed the eng-1187-implement-get-and-set-setttings-as-block-properties-prod branch from 1ebeb69 to 22de4b1 Compare January 7, 2026 18:38

sid597 force-pushed the eng-1189-prod-init-schema branch 2 times, most recently from e30930e to 0ca99fc Compare January 8, 2026 05:18

sid597 force-pushed the eng-1187-implement-get-and-set-setttings-as-block-properties-prod branch from 22de4b1 to 9464473 Compare January 8, 2026 05:18

sid597 force-pushed the eng-1189-prod-init-schema branch from 0ca99fc to 7ea650f Compare January 11, 2026 06:31

sid597 force-pushed the eng-1187-implement-get-and-set-setttings-as-block-properties-prod branch from 9464473 to 3d7e4c7 Compare January 11, 2026 06:31

This was referenced Jan 11, 2026

ENG-1188: prod: pull watchers for prop based settings #682

Merged

ENG-1274: Migrate feature flags #684

Closed

sid597 force-pushed the eng-1187-implement-get-and-set-setttings-as-block-properties-prod branch from 3d7e4c7 to fb87a45 Compare January 11, 2026 19:54

sid597 mentioned this pull request Jan 12, 2026

ENG-1272 Migrate all small global settings components #688

Open

sid597 force-pushed the eng-1187-implement-get-and-set-setttings-as-block-properties-prod branch from fb87a45 to 3fbeb9f Compare January 12, 2026 15:26

sid597 force-pushed the eng-1189-prod-init-schema branch from 0365026 to 76d487b Compare January 12, 2026 15:26

sid597 force-pushed the eng-1189-prod-init-schema branch from 76d487b to 3c7d025 Compare January 17, 2026 18:16

sid597 force-pushed the eng-1187-implement-get-and-set-setttings-as-block-properties-prod branch from 3fbeb9f to 6cf85ea Compare January 17, 2026 18:16

sid597 force-pushed the eng-1187-implement-get-and-set-setttings-as-block-properties-prod branch from c43eb11 to 67cb84e Compare January 23, 2026 15:01

sid597 changed the base branch from graphite-base/669 to eng-1189-prod-init-schema January 23, 2026 15:01

sid597 requested a review from mdroidian January 23, 2026 15:35

devin-ai-integration bot reviewed Jan 24, 2026

View reviewed changes

apps/roam/src/components/settings/utils/init.ts Outdated Show resolved Hide resolved

mdroidian requested changes Jan 25, 2026

View reviewed changes

sid597 changed the base branch from eng-1189-prod-init-schema to graphite-base/669 January 28, 2026 09:53

sid597 force-pushed the graphite-base/669 branch from 8a707cf to c0ca4ef Compare January 28, 2026 09:53

sid597 force-pushed the eng-1187-implement-get-and-set-setttings-as-block-properties-prod branch from 67cb84e to 177a47d Compare January 28, 2026 09:53

sid597 changed the base branch from graphite-base/669 to main January 28, 2026 09:53

sid597 requested a review from mdroidian January 28, 2026 09:54

devin-ai-integration bot reviewed Jan 28, 2026

View reviewed changes

apps/roam/src/components/settings/utils/accessors.ts Show resolved Hide resolved

sid597 force-pushed the eng-1187-implement-get-and-set-setttings-as-block-properties-prod branch from 177a47d to b40fcdc Compare January 28, 2026 18:08

mdroidian requested changes Feb 2, 2026

View reviewed changes

sid597 force-pushed the eng-1187-implement-get-and-set-setttings-as-block-properties-prod branch from b40fcdc to e11e431 Compare February 2, 2026 09:21

sid597 mentioned this pull request Feb 3, 2026

ENG-1345: Modify existing RoamJS component onChange to also change props #745

Merged

sid597 force-pushed the eng-1187-implement-get-and-set-setttings-as-block-properties-prod branch 2 times, most recently from 0cf76d1 to c95a994 Compare February 5, 2026 15:12

address review

132d1c5

sid597 force-pushed the eng-1187-implement-get-and-set-setttings-as-block-properties-prod branch from c95a994 to 132d1c5 Compare February 5, 2026 16:50

sid597 requested a review from mdroidian February 5, 2026 16:51

devin-ai-integration bot reviewed Feb 5, 2026

View reviewed changes

apps/roam/src/components/settings/utils/accessors.ts Show resolved Hide resolved

address review

98ea51c

mdroidian requested changes Feb 6, 2026

View reviewed changes

mdroidian approved these changes Feb 6, 2026

View reviewed changes

address michael review

0fd984d

sid597 merged commit a35a194 into main Feb 6, 2026
6 checks passed

sid597 deleted the eng-1187-implement-get-and-set-setttings-as-block-properties-prod branch February 6, 2026 12:08

devin-ai-integration bot reviewed Feb 6, 2026

View reviewed changes

apps/roam/src/components/settings/utils/accessors.ts Show resolved Hide resolved

Conversation

sid597 commented Jan 5, 2026 • edited by devin-ai-integration bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

linear bot commented Jan 5, 2026

Uh oh!

supabase bot commented Jan 5, 2026

Uh oh!

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

Uh oh!

Uh oh!

sid597 commented Jan 5, 2026

Uh oh!

coderabbitai bot commented Jan 5, 2026

Uh oh!

coderabbitai bot commented Jan 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Uh oh!

devin-ai-integration bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mdroidian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mdroidian Jan 25, 2026

Choose a reason for hiding this comment

Uh oh!

sid597 Jan 28, 2026

Choose a reason for hiding this comment

Uh oh!

mdroidian Feb 1, 2026

Choose a reason for hiding this comment

Uh oh!

sid597 Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

mdroidian Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

mdroidian commented Jan 25, 2026

Uh oh!

sid597 commented Jan 28, 2026

Uh oh!

devin-ai-integration bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mdroidian left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

devin-ai-integration bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mdroidian left a comment

Choose a reason for hiding this comment

sid597 commented Jan 5, 2026 •

edited by devin-ai-integration bot

Loading

sid597 commented Jan 5, 2026 •

edited

Loading

coderabbitai bot commented Jan 5, 2026 •

edited

Loading

mdroidian left a comment •

edited

Loading