fix: append file extension to WhatsApp document filename fallback#48745
fix: append file extension to WhatsApp document filename fallback#48745Nic-vdwalt wants to merge 1 commit into
Conversation
When sending documents without an explicit filename, the recipient received files named "file" with no extension. Use extensionForMime() to derive the correct extension from the MIME type in both the auto-reply and outbound send paths. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Greptile SummaryThis PR fixes a UX bug where WhatsApp document messages sent without an explicit filename were received as Key changes:
Minor concern: The two code paths now diverge slightly — Confidence Score: 4/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/web/inbound/send-api.ts
Line: 42
Comment:
**Inconsistency: explicit filenames without extensions not handled**
`deliver-reply.ts` applies the extension-appending logic to all filenames — including explicitly provided ones that lack an extension — via its `path.extname(rawName)` check. But here, if a caller passes `sendOptions.fileName = "invoice"` (no extension), it will be used as-is without appending the MIME-derived extension.
Whether this is intentional depends on the contract: callers of this API are presumably in control of the filename, but the two codepaths now behave differently for the same scenario. Consider aligning them:
```suggestion
const rawName = sendOptions?.fileName?.trim() || "file";
const fileName = path.extname(rawName) ? rawName : `${rawName}${extensionForMime(mediaType) ?? ""}`;
```
(This would also require importing `path` from `"node:path"` in this file.)
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 591270b |
| }; | ||
| } else { | ||
| const fileName = sendOptions?.fileName?.trim() || "file"; | ||
| const fileName = sendOptions?.fileName?.trim() || `file${extensionForMime(mediaType) ?? ""}`; |
There was a problem hiding this comment.
deliver-reply.ts applies the extension-appending logic to all filenames — including explicitly provided ones that lack an extension — via its path.extname(rawName) check. But here, if a caller passes sendOptions.fileName = "invoice" (no extension), it will be used as-is without appending the MIME-derived extension.
Whether this is intentional depends on the contract: callers of this API are presumably in control of the filename, but the two codepaths now behave differently for the same scenario. Consider aligning them:
| const fileName = sendOptions?.fileName?.trim() || `file${extensionForMime(mediaType) ?? ""}`; | |
| const rawName = sendOptions?.fileName?.trim() || "file"; | |
| const fileName = path.extname(rawName) ? rawName : `${rawName}${extensionForMime(mediaType) ?? ""}`; |
(This would also require importing path from "node:path" in this file.)
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/web/inbound/send-api.ts
Line: 42
Comment:
**Inconsistency: explicit filenames without extensions not handled**
`deliver-reply.ts` applies the extension-appending logic to all filenames — including explicitly provided ones that lack an extension — via its `path.extname(rawName)` check. But here, if a caller passes `sendOptions.fileName = "invoice"` (no extension), it will be used as-is without appending the MIME-derived extension.
Whether this is intentional depends on the contract: callers of this API are presumably in control of the filename, but the two codepaths now behave differently for the same scenario. Consider aligning them:
```suggestion
const rawName = sendOptions?.fileName?.trim() || "file";
const fileName = path.extname(rawName) ? rawName : `${rawName}${extensionForMime(mediaType) ?? ""}`;
```
(This would also require importing `path` from `"node:path"` in this file.)
How can I resolve this? If you propose a fix, please make it concise.|
Codex review: needs real behavior proof before merge. Summary Reproducibility: yes. by source inspection: current main still emits Real behavior proof Next step before merge Security Review findings
Review detailsBest possible solution: Retarget the MIME-derived fallback to the active WhatsApp plugin send and outbound media normalization paths, update focused plugin tests, and collect redacted live WhatsApp delivery proof. Do we have a high-confidence way to reproduce the issue? Yes, by source inspection: current main still emits Is this the best way to solve the issue? No. The intended fix is narrow and reasonable, but this branch changes removed legacy Full review comments:
Overall correctness: patch is incorrect Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 54c9820ed9c2. |
|
Thanks for the PR and for working on this. We checked the current main branch, and this fix is already in main via #82851, which landed in commit 5040eb5. WhatsApp document sends without explicit filenames now use MIME-aware fallback names such as Because that has landed, I’m closing this PR as superseded by #82851. Thanks again for the work here. If you think this closure is mistaken and your PR still fixes something meaningfully different on current main, feel free to open a new PR with that explanation. |
Summary
extensionForMime()to derive the correct extension from the MIME type in both the auto-reply (deliver-reply.ts) and outbound send (send-api.ts) pathsdeliver-reply.ts, also check if the raw filename already has an extension before appending oneTest plan
"file.pdf"text/csvtest added expecting"file.csv"to verify dynamic extension derivationsend-api.test.tspass (npx vitest run src/web/inbound/send-api.test.ts)"file"fallbacks — none found🤖 Generated with Claude Code