[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 👍 / 👎.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@truph01 please fix the faling checks and the codex comment above |
2 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