[Due for payment 2026-06-17] [$250] Attachment - Download button quickly showing when opening broken image

[jponikarchuk]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Version Number: v9.3.70-1
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught during regression testing, add the test name, ID and link from BrowserStack: https://test-management.browserstack.com/projects/2219752/test-runs/TR-28059/folder/13176710/41236964/1407055530
Email or phone of affected tester (no customers): N/A
Issue reported by: Applause Internal Team
Bug source: Regression TC Execution
Device used: Windows 11 / Chrome
App Component: Other

Action Performed:

1. Paste " " in the any chat and send
2. Wait until it loads
3. Open the attachment

Expected Result:

When the attachment is broken there should be no download button even the loading spinner is running

Actual Result:

Download button is visible and clickable when broken attachment is being opened

Workaround:

Unknown

Platforms:

• Android: App
• Android: mWeb Chrome
• iOS: App
• iOS: mWeb Safari
• iOS: mWeb Chrome
• Windows: Chrome
• MacOS: Chrome / Safari

Screenshots/Videos

Video.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~022054141833029652388
• Upwork Job ID: 2054141833029652388
• Last Price Increase: 2026-05-12

Issue Owner

Current Issue Owner: @mallenexpensify

[MelvinBot]

Proposal

What is the root cause of that problem?

The download button visibility is governed by shouldShowDownloadButton in AttachmentModalBaseContent, which relies on two guards: isErrorInAttachment(source) and isAttachmentLoaded(source). Both guards fail for broken images due to a default-true bug in isAttachmentLoaded:

const isAttachmentLoaded = useCallback(
  (key: AttachmentSource) => attachmentLoaded?.[convertSourceToString(key)] !== false,
  [attachmentLoaded],
);

When a key is not in the map, undefined !== false evaluates to true — so any image that was never tracked (including broken images) is treated as "loaded." This also short-circuits BaseImage's mount effect that should initialize the state to false, because the guard returns early before setAttachmentLoaded(source, false) is ever called. Since setAttachmentLoaded(source, true) is only called on successful load, broken images remain in the default-true state permanently.

What changes do you think we should make in order to solve the problem?

Fix isAttachmentLoaded to return false for unknown keys instead of true:

Change attachmentLoaded?.[convertSourceToString(key)] !== false → attachmentLoaded?.[convertSourceToString(key)] === true

This ensures that images not yet in the map (or images that errored and were never marked loaded) correctly resolve as "not loaded," hiding the download button until a successful load event fires.

This also un-breaks the BaseImage mount effect — since unknown keys now return false, the guard if (isAttachmentLoaded?.(source)) won't short-circuit, and the explicit setAttachmentLoaded(source, false) initialization will execute as intended.

What alternative solutions did you explore? (Optional)

1. Remove the guard in BaseImage's mount effect — Would fix the initialization but still leaves the semantic bug where unknown keys default to "loaded."
2. Fix the error-reporting path (onAttachmentError in AttachmentView) to work without fallbackSource — This would fix the isErrorInAttachment guard for non-carousel paths, but the isAttachmentLoaded default-true bug would remain and could cause similar issues elsewhere.

The proposed fix is the simplest single-line change that addresses both the download button bug and the mount-effect short-circuit.

---

Next Steps for Contributor+ team: Reply with @MelvinBot implement this to create a draft PR, @MelvinBot <your feedback> to refine this analysis, or explain why you are rejecting Melvin's proposal.

[yusufdeveloper2903]

🚨 Edited by proposal-police: This proposal was edited at 2026-05-12 10:12:43 UTC.

Proposal

What is the root cause of that problem?

When a broken image is opened in AttachmentModalScreen, the download button briefly renders because shouldShowDownloadButton evaluates to true on the very first render — before the image even gets a chance to fail. The gating selector reads isAttachmentLoaded?.(source), and the provider's default for an unknown source returns true.

AttachmentStateContextProvider.tsx defines:

App/src/pages/media/AttachmentModalScreen/AttachmentModalBaseContent/AttachmentStateContextProvider.tsx

