Fix undeclared allReportsViolations in hasVisibleReportFieldViolations#84952
Fix undeclared allReportsViolations in hasVisibleReportFieldViolations#84952
Conversation
…Violations Add reportViolations as a parameter to hasVisibleReportFieldViolations and pipe the data through from the Onyx derived config in reportAttributes.ts. The function was referencing an undeclared module-level variable allReportsViolations, causing typecheck, ESLint, and test failures. Co-authored-by: Daniel Gale-Rosen <dangrous@users.noreply.github.com>
The reportAttributes config now has 12 dependencies after adding REPORT_VIOLATIONS, but the test was still passing arrays with only 11 elements, causing a typecheck failure. Co-authored-by: Daniel Gale-Rosen <dangrous@users.noreply.github.com>
|
Fixed the failing typecheck: The |
|
@ShridharGoel 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] |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 909176f748
ℹ️ 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".
| ONYXKEYS.SESSION, | ||
| ONYXKEYS.COLLECTION.POLICY, | ||
| ONYXKEYS.COLLECTION.POLICY_TAGS, | ||
| ONYXKEYS.COLLECTION.REPORT_VIOLATIONS, |
There was a problem hiding this comment.
Adding ONYXKEYS.COLLECTION.REPORT_VIOLATIONS as a dependency here causes compute to run when report-field violations change, but the incremental path still only treats report/report-metadata/report-actions/report-name-value-pairs/transaction updates as actionable; when isFullyComputed is true and only report-violation keys change, it returns early via the "No updates to process" path, so hasVisibleReportFieldViolations(...) is not re-evaluated and sidebar brick-road state can stay stale until an unrelated update arrives.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@MelvinBot can you double check this one? Also please attempt to figure out why this passed tests on #83424 but then failed them on the workflow. I'm guessing it is because of recent changes to main, but that might give us a clue as to a better solution here, too.
There was a problem hiding this comment.
The concern flagged here is valid. Adding ONYXKEYS.COLLECTION.REPORT_VIOLATIONS as a dependency means compute will fire when report-field violations change, but the incremental update path (after isFullyComputed is true) doesn't track reportViol
8000
ationsUpdates — so it hits the "No updates to process" early-return and hasVisibleReportFieldViolations is never re-evaluated for the affected report.
Whether this is a regression vs. pre-existing: Before PR #83424, hasVisibleReportFieldViolations didn't exist at all, so there was no brick-road indicator for report field violations in the first place. The original code in #83424 used a module-level allReportsViolations via Onyx.connect, which would have had its own reactivity path. This PR correctly fixes the broken reference, but doesn't fully replicate the reactivity that the original approach intended.
Practical impact: If report-field violations can change independently of report/transaction updates (e.g., pushed via Pusher after a server-side policy check), the sidebar brick-road indicator would go stale until some other Onyx update triggers a full recompute. If they only change alongside report updates (which are already tracked), the gap is latent.
Recommendation: Since this PR is an urgent fix for a build breakage on main, the current approach is fine to merge as-is — it restores the build and is no worse than having no report-field violation indicator at all (which was the state before #83424). The incremental update handling should be added as a follow-up to fully complete the feature.
There was a problem hiding this comment.
Yes I think we should merge this, and the above P1 should be handled as a follow up of #83424
|
@dangrous This is pretty bad and should be CPed to staging at least. |
🔍 PR Review & InvestigationWhy PR #83424 passed CI but broke
|
| Timestamp (UTC) | Event |
|---|---|
| 2026-03-07 00:12 | Last main commit that PR #83424 synced with |
| 2026-03-09 04:45 | PR #83424 last merges from main ("resolve conflict") |
| 2026-03-09 ~04:47 | PR #83424 CI passes — allReportsViolations still exists on main |
| 2026-03-09 19:32 | PR #80104 merges to main — removes allReportsViolations entirely |
| 2026-03-11 18:57 | PR #83424 merges to main — breakage (references now-deleted variable) |
| 2026-03-11 19:27 | PR #84952 created to fix the breakage |
PR #80104 ("[No QA] Removing ONYXKEYS.COLLECTION.REPORT_VIOLATIONS in ReportUtils") removed the module-level allReportsViolations variable and its Onyx.connect block from ReportUtils.ts. This happened ~15 hours after PR #83424's last CI run but ~2 days before it was merged. Since #83424 was never re-synced with main after #80104 landed, its green CI was stale.
GitHub's branch protection checks CI on the PR branch's head commit but not against the current state of main at merge time, which is how this slipped through.
Review of PR #84952 fix
The core approach is correct. Passing reportViolations as an explicit parameter to hasVisibleReportFieldViolations is the right pattern — it's consistent with how hasReportViolations and getFieldViolation already work in ReportUtils.ts, and the codebase has moved away from module-level Onyx subscriptions for violations.
However, there is one gap: missing incremental update handling for REPORT_VIOLATIONS.
In reportAttributes.ts, every other collection dependency has explicit incremental update detection (lines 105-110):
REPORT→reportUpdatesREPORT_METADATA→reportMetadataUpdatesREPORT_ACTIONS→reportActionsUpdatesREPORT_NAME_VALUE_PAIRS→reportNameValuePairsUpdatesTRANSACTION→transactionsUpdatesTRANSACTION_VIOLATIONS→transactionViolationsUpdates
REPORT_VIOLATIONS is completely absent from this incremental logic. After isFullyComputed becomes true, if only report violations change (without concurrent report/transaction changes), the compute function will hit the early-return and return stale currentValue. The fix should add something like:
const reportViolationsUpdates = sourceValues?.[ONYXKEYS.COLLECTION.REPORT_VIOLATIONS];...and include it in the incremental update conditional, plus logic to determine which reports need re-computation when report violations change (similar to how transactionViolationsUpdates is handled, but simpler since reportViolations_ keys map directly to report IDs).
Whether this matters in practice depends on whether report violations can change independently of report/transaction updates. If they only change alongside report changes (which are already tracked), this gap may be latent. But if violations can be pushed independently from the server (e.g., via Pusher), stale brick-road indicators would result.
Summary
- Why CI passed on fix: GBR appears when there is report field error #83424: Merge skew —
allReportsViolationswas removed frommainby PR [No QA]Removing ONYXKEYS.COLLECTION.REPORT_VIOLATIONS in ReportUtils #80104 between fix: GBR appears when there is report field error #83424's last CI run and its merge. - Is the fix correct: Yes, the parameter-passing approach is correct and consistent with codebase patterns.
- Potential improvement: Add incremental update handling for
REPORT_VIOLATIONSin thereportAttributes.tscompute function to pr 7D4E event stale data after the initial full compute.
Next Steps: Reply with @MelvinBot implement this to have me add the missing incremental update handling, or @MelvinBot <your feedback> to discuss further.
|
@MelvinBot implement this ^ |
…utes When report violations change independently after the initial full compute, the reportAttributes derived value now correctly detects the change and recomputes the affected reports. Without this, report field violation changes could result in stale brick-road indicators. Co-authored-by: Daniel Gale-Rosen <dangrous@users.noreply.github.com>
|
Added incremental update handling for
This ensures that when report violations change independently (after the initial full compute), the affected reports are re-evaluated and brick-road indicators stay accurate. All 35 OnyxDerived tests pass. Full typecheck couldn't run in this environment due to memory limits, but ESLint passes clean and the changes are structurally identical to the existing patterns. |
|
@ShridharGoel I think this is ready for a final look, the update wasn't too tricky it seems. I'll get internal eyes on it to review/merge once tests pass |
| reportAttributes.compute([reports, undefined, undefined, undefined, undefined, undefined, undefined, undefined, undefined, undefined, undefined, undefined], {}); | ||
| const reportAttributesComputedValue = reportAttributes.compute( | ||
| [reports, undefined, undefined, undefined, undefined, undefined, undefined, undefined, undefined, undefined, undefined], | ||
| [reports, undefined, undefined, undefined, undefined, undefined, undefined, undefined, undefined, undefined, undefined, undefined], |
There was a problem hiding this comment.
It would be really nice to have a test where this new value IS used. NAB because it's not necessary to fix the blocker, but if there are any follow-up issues, please add a test covering this.
There was a problem hiding this comment.
Yeah that makes sense, I'll ping on the initial issue
|
I discussed on Slack with @dangrous about adding some actual QA tests for this to ensure it's working (especially the report violations). I can merge it as-is and the tests can be updated aftewards. |
|
@tgolen looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
🚧 @tgolen 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. |
|
Since the original PR is linked to couple blockers, I have gone ahead and reverted this and the original PR so @daledah can fix and test it thoroughly. I hope that works |
|
🚀 Cherry-picked to staging by https://github.com/tgolen in version: 9.3.37-0 🚀
|
Explanation of Change
PR #83424 introduced
hasVisibleReportFieldViolationsinReportUtils.ts, which references an undeclared variableallReportsViolations. This caused typecheck, ESLint, and test failures on main (issues #84928, #84929, #84930).This fix adds
reportViolationsas an explicit parameter tohasVisibleReportFieldViolationsinstead of relying on a nonexistent module-level variable. The caller inreportAttributes.tsnow includesONYXKEYS.COLLECTION.REPORT_VIOLATIONSas an Onyx dependency and passes the specific report's violations to the function.Fixed Issues
$ #84930
$ #84929
$ #84928
Tests
npx tsc --noEmit)Offline tests
Same as QA
QA Steps
Precondition:
Create a text type report field in workspace settings > Reports.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand 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
N/A — build/compilation fix only, no UI changes.