Repository navigation
feat(db,permissions): DM file-attachment foundations (PR 2/N) - #1189
Merged
Merged
Conversation
Adds fileConversations join table mirroring filePages (composite PK on fileId+conversationId, both FKs cascade, linkedBy set-null, file/conv indexes). Drops NOT NULL on files.driveId so conversation-uploaded files can persist without a drive. Adds nullable fileId (FK to files, on delete set null, indexed) and attachmentMeta jsonb on direct_messages, typed via the shared AttachmentMeta from storage. Generated migration 0114_nifty_true_believers.sql via pnpm db:generate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
canUserAccessFile now accepts a nullable driveId. Adds a second linkage branch that joins fileConversations against dmConversations and grants when the user is participant1 or participant2. Page and conversation branches are checked independently so a user who fails the page check on a dual-linked file can still grant via the conversation path, and vice versa. Falls back to drive membership only when no linkages exist and driveId is non-null; deny when no linkages and driveId is null. Test count goes from 7 to 15: page-branch grant/deny/short-circuit, participant1/participant2/non-participant on the conversation branch, three dual-linkage cases, and the null-drive-no-linkages deny. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
After the canUserAccessFile check, view/download routes return 503 when file.driveId is null. createDriveServiceToken requires a non-null driveId, so without this guard the routes fail typecheck once schema makes driveId nullable. No conversation-uploaded files exist yet, so the guard is dead at runtime today. The polymorphic upload core PR will replace it with a conversation-scoped token mint. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Match the schema after files.driveId became nullable. Conversation-uploaded files appear in the export with driveId === null. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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 (15)
✨ 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. Review rate limit: 0/1 reviews remaining, refill in 28 minutes and 39 seconds.Comment |
![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: e7b631826a
ℹ️ 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".
After files.driveId became nullable, processor's delete-file rbac path inferred string | null | undefined from the db query but the local DeleteFileContext interface and isResourceBindingAllowed parameter still typed it as string | undefined. This broke processor's tsc build (TS2322). Widen both to string | null | undefined. All consumers already handle null correctly: the binding equality check returns false against null, and the truthy guard before getUserDrivePermissions narrows to string. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Match the pattern in file-links.ts:69 — coerce null→undefined where the db result is read, keeping fileDriveId typed as string | undefined throughout the rest of rbac. Behavior is identical; this is just consistency cleanup of the previous build fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
After this branch made files.driveId nullable and introduced file_conversations + direct_messages.fileId, the orphan detector predicate (and the type returned from it) drifted out of sync with the schema. Without this fix, polymorphic uploads landing in PR #6 of the DM-files epic would be silently harvested by the cleanup cron because their references aren't in any of file_pages, channel_messages, or pages.filePath. - findOrphanedFileRecords / isFileOrphaned: add LEFT JOINs against file_conversations and direct_messages with the corresponding IS NULL guards. Extend the storagePath sibling-EXISTS check to consider the new tables too. - OrphanedFile.driveId: widen to string | null to match schema. - Cron route: skip null-driveId orphans (warn + defer); they need a conversation-scoped token mint that lands with the polymorphic upload core PR. Without this guard, createDriveServiceToken (which requires non-null driveId) fails typecheck once schema becomes nullable. - Tests: lock the predicate by asserting the SQL references both new tables, cover null-driveId mapping, and lock the cron skip path. Dead at runtime today (no rows in either new table) but eliminates a real footgun for the next PR in the stack. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 2, 2026
2witstudios
added a commit
that referenced
this pull request
May 15, 2026
* feat(db): DM file-attachment schema groundwork Adds fileConversations join table mirroring filePages (composite PK on fileId+conversationId, both FKs cascade, linkedBy set-null, file/conv indexes). Drops NOT NULL on files.driveId so conversation-uploaded files can persist without a drive. Adds nullable fileId (FK to files, on delete set null, indexed) and attachmentMeta jsonb on direct_messages, typed via the shared AttachmentMeta from storage. Generated migration 0114_nifty_true_believers.sql via pnpm db:generate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(permissions): conv-linked file access canUserAccessFile now accepts a nullable driveId. Adds a second linkage branch that joins fileConversations against dmConversations and grants when the user is participant1 or participant2. Page and conversation branches are checked independently so a user who fails the page check on a dual-linked file can still grant via the conversation path, and vice versa. Falls back to drive membership only when no linkages exist and driveId is non-null; deny when no linkages and driveId is null. Test count goes from 7 to 15: page-branch grant/deny/short-circuit, participant1/participant2/non-participant on the conversation branch, three dual-linkage cases, and the null-drive-no-linkages deny. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(files): guard null driveId before drive token After the canUserAccessFile check, view/download routes return 503 when file.driveId is null. createDriveServiceToken requires a non-null driveId, so without this guard the routes fail typecheck once schema makes driveId nullable. No conversation-uploaded files exist yet, so the guard is dead at runtime today. The polymorphic upload core PR will replace it with a conversation-scoped token mint. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(gdpr): widen UserFileExport.driveId nullable Match the schema after files.driveId became nullable. Conversation-uploaded files appear in the export with driveId === null. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(processor): widen rbac fileDriveId for nullable schema After files.driveId became nullable, processor's delete-file rbac path inferred string | null | undefined from the db query but the local DeleteFileContext interface and isResourceBindingAllowed parameter still typed it as string | undefined. This broke processor's tsc build (TS2322). Widen both to string | null | undefined. All consumers already handle null correctly: the binding equality check returns false against null, and the truthy guard before getUserDrivePermissions narrows to string. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(processor): collapse rbac driveId at db boundary Match the pattern in file-links.ts:69 — coerce null→undefined where the db result is read, keeping fileDriveId typed as string | undefined throughout the rest of rbac. Behavior is identical; this is just consistency cleanup of the previous build fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(cleanup): extend orphan detector to DM linkage tables After this branch made files.driveId nullable and introduced file_conversations + direct_messages.fileId, the orphan detector predicate (and the type returned from it) drifted out of sync with the schema. Without this fix, polymorphic uploads landing in PR #6 of the DM-files epic would be silently harvested by the cleanup cron because their references aren't in any of file_pages, channel_messages, or pages.filePath. - findOrphanedFileRecords / isFileOrphaned: add LEFT JOINs against file_conversations and direct_messages with the corresponding IS NULL guards. Extend the storagePath sibling-EXISTS check to consider the new tables too. - OrphanedFile.driveId: widen to string | null to match schema. - Cron route: skip null-driveId orphans (warn + defer); they need a conversation-scoped token mint that lands with the polymorphic upload core PR. Without this guard, createDriveServiceToken (which requires non-null driveId) fails typecheck once schema becomes nullable. - Tests: lock the predicate by asserting the SQL references both new tables, cover null-driveId mapping, and lock the cron skip path. Dead at runtime today (no rows in either new table) but eliminates a real footgun for the next PR in the stack. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
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
Foundations for DM file attachments — the channel-side DRY refactors landed in #1175; this PR lands the DB schema and the permission helper extension that everything else hangs off.
file_conversationsjoin table mirroringfile_pages;files.driveIdbecomes nullable;direct_messagesgains nullablefileId(FK set-null) andattachmentMetajsonb. Migration0114_nifty_true_believers.sql.canUserAccessFile(userId, fileId, driveId: string | null)now checksfilePagesandfileConversations(joined againstdmConversations) in parallel. Page and conversation branches grant independently. Drive fallback only when no linkages exist anddriveId !== null. 15 tests cover all branches incl. dual-linkage independence and the null-drive-no-linkages deny.file_conversationsanddirect_messages.fileIdso polymorphic uploads landing in PR Add sheet unit tests and document the new page type #6 of the epic don't get silently harvested by the cleanup cron.OrphanedFile.driveIdwidened tostring | nullto match schema. The cron route skips null-driveId orphans (deferred until a conversation-scoped token mint lands).view/downloadroutes return 503 whenfile.driveId === nullafter the access check. Dead at runtime (no DM-uploaded files exist yet) — present only to keep typecheck green and to be removed by the polymorphic upload core PR.UserFileExport.driveIdwidened tostring | nullto match schema.apps/processor/src/services/rbac.tscollapsesfileRecord?.driveId ?? undefinedat the db boundary soDeleteFileContext.fileDriveIdstaysstring | undefined. Matches the existing pattern inapps/processor/src/services/file-links.ts:69. Behavior unchanged.No user-visible behavior change.
Epic position
Per
tasks/dm-file-attachments.md:MessageAttachment,useAttachmentUploadcanUserAccessFileconversation branch (this PR)Test plan
pnpm --filter @pagespace/db build— schema compiles, drizzle-kit generates clean migrationpnpm --filter @pagespace/lib typecheck+lintpnpm --filter web typecheckpnpm --filter @pagespace/processor build(regression for the schema-induced rbac break)pnpm --filter @pagespace/processor test→ 923/923vitest run packages/lib/src/permissions packages/lib/src/compliance/export packages/lib/src/compliance/file-cleanup→ 256+/256+file-access.test.ts→ 15/15orphan-detector.test.ts→ 13/13 (incl. new file_conversations / direct_messages predicate locks)cleanup-orphaned-files/__tests__/route.test.ts→ 5/5 (incl. null-driveId skip)IF NOT EXISTSguards on indexes / FKs)🤖 Generated with Claude Code