Line 50 in 3a13517

const isAttachmentLoaded = useCallback((key: AttachmentSource) => attachmentLoaded?.[convertSourceToString(key)] !== false, [attachmentLoaded]);

const isAttachmentLoaded = useCallback(
    (key) => attachmentLoaded?.[convertSourceToString(key)] !== false,
    [attachmentLoaded],
);

For a freshly opened attachment, attachmentLoaded[url] is undefined, and undefined !== false is true. So the modal treats every brand-new source as already loaded and renders the download button immediately:

App/src/pages/media/AttachmentModalScreen/AttachmentModalBaseContent/index.tsx

Lines 191 to 211 in 3a13517

	const shouldShowDownloadButton = useMemo(() => {
	const isValidContext = !isEmptyObject(report) || type === CONST.ATTACHMENT_TYPE.SEARCH || shouldAllowDownloadOutsideReportContext;
	if (!isValidContext || isErrorInAttachment(source) || isEReceipt) {
	return false;
	}
	return !!onDownloadAttachment && isDownloadButtonReadyToBeShown && !shouldShowNotFoundPage && !isOffline && !isLocalSource && isAttachmentLoaded?.(source);
	}, [
	isAttachmentLoaded,
	isDownloadButtonReadyToBeShown,
	isErrorInAttachment,
	isLocalSource,
	isOffline,
	onDownloadAttachment,
	report,
	shouldAllowDownloadOutsideReportContext,
	shouldShowNotFoundPage,
	source,
	type,
	isEReceipt,
	]);

Historically this was masked because BaseImage unconditionally called setAttachmentLoaded(source, false) from a mount effect, flipping the state to false before paint. That guard was lost in PR #76682 (commit 7fab67fe67b), which wrapped the effect in an early-return:

App/src/components/Image/BaseImage.tsx

Lines 15 to 22 in 3a13517

	useEffect(() => {
	if (isAttachmentLoaded?.(source as AttachmentSource)) {
	return;
	}
	setAttachmentLoaded?.(source as AttachmentSource, false);
	// eslint-disable-next-line react-hooks/exhaustive-deps
	}, []);

useEffect(() => {
    if (isAttachmentLoaded?.(source as AttachmentSource)) {
        return;
    }
    setAttachmentLoaded?.(source as AttachmentSource, false);
}, []);

On first mount isAttachmentLoaded?.(source) returns true (default), the effect early-returns, setAttachmentLoaded(false) is never called, and the download button stays visible for the entire loading window. For a working image the success onLoad writes true and nobody notices; for a broken image the bot only hides it later via setAttachmentError, hence the "flashes then disappears" symptom described in the bug.

What changes do you think we should make in order to solve the problem?

Flip the default of isAttachmentLoaded so an unknown source is treated as not loaded (the natural semantic for a download-gating flag). This is a 1-character semantic fix that restores the pre-#76682 behavior without re-introducing the bug that #76682 was actually trying to address (re-mounting an already-loaded large image and incorrectly resetting it back to false).

- const isAttachmentLoaded = useCallback((key: AttachmentSource) => attachmentLoaded?.[convertSourceToString(key)] !== false, [attachmentLoaded]);
+ const isAttachmentLoaded = useCallback((key: AttachmentSource) => attachmentLoaded?.[convertSourceToString(key)] === true, [attachmentLoaded]);

After the change:

