fix issue #1625 #1702
fix issue #1625 #1702
Conversation
WalkthroughThe pull request introduces changes to the A utility function These changes streamline the code's structure and improve the management of layout-related state and timeout handling. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/layout/lib/layoutModelHooks.ts(3 hunks)
🔇 Additional comments (2)
frontend/layout/lib/layoutModelHooks.ts (2)
20-25: Clean refactoring using computeIfAbsent!The refactoring improves code readability while maintaining the same functionality and proper error handling.
14-14: Consider memory management for the global timeoutMap.The global
timeoutMapcould potentially lead to memory leaks if timeouts aren't properly cleaned up when components unmount or when blocks are deleted. Consider adding cleanup logic to remove stale entries.Let's verify if there are any block deletion or cleanup events that should clear the timeouts:
| const timeout = setTimeout(() => { | ||
| setInnerRect(nodeInnerRect); | ||
| }, animationTimeS * 1000); | ||
| computeIfAbsent(timeoutMap, nodeModel.blockId, (_) => timeout) | ||
| }, | ||
| [animationTimeS] |
There was a problem hiding this comment.
Update dependency array for setInnerRectDebounced.
The callback depends on clearInnerRectDebounce and nodeModel.blockId but they're not included in the dependency array.
Add missing dependencies:
- [animationTimeS]
+ [animationTimeS, clearInnerRectDebounce, nodeModel.blockId]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const timeout = setTimeout(() => { | |
| setInnerRect(nodeInnerRect); | |
| }, animationTimeS * 1000); | |
| computeIfAbsent(timeoutMap, nodeModel.blockId, (_) => timeout) | |
| }, | |
| [animationTimeS] | |
| const timeout = setTimeout(() => { | |
| setInnerRect(nodeInnerRect); | |
| }, animationTimeS * 1000); | |
| computeIfAbsent(timeoutMap, nodeModel.blockId, (_) => timeout) | |
| }, | |
| [animationTimeS, clearInnerRectDebounce, nodeModel.blockId] |
| const timeout = setTimeout(() => { | ||
| setInnerRect(nodeInnerRect); | ||
| }, animationTimeS * 1000); | ||
| computeIfAbsent(timeoutMap, nodeModel.blockId, (_) => timeout) |
There was a problem hiding this comment.
Fix incorrect usage of computeIfAbsent for timeouts.
The current implementation won't update existing timeouts because computeIfAbsent only sets the value if the key doesn't exist. This could lead to stale timeouts.
Replace with direct map setting:
- computeIfAbsent(timeoutMap, nodeModel.blockId, (_) => timeout)
+ timeoutMap.set(nodeModel.blockId, timeout);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const timeout = setTimeout(() => { | |
| setInnerRect(nodeInnerRect); | |
| }, animationTimeS * 1000); | |
| computeIfAbsent(timeoutMap, nodeModel.blockId, (_) => timeout) | |
| const timeout = setTimeout(() => { | |
| setInnerRect(nodeInnerRect); | |
| }, animationTimeS * 1000); | |
| timeoutMap.set(nodeModel.blockId, timeout); |
| const clearInnerRectDebounce = function () { | ||
| if (timeoutMap.has(nodeModel.blockId)) { | ||
| const innerRectDebounceTimeout = timeoutMap.get(nodeModel.blockId); | ||
| if (innerRectDebounceTimeout) { | ||
| clearTimeout(innerRectDebounceTimeout); | ||
| } | ||
| timeoutMap.delete(nodeModel.blockId); | ||
| } | ||
| }, [innerRectDebounceTimeout]); | ||
| }; |
There was a problem hiding this comment.
Add cleanup on component unmount.
The timeout needs to be cleared when the component unmounts to prevent memory leaks and potential state updates on unmounted components.
Add a cleanup effect:
+ useEffect(() => {
+ return () => clearInnerRectDebounce();
+ }, []);Committable suggestion skipped: line range outside the PR's diff.
| function computeIfAbsent<V, F>(map: Map<V, F>, key: V, mappingFunction: (a: V) => F): F { | ||
| if (!map.has(key)) { | ||
| const newValue = mappingFunction(key); | ||
| map.set(key, newValue); | ||
| } | ||
| return map.get(key); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and documentation of computeIfAbsent.
The utility function needs improvements in type safety and documentation:
- The return type should handle undefined values from Map.get()
- Missing JSDoc documentation
Apply these improvements:
+/**
+ * Returns the value associated with the key if it exists in the map,
+ * otherwise computes a new value using the mapping function, stores it, and returns it.
+ * @param map - The map to check/update
+ * @param key - The key to look up
+ * @param mappingFunction - Function to compute the value if not present
+ * @throws Error if the computed value is undefined
+ */
-function computeIfAbsent<V, F>(map: Map<V, F>, key: V, mappingFunction: (a: V) => F): F {
+function computeIfAbsent<V, F>(map: Map<V, F>, key: V, mappingFunction: (a: V) => F): F {
if (!map.has(key)) {
const newValue = mappingFunction(key);
+ if (newValue === undefined) {
+ throw new Error(`Mapping function returned undefined for key: ${key}`);
+ }
map.set(key, newValue);
}
- return map.get(key);
+ return map.get(key)!;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function computeIfAbsent<V, F>(map: Map<V, F>, key: V, mappingFunction: (a: V) => F): F { | |
| if (!map.has(key)) { | |
| const newValue = mappingFunction(key); | |
| map.set(key, newValue); | |
| } | |
| return map.get(key); | |
| } | |
| /** | |
| * Returns the value associated with the key if it exists in the map, | |
| * otherwise computes a new value using the mapping function, stores it, and returns it. | |
| * @param map - The map to check/update | |
| * @param key - The key to look up | |
| * @param mappingFunction - Function to compute the value if not present | |
| * @throws Error if the computed value is undefined | |
| */ | |
| function computeIfAbsent<V, F>(map: Map<V, F>, key: V, mappingFunction: (a: V) => F): F { | |
| if (!map.has(key)) { | |
| const newValue = mappingFunction(key); | |
| if (newValue === undefined) { | |
| throw new Error(`Mapping function returned undefined for key: ${key}`); | |
| } | |
| map.set(key, newValue); | |
| } | |
| return map.get(key)!; | |
| } |
Created a global map cache for each block’s timeout event to address issue #1625.