Repository navigation
Tech debt & feature gaps: auth, persistence, cleanup, docs - #508
Conversation
, #430, #432, #438, #424, #423) Fixes seven open issues covering auth policy, dead code removal, missing persistence, cache hardening, and security documentation. - #437: Calendar event edit now allows drive admins/owners, not just creator - #433: Integration configOverrides.rateLimit passed to rate limiter - #430: Page-agent conversation PATCH persists title via upsert - #432: Remove unused driveInvitations table and retention cleanup - #438: DriveSwitcher sorts recent drives by real lastAccessedAt - #424: Document permission cache TTL, audit and fix bypassCache on mutations - #423: Document desktop MCP trust model exception to zero-trust Fixes #437, Fixes #433, Fixes #430, Fixes #432, Fixes #438, Fixes #424, Fixes #423 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded
⌛ 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. 📝 WalkthroughWalkthroughThis PR addresses five linked technical debts: (1) implements persistent title updates for page-agent conversations via a new upsert repository method, (2) adds drive-admin authorization to calendar event editing with comprehensive tests, (3) extends permission-check functions with a bypassCache option for sensitive mutations, (4) introduces drive last-access tracking with a new endpoint and updates drive-switcher ordering, and (5) removes obsolete driveInvitations schema and related retention logic, updating docs to reflect direct-add membership model. Additionally, it adds connection-level rate-limit overrides for integrations and comprehensive security documentation for permission caching and desktop MCP trust model. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as DriveSwitcher Component
participant API as POST /drives/[driveId]/access
participant Service as Drive Service
participant DB as Database<br/>(driveMembers)
participant Response as JSON Response
UI->>API: POST /api/drives/{driveId}/access<br/>(authenticated, CSRF protected)
activate API
API->>API: Extract driveId from route params
API->>API: Authenticate request<br/>(session or MCP)
alt Auth Successful
API->>Service: updateDriveLastAccessed<br/>(userId, driveId)
activate Service
Service->>DB: UPDATE driveMembers<br/>SET lastAccessedAt = NOW()<br/>WHERE userId=? AND driveId=?
activate DB
DB-->>Service: Confirmation
deactivate DB
Service-->>API: Void (Promise)
deactivate Service
API->>Response: { success: true }
else Auth Failed
API->>Response: { error, statusCode }
else Error Occurs
API->>Response: { error: 'An error occurred',<br/>statusCode: 500 }
end
deactivate API
Response-->>UI: JSON Response
Note over UI: Fire-and-forget:<br/>silently ignore errors
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/src/app/api/ai/page-agents/[agentId]/conversations/[conversationId]/route.ts (1)
45-65:⚠️ Potential issue | 🟠 MajorMissing input validation for
title.
titleis destructured from the request body without any validation. If the client sendsundefined,null, a non-string value, or an excessively long string, it flows straight into the DB upsert. Since this is the endpoint's core purpose,titleshould be validated as a required, non-empty string with a reasonable length cap.Proposed fix
// Parse request body const body = await request.json(); const { title } = body; + if (typeof title !== 'string' || title.trim().length === 0) { + return NextResponse.json( + { error: 'Title is required and must be a non-empty string' }, + { status: 400 } + ); + } + + if (title.length > 255) { + return NextResponse.json( + { error: 'Title must be 255 characters or fewer' }, + { status: 400 } + ); + } + // Validate that the conversation exists (has at least one active message)You'll also want to add corresponding test cases for bad/missing
titlein the test file.packages/lib/src/services/drive-service.ts (1)
138-148:⚠️ Potential issue | 🟠 MajorOwned drives will always have
nulllastAccessedAt— access tracking fails silently because owners lackdriveMembersrows.
driveLastAccessedis populated solely fromdriveMembersrows (lines 85-110). When a user owns a drive and calls the access-tracking POST endpoint at/api/drives/[driveId]/access,updateDriveLastAccessedattempts to UPDATEdriveMemberswhereuserIdanddriveIdmatch (lines 386-392). Since the drive owner is never added todriveMembersduring drive creation (line 169-179 only inserts into thedrivestable), this UPDATE affects 0 rows and silently fails. Consequently,driveLastAccessed.get(drive.id)remains undefined, defaulting tonull(line 142), and owned drives never appear in the "Recent" section based on recency.To fix this, either:
- Add the owner to
driveMembersautomatically when creating a drive, or- Update
updateDriveLastAccessedto handle owned drives separately (using thedrives.ownerIdfield)
🤖 Fix all issues with AI agents
In `@docs/2.0-architecture/2.2-backend/permissions.md`:
- Around line 276-277: Update this section to use the same function names as
Section 5: replace any occurrences of grantPagePermission and
revokePagePermission with grantPagePermissions and revokePagePermissions
(including prose, examples, and any referenced invalidation function names) so
the docs consistently reference grantPagePermissions / revokePagePermissions
everywhere; ensure accompanying sentences that mention “invalidation functions”
call out the pluralized names too.
In `@docs/security/permission-cache-threat-model.md`:
- Line 62: The sentence currently says "Sub-millisecond" while also stating a
1–5ms range; update the text so they match by either changing the phrase
"Sub-millisecond" to "few milliseconds" (or "1–5 milliseconds") or by adjusting
the numeric range to be <1ms; specifically edit the line containing the quoted
phrase "Timing differences are sub-millisecond" and the adjacent numeric range
"1-5ms" so both use consistent wording (e.g., "few milliseconds (≈1–5 ms)") for
clarity.
🧹 Nitpick comments (7)
docs/security/desktop-mcp-trust-model.md (1)
36-36: Tighten phrasing.“Outside of” is redundant here—“outside” reads cleaner.
✏️ Proposed tweak
- Modify PageSpace configuration outside of their own tool responses -- the MCP manager controls config read/write + Modify PageSpace configuration outside their own tool responses -- the MCP manager controls config read/writedocs/security/zero-trust-architecture.md (1)
1470-1472: Vary repeated sentence starts.Three consecutive bullets start with “MCP servers do…”. Consider rephrasing one or two for readability.
packages/lib/src/compliance/retention/retention-engine.test.ts (1)
1-5: Minor: consider consolidating the three separate@pagespace/dbimports.Lines 2–5 have three separate import statements from
@pagespace/db. This is pre-existing, but since line 3 was touched in this change, it could be a good opportunity to merge them.-import { db, users, sessions, socketTokens, verificationTokens, emailUnsubscribeTokens, pageVersions, driveBackups, aiUsageLogs } from '@pagespace/db'; -import { pagePermissions } from '@pagespace/db'; -import { pulseSummaries } from '@pagespace/db'; -import { drives, pages } from '@pagespace/db'; +import { db, users, sessions, socketTokens, verificationTokens, emailUnsubscribeTokens, pageVersions, driveBackups, aiUsageLogs, pagePermissions, pulseSummaries, drives, pages } from '@pagespace/db';apps/web/src/components/layout/navbar/DriveSwitcher.tsx (1)
110-111: Local store not updated after tracking access — stale recency until next fetch.The fire-and-forget POST updates the server, but the local
drivesarray in the Zustand store still holds the oldlastAccessedAt. If the user re-opens the switcher without a drives refetch, the "Recent" ordering won't reflect the access they just made. Consider optimistically updating the drive'slastAccessedAtin the store (e.g., viauseDriveStore.getState()) after the POST, so the dropdown is immediately consistent.apps/web/src/app/api/drives/[driveId]/access/route.ts (1)
18-19: Consider logging the caught error for observability.The catch block discards error details. Since this is a new endpoint, adding a log line would help diagnose issues in production without changing the 500 response.
🔧 Suggested improvement
- } catch { + } catch (error) { + console.error('Failed to update drive access time:', error); return NextResponse.json({ error: 'Failed to update access time' }, { status: 500 }); }packages/lib/src/services/drive-service.ts (2)
37-37:Date | string | nullis a loose type for a service-layer interface.The service layer (backed by Drizzle) should consistently return
Date | null. Thestringvariant leaks serialization concerns into the domain type. The client-facingDrivetype intypes.tscorrectly usesstring | nullfor the API boundary.♻️ Tighten the service-layer type
- lastAccessedAt: Date | string | null; + lastAccessedAt: Date | null;
310-317:getDriveWithAccessalways returnslastAccessedAt: null— actual value not fetched.Unlike
listAccessibleDriveswhich queriesdriveMembers.lastAccessedAt, this function hardcodesnull. If this endpoint is ever used wherelastAccessedAtmatters, the value will be silently missing. If this is intentional (only the list view needs it), a comment would clarify the design choice.
- Add title validation (type, empty, length) to conversation PATCH with 3 tests - Fix drive owner access tracking: upsert driveMembers row for owners - Optimistically update local store on drive switch for immediate recency - Log errors in drive access endpoint for observability - Fix function name mismatch in permissions docs (plural → singular) - Fix timing claim inconsistency in permission cache threat model - Tighten DriveWithAccess.lastAccessedAt to Date | null (service layer) - Consolidate split @pagespace/db imports in retention engine - Vary sentence starts in zero-trust MCP exception section - Fix "outside of" → "outside" in MCP trust model doc - Add clarifying comment on getDriveWithAccess hardcoded lastAccessedAt Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Review Feedback Addressed — de56b25All actionable comments and nitpicks from the CodeRabbit review have been addressed in commit de56b25. Here's the summary: Major Issues (2/2 resolved)1. Missing title validation (conversation PATCH)
2. Owned drives lack driveMembers rows
Minor Issues (2/2 resolved)3. Doc function name mismatch — Fixed Section 5 to use singular 4. Timing claim inconsistency — Changed "sub-millisecond" to "a few milliseconds" to match the 1-5 ms range. Nitpicks (7/7 addressed)
Verification
|
1 participant
Summary
Fixes 7 open issues covering auth policy gaps, dead code, missing persistence, cache hardening, and security documentation.
canEditEvent()now allows drive admins/owners to edit drive events, not just the creator. 7 new tests.execute-tool.tsnow passesconnection.configOverrides.rateLimitto the rate limiter instead ofundefined. 2 new tests.upsertConversationTitle()using the conversations table, replacing the placeholder response. 3 new tests.driveInvitationstable definition,invitationStatusenum, and retention engine cleanup for the always-empty table (-102 lines). Migration generation deferred due to pre-existing snapshot collision.lastAccessedAtfromdriveMembers, with fire-and-forget access tracking on drive switch. New POST/api/drives/[driveId]/accessendpoint.bypassCache: trueto 10 mutation handlers that were missing it, created threat model doc.Fixes #437, Fixes #433, Fixes #430, Fixes #432, Fixes #438, Fixes #424, Fixes #423
Test plan
pnpm typecheck— all packages pass (web TS6053 errors are pre-existing missing.next/types)pnpm --filter web lint— zero warnings or errorspnpm vitest run— 208/209 files pass (1 pre-existing DB connection failure inadmin-role-version.test.ts)pnpm db:generatefor Tech debt: Reconcile drive invitation model with auto-accepted member adds #432 migration after snapshot collision is resolved on master🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Improvements