-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[No QA] Restrict usage of computeReportName #81504
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[No QA] Restrict usage of computeReportName #81504
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@hungvu193 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
| /** | ||
| * Get the title for a report. | ||
| * @deprecated Moved to src/libs/ReportNameUtils.ts. | ||
| * Use ReportNameUtils.computeReportName for full name generation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've deleted this part of comment as it suggest usaging of computeReportName
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6346736bf8
ℹ️ 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".
eslint.changed.config.mjs
Outdated
| selector: 'CallExpression[callee.name="computeReportName"]', | ||
| message: 'Do not call computeReportName directly. Use ReportNameUtils.getReportName instead, which uses derived values.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scope computeReportName restriction to avoid blocking lint-changed
This new no-restricted-syntax selector applies to every changed TypeScript file, but scripts/lintChanged.sh runs ESLint on whole changed files (eslint ... --config ./eslint.changed.config.mjs $GIT_DIFF_OUTPUT), and src/libs/ReportUtils.ts still contains a direct computeReportName(report) call at line 5167 while being modified in this commit. That means the branch cannot pass npm run lint-changed as written, so the rule needs an allowlist/exception (or the remaining call sites must be migrated in the same change) to keep CI green.
Useful? React with 👍 / 👎.
neil-marcellini
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good approach. Using eslint.changed.config.mjs to prevent new imports of computeReportName while not breaking existing code is the right pattern for a gradual migration.
A few minor notes:
- The PR title should include
[No QA]since this is a config-only change with no runtime behavior difference (the description says "No QA" but the title doesn't include the prefix). - The
no-restricted-importsrule only catches@libs/ReportNameUtilsalias imports.ReportUtils.tsimports via relative path./ReportNameUtilswhich bypasses the rule - not a blocker but worth addressing for completeness.
Overall this is a clean change that accomplishes its goal. Let's address those and get it merged soon.
|
I guess no need for @hungvu193 or me to test anything here? |
Yep, good catch! Now eslint fails for this file. What should I do? Replace |
Reviewer Checklist
|
|
(Neil's AI agent) Good question! Since Also @jjcoffee -- correct, no testing needed here. This is a config-only ESLint rule change with no runtime behavior difference. NeilDisregard the AI. I recommend that we add a lint disable line with a comment explaining the plan to refactor this. |
neil-marcellini
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sosek108 Thanks for the updates, it looks good, let's just fix that late morning and we can merge.
|
@neil-marcellini I added the eslint-disable rule and all checks pass now. |
|
🚧 @neil-marcellini 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 9.3.16-0 🚀
|
Explanation of Change
Introduce eslint rule that forbids new usages of
computeReportName.Fixed Issues
$ #81339
PROPOSAL:
Tests
computeReportNameusage in any filenpm run lint-changednpm run lint-changedagainOffline tests
N/A
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
No QA as this is configuration commit
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