Skip to content

Fix: Images Saved Before Upload Completes Are Silently Lost#511

Open
vanshika6900 wants to merge 1 commit into
frappe:developfrom
vanshika6900:fix/editor-image-upload-race
Open

Fix: Images Saved Before Upload Completes Are Silently Lost#511
vanshika6900 wants to merge 1 commit into
frappe:developfrom
vanshika6900:fix/editor-image-upload-race

Conversation

@vanshika6900

@vanshika6900 vanshika6900 commented Jun 30, 2026

Copy link
Copy Markdown

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:
private file => <img src="/private/files/x.png" ...> ← kept
https => ← kept
blob preview => ← src stripped
data uri => ← src stripped
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).

@greptile-apps

greptile-apps Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Confidence Score: 4/5

The 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.

Important Files Changed

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
Loading
%%{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
Loading
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')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 User-visible string not wrapped in __(...) — this text won't be translated for non-English locales, violating Frappe's i18n convention.

Suggested change
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.'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 User-visible error message not wrapped in __(...) — same Frappe i18n requirement applies here as in CommentsArea.vue.

Suggested change
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!

@vanshika6900

Copy link
Copy Markdown
Author

@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.
This PR: an image added before its upload finished was saved with its blob: src stripped by the sanitizer, leaving an invisible . Fixed on both layers (frontend upload-guard + backend sanitizer), with tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant