-
Notifications
You must be signed in to change notification settings - Fork 2
Eng 898 create database query functions for base dg queries #474
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Eng 898 create database query functions for base dg queries #474
Conversation
Updates to Preview Branch (eng-898-create-database-query-functions-for-base-dg-queries) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
f41d756
to
7287a34
Compare
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughIntroduces relation accessor SQL functions and surfaces them in types. Expands getNodes API and adds initNodeSchemaCache. Updates step definitions to use logged-in clients, new ID-prefix semantics ($), and new query/upsert steps. Adds a comprehensive BDD feature for concept upsert/query. Minor feature table header change (@id → $id). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Tester as Cucumber Scenarios
participant Steps as stepdefs.ts
participant Query as getNodes (queries.ts)
participant DB as Supabase/Postgres
participant Fns as SQL funcs
Tester->>Steps: Given user logs in space / seeds data ($-prefixed IDs)
Steps->>Steps: substituteLocalReferences / map $local → DB ids
Steps->>DB: Upsert RPC / inserts (logged-in client)
Note right of Steps: Stores world.localRefs
Tester->>Steps: When querying nodes with parameters
Steps->>Query: getNodes({ baseNodeLocalIds, nodeAuthor, inRels... })
Query->>DB: Select Concepts/Contents with filters
alt Relation filters present
Query->>Fns: concepts_of_relation / concept_in_relations
Fns-->>Query: Related concept rows
end
DB-->>Query: Result rows (+schema_id, inner content as needed)
Query-->>Steps: Nodes list
Steps->>Steps: Sort and local-ref substitution
Steps-->>Tester: Then results should match
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/database/features/contentAccess.feature
(1 hunks)packages/database/features/queryConcepts.feature
(1 hunks)packages/database/features/step-definitions/stepdefs.ts
(10 hunks)packages/database/src/dbTypes.ts
(1 hunks)packages/database/src/lib/queries.ts
(6 hunks)packages/database/supabase/migrations/20250929154709_relation_access_functions.sql
(1 hunks)packages/database/supabase/schemas/concept.sql
(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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
packages/database/supabase/migrations/20250929154709_relation_access_functions.sql
Show resolved
Hide resolved
04d8ccc
to
a5c0e48
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, there's a lot going on here. I've made this as Request changes
just so it gets back to you and tagged in linear when re-requested for review.
Could you help me understand what the purpose of this PR is?
I see the title "create database query functions for base dg queries" and in the linear ticket there is a comment with list of 7 queries. Yet in this PR I do not see any new queries nor any reference to how these changes are related or meant to apply to those 7 queries.
Is this just the base and specific queries will be created in the future? If so, is there an additional linear ticket for that? Or are the dev's supposed to use this base query and be able to construct all 7 required queries when they need to use them? Is documentation on these additional parameters forthcoming?
If it is the case that this base query is to be used by other dev's to construct a query, could you give some examples of what that would look like for a few examples:
- discourse context: query all nodes in any relation with node x, and the relation label
- query all discourse nodes of type x
- query all discourse relations containing specific discourse node x
And the user user3 opens the Roam plugin in space s2 | ||
And Document are added to the database: | ||
| @id | _space_id | source_local_id | _author_id | created | last_modified | | ||
| $id | _space_id | source_local_id | _author_id | created | last_modified | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this didn't get prettier formatted. It is helpful to keep up the format with each pr/change because when it is missed it adds a lot of noise to the subsequent pr that applies it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it looks like this is the case with most of the files in this PR.
Please go through and ensure each file is formatted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I have to look into why prettier is not auto-applied.
In the case of the feature files, the zed formatter for gerkhin (the cucumber .feature file format) fights with prettier, and wants to right-align dates, which is... annoying but not wrong?
I will leave that part as-is for now to avoid churn.
But did apply prettier elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prettier should be applied on save.
When is the zed formatter for gerkhin applied? If it is also applied on save, I would try to turn it off so we can have a single primary formatter across all files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'll have to dig into zed configurations. Trying to punt this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got zed to auto-prettify ts files, which is good. It's still unwilling to use prettier on gherkin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need to add something like this to zed's settings file?
https://www.npmjs.com/package/prettier-plugin-gherkin/v/0.0.1
a5c0e48
to
196f3de
Compare
As to your main comment: |
ac99941
to
71e4ccf
Compare
@mdroidian lmk if you feel the documentation is adequate now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think there’s quite a bit of room for improvement from a DX perspective, but I won’t block on it.
For example — when I’m coding a component and need nodes of type x quickly, I’ll type in getConcepts
(which already requires a small context switch), and then I’m greeted with this:

At that point, I find myself asking: “Which params do I actually need to query nodes of type x?” To figure that out, I have to open getConcepts
and read through a long block of text — sorting through relation-specific params just to find what I need. The new documentation definitely helps with this, but I still think we could simplify things quite a bit. It might make sense to split it into a few smaller, more focused queries down the road.
That said, this is totally fine as a starting point.
@@ -0,0 +1,33 @@ | |||
# Designing cucumber tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... Somehow, Zed's enforced prettier settings don't agree with our prettier settings. Sigh.
Did apply prettier, will have to dig deeper into settings.
Re the number of arguments: I understand that is not ideal to have 17 parameters, only 4 of which would be recurrent across versions, but think of the combinatorics of having small functions for any subset of the other 13! |
Good point about avoiding combinatorics. I’m not advocating a pile of tiny functions—just clearer grouping and a couple of thin presets so the common queries don’t require reading the entire param wall. This is also probably a good time to consider adding zod. |
Hmmm... I do not see the need for this but you do; I think this is worth a synchronous co-design session. |
Here's an example: #488 Zod would help catch bad combinations. We can discuss in the dev meeting today. |
Base database functions for dg queries. (WIP)
Summary by CodeRabbit