ENG-764 Bulk account uploading #357
Conversation
|
Updates to Preview Branch (eng-764-bulk-account-uploading) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds a new composite type Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant RPC as DB RPC
participant PA as PlatformAccount
participant SA as SpaceAccess
participant AI as AgentIdentifier
Client->>RPC: upsert_account_in_space(space_id, local_account)
Note over RPC: Resolve platform from space_id
RPC->>PA: INSERT ... ON CONFLICT (platform_id, account_local_id) DO UPDATE
RPC->>SA: INSERT ... ON CONFLICT (space_id, account_id) DO UPDATE (editor from local_account.space_editor)
alt email provided
RPC->>AI: INSERT ... ON CONFLICT (account_id, type='email', value) DO UPDATE (trusted)
end
RPC-->>Client: account_id
sequenceDiagram
autonumber
actor Client
participant RPC as DB RPC
participant Map as Local→DB Mapper
participant Acc as upsert_account_in_space
participant Doc as Document
participant Cnt as Content
Client->>RPC: upsert_documents(space_id, docs_json)
loop each document
RPC->>Map: _local_document_to_db_document(local)
alt author_inline present and id missing
RPC->>Acc: upsert_account_in_space(space_id, author_inline)
Acc-->>RPC: author_id
end
RPC->>Doc: INSERT ... ON CONFLICT DO UPDATE
end
RPC-->>Client: [document_id...]
Client->>RPC: upsert_content(space_id, content_json, creator_id?, as_document?)
loop each content
RPC->>Map: _local_content_to_db_content(local)
alt creator_inline present and id missing
RPC->>Acc: upsert_account_in_space(space_id, creator_inline)
Acc-->>RPC: creator_id
end
alt author_inline present and id missing
RPC->>Acc: upsert_account_in_space(space_id, author_inline)
Acc-->>RPC: author_id
end
RPC->>Cnt: INSERT ... ON CONFLICT DO UPDATE
end
RPC-->>Client: [content_id...]
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/database/supabase/schemas/account.sql (1)
162-170: Singular upsert: STRICT placement and AgentIdentifier column aliasMirror the fixes from the bulk path.
Apply this diff:
- SELECT platform INTO platform_ STRICT FROM public."Space" WHERE id = space_id_; + SELECT platform INTO STRICT platform_ FROM public."Space" WHERE id = space_id_; @@ - ON CONFLICT (value, identifier_type, account_id) - DO UPDATE SET trusted = COALESCE(local_account.email_trusted, ai.email_trusted, false); + ON CONFLICT (value, identifier_type, account_id) + DO UPDATE SET trusted = COALESCE(local_account.email_trusted, ai.trusted, false);Also applies to: 171-178
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/database/supabase/migrations/20250823135620_bulk_account_upsert.sql(1 hunks)packages/database/supabase/schemas/account.sql(2 hunks)packages/database/supabase/schemas/content.sql(7 hunks)packages/database/types.gen.ts(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 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/types.gen.ts
📚 Learning: 2025-05-20T14:04:19.632Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#165
File: packages/database/supabase/schemas/embedding.sql:66-95
Timestamp: 2025-05-20T14:04:19.632Z
Learning: In the `match_embeddings_for_subset_nodes` SQL function in packages/database/supabase/schemas/embedding.sql, the number of results is implicitly limited by the length of the input array parameter `p_subset_roam_uids` since the function filters content using `WHERE c.source_local_id = ANY(p_subset_roam_uids)`.
Applied to files:
packages/database/types.gen.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 (15)
packages/database/supabase/schemas/content.sql (7)
178-179: Type switch to account_local_input for author_inline looks correctThis aligns inline account handling with the new composite type surface. No issues spotted.
231-234: Graceful fallback: resolve author_id from inline account local_idGood addition. If author_local_id is absent but an inline account is provided, the author_id is derived. This keeps the mapping consistent with local inputs.
270-280: Inline creator/author resolution for Content mirrors Document logicThe ELSIF branches ensure creator_id/author_id are resolved from inline account local IDs when explicit local IDs are absent. This is consistent with the new inline account model.
327-329: Upsert missing document author from inline account before insert/updateNice safety net: if db_document.author_id is still NULL but an inline author is present, you upsert and assign. This avoids NOT NULL violations down the line.
389-389: Good: Explicit search_path for SECURITY DEFINER postureSetting
SET search_path = ''avoids search_path hijacking during the function’s execution.
508-510: Embedding upsert hook is guarded and non-failing—goodThe embedding write is optional and won’t break the main upsert path. Looks good.
389-389: Ensure inline account upserts occur before computingdb_contentCurrently, you call
db_content := public._local_content_to_db_content(local_content);before running the two inline-account upsert blocks. That means even if
author_inline(local_content)orcreator_inline(local_content)produces a new account,db_content.author_idanddb_content.creator_idstay NULL. You need to make sure resolved IDs flow intodb_content.Possible fixes:
• Move the inline-account upserts above the
_local_content_to_db_contentcall and then computedb_contentfrom the updatedlocal_content:- db_content := public._local_content_to_db_content(local_content); + -- First resolve inline accounts so their IDs end up in the final record + IF account_local_id(author_inline(local_content)) IS NOT NULL THEN + -- upsert author and set local_content.author_id + … + END IF; + IF account_local_id(creator_inline(local_content)) IS NOT NULL THEN + -- upsert creator and set local_content.creator_id + … + END IF; + -- Now build db_content with the populated IDs + db_content := public._local_content_to_db_content(local_content);• If you’d rather avoid recomputing, simply patch
db_contentafter the upserts to include any newly resolved IDs:IF account_local_id(author_inline(local_content)) IS NOT NULL THEN … local_content.author_id := upsert_id; END IF; IF account_local_id(creator_inline(local_content)) IS NOT NULL THEN … local_content.creator_id := upsert_id; END IF; + -- Back-fill any NULL IDs in db_content with the inline-upserted values + db_content.author_id := COALESCE(db_content.author_id, local_content.author_id); + db_content.creator_id := COALESCE(db_content.creator_id, local_content.creator_id);This change is required to guarantee that
Content.author_idandContent.creator_idaren’t left NULL when only inline accounts are provided.⛔ Skipped due to learnings
Learnt from: maparent PR: DiscourseGraphs/discourse-graph#0 File: :0-0 Timestamp: 2025-06-07T02:56:57.093Z Learning: In the discourse-graph project's upsert_content function, null creator_id values are tolerated as an acceptable trade-off, even though they may be annoying, rather than failing the entire operation.packages/database/types.gen.ts (3)
675-685: New RPCs exposed in typings match schema intent
upsert_account_in_spaceandupsert_accounts_in_spaceare present with the newaccount_local_inputcomposite. Note: argument key ordering in generated types may differ from SQL declaration; per prior learnings in this repo, that’s expected and safe when using named args with Supabase RPC.
759-765: Composite type account_local_input correctly mirrors SQLFields
name,account_local_id,email_trusted,space_editorare present and nullable, matching the SQL type.
642-646: No downstream destructuring of embedding fields detected; change is safe to mergeI searched for any direct uses of
match_content_embeddingsormatch_embeddings_for_subset_nodesand for occurrences of the returned fields (content_id,roam_uid,similarity,text_content) in TS/TSX files. The only consumer is the Next.js API route, which simply passes the RPC response through unchanged—there’s no destructuring or aliasing of those fields in client code:
apps/website/app/api/supabase/similarity-rank/route.tscallssupabase.rpc("match_embeddings_for_subset_nodes", …)and returns the full response viacreateApiResponsewithout touching individual fields.- No other
.ts/.tsxfile calls either RPC or accesses the shape’s properties directly (e.g., via destructuring).- The
match_content_embeddingsRPC is currently unused in the codebase.Since no client code expects renamed properties or destructures these fields, the updated return shape with
content_idandtext_contentis fully compatible.packages/database/supabase/schemas/account.sql (4)
101-109: account_local_input composite looks goodThe composite captures core account attributes and local flags. No issues.
183-198: Legacy wrapper correctly maps to composite inputRow construction order matches the composite and preserves
editor_intospace_editor. Good for backward compatibility.
200-291: Security/RLS posture remains consistentRLS policies and SECURITY DEFINER utilities retain the intended access model. No changes required.
111-149: Fix STRICT placement & attribute/alias mismatches in upsert functionsThe two
upsertfunctions inpackages/database/supabase/schemas/account.sql(lines 123–149 and 150–182) currently contain three critical SQL bugs that will cause runtime errors or incorrect data writes:• STRICT placement
PostgreSQL’s PL/pgSQL syntax requiresSTRICTto appear immediately afterINTO, not after the variable name.
ReplaceSELECT platform INTO platform_ STRICT FROM public."Space" WHERE id = space_id_;with
SELECT platform INTO STRICT platform_ FROM public."Space" WHERE id = space_id_;• Wrong composite‐type field
The composite typepublic.account_local_inputdefines the fifth attribute asspace_editor, noteditor. All references tolocal_account.editormust be updated:
- In the
INSERT … SpaceAccessstatements, replaceCOALESCE(local_account.editor, …)withCOALESCE(local_account.space_editor, …).- In the
ROW(…)::public.account_local_inputcasts at the bottom of each function, ensure the fifth element aligns (e.g.,ROW(name_, account_local_id_, email_, email_trusted, space_editor_)).• Wrong AgentIdentifier column alias
TheAgentIdentifiertable column is namedtrusted, notemail_trusted. In allON CONFLICT … DO UPDATEclauses, replaceai.email_trustedwithai.trusted:DO UPDATE SET trusted = COALESCE(local_account.email_trusted, ai.trusted, false);Locations needing fixes (in
schemas/account.sql):
upsert_accounts_in_space(lines 123–149)upsert_account_in_space(lines 150–182)Migration files (e.g.
20250823135620_bulk_account_upsert.sql) already contain the same issues. Per our migration‐history policy, do not edit existing migrations; instead, create a new corrective migration to apply the same three fixes there.packages/database/supabase/migrations/20250823135620_bulk_account_upsert.sql (1)
291-296: Remove invalid reference to db_document.creator_id
Documenthas nocreator_id; referencing it will fail. Remove the block.- IF (db_document.creator_id IS NULL AND creator_inline(local_content) IS NOT NULL) THEN - db_document.creator_id := upsert_account_in_space(v_space_id, creator_inline(local_content)); - END IF;⛔ Skipped due to learnings
Learnt from: maparent PR: DiscourseGraphs/discourse-graph#0 File: :0-0 Timestamp: 2025-06-07T02:56:57.093Z Learning: In the discourse-graph project's upsert_content function, null creator_id values are tolerated as an acceptable trade-off, even though they may be annoying, rather than failing the entire operation.
packages/database/supabase/migrations/20250823135620_bulk_account_upsert.sql
Outdated
Show resolved
Hide resolved
packages/database/supabase/migrations/20250823135620_bulk_account_upsert.sql
Outdated
Show resolved
Hide resolved
packages/database/supabase/migrations/20250823135620_bulk_account_upsert.sql
Outdated
Show resolved
Hide resolved
| DECLARE | ||
| v_platform public."Platform"; | ||
| db_document public."Document"%ROWTYPE; | ||
| document_id BIGINT; | ||
| local_content public.content_local_input; | ||
| db_content public."Content"%ROWTYPE; | ||
| content_row JSONB; | ||
| upsert_id BIGINT; | ||
| BEGIN | ||
| SELECT platform INTO STRICT v_platform FROM public."Space" WHERE id=v_space_id; | ||
| FOR content_row IN SELECT * FROM jsonb_array_elements(data) | ||
| LOOP | ||
| local_content := jsonb_populate_record(NULL::public.content_local_input, content_row); | ||
| local_content.space_id := v_space_id; | ||
| db_content := public._local_content_to_db_content(local_content); | ||
| IF account_local_id(author_inline(local_content)) IS NOT NULL THEN | ||
| SELECT public.create_account_in_space( | ||
| v_space_id, | ||
| account_local_id(author_inline(local_content)), | ||
| name(author_inline(local_content)) | ||
| ) INTO STRICT upsert_id; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider moving inline account upserts before building db_content
Same concern as in the schema file: build db_content after upserting inline author/creator so IDs propagate. See the suggested variants in the other comment.
🏁 Script executed:
#!/bin/bash
# Show the snippet around lines 248-268 in the migration file
sed -n '230,300p' packages/database/supabase/migrations/20250823135620_bulk_account_upsert.sql
# Find the definition of _local_content_to_db_content
rg -n "_local_content_to_db_content" -g "packages/database/supabase/**/*.sql"Length of output: 5449
Reorder inline account upserts before converting to db_content
In packages/database/supabase/migrations/20250823135620_bulk_account_upsert.sql, inside the upsert_content function (around line 260), the code currently builds db_content before doing the inline author/creator upserts—so any newly-created author_id/creator_id won’t be reflected in the content row. To fix this, move the two IF-blocks that call create_account_in_space above the call to _local_content_to_db_content.
• File:
packages/database/supabase/migrations/20250823135620_bulk_account_upsert.sql
• Location:
In upsert_content, swap the order of these lines:
- db_content := public._local_content_to_db_content(local_content);
IF account_local_id(author_inline(local_content)) IS NOT NULL THEN
SELECT public.create_account_in_space(
v_space_id,
account_local_id(author_inline(local_content)),
name(author_inline(local_content))
) INTO STRICT upsert_id;
local_content.author_id := upsert_id;
END IF;
IF account_local_id(creator_inline(local_content)) IS NOT NULL THEN
SELECT public.create_account_in_space(
v_space_id,
account_local_id(creator_inline(local_content)),
name(creator_inline(local_content))
) INTO STRICT upsert_id;
local_content.creator_id := upsert_id;
END IF;
+ db_content := public._local_content_to_db_content(local_content);This ensures that local_content.author_id and local_content.creator_id are set before the conversion utility reads them.
🤖 Prompt for AI Agents
In packages/database/supabase/migrations/20250823135620_bulk_account_upsert.sql
around lines 248 to 268, the function builds db_content before performing inline
author/creator account upserts so newly-created author_id/creator_id won't be
present in the converted row; move the two IF blocks that call
public.create_account_in_space (the inline author and creator upserts) to run
immediately after local_content is populated and before the call to db_content
:= public._local_content_to_db_content(local_content) so local_content.author_id
and local_content.creator_id are set prior to conversion.
maparent
force-pushed
the
eng-764-bulk-account-uploading
branch
from
7bb5f41 to
ec8109d
Compare
|
@CodeRabbit review again |
|
@maparent I'll conduct a comprehensive review of all the changes in this PR for bulk account uploading functionality. This includes the database migration, schema updates, and TypeScript type definitions. ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/database/supabase/schemas/content.sql (1)
435-437: Duplicate comment: Document has no creator_id column; referencing db_document.creator_id will fail
public."Document"does not definecreator_id. Accessingdb_document.creator_idin PL/pgSQL raises "record ... has no field 'creator_id'". Remove this block.Apply this diff:
- IF (db_document.author_id IS NULL AND author_inline(local_content) IS NOT NULL) THEN - db_document.author_id := upsert_account_in_space(v_space_id, author_inline(local_content)); - END IF;packages/database/supabase/migrations/20250823135620_bulk_account_upsert.sql (2)
62-62: Variable name inconsistency in bulk functionLine 62 uses
space_idinstead ofspace_id_when callingupsert_account_in_space. This should match the function parameter.Apply this diff:
- RETURN NEXT public.upsert_account_in_space(space_id, local_account); + RETURN NEXT public.upsert_account_in_space(space_id_, local_account);
246-262: Account creation assignments may not affect database operationsSimilar to the issue in the schema file, the assignments to
db_content.author_idanddb_content.creator_idafter conversion may not be effective sincedb_contentwas already built fromlocal_content.Consider reordering the operations:
local_content := jsonb_populate_record(NULL::public.content_local_input, content_row); local_content.space_id := v_space_id; - db_content := public._local_content_to_db_content(local_content); IF account_local_id(author_inline(local_content)) IS NOT NULL THEN SELECT public.create_account_in_space( v_space_id, account_local_id(author_inline(local_content)), name(author_inline(local_content)) ) INTO STRICT upsert_id; - db_content.author_id := upsert_id; + local_content.author_id := upsert_id; END IF; IF account_local_id(creator_inline(local_content)) IS NOT NULL THEN SELECT public.create_account_in_space( v_space_id, account_local_id(creator_inline(local_content)), name(creator_inline(local_content)) ) INTO STRICT upsert_id; - db_content.creator_id := upsert_id; + local_content.creator_id := upsert_id; END IF; + db_content := public._local_content_to_db_content(local_content);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/database/supabase/migrations/20250823135620_bulk_account_upsert.sql(1 hunks)packages/database/supabase/schemas/account.sql(2 hunks)packages/database/supabase/schemas/content.sql(8 hunks)packages/database/types.gen.ts(5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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/types.gen.ts
📚 Learning: 2025-05-20T14:04:19.632Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#165
File: packages/database/supabase/schemas/embedding.sql:66-95
Timestamp: 2025-05-20T14:04:19.632Z
Learning: In the `match_embeddings_for_subset_nodes` SQL function in packages/database/supabase/schemas/embedding.sql, the number of results is implicitly limited by the length of the input array parameter `p_subset_roam_uids` since the function filters content using `WHERE c.source_local_id = ANY(p_subset_roam_uids)`.
Applied to files:
packages/database/types.gen.ts
📚 Learning: 2025-06-07T02:56:57.093Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#0
File: :0-0
Timestamp: 2025-06-07T02:56:57.093Z
Learning: In the discourse-graph project's upsert_content function, null creator_id values are tolerated as an acceptable trade-off, even though they may be annoying, rather than failing the entire operation.
Applied to files:
packages/database/supabase/schemas/content.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 (25)
packages/database/supabase/schemas/content.sql (7)
178-178: LGTM! Correct type conversion for inline account inputThe change from
public."PlatformAccount"topublic.account_local_inputforauthor_inlinefield indocument_local_inputtype is correct and aligns with the new bulk account uploading functionality.
207-210: LGTM! Consistent inline account type updatesThe updates to use
public.account_local_inputfor bothauthor_inlineandcreator_inlinefields incontent_local_input, plus the addition of thevariantfield, are consistent with the bulk account uploading requirements.
231-234: Enhanced inline account resolution logicThe addition of inline account resolution logic in
_local_document_to_db_documentcorrectly checks if an inline account is available and resolves itsaccount_local_idto populatedocument.author_id. This provides a fallback whenauthor_local_idis not directly provided.
270-280: Enhanced inline account resolution for contentThe similar inline account resolution logic for both creator and author in
_local_content_to_db_contentmaintains consistency with the document function and correctly handles the fallback scenarios.
327-329: Effective inline account upsert integrationThe addition of inline account upserting in
upsert_documentsusing the newupsert_account_in_spacefunction properly handles the case wheredb_document.author_idis NULL but inline account data is available. This complements the existing inline account creation logic.
389-389: Good practice: Explicit search pathAdding
SET search_path = ''to theupsert_contentfunction ensures explicit schema resolution during operations, which is a good security and reliability practice.
413-413: Potential issue: Assignments may not propagate to database operationsThe code assigns
upsert_idtodb_content.author_idanddb_content.creator_idafter callingpublic._local_content_to_db_content(local_content). However, these assignments to the already-converteddb_contentrecord may not affect subsequent database operations since the local record was already processed.Consider moving the inline account creation logic before the
_local_content_to_db_contentcall, or alternatively, add additional upsert logic similar to what's done for documents:db_content := public._local_content_to_db_content(local_content); IF account_local_id(author_inline(local_content)) IS NOT NULL THEN SELECT public.create_account_in_space( v_space_id, account_local_id(author_inline(local_content)), name(author_inline(local_content)) ) INTO STRICT upsert_id; - db_content.author_id := upsert_id; + local_content.author_id := upsert_id; END IF; IF account_local_id(creator_inline(local_content)) IS NOT NULL THEN SELECT public.create_account_in_space( v_space_id, account_local_id(creator_inline(local_content)), name(creator_inline(local_content)) ) INTO STRICT upsert_id; - db_content.creator_id := upsert_id; + local_content.creator_id := upsert_id; END IF; + -- Rebuild db_content after setting local_content IDs + db_content := public._local_content_to_db_content(local_content);Or add fallback upsert logic after the conversion:
+ -- Handle remaining NULL author/creator IDs from inline data + IF (db_content.author_id IS NULL AND author_inline(local_content) IS NOT NULL) THEN + db_content.author_id := upsert_account_in_space(v_space_id, author_inline(local_content)); + END IF; + IF (db_content.creator_id IS NULL AND creator_inline(local_content) IS NOT NULL) THEN + db_content.creator_id := upsert_account_in_space(v_space_id, creator_inline(local_content)); + END IF;Also applies to: 421-421
⛔ Skipped due to learnings
Learnt from: maparent PR: DiscourseGraphs/discourse-graph#0 File: :0-0 Timestamp: 2025-06-07T02:56:57.093Z Learning: In the discourse-graph project's upsert_content function, null creator_id values are tolerated as an acceptable trade-off, even though they may be annoying, rather than failing the entire operation.packages/database/types.gen.ts (5)
759-765: New composite type definition looks correctThe
account_local_inputcomposite type definition with nullable fields for name, account_local_id, email, email_trusted, and space_editor aligns with the database schema changes and supports the bulk account uploading functionality.
675-685: New account upsert function signatures are well-definedBoth
upsert_account_in_spaceandupsert_accounts_in_spacefunction signatures correctly reflect the database functions with proper parameter types and return types.
806-806: Inline account type updates are consistentThe updates to
content_local_input.author_inlineandcontent_local_input.creator_inlineto use the newaccount_local_inputcomposite type maintain consistency across the type system.Also applies to: 809-810
828-828: Document inline account type updated correctlyThe
document_local_input.author_inlinefield correctly uses the newaccount_local_inputcomposite type, maintaining consistency with the content types.
642-646: Updated embedding function return typesThe return type updates for
match_content_embeddingsandmatch_embeddings_for_subset_nodesto includecontent_id,roam_uid,similarity, andtext_contentappear to be improvements to the embedding search functionality, though they're not directly related to the bulk account uploading feature.Also applies to: 654-654
packages/database/supabase/schemas/account.sql (4)
101-109: New composite type is well-structuredThe
account_local_inputcomposite type appropriately separates PlatformAccount columns from local values, providing a clean interface for bulk account operations.
111-142: Robust account upsert implementationThe
upsert_account_in_spacefunction correctly handles the complete account creation workflow including PlatformAccount insertion, SpaceAccess management, and optional AgentIdentifier creation. The use ofSECURITY DEFINERand explicit search path is appropriate.
144-165: Efficient bulk upsert implementationThe
upsert_accounts_in_spacefunction properly delegates to the single account upsert function, providing clean bulk functionality without code duplication.
168-181: Effective legacy wrapper maintains backward compatibilityThe legacy
create_account_in_spacefunction provides a smooth transition path by wrapping the new composite-based function while maintaining the original signature and default values.packages/database/supabase/migrations/20250823135620_bulk_account_upsert.sql (9)
1-9: Composite type definition is consistentThe
account_local_inputcomposite type definition matches the schema file and provides the necessary fields for bulk account operations.
11-42: Account upsert function implementation is solidThe
upsert_account_in_spacefunction correctly handles all aspects of account creation including platform resolution, account insertion, space access management, and email identifier handling.
44-65: Bulk upsert function with proper delegationThe
upsert_accounts_in_spacefunction correctly processes JSONB arrays and delegates to the single account upsert function.
68-81: Legacy wrapper maintains compatibilityThe legacy
create_account_in_spacefunction provides backward compatibility while using the new composite type internally.
83-86: Type alterations update existing schemasThe ALTER TYPE statements correctly update the existing composite types to use the new
account_local_inputtype for inline account fields.
88-118: Document conversion function is well-implementedThe
_local_document_to_db_documentfunction properly handles both direct local IDs and inline account resolution with appropriate fallbacks.
120-165: Content conversion function maintains consistencyThe
_local_content_to_db_contentfunction correctly handles document, creator, author, and part relationships with proper inline account resolution.
167-225: Document upsert function handles inline accounts properlyThe
upsert_documentsfunction correctly processes inline accounts and handles the document upsert logic with proper conflict resolution.
227-351: Comprehensive content upsert implementationThe
upsert_contentfunction provides a complete content upsert workflow including inline account creation, document handling, content insertion, and embedding processing. The logic flow is well-structured and handles various scenarios appropriately.
| FOR account_row IN SELECT * FROM jsonb_array_elements(accounts) | ||
| LOOP | ||
| local_account := jsonb_populate_record(NULL::public.account_local_input, account_row); | ||
| RETURN NEXT public.upsert_account_in_space(space_id, local_account); |
There was a problem hiding this comment.
Variable name inconsistency
Line 162 uses space_id instead of space_id_ when calling upsert_account_in_space. This should be space_id_ to match the function parameter.
Apply this diff:
- RETURN NEXT public.upsert_account_in_space(space_id, local_account);
+ RETURN NEXT public.upsert_account_in_space(space_id_, local_account);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RETURN NEXT public.upsert_account_in_space(space_id, local_account); | |
| RETURN NEXT public.upsert_account_in_space(space_id_, local_account); |
🤖 Prompt for AI Agents
In packages/database/supabase/schemas/account.sql around line 162, the call uses
the wrong variable name `space_id` instead of the function parameter
`space_id_`; update the call to pass `space_id_` to
public.upsert_account_in_space so the parameter names match and the correct
value is forwarded.
ec8109d to
bdb0180
Compare
Add functions to upsert platform accounts (individually or in bulk) Keep the old create_account_in_space function as a shim for now. Use the new upsert_account in upsert_documents and upsert_content, allowing for more complete inline information.
maparent
force-pushed
the
eng-764-bulk-account-uploading
branch
from
bdb0180 to
18a0d92
Compare
|
@CodeRabbit good catch for Otherwise, when there is a PR involving |
|
Tip For best results, initiate chat on the files or code changes. @maparent Thank you for the feedback! I appreciate the clarification on I'll remember your preference about focusing on schema files when both schemas and migrations are present in a PR to avoid redundant error reporting. This will help keep the reviews more focused and reduce noise. 🐰✨ ✏️ Learnings added
|
|
The use case: The global node uploading (Sid's work on #343) failed when the accounts are not specified. There's a way to make it work (with inline accounts) but it's verbose. Pre-filling the accounts is way more efficient. Sorry I did not point to it, but most of the conversation is there. |
|
Do we intend to use |
|
the former will be more efficient. |
2 participants
https://linear.app/discourse-graphs/issue/ENG-764/bulk-account-uploading
Summary by CodeRabbit
New Features
Refactor