Repository navigation
docs: update stale compliance doc and prototype panes - #864
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 19 minutes and 1 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughUpdated compliance docs and prototype UI to reflect implemented retention/erasure behaviors, DSAR export endpoints and distributed rate-limiting, and daily audit-log integrity verification; message hard-delete retention and activity/audit log hash-chain fixes are documented and UI status cards adjusted accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API as API Server
participant RL as Redis (rate-limit)
participant DB
participant S3
participant Cron
User->>API: GET /api/account/export or /api/admin/users/:id/export
API->>RL: check/increment export quota (distributed)
RL-->>API: allowed
API->>DB: gather export data (accounts, messages, AI logs)
DB-->>API: export payload (JSON)
API->>S3: upload ZIP (optional large exports)
S3-->>API: URL
API-->>User: ZIP/JSON response (or S3 link)
Note over Cron,DB: periodic jobs
Cron->>DB: purge soft‑deleted messages older than 30 days
Cron->>DB: run AI log anonymize/purge per retention config
DB-->>Cron: purge results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 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 `@docs/security/compliance-sovereignty-analysis.md`:
- Around line 565-569: The document has an internal contradiction: the list near
line 565 marks "Formal audit log integrity verification" as resolved, but
Section 6 still lists "Add audit log integrity verification cron" as pending (P2
item `#8`); update P2 item `#8` in Section 6 to reflect the resolved state (e.g.,
mark as Resolved/Completed or change text to "Verification cron added — activity
log chain fix in progress") so wording matches the resolved note shown in the
audit-log summary; ensure you reference and adjust the "Add audit log integrity
verification cron" entry in Section 6 and keep the parenthetical "activity log
chain fix in progress" if appropriate.
In `@prototypes/pagespace-endgame/src/components/panes/GdprPane.tsx`:
- Around line 219-224: The GdprPane component's Gaps summary still claims there
are message-deletion edge gaps even though the "Hard-delete for messages — Done"
section (the h4 and following paragraph) marks hard-delete implemented; update
the Gaps intro text inside the GdprPane component to remove or reword the
sentence that references message-deletion edge gaps so the pane is internally
consistent with the hard-delete done statement (search for the Gaps summary
JSX/variable and the sentence mentioning message-deletion edge gaps and delete
or replace it with a note that the 30-day purge cron and FK cascade satisfy
deletion requirements).
🪄 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: ecc49b0c-fac9-4b69-a902-a7eee3114b06
📒 Files selected for processing (3)
docs/security/compliance-sovereignty-analysis.mdprototypes/pagespace-endgame/src/components/panes/CompliancePane.tsxprototypes/pagespace-endgame/src/components/panes/GdprPane.tsx
![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: b2e8396422
ℹ️ 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".
| - No API endpoint to delete a user's AI usage history | ||
| **Status — partially resolved**: | ||
| - ~~Account deletion does NOT purge these logs~~ → **Fixed**: `deleteAiUsageLogsForUser()` purges on account deletion | ||
| - ~~No API endpoint to delete a user's AI usage history~~ → **Fixed**: account deletion handles this via FK cascade |
There was a problem hiding this comment.
This line says account deletion relies on FK cascade for ai_usage_logs, but that guarantee does not exist: ai_usage_logs.userId is not declared as a foreign key in packages/db/src/schema/monitoring.ts, and account deletion explicitly calls deleteAiUsageLogsForUser() with an in-code note that no FK cascade exists (apps/web/src/app/api/account/route.ts). Keeping the FK-cascade claim here can mislead compliance reviews and future refactors into removing the only real deletion path.
Useful? React with 👍 / 👎.
| **Status — partially resolved**: | ||
| - ~~Account deletion does NOT purge these logs~~ → **Fixed**: `deleteAiUsageLogsForUser()` purges on account deletion | ||
| - ~~No API endpoint to delete a user's AI usage history~~ → **Fixed**: account deletion handles this via FK cascade | ||
| - ~~Conversation messages retained indefinitely~~ → **Fixed**: 30-day purge cron for soft-deleted messages + FK cascade on account deletion |
There was a problem hiding this comment.
Declaring conversation-message retention “fixed” overstates current behavior: page chat assistant rows are saved with userId: null (apps/web/src/app/api/ai/chat/route.ts), so user-account cascade deletion cannot remove them, and the purge job only hard-deletes rows already soft-deleted (isActive=false). For users who chatted in shared pages, account deletion can leave assistant replies (which may still contain user-derived personal data), so Art. 17 coverage is not complete yet.
Useful? React with 👍 / 👎.
…d fixes DSAR export, message hard-delete, AI log purge on account deletion, and audit chain verification are now implemented — update stale gap claims. Note in-progress work on pu/hash-chain-pii and pu/export-rate-limit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Update for #865 (Redis export rate limit), #866 (activity log PII exclusion), #867 (activity chain serialization), #868-870 (audit service wiring), #863 (GDPR cron jobs), #861 (password auth removed). Fix code review comments: AI usage log deletion is explicit call not FK cascade; note shared-page assistant messages survive account deletion; resolve P2 #8 and #9 contradictions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
b2e8396 to
172f744
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
prototypes/pagespace-endgame/src/components/panes/GdprPane.tsx (1)
27-30:⚠️ Potential issue | 🟡 MinorMake DSAR scope wording precise in UI copy.
Line 27 says “all user data,” which is easy to read as author-only data. Match implementation intent by calling out workspace-scoped accessible data.
Based on learnings: DSAR export intentionally includes all pages from drives where the user was a member (workspace-scoped access), not only pages authored by that user.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prototypes/pagespace-endgame/src/components/panes/GdprPane.tsx` around lines 27 - 30, Update the DSAR UI copy in the GdprPane component to avoid saying “all user data” and instead explicitly state that the export contains all workspace-scoped data the user has access to (e.g., pages from drives where the user was a member) — edit the descriptive text in GdprPane.tsx (the paragraph listing profile, drives, pages, messages, files metadata, activity logs, AI usage logs, and tasks) to replace “all user data” with phrasing like “all workspace-accessible data (including pages from drives where you were a member)” so it matches the implementation intent.
🧹 Nitpick comments (1)
prototypes/pagespace-endgame/src/components/panes/GdprPane.tsx (1)
69-71: Consider migrating newly added inline styles to Tailwind utility classes.These new blocks continue inline
styleusage. If you’re already touching this pane, moving these additions to utility classes would align with repo styling rules.As per coding guidelines: "
**/*.{tsx,css}: Use Tailwind CSS and shadcn/ui components for styling and UI".Also applies to: 159-171, 210-220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prototypes/pagespace-endgame/src/components/panes/GdprPane.tsx` around lines 69 - 71, Replace the inline style props in GdprPane.tsx with Tailwind utility classes: remove style={{ marginBottom: 16 }} on the Card(s) and add a className like "mb-4" (or "mb-6" to match spacing), replace p style={{ marginTop: 6, fontSize: 12 }} with className "mt-1 text-xs", and apply equivalent Tailwind classes for the other referenced blocks (the similar Card/p usages around the other sections) so all styling uses className and Tailwind utilities consistent with shadcn/ui usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/security/compliance-sovereignty-analysis.md`:
- Around line 491-495: Resolve the contradiction by reconciling the statement
"Standardize bcrypt cost factor — N/A. Password-based authentication removed
entirely" with the claim in Section 7.1 that "local white-label can use local
email+password auth": update Section 7.1 (the local white-label authentication
paragraph) to reflect the current truth (either remove mention of local
email+password auth if it is deprecated/removed, or change item 9 to indicate
it's deprecated/optional and provide migration/exception details), and add a
brief note referencing the relevant change requests/PRs (`#861`) and the expected
status (removed, deprecated, or supported) so both places consistently state the
same policy.
- Around line 253-257: The docs wording claiming the DSAR export covers “all
user data” is ambiguous; update the documentation entry describing `GET
/api/account/export` (and the admin `GET /api/admin/users/[userId]/export`) to
state explicitly that exports are workspace-scoped and include data accessible
to the user as a member of drives/workspaces (i.e., exactly what the
implementation in packages/lib/src/compliance/export/gdpr-export.ts does—for
example `collectUserPages` exports pages from drives where the user is a
member), so rephrase the bullet to clarify scope and match the implementation
details.
In `@prototypes/pagespace-endgame/src/components/panes/GdprPane.tsx`:
- Around line 69-76: The UI text in the GdprPane Card (the descriptive paragraph
inside the Card component) overstates compliance with GDPR Art.17; update the
copy to explicitly note the caveat that shared-page assistant messages with
userId: null may persist even after account deletion. Edit the message text (the
paragraph rendered alongside the <Card> component and the other similar block
around lines 213-216) to add a short clause stating that messages without a
userId on shared pages are not removed by account cascade and may remain, so the
UI accurately reflects the limitation.
---
Outside diff comments:
In `@prototypes/pagespace-endgame/src/components/panes/GdprPane.tsx`:
- Around line 27-30: Update the DSAR UI copy in the GdprPane component to avoid
saying “all user data” and instead explicitly state that the export contains all
workspace-scoped data the user has access to (e.g., pages from drives where the
user was a member) — edit the descriptive text in GdprPane.tsx (the paragraph
listing profile, drives, pages, messages, files metadata, activity logs, AI
usage logs, and tasks) to replace “all user data” with phrasing like “all
workspace-accessible data (including pages from drives where you were a member)”
so it matches the implementation intent.
---
Nitpick comments:
In `@prototypes/pagespace-endgame/src/components/panes/GdprPane.tsx`:
- Around line 69-71: Replace the inline style props in GdprPane.tsx with
Tailwind utility classes: remove style={{ marginBottom: 16 }} on the Card(s) and
add a className like "mb-4" (or "mb-6" to match spacing), replace p style={{
marginTop: 6, fontSize: 12 }} with className "mt-1 text-xs", and apply
equivalent Tailwind classes for the other referenced blocks (the similar Card/p
usages around the other sections) so all styling uses className and Tailwind
utilities consistent with shadcn/ui usage.
🪄 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: ece0711b-2761-4280-b6ea-f315e0627e00
📒 Files selected for processing (3)
docs/security/compliance-sovereignty-analysis.mdprototypes/pagespace-endgame/src/components/panes/CompliancePane.tsxprototypes/pagespace-endgame/src/components/panes/GdprPane.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- prototypes/pagespace-endgame/src/components/panes/CompliancePane.tsx
| **Current state**: Data subject access and export is implemented: | ||
| - **User DSAR export**: `GET /api/account/export` — ZIP archive with JSON files | ||
| covering profile, drives, pages, messages, files metadata, activity logs, | ||
| AI usage logs, and tasks. Rate-limited to 1 export per 24 hours. | ||
| - **Admin DSAR endpoint**: `GET /api/admin/users/[userId]/export` — admin-only |
There was a problem hiding this comment.
Clarify DSAR export scope to avoid legal ambiguity.
Line 254 says export covers “all user data,” which can be interpreted as author-only ownership. The implementation/export policy is workspace-scoped (data accessible during membership), so this should be explicit in this section too.
Suggested doc patch
-- **User DSAR export**: `GET /api/account/export` — ZIP archive with JSON files
- covering profile, drives, pages, messages, files metadata, activity logs,
+-- **User DSAR export**: `GET /api/account/export` — ZIP archive with JSON files
+ covering profile, drives, and workspace-scoped pages/messages/files/activity
+ data the user had access to during membership, plus AI usage logs and tasks.
- AI usage logs, and tasks. Rate-limited to 1 export per 24 hours.
+ Rate-limited to 1 export per 24 hours.Based on learnings: In packages/lib/src/compliance/export/gdpr-export.ts, collectUserPages intentionally exports all pages from drives where the user is a member (workspace-scoped export).
📝 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.
| **Current state**: Data subject access and export is implemented: | |
| - **User DSAR export**: `GET /api/account/export` — ZIP archive with JSON files | |
| covering profile, drives, pages, messages, files metadata, activity logs, | |
| AI usage logs, and tasks. Rate-limited to 1 export per 24 hours. | |
| - **Admin DSAR endpoint**: `GET /api/admin/users/[userId]/export` — admin-only | |
| **Current state**: Data subject access and export is implemented: | |
| - **User DSAR export**: `GET /api/account/export` — ZIP archive with JSON files | |
| covering profile, drives, and workspace-scoped pages/messages/files/activity | |
| data the user had access to during membership, plus AI usage logs and tasks. | |
| Rate-limited to 1 export per 24 hours. | |
| - **Admin DSAR endpoint**: `GET /api/admin/users/[userId]/export` — admin-only |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/security/compliance-sovereignty-analysis.md` around lines 253 - 257, The
docs wording claiming the DSAR export covers “all user data” is ambiguous;
update the documentation entry describing `GET /api/account/export` (and the
admin `GET /api/admin/users/[userId]/export`) to state explicitly that exports
are workspace-scoped and include data accessible to the user as a member of
drives/workspaces (i.e., exactly what the implementation in
packages/lib/src/compliance/export/gdpr-export.ts does—for example
`collectUserPages` exports pages from drives where the user is a member), so
rephrase the bullet to clarify scope and match the implementation details.
Section 7.1 referenced "local email+password auth" — password auth was removed in #861; on-prem now uses magic links + passkeys. GdprPane message deletion cards now note shared-page assistant messages with userId: null may survive account deletion. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
prototypes/pagespace-endgame/src/components/panes/GdprPane.tsx (1)
69-77: Prefer Tailwind classes over inline style props in these updated blocks.The newly updated cards/paragraphs still use inline
styleprops; please migrate these changed spots to Tailwind utility classes for consistency with repo UI conventions.As per coding guidelines: "Use Tailwind CSS and shadcn/ui components for styling and UI."
Also applies to: 159-167, 170-176, 213-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@prototypes/pagespace-endgame/src/components/panes/GdprPane.tsx` around lines 69 - 77, In GdprPane replace inline style props on the Card and p elements with Tailwind utility classes: remove style={{ marginBottom: 16 }} on the Card (e.g., add a spacing class such as "mb-4" or "mb-6" as appropriate) and replace style={{ marginTop: 6, fontSize: 12 }} on the paragraph with equivalent Tailwind classes (e.g., "mt-1 text-xs"); apply the same pattern to the other updated blocks referenced (the other Card and p occurrences around the listed ranges) so all inline style usage in GdprPane is converted to Tailwind utility classes while keeping the same visual spacing and font size.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@prototypes/pagespace-endgame/src/components/panes/GdprPane.tsx`:
- Around line 69-77: In GdprPane replace inline style props on the Card and p
elements with Tailwind utility classes: remove style={{ marginBottom: 16 }} on
the Card (e.g., add a spacing class such as "mb-4" or "mb-6" as appropriate) and
replace style={{ marginTop: 6, fontSize: 12 }} on the paragraph with equivalent
Tailwind classes (e.g., "mt-1 text-xs"); apply the same pattern to the other
updated blocks referenced (the other Card and p occurrences around the listed
ranges) so all inline style usage in GdprPane is converted to Tailwind utility
classes while keeping the same visual spacing and font size.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3cdf067b-c967-468d-b02e-4b3e57d3e800
📒 Files selected for processing (2)
docs/security/compliance-sovereignty-analysis.mdprototypes/pagespace-endgame/src/components/panes/GdprPane.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/security/compliance-sovereignty-analysis.md
Resolved items were sitting in Gaps/End Game with "Fixed"/"Done" labels, breaking the narrative flow. Now: - Current: hash chain integrity, distributed rate limit, message hard-delete all live in their natural subsections - Gaps: only genuine gaps remain (cookie consent, data residency, SIEM, audit coverage, agent trails) - End Game: only future work (no "Done" items cluttering the roadmap) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Test plan
cd prototypes/pagespace-endgame && pnpm dev)🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
New Features
Bug Fixes
Known Gap