fix(services): silence debug console.log on cancelled file upload#8950
fix(services): silence debug console.log on cancelled file upload#8950selenaalpha77-sketch wants to merge 2 commits intomakeplane:previewfrom
Conversation
When uploadFile() is intentionally cancelled via cancelUpload(), the catch handler logged the cancellation message with console.log. In production this surfaces a spurious log line for every user-initiated upload cancellation. Replace the console.log with an early return so intentional cancellations are handled silently, consistent with the behaviour described by the JSDoc comment that says the promise resolves to void.
|
evan seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
📝 WalkthroughWalkthroughUpdated error handling in the file upload service to silently ignore intentional Axios request cancellations while preserving the original error-throwing behavior for other errors. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/services/src/file/file-upload.service.ts (1)
41-47: LGTM — silent cancel is consistent with prior resolution semantics.The previous
console.log(error.message)also implicitly returnedundefined, so the catch already resolved the promise toundefinedon cancel. The explicitreturnpreserves that behavior while removing the noise, which matches thePromise<void>JSDoc. Callers that chain afteruploadFile(e.g.updateWorkspaceAssetUploadStatusinapps/web/core/services/file.service.ts) will continue to run on cancel exactly as they did before — no regression introduced here.Optional nit: since the
ifbranch returns, theelseis redundant and can be flattened for slightly cleaner control flow.♻️ Optional flatten
.catch((error) => { if (axios.isCancel(error)) { // Upload was intentionally cancelled; swallow the cancellation silently. return; - } else { - throw error?.response?.data; } + throw error?.response?.data; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/services/src/file/file-upload.service.ts` around lines 41 - 47, The catch block in uploadFile currently uses an if/else where the if branch returns on axios.isCancel(error), making the else redundant; simplify the control flow by removing the else and unwrapping the throw so that after the early return you directly throw error?.response?.data; update the catch in file-upload.service.ts (the axios.isCancel check inside the uploadFile error handler) to reflect this flattened structure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/services/src/file/file-upload.service.ts`:
- Around line 41-47: The catch block in uploadFile currently uses an if/else
where the if branch returns on axios.isCancel(error), making the else redundant;
simplify the control flow by removing the else and unwrapping the throw so that
after the early return you directly throw error?.response?.data; update the
catch in file-upload.service.ts (the axios.isCancel check inside the uploadFile
error handler) to reflect this flattened structure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8856b238-4b18-46cc-b1ba-6c3fbac219e8
📒 Files selected for processing (1)
packages/services/src/file/file-upload.service.ts
What
When
uploadFile()is intentionally cancelled viacancelUpload(), thecatchhandler logged the cancellation message withconsole.log(error.message). In production this surfaces a spurious log line for every user-initiated upload cancellation.Why
Intentional cancellations are not errors and should not produce console output. The method's own JSDoc states the promise resolves to
void; silently returning on cancellation is the correct behaviour.Change
Replace
console.log(error.message)with an earlyreturnin theaxios.isCancelbranch ofuploadFile. Non-cancellation errors continue to propagate by re-throwing.Testing
Cancel an in-progress file upload and confirm no log output appears in the browser console.
Summary by CodeRabbit