Open
Conversation
loitly
approved these changes
Feb 5, 2026
Contributor
loitly
left a comment
There was a problem hiding this comment.
I didn't test all of the applications, but the ones I tested are working as expected. I am glad you found and fixed the issue in TAP panel. It’s reassuring to know this was a real bug rather than a false positive from the React compiler. I am fine with merging and monitoring it closely.
Comment on lines
+98
to
+102
| setCapabilities(getLoadedCapability(serviceUrl)); | ||
| if (!isCapabilityLoaded(serviceUrl)) { | ||
| loadTapCapabilities(serviceUrl) | ||
| .then((c) => setCapabilitiesChange(c??{})) | ||
| .catch( (error) => { | ||
| setError(`Fail to retrieve capability for: ${serviceUrl}`); | ||
| }); | ||
| .then((c) => setCapabilities(c ?? getLoadedCapability(serviceUrl))) | ||
| .catch(() => setError(`Fail to retrieve capability for: ${serviceUrl}`)); |
Contributor
There was a problem hiding this comment.
Should it be like this so that it doesn't setState twice when not loaded?
if (isCapabilityLoaded(serviceUrl)) {
setCapabilities(getLoadedCapability(serviceUrl));
} else {
loadTapCapabilities(serviceUrl)
.then((c) => setCapabilities(c ?? getLoadedCapability(serviceUrl)))
.catch(() => setError(`Fail to retrieve capability for: ${serviceUrl}`));
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ticket: FIREFLY-1933
React DevTools->Componentsand any components that haveMemo :sparkles:(sparkles is the emoji with 3 stars) next to them have been memoized by React Compiler.Metaand I think evenGithub(inspect & seecomponentsin React DevTools while ongithub.com)useMemoanduseCallbackas is (removing them carefully, over time, is optional).as an escape hatch to provide control over which values are memoizedaccording to react)eslint.The bug I encountered in TAP and why React Compiler still chose to memoize this component:
BasicUIcomponent (inTapViewType) was giving me issues after setting up React Compiler. After it applied memoization, a part of the component was only renderingSkeleton.const capabilities = getLoadedCapability(serviceUrl);capabilitiesis not a react state here. Pre-memoization of this component, we had a number of incidental re-renders that eventually madecapabilitieshave atruthyvalue.capabilitieswas eventually populated with the call togetLoadedCapability, this would not cause BasicUI component to re-render!capabilitiesinto a react state (my code change) fixes this bug.falsyis not technically breaking any Rules of React (react compiler is only concerned with React props/state/context, and not global values, etc.)capabilitiesbeingfalsydid lead to this being false:{(capabilities && columnsModel) ?<Skeleton/>instead, which is a UI bug, yes but not technically a bug React compiler would catch).If we find other components with similar issues, they will need to be handled manually. But I am happy to report I didn't come across any more in my testing. If we do happen to find several issues in testing, we could still keep React Compiler, but adopt incremental adoption instead (https://react.dev/learn/react-compiler/incremental-adoption)
Note: I may change/remove/add some of the
eslintrules.Testing: