feat: skip upload for zero-byte files and publish creation event - #422
Conversation
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughZero-byte temporal files now bypass upload operations and publish an empty-content event. Drive-server error mapping recognizes empty-file responses, propagating them through persistence, overrides, and replacement notifications. FUSE release tests now use deterministic asynchronous synchronization. ChangesEmpty-file upload handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant TemporalFileUploader
participant eventBus
participant driveServerClient
participant CreateFileOnTemporalFileUploaded
participant SyncFileMessenger
TemporalFileUploader->>TemporalFileUploader: detect zero-byte file
TemporalFileUploader->>eventBus: publish empty-content event
driveServerClient->>driveServerClient: map status and message
driveServerClient-->>CreateFileOnTemporalFileUploaded: propagate EMPTY_FILE error
CreateFileOnTemporalFileUploaded->>SyncFileMessenger: report replacement upload issue
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/context/storage/TemporalFiles/application/upload/TemporalFileUploader.test.ts`:
- Around line 30-35: Update the zero-byte upload test using an image extension
so thumbnail-capable handling is exercised, assert that repository.read is not
called, and inspect the published event to verify the aggregate/content ID is
empty while size remains 0 and the path is preserved.
In
`@src/context/storage/TemporalFiles/application/upload/TemporalFileUploader.ts`:
- Around line 35-41: Update TemporalFileUploader’s zero-size branch to publish
the empty-file event without invoking thumbnail extraction or repository.read;
preserve EMPTY_CONTENTS_ID and expected event metadata. In
src/context/storage/TemporalFiles/application/upload/TemporalFileUploader.test.ts
lines 30-35, use an extension that supports thumbnails for the empty file. In
lines 123-131, assert repository.read is not called and verify the published
event contains EMPTY_CONTENTS_ID and the expected metadata.
🪄 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 Plus
Run ID: dfacca72-0075-4851-b2ff-d4c2a056c3d0
📒 Files selected for processing (2)
src/context/storage/TemporalFiles/application/upload/TemporalFileUploader.test.tssrc/context/storage/TemporalFiles/application/upload/TemporalFileUploader.ts
|
You are still not handling the possible 402 caused by Then, for the creation of files i would try and find a way of pushing an issue exactly stating the error that has happened, probably on FileCreator But i dont know, thats up to your criteria where to implement such thing For the updating of files it should be pretty much the same, probably i would do it on CreateFileOnTemporalFileUploaded.ts.on() probably because here is where we are already checking for the files size rejection. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/context/virtual-drive/files/application/create/CreateFileOnOfflineFileUploaded.test.ts (1)
102-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the notification filename.
The production change adds
name: basename(event.path), but this test only verifieserrorandcause. Assert the expected basename as well so path-handling regressions are detected.🤖 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 `@src/context/virtual-drive/files/application/create/CreateFileOnOfflineFileUploaded.test.ts` around lines 102 - 120, Update the notifier.issues assertion in the override-rejection test to also verify that the notification’s name equals the basename of uploadedEvent.path, while preserving the existing error and cause assertions.
🤖 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
`@src/context/virtual-drive/files/application/create/CreateFileOnTemporalFileUploaded.ts`:
- Around line 61-70: Protect the awaited notifier.issues call in the on() error
path so notifier failures are caught and logged, while processing continues with
the original upload error. Preserve the existing FILE_TOO_BIG handling and
logging behavior after notification attempts.
In `@src/context/virtual-drive/files/infrastructure/SDKRemoteFileSystem.test.ts`:
- Around line 72-82: Run Prettier on the test block in
SDKRemoteFileSystem.test.ts and retain the formatter’s output, including
formatting the long createFileMock.mockResolvedValue error object and assertions
according to the project’s style.
In `@src/infra/drive-server/drive-server.error.ts`:
- Around line 26-45: The mapStatusToErrorCause function must distinguish all
HTTP 402 categories instead of treating every unknown or empty-file response as
FILE_TOO_BIG. Match the backend’s exact size, empty-file, and empty-file-limit
message categories, return distinct existing causes where available, and use a
safe non-size-specific fallback for unknown or missing-message 402 responses;
add regression tests covering each category and fallback.
---
Nitpick comments:
In
`@src/context/virtual-drive/files/application/create/CreateFileOnOfflineFileUploaded.test.ts`:
- Around line 102-120: Update the notifier.issues assertion in the
override-rejection test to also verify that the notification’s name equals the
basename of uploadedEvent.path, while preserving the existing error and cause
assertions.
🪄 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 Plus
Run ID: 28a3b42f-840f-4454-a622-ea24d677a93c
📒 Files selected for processing (12)
src/apps/drive/dependency-injection/virtual-drive/registerFilesServices.tssrc/context/storage/TemporalFiles/application/upload/TemporalFileUploader.test.tssrc/context/storage/TemporalFiles/application/upload/TemporalFileUploader.tssrc/context/virtual-drive/files/application/create/CreateFileOnOfflineFileUploaded.test.tssrc/context/virtual-drive/files/application/create/CreateFileOnTemporalFileUploaded.tssrc/context/virtual-drive/files/application/override/FileOverrider.tssrc/context/virtual-drive/files/infrastructure/SDKRemoteFileSystem.test.tssrc/context/virtual-drive/files/infrastructure/SDKRemoteFileSystem.tssrc/infra/drive-server/drive-server.client.tssrc/infra/drive-server/drive-server.error.test.tssrc/infra/drive-server/drive-server.error.tsvitest.setup.main.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/context/storage/TemporalFiles/application/upload/TemporalFileUploader.test.ts
- src/context/storage/TemporalFiles/application/upload/TemporalFileUploader.ts
| c.get(FileOverrider), | ||
| c.get(Environment), | ||
| user.bucket, | ||
| c.get(SyncFileMessenger), |
There was a problem hiding this comment.
Non-blocking: We could get rid of this class and just use functions as this class has no internal state therefore we could remove one more class that uses diod and this legacy way of importing classes. We could generate a task for it and add a todo inside MainProcessSyncFileMessenger with the ticket name attached to it so that it can be done in a separate class, what do you think?
There was a problem hiding this comment.
Once we finish this compatibility release, we can get back to addressing the project's technical debt and create tickets to minimize our dependency on diod.
There was a problem hiding this comment.
I know, i was just saying that you could generate a jira ticket, and add a comment that references said jira ticket so that it does not get forgotten
| if (message) { | ||
| const normalizedMessage = message.toLowerCase(); | ||
|
|
||
| if (normalizedMessage.includes('empty file') || normalizedMessage.includes('empty files')) { |
There was a problem hiding this comment.
on drive server wip
You can not have empty files, upgrade your plan to get more features -> is a 402
throw new PaymentRequiredException(
'You can not have empty files, upgrade your plan to get more features',
);You can not have more empty files -> is a bad request 400
if (limit.shouldLimitBeEnforced({ currentCount: emptyFilesCount })) {
throw new BadRequestException('You can not have more empty files');
}and it should be handled accordingly.
As well for the file size limit exceeded, check the error in drive-server-wip
throw new PaymentRequiredException(
'File size exceeds the maximum allowed by your plan',
PaymentRequiredErrorCode.FileUploadSizeExceeded,
);
| if (status === 402) return 'FILE_TOO_BIG'; | ||
| if (status === 400) { | ||
| if (isEmptyFileMessage(message)) return 'EMPTY_FILE'; | ||
| if (isFileSizeLimitMessage(message)) return 'FILE_TOO_BIG'; |
There was a problem hiding this comment.
Why a 400? Havent found a case on the drive-server-wip that maps a file size limit exceeded error to a 400
There was a problem hiding this comment.
You're right, I misinterpreted the comment you left me earlier, and when I checked the map, I got confused. I've left only the FILE_EMPTY error, which covers the error indicating that the limit for empty files has been reached.
| normalizedMessage.includes('too big') || | ||
| normalizedMessage.includes('too large') || | ||
| normalizedMessage.includes('file size exceeds') || | ||
| normalizedMessage.includes('size exceeds the maximum allowed') |
There was a problem hiding this comment.
We already know that the backend maps these errors: File size exceeds the maximum allowed by your plan for a file that exceeds the file size, so I would delete the 2 first expressions
Check this
if (Number(fileSize) > Number(limitValue)) {
throw new PaymentRequiredException(
'File size exceeds the maximum allowed by your plan',
PaymentRequiredErrorCode.FileUploadSizeExceeded,
);
}
|
|
||
| const normalizedMessage = message.toLowerCase(); | ||
|
|
||
| return normalizedMessage.includes('empty file') || normalizedMessage.includes('empty files'); |
There was a problem hiding this comment.
It would be nice to make a distintion here not only that the error is an empty file but also which type of empty file error we got since the backend already does that for us
402 -> 'You can not have empty files, upgrade your plan to get more features'
400 -> 'You can not have more empty files'
this way we can show different issues
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)
src/infra/drive-server/drive-server.error.test.ts (1)
4-5: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPreserve the distinction between empty-file rejection causes.
Generic empty-file rejection and exceeding the permitted number of empty files currently both map to
EMPTY_FILE. Introduce a distinct quota-related cause (or preserve a structured backend reason) so file creation and override callers can report the correct user-facing error.Also applies to: 24-27
🤖 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 `@src/infra/drive-server/drive-server.error.test.ts` around lines 4 - 5, Update mapStatusToErrorCause and its callers so generic empty-file rejection remains distinct from exceeding the permitted empty-file quota; introduce or preserve a structured quota-specific cause for the “You can not have more empty files” response, and ensure file creation and override flows use it for the correct user-facing error.
🧹 Nitpick comments (1)
src/context/storage/TemporalFiles/application/upload/TemporalFileUploader.test.ts (1)
105-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover failed space validation.
The test covers
hasSpace: false, but not{ error: ... }, which maps toBAD_RESPONSEinTemporalFileUploader. Add assertions for the error cause/message and that watch, stream, uploader, and event publication remain untouched.🤖 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 `@src/context/storage/TemporalFiles/application/upload/TemporalFileUploader.test.ts` around lines 105 - 119, Add a test case alongside the existing insufficient-space test for validateSpaceMock resolving with an error response, and assert that TemporalFileUploader.run rejects with cause BAD_RESPONSE and the corresponding error message. Also verify repository.watchFile, repository.stream, uploaderFactory.build, and eventBus.publish are not called.
🤖 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 `@packages/fuse-daemon/internal/filesystem/file_test.go`:
- Around line 52-59: Update the OperationRelease handler in TestRelease to
continue validating received.ProcessName as well as received.Path. Capture both
request fields or assert ProcessName directly in the handler, preserving
coverage of the processName property while retaining the existing path
verification.
---
Outside diff comments:
In `@src/infra/drive-server/drive-server.error.test.ts`:
- Around line 4-5: Update mapStatusToErrorCause and its callers so generic
empty-file rejection remains distinct from exceeding the permitted empty-file
quota; introduce or preserve a structured quota-specific cause for the “You can
not have more empty files” response, and ensure file creation and override flows
use it for the correct user-facing error.
---
Nitpick comments:
In
`@src/context/storage/TemporalFiles/application/upload/TemporalFileUploader.test.ts`:
- Around line 105-119: Add a test case alongside the existing insufficient-space
test for validateSpaceMock resolving with an error response, and assert that
TemporalFileUploader.run rejects with cause BAD_RESPONSE and the corresponding
error message. Also verify repository.watchFile, repository.stream,
uploaderFactory.build, and eventBus.publish are not called.
🪄 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 Plus
Run ID: af0171c6-6931-485b-af46-c016629fd513
📒 Files selected for processing (5)
packages/fuse-daemon/internal/filesystem/file_test.gosrc/context/storage/TemporalFiles/application/upload/TemporalFileUploader.test.tssrc/context/storage/TemporalFiles/application/upload/TemporalFileUploader.tssrc/context/virtual-drive/files/application/override/FileOverrider.test.tssrc/infra/drive-server/drive-server.error.test.ts
| client.OperationRelease: func(w http.ResponseWriter, r *http.Request) { | ||
| var received releaseRequest | ||
| body, _ := io.ReadAll(r.Body) | ||
| _ = json.Unmarshal(body, &received) | ||
| select { | ||
| case releasePaths <- received.Path: | ||
| default: | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep asserting ProcessName
The test now forwards only received.Path, so it no longer verifies the processName field promised by TestRelease and represented by releaseRequest. Capture both fields (or assert ProcessName directly in the handler) so regressions in that request property are still detected.
🤖 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 `@packages/fuse-daemon/internal/filesystem/file_test.go` around lines 52 - 59,
Update the OperationRelease handler in TestRelease to continue validating
received.ProcessName as well as received.Path. Capture both request fields or
assert ProcessName directly in the handler, preserving coverage of the
processName property while retaining the existing path verification.
…System and drive-server.error
…add environment variable support
|
2 participants
What is Changed / Added
TemporalFileUploader.0, upload is skipped.ContentsId('' as ContentsId).isZeroSizeFile(...)for explicit guard logic.Why
fileSize: 0withInvalidUploadSizeError.contentsId.Summary by CodeRabbit
New Features
Bug Fixes