Skip to content

Conversation

@sid597
Copy link
Collaborator

@sid597 sid597 commented Dec 22, 2025

Summary by CodeRabbit

  • New Features
    • Expanded settings management system with enhanced support for nested configuration storage and retrieval directly from block properties, enabling more flexible settings organization.

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

@linear
Copy link

linear bot commented Dec 22, 2025

@supabase
Copy link

supabase bot commented Dec 22, 2025

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 sid597 changed the title get and set block prop based settings ENG-1187: get and set block prop based settings Dec 22, 2025
@sid597 sid597 marked this pull request as ready for review December 22, 2025 05:32
Copy link
Collaborator Author

sid597 commented Dec 22, 2025

@coderabbitai full review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 22, 2025

✅ Actions performed

Full review triggered.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 22, 2025

📝 Walkthrough

Walkthrough

Introduces a new utility module for managing nested block properties on Roam pages, exposing functions to retrieve and update settings stored as block props along a path of keys, with support for deep-cloning and nested structure creation.

Changes

Cohort / File(s) Summary
Block Props Settings Utility
apps/roam/src/utils/settingsUsingBlockProps.ts
New module exporting constant DG_BLOCK_PROP_SETTINGS_PAGE_TITLE, and two functions: getBlockPropBasedSettings() to retrieve nested block prop values by key path, and setBlockPropBasedSettings() to update nested block props with deep-clone and structure creation support.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Key areas requiring attention:
    • Deep-cloning logic and nested property traversal implementation
    • Edge case handling (empty keys, single vs. multi-key paths, undefined intermediate structures)
    • Validation of block resolution and property persistence via setBlockProps

Possibly related PRs

Pre-merge checks

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title 'ENG-1187: get and set block prop based settings' directly and clearly describes the main change—introducing utility functions to manage nested block properties on Roam pages.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
apps/roam/src/utils/settingsUsingBlockProps.ts (1)

47-53: Add explicit return type per coding guidelines.

The function is missing an explicit return type. As per coding guidelines, functions should have explicit return types.

🔎 Proposed fix
 export const setBlockPropBasedSettings = ({
   keys,
   value,
 }: {
   keys: string[];
   value: json;
-}) => {
+}): void => {
📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between eeca848 and 46771fe.

📒 Files selected for processing (1)
  • apps/roam/src/utils/settingsUsingBlockProps.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (.cursor/rules/main.mdc)

**/*.{ts,tsx}: Use Tailwind CSS for styling where possible
When refactoring inline styles, use tailwind classes
Prefer type over interface in TypeScript
Use explicit return types for functions
Avoid any types when possible
Prefer arrow functions over regular function declarations
Use named parameters (object destructuring) when a function has more than 2 parameters
Use PascalCase for components and types
Use camelCase for variables and functions
Use UPPERCASE for constants
Function names should describe their purpose clearly
Prefer early returns over nested conditionals for better readability

Files:

  • apps/roam/src/utils/settingsUsingBlockProps.ts
apps/roam/**/*.{js,ts,tsx,jsx,json}

📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)

Prefer existing dependencies from package.json when working on the Roam Research extension

Files:

  • apps/roam/src/utils/settingsUsingBlockProps.ts
apps/roam/**/*.{ts,tsx,jsx,js,css,scss}

📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)

Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension

Files:

  • apps/roam/src/utils/settingsUsingBlockProps.ts
apps/roam/**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)

apps/roam/**/*.{ts,tsx,js,jsx}: Use the roamAlphaApi docs from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when implementing Roam functionality
Use Roam Depot/Extension API docs from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when implementing extension functionality

Files:

  • apps/roam/src/utils/settingsUsingBlockProps.ts
apps/roam/**

📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)

Implement the Discourse Graph protocol in the Roam Research extension

Files:

  • apps/roam/src/utils/settingsUsingBlockProps.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-11-06T13:48:35.007Z
Learning: In apps/roam/src/utils/createReifiedBlock.ts, the `setBlockProps` function does not need await as it is synchronous or fire-and-forget.
📚 Learning: 2025-11-06T13:48:35.007Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-11-06T13:48:35.007Z
Learning: In apps/roam/src/utils/createReifiedBlock.ts, the `setBlockProps` function does not need await as it is synchronous or fire-and-forget.

Applied to files:

  • apps/roam/src/utils/settingsUsingBlockProps.ts
📚 Learning: 2025-11-25T00:52:41.934Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-11-25T00:52:41.934Z
Learning: Applies to apps/roam/**/*.{ts,tsx,js,jsx} : Use the roamAlphaApi docs from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when implementing Roam functionality

Applied to files:

  • apps/roam/src/utils/settingsUsingBlockProps.ts
📚 Learning: 2025-11-25T00:52:41.934Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-11-25T00:52:41.934Z
Learning: Applies to apps/roam/**/*.{ts,tsx,js,jsx} : Use Roam Depot/Extension API docs from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when implementing extension functionality

Applied to files:

  • apps/roam/src/utils/settingsUsingBlockProps.ts
🧬 Code graph analysis (1)
apps/roam/src/utils/settingsUsingBlockProps.ts (1)
apps/roam/src/utils/getBlockProps.ts (1)
  • json (1-7)
🔇 Additional comments (2)
apps/roam/src/utils/settingsUsingBlockProps.ts (2)

1-6: LGTM!

Imports are appropriate, and the constant follows the UPPERCASE naming convention as per coding guidelines.


69-99: LGTM!

The deep-clone approach using JSON.parse/JSON.stringify is appropriate for the json type. The reduce logic correctly creates intermediate objects when traversing the path. Based on learnings, setBlockProps is fire-and-forget, so no await is needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants