ENG-960 Improve DX for base DG queries #488
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@mdroidian I made a lot of changes. Ideally you should review them, but it seems I cannot make you a reviewer since you're the author. Should I make another branch? |
Looks like it still shows up in my queue, so we are good. |
maparent
force-pushed
the
eng-960-improve-dx-for-base-dg-queries
branch
2 times, most recently
from
3a4244c to
745edfc
Compare
maparent
force-pushed
the
eng-960-improve-dx-for-base-dg-queries
branch
from
3c741ae to
bffce97
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThis PR refactors the concept querying API from ad-hoc parameters to a unified, scope-based structure. New types and helper functions are introduced to provide structured filtering, field selection, and pagination. Existing call sites are updated to use the new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes The refactor introduces a new grouped API surface with multiple new types and functions, requiring verification across call sites and test scenarios. While many test updates follow repetitive patterns, the core refactoring in queries.ts is dense with structural logic changes, nested type definitions, and updated internal query composition. The heterogeneous nature of changes—spanning type definitions, function signatures, internal logic, and multiple call sites—adds complexity. Possibly related PRs
Pre-merge checks✅ Passed checks (3 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/database/src/lib/queries.ts (2)
391-398: Off-by-one in pagination; Supabase range end is inclusive.range(offset, offset + limit) returns limit+1 rows.
- if (offset > 0) { - query = query.range(offset, offset + limit); - } else if (limit > 0) { + if (offset > 0) { + const to = Math.max(offset, offset + limit - 1); + query = query.range(offset, to); + } else if (limit > 0) { query = query.limit(limit); }
449-506: Cache key collision across spaces for local IDs.NODE_SCHEMA_CACHE is keyed only by source_local_id. If two spaces share a localId, mapping can resolve to the wrong dbId and yield empty results under space filters.
Suggested direction (no full patch): key by spaceId+localId, e.g.,
${spaceId}:${localId}, and adapt getSchemaConcepts/getLocalToDbIdMapping to read/write using that composite key. Alternatively, keep current keys but validate entry.spaceId === requested spaceId before using; treat mismatches as cache miss.
🧹 Nitpick comments (4)
packages/database/src/lib/queries.ts (2)
19-27: Docs reference nonexistent getRelationsFromNode.Replace with getDiscourseContext or getRelationsFromNodeType (after the fix above).
-// // Query a specific node and its relations -// const relations = await getRelationsFromNode({ -// supabase, spaceId, nodeIds: ["node-123"] -// }); +// // Query a specific node and its relations +// const context = await getDiscourseContext({ +// supabase, spaceId, nodeIds: ["node-123"] +// });
69-73: Use forEach for side effects (tiny nit).map’s return value is ignored.
- Object.keys(NODE_SCHEMA_CACHE).map((k) => { + Object.keys(NODE_SCHEMA_CACHE).forEach((k) => { if (k !== NODE_SCHEMAS) delete NODE_SCHEMA_CACHE[k]; - }); + });apps/roam/src/components/settings/AdminPanel.tsx (1)
163-169: Leverage helper API for clarity.You can simplify with getNodesByType (and a dedicated helper for schemas if desired), improving readability and consistency with the new DX helpers.
Example:
const fetch = showingSchema.sourceLocalId === nodeSchemaSignature.sourceLocalId ? getConcepts({ supabase, spaceId, scope: { schemas: true } }) : getNodesByType({ supabase, spaceId, ofType: [showingSchema.sourceLocalId] });packages/database/features/queryConcepts.feature (1)
50-114: Add pagination scenarios to guard against range off-by-one.Include cases like limit=2, offset=2 to pin the inclusive range semantics and prevent regressions.
You can add:
Scenario: Query with pagination And a user logged in space s1 and calling getConcepts with these parameters: '{"scope":{"type":"all"},"pagination":{"limit":2,"offset":2}}' Then exactly 2 results are returned
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/roam/src/components/settings/AdminPanel.tsx(1 hunks)packages/database/features/queryConcepts.feature(2 hunks)packages/database/src/lib/queries.ts(9 hunks)
| /** | ||
| * Defines what type of concepts to query and any specific constraints. | ||
| * | ||
| * @example | ||
| * ```typescript | ||
| * // Query all nodes of specific types | ||
| * { type: "nodes", ofSchemaLocal: ["page", "note"] } | ||
| * | ||
| * // Query all relations | ||
| * { type: "relations" } | ||
| * | ||
| * // Query specific nodes by their local IDs | ||
| * { type: "nodes", nodeIds: ["node-123", "node-456"] } | ||
| * | ||
| * // Query relation schemas | ||
| * { type: "relations", schemas: true } | ||
| * ``` | ||
| */ |
There was a problem hiding this comment.
JSDoc example property mismatch (‘ofSchemaLocal’ → ‘ofType’).
Example uses ofSchemaLocal, but the API is ofType: string[].
- * { type: "nodes", ofSchemaLocal: ["page", "note"] }
+ * { type: "nodes", ofType: ["page", "note"] }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Defines what type of concepts to query and any specific constraints. | |
| * | |
| * @example | |
| * ```typescript | |
| * // Query all nodes of specific types | |
| * { type: "nodes", ofSchemaLocal: ["page", "note"] } | |
| * | |
| * // Query all relations | |
| * { type: "relations" } | |
| * | |
| * // Query specific nodes by their local IDs | |
| * { type: "nodes", nodeIds: ["node-123", "node-456"] } | |
| * | |
| * // Query relation schemas | |
| * { type: "relations", schemas: true } | |
| * ``` | |
| */ | |
| /** | |
| * Defines what type of concepts to query and any specific constraints. | |
| * | |
| * @example | |
| * |
🤖 Prompt for AI Agents
In packages/database/src/lib/queries.ts around lines 104 to 121, the JSDoc
examples reference the non-existent property ofSchemaLocal but the API expects
ofType: string[]; update all examples and any explanatory text to use ofType
instead of ofSchemaLocal, adjust example values to match the ofType shape (e.g.,
ofType: ["page", "note"]) and verify the types described in the comment match
the actual function/type definitions.
| * @example | ||
| * ```typescript | ||
| * const results = await getConcepts({ | ||
| * supabase, | ||
| * spaceId: 123, | ||
| * scope: { type: "nodes", ofTypes: ["page"] }, | ||
| * relations: { ofType: ["cites"] }, | ||
| * author: "user123", | ||
| * pagination: { limit: 50 } | ||
| * }); | ||
| * ``` |
There was a problem hiding this comment.
JSDoc example typo (‘ofTypes’ → ‘ofType’).
Avoid confusing callers.
- * scope: { type: "nodes", ofTypes: ["page"] },
+ * scope: { type: "nodes", ofType: ["page"] },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * @example | |
| * ```typescript | |
| * const results = await getConcepts({ | |
| * supabase, | |
| * spaceId: 123, | |
| * scope: { type: "nodes", ofTypes: ["page"] }, | |
| * relations: { ofType: ["cites"] }, | |
| * author: "user123", | |
| * pagination: { limit: 50 } | |
| * }); | |
| * ``` | |
| * @example | |
| * |
🤖 Prompt for AI Agents
In packages/database/src/lib/queries.ts around lines 228 to 238, the JSDoc
example uses the incorrect property name `ofTypes` for the `scope` object (it
should be `ofType`), which can confuse callers; update the example to use
`scope: { type: "nodes", ofType: ["page"] }` so it matches the function's
expected parameter shape and any type definitions or runtime checks.
@maparent Looks good to me |
Example DX improvement (one of many options)
Before
After
Summary by CodeRabbit