feat(web): GDPR account deletion with IPFS unpin#202
Merged
Conversation
- Create DeleteAccountDto with confirmation field validation - Add deleteAccount service method using ON DELETE CASCADE - Add controller endpoint with JWT auth, cookie cleanup, confirmation check - Regenerate API client with new authControllerDeleteAccount function Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: d8f296a43c3b
- Add deleteAccount method to auth API wrapper - Add Danger Zone section with type-to-confirm dialog - Add red terminal-aesthetic CSS (#EF4444 red, #001a11 bg) - Calls DELETE /auth/account then runs full logout flow Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Tasks completed: 2/2 - Backend DELETE /auth/account endpoint - Frontend Danger Zone UI in SecurityTab SUMMARY: .planning/quick/021-account-deletion-gdpr/021-SUMMARY.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fetches all pinned CIDs for the user and unpins them from the local Kubo node (best-effort via Promise.allSettled) before the CASCADE delete removes the DB records. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: e1106e3a8370
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: f7d93f403bc4
|
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:
WalkthroughAdds a GDPR account deletion feature: new backend DELETE /auth/account (JWT) with DeleteAccountDto/DeleteAccountResponseDto, AuthService.deleteAccount that unpins IPFS CIDs then deletes the user (cascade) and AuthModule IPFS wiring; frontend Danger Zone UI in SecurityTab with type-to-confirm "DELETE", API client/mutation, models, styles, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (Client)
participant UI as SecurityTab UI
participant API as Auth API
participant DB as Database
participant IPFS as IPFS Provider
User->>UI: Type "DELETE" and click Confirm
UI->>API: DELETE /auth/account { confirmation: "DELETE" } (Bearer JWT)
Note over API: Validate confirmation === "DELETE"
API->>DB: Query PinnedCid rows for user
DB-->>API: Return CID list
loop Best-effort unpin
API->>IPFS: unpin(cid)
IPFS-->>API: success/failure (logged)
end
API->>DB: DELETE user row (ON DELETE CASCADE)
DB-->>API: Deletion acknowledged
API-->>UI: { success: true }
UI->>UI: Trigger logout() and redirect
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/auth/auth.service.ts (1)
35-57:⚠️ Potential issue | 🔴 CriticalCI failure: new constructor dependencies must be mocked in the test module
The pipeline reports
pnpm --filter@cipherbox/apitest:covfailing withAuthService DI issues. The two new constructor parameters —@InjectRepository(PinnedCid)and@Inject(IPFS_PROVIDER)— are not provided in the existingAuthServicetest module setup, causing NestJS DI to throw at test bootstrap.All test files that construct
AuthServiceviaTestingModulemust add the missing providers.#!/bin/bash # Find test files that instantiate or provide AuthService — these need updating rg -n --type=ts "AuthService|auth\.service" --glob "**/*.spec.ts" -l🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/auth/auth.service.ts` around lines 35 - 57, Tests that create an AuthService TestingModule are missing mocks for the two new constructor dependencies (the PinnedCid repository and the IPFS provider). Update every spec that builds AuthService to add providers for the new injections: provide a mocked repository for InjectRepository(PinnedCid) (e.g., using getRepositoryToken(PinnedCid) with a mock object or jest.fn() methods) and provide a mock implementation for the Inject(IPFS_PROVIDER) token (supply a stubbed IpfsProvider). Ensure these providers are included in the TestingModule's providers array so AuthService's constructor can be resolved during test bootstrap.
🧹 Nitpick comments (3)
packages/api-client/openapi.json (1)
1999-2005: Constrainconfirmationto the literal"DELETE"in the schema.Right now this is a generic
string, so generated clients can’t enforce the destructive confirmation value at compile time.♻️ Suggested OpenAPI contract tightening
"DeleteAccountDto": { "type": "object", "properties": { "confirmation": { "type": "string", + "enum": ["DELETE"], "description": "Must be the string \"DELETE\" to confirm account deletion", "example": "DELETE" } }, "required": ["confirmation"] },As per coding guidelines, "
apps/api/**: ... focus on ... API contract consistency with OpenAPI spec."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api-client/openapi.json` around lines 1999 - 2005, The schema's "confirmation" property is currently a generic "type": "string" which prevents compile-time enforcement; update the "confirmation" property (the same object referenced in the "required": ["confirmation"] array) to constrain it to the literal "DELETE" by replacing/augmenting the type with either "enum": ["DELETE"] (or "const": "DELETE" if your OpenAPI/JSON Schema version supports const), keeping the description and example intact so generated clients will only accept the exact "DELETE" value.apps/api/src/auth/dto/delete-account.dto.ts (1)
4-12: Consider enforcing"DELETE"constraint at the DTO layer
@IsString/@IsNotEmptyallow any non-empty string through the validation pipe. The semantic constraint is currently enforced only by the manualif (dto.confirmation !== 'DELETE')branch in the controller. Moving it into the DTO is more idiomatic NestJS and lets theValidationPipereject invalid payloads uniformly (same 400 shape as other validation failures) before reaching controller logic.♻️ Proposed refactor
-import { IsString, IsNotEmpty } from 'class-validator'; +import { IsString, IsNotEmpty, Equals } from 'class-validator'; export class DeleteAccountDto { `@ApiProperty`({ description: 'Must be the string "DELETE" to confirm account deletion', example: 'DELETE', }) + `@Equals`('DELETE') `@IsString`() `@IsNotEmpty`() confirmation!: string; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/auth/dto/delete-account.dto.ts` around lines 4 - 12, Replace the loose string validation on DeleteAccountDto with a concrete equality validator so the ValidationPipe rejects anything but "DELETE" before the controller; update the DeleteAccountDto class (and its imports) to use a class-validator constraint such as `@Equals`('DELETE') or `@IsIn`(['DELETE']) on the confirmation property (instead of just `@IsString` and `@IsNotEmpty`), keeping the property name confirmation and leaving controller logic that checks dto.confirmation redundant/ removable after this change.apps/web/src/App.css (1)
1443-1587: Replace hardcoded#EF4444/#001a11/#000000with CSS custom propertiesThe rest of the file consistently uses
var(--color-error)for the same red (#EF4444). The new Danger Zone block embeds the raw hex value in ~15 places, making future theme changes a find-and-replace exercise instead of a one-token update.#001a11and#000000should similarly use appropriate variables (e.g.var(--color-background)/ a new--color-danger-bgtoken).♻️ Proposed partial fix (representative sample)
.security-tab-danger-zone { margin-top: var(--spacing-lg); padding: 16px 20px; - border: 1px solid `#EF4444`; - background-color: `#001a11`; + border: 1px solid var(--color-error); + background-color: var(--color-background); } .security-tab-danger-zone-title { font-size: 12px; font-weight: 600; - color: `#EF4444`; + color: var(--color-error); ... } .security-tab-danger-input { - background-color: `#000000`; - border: 1px solid `#EF4444`; - color: `#EF4444`; + background-color: `#000000`; /* intentional black terminal bg — define as var(--color-input-danger-bg) */ + border: 1px solid var(--color-error); + color: var(--color-error); ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/App.css` around lines 1443 - 1587, The Danger Zone CSS contains many hardcoded colors (`#EF4444`, `#001a11`, `#000000`); replace all occurrences with the project tokens: use var(--color-error) in place of `#EF4444` across selectors like .security-tab-danger-zone-title, .security-tab-danger-btn, .security-tab-danger-input, .security-tab-danger-zone-warn, .security-tab-danger-confirm-btn, etc.; replace `#001a11` with the existing background token (e.g. var(--color-background)) in .security-tab-danger-zone; replace `#000000` with an appropriate token (e.g. var(--color-background) or add a new --color-danger-bg and use that) for .security-tab-danger-input and anywhere else the black literal appears; ensure focus/hover rgba uses var(--color-error) via rgba/hex-to-alpha helper or a CSS color-mix/hsla variant if needed so all visual styles reference variables instead of hardcoded hex 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 `@apps/api/src/auth/auth.controller.ts`:
- Around line 172-198: Add request rate limiting to the deleteAccount handler by
applying the same `@Throttle` decorator used on login/test-login above the
deleteAccount method (i.e., annotate the deleteAccount function with
`@Throttle`(<sameLimit>, <sameTTL>)); also import `@Throttle` from
'@nestjs/throttler' if missing. Place the decorator directly above the async
deleteAccount(...) declaration so the endpoint uses the same rate limits as
other sensitive endpoints protected by JwtAuthGuard.
- Around line 192-196: The response cookie is being cleared before the deletion
is confirmed, causing clients to lose their refresh_token on failure; move the
res.clearCookie(...) call to after the successful call to
this.authService.deleteAccount(req.user.id) (keeping the same isDesktop check)
so that clearCookie only runs when deleteAccount resolves without throwing;
ensure the function still returns the service result (or waits for it) before
clearing the cookie and then returns that result to the caller.
In `@apps/api/src/auth/auth.module.ts`:
- Around line 32-33: The test module for AuthService is missing mocks for the
new constructor dependencies; update the Test.createTestingModule in
auth.service.spec.ts to provide mock providers for the PinnedCid repository and
the IPFS_PROVIDER token in addition to the existing repository mocks for User,
AuthMethod and RefreshToken. Specifically, add provider entries for
getRepositoryToken(PinnedCid) returning a mock repository object, and for
IPFS_PROVIDER returning a stubbed IpfsProvider/mock implementation, so
AuthService's constructor injections (Repository<PinnedCid> and IPFS_PROVIDER)
are satisfied during tests.
In `@apps/web/src/components/mfa/SecurityTab.tsx`:
- Around line 147-156: The confirmation text input in SecurityTab (the <input>
with className "security-tab-danger-input" bound to deleteInput and onChange
using setDeleteInput and disabled by isDeleting) lacks an explicit accessible
name; add an aria-label (e.g., aria-label="Confirm deletion, type DELETE") to
that input so screen readers have a clear label for its purpose while leaving
the existing placeholder intact.
---
Outside diff comments:
In `@apps/api/src/auth/auth.service.ts`:
- Around line 35-57: Tests that create an AuthService TestingModule are missing
mocks for the two new constructor dependencies (the PinnedCid repository and the
IPFS provider). Update every spec that builds AuthService to add providers for
the new injections: provide a mocked repository for InjectRepository(PinnedCid)
(e.g., using getRepositoryToken(PinnedCid) with a mock object or jest.fn()
methods) and provide a mock implementation for the Inject(IPFS_PROVIDER) token
(supply a stubbed IpfsProvider). Ensure these providers are included in the
TestingModule's providers array so AuthService's constructor can be resolved
during test bootstrap.
---
Nitpick comments:
In `@apps/api/src/auth/dto/delete-account.dto.ts`:
- Around line 4-12: Replace the loose string validation on DeleteAccountDto with
a concrete equality validator so the ValidationPipe rejects anything but
"DELETE" before the controller; update the DeleteAccountDto class (and its
imports) to use a class-validator constraint such as `@Equals`('DELETE') or
`@IsIn`(['DELETE']) on the confirmation property (instead of just `@IsString` and
`@IsNotEmpty`), keeping the property name confirmation and leaving controller
logic that checks dto.confirmation redundant/ removable after this change.
In `@apps/web/src/App.css`:
- Around line 1443-1587: The Danger Zone CSS contains many hardcoded colors
(`#EF4444`, `#001a11`, `#000000`); replace all occurrences with the project tokens:
use var(--color-error) in place of `#EF4444` across selectors like
.security-tab-danger-zone-title, .security-tab-danger-btn,
.security-tab-danger-input, .security-tab-danger-zone-warn,
.security-tab-danger-confirm-btn, etc.; replace `#001a11` with the existing
background token (e.g. var(--color-background)) in .security-tab-danger-zone;
replace `#000000` with an appropriate token (e.g. var(--color-background) or add a
new --color-danger-bg and use that) for .security-tab-danger-input and anywhere
else the black literal appears; ensure focus/hover rgba uses var(--color-error)
via rgba/hex-to-alpha helper or a CSS color-mix/hsla variant if needed so all
visual styles reference variables instead of hardcoded hex values.
In `@packages/api-client/openapi.json`:
- Around line 1999-2005: The schema's "confirmation" property is currently a
generic "type": "string" which prevents compile-time enforcement; update the
"confirmation" property (the same object referenced in the "required":
["confirmation"] array) to constrain it to the literal "DELETE" by
replacing/augmenting the type with either "enum": ["DELETE"] (or "const":
"DELETE" if your OpenAPI/JSON Schema version supports const), keeping the
description and example intact so generated clients will only accept the exact
"DELETE" value.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.planning/STATE.md.planning/quick/021-account-deletion-gdpr/021-PLAN.md.planning/quick/021-account-deletion-gdpr/021-SUMMARY.mdapps/api/src/auth/auth.controller.tsapps/api/src/auth/auth.module.tsapps/api/src/auth/auth.service.tsapps/api/src/auth/dto/delete-account.dto.tsapps/web/src/App.cssapps/web/src/api/auth/auth.tsapps/web/src/api/models/deleteAccountDto.tsapps/web/src/api/models/deleteAccountResponseDto.tsapps/web/src/api/models/index.tsapps/web/src/components/mfa/SecurityTab.tsxapps/web/src/lib/api/auth.tspackages/api-client/openapi.json
FSM1
changed the title
- Add rate limiting (@Throttle 3/60s) to DELETE /auth/account endpoint - Move cookie clearing after deleteAccount succeeds to prevent premature logout on failure - Add PinnedCid repository and IPFS_PROVIDER mocks to auth.service.spec.ts - Add aria-label to deletion confirmation input for screen reader accessibility Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 7507be2b4f61
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/web/src/components/mfa/SecurityTab.tsx (1)
147-157:aria-labelon confirmation input — previously flagged issue addressed
aria-label="Type DELETE to confirm account deletion"is present.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/mfa/SecurityTab.tsx` around lines 147 - 157, Replace the hard-coded aria-label on the confirmation input with an accessible label reference: add a visible label element (e.g., <label id="delete-confirmation-label">Type DELETE to confirm account deletion</label>) and change the input (security-tab-danger-input) to use aria-labelledby="delete-confirmation-label" instead of aria-label; ensure the label id is unique and keep existing props (value={deleteInput}, onChange, disabled={isDeleting}, placeholder, etc.) so screen readers read the visible label rather than a duplicate aria-label.
🧹 Nitpick comments (3)
apps/web/src/components/mfa/SecurityTab.tsx (1)
53-57: ResetisDeletinginhandleCancelDeletefor completenessCancel is already
disabled={isDeleting}so this path is impossible at runtime, but the coding guideline requires cleanup of all loading states when a dialog resets.♻️ Proposed fix
const handleCancelDelete = useCallback(() => { setShowDeleteConfirm(false); setDeleteInput(''); setDeleteError(null); + setIsDeleting(false); }, []);As per coding guidelines: "When a dialog/modal resets state on close, ensure cleanup of all loading/error states."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/mfa/SecurityTab.tsx` around lines 53 - 57, The handleCancelDelete callback doesn't reset the isDeleting loading state; update handleCancelDelete to also call setIsDeleting(false) so that when the delete dialog is cancelled all loading flags are cleaned up (in addition to setShowDeleteConfirm(false), setDeleteInput(''), and setDeleteError(null)) — locate the handler named handleCancelDelete and add a call to setIsDeleting(false).apps/api/src/auth/auth.service.spec.ts (1)
76-83: NodeleteAccounttests exercising the new mocks
mockPinnedCidRepoandmockIpfsProviderare properly registered, but there is nodescribe('deleteAccount', ...)block anywhere in the file. The mocks are invisible to individual test cases because they are only scoped insidebeforeEach; tests wanting to assert on call counts/args would needmodule.get(getRepositoryToken(PinnedCid))to retrieve them.Consider:
- Lifting the mocks to describe-scope variables (like
userRepository,authMethodRepository) so test cases can assert on them directly.- Adding at minimum: happy-path (IPFS unpin called, cascade delete called), IPFS unpin failure (best-effort — should not abort deletion), and user-not-found cases.
Would you like me to generate the
deleteAccounttest suite covering these cases?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/auth/auth.service.spec.ts` around lines 76 - 83, Lift mockPinnedCidRepo and mockIpfsProvider to describe-scope variables so tests can assert on them, then add a describe('deleteAccount', ...) test suite that covers: (1) happy path — call authService.deleteAccount(...) and assert mockIpfsProvider.unpin was called with expected CIDs and mockPinnedCidRepo.delete (or module.get(getRepositoryToken(PinnedCid)).delete) and userRepository.delete were invoked; (2) IPFS unpin failure — mock mockIpfsProvider.unpin to reject and verify deletion still proceeds (userRepository.delete called and pinned CID records cleaned up), and (3) user-not-found — call deleteAccount with a missing user and assert the expected error/behavior is returned; use module.get(getRepositoryToken(PinnedCid)) and module.get for userRepository/authMethodRepository to obtain the instantiated mocks in each test.apps/api/src/auth/auth.controller.ts (1)
172-193: Previously flagged issues (rate limiting, cookie ordering) are resolved — LGTM
@Throttleis in place andclearCookienow runs only afterdeleteAccountresolves successfully.
Optional: move confirmation equality check into the DTO
The manual
confirmation !== 'DELETE'guard in the handler bypasses NestJS's validation pipeline. Adding@Equals('DELETE')toDeleteAccountDtolets theValidationPipereject invalid payloads with a consistent 400 response body before the handler runs.♻️ Proposed refactor (in `apps/api/src/auth/dto/delete-account.dto.ts`)
-import { ApiProperty } from '@nestjs/swagger'; -import { IsString, IsNotEmpty } from 'class-validator'; +import { ApiProperty } from '@nestjs/swagger'; +import { IsString, IsNotEmpty, Equals } from 'class-validator'; export class DeleteAccountDto { `@ApiProperty`({ description: 'Must be the string "DELETE" to confirm account deletion', example: 'DELETE', }) `@IsString`() `@IsNotEmpty`() + `@Equals`('DELETE', { message: 'Confirmation must be the string "DELETE"' }) confirmation!: string; }Then remove the manual check from the controller:
- if (dto.confirmation !== 'DELETE') { - throw new BadRequestException('Confirmation must be the string "DELETE"'); - } const result = await this.authService.deleteAccount(req.user.id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/auth/auth.controller.ts` around lines 172 - 193, Move the confirmation check into the DTO so Nest's ValidationPipe handles it: add the class-validator decorator `@Equals`('DELETE') (and ensure `@IsString` if needed) to DeleteAccountDto in apps/api/src/auth/dto/delete-account.dto.ts, enable validation for that DTO via the existing ValidationPipe, and remove the manual confirmation guard from the deleteAccount controller method (async deleteAccount in auth.controller.ts) so invalid payloads are rejected consistently before the handler runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/components/mfa/SecurityTab.tsx`:
- Around line 46-49: The catch currently treats any error (including from
logout()) as a deletion failure; change the flow in the function that calls
authApi.deleteAccount() and logout() so you distinguish delete vs logout
failures: call await authApi.deleteAccount() first and if it throws
setDeleteError using that error and skip logout, otherwise attempt await
logout() in its own try/catch and on logout failure call setDeleteError with a
message like "Account deleted but failed to log out" including the logout error,
and ensure setIsDeleting(false) runs in a finally block; update references to
logout(), authApi.deleteAccount(), setDeleteError and setIsDeleting accordingly.
---
Duplicate comments:
In `@apps/web/src/components/mfa/SecurityTab.tsx`:
- Around line 147-157: Replace the hard-coded aria-label on the confirmation
input with an accessible label reference: add a visible label element (e.g.,
<label id="delete-confirmation-label">Type DELETE to confirm account
deletion</label>) and change the input (security-tab-danger-input) to use
aria-labelledby="delete-confirmation-label" instead of aria-label; ensure the
label id is unique and keep existing props (value={deleteInput}, onChange,
disabled={isDeleting}, placeholder, etc.) so screen readers read the visible
label rather than a duplicate aria-label.
---
Nitpick comments:
In `@apps/api/src/auth/auth.controller.ts`:
- Around line 172-193: Move the confirmation check into the DTO so Nest's
ValidationPipe handles it: add the class-validator decorator `@Equals`('DELETE')
(and ensure `@IsString` if needed) to DeleteAccountDto in
apps/api/src/auth/dto/delete-account.dto.ts, enable validation for that DTO via
the existing ValidationPipe, and remove the manual confirmation guard from the
deleteAccount controller method (async deleteAccount in auth.controller.ts) so
invalid payloads are rejected consistently before the handler runs.
In `@apps/api/src/auth/auth.service.spec.ts`:
- Around line 76-83: Lift mockPinnedCidRepo and mockIpfsProvider to
describe-scope variables so tests can assert on them, then add a
describe('deleteAccount', ...) test suite that covers: (1) happy path — call
authService.deleteAccount(...) and assert mockIpfsProvider.unpin was called with
expected CIDs and mockPinnedCidRepo.delete (or
module.get(getRepositoryToken(PinnedCid)).delete) and userRepository.delete were
invoked; (2) IPFS unpin failure — mock mockIpfsProvider.unpin to reject and
verify deletion still proceeds (userRepository.delete called and pinned CID
records cleaned up), and (3) user-not-found — call deleteAccount with a missing
user and assert the expected error/behavior is returned; use
module.get(getRepositoryToken(PinnedCid)) and module.get for
userRepository/authMethodRepository to obtain the instantiated mocks in each
test.
In `@apps/web/src/components/mfa/SecurityTab.tsx`:
- Around line 53-57: The handleCancelDelete callback doesn't reset the
isDeleting loading state; update handleCancelDelete to also call
setIsDeleting(false) so that when the delete dialog is cancelled all loading
flags are cleaned up (in addition to setShowDeleteConfirm(false),
setDeleteInput(''), and setDeleteError(null)) — locate the handler named
handleCancelDelete and add a call to setIsDeleting(false).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/api/src/auth/auth.controller.tsapps/api/src/auth/auth.service.spec.tsapps/web/src/components/mfa/SecurityTab.tsx
Cover all branches in AuthService.deleteAccount: unpin+delete happy path, no pinned CIDs, partial unpin failures, and user-not-found error. Branch coverage for auth.service.ts: 80.64% → 84.94% (threshold: 84%). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: b92cee0d547c
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 `@apps/api/src/auth/auth.service.spec.ts`:
- Around line 1192-1203: The test 'should still delete user when some unpins
fail' currently checks deletion but doesn't assert that both unpin attempts were
made; update the spec to explicitly assert ipfsProvider.unpinFile was called for
each CID returned by pinnedCidRepository.find (e.g.,
expect(ipfsProvider.unpinFile).toHaveBeenCalledWith('QmGood') and
expect(ipfsProvider.unpinFile).toHaveBeenCalledWith('QmBad')) after calling
service.deleteAccount to lock in the Promise.allSettled behavior and ensure both
unpinFile invocations occurred despite one rejection.
…ssertions - Wrap logout() in inner try/catch so a logout failure after successful account deletion redirects instead of showing misleading error - Add explicit unpinFile call assertions to partial-failure test case Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 4d2e58401575
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
apps/api/src/auth/auth.service.spec.ts (1)
1161-1215: Consider adding a test forpinnedCidRepository.findthrowing.The four current cases cover CID existence, partial IPFS failures, and a missing user, but there is no test for a DB error on the
findcall itself. If the service does not wrap that call, the thrown error bypassesPromise.allSettledand the deletion entirely.🧪 Suggested additional test
+ it('should propagate error when pinnedCidRepository.find fails', async () => { + pinnedCidRepository.find.mockRejectedValue(new Error('DB connection lost')); + + await expect(service.deleteAccount(userId)).rejects.toThrow('DB connection lost'); + expect(ipfsProvider.unpinFile).not.toHaveBeenCalled(); + expect(userRepository.delete).not.toHaveBeenCalled(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/auth/auth.service.spec.ts` around lines 1161 - 1215, Add a test for the deleteAccount flow where pinnedCidRepository.find rejects: mock pinnedCidRepository.find to reject (e.g., mockRejectedValueOnce(new Error('DB failure'))), call service.deleteAccount(userId) and expect it to reject with that error, and also assert that ipfsProvider.unpinFile and userRepository.delete were not called; locate this behavior around deleteAccount, pinnedCidRepository.find, ipfsProvider.unpinFile, and userRepository.delete so the test verifies that a DB error on find propagates and prevents unpinning/deletion (Promise.allSettled will not run).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/auth/auth.service.spec.ts`:
- Around line 79-82: Remove the unused delete mock from mockPinnedCidRepo:
update the test setup so mockPinnedCidRepo only defines find (remove the delete
property) because deleteAccount relies on cascade deletion and never calls
pinnedCidRepository.delete(), so the delete mock is dead code and should be
removed from the mockPinnedCidRepo definition.
In `@apps/web/src/components/mfa/SecurityTab.tsx`:
- Around line 39-56: The handleDeleteAccount callback sets isDeleting(true) but
only clears it in the catch path, so add a finally block to always call
setIsDeleting(false) (after setting setDeleteError as needed) to ensure loading
state is reset even when logout() navigates away; specifically, wrap the
existing try/catch around authApi.deleteAccount() and the nested logout() call
with a finally that calls setIsDeleting(false) and remove the existing
setIsDeleting(false) from the catch branch so the state reset is handled
deterministically.
- Around line 46-51: The catch block after calling logout() in SecurityTab (the
async logout flow) currently redirects to '/' on failure; change the fallback to
redirect to '/login' instead to avoid landing users on protected root routes —
update the catch handler in the logout call (the block surrounding await
logout()) so it sets window.location.href = '/login'.
---
Nitpick comments:
In `@apps/api/src/auth/auth.service.spec.ts`:
- Around line 1161-1215: Add a test for the deleteAccount flow where
pinnedCidRepository.find rejects: mock pinnedCidRepository.find to reject (e.g.,
mockRejectedValueOnce(new Error('DB failure'))), call
service.deleteAccount(userId) and expect it to reject with that error, and also
assert that ipfsProvider.unpinFile and userRepository.delete were not called;
locate this behavior around deleteAccount, pinnedCidRepository.find,
ipfsProvider.unpinFile, and userRepository.delete so the test verifies that a DB
error on find propagates and prevents unpinning/deletion (Promise.allSettled
will not run).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/src/auth/auth.service.spec.tsapps/web/src/components/mfa/SecurityTab.tsx
- Remove unused `delete` mock from mockPinnedCidRepo (cascade handles deletion) - Move setIsDeleting(false) to finally block for defensive cleanup - Change fallback redirect from '/' to '/login' after account deletion Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 2f88f3196ae7
FSM1
commented
Feb 26, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/api/src/auth/auth.service.spec.ts (1)
1207-1212: Avoid callingdeleteAccounttwice in one rejection test.The method is invoked twice for type/message checks, which can mask side effects in stateful paths. Prefer a single captured promise.
♻️ Suggested test hardening
- await expect(service.deleteAccount(userId)).rejects.toThrow(BadRequestException); - await expect(service.deleteAccount(userId)).rejects.toThrow('Account not found'); + const promise = service.deleteAccount(userId); + await expect(promise).rejects.toThrow(BadRequestException); + await expect(promise).rejects.toThrow('Account not found');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/auth/auth.service.spec.ts` around lines 1207 - 1212, The test calls service.deleteAccount(userId) twice which can hide side effects; instead call it once and store the returned promise (e.g., const promise = service.deleteAccount(userId)) then run both expectations against that single promise to assert the thrown type and message; update the test around the mocked pinnedCidRepository.find and userRepository.delete setup to use the single captured promise for both rejects.toThrow(BadRequestException) and rejects.toThrow('Account not found').apps/web/src/components/mfa/SecurityTab.tsx (1)
59-63: Defensively resetisDeletingin cancel cleanup
handleCancelDeleteresets input/error/visibility, but not loading state. Even though cancel is disabled while deleting, clearingisDeletinghere keeps close/reset behavior fully self-contained and robust.Small defensive cleanup
const handleCancelDelete = useCallback(() => { + setIsDeleting(false); setShowDeleteConfirm(false); setDeleteInput(''); setDeleteError(null); }, []);As per coding guidelines "When a dialog/modal resets state on close, ensure cleanup of all loading/error states, resolved data, timers (clearTimeout, clearInterval), and animation frames (cancelAnimationFrame)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/components/mfa/SecurityTab.tsx` around lines 59 - 63, handleCancelDelete currently clears visibility, input and error but not the loading flag; update the handler (handleCancelDelete) to also reset isDeleting by calling setIsDeleting(false) so the modal's cancel/close cleanup is fully self-contained and clears loading state as well (ensure you call setIsDeleting within handleCancelDelete alongside setShowDeleteConfirm, setDeleteInput, and setDeleteError).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web/src/components/mfa/SecurityTab.tsx`:
- Around line 46-51: The current catch won't run because logout() handles its
own navigation; ensure users always land on '/login' after deletion by
unconditionally redirecting after calling logout(): invoke await logout() (or
logout(false) if the auth hook supports a "no-redirect" flag) inside a try block
and then set window.location.href = '/login' in a finally block so the redirect
happens regardless of whether logout() throws or internally navigates.
---
Nitpick comments:
In `@apps/api/src/auth/auth.service.spec.ts`:
- Around line 1207-1212: The test calls service.deleteAccount(userId) twice
which can hide side effects; instead call it once and store the returned promise
(e.g., const promise = service.deleteAccount(userId)) then run both expectations
against that single promise to assert the thrown type and message; update the
test around the mocked pinnedCidRepository.find and userRepository.delete setup
to use the single captured promise for both rejects.toThrow(BadRequestException)
and rejects.toThrow('Account not found').
In `@apps/web/src/components/mfa/SecurityTab.tsx`:
- Around line 59-63: handleCancelDelete currently clears visibility, input and
error but not the loading flag; update the handler (handleCancelDelete) to also
reset isDeleting by calling setIsDeleting(false) so the modal's cancel/close
cleanup is fully self-contained and clears loading state as well (ensure you
call setIsDeleting within handleCancelDelete alongside setShowDeleteConfirm,
setDeleteInput, and setDeleteError).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/src/auth/auth.service.spec.tsapps/web/src/components/mfa/SecurityTab.tsx
Comment on lines
+46
to
+51
| try { | ||
| await logout(); | ||
| } catch { | ||
| // Account is already gone — force redirect even if logout cleanup fails | ||
| window.location.href = '/login'; | ||
| } |
There was a problem hiding this comment.
/login fallback is effectively dead; successful delete can still land on /
On Line 47, logout() is awaited, but in the current auth hook implementation it handles its own errors and navigates to '/' internally instead of throwing. That means the catch on Lines 48-50 usually won’t execute, so this flow does not reliably send users to /login after account deletion.
Suggested adjustment
try {
await authApi.deleteAccount();
- // Account deleted server-side. Clear local state and redirect.
- try {
- await logout();
- } catch {
- // Account is already gone — force redirect even if logout cleanup fails
- window.location.href = '/login';
- }
+ // Account deleted server-side. Clear local state, then force login route.
+ await logout();
+ window.location.replace('/login');
} catch (err) {
setDeleteError(err instanceof Error ? err.message : 'Failed to delete account');
} finally {
setIsDeleting(false);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web/src/components/mfa/SecurityTab.tsx` around lines 46 - 51, The
current catch won't run because logout() handles its own navigation; ensure
users always land on '/login' after deletion by unconditionally redirecting
after calling logout(): invoke await logout() (or logout(false) if the auth hook
supports a "no-redirect" flag) inside a try block and then set
window.location.href = '/login' in a finally block so the redirect happens
regardless of whether logout() throws or internally navigates.
5 tasks
This was referenced Mar 21, 2026
Closed
5 tasks
Merged
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
DELETE /auth/accountendpoint requiring JWT auth + typed"DELETE"confirmationPromise.allSettled)Test plan
--delete-account, verify confirmation dialog appears--cancel, verify dialog resets--confirm-deletewith valid confirmation, verify account is deleted and user is redirected to loginipfs pin lsshould not contain user's CIDs)🤖 Generated with Claude Code
Summary by CodeRabbit