83862 migrate workspace tags settings#86847
Conversation
melvin-bot
bot
requested review from
mananjadhav and
trjExpensify
and removed request for
a team
|
@mananjadhav 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] |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@collectioneur I’m running into an issue after editing a tag. App/src/pages/workspace/tags/TagSettingsPage.tsx Lines 69 to 74 in 0e974a4 Screen.Recording.2026-04-01.at.15.21.22.movHowever, this changes the URL to: |
|
I’ve updated Specifically, Could you please review it? If possible, could you add this logic to your PR? |
|
|
||
| for (const [key, value] of Object.entries(params)) { | ||
| if (value) { | ||
| if (value !== undefined && value !== '') { |
There was a problem hiding this comment.
I updated this to handle the case where the value is 0.
![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: 0e974a4ad2
ℹ️ 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".
| isQuickSettingsFlow | ||
| ? ROUTES.SETTINGS_TAG_EDIT.getRoute(policyID, orderWeight, currentPolicyTag.name, backTo) | ||
| : ROUTES.WORKSPACE_TAG_EDIT.getRoute(policyID, orderWeight, currentPolicyTag.name), | ||
| isQuickSettingsFlow ? createDynamicRoute(DYNAMIC_ROUTES.SETTINGS_TAG_EDIT.path) : ROUTES.WORKSPACE_TAG_EDIT.getRoute(policyID, orderWeight, currentPolicyTag.name), |
There was a problem hiding this comment.
In quick settings flow this navigation now appends tag-edit to the current dynamic URL instead of building the route from currentPolicyTag.name. After a rename, this page updates only route.params (navigation.setParams) while the stored dynamic path can still contain the old tagName, so opening edit again can target a stale/deleted tag and fail to rename the actual current tag. The previous route construction avoided this by always using the latest tag name.
Useful? React with 👍 / 👎.
| Navigation.goBack( | ||
| isQuickSettingsFlow ? ROUTES.SETTINGS_TAG_SETTINGS.getRoute(policyID, orderWeight, tagName, backTo) : ROUTES.WORKSPACE_TAG_SETTINGS.getRoute(policyID, orderWeight, tagName), | ||
| ); | ||
| Navigation.goBack(isQuickSettingsFlow ? undefined : ROUTES.WORKSPACE_TAG_SETTINGS.getRoute(policyID, orderWeight, tagName)); |
There was a problem hiding this comment.
For quick settings, goBack now passes undefined, which depends on in-memory history. If the approver page is opened via refresh or direct deep link (no prior stack entry), Navigation.goBack() cannot pop and the user cannot return to tag settings from the header/save callback. Keeping an explicit fallback route here avoids trapping users on this screen in those entry paths.
Useful? React with 👍 / 👎.
|
|
||
| const goBack = () => { | ||
| Navigation.goBack( | ||
| isQuickSettingsFlow ? ROUTES.SETTINGS_TAG_SETTINGS.getRoute(policyID, orderWeight, tagName, backTo) : ROUTES.WORKSPACE_TAG_SETTINGS.getRoute(policyID, orderWeight, tagName), |
There was a problem hiding this comment.
ROUTES.SETTINGS_TAG_SETTINGS.getRoute(policyID, orderWeight, tagName, backTo)
I migrated the SETTINGS_TAG_SETTINGS route path. However, other pages still rely on this route for navigation (e.g. going back).
In this case, how should we navigate to SETTINGS_TAG_SETTINGS? We can’t create a new dynamic route for it.
There was a problem hiding this comment.
I think our best approach here might be to use the backPath from useDynamicBackPath instead of undefined. It feels a lot more intuitive (we just trim the suffix and go back to the previous page, even if it’s not settings-tag-settings). I feel like this is a pretty reasonable tradeoff. Otherwise, we'd have to add even more custom logic that probably wouldn't be useful outside of this specific case anyway 🙂
There was a problem hiding this comment.
Screen.Recording.2026-04-02.at.08.40.08.mov
We can’t rely on backPath here because it resolves to a different route.
How should we handle navigating back to the tag settings screen in this case?
Should we explicitly navigate to the route, for example:
Navigation.goBack(
isQuickSettingsFlow
? ROUTES.SETTINGS_TAGS_ROOT + '/' + DYNAMIC_ROUTES.SETTINGS_TAG_SETTINGS.getRoute(policyID, orderWeight, tagName)
: ROUTES.WORKSPACE_TAG_SETTINGS.getRoute(policyID, orderWeight, tagName)
);
There was a problem hiding this comment.
I think our best approach here might be to use the backPath from useDynamicBackPath instead of undefined
#86934 yes
|
PR doesn’t need product input as a refactor PR. Unassigning and unsubscribing myself. |
|
@huult Taking a look right now! 👀 |
|
@collectioneur thank you ❤️ |
I think the changes in Oh, and by the way, I encountered a bug while testing this PR. I'm not sure if it's because of your updates, but could you check it, please? Screen.Recording.2026-04-01.at.16.56.29.mov |
…pace-tags-settings
@collectioneur #86847 (comment) This issue comes from this comment. Could you please check it? |
|
@huult I double-checked a few things. As for the Would you mind putting a pin in this PR for just a little bit? Feel free to shift your focus to other PRs in the meantime while I get this sorted out. Thanks so much! 😄 |
|
Yes, I will work on other things while you’re doing this. Many thanks. |
3 participants
Explanation of Change
Fixed Issues
$ #83862
PROPOSAL:
Tests
Same QA step
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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