• Unknown source (first paint) → undefined === true → false → shouldShowDownloadButton returns false. No flash.
• Image loaded successfully → BaseImage.imageLoadedSuccessfully writes true → button appears.
• Broken image → state stays undefined/false, and the existing onError → setAttachmentError path independently keeps it hidden. Both gates now agree.
• Re-mount of an already-loaded source (the fix download icon hide large imgage #76682 case) → state is still true, the BaseImage early-return still fires, no spurious reset. The optimization PR fix download icon hide large imgage #76682 added is preserved.

The default value of the context (isAttachmentLoaded: () => false at line 26) already returns false for unknown sources, so the selector now matches the documented intent of the context.

No other call sites need to change — there are only three readers of isAttachmentLoaded (the two BaseImage mount effects and the modal's shouldShowDownloadButton), and all three behave correctly under the new semantic.

What alternative solutions did you explore? (Optional)

1. Reverting PR fix download icon hide large imgage #76682's early-return in BaseImage and unconditionally calling setAttachmentLoaded(source, false) on every mount. This works but re-introduces the regression fix download icon hide large imgage #76682 fixed (large-image re-open flashes the spinner and hides the icon). The selector-level fix avoids that.
2. Forcing shouldShowDownloadButton to additionally check !isLoading / a local "has-fired-onLoad" ref at the modal level. Bigger surface area, duplicates state already owned by AttachmentStateContext, and doesn't address the wrong-by-default semantic at the source.

[dilshodmackbook-sketch]

Proposal

What is the root cause of that problem?

When the attachment modal opens an image, shouldShowDownloadButton in AttachmentModalBaseContent gates the header download button on isAttachmentLoaded(source). The intent is "hide the download button until the image actually loads," but that signal is wrong while the image is still loading.

The reason is the initialization effect in BaseImage.tsx (and the matching native file BaseImage.native.tsx):

useEffect(() => {
    if (isAttachmentLoaded?.(source as AttachmentSource)) {
        return;
    }
    setAttachmentLoaded?.(source as AttachmentSource, false);
}, []);

It guards the "mark as loading" call with isAttachmentLoaded(source). But the default in AttachmentStateContextProvider.tsx#L50 is:

const isAttachmentLoaded = useCallback(
    (key: AttachmentSource) => attachmentLoaded?.[convertSourceToString(key)] !== false,
    [attachmentLoaded],
);

For any source that hasn't been tracked yet, attachmentLoaded[source] is undefined, and undefined !== false returns true. So the guard short-circuits and setAttachmentLoaded(source, false) is never executed for a fresh image. That leaves isAttachmentLoaded(source) returning true (the default) the entire time the image is loading, so the header download button is visible while the spinner runs. For a broken URL, expo-image's onLoad never fires either, so the value never gets updated and the button remains visible/clickable until the image error eventually propagates through setAttachmentError (which itself only flips when a fallbackSource is configured — see AttachmentView/index.tsx#L181-L185).

We can't just flip the default in AttachmentStateContextProvider to "not loaded", because that signal is also relied on for non-image attachments (PDF, video, etc.) which never call setAttachmentLoaded at all — flipping the default would hide the download button for those forever.

What changes do you think we should make in order to solve the problem?

The fix is to drop the broken guard in BaseImage so images always mount in the "loading" state and only flip to "loaded" when expo-image's onLoad actually fires. BaseImage.tsx:

 useEffect(() => {
-    if (isAttachmentLoaded?.(source as AttachmentSource)) {
-        return;
-    }
-
     setAttachmentLoaded?.(source as AttachmentSource, false);
     // eslint-disable-next-line react-hooks/exhaustive-deps
 }, []);

Apply the same change to BaseImage.native.tsx. We can also drop the now-unused isAttachmentLoaded from useContext(AttachmentStateContext) in both files.

After the fix:

• Fresh image mounts → attachmentLoaded[source] = false → download button hidden while the spinner runs.
• expo-image onLoad fires on success → setAttachmentLoaded(source, true) → button appears.
• Broken URL → onLoad never fires → button stays hidden; once imageError propagates via onAttachmentError, the error branch in shouldShowDownloadButton keeps it hidden as well.
• PDFs and other non-image attachments are unaffected because they don't render BaseImage and the default in AttachmentStateContextProvider is unchanged.

What alternative solutions did you explore? (Optional)

1. Flip the default in AttachmentStateContextProvider.isAttachmentLoaded to === true. Cleaner semantically (untracked = not loaded), but it would regress non-image attachments (PDF, video, etc.) that never call setAttachmentLoaded — their download button would never appear.
2. Add an explicit isImage carve-out in the shouldShowDownloadButton memo (e.g., only require isAttachmentLoaded for image sources). Works, but spreads attachment-type logic into the modal and leaves the misleading BaseImage guard in place to bite the next person.
3. Have AttachmentView's image onError also call setAttachmentLoaded(source, false). Treats the symptom for broken images only — the button would still flash during normal slow loads.

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @suneox (External)

[Abdulloh0109]

🚫 Duplicated proposal withdrawn by 🤖 ProposalPolice.

[MobileMage]

Proposal

Please re-state the problem that we are trying to solve in this issue.

When opening a broken image attachment in the attachment modal, the download button briefly appears in the header and is clickable before the image load failure is detected. It should remain hidden until the image has successfully loaded.

What is the root cause of that problem?

The shouldShowDownloadButton computation in AttachmentModalBaseContent depends on isAttachmentLoaded(source), which comes from AttachmentStateContextProvider. The isAttachmentLoaded function returns true for any source that has not yet been recorded in its state map — specifically, its implementation is attachmentLoaded?.[key] !== false, so an absent key is treated as "loaded" rather than "unknown/pending".

App/src/pages/media/AttachmentModalScreen/AttachmentModalBaseContent/AttachmentStateContextProvider.tsx

Lines 50 to 52 in 3a13517

	const isAttachmentLoaded = useCallback((key: AttachmentSource) => attachmentLoaded?.[convertSourceToString(key)] !== false, [attachmentLoaded]);
	const value = useMemo(() => ({setAttachmentLoaded, clearAttachmentLoaded, isAttachmentLoaded}), [setAttachmentLoaded, clearAttachmentLoaded, isAttachmentLoaded]);
	return <AttachmentStateContext.Provider value={value}>{children}</AttachmentStateContext.Provider>;

BaseImage marks the source as false (not yet loaded) inside a useEffect on mount. Because useEffect runs after the first paint, there is one render cycle where the source is absent from the map and isAttachmentLoaded returns true, which causes shouldShowDownloadButton to evaluate to true and the download button to flash in the header. For broken images, setAttachmentLoaded(source, true) is never called (only onError fires), so the button disappears once the useEffect runs on the next cycle — but the flash has already happened and, crucially, the button remains permanently shown when there is no onLoad prop wired up to imageLoadedSuccessfully.

What changes do you think we should make in order to solve the problem?

The fix should change the default return value of isAttachmentLoaded so that an unknown (never-registered) source is treated as not loaded rather than loaded. In AttachmentStateContextProvider, change line 50 from attachmentLoaded?.[convertSourceToString(key)] !== false to attachmentLoaded?.[convertSourceToString(key)] === true. This makes the function return false for both an absent key and a key explicitly set to false, and only true once the image fires its onLoad and the entry is explicitly set to true. This single-character semantic change eliminates the initial flash because the download button will now be hidden until BaseImage confirms the image loaded successfully, and will remain hidden for broken images since they never fire onLoad.

App/src/pages/media/AttachmentModalScreen/AttachmentModalBaseContent/AttachmentStateContextProvider.tsx

Lines 50 to 52 in 3a13517

	const isAttachmentLoaded = useCallback((key: AttachmentSource) => attachmentLoaded?.[convertSourceToString(key)] !== false, [attachmentLoaded]);
	const value = useMemo(() => ({setAttachmentLoaded, clearAttachmentLoaded, isAttachmentLoaded}), [setAttachmentLoaded, clearAttachmentLoaded, isAttachmentLoaded]);
	return <AttachmentStateContext.Provider value={value}>{children}</AttachmentStateContext.Provider>;

The default context value on line 24 (the no-op isAttachmentLoaded: () => false) already returns false for the out-of-context case and does not need to change. The useEffect in BaseImage that pre-marks the source as false on mount also becomes redundant with this fix, but can be left in place without harm.

What alternative solutions did you explore? (Optional)

N/A

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~022054141833029652388

[github-actions]

⚠️ @Abdulloh0109 Your proposal is a duplicate of an already existing proposal and has been automatically withdrawn to prevent spam. Please review the existing proposals before submitting a new one.