ENG-771 Handle empty account name #364
Conversation
|
Updates to Preview Branch (eng-771-handle-empty-account-name) ↗︎
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 SQL function public.upsert_account_in_space for upserting accounts into spaces, handling platform resolution, account name updates without overwriting with empty strings, space access updates, and optional email identifier upserts with trusted flags. Adjusts ON CONFLICT name update to treat empty string as NULL. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant DB as public.upsert_account_in_space
participant Space as Space
participant Plat as Platform
participant PA as PlatformAccount
participant SA as SpaceAccess
participant AI as AgentIdentifier
Client->>DB: upsert_account_in_space(space_id, local_account)
DB->>Space: SELECT platform_id FROM Space WHERE id=space_id
Space-->>DB: platform_id
DB->>Plat: SELECT platform FROM Platform WHERE id=platform_id
Plat-->>DB: platform
rect rgba(200,240,255,0.3)
note right of DB: Upsert platform account (preserve name if NULL/empty)
DB->>PA: INSERT ... ON CONFLICT (account_local_id, platform)<br/>DO UPDATE SET name = COALESCE(NULLIF(local_account.name,''), pa.name)
PA-->>DB: RETURNING id INTO account_id
end
rect rgba(220,255,220,0.3)
note right of DB: Upsert space access
DB->>SA: INSERT ... ON CONFLICT (space_id, account_id)<br/>DO UPDATE SET editor = COALESCE(local_account.space_editor, sa.editor, true)
end
alt local_account.email IS NOT NULL
rect rgba(255,245,200,0.4)
note right of DB: Upsert email identifier
DB->>AI: INSERT (account_id, identifier_type='email', value, trusted)<br/>ON CONFLICT (identifier_type,value)<br/>DO UPDATE SET trusted = COALESCE(local_account.email_trusted, ai.trusted, false)
end
end
DB-->>Client: account_id
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 3
🧹 Nitpick comments (3)
packages/database/supabase/migrations/20250824134305_do_not_overwrite_with_empty_name.sql (3)
1-6: SECURITY DEFINER + empty search_path is good; ensure intended owner and grantsSolid hardening by schema-qualifying objects and setting an empty search_path. Double-check that the function owner is the expected role (e.g., service role) and that execution privileges are granted only to roles that need it. If not handled elsewhere in migrations, add ALTER FUNCTION ... OWNER TO ... and GRANT EXECUTE statements.
12-22: Consider preventing insertion of empty/whitespace names on initial insertThe ON CONFLICT branch preserves existing names when the input is empty, but the initial INSERT can still write an empty or whitespace-only name (column is NOT NULL, so '' is permitted). If empty names should never be stored, consider normalizing at INSERT time or adding a CHECK constraint.
Two options:
- Minimal change (normalize at INSERT, fallback to account_local_id or raise):
- local_account.account_local_id, local_account.name, platform_ + local_account.account_local_id, + COALESCE(NULLIF(BTRIM(local_account.name), ''), local_account.account_local_id), + platform_
- Schema-level guard (follow-up migration):
ALTER TABLE public."PlatformAccount" ADD CONSTRAINT platform_account_name_not_blank CHECK (BTRIM(name) <> '');Confirm desired behavior before adopting, since the first option changes persisted values for blank inputs.
11-12: SELECT ... INTO STRICT is appropriate; consider exception message for DXSTRICT will raise if the space_id is invalid. Optionally wrap to provide a clearer SQLSTATE/message that includes space_id_, to help callers distinguish input errors from internal failures.
📜 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 (2)
packages/database/supabase/migrations/20250824134305_do_not_overwrite_with_empty_name.sql(1 hunks)packages/database/supabase/schemas/account.sql(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#272
File: packages/ui/src/lib/supabase/contextFunctions.ts:50-111
Timestamp: 2025-07-08T14:48:38.048Z
Learning: The Supabase client does not offer transaction support, so idempotent upserts with proper conflict resolution (using onConflict with ignoreDuplicates: false) are the preferred approach for multi-step database operations in packages/ui/src/lib/supabase/contextFunctions.ts. This pattern prevents orphaned records when retrying operations.
📚 Learning: 2025-07-08T14:48:38.048Z
Learnt from: maparent
PR: DiscourseGraphs/discourse-graph#272
File: packages/ui/src/lib/supabase/contextFunctions.ts:50-111
Timestamp: 2025-07-08T14:48:38.048Z
Learning: The Supabase client does not offer transaction support, so idempotent upserts with proper conflict resolution (using onConflict with ignoreDuplicates: false) are the preferred approach for multi-step database operations in packages/ui/src/lib/supabase/contextFunctions.ts. This pattern prevents orphaned records when retrying operations.
Applied to files:
packages/database/supabase/migrations/20250824134305_do_not_overwrite_with_empty_name.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 (3)
packages/database/supabase/migrations/20250824134305_do_not_overwrite_with_empty_name.sql (3)
23-25: SpaceAccess upsert logic looks solidInsert defaulting editor to true, and conflict update preserving existing value when input is NULL is sensible and idempotent. Nice alignment with the Supabase no-transaction upsert pattern noted in past learnings.
23-34: RETURNING ... INTO STRICT pattern is correct and safeGood use of RETURNING with INTO STRICT to guarantee account_id_ is set in both insert and update paths.
1-36: Overall: Idempotent upsert flow aligns with our Supabase retry patternThe function composes PlatformAccount, SpaceAccess, and AgentIdentifier upserts in an idempotent way suitable for clients without transaction support. Good job minimizing race surfaces with proper ON CONFLICT clauses.
| ) ON CONFLICT (account_local_id, platform) DO UPDATE SET | ||
| name = COALESCE( | ||
| CASE WHEN length(local_account.name) = 0 | ||
| THEN NULL ELSE local_account.name END, | ||
| pa.name | ||
| ) | ||
| RETURNING id INTO STRICT account_id_; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make name update whitespace-safe and rely on EXCLUDED to reflect the proposed value
Same point as in schema: use NULLIF(BTRIM(...), '') and EXCLUDED for clarity and to prevent overwriting with whitespace-only input.
- ) ON CONFLICT (account_local_id, platform) DO UPDATE SET
- name = COALESCE(
- CASE WHEN length(local_account.name) = 0
- THEN NULL ELSE local_account.name END,
- pa.name
- )
+ ) ON CONFLICT (account_local_id, platform) DO UPDATE SET
+ name = COALESCE(NULLIF(BTRIM(EXCLUDED.name), ''), pa.name)📝 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.
| ) ON CONFLICT (account_local_id, platform) DO UPDATE SET | |
| name = COALESCE( | |
| CASE WHEN length(local_account.name) = 0 | |
| THEN NULL ELSE local_account.name END, | |
| pa.name | |
| ) | |
| RETURNING id INTO STRICT account_id_; | |
| ) ON CONFLICT (account_local_id, platform) DO UPDATE SET | |
| name = COALESCE(NULLIF(BTRIM(EXCLUDED.name), ''), pa.name) | |
| RETURNING id INTO STRICT account_id_; |
🤖 Prompt for AI Agents
In
packages/database/supabase/migrations/20250824134305_do_not_overwrite_with_empty_name.sql
around lines 16 to 22, the ON CONFLICT update currently uses a CASE on
local_account.name and allows whitespace-only strings to overwrite the existing
name; change it to use EXCLUDED (the proposed insert value) and make the check
whitespace-safe by trimming and nullifying empty results, e.g. set name =
COALESCE(NULLIF(BTRIM(EXCLUDED.name), ''), pa.name) so whitespace-only or empty
names do not overwrite the prior value.
| IF local_account.email IS NOT NULL THEN | ||
| -- TODO: how to distinguish basic untrusted from platform placeholder email? | ||
| INSERT INTO public."AgentIdentifier" as ai (account_id, value, identifier_type, trusted) VALUES (account_id_, local_account.email, 'email', COALESCE(local_account.email_trusted, false)) | ||
| ON CONFLICT (value, identifier_type, account_id) | ||
| DO UPDATE SET trusted = COALESCE(local_account.email_trusted, ai.trusted, false); | ||
| END IF; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Normalize email value and guard against blank/whitespace; consider case-insensitive storage
To avoid duplicates and inconsistent matching:
- Skip upsert when email is blank/whitespace.
- Normalize to lower(trim(email)) before storage and conflict checks.
- IF local_account.email IS NOT NULL THEN
+ IF local_account.email IS NOT NULL AND BTRIM(local_account.email) <> '' THEN
- INSERT INTO public."AgentIdentifier" as ai (account_id, value, identifier_type, trusted) VALUES (account_id_, local_account.email, 'email', COALESCE(local_account.email_trusted, false))
+ INSERT INTO public."AgentIdentifier" as ai (account_id, value, identifier_type, trusted)
+ VALUES (account_id_, LOWER(BTRIM(local_account.email)), 'email', COALESCE(local_account.email_trusted, false))
ON CONFLICT (value, identifier_type, account_id)
DO UPDATE SET trusted = COALESCE(local_account.email_trusted, ai.trusted, false);Note: If existing data isn’t normalized yet and primary key is case-sensitive, this may require a data backfill and possibly switching value to CITEXT or a functional PK using LOWER(value). Suggest as a follow-up migration.
📝 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.
| IF local_account.email IS NOT NULL THEN | |
| -- TODO: how to distinguish basic untrusted from platform placeholder email? | |
| INSERT INTO public."AgentIdentifier" as ai (account_id, value, identifier_type, trusted) VALUES (account_id_, local_account.email, 'email', COALESCE(local_account.email_trusted, false)) | |
| ON CONFLICT (value, identifier_type, account_id) | |
| DO UPDATE SET trusted = COALESCE(local_account.email_trusted, ai.trusted, false); | |
| END IF; | |
| IF local_account.email IS NOT NULL AND BTRIM(local_account.email) <> '' THEN | |
| -- TODO: how to distinguish basic untrusted from platform placeholder email? | |
| INSERT INTO public."AgentIdentifier" as ai (account_id, value, identifier_type, trusted) | |
| VALUES ( | |
| account_id_, | |
| LOWER(BTRIM(local_account.email)), | |
| 'email', | |
| COALESCE(local_account.email_trusted, false) | |
| ) | |
| ON CONFLICT (value, identifier_type, account_id) | |
| DO UPDATE SET trusted = COALESCE(local_account.email_trusted, ai.trusted, false); | |
| END IF; |
🤖 Prompt for AI Agents
packages/database/supabase/migrations/20250824134305_do_not_overwrite_with_empty_name.sql
around lines 26-31: the upsert currently inserts raw local_account.email and
will overwrite or duplicate entries for blank/whitespace or case variants;
change the logic to skip the INSERT when email is NULL or consists only of
whitespace, compute a normalized value once (lower(trim(local_account.email)))
and use that normalized value in the INSERT and the ON CONFLICT key checks, and
preserve the existing trusted flag when COALESCE’ing (i.e., DO UPDATE SET
trusted = COALESCE(local_account.email_trusted, ai.trusted, false)); add a note
to follow up with a data backfill and/or switching the column to CITEXT or
creating a functional unique index on lower(value) if existing data is not
normalized.
| name = COALESCE( | ||
| CASE WHEN length(local_account.name) = 0 | ||
| THEN NULL ELSE local_account.name END, | ||
| pa.name | ||
| ) | ||
| RETURNING id INTO STRICT account_id_; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prefer NULLIF(BTRIM(...), '') and EXCLUDED for clearer, whitespace-safe updates
Current CASE handles empty string but not whitespace-only names, and references the PL/pgSQL parameter instead of the proposed row via EXCLUDED. Recommend simplifying and making it robust to whitespace:
- Treat whitespace-only names as empty.
- Use EXCLUDED.name to reflect the incoming value in the ON CONFLICT branch.
Apply:
- name = COALESCE(
- CASE WHEN length(local_account.name) = 0
- THEN NULL ELSE local_account.name END,
- pa.name
- )
+ name = COALESCE(NULLIF(BTRIM(EXCLUDED.name), ''), pa.name)📝 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.
| name = COALESCE( | |
| CASE WHEN length(local_account.name) = 0 | |
| THEN NULL ELSE local_account.name END, | |
| pa.name | |
| ) | |
| RETURNING id INTO STRICT account_id_; | |
| name = COALESCE(NULLIF(BTRIM(EXCLUDED.name), ''), pa.name) | |
| RETURNING id INTO STRICT account_id_; |
There was a problem hiding this comment.
Good catch, made the change. But I used trim, curious why you thought to use btrim? The name is a varchar.
There was a problem hiding this comment.
@maparent You're absolutely right! For a VARCHAR field, TRIM is actually the better choice here. Both TRIM and BTRIM would work similarly for removing leading and trailing whitespace, but:
TRIMis the SQL standard function and more portableBTRIMis PostgreSQL-specific and essentially equivalent for basic whitespace removal- For VARCHAR fields with standard whitespace trimming,
TRIMis the cleaner, more conventional choice
I suggested BTRIM out of PostgreSQL-specific habit, but your use of TRIM is more appropriate and standard. Thanks for the good catch and question!
maparent
force-pushed
the
eng-771-handle-empty-account-name
branch
from
555b668 to
dc62510
Compare
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
Why is there a large CREATE OR REPLACE in the migration, but I don't see this code in any new .sql file?
Also, could you give an example of how this would be used in practice on the client side?
maparent
force-pushed
the
eng-771-handle-empty-account-name
branch
3 times, most recently
from
a5ea9c5 to
46c1d36
Compare
maparent
force-pushed
the
eng-771-handle-empty-account-name
branch
from
46c1d36 to
5cdbbd8
Compare
maparent
force-pushed
the
eng-771-handle-empty-account-name
branch
from
5cdbbd8 to
5212d4c
Compare
|
Missed that comment. the migration has a (smallish?) create or replace for |
* Do not overwrite name with empty name * document upsert_accounts_in_space
https://linear.app/discourse-graphs/issue/ENG-771/handle-empty-account-name
Context: Now that we do bulk account upsert, we sometimes get empty names from roam. We should not overwrite defined names with empty names.
Summary by CodeRabbit
New Features
Bug Fixes