♻️ Migrate from AWS Cognito to Supabase OTP authentication #2469
Conversation
- Replace Cognito with Supabase for auth (email OTP flow) - Add new supabase service with OTP methods (signInWithOtp, verifyOtp) - Add supabase authenticator for ember-simple-auth - Replace JSONAPI remote source with Supabase data source - Simplify login flow: single 2-step OTP process for new/existing users - Remove password-related flows (forgot-password, register-confirm) - Update tests for new OTP authentication flow - Add migration documentation BREAKING: Requires SUPABASE_URL and SUPABASE_ANON_KEY env vars
- Define palettes and colors tables with proper constraints - Add Row Level Security policies for user data isolation - Include indexes for query performance - Add helper functions for data migration from Orbit.js
📝 WalkthroughWalkthroughReplaces AWS Cognito/JSON:API with Supabase: adds OTP-based Supabase authentication, a Supabase-backed Orbit remote source with realtime sync for palettes/colors, removes Cognito services/components, updates environment/config, dependencies, tests, and adds migration docs and a SQL schema. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Login as Login Component
participant Supabase as Supabase Service
participant Session as Session Service
participant Router as Router
rect rgb(240,248,255)
note right of Login: OTP Request flow
User->>Login: submit email
Login->>Supabase: signInWithOtp(email)
Supabase->>Supabase: initialize client from config
Supabase-->>Login: confirmation
Login-->>User: show OTP step
end
rect rgb(240,255,240)
note right of Login: OTP Verify & Authenticate
User->>Login: submit 6-digit code
Login->>Supabase: verifyOtp(email, token)
Supabase-->>Login: returns user & session
Login->>Session: authenticate('supabase', SupabaseAuthData)
Session-->>Login: stored
Login->>Router: transition to settings.cloud.profile
end
sequenceDiagram
participant App as App/UI
participant DataSvc as Data Service
participant SupabaseSrc as Supabase Source
participant Client as Supabase Client
participant DB as PostgreSQL
participant RT as Realtime Listener
rect rgb(245,250,255)
note right of DataSvc: Query palettes with colors
App->>DataSvc: queryPalettes()
DataSvc->>SupabaseSrc: queryPalettes()
SupabaseSrc->>Client: SELECT palettes + colors (user filter)
Client->>DB: execute
DB-->>Client: rows
SupabaseSrc-->>DataSvc: Orbit records
DataSvc-->>App: UI update
end
rect rgb(245,255,245)
note right of SupabaseSrc: Realtime sync
SupabaseSrc->>Client: setupRealtimeSync()
Client->>DB: subscribe
DB-->>RT: emit change
RT->>SupabaseSrc: onInsert/onUpdate/onDelete
SupabaseSrc->>DataSvc: propagate change events
DataSvc->>App: update local store/UI
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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 |
- Fix ESLint errors in supabase service (side-effects, unsafe return) - Fix unused vars and unnecessary type assertions - Convert register component to template-only - Add electron-app/ember-test to lint ignore files (build artifacts) - Format files with prettier
- Add sort_index column to palettes table in schema - Update remote.ts to transform index <-> sort_index - Update palettes_with_colors view to include sort_index
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (14)
app/services/session.ts (1)
7-15: Consider importingSupabaseAuthDatafrom the authenticator to avoid duplication.This interface mirrors the one defined in
app/authenticators/supabase.ts(lines 6-12). The structure here correctly wraps it inauthenticatedfor ember-simple-auth, but the inner type could be imported.🔎 Suggested refactor
+import type { SupabaseAuthData } from '../authenticators/supabase.ts'; + -interface SupabaseAuthData { - authenticated: { - userId: string; - email: string; - accessToken: string; - refreshToken: string; - expiresAt: number; - }; -} +interface SupabaseSessionData { + authenticated: SupabaseAuthData; +} -export default class SessionService extends BaseSessionService<SupabaseAuthData> { +export default class SessionService extends BaseSessionService<SupabaseSessionData> {docs/SUPABASE_MIGRATION.md (1)
45-45: Convert bare URL to proper Markdown link.The static analysis flagged this as MD034 (no-bare-urls). Convert to a Markdown link for better accessibility.
🔎 Suggested fix
-1. Create a new Supabase project at https://supabase.com +1. Create a new Supabase project at [supabase.com](https://supabase.com)app/templates/settings/cloud/profile.gts (1)
9-20: Consider fallback display when email is unavailable.If
@model.emailis undefined, users see "Signed in as..." with nothing below, which could be confusing. Consider displayinguserIdas a fallback or showing a placeholder.🔎 Suggested improvement
{{#if @model.email}} <div data-test-profile-detail="email" class="border-input-border border-t border-b py-5 text-menu-text text-sm" > {{@model.email}} </div> + {{else if @model.userId}} + <div + data-test-profile-detail="userId" + class="border-input-border border-t border-b py-5 text-menu-text text-sm text-gray-500" + > + User ID: {{@model.userId}} + </div> {{/if}}app/services/data.ts (1)
199-210: Consider handling partial upload failures.Sequential uploads without transaction semantics could leave data in an inconsistent state if palette uploads fail after colors have been added. Consider either:
- Wrapping in a try-catch to log/report partial failures
- Using Supabase transactions if atomicity is required
For the initial sync scenario (first login), this may be acceptable since a retry would re-upload, but orphaned colors could accumulate on repeated failures.
🔎 Proposed error handling improvement
// Upload colors first + const uploadedColors: InitializedRecord[] = []; + const uploadedPalettes: InitializedRecord[] = []; + + try { for (const color of colors) { - await remote.addRecord(color); + await remote.addRecord(color); + uploadedColors.push(color); } // Then upload palettes for (const palette of palettes) { await remote.addRecord(palette); + uploadedPalettes.push(palette); } + } catch (error) { + console.error( + '[Data Service] Partial sync failure:', + { uploadedColors: uploadedColors.length, uploadedPalettes: uploadedPalettes.length }, + error + ); + throw error; + }config/environment.js (1)
25-28: Redundant Supabase config definition in production block.The production block (lines 64-69) duplicates the default config (lines 25-28) with identical logic. Since the default already reads from
process.env, the production override is unnecessary.🔎 Proposed simplification
if (environment === 'production') { - // Production Supabase config from environment variables - ENV.supabase = { - url: process.env.SUPABASE_URL || '', - anonKey: process.env.SUPABASE_ANON_KEY || '', - }; + // Default supabase config already reads from process.env + // Add production-specific overrides here if needed }Also applies to: 64-69
app/authenticators/supabase.ts (1)
20-23: Consider defensive check for supabase service lookup.If
getOwner(this)returns undefined or the service isn't registered, this will throw a less informative error at runtime when accessing methods.🔎 Proposed defensive getter
get supabase(): SupabaseService { - return getOwner(this)?.lookup('service:supabase') as SupabaseService; + const service = getOwner(this)?.lookup('service:supabase') as SupabaseService | undefined; + if (!service) { + throw new Error('SupabaseService not available'); + } + return service; }tests/acceptance/settings/cloud-test.ts (1)
162-168: Consider using a data-test attribute for error message assertion.The selector
.bg-red-400is tied to styling and may break if the error styling changes. A data-test attribute would be more resilient.🔎 Proposed improvement
In the login component template, add a data-test attribute:
- <div class="bg-red-400 my-2 p-4 rounded-sm text-xs text-red-800"> + <div class="bg-red-400 my-2 p-4 rounded-sm text-xs text-red-800" data-test-error-message>Then update the test:
- assert.dom('.bg-red-400').exists('error message is shown'); + assert.dom('[data-test-error-message]').exists('error message is shown');app/data-sources/remote.ts (1)
317-333: Sequential updates in replaceRelatedRecords could be slow for large palettes.Each color update is a separate network call. For palettes with many colors, consider batching if Supabase supports it, though this is likely acceptable for typical palette sizes.
app/components/login.gts (2)
180-185: OTP validation could be stricter.The validation only checks length (
this.otpCode.length !== 6), but the input allows any characters. While the HTML pattern attribute provides client-side hints, consider validating numeric content in the action.🔎 Proposed validation improvement
@action async verifyOtp(): Promise<void> { - if (!this.otpCode || this.otpCode.length !== 6) { - this.errorMessage = 'Please enter the 6-digit code'; + if (!this.otpCode || !/^\d{6}$/.test(this.otpCode)) { + this.errorMessage = 'Please enter a valid 6-digit code'; return; }
44-48: Consider adding a data-test attribute to the error message div.This would make test assertions more resilient (as noted in the test file review).
🔎 Proposed addition
{{#if this.errorMessage}} - <div class="bg-red-400 my-2 p-4 rounded-sm text-xs text-red-800"> + <div class="bg-red-400 my-2 p-4 rounded-sm text-xs text-red-800" data-test-error-message> {{this.errorMessage}} </div> {{/if}}app/services/supabase.ts (1)
122-128: Useerror instanceof AuthSessionMissingErrorinstead of string matching.Checking
error.message?.includes('not authenticated')is fragile and will break if the Supabase SDK updates error messages. The SDK providesAuthSessionMissingErrorclass (imported from@supabase/supabase-js) for this specific case. Useerror instanceof AuthSessionMissingErroror checkerror.name === 'AuthSessionMissingError'instead.supabase-schema.sql (3)
5-6: Consider removing the uuid-ossp extension.The schema uses
gen_random_uuid()throughout (lines 12, 35), which is available in PostgreSQL 13+ by default and doesn't require theuuid-osspextension. Theuuid-osspextension provides functions likeuuid_generate_v4(), which aren't used here.🔎 Verify Supabase PostgreSQL version and extension requirements
What version of PostgreSQL does Supabase use and is gen_random_uuid available by default?
57-68: Consider adding indexes for palette ordering.The existing indexes provide good coverage. However, if palettes are frequently ordered by
sort_indexor queried by user with time-based sorting, consider adding:🔎 Suggested additional indexes
-- For palette ordering by sort_index CREATE INDEX IF NOT EXISTS idx_palettes_sort_index ON palettes(sort_index); -- For time-based queries scoped to user (if frequently used) CREATE INDEX IF NOT EXISTS idx_palettes_user_created ON palettes(user_id, created_at DESC);These additions depend on your actual query patterns. The
sort_indexindex is particularly relevant since the column was added specifically for palette ordering (per PR commits).
115-130: Consider modern PL/pgSQL syntax.The trigger function works correctly but uses quoted language syntax. Modern PostgreSQL prefers unquoted language identifiers.
🔎 Suggested syntax update
RETURN NEW; END; -$$ language 'plpgsql'; +$$ LANGUAGE plpgsql;Apply the same change to other functions (lines 175, 239, 261).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (29)
.prettierignore.stylelintignoreapp/authenticators/cognito.jsapp/authenticators/supabase.tsapp/components/forgot-password.gtsapp/components/login.gtsapp/components/register-confirm.gtsapp/components/register.gtsapp/config/environment.tsapp/data-sources/remote.jsapp/data-sources/remote.tsapp/router.tsapp/routes/settings/cloud/index.tsapp/routes/settings/cloud/profile.tsapp/services/cognito.jsapp/services/data.tsapp/services/session.tsapp/services/supabase.tsapp/templates/settings/cloud/forgot-password.gtsapp/templates/settings/cloud/profile.gtsapp/templates/settings/cloud/register.gtsapp/templates/settings/cloud/register/confirm.gtsapp/templates/settings/cloud/register/index.gtsconfig/environment.jsdocs/SUPABASE_MIGRATION.mdeslint.config.mjspackage.jsonsupabase-schema.sqltests/acceptance/settings/cloud-test.ts
💤 Files with no reviewable changes (9)
- app/components/forgot-password.gts
- app/data-sources/remote.js
- app/services/cognito.js
- app/templates/settings/cloud/forgot-password.gts
- app/authenticators/cognito.js
- app/templates/settings/cloud/register/index.gts
- app/routes/settings/cloud/index.ts
- app/components/register-confirm.gts
- app/templates/settings/cloud/register/confirm.gts
🧰 Additional context used
🧬 Code graph analysis (7)
app/data-sources/remote.ts (2)
app/services/supabase.ts (1)
SupabaseService(23-165)app/services/session.ts (1)
SessionService(17-31)
app/routes/settings/cloud/profile.ts (1)
app/routes/settings/cloud/index.ts (1)
SettingsAccountRoute(9-26)
app/authenticators/supabase.ts (1)
app/services/supabase.ts (1)
SupabaseService(23-165)
app/services/supabase.ts (1)
app/config/environment.ts (1)
EnvironmentConfig(28-43)
app/services/session.ts (1)
app/authenticators/supabase.ts (1)
SupabaseAuthData(7-13)
tests/acceptance/settings/cloud-test.ts (3)
app/services/supabase.ts (1)
SupabaseService(23-165)tests/helpers.ts (1)
waitForAll(24-38)app/services/data.ts (1)
DataService(14-216)
app/services/data.ts (1)
app/data-sources/remote.ts (1)
SupabaseSource(55-509)
🪛 LanguageTool
docs/SUPABASE_MIGRATION.md
[grammar] ~96-~96: Ensure spelling is correct
Context: ...oDB: 1. Export your current data using Swach's export feature (Settings > Data > Expor...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
docs/SUPABASE_MIGRATION.md
45-45: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (31)
eslint.config.mjs (1)
54-54: LGTM! Consistent ignore pattern for compiled output.The addition of
electron-app/ember-test/**/*to the ignore list is appropriate for excluding compiled/generated test artifacts from linting..stylelintignore (1)
6-6: LGTM! Properly categorized under compiled output.The addition of
/electron-app/ember-test/aligns with the corresponding ESLint and Prettier ignore patterns, ensuring consistent exclusion of generated test artifacts across all tooling..prettierignore (1)
7-7: LGTM! Consistent ignore pattern across all tooling.The addition of
/electron-app/ember-test/completes the consistent exclusion of this compiled output directory across ESLint, Stylelint, and Prettier, which is the correct approach for generated test artifacts.app/services/session.ts (1)
20-30: LGTM!The session lifecycle hooks correctly trigger data synchronization on authentication and reset on invalidation. The
voidpattern is appropriate for these fire-and-forget background operations.app/templates/settings/cloud/register.gts (1)
1-3: LGTM!Clean template-only route that delegates to the Register component. This aligns with the simplified OTP flow where registration and login share the same email → code verification pattern.
docs/SUPABASE_MIGRATION.md (1)
106-112: Verify real-time sync is actually implemented.The PR objectives note "real-time sync foundation" as future work, but this documentation describes it as a current feature. Ensure the documentation accurately reflects the current implementation state, or mark these as planned features.
app/templates/settings/cloud/profile.gts (1)
34-38: LGTM!The type signature correctly reflects the updated
ProfileModelfrom the route, maintaining type consistency across the template and route.app/router.ts (1)
14-19: LGTM!Route simplification correctly reflects the OTP flow: no password-related routes needed, and registration/confirmation are now handled inline. Clean structure.
app/routes/settings/cloud/profile.ts (1)
7-22: LGTM!The model correctly extracts display-relevant fields from the session's authenticated data. The optional chaining is defensive, though the parent route (
settings/cloud/index.ts) ensures authentication before reaching this route.app/components/register.gts (1)
3-45: Clean migration to template-only OTP flow.The component correctly explains the unified OTP flow where registration and login are merged. Both CTAs appropriately redirect to the login route since OTP handles new user creation automatically.
app/services/data.ts (1)
169-215: Good simplification of remote data sync logic.The refactored method cleanly separates the authenticated/unauthenticated paths and the sequential upload approach (colors → palettes) respects foreign key constraints in the Supabase schema.
config/environment.js (1)
56-61: Test mock configuration looks correct.Using explicit mock values for tests ensures test isolation from real Supabase credentials.
app/authenticators/supabase.ts (2)
43-68: Restore correctly delegates to Supabase's persisted session.Ignoring the
_dataparameter and fetching fresh session/user from Supabase is the right approach since Supabase handles its own session persistence. This ensures token refresh works correctly.
29-41: Authenticate method is clean and well-structured.Properly delegates OTP verification to the Supabase service and maps the response to the expected auth data format.
tests/acceptance/settings/cloud-test.ts (2)
64-114: Comprehensive OTP login flow test.The test properly mocks the authentication chain (signInWithOtp → authenticate → synchronize) and verifies the complete flow including navigation to the profile page.
34-62: Good test isolation with service stubs.Properly stubs
signInWithOtpat the service level before user interaction, ensuring predictable behavior.app/data-sources/remote.ts (4)
434-474: Well-structured transform functions with proper defaults.The
transformFromOrbitmethod correctly handles both palette and color types, applies sensible defaults, and ensuresuser_idis attached for RLS compliance.
481-508: Real-time sync implementation is clean.Properly cleans up existing channel before creating a new subscription, preventing duplicate subscriptions. The wildcard event subscription (
'*') captures all change types.
511-521: Factory pattern for SupabaseSource creation.The default export provides a clean factory that applies standard Orbit injections and configures the source. Setting
autoActivate: falseis appropriate for controlled initialization.
60-66: Use standardgetOwner(this)pattern instead ofgetOwner(injections).The
SupabaseSourceInjectionsinterface doesn't declare anownerproperty, yetgetOwner(injections)relies onapplyStandardSourceInjectionsto implicitly set it. This is non-standard—all other service lookups in the codebase usegetOwner(this)(see supabase.ts, authenticators/supabase.ts, routes/application.ts). Pass the owner explicitly or callgetOwner(this)in the constructor aftersuper(injections)completes.app/services/supabase.ts (3)
34-52: Lazy initialization with proper config validation.The client is lazily initialized on first access, and missing configuration throws a clear error message directing users to set the required environment variables.
136-143: Non-throwing signOut is the right choice.Logging the error but not throwing ensures local state can be cleared even if the remote sign-out fails (e.g., network issues). This prevents users from getting stuck in a "logged in" state.
159-164: Good DRY approach for resendOtp.Delegating to
signInWithOtpavoids code duplication since both operations trigger the same OTP flow on Supabase's end.app/components/login.gts (3)
63-77: Good accessibility for OTP input.The OTP input includes proper attributes:
inputmode="numeric",pattern="[0-9]*",autocomplete="one-time-code", and a screen-reader-only label. The centered tracking-widest styling is also appropriate for code entry.
225-231: Clean state reset in backToEmail.Properly clears OTP code and messages when returning to the email step, ensuring a fresh state for retry.
210-223: Resend functionality is well-implemented.Separate loading state (
resendLoading) prevents UI confusion with the main submit button. Success message confirms the resend action.supabase-schema.sql (5)
34-50: LGTM!The colors table structure is well-designed with appropriate constraints, foreign keys with CASCADE deletes, and proper bounds checking for RGBA values.
75-108: LGTM!The RLS policies provide comprehensive security coverage for all CRUD operations on both tables, correctly scoping access to the authenticated user via
auth.uid().
182-206: LGTM!The view correctly aggregates colors into a JSONB array using appropriate null handling with
COALESCEandFILTER. The structure matches the Orbit.js format expected by the frontend.
241-261: LGTM!The color migration function correctly extracts scalar values from JSONB using the
->>operator and applies appropriate type casts with proper default handling for the optional alpha channel.
1-261: Verify the schema in a Supabase test environment.Before deploying to production, thoroughly test this schema in a Supabase environment to ensure:
- The UNIQUE constraint fix allows multiple regular palettes per user
- Sample data creation populates
color_orderwith valid UUIDs- Migration functions correctly handle various Orbit.js data formats
- RLS policies work correctly with Supabase's
auth.uid()functionConsider creating integration tests that:
- Create multiple regular palettes for a user (validates constraint fix)
- Call
create_sample_color_history_for_user()and verify the color_order array- Test the migration functions with sample Orbit.js exports
| supabase: { | ||
| url: string; | ||
| anonKey: string; | ||
| }; | ||
| api?: { | ||
| host: string; | ||
| }; | ||
| } |
There was a problem hiding this comment.
Add runtime assertion for supabase configuration.
The interface marks supabase as required, but there's no runtime assertion like other critical config fields (lines 7-26). Missing supabase config will cause runtime errors when the Supabase client initializes.
🔎 Suggested fix
Add this assertion after line 26:
assert(
'supabase was not detected on your config',
'supabase' in config &&
typeof config.supabase === 'object' &&
config.supabase !== null &&
'url' in config.supabase &&
'anonKey' in config.supabase
);🤖 Prompt for AI Agents
In app/config/environment.ts around lines 36 to 43, the supabase config is
declared as required in the type but lacks a runtime assertion like other
critical fields; add a runtime assertion immediately after line 26 that verifies
config.supabase exists, is a non-null object, and contains both url and anonKey
keys, and throw/assert with a clear message if the check fails so missing
supabase config is caught at startup.
- Remove UNIQUE(user_id, is_color_history) constraint that prevented multiple regular palettes per user (FALSE values treated as identical) - Add partial unique index that only enforces uniqueness for color history rows (WHERE is_color_history = TRUE) - Move constraint to indexes section for proper organization
- Update from outdated 2.49.4 to latest stable 2.89.0 - All tests pass with the updated client
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
supabase-schema.sql (2)
158-164: Critical: Multi-row INSERT with RETURNING INTO still only captures first UUID.This issue was flagged in the previous review and remains unresolved. PostgreSQL's
RETURNING ... INTOsyntax only populates variables from the first returned row, leavingcolor_uuid2andcolor_uuid3as NULL. This causes thecolor_orderarray (lines 168-172) to contain null UUIDs, breaking color references.🔎 Proposed fix with separate INSERTs
- -- Create sample colors - INSERT INTO colors (user_id, palette_id, name, r, g, b, a) - VALUES - (user_uuid, palette_uuid, 'Pure Black', 0, 0, 0, 1.0), - (user_uuid, palette_uuid, 'Pure White', 255, 255, 255, 1.0), - (user_uuid, palette_uuid, 'Pure Red', 255, 0, 0, 1.0) - RETURNING id INTO color_uuid1, color_uuid2, color_uuid3; + -- Create sample colors (separate inserts to capture each UUID) + INSERT INTO colors (user_id, palette_id, name, r, g, b, a) + VALUES (user_uuid, palette_uuid, 'Pure Black', 0, 0, 0, 1.0) + RETURNING id INTO color_uuid1; + + INSERT INTO colors (user_id, palette_id, name, r, g, b, a) + VALUES (user_uuid, palette_uuid, 'Pure White', 255, 255, 255, 1.0) + RETURNING id INTO color_uuid2; + + INSERT INTO colors (user_id, palette_id, name, r, g, b, a) + VALUES (user_uuid, palette_uuid, 'Pure Red', 255, 0, 0, 1.0) + RETURNING id INTO color_uuid3;
234-234: Major: JSONB type mismatch in colorOrder extraction remains unresolved.This issue was flagged in the previous review and is still present. Line 234 uses the
->>operator which extracts JSON as TEXT, then attempts toCOALESCEwith a JSONB literal. Since thecolor_ordercolumn expects JSONB (line 22), this creates a type mismatch that may cause runtime errors or implicit conversions.🔎 Proposed fix using single-arrow operator
- COALESCE(palette_data->>'colorOrder', '[]'::jsonb) + COALESCE(palette_data->'colorOrder', '[]'::jsonb)Use
->(returns JSONB) instead of->>(returns TEXT) when the target column is JSONB.
🧹 Nitpick comments (1)
supabase-schema.sql (1)
5-6: Optional: Remove unused uuid-ossp extension.The schema uses
gen_random_uuid()(line 12) which is a native PostgreSQL function (available since PostgreSQL 13) and doesn't require theuuid-osspextension. Theuuid-osspextension provides functions likeuuid_generate_v4(), which aren't used here.🔎 Proposed simplification
--- Enable UUID extension for generating IDs -CREATE EXTENSION IF NOT EXISTS "uuid-ossp";If you need to support PostgreSQL versions older than 13, you could use
pgcryptoextension instead, which also providesgen_random_uuid().
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
supabase-schema.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). (1)
- GitHub Check: Tests
🔇 Additional comments (7)
supabase-schema.sql (7)
11-27: LGTM: Palettes table definition is well-structured.The removal of the problematic
UNIQUE (user_id, is_color_history)constraint (from the previous review) correctly allows multiple regular palettes per user, while the partial unique index at lines 69-70 enforces the intended single-color-history-per-user rule.
32-48: LGTM: Colors table definition is well-constrained.The table properly enforces valid RGBA ranges, includes appropriate CASCADE behavior for foreign keys, and maintains data integrity with NOT NULL checks.
68-70: Excellent: Partial unique index correctly enforces single color history per user.This implementation properly addresses the previous constraint issue by ensuring uniqueness only for color history palettes (
WHERE is_color_history = TRUE) while allowing unlimited regular palettes.
76-110: LGTM: Row Level Security policies are correctly implemented.All policies properly restrict access to user-owned data using
auth.uid() = user_id, with appropriate use ofUSINGfor row visibility andWITH CHECKfor insert validation.
117-132: LGTM: Timestamp update triggers are properly configured.The trigger function and table triggers follow PostgreSQL best practices for automatically maintaining
updated_attimestamps.
184-208: LGTM: View efficiently preloads colors with proper user scoping.The JSONB aggregation with
FILTERandCOALESCEcorrectly handles palettes without colors, and theauth.uid()filter ensures proper row-level security.
243-263: LGTM: Color migration function correctly extracts and casts values.The function properly uses
->>for text extraction followed by explicit casting to the appropriate types (INTEGER for RGB, REAL for alpha), with a sensible default for the alpha channel.
PostgreSQL only populates INTO from the first returned row in a multi-row INSERT. Split into three separate INSERT statements so each color UUID variable is properly populated.
Use single-arrow operator (->) instead of double-arrow (->>) for colorOrder to return JSONB instead of text, matching the column type.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
supabase-schema.sql (1)
5-6: Optional: uuid-ossp extension is unused.The schema creates the
uuid-osspextension but usesgen_random_uuid()throughout (lines 12, 33), which is available natively in PostgreSQL 13+ and Supabase. Theuuid-osspextension providesuuid_generate_v4()instead. Since Supabase runs modern PostgreSQL, this extension declaration is harmless but unnecessary.🔎 Optional cleanup
You can either remove the extension or use its functions:
-CREATE EXTENSION IF NOT EXISTS "uuid-ossp"; +-- gen_random_uuid() is available natively in PostgreSQL 13+Or keep the extension and use its function:
- id UUID DEFAULT gen_random_uuid() PRIMARY KEY, + id UUID DEFAULT uuid_generate_v4() PRIMARY KEY,
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
supabase-schema.sql
🔇 Additional comments (7)
supabase-schema.sql (7)
32-48: LGTM: Colors table structure is well-designed.The RGBA constraints are properly defined with appropriate ranges (0-255 for RGB, 0.0-1.0 for alpha), and the CASCADE delete ensures data integrity when palettes are removed. The foreign key relationships are correct.
54-70: LGTM: Excellent index strategy.The use of partial indexes for boolean fields (lines 57-58, 69-70) and GIN index for JSONB queries (line 66) demonstrates good PostgreSQL optimization practices. The partial unique index correctly ensures only one color history palette per user.
76-110: LGTM: RLS policies correctly implement data isolation.All policies properly restrict access using
auth.uid() = user_id, ensuring users can only interact with their own data. The comprehensive coverage of SELECT, INSERT, UPDATE, and DELETE operations provides complete multi-tenant security.
117-132: LGTM: Standard timestamp update pattern.The trigger function and BEFORE UPDATE triggers correctly implement automatic
updated_atmaintenance, following PostgreSQL best practices.
139-182: LGTM: Sample data function correctly creates initial palette.The function properly handles UUID capture with separate INSERT statements (addressing the previous RETURNING INTO limitation), constructs JSONB color references correctly, and provides a useful onboarding experience with pre-populated sample colors.
189-213: LGTM: View efficiently aggregates palette data.The view correctly aggregates colors into JSONB format using
FILTERto exclude NULL joins, provides proper GROUP BY clauses, and includes RLS filtering for security. This will support efficient querying of palettes with their colors.
248-268: LGTM: Color migration function is well-structured.The function correctly extracts color properties from JSONB, performs appropriate type casting, and handles the optional alpha channel with a sensible default of 1.0.
| CREATE TABLE IF NOT EXISTS palettes ( | ||
| id UUID DEFAULT gen_random_uuid() PRIMARY KEY, | ||
| user_id UUID REFERENCES auth.users(id) ON DELETE CASCADE, | ||
| created_at TIMESTAMP WITH TIME ZONE DEFAULT NOW(), | ||
| updated_at TIMESTAMP WITH TIME ZONE DEFAULT NOW(), | ||
| name TEXT, | ||
| is_color_history BOOLEAN DEFAULT FALSE, | ||
| is_favorite BOOLEAN DEFAULT FALSE, | ||
| is_locked BOOLEAN DEFAULT FALSE, | ||
| selected_color_index INTEGER DEFAULT 0, | ||
| sort_index INTEGER DEFAULT 0, | ||
| color_order JSONB DEFAULT '[]'::jsonb, | ||
|
|
||
| -- Constraints | ||
| CONSTRAINT palettes_user_not_null CHECK (user_id IS NOT NULL), | ||
| CONSTRAINT palettes_name_length CHECK (char_length(name) <= 255) | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, verify the actual schema content
find . -name "supabase-schema.sql" -o -name "*schema*.sql" | head -5Repository: shipshapecode/swach
Length of output: 84
🏁 Script executed:
# Search for palette name references in the codebase
rg -n "palette" --type=ts --type=js --type=tsx --type=jsx -C 2 'name' -g '!node_modules/**' -g '!dist/**' -g '!.next/**' | head -150Repository: shipshapecode/swach
Length of output: 90
🏁 Script executed:
# Look for any schema comments or migration notes about palettes
rg -n "palette" --type=sql -C 3 | head -100Repository: shipshapecode/swach
Length of output: 5814
🏁 Script executed:
# Check if there are any UI forms or creation logic for palettes
rg -n "palette" --type=ts --type=tsx -C 2 'create|form|input' -g '!node_modules/**' -g '!dist/**' | head -80Repository: shipshapecode/swach
Length of output: 90
🏁 Script executed:
# Search for palette creation logic in TypeScript files
rg -n "palette" --type=ts --type=js -C 3 'create|insert|new' -g '!node_modules/**' -g '!dist/**' | head -120Repository: shipshapecode/swach
Length of output: 120
🏁 Script executed:
# Look for palette form or creation components
fd -e ts -e js | xargs rg -l "palette" | head -10Repository: shipshapecode/swach
Length of output: 368
🏁 Script executed:
# Check the sample data function to see how it creates palettes
sed -n '138,180p' supabase-schema.sqlRepository: shipshapecode/swach
Length of output: 1465
🏁 Script executed:
# Search for any database operations or insert statements for palettes
rg -n "palettes" --type=ts --type=js -C 2 'insert|INSERT' -g '!node_modules/**' -g '!dist/**'Repository: shipshapecode/swach
Length of output: 116
🏁 Script executed:
# Check test fixtures for palette data
cat tests/orbit/fixtures/palettes.jsRepository: shipshapecode/swach
Length of output: 968
🏁 Script executed:
# Search for palette creation in tests
rg -n "palette" tests/ --type=js --type=ts -C 2 | head -80Repository: shipshapecode/swach
Length of output: 5041
🏁 Script executed:
# Look at seed data
cat tests/orbit/seed.js | head -100Repository: shipshapecode/swach
Length of output: 2939
🏁 Script executed:
# Search for any .insert or .create patterns in TypeScript/JavaScript for palettes
rg -n "palettes" . --type=ts --type=js -g '!node_modules/**' -g '!dist/**' | grep -E '(insert|create|INSERT|CREATE)' | head -20Repository: shipshapecode/swach
Length of output: 45
🏁 Script executed:
# Look for addRecord or create patterns in application code that might create palettes
rg -n "addRecord.*palette\|createRecord.*palette" --type=ts --type=js -g '!node_modules/**' -g '!dist/**' -g '!tests/**' -B2 -A5Repository: shipshapecode/swach
Length of output: 45
🏁 Script executed:
# Check for any form components or creation dialogs
fd -e ts -e js -g '!node_modules/**' -g '!dist/**' -g '!tests/**' | xargs rg -l "palette" | head -15Repository: shipshapecode/swach
Length of output: 637
🏁 Script executed:
# Look at the palette model/type definition
fd -e ts -g '*palette*' -g '!node_modules/**' -g '!dist/**' -g '!tests/**'Repository: shipshapecode/swach
Length of output: 277
🏁 Script executed:
# List the top-level directory structure
ls -la | head -30Repository: shipshapecode/swach
Length of output: 1931
🏁 Script executed:
# Find all TypeScript files not in node_modules or tests
fd "\.ts$" --exclude "node_modules" --exclude "tests" --exclude "dist" | head -20Repository: shipshapecode/swach
Length of output: 654
🏁 Script executed:
# Look for palette data model or service
rg -n "palette" --type=ts --exclude "node_modules" --exclude "tests" -g "!dist" | head -30Repository: shipshapecode/swach
Length of output: 474
🏁 Script executed:
# Read the palette data model
cat app/data-models/palette.tsRepository: shipshapecode/swach
Length of output: 743
🏁 Script executed:
# Read the palettes controller
cat app/controllers/palettes.tsRepository: shipshapecode/swach
Length of output: 9008
🏁 Script executed:
# Check for any palette creation/form related components
fd "palette" -e ts app/ | grep -v node_modules | head -20Repository: shipshapecode/swach
Length of output: 140
Add NOT NULL constraint to palette name field
The name field in the palettes table should have a NOT NULL constraint. While the TypeScript model defines name as non-nullable (name!: string in app/data-models/palette.ts) and the application always provides a name when creating palettes (defaulting to 'Palette'), the database schema currently allows NULL values. This creates a mismatch between the type system and database constraints, allowing potential data integrity issues from direct database operations.
name TEXT NOT NULL,🤖 Prompt for AI Agents
In supabase-schema.sql around lines 11 to 27, the palettes.name column is
currently nullable but the app model expects a non-nullable string; change the
column definition to be NOT NULL (i.e., make name TEXT NOT NULL) and keep the
existing length check; if this is for an existing database, first update any
NULL name rows to a sensible default (e.g., 'Palette') and then ALTER TABLE to
set NOT NULL so the database matches the TypeScript model and prevents future
nulls.
| CREATE OR REPLACE FUNCTION migrate_orbit_palette_data( | ||
| user_uuid UUID, | ||
| palette_data JSONB, | ||
| color_data JSONB | ||
| ) | ||
| RETURNS UUID AS $$ | ||
| DECLARE | ||
| palette_uuid UUID; | ||
| BEGIN | ||
| -- Insert palette | ||
| INSERT INTO palettes (user_id, name, is_color_history, is_favorite, is_locked, selected_color_index, sort_index, color_order) | ||
| VALUES ( | ||
| user_uuid, | ||
| palette_data->>'name', | ||
| (palette_data->>'isColorHistory')::BOOLEAN, | ||
| (palette_data->>'isFavorite')::BOOLEAN, | ||
| (palette_data->>'isLocked')::BOOLEAN, | ||
| COALESCE((palette_data->>'selectedColorIndex')::INTEGER, 0), | ||
| COALESCE((palette_data->>'sortIndex')::INTEGER, 0), | ||
| COALESCE(palette_data->'colorOrder', '[]'::jsonb) | ||
| ) | ||
| RETURNING id INTO palette_uuid; | ||
|
|
||
| -- Return the new palette UUID for relationships | ||
| RETURN palette_uuid; | ||
| END; | ||
| $$ LANGUAGE plpgsql; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused color_data parameter.
The color_data parameter (line 223) is declared but never used in the function body. Since colors are migrated separately via migrate_orbit_color_data, this parameter appears to be unnecessary and misleading.
🔎 Proposed fix
CREATE OR REPLACE FUNCTION migrate_orbit_palette_data(
user_uuid UUID,
- palette_data JSONB,
- color_data JSONB
+ palette_data JSONB
)
RETURNS UUID AS $$📝 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.
| CREATE OR REPLACE FUNCTION migrate_orbit_palette_data( | |
| user_uuid UUID, | |
| palette_data JSONB, | |
| color_data JSONB | |
| ) | |
| RETURNS UUID AS $$ | |
| DECLARE | |
| palette_uuid UUID; | |
| BEGIN | |
| -- Insert palette | |
| INSERT INTO palettes (user_id, name, is_color_history, is_favorite, is_locked, selected_color_index, sort_index, color_order) | |
| VALUES ( | |
| user_uuid, | |
| palette_data->>'name', | |
| (palette_data->>'isColorHistory')::BOOLEAN, | |
| (palette_data->>'isFavorite')::BOOLEAN, | |
| (palette_data->>'isLocked')::BOOLEAN, | |
| COALESCE((palette_data->>'selectedColorIndex')::INTEGER, 0), | |
| COALESCE((palette_data->>'sortIndex')::INTEGER, 0), | |
| COALESCE(palette_data->'colorOrder', '[]'::jsonb) | |
| ) | |
| RETURNING id INTO palette_uuid; | |
| -- Return the new palette UUID for relationships | |
| RETURN palette_uuid; | |
| END; | |
| $$ LANGUAGE plpgsql; | |
| CREATE OR REPLACE FUNCTION migrate_orbit_palette_data( | |
| user_uuid UUID, | |
| palette_data JSONB | |
| ) | |
| RETURNS UUID AS $$ | |
| DECLARE | |
| palette_uuid UUID; | |
| BEGIN | |
| -- Insert palette | |
| INSERT INTO palettes (user_id, name, is_color_history, is_favorite, is_locked, selected_color_index, sort_index, color_order) | |
| VALUES ( | |
| user_uuid, | |
| palette_data->>'name', | |
| (palette_data->>'isColorHistory')::BOOLEAN, | |
| (palette_data->>'isFavorite')::BOOLEAN, | |
| (palette_data->>'isLocked')::BOOLEAN, | |
| COALESCE((palette_data->>'selectedColorIndex')::INTEGER, 0), | |
| COALESCE((palette_data->>'sortIndex')::INTEGER, 0), | |
| COALESCE(palette_data->'colorOrder', '[]'::jsonb) | |
| ) | |
| RETURNING id INTO palette_uuid; | |
| -- Return the new palette UUID for relationships | |
| RETURN palette_uuid; | |
| END; | |
| $$ LANGUAGE plpgsql; |
🤖 Prompt for AI Agents
In supabase-schema.sql around lines 220 to 246, the function
migrate_orbit_palette_data declares a color_data parameter that is never used;
remove the unused color_data parameter from the function signature and the
RETURNS block if necessary, update the function header to accept only (user_uuid
UUID, palette_data JSONB), and update any places that call
migrate_orbit_palette_data to stop passing the extra color_data argument so
callers match the new two-argument signature.
- Add dotenv as dev dependency - Load .env at top of config/environment.js - Allows SUPABASE_URL and SUPABASE_ANON_KEY to be read from .env file
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
config/environment.js (1)
67-71: Consider removing redundant production override.The production block sets
ENV.supabaseto the exact same values as the base configuration at lines 27-30. Since the base configuration already reads fromprocess.env, this override is unnecessary.🔎 Proposed simplification
The production block can be removed entirely since the base configuration already handles this:
- if (environment === 'production') { - // Production Supabase config from environment variables - ENV.supabase = { - url: process.env.SUPABASE_URL || '', - anonKey: process.env.SUPABASE_ANON_KEY || '', - }; - }Alternatively, if you want to keep it for documentation purposes, add validation as suggested in the previous comment to ensure required values are present.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
config/environment.jspackage.json
⏰ 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). (1)
- GitHub Check: Tests
🔇 Additional comments (4)
package.json (2)
147-147: LGTM!Placing
dotenvindevDependenciesis appropriate since it's only needed during the build process to load environment variables inconfig/environment.js.
67-67: Version 2.89.0 is current and secure. The upgrade from 2.49.4 is substantial. As of January 2026, version 2.89.0 remains the latest release with no known security vulnerabilities.config/environment.js (2)
3-3: LGTM!Loading dotenv at the top is standard practice for local development. The
dotenvpackage handles missing.envfiles gracefully, so this is safe.
59-63: LGTM!Using mock Supabase credentials in the test environment is the correct approach to ensure tests don't require real Supabase configuration.
| supabase: { | ||
| url: process.env.SUPABASE_URL || '', | ||
| anonKey: process.env.SUPABASE_ANON_KEY || '', | ||
| }, |
There was a problem hiding this comment.
Add validation for required Supabase environment variables.
The PR objectives document that SUPABASE_URL and SUPABASE_ANON_KEY are required breaking changes, but the current implementation falls back to empty strings if they're missing. This will cause cryptic runtime failures rather than failing fast with a clear error during the build or startup.
Consider adding validation that throws an error if these required values are missing in production environments.
🔎 Proposed validation for required config
+ // Validate required Supabase config in production
+ if (environment === 'production') {
+ if (!process.env.SUPABASE_URL || !process.env.SUPABASE_ANON_KEY) {
+ throw new Error(
+ 'Missing required Supabase configuration. ' +
+ 'SUPABASE_URL and SUPABASE_ANON_KEY environment variables must be set in production.'
+ );
+ }
+ }
+
supabase: {
url: process.env.SUPABASE_URL || '',
anonKey: process.env.SUPABASE_ANON_KEY || '',
},Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In config/environment.js around lines 27 to 30, the Supabase config silently
falls back to empty strings for SUPABASE_URL and SUPABASE_ANON_KEY; update the
file to validate these env vars and fail fast by throwing a descriptive Error if
either is missing (at least in production/startup), e.g. read
process.env.SUPABASE_URL and process.env.SUPABASE_ANON_KEY into variables, check
they are non-empty, and if not throw new Error('Missing required environment
variable SUPABASE_URL') / 'SUPABASE_ANON_KEY' (or a combined message) so the
process exits with a clear failure instead of proceeding with empty values.
| # compiled output | ||
| /dist/ | ||
| /electron-app/dist/ | ||
| /electron-app/ember-test/ |
There was a problem hiding this comment.
| /electron-app/ember-test/ |
I do not think there is an electron-app/ember-test folder anymore.
|
|
||
| # compiled output | ||
| /dist/ | ||
| /electron-app/ember-test/ |
There was a problem hiding this comment.
| /electron-app/ember-test/ |
| id: string; | ||
| userId: string; | ||
| email: string; | ||
| accessToken: string; |
There was a problem hiding this comment.
Should any of these properties be inherited from a Supabase library or something? Just wondering if they might ever change and if Supabase has standards setup around this?
| @@ -0,0 +1,3 @@ | |||
| import Register from '../../../components/register.gts'; | |||
|
|
|||
| <template><Register /></template> | |||
There was a problem hiding this comment.
If we always email a code, instead of using passwords, I wonder if we should combine login and register into one route?
| 'declarations/**/*', | ||
| 'dist/**/*', | ||
| 'electron-app/dist/**/*', | ||
| 'electron-app/ember-test/**/*', |
There was a problem hiding this comment.
| 'electron-app/ember-test/**/*', |
| palettes = palettes.map((p) => { | ||
| const { id, type, attributes, relationships } = p; | ||
|
|
||
| if (relationships?.['colors']?.data) { |
There was a problem hiding this comment.
I'm seeing a lot of deleted logic here. Was this moved somewhere or is it no longer needed? I think Dan wrote a lot of this data stuff so I am less familiar with why we were doing all this.
Summary
Changes
supabaseservice and authenticator with OTP methods (signInWithOtp,verifyOtp)SupabaseSourcefor Orbit.js syncsupabase-schema.sqlwith palettes/colors tables, RLS policies, and migration helpersBreaking Changes
Requires environment variables:
Future Enhancements
Summary by CodeRabbit
New Features
Removed Features
UI Changes
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.