ENG-833 Isolate node query functions. #427
Conversation
|
Updates to Preview Branch (eng-833-isolate-node-query-functions) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
maparent
force-pushed
the
eng-833-isolate-node-query-functions
branch
from
764501f to
70c41bf
Compare
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds two SQL functions linking Concept rows (schema_of_concept, instances_of_schema), updates generated TypeScript DB types to expose them, and introduces a new typed query helper module (getNodeSchemas, getNodes) that builds/executessupabase queries returning Concepts with nested Content/Document and schema filtering. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as App code
participant Q as queries.ts
participant SB as Supabase Client
participant DB as PostgREST/DB
Note over App,Q: Retrieve Concepts (optional nested Content → Document)
App->>Q: getNodes({supabase, spaceId?, schemaUids?, fieldSets?})
Q->>Q: composeQuery(schemaUids, fieldSets, spaceId)
Q->>SB: from("Concept").select(...joins...).maybeFilter(space_id, schema filters)
SB->>DB: SQL/select (may call public.schema_of_concept / instances_of_schema via PostgREST joins)
DB-->>SB: Concept[] with nested Content/Document
SB-->>Q: rows or error
Q-->>App: PConcept[] (empty on error)
Note over App,Q: Retrieve available node schemas
App->>Q: getNodeSchemas(supabase, spaceId)
Q->>SB: from("Concept").select(id,name).eq("is_schema", true).maybeEq("space_id", spaceId)
SB->>DB: Execute select
DB-->>SB: rows
SB-->>Q: results
Q-->>App: [nodeSchemaSignature, ...results] or fallback
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks (3 passed)✅ 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: 2
🧹 Nitpick comments (5)
packages/database/supabase/schemas/concept.sql (2)
141-150: Computed relationship function is correct; consider NULL-handling contractLooks good for PostgREST computed rel. Optional: declare RETURNS NULL ON NULL INPUT (STRICT) if you prefer null input to shortcut rather than scan with NULL schema_id (today it returns empty set), and add a brief COMMENT ON FUNCTION for discoverability.
-CREATE OR REPLACE FUNCTION public.schema_of_concept(concept public."Concept") +CREATE OR REPLACE FUNCTION public.schema_of_concept(concept public."Concept") RETURNS SETOF public."Concept" STABLE ROWS 1 SET search_path = '' LANGUAGE sql AS $$ SELECT * from public."Concept" WHERE id=concept.schema_id; $$; +COMMENT ON FUNCTION public.schema_of_concept(public."Concept") + IS 'Computed one-to-one: returns the schema Concept for a given Concept (by schema_id).';
151-158: Instances function OK; add docs for symmetryImplementation matches index on schema_id; add COMMENT for introspection tools.
CREATE OR REPLACE FUNCTION public.instances_of_schema(schema public."Concept") RETURNS SETOF public."Concept" STABLE SET search_path = '' LANGUAGE sql AS $$ SELECT * from public."Concept" WHERE schema_id=schema.id; $$; +COMMENT ON FUNCTION public.instances_of_schema(public."Concept") + IS 'Computed one-to-many: returns all Concept instances that reference the given schema Concept.';packages/database/src/lib/queries.ts (3)
15-22: Export the nested payload types for reuseIf callers consume the shapes returned by getNodes, exporting PDocument and PContent avoids re-declaring these in downstream code.
-type PDocument = Partial<Tables<"Document">>; -type PContent = Partial<Tables<"Content">> & { +export type PDocument = Partial<Tables<"Document">>; +export type PContent = Partial<Tables<"Content">> & { Document: PDocument | null; };
24-38: Minor: remove redundant default of undefinedTypes already mark these optional; defaulting to undefined is noisy.
- spaceId = undefined, - schemaName = NODE_SCHEMAS, + schemaName = NODE_SCHEMAS,
63-74: Type the response instead of castingLeverage supabase generics to avoid manual casts.
- const res = (await q) as PostgrestResponse<NodeSignature>; + const res: PostgrestResponse<NodeSignature> = await q;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/database/src/dbTypes.ts(2 hunks)packages/database/src/lib/queries.ts(1 hunks)packages/database/supabase/migrations/20250909142447_utility_concept_function.sql(1 hunks)packages/database/supabase/schemas/concept.sql(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
📚 Learning: 2025-06-09T16:57:14.681Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#198
File: packages/database/supabase/migrations/20250605083319_alpha_upload.sql:157-337
Timestamp: 2025-06-09T16:57:14.681Z
Learning: Migration files in packages/database/supabase/migrations/ are historical snapshots that preserve database schema and functions as they existed at the time of creation. These files should not be updated to reflect current schema changes, even if they reference tables or columns that have since been modified or removed. Schema incompatibilities in migration files are expected and acceptable as they represent the valid state at the time of migration.
Applied to files:
packages/database/supabase/migrations/20250909142447_utility_concept_function.sql
📚 Learning: 2025-06-25T18:03:52.669Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#241
File: packages/database/tsconfig.json:3-7
Timestamp: 2025-06-25T18:03:52.669Z
Learning: The packages/database directory in the discourse-graph repository has a unique structure as a database schema/migration package. It contains doc/, scripts/, supabase/ directories and TypeScript files at the root level, but no typical src/, test/, dist/, or node_modules directories. The current tsconfig.json with "include": ["."] and "exclude": ["supabase"] is appropriate for this structure.
Applied to files:
packages/database/src/lib/queries.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:
packages/database/src/dbTypes.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Supabase Preview
- GitHub Check: Supabase Preview
🔇 Additional comments (3)
packages/database/supabase/migrations/20250909142447_utility_concept_function.sql (1)
1-17: Migration and schema definitions are in lockstep
The function bodies forschema_of_conceptandinstances_of_schemamatch exactly between the migration and the schema files—no action needed.packages/database/src/dbTypes.ts (2)
635-654: Types for instances_of_schema look consistent with SQLArgs/return types align with Concept row shape.
692-711: Types for schema_of_concept look consistent with SQLMatches Concept row fields; OK.
|
@sid597 let me know if these functions would do what you want them to |
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
I won't block on this, but I strongly suggest:
- only exporting what is intended to be used by majority consumers. (eg: consumers of
getNodesandgetNodeSchemas. If something else needs to be exported later for some other use case, we will export it at that time. - split the queries into their own files. As I mentioned, there's a lot of cognitive overhead just trying to find the queries right now
- add documentation to the queries. LLM's are really good at this :)
|
-1 on putting utility stubs in their own files, but I hear you on the cognitive overhead and will add comments. As for the generic function, @sid597 told me he wanted to test against a set of types, which is not accessible from the stubs. This is a good case of BIKINI (But I Know I Need It) against YAGNI. The real question: How do we refer to types? Db ids probably require a round-trip (but should be cached globally), names are unique in Roam by design but certainly not across spaces, so yes Uids is probably better. Will change. Not merging until we have agreement on that last point. |
|
Ah I remember why I did not use Uid. It's more fragile because of the missing content, and requires another join (which has a small performance impact.) My initial implementation using database Ids was actually the best solution… |
|
Ok, I have thought of something better... |
|
Host application |
|
And another join. I've cached the roundtrip instead, this will mean I avoid two joins, I think it's worth it. |
|
This has changed enough that I think it's worth another review. |
|
@CodeRabbit full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/database/src/lib/queries.ts (1)
151-160: Filtering by embedded Content UID is fine; note: left-join semanticsThe additional
.not("Content.source_local_id","is",null)effectively inner-joins Content for the filter. This is acceptable given prior finding that!innerdoesn’t apply to computed rels; here it’s a normal rel so either approach works.
🧹 Nitpick comments (4)
packages/database/supabase/schemas/concept.sql (2)
141-152: Computed 1:1 function looks correct; minor nit about STRICT on composite argDefinition matches PostgREST computed rel requirements and uses ROWS 1 and search_path = ''. Note STRICT on a composite arg only short-circuits when the whole record is NULL (not when schema_id is NULL), but the WHERE handles the NULL case anyway. OK to leave as-is.
153-162: instances_of_schema: planner hint optionalLooks good and benefits from the existing index on Concept(schema_id). Optionally add ROWS N (e.g., ROWS 10/100) as a planner hint if you notice suboptimal plans, but not required.
packages/database/src/lib/queries.ts (2)
124-147: UID cache is keyed only by source_local_id (cross-space collision risk)If two spaces reuse the same local UID, entries can overwrite. Consider a compound key
${spaceId}:${uid}or a two-level map keyed by spaceId.I can draft a scoped cache if desired.
31-38: Type of schema_of_concept in PConceptPConcept includes
schema_of_concept: { name: string } | null, but the current select never embeds it. Fine for future use; just noting it’ll be absent unless added to select.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
packages/database/package.json(1 hunks)packages/database/src/dbTypes.ts(2 hunks)packages/database/src/lib/queries.ts(1 hunks)packages/database/supabase/migrations/20250909142447_utility_concept_function.sql(1 hunks)packages/database/supabase/schemas/concept.sql(1 hunks)
🧰 Additional context used
🧠 Learnings (11)
📚 Learning: 2025-09-11T23:54:41.830Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#431
File: packages/database/package.json:18-22
Timestamp: 2025-09-11T23:54:41.830Z
Learning: In the discourse-graph repository's packages/database/package.json, the typesVersions mapping for "./dbDotEnv" must retain the leading "./" prefix. Removing it (as suggested by standard TypeScript documentation) fails in practice in their specific build environment/tooling setup.
Applied to files:
packages/database/package.json
📚 Learning: 2025-06-25T18:03:52.669Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#241
File: packages/database/tsconfig.json:3-7
Timestamp: 2025-06-25T18:03:52.669Z
Learning: The packages/database directory in the discourse-graph repository has a unique structure as a database schema/migration package. It contains doc/, scripts/, supabase/ directories and TypeScript files at the root level, but no typical src/, test/, dist/, or node_modules directories. The current tsconfig.json with "include": ["."] and "exclude": ["supabase"] is appropriate for this structure.
Applied to files:
packages/database/package.jsonpackages/database/src/lib/queries.ts
📚 Learning: 2025-07-19T22:34:08.725Z
Learnt from: CR
PR: DiscourseGraphs/discourse-graph#0
File: .cursor/rules/main.mdc:0-0
Timestamp: 2025-07-19T22:34:08.725Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use Prettier with the project's configuration
Applied to files:
packages/database/package.json
📚 Learning: 2025-06-09T16:57:14.681Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#198
File: packages/database/supabase/migrations/20250605083319_alpha_upload.sql:157-337
Timestamp: 2025-06-09T16:57:14.681Z
Learning: Migration files in packages/database/supabase/migrations/ are historical snapshots that preserve database schema and functions as they existed at the time of creation. These files should not be updated to reflect current schema changes, even if they reference tables or columns that have since been modified or removed. Schema incompatibilities in migration files are expected and acceptable as they represent the valid state at the time of migration.
Applied to files:
packages/database/supabase/migrations/20250909142447_utility_concept_function.sql
📚 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:
packages/database/src/dbTypes.ts
📚 Learning: 2025-09-09T15:49:21.966Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#427
File: packages/database/src/lib/queries.ts:40-47
Timestamp: 2025-09-09T15:49:21.966Z
Learning: The `!inner` join hint does not work on computed relationships in PostgREST/Supabase, despite what general documentation might suggest. This is specific behavior that differs from regular table relationships.
Applied to files:
packages/database/src/lib/queries.ts
📚 Learning: 2025-05-20T03:11:07.917Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#165
File: packages/database/supabase/migrations/20250504202930_content_tables.sql:37-45
Timestamp: 2025-05-20T03:11:07.917Z
Learning: When reviewing SQL migrations in the discourse-graph codebase, understand that historical migration files should not be modified once they're applied. Instead, issues should be fixed in subsequent migrations or schema definition files. This is why maparent indicated "migration, not changing. corrected in schema."
Applied to files:
packages/database/src/lib/queries.ts
📚 Learning: 2025-05-20T03:11:07.917Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#165
File: packages/database/supabase/migrations/20250504202930_content_tables.sql:37-45
Timestamp: 2025-05-20T03:11:07.917Z
Learning: When reviewing SQL migrations in this codebase, understand that historical migration files should not be modified once they're applied. Instead, issues should be fixed in subsequent migrations or schema definition files. This is why maparent indicated "migration, not changing. corrected in schema."
Applied to files:
packages/database/src/lib/queries.ts
📚 Learning: 2025-06-17T23:37:45.289Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#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:
packages/database/src/lib/queries.ts
📚 Learning: 2025-06-17T23:37:45.289Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#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:
packages/database/src/lib/queries.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:
packages/database/src/lib/queries.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Supabase Preview
- GitHub Check: Supabase Preview
🔇 Additional comments (6)
packages/database/package.json (1)
79-81: Pinning TS 5.9.2 — confirm toolchain & workspace resolutionCannot verify — repo scan returned "NO package.json files found". Run and paste outputs of the following locally:
- find . -name package.json -print
- find . -name package.json -exec jq -r '(.name // "") + " => typescript: " + ((.dependencies.typescript // .devDependencies.typescript // .peerDependencies.typescript) // "none")' {} +
- cat packages/database/package.json
- cd packages/database && npx tsc -v && npm run check-types
- rg -n '"typescript"' pnpm-lock.yaml yarn.lock package-lock.json || true
Confirm that tsx (≈^4.20) and ts-node-maintained (≈^10.9) used in the workspace are compatible with TS 5.9.2 and that no other package/lockfile hoists a conflicting TypeScript version. Ensure typesVersions keys retain the leading "./" in package.json.
packages/database/supabase/migrations/20250909142447_utility_concept_function.sql (1)
1-21: Migration functions must match canonical schema — verify & sync
- schema_of_concept: identical.
- instances_of_schema: packages/database/supabase/schemas/concept.sql contains an extra private function (public._local_concept_to_db_concept) immediately after instances_of_schema that is missing from packages/database/supabase/migrations/20250909142447_utility_concept_function.sql. Confirm whether the migration should include that helper or the schema should be adjusted so the migration and canonical schema stay in lockstep.
packages/database/src/dbTypes.ts (2)
635-654: Types for instances_of_schema: OKArgs/return shape align with the SQL function returning Concept rows. No action needed.
692-711: Types for schema_of_concept: OK (one-to-one modeled as SETOF)Even though it’s logically 1:1, SETOF works best for PostgREST computed embeds; typing as an array is fine.
packages/database/src/lib/queries.ts (2)
63-83: Confirm arity=0 is intended for both schemas and instancescomposeQuery hard-filters arity to 0 for all modes. If some schema concepts or instances can have non-zero arity, they’ll be excluded.
Would you like this to be configurable (e.g., optional arity filter) or is “node == arity 0” an invariant here?
85-120: getNodeSchemas fallback path depends on cache fixOnce the cache key bug is fixed, the fallback to the built-in Node types will work. Current logic otherwise looks good.
After fixing the cache key, please re-run a smoke query to ensure the fallback returns
[nodeSchemaSignature]on error.
…efficiency. Some typechecks now require a newer typescript.
maparent
force-pushed
the
eng-833-isolate-node-query-functions
branch
from
10789bb to
13fe4f2
Compare
maparent
force-pushed
the
eng-833-isolate-node-query-functions
branch
from
36308c0 to
d865ad4
Compare
maparent
force-pushed
the
eng-833-isolate-node-query-functions
branch
from
d865ad4 to
05df276
Compare
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
There's a few "re-opened" comments that should be address, but I'll approve so it can be merged after.
|
pushed a nit, waiting for your response to my latest comments. |
|
responded |
* Utility query functions for the database * Use localIds as keys for node queries. Cache corresponding dbIds for efficiency. Some typechecks now require a newer typescript.
https://linear.app/discourse-graphs/issue/ENG-833/isolate-node-query-functions
Summary by CodeRabbit
New Features
Chores