feat(platform): admin UI for data subject requests (GDPR Art. 17)#1696
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR implements a complete admin-facing GDPR Article 17 (erasure) data subject request workflow. It adds a new "Data subject requests" governance page with a paginated request list, filing dialog, detail drawer, deadline extension capability, and retry functionality. Backend changes include new Convex mutations ( Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 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)
Comment |
![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: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/de/platform/admin/data-subject-requests.md`:
- Line 55: Fix the mismatched quotation marks in the retry sentence: replace the
straight closing quotes with matching German closing quotes so both phrases use
paired German quotes—change „Sperre aufheben" to „Sperre aufheben“ and
„Wiederholen" to „Wiederholen“ (the sentence containing "Erneut versuchen" and
those two phrases).
In `@docs/fr/platform/admin/data-subject-requests.md`:
- Around line 12-14: The paragraph incorrectly labels the typed confirmation
token "ERASE" as an identity-verification (IDV) control; update the text so it
does not claim that typing "ERASE" verifies the subject's identity—instead state
that "ERASE" is an auditable operator intent confirmation used after the admin
has already completed out‑of‑band IDV. Locate the passage referencing the admin
as authoritative and the phrase "porte IDV" (and the quoted token "ERASE") and
either remove the IDV wording or rephrase to clarify it is only a deliberate,
auditable confirmation of intent, not an identity check.
In
`@services/platform/app/features/settings/governance/data-subject-requests/file-request-dialog.tsx`:
- Around line 28-35: Replace the hardcoded ERASURE_CONFIRM_PHRASE constant with
a value resolved from the translation/config layer and use the translation hook
where FileRequestDialog validates input; specifically remove
ERASURE_CONFIRM_PHRASE and instead call the i18n lookup (e.g.
t('erasureConfirmPhrase') or config.get('erasureConfirmPhrase')) inside the
component that performs the equality check so the typed phrase comes from
translations/config and comparisons use that resolved value, updating any
tests/stories to reference the translation key.
In
`@services/platform/app/features/settings/governance/data-subject-requests/request-detail-drawer.tsx`:
- Around line 207-220: The ordered list rendering the audit timeline (the <ol>
that maps over auditEntries in request-detail-drawer.tsx) is missing an explicit
role and therefore loses list semantics in Safari/VoiceOver due to Tailwind
Preflight; add role="list" to that <ol> element that contains the auditEntries
mapping so assistive tech announces it correctly while keeping the same children
(Text, TableDateCell, errorMessage) and keys (String(entry._id)).
In
`@services/platform/app/features/settings/governance/data-subject-requests/requests-list-section.tsx`:
- Around line 65-76: The nested loops in the onChange handler (inside the
onChange prop that builds next and calls setStatusFilter) are O(n×m); replace
them with a membership-based filter: create a Set from ERASURE_STATUSES (or use
ERASURE_STATUSES.includes in a type-guard) and map/filter the incoming values:
filter values by membership in that Set and cast/guard them to ErasureStatus,
then call setStatusFilter with the result; update the onChange implementation to
use this O(n) approach and keep references to ERASURE_STATUSES, ErasureStatus,
and setStatusFilter to locate the change.
In
`@services/platform/app/routes/dashboard/`$id/settings/governance/data-subject-requests/$requestId.tsx:
- Around line 22-26: The close handler currently navigates back with a push,
causing the history stack to be list → detail → list; update the handleClose
implementation (the function named handleClose that calls navigate({...})) to
navigate back using history replacement instead of push by passing the replace
option (e.g. include replace: true in the navigate call or use the framework’s
equivalent replace flag) so closing the drawer replaces the current entry rather
than adding a new one.
In `@services/platform/convex/governance/__tests__/erasure_extend.test.ts`:
- Around line 149-179: Update the two auth-related tests for
extendErasureDeadline to assert the Convex error code in the thrown error object
rather than only matching the message text: when mockGetAuthUser resolves to
null assert the rejected error has data.code === 'unauthenticated', and when
mockGetAuthUser returns MEMBER_USER but mockGetOrganizationMember returns {
role: 'member' } assert the rejected error has data.code === 'forbidden'; locate
these checks around extendErasureDeadline.handler and replace or supplement the
message-based expect(...) assertions with assertions that the rejected error
object contains the correct data.code (e.g., using rejects.toHaveProperty or
rejects.toMatchObject) so regressions to plain Error will fail the tests.
In `@services/platform/convex/governance/erasure_queries.ts`:
- Around line 147-158: The current multi-status branch paginates
gdprErasureRequests using withIndex('by_organizationId_status') which orders by
status before _creationTime, so post-filtering by statuses will not produce a
global newest-first stream; instead either query an organization+time ordered
index (e.g., use or add and use an index like 'by_organizationId_creationTime'
and paginate that with q.eq('organizationId', args.organizationId) then filter
the result by the statuses Set) or implement a merge of per-status paginated
streams (issue parallel queries per status with
withIndex('by_organizationId_status') and merge-sort by _creationTime while
advancing cursors) so pagination and cursors reflect true chronological order
for the statuses filter (refer to symbols: statuses, gdprErasureRequests,
withIndex('by_organizationId_status'), args.organizationId, paginate and
paginationOpts).
In `@services/platform/convex/governance/erasure.ts`:
- Around line 1516-1525: The code currently truncates args.extraDays to an
integer (const extraDays = Math.trunc(args.extraDays)) which silently accepts
fractional inputs; instead validate the original value with
Number.isInteger(args.extraDays) and that it is finite and within range before
using it (remove or avoid truncation). Update the check to: ensure
Number.isFinite(args.extraDays) && Number.isInteger(args.extraDays) &&
args.extraDays >= 1 && args.extraDays <= MAX_EXTENSION_DAYS, and throw the same
ConvexError if validation fails, then assign extraDays = args.extraDays for
subsequent use.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1a66f401-fcfc-41e6-84c1-33e8ef42bf90
⛔ Files ignored due to path filters (1)
services/platform/convex/_generated/api.d.tsis excluded by!**/_generated/**
📒 Files selected for processing (32)
docs/de-CH/platform/admin/data-subject-requests.mddocs/de/platform/admin/data-subject-requests.mddocs/en/platform/admin/data-subject-requests.mddocs/fr/platform/admin/data-subject-requests.mddocs/nav.jsonservices/platform/app/features/settings/governance/data-subject-requests/data-subject-requests-errors.test.tsservices/platform/app/features/settings/governance/data-subject-requests/data-subject-requests-errors.tsservices/platform/app/features/settings/governance/data-subject-requests/extend-deadline-dialog.tsxservices/platform/app/features/settings/governance/data-subject-requests/file-request-dialog.tsxservices/platform/app/features/settings/governance/data-subject-requests/hooks/mutations.tsservices/platform/app/features/settings/governance/data-subject-requests/hooks/queries.tsservices/platform/app/features/settings/governance/data-subject-requests/legal-hold-block-panel.tsxservices/platform/app/features/settings/governance/data-subject-requests/request-detail-drawer.tsxservices/platform/app/features/settings/governance/data-subject-requests/requests-list-section.tsxservices/platform/app/features/settings/governance/data-subject-requests/retry-dialog.tsxservices/platform/app/features/settings/governance/data-subject-requests/sla-countdown-badge.test.tsservices/platform/app/features/settings/governance/data-subject-requests/sla-countdown-badge.tsxservices/platform/app/features/settings/governance/data-subject-requests/status-badge.tsxservices/platform/app/routeTree.gen.tsservices/platform/app/routes/dashboard/$id/settings/governance/data-subject-requests/$requestId.tsxservices/platform/app/routes/dashboard/$id/settings/governance/data-subject-requests/route.tsxservices/platform/app/routes/dashboard/$id/settings/governance/route.tsxservices/platform/convex/governance/__tests__/erasure_extend.test.tsservices/platform/convex/governance/__tests__/erasure_queries.test.tsservices/platform/convex/governance/erasure.tsservices/platform/convex/governance/erasure_constants.tsservices/platform/convex/governance/erasure_queries.tsservices/platform/convex/governance/schema.tsservices/platform/messages/de-CH.jsonservices/platform/messages/de.jsonservices/platform/messages/en.jsonservices/platform/messages/fr.json
| - `blocked` — Anfrage wurde am Legal-Hold-Gate abgewiesen. Sperre lösen, dann erneut versuchen. | ||
| - `failed` — Kaskade ist abgestürzt (RAG-Service nicht erreichbar, transienter Infrastrukturfehler) oder wurde vom Watchdog nach Überschreiten des 30-Minuten-Limits erfasst. | ||
|
|
||
| Die Aktion **Erneut versuchen** im Drawer plant den Processor neu ein. Das Hold-Gate läuft beim Start des Processors erneut, schließt also das Fenster zwischen „Sperre aufheben" und „Wiederholen". |
Contributor
There was a problem hiding this comment.
Fix mismatched quotation marks in the retry sentence.
Line 55 mixes German opening quotes with straight closing quotes, which renders inconsistently in docs. Use paired German quotes for both phrases.
✏️ Suggested fix
-Die Aktion **Erneut versuchen** im Drawer plant den Processor neu ein. Das Hold-Gate läuft beim Start des Processors erneut, schließt also das Fenster zwischen „Sperre aufheben" und „Wiederholen".
+Die Aktion **Erneut versuchen** im Drawer plant den Processor neu ein. Das Hold-Gate läuft beim Start des Processors erneut, schließt also das Fenster zwischen „Sperre aufheben“ und „Wiederholen“.🧰 Tools
🪛 LanguageTool
[typographical] ~55-~55: Zeichen ohne sein Gegenstück: ‚“‘ scheint zu fehlen
Context: ...eut, schließt also das Fenster zwischen „Sperre aufheben" und „Wiederholen". ## ...
(DE_UNPAIRED_QUOTES)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/de/platform/admin/data-subject-requests.md` at line 55, Fix the
mismatched quotation marks in the retry sentence: replace the straight closing
quotes with matching German closing quotes so both phrases use paired German
quotes—change „Sperre aufheben" to „Sperre aufheben“ and „Wiederholen" to
„Wiederholen“ (the sentence containing "Erneut versuchen" and those two
phrases).
Comment on lines
+12
to
+14
| Tale est administré par conception — il n'existe pas de portail libre-service côté personne concernée. L'admin déposant **est** le point de vérification d'identité, ayant confirmé l'identité via le processus de l'organisation (offboarding RH, ticket support, vérification en personne, etc.) avant d'ouvrir le dialogue. Le produit n'ajoute pas d'étape IDV en flux. | ||
|
|
||
| Ce contrat rend l'admin autoritaire pour la demande. Le conseil juridique peut traiter la phrase de confirmation (« ERASE ») dans le dialogue de dépôt comme la porte IDV : la frapper est un signal délibéré, audité, que l'admin a vérifié la personne. |
Contributor
There was a problem hiding this comment.
Don’t describe ERASE as an identity-verification control.
The previous paragraph correctly says IDV is handled out of band by the org admin, but Line 14 then re-labels the typed confirmation as the “porte IDV”. The UI only confirms operator intent; it does not verify the subject’s identity. That contradiction is risky in compliance docs and should be removed or reframed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/fr/platform/admin/data-subject-requests.md` around lines 12 - 14, The
paragraph incorrectly labels the typed confirmation token "ERASE" as an
identity-verification (IDV) control; update the text so it does not claim that
typing "ERASE" verifies the subject's identity—instead state that "ERASE" is an
auditable operator intent confirmation used after the admin has already
completed out‑of‑band IDV. Locate the passage referencing the admin as
authoritative and the phrase "porte IDV" (and the quoted token "ERASE") and
either remove the IDV wording or rephrase to clarify it is only a deliberate,
auditable confirmation of intent, not an identity check.
Comment on lines
+28
to
+35
| /** | ||
| * Locale-stable confirm phrase. Industry pattern (GitHub repo deletion, | ||
| * AWS S3 bucket policy removal): the typed phrase stays in English so | ||
| * the typing requirement is the same across languages — only the | ||
| * surrounding label localizes. | ||
| */ | ||
| const ERASURE_CONFIRM_PHRASE = 'ERASE'; | ||
|
|
Contributor
There was a problem hiding this comment.
Move the confirm phrase into the translation/config layer instead of hardcoding ERASE.
This string is user-facing and is also used in the equality check, so keeping it as an English literal here violates the repo i18n rule and makes future phrase changes easy to miss. If the phrase must stay locale-stable, each locale can still resolve the same value from i18n.
As per coding guidelines, "No hardcoded user-facing strings in React — always use the translation hook" and "Every user-facing string goes through the translation layer — never compare against an English literal in code, tests, stories, or comments."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@services/platform/app/features/settings/governance/data-subject-requests/file-request-dialog.tsx`
around lines 28 - 35, Replace the hardcoded ERASURE_CONFIRM_PHRASE constant with
a value resolved from the translation/config layer and use the translation hook
where FileRequestDialog validates input; specifically remove
ERASURE_CONFIRM_PHRASE and instead call the i18n lookup (e.g.
t('erasureConfirmPhrase') or config.get('erasureConfirmPhrase')) inside the
component that performs the equality check so the typed phrase comes from
translations/config and comparisons use that resolved value, updating any
tests/stories to reference the translation key.
Comment on lines
+207
to
+220
| <ol className="border-border flex flex-col gap-2 border-l pl-3"> | ||
| {auditEntries.map((entry) => ( | ||
| <li key={String(entry._id)} className="flex flex-col gap-0.5"> | ||
| <Text as="span" className="text-xs font-medium"> | ||
| {entry.action} | ||
| </Text> | ||
| <Text as="span" variant="muted" className="text-xs"> | ||
| <TableDateCell date={entry.timestamp} /> | ||
| {entry.errorMessage ? ` — ${entry.errorMessage}` : ''} | ||
| </Text> | ||
| </li> | ||
| ))} | ||
| </ol> | ||
| )} |
Contributor
There was a problem hiding this comment.
Add role="list" to the audit timeline <ol> element.
Tailwind Preflight resets list-style: none, which causes Safari/VoiceOver to remove list semantics. Explicitly adding role="list" restores proper announcement for assistive technology.
🛠️ Proposed fix
) : (
- <ol className="border-border flex flex-col gap-2 border-l pl-3">
+ <ol className="border-border flex flex-col gap-2 border-l pl-3" role="list">
{auditEntries.map((entry) => (As per coding guidelines, when reviewing React components in .tsx files, do not remove the role="list" attribute from ul/ol elements. This maintains accessibility for VoiceOver users and helps meet WCAG 2.1 Level AA requirements.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <ol className="border-border flex flex-col gap-2 border-l pl-3"> | |
| {auditEntries.map((entry) => ( | |
| <li key={String(entry._id)} className="flex flex-col gap-0.5"> | |
| <Text as="span" className="text-xs font-medium"> | |
| {entry.action} | |
| </Text> | |
| <Text as="span" variant="muted" className="text-xs"> | |
| <TableDateCell date={entry.timestamp} /> | |
| {entry.errorMessage ? ` — ${entry.errorMessage}` : ''} | |
| </Text> | |
| </li> | |
| ))} | |
| </ol> | |
| )} | |
| <ol className="border-border flex flex-col gap-2 border-l pl-3" role="list"> | |
| {auditEntries.map((entry) => ( | |
| <li key={String(entry._id)} className="flex flex-col gap-0.5"> | |
| <Text as="span" className="text-xs font-medium"> | |
| {entry.action} | |
| </Text> | |
| <Text as="span" variant="muted" className="text-xs"> | |
| <TableDateCell date={entry.timestamp} /> | |
| {entry.errorMessage ? ` — ${entry.errorMessage}` : ''} | |
| </Text> | |
| </li> | |
| ))} | |
| </ol> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@services/platform/app/features/settings/governance/data-subject-requests/request-detail-drawer.tsx`
around lines 207 - 220, The ordered list rendering the audit timeline (the <ol>
that maps over auditEntries in request-detail-drawer.tsx) is missing an explicit
role and therefore loses list semantics in Safari/VoiceOver due to Tailwind
Preflight; add role="list" to that <ol> element that contains the auditEntries
mapping so assistive tech announces it correctly while keeping the same children
(Text, TableDateCell, errorMessage) and keys (String(entry._id)).
Comment on lines
+65
to
+76
| onChange: (values: string[]) => { | ||
| const next: ErasureStatus[] = []; | ||
| for (const value of values) { | ||
| for (const s of ERASURE_STATUSES) { | ||
| if (s === value) { | ||
| next.push(s); | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| setStatusFilter(next); | ||
| }, |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider simplifying the type-narrowing loop.
The nested loop to narrow string[] to ErasureStatus[] works but scales O(n×m). A more direct approach uses filter with a type-guard helper or a Set for O(1) membership:
♻️ Cleaner alternative using filter + includes with type assertion
onChange: (values: string[]) => {
- const next: ErasureStatus[] = [];
- for (const value of values) {
- for (const s of ERASURE_STATUSES) {
- if (s === value) {
- next.push(s);
- break;
- }
- }
- }
- setStatusFilter(next);
+ setStatusFilter(
+ values.filter((v): v is ErasureStatus =>
+ ERASURE_STATUSES.includes(v as ErasureStatus)
+ )
+ );
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| onChange: (values: string[]) => { | |
| const next: ErasureStatus[] = []; | |
| for (const value of values) { | |
| for (const s of ERASURE_STATUSES) { | |
| if (s === value) { | |
| next.push(s); | |
| break; | |
| } | |
| } | |
| } | |
| setStatusFilter(next); | |
| }, | |
| onChange: (values: string[]) => { | |
| setStatusFilter( | |
| values.filter((v): v is ErasureStatus => | |
| ERASURE_STATUSES.includes(v as ErasureStatus) | |
| ) | |
| ); | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@services/platform/app/features/settings/governance/data-subject-requests/requests-list-section.tsx`
around lines 65 - 76, The nested loops in the onChange handler (inside the
onChange prop that builds next and calls setStatusFilter) are O(n×m); replace
them with a membership-based filter: create a Set from ERASURE_STATUSES (or use
ERASURE_STATUSES.includes in a type-guard) and map/filter the incoming values:
filter values by membership in that Set and cast/guard them to ErasureStatus,
then call setStatusFilter with the result; update the onChange implementation to
use this O(n) approach and keep references to ERASURE_STATUSES, ErasureStatus,
and setStatusFilter to locate the change.
Comment on lines
+22
to
+26
| const handleClose = () => { | ||
| void navigate({ | ||
| to: '/dashboard/$id/settings/governance/data-subject-requests', | ||
| params: { id: organizationId }, | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In TanStack Router v1, does navigate() push a new history entry by default, and is replace: true the correct way to avoid adding a new entry on modal/drawer close?
💡 Result:
Yes. In TanStack Router v1, navigate pushes a new history entry by default (replace defaults to false). [1][2] To avoid adding a new entry when closing a modal/drawer, use navigate(..., { replace: true }). This tells the router to commit the new location with history.replace instead of history.push. [1][2] Example: navigate({ to: '/your-route', replace: true })
Citations:
- 1: https://tanstack.com/router/v1/docs/guide/navigation
- 2: https://tanstack.com/router/v1/docs/api/router/NavigateOptionsType
🏁 Script executed:
# Check the actual file content at the specified location
cat -n services/platform/app/routes/dashboard/\$id/settings/governance/data-subject-requests/\$requestId.tsx | head -50Repository: tale-project/tale
Length of output: 1555
Use history replacement when closing the drawer route.
On line 23, navigate(...) uses default push behavior. Closing from the list route creates list → detail → list, so browser Back reopens the drawer unexpectedly.
🔧 Suggested fix
const handleClose = () => {
void navigate({
to: '/dashboard/$id/settings/governance/data-subject-requests',
params: { id: organizationId },
+ replace: true,
});
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handleClose = () => { | |
| void navigate({ | |
| to: '/dashboard/$id/settings/governance/data-subject-requests', | |
| params: { id: organizationId }, | |
| }); | |
| const handleClose = () => { | |
| void navigate({ | |
| to: '/dashboard/$id/settings/governance/data-subject-requests', | |
| params: { id: organizationId }, | |
| replace: true, | |
| }); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@services/platform/app/routes/dashboard/`$id/settings/governance/data-subject-requests/$requestId.tsx
around lines 22 - 26, The close handler currently navigates back with a push,
causing the history stack to be list → detail → list; update the handleClose
implementation (the function named handleClose that calls navigate({...})) to
navigate back using history replacement instead of push by passing the replace
option (e.g. include replace: true in the navigate call or use the framework’s
equivalent replace flag) so closing the drawer replaces the current entry rather
than adding a new one.
Comment on lines
+149
to
+179
| it('rejects when caller is not authenticated', async () => { | ||
| mockGetAuthUser.mockResolvedValue(null); | ||
| const { extendErasureDeadline } = | ||
| (await import('../erasure')) as unknown as { | ||
| extendErasureDeadline: { handler: Function }; | ||
| }; | ||
| const ctx = createMockCtx([rowFixture()]); | ||
| await expect( | ||
| extendErasureDeadline.handler(ctx, { | ||
| requestId: 'er_1', | ||
| extraDays: 7, | ||
| extensionReason: 'complex multi-jurisdiction review', | ||
| }), | ||
| ).rejects.toThrow(/sign in/i); | ||
| }); | ||
|
|
||
| it('rejects when caller is not an admin', async () => { | ||
| mockGetAuthUser.mockResolvedValue(MEMBER_USER); | ||
| mockGetOrganizationMember.mockResolvedValue({ role: 'member' }); | ||
| const { extendErasureDeadline } = | ||
| (await import('../erasure')) as unknown as { | ||
| extendErasureDeadline: { handler: Function }; | ||
| }; | ||
| const ctx = createMockCtx([rowFixture()]); | ||
| await expect( | ||
| extendErasureDeadline.handler(ctx, { | ||
| requestId: 'er_1', | ||
| extraDays: 7, | ||
| extensionReason: 'complex multi-jurisdiction review', | ||
| }), | ||
| ).rejects.toThrow(/admin/i); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Assert the Convex error codes on the auth branches too.
mapDsrError() keys off error.data.code, but these two tests only check message text. If the mutation regresses to throwing a plain Error, this suite still passes while the UI breaks. Match data.code for unauthenticated and forbidden here, like the later cases do.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@services/platform/convex/governance/__tests__/erasure_extend.test.ts` around
lines 149 - 179, Update the two auth-related tests for extendErasureDeadline to
assert the Convex error code in the thrown error object rather than only
matching the message text: when mockGetAuthUser resolves to null assert the
rejected error has data.code === 'unauthenticated', and when mockGetAuthUser
returns MEMBER_USER but mockGetOrganizationMember returns { role: 'member' }
assert the rejected error has data.code === 'forbidden'; locate these checks
around extendErasureDeadline.handler and replace or supplement the message-based
expect(...) assertions with assertions that the rejected error object contains
the correct data.code (e.g., using rejects.toHaveProperty or
rejects.toMatchObject) so regressions to plain Error will fail the tests.
Comment on lines
+147
to
+158
| // Multi-status (or unfiltered) path. We must paginate over the | ||
| // org-scoped collection and filter post-fetch; statuses-set membership | ||
| // is the discriminator. With a single status we'd take the indexed | ||
| // branch above, so this only fires for genuinely multi-valued filters. | ||
| const set = statuses ? new Set<string>(statuses) : null; | ||
| const result = await ctx.db | ||
| .query('gdprErasureRequests') | ||
| .withIndex('by_organizationId_status', (q) => | ||
| q.eq('organizationId', args.organizationId), | ||
| ) | ||
| .order('desc') | ||
| .paginate(args.paginationOpts); |
Contributor
There was a problem hiding this comment.
Use a chronologically ordered source for multi-status pages.
When statuses is empty or has more than one value, this paginates by_organizationId_status with only organizationId constrained. That stream is ordered by status before _creationTime, so these pages are grouped by status rather than truly newest-first as the API contract says. Because the filtering happens after pagination, the cursor also advances through that status-sorted stream. This needs an org+time index or a merge of per-status streams if you want multi-status pagination to stay globally chronological.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@services/platform/convex/governance/erasure_queries.ts` around lines 147 -
158, The current multi-status branch paginates gdprErasureRequests using
withIndex('by_organizationId_status') which orders by status before
_creationTime, so post-filtering by statuses will not produce a global
newest-first stream; instead either query an organization+time ordered index
(e.g., use or add and use an index like 'by_organizationId_creationTime' and
paginate that with q.eq('organizationId', args.organizationId) then filter the
result by the statuses Set) or implement a merge of per-status paginated streams
(issue parallel queries per status with withIndex('by_organizationId_status')
and merge-sort by _creationTime while advancing cursors) so pagination and
cursors reflect true chronological order for the statuses filter (refer to
symbols: statuses, gdprErasureRequests, withIndex('by_organizationId_status'),
args.organizationId, paginate and paginationOpts).
Comment on lines
+1516
to
+1525
| const extraDays = Math.trunc(args.extraDays); | ||
| if ( | ||
| !Number.isFinite(extraDays) || | ||
| extraDays < 1 || | ||
| extraDays > MAX_EXTENSION_DAYS | ||
| ) { | ||
| throw new ConvexError({ | ||
| code: 'validation', | ||
| message: `extraDays must be an integer between 1 and ${MAX_EXTENSION_DAYS}.`, | ||
| }); |
Contributor
There was a problem hiding this comment.
Reject fractional extraDays instead of truncating it.
Math.trunc() turns values like 1.9 into 1, so invalid input is silently accepted even though the API says this must be an integer. Validate the original value with Number.isInteger(args.extraDays) before computing the deadline.
Suggested fix
- const extraDays = Math.trunc(args.extraDays);
- if (
- !Number.isFinite(extraDays) ||
- extraDays < 1 ||
- extraDays > MAX_EXTENSION_DAYS
- ) {
+ if (
+ !Number.isInteger(args.extraDays) ||
+ args.extraDays < 1 ||
+ args.extraDays > MAX_EXTENSION_DAYS
+ ) {
throw new ConvexError({
code: 'validation',
message: `extraDays must be an integer between 1 and ${MAX_EXTENSION_DAYS}.`,
});
}
+ const extraDays = args.extraDays;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const extraDays = Math.trunc(args.extraDays); | |
| if ( | |
| !Number.isFinite(extraDays) || | |
| extraDays < 1 || | |
| extraDays > MAX_EXTENSION_DAYS | |
| ) { | |
| throw new ConvexError({ | |
| code: 'validation', | |
| message: `extraDays must be an integer between 1 and ${MAX_EXTENSION_DAYS}.`, | |
| }); | |
| if ( | |
| !Number.isInteger(args.extraDays) || | |
| args.extraDays < 1 || | |
| args.extraDays > MAX_EXTENSION_DAYS | |
| ) { | |
| throw new ConvexError({ | |
| code: 'validation', | |
| message: `extraDays must be an integer between 1 and ${MAX_EXTENSION_DAYS}.`, | |
| }); | |
| } | |
| const extraDays = args.extraDays; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@services/platform/convex/governance/erasure.ts` around lines 1516 - 1525, The
code currently truncates args.extraDays to an integer (const extraDays =
Math.trunc(args.extraDays)) which silently accepts fractional inputs; instead
validate the original value with Number.isInteger(args.extraDays) and that it is
finite and within range before using it (remove or avoid truncation). Update the
check to: ensure Number.isFinite(args.extraDays) &&
Number.isInteger(args.extraDays) && args.extraDays >= 1 && args.extraDays <=
MAX_EXTENSION_DAYS, and throw the same ConvexError if validation fails, then
assign extraDays = args.extraDays for subsequent use.
Org admins file, monitor, retry, and extend GDPR Art. 17 erasure requests directly from Settings > Governance, instead of going through the Convex dashboard. Backend gains a structured reasonCode on every request and a single-grant Art. 12(3) extension model so the SLA badge derives from the controller's actual response window. Closes #1688.
53 review sub-agents (35 + 18 verifier) flagged ~30 verified Critical /
High / Medium issues on the data-subject-requests branch. This PR
addresses every one of them plus expands cascade scope to wfExecutions
and personal-scope promptTemplates.
Backend correctness:
- Cross-org IDOR fix: requestErasure now validates the target userId is
a member of the requesting org, closing a global lockout-bypass
primitive (twoFactorAttempts / loginAttempts are global tables).
- Concurrency: replace the racy ALREADY_PENDING range probe with an
activeErasureClaims claim row mirroring the activeLegalHoldClaims
pattern; covers partial / failed states the old guard missed.
- scrubSubjectAuditLogs: add a resource-pass on
by_org_resourceType_resourceId so admin-authored rows about the
subject (gdpr_erasure_requested itself, password resets, etc.) are
scrubbed; signed checkpoint reflects both passes.
- Index correctness: eraseSubjectUserMemories switches to the existing
by_user_org_status_deleted_created prefix scan; eraseSubjectMessage
Feedback uses a new by_org_user index. Removes 16K-cap spoliation
pattern previously only fixed for threadMetadata.
- eraseSubjectNotifications: new by_org_subject indexed pass via
notifications.subjectUserId (populated at write time by the lockout
caller); legacy email-hash pass kept as best-effort for pre-fix rows.
- Cascade completeness: notifications / wfExecutions / promptTemplates
added to PerCategoryCounts; thread cascade now distinguishes
hold-skip from clean-completion via skippedByHold flag so receipts
do not over-report threadsErased.
- Watchdog: ERASURE_WATCHDOG_TIMEOUT_MESSAGE constant; late-finalize
guard preserves the watchdog verdict but persists the counts the
action did complete + records lateFinalizeAt for forensics.
- Retry: finalizeProcessing accumulates documentsErased,
ragDocumentsRemoved, wfExecutionsErased, promptTemplatesErased
across attempts so partial -> retry no longer under-reports.
- New erasers: eraseSubjectWfExecutions (with PII blob deletion) and
eraseSubjectPromptTemplates (scope=personal only).
- beginProcessing: blacklist -> whitelist guard, fail-closed for any
future status.
- cascade_attempts_exhausted audit row keyed to user (not thread) so
the receipt UI's resource filter actually surfaces it; renamed to
underscore form for action-name consistency.
UI / a11y / i18n:
- Drawer: handle isError, translate audit action names, mount
LegalHoldBlockPanel for blocked status, surface threadsBlockedByHold
/ threadsSkippedByHold / startedAt / completedAt counts, gate
canExtend on slaDeadlineAt > now, drop dual-title contradiction.
- List: derive isInitialLoading / isLoadingMore from status, switch to
DataTable.infiniteScroll, gate query on ability, drop misleading
Trash2 icon, prune dead useCallback / useMemo.
- StatusBadge: distinct hue per state (pending=amber, running=sky) +
leading icon to remove color-only signal; opacity /15 -> /20 for
WCAG 1.4.11 non-text contrast.
- SlaCountdownBadge: same opacity bump + tone icon + role=status.
- LegalHoldBlockPanel: drop contradictory role=alert + aria-live=polite,
switch to role=status; title text -> foreground for AA contrast.
- i18n: 4-locale keys for auditActions, drawer.errorState,
threadsBlockedByHold / threadsSkippedByHold / startedAt /
completedAt; de DSR section aligned to 'Legal Hold' loanword
convention; de.json nav matches page title; fr neutral status
forms ('Termine', etc.) matching masculine 'Statut' header; fr
'Personne' -> 'Personne concernee' for RGPD canonical term; de-CH
adds ss override for the one DSR string with eszett and drops
byte-identical redundant override.
Cleanup:
- 16 sites of 'as unknown as { handler: Function }' across two test
files consolidated to a single justified loadErasure / importQueries
helper per AGENTS.md's framework-gap exception.
- Drop redundant cast in erasure.ts (PerCategoryCounts.fileMetadata
already types ragPurgeStorageIds).
- Drop hardcoded English t('groups.trash', 'Trash') fallback.
- Drop tautological 'module sanity' test.
Schema additions are all additive-only (new optional fields, new
table, new indexes). No expand-contract migration needed.
Tests: +13 new cases (8 cross-org + 5 claim) on top of existing 184.
All 197 governance tests pass; lint + typecheck clean.
larryro
force-pushed
the
feat/data-subject-requests-ui
branch
from
68bb531 to
9e71312
Compare
`eraseSubjectFileMetadata` returns `{rows, blobs, ragPurgeStorageIds,
skippedByHold}`; the processor assigns this to
`perCategory.fileMetadata` and forwards the whole `perCategory` to
`finalizeProcessing`. The validator there only declared three fields,
so Convex's strict-extra-field check rejected every real run with
ArgumentValidationError on `.perCategory.fileMetadata.ragPurgeStorageIds`.
Tests didn't catch it because they mock `runMutation` and never pass
through the Convex validator boundary. Caught at runtime against a
real deployment.
Fix: add `ragPurgeStorageIds: v.optional(v.array(v.string()))` to the
fileMetadata sub-validator. The receipt and audit log carry it
verbatim — harmless storage ids, no PII.
Two governance / UX gaps surfaced after the previous PR went live with
real test runs:
1. The receipt drawer showed only 3 counters (threads / RAG / docs)
while the cascade actually clears 13 categories — regulator-facing
accountability risk.
2. Filing was a one-click destructive action: a compromised admin
credential could permanently erase any subject's data instantly,
FRCP-37(e) spoliation risk and well below industry standard
(OneTrust / TrustArc default to 24-72h cooling-off + dual approval).
This PR closes both: receipts are now self-contained across all 13
categories, and filing goes through a configurable cooling-off window
with cancellation, dual-admin approval, rate limiting, and self-
deletion refusal.
Backend:
- new `governancePolicies` policyType `dsar_governance` with config
`{ coolingOffHours: 0..72 = 24, requireDualApproval: bool = false,
dailyLimitPerAdmin: 1..50 = 5 }`. Reuses the existing single-row
per-org / Zod-validated config pattern.
- `gdprErasureRequests` adds optional `effectiveAt`,
`scheduledJobId`, `cancelledAt`/`cancelledBy`/`cancellationReason`,
and `perCategorySnapshot`; new terminal status `cancelled`.
- `requestErasure` now: rate-limits per (org, admin) at the
platform-level rate limiter (5/day default), refuses
self-deletion, reads `dsar_governance` policy, schedules
`processErasureRequest` via `runAfter(coolingOffMs)` and stores
the job id on the row, and fans out a `notifications` row to
every admin in the org. When `requireDualApproval` is on, the
call writes an `approvals` row (`resourceType: 'erasure'`,
filer in metadata) instead of scheduling — a second admin must
approve via the existing approvals flow.
- `approvals.updateApprovalStatus` dispatches `executing` for
`'erasure'` resources to a new internal mutation
`confirmAndScheduleErasure` that enforces filer != approver and
then sets `effectiveAt + scheduledJobId` itself.
- new `cancelErasureRequest` mutation: any org admin can call it
while `status='pending' && effectiveAt > now`. Calls
`ctx.scheduler.cancel(jobId)`, clears the per-subject claim row,
fans out a cancelled-notification, writes a
`gdpr_erasure_cancelled` audit row.
- `beginProcessing` adds a hard `effectiveAt > now` guard so a
stray scheduler call cannot pull the action up early — `running`
now strictly means "cascade is actively executing".
- `retryErasureRequest` also enforces self-deletion refusal and
goes through the same cooling-off window (no bypass via retry).
- `finalizeProcessing` writes `perCategorySnapshot` to the row so
the receipt is regulator-self-contained without depending on
audit-log JSON (which may itself be PII-scrubbed in some
futures); late-finalize-after-watchdog branch also writes it.
- per-rate-limiter bucket `governance:dsar_request` (fixed window,
5/day) added; new `'erasure'` literal in
`approvalResourceTypeValidator`.
UI:
- drawer: cooling-off banner with countdown + Cancel button when
`pending && effectiveAt > now`; cancelled-state block with
cancellation reason / actor / time when `status='cancelled'`;
`wfExecutionsErased` and `promptTemplatesErased` rows for Tier
1 transparency; `<details>`-collapsed Full Breakdown across all
13 categories rendered from `perCategorySnapshot`, with zero
values aggregated to a single line.
- StatusBadge: pending sub-state labels — "Pending · Nh until
execution" with Clock icon for cooling-off, "Awaiting approval"
with UserCheck icon for dual-approval pending, plain "Pending"
otherwise; new `cancelled` status (Ban icon, muted tone).
- new CancelDialog (FormDialog + zod, 10-char min reason).
- new DsarPolicyEditor section embedded above the requests list
on `/governance/data-subject-requests` (no separate route).
- list view forwards `effectiveAt` to StatusBadge so cooling-off
countdowns appear in the table.
i18n:
- 4-locale keys for status sub-states, drawer cooling-off banner /
cancelled block / Full Breakdown, 12 category labels, cancel
dialog, cancellation toasts, new error codes
(`notCancellable`, `cannotCancelAfterCooldown`, `rateLimited`,
`dualApprovalRequired`), notifications (`dsarScheduled`,
`dsarApprovalNeeded`, `dsarCancelled`), DSR policy editor
labels and validation messages.
- notification i18n keys added to `keys-dynamic.txt` since they
are looked up from server-stored row strings (titleKey/bodyKey).
Tests:
- new `erasure_cancellation.test.ts` (6 cases): happy cancel
releases scheduler + claim + fans notification; refuses past
cooling-off; refuses non-admin; refuses short reason;
self-deletion guard fires + audits with
`self_deletion_forbidden`; rate-limit guard fires + audits with
`rate_limited`.
- existing `erasure_cross_org.test.ts` and `erasure_claim.test.ts`
extended with mocks for the new dependencies (rateLimiter,
dsar_policy, notifications, approvals).
- 218 governance/notifications/audit_logs/i18n tests pass; lint
+ typecheck clean.
Schema additions are all additive-only. No expand-contract
migration. `coolingOffHours: 0` retains the previous immediate-
execution behavior for orgs that prefer the legacy model.
Two issues with the prior commit's editor:
1. The Save button rendered the raw key `common.actions.save`
because `useT('governance')` namespaces the lookup as
`governance.common.actions.save`, which doesn't exist.
2. Three infrequent governance fields don't merit an explicit Save
button — modern admin UX (Notion / Linear) auto-saves on blur,
removing the "did I save?" cognitive load.
Fix both at once by removing the Save button entirely:
- Each number Input commits on `onBlur`. Validation (Zod-aligned
range checks: 0–72 for cooling-off, 1–50 for daily limit) runs
inline; invalid values toast an error and revert the field to
the last saved value.
- The Switch commits immediately on `onCheckedChange` since
toggles have no blur affordance.
- Inputs and Switch disable while `upsertMutation.isPending` to
prevent rapid double-commits.
The orphan `common.actions.save` lookup goes away with the button.
No new i18n keys needed; the existing `dsarPolicy.saved` /
`dsarPolicy.saveFailed` / `dsarPolicy.invalidCoolingOffHours` /
`dsarPolicy.invalidDailyLimit` keys cover all toast paths.
Closes a meta-vulnerability in the DSAR governance policy: an admin
who could already file an erasure request could also disable the
safeguards that constrain it (cooling-off, dual approval, daily
limit). Two-pronged fix:
- Writes restricted to the org owner. Admins can still read and
cancel pending changes, but the dispatch path goes through
`proposeDsarPolicy`; the generic `upsertPolicy` refuses
`dsar_governance` and redirects.
- Any *weakening* change (shorter cooling-off, disabling dual
approval, raising the daily limit) is staged on the row instead
of applied. The scheduler flips it 24 hours later via an
idempotent internal mutation. Tightening still applies
immediately. Any admin can cancel during the window — so a
compromised owner can't both lower the bar and use it within a
day. Mirrors the password_policy pending-effective pattern.
Cross-admin notifications fan out at every transition (tighten,
loosen-proposed, loosen-applied, loosen-cancelled) so other admins
can react before a weakened policy takes effect.
UI:
- Pending banner shows proposer's email + user id together
("foo@bar (k977…)"), the diff per axis, and a Cancel button.
- Form resyncs reactively from Convex on every push (was
initialize-once, so cancel/apply didn't visibly restore the
toggle without a reload).
- During the grace window every input is greyed out and
pointer-events disabled, with a Lock + "Locked while review
window is open" hint on the specific field that's pending.
Tests: 14 new cases covering isLoosening axes, owner-only refusal,
tightening immediate-apply, refuse-when-pending-exists, admin
cancel, scheduler.cancel, and idempotent apply.
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
reasonCode(Art. 17(1)(a)–(f) plus operationalcontract_termination) captured at request time so receipts can be classified by ground.listErasureRequests/getErasureRequestqueries plus the receipt UI: list, detail drawer with audit timeline, file / extend / retry dialogs, color-coded SLA countdown badge, inline legal-hold block panel.The page is named for the DSR umbrella so future Art. 16 / Art. 20 flows can land as additional
kindvalues without a rename. Identity verification stays out of band by design — admin-mediated B2B contract.Closes #1688.
Pre-PR checklist
bun run check(format, lint, typecheck, all tests). 241 test files / 3280 tests / 0 lint errors.services/platform/messages/{en,de,fr}.json(andde-CH.jsonoverrides for parity)./docs/{en,de,fr}/for every user-visible change (andde-CH/); new admin doc linked fromdocs/nav.json.bun run --filter @tale/docs lintandbun run --filter @tale/docs test. 7 files / 26 tests green.README.md,README.de.md,README.fr.md— N/A (not a feature highlight).Test plan
done; receipt drawer showsreasonCode,threadsErased,ragDocumentsRemoved, audit timeline.LegalHoldBlockPanelappears in the dialog with held thread / document counts; click navigates to legal-hold settings; receipt row is recorded withstatus=blocked.partialrow → Retry → cascade re-runs; new audit entrygdpr_erasure_retriedappears in the timeline.extensionDeadlineAt(green/yellow/red threshold check); second extension attempt rejected withALREADY_EXTENDED; extension afterslaDeadlineAtrejected withDEADLINE_LAPSED.useAbility('write','orgSettings'); deep-link to/data-subject-requestsshowsAccessDenied; deep-link to/.../$requestId403s on the query./data-subject-requests/$requestIdopens detail; close returns to list; keyboard reachable; status changes havearia-live.en,de,fr,de-CH) render labels, toasts, and the file-request dialog correctly.Issue-vs-code reconciliation
The issue body has three small drifts from the live code on
main(post-#1676); the PR treats live code as ground truth:retryErasureProcessor; live code already hasretryErasureRequest(admin-gated, only valid onpartial/failed/blocked, idempotent on terminal states). Reused as-is.gdpr_erasure_retry_requested; live code emitsgdpr_erasure_retried. Reused as-is.blocked).blockedis included as a filter value because it is part of the live state machine and the audit trail.Summary by CodeRabbit