Repository navigation
Conversation
…t for the lightning bolt Co-authored-by: thelullabyy <thelullabyy@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: The PR's only documented test step is "Verify that no errors appear in the JS console", so I ran it across the changed distance-rate flows on a GBP Control workspace: no errors or broken UI appeared. Two gaps: this dev backend returned only the standard GB (AMAP) rate, so fuel-band rows never appeared, and the web tool cannot read raw console output, so the console check relies on no visible errors.
|
|
🤖 Android test steps verification results: ✅ pass On Android, GB government rates were auto-generated with the lightning bolt, and turning on Track tax and regenerating the rates caused no errors. The bolt stayed through every Tax reclaimable edit, and no JS errors or redboxes appeared. As on Web, only the standard GB (AMAP) rates appeared, never fuel bands, and logs weren't streamed, so the console check relies on no visible errors.
|
|
@MelvinBot Please replace all |
|
@thelullabyy 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] |
|
Done. I updated the PR description and marked the PR ready for review.
Earlier dev-environment runs showed only AMAP rates, so you may need staging to see the fuel-band rows. I left Offline tests and Screenshots/Videos empty for you to fill in. |
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. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-10-20.at.00.13.18.movScreen.Recording.2026-10-20.at.00.16.20.mov |
thelullabyy
left a comment
thelullabyy
left a comment
There was a problem hiding this comment.
Overall the logic looks good to me. A few comments inline.
One small thing in the PR description: Tests step 17 says to set a tax rate on a Great Britain rate on the AMAP workspace from step 1, but that workspace has Taxes / Track tax off, so the tax rate and Tax reclaimable on fields aren't available there. Can you update that step (e.g. turn on Taxes but leave Track tax off, or remove it)?
|
|
||
| if (shouldAutoUpdateGovernmentDistanceRates && customUnit) { | ||
| const copiedSourceRateIDs = new Set(Object.values(customUnit.rates ?? {}).map((rate) => rate.attributes?.governmentRate?.sourceRateID)); | ||
| const shouldUseFuelBandRates = isTaxTrackingEnabled(true, policy, true); |
There was a problem hiding this comment.
This is only evaluated when auto-update is turned on. What happens if a GBP workspace already has auto-update on with the AMAP rates and the admin then turns on Track tax (or turns it off on a workspace that has the bands)? enableDistanceRequestTax doesn't touch the rates, so optimistically nothing changes.
Does the server swap AMAP <-> fuel bands when distance tax is toggled? If it does, we should mirror that optimistically in enableDistanceRequestTax (and the policy Taxes toggle). If it doesn't, the workspace stays on AMAP after enabling tax, which seems to go against the goal here. Either way, can we add a test step covering this order (auto-update first, Track tax second)?
There was a problem hiding this comment.
Good catch. The server does handle this order, but this PR doesn't mirror it optimistically, on purpose.
- Turning tax on: when turning on distance tax (or turning tax tracking back on) makes a GBP auto-update workspace eligible, the server adds the missing fuel-band rates. It doesn't remove the existing AMAP rates. Mirroring that in
enableDistanceRequestTaxandenablePolicyTaxesis the follow-up [HOLD #104135] Optimistic UK fuel-band rates when turning on tax, which is on hold for this PR and reusesbuildOnyxDataForGovernmentRateAutoUpdate. - Turning tax off: nothing is swapped. Existing fuel-band rates stay, the same way auto-update never modifies existing rates.
I added a "Turning on tax after auto-update" block to Tests and QA (steps 17–19): on the AMAP workspace, turn on Taxes and Track tax, then check the AMAP rates are unchanged and keep the bolt. That also fixes the old step 17, which set a tax rate while tax was off.
|
|
||
| /** Whether a government rate is a UK fuel band, whose series carries the band after the country code. */ | ||
| function isFuelBandGovernmentRate(sourceRateID: string): boolean { | ||
| return getGovernmentRateSeries(sourceRateID) !== sourceRateID.split('_').at(0); |
There was a problem hiding this comment.
NAB: this computes the series and splits the ID again just to check whether there is a segment between the country and the date. Could be simpler and easier to read:
return sourceRateID.split('_').length > 2;There was a problem hiding this comment.
Done in edc5108: isFuelBandGovernmentRate now returns sourceRateID.split('_').length > 2. Results don't change: GB_2026-04-06 has 2 parts and band IDs have more.
|
@MelvinBot Please update your PR based on above reviews |
Co-authored-by: thelullabyy <thelullabyy@users.noreply.github.com>
|
@thelullabyy @neil-marcellini One of you needs to 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] |
|
I updated the PR based on your review.
Typecheck, ESLint on the changed file, and 309 Jest tests in |
|
I reviewed part of the changes here, then found that issue. Once you get it fixed, I'll review again. |
|
@MelvinBot Please check and fix |
… are published Co-authored-by: thelullabyy <thelullabyy@users.noreply.github.com>
|
The failing checks are unrelated to this PR, so I didn't push a fix.
All other checks pass. To clear the Jest failure, rerun the failed job in GitHub Actions. |
|
The failing checks are still unrelated to this PR, so I didn't push a fix. Nothing has changed since my earlier comment.
I can't rerun jobs. To clear the Jest failure, open the Jest run and select Re-run failed jobs. |
|
@MelvinBot Please merge main |
…rnmentRates Co-authored-by: thelullabyy <thelullabyy@users.noreply.github.com>
|
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. |
|
@MelvinBot Please merge main, resolve conflicts, and add fix for comments in PR if needed |
…rnmentRates Co-authored-by: thelullabyy <thelullabyy@users.noreply.github.com> # Conflicts: # tests/unit/DistanceRateTest.ts
|
I merged
Typecheck, ESLint on the changed file, and |
|
@neil-marcellini Could you please do another review? |
neil-marcellini
left a comment
neil-marcellini
left a comment
There was a problem hiding this comment.
A few NABs and one thing to fix.
| setWorkspaceDistanceAutoUpdate( | ||
| policyID, | ||
| customUnit, | ||
| policy, | ||
| isOn, | ||
| governmentMileageRates ?? [], | ||
| policy?.outputCurrency, | ||
| policy.outputCurrency, | ||
| isOn && isSharedCurrency ? autoUpdateCountry : undefined, | ||
| !!policy?.shouldAutoUpdateGovernmentDistanceRates, | ||
| !!policy.shouldAutoUpdateGovernmentDistanceRates, | ||
| isSharedCurrency ? autoUpdateCountry : undefined, | ||
| ); |
There was a problem hiding this comment.
NAB: We can remove the parameters that come directly from the policy and set them up within the function.
There was a problem hiding this comment.
Done in 59957bb. setWorkspaceDistanceAutoUpdate now takes (policy, isOn, governmentMileageRates, countryCode?). It reads the currency, the previous flag and the previous country from policy. All three callers and the tests are updated.
buildOnyxDataForGovernmentRateAutoUpdate keeps those parameters, because updateGeneralSettings passes the new currency, not the policy's current one.
There was a problem hiding this comment.
Okay, I guess it makes sense that "Build Onyx data for government rate auto update" does keep a separate currency parameter, but the "should auto update government distance rates" parameter should be removed.
There was a problem hiding this comment.
Done in e01722a. The previous auto-update flag (and previous country) parameters are removed from buildOnyxDataForGovernmentRateAutoUpdate, which now reads them from policy. The currency parameter stays.
| // Then no rates are copied optimistically, because the server copies the fuel bands and its response adds them | ||
| const apiCallArgs = apiWriteSpy.mock.calls.find((call) => call.at(0) === WRITE_COMMANDS.UPDATE_WORKSPACE_GENERAL_SETTINGS); | ||
| const params = requireRecord(requireCallArgument(apiCallArgs, 1)); | ||
| expect(params.optimisticRateIDs).toBeUndefined(); |
There was a problem hiding this comment.
This is a problem. The governmentRateCountry is only meant to be used when a user has to pick which country they want to use with the EUR currency. For other currencies like GBP it doesn't apply because we know the country based on the currency. So governmentRateCountry should not be passed and the AFR rates should still be added optimistically.
There was a problem hiding this comment.
Fixed in 59957bb.
- Copy on currency change:
updateGeneralSettingsnow copies the rates optimistically when auto-update is on and the currency changes to one that maps to a single country (GBP, USD, CAD, AUD). The builder filters the rates by currency, so no country is needed. - Params:
governmentRateCountryis only sent for EUR.optimisticRateIDsis sent whenever rates are copied. - Tests: the GBP tests no longer pass a country. They check that an eligible workspace gets only the fuel bands, a workspace without tax tracking gets the standard rate, and auto-update off copies nothing.
Please confirm one thing on the server side. Does UpdateWorkspaceGeneralSettings read optimisticRateIDs when governmentRateCountry is absent? Until now the App only sent them together. If the server ignores the IDs, its rates won't match the optimistic ones.
|
@MelvinBot could you please check and resolve comments above |
…es and derive auto-update state from the policy Co-authored-by: thelullabyy <thelullabyy@users.noreply.github.com>
|
I fixed all three of
One open question for Neil: does the server read The 324 tests in |
|
@neil-marcellini could you please recheck, the PR is updated. I also re-tested the PR, it works well |
neil-marcellini
left a comment
neil-marcellini
left a comment
There was a problem hiding this comment.
Looking really close, requesting changes to align the proper optimistic behavior.
| const {optimisticRateIDs, onyxData} = buildOnyxDataForGovernmentRateAutoUpdate( | ||
| policy, | ||
| shouldAutoUpdateGovernmentDistanceRates, | ||
| governmentMileageRates, | ||
| outputCurrency, | ||
| policy.outputCurrency, | ||
| countryCode, | ||
| previousAutoUpdateEnabled, | ||
| previousCountryCode, | ||
| !!policy.shouldAutoUpdateGovernmentDistanceRates, | ||
| policy.autoUpdateGovernmentRateCountry, |
There was a problem hiding this comment.
NAB: I have the same non-blocking comment now on this function instead of set workspace distance auto update. If we can get the parameter from the policy object directly, there's no need to pass it separately. You can fix this in a follow-up.
There was a problem hiding this comment.
Done in e01722a. buildOnyxDataForGovernmentRateAutoUpdate no longer takes previousAutoUpdateEnabled or previousCountryCode. It reads both from policy for the failure data. It keeps the currency parameter, since the currency change passes the new currency.
| setWorkspaceDistanceAutoUpdate( | ||
| policyID, | ||
| customUnit, | ||
| policy, | ||
| isOn, | ||
| governmentMileageRates ?? [], | ||
| policy?.outputCurrency, | ||
| policy.outputCurrency, | ||
| isOn && isSharedCurrency ? autoUpdateCountry : undefined, | ||
| !!policy?.shouldAutoUpdateGovernmentDistanceRates, | ||
| !!policy.shouldAutoUpdateGovernmentDistanceRates, | ||
| isSharedCurrency ? autoUpdateCountry : undefined, | ||
| ); |
There was a problem hiding this comment.
Okay, I guess it makes sense that "Build Onyx data for government rate auto update" does keep a separate currency parameter, but the "should auto update government distance rates" parameter should be removed.
| for (const rate of Object.values(customUnit.rates ?? {})) { | ||
| const sourceRateID = rate.attributes?.governmentRate?.sourceRateID; | ||
| if (sourceRateID) { | ||
| addSeriesRate(sourceRateID, rate.startDate ?? rate.attributes?.governmentRate?.startDate, rate.enabled); |
There was a problem hiding this comment.
(Neil's AI agent, human reviewed)
I noticed the server and the App disagreed on when a copied rate inherits enabled. So I updated the issue description (and the backend spec) with a clearer rule. We're aligning the backend (Auth #25931) and the frontend on it.
The behavior we want:
- A copied rate takes
enabledfrom the previous rate in its series, in every country. An admin who disables a government rate opts out of it from then on. This part already works. - It only does so when that previous rate is still an unmodified government rate. A rate the admin edited is their own, so its enabled state shouldn't decide the next government rate. If the latest previous rate was edited, the new rate is enabled, even if an older period was disabled.
- "Previous" goes by the government start date in the snapshot, not the rate's own dates, which the admin can edit.
Example for one band:
- March: unmodified, disabled. June: edited by the admin. September: copied now, so it should be enabled.
- If June were unmodified and disabled, September should be disabled.
Today, an admin who edits GB_PETROL_UP_TO_1400_2026-06-01 to their own amount and disables it gets the September band added disabled, even though they never opted out of the government rate.
Please also update the builder's doc comment ("matching the server's copy") and the note in the PR description.
There was a problem hiding this comment.
Fixed in e01722a (from thelullabyy's draft). A copied rate now takes enabled from the previous rate in its series only when that rate is an unmodified government rate. If the previous rate was edited, or there is none, the copied rate is enabled. "Previous" is ordered by the snapshot's start date. I updated the builder's doc comment and the PR description note.
| expect(rates).toHaveLength(2); | ||
| }); | ||
|
|
||
| it('should keep a disabled fuel band disabled for every later rate of its series in the same request', async () => { |
There was a problem hiding this comment.
(Neil's AI agent, human reviewed)
Please extend this test for the "unmodified only" rule from the comment on the builder: add an existing diesel rate (for example GB_DIESEL_UP_TO_1600_2026-03-01) whose amount the admin edited and which they disabled. Then assert that the copied September diesel band is enabled.
There was a problem hiding this comment.
Done in e01722a. The test now adds an edited, disabled GB_DIESEL_UP_TO_1600_2026-03-01 rate and asserts that the copied September diesel band is enabled.
|
@MelvinBot I draft a solution for above comments/suggestions here thelullabyy@3396940 Please check and bring it here |
…ad previous auto-update state from the policy Applies thelullabyy's draft (thelullabyy/App@3396940). Co-authored-by: thelullabyy <thelullabyy@users.noreply.github.com>
|
@MelvinBot Deep review and fix issues (pipelines,...) |
|
I brought your draft into this PR unchanged in e01722a, with you as co-author. It matches the rule in the issue description, so I didn't change any logic.
I updated the PR description and replied in Neil's four threads. Typecheck passes, and so do 324 tests in |
…d test the latest-edited-rate rule Co-authored-by: thelullabyy <thelullabyy@users.noreply.github.com>
|
I fixed the two failing checks and two small issues from a deep review in 20b8873.
All other checks passed on the previous commit. The review found no other logic bugs. One question for Neil is still open: does Typecheck, cspell, Oxfmt and 326 tests in |
|
Both checks are already fixed in 20b8873, and both now pass on the latest run. My previous commit caused both failures.
That commit also has two extra changes:
The 326 tests in |
Explanation of Change
The UK publishes fuel-band mileage rates alongside its standard (AMAP) rate. The server will copy the bands, with their VAT-reclaimable fraction, only to workspaces that track tax and have distance tax on, and AMAP to every other workspace. The App's optimistic copy didn't know about bands, so it would have added AMAP plus every band, without the fraction. The lightning bolt also ignored the reclaimable amount, so an edited band still looked auto-generated.
GovernmentMileageRateandGovernmentRateSnapshotget an optionaltaxClaimablePercentage.buildOnyxDataForGovernmentRateAutoUpdateusesisTaxTrackingEnabled(true, policy, true)asshouldUseFuelBandRates. For GB rates only, it copies the bands when that is true and the standard rate otherwise. Other countries are unaffected.getGovernmentRateSeriesstrips the trailing_<startDate>withlastIndexOf('_'), so band IDs likeGB_PETROL_UP_TO_1400_2026-06-01(whose band names contain_) resolve toGB_PETROL_UP_TO_1400.taxClaimablePercentagegoes on the rate and its snapshot when the reference rate has one.enabledcomes from the previous rate in the same series, ordered by the snapshot's start date and including rates copied earlier in the same request. It only does so when that previous rate is an unmodified government rate. If the previous rate was edited, or there is none, the copied rate is enabled.taxRateExternalIDis left to the server response.buildOnyxDataForGovernmentRateAutoUpdatenow takespolicyinstead ofpolicyID+customUnit, which are derived inside. It also reads the previous auto-update flag and country frompolicyfor the failure data.setWorkspaceDistanceAutoUpdatetakespolicy, the new flag, the reference rates and an optional country. It reads the currency frompolicy.updateGeneralSettingsnow copies that country's rates optimistically and sendsoptimisticRateIDs.governmentRateCountryis only sent for EUR.isGovernmentRateUnmodifiedalso compares the reclaimable amount at the 4-decimal precision the edit page shows. Re-entering the shown value therefore brings the bolt back. Snapshots without a fraction, such as US/EUR rates, ignore it as before. The edit page now uses the samegetTaxClaimableDisplayAmounthelper, and itsuseCallbackwas removed so React Compiler can still compile the component.Note: the
enabledinheritance applies to every country's copied rates, following the rule in the issue description.AI tests run by MelvinBot:
npm run typecheck,npm run lint-changed,npm run fmt,npm run spell-changed,react-compiler-compliance-checkon the changed pages, and Jest forDistanceRateTest,PolicyDistanceRatesUtilsTest,PolicyTest,PolicyUtilsTest,WorkspaceOverviewCurrencyPageTest,WorkspaceUpgradeTest,IOURequestStepDistanceRateTestand the distance-rate hook tests (all passed). The full Jest suite and the Storybook smoke test were not run locally.Fixed Issues
$ #104135
PROPOSAL: #104135 (comment)
Tests
Standard (AMAP) rates — workspace without distance tax
Fuel-band rates — workspace with distance tax
These steps need the server to publish GB fuel-band reference rates. Until it does, only the standard rates appear.
6. Create another Control workspace and set its default currency to GBP.
7. Go to Workspace > More features and turn on Distance rates and Taxes.
8. Go to Distance rates > Settings and turn on Track tax.
9. In the same Settings page, turn on Auto-update government rate.
10. Go back to Distance rates. Verify the Great Britain fuel-band rates are added (one rate per fuel type / engine size band), each with the lightning bolt.
11. Verify the standard Great Britain (AMAP) rate is not added.
Lightning bolt and tax reclaimable
12. Open one of the fuel-band rates and note the Tax reclaimable on amount.
13. Change the Tax reclaimable on amount to a different value and save.
14. Verify the lightning bolt no longer shows for that rate.
15. Open the same rate, set Tax reclaimable on back to the amount from step 12, and save.
16. Verify the lightning bolt shows again for that rate.
Turning on tax after auto-update
17. On the workspace from step 1, go to More features and turn on Taxes. Then go to Distance rates > Settings and turn on Track tax.
18. Go back to Distance rates. Verify the existing Great Britain (AMAP) rates are unchanged and still show the lightning bolt. Adding fuel-band rates at this point is out of scope here and tracked in [HOLD #104135] Optimistic UK fuel-band rates when turning on tax.
19. Open one of these AMAP rates, set a tax rate, change its Tax reclaimable on amount, and save. Verify the lightning bolt still shows (AMAP rates ignore the reclaimable amount).
Offline tests
QA Steps
Standard (AMAP) rates — workspace without distance tax
Fuel-band rates — workspace with distance tax
These steps need the server to publish GB fuel-band reference rates. Until it does, only the standard rates appear.
6. Create another Control workspace and set its default currency to GBP.
7. Go to Workspace > More features and turn on Distance rates and Taxes.
8. Go to Distance rates > Settings and turn on Track tax.
9. In the same Settings page, turn on Auto-update government rate.
10. Go back to Distance rates. Verify the Great Britain fuel-band rates are added (one rate per fuel type / engine size band), each with the lightning bolt.
11. Verify the standard Great Britain (AMAP) rate is not added.
Lightning bolt and tax reclaimable
12. Open one of the fuel-band rates and note the Tax reclaimable on amount.
13. Change the Tax reclaimable on amount to a different value and save.
14. Verify the lightning bolt no longer shows for that rate.
15. Open the same rate, set Tax reclaimable on back to the amount from step 12, and save.
16. Verify the lightning bolt shows again for that rate.
Turning on tax after auto-update
17. On the workspace from step 1, go to More features and turn on Taxes. Then go to Distance rates > Settings and turn on Track tax.
18. Go back to Distance rates. Verify the existing Great Britain (AMAP) rates are unchanged and still show the lightning bolt.
19. Open one of these AMAP rates, set a tax rate, change its Tax reclaimable on amount, and save. Verify the lightning bolt still shows.
Regression — other countries
20. Create a Control workspace in USD, turn on Distance rates, and turn on Auto-update government rate.
21. Verify the United States government rates are added with the lightning bolt, as before.
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 in all cases)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