[NO QA] feat: introduce ExportDownloadStatusModal#91490
Conversation
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const {translate} = useLocalize(); | ||
| // eslint-disable-next-line rulesdir/prefer-shouldUseNarrowLayout-instead-of-isSmallScreenWidth | ||
| const {isSmallScreenWidth} = useResponsiveLayout(); | ||
| const {accountID: currentUserAccountID} = useCurrentUserPersonalDetails(); |
There was a problem hiding this comment.
❌ CONSISTENCY-5 (docs)
The eslint-disable comment on line 39 suppresses rulesdir/prefer-shouldUseNarrowLayout-instead-of-isSmallScreenWidth but provides no justification explaining why the rule is intentionally bypassed. Without a comment, future maintainers cannot determine whether this suppression is permanent and intentional or a temporary workaround that should be revisited.
Add a justification comment explaining why isSmallScreenWidth is used instead of shouldUseNarrowLayout, or if this is a temporary suppression, consider using SEATBELT_INCREASE instead. For example:
// eslint-disable-next-line rulesdir/prefer-shouldUseNarrowLayout-instead-of-isSmallScreenWidth
// isSmallScreenWidth is needed here because the modal type depends on actual screen width, not layout mode
const {isSmallScreenWidth} = useResponsiveLayout();Reviewed at: 2a94a51 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a94a5109a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const lastKnownExportRef = useRef<ExportDownload | null>(null); | ||
| if (exportDownload) { | ||
| lastKnownExportRef.current = exportDownload; | ||
| } | ||
|
|
||
| const displayedExport = exportDownload ?? lastKnownExportRef.current; |
There was a problem hiding this comment.
lastKnownExportRef persists across prop changes and is never cleared when exportID changes, so reusing this modal instance for a different export can render/actions against stale data from the previous export. In that case the new modal can briefly show the old state (and even trigger the old downloadURL via the ready-state effect) before the new Onyx entry arrives, which can download the wrong file and send incorrect payloads to clearExportDownload/sendExportFileFromConcierge.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
lastKnownExportRef has been removed in the latest code change
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@truph01 please fix the faling checks and the codex comment above |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f851b865e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!isReady || !downloadURL) { | ||
| return; |
There was a problem hiding this comment.
Guard the auto-download effect against the Concierge flow: after users press “Send me the file when it’s ready”, shouldSendFromConcierge remains true and the UI switches to the Concierge state, but this effect still downloads as soon as state becomes ready. That contradicts the selected delivery mode and can trigger an unexpected local download while the user is waiting for a Concierge message.
Useful? React with 👍 / 👎.
| return; | ||
| } | ||
| fileDownload(translate, downloadURL); | ||
| }, [isReady, downloadURL, translate]); |
There was a problem hiding this comment.
Avoid depending on translate in this effect because it is provided by context and can change identity across re-renders; with isReady true, any subsequent render (e.g., from Onyx updates while the modal is open) will re-run the effect and invoke fileDownload again. This can produce repeated download attempts for the same export instead of a single automatic download.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Removed translate from deps
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84a9543f6b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| fileDownload(translate, downloadURL); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [isReady]); |
There was a problem hiding this comment.
The auto-download effect reads downloadURL and shouldSendFromConcierge but only re-runs when isReady changes. If the export transitions to ready before downloadURL is merged into Onyx (or shouldSendFromConcierge flips after ready), this effect exits early once and never retries, so the automatic download is skipped even though the file later becomes available. Add the missing dependencies (or key the effect off the full readiness condition) so late-arriving Onyx fields still trigger the download.
Useful? React with 👍 / 👎.
|
@heyjennahay could you please help with the the auto translations here? |
| /> | ||
| <Button | ||
| text={translate('exportDownload.dismiss')} | ||
| onPress={handleDismiss} |
There was a problem hiding this comment.
I think we only need to call the onClose here.
| @@ -0,0 +1,186 @@ | |||
| import {hasSeenTourSelector} from '@selectors/Onboarding'; | |||
| import React, {useCallback, useEffect} from 'react'; | |||
There was a problem hiding this comment.
Please don't use the useCallBack in any new component because the component must compile with React Compiler, quote from here
There was a problem hiding this comment.
I removed useCallback
|
@truph01 please check the bot and my comment above |
|
@truph01 are you able to address those today? |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
🚧 @mollfpr has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
5 participants
Explanation of Change
Fixed Issues
$ #90296
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include ""[No QA].""
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari