83836 migrate expensify cards settings#89395
Conversation
|
@Beamanator 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] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| ? createDynamicRoute(DYNAMIC_ROUTES.EXPENSIFY_CARD_DETAILS.getRoute(String(cardID)), ROUTES.WORKSPACE_EXPENSIFY_CARD.getRoute(policyID)) | ||
| : ROUTES.SETTINGS_DOMAIN_CARD_DETAIL.getRoute(String(cardID)); | ||
|
|
||
| console.log('****** shouldNavigateToCardDetails ******', shouldNavigateToCardDetails); |
There was a problem hiding this comment.
❌ CONSISTENCY-2 (docs)
Three console.log statements with debug markers ('****** shouldNavigateToCardDetails ******', etc.) have been left in the production code. These appear to be debugging leftovers and should be removed before merging.
Remove the three console.log lines:
- console.log('****** shouldNavigateToCardDetails ******', shouldNavigateToCardDetails);
- console.log('****** navigateRoute ******', navigateRoute);
- console.log('****** ${environmentURL}/${navigateRoute} ******', `${environmentURL}/${navigateRoute}`);Reviewed at: c49da09 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
| @@ -10,6 +10,8 @@ const oldRoutes: Record<string, string> = { | |||
| '/workspaces/*/connections/quickbooks-online/advanced/autosync/accounting-method': | |||
| '/workspaces/$1/accounting/quickbooks-online/advanced/quickbooks-online-autosync/quickbooks-online-accounting-method', | |||
| '/workspaces/*/connections/quickbooks-online/advanced/autosync': '/workspaces/$1/accounting/quickbooks-online/advanced/quickbooks-online-autosync', | |||
There was a problem hiding this comment.
❌ CONSISTENCY-6 (docs)
The old route patterns use $1 as a literal in the key instead of * (the wildcard). Looking at patternToRegex, only * is treated as a wildcard that creates capture groups. Using $1 in the key means the pattern will only match URLs containing the literal string $1, not actual cardIDs.
For example, '/workspaces/*/expensify-card/$1' would match /workspaces/abc/expensify-card/$1 literally, but NOT /workspaces/abc/expensify-card/123.
Fix both entries to use * as the second wildcard:
- '/workspaces/*/expensify-card/$1': '/workspaces/$1/expensify-card/details/$2',
- '/settings/*/expensify-card/$1': '/workspaces/$1/expensify-card/details/$2',
+ '/workspaces/*/expensify-card/*': '/workspaces/$1/expensify-card/details/$2',
+ '/settings/*/expensify-card/*': '/workspaces/$1/expensify-card/details/$2',Reviewed at: c49da09 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
…sify-cards-settings
![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: c49da097fe
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| '/workspaces/*/expensify-card/$1': '/workspaces/$1/expensify-card/details/$2', | ||
| '/settings/*/expensify-card/$1': '/workspaces/$1/expensify-card/details/$2', |
There was a problem hiding this comment.
getMatchingNewRoute() only treats * as a capture wildcard (via patternToRegex), but these new keys include a literal $1 segment in the pattern. That means legacy URLs like /workspaces/<policyID>/expensify-card/<cardID> and /settings/<policyID>/expensify-card/<cardID> will not match and therefore will not be migrated to the new /details/:cardID shape, breaking existing deep links/bookmarks that this change is meant to preserve.
Useful? React with 👍 / 👎.
| console.log('****** shouldNavigateToCardDetails ******', shouldNavigateToCardDetails); | ||
| console.log('****** navigateRoute ******', navigateRoute); | ||
| console.log('****** ${environmentURL}/${navigateRoute} ******', `${environmentURL}/${navigateRoute}`); |
There was a problem hiding this comment.
These console.log calls execute whenever card-issued report actions are rendered, which can happen frequently in chat/report UIs. Leaving them in will spam client logs and expose generated navigation URLs in production diagnostics, adding noise and leaking internal routing details without user value.
Useful? React with 👍 / 👎.
|
no review needed here yet, right? |
…sify-cards-settings
Yes, this is a migration task, so you can skip it. Sorry for the confusion |
2 participants
Explanation of Change
Fixed Issues
$
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari