Render the Concierge system message for an automatic distance rate change - #100781
Conversation
Register the CONCIERGEAUTOSELECTDISTANCERATE action type so it is no longer filtered out by shouldReportActionBeVisible, and build its copy in a single shared helper that the report, the LHN preview, copy to clipboard and the thread title all call, so the message is localized on every surface instead of falling back to the English text the backend writes. Co-authored-by: dmkt9 <dmkt9@users.noreply.github.com>
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
This comment has been minimized.
This comment has been minimized.
|
@MelvinBot apply these translations to fix the TS failure #100781 (comment) |
…pply oxfmt Co-authored-by: dmkt9 <dmkt9@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
|
🤖 Android test steps verification results: ✅ pass The PR's
Two things worth flagging:
|
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
Fixed and pushed as What was wrong: the PR body assumed What I changed:
Checks now:
No other check is failing. Local verification
One correction to the PR body, since it will otherwise mislead the reviewer: the bullet saying "Only Two things I did not touch, both worth a human eye:
|
|
Already done — no push needed from this run. Those translations landed in
One thing to be aware of so it isn't mistaken for an outstanding failure: Polyglot Parrot ran again after that push and posted a second diff that rewords the same keys rather than adding missing ones — for example |
|
@MelvinBot apply these translations |
… rate keys Applies the translation diff from the second Polyglot Parrot run verbatim across 9 locale files. fr is unchanged by that run. Co-authored-by: dmkt9 <dmkt9@users.noreply.github.com>
|
Applied in Every one of the 9 resulting blob hashes matches the Checks run
The generated locale files are excluded from ESLint and cspell by config, so those two checks cover the hand-written files on the branch rather than the 9 files changed here. Two notes on the diff itself, both from the parrot and applied as-is:
|
|
No fix needed this time — the checks are already green on The three failures I fixed earlier (
That last one is the only red check, and it clears when a reviewer approves. One thing to be aware of before you review. A third commit, The two items I flagged last time still stand: the PR body bullet saying "Only |
| // The rate is stored in the CONST.POLICY.CUSTOM_UNIT_RATE_BASE_OFFSET scale, which is the scale convertAmountToDisplayString divides by, so 67 is displayed as $0.67. | ||
| const formattedRate = `${convertAmountToDisplayString(rate, currency)} / ${unit}`; | ||
| // The workspace name is interpolated into a message that is rendered as HTML, so encode it to prevent a name containing markup from being parsed as HTML. | ||
| const encodedPolicyName = Str.htmlEncode(policyName); |
There was a problem hiding this comment.
@MelvinBot policyName is not allowed to contain HTML characters, so encoding isn't necessary.
There was a problem hiding this comment.
Agreed — removed the encoding in c182433.
It also brings this helper in line with how the other report action helpers treat a policyName that comes off originalMessage. getWorkspaceDemotedMessage interpolates it straight into the message with no encoding, and so does getPolicyChangeMessage. The one place that does encode a workspace name is getPolicyChangeLogCopyMessage, where the name goes inside an <a> tag — not the case here.
I also dropped the unit test that asserted the escaping, since it was testing the behavior that just went away. Str is still imported for other uses in the file.
Checks after the change: npm run lint clean on both changed files, npm run typecheck fully passing (the generated locale files now have the new keys, so the failure noted in the PR description is resolved), and npm test tests/unit/ReportActionsUtilsTest.ts at 419 passed.
view run · no recording available
policyName is not allowed to contain HTML characters, so encoding it is unnecessary. This also matches how the other report action helpers interpolate a policyName from originalMessage, such as getWorkspaceDemotedMessage. Removes the test that asserted the encoding behavior. Co-authored-by: dmkt9 <dmkt9@users.noreply.github.com>
|
@codex review |
|
@MelvinBot Since the BE doesn't support this feature yet, let's create some mock test data for me to test it on my side in a real browser |
![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: c182433b67
ℹ️ 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".
| return translate('iou.conciergeAutoSelectedDistanceRateForMovedReport', {rate: formattedRate, policyName}); | ||
| } | ||
|
|
||
| return translate('iou.conciergeAutoSelectedDistanceRate', {rate: formattedRate, policyName}); |
There was a problem hiding this comment.
When a workspace name contains markup, such as <strong>Ops</strong>, both translation paths interpolate it unchanged into a string that ActionContentRouter passes to RenderHTML; the report therefore interprets the name as markup, while Parser.htmlToText removes the tags from previews and thread titles. This input is valid because WorkspaceNamePage only checks that names are nonempty and within the length limit, and the sibling policy-copy helper explicitly HTML-encodes policy names for this reason. Preserve a plain-text form for previews/copy and encode policyName for the HTML-rendered form.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@MelvinBot verify this #100781 (comment). I remember that allowHTML === false for policyName
There was a problem hiding this comment.
You're right about allowHTML, and I verified it both ways — but it doesn't fully cover this case, and my earlier reasoning in c182433 was wrong.
Your recall is correct. WorkspaceNamePage renders FormProvider with no allowHTML prop, and it defaults to false at FormProvider.tsx:142, which runs the HTML-tag check at FormProvider.tsx:211-252. I confirmed it in the browser too — <strong>Ops</strong> is rejected with Invalid character and does not save, so the bot's specific example is not a valid input.
Two things it doesn't cover, though:
<br>is explicitly whitelisted.CONST.WHITELISTED_TAGSis[/<>/, /< >/, /<->/, /<-->/, /<br>/, /<br\/>/], soOps<br>Teampasses validation.<br>is a tagRenderHTMLactually renders.- Not every workspace name goes through a form.
App.ts:891readspolicyNamestraight off the OldDot transition URL's query params and passes it tocreateWorkspaceatApp.ts:902with no validation. AndoriginalMessage.policyNameon this action is written by Auth, not by NewDot — so the form is not the boundary for this value at all.
Correcting myself: the parity argument I gave when I removed the encoding doesn't hold. The helpers I cited render through plain <Text> via ReportActionItemBasicMessage.tsx:37 — no HTML parsing, so they genuinely don't need encoding. This helper is different: its output goes into RenderHTML at ActionContentRouter.tsx:366 and is returned as an html fragment at ReportActionsUtils.ts:2513. The rule across the codebase is consistent once you sort by sink: encode when the string reaches RenderHTML, don't when it reaches <Text>. getPolicyChangeLogCopyMessage encodes for exactly that stated reason.
Other workspace-name write paths I checked
Through FormProvider with allowHTML unset (validated):
- Workspace rename —
src/pages/workspace/WorkspaceNamePage.tsx:82-92 - New workspace confirmation —
src/components/WorkspaceConfirmationForm.tsx:212-233 - Duplicate workspace —
src/pages/workspace/duplicate/WorkspaceDuplicateForm.tsx:123-136
All three validate only isRequiredFulfilled + CONST.TITLE_CHARACTER_LIMIT; the HTML check comes solely from FormProvider.
Bypassing the form (name generated or externally supplied):
- OldDot transition URL —
src/libs/actions/App.ts:891,:902 - Onboarding / track / submit auto-create and the KYC-wall pay flow all use
generateDefaultWorkspaceName(src/libs/actions/Policy/Policy.ts:2492-2509), which builds the name from the server-providedpersonalDetails.displayName.
I did not check whether Auth or Web-Expensify reject HTML in CreateWorkspace / UpdateWorkspaceGeneralSettings — neither repo is checked out here. If the backend does strip markup, the practical exposure narrows to the <br> whitelist case.
Separately, getMovedActionMessage interpolates an unencoded toPolicyName into an <a href> that RenderHTML renders. Same class, already on main — not this PR's problem, just noting it.
The practical impact is cosmetic (a stray line break in the message and in the LHN preview), not an XSS — RenderHTML only renders a fixed tag set. So this is your call: re-adding Str.htmlEncode(policyName) is one line and matches how the other RenderHTML-bound helper does it, or leaving it is defensible if you consider <br> in a workspace name not worth guarding. Tell me which and I'll make the change.
view run · no recording available
There was a problem hiding this comment.
Re-added the encoding in 2815d3f. Your recall about allowHTML was right, but it isn't the deciding factor here — the sinks are.
Why encode: every consumer of this helper treats its return value as HTML. Four of them run it back through Parser.htmlToText, which is the codebase stating the contract explicitly:
| Consumer | Treats it as |
|---|---|
ActionContentRouter.tsx:366 |
RenderHTML |
ReportActionsUtils.ts:2513 |
html fragment + Parser.htmlToText for text |
ReportAlternateTextUtils.ts:762, :1493 |
Parser.htmlToText |
ReportNameUtils.ts:878 |
Parser.htmlToText |
ContextMenuActions.tsx:1365 |
Clipboard.setHtml + Parser.htmlToText |
policyName comes off originalMessage, which Auth writes — so the NewDot form is not the trust boundary for this value at all, whatever it validates.
Encoding is free here. htmlToText ends in Str.htmlDecode (ExpensiMark.js:1596), so all four plain-text surfaces decode straight back to the name as typed. That's why the existing tests — which already assert through Parser.htmlToText — passed unchanged.
The concrete bug without it: htmlToText's first rule rewrites <br[^>]*> to \n, so a workspace name Ops<br>Team injects a newline into the single-line LHN preview, and RenderHTML breaks the line in the report body. <br> survives form validation because CONST.WHITELISTED_TAGS whitelists it and FormProvider.tsx:234 allows anything it matches. Added a regression test covering both directions.
Correcting my earlier reasoning in c182433: the parity argument was wrong. getWorkspaceDemotedMessage and getPolicyChangeMessage render through plain <Text>, so they genuinely need no encoding. Sorted by sink, the codebase is consistent — encode for RenderHTML, don't for <Text> — and getPolicyChangeLogCopyMessage encodes for exactly this reason.
Impact is cosmetic, not XSS — RenderHTML only renders a fixed tag set.
Checks
| Check | Result |
|---|---|
npx eslint on both changed files |
✅ no new violations — the only hit in the helper is the pre-existing no-unsafe-type-assertion on the getOriginalMessage cast, which fires identically on the sibling helpers at :3444, :3454, :3463 |
npm run typecheck |
✅ fully passing |
npm test tests/unit/ReportActionsUtilsTest.ts |
✅ 420 passed (1 new) |
npm test — ReportAlternateTextUtils, ReportAlternateTextParity, ReportNameUtils, ContextMenuActionsCopyMessage |
✅ 345 passed, 145 snapshots |
| cspell on both changed files | ✅ clean |
npm run lint-changed and npm run spell-changed both abort in this environment with git merge-base failed (shallow clone), so I ran eslint/cspell against the two files directly instead.
I also tried to confirm the <br> acceptance in a browser, but the session rendered a blank page and never recovered, so that claim rests on the two code references above rather than on an observation.
There was a problem hiding this comment.
Re-added the encoding in 2815d3f. Your recall about allowHTML was right, but it isn't the deciding factor here — the sinks are.
Why encode: every consumer of this helper treats its return value as HTML. Four of them run it back through Parser.htmlToText, which is the codebase stating the contract explicitly:
| Consumer | Treats it as |
|---|---|
ActionContentRouter.tsx:366 |
RenderHTML |
ReportActionsUtils.ts:2513 |
html fragment + Parser.htmlToText for text |
ReportAlternateTextUtils.ts:762, :1493 |
Parser.htmlToText |
ReportNameUtils.ts:878 |
Parser.htmlToText |
ContextMenuActions.tsx:1365 |
Clipboard.setHtml + Parser.htmlToText |
policyName comes off originalMessage, which Auth writes — so the NewDot form is not the trust boundary for this value at all, whatever it validates.
Encoding is free here. htmlToText ends in Str.htmlDecode (ExpensiMark.js:1596), so all four plain-text surfaces decode straight back to the name as typed. That's why the existing tests — which already assert through Parser.htmlToText — passed unchanged.
The concrete bug without it: htmlToText's first rule rewrites <br[^>]*> to \n, so a workspace name Ops<br>Team injects a newline into the single-line LHN preview, and RenderHTML breaks the line in the report body. <br> survives form validation because CONST.WHITELISTED_TAGS whitelists it and FormProvider.tsx:234 allows anything it matches. Added a regression test covering both directions.
Correcting my earlier reasoning in c182433: the parity argument was wrong. getWorkspaceDemotedMessage and getPolicyChangeMessage render through plain <Text>, so they genuinely need no encoding. Sorted by sink, the codebase is consistent — encode for RenderHTML, don't for <Text> — and getPolicyChangeLogCopyMessage encodes for exactly this reason.
Impact is cosmetic, not XSS — RenderHTML only renders a fixed tag set.
Checks
| Check | Result |
|---|---|
npx eslint on both changed files |
✅ no new violations — the only hit in the helper is the pre-existing no-unsafe-type-assertion on the getOriginalMessage cast, which fires identically on the sibling helpers at :3444, :3454, :3463 |
npm run typecheck |
✅ fully passing |
npm test tests/unit/ReportActionsUtilsTest.ts |
✅ 420 passed (1 new) |
npm test — ReportAlternateTextUtils, ReportAlternateTextParity, ReportNameUtils, ContextMenuActionsCopyMessage |
✅ 345 passed, 145 snapshots |
| cspell on both changed files | ✅ clean |
npm run lint-changed and npm run spell-changed both abort in this environment with git merge-base failed (shallow clone), so I ran eslint/cspell against the two files directly instead.
I also tried to confirm the <br> acceptance in a browser, but the session rendered a blank page and never recovered, so that claim rests on the two code references above rather than on an observation.
There was a problem hiding this comment.
Re-added the encoding in 2815d3f. Your recall about allowHTML was right, but it isn't the deciding factor here — the sinks are.
Why encode: every consumer of this helper treats its return value as HTML. Four of them run it back through Parser.htmlToText, which is the codebase stating the contract explicitly:
| Consumer | Treats it as |
|---|---|
ActionContentRouter.tsx:366 |
RenderHTML |
ReportActionsUtils.ts:2513 |
html fragment + Parser.htmlToText for text |
ReportAlternateTextUtils.ts:762, :1493 |
Parser.htmlToText |
ReportNameUtils.ts:878 |
Parser.htmlToText |
ContextMenuActions.tsx:1365 |
Clipboard.setHtml + Parser.htmlToText |
policyName comes off originalMessage, which Auth writes — so the NewDot form is not the trust boundary for this value at all, whatever it validates.
Encoding is free here. htmlToText ends in Str.htmlDecode (ExpensiMark.js:1596), so all four plain-text surfaces decode straight back to the name as typed. That's why the existing tests — which already assert through Parser.htmlToText — passed unchanged.
The concrete bug without it: htmlToText's first rule rewrites <br[^>]*> to \n, so a workspace name Ops<br>Team injects a newline into the single-line LHN preview, and RenderHTML breaks the line in the report body. <br> survives form validation because CONST.WHITELISTED_TAGS whitelists it and FormProvider.tsx:234 allows anything it matches. Added a regression test covering both directions.
Correcting my earlier reasoning in c182433: the parity argument was wrong. getWorkspaceDemotedMessage and getPolicyChangeMessage render through plain <Text>, so they genuinely need no encoding. Sorted by sink, the codebase is consistent — encode for RenderHTML, don't for <Text> — and getPolicyChangeLogCopyMessage encodes for exactly this reason.
Impact is cosmetic, not XSS — RenderHTML only renders a fixed tag set.
Checks
| Check | Result |
|---|---|
npx eslint on both changed files |
✅ no new violations — the only hit in the helper is the pre-existing no-unsafe-type-assertion on the getOriginalMessage cast, which fires identically on the sibling helpers at :3444, :3454, :3463 |
npm run typecheck |
✅ fully passing |
npm test tests/unit/ReportActionsUtilsTest.ts |
✅ 420 passed (1 new) |
npm test — ReportAlternateTextUtils, ReportAlternateTextParity, ReportNameUtils, ContextMenuActionsCopyMessage |
✅ 345 passed, 145 snapshots |
| cspell on both changed files | ✅ clean |
npm run lint-changed and npm run spell-changed both abort in this environment with git merge-base failed (shallow clone), so I ran eslint/cspell against the two files directly instead.
I also tried to confirm the <br> acceptance in a browser, but the session rendered a blank page and never recovered, so that claim rests on the two code references above rather than on an observation.
There was a problem hiding this comment.
Re-added the encoding in 2815d3f. Your recall about allowHTML was right, but it isn't the deciding factor here — the sinks are.
Why encode: every consumer of this helper treats its return value as HTML. Four of them run it back through Parser.htmlToText, which is the codebase stating the contract explicitly:
| Consumer | Treats it as |
|---|---|
ActionContentRouter.tsx:366 |
RenderHTML |
ReportActionsUtils.ts:2513 |
html fragment + Parser.htmlToText for text |
ReportAlternateTextUtils.ts:762, :1493 |
Parser.htmlToText |
ReportNameUtils.ts:878 |
Parser.htmlToText |
ContextMenuActions.tsx:1365 |
Clipboard.setHtml + Parser.htmlToText |
policyName comes off originalMessage, which Auth writes — so the NewDot form is not the trust boundary for this value at all, whatever it validates.
Encoding is free here. htmlToText ends in Str.htmlDecode (ExpensiMark.js:1596), so all four plain-text surfaces decode straight back to the name as typed. That's why the existing tests — which already assert through Parser.htmlToText — passed unchanged.
The concrete bug without it: htmlToText's first rule rewrites <br[^>]*> to \n, so a workspace name Ops<br>Team injects a newline into the single-line LHN preview, and RenderHTML breaks the line in the report body. <br> survives form validation because CONST.WHITELISTED_TAGS whitelists it and FormProvider.tsx:234 allows anything it matches. Added a regression test covering both directions.
Correcting my earlier reasoning in c182433: the parity argument was wrong. getWorkspaceDemotedMessage and getPolicyChangeMessage render through plain <Text>, so they genuinely need no encoding. Sorted by sink, the codebase is consistent — encode for RenderHTML, don't for <Text> — and getPolicyChangeLogCopyMessage encodes for exactly this reason.
Impact is cosmetic, not XSS — RenderHTML only renders a fixed tag set.
Checks
| Check | Result |
|---|---|
npx eslint on both changed files |
✅ no new violations — the only hit in the helper is the pre-existing no-unsafe-type-assertion on the getOriginalMessage cast, which fires identically on the sibling helpers at :3444, :3454, :3463 |
npm run typecheck |
✅ fully passing |
npm test tests/unit/ReportActionsUtilsTest.ts |
✅ 420 passed (1 new) |
npm test — ReportAlternateTextUtils, ReportAlternateTextParity, ReportNameUtils, ContextMenuActionsCopyMessage |
✅ 345 passed, 145 snapshots |
| cspell on both changed files | ✅ clean |
npm run lint-changed and npm run spell-changed both abort in this environment with git merge-base failed (shallow clone), so I ran eslint/cspell against the two files directly instead.
I also tried to confirm the <br> acceptance in a browser, but the session rendered a blank page and never recovered, so that claim rests on the two code references above rather than on an observation.
There was a problem hiding this comment.
Re-added the encoding in 2815d3f. Your recall about allowHTML was right, but it isn't the deciding factor here — the sinks are.
Why encode: every consumer of this helper treats its return value as HTML. Four of them run it back through Parser.htmlToText, which is the codebase stating the contract explicitly:
| Consumer | Treats it as |
|---|---|
ActionContentRouter.tsx:366 |
RenderHTML |
ReportActionsUtils.ts:2513 |
html fragment + Parser.htmlToText for text |
ReportAlternateTextUtils.ts:762, :1493 |
Parser.htmlToText |
ReportNameUtils.ts:878 |
Parser.htmlToText |
ContextMenuActions.tsx:1365 |
Clipboard.setHtml + Parser.htmlToText |
policyName comes off originalMessage, which Auth writes — so the NewDot form is not the trust boundary for this value at all, whatever it validates.
Encoding is free here. htmlToText ends in Str.htmlDecode (ExpensiMark.js:1596), so all four plain-text surfaces decode straight back to the name as typed. That's why the existing tests — which already assert through Parser.htmlToText — passed unchanged.
The concrete bug without it: htmlToText's first rule rewrites <br[^>]*> to \n, so a workspace name Ops<br>Team injects a newline into the single-line LHN preview, and RenderHTML breaks the line in the report body. <br> survives form validation because CONST.WHITELISTED_TAGS whitelists it and FormProvider.tsx:234 allows anything it matches. Added a regression test covering both directions.
Correcting my earlier reasoning in c182433: the parity argument was wrong. getWorkspaceDemotedMessage and getPolicyChangeMessage render through plain <Text>, so they genuinely need no encoding. Sorted by sink, the codebase is consistent — encode for RenderHTML, don't for <Text> — and getPolicyChangeLogCopyMessage encodes for exactly this reason.
Impact is cosmetic, not XSS — RenderHTML only renders a fixed tag set.
Checks
| Check | Result |
|---|---|
npx eslint on both changed files |
✅ no new violations — the only hit in the helper is the pre-existing no-unsafe-type-assertion on the getOriginalMessage cast, which fires identically on the sibling helpers at :3444, :3454, :3463 |
npm run typecheck |
✅ fully passing |
npm test tests/unit/ReportActionsUtilsTest.ts |
✅ 420 passed (1 new) |
npm test — ReportAlternateTextUtils, ReportAlternateTextParity, ReportNameUtils, ContextMenuActionsCopyMessage |
✅ 345 passed, 145 snapshots |
| cspell on both changed files | ✅ clean |
npm run lint-changed and npm run spell-changed both abort in this environment with git merge-base failed (shallow clone), so I ran eslint/cspell against the two files directly instead.
I also tried to confirm the <br> acceptance in a browser, but the session rendered a blank page and never recovered, so that claim rests on the two code references above rather than on an observation.
view run · no recording available
|
Here's a console snippet that injects a mock I could not verify this in a browser myself — the web prep for this run failed, so I had no way to drive NewDot. The snippet is built from the shapes in the diff, not from a run I watched. The snippetOpen the report you want to test, grab its (async () => {
// ---- edit these ----
const reportID = 'PASTE_REPORT_ID_HERE';
const changeType = 'workspaceChanged'; // or 'reportMoved'
const rate = 67; // 67 renders as $0.67 (CUSTOM_UNIT_RATE_BASE_OFFSET scale)
const currency = 'USD';
const unit = 'mi'; // or 'km'
const policyName = "Hal's Burgers";
// --------------------
const actions = (await window.Onyx.get(`reportActions_${reportID}`)) ?? {};
const newest = Object.values(actions).sort((a, b) => (a?.created < b?.created ? 1 : -1)).at(0);
const reportActionID = `mock-concierge-rate-${Date.now()}`;
const created = new Date().toISOString().replace('T', ' ').replace('Z', '');
// Deliberately WRONG on purpose: this stands in for the English string Auth writes.
// If you see this text anywhere in the UI, the translation path was not used.
const backendText = 'BACKEND FALLBACK - should not be visible';
await window.Onyx.merge(`reportActions_${reportID}`, {
[reportActionID]: {
reportActionID,
actionName: 'CONCIERGEAUTOSELECTDISTANCERATE',
actorAccountID: newest?.actorAccountID,
created,
message: [{type: 'COMMENT', text: backendText, html: backendText}],
originalMessage: {rate, currency, unit, policyName, changeType},
person: [{type: 'TEXT', style: 'strong', text: 'Concierge'}],
automatic: true,
shouldShow: true,
avatar: '',
},
});
await window.Onyx.merge(`report_${reportID}`, {
lastVisibleActionCreated: created,
lastActionType: 'CONCIERGEAUTOSELECTDISTANCERATE',
lastActorAccountID: newest?.actorAccountID,
lastMessageText: backendText,
});
console.log('Injected', reportActionID, '- to remove it, run the cleanup snippet with this ID.');
})();What to expect
The sentinel To exercise the fallback branch ( Cleanupawait window.Onyx.merge(`reportActions_${'PASTE_REPORT_ID_HERE'}`, {'PASTE_ACTION_ID_HERE': null});Two caveats worth knowing before you start: Field shapes come from |
|
@MelvinBot Use the snippet data to create the steps under the Tests section in your checklist. Replace |
|
Done — the The steps are numbered 1–11, wrapping the snippet with the checks the "What to expect" table implied: report body, LHN preview against the I have not run these steps. The web test path here cannot execute console JavaScript — arbitrary page scripting is outside what it supports — so injecting the mock action wasn't possible. The steps are derived from the code, not from a run I watched. Two details I did verify against the branch while writing them:
One correction to the earlier snippet comment: it cited line 3421 for that guard, which is the function declaration. The guard is at 3424. I left the view run · no recording available |
|
@MelvinBot Update your checklist:
|
|
🤖 Checklist updated. All three changes are in the PR description:
I diffed the new body against the old one before applying it and confirmed nothing outside those three changes moved. Two things you should know before this goes up for review:
|
|
@MelvinBot handle this #100781 (comment) |
The string this helper returns is HTML: it goes to RenderHTML in the report and is returned as the html fragment of the action, and every plain-text surface (LHN preview, thread title, clipboard) runs it back through Parser.htmlToText. policyName comes off originalMessage, which Auth writes, so it has to be encoded for the HTML sinks. Encoding round-trips cleanly: htmlToText ends in Str.htmlDecode, so the plain-text surfaces still show the name exactly as typed. Without the encode, a name containing <br> renders as a line break in the report and is turned into a newline in the single-line LHN preview. This reverts c182433, whose parity argument compared this helper against ones that render through plain <Text> and so genuinely need no encoding. Co-authored-by: dmkt9 <dmkt9@users.noreply.github.com>
|
@MelvinBot address this |
…umbers A modified expense whose merchant carries a thousands separator (1,234.56 mi) failed CONST.REGEX.DISTANCE_MERCHANT, so getForDistanceRequest fell through to its else branch and described a rate-only change as a distance change. Co-authored-by: dmkt9 <dmkt9@users.noreply.github.com>
|
Fixed in -DISTANCE_MERCHANT: /^[0-9.]+ \w+ @ (-|-\()?[^0-9.\s]{1,3} ?[0-9.]+\)? \/ \w+$/,
+DISTANCE_MERCHANT: /^[0-9.,]+ \w+ @ (-|-\()?[^0-9.,\s]{1,3} ?[0-9.,]+\)? \/ \w+$/,Your read of the cause is right. Both numeric groups were Worth flagging: this was two bugs sharing a line. Grouped numbers are the one you hit, but the rate also carries a locale decimal separator — Two cases added in On the backend half you suspected — if the server ever writes the old merchant with a different formatter than the new one, the two distances compare unequal as strings and the message still falls back to "distance" even with this fix in. I left that alone since it's outside this repo. AI Tests
No browser run for this one. The message is built from a backend-written |
|
@MelvinBot fix the oxfmt failure |
Co-authored-by: dmkt9 <dmkt9@users.noreply.github.com>
|
Fixed in
The other red check, |
|
@neil-marcellini This PR is ready to go again |
neil-marcellini
left a comment
neil-marcellini
left a comment
There was a problem hiding this comment.
Great. Good to go, thank you.
|
🚧 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! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
Change LGTM 👍 |
|
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 9.4.82-0 🚀
|
|
Yes, help-site changes are required. Draft PR: Docs updates for E/A#100781 distance rate auto-selection on workspace change One article is now wrong, not just missing.
Both halves break under this PR's behavior. The rate is re-selected from the destination workspace automatically, so a member following the old FAQ waits for a violation that never appears. That is the substantive fix in the draft PR — the new answer covers the auto-selection, the Concierge message, and why the message names a workspace but no rate. Two smaller fixes ride along: Important Worth holding until Auth ships. This PR renders the message but does not create it — your own QA Steps say the test steps "can't run on staging until Auth emits the action." Auth still has to post How the copy and labels were verifiedThe Concierge string in the article is quoted from Distance rates, Add rate, Settings, Rate, and Distance were checked against dev NewDot web. Change workspace was confirmed from
Articles checked and left alone: @dmkt9, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
@dmkt9 How do we check and do the following steps, could you help? Re-run the snippet with policyName removed from the originalMessage object. Verify the guard at src/libs/ReportActionsUtils.ts:3423 falls back to the message text, so BACKEND FALLBACK - should not be visible renders instead. |
@kavimuru In the snippet, you need to change |
|
@dmkt9 Thanks, We can't check this I think
|
|
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 9.4.86-0 🚀
|
|
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 9.4.89-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.4.89-0 🚀
Bundle Size Analysis (Sentry): |
8 participants
Explanation of Change
Registers the new
CONCIERGEAUTOSELECTDISTANCERATEConcierge system message and builds its copy from the translations on every surface that shows it.Per the updated plan, the action on the report names no rate. A report can hold many distance expenses, each landing on a different rate when the report's workspace changes, so one rate can't describe the outcome. The per-expense rate changes are described by a
MODIFIEDEXPENSEaction on each transaction thread instead, which also keeps cause and effect in the same report. That action is produced by the backend and rendered by the existing message generation, so it needs no change here.The report's system message
src/CONST/index.ts— Adds the action type, which is what puts it in thesupportedActionTypesallowlist so the action is no longer filtered out before rendering.src/types/onyx/OriginalMessage.ts— Adds theoriginalMessageshape, which is just the destination workspace name.src/languages/en.ts— Adds the copy, plus the ten other locales.src/libs/ReportActionsUtils.ts— AddsgetConciergeAutoSelectDistanceRateMessage, the single source of the string, and returns it as the action's message fragments.src/pages/inbox/report/actionContents/ActionContentRouter.tsx— Renders the message in the report body as a muted system line.src/libs/ReportAlternateTextUtils.ts,src/libs/ReportNameUtils.ts,src/pages/inbox/report/ContextMenu/ContextMenuActions.tsx— Use the helper for the LHN last-message preview, the thread title, and copy to clipboard.Tests —
tests/unit/ReportActionsUtilsTest.tsandtests/unit/ReportAlternateTextUtilsTest.tscover the copy, the missing-data fallback, and the LHN preview.The action name string must match Auth's exactly. Auth still writes its own English
lastMessageText, which is now only a fallback for clients that predate this PR.Note
Two parts of the updated plan are Auth-side and are not in this PR: posting the
MODIFIEDEXPENSEaction on each transaction thread when an expense's workspace changes, and using Concierge as itsactorAccountID. This PR renders the Concierge system message; theMODIFIEDEXPENSEaction renders through the existing path.Fixed Issues
$ #100556
PROPOSAL: #100556 (comment)
Tests
Auth doesn't emit the action yet, so these steps inject a mock
CONCIERGEAUTOSELECTDISTANCERATEreport action from the devtools console.window.Onyxis only exposed on non-production builds (src/setup/addUtilsToWindow.ts:26), so run this against a local dev build. Nothing is committed.The mock deliberately sets
report.lastMessageTextto a wrong sentinel string, standing in for the English text Auth writes. That sentinel is the point of the test: wherever the real copy appears instead, the string was built from the translations rather than from the backend text.reportIDfrom the URL.reportIDinto the snippet below, and run it:distance rates updated for the new workspace - Hal's Burgers.BACKEND FALLBACK - should not be visible. This is the check that the preview is built from the translations rather than from the backend's text.se actualizaron las tasas de distancia para el nuevo espacio de trabajo - Hal's Burgers. Switch back to English.policyNameremoved from theoriginalMessageobject. Verify the guard atsrc/libs/ReportActionsUtils.ts:3423falls back to the message text, soBACKEND FALLBACK - should not be visiblerenders instead.reportActionIDthe snippet logged:Two things to know before you start:
Onyx.mergepersists, so the mock survives a reload until you clean it up; and reopening the report firesOpenReport, which can overwritereport.lastMessageTextwith the server's value — re-run the snippet if the LHN preview reverts.Offline tests
Same as Tests.
QA Steps
None. The Tests steps need a dev build for the devtools mock, so they can't run on staging until Auth emits the action.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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.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