fix: White screen after deleting a tag#340
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe tree rebuild logic in ChangesTag Tree Rebuild Fix
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@katsyuta could you update pull request description to explain in one paragraph what is the root cause of the problem and how this changes fix that problem. |
|
@vitonsky, description is ready |
| const prevTagsRef = useRef(tags); | ||
| if (prevTagsRef.current !== tags) { | ||
| prevTagsRef.current = tags; | ||
|
|
There was a problem hiding this comment.
How it does work?
- How to trigger first tree build? At first render this condition will not be passed
- Why not to use
useMemo? - Originally the deps is
[tags, tree], why we depend ontagsonly now?
There was a problem hiding this comment.
1. tree.rebuildTree() is only needed when the source data changes. The initial tree build is triggered inside useTree with the actual data
2. I don't now how useMemo would help here, the only requirement is to ensure that rebuild is called after tags change and before child components are rendered
3. As i understand, tree was in the dependencies because it was just an esLint requirement. tree is a stable instance created by useTree, the rebuild is only required when the data is changed
There was a problem hiding this comment.
Are you sure rebuildTree can be called while rendering?
In case that call would start another render we may have problem, because such logic must not be run while rendering. Why this logic is wrapped via useEffect for now? Maybe just to run it after rendering, i don't know. Check it.
I don't now how useMemo would help here, the only requirement is to ensure that rebuild is called after tags change and before child components are rendered
React components must looks declarative. Don't write imperative code in component body. Invent some hook for that logic or search for NPM.
There was a problem hiding this comment.
Calling rebuildTree trigger a re-render, but with the guard it is safe - no infinite loop.
When state updates during render, React re-renders that same component immediately, before rendering its children, within the same render cycle.
I wrap this logic to hook
There was a problem hiding this comment.
@katsyuta as I understand, the re-render call while run another render is unsafe and can throw errors by itself.
The problem is not in infinite loop. I saw errors about that, but I don't remember exactly why that's bad. Research that
There was a problem hiding this comment.
In the react docs, they write about the pattern that i suggested.
If the state belonging to the component itself is updated, no error will occur. Under the hood, useTree uses a state to trigger a render, so when rebuildTree is called we update the state owned by the component itself
As i understand, the error can occur if update a different component while rendering. I found a some issue about this.
2 participants
Closes: #339
Problem:
After deleting a tag the tags list in Redux is updated, triggering a new render. But the tags tree is rebuilt later in
useEffect, soVirtualTagsListrenders using an old tree that still contains the deleted tag. As a result, the component tries to load data for the deleted item by looking up its id in the tags list, but the tag not exists, so the lookup returnsundefinedSolution:
Rebuild the tags tree before rendering the child component so it always renders with the latest tree state
Summary by CodeRabbit
Summary by CodeRabbit