Skip to content

Conversation

@maparent
Copy link
Collaborator

@maparent maparent commented Dec 22, 2025

https://linear.app/discourse-graphs/issue/ENG-1201/update-roamjs-components-and-use-proper-backend-query-signature

Summary by CodeRabbit

  • Chores

    • Updated component library dependency.
  • Refactor

    • Enhanced type safety and improved code clarity by removing unnecessary type-checking suppressions.
    • Simplified async call patterns across the application.
    • Refined type assertions for better type inference.

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

@maparent maparent requested review from mdroidian and removed request for mdroidian December 22, 2025 20:44
@maparent
Copy link
Collaborator Author

@CodeRabbit review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 22, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 22, 2025

📝 Walkthrough

Walkthrough

The PR removes TypeScript ignore directives, Promise.resolve wrappers around Roam API calls, unnecessary comments, and refines type assertions across multiple utility and component files. A roamjs-components dependency is also bumped.

Changes

Cohort / File(s) Summary
Dependency updates
apps/roam/package.json
Version bump: roamjs-components 0.85.6 → 0.85.7
TypeScript directive removal
apps/roam/src/components/SuggestionsBody.tsx, apps/roam/src/utils/fireQuery.ts
Removed // @ts-ignore`` comments preceding Roam API backend.q() calls; runtime behavior unchanged
Comment cleanup
apps/roam/src/components/canvas/Clipboard.tsx
Removed TODO and eslint-disable-next-line comments from AddPageModal's fetchAllPages function
Roam API call pattern refactoring
apps/roam/src/utils/getAllDiscourseNodesSince.ts, apps/roam/src/utils/hyde.ts, apps/roam/src/utils/syncDgNodesToSupabase.ts
Replaced Promise.resolve-wrapped Roam API calls with direct async/await patterns; refined type assertions from unknown to unknown[] as needed; preserved control flow and returned data shapes

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

The changes follow consistent patterns across multiple files (Promise.resolve removal, type assertion refinement, ts-ignore removal), but span 7 files with varying contexts. While individually straightforward, verification of type safety improvements and confirmation that removal of Promise.resolve wrappers maintain correct async semantics requires attention across all utility functions.

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 title directly describes the main changes: updating roamjs-components and adopting the proper (promise-based) backend query signature, which aligns with the changeset across multiple files.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

📜 Recent review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 670fe22 and bf683ab.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (7)
  • apps/roam/package.json
  • apps/roam/src/components/SuggestionsBody.tsx
  • apps/roam/src/components/canvas/Clipboard.tsx
  • apps/roam/src/utils/fireQuery.ts
  • apps/roam/src/utils/getAllDiscourseNodesSince.ts
  • apps/roam/src/utils/hyde.ts
  • apps/roam/src/utils/syncDgNodesToSupabase.ts
💤 Files with no reviewable changes (3)
  • apps/roam/src/components/canvas/Clipboard.tsx
  • apps/roam/src/utils/fireQuery.ts
  • apps/roam/src/components/SuggestionsBody.tsx
🧰 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/syncDgNodesToSupabase.ts
  • apps/roam/src/utils/getAllDiscourseNodesSince.ts
  • apps/roam/src/utils/hyde.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/syncDgNodesToSupabase.ts
  • apps/roam/package.json
  • apps/roam/src/utils/getAllDiscourseNodesSince.ts
  • apps/roam/src/utils/hyde.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/syncDgNodesToSupabase.ts
  • apps/roam/src/utils/getAllDiscourseNodesSince.ts
  • apps/roam/src/utils/hyde.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/syncDgNodesToSupabase.ts
  • apps/roam/src/utils/getAllDiscourseNodesSince.ts
  • apps/roam/src/utils/hyde.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/syncDgNodesToSupabase.ts
  • apps/roam/package.json
  • apps/roam/src/utils/getAllDiscourseNodesSince.ts
  • apps/roam/src/utils/hyde.ts
