Eng 403 refactor accounts #189
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis change implements a major refactor of the database schema and API routes, replacing the previous agent and account model with a unified "PlatformAccount" and "AgentIdentifier" system. It updates all related API endpoints, schema files, type definitions, and migrations to align with the new structure, removing obsolete entities and relationships. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Supabase
Note over API: PlatformAccount POST flow
Client->>API: POST /platform_account (with JSON)
API->>API: Validate input
alt Validation fails
API-->>Client: 400 Bad Request (error)
else Validation passes
API->>Supabase: Insert or get PlatformAccount
Supabase-->>API: PlatformAccount record
API-->>Client: 200 OK (PlatformAccount)
end
sequenceDiagram
participant Client
participant API
participant Supabase
Note over API: AgentIdentifier POST flow
Client->>API: POST /agent_identifier (with JSON)
API->>API: Validate input
alt Validation fails
API-->>Client: 400 Bad Request (error)
else Validation passes
API->>Supabase: Insert or get AgentIdentifier
Supabase-->>API: AgentIdentifier record
API-->>Client: 200 OK (AgentIdentifier)
end
Possibly related PRs
Suggested reviewers
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
maparent
force-pushed
the
ENG-403-refactor-accounts
branch
from
95c1a17 to
2457e58
Compare
|
@CodeRabbit review |
✅ 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: 11
🧹 Nitpick comments (3)
apps/website/app/api/supabase/agent_identifier/route.ts (1)
28-29: Consider more precise account_id validation.While functionally correct, the validation could be more specific for database IDs.
Consider this improvement:
- if (!account_id || typeof account_id !== "number") + if (!account_id || typeof account_id !== "number" || !Number.isInteger(account_id) || account_id <= 0) return "Missing or invalid account_id";apps/website/app/api/supabase/platform_account/route.ts (1)
31-39: Consider accepting metadata as an object instead of a JSON string.The current implementation requires metadata to be a JSON string, which adds unnecessary serialization/deserialization overhead. Since the database stores it as JSONB, consider accepting it directly as an object.
If you need to keep the string format for backward compatibility, at least improve the error handling:
if (metadata !== undefined) { - if (typeof metadata != 'string') - return "metadata should be a JSON string"; - else try { - JSON.parse(metadata) - } catch (error) { - return "metadata should be a JSON string"; - } + if (typeof metadata === 'string') { + try { + JSON.parse(metadata); + } catch (error) { + return "metadata contains invalid JSON"; + } + } else if (typeof metadata === 'object' && metadata !== null) { + // Accept object directly + } else { + return "metadata should be a JSON string or object"; + } }packages/database/supabase/migrations/20250603144146_account_centric.sql (1)
179-180: Consider adding existence checks before dropping tables.While the migration order is correct, consider adding
IF EXISTSclauses to the DROP TABLE statements for safety:-DROP TABLE public."Agent"; +DROP TABLE IF EXISTS public."Agent";This would make the migration more idempotent and safer to re-run if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/database/schema.svgis excluded by!**/*.svg
📒 Files selected for processing (21)
apps/website/app/api/supabase/account/route.ts(0 hunks)apps/website/app/api/supabase/agent_identifier/[id].ts(1 hunks)apps/website/app/api/supabase/agent_identifier/route.ts(1 hunks)apps/website/app/api/supabase/person/[id].ts(0 hunks)apps/website/app/api/supabase/person/route.ts(0 hunks)apps/website/app/api/supabase/platform/route.ts(0 hunks)apps/website/app/api/supabase/platform_account/[id].ts(1 hunks)apps/website/app/api/supabase/platform_account/route.ts(1 hunks)apps/website/app/api/supabase/space/route.ts(2 hunks)packages/database/schema.puml(3 hunks)packages/database/schema.yaml(11 hunks)packages/database/supabase/config.toml(0 hunks)packages/database/supabase/migrations/20250603144146_account_centric.sql(1 hunks)packages/database/supabase/schemas/account.sql(2 hunks)packages/database/supabase/schemas/agent.sql(0 hunks)packages/database/supabase/schemas/base.sql(2 hunks)packages/database/supabase/schemas/concept.sql(1 hunks)packages/database/supabase/schemas/content.sql(2 hunks)packages/database/supabase/schemas/contributor.sql(2 hunks)packages/database/supabase/schemas/space.sql(1 hunks)packages/database/types.gen.ts(14 hunks)
💤 Files with no reviewable changes (6)
- packages/database/supabase/config.toml
- apps/website/app/api/supabase/platform/route.ts
- apps/website/app/api/supabase/account/route.ts
- apps/website/app/api/supabase/person/route.ts
- apps/website/app/api/supabase/person/[id].ts
- packages/database/supabase/schemas/agent.sql
🧰 Additional context used
🧬 Code Graph Analysis (3)
apps/website/app/api/supabase/agent_identifier/[id].ts (2)
apps/website/app/utils/supabase/apiUtils.ts (3)
makeDefaultGetHandler(92-113)defaultOptionsHandler(79-84)makeDefaultDeleteHandler(118-136)apps/website/app/api/supabase/agent_identifier/route.ts (1)
OPTIONS(56-56)
apps/website/app/api/supabase/platform_account/[id].ts (3)
apps/website/app/api/supabase/agent_identifier/[id].ts (3)
GET(7-7)OPTIONS(9-9)DELETE(11-11)apps/website/app/utils/supabase/apiUtils.ts (3)
makeDefaultGetHandler(92-113)defaultOptionsHandler(79-84)makeDefaultDeleteHandler(118-136)apps/website/app/api/supabase/platform_account/route.ts (1)
OPTIONS(76-76)
apps/website/app/api/supabase/agent_identifier/route.ts (6)
packages/database/types.gen.ts (1)
TablesInsert(597-618)apps/website/app/utils/supabase/dbUtils.ts (2)
ItemValidator(169-169)getOrCreateEntity(83-156)apps/website/app/api/supabase/platform_account/route.ts (2)
POST(53-74)OPTIONS(76-76)apps/website/app/utils/supabase/server.ts (1)
createClient(5-47)apps/website/app/utils/supabase/apiUtils.ts (4)
createApiResponse(20-48)asPostgrestFailure(138-156)handleRouteError(54-74)defaultOptionsHandler(79-84)apps/website/app/api/supabase/agent_identifier/[id].ts (1)
OPTIONS(9-9)
🔇 Additional comments (28)
apps/website/app/api/supabase/agent_identifier/[id].ts (2)
7-7: LGTM: Table reference updated to AgentIdentifier.The change from "Account" to "AgentIdentifier" table correctly aligns with the database schema refactor objectives.
11-11: LGTM: Consistent table reference update.The DELETE handler correctly uses the same "AgentIdentifier" table, maintaining consistency with the GET handler.
packages/database/supabase/schemas/base.sql (2)
40-40: LGTM: Added helpful documentation.The comment clarifying the purpose of the EntityType enum improves code documentation.
27-27:✅ Verification successful
Verify enum usage consistency across the codebase.
The change from
'Account'to'PlatformAccount'in the EntityType enum is fundamental and affects all code that references this enum value.
🏁 Script executed:
#!/bin/bash # Description: Verify all references to 'Account' EntityType have been updated to 'PlatformAccount' # Expected: Only references to 'PlatformAccount' enum value, no remaining 'Account' enum references echo "Searching for 'Account' EntityType references..." rg -A 3 -B 3 "EntityType.*Account" --type sql --type ts --type js echo "Searching for 'PlatformAccount' EntityType references..." rg -A 3 -B 3 "EntityType.*PlatformAccount" --type sql --type ts --type jsLength of output: 2635
Enum ‘Account’ → ‘PlatformAccount’ references verified
I confirmed that the only occurrences of the old
'Account'value are in the initial migration (20250504202930_content_tables.sql) and that the subsequent migration (20250603144146_account_centric.sql) renames it to'PlatformAccount'. No other SQL, TS, or JS files reference the old value—everything will resolve toPlatformAccountat runtime.packages/database/supabase/schemas/concept.sql (1)
73-73:✅ Verification successful
Verify foreign key reference update is coordinated across the system.
The foreign key constraint change from "Agent" to "PlatformAccount" table is a breaking change that affects how Concept authors are referenced throughout the system.
🏁 Script executed:
#!/bin/bash # Description: Verify all code that queries Concept.author_id handles PlatformAccount references # Expected: Code should join with PlatformAccount table, not Agent table echo "Searching for queries that join Concept with author references..." rg -A 5 -B 5 "Concept.*author|author.*Concept" --type sql --type ts --type js echo "Searching for remaining Agent table references..." rg -A 3 -B 3 "Agent\b" --type sql --type ts --type jsLength of output: 19427
Foreign key reference to PlatformAccount fully integrated
All code paths now reference PlatformAccount instead of Agent and migrations properly deprecate the old Agent table. No residual joins or type definitions remain.
- concept.sql’s FK on author_id updated to PlatformAccount (lines 73–73).
- Generated types (packages/database/types.gen.ts) reference PlatformAccount; “Agent” is no longer present.
- Migrations run in order: older content tables use Agent, then 20250603144146_account_centric.sql drops Agent and rebinds FKs to PlatformAccount.
No further changes needed—this breaking refactor is correctly coordinated across SQL schemas, migrations, and TS types.
apps/website/app/api/supabase/platform_account/[id].ts (2)
7-7: LGTM: Table reference updated to PlatformAccount.The change from "Platform" to "PlatformAccount" table correctly implements the consolidation of platform and account entities.
11-11: LGTM: Consistent handler pattern maintained.The DELETE handler correctly uses the same "PlatformAccount" table, following the established pattern from other API routes in this refactor.
packages/database/supabase/schemas/contributor.sql (1)
17-19: LGTM! Foreign key updates are correct.The foreign key constraint updates properly reference the new
PlatformAccounttable instead of the removedAgenttable, maintaining the existing cascade behaviors for referential integrity.Also applies to: 37-39
apps/website/app/api/supabase/space/route.ts (1)
20-20: LGTM! Platform enum migration is correctly implemented.The destructuring and data insertion correctly use the new
platformenum field instead of the numericplatform_id.Also applies to: 38-38, 52-52
packages/database/supabase/schemas/space.sql (2)
1-6: LGTM! Platform enum creation follows best practices.The PostgreSQL enum type creation is correctly structured with appropriate values and ownership assignment.
14-14: LGTM! Space table correctly uses the new Platform enum.The column definition properly references the new enum type, eliminating the need for a foreign key to a separate Platform table.
apps/website/app/api/supabase/agent_identifier/route.ts (2)
43-48: LGTM! Entity creation logic follows established patterns.The
getOrCreateEntitycall correctly uses the unique constraint on value, identifier_type, and account_id to prevent duplicates while following the established API pattern.
33-54: LGTM! API route structure is consistent and well-implemented.The POST handler and OPTIONS export follow the established patterns from other API routes, with proper error handling and response creation.
Also applies to: 56-56
packages/database/supabase/schemas/content.sql (1)
34-36: Foreign key updates align with the PlatformAccount refactor.The foreign key constraint updates from
AgenttoPlatformAccountare consistent across all author and creator references. The cascade behaviors (ON DELETE CASCADEfor Document.author_id,ON DELETE SET NULLfor Content author/creator) are appropriate for maintaining referential integrity.Also applies to: 86-88, 91-93
packages/database/supabase/schemas/account.sql (2)
21-34: Schema design looks solid with appropriate constraints.The
PlatformAccounttable is well-designed with:
- Appropriate NOT NULL constraints on required fields
- Good default values for boolean flags
- Proper foreign key to auth.users with ON DELETE SET NULL
- Unique index on (account_local_id, platform) for preventing duplicates
47-53: Verify if multiple accounts should share the same identifier value.The composite primary key
(value, identifier_type, account_id)allows multiple accounts to have the same identifier value (e.g., same email address). Is this intentional?If identifiers should be unique across all accounts, consider adding a unique constraint:
CREATE UNIQUE INDEX agent_identifier_unique_value_idx ON public."AgentIdentifier" (value, identifier_type);packages/database/schema.puml (1)
5-96: Well-structured entity relationship diagram.The PlantUML diagram clearly shows the refactored schema with:
- Clear separation of concerns between PlatformAccount and AgentIdentifier
- Proper cardinality notation for relationships
- Good use of immutability markers (i) in ConceptSchema
- Comprehensive field definitions
packages/database/types.gen.ts (4)
12-37: LGTM! Well-structured identifier table.The
AgentIdentifiertable provides a flexible way to store multiple identifiers per account, supporting email and ORCID types with proper foreign key constraints toPlatformAccount.
89-90: Consistent foreign key updates across all tables.All foreign key relationships have been properly updated from
Agent/AccounttoPlatformAccount, maintaining referential integrity throughout the schema.Also applies to: 140-141, 193-194, 200-201, 251-252, 324-325, 414-415
372-392: Platform enum correctly replaces foreign key.The
Spacetable properly uses the newPlatformenum instead of a foreign key reference, which aligns with the removal of thePlatformtable.
517-518: Well-defined enums support the new schema.The new enums (
AgentIdentifierType,AgentType,Platform) and the updatedEntityTypeenum properly support the refactored schema structure.Also applies to: 676-677, 677-678, 710-711
packages/database/schema.yaml (2)
158-174: Well-designed identifier storage structure.The
AgentIdentifierclass provides a flexible and normalized way to store multiple identifiers per account with proper uniqueness constraints.
192-192: Consistent slot range updates throughout schema.All slot ranges have been properly updated from
Agent/AccounttoPlatformAccount, maintaining consistency with the new entity model.Also applies to: 390-390, 394-394, 399-399
packages/database/supabase/migrations/20250603144146_account_centric.sql (5)
1-29: Well-structured initial migration steps.The enum creation and table rename operations are properly ordered, preserving existing data while setting up the new schema structure.
30-47: Proper column additions with sensible defaults.The new columns added to
PlatformAccounthave appropriate defaults, ensuring existing data remains valid during the migration.
70-79: Correct foreign key migration pattern.The
SpaceAccessforeign key is properly migrated by first updating the data, then recreating the constraint with appropriate cascade behaviors.
83-116: Excellent normalization of identifier storage.The migration properly extracts email and ORCID identifiers into the new
AgentIdentifiertable, preserving all data while improving the schema design.
140-177: Consistent foreign key updates across all related tables.All remaining foreign key constraints are properly updated to reference
PlatformAccountwith appropriate cascade behaviors maintained.
packages/database/supabase/migrations/20250603144146_account_centric.sql
Show resolved
Hide resolved
packages/database/supabase/migrations/20250603144146_account_centric.sql
Show resolved
Hide resolved
|
@CodeRabbit review last commit |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll help you review the last commit. Let me first identify what changes were made in the most recent commit. 🏁 Script executed: Length of output: 711 Let me try a different approach to get the commit information: 🏁 Script executed: Length of output: 19593 Let me focus on the specific changes mentioned in the commit message - the "trusted column to AgentIdentifier": 🏁 Script executed: Length of output: 8345 @maparent Looking at the last commit that adds the ✅ Positive aspects:
|
maparent
force-pushed
the
ENG-403-refactor-accounts
branch
from
4e6a71e to
d180b05
Compare
|
@maparent I see some more commits since the review. Are there additional changes that need reviewing? |
|
No, I did a rebase on main but no new commits
…On Thu, Jun 5, 2025 at 1:17 PM Michael Gartner ***@***.***> wrote:
*mdroidian* left a comment (DiscourseGraphs/discourse-graph#189)
<#189 (comment)>
@maparent <https://github.com/maparent> I see some more commits since the
review. Are there additional changes that need reviewing?
—
Reply to this email directly, view it on GitHub
<#189 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABRPQ6NHCMDKAHOD65R3QT3CB3S5AVCNFSM6AAAAAB6SIA52SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDSNBVGM2DKNBUG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Replace the old structure, where arbitrary Accounts were organized around a Person, to one where PlatformAccounts are organized around a Supabase User.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores