Avoid page reload when navigating away from viewer #8396
Conversation
This reverts commit 368c005.
📝 WalkthroughWalkthroughThe pull request introduces multiple resource management and cleanup enhancements across the codebase. Several modules now include new destroy and reset methods to better handle lifecycle events. Additionally, import adjustments, method renamings, and minor UI tidy-ups have been made. The changes also include new action creators for state management and improvements to saga control flow with enhanced error handling using null-safe operators. Changes
Suggested labels
Poem
✨ Finishing Touches
🪧 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 (
|
philippotto
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
frontend/javascripts/oxalis/model/helpers/shader_editor.ts (1)
16-19: Good addition of the destroy method for resource cleanup.The new
destroy()method properly resets bothwindow.materialsandwindow.managersarrays, which aligns with the PR objective of performing careful state cleanup instead of a full page reload. This should help prevent memory leaks by removing references to materials and managers when navigating away from the viewer.For consistency with the initialization pattern used in the
addBucketManagersandaddMaterialmethods, consider adding null checks:destroy() { - window.materials = []; - window.managers = []; + window.materials = window.materials || []; + window.materials.length = 0; + window.managers = window.managers || []; + window.managers.length = 0; },This ensures the properties exist before attempting to reset them, matching the defensive programming style used elsewhere in the file.
frontend/javascripts/oxalis/geometries/plane.ts (1)
202-204: Add null check before calling destroy on materialFactory.The new
destroy()method properly callsdestroy()on thematerialFactory, helping with resource cleanup. However, ifmaterialFactoryis not initialized for any reason (for example, ifcreateMeshes()wasn't called or failed), this could result in a runtime error.For more robust error handling, consider adding a null check:
destroy() { - this.materialFactory.destroy(); + if (this.materialFactory) { + this.materialFactory.destroy(); + } }This ensures the code is defensive against potential null/undefined values, preventing exceptions when cleaning up resources.
frontend/javascripts/oxalis/model/bucket_data_handling/texture_bucket_manager.ts (1)
345-354: Comprehensive cleanup in the destroy method.The
destroymethod properly cleans up resources by disposing textures and resetting internal state. However, there are a couple of improvements to consider:
- The use of
@ts-ignoresuggests a type issue when settinglookUpCuckooTableto null.- Consider adding a check to prevent multiple calls to destroy.
destroy() { + if (this.isDestroyed) { + return; + } for (const texture of this.getTextures()) { texture.dispose(); } this.dataTextures = []; - // @ts-ignore - this.lookUpCuckooTable = null; + this.lookUpCuckooTable = null as unknown as CuckooTableVec5; this.isDestroyed = true; this.activeBucketToIndexMap = new Map(); }frontend/javascripts/oxalis/geometries/cube.ts (1)
215-220: Proper cleanup in destroy method.The
destroymethod correctly calls all unsubscribe functions and resets the array. However, it would be good to add disposal of Three.js resources.Consider disposing of the Three.js geometries and materials in the
destroymethod:destroy() { for (const fn of this.storePropertyUnsubscribers) { fn(); } this.storePropertyUnsubscribers = []; + + // Dispose Three.js resources + this.cube.geometry.dispose(); + (this.cube.material as THREE.Material).dispose(); + + for (const plane of Object.values(this.crossSections)) { + plane.geometry.dispose(); + (plane.material as THREE.Material).dispose(); + } }frontend/javascripts/oxalis/view/action_bar_view.tsx (1)
143-192: Improved annotation creation with hooks-based component.The new
CreateAnnotationButtoncomponent refactors the annotation creation logic into a modern functional component using React hooks. This improves code organization by:
- Using
useHistoryanduseSelectorhooks for better state management- Properly handling segmentation layer fallback support
- Processing mapping information
- Managing authentication requirements
This refactoring enhances maintainability without changing the core functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
frontend/javascripts/libs/UpdatableTexture.ts(1 hunks)frontend/javascripts/main.tsx(2 hunks)frontend/javascripts/navbar.tsx(0 hunks)frontend/javascripts/oxalis/controller/renderer.ts(1 hunks)frontend/javascripts/oxalis/controller/scene_controller.ts(4 hunks)frontend/javascripts/oxalis/controller/scene_controller_provider.ts(1 hunks)frontend/javascripts/oxalis/controller/url_manager.ts(2 hunks)frontend/javascripts/oxalis/controller/viewmodes/arbitrary_controller.tsx(1 hunks)frontend/javascripts/oxalis/controller/viewmodes/plane_controller.tsx(2 hunks)frontend/javascripts/oxalis/geometries/arbitrary_plane.ts(2 hunks)frontend/javascripts/oxalis/geometries/cube.ts(3 hunks)frontend/javascripts/oxalis/geometries/materials/edge_shader.ts(3 hunks)frontend/javascripts/oxalis/geometries/materials/node_shader.ts(4 hunks)frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts(3 hunks)frontend/javascripts/oxalis/geometries/plane.ts(1 hunks)frontend/javascripts/oxalis/geometries/skeleton.ts(4 hunks)frontend/javascripts/oxalis/model.ts(1 hunks)frontend/javascripts/oxalis/model/actions/actions.ts(2 hunks)frontend/javascripts/oxalis/model/bucket_data_handling/data_cube.ts(3 hunks)frontend/javascripts/oxalis/model/bucket_data_handling/layer_rendering_manager.ts(2 hunks)frontend/javascripts/oxalis/model/bucket_data_handling/mappings.ts(3 hunks)frontend/javascripts/oxalis/model/bucket_data_handling/polyhedron_rasterizer.ts(0 hunks)frontend/javascripts/oxalis/model/bucket_data_handling/texture_bucket_manager.ts(3 hunks)frontend/javascripts/oxalis/model/data_layer.ts(1 hunks)frontend/javascripts/oxalis/model/helpers/shader_editor.ts(1 hunks)frontend/javascripts/oxalis/model/reducers/ui_reducer.ts(2 hunks)frontend/javascripts/oxalis/model/sagas/root_saga.ts(1 hunks)frontend/javascripts/oxalis/store.ts(1 hunks)frontend/javascripts/oxalis/view/action_bar_view.tsx(3 hunks)frontend/javascripts/oxalis/view/input_catcher.tsx(1 hunks)frontend/javascripts/oxalis/view/layouting/tracing_layout_view.tsx(3 hunks)frontend/javascripts/oxalis/view/plane_view.ts(2 hunks)
💤 Files with no reviewable changes (2)
- frontend/javascripts/navbar.tsx
- frontend/javascripts/oxalis/model/bucket_data_handling/polyhedron_rasterizer.ts
🧰 Additional context used
🧠 Learnings (1)
frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts (1)
Learnt from: dieknolle3333
PR: scalableminds/webknossos#8168
File: frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts:568-585
Timestamp: 2025-03-12T13:04:06.063Z
Learning: In the file `frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts`, the uniform name `allResolutions` should remain unchanged to maintain consistency with the shader code.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (64)
frontend/javascripts/oxalis/model.ts (1)
357-366: Well-implemented reset method with proper null checks.The new
reset()method effectively destroys all layers and resets thedataLayersobject. The implementation correctly:
- Checks if
this.dataLayersexists before operating on it- Uses
_.values()to convert object values to an array for iteration- Calls
destroy()on each layer to ensure proper cleanup- Resets
dataLayersto an empty object after cleanupThis approach aligns perfectly with the PR objective of performing careful state cleanup instead of relying on a full page reload, which should improve performance when navigating away from the viewer.
frontend/javascripts/oxalis/controller/viewmodes/arbitrary_controller.tsx (1)
382-382:✅ Verification successful
Changed method from destroy() to stop() on plane object.
The line was modified to call
this.plane.stop()instead of the previousthis.plane.destroy(). This indicates a change in the cleanup approach from complete destruction to a potentially stateful suspension.Verify that:
- The
ArbitraryPlaneclass has a properly implementedstop()method- The
stop()method performs appropriate resource cleanupThis change seems aligned with the PR's goal of carefully managing resource cleanup rather than forcing a full reset, but it's important to ensure the
stop()method properly handles all necessary cleanup operations.
🏁 Script executed:
#!/bin/bash # Check if ArbitraryPlane has a stop method and what it does echo "Checking for stop method in ArbitraryPlane class:" rg -A 10 "stop\(\)" frontend/javascripts/oxalis/geometries/arbitrary_plane.tsLength of output: 588
Verification: ArbitraryPlane stop() method performs expected cleanup
The review comment has been verified. The
ArbitraryPlaneclass now implements astop()method that calls boththis.stopStoreListening()andthis.materialFactory.stopListening(), which aligns with the intended change from a complete destruction approach to a stateful suspension. Please ensure that the internal implementations ofstopStoreListening()andmaterialFactory.stopListening()indeed free up all necessary resources.
- Verified that the
stop()method exists and calls the expected functions.- The change is consistent with the PR’s goals for careful resource management.
frontend/javascripts/libs/UpdatableTexture.ts (1)
120-122: Good addition for renderer cleanupThis function allows external components to reset the
originalTexSubImage2Dvariable when a renderer is disposed, preventing potential memory leaks or stale WebGL context references.frontend/javascripts/oxalis/model/data_layer.ts (1)
74-77: Improved resource cleanup in destroy methodAdding explicit destruction of the cube and mappings instances improves resource management and helps prevent memory leaks when navigating away from the viewer.
frontend/javascripts/oxalis/store.ts (1)
657-659: Good function rename for clarityRenaming from
startSagastostartSagabetter reflects that the function launches a single saga, improving API clarity and consistency.frontend/javascripts/oxalis/controller/renderer.ts (2)
1-1: Good import for coordinating cleanupImporting the
notifyAboutDisposedRendererfunction enables proper coordination between renderer disposal and texture management.
8-15: Well-implemented renderer cleanup functionThe
destroyRendererfunction properly handles renderer disposal with appropriate null checks. This is a critical component for preventing memory leaks when navigating away from the viewer without page reloads.frontend/javascripts/oxalis/model/reducers/ui_reducer.ts (2)
15-25: Clean implementation of state reset while preserving essential data.The new "RESET_STORE" case appropriately handles state reset by using defaultState as a base while preserving user-specific data (activeUser and activeOrganization) and layout preferences (storedLayouts). This aligns well with the PR's objective of performing careful cleanup when navigating away from the viewer instead of a full page reload.
2-2: Good addition of defaultState import.This import is necessary for the new RESET_STORE reducer case, as it provides the clean slate for resetting the application state.
frontend/javascripts/oxalis/controller/url_manager.ts (3)
96-96: Proper resource management with optional unsubscribe function.Adding this property to store the unsubscribe function is a good practice for resource cleanup.
236-236: Enhanced subscription management by storing the unsubscribe function.Storing the return value from Store.subscribe() in this.stopStoreListening enables proper cleanup later, which is essential for preventing memory leaks when navigating away from the viewer.
241-246: Well-implemented cleanup method for URL management.This method properly handles the cleanup of store subscription and hash change listener. The null check before calling stopStoreListening is a good defensive programming practice.
frontend/javascripts/oxalis/geometries/arbitrary_plane.ts (2)
42-45: Clear separation of listener cleanup in renamed method.Renaming the original destroy method to stop() and focusing it on stopping listeners improves separation of concerns in resource management.
173-175: Proper implementation of resource cleanup in new destroy method.The new destroy() method correctly delegates to materialFactory.destroy(), completing the separation between stopping listeners and cleaning up resources.
frontend/javascripts/oxalis/view/input_catcher.tsx (1)
134-134: Simplified cleanup logic in effect hook.The cleanup function has been simplified by directly deleting the viewport entry from renderedInputCatchers without checking if domElementRef.current exists. This is safe since deleting a key from a Map works even if the key doesn't exist.
frontend/javascripts/oxalis/controller/viewmodes/plane_controller.tsx (2)
24-26: Good addition of null-safe access.Adding the import for
getSceneControllerOrNullis a good practice to handle edge cases when the scene controller may not be available.
591-592: Nice defensive programming approach.Using
getSceneControllerOrNull()?.stopPlaneMode()with optional chaining instead of direct access prevents potential runtime errors when navigating away. This is essential for enabling smooth navigation without page reloads.frontend/javascripts/oxalis/model/bucket_data_handling/data_cube.ts (3)
89-89: Good addition of cleanup tracking.Adding the
storePropertyUnsubscribersarray is a good practice for tracking listeners that need to be cleaned up.
135-145: Proper listener tracking implementation.Storing the unsubscribe function in the array allows for proper cleanup later, preventing memory leaks and unwanted behavior when navigating away from the viewer.
975-980: Essential cleanup implementation.The
destroymethod properly cleans up resources by:
- Clearing the cubes and buckets collections
- Calling all unsubscribe functions
- Resetting the unsubscribers array
This is crucial for allowing navigation without page reloads and preventing memory leaks.
frontend/javascripts/oxalis/view/plane_view.ts (2)
9-11: Good addition of null-safe access.Adding the import for
getSceneControllerOrNullimproves error handling, consistent with changes in other files.
224-229: Improved error resilience in cleanup.Using a null check with
getSceneControllerOrNull()before removing cameras prevents errors when stopping the plane view. This defensive approach is important during cleanup phases when dependencies might already be in the process of being destroyed.frontend/javascripts/main.tsx (2)
3-4: Streamlined saga management.Replacing the root saga import with a specific saga function indicates a move toward more modular and targeted saga management.
39-39: Targeted saga initialization.Changed from initializing a root saga to specifically starting only the email verification saga. This could improve cleanup capabilities when navigating away from the viewer.
frontend/javascripts/oxalis/model/bucket_data_handling/texture_bucket_manager.ts (2)
93-93: Good addition of tracking destroyed state.Adding the
isDestroyedflag is a good practice for managing the lifecycle of this component and preventing operations on a destroyed instance.
186-189: Prevents animation frame loop from continuing after destruction.This check appropriately stops the animation frame loop when the instance is destroyed, preventing memory leaks and unnecessary processing.
frontend/javascripts/oxalis/model/actions/actions.ts (3)
38-39: Appropriate action type additions.Adding these new action types to the
Actionunion is necessary to maintain type safety in the Redux store.
47-50: Well-defined resetStoreAction creator.The
resetStoreActionfollows the established pattern for Redux action creators in this codebase.
62-65: Well-defined cancelSagaAction creator.The
cancelSagaActionfollows the established pattern for Redux action creators in this codebase.frontend/javascripts/oxalis/geometries/cube.ts (2)
37-37: Good addition for tracking unsubscribers.Adding this array to store unsubscribe functions is a good pattern for managing cleanup of store listeners.
62-67: Improved store subscriber management.Pushing the unsubscribe function into the array instead of using it directly improves resource management.
frontend/javascripts/oxalis/controller/scene_controller.ts (5)
19-19: Important import for renderer destruction.Adding the
destroyRendererimport is essential for proper cleanup of WebGL resources.
79-79: Good addition for tracking unsubscribers.Adding and initializing the
storePropertyUnsubscribersarray is a good pattern for managing cleanup of store listeners.Also applies to: 93-93
537-572: Comprehensive scene controller cleanup.The
destroymethod thoroughly cleans up resources including:
- Window debugging methods
- Skeletons
- Store property listeners
- WebGL renderer
- Scene objects (cubes and planes)
This is critical for preventing memory leaks when navigating away from the viewer.
Two minor suggestions:
- Consider disposing of Three.js materials and geometries
- Add a check to prevent multiple calls to destroy
558-560: Proper renderer cleanup.Calling
destroyRenderer()and nullifying the renderer reference is essential for WebGL resource cleanup.
575-610: Improved store subscriber management.Consolidating all the store property listeners into a single array makes cleanup much more maintainable. This implementation ensures all listeners are properly tracked and can be unsubscribed when the component is destroyed.
frontend/javascripts/oxalis/controller/scene_controller_provider.ts (2)
12-14: Good addition of safe access to the sceneController.Adding a null-safe way to access the scene controller enhances error handling. This helps prevent runtime exceptions when trying to access the scene controller that might not be initialized yet.
20-25: Nice addition of proper cleanup method.This new destroy method properly cleans up resources by checking if the scene controller exists before calling destroy and then setting it to null. This is an important part of the PR's goal to "perform careful cleanup of the current state" when navigating away from the viewer.
frontend/javascripts/oxalis/model/bucket_data_handling/mappings.ts (3)
64-64: Good addition of subscription tracking.Adding an array to track store property unsubscribers is a good practice for managing subscriptions in components with lifecycle events.
80-89: Improved subscription management.Storing the unsubscribe function in an array rather than calling it directly allows for proper cleanup when the component is destroyed, preventing potential memory leaks from lingering subscriptions.
158-161: Good implementation of resource cleanup.The destroy method properly cleans up by calling all unsubscribe functions and resetting the array. This ensures that all subscriptions are properly removed when navigating away from the viewer.
frontend/javascripts/oxalis/geometries/materials/node_shader.ts (4)
32-32: Good addition of subscription tracking.Similar to the Mappings class, adding an array to track store property unsubscribers is a good practice for managing subscriptions in the NodeShader class.
98-121: Improved subscription management for store listeners.The refactoring to store unsubscribe functions in an array allows for proper cleanup when the component is destroyed, which is essential for navigating away from the viewer without memory leaks.
132-156: Clean implementation of transform listener.The transformation listener is now properly managed with the same subscription pattern as the other listeners, ensuring it will be properly cleaned up on destroy.
431-437: Well-implemented destroy method.The destroy method properly cleans up resources by:
- Calling all unsubscribe functions
- Resetting the subscriptions array
- Nullifying the treeColors reference
This is crucial for proper memory management when navigating away from the viewer.
frontend/javascripts/oxalis/geometries/skeleton.ts (4)
117-118: Good property additions for shader management.Adding explicit properties to hold the shader instances improves code clarity and maintainability, while also enabling proper shader cleanup.
142-154: Enhanced destroy method with comprehensive cleanup.The enhanced destroy method now properly:
- Cleans up the store subscription
- Disposes of texture resources
- Disposes of materials
- Calls destroy on the shader instances
This thorough cleanup is essential for preventing memory leaks when navigating away from the viewer.
173-174: Good refactoring to store shader instances.Storing the NodeShader and EdgeShader instances in class properties enables proper cleanup in the destroy method. This change aligns with the goal of careful state cleanup when navigating away.
190-199: Refactored to use shader instances consistently.The code now consistently uses the stored shader instances to get materials for buffer initialization, making the code more maintainable and consistent.
frontend/javascripts/oxalis/view/layouting/tracing_layout_view.tsx (4)
14-23: Comprehensive cleanup imports added to support navigation without page reload.These new imports enable the component to properly clean up resources when unmounting, which is essential for the feature that prevents page reloads when navigating away from the viewer.
81-81: New feature flag to control reload behavior.This flag controls whether a full page reload occurs when exiting the viewer. Setting it to
falseenables the new behavior where navigation doesn't trigger a page reload.
117-119: Initialize component with root saga.Initializing the saga when the component mounts ensures that all the necessary background tasks are started properly with the component lifecycle.
121-130: Enhanced cleanup process when unmounting.This improved unmount method properly cleans up resources by:
- Stopping the URL updater
- Resetting the Model
- Destroying the scene controller
- Resetting the Redux store
- Canceling any running sagas
- Only reloading the page if explicitly required
This aligns perfectly with the PR objective of avoiding page reloads when navigating away from the viewer.
frontend/javascripts/oxalis/view/action_bar_view.tsx (1)
242-242: Clean implementation of the refactored button.The render method is now much simpler, delegating all the annotation creation logic to the specialized component.
frontend/javascripts/oxalis/model/sagas/root_saga.ts (2)
27-27: Improved return type for root saga.Changed from
Saga<never>toSaga<void>for better type clarity.
31-42: Enhanced saga flow control with cancellation support.This implementation adds proper saga cancellation support, which is crucial for the feature that avoids page reloads when navigating away from the viewer. The changes:
- Use
raceto concurrently listen for either restart or cancel actions- Handle UI state properly based on the action received
- Allow for complete termination of the saga when canceling
- Enable clean navigation without resource leaks
This is a key improvement for resource management when transitioning between views.
frontend/javascripts/oxalis/model/bucket_data_handling/layer_rendering_manager.ts (2)
43-45: Simplified memoization implementation.The memoization of
asyncBucketPickis now more concisely formatted for better readability.
284-291: Comprehensive resource cleanup in destroy method.The enhanced
destroymethod now properly cleans up all associated resources:
- Destroys the texture bucket manager
- Clears the shared lookup table cache
- Clears the async bucket picker memoization cache
- Destroys the shader editor
- Cleans up the cuckoo table reference
This thorough cleanup prevents memory leaks and ensures resources are properly released when navigating away from the viewer, which is essential for the no-reload navigation feature.
frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts (3)
110-110: Good update to material property type.The property type now correctly reflects that
materialcan benullorundefinedduring initialization or after destruction, improving type safety.
975-977: Well-implemented null check for defensive programming.Adding an explicit check for null material helps prevent obscure errors when trying to access the material after it has been destroyed, and provides a clear error message instead.
1161-1173: Good resource cleanup implementation.The
destroymethod properly cleans up resources by:
- Unsubscribing from store listeners
- Unsubscribing from color and mapping seeds
- Nullifying the material reference
- Canceling any pending shader recomputations
This aligns with the PR objective of careful state cleanup when navigating away from the viewer.
frontend/javascripts/oxalis/geometries/materials/edge_shader.ts (3)
21-21: Good addition of store property unsubscribers array.Adding a dedicated array to track unsubscribe functions is a good pattern for resource management.
62-96: Well-structured listener registration with cleanup support.Refactoring the store property listeners to store their unsubscribe functions in an array makes cleanup more manageable and helps prevent memory leaks.
207-213: Good implementation of resource cleanup.The
destroymethod properly:
- Calls all unsubscribe functions to clean up listeners
- Resets the unsubscribers array
- Nullifies the tree colors texture reference
This complements the PR's goal of performing careful cleanup when navigating away from the viewer.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/javascripts/oxalis/view/layouting/tracing_layout_view.tsx (1)
119-137: Remove leftover “todop” comment and finalize cleanup strategy.
The extensive unmount logic properly resets the model, controller, and Redux store. However, there are a couple of concerns:
- The comment on line 126 (“todop: remove before merging”) is likely a temporary placeholder.
- The hard-coded
ENFORCE_FOR_TESTING = truemight be better managed through an environment variable or dedicated config flag to avoid accidental production toggles.Below is a minimal fix to remove the leftover comment and replace the hardcoded testing variable with an environment-based approach:
- // todop: remove before merging - const ENFORCE_FOR_TESTING = true; + const ENFORCE_FOR_TESTING = + process.env.REACT_APP_ENFORCE_FOR_TESTING === "true";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/oxalis/view/layouting/tracing_layout_view.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (2)
frontend/javascripts/oxalis/view/layouting/tracing_layout_view.tsx (2)
14-23: Imports look well-organized, but watch out for consistent usage.
These newly added imports for saga management (rootSaga,startSaga,cancelSagaAction,resetStoreAction) and scene handling (destroySceneController,Model.reset(),UrlManager) appear consistent with the updated lifecycle methods. Ensure that these imports are only used where necessary and verify that the codebase remains free from potential circular dependencies or partial integration.
115-117:✅ Verification successful
Ensure correct saga lifecycle management.
Starting the saga incomponentDidMount()is fine, as long as you confirm that the saga is effectively canceled incomponentWillUnmount(). If future code adds more concurrency or asynchronous flows, watch for any race conditions or memory leaks.Consider verifying that the saga is fully stopped upon unmount with a script:
🏁 Script executed:
#!/bin/bash # Search for all references to `cancelSagaAction()` to ensure they match usage in the unmount lifecycle. rg -A 3 'cancelSagaAction'Length of output: 1558
Saga Cancellation Confirmed on Unmount.
The search results show that
cancelSagaAction()is indeed referenced in thetracing_layout_view.tsxfile. This confirms that the saga started incomponentDidMount()is being explicitly canceled, likely in thecomponentWillUnmount()lifecycle method (albeit marked with a temporary note). Please ensure any temporary removal messages such as “todop: remove before merging” are resolved prior to finalizing the PR.
- Confirm that
Store.dispatch(cancelSagaAction())is executed on unmount.- Remove any temporary or testing flags before merging.
|
I just noticed a small visual bug in the time tracking button in the navbar for non-Admin users. Could you maybe please neak a missing whitespace into this PR? Here is a screenshot of whats visible when logged in as sample2 user: |
There was a problem hiding this comment.
This is very awesome 🚀
Not having to reload is such an awesome improvement :D
I found minor suggestions regarding the code and during testing I noticed:
- When getting a new task in the annotation view, once the new task is loaded the modal telling the user things about the task and what to does is not present / does not open up. It only did on the initial task.
I didn't find anything else during testing 🎉 Well done!
| getSceneControllerOrNull()?.stopPlaneMode(); | ||
|
|
There was a problem hiding this comment.
Under what circumstances can it be null here? This code reads like it is subject to race conditions 🤔
There was a problem hiding this comment.
I added this comment:
// SceneController might already be null, because componentWillUnmount will trigger
// earlier for outer components and later for inner components. The outer
// component TracingLayoutView is responsible for the destroy call which already
// happened when the stop method here is called.
it's not ideal, but I couldn't come up with another simple solution.
frontend/javascripts/oxalis/geometries/materials/edge_shader.ts
Outdated
Show resolved
Hide resolved
frontend/javascripts/oxalis/geometries/materials/node_shader.ts
Outdated
Show resolved
Hide resolved
| Store.dispatch(resetStoreAction()); | ||
| Store.dispatch(cancelSagaAction()); | ||
|
|
||
| // todop: remove before merging |
There was a problem hiding this comment.
Here is an open TODO :)
| UrlManager.stopUrlUpdater(); | ||
| Model.reset(); | ||
| destroySceneController(); | ||
| Store.dispatch(resetStoreAction()); | ||
| Store.dispatch(cancelSagaAction()); |
There was a problem hiding this comment.
Cant this be neglected if the page is reloaded anyway? So only do this function calls in case of an active super user 🤔?
There was a problem hiding this comment.
yes, in theory it's not necessary, but I don't trust the browser to perform location.reload() in an atomic matter. for example, the user could hit escape during reload and I'm not sure that everything will just work then. so, I'd rather keep the potentially unnecessary clean up here :)
|
Hmmm 🤔
The same behaviour seems to happen on master. Is this maybe due to always having the same task type? 🤔 And is this intended behaviour? |
yes, this is by design if the task type did not change. |
|
thanks for your feedback, @MichaelBuessemeyer! please have another look. if you give your "go", I'll remove the last todo that is meant for testing :) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
frontend/javascripts/components/error_boundary.tsx (4)
35-35: Consider using React-Redux hooks instead of direct store access.Direct access to the Redux store with
Store.getState()is generally not recommended. Instead, consider using React-Redux'suseSelectorhook or theconnectHOC for more maintainable and testable code.-import { Store } from "oxalis/singletons"; +import { useSelector } from "react-redux"; import React, { type ErrorInfo } from "react"; import { getAntdTheme, getThemeFromUser } from "theme";Then refactor the class component to a functional component or use a connected component approach.
38-38: Add error handling for theme retrieval.If
getThemeFromUserorgetAntdThemethrows an error, it could lead to a cascade of errors within the error boundary itself. Consider adding a try-catch block to gracefully handle theme retrieval failures.render() { if (this.state.error != null) { const { error, info } = this.state; const componentStack = info?.componentStack; const errorMessage = (error || "").toString(); const errorDescription = componentStack; - const userTheme = getThemeFromUser(Store.getState().activeUser); + let userTheme; + try { + userTheme = getThemeFromUser(Store.getState().activeUser); + } catch (themeError) { + // Fall back to default theme if theme retrieval fails + userTheme = "light"; + console.error("Failed to retrieve user theme:", themeError); + } return ( - <ConfigProvider theme={getAntdTheme(userTheme)}> + <ConfigProvider theme={(() => { + try { + return getAntdTheme(userTheme); + } catch (themeError) { + console.error("Failed to get Ant Design theme:", themeError); + return {}; // Fall back to default Ant Design theme + } + })()}>Also applies to: 64-64
52-54: Enhance accessibility for the localStorage clearing link.The link for clearing localStorage could benefit from additional accessibility attributes such as
aria-labelto better describe its purpose.-<a href="#" onClick={this.clearLocalStorageAndReload}> +<a + href="#" + onClick={this.clearLocalStorageAndReload} + aria-label="Clear local storage and reload the page" +> here </a>
39-39: Consider using theme tokens instead of hardcoded margin values.Using a hardcoded margin value (
margin: 32) may not adapt well to different themes or screen sizes. Consider using theme tokens for spacing to ensure consistency with the design system.-<div style={{ margin: 32 }}> +<div style={{ margin: token.marginLG }}>You would need to extract the token from the theme:
const token = theme.useToken().token;frontend/javascripts/oxalis/model/actions/actions.ts (1)
64-67: Good implementation of saga cancellation action.This action creator will allow for canceling ongoing sagas when navigating away from the viewer, preventing potential issues with sagas continuing to run in the background.
Consider documenting the intended use cases for this action in a comment, as saga cancellation might have subtle implications for application state.
export const cancelSagaAction = () => ({ type: "CANCEL_SAGA", + // This action is used to cancel ongoing sagas when navigating away from the viewer }) as const;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
frontend/javascripts/components/error_boundary.tsx(2 hunks)frontend/javascripts/libs/UpdatableTexture.ts(1 hunks)frontend/javascripts/navbar.tsx(0 hunks)frontend/javascripts/oxalis/controller/scene_controller.ts(4 hunks)frontend/javascripts/oxalis/controller/url_manager.ts(2 hunks)frontend/javascripts/oxalis/controller/viewmodes/arbitrary_controller.tsx(1 hunks)frontend/javascripts/oxalis/controller/viewmodes/plane_controller.tsx(2 hunks)frontend/javascripts/oxalis/geometries/arbitrary_plane.ts(2 hunks)frontend/javascripts/oxalis/geometries/materials/edge_shader.ts(3 hunks)frontend/javascripts/oxalis/geometries/materials/node_shader.ts(4 hunks)frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts(3 hunks)frontend/javascripts/oxalis/model/actions/actions.ts(2 hunks)frontend/javascripts/oxalis/model/bucket_data_handling/data_cube.ts(3 hunks)frontend/javascripts/oxalis/model/bucket_data_handling/layer_rendering_manager.ts(2 hunks)frontend/javascripts/oxalis/model/bucket_data_handling/texture_bucket_manager.ts(3 hunks)frontend/javascripts/oxalis/model/helpers/shader_editor.ts(1 hunks)frontend/javascripts/oxalis/model/reducers/ui_reducer.ts(2 hunks)frontend/javascripts/oxalis/model/sagas/root_saga.ts(1 hunks)frontend/javascripts/oxalis/store.ts(1 hunks)frontend/javascripts/oxalis/view/action_bar_view.tsx(3 hunks)frontend/javascripts/oxalis/view/arbitrary_view.ts(3 hunks)frontend/javascripts/oxalis/view/input_catcher.tsx(1 hunks)frontend/javascripts/oxalis/view/layouting/tracing_layout_view.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- frontend/javascripts/navbar.tsx
🚧 Files skipped from review as they are similar to previous changes (9)
- frontend/javascripts/oxalis/controller/viewmodes/arbitrary_controller.tsx
- frontend/javascripts/libs/UpdatableTexture.ts
- frontend/javascripts/oxalis/model/helpers/shader_editor.ts
- frontend/javascripts/oxalis/store.ts
- frontend/javascripts/oxalis/controller/url_manager.ts
- frontend/javascripts/oxalis/controller/viewmodes/plane_controller.tsx
- frontend/javascripts/oxalis/model/reducers/ui_reducer.ts
- frontend/javascripts/oxalis/view/input_catcher.tsx
- frontend/javascripts/oxalis/model/bucket_data_handling/layer_rendering_manager.ts
🧰 Additional context used
🧠 Learnings (1)
frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts (1)
Learnt from: dieknolle3333
PR: scalableminds/webknossos#8168
File: frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts:568-585
Timestamp: 2025-04-02T13:29:57.427Z
Learning: In the file `frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts`, the uniform name `allResolutions` should remain unchanged to maintain consistency with the shader code.
🧬 Code Definitions (2)
frontend/javascripts/oxalis/controller/scene_controller.ts (2)
frontend/javascripts/oxalis/controller/renderer.ts (1)
destroyRenderer(8-15)frontend/javascripts/oxalis/model/accessors/dataset_accessor.ts (1)
getDataLayers(128-130)
frontend/javascripts/oxalis/view/layouting/tracing_layout_view.tsx (4)
frontend/javascripts/oxalis/store.ts (1)
startSaga(667-669)frontend/javascripts/oxalis/model/sagas/root_saga.ts (1)
rootSaga(28-45)frontend/javascripts/oxalis/controller/scene_controller_provider.ts (1)
destroySceneController(20-25)frontend/javascripts/oxalis/model/actions/actions.ts (2)
resetStoreAction(49-52)cancelSagaAction(64-67)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (36)
frontend/javascripts/oxalis/geometries/arbitrary_plane.ts (2)
42-45: Approve method rename and functionality splitThe renaming of the previous
destroy()method tostop()with focused functionality aligns well with the PR objective of preventing page reload when navigating away from the viewer. This method now specifically handles pausing listeners without full destruction.
173-176: Approve new destroy method implementationThe new
destroy()method provides comprehensive cleanup by stopping store listening and destroying the material factory. This addresses the previous review comment about includingthis.stopStoreListening().One consideration: If
stop()is called beforedestroy(),this.stopStoreListening()would be called twice. Verify that this function is idempotent or consider havingdestroy()callstop()instead of duplicating the stop listening logic:destroy() { - this.stopStoreListening(); + this.stop(); this.materialFactory.destroy(); }frontend/javascripts/components/error_boundary.tsx (2)
1-1: Updated imports enhance the component's theming capabilities.The added imports for
ConfigProvider,Typography,Store, and theme utilities allow for proper theming of the error boundary component. This aligns with best practices for maintaining consistent UI across the application.Also applies to: 3-3, 5-5
38-64: Improved error UI with proper theming support.The error UI has been significantly improved by:
- Wrapping the content in
ConfigProviderto apply the user's theme- Using semantic typography components instead of basic HTML elements
- Maintaining a consistent layout with better visual hierarchy
This change ensures the error page follows the application's design system, providing a more polished user experience even during failures.
frontend/javascripts/oxalis/model/bucket_data_handling/data_cube.ts (3)
89-89: Good addition of tracking for cleanup.Adding a dedicated array to store unsubscribe functions is a clean approach to ensure proper resource management.
135-145: Improved store property listener management.The code now properly tracks the unsubscribe function returned by
listenToStoreProperty, which will allow for proper cleanup later. This change helps prevent potential memory leaks that could occur if listeners remained active after a DataCube instance is no longer needed.
990-995: Well-implemented destruction method for proper cleanup.The
destroymethod properly handles cleanup by:
- Clearing cube and bucket references
- Calling all store property unsubscribe functions
- Resetting the unsubscribers array
This is essential for preventing memory leaks when navigating away from the viewer without a full page reload.
frontend/javascripts/oxalis/model/actions/actions.ts (2)
40-41: Good addition of new action types.These additions to the union type ensure proper type safety when using the new actions.
49-52: Well-defined action creator for store reset.This action creator follows the existing pattern in the codebase and will be used to trigger store cleanup when navigating away from the viewer.
frontend/javascripts/oxalis/controller/scene_controller.ts (4)
19-19: Correctly imported destroyRenderer function.Importing the destroyRenderer function allows for proper cleanup of the renderer when the scene controller is destroyed.
79-79: Good addition of tracking for cleanup.Adding a property to store unsubscribe functions and initializing it in the constructor is a clean approach to resource management.
Also applies to: 93-93
537-572: Comprehensive destruction method for proper cleanup.The
destroymethod thoroughly cleans up all resources managed by the SceneController:
- Removes debugging methods from the window object
- Removes all skeletons
- Unsubscribes from all store properties
- Destroys the renderer
- Cleans up bounding boxes, planes, and other geometries
- Resets the root node
This is an essential addition to prevent memory leaks and ensure proper resource management when navigating away from the viewer.
575-610: Improved event binding with proper cleanup tracking.The refactored
bindToEventsmethod now stores all unsubscribe functions in an array, making it easier to clean them up when the scene controller is destroyed. This is a significant improvement for resource management.frontend/javascripts/oxalis/model/bucket_data_handling/texture_bucket_manager.ts (3)
95-95: Good addition of destruction tracking flag.This flag will help prevent unnecessary processing after the manager is destroyed.
183-186: Proper early return for destroyed instances.Checking if the instance is destroyed before proceeding with processing is an important optimization that prevents unnecessary work and avoids scheduling new animation frames after cleanup.
346-355: Well-implemented destruction method for resource cleanup.The
destroymethod thoroughly cleans up resources managed by the TextureBucketManager:
- Disposes of all textures to free GPU memory
- Clears data texture references
- Removes the lookup table reference
- Sets the destroyed flag to prevent further processing
- Resets the bucket map
This is an essential addition to prevent GPU memory leaks and ensure proper resource management when navigating away from the viewer.
frontend/javascripts/oxalis/geometries/materials/node_shader.ts (4)
32-32: Good addition of a property to store unsubscribe functions.Adding
storePropertyUnsubscribersenables proper cleanup of event listeners when the shader is destroyed, which helps prevent memory leaks.
98-121: Improved store subscription management.Storing the unsubscribe functions in an array allows for proper cleanup. This pattern enables better lifecycle management of the component.
132-156: Consistent pattern for store subscription management.The code correctly adds another store subscription to the array of unsubscribe functions, maintaining the pattern established earlier.
431-441: Good implementation of the destroy method.The destroy method properly:
- Calls all unsubscribe functions to stop listening to store changes
- Clears the array of unsubscribe functions
- Sets all uniform values to null to prevent memory leaks
This addresses the previous review comment about only clearing tree colors.
frontend/javascripts/oxalis/geometries/materials/edge_shader.ts (3)
21-21: Good addition of a property to store unsubscribe functions.Adding
storePropertyUnsubscribersenables proper cleanup of event listeners when the shader is destroyed, consistent with the pattern in NodeShader.
62-96: Improved store subscription management.Storing the unsubscribe functions in an array allows for proper cleanup. This pattern enables better lifecycle management of the component.
207-217: Good implementation of the destroy method.The destroy method properly cleans up all resources by:
- Calling all unsubscribe functions
- Clearing the array of unsubscribe functions
- Setting all uniform values to null to prevent memory leaks
This addresses the previous review comment about only clearing tree colors.
frontend/javascripts/oxalis/view/layouting/tracing_layout_view.tsx (4)
14-23: Good imports for the new cleanup functionality.The imports add necessary functions and actions for proper cleanup when unmounting the component.
115-117: Good addition of componentDidMount.Starting the root saga in componentDidMount ensures the saga is initialized when the component is mounted.
120-124: Thorough cleanup in componentWillUnmount.The component properly cleans up resources by:
- Stopping the URL updater
- Resetting the Model
- Destroying the scene controller
- Dispatching reset and cancel actions to the store
This comprehensive cleanup helps prevent memory leaks and ensures a clean state when navigating away.
135-165: Good implementation of conditional page reload.The code now conditionally skips page reload for super users, which aligns with the PR objective to "prevent the page from reloading when users navigate away from the webknossos viewer." This enables controlled testing of the performance improvement before rolling it out to all users.
frontend/javascripts/oxalis/view/arbitrary_view.ts (3)
6-8: Good addition of null-safe controller access.Adding the
getSceneControllerOrNullimport allows for safer access to the scene controller, which helps prevent potential runtime errors.
123-127: Improved error handling with null checking.Using the optional chaining operator with
getSceneControllerOrNull()prevents errors when the scene controller has been destroyed. The detailed comment also explains the component lifecycle and why this check is necessary.
269-274: Good defensive programming with early return.Checking if the scene controller is null before proceeding prevents potential errors in the
resizeImplmethod. This is particularly important since this method can be called in a throttled manner, possibly after the component has been unmounted.frontend/javascripts/oxalis/view/action_bar_view.tsx (2)
143-193: Solid implementation of the CreateAnnotationButton component!The new functional component effectively encapsulates the annotation creation logic, providing a cleaner separation of concerns. It properly uses React hooks (useHistory, useSelector) to access routing and state management capabilities.
The onClick handler handles all the necessary logic:
- Gets current state
- Checks for segmentation layer support
- Handles mapping information
- Creates the annotation
- Navigates to the new annotation
This is a nice improvement that aligns with modern React patterns and makes the code more maintainable.
242-244: Good refactoring to use the new componentClean replacement of the old method with the new component, making the code more maintainable.
frontend/javascripts/oxalis/model/sagas/root_saga.ts (1)
27-45: Enhanced saga control flow with cancellation supportThis change significantly improves the application's ability to clean up resources when navigating away from the viewer. The addition of the race effect to listen for both restart and cancel actions provides more flexibility in saga lifecycle management.
Key improvements:
- Using race instead of just take allows handling competing actions
- Adding a cancel path enables proper cleanup when navigating away
- Setting isWkReady to false when restarting ensures proper state indication
This will prevent page reloads as specified in the PR objectives by providing a graceful way to cancel sagas when navigating away.
frontend/javascripts/oxalis/geometries/materials/plane_material_factory.ts (3)
120-120: Improved type safety for material propertyUpdating the material property type to allow null/undefined values makes the code more robust by explicitly handling cases where the material might not be initialized or has been destroyed.
984-989: Good defensive programming with null checkAdding a null check before returning the material helps prevent runtime errors and provides a clearer error message if someone tries to access the material after it's been destroyed.
1171-1188: Excellent resource cleanup implementationThe new destroy method is a critical addition for preventing memory leaks when navigating away from the viewer. It thoroughly cleans up:
- Event listeners via stopListening()
- Subscription callbacks for color and mapping seeds
- The material reference itself
- Cancels any pending shader recomputation
- Nullifies all uniform values to prevent reference retention
This implementation is comprehensive and will help achieve the PR's goal of avoiding page reloads by ensuring proper cleanup when navigating away from the viewer.
frontend/javascripts/oxalis/view/layouting/tracing_layout_view.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Awesome, code looks good. Thanks for applying my feedback 🦶 and answering my questions 🤙
During testing I noticed that when trying to get a new task but there aren't any more tasks, that the navigation back to the dashboard still includes a page refresh. At least I tried this twice and it looked like that,
But I don't think this is much of a blocker for this pr and can be tackled on demand in a future pr
I think, this will be easy to fix (remove a |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/javascripts/oxalis/view/layouting/tracing_layout_view.tsx (2)
119-124: Consider adding error handling for cleanup operationsThe cleanup sequence is good, but there's no error handling if any of these operations fail. If one operation fails, it could prevent subsequent cleanup steps from executing.
Consider wrapping these operations in try-catch blocks:
componentWillUnmount() { - UrlManager.stopUrlUpdater(); - Model.reset(); - destroySceneController(); - Store.dispatch(resetStoreAction()); - Store.dispatch(cancelSagaAction()); + try { + UrlManager.stopUrlUpdater(); + } catch (error) { + console.error("Failed to stop URL updater:", error); + } + + try { + Model.reset(); + } catch (error) { + console.error("Failed to reset model:", error); + } + + try { + destroySceneController(); + } catch (error) { + console.error("Failed to destroy scene controller:", error); + } + + try { + Store.dispatch(resetStoreAction()); + Store.dispatch(cancelSagaAction()); + } catch (error) { + console.error("Failed to dispatch cleanup actions:", error); + }
134-135: Consider adding more context in the commentThe comment about enforcing a reload could be more informative by explaining why a reload is necessary for non-super users and what specific issues it addresses.
- // Enforce a reload to absolutely ensure a clean slate. + // Enforce a reload for non-super users to absolutely ensure a clean slate. + // This prevents potential memory leaks and state inconsistencies + // until we're confident the manual cleanup is reliable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/javascripts/oxalis/store.ts(1 hunks)frontend/javascripts/oxalis/view/layouting/tracing_layout_view.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/oxalis/store.ts
🧰 Additional context used
🧬 Code Definitions (1)
frontend/javascripts/oxalis/view/layouting/tracing_layout_view.tsx (4)
frontend/javascripts/oxalis/store.ts (1)
startSaga(668-670)frontend/javascripts/oxalis/model/sagas/root_saga.ts (1)
rootSaga(28-45)frontend/javascripts/oxalis/controller/scene_controller_provider.ts (1)
destroySceneController(20-25)frontend/javascripts/oxalis/model/actions/actions.ts (2)
resetStoreAction(49-52)cancelSagaAction(64-67)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (3)
frontend/javascripts/oxalis/view/layouting/tracing_layout_view.tsx (3)
14-23: Good addition of necessary imports for cleanup functionalityThese imports support the new lifecycle management approach by bringing in the required dependencies for saga management, controller destruction, and store reset actions.
115-117: Good implementation of componentDidMountStarting the root saga when the component mounts is the correct approach. This ensures that all saga-based side effects are properly initialized when the view is loaded.
126-132: Good implementation of conditional page reload for super usersThis approach aligns well with the PR objectives to prevent page reloads for super users as a first step. The nullish operator
?.correctly handles the case whereactiveUsermight be null.
Don't reload the page when navigating away from the wk viewer. Instead everything should carefully be cleaned up. This performance improvement is only enabled for super users for now (in case we find other issues). We can roll it out to the remaining users later.
URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)