🧠 Learnings (19)
📓 Common learnings
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/**/*.{js,ts,tsx,jsx,json} : Prefer existing dependencies from package.json when working on the Roam Research extension
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
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,jsx,js,css,scss} : Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension
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
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-10-18T18:58:16.100Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 504
File: apps/roam/src/utils/syncDgNodesToSupabase.ts:523-531
Timestamp: 2025-10-18T18:58:16.100Z
Learning: In `apps/roam/src/utils/syncDgNodesToSupabase.ts`, partial successes from `upsertNodesToSupabaseAsContent` and `addMissingEmbeddings` (indicated by numeric return values showing the count of successful operations) should NOT trigger backoff. Only complete failures (false) should trigger the exponential backoff mechanism. This design allows the sync process to continue making progress even when some items fail.

Applied to files:

  • apps/roam/src/utils/syncDgNodesToSupabase.ts
  • apps/roam/src/utils/getAllDiscourseNodesSince.ts
📚 Learning: 2025-06-22T10:40:52.752Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 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/syncDgNodesToSupabase.ts
  • apps/roam/src/utils/getAllDiscourseNodesSince.ts
  • apps/roam/src/utils/hyde.ts
📚 Learning: 2025-11-05T21:57:14.909Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 534
File: apps/roam/src/utils/createReifiedBlock.ts:40-48
Timestamp: 2025-11-05T21:57:14.909Z
Learning: In the discourse-graph repository, the function `getPageUidByPageTitle` from `roamjs-components/queries/getPageUidByPageTitle` is a synchronous function that returns a string directly (the page UID or an empty string if not found), not a Promise. It should be called without `await`.

Applied to files:

  • apps/roam/src/utils/syncDgNodesToSupabase.ts
  • apps/roam/src/utils/hyde.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/syncDgNodesToSupabase.ts
  • apps/roam/package.json
  • apps/roam/src/utils/getAllDiscourseNodesSince.ts
  • apps/roam/src/utils/hyde.ts
📚 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/syncDgNodesToSupabase.ts
  • apps/roam/src/utils/getAllDiscourseNodesSince.ts
  • apps/roam/src/utils/hyde.ts
📚 Learning: 2025-11-23T23:53:43.094Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-11-23T23:53:43.094Z
Learning: In `apps/roam/src/utils/supabaseContext.ts`, the Roam URL normalization uses a targeted string replacement `url.replace("/?server-port=3333#/", "/#/")` rather than URL parsing to avoid impacting unforeseen URL patterns. This conservative approach is intentional to handle only the specific known case.

Applied to files:

  • apps/roam/src/utils/syncDgNodesToSupabase.ts
  • apps/roam/src/utils/hyde.ts
📚 Learning: 2025-12-19T16:59:40.640Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-12-19T16:59:40.640Z
Learning: In this codebase, type assertions at system boundaries (e.g., Roam datalog queries) are acceptable when the output shape is known from the query structure, such as when using `as Array<[string, string]>` after datalog queries with `:find ?uid ?title` clauses.

Applied to files:

  • apps/roam/src/utils/syncDgNodesToSupabase.ts
  • apps/roam/src/utils/getAllDiscourseNodesSince.ts
  • apps/roam/src/utils/hyde.ts
📚 Learning: 2025-07-13T16:47:14.352Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-07-13T16:47:14.352Z
Learning: In the discourse-graph codebase, types.gen.ts contains automatically generated database function type definitions that may have reordered signatures between versions. This reordering is expected behavior from the code generation process and should not be flagged as an issue requiring fixes.

Applied to files:

  • apps/roam/src/utils/syncDgNodesToSupabase.ts
  • apps/roam/src/utils/getAllDiscourseNodesSince.ts
📚 Learning: 2025-06-17T23:37:45.289Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 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/syncDgNodesToSupabase.ts
  • apps/roam/src/utils/getAllDiscourseNodesSince.ts
📚 Learning: 2025-06-17T23:37:45.289Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 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/syncDgNodesToSupabase.ts
  • apps/roam/src/utils/getAllDiscourseNodesSince.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/**/*.{js,ts,tsx,jsx,json} : Prefer existing dependencies from package.json when working on the Roam Research extension

Applied to files:

  • apps/roam/package.json
  • apps/roam/src/utils/hyde.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/package.json
  • apps/roam/src/utils/hyde.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,jsx,js,css,scss} : Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension

Applied to files:

  • apps/roam/package.json
📚 Learning: 2025-08-11T19:09:58.252Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 337
File: apps/roam/src/components/DiscourseFloatingMenu.tsx:43-43
Timestamp: 2025-08-11T19:09:58.252Z
Learning: The roam subdirectory (apps/roam) is constrained to React 17 and cannot use React 18+ features like createRoot API. ReactDOM.render should be used instead of createRoot in this subdirectory.

Applied to files:

  • apps/roam/package.json
📚 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/** : Implement the Discourse Graph protocol in the Roam Research extension

Applied to files:

  • apps/roam/package.json
📚 Learning: 2025-06-22T10:40:21.679Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 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/utils/getAllDiscourseNodesSince.ts
📚 Learning: 2025-06-23T11:49:45.457Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 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/getAllDiscourseNodesSince.ts
  • apps/roam/src/utils/hyde.ts
📚 Learning: 2025-12-07T20:54:20.007Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 559
File: apps/roam/src/utils/findDiscourseNode.ts:37-39
Timestamp: 2025-12-07T20:54:20.007Z
Learning: In apps/roam/src/utils/findDiscourseNode.ts, the function findDiscourseNodeByTitleAndUid accepts both uid and title parameters where uid is primarily used for cache access (as the cache key) while title is used for the actual matching via matchDiscourseNode. This design reflects the pattern where downstream, the uid is mostly used to fetch the title, so the function caches by uid but matches by title.

Applied to files:

  • apps/roam/src/utils/hyde.ts
🧬 Code graph analysis (1)
apps/roam/src/utils/syncDgNodesToSupabase.ts (1)
apps/roam/src/utils/getAllDiscourseNodesSince.ts (1)
  • RoamDiscourseNodeData (8-17)
🔇 Additional comments (9)
apps/roam/src/utils/getAllDiscourseNodesSince.ts (2)

100-104: LGTM!

Consistent with the pattern applied at lines 60-65. The direct await usage and refined type assertion align with the updated Roam API signature.


60-66: Verify Promise return type change for roamAlphaAPI.data.backend.q in the updated dependency.

Unable to confirm from public documentation whether roamjs-components changed the roamAlphaAPI.data.backend.q signature to return a Promise natively. The API at roamAlphaAPI.data.backend.q does not appear in public Roam API documentation, and the specific version mentioned (0.85.7) could not be verified. Examine the actual dependency upgrade and test the code behavior to confirm the API change.

apps/roam/src/utils/syncDgNodesToSupabase.ts (2)

199-202: LGTM!

The refactored Roam API call pattern matches the changes in getAllDiscourseNodesSince.ts. The refined type assertion (unknown[] as RoamDiscourseNodeData[]) provides better type safety at the system boundary.


340-345: LGTM!

Consistent application of the direct await pattern. The inline type annotation for the author data is clear and appropriately scoped.

apps/roam/src/utils/hyde.ts (4)

352-354: LGTM!

The simplified direct return of the awaited Roam API call removes the unnecessary Promise.resolve wrapper. The type assertion as [string, string][] aligns with the datalog query's :find ?pageName ?pageUid clause.


360-369: LGTM!

Consistent refactoring to remove the Promise.resolve wrapper. The direct await pattern simplifies the code while maintaining the same behavior.


375-384: LGTM!

Same refactoring pattern as extractPagesFromChildBlock. The code is cleaner and more straightforward with the direct await.


390-399: LGTM!

Final function in the file follows the same consistent refactoring pattern. The removal of Promise.resolve wrappers across all four functions in this file improves code clarity.

apps/roam/package.json (1)

71-71: Update roamjs-components to a version that exists on npm registry.

The latest version available is 0.85.1, but the package.json specifies 0.85.7 which does not exist in the npm registry. Update to 0.85.1 or an earlier stable version. Regarding security, no direct vulnerabilities have been found for this package in Snyk's vulnerability database, though this does not include vulnerabilities in dependencies.

Likely an incorrect or invalid review comment.


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.

@maparent
Copy link
Collaborator Author

@maparent maparent requested a review from mdroidian December 22, 2025 21:32
@maparent maparent merged commit 8433a55 into main Dec 24, 2025
5 checks passed
@maparent maparent deleted the eng-1201-update-roamjs-components-and-use-proper-backend-query branch December 24, 2025 01:43
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.

3 participants