Repository navigation
Conversation
Co-authored-by: Shubham Agrawal <shubham1206agra@users.noreply.github.com>
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
🤖 Web test steps verification results: A report field that fails to save because of an invalid formula now stays in the list with the server's error on its row, and dismissing the error removes the failed row; a valid field still saves cleanly. The PR has no Tests section yet, so I used the linked issue's reproduction steps, and the JS console could not be inspected on this web setup.
|
|
🤖 Android test steps verification results: On Android, a report field that fails to save because of an invalid formula stays in the list with the server's error on its row, and dismissing the error removes the failed row; a valid field still saves cleanly. The PR has no Tests section yet, so I used the linked issue's reproduction steps, and device logs were not captured, so the log check is based on the UI only.
|
| // Keep the failed field in the list so its error shows on its own row until the user dismisses it | ||
| fieldList: { | ||
| [fieldKey]: null, | ||
| [fieldKey]: {...optimisticReportFieldDataForPolicy, pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD}, |
There was a problem hiding this comment.
@MelvinBot We don't need to update anything in case of failure. Maybe remove this altogether. Also, wrong errorField is added, fix that too.
There was a problem hiding this comment.
I made both changes in f6b7910.
- Failure data:
createReportFieldno longer updates the policy'sfieldListon failure. The optimistic data already keeps the row withpendingAction: ADD. The failure data still removes the field from each report'sfieldList, so a failed field doesn't stay on reports. - Error field: The generic error now goes to
errorFields.fieldList.<fieldKey>.defaultValue, where the server returns its error (ReportField.ts:112). I made the same fix inupdateReportFieldInitialValue, which used the same wrong key. Since nothing writeserrorFields.<fieldKey>anymore, I removed the row's fallback to it and stopped clearing it.
Typecheck, lint, and ReportFieldTest/ReportUtilsTest (1567 tests) pass. I couldn't re-test on Web because the test session showed a blank page. The first commit passed on Web.
view run · no recording available
…undant failure fieldList update The optimistic data already keeps the new field on the policy with pendingAction ADD, so the failure data no longer re-writes it. The generic failure error now goes to errorFields.fieldList.<fieldKey>.defaultValue, where the server returns its errors, so the row reads a single location. Co-authored-by: Shubham Agrawal <shubham1206agra@users.noreply.github.com>
| <OfflineWithFeedback | ||
| pendingAction={item.pendingAction} | ||
| errors={item.errors} | ||
| errorRowStyles={[cardPaddingHorizontal, styles.mb2]} |
There was a problem hiding this comment.
| errorRowStyles={[cardPaddingHorizontal, styles.mb2]} | |
| errorRowStyles={cardPaddingHorizontal} | |
| errorRowTextStyles={styles.mv2} |
@MelvinBot Implement this suggestion
There was a problem hiding this comment.
I applied your suggestion in 73fc3e7. The error row now uses errorRowStyles={cardPaddingHorizontal} and errorRowTextStyles={styles.mv2}. I also changed styles.mb2 to styles.mv2 in the renderItem dependency list (WorkspaceFieldsSection.tsx:179-195).
On Web, the error lines up with the field name and has even spacing above and below. Dismissing it still removes the failed field.
Typecheck and the React Compiler check pass. ESLint reports only the MenuItem deprecation, which is already in the seatbelt baseline for this file.
view run · no recording available
Co-authored-by: Shubham Agrawal <shubham1206agra@users.noreply.github.com>
| // If the report is unsettled, we want to merge the new fields that get added to the policy with the fields that | ||
| // are attached to the report. | ||
| const mergedFieldIds = Array.from(new Set([...policyReportFields.map(({fieldID}) => fieldID), ...reportFields.map(({fieldID}) => fieldID)])); | ||
| // Skip policy fields pending ADD that aren't on the report yet. Optimistic adds are written to the report too, so this |
There was a problem hiding this comment.
@MelvinBot Do we really need this condition given that we are clearing that field from reports on failure?
There was a problem hiding this comment.
Yes, we still need it. Removing the field from each report's fieldList on failure isn't enough, because getAvailableReportFields also pulls fields from the policy.
- The function merges the policy's field IDs with the report's (
ReportUtils.ts:5656-5657). - When the report has no copy of a field, it returns the policy's version (
ReportUtils.ts:5675-5677). - With this PR, a failed field stays on the policy with
pendingAction: ADDuntil the user dismisses the error (ReportField.ts:281-289).
Without the filter, that failed field would come back on every unsettled report until the error is dismissed. Before this PR, the failure data removed the field from the policy too, so this didn't come up.
The reportFieldIDs.has(fieldID) part keeps optimistic adds visible: they're written to the report as well, so a successful add in progress still shows. The test at ReportUtilsTest.ts:22765 covers the failed case, and the one at ReportUtilsTest.ts:22782 covers the in-progress case.
|
The failing check is unrelated to this PR, so I didn't push a fix. Analysis: The only failing code check is Jest Evidence:
Next step: Rerun the failed Jest job (run). I couldn't start the rerun from here.
|
|
The failing Failing test: Why it's unrelated
The other failing checks ( |
| * Returns the errorFields update that sets a report field's generic failure error at errorFields.fieldList.<fieldKey>.defaultValue, where the server also returns its errors. | ||
| */ | ||
| function getReportFieldFailureErrorFields(fieldKey: string) { | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-type-assertion -- ErrorFields doesn't model the nested fieldList.<fieldKey>.defaultValue shape the server uses, same as the report title error |
There was a problem hiding this comment.
@MelvinBot Is there any way to fix this? If not, point me to similar functions.
There was a problem hiding this comment.
No. The as unknown as ErrorFields cast can't be removed with a local change. Policy.errorFields is typed as ErrorFields, which is one level too shallow for fieldList.<fieldKey>.defaultValue.<microtime>. The App already writes the report title error the same way, with the same cast.
Similar functions:
Policy.ts:7298-7315: the failure data insetPolicyDefaultReportTitlewriteserrorFields.fieldList.<title>.defaultValuewith the same cast. This is the only other one insrc.Policy.ts:8190-8201:clearPolicyTitleFieldErroruses the same shape asgetReportFieldErrorFieldsToClear.ReportsDefaultTitle.tsx:88-89: reads the error the same way asWorkspaceFieldsSection.tsx:156.
Why a type change is out of scope
ErrorFieldsisRecord<string, Errors>, andErrorsisRecord<string, string | null>(OnyxCommon.ts:27-30). SoerrorFields.fieldList.<fieldKey>has to be a string, not{defaultValue: {...}}.- Adding
& {fieldList?: Record<string, ErrorFields>}doesn't work. The intersection with thestringindex collapses the values tonull. - A separate
PolicyErrorFieldstype with a union index would change what every reader ofpolicy.errorFieldsgets back. There are about 320 readers insrc, includinggetLatestErrorField(policy, ...). That belongs in its own PR.
The cast in the title code has no disable comment, so I assume it's covered by the lint baseline. The new helper uses an eslint-disable with a reason instead, so the cast stays in one documented place.
|
@MelvinBot Ready the PR for review |
|
The failing The other two failures, Analysis
|
|
🤖 I marked this PR as ready for review. The Tests, Offline tests, and QA Steps sections still have TODO placeholders. |
|
@shubham1206agra 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] |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
The failing Jest check is unrelated to this PR. It looks flaky, so I didn't push a fix. Please rerun the failed job. Analysis: Evidence:
The other two failures, |
![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: 73fc3e7497
ℹ️ 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".
| // Skip policy fields pending ADD that aren't on the report yet. Optimistic adds are written to the report too, so this | ||
| // keeps fields that failed to be created (kept on the policy until their error is dismissed) off reports. | ||
| const reportFieldIDs = new Set(reportFields.map(({fieldID}) => fieldID)); | ||
| const availablePolicyReportFields = policyReportFields.filter(({fieldID, pendingAction}) => pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD || reportFieldIDs.has(fieldID)); |
There was a problem hiding this comment.
When a field is created while offline and then a new report is created before that request is sent, the new report was not present in policyReportIDs and therefore does not contain the optimistic field in its own fieldList. This filter consequently hides the policy field solely because it is pending ADD, so the newly created report omits a valid optimistic field until reconnection and API success. Failed additions need to be distinguished by their error state rather than treating every pending addition absent from a report as failed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8928621. The filter now hides a policy field only when it's pending ADD and also has an error, so it skips only failed adds (ReportUtils.ts:5661-5663). A field added offline still shows on reports created before the request is sent.
getAvailableReportFieldsandgetReportFieldMapstake an optionalpolicyErrorFields. The filter checkserrorFields.fieldList.<fieldKey>, the same key the row error reads.- All 5 callers in
srcnow passpolicy.errorFields. - I added a test for the offline case (
ReportUtilsTest.ts:22773). The failed-add test now passes an error.
Typecheck, ESLint on the changed files, the React Compiler check, and ReportUtilsTest/ReportFieldTest (1568 tests) pass. I didn't re-test on Web or run the spell check.
…adds Co-authored-by: Shubham Agrawal <shubham1206agra@users.noreply.github.com>
Reviewer Checklist
Screenshots/VideosScreen.Recording.2026-10-25.at.8.46.57.PM.mov |
|
Lots of conflicts @shubham1206agra |
|
@MelvinBot Fix conflicts |
There was a problem hiding this comment.
1. Medium: a failed field's row still opens its settings page.
The failed field stays in policy.fieldList with pendingAction: ADD, so navigateToFieldSettings opens its settings page. There, you can edit the initial value or delete the field. Both send API calls for a field the server doesn't have, so they fail again. Separately, isReportFieldNameExisting blocks creating a new field with the same name until you dismiss the error. That is the obvious way to retry with a fixed formula. I'd stop navigation for rows that are pending ADD and have an error, or at least add a QA step for this flow.
2. Low: errors stored under the old key can now stay forever.
Before this PR, the generic error went to errorFields.expensify_<id>, and the next create or update wiped it with errorFields: null. Now the optimistic data clears only errorFields.fieldList.<key>, and nothing clears the old key. hasPolicyErrorFields (src/libs/PolicyUtils/errors.ts:65) treats any non-empty errorFields entry as a workspace error. So users who already have one of these old errors keep a red dot they can't dismiss. A fix: also set errorFields[fieldKey]: null in the optimistic data.
3. Low: other code still treats failed fields as real.
These read policy.fieldList directly and don't skip failed fields:
- The Search report-field filter (
src/components/Search/FilterComponents/ReportField/index.tsx:210and:283). hasCircularReferences(src/libs/Formula.ts:224).useAdvancedSearchFilters.ts:247.
The impact is minor until you dismiss the error.
|
@MelvinBot Address points 1 and 3 from #104938 (review) |
…dFormulaError Co-authored-by: Shubham Agrawal <shubham1206agra@users.noreply.github.com> # Conflicts: # src/components/MoneyRequestReportView/MoneyRequestViewReportFields.tsx # src/components/ReportActionItem/MoneyReportView.tsx # src/pages/workspace/fields/WorkspaceFieldsSection.tsx
|
I merged Since this branch was created,
|
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
…r row Their row no longer opens the settings page, and the name check, the circular reference check, and the Search report field filters skip them. Co-authored-by: Shubham Agrawal <shubham1206agra@users.noreply.github.com>
|
I addressed points 1 and 3 from The new 1. Failed rows:
3. Other readers of
I added unit tests for |
Explanation of Change
When the server rejects a new report field (for example, because its default value has an invalid formula like
{field:DoesNotExist}), the App removed the field from the list and stored a generic error that nothing displayed. The user got no feedback.The backend now returns the specific error at
errorFields.fieldList.<fieldKey>.defaultValue.<microtime>, the same shape the App already reads for the report title error. This PR shows that error on the failed field's row, as Categories and Tags do:createReportField's failure data no longer touches the policy'sfieldList, so the field stays on the policy with thependingAction: ADDset by the optimistic data. It is still removed from each report'sfieldList.createReportFieldandupdateReportFieldInitialValueis now set aterrorFields.fieldList.<fieldKey>.defaultValue, instead oferrorFields.<fieldKey>.WorkspaceFieldsSectionpasses each row's latesterrorFields.fieldList[fieldKey].defaultValueerror toOfflineWithFeedback.clearReportFieldErrorsclears the field's errors. If the field is still pending ADD (it failed to be created), it also removes the field, mirroringclearCategoryErrors.getAvailableReportFieldsskips policy fields that are pending ADD and not already on the report. Optimistic adds are written to the report'sfieldList, so they still show.createReportFieldandupdateReportFieldInitialValueusederrorFields: null, which wiped every policy error, including the report title error. They now clear only that field's errors.updateReportFieldInitialValuealready keeps the row and reverts the value on failure, so its server error now also shows on the row.Fixed Issues
$ #104067
PROPOSAL: #104067 (comment)
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review"
// Please describe what tests you performed that validates your changed worked.
Offline tests
QA Steps
// TODO: The human co-author must fill out the QA tests you ran before marking this PR as "ready for review".
// Please describe what QA needs to do to validate your changes and what areas do they need to test for regressions.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)npm run compress-svg)Avataris modified, I verified thatAvataris working as expected)Designlabel and/or tagged@Expensify/designso the design team can review the changes.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
AI Tests
npm run typecheck: passednpm run lint -- <changed files>: passed (lint-changedcouldn't resolve the base branch in CI)npm run fmt(oxfmt): no changes outside the edited filesnpm run react-compiler-compliance-check check src/pages/workspace/fields/WorkspaceFieldsSection.tsx: passed on the first commit (check-changedcouldn't resolve the base branch in CI)npm test -- tests/actions/ReportFieldTest.ts tests/unit/ReportUtilsTest.ts: 1567 passed, including tests for failed-create, dismiss, narrow error clearing, andgetAvailableReportFieldsspell-changed: not run on the latest commit (couldn't resolve the base branch in CI)npm test, fullnpm run lint, and Storybook smoke test: not run