ENG-810 Repair supabase typing #397
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughRefactors Roam utilities to use shared database types and adjust ID handling (string→number, nullable source_local_id), while switching Roam’s Octokit imports to CommonJS require. Obsidian publish script comment updated. No functional control-flow changes; edits are type/import shifts and formatting. Changes
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/roam/scripts/publish.ts (1)
43-71: Don’t require GITHUB_TOKEN for read-only git commands; make redaction optional.
execGitCommandthrows ifGITHUB_TOKENis unset even though commands likegit rev-parse HEADdon’t need it. Make the token optional and only use it for redaction.Apply:
- const token = getRequiredEnvVar("GITHUB_TOKEN"); + const token = process.env.GITHUB_TOKEN; @@ - } catch (error) { - const sanitizedError = new Error( - (error as Error).message.replace(token, "***"), - ); + } catch (error) { + const msg = (error as Error).message; + const sanitizedError = new Error( + token ? msg.replace(token, "***") : msg, + );
🧹 Nitpick comments (5)
apps/roam/scripts/publish.ts (2)
86-93: Avoid hard-coding installationId.Move
installationIdto an env var (e.g.,APP_INSTALLATION_ID) to keep this script portable across installations and environments.- installationId: 59416220, + installationId: parseInt(getRequiredEnvVar("APP_INSTALLATION_ID"), 10),
88-91: Private key newline handling (optional).If
APP_PRIVATE_KEYis provided with\nescapes, normalize before passing to Octokit to avoid auth failures.Example:
privateKey: getRequiredEnvVar("APP_PRIVATE_KEY").replace(/\\n/g, "\n")apps/roam/src/utils/cleanupOrphanedNodes.ts (1)
24-31: Null-safe mappings on joined content look solid; add one minor guard.Supabase typically returns
[], but being defensive onschemasavoids surprises.- const schemaIds = schemas.map((s: { id: number }) => s.id); + const schemaIds = (schemas ?? []).map((s: { id: number }) => s.id);Also applies to: 52-56, 75-77, 114-121
apps/roam/src/utils/upsertNodesAsContentWithEmbeddings.ts (1)
55-59: Implement bounded retries with backoff for transient embedding API errors.You already noted throttling; add lightweight retries to de-flake batch uploads.
- const response = await fetch(nextApiRoot() + "/embeddings/openai/small", { - method: "POST", - headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ input: batch }), - }); + const maxAttempts = 3; + let response: Response | undefined; + for (let attempt = 1; attempt <= maxAttempts; attempt++) { + response = await fetch(nextApiRoot() + "/embeddings/openai/small", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ input: batch }), + }); + if (response.ok) break; + const retriable = [429, 500, 502, 503, 504].includes(response.status); + if (!retriable || attempt === maxAttempts) break; + await new Promise((r) => setTimeout(r, 1000 * attempt)); + }apps/roam/src/utils/syncDgNodesToSupabase.ts (1)
52-57: Use the SYNC_FUNCTION constant for consistency.Minor consistency fix; avoids magic strings.
- s_function: "embedding", + s_function: SYNC_FUNCTION,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
apps/obsidian/scripts/publish.ts(2 hunks)apps/roam/scripts/publish.ts(1 hunks)apps/roam/src/utils/cleanupOrphanedNodes.ts(11 hunks)apps/roam/src/utils/syncDgNodesToSupabase.ts(3 hunks)apps/roam/src/utils/upsertNodesAsContentWithEmbeddings.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use the roamAlphaApi documentation from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when working with the Roam API
Applied to files:
apps/roam/scripts/publish.ts
📚 Learning: 2025-07-19T22:34:23.619Z
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/roam.mdc:0-0
Timestamp: 2025-07-19T22:34:23.619Z
Learning: Applies to apps/roam/**/*.{js,jsx,ts,tsx} : Use Roam Depot/Extension API documentation from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when working with the Roam Extension API
Applied to files:
apps/roam/scripts/publish.ts
📚 Learning: 2025-05-30T14:49:24.016Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#182
File: apps/website/app/utils/supabase/dbUtils.ts:22-28
Timestamp: 2025-05-30T14:49:24.016Z
Learning: In apps/website/app/utils/supabase/dbUtils.ts, expanding the KNOWN_EMBEDDINGS and DEFAULT_DIMENSIONS mappings to support additional embedding models requires corresponding database model changes (creating new embedding tables), which should be scoped as separate work from API route implementations.
Applied to files:
apps/roam/src/utils/upsertNodesAsContentWithEmbeddings.ts
📚 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/upsertNodesAsContentWithEmbeddings.ts
📚 Learning: 2025-07-21T14:22:20.752Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#291
File: packages/database/supabase/functions/create-space/index.ts:0-0
Timestamp: 2025-07-21T14:22:20.752Z
Learning: In the discourse-graph codebase, types.gen.ts is not accessible from Supabase edge functions, requiring duplication of types and utilities when needed in the edge function environment at packages/database/supabase/functions/.
Applied to files:
apps/roam/src/utils/cleanupOrphanedNodes.ts
📚 Learning: 2025-07-13T16:47:14.352Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#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
🔇 Additional comments (11)
apps/roam/scripts/publish.ts (1)
6-9: Correct: switch to CommonJS for Octokit in Roam runtime.Roam’s module env does require
require(...)here. LGTM.apps/obsidian/scripts/publish.ts (1)
7-10: Clarified TODO about ESM/require—good.Comment aligns with the Roam script approach and keeps behavior clear until ESM migration.
apps/roam/src/utils/cleanupOrphanedNodes.ts (4)
1-2: Type-only imports: good step toward cleaner build output.
165-172: Order of deletes is correct (concepts referencing content first), with targeted filters.No further action.
Also applies to: 177-187
221-241: Good: numeric IDs and explicit type guards prevent accidental null propagation.This will reduce runtime errors during cascaded deletes.
Also applies to: 265-270, 286-288
334-352: Document cleanup keyed by space_id is a nice touch.Prevents cross-space deletions for identical local IDs.
apps/roam/src/utils/upsertNodesAsContentWithEmbeddings.ts (2)
5-9: Strong typing via CompositeTypes is the right direction.This improves RPC payload safety.
93-100: Upsert batching and inline embeddings shape match the DB contract.RPC call site casting to
Jsonis acceptable given composite typing upstream.Also applies to: 102-121
apps/roam/src/utils/syncDgNodesToSupabase.ts (3)
22-27: Good: centralizing DB types via imports.Reduces drift across modules.
147-162: Query formatting change only—OK.Keeps query readable; no semantic shift.
314-320: Roam user fetch query tidy-up—OK.Mapping to
AccountLocalInputlooks correct.
|
@CodeRabbit: |
|
Tip For best results, initiate chat on the files or code changes. @maparent Thank you for the feedback and clarifications! I understand your approach:
Your scope management approach keeps this PR focused on the core typing improvements, which is solid practice. 🐰✨ ears perked up with understanding ✏️ Learnings added
|
|
Note: I was tempted to introduce a Nullable type to handle all the |
maparent
force-pushed
the
eng-810-repair-supabase-typing
branch
from
c0313ef to
0e75ac7
Compare
2 participants
https://linear.app/discourse-graphs/issue/ENG-810/repair-supabase-typing
Restore type imports in roam.
A few subsequent type fixes.
Mark remaining requires appropriately; none are likely to happen soon, but we want to indicate why.
Summary by CodeRabbit
No user-facing behavior changes.