Conversation
# Conflicts: # src/hooks/properties.ts # src/redux/store.ts
… visibility in redux
…cene graph nodes as well
…t need to subscribe anymore
There was a problem hiding this comment.
Pull request overview
This PR refactors the property tree data model and subscriptions to improve UI performance and consistency, introducing normalized Redux entity adapters, a batched property update topic, and derived visibility state for scene graph nodes.
Changes:
- Introduces
propertyTreeslices using RTKcreateEntityAdapterfor properties and property owners, plus a newpropertyTreetopic andPropertyBatcherto batch property updates intoupdateManycalls. - Centralizes URI and property-tree helper logic into
src/util/uris.ts,propertyTreeHelpers, andpropertyTreeSelectors, and rewires hooks/components (usePropertyValue, property-owner visibility, scene tree, settings, navigation, night sky, etc.) to consume the new structure and visibility state. - Adds throttling for high-frequency topics (time and camera) and simplifies property access hooks to read from Redux rather than per-property subscriptions.
Reviewed changes
Copilot reviewed 71 out of 71 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/util/uris.ts |
New centralized URI helpers for scene graph nodes, renderables, enabled/fade properties, globe layers, and transformations to support consistent URI handling across the app. |
src/util/propertyTreeSelectors.ts |
New selectors for computing property-owner visibility and whether a property owner has visible children, based on the normalized properties/owners structures. |
src/util/propertyTreeHooks.ts |
Updates hooks for featured, anchor, and aim nodes to use entity selectors and the new URI helpers; note that useFeaturedNodes now maps marked IDs to owners via sgnUri. |
src/util/propertyTreeHelpers.ts |
Simplifies property-tree helpers to focus on visibility computation and displayName, introducing Visibility-returning checkVisibilityTest while keeping boolean checkVisibility. |
src/util/networkingHooks.ts |
Switches useWebGuiUrl to use usePropertyValue instead of useProperty, aligning with the new property access pattern. |
src/types/types.ts |
Adds a Visibility string union, redefines Properties/PropertyOwners as non-optional maps, and removes the old PropertyOverview map in favor of using meta data directly. |
src/redux/time/timeMiddleware.ts |
Wraps time topic updates in a 500ms lodash.throttle to reduce dispatch frequency while keeping UI-refresh behavior. |
src/redux/store.ts |
Replaces legacy propertytree reducers with new propertyTree/propertyOwnerSlice and propertyTree/propertySlice reducers in the root store configuration. |
src/redux/sessionrecording/sessionRecordingMiddleware.ts |
Uses propertySelectors plus the new setPropertyValue action to control GUI-related properties during session playback; removes direct dependency on the old properties slice. |
src/redux/propertytree/propertyowner/propertyOwnerSlice.ts |
Deletes the old hand-rolled PropertyOwnersState slice in favor of the new entity-adapter-based implementation. |
src/redux/propertytree/propertyTreeMiddleware.ts |
Deletes the old property-tree middleware that flattened owners/properties and managed per-URI propertyTree reload/remove actions. |
src/redux/propertytree/properties/types.ts |
Removes the old PropertyPayload type used by the previous per-property subscription middleware. |
src/redux/propertytree/properties/propertiesSlice.ts |
Removes the legacy properties slice (including propertyOverview maintenance) in favor of normalized entities. |
src/redux/propertytree/properties/propertiesMiddleware.ts |
Removes the old per-property subscription middleware (subscribe/unsubscribe logic and throttled property updates). |
src/redux/propertyTree/propertyTreeMiddleware.ts |
New middleware that loads the full property tree via getRoot, subscribes to a unified propertyTree topic (batched by PropertyBatcher), normalizes entities, computes scene-graph-node visibility, and exposes addUriToPropertyTree, removeUriFromPropertyTree, and setPropertyValue. |
src/redux/propertyTree/propertySlice.ts |
New properties slice using createEntityAdapter keyed by property URI with sorted IDs and standard CRUD reducers/selectors. |
src/redux/propertyTree/propertyOwnerSlice.ts |
New propertyOwners slice using createEntityAdapter, with custom reducers to upsert owners, maintain parent–subowner links, and recursively remove owners plus their flattened children. |
src/redux/propertyTree/propertyBatcher.ts |
New PropertyBatcher class that buffers Update<AnyProperty, Uri> records and flushes them after a short delay to a provided update function (used to batch updateMany dispatches). |
src/redux/local/localSlice.ts |
Extends local state with a sceneTree.visibility map keyed by SGN URI and adds setSceneGraphNodesVisibility/setVisibility reducers to track visibility state. |
src/redux/local/localMiddleware.ts |
Adds listeners that watch property updates for SGN Enabled/Fade properties, derive Visibility via checkVisibilityTest, and maintain sceneTree.visibility; also introduces an internal setVisibilityForUri action. |
src/redux/listenerMiddleware.ts |
Updates listeners to remove the old properties/propertyTree middlewares and register the new propertyTree middleware in the correct order. |
src/redux/groups/util.ts |
Adjusts computeGroups to operate on a PropertyOwner[] plus normalized Properties, using isSceneGraphNode from uris.ts. |
src/redux/groups/groupsSliceMiddleware.ts |
Reworks groups middleware to pull owners/properties from the entity adapters, recompute groups/tags on addUriToPropertyTree.fulfilled, and use the new PropertyOwner-array signature. |
src/redux/events/eventsMiddleware.ts |
Points property-tree add/remove imports to the new propertyTree middleware path. |
src/redux/camera/cameraMiddleware.ts |
Wraps camera topic updates in a 500ms lodash.throttle to reduce dispatch load on high-frequency camera updates. |
src/panels/SkyBrowserPanel/WorldWideTelescope/hooks.ts |
Switches inverse-zoom property access to usePropertyValue. |
src/panels/SkyBrowserPanel/WorldWideTelescope/WwtProvider/WwtProvider.tsx |
Uses usePropertyValue for the WWT image collection URL while preserving connection logic. |
src/panels/SettingsPanel/SettingsSearchListItem.tsx |
Uses removeLastWordFromUri from the new uris helper module. |
src/panels/SettingsPanel/SettingsPanel.tsx |
Refactors settings search to use property/owner selectors, the new visibility helpers, and inlined meta data (metaData.guiName and metaData.visibility) instead of a separate propertyOverview map. |
src/panels/SessionRecordingPanel/PlaySession.tsx |
Removes explicit useSubscribeToProperty calls for GUI properties, relying on the new global property-tree topic instead. |
src/panels/Scene/hooks.ts |
Uses usePropertyValue for time-frame membership checks while still using usePropertyOwner for TimeFrame subowners. |
src/panels/Scene/SceneTree/hooks.ts |
Adapts scene tree data assembly to the normalized property/owner selectors and the new sceneTree.visibility filter for “show only visible” nodes. |
src/panels/Scene/SceneTree/SceneTree.tsx |
Passes the tree instance into SceneTreeNodeContent for search results, and otherwise keeps tree usage aligned with the updated hooks. |
src/panels/Scene/SceneGraphNode/SceneGraphNodeView.tsx |
Switches isRenderable/isSgnTransform imports to the new uris utilities. |
src/panels/Scene/SceneGraphNode/SceneGraphNodeMoreMenu.tsx |
Uses displayName from propertyTreeHelpers and identifierFromUri from the new uris helper. |
src/panels/Scene/SceneGraphNode/SceneGraphNodeMetaInfo.tsx |
Uses usePropertyValue for GuiPath/GuiDescription, adjusts URI helper imports, and keeps description cleaning logic intact. |
src/panels/Scene/SceneGraphNode/SceneGraphNodeHeader.tsx |
Integrates the new visibility model by using useSceneGraphNodeVisibility and useSetPropertyOwnerVisibility, and wires these into PropertyOwnerVisibilityCheckbox. |
src/panels/Scene/Scene.tsx |
Checks whether the scene has loaded using propertyOwnerSelectors.selectIds instead of the old state.propertyOwners.propertyOwners map. |
src/panels/NightSkyPanel/tabs/SunTab.tsx |
Uses property-owner entities selector for sun-related nodes and sgnUri from uris.ts. |
src/panels/NightSkyPanel/tabs/StarsTab.tsx |
Adds usePropertyOwnerVisibility for stars and label visibility checkboxes and passes the visibility/setter into PropertyOwnerVisibilityCheckbox. |
src/panels/NightSkyPanel/tabs/MarkingsTab/MarkingBox.tsx |
Uses usePropertyOwnerVisibility plus uris.ts helpers for markings visibility and URIs. |
src/panels/NightSkyPanel/tabs/MarkingsTab/ConstellationsShowLinesBox.tsx |
Switches to uris.ts helpers and uses visibility from usePropertyOwnerVisibility to gate the “show lines” checkbox. |
src/panels/NightSkyPanel/tabs/MarkingsTab/ConstellationsShowArtBox.tsx |
Adjusts URI helpers import to use the new uris module. |
src/panels/NightSkyPanel/tabs/MarkingsTab/CardinalDirectionsBox.tsx |
Uses usePropertyValue and the new visibility-based API from usePropertyOwnerVisibility to compute whether a given cardinal-direction variant is active. |
src/panels/NavigationPanel/RemainingFlightTimeIndicator.tsx |
Uses sgnUri from uris.ts for flight-time calculations. |
src/panels/NavigationPanel/NavigationPanel.tsx |
Reads navigation anchor/aim properties via propertySelectors and uses useFeaturedNodes from propertyTreeHooks. |
src/panels/NavigationPanel/FocusView/FocusView.tsx |
Uses usePropertyValue for the current aim node and otherwise keeps focus logic the same. |
src/panels/NavigationPanel/AnchorAimView/AnchorAimView.tsx |
Uses property-owner entities for node lookup and sgnUri from uris.ts. |
src/panels/Menu/hooks.ts |
Switches the user-level property lookup to usePropertyValue while preserving menu filtering behavior. |
src/panels/GlobeImageryBrowserPanel/hooks.ts |
Uses sgnRenderableUri/sgnUri from uris.ts when working with globe imagery. |
src/panels/GlobeImageryBrowserPanel/GlobeImageryBrowserPanel.tsx |
Reads the current anchor via usePropertyValue instead of useProperty. |
src/panels/GettingStartedPanel/Tasks/SetPropertyTask.tsx |
Uses usePropertyValue for detecting when a property reaches its target value. |
src/panels/GettingStartedPanel/Tasks/NavigationTask.tsx |
Tracks the current anchor node via usePropertyValue and keeps camera-subscription logic intact. |
src/panels/GettingStartedPanel/Tasks/FocusTask.tsx |
Uses usePropertyValue for NavigationAnchorKey to compute task completion. |
src/panels/GettingStartedPanel/Tasks/Components/CurrentFocus.tsx |
Uses usePropertyValue for the current anchor and useSceneGraphNode to show its label. |
src/panels/GettingStartedPanel/Tasks/ChangePropertyTask.tsx |
Uses usePropertyValue for detecting property changes in tasks. |
src/panels/GettingStartedPanel/Tasks/AltitudeTask.tsx |
Uses usePropertyValue for NavigationAnchorKey while subscribing to camera altitude as before. |
src/panels/GeoLocationPanel/AddedCustomNodes.tsx |
Switches URI helpers to the uris module. |
src/panels/ExoplanetsPanel/ExoplanetsPanel.tsx |
Uses property-owner entities for exoplanet scene graph nodes and sgnUri from uris.ts. |
src/hooks/sceneGraphNodes/hooks.ts |
Reworks scene-graph-node hooks to read from the property/owner entity adapters and uses them to compute GUI settings and focusability. |
src/hooks/propertyOwner.ts |
Refactors property-owner hooks to use the new selectors, replaces useProperty with usePropertyValue where needed, introduces useSceneGraphNodeVisibility and useSetPropertyOwnerVisibility, and adapts usePropertyOwnerVisibility to the new visibility model. |
src/hooks/properties.ts |
Introduces usePropertyValue, simplifies useProperty to read from the entity adapter and send setPropertyValue actions (without manual subscription), and uses usePropertyValue for the global confirmation modal setting. |
src/components/PropertyOwner/VisiblityCheckbox.tsx |
Reworks the visibility checkbox to take explicit visibility and setVisibility props, show a radial sweep icon while fading, and rely on usePropertyValue for the fade percentage. |
src/components/PropertyOwner/PropertyOwnerCollapsable.tsx |
Retrieves visibility and setter via usePropertyOwnerVisibility and passes them into PropertyOwnerVisibilityCheckbox. |
src/components/PropertyOwner/PropertyOwner.tsx |
Uses usePropertyValue and uris.ts helpers to detect globe-layer owners and delegates to the appropriate specialized property-owner components. |
src/components/PropertyOwner/ProgressIcon.tsx |
New RadialSweepIcon component for showing a radial progress/fade state, used by the visibility checkbox during transitions. |
src/components/PropertyOwner/Custom/GlobeLayers/GlobeLayersPropertyOwner.tsx |
Uses sgnIdentifierFromSubownerUri from uris.ts and displayName to label custom globe layer groups. |
src/components/PropertyOwner/Custom/GlobeLayers/GlobeLayersGroup.tsx |
Uses propertySelectors and isPropertyOwnerVisible to compute the count of active layers in each group. |
src/components/PropertyOwner/Custom/GlobeLayers/GlobeLayer.tsx |
Integrates the new usePropertyOwnerVisibility API and passes visibility/setter into PropertyOwnerVisibilityCheckbox, also using the visibility state to color active layer titles. |
src/components/Property/Property.tsx |
Reads property meta data through propertySelectors.selectById to determine which property component to render. |
src/components/DynamicMap/ViewCone.tsx |
Uses usePropertyValue for the evaluated interaction sphere property while leaving the cone computation intact. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WeirdRubberDuck
left a comment
There was a problem hiding this comment.
Here's a round with some code comments and questions. Will do stress-testing as well :)
src/util/batcher.ts
Outdated
| /** | ||
| * @class PropertyBatcher | ||
| * @description A class that batches property updates to avoid flooding the Redux store with too many updates. | ||
| * | ||
| * @constructor |
There was a problem hiding this comment.
Cool. Do we plan to start using JSDoc, in general? Is this something you have discussed?
There was a problem hiding this comment.
@engbergandreas and I have not discussed it, I'm open to discussion. I guess we want to have some kind of comment style for ts classes as we now have two
| if (visibility === 'Fading' && fade !== undefined) { | ||
| return ( | ||
| <ActionIcon size={20}> | ||
| <RadialSweepIcon value={fade * 100} background={'transparent'} size={20} /> |
There was a problem hiding this comment.
This is a fun detail, but maybe we should check with the team if this type of "status icon" is something we want.
Mentioning because I once made a version that communicates the status, but this was replaced with just a regular checkbox after feedback 😅 IIRC the comment was something like "the users don't care about the current fade state", and that we should keep it simple
There was a problem hiding this comment.
Ultimately iirc I came down to that it was hard to keep consistent and also introduced a lot of rerenders that had some performance input 🤔
If both of these are not an issue, I wouldn't see a problem add the marker. I see it more of "visual flair that something is happening" rather than indicating the exact state
|
Found another issue - error when opening layers: 2026-02-04.15-46-34.movI was planning on testing this issue OpenSpace/OpenSpace#3868, which seems like it could be related to the layers not being updated in the store correctly once removed 🤔 Seemed like it might be relevant to this PR, but since it is not possible to open the layers list, I could not try it with this PR |
src/hooks/sceneGraphNodes/hooks.ts
Outdated
| useAppSelector((state) => isSgnFocusable(uri, state.properties.properties)) || false | ||
| useAppSelector((state) => | ||
| isSgnFocusable(uri, propertySelectors.selectEntities(state)) | ||
| ) || false |
There was a problem hiding this comment.
I guess React question... why is the || false necessary here?
There was a problem hiding this comment.
It isn't :) Good catch
This is not guaranteed to run after the propertyowners are added
|
Fixed the reported issues now - layers + adding and removing scene graph nodes should now work. OpenSpace/OpenSpace#3868 is not going to be fixed with this PR as it is related to an issue with the |


A bunch of performance optimizations regarding the data passing to and from the UI with the purpose of doing a long-term solve for the UI meltdown.
So for testing, try and make the meltdown happen!
entityAdapters. This is because we were pretty much implementing that anyway, but now we can instead rely on RTK and get some nice selectors generated for us.PropertyBatcherclass. This essentially functions as a throttle, but without the risk of missing values.To be used with engine PR
Some future work:
useAppSelector. This is suboptimal as whenever any property changes, this will trigger a re-render. I started to look into this but this PR is big enough as it is. Also I think it will require a big refactor of the scene tree hooks. To extract all propertyOwners is not as bad, as the propertyOwner state doesn't change that often.connectionMiddlewareinstead of the middlewares for the topics. Now, all of them fire ononConnection, but we don't have a lot of control over which fire first. This would help to make the UI feel way snappier on startup, probably by first getting the property tree as it basically freezes everything for a bit.Recommended reading: https://redux-toolkit.js.org/api/createEntityAdapter
Edit:
PropertyBatcheris therefore renamed toBatcher