Conversation
|
Updates to Preview Branch (eng-1233-group-functionality-testing) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
maparent
changed the base branch from
main
to
eng-1238-group-creation-function
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds a new Gherkin feature testing group-based content access and extends step definitions with group creation and membership steps; expands DB cleanup to remove group-related data and call admin deleteUser for group-associated accounts. (≤50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant API as App/API
participant DB
participant Admin as AdminService
TestRunner->>API: POST create-group (user in s1)
API->>DB: INSERT group row
DB-->>API: group_id
API-->>TestRunner: group_id
TestRunner->>API: POST add-space-to-group (s1 adds s2)
API->>DB: INSERT group_membership (account_local_id -> group_id)
DB-->>API: success
API-->>TestRunner: success
TestRunner->>API: create/publish content to group
API->>DB: INSERT content + ContentAccess mappings
DB-->>API: success
API-->>TestRunner: success
TestRunner->>API: GET content visibility (as user in s2)
API->>DB: CHECK ContentAccess / group membership
DB-->>API: visibility true/false
API-->>TestRunner: visibility result
Note right of Admin: Cleanup
TestRunner->>Admin: admin.deleteUser (for group-associated accounts)
Admin->>DB: delete related rows
DB-->>Admin: success
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 3
🤖 Fix all issues with AI agents
In @packages/database/features/step-definitions/stepdefs.ts:
- Around line 67-73: The Set of group IDs built from r3.data may include nulls,
so before calling client.auth.admin.deleteUser you should filter out
null/undefined values: when creating groupIds from (r3.data || []), only map and
add entries where group_id !== null && group_id !== undefined (or use
.filter(({group_id})=>group_id != null) before mapping), then iterate that
filtered set and call client.auth.admin.deleteUser(id) as before and assert
ur.error is null.
- Around line 406-419: The handler for When("user of space {word} creates group
{word}") accesses response.data!.group_id without verifying response.data; add a
null/undefined check on response.data (and on response.data.group_id) after the
invoke call and before assigning to localRefs so you don't use a non-null
assertion. If data is missing, assert.fail or throw a clear error (e.g.,
referencing response.error and response) so failures are explicit; update the
assignment to localRefs[name] only after confirming response.data.group_id
exists.
- Around line 74-79: The .not filter is using the string 'null' instead of the
JavaScript null which makes the Supabase/PostgREST query incorrect; update the
query call on client.from("PlatformAccount").select(...) to use
.not('dg_account', 'is', null) so the filter checks for actual NULLs, then keep
the subsequent loop that calls client.auth.admin.deleteUser(dg_account!) and the
assertions as-is.
🧹 Nitpick comments (1)
packages/database/features/step-definitions/stepdefs.ts (1)
430-441: Remove unused client1 variable.
client1is created on line 430 but never used. Onlyclient2is needed to query the member'sdg_account, and thenclient1is used again to insert the group membership. This works, but the initialclient1assignment on line 430 serves no purpose.♻️ Simplify by removing unused initial assignment
- const client1 = await getLoggedinDatabase(space1Id as number); const client2 = await getLoggedinDatabase(space2Id as number); const r1 = await client2.from("PlatformAccount").select("dg_account").eq("account_local_id", spaceAnonUserEmail("Roam", space2Id)).maybeSingle(); assert.equal(r1.error, null); const memberId = r1.data?.dg_account; assert(!!memberId); + const client1 = await getLoggedinDatabase(space1Id as number); const r2 = await client1.from("group_membership").insert({ group_id: groupId, member_id: memberId! }); assert.equal(r2.error, null);
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/database/features/groupAccess.featurepackages/database/features/step-definitions/stepdefs.ts
🧰 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/features/step-definitions/stepdefs.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). (3)
- GitHub Check: Supabase Preview
- GitHub Check: Supabase Preview
- GitHub Check: Supabase Preview
🔇 Additional comments (2)
packages/database/features/groupAccess.feature (1)
1-36: LGTM! Well-structured test scenario.The feature file clearly defines the group content access workflow with appropriate acceptance criteria. The test scenario correctly validates that content remains isolated until explicitly published to a group via ContentAccess mapping, which aligns with the user story.
packages/database/features/step-definitions/stepdefs.ts (1)
407-407: Type change in localRefs - ensure downstream compatibility.The
localRefstype has been widened fromRecord<string, number>toRecord<string, number|string>to accommodate group IDs (UUIDs). Ensure that all existing code accessinglocalRefshandles this correctly, particularly code that assumes numeric values.#!/bin/bash # Description: Find all usages of localRefs to ensure they handle the union type correctly # Search for localRefs assignments and accesses rg -n "localRefs\[" packages/database/features/step-definitions/stepdefs.ts -C 2
maparent
force-pushed
the
eng-1238-group-creation-function
branch
from
985452a to
7657610
Compare
maparent
force-pushed
the
eng-1233-group-functionality-testing
branch
3 times, most recently
from
73844a0 to
cc5c6ad
Compare
maparent
changed the base branch from
main
to
eng-1311-unify-conceptaccess-and-contentaccess-tables
maparent
force-pushed
the
eng-1233-group-functionality-testing
branch
2 times, most recently
from
67ca15e to
3dda5e8
Compare
maparent
force-pushed
the
eng-1311-unify-conceptaccess-and-contentaccess-tables
branch
2 times, most recently
from
c306427 to
4e8f0a4
Compare
maparent
force-pushed
the
eng-1233-group-functionality-testing
branch
4 times, most recently
from
4ca362b to
f0d930e
Compare
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/database/features/groupAccess.feature`:
- Around line 17-36: The scenario is incorrectly declared as "Scenario Outline"
without any Examples table; change the keyword to "Scenario" in the feature
("Creating content") so the test runner treats it as a normal scenario (update
the header line containing "Scenario Outline: Creating content" to "Scenario:
Creating content").
🧹 Nitpick comments (2)
packages/database/features/step-definitions/stepdefs.ts (2)
406-421: Good improvement with the null assertion, but consider validatinggroup_idspecifically.The addition of
assert.notStrictEqual(response.data, null)on line 415 addresses the past review concern. However,response.data.group_idcould theoretically be missing. Consider a more explicit check:🔧 Optional: More explicit validation
const response = await client.functions.invoke<{group_id: string}>("create-group", {body:{name}}); assert.equal(response.error, null); - assert.notStrictEqual(response.data, null); - localRefs[name] = response.data!.group_id; + assert.ok(response.data?.group_id, "create-group response missing group_id"); + localRefs[name] = response.data.group_id;
423-445: Consider validating before type assertion for consistency.The type assertions on lines 426-428 occur before the undefined checks on lines 429-431. While this works at runtime, it's inconsistent with the pattern used elsewhere (e.g., lines 408-410) and can be misleading. Also,
assert(!!memberId)on line 436 could benefit from a descriptive message.🔧 Suggested pattern alignment
When("user of space {word} adds space {word} to group {word}", async (space1Name: string, space2Name:string, groupName: string)=>{ const localRefs = (world.localRefs || {}) as Record<string, number|string>; - const space1Id = localRefs[space1Name] as number; - const space2Id = localRefs[space2Name] as number; - const groupId = localRefs[groupName] as string; - if (space1Id === undefined) assert.fail("space1Id"); - if (space2Id === undefined) assert.fail("space2Id"); - if (groupId === undefined) assert.fail("groupId"); + const space1Id = localRefs[space1Name]; + const space2Id = localRefs[space2Name]; + const groupId = localRefs[groupName]; + if (space1Id === undefined) assert.fail("space1Id not found"); + if (space2Id === undefined) assert.fail("space2Id not found"); + if (groupId === undefined) assert.fail("groupId not found"); - const client2 = await getLoggedinDatabase(space2Id); + const client2 = await getLoggedinDatabase(space2Id as number); const r1 = await client2.from("PlatformAccount").select("dg_account").eq("account_local_id", spaceAnonUserEmail("Roam", space2Id)).maybeSingle(); assert.equal(r1.error, null); const memberId = r1.data?.dg_account; - assert(!!memberId); - const client1 = await getLoggedinDatabase(space1Id); + assert.ok(memberId, "memberId not found for space2"); + const client1 = await getLoggedinDatabase(space1Id as number); const r2 = await client1.from("group_membership").insert({ /* eslint-disable `@typescript-eslint/naming-convention` */ - group_id: groupId, + group_id: groupId as string, member_id: memberId /* eslint-enable `@typescript-eslint/naming-convention` */ });
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
maparent
force-pushed
the
eng-1233-group-functionality-testing
branch
from
e79d62a to
84310c1
Compare
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
@CodeRabbit review |
maparent
changed the base branch from
main
to
eng-1355-allow-partial-space-access-for-shared-data
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 3 new potential issues.
🔴 3 issues in files not directly in the diff
🔴 SQL function references non-existent column sa.editor after column was replaced with permissions (packages/database/supabase/schemas/account.sql:209)
The upsert_account_in_space function in account.sql references sa.editor on line 209, but the editor column on SpaceAccess table has been replaced with a permissions column of type SpaceAccessPermissions enum.
Click to expand
Cause
The schema file was updated to use permissions column instead of editor, but the ON CONFLICT DO UPDATE SET clause in the function still references the old sa.editor column:
DO UPDATE SET permissions = CASE
WHEN COALESCE(local_account.space_editor, sa.editor, true) THEN 'editor'
ELSE 'reader' END;The sa.editor reference should be sa.permissions = 'editor' or similar logic.
Impact
This will cause a PostgreSQL error when the upsert_account_in_space function is called during an ON CONFLICT update scenario, as it tries to reference a column that doesn't exist. This breaks account upsert functionality.
Expected behavior
The function should reference sa.permissions instead of sa.editor.
Recommendation: Replace sa.editor with sa.permissions = 'editor' to check if current permissions are 'editor', e.g.: WHEN COALESCE(local_account.space_editor, sa.permissions = 'editor', true) THEN 'editor'
🔴 SQL migration references non-existent column sa.editor before dropping it (packages/database/supabase/migrations/20260124215014_PartialSpaceAccess.sql:41)
The migration file creates a new upsert_account_in_space function that references sa.editor on line 41, but this migration also drops the editor column later (line 303). The function will fail after the migration completes.
Click to expand
Cause
The migration at 20260124215014_PartialSpaceAccess.sql defines the upsert_account_in_space function (lines 13-55) that references sa.editor in the ON CONFLICT DO UPDATE SET clause:
DO UPDATE SET permissions = CASE
WHEN COALESCE(local_account.space_editor, sa.editor, true) THEN 'editor'
ELSE 'reader' END;Later in the same migration (line 303), the editor column is dropped:
ALTER TABLE public."SpaceAccess" DROP COLUMN editor;Impact
After the migration runs, any call to upsert_account_in_space that triggers the ON CONFLICT path will fail with a column not found error.
Recommendation: Replace sa.editor with sa.permissions = 'editor' to check the permissions column instead of the dropped editor column.
🔴 Missing changed = true when migrating settings with missing id (apps/obsidian/src/index.ts:356-358)
In migrateSettings, when an object is missing an id field and one is generated, the changed flag is not set to true, causing the settings not to be saved.
Click to expand
Cause
The migrateSettings function sets changed = true when created or modified fields are missing, but not when id is missing:
if (!typeObject.id) {
typeObject.id = generateUid("rel3");
// Missing: changed = true;
}Compare with the handling of created and modified which correctly set changed = true:
if (!typeObject.created) {
typeObject.created = now;
changed = true; // This is present
}Impact
If a settings object only has a missing id (but has created and modified), the generated ID will be assigned to the in-memory object but not persisted. On subsequent loads, a new ID will be generated, leading to potential data inconsistencies.
Recommendation: Add changed = true; after setting the id: if (!typeObject.id) { typeObject.id = generateUid("rel3"); changed = true; }
View issues and 8 additional flags in Devin Review.
maparent
force-pushed
the
eng-1233-group-functionality-testing
branch
from
97b6e10 to
3474940
Compare
| ELSE 'reader' END) | ||
| ON CONFLICT (space_id, account_uid) | ||
| DO UPDATE SET permissions = CASE | ||
| WHEN COALESCE(local_account.space_editor, sa.editor, true) THEN 'editor' |
There was a problem hiding this comment.
🔴 SQL migration references non-existent column sa.editor before dropping it
The migration file creates a new upsert_account_in_space function that references sa.editor on line 41, but this migration also drops the editor column later (line 303). The function will fail after the migration completes.
Click to expand
Cause
The migration at 20260124215014_PartialSpaceAccess.sql defines the upsert_account_in_space function (lines 13-55) that references sa.editor in the ON CONFLICT DO UPDATE SET clause:
DO UPDATE SET permissions = CASE
WHEN COALESCE(local_account.space_editor, sa.editor, true) THEN 'editor'
ELSE 'reader' END;Later in the same migration (line 303), the editor column is dropped:
ALTER TABLE public."SpaceAccess" DROP COLUMN editor;Impact
After the migration runs, any call to upsert_account_in_space that triggers the ON CONFLICT path will fail with a column not found error.
Recommendation: Replace sa.editor with sa.permissions = 'editor' to check the permissions column instead of the dropped editor column.
Was this helpful? React with 👍 or 👎 to provide feedback.
| ON CONFLICT (space_id, account_uid) | ||
| DO UPDATE SET editor = COALESCE(local_account.space_editor, sa.editor, true); | ||
| DO UPDATE SET permissions = CASE | ||
| WHEN COALESCE(local_account.space_editor, sa.editor, true) THEN 'editor' |
There was a problem hiding this comment.
🔴 SQL function references non-existent column sa.editor after column was replaced with permissions
The upsert_account_in_space function in account.sql references sa.editor on line 209, but the editor column on SpaceAccess table has been replaced with a permissions column of type SpaceAccessPermissions enum.
Click to expand
Cause
The schema file was updated to use permissions column instead of editor, but the ON CONFLICT DO UPDATE SET clause in the function still references the old sa.editor column:
DO UPDATE SET permissions = CASE
WHEN COALESCE(local_account.space_editor, sa.editor, true) THEN 'editor'
ELSE 'reader' END;The sa.editor reference should be sa.permissions = 'editor' or similar logic.
Impact
This will cause a PostgreSQL error when the upsert_account_in_space function is called during an ON CONFLICT update scenario, as it tries to reference a column that doesn't exist. This breaks account upsert functionality.
Expected behavior
The function should reference sa.permissions instead of sa.editor.
Recommendation: Replace sa.editor with sa.permissions = 'editor' to check if current permissions are 'editor', e.g.: WHEN COALESCE(local_account.space_editor, sa.permissions = 'editor', true) THEN 'editor'
Was this helpful? React with 👍 or 👎 to provide feedback.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| .order("last_modified", { ascending: false }) | ||
| .limit(1) | ||
| .maybeSingle(); | ||
| return new Date((data?.last_modified || DEFAULT_TIME) + "Z"); |
There was a problem hiding this comment.
Same string concatenation bug with Date object as line 178. When data?.last_modified is null/undefined, attempting to concatenate the Date object DEFAULT_TIME with "Z" produces an invalid date string.
Fix:
return new Date(data?.last_modified ? data.last_modified + "Z" : DEFAULT_TIME);| return new Date((data?.last_modified || DEFAULT_TIME) + "Z"); | |
| return new Date(data?.last_modified ? data.last_modified + "Z" : DEFAULT_TIME); |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
| DO UPDATE SET permissions = CASE | ||
| WHEN COALESCE(local_account.space_editor, sa.editor, true) THEN 'editor' | ||
| ELSE 'reader' END; |
There was a problem hiding this comment.
Critical SQL error: Function references column sa.editor which is dropped at line 303 of this migration. After the migration completes, any call to upsert_account_in_space() will fail with a column not found error.
Fix: Remove reference to the dropped column:
DO UPDATE SET permissions = CASE
WHEN COALESCE(local_account.space_editor, true) THEN 'editor'
ELSE 'reader' END;| DO UPDATE SET permissions = CASE | |
| WHEN COALESCE(local_account.space_editor, sa.editor, true) THEN 'editor' | |
| ELSE 'reader' END; | |
| DO UPDATE SET permissions = CASE | |
| WHEN COALESCE(local_account.space_editor, true) THEN 'editor' | |
| ELSE 'reader' END; |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
maparent
force-pushed
the
eng-1355-allow-partial-space-access-for-shared-data
branch
from
f525be4 to
5ca25e3
Compare
maparent
force-pushed
the
eng-1233-group-functionality-testing
branch
from
3474940 to
9fad6d7
Compare
maparent
force-pushed
the
eng-1355-allow-partial-space-access-for-shared-data
branch
from
5ca25e3 to
3203c41
Compare
maparent
force-pushed
the
eng-1233-group-functionality-testing
branch
2 times, most recently
from
78e8e4c to
4c3ef44
Compare
maparent
force-pushed
the
eng-1355-allow-partial-space-access-for-shared-data
branch
from
3ad7cc2 to
21f4862
Compare
maparent
force-pushed
the
eng-1233-group-functionality-testing
branch
from
4c3ef44 to
ae64dee
Compare
maparent
force-pushed
the
eng-1355-allow-partial-space-access-for-shared-data
branch
from
21f4862 to
8f3429e
Compare
maparent
force-pushed
the
eng-1233-group-functionality-testing
branch
from
ae64dee to
cf1f246
Compare
maparent
force-pushed
the
eng-1233-group-functionality-testing
branch
2 times, most recently
from
ef6df50 to
51a8bed
Compare
maparent
force-pushed
the
eng-1233-group-functionality-testing
branch
from
51a8bed to
4603ec1
Compare
1 participant
https://linear.app/discourse-graphs/issue/ENG-1233/group-functionality-testing
Verify that publishing content to a group makes it available to group members
https://www.loom.com/share/e86816cb51cd4646847cdadb33bca006
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.