Fix: Images Saved Before Upload Completes Are Silently Lost#511
Fix: Images Saved Before Upload Completes Are Silently Lost#511vanshika6900 wants to merge 1 commit into
Conversation
Confidence Score: 4/5The change is safe to merge; both the frontend guard and the backend cleanup work correctly for the described failure scenario. The two-layer fix is well-reasoned and the regex logic handles the documented edge cases correctly. The only issue is that two new user-visible strings bypass Frappe's translation wrapper, which is a minor convention miss with no functional impact. CommentsArea.vue and useNewDiscussion.ts both have untranslated user-facing strings.
|
| Filename | Overview |
|---|---|
| frontend/src/utils/index.ts | Adds hasPendingImageUpload() helper using a regex that correctly matches blob:/data: src attributes; logic looks sound. |
| frontend/src/components/CommentsArea.vue | Disables Send button and adds toast guard for keyboard shortcut; toast string is not wrapped in __() as required by Frappe i18n convention. |
| frontend/src/pages/NewDiscussion/useNewDiscussion.ts | Adds upload-pending guard to validateForm(); error message string is not wrapped in __() per Frappe i18n convention. |
| gameplan/utils/sanitizer.py | Adds remove_srcless_images() post-bleach cleanup using a regex; correctly handles data-src lookalike. |
| gameplan/tests/test_sanitizer.py | Five focused regression tests covering private file, https, blob, data-uri, and data-src lookalike; all cover the described edge cases well. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User submits comment/discussion] --> B{hasPendingImageUpload?}
B -- Yes --> C[Block submission\n+ show error message]
B -- No --> D[Submit content to backend]
D --> E[sanitize_content via bleach]
E --> F{img has blob:/data: src?}
F -- Yes --> G[bleach strips src attribute]
G --> H[remove_srcless_images\ndrops the img tag entirely]
F -- No --> I[img preserved with real src]
H --> J[Clean HTML persisted]
I --> J
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[User submits comment/discussion] --> B{hasPendingImageUpload?}
B -- Yes --> C[Block submission\n+ show error message]
B -- No --> D[Submit content to backend]
D --> E[sanitize_content via bleach]
E --> F{img has blob:/data: src?}
F -- Yes --> G[bleach strips src attribute]
G --> H[remove_srcless_images\ndrops the img tag entirely]
F -- No --> I[img preserved with real src]
H --> J[Clean HTML persisted]
I --> J
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
frontend/src/components/CommentsArea.vue:641
User-visible string not wrapped in `__(...)` — this text won't be translated for non-English locales, violating Frappe's i18n convention.
```suggestion
toast.error(__('Please wait for the image to finish uploading'))
```
### Issue 2 of 2
frontend/src/pages/NewDiscussion/useNewDiscussion.ts:127
User-visible error message not wrapped in `__(...)` — same Frappe i18n requirement applies here as in `CommentsArea.vue`.
```suggestion
errorMessage.value = __('Please wait for the image to finish uploading.')
```
Reviews (1): Last reviewed commit: "fix: don't lose images saved before uplo..." | Re-trigger Greptile
| if (commentEmpty.value || comments.insert.loading) return | ||
| // Safety net for keyboard submit (Ctrl/Cmd+Enter) which bypasses the disabled button. | ||
| if (commentHasPendingUpload.value) { | ||
| toast.error('Please wait for the image to finish uploading') |
There was a problem hiding this comment.
User-visible string not wrapped in
__(...) — this text won't be translated for non-English locales, violating Frappe's i18n convention.
| toast.error('Please wait for the image to finish uploading') | |
| toast.error(__('Please wait for the image to finish uploading')) |
Context Used: This is a Frappe Framework application (Python bac... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/components/CommentsArea.vue
Line: 641
Comment:
User-visible string not wrapped in `__(...)` — this text won't be translated for non-English locales, violating Frappe's i18n convention.
```suggestion
toast.error(__('Please wait for the image to finish uploading'))
```
**Context Used:** This is a Frappe Framework application (Python bac... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| return false | ||
| } | ||
| if (hasPendingImageUpload(draftData.value.content)) { | ||
| errorMessage.value = 'Please wait for the image to finish uploading.' |
There was a problem hiding this comment.
User-visible error message not wrapped in
__(...) — same Frappe i18n requirement applies here as in CommentsArea.vue.
| errorMessage.value = 'Please wait for the image to finish uploading.' | |
| errorMessage.value = __('Please wait for the image to finish uploading.') |
Context Used: This is a Frappe Framework application (Python bac... (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: frontend/src/pages/NewDiscussion/useNewDiscussion.ts
Line: 127
Comment:
User-visible error message not wrapped in `__(...)` — same Frappe i18n requirement applies here as in `CommentsArea.vue`.
```suggestion
errorMessage.value = __('Please wait for the image to finish uploading.')
```
**Context Used:** This is a Frappe Framework application (Python bac... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0))
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
@netchampfaris — I'm applying to Frappe and picked Gameplan to get hands-on. While using it locally I hit a couple of real bugs and fixed them. |
Problem
Adding a photo to a comment (or discussion) sometimes results in an invisible image — the post saves with no error, but nothing renders. The stored content ends up as:
![]()
Note the < img > has dimensions but no src — so it renders as nothing.Root cause
The editor inserts an image with a temporary blob:/data: preview URL while the upload is in flight, then swaps in the real /private/files/... URL once it completes.
If the content is saved before that swap finishes (or the upload fails), the preview src is still a blob:/data: URL. The backend sanitizer (gameplan/utils/sanitizer.py) only allows the cid/http/https/mailto protocols, so it strips the preview src — leaving a broken, srcless
that is persisted and never displays.
Confirmed via the sanitizer directly:
← kept
← src stripped
← src stripped
private file => <img src="/private/files/x.png" ...> ← kept
https =>
blob preview =>
data uri =>
The blob: result is byte-for-byte what gets stored in the broken comment.
Fix (two layers)
Frontend — prevent saving an unresolved upload (the primary fix)
New shared helper hasPendingImageUpload(html) detects content that still holds a blob:/data: image src.
Comment composer (CommentsArea.vue): disables Send while an upload is pending, with a toast safety‑net for keyboard submit (Ctrl/Cmd+Enter).
Discussion composer (useNewDiscussion.ts): blocks Publish with a clear message.
Backend — never persist a broken placeholder (defense in depth)
sanitize_content now drops
tags left without a usable src. This covers both comments and discussions (both call sanitize_content), so even a client that bypasses the guard can't store an invisible image.
Testing
gameplan/tests/test_sanitizer.py — 5 new tests (keeps /private/files + https; drops blob: + data:; not fooled by a data-src look‑alike). All pass.
Manually verified: a fully‑uploaded image still posts and renders correctly; submitting mid‑upload is now blocked instead of silently losing the image.
yarn build passes.
Notes
The backend change is intentionally minimal and narrowly scoped (only removes media that has no src after sanitization).