feat(documents): Move document between organizations - #1308
Open
Pallavikumarimdb wants to merge 5 commits into
Open
feat(documents): Move document between organizations #1308Pallavikumarimdb wants to merge 5 commits into
Pallavikumarimdb wants to merge 5 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds document moving between organizations across server persistence, API handling, activity logging, and client dialog wiring. ChangesMove Document Feature
Estimated code review effort: 3 (Moderate) | ~30 minutes Related issue: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
Pallavikumarimdb
marked this pull request as ready for review
July 2, 2026 13:25
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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
`@apps/papra-client/src/modules/documents/components/move-document-dialog.component.tsx`:
- Around line 14-24: Reset the selected target organization whenever
MoveDocumentDialog closes and when it is opened for a new document, because the
component’s createSignal state persists across opens in
MoveDocumentDialogProvider. Update the MoveDocumentDialog logic around
getTargetOrgId/setTargetOrgId so cancel, success, outside-close, and prop
changes clear the selection before the next open. While touching the dialog UI,
surface props.documentName in the title/body so the user can confirm which
document is being moved.
- Around line 59-64: The error handling in move-document-dialog.component.tsx is
surfacing raw backend text via error?.message, which bypasses localization.
Update the onError callback to always use a translated user-facing message
through t(...) (for example, a generic move failure string), and only log or
inspect the raw error separately if needed. Keep the fix within the move
dialog’s createToast error path so the UI remains consistent for all locales.
In `@apps/papra-server/src/modules/app/database/database.test-utils.ts`:
- Line 43: The test database helper is creating file-backed databases with
Math.random() and leaving behind stray test-*.db files. Update the database
setup in database.test-utils.ts, specifically the setupDatabase call that builds
the url, to use a stronger unique identifier and ensure cleanup is handled
either in the helper itself or in the tests that consume it. If cleanup is
intentionally external, confirm the generated files are ignored and purged, and
document that behavior near setupDatabase so it is clear where teardown happens.
In `@apps/papra-server/src/modules/documents/documents.repository.test.ts`:
- Around line 360-447: Add coverage in the moveDocument test block for the
ownership-mismatch guard and unchanged metadata. Extend
createDocumentsRepository/moveDocument tests to assert it rejects or throws when
sourceOrganizationId does not match the document’s current organizationId, and
verify the returned document keeps fields like name, originalName,
originalStorageKey, originalSha256Hash, and mimeType unchanged while only
organizationId changes. Use the existing moveDocument setup and
createInMemoryDatabase fixtures to keep the new cases close to the current
happy-path test.
In `@apps/papra-server/src/modules/documents/documents.repository.ts`:
- Around line 578-629: The `moveDocument` transaction currently assumes
`sourceOrganizationId` and `targetOrganizationId` are different, but that
protection only exists in the route layer. Add a same-organization guard inside
`moveDocument` itself, before the `db.transaction` work begins, so the
repository use case enforces its own contract. If the IDs are equal, fail fast
with the same kind of invalid-request/domain error used elsewhere in
`documents.repository.ts`, and keep the destructive deletes in
`documentsTagsTable` and `documentCustomPropertyValuesTable` from running.
🪄 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 Plus
Run ID: db83d287-4855-4d92-844b-6027b09a6ed8
📒 Files selected for processing (14)
apps/papra-client/src/index.tsxapps/papra-client/src/locales/en.dictionary.tsapps/papra-client/src/modules/documents/components/document-management-dropdown.component.tsxapps/papra-client/src/modules/documents/components/move-document-dialog.component.tsxapps/papra-client/src/modules/documents/documents.services.tsapps/papra-server/src/modules/app/database/database.test-utils.tsapps/papra-server/src/modules/app/events/events.handlers.tsapps/papra-server/src/modules/documents/document-activity/document-activity.constants.tsapps/papra-server/src/modules/documents/documents.events.types.tsapps/papra-server/src/modules/documents/documents.repository.test.tsapps/papra-server/src/modules/documents/documents.repository.tsapps/papra-server/src/modules/documents/documents.routes.tsapps/papra-server/src/modules/documents/documents.usecases.tsapps/papra-server/src/modules/documents/events/activity-log.document-moved.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/papra-server/src/modules/documents/documents.repository.ts (1)
594-632: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMove the underlying file on organization transfer.
buildOriginalDocumentKeyprefixesorganizationId, so this leaves the document row in the target org while the file stays under the source org namespace. UpdateoriginalStorageKeyand move/copy the object as part of this transaction.🤖 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 `@apps/papra-server/src/modules/documents/documents.repository.ts` around lines 594 - 632, The organization transfer flow in documents.repository.ts updates the document row and related tables but leaves the underlying file in the old org namespace because buildOriginalDocumentKey includes organizationId. In the transaction around the document move logic, update originalStorageKey to the target organization’s key and move/copy the stored object so the file is relocated with the document. Use the existing documentId transfer path and the surrounding tx block in the repository method to keep the row, metadata, and storage key in sync.
♻️ Duplicate comments (2)
apps/papra-server/src/modules/documents/documents.repository.test.ts (1)
359-478: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing ownership-mismatch and metadata-preservation coverage.
The previously requested test cases (rejecting when
sourceOrganizationIddoesn't match the document's actual org, and asserting unrelated fields likename/originalStorageKey/mimeTypesurvive the move unchanged) still aren't present. Only the success path and the same-organization guard are covered.✅ Suggested additional test case
+ test('throws when the document does not belong to the source organization', async () => { + const { db } = await createInMemoryDatabase({ + organizations: [ + { id: 'org-1', name: 'Org 1' }, + { id: 'org-2', name: 'Org 2' }, + ], + documents: [ + { + id: 'doc-1', + organizationId: 'org-1', + name: 'Doc 1', + originalName: 'doc-1.pdf', + originalStorageKey: 'key-1', + originalSha256Hash: 'hash1', + mimeType: 'application/pdf', + }, + ], + }); + + const documentsRepository = createDocumentsRepository({ db }); + + await expect( + documentsRepository.moveDocument({ + documentId: 'doc-1', + sourceOrganizationId: 'org-2', + targetOrganizationId: 'org-1', + }), + ).rejects.toThrow(); + });🤖 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 `@apps/papra-server/src/modules/documents/documents.repository.test.ts` around lines 359 - 478, Add the missing moveDocument test coverage in documents.repository.test.ts: the current describe('moveDocument') block only checks the success path and same-organization guard, but it still lacks a case for sourceOrganizationId not matching the document’s actual organization and a case asserting metadata preservation. Extend createDocumentsRepository/moveDocument tests to verify the repository rejects ownership mismatches, and in the successful move path confirm unrelated document fields such as name, originalStorageKey, and mimeType remain unchanged after the organization transfer.apps/papra-client/src/modules/documents/components/move-document-dialog.component.tsx (1)
65-70: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRaw backend error message still surfaced, bypassing i18n.
error?.messageis shown verbatim on failure, which will display untranslated/technical backend text to non-English users. This was previously flagged and remains unaddressed.💬 Suggested fix
onError: (error: any) => { createToast({ - message: error?.message || t('documents.move.error'), + message: t('documents.move.error'), type: 'error', }); }🤖 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 `@apps/papra-client/src/modules/documents/components/move-document-dialog.component.tsx` around lines 65 - 70, The move document error handling in move-document-dialog.component.tsx is still surfacing raw backend text via the onError callback. Update the error toast logic to always use the translated fallback from t('documents.move.error') for user-facing messaging, and only log or inspect error?.message internally if needed; keep the behavior localized by changing the onError handler and createToast call in MoveDocumentDialog.
🤖 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
`@apps/papra-client/src/modules/documents/components/move-document-dialog.component.tsx`:
- Line 84: The DialogTitle in move-document-dialog.component.tsx hardcodes the
colon separator outside the translation, which bypasses localization. Update the
documents.move.title usage so the document name is passed through the i18n call
as an interpolated value, and adjust the translation key/value to include the
full localized title format rather than concatenating with a literal separator.
---
Outside diff comments:
In `@apps/papra-server/src/modules/documents/documents.repository.ts`:
- Around line 594-632: The organization transfer flow in documents.repository.ts
updates the document row and related tables but leaves the underlying file in
the old org namespace because buildOriginalDocumentKey includes organizationId.
In the transaction around the document move logic, update originalStorageKey to
the target organization’s key and move/copy the stored object so the file is
relocated with the document. Use the existing documentId transfer path and the
surrounding tx block in the repository method to keep the row, metadata, and
storage key in sync.
---
Duplicate comments:
In
`@apps/papra-client/src/modules/documents/components/move-document-dialog.component.tsx`:
- Around line 65-70: The move document error handling in
move-document-dialog.component.tsx is still surfacing raw backend text via the
onError callback. Update the error toast logic to always use the translated
fallback from t('documents.move.error') for user-facing messaging, and only log
or inspect error?.message internally if needed; keep the behavior localized by
changing the onError handler and createToast call in MoveDocumentDialog.
In `@apps/papra-server/src/modules/documents/documents.repository.test.ts`:
- Around line 359-478: Add the missing moveDocument test coverage in
documents.repository.test.ts: the current describe('moveDocument') block only
checks the success path and same-organization guard, but it still lacks a case
for sourceOrganizationId not matching the document’s actual organization and a
case asserting metadata preservation. Extend
createDocumentsRepository/moveDocument tests to verify the repository rejects
ownership mismatches, and in the successful move path confirm unrelated document
fields such as name, originalStorageKey, and mimeType remain unchanged after the
organization transfer.
🪄 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 Plus
Run ID: 86cb095c-64d2-4a5e-b232-404fda0a73de
📒 Files selected for processing (4)
apps/papra-client/src/modules/documents/components/move-document-dialog.component.tsxapps/papra-server/src/modules/documents/documents.errors.tsapps/papra-server/src/modules/documents/documents.repository.test.tsapps/papra-server/src/modules/documents/documents.repository.ts
Pallavikumarimdb
force-pushed
the
feat/move-document-between-organizations
branch
from
July 4, 2026 10:13
80447bc to
e6e1631
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@apps/papra-server/src/modules/documents/documents.routes.ts`:
- Around line 567-574: The same-organization validation in the documents route
duplicates the repository’s shared guard and emits a different error shape.
Update the `documents.routes.ts` move flow to use
`createDocumentSameOrganizationError` from `documents.errors.ts` instead of
constructing the inline `createError` in the same-organization check, so the
route and `moveDocument` return the same `code` and message consistently.
🪄 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 Plus
Run ID: 90857687-042a-4971-bbb0-7ee65fe98873
📒 Files selected for processing (15)
apps/papra-client/src/index.tsxapps/papra-client/src/locales/en.dictionary.tsapps/papra-client/src/modules/documents/components/document-management-dropdown.component.tsxapps/papra-client/src/modules/documents/components/move-document-dialog.component.tsxapps/papra-client/src/modules/documents/documents.services.tsapps/papra-server/src/modules/app/database/database.test-utils.tsapps/papra-server/src/modules/app/events/events.handlers.tsapps/papra-server/src/modules/documents/document-activity/document-activity.constants.tsapps/papra-server/src/modules/documents/documents.errors.tsapps/papra-server/src/modules/documents/documents.events.types.tsapps/papra-server/src/modules/documents/documents.repository.test.tsapps/papra-server/src/modules/documents/documents.repository.tsapps/papra-server/src/modules/documents/documents.routes.tsapps/papra-server/src/modules/documents/documents.usecases.tsapps/papra-server/src/modules/documents/events/activity-log.document-moved.ts
…, and clean up test DBs
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/papra-server/src/modules/documents/documents.repository.ts (1)
619-676: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftMove the storage mutation out of the transaction
The file copy/delete happens before the remaining DB writes. If a later statement fails, the SQL transaction rolls back but the storage changes do not, leaving the document row and file state out of sync. Defer the move until after the DB work succeeds, or add compensating cleanup.
🤖 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 `@apps/papra-server/src/modules/documents/documents.repository.ts` around lines 619 - 676, The move logic in documents.repository.ts is performing storage copy/delete inside the same transaction as the database updates, which can leave file state out of sync if later tx writes fail. Refactor the document move flow around the document move/update method so the tx only handles DB mutations first, then perform the documentsStorageService file copy/delete after the transaction succeeds, or add a compensating rollback path for the storage operations. Keep the existing behavior tied to documentToMove, documentsStorageService, saveFile, deleteFile, and the tx update/delete calls.
🤖 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 `@apps/papra-client/src/locales/en.dictionary.ts`:
- Line 450: The locale entry for documents.move.title is using single-brace
placeholder syntax, which won’t be interpolated by the translation call. Update
the documents.move.title string in the en.dictionary locale to use double-brace
interpolation so the existing t('documents.move.title', { name:
props.documentName }) usage in the move dialog renders the document name
correctly.
---
Outside diff comments:
In `@apps/papra-server/src/modules/documents/documents.repository.ts`:
- Around line 619-676: The move logic in documents.repository.ts is performing
storage copy/delete inside the same transaction as the database updates, which
can leave file state out of sync if later tx writes fail. Refactor the document
move flow around the document move/update method so the tx only handles DB
mutations first, then perform the documentsStorageService file copy/delete after
the transaction succeeds, or add a compensating rollback path for the storage
operations. Keep the existing behavior tied to documentToMove,
documentsStorageService, saveFile, deleteFile, and the tx update/delete calls.
🪄 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 Plus
Run ID: 99bf4f13-dfe0-4ff2-a1c2-aa0f3351487e
📒 Files selected for processing (7)
apps/papra-client/src/locales/en.dictionary.tsapps/papra-client/src/modules/documents/components/move-document-dialog.component.tsxapps/papra-server/src/modules/app/database/database.test-utils.tsapps/papra-server/src/modules/documents/documents.repository.test.tsapps/papra-server/src/modules/documents/documents.repository.tsapps/papra-server/src/modules/documents/documents.routes.tsapps/papra-server/src/modules/documents/documents.usecases.ts
…ck on document move and fix localization interpolation syntax
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@apps/papra-client/src/locales/en.dictionary.ts`:
- Line 450: The interpolation in the documents.move.title dictionary entry is
inconsistent with the rest of the locale file because it uses an unspaced
placeholder. Update the documents.move.title string in en.dictionary.ts to match
the existing interpolation style used by other entries such as those with {{
name }} and {{ provider }}, keeping the same key but normalizing the placeholder
formatting.
In `@apps/papra-server/src/modules/documents/documents.repository.ts`:
- Around line 621-642: In the document move flow inside documents.repository.ts,
the current logic in the file copy block skips saving when
documentsStorageService.fileExists returns false but still allows the
transaction to update originalStorageKey to the new value. Update the move path
around the fileExists/getFileStream/saveFile sequence so that a missing source
file causes the move to fail and aborts the transaction instead of proceeding
with the DB pointer update. Use the existing documentToMove,
documentsStorageService, and newStorageKey handling to ensure the metadata
update only happens after a successful file copy.
- Around line 694-710: The rollback path in documents.repository.ts is deleting
the newly moved file even after the database update has already succeeded, which
leaves the document pointing at missing storage. Update the move flow around the
save/delete logic in the repository method so that newStorageKey is only cleaned
up when the DB transaction or save fails before commit, and never from the outer
catch after the commit has succeeded. Keep the cleanup for
documentToMove.originalStorageKey on success, and adjust the
savedNewFile/documentsStorageService error handling so the catch only rolls back
uncommitted changes.
🪄 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 Plus
Run ID: 08ca8ebe-527b-4e8c-97df-cf8aa4e4870a
📒 Files selected for processing (3)
apps/papra-client/src/locales/en.dictionary.tsapps/papra-server/src/modules/documents/documents.repository.test.tsapps/papra-server/src/modules/documents/documents.repository.ts
Contributor License Agreement✅ Everyone who contributed to this pull request has signed the CLA for it. |
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.
Description
Allows moving documents between organizations, handles clearing of organization-scoped tags/custom property values, updates FTS search indexes and share links, records activity logs, and integrates the Move dialog UI on the client.
Data Integrity & Scope Transitions
What is cleared during the move:
What is preserved:
Closes: #272