ENG-990 Use all pages query from supabase not using roam

[sid597]

Summary by CodeRabbit

• New Features
  • Added optional node type filtering for enhanced query control
• Bug Fixes
  • Improved node filtering logic to consider both creation and edit timestamps when determining recent changes
• Refactor
  • Migrated suggestions source from local pages to backend database retrieval

[linear]

ENG-990 Use all pages query from supabase not using roam

[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]

• ENG-800 handle update embeddings button #510 : 2 dependent PRs (#527 , #531 )
• ENG-990 Use all pages query from supabase not using roam #521 👈 (View in Graphite)
• 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

Three utilities were updated to enhance node querying and data retrieval. The Roam query in getAllDiscourseNodesSince was rewritten to compute filterTime as the maximum of node create and edit times (with defaults), applying the since filter to this combined timestamp. The hyde utility was migrated from local Roam page UID retrieval to Supabase-backed getAllNodes calls. The getAllNodes function signature was extended with an optional ofTypes parameter to enable node-type filtering.

Changes

Cohort / File(s)	Summary
Roam query utilities apps/roam/src/utils/getAllDiscourseNodesSince.ts	Rewrote Roam query: author fallback changed to "Unknown-Creator", nodeEditTime computation defaults to 0, and introduced filterTime derived as max(nodeEditTime, nodeCreateTime) to replace simple edit-time filtering with time-bound check.
Data source migration apps/roam/src/utils/hyde.ts	Replaced local Roam page UID-based retrieval with Supabase-backed getAllNodes calls; added context and client initialization with null guards; remapped fetched nodes to SuggestedNode structure using source_local_id and text fields.
Database query utilities packages/database/src/lib/queries.ts	Extended getAllNodes function signature with optional ofTypes?: string[] parameter and propagated it into getConcepts scope; minor stylistic quote normalization.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• getAllDiscourseNodesSince.ts: Time filtering logic change requires verification that max(create_time, edit_time) semantics are correct and handle defaults appropriately
• hyde.ts: Data source migration introduces Supabase client initialization and null guard logic; node mapping transformation from Roam-based to Supabase-based structure needs careful validation
• queries.ts: Ensure ofTypes parameter is correctly threaded through getConcepts and filters as intended

Possibly related PRs

• Roam: Eng-749 hyde functionality #344: Introduced the hyde module with initial page UID-based retrieval logic that is now being replaced with Supabase-backed calls in this PR
• ENG-448: Query Roam Discourse Nodes created or modified since the last update #232: Added the original getAllDiscourseNodesSince query with edit-time filtering, which is being rewritten here to use combined create/edit time logic
• ENG-833 Isolate node query functions. #427: Modified packages/database/src/lib/queries.ts with related node query utilities and filtering mechanisms that work with the ofTypes extension in this PR

Pre-merge checks

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The pull request title "ENG-990 Use all pages query from supabase not using roam" directly and accurately describes the primary change in the changeset. The most significant alteration is in apps/roam/src/utils/hyde.ts, which replaces the local Roam page UID-based retrieval (getAllPageByUidAsync) with a Supabase-backed approach (getAllNodes with Supabase client). The supporting changes to getAllDiscourseNodesSince.ts and queries.ts enhance the filtering and query capabilities to enable this transition. The title is specific, concise, and clearly communicates the main architectural shift from Roam-based to Supabase-based data retrieval.

---

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]

Actionable comments posted: 0

🧹 Nitpick comments (1)

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

465-482: Consider adding error handling for the Supabase query.

The getAllNodes call could throw if the Supabase request fails or if there are network issues. Wrapping this in a try-catch block would prevent unhandled exceptions from bubbling up and allow graceful fallback to an empty array.

Apply this diff to add error handling:

+  try {
     candidateNodesForHyde = (
       await getAllNodes({
         supabase,
         spaceId,
         fields: { content: ["source_local_id", "text"] },
         ofTypes: validTypes,
         pagination: { limit: 10000 },
       })
     )
       .map((c) => {
         const node = findDiscourseNode(c.Content?.source_local_id || "");
         return {
           uid: c.Content?.source_local_id || "",
           text: c.Content?.text || "",
           type: node ? node.type : "",
         };
       })
       .filter((n) => n.uid && n.text && n.type);
+  } catch (error) {
+    console.error("Failed to fetch nodes from Supabase:", error);
+    return [];
+  }

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d803704 and 77b4cfd.

📒 Files selected for processing (3)

• apps/roam/src/utils/getAllDiscourseNodesSince.ts (1 hunks)
• apps/roam/src/utils/hyde.ts (2 hunks)
• packages/database/src/lib/queries.ts (2 hunks)

🧰 Additional context used

🧠 Learnings (3)

📓 Common learnings

Learnt from: sid597
PR: DiscourseGraphs/discourse-graph#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.

📚 Learning: 2025-06-22T10:40:52.752Z

Learnt from: sid597
PR: DiscourseGraphs/discourse-graph#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/getAllDiscourseNodesSince.ts

📚 Learning: 2025-06-22T10:40:21.679Z

Learnt from: sid597
PR: DiscourseGraphs/discourse-graph#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

🔇 Additional comments (5)

apps/roam/src/utils/getAllDiscourseNodesSince.ts (1)

79-92: Query logic correctly implements "last modified" filtering.

The dual get-else pattern on lines 89-90 correctly handles nodes that may not have an :edit/time:

• nodeEditTime (line 89) captures the actual edit time or 0 for the result payload
• filterTime (line 90) uses edit time if present, otherwise falls back to create time for filtering

This ensures the since filter applies to whichever timestamp is more recent or available.

Based on learnings.

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

459-463: LGTM: Proper null guards for Supabase client and context.

The early returns prevent downstream null/undefined errors and gracefully degrade to empty results when the Supabase context or client is unavailable.

---

475-475: Remove this review comment - findDiscourseNode already handles empty string correctly.

The function explicitly accepts empty string as a default parameter and is designed to handle this case. When an empty uid is passed, it attempts to find a matching node and returns either the node or false—both handled gracefully by the optional chaining (node?.type) at the call site. No changes needed.

Likely an incorrect or invalid review comment.

packages/database/src/lib/queries.ts (2)

328-328: Cosmetic quote style change.

Changed single quotes to double quotes for consistency. No functional impact.

---

598-620: LGTM: Clean API extension for type filtering.

The ofTypes parameter is properly added as optional, maintains backward compatibility, and follows the established pattern used in other query functions like getNodesByType. The parameter is correctly propagated to getConcepts via the scope object.

[mdroidian]

Let's not adjust getAllNodes unless we are absolutely sure it is necessary.