ENG-1232 Create group table and use it in space access functions #661
Conversation
|
Updates to Preview Branch (eng-1232-create-group-table-and-use-it-in-space-access-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-1232-create-group-table-and-use-it-in-space-access-functions
branch
from
2ba5c38 to
edb82f9
Compare
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
maparent
force-pushed
the
eng-1232-create-group-table-and-use-it-in-space-access-functions
branch
from
edb82f9 to
07de7b5
Compare
maparent
changed the base branch from
main
to
eng-1231-migrate-spaceaccess-to-use-user-ids-not-platformaccount-ids
|
@CodeRabbit I changed the base, review from 1231 only. |
📝 WalkthroughWalkthroughIntroduces a group-based access control system with a new Changes
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 |
🧩 Analysis chain🏁 Script executed: Length of output: 996 @maparent I understand - you've changed the base branch to I'll trigger the review now. 🧠 Learnings used✅ Actions performedReview triggered.
|
![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
🧹 Nitpick comments (3)
packages/database/supabase/migrations/20260101183250_group_access.sql (3)
63-71: Potential issue:my_user_accounts()returns NULL when user is unauthenticated.
auth.uid()returnsNULLfor unauthenticated requests. TheUNIONwill include thisNULLin the result set. Functions that join onmy_user_accounts()may behave unexpectedly ifNULLis matched.Consider filtering out
NULL:🔎 Proposed fix to exclude NULL from results
CREATE OR REPLACE FUNCTION public.my_user_accounts() RETURNS SETOF UUID STABLE SECURITY DEFINER SET search_path = '' LANGUAGE sql AS $$ - SELECT auth.uid() UNION + SELECT auth.uid() WHERE auth.uid() IS NOT NULL + UNION SELECT group_id FROM public.group_membership WHERE member_id = auth.uid(); $$;
123-139: Consider adding indexes for performance on common access patterns.The
my_accountsview joinsLocalAccess,SpaceAccess, andmy_user_accounts(). For large datasets, queries could be slow without indexes ongroup_membership(member_id)andgroup_membership(group_id).🔎 Proposed indexes for group_membership
CREATE INDEX IF NOT EXISTS group_membership_member_id_idx ON public.group_membership (member_id); CREATE INDEX IF NOT EXISTS group_membership_group_id_idx ON public.group_membership (group_id);
145-145: Remove or document the commented-out ALTER TYPE.The commented-out
ALTER TYPE public."ContentVariant" ADD VALUE 'full'appears unrelated to group access. Either remove it or add a comment explaining why it's preserved here.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/database/src/dbTypes.tspackages/database/supabase/migrations/20260101183250_group_access.sqlpackages/database/supabase/schemas/account.sql
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/main.mdc)
**/*.{ts,tsx}: Use Tailwind CSS for styling where possible
When refactoring inline styles, use tailwind classes
Prefertypeoverinterfacein TypeScript
Use explicit return types for functions
Avoidanytypes when possible
Prefer arrow functions over regular function declarations
Use named parameters (object destructuring) when a function has more than 2 parameters
Use PascalCase for components and types
Use camelCase for variables and functions
Use UPPERCASE for constants
Function names should describe their purpose clearly
Prefer early returns over nested conditionals for better readability
Files:
packages/database/src/dbTypes.ts
🧠 Learnings (9)
📚 Learning: 2025-07-13T16:47:14.352Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 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-06-09T16:57:14.681Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 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/schemas/account.sqlpackages/database/supabase/migrations/20260101183250_group_access.sql
📚 Learning: 2025-07-14T05:51:46.322Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-07-14T05:51:46.322Z
Learning: The concept_in_space function in the discourse-graph project is slower than using a more direct approach with in_space(space_id) directly in RLS policies, which is why a more direct approach is sometimes preferred for performance reasons.
Applied to files:
packages/database/supabase/schemas/account.sql
📚 Learning: 2025-10-08T18:55:18.029Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 0
File: :0-0
Timestamp: 2025-10-08T18:55:18.029Z
Learning: In PostgreSQL queries within the discourse-graph project, adding `LIMIT 1` to `auth.uid()` lookups provides positive performance impact and should be retained as an optimization technique.
Applied to files:
packages/database/supabase/schemas/account.sqlpackages/database/supabase/migrations/20260101183250_group_access.sql
📚 Learning: 2025-10-15T00:12:54.623Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 482
File: packages/database/supabase/schemas/access_token.sql:43-43
Timestamp: 2025-10-15T00:12:54.623Z
Learning: In PostgreSQL RLS policies, you cannot combine multiple operations like `FOR SELECT, UPDATE, DELETE` in a single CREATE POLICY statement. Use `FOR ALL` to cover all operations, or create separate policies for each operation type (FOR SELECT, FOR INSERT, FOR UPDATE, FOR DELETE).
Applied to files:
packages/database/supabase/schemas/account.sql
📚 Learning: 2026-01-01T18:13:07.400Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 660
File: packages/database/supabase/migrations/20251231203152_migrate_space_access_to_user.sql:182-192
Timestamp: 2026-01-01T18:13:07.400Z
Learning: In PostgreSQL migrations, when a table is renamed using ALTER TABLE ... RENAME TO, the renamed table retains all of its properties including Row Level Security (RLS) settings. Therefore, if RLS was enabled on the original table, it remains enabled on the renamed table without needing to re-execute ALTER TABLE ... ENABLE ROW LEVEL SECURITY.
Applied to files:
packages/database/supabase/schemas/account.sql
📚 Learning: 2025-05-20T03:02:36.027Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/schemas/extensions.sql:1-11
Timestamp: 2025-05-20T03:02:36.027Z
Learning: The necessary schemas for Supabase extensions (like 'extensions', 'graphql', and 'vault') are automatically created by Supabase before extensions are installed, so they don't need to be explicitly created in SQL migration files.
Applied to files:
packages/database/supabase/migrations/20260101183250_group_access.sql
📚 Learning: 2025-05-20T03:01:11.220Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 165
File: packages/database/supabase/migrations/20250504195841_remote_schema.sql:1-7
Timestamp: 2025-05-20T03:01:11.220Z
Learning: Supabase automatically creates necessary schemas like the "extensions" schema before migrations run, so there's no need to explicitly create these schemas in migration files.
Applied to files:
packages/database/supabase/migrations/20260101183250_group_access.sql
📚 Learning: 2026-01-01T18:13:07.400Z
Learnt from: maparent
Repo: DiscourseGraphs/discourse-graph PR: 660
File: packages/database/supabase/migrations/20251231203152_migrate_space_access_to_user.sql:182-192
Timestamp: 2026-01-01T18:13:07.400Z
Learning: In PostgreSQL migrations, if you rename a table with ALTER TABLE ... RENAME TO, all properties including Row Level Security (RLS) settings remain on the table. If RLS was enabled before the rename, it will still be enabled afterward and does not need to be re-enabled via ALTER TABLE ... ENABLE ROW LEVEL SECURITY. Apply this guidance to any migration that uses RENAME TO in order to avoid unnecessary SQL for RLS.
Applied to files:
packages/database/supabase/migrations/20260101183250_group_access.sql
⏰ 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 (13)
packages/database/supabase/migrations/20260101183250_group_access.sql (6)
1-5: Verify thatadmindefaulting totrueis intentional.New group members will be admins by default. This is unusual—typically new members should default to non-admin (
false), with admin privileges granted explicitly. If this is intentional (e.g., the first member who creates a group should be admin), consider documenting the rationale.
20-23: Clarify the group modeling approach.Both
member_idandgroup_idreferenceauth.users. This implies groups are represented as user accounts rather than having a dedicatedgroupstable. While this can work, it conflates user and group identities. Ensure this design is intentional and consider adding a comment explaining the rationale (e.g., "Groups are modeled as special auth.users entries").
30-37: LGTM onin_groupfunction.The function correctly checks group membership for the current user with
STABLE SECURITY DEFINERand an emptysearch_path.
48-54: Consider addingLIMIT 1insidegroup_existsfor consistency.The
LIMIT 1is already present here, which aligns with the project's learning about performance optimization forauth.uid()lookups. Good practice.
56-60: RLS policies look correct for group membership control.
- SELECT: Only group members can see membership records
- DELETE: Members can remove themselves, or group admins can remove others
- INSERT: Group admins can add members, or anyone can bootstrap a new group
- UPDATE: Only group admins can modify membership
The bootstrap mechanism (
NOT public.group_exists(group_id)) allows creating new groups without requiring prior admin status.
75-93: Space access functions correctly integrate group membership.The
my_space_ids()andin_space()functions properly join withmy_user_accounts()to include group-based access. The implementation aligns with the PR objective of using the group table in space access functions.packages/database/src/dbTypes.ts (2)
535-552: Auto-generated types align with migration.The
group_membershiptable types correctly reflect the SQL schema:adminas nullable boolean,group_idandmember_idas required strings (UUIDs). The emptyRelationshipsarray is expected since foreign keys referenceauth.usersin a different schema.
1473-1474: New function signatures correctly registered.The function signatures for
group_exists,in_group,is_group_admin, andmy_user_accountsmatch the SQL function definitions.Also applies to: 1501-1501, 1527-1527
packages/database/supabase/schemas/account.sql (5)
131-157: Schema definition consistent with migration.The
group_membershiptable definition matches the migration file. The same considerations aboutadmindefaulting totrueand group modeling apply here.
263-299: Security utility functions correctly defined.The functions
my_user_accounts(),in_group(),is_group_admin(), andgroup_exists()are properly implemented withSTABLE SECURITY DEFINERand emptysearch_path. The same recommendation about filteringNULLfrommy_user_accounts()applies here as noted in the migration file.
308-308: Space access functions properly updated to usemy_user_accounts().The
my_space_ids(),in_space(),account_in_shared_space(), andunowned_account_in_shared_space()functions now correctly join withmy_user_accounts()to include group-based access. This fulfills the PR objective.Also applies to: 319-320, 334-334, 348-348
382-398:my_accountsview correctly updated for group membership.The view now joins with
my_user_accounts()to include accounts accessible through group membership, consistent with the other access functions.
468-481: RLS policies forgroup_membershipare well-structured.The policies correctly implement:
- SELECT restricted to group members
- DELETE allowed for self-removal or by group admins
- INSERT allowed for group admins or when bootstrapping a new group
- UPDATE restricted to group admins
The
DROP POLICY IF EXISTSpattern ensures idempotent schema application.
|
@CodeRabbit good point on the null uid, corrected. The member index is implicit in the primary key, and not needed otherwise. The group index would only be useful in the group_exists function, but new group insertion should be rare enough, I don't think the cost of maintaining the index is worth the speedup of this rare case. Am I missing another use case where the group index would be useful? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 9860 🏁 Script executed: Length of output: 2997 🧠 Learnings used |
|
@CodeRabbit I'm reading your analysis chain, but I did not get an answer to my question. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@maparent You're right, I apologize for not answering directly. Let me analyze the use cases for a 🏁 Script executed: Length of output: 607 🏁 Script executed: Length of output: 11288 @maparent You're not missing anything for the current implementation. Based on my analysis of the queries: Current queries on
The missing use case would be: If you ever add group management features that need to list or count all members of a specific group, such as:
These queries would scan the entire table without a If you add group administration features later, revisit the index decision at that point. 🧠 Learnings used |
|
Actually, you are right, group membership is a good enough reason. Adding the index. |
https://linear.app/discourse-graphs/issue/ENG-1232/create-group-table-and-use-it-in-space-access-functions
This is the second stage: Group table used in space access.
https://www.loom.com/share/39fdfa9ca9db45bb880f6846cd630729
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.