♻️ Migrate from AWS Cognito to Supabase OTP authentication #2469
Conversation
📝 WalkthroughWalkthroughThis pull request migrates the application from AWS Cognito authentication and JSON:API backend to Supabase-based authentication and data storage. The migration removes Cognito-dependent components, introduces a new OTP-based login flow via Supabase, replaces the JSON:API data source with a Supabase source, updates the database schema, and reconfigures environment variables accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LoginComponent
participant SupabaseService
participant SupabaseAuth
participant Authenticator
participant Session
participant Router
User->>LoginComponent: Enter email, click "Send Code"
LoginComponent->>SupabaseService: signInWithOtp(email)
SupabaseService->>SupabaseAuth: signInWithOtp(email)
SupabaseAuth-->>SupabaseService: OTP sent
SupabaseService-->>LoginComponent: success
LoginComponent->>LoginComponent: step = 'otp'
User->>LoginComponent: Enter OTP code, click "Verify Code"
LoginComponent->>Session: authenticate({email, token}, 'supabase')
Session->>Authenticator: authenticate(credentials)
Authenticator->>SupabaseService: verifyOtp(email, token)
SupabaseService->>SupabaseAuth: verifyOtp(email, token)
SupabaseAuth-->>SupabaseService: {user, session}
SupabaseService-->>Authenticator: {user, session}
Authenticator-->>Session: SupabaseAuthData
Session-->>LoginComponent: authenticated
LoginComponent->>Router: transitionTo('settings.cloud.profile')
Router-->>User: Navigate to profile
sequenceDiagram
participant App
participant DataService
participant SupabaseSource
participant SupabaseClient
participant RemoteDB
App->>DataService: synchronize()
DataService->>SupabaseSource: queryPalettes()
SupabaseSource->>SupabaseClient: fetch palettes & colors
SupabaseClient->>RemoteDB: SELECT * FROM palettes_with_colors
RemoteDB-->>SupabaseClient: palette rows with color data
SupabaseClient-->>SupabaseSource: results
SupabaseSource-->>DataService: Orbit records
DataService->>DataService: merge local & remote
DataService->>SupabaseSource: addRecord(palette)
SupabaseSource->>SupabaseClient: insert palette
SupabaseClient->>RemoteDB: INSERT INTO palettes
RemoteDB-->>SupabaseClient: success
DataService->>SupabaseSource: addRecord(color)
SupabaseSource->>SupabaseClient: insert color
SupabaseClient->>RemoteDB: INSERT INTO colors
RemoteDB-->>SupabaseClient: success
SupabaseSource-->>DataService: sync complete
DataService-->>App: synchronized
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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.
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.
|
@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.
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.
| 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.
- 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
- 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
- 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
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.
- 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
RobbieTheWagner
force-pushed
the
feature/supabase-otp-migration
branch
from
5712ef0 to
424f741
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @app/services/session.ts:
- Around line 7-15: You have a duplicate SupabaseAuthData interface in
session.ts that conflicts with the one in app/authenticators/supabase.ts; remove
the local interface definition in session.ts and import the exported
SupabaseAuthData from the authenticator module instead, then update any local
type references to use that imported symbol; if the authenticator does not
export SupabaseAuthData, export it from app/authenticators/supabase.ts first so
session.ts can import and reuse the single canonical interface.
In @docs/SUPABASE_MIGRATION.md:
- Around line 106-113: Update the documentation to accurately reflect that
real-time sync is not currently active: change phrasing like "Changes sync
automatically across devices" to indicate real-time sync is "available" or
"planned" rather than active, and add a brief note that the code contains an
implementation (setupRealtimeSync) but it is not invoked by the data service
(activate() does not call setupRealtimeSync, synchronize() performs one-time
fetches, and autoActivate = false). Mention these symbols (setupRealtimeSync,
activate, synchronize, autoActivate) to make it clear why the feature is dormant
and that activation will require wiring setupRealtimeSync into activate() or
enabling autoActivate.
In @supabase-schema.sql:
- Around line 32-48: The colors table currently contradicts itself by defining
palette_id as nullable in the column declaration but enforcing NOT NULL via
CONSTRAINT colors_palette_not_null; decide desired behavior and apply one of two
fixes: (A) if orphaned colors must be allowed, remove the CHECK constraint
colors_palette_not_null from the colors table definition so palette_id can be
null and leave app/data-sources/remote.ts unchanged, or (B) if colors must
always belong to a palette, keep the constraint and update the orphaning logic
in app/data-sources/remote.ts (around the code that sets palette_id to null) to
instead delete the color records (or reassign them) — modify the function
handling palette deletion/orphaning so it does not set palette_id = NULL but
performs deletion or reassignment consistent with the NOT NULL constraint.
- Around line 220-246: In migrate_orbit_palette_data change the JSON key used
for the palette sort index so the DB column sort_index receives the correct
value: replace extraction of palette_data->>'sortIndex' with
palette_data->>'index' (keep the COALESCE cast to INTEGER and default 0),
ensuring INSERT INTO palettes (… sort_index …) uses the correct source;
references: function migrate_orbit_palette_data, palette_data, sort_index.
🧹 Nitpick comments (8)
docs/SUPABASE_MIGRATION.md (1)
45-45: Format the bare URL as a markdown link.The URL should be formatted as a markdown link to comply with markdown linting rules and improve readability.
📝 Proposed fix
-1. Create a new Supabase project at https://supabase.com +1. Create a new Supabase project at [https://supabase.com](https://supabase.com)app/data-sources/remote.ts (3)
317-333: Consider batch updates for relationship operations.The
replaceRelatedRecordsmethod updates colors individually in a loop, which could result in N network round-trips for N colors. For better performance, consider using Supabase's batch update capabilities or a single update with an IN clause.⚡ Suggested optimization
private async replaceRelatedRecords( record: RecordIdentity, relationship: string, relatedRecords: RecordIdentity[] ): Promise<void> { if (record.type === 'palette' && relationship === 'colors') { - // Update all colors to point to this palette - for (const color of relatedRecords) { - const { error } = await this.supabase - .from('colors') - .update({ palette_id: record.id }) - .eq('id', color.id); - - if (error) throw new Error(`Supabase update error: ${error.message}`); - } + // Update all colors in a single query + const colorIds = relatedRecords.map(c => c.id); + const { error } = await this.supabase + .from('colors') + .update({ palette_id: record.id }) + .in('id', colorIds); + + if (error) throw new Error(`Supabase update error: ${error.message}`); } }
352-366: Apply the same batch optimization pattern.Similar to
replaceRelatedRecords, theaddToRelatedRecordsmethod would benefit from the same batch update optimization if multiple records need to be added at once.
511-522: Document the factory function parameters.The factory function accepts
injectionswith a default schema but lacks documentation explaining what injections are expected and how this integrates with Ember Orbit's initialization flow.📝 Suggested documentation
+/** + * Factory function to create a SupabaseSource for Ember Orbit. + * + * @param injections - Source injections including schema, name, and autoActivate + * @returns A configured SupabaseSource instance + * + * The factory applies standard Ember Orbit source injections and configures + * the source as 'remote' with manual activation. + */ export default { create( injections: SupabaseSourceInjections = { schema: {} as RecordSchema } ) {app/templates/settings/cloud/profile.gts (1)
9-20: Consider UX when email is unavailable.The heading "Signed in as..." is displayed unconditionally, but if
@model.emailis missing, the user sees "Signed in as..." with nothing below it, which could be confusing. Consider either:
- Showing
userIdas a fallback identifier- Conditionally adjusting the heading text
♻️ Optional: Show userId as fallback
<h2 class="font-bold mb-6 text-2xl"> Signed in as... </h2> {{#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" + > + User ID: {{@model.userId}} + </div> {{/if}}config/environment.js (1)
27-30: Remove redundant production Supabase config override.The base
ENV.supabaseconfiguration (lines 27-30) already reads fromprocess.env.SUPABASE_URLandprocess.env.SUPABASE_ANON_KEY. The production block (lines 66-72) duplicates this exact configuration unnecessarily.♻️ Remove redundant production override
if (environment === 'production') { - // Production Supabase config from environment variables - ENV.supabase = { - url: process.env.SUPABASE_URL || '', - anonKey: process.env.SUPABASE_ANON_KEY || '', - }; + // Supabase config already set from environment variables in base ENV }Or simply remove the entire production
ifblock if it has no other purpose.Also applies to: 66-72
tests/acceptance/settings/cloud-test.ts (1)
71-72: Unused stub:signOutis stubbed but never invoked in this test.The
signOutstub on line 72 is never called or verified during the "user can verify OTP and log in" test. If sign-out behavior isn't being tested here, consider removing the stub to keep tests focused. If it's preparation for future logout testing mentioned in PR notes, consider adding a comment.app/components/login.gts (1)
180-185: Add numeric validation for OTP code.The validation at line 182 checks length but not format. Users could potentially enter non-numeric characters like "abcdef" which would pass the length check but fail server-side. While the HTML input has
pattern="[0-9]*", this doesn't prevent programmatic or paste input of non-numeric characters.♻️ Add numeric validation
@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; }
📜 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 (26)
app/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.mdpackage.jsonsupabase-schema.sqltests/acceptance/settings/cloud-test.ts
💤 Files with no reviewable changes (9)
- app/components/register-confirm.gts
- app/components/forgot-password.gts
- app/data-sources/remote.js
- app/templates/settings/cloud/forgot-password.gts
- app/services/cognito.js
- app/routes/settings/cloud/index.ts
- app/templates/settings/cloud/register/confirm.gts
- app/authenticators/cognito.js
- app/templates/settings/cloud/register/index.gts
🚧 Files skipped from review as they are similar to previous changes (7)
- package.json
- app/router.ts
- app/services/data.ts
- app/templates/settings/cloud/register.gts
- app/config/environment.ts
- app/services/supabase.ts
- app/authenticators/supabase.ts
🧰 Additional context used
🧬 Code graph analysis (4)
app/routes/settings/cloud/profile.ts (1)
app/routes/settings/cloud/index.ts (1)
SettingsAccountRoute(9-26)
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/data-sources/remote.ts (2)
app/services/supabase.ts (1)
SupabaseService(23-165)app/services/session.ts (1)
SessionService(17-31)
🪛 markdownlint-cli2 (0.18.1)
docs/SUPABASE_MIGRATION.md
45-45: Bare URL used
(MD034, no-bare-urls)
⏰ 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 (14)
app/components/register.gts (1)
1-45: Excellent simplification for OTP flow!The conversion to a template-only component that redirects to the login flow is the right approach for OTP authentication. The user messaging clearly explains that accounts are created automatically, which improves the user experience by removing friction from the signup process.
app/data-sources/remote.ts (2)
81-86: Consider consistent error messaging.The error message "Remote requests require authentication" is clear and appropriate. This pattern is repeated throughout the file, which is good for consistency.
482-500: Consider filtering PostgreSQL change events by type (INSERT, UPDATE, DELETE) rather than listening to all events.The real-time subscription currently listens to all PostgreSQL events (
event: '*') on both tables. Since the callback only needs specific event types, filtering byevent: 'INSERT','UPDATE', or'DELETE'would reduce network traffic and callback invocations. Determine which event types are actually required for your use case and filter accordingly to improve performance.supabase-schema.sql (2)
55-70: Well-designed index strategy.The index design is thoughtful:
- Partial indexes on boolean flags reduce index size
- GIN index on JSONB column for efficient array operations
- Unique partial index ensuring one color history per user
- Coverage of foreign keys and common query patterns
77-110: RLS policies provide strong data isolation.The Row Level Security policies correctly enforce user data isolation for all CRUD operations on both tables. Using
auth.uid()ensures that users can only access their own data, which aligns with the security requirements mentioned in the documentation.app/templates/settings/cloud/profile.gts (1)
34-38: LGTM!The type update correctly reflects the new Supabase-based session data model, removing
email_verified(no longer needed with OTP) and addinguserId.app/routes/settings/cloud/profile.ts (1)
7-22: LGTM!Clean migration from Cognito to session-based authentication data. The
ProfileModelinterface clearly documents the expected shape, and optional chaining appropriately handles cases where session data may be missing. Route protection is handled by the parent route'sbeforeModelhook (as seen inapp/routes/settings/cloud/index.ts).config/environment.js (1)
58-63: LGTM on test configuration.Using mock Supabase credentials for tests ensures test isolation and prevents accidental calls to real Supabase endpoints.
tests/acceptance/settings/cloud-test.ts (2)
64-114: Comprehensive OTP verification test.The test correctly covers the full OTP flow: email entry → OTP request → code entry → authentication → navigation to profile. The stubbed authenticator response properly includes all required session data fields. Good use of
deepEqualto verify the exact credentials passed to the authenticator.
138-169: Good error handling coverage.The test properly verifies that:
- Invalid OTP triggers an error from the authenticator
- Error message is displayed to the user (
.bg-red-400)- User remains on the login page
This ensures a graceful failure experience for users entering incorrect codes.
app/components/login.gts (4)
158-178: LGTM onsendOtpimplementation.The email validation, loading state management, error handling, and step transition are all handled correctly. The success message provides clear user feedback about what to expect next.
210-223: LGTM onresendCodeimplementation.Properly uses separate
resendLoadingstate to avoid interfering with the main loading indicator, and provides appropriate success/error feedback. Good UX for users who need to request a new code.
225-231: LGTM onbackToEmailimplementation.Correctly resets all OTP-related state when returning to the email step, providing a clean slate for re-entry.
63-77: Good OTP input accessibility attributes.The input correctly uses
inputmode="numeric"for mobile keyboards,autocomplete="one-time-code"for browser autofill from SMS, andpattern="[0-9]*"for validation hints. Thesr-onlylabel ensures screen reader accessibility.
| ## Real-time Sync | ||
|
|
||
| Supabase enables real-time sync capabilities: | ||
|
|
||
| - Changes sync automatically across devices | ||
| - Other users' changes are invisible (RLS) | ||
| - Offline changes queue and sync when online | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for real-time sync activation in the data service and application initialization
rg -nP -C3 'setupRealtimeSync|realtime.*sync' --type=ts -g '!node_modules/**'Repository: shipshapecode/swach
Length of output: 500
🏁 Script executed:
#!/bin/bash
# Search for invocations of setupRealtimeSync (where it's called, not defined)
rg -nP 'setupRealtimeSync|teardownRealtimeSync' --type=ts -g '!node_modules/**' | head -20Repository: shipshapecode/swach
Length of output: 237
🏁 Script executed:
#!/bin/bash
# Look at the full implementation of setupRealtimeSync in remote.ts
cat -n app/data-sources/remote.ts | sed -n '482,530p'Repository: shipshapecode/swach
Length of output: 1427
🏁 Script executed:
#!/bin/bash
# Check for test files related to real-time sync
fd -e test.ts -e spec.ts -e test.tsx -e spec.tsx | xargs grep -l 'realtime\|setupRealtime' 2>/dev/nullRepository: shipshapecode/swach
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Search for where RemoteDataSource is instantiated/initialized to see if setupRealtimeSync is called
rg -nP 'new RemoteDataSource|setupRealtimeSync' --type=ts --type=tsx -g '!node_modules/**'Repository: shipshapecode/swach
Length of output: 90
🏁 Script executed:
#!/bin/bash
# Search without type restriction to find any invocations
rg -nP 'setupRealtimeSync|teardownRealtimeSync' --type-list | head -20Repository: shipshapecode/swach
Length of output: 452
🏁 Script executed:
#!/bin/bash
# Search for any invocations of setupRealtimeSync in all files
rg 'setupRealtimeSync|teardownRealtimeSync' -g '!node_modules/**' | grep -v 'app/data-sources/remote.ts'Repository: shipshapecode/swach
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Look for app initialization, bootstrap, or setup files
fd -e js -e ts -e tsx | xargs grep -l 'SupabaseSource\|RemoteDataSource\|setupRealtimeSync' 2>/dev/null | head -20Repository: shipshapecode/swach
Length of output: 110
🏁 Script executed:
#!/bin/bash
# Check if the data source is ever activated in the app
rg -nP 'activate|remote.*data' --type=ts -A 2 -B 2 | head -40Repository: shipshapecode/swach
Length of output: 2214
🏁 Script executed:
#!/bin/bash
# Check the data service to see what activate() and synchronize() do
cat -n app/services/data.ts | head -100Repository: shipshapecode/swach
Length of output: 4049
🏁 Script executed:
#!/bin/bash
# Search for setupRealtimeSync calls in data.ts
rg -n 'setupRealtimeSync' app/services/data.tsRepository: shipshapecode/swach
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Look at the synchronize method in data service
rg -nP 'synchronize.*\(' app/services/data.ts -A 20Repository: shipshapecode/swach
Length of output: 970
🏁 Script executed:
#!/bin/bash
# Look at the SUPABASE_MIGRATION.md file around the real-time sync section
cat -n docs/SUPABASE_MIGRATION.md | sed -n '100,120p'Repository: shipshapecode/swach
Length of output: 775
🏁 Script executed:
#!/bin/bash
# Check if there are any TODO or comments about real-time sync being planned
rg -n 'TODO|FIXME|planned|WIP.*realtime' --type=md docs/Repository: shipshapecode/swach
Length of output: 45
Clarify real-time sync status in documentation.
The real-time sync implementation exists in the codebase (setupRealtimeSync in app/data-sources/remote.ts), but it is not currently activated or integrated into the application. The activate() method in the data service does not call setupRealtimeSync, and synchronize() uses one-time data fetching rather than real-time subscriptions. The real-time sync capability is available but disabled (autoActivate = false).
Update the documentation to reflect the actual state: change "Changes sync automatically across devices" and related statements to indicate real-time sync is "available" or "planned for future versions" rather than implying it is an active current capability.
🤖 Prompt for AI Agents
In @docs/SUPABASE_MIGRATION.md around lines 106 - 113, Update the documentation
to accurately reflect that real-time sync is not currently active: change
phrasing like "Changes sync automatically across devices" to indicate real-time
sync is "available" or "planned" rather than active, and add a brief note that
the code contains an implementation (setupRealtimeSync) but it is not invoked by
the data service (activate() does not call setupRealtimeSync, synchronize()
performs one-time fetches, and autoActivate = false). Mention these symbols
(setupRealtimeSync, activate, synchronize, autoActivate) to make it clear why
the feature is dormant and that activation will require wiring setupRealtimeSync
into activate() or enabling autoActivate.
| 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.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the export format from the data service to verify field naming
rg -nP -A10 'export.*palette|serializeRecord|toJSON' app/services/data.ts app/data-models/palette.ts --type=tsRepository: shipshapecode/swach
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Find TypeScript files related to palette and data
find . -type f -name "*.ts" -o -name "*.tsx" | grep -E "(palette|data|orbit|service)" | head -30Repository: shipshapecode/swach
Length of output: 593
🏁 Script executed:
#!/bin/bash
# Check the palette data model
echo "=== app/data-models/palette.ts ==="
cat -n app/data-models/palette.ts
echo -e "\n=== app/services/data.ts (first 100 lines) ==="
head -100 app/services/data.ts | cat -nRepository: shipshapecode/swach
Length of output: 4939
🏁 Script executed:
#!/bin/bash
# Search for sortIndex references
echo "=== Searching for sortIndex ==="
rg -n "sortIndex" --type=ts --type=sql
echo -e "\n=== Searching for palette export/serialization ==="
rg -n -A5 "serialize|toJSON|export.*palette" app/data-sources/ app/services/ --type=ts | head -80Repository: shipshapecode/swach
Length of output: 226
🏁 Script executed:
#!/bin/bash
# Find the CREATE TABLE statement for palettes
rg -n -B5 -A30 "CREATE TABLE.*palettes" supabase-schema.sqlRepository: shipshapecode/swach
Length of output: 1563
Fix field name mismatch in migration function: index not sortIndex.
The migration function attempts to extract palette_data->>'sortIndex', but the Orbit.js model defines this field as index, not sortIndex. This mismatch will result in NULL values being inserted for sort_index in the database. Update line 238 from:
COALESCE((palette_data->>'sortIndex')::INTEGER, 0),to:
COALESCE((palette_data->>'index')::INTEGER, 0),The colorOrder JSONB structure mapping (storing array of {type, id} objects) is compatible with the database schema.
🤖 Prompt for AI Agents
In @supabase-schema.sql around lines 220 - 246, In migrate_orbit_palette_data
change the JSON key used for the palette sort index so the DB column sort_index
receives the correct value: replace extraction of palette_data->>'sortIndex'
with palette_data->>'index' (keep the COALESCE cast to INTEGER and default 0),
ensuring INSERT INTO palettes (… sort_index …) uses the correct source;
references: function migrate_orbit_palette_data, palette_data, sort_index.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @tests/acceptance/settings/cloud-test.ts:
- Line 154: Replace the brittle CSS selector used in the test: change the
assertion that targets assert.dom('.bg-red-400').exists(...) to
assert.dom('[data-test-error-message]').exists(...), and add a
data-test-error-message attribute to the error message element in the
corresponding component template (the element currently rendered with the
.bg-red-400 class) so the test targets the stable data-test hook rather than a
Tailwind class.
- Around line 62-63: Remove the unused stubs in the login test: delete the
sinon.stub for supabaseService.signOut() and the sinon.stub for session.reset()
since only signInWithOtp() is used in this flow; locate the calls to
sinon.stub(supabaseService, 'signOut') and sinon.stub(session, 'reset') in the
tests/acceptance/settings/cloud-test.ts login test and remove them so the test
only stubs supabaseService.signInWithOtp(), keeping references to
authenticator.invalidate() and session.handleInvalidation() in mind as those are
only exercised during logout.
🧹 Nitpick comments (2)
tests/acceptance/settings/cloud-test.ts (2)
28-33: Consider explicit stub restoration for test isolation.While
setupApplicationTestmay handle cleanup, explicitly restoring sinon stubs ensures test isolation and follows best practices. Consider usingsinon.restore()in a test teardown or storing stubs for individual restoration.♻️ Example pattern for explicit cleanup
test('user can request OTP code', async function (assert) { await visit('/settings/cloud/login'); const supabaseService = this.owner.lookup( 'service:supabase' ) as SupabaseService; const signInWithOtpStub = sinon .stub(supabaseService, 'signInWithOtp') .resolves(); + + // Cleanup after test + this.set('cleanup', () => signInWithOtpStub.restore()); await fillIn('[data-test-login-input-user]', '[email protected]'); await click('[data-test-login-submit]'); await waitForAll(); // assertions... + + this.cleanup(); });Alternatively, use
hooks.afterEach(() => sinon.restore())at the module level.
107-127: Consider removing redundant test.This test appears redundant with the earlier "user can request OTP code" test (lines 25-53), which already verifies:
signInWithOtpis called with the correct email- OTP input appears after requesting OTP
- Button text changes to "Verify Code"
Unless this test serves a specific distinct purpose, consider removing it to reduce maintenance overhead.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/components/register.gtsapp/router.tstests/acceptance/settings/cloud-test.ts
💤 Files with no reviewable changes (2)
- app/components/register.gts
- app/router.ts
🧰 Additional context used
🧬 Code graph analysis (1)
tests/acceptance/settings/cloud-test.ts (2)
app/services/supabase.ts (1)
SupabaseService(23-165)tests/helpers.ts (1)
waitForAll(24-38)
⏰ 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 (1)
tests/acceptance/settings/cloud-test.ts (1)
15-23: LGTM!The navigation test is well-structured and uses appropriate data-test attributes for reliable element selection.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @app/components/login.gts:
- Around line 184-211: The verifyOtp method currently only checks otpCode
length; add an explicit numeric format check using a regex like /^\d{6}$/ before
attempting authentication: in verifyOtp (and/or where otpCode is validated)
replace or augment the length check with a test that otpCode matches /^\d{6}$/
and if it fails set this.errorMessage = 'Please enter the 6-digit code' (or a
clearer 'Please enter a 6-digit numeric code') and return so authenticate isn't
called with non-numeric input; ensure this check runs before setting
this.loading or calling this.session.authenticate.
In @tests/acceptance/settings/cloud-test.ts:
- Around line 25-53: The test creates a sinon stub signInWithOtpStub on the
SupabaseService but never restores it, which can leak into other tests; fix by
restoring stubs after each test—either add a module-level hooks.afterEach that
calls sinon.restore() (preferred) or call signInWithOtpStub.restore() at the end
of this test; ensure the cleanup references the same stub variable name
signInWithOtpStub or simply uses sinon.restore() to clear all stubs.
- Line 81: The test currently stubs dataService.reset(), which is unnecessary
for the login flow being tested; remove the sinon.stub(dataService,
'reset').resolves() line so the test only stubs behaviors relevant to login
(locate the stub of dataService.reset in the test file and delete that stub
call), keeping other necessary stubs intact.
🧹 Nitpick comments (1)
app/components/login.gts (1)
162-181: Consider adding email format validation.The email validation only checks if the field is truthy but doesn't verify the email format. While the HTML5
type="email"attribute provides client-side validation, adding explicit format validation in the action provides better error messages and a more robust user experience.📧 Proposed enhancement for email validation
@action async sendOtp(): Promise<void> { if (!this.email) { this.errorMessage = 'Please enter your email address'; return; } + + // Basic email format validation + const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; + if (!emailRegex.test(this.email)) { + this.errorMessage = 'Please enter a valid email address'; + return; + } this.loading = true; this.errorMessage = undefined; this.successMessage = undefined;
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/components/login.gtstests/acceptance/settings/cloud-test.ts
🧰 Additional context used
🧬 Code graph analysis (1)
tests/acceptance/settings/cloud-test.ts (2)
app/services/supabase.ts (1)
SupabaseService(23-165)tests/helpers.ts (1)
waitForAll(24-38)
⏰ 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 (1)
app/components/login.gts (1)
214-226: Rate limiting is provided by Supabase by default.Supabase applies built-in rate limiting to OTP operations: 30 OTPs per hour and a 60-second minimum between sends to the same user. These limits protect against email bombing and abuse. If your application requires different limits, they can be customized via the Supabase Management API.
| async verifyOtp(): Promise<void> { | ||
| if (!this.otpCode || this.otpCode.length !== 6) { | ||
| this.errorMessage = 'Please enter the 6-digit code'; | ||
| return; | ||
| } | ||
|
|
||
| const { username, password } = this; | ||
| const credentials = { username, password }; | ||
| this.loading = true; | ||
| this.errorMessage = undefined; | ||
| this.successMessage = undefined; | ||
|
|
||
| try { | ||
| await this.session.authenticate('authenticator:cognito', credentials); | ||
| await this.session.authenticate('authenticator:supabase', { | ||
| email: this.email, | ||
| token: this.otpCode, | ||
| }); | ||
|
|
||
| // We want to skip this in tests, since once a user has logged in routes become inaccessible | ||
| if (!isTesting()) { | ||
| this.settings.set('userHasLoggedInBefore', true); | ||
| } | ||
|
|
||
| this.router.transitionTo('settings.cloud'); | ||
| this.router.transitionTo('settings.cloud.profile'); | ||
| } catch (error: unknown) { | ||
| this.errorMessage = (error as Error).message || (error as string); | ||
| this.errorMessage = (error as Error).message || 'Invalid or expired code'; | ||
| } finally { | ||
| this.loading = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Supabase authenticator OTP token validation numeric digits
💡 Result:
Short answer: Supabase OTPs are numeric 6‑digit pins by default for SMS (and the email OTP/magic‑code examples also use 6 digits). The client/server verify call accepts the token as a string (you send it to auth.verifyOtp); Supabase enforces the token match/expiry server‑side (SMS tokens expire ~60s). If you want client‑side validation, check for six numeric digits (e.g. /^\d{6}$/) before calling verifyOtp. [1][2]
Sources:
- Phone login / SMS OTP (6‑digit pin, 60s expiry). [1]
- verifyOtp API (token parameter; examples use 6‑digit tokens). [2]
Add explicit format validation for numeric-only OTP input.
The code validates OTP length but not format. While the HTML pattern="[0-9]*" attribute helps prevent bypass, Supabase enforces token match and expiry server-side but does not explicitly validate that the token contains only digits. Add client-side format validation (e.g., /^\d{6}$/) before calling verifyOtp to ensure only numeric input is accepted.
🤖 Prompt for AI Agents
In @app/components/login.gts around lines 184 - 211, The verifyOtp method
currently only checks otpCode length; add an explicit numeric format check using
a regex like /^\d{6}$/ before attempting authentication: in verifyOtp (and/or
where otpCode is validated) replace or augment the length check with a test that
otpCode matches /^\d{6}$/ and if it fails set this.errorMessage = 'Please enter
the 6-digit code' (or a clearer 'Please enter a 6-digit numeric code') and
return so authenticate isn't called with non-numeric input; ensure this check
runs before setting this.loading or calling this.session.authenticate.
| test('user can request OTP code', async function (assert) { | ||
| await visit('/settings/cloud/login'); | ||
|
|
||
| await visit('/settings/cloud/register'); | ||
| await fillIn('[data-test-register-input-user]', '[email protected]'); | ||
| await fillIn('[data-test-register-input-password]', 'password'); | ||
| await click('[data-test-register-submit]'); | ||
| const supabaseService = this.owner.lookup( | ||
| 'service:supabase' | ||
| ) as SupabaseService; | ||
| const signInWithOtpStub = sinon | ||
| .stub(supabaseService, 'signInWithOtp') | ||
| .resolves(); | ||
|
|
||
| await fillIn('[data-test-login-input-user]', '[email protected]'); | ||
| await click('[data-test-login-submit]'); | ||
| await waitForAll(); | ||
|
|
||
| assert.strictEqual( | ||
| currentURL(), | ||
| '/settings/cloud/register/confirm', | ||
| 'transitioned to register/confirm route' | ||
| signInWithOtpStub.callCount, | ||
| 1, | ||
| 'signInWithOtp was called' | ||
| ); | ||
| await fillIn('[data-test-register-input-user]', '[email protected]'); | ||
| await fillIn('[data-test-register-input-code]', '1234'); | ||
| await click('[data-test-register-submit]'); | ||
| await waitForAll(); | ||
|
|
||
| assert.strictEqual( | ||
| currentURL(), | ||
| '/settings/cloud/login', | ||
| 'transitioned to login route' | ||
| signInWithOtpStub.firstCall.args[0], | ||
| '[email protected]', | ||
| 'email was passed correctly' | ||
| ); | ||
|
|
||
| // Should now show OTP input | ||
| assert.dom('[data-test-login-input-otp]').exists('OTP input is shown'); | ||
| assert.dom('[data-test-login-submit]').hasText('Verify Code'); | ||
| }); |
There was a problem hiding this comment.
Critical: Restore sinon stubs to prevent test pollution.
The signInWithOtpStub is never restored after the test completes. This will cause the stub to persist across subsequent tests, potentially breaking other tests that depend on the real implementation or different stub behavior.
🔧 Proposed fix: restore stubs in cleanup
Add cleanup in the module hooks or restore the stub at the end of each test:
module('Acceptance | settings/cloud', function (hooks) {
setupApplicationTest(hooks);
resetStorage(hooks, { seed: { source: 'backup', scenario: 'basic' } });
+
+ hooks.afterEach(function () {
+ sinon.restore();
+ });This will automatically restore all stubs after each test. Alternatively, you can call signInWithOtpStub.restore() at the end of each test, but using hooks.afterEach with sinon.restore() is more maintainable.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @tests/acceptance/settings/cloud-test.ts around lines 25 - 53, The test
creates a sinon stub signInWithOtpStub on the SupabaseService but never restores
it, which can leak into other tests; fix by restoring stubs after each
test—either add a module-level hooks.afterEach that calls sinon.restore()
(preferred) or call signInWithOtpStub.restore() at the end of this test; ensure
the cleanup references the same stub variable name signInWithOtpStub or simply
uses sinon.restore() to clear all stubs.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @app/data-sources/remote.ts:
- Around line 494-512: The realtime subscriptions in setupRealtimeSync register
on all changes to the palettes and colors tables, causing unnecessary traffic
and cross-user notifications; update the channel subscriptions created via
this.supabase.channel('db-changes') so each .on('postgres_changes', ...)
includes a filter for the current user's id (e.g., add a filter on the user_id
column) before calling .subscribe(), and ensure existing this.realtimeChannel is
properly removed via this.supabase.removeChannel(this.realtimeChannel) as
already done; use the current authenticated user's id from the same client
context to construct the per-table filter for palettes and colors so only that
user’s row changes trigger onUpdate callbacks.
- Around line 376-392: removeFromRelatedRecords currently deletes color rows
when removing a color from a palette, which breaks color-move logic that calls
removeFromRelatedRecords then addToRelatedRecords; instead of deleting in
removeFromRelatedRecords (the branch inside removeFromRelatedRecords for
record.type === 'palette' && relationship === 'colors'), either make it a no-op
and let the controller use replaceRelatedRecord/replaceRelatedRecords to
reassign palette ownership, or update the color record(s) to change their
palette FK (e.g., set palette_id=NULL or to the new palette id) rather than
deleting; update the implementation in removeFromRelatedRecords accordingly and
ensure palettes controller uses replaceRelatedRecord/replaceRelatedRecords flow
rather than rely on delete-then-add.
🧹 Nitpick comments (3)
supabase-schema.sql (2)
11-27: Consider declaringuser_idas NOT NULL directly in the column definition.Line 13 defines
user_idas nullable, but line 25 adds a CHECK constraint requiring it to be NOT NULL. While functionally equivalent, declaringNOT NULLdirectly in the column definition is clearer and standard practice.♻️ Proposed refactor for clarity
CREATE TABLE IF NOT EXISTS palettes ( id UUID DEFAULT gen_random_uuid() PRIMARY KEY, - user_id UUID REFERENCES auth.users(id) ON DELETE CASCADE, + user_id UUID NOT NULL 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) );
218-266: Consider adding conflict handling for production migrations.The migration helper functions work well for initial data import, but lack handling for ID conflicts if records already exist. For production use, consider using
ON CONFLICTclauses to make migrations idempotent.♻️ Example: Adding upsert capability
For
migrate_orbit_palette_data:-- Insert palette with conflict handling INSERT INTO palettes (id, user_id, name, is_color_history, is_favorite, is_locked, selected_color_index, sort_index, color_order) VALUES ( (palette_data->>'id')::UUID, -- Use provided ID from Orbit data 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) ) ON CONFLICT (id) DO UPDATE SET name = EXCLUDED.name, is_color_history = EXCLUDED.is_color_history, -- ... other fields updated_at = NOW() RETURNING id INTO palette_uuid;This allows re-running migrations safely.
app/data-sources/remote.ts (1)
194-315: Consider enhancing error handling with custom error classes.The current error handling wraps Supabase errors in generic
Errorobjects with the message (e.g., lines 203, 214, 274). While functional, this loses error context and makes it harder for calling code to handle specific error cases.♻️ Example: Custom error classes for better error handling
// Add custom error classes class SupabaseQueryError extends Error { constructor( message: string, public operation: string, public recordType: string, public cause?: unknown ) { super(message); this.name = 'SupabaseQueryError'; } } class RecordNotFoundError extends Error { constructor(public recordType: string, public recordId: string) { super(`${recordType} with id ${recordId} not found`); this.name = 'RecordNotFoundError'; } } // Then use in methods: if (error) { if (error.code === 'PGRST116') { throw new RecordNotFoundError(type, id); } throw new SupabaseQueryError( error.message, 'findRecord', type, error ); }This allows calling code to catch and handle specific error types more precisely.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/authenticators/supabase.tsapp/data-sources/remote.tsapp/services/session.tssupabase-schema.sqltests/acceptance/settings/cloud-test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/authenticators/supabase.ts
🧰 Additional context used
🧬 Code graph analysis (3)
app/data-sources/remote.ts (2)
app/services/supabase.ts (1)
SupabaseService(23-165)app/services/session.ts (1)
SessionService(12-26)
tests/acceptance/settings/cloud-test.ts (2)
app/services/supabase.ts (1)
SupabaseService(23-165)app/services/data.ts (1)
DataService(14-216)
app/services/session.ts (1)
app/authenticators/supabase.ts (1)
SupabaseAuthData(14-20)
⏰ 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 (6)
supabase-schema.sql (1)
137-180: LGTM! Sample data function is well-structured.The function correctly creates a color history palette with sample colors, captures UUIDs through separate inserts, and updates the color_order JSONB field with proper references. This aligns well with the Orbit.js data structure used in the application.
app/services/session.ts (1)
1-26: LGTM! Clean type-safe refactor for Supabase authentication.The changes properly introduce typed session data using
SupabaseAuthDataand update the service to extendBaseSessionService<SessionData>. The type-only imports are appropriate, and the existinghandleAuthenticationandhandleInvalidationlogic remains unchanged while now operating on strongly-typed session data.tests/acceptance/settings/cloud-test.ts (1)
25-53: LGTM! OTP request test properly validates the flow.The test correctly stubs
signInWithOtp, verifies it's called with the expected email, and confirms the UI transitions to the OTP verification step. Good coverage of the first phase of the OTP flow.app/data-sources/remote.ts (3)
22-79: LGTM! Clean interface definitions and service integration.The interfaces correctly model the Supabase schema with appropriate nullability (especially
palette_id: stringon line 39 matching the NOT NULL constraint). The constructor properly injects services viagetOwner, and the authentication state getters provide safe null handling for unauthenticated scenarios.
81-104: LGTM! Proper authentication gating on all remote operations.All public methods and the
_queryimplementation correctly verify authentication before proceeding. The consistent error message "Remote requests require authentication" provides clear feedback. This ensures data access is properly secured through the authentication layer.
442-486: LGTM! Transform functions properly enforce schema constraints.The assertions on lines 445 and 472 correctly enforce that:
- User ID must be present for remote operations
- Colors must always have a palette ID (matching the NOT NULL constraint in the schema)
The bidirectional transformation properly maintains relationship data in both directions, ensuring Orbit.js records and Supabase records stay synchronized.
| test('user can verify OTP and log in', async function (assert) { | ||
| await visit('/settings/cloud/login'); | ||
| await waitForAll(); | ||
|
|
||
| await mockCognitoUser({ | ||
| username: '[email protected]', | ||
| attributes: { | ||
| sub: 'aaaabbbb-cccc-dddd-eeee-ffffgggghhhh', | ||
| email: '[email protected]', | ||
| email_verified: 'false', | ||
| }, | ||
| }); | ||
| const supabaseService = this.owner.lookup( | ||
| 'service:supabase' | ||
| ) as SupabaseService; | ||
| sinon.stub(supabaseService, 'signInWithOtp').resolves(); | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| const authenticator = this.owner.lookup('authenticator:cognito') as any; | ||
| const authenticator = this.owner.lookup('authenticator:supabase') as any; | ||
| const authenticateStub = sinon | ||
| .stub(authenticator, 'authenticate') | ||
| .resolves(); | ||
| .resolves({ | ||
| userId: 'test-user-id', | ||
| email: '[email protected]', | ||
| accessToken: 'test-access-token', | ||
| refreshToken: 'test-refresh-token', | ||
| expiresAt: Date.now() + 3600000, | ||
| }); | ||
|
|
||
| const dataService = this.owner.lookup( | ||
| 'service:data' | ||
| ) as unknown as DataService; | ||
| const synchronizeStub = sinon.stub(dataService, 'synchronize').resolves(); | ||
| const resetStub = sinon.stub(dataService, 'reset').resolves(); | ||
|
|
||
| // Step 1: Enter email and request OTP | ||
| await fillIn('[data-test-login-input-user]', '[email protected]'); | ||
| await fillIn('[data-test-login-input-password]', 'password'); | ||
| await click('[data-test-login-submit]'); | ||
| await waitForAll(); | ||
|
|
||
| // Step 2: Enter OTP code | ||
| await fillIn('[data-test-login-input-otp]', '123456'); | ||
| await click('[data-test-login-submit]'); | ||
| await waitForAll(); | ||
|
|
||
| assert.strictEqual(authenticateStub.callCount, 1, 'authenticate called'); | ||
| assert.deepEqual( | ||
| authenticateStub.firstCall.args[0], | ||
| { email: '[email protected]', token: '123456' }, | ||
| 'authenticate called with correct credentials' | ||
| ); | ||
| assert.strictEqual(synchronizeStub.callCount, 1, 'synchronize called'); | ||
| assert.strictEqual(resetStub.callCount, 0, 'reset NOT called yet'); | ||
|
|
||
| assert.strictEqual(currentURL(), '/settings/cloud/profile'); | ||
| assert | ||
| .dom('[data-test-profile-detail="email"]') | ||
| .hasText('[email protected]'); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find SupabaseAuthData interface definition
echo "=== Searching for SupabaseAuthData interface ==="
rg -n "SupabaseAuthData" -A 15 --type ts
# Check how expiresAt is used in the authenticator and related code
echo -e "\n=== Searching for expiresAt usage ==="
rg -n "expiresAt" --type ts -B 2 -A 2
# Look for any Supabase session/auth type definitions
echo -e "\n=== Searching for Session or Auth type definitions ==="
rg -n "expires_at|expiresIn" --type ts -B 2 -A 2 | head -100Repository: shipshapecode/swach
Length of output: 6478
Fix timestamp format in mock expiresAt value to match Supabase expectations.
Line 73 sets expiresAt: Date.now() + 3600000, which is in milliseconds. However, Supabase's Session.expires_at (the type that SupabaseAuthData.expiresAt uses) expects Unix timestamps in seconds, not milliseconds. Update to: expiresAt: Math.floor(Date.now() / 1000) + 3600.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @app/data-sources/remote.ts:
- Around line 375-386: The no-op method removeFromRelatedRecords is marked async
and returns Promise<void> despite having no await; remove the async keyword and
change its return type from Promise<void> to void, and rename the unused
parameter relatedRecord to _relatedRecord to silence the unused-variable lint
error while keeping the signature semantics (i.e., update the method declaration
for removeFromRelatedRecords(record: RecordIdentity, relationship: string,
_relatedRecord: RecordIdentity): void).
🧹 Nitpick comments (3)
docs/SUPABASE_MIGRATION_VERIFICATION.md (1)
12-12: Consider formatting documentation syntax to avoid markdown linting warnings.The static analysis tool flagged bare URL warnings at lines 12 and 61. While these aren't actual URLs (they're arrow notation in architecture diagrams), consider adjusting the format to satisfy markdown linters if this causes issues in your documentation pipeline.
Also applies to: 61-61
app/data-sources/remote.ts (1)
193-218: Consider adding error handling for empty results.The
findRecordsmethod correctly fetches data from Supabase and transforms it to Orbit format. However, when no records are found, Supabase returns an empty array rather than an error, which is handled correctly.One optional improvement: consider adding logging for debugging purposes when queries return empty results, especially during development.
WARP.md (1)
148-152: Clarify breaking change and environment setup requirements.The documentation introduces new Supabase configuration (lines 148–152) but doesn't emphasize in the "Important Notes" section that
supabaseUrlandsupabaseAnonKeyare new required environment variables. Per the PR objectives, this is a breaking change that requires explicit setup.Consider adding a note in the "Important Notes" section (after line 208) to warn developers about the migration impact and required environment variables.
💡 Suggested addition to Important Notes section
### Supabase Configuration (Breaking Change) This version requires Supabase environment variables: - `SUPABASE_URL` - Supabase project URL - `SUPABASE_ANON_KEY` - Supabase anonymous public key These replace the deprecated AWS Cognito authentication and must be configured in `config/environment.js` before running the app.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
WARP.mdapp/data-models/color.tsapp/data-models/palette.tsapp/data-sources/remote.tsdocs/SUPABASE_MIGRATION_VERIFICATION.mdtests/unit/data-sources/remote-test.ts
🧰 Additional context used
🧬 Code graph analysis (2)
tests/unit/data-sources/remote-test.ts (1)
app/data-sources/remote.ts (3)
transformPaletteToOrbit(389-413)transformColorToOrbit(416-435)transformFromOrbit(438-482)
app/data-sources/remote.ts (2)
app/services/supabase.ts (1)
SupabaseService(23-165)app/services/session.ts (1)
SessionService(12-26)
🪛 GitHub Actions: Linting
tests/unit/data-sources/remote-test.ts
[warning] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues in this file.
[error] 55-55: A method that is not declared with this: void may cause unintentional scoping of this when separated from its object. Consider using an arrow function or explicitly .bind()ing the method to avoid calling the method with an unintended this value. If a function does not access this, it can be annotated with this: void @typescript-eslint/unbound-method
[error] 142-142: A method that is not declared with this: void may cause unintentional scoping of this when separated from its object. Consider using an arrow function or explicitly .bind()ing the method to avoid calling the method with an unintended this value. If a function does not access this, it can be annotated with this: void @typescript-eslint/unbound-method
[error] 205-205: A method that is not declared with this: void may cause unintentional scoping of this when separated from its object. Consider using an arrow function or explicitly .bind()ing the method to avoid calling the method with an unintended this value. If a function does not access this, it can be annotated with this: void @typescript-eslint/unbound-method
[error] 270-270: A method that is not declared with this: void may cause unintentional scoping of this when separated from its object. Consider using an arrow function or explicitly .bind()ing the method to avoid calling the method with an unintended this value. If a function does not access this, it can be annotated with this: void @typescript-eslint/unbound-method
app/data-sources/remote.ts
[error] 375-375: Async method 'removeFromRelatedRecords' has no 'await' expression @typescript-eslint/require-await
[error] 378-378: 'relatedRecord' is defined but never used @typescript-eslint/no-unused-vars
🪛 markdownlint-cli2 (0.18.1)
docs/SUPABASE_MIGRATION_VERIFICATION.md
12-12: Bare URL used
(MD034, no-bare-urls)
61-61: Bare URL used
(MD034, no-bare-urls)
⏰ 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 (10)
app/data-models/color.ts (1)
11-11: LGTM! updatedAt attribute aligns with Supabase schema.The addition of the
updatedAtdatetime attribute is consistent with the existingcreatedAtfield and properly maps to theupdated_atcolumn in the Supabase schema, which is automatically managed by database triggers.app/data-models/palette.ts (1)
7-7: LGTM! updatedAt attribute aligns with Supabase schema.The addition of the
updatedAtdatetime attribute maintains consistency with the ColorModel and properly maps to the Supabaseupdated_atcolumn.app/data-sources/remote.ts (5)
59-65: Constructor properly injects Supabase and Session services.The constructor uses
getOwnerto look up required services, which is the correct pattern for Orbit sources in Ember. The services are strongly typed and will be available at runtime.
96-123: Query implementation correctly handles findRecords and findRecord operations.The
_querymethod properly:
- Validates authentication before proceeding
- Handles both single and multiple query expressions
- Delegates to appropriate find methods based on operation type
- Returns results in the expected format
126-190: Update implementation covers all necessary record operations.The
_updatemethod correctly handles all Orbit record operations:
- CRUD operations (add, update, remove)
- Relationship operations (replace, add, remove)
- Proper error handling and authentication checks
388-434: Transformation methods correctly map between Supabase and Orbit formats.The transformation methods (
transformPaletteToOrbit,transformColorToOrbit) properly handle:
- Snake_case ↔ camelCase conversion
- Timestamp mapping (created_at/updated_at → createdAt/updatedAt)
- Foreign key relationships (palette_id → palette relationship)
- Nested color data for palettes
The mappings align with the unit tests and Supabase schema.
437-482: transformFromOrbit correctly omits timestamps and includes user_id.The method properly:
- Asserts user_id is available before proceeding
- Includes user_id in all payloads for RLS policies
- Omits created_at and updated_at (managed by database triggers)
- Converts camelCase attributes to snake_case
- Maps palette relationship to palette_id foreign key
This approach is correct for letting Supabase manage timestamps automatically.
tests/unit/data-sources/remote-test.ts (1)
53-60: Address unbound-method warnings and consider testing approach.The tests access private methods via prototype reflection and use
.call()to invoke them. This pattern triggers@typescript-eslint/unbound-methodwarnings at lines 55, 142, 205, and 270 because the methods are called without properthisbinding.Consider one of these approaches:
- Make transformation methods protected or add public test-only accessors to enable safer testing without bypassing access control.
- Test through public APIs (e.g.,
_query,_update,addRecord) which exercise the transformations indirectly but more realistically.- If keeping the current approach, bind the methods explicitly to avoid warnings:
Option 3: Explicit binding example
-// @ts-expect-error - accessing private method for testing -const transformPaletteToOrbit = SupabaseSource.prototype.transformPaletteToOrbit; -const result: InitializedRecord = transformPaletteToOrbit.call( - {}, - mockSupabasePalette, - mockColors -); +// @ts-expect-error - accessing private method for testing +const transformPaletteToOrbit = SupabaseSource.prototype.transformPaletteToOrbit.bind({}); +const result: InitializedRecord = transformPaletteToOrbit( + mockSupabasePalette, + mockColors +);Additionally, the file needs Prettier formatting. Run
prettier --write tests/unit/data-sources/remote-test.tsto fix.Also applies to: 140-146, 203-206, 268-271
WARP.md (2)
98-109: Sync Strategies section is well-documented.The expanded explanation of blocking vs. non-blocking sync behavior, optimistic UI updates, and the explicit note about "no realtime subscriptions" (line 109) clearly communicates the architectural decision to coordinate all sync through Orbit. Good clarity on when sync occurs (startup, before queries, on updates).
158-161: Implementation verified—auth state subscription is appropriate.The SupabaseService correctly uses ONLY auth operations and remote API calls. The
onAuthStateChangesubscription (line 149-157) is an authentication state listener, not a realtime database subscription, so it does not conflict with the documented constraint that "all sync is handled by Orbit." No realtime database subscriptions or channel subscriptions were found.
app/data-sources/remote.ts
Outdated
| private async removeFromRelatedRecords( | ||
| record: RecordIdentity, | ||
| relationship: string, | ||
| relatedRecord: RecordIdentity | ||
| ): Promise<void> { | ||
| if (record.type === 'palette' && relationship === 'colors') { | ||
| // No-op: Color moves between palettes should use replaceRelatedRecord/replaceRelatedRecords | ||
| // to update the palette_id FK. Deleting here breaks color-move operations that | ||
| // call removeFromRelatedRecords then addToRelatedRecords. | ||
| // Colors are only truly deleted via explicit removeRecord operations. | ||
| } | ||
| } |
There was a problem hiding this comment.
Remove async from no-op method to fix linting error.
The removeFromRelatedRecords method is intentionally a no-op (as explained in the comment) but is marked as async without any await expressions. This triggers a linting error. Since the method performs no asynchronous operations, remove the async keyword.
🔧 Proposed fix
- private async removeFromRelatedRecords(
+ private removeFromRelatedRecords(
record: RecordIdentity,
relationship: string,
- relatedRecord: RecordIdentity
+ _relatedRecord: RecordIdentity
): Promise<void> {
if (record.type === 'palette' && relationship === 'colors') {
// No-op: Color moves between palettes should use replaceRelatedRecord/replaceRelatedRecords
// to update the palette_id FK. Deleting here breaks color-move operations that
// call removeFromRelatedRecords then addToRelatedRecords.
// Colors are only truly deleted via explicit removeRecord operations.
}
+ return Promise.resolve();
}Note: Also prefix relatedRecord with _ to indicate it's intentionally unused, fixing the second linting error.
🧰 Tools
🪛 GitHub Actions: Linting
[error] 375-375: Async method 'removeFromRelatedRecords' has no 'await' expression @typescript-eslint/require-await
[error] 378-378: 'relatedRecord' is defined but never used @typescript-eslint/no-unused-vars
🤖 Prompt for AI Agents
In @app/data-sources/remote.ts around lines 375 - 386, The no-op method
removeFromRelatedRecords is marked async and returns Promise<void> despite
having no await; remove the async keyword and change its return type from
Promise<void> to void, and rename the unused parameter relatedRecord to
_relatedRecord to silence the unused-variable lint error while keeping the
signature semantics (i.e., update the method declaration for
removeFromRelatedRecords(record: RecordIdentity, relationship: string,
_relatedRecord: RecordIdentity): void).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @app/services/data.ts:
- Around line 202-209: The loop that uploads colors and palettes via
remote.update((t: any) => t.addRecord(...)) has no per-record error handling so
a single failure can leave the remote in a partially-synced state; wrap each
await remote.update call for both colors and palettes in try/catch, record
failed records (e.g., failedUploads: InitializedRecord[]), log the error
including the record id and error details, and then decide on follow-up behavior
(retry, queue for later, or surface an aggregate warning) after the loops so
sync can be retried or rolled back as needed.
- Around line 191-214: The current code uploads colors before palettes which
violates the colors.palette_id FK; change the sequence so palettes are uploaded
before colors by moving the palettes upload loop (the for..of using
remote.update((t:any) => t.addRecord(palette))) to run before the colors upload
loop, ensuring palettes are added first; keep the existing mapping of colors and
palettes (the .map that returns { id, type, attributes, relationships }) and
leave the final re-fetch via remote.query((q:any) => q.findRecords('palette'))
as-is to return InitializedRecord[].
🧹 Nitpick comments (1)
app/services/data.ts (1)
174-176: Consider adding proper typing for query/update callbacks.Using
anyfor the query builder parameter bypasses TypeScript's type checking. Iforbit-supabaseexports appropriate types for the query builder, consider using them for better type safety.💡 Suggested approach
- const remotePaletteRecords = (await remote.query((q: any) => + const remotePaletteRecords = (await remote.query((q) => q.findRecords('palette') )) as InitializedRecord[];If the types are inferred correctly from
SupabaseSource.query, the explicitanyannotation may be unnecessary. If not, consider importing and using the appropriate query builder type from the orbit-supabase package.
📜 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 (3)
app/data-sources/remote.tsapp/services/data.tspackage.json
🚧 Files skipped from review as they are similar to previous changes (1)
- app/data-sources/remote.ts
🧰 Additional context used
🧬 Code graph analysis (1)
app/services/data.ts (1)
app/data-sources/remote.ts (1)
SupabaseSource(17-75)
🔇 Additional comments (4)
package.json (3)
148-148: LGTM!Adding
dotenvto devDependencies is appropriate for managing the newSUPABASE_URLandSUPABASE_ANON_KEYenvironment variables mentioned in the PR as breaking changes.
60-60: No action needed. The@orbit/jsonapipackage is still required in the codebase. WhileJSONAPISourcehas been replaced bySupabaseSource, the package is still imported inapp/data-strategies/remote-updatefail.jsfor theNetworkErrorclass used in error handling.Likely an incorrect or invalid review comment.
67-67: Supabase dependencies are appropriate for the migration.The addition of
@supabase/supabase-jsaligns with the migration from Cognito. Theorbit-supabasepackage at version^0.1.1is a pre-1.0 release with only two versions available, indicating early maturity and potential for breaking changes in future releases—monitor for updates and test accordingly.app/services/data.ts (1)
8-12: LGTM!Import changes correctly reflect the migration from JSON:API to Supabase. The
SupabaseSourcetype aligns with the class exported fromapp/data-sources/remote.ts.
| // Add colors first, then palettes | ||
| colors = colors.map((c) => { | ||
| const { id, type, attributes } = c; | ||
|
|
||
| return { id, type, attributes }; | ||
| const { id, type, attributes, relationships } = c; | ||
| return { id, type, attributes, relationships }; | ||
| }); | ||
| palettes = palettes.map((p) => { | ||
| const { id, type, attributes, relationships } = p; | ||
|
|
||
| if (relationships?.['colors']?.data) { | ||
| paletteColors.push({ | ||
| palette: p, | ||
| colors: relationships['colors'].data as RecordIdentity[], | ||
| }); | ||
| } | ||
|
|
||
| return { id, type, attributes }; | ||
| return { id, type, attributes, relationships }; | ||
| }); | ||
|
|
||
| if (colors.length > 0) { | ||
| await remote.update<InitializedRecord[]>( | ||
| (t) => colors.map((r) => t.addRecord(r)), | ||
| { parallelRequests: true } | ||
| ); | ||
| } | ||
|
|
||
| if (palettes.length > 0) { | ||
| await remote.update<InitializedRecord[]>( | ||
| (t) => palettes.map((r) => t.addRecord(r)), | ||
| { parallelRequests: true } | ||
| ); | ||
| // Upload colors first | ||
| for (const color of colors) { | ||
| await remote.update((t: any) => t.addRecord(color)); | ||
| } | ||
|
|
||
| if (paletteColors.length > 0) { | ||
| await remote.update<InitializedRecord[]>( | ||
| (t) => | ||
| paletteColors.map((p) => | ||
| t.replaceRelatedRecords(p.palette, 'colors', p.colors) | ||
| ), | ||
| { parallelRequests: true } | ||
| ); | ||
| // Then upload palettes | ||
| for (const palette of palettes) { | ||
| await remote.update((t: any) => t.addRecord(palette)); | ||
| } | ||
|
|
||
| // Re-fetch palettes and colors from remote | ||
| return remote.query<InitializedRecord[]>( | ||
| (q) => q.findRecords('palette'), | ||
| { include: ['colors'] } | ||
| ); | ||
| // Re-fetch palettes from remote | ||
| return (await remote.query((q: any) => | ||
| q.findRecords('palette') | ||
| )) as InitializedRecord[]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the data.ts file and verify lines 191-214
find . -name "data.ts" -type f | head -20Repository: shipshapecode/swach
Length of output: 115
🏁 Script executed:
# Search for SupabaseSource config to understand the relationships
rg "SupabaseSource|palette_id" -t ts -t js -A 5 -B 5 | head -100Repository: shipshapecode/swach
Length of output: 4655
🏁 Script executed:
# Look for SQL schema files or migrations that define the FK constraint
fd -e sql -e json | xargs grep -l "palette_id\|foreign key" 2>/dev/null | head -20Repository: shipshapecode/swach
Length of output: 82
🏁 Script executed:
# Read the actual data.ts file around lines 191-214
wc -l app/services/data.tsRepository: shipshapecode/swach
Length of output: 87
🏁 Script executed:
# Read lines 191-214 from data.ts
sed -n '191,214p' app/services/data.tsRepository: shipshapecode/swach
Length of output: 897
🏁 Script executed:
# Check the SQL schema file for FK constraints
cat supabase-schema.sqlRepository: shipshapecode/swach
Length of output: 8808
Upload order violates foreign key constraint and will cause sync to fail.
Colors are uploaded before palettes (lines 202-204), but the Supabase schema enforces a NOT NULL foreign key constraint on colors.palette_id referencing palettes(id). Inserting colors before their parent palettes exist will fail with a foreign key constraint violation.
The upload order must be reversed: upload palettes first (lines 207-209), then colors (lines 202-204).
🐛 Proposed fix
- // Add colors first, then palettes
+ // Add palettes first (parent records), then colors (child records with FK)
colors = colors.map((c) => {
const { id, type, attributes, relationships } = c;
return { id, type, attributes, relationships };
});
palettes = palettes.map((p) => {
const { id, type, attributes, relationships } = p;
return { id, type, attributes, relationships };
});
- // Upload colors first
- for (const color of colors) {
- await remote.update((t: any) => t.addRecord(color));
- }
-
- // Then upload palettes
+ // Upload palettes first
for (const palette of palettes) {
await remote.update((t: any) => t.addRecord(palette));
}
+ // Then upload colors
+ for (const color of colors) {
+ await remote.update((t: any) => t.addRecord(color));
+ }🤖 Prompt for AI Agents
In @app/services/data.ts around lines 191 - 214, The current code uploads colors
before palettes which violates the colors.palette_id FK; change the sequence so
palettes are uploaded before colors by moving the palettes upload loop (the
for..of using remote.update((t:any) => t.addRecord(palette))) to run before the
colors upload loop, ensuring palettes are added first; keep the existing mapping
of colors and palettes (the .map that returns { id, type, attributes,
relationships }) and leave the final re-fetch via remote.query((q:any) =>
q.findRecords('palette')) as-is to return InitializedRecord[].
| for (const color of colors) { | ||
| await remote.update((t: any) => t.addRecord(color)); | ||
| } | ||
|
|
||
| if (paletteColors.length > 0) { | ||
| await remote.update<InitializedRecord[]>( | ||
| (t) => | ||
| paletteColors.map((p) => | ||
| t.replaceRelatedRecords(p.palette, 'colors', p.colors) | ||
| ), | ||
| { parallelRequests: true } | ||
| ); | ||
| // Then upload palettes | ||
| for (const palette of palettes) { | ||
| await remote.update((t: any) => t.addRecord(palette)); | ||
| } |
There was a problem hiding this comment.
Consider adding error handling for individual uploads.
If any single upload fails, the sync will be left in an inconsistent state with partial data on the remote. Consider wrapping uploads in try/catch to handle failures gracefully, potentially with retry logic or a rollback mechanism.
💡 Suggested approach
// Upload palettes first (after fixing order)
const failedUploads: InitializedRecord[] = [];
for (const palette of palettes) {
try {
await remote.update((t: any) => t.addRecord(palette));
} catch (error) {
console.error(`[Data Service] Failed to upload palette ${palette.id}:`, error);
failedUploads.push(palette);
}
}
// Similar handling for colors...
if (failedUploads.length > 0) {
console.warn(`[Data Service] ${failedUploads.length} records failed to sync`);
// Consider: retry logic, user notification, or queuing for later sync
}🤖 Prompt for AI Agents
In @app/services/data.ts around lines 202 - 209, The loop that uploads colors
and palettes via remote.update((t: any) => t.addRecord(...)) has no per-record
error handling so a single failure can leave the remote in a partially-synced
state; wrap each await remote.update call for both colors and palettes in
try/catch, record failed records (e.g., failedUploads: InitializedRecord[]), log
the error including the record id and error details, and then decide on
follow-up behavior (retry, queue for later, or surface an aggregate warning)
after the loops so sync can be retried or rolled back as needed.
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
Release Notes
New Features
Removed Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.