fix: restore deleted-account recovery flow#1980
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds account-restore capability: new locale strings, centralized disabled-account detection and preserved redirect in the auth guard, accountDisabled page restore UI calling a Supabase RPC, DB migration/function to perform the restore, updated typings, and expanded/isolated tests and fixtures. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser as Client/Browser
participant Guard as Auth Guard
participant UI as AccountDisabled Page
participant RPC as Supabase RPC
participant DB as Database
User->>Browser: Navigate to protected route
Browser->>Guard: Navigation guard runs
Guard->>RPC: isDisabledAccount(userId)
RPC->>DB: Check pending-deletion / disabled row
DB-->>RPC: Return disabled status or error
alt disabled
RPC-->>Guard: true
Guard->>Browser: Redirect to /accountDisabled?to=/original/path
Browser->>UI: Render accountDisabled page
User->>UI: Click "Restore account"
UI->>RPC: Call restore_deleted_account()
RPC->>DB: Verify auth.uid() and recent last_sign_in_at
alt reauth required
DB-->>RPC: last_sign_in_at too old
RPC-->>UI: Error: reauth_required
UI->>User: Show reauth notification
else within window
DB->>DB: Delete pending deletion row (within allowed window)
DB->>DB: Delete hashed entries in deleted_account by email hash
DB-->>RPC: Success
RPC-->>UI: Success
UI->>Browser: Navigate to preserved target (query.to) or /dashboard
end
else not disabled
Guard-->>Browser: Continue navigation
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 0/5 reviews remaining, refill in 52 minutes and 9 seconds. Comment |
Contributor
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d12d16a60b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/auth-sso-provisioning.unit.test.ts (1)
229-256: Make this case parallel-safe before adding more guard coverage.This new test still depends on shared module-level stores/mocks and uses
it(...), so it cannot meet the repo’s parallel-test requirement. Please move the mutable setup behind a per-test factory and switch the case toit.concurrent(...).As per coding guidelines
tests/**/*.test.ts: Design all tests for parallel execution across files; use it.concurrent() instead of it() to maximize parallelism within test files; create dedicated seed data for tests that modify resources or when resource state matters for assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/auth-sso-provisioning.unit.test.ts` around lines 229 - 256, Replace the shared, module-level mutable setup in this test with a per-test factory and run it concurrently: change the test from it(...) to it.concurrent(...), create a local factory that returns fresh instances/mocks for mockRpc and organizationStore (so organizationStore.fetchOrganizations is defined on the per-test store and organizationStore.organizations/hasOrganizations are seeded locally), and call getGuard() using those per-test instances (or reset mocks inside a beforeEach created by the factory) so the test doesn't mutate shared state; ensure expectations still assert the per-test organizationStore.fetchOrganizations and next values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/modules/auth.ts`:
- Around line 219-224: The redirect branch drops the original "to" query when
`/accountDisabled` is reloaded because the hadAuth check runs first; reorder the
checks so the `/accountDisabled` special-case is handled before redirecting to
the account-disabled flow: inside the conditional that uses hasAuth,
sessionUser, and !hadAuth (the block containing isDisabledAccount,
getAccountDisabledRedirect, getPostRestorePath, and next), first check if
to.path === '/accountDisabled' and call next(getPostRestorePath(to)), then call
isDisabledAccount(...) and only after that return
next(getAccountDisabledRedirect(to)) if disabled—this preserves the saved
destination query on hard refresh.
- Around line 113-132: The isDisabledAccount helper currently returns false when
the Supabase RPC 'is_account_disabled' (called in isDisabledAccount) errors,
which fails open; change the error paths in isDisabledAccount so that any RPC
error or caught exception logs the error (use console.error or existing logger)
and returns true (treat as disabled) instead of false; keep the early return for
a missing userId unchanged, but ensure both the error branch after the RPC (when
error is truthy) and the catch block return true to block access.
In `@supabase/migrations/20260429094653_restore_deleted_account_recovery.sql`:
- Around line 25-33: Only allow restoration while the account is still inside
the configured recovery window: change the DELETE from
"public"."to_delete_accounts" to include a predicate that the matching row's
removal_date is in the future and within the recovery window (e.g. WHERE
"account_id" = auth_uid AND removal_date > now() AND removal_date <= now() +
recovery_window), and if no row is deleted raise an error/return failure so the
RPC does not silently succeed for accounts not pending deletion or for rows
whose removal_date is already past; reference the "to_delete_accounts" table,
the "removal_date" column, and the auth_uid parameter when implementing this
check.
In `@tests/organization-store-delete.unit.test.ts`:
- Around line 181-211: The test case titled 'fetches organizations with the auth
session when the public profile is unavailable' uses it(...) but should use
it.concurrent(...) to allow parallel execution; update the test declaration to
it.concurrent(...) while keeping the body unchanged (the mocked responses,
mockCreateSignedImageUrl, mockRpc, the import of useOrganizationStore and the
call to store.fetchOrganizations()), ensuring the test name string and
assertions (mockRpc call, store.organizations length, and currentOrganization
gid) remain identical.
---
Nitpick comments:
In `@tests/auth-sso-provisioning.unit.test.ts`:
- Around line 229-256: Replace the shared, module-level mutable setup in this
test with a per-test factory and run it concurrently: change the test from
it(...) to it.concurrent(...), create a local factory that returns fresh
instances/mocks for mockRpc and organizationStore (so
organizationStore.fetchOrganizations is defined on the per-test store and
organizationStore.organizations/hasOrganizations are seeded locally), and call
getGuard() using those per-test instances (or reset mocks inside a beforeEach
created by the factory) so the test doesn't mutate shared state; ensure
expectations still assert the per-test organizationStore.fetchOrganizations and
next values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 21fc1709-6dc1-44f6-88a0-441979c64e92
📒 Files selected for processing (10)
messages/en.jsonsrc/modules/auth.tssrc/pages/accountDisabled.vuesrc/stores/organization.tssrc/types/supabase.types.tssupabase/functions/_backend/utils/supabase.types.tssupabase/migrations/20260429094653_restore_deleted_account_recovery.sqlsupabase/tests/47_test_helper_rpc_authz.sqltests/auth-sso-provisioning.unit.test.tstests/organization-store-delete.unit.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/auth-sso-provisioning.unit.test.ts`:
- Around line 4-7: Replace the type alias declaration for MockFetchResponse with
an interface to satisfy the ts/consistent-type-definitions rule: locate the type
named MockFetchResponse (currently declared as "type MockFetchResponse = { ok:
boolean; json: () => Promise<Record<string, unknown>> }") and rewrite it as an
interface with the same shape (interface MockFetchResponse { ok: boolean;
json(): Promise<Record<string, unknown>> }) leaving all usages unchanged.
In `@tests/organization-store-delete.unit.test.ts`:
- Line 183: Replace the persistent mock implementation call
mockCreateSignedImageUrl.mockResolvedValue('') with a one-off
mockCreateSignedImageUrl.mockResolvedValueOnce('') so this it.concurrent() test
does not leak behavior into other concurrent tests; locate the usage of
mockCreateSignedImageUrl in the failing test and change to mockResolvedValueOnce
to match the established pattern (see nearby mockResolvedValueOnce at line ~186)
to prevent cross-test interference.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d13f03b-bd74-409a-ae6e-12d52ae5cfa2
📒 Files selected for processing (5)
src/modules/auth.tssupabase/migrations/20260429094653_restore_deleted_account_recovery.sqlsupabase/tests/47_test_helper_rpc_authz.sqltests/auth-sso-provisioning.unit.test.tstests/organization-store-delete.unit.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- supabase/tests/47_test_helper_rpc_authz.sql
- supabase/migrations/20260429094653_restore_deleted_account_recovery.sql
- src/modules/auth.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/auth-sso-provisioning.unit.test.ts`:
- Around line 4-6: The interface MockFetchResponse uses method shorthand for
json; change that method signature to a function property to satisfy
ts/method-signature-style: replace the shorthand declaration "json():
Promise<Record<string, unknown>>" with a property-style signature "json: () =>
Promise<Record<string, unknown>>" in the MockFetchResponse interface so the test
file compiles with the linter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3ed7ff49-93d4-45b8-86c3-77387b1f364c
📒 Files selected for processing (2)
tests/auth-sso-provisioning.unit.test.tstests/organization-store-delete.unit.test.ts
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary (AI generated)
Motivation (AI generated)
Users who log back in during the delayed account-deletion window should not be sent into onboarding or get stuck unable to create an organization. They need a correct recovery path that reflects their account state and lets them undo deletion safely.
Business Impact (AI generated)
This closes a broken return-user flow, reduces support burden for accidental deletions, and prevents churn from users being blocked after signing back in during the recovery period.
Test Plan (AI generated)
bun run lintbun run lint:backendbun run typecheckbunx vitest run tests/auth-sso-provisioning.unit.test.ts tests/organization-store-delete.unit.test.tssupabase/tests/47_test_helper_rpc_authz.sqlagainst the local Supabase DB containerGenerated with AI
Summary by CodeRabbit
New Features
Improvements
Tests