refactor: isolates renamePolicyTag from Onyx.connect data#71909
Conversation
…x-connect/renamePolicyTag-src-libs-actions-Policy-Tag.ts # Conflicts: # tests/actions/PolicyTagTest.ts
Codecov Report✅ All modified and coverable lines are covered by tests. |
Skalakid
left a comment
Skalakid
left a comment
There was a problem hiding this comment.
LGTM! Left some minor code formatting suggestions to be consistent
…x-connect/renamePolicyTag-src-libs-actions-Policy-Tag.ts # Conflicts: # src/libs/actions/Policy/Tag.ts
melvin-bot
bot
requested review from
shubham1206agra
and removed request for
a team
|
@shubham1206agra 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/VideosScreen.Recording.2025-11-01.at.5.30.38.PM.mov |
| if (currentTagName !== tagName) { | ||
| renamePolicyTag(policyID, {oldName: route.params.tagName, newName: values.tagName.trim()}, route.params.orderWeight); | ||
| try { | ||
| if (!policy) { |
There was a problem hiding this comment.
How come you've added this try/catch and error logic which didn't exist before? How is it possible that there couldn't be a policy, and if that's the case, it would mean you've probably lost access to the policyID in the URL. For that, wouldn't we rather show a "not found" page instead of letting the user edit the tags and then do this error handling?
There was a problem hiding this comment.
From a TypeScript perspective, policy has the type Policy | undefined here. This is the type returned by useOnyx, and there's no way to narrow it without using an if.
Calling renamePolicyTag with policy == undefined doesn't make sense because nothing will happen, and I think it would be a good idea to log this to some system as an error.
In the current code, this will never happen, because this page has the AccessOrNotFoundWrapper as a child component.
However, if someone accidentally corrupts the AccessOrNotFoundWrapper configuration or calls editTag before the AccessOrNotFoundWrapper has rendered, we might get policy == undefined.
Currently, using this additional if is necessary to narrow down the TS types.
There was a problem hiding this comment.
The current AccessOrNotFoundWrapper implementation doesn’t influence TypeScript types in surrounding functions, which often leads to unsafe or inconsistent type behavior.
I'm considering drafting a proposal to update this implementation in a way that would enable safer TypeScript type restrictions. I'm still gathering supporting arguments for this idea.
Specifically, I’ve been exploring an approach inspired by the Next.js framework. It uses a dedicated NotFoundErrorBoundary that only catches NotFoundError instances. This pattern makes it possible to throw errors like NotFoundError("Policy is required.") from any child component while maintaining strong type safety.
If this sounds like a good direction, I can prioritize writing up the proposal.
import React from "react";
import { useErrorBoundary } from "react-error-boundary";
import { useOnyx } from "react-native-onyx";
// assume these are implemented elsewhere
import { NotFoundError } from "./errors";
import { NotFoundErrorBoundary } from "./NotFoundErrorBoundary";
// PARENT
export function Page() {
return (
<NotFoundErrorBoundary>
<PolicySection />
<AsyncSection />
</NotFoundErrorBoundary>
);
}
// CHILD 1 – synchronous check using Onyx data
function PolicySection() {
const [policy] = useOnyx("policy");
if (!policy) {
throw new NotFoundError("Policy is required.");
}
return <div>Policy: {policy.id}</div>;
}
// CHILD 2 – async case, calling showBoundary
function AsyncSection() {
const { showBoundary } = useErrorBoundary();
React.useEffect(() => {
let cancelled = false;
(async () => {
const result = await fetch("/api/something").then((r) => r.json());
if (!result && !cancelled) {
showBoundary(new NotFoundError("Async resource is missing."));
}
})();
return () => {
cancelled = true;
};
}, [showBoundary]);
return <div>Loading async data…</div>;There was a problem hiding this comment.
OK, thanks for that information.
Calling renamePolicyTag with policy == undefined doesn't make sense because nothing will happen, and I think it would be a good idea to log this to some system as an error.
In the current code, this will never happen, because this page has the AccessOrNotFoundWrapper as a child component.
I think because of this, the additional try/catch and if statements are unnecessary. I agree with what you said in theory, but in practice, it doesn't matter since it would never happen in the current code. Can you please remove the extra logic added here?
I would support a broader proposal to fix this more holistically across the app, because I am guessing there are probably hundreds (if not thousands) of problems like this across the code. Having a specific error boundary like you suggested I think makes sense, but I wouldn't prioritize it since it doesn't seem to be a problem.
There was a problem hiding this comment.
bump @dariusz-biela on removing that logic. Can you get to this soon so we can merge this?
There was a problem hiding this comment.
@Skalakid or @shubham1206agra do one of you want to take over this PR?
There was a problem hiding this comment.
I'm fixing it now.
I had a long weekend because yesterday was a holiday in Poland.
Sorry I didn't reply to you earlier, but I haven't been checking GitHub in my free time.
melvin-bot
bot
requested review from
JmillsExpensify
and removed request for
a team
|
I am fixing Eslint errors now |
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
Not relevant for product PR review. Unsubscribing.
…x-connect/renamePolicyTag-src-libs-actions-Policy-Tag.ts Merged main into current branch, keeping main's implementation. Changes: - Kept main's implementation of renamePolicyTag with PolicyData parameter - Kept main's implementation of EditTagPage using usePolicyData hook - Kept main's test file structure Test refactors were not preserved in favor of main's implementation.
…x-connect/renamePolicyTag-src-libs-actions-Policy-Tag.ts
|
This PR is merged and fixed the Onyx.connect issue: I commented on that PR that the potentially incorrect TS type was merged there, as it doesn't account for the possibility that policy could be undefined: In my current PR, I'm leaving the changes to the unit tests alone. |
|
🚀 Deployed to staging by https://github.com/tgolen in version: 9.2.59-0 🚀
|
|
🚀 Deployed to production by https://github.com/grgia in version: 9.2.59-5 🚀
|
6 participants
Explanation of Change
This PR is part of a refactor to remove
Onyx.connectfrom thesrc/libs/actions/Policy/Tag.tsfile and replace it withuseOnyx.It isolates the
renamePolicyTagfunction from theOnyx.connectdata.To ensure this refactor doesn't break anything, it adds automated tests to the
renamePolicyTagfunction.Fixed Issues
This PR partially addresses these issues:
$ #69022
Tests
Prerequisites:
Tag should change it's name.
Verify that no errors appear in the JS console
Offline tests
Same as in the Tests section
QA Steps
Same as in the Tests section
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
Screen.Recording.2025-10-07.at.11.52.49.mov