Enhance Properties panel section behavior and persistence - #3787
Conversation
Summary of ChangesHello @jsjgdh, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the user experience of the properties panel by enabling sections to be collapsed and expanded, with their state being persisted. It also introduces convenient shortcuts for managing the expansion of all sections at once, improving navigation and focus within complex property layouts. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a great enhancement to the Properties panel by making sections collapsible and persisting their state. The implementation is well-structured, touching both the backend state management and the frontend UI components. The use of Alt+click to expand/collapse all sections is a nice UX improvement.
My main feedback is regarding the implementation of the "expand/collapse all" feature, which currently has a bug where it doesn't affect sections that haven't been individually toggled. I've left detailed comments with suggestions on how to address this by introducing a global expansion state.
Overall, this is a solid contribution that improves user experience. Addressing the issue I've pointed out will make it even more robust.
| #[derive(Debug, Clone, Default, ExtractField)] | ||
| pub struct PropertiesPanelMessageHandler {} | ||
| pub struct PropertiesPanelMessageHandler { | ||
| pub section_expanded: HashMap<u64, bool>, | ||
| } |
There was a problem hiding this comment.
The current implementation of SetAllSectionsExpanded has a flaw where it only affects sections that have been individually toggled. To fix this, I suggest adding a field to track a global expansion state. I'll leave another comment on the message handlers with more details.
| #[derive(Debug, Clone, Default, ExtractField)] | |
| pub struct PropertiesPanelMessageHandler {} | |
| pub struct PropertiesPanelMessageHandler { | |
| pub section_expanded: HashMap<u64, bool>, | |
| } | |
| #[derive(Debug, Clone, Default, ExtractField)] | |
| pub struct PropertiesPanelMessageHandler { | |
| pub section_expanded: HashMap<u64, bool>, | |
| pub all_sections_expanded: Option<bool>, | |
| } |
| PropertiesPanelMessage::SetAllSectionsExpanded { expanded } => { | ||
| for value in self.section_expanded.values_mut() { | ||
| *value = expanded; | ||
| } | ||
| responses.add(PropertiesPanelMessage::Refresh); | ||
| } | ||
| PropertiesPanelMessage::SetSectionExpanded { node_id, expanded } => { | ||
| self.section_expanded.insert(node_id, expanded); | ||
| responses.add(PropertiesPanelMessage::Refresh); | ||
| } |
There was a problem hiding this comment.
Using the new all_sections_expanded field, this updated logic correctly handles the global expand/collapse behavior.
SetAllSectionsExpandednow acts as a global override by settingall_sections_expandedand clearing individual states.SetSectionExpandedclears the global override, allowing individual control again.
To complete this change, you'll also need to:
- Pass
self.all_sections_expandedtoNodePropertiesContextin theRefreshhandler (this requires addingall_sections_expanded: Option<bool>toNodePropertiesContext). - In
generate_node_properties, prioritizeall_sections_expandedwhen determining a section'sexpandedstate.
| PropertiesPanelMessage::SetAllSectionsExpanded { expanded } => { | |
| for value in self.section_expanded.values_mut() { | |
| *value = expanded; | |
| } | |
| responses.add(PropertiesPanelMessage::Refresh); | |
| } | |
| PropertiesPanelMessage::SetSectionExpanded { node_id, expanded } => { | |
| self.section_expanded.insert(node_id, expanded); | |
| responses.add(PropertiesPanelMessage::Refresh); | |
| } | |
| PropertiesPanelMessage::SetAllSectionsExpanded { expanded } => { | |
| self.all_sections_expanded = Some(expanded); | |
| self.section_expanded.clear(); | |
| responses.add(PropertiesPanelMessage::Refresh); | |
| } | |
| PropertiesPanelMessage::SetSectionExpanded { node_id, expanded } => { | |
| self.all_sections_expanded = None; | |
| self.section_expanded.insert(node_id, expanded); | |
| responses.add(PropertiesPanelMessage::Refresh); | |
| } |
3697b7d to
90533e6
Compare
9b97ab7 to
2e842cb
Compare
f07c79b to
76938eb
Compare
4b7a823 to
847b8e9
Compare
15fcaac to
d5f0140
Compare
|
@jsjgdh I have started the AI code review. It will take a few minutes to complete. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
8 issues found across 14 files
Confidence score: 2/5
- In
editor/src/messages/portfolio/document/utility_types/network_interface/types.rsandeditor/src/messages/portfolio/document/utility_types/network_interface/template.rs, thecollapsedstate is not consistently carried through metadata andNodeTemplate::from_parts, so users can lose explicit Properties-panel collapse choices after save/reopen or template split/join cycles — threadcollapsedthrough all serialize/deserialize and join paths. ToggleNodePropertiesSectionExpandedineditor/src/messages/portfolio/document/document_message_handler.rsandSetCollapsedhandling ineditor/src/messages/portfolio/document/node_graph/node_graph_message_handler.rsdisagree on network path (&[]vsselection_network_path), which can make root section toggles fail in nested selections — align both read/write operations to the same target network path.editor/src/messages/portfolio/document/graph_operation/utility_types.rsappears to fan out per-node Properties panel side effects during bulk SVG import, risking UI churn and heavy graph/autosave traffic on large files — batch or defer collapse/side-effect messages during bulk operations.- In
editor/src/messages/portfolio/document/document_message_handler.rs, older documents can drop prior collapse preferences because legacy fields are no longer migrated intopersistent_metadata.collapsed— keep a compatibility serde field and migrate IDs on load to preserve user state.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="editor/src/messages/portfolio/document/utility_types/network_interface/types.rs">
<violation number="1" location="editor/src/messages/portfolio/document/utility_types/network_interface/types.rs:603">
P1: Explicit Properties-panel collapse choices are lost on persistence, so reopening a document can restore a section to the implementation default instead of the user’s state. Carry `collapsed` through the GDD metadata and template conversion paths, or intentionally persist it in the existing session view metadata.</violation>
</file>
<file name="editor/src/messages/portfolio/document/node_graph/node_graph_message_handler.rs">
<violation number="1" location="editor/src/messages/portfolio/document/node_graph/node_graph_message_handler.rs:2021">
P2: Root Properties sections can fail to expand or collapse when the current selection is inside a nested network: `SetCollapsed` targets `selection_network_path`, even though the document-level toggle explicitly targets the root network. Carry the node's containing network path in `SetCollapsed` (and pass it from each caller) so the mutation and the read use the same target.</violation>
</file>
<file name="editor/src/messages/portfolio/document/document_message_handler.rs">
<violation number="1" location="editor/src/messages/portfolio/document/document_message_handler.rs:252">
P2: Opening a document saved by the previous editor drops its Properties-panel collapse preferences. Retaining a legacy serde field and migrating its node IDs into `persistent_metadata.collapsed` during deserialization would preserve existing documents.</violation>
<violation number="2" location="editor/src/messages/portfolio/document/document_message_handler.rs:252">
P3: The `properties_panel_collapsed_sections` field on `NodePropertiesContext` and `PropertiesPanelMessageContext` is now dead code. The collapsed state is read from `network_interface.is_collapsed()` instead (`node_properties.rs:2464`). The field is always passed as `&[]` and never consumed. Remove it from both context structs and all construction sites to avoid confusion.</violation>
<violation number="3" location="editor/src/messages/portfolio/document/document_message_handler.rs:1409">
P2: `ToggleNodePropertiesSectionExpanded` checks the collapsed state in the document network (`&[]`), but the subsequent `SetCollapsed` message will be processed with `selection_network_path` from the `NodeGraphMessageHandler` context, which may point to a nested network. If the user is working inside a nested network (e.g. a group layer), `is_collapsed(&node_id, &[])` won't find the node there and returns `false` (default), so the toggle always computes `collapsed = true` — the section stays collapsed regardless of the previous state, breaking the toggle for nested-network nodes. The read and write must use the same network path.</violation>
</file>
<file name="editor/src/messages/portfolio/document/graph_operation/utility_types.rs">
<violation number="1" location="editor/src/messages/portfolio/document/graph_operation/utility_types.rs:65">
P2: During bulk SVG import this queues a large flood of PropertiesPanelMessage cascades (SetCollapsed + SetLockedOrVisibilitySideEffects → RunDocumentGraph/SendGraph/UpdateLayerPanel/AutoSave/Refresh) for every created layer, since create_layer is invoked once per node. The collapsed section state is only needed for layers the user creates interactively, so guard the emission with `self.import` to skip it on bulk import.</violation>
</file>
<file name="editor/src/messages/portfolio/document/utility_types/network_interface/view.rs">
<violation number="1" location="editor/src/messages/portfolio/document/utility_types/network_interface/view.rs:189">
P3: This compares the implementation name against a hardcoded `"Merge"` literal, even though the same PR adds a public `MERGE_NODE_IDENTIFIER: &str = "Merge"` constant in `document_node_definitions.rs` and the merge node definition now uses it. Reusing that constant here keeps the merge-node identity in one place so a future rename of the node identifier doesn't silently break the default-collapsed behavior for merge nodes.</violation>
</file>
<file name="editor/src/messages/portfolio/document/utility_types/network_interface/template.rs">
<violation number="1" location="editor/src/messages/portfolio/document/utility_types/network_interface/template.rs:193">
P1: The new `collapsed` state is silently lost whenever a node passes through the `NodeTemplate` split/join cycle, defeating the persistence this PR is adding. `NodeTemplate::from_parts` drops the field on join (`collapsed: _`) and `into_parts` unconditionally writes `collapsed: None` on split. `NodeTemplate::normalize_stored_types` round-trips every nested node through these two methods and is called on every document open (`document_migration_upgrades` -> `normalize_stored_types` at document_migration.rs:1134), so any collapse preference the user saves is reset to `None` on the next load. Since `collapsed` is part of `DocumentNodePersistentMetadata` (serde-persisted), it should be carried through the template shape like `pinned`/`locked` rather than dropped.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| #[serde(default)] | ||
| pub pinned: bool, | ||
| #[serde(default)] | ||
| pub collapsed: Option<bool>, |
There was a problem hiding this comment.
P1: Explicit Properties-panel collapse choices are lost on persistence, so reopening a document can restore a section to the implementation default instead of the user’s state. Carry collapsed through the GDD metadata and template conversion paths, or intentionally persist it in the existing session view metadata.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/utility_types/network_interface/types.rs, line 603:
<comment>Explicit Properties-panel collapse choices are lost on persistence, so reopening a document can restore a section to the implementation default instead of the user’s state. Carry `collapsed` through the GDD metadata and template conversion paths, or intentionally persist it in the existing session view metadata.</comment>
<file context>
@@ -599,6 +599,8 @@ pub struct DocumentNodePersistentMetadata {
#[serde(default)]
pub pinned: bool,
+ #[serde(default)]
+ pub collapsed: Option<bool>,
/// Metadata that is specific to either nodes or layers, which are chosen states for displaying as a left-to-right node or bottom-to-top layer.
/// All fields in NodeTypePersistentMetadata should automatically be updated by using the network interface API
</file context>
| output_names, | ||
| locked, | ||
| pinned, | ||
| collapsed: None, |
There was a problem hiding this comment.
P1: The new collapsed state is silently lost whenever a node passes through the NodeTemplate split/join cycle, defeating the persistence this PR is adding. NodeTemplate::from_parts drops the field on join (collapsed: _) and into_parts unconditionally writes collapsed: None on split. NodeTemplate::normalize_stored_types round-trips every nested node through these two methods and is called on every document open (document_migration_upgrades -> normalize_stored_types at document_migration.rs:1134), so any collapse preference the user saves is reset to None on the next load. Since collapsed is part of DocumentNodePersistentMetadata (serde-persisted), it should be carried through the template shape like pinned/locked rather than dropped.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/utility_types/network_interface/template.rs, line 193:
<comment>The new `collapsed` state is silently lost whenever a node passes through the `NodeTemplate` split/join cycle, defeating the persistence this PR is adding. `NodeTemplate::from_parts` drops the field on join (`collapsed: _`) and `into_parts` unconditionally writes `collapsed: None` on split. `NodeTemplate::normalize_stored_types` round-trips every nested node through these two methods and is called on every document open (`document_migration_upgrades` -> `normalize_stored_types` at document_migration.rs:1134), so any collapse preference the user saves is reset to `None` on the next load. Since `collapsed` is part of `DocumentNodePersistentMetadata` (serde-persisted), it should be carried through the template shape like `pinned`/`locked` rather than dropped.</comment>
<file context>
@@ -189,6 +190,7 @@ impl NodeTemplate {
output_names,
locked,
pinned,
+ collapsed: None,
node_type_metadata,
network_metadata,
</file context>
| network_interface.set_pinned(&node_id, selection_network_path, pinned); | ||
| } | ||
| NodeGraphMessage::SetCollapsed { node_id, collapsed } => { | ||
| network_interface.set_collapsed(&node_id, selection_network_path, collapsed); |
There was a problem hiding this comment.
P2: Root Properties sections can fail to expand or collapse when the current selection is inside a nested network: SetCollapsed targets selection_network_path, even though the document-level toggle explicitly targets the root network. Carry the node's containing network path in SetCollapsed (and pass it from each caller) so the mutation and the read use the same target.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/node_graph/node_graph_message_handler.rs, line 2021:
<comment>Root Properties sections can fail to expand or collapse when the current selection is inside a nested network: `SetCollapsed` targets `selection_network_path`, even though the document-level toggle explicitly targets the root network. Carry the node's containing network path in `SetCollapsed` (and pass it from each caller) so the mutation and the read use the same target.</comment>
<file context>
@@ -2023,6 +2017,9 @@ impl<'a> MessageHandler<NodeGraphMessage, NodeGraphMessageContext<'a>> for NodeG
network_interface.set_pinned(&node_id, selection_network_path, pinned);
}
+ NodeGraphMessage::SetCollapsed { node_id, collapsed } => {
+ network_interface.set_collapsed(&node_id, selection_network_path, collapsed);
+ }
NodeGraphMessage::SetVisibility { node_id, network_path, visible } => {
</file context>
| fonts, | ||
| properties_panel_open, | ||
| properties_panel_collapsed_sections: &self.properties_panel_collapsed_sections, | ||
| properties_panel_collapsed_sections: &[], |
There was a problem hiding this comment.
P2: Opening a document saved by the previous editor drops its Properties-panel collapse preferences. Retaining a legacy serde field and migrating its node IDs into persistent_metadata.collapsed during deserialization would preserve existing documents.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/document_message_handler.rs, line 252:
<comment>Opening a document saved by the previous editor drops its Properties-panel collapse preferences. Retaining a legacy serde field and migrating its node IDs into `persistent_metadata.collapsed` during deserialization would preserve existing documents.</comment>
<file context>
@@ -253,7 +249,7 @@ impl MessageHandler<DocumentMessage, DocumentMessageContext<'_>> for DocumentMes
fonts,
properties_panel_open,
- properties_panel_collapsed_sections: &self.properties_panel_collapsed_sections,
+ properties_panel_collapsed_sections: &[],
};
self.properties_panel_message_handler.process_message(message, responses, context);
</file context>
| pub fn create_layer(&mut self, new_id: NodeId) -> LayerNodeIdentifier { | ||
| let new_merge_node = resolve_network_node_type("Merge").expect("Merge node").default_node_template(); | ||
| self.network_interface.insert_node(new_id, new_merge_node, &[]); | ||
| self.responses.add(PropertiesPanelMessage::SetSectionExpanded { node_id: new_id.0, expanded: false }); |
There was a problem hiding this comment.
P2: During bulk SVG import this queues a large flood of PropertiesPanelMessage cascades (SetCollapsed + SetLockedOrVisibilitySideEffects → RunDocumentGraph/SendGraph/UpdateLayerPanel/AutoSave/Refresh) for every created layer, since create_layer is invoked once per node. The collapsed section state is only needed for layers the user creates interactively, so guard the emission with self.import to skip it on bulk import.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/graph_operation/utility_types.rs, line 65:
<comment>During bulk SVG import this queues a large flood of PropertiesPanelMessage cascades (SetCollapsed + SetLockedOrVisibilitySideEffects → RunDocumentGraph/SendGraph/UpdateLayerPanel/AutoSave/Refresh) for every created layer, since create_layer is invoked once per node. The collapsed section state is only needed for layers the user creates interactively, so guard the emission with `self.import` to skip it on bulk import.</comment>
<file context>
@@ -62,6 +62,7 @@ impl<'a> ModifyInputsContext<'a> {
pub fn create_layer(&mut self, new_id: NodeId) -> LayerNodeIdentifier {
let new_merge_node = resolve_network_node_type("Merge").expect("Merge node").default_node_template();
self.network_interface.insert_node(new_id, new_merge_node, &[]);
+ self.responses.add(PropertiesPanelMessage::SetSectionExpanded { node_id: new_id.0, expanded: false });
LayerNodeIdentifier::new(new_id, self.network_interface)
}
</file context>
| self.properties_panel_collapsed_sections.push(node_id); | ||
| } | ||
| responses.add(PropertiesPanelMessage::Refresh); | ||
| let collapsed = !self.network_interface.is_collapsed(&node_id, &[]); |
There was a problem hiding this comment.
P2: ToggleNodePropertiesSectionExpanded checks the collapsed state in the document network (&[]), but the subsequent SetCollapsed message will be processed with selection_network_path from the NodeGraphMessageHandler context, which may point to a nested network. If the user is working inside a nested network (e.g. a group layer), is_collapsed(&node_id, &[]) won't find the node there and returns false (default), so the toggle always computes collapsed = true — the section stays collapsed regardless of the previous state, breaking the toggle for nested-network nodes. The read and write must use the same network path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/document_message_handler.rs, line 1409:
<comment>`ToggleNodePropertiesSectionExpanded` checks the collapsed state in the document network (`&[]`), but the subsequent `SetCollapsed` message will be processed with `selection_network_path` from the `NodeGraphMessageHandler` context, which may point to a nested network. If the user is working inside a nested network (e.g. a group layer), `is_collapsed(&node_id, &[])` won't find the node there and returns `false` (default), so the toggle always computes `collapsed = true` — the section stays collapsed regardless of the previous state, breaking the toggle for nested-network nodes. The read and write must use the same network path.</comment>
<file context>
@@ -1411,12 +1406,12 @@ impl MessageHandler<DocumentMessage, DocumentMessageContext<'_>> for DocumentMes
- self.properties_panel_collapsed_sections.push(node_id);
- }
- responses.add(PropertiesPanelMessage::Refresh);
+ let collapsed = !self.network_interface.is_collapsed(&node_id, &[]);
+ responses.add(NodeGraphMessage::SetCollapsed { node_id, collapsed });
+ responses.add(NodeGraphMessage::SetLockedOrVisibilitySideEffects {
</file context>
|
|
||
| pub fn is_collapsed(&self, node_id: &NodeId) -> Result<bool, NetworkError> { | ||
| let node_metadata = self.node_metadata(node_id)?; | ||
| let collapsed = node_metadata.persistent_metadata.collapsed.unwrap_or_else(|| self.implementation_name(node_id) == "Merge"); |
There was a problem hiding this comment.
P3: This compares the implementation name against a hardcoded "Merge" literal, even though the same PR adds a public MERGE_NODE_IDENTIFIER: &str = "Merge" constant in document_node_definitions.rs and the merge node definition now uses it. Reusing that constant here keeps the merge-node identity in one place so a future rename of the node identifier doesn't silently break the default-collapsed behavior for merge nodes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/utility_types/network_interface/view.rs, line 189:
<comment>This compares the implementation name against a hardcoded `"Merge"` literal, even though the same PR adds a public `MERGE_NODE_IDENTIFIER: &str = "Merge"` constant in `document_node_definitions.rs` and the merge node definition now uses it. Reusing that constant here keeps the merge-node identity in one place so a future rename of the node identifier doesn't silently break the default-collapsed behavior for merge nodes.</comment>
<file context>
@@ -184,6 +184,12 @@ impl<'a, 'p> NetworkView<'a, 'p> {
+ pub fn is_collapsed(&self, node_id: &NodeId) -> Result<bool, NetworkError> {
+ let node_metadata = self.node_metadata(node_id)?;
+ let collapsed = node_metadata.persistent_metadata.collapsed.unwrap_or_else(|| self.implementation_name(node_id) == "Merge");
+ Ok(collapsed)
+ }
</file context>
| fonts, | ||
| properties_panel_open, | ||
| properties_panel_collapsed_sections: &self.properties_panel_collapsed_sections, | ||
| properties_panel_collapsed_sections: &[], |
There was a problem hiding this comment.
P3: The properties_panel_collapsed_sections field on NodePropertiesContext and PropertiesPanelMessageContext is now dead code. The collapsed state is read from network_interface.is_collapsed() instead (node_properties.rs:2464). The field is always passed as &[] and never consumed. Remove it from both context structs and all construction sites to avoid confusion.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/document_message_handler.rs, line 252:
<comment>The `properties_panel_collapsed_sections` field on `NodePropertiesContext` and `PropertiesPanelMessageContext` is now dead code. The collapsed state is read from `network_interface.is_collapsed()` instead (`node_properties.rs:2464`). The field is always passed as `&[]` and never consumed. Remove it from both context structs and all construction sites to avoid confusion.</comment>
<file context>
@@ -253,7 +249,7 @@ impl MessageHandler<DocumentMessage, DocumentMessageContext<'_>> for DocumentMes
fonts,
properties_panel_open,
- properties_panel_collapsed_sections: &self.properties_panel_collapsed_sections,
+ properties_panel_collapsed_sections: &[],
};
self.properties_panel_message_handler.process_message(message, responses, context);
</file context>
There was a problem hiding this comment.
3 existing issues remain and 3 new issues found across 14 files
Confidence score: 3/5
- In
editor/src/messages/portfolio/document/node_graph/node_graph_message_handler.rs, Properties section expand/collapse mutates persistent metadata without a guaranteed document transaction, so users can make UI state changes that are not undoable and desync expected history behavior — start a document transaction before dispatchingSetCollapsed. - In
editor/src/messages/portfolio/document/graph_operation/utility_types.rs,create_layernow always enqueuesPropertiesPanelMessage::SetSectionExpanded, which fans out intoNodeGraphMessage::SetCollapsedand side-effect messages that can trigger extra RunDocumentGraph work; this can introduce unexpected graph churn during layer creation — gate or defer this message so it only fires when section state actually changes. - In
editor/src/messages/portfolio/document/utility_types/network_interface/template.rs, NodeTemplate conversion drops section collapsed state (from_partsdiscards it andinto_partsforcesNone), so template round-trips can reset panel state and produce confusing UI regressions — preserve and serialize the collapsed value through both conversion directions. - In
editor/src/messages/portfolio/document/utility_types/network_interface/view.rsand related node-graph files, merge-node checks are split betweenis_merge()and hardcoded"Merge"/new constant usage, increasing drift risk where behavior diverges across call sites — standardize on one identifier path (preferis_merge()/shared constant) and remove duplicate string comparisons.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="editor/src/messages/portfolio/document/node_graph/document_node_definitions.rs">
<violation number="1" location="editor/src/messages/portfolio/document/node_graph/document_node_definitions.rs:26">
P3: The new pub const MERGE_NODE_IDENTIFIER is used in only one place, while the identical literal "Merge" remains hardcoded in sibling comparisons (e.g. document_node_derive.rs, view.rs is_merge/is_collapsed, document_migration.rs, graph_modification_utils.rs). Since the constant names the shared Merge network identifier, using it consistently avoids future drift if the display/identifier value ever changes. Consider referencing MERGE_NODE_IDENTIFIER in those checks as well, or dropping the constant if only this one site is intended.</violation>
</file>
<file name="editor/src/messages/portfolio/document/node_graph/node_graph_message_handler.rs">
<violation number="1" location="editor/src/messages/portfolio/document/node_graph/node_graph_message_handler.rs:2027">
P2: Expanding or collapsing a Properties section is not undoable because this handler mutates the persistent node metadata without ensuring a transaction has started. Start a document transaction before dispatching `SetCollapsed` from each direct caller, or provide a transaction-aware collapse command.</violation>
</file>
<file name="editor/src/messages/portfolio/document/utility_types/network_interface/view.rs">
<violation number="1" location="editor/src/messages/portfolio/document/utility_types/network_interface/view.rs:197">
P3: The fallback identity check `self.implementation_name(node_id) == "Merge"` duplicates the canonical `is_merge()` method defined just above in this same impl block, which compares the node's `DefinitionIdentifier` against `DefinitionIdentifier::Network("Merge")`. Using the human-display string for identity is fragile and redundant: it breaks if the Merge node's display name ever differs from its registry key (the type docs note network display names "don't necessarily have to be the same"), and it re-implements logic already centralized in `is_merge()`. Replace with `self.is_merge(node_id)`.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 3 unresolved issues already reported by Cubic.
Re-trigger cubic
| network_interface.set_pinned(&node_id, selection_network_path, pinned); | ||
| } | ||
| NodeGraphMessage::SetCollapsed { node_id, collapsed } => { | ||
| network_interface.set_collapsed(&node_id, selection_network_path, collapsed); |
There was a problem hiding this comment.
P2: Expanding or collapsing a Properties section is not undoable because this handler mutates the persistent node metadata without ensuring a transaction has started. Start a document transaction before dispatching SetCollapsed from each direct caller, or provide a transaction-aware collapse command.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/node_graph/node_graph_message_handler.rs, line 2027:
<comment>Expanding or collapsing a Properties section is not undoable because this handler mutates the persistent node metadata without ensuring a transaction has started. Start a document transaction before dispatching `SetCollapsed` from each direct caller, or provide a transaction-aware collapse command.</comment>
<file context>
@@ -2029,6 +2023,9 @@ impl<'a> MessageHandler<NodeGraphMessage, NodeGraphMessageContext<'a>> for NodeG
network_interface.set_pinned(&node_id, selection_network_path, pinned);
}
+ NodeGraphMessage::SetCollapsed { node_id, collapsed } => {
+ network_interface.set_collapsed(&node_id, selection_network_path, collapsed);
+ }
NodeGraphMessage::SetVisibility { node_id, network_path, visible } => {
</file context>
| use graphene_std::*; | ||
| use std::collections::{HashMap, VecDeque}; | ||
|
|
||
| pub const MERGE_NODE_IDENTIFIER: &str = "Merge"; |
There was a problem hiding this comment.
P3: The new pub const MERGE_NODE_IDENTIFIER is used in only one place, while the identical literal "Merge" remains hardcoded in sibling comparisons (e.g. document_node_derive.rs, view.rs is_merge/is_collapsed, document_migration.rs, graph_modification_utils.rs). Since the constant names the shared Merge network identifier, using it consistently avoids future drift if the display/identifier value ever changes. Consider referencing MERGE_NODE_IDENTIFIER in those checks as well, or dropping the constant if only this one site is intended.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/node_graph/document_node_definitions.rs, line 26:
<comment>The new pub const MERGE_NODE_IDENTIFIER is used in only one place, while the identical literal "Merge" remains hardcoded in sibling comparisons (e.g. document_node_derive.rs, view.rs is_merge/is_collapsed, document_migration.rs, graph_modification_utils.rs). Since the constant names the shared Merge network identifier, using it consistently avoids future drift if the display/identifier value ever changes. Consider referencing MERGE_NODE_IDENTIFIER in those checks as well, or dropping the constant if only this one site is intended.</comment>
<file context>
@@ -23,6 +23,8 @@ use graphene_std::vector::Vector;
use graphene_std::*;
use std::collections::{HashMap, VecDeque};
+pub const MERGE_NODE_IDENTIFIER: &str = "Merge";
+
pub struct NodePropertiesContext<'a> {
</file context>
|
|
||
| pub fn is_collapsed(&self, node_id: &NodeId) -> Result<bool, NetworkError> { | ||
| let node_metadata = self.node_metadata(node_id)?; | ||
| let collapsed = node_metadata.persistent_metadata.collapsed.unwrap_or_else(|| self.implementation_name(node_id) == "Merge"); |
There was a problem hiding this comment.
P3: The fallback identity check self.implementation_name(node_id) == "Merge" duplicates the canonical is_merge() method defined just above in this same impl block, which compares the node's DefinitionIdentifier against DefinitionIdentifier::Network("Merge"). Using the human-display string for identity is fragile and redundant: it breaks if the Merge node's display name ever differs from its registry key (the type docs note network display names "don't necessarily have to be the same"), and it re-implements logic already centralized in is_merge(). Replace with self.is_merge(node_id).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/portfolio/document/utility_types/network_interface/view.rs, line 197:
<comment>The fallback identity check `self.implementation_name(node_id) == "Merge"` duplicates the canonical `is_merge()` method defined just above in this same impl block, which compares the node's `DefinitionIdentifier` against `DefinitionIdentifier::Network("Merge")`. Using the human-display string for identity is fragile and redundant: it breaks if the Merge node's display name ever differs from its registry key (the type docs note network display names "don't necessarily have to be the same"), and it re-implements logic already centralized in `is_merge()`. Replace with `self.is_merge(node_id)`.</comment>
<file context>
@@ -192,6 +192,12 @@ impl<'a, 'p> NetworkView<'a, 'p> {
+ pub fn is_collapsed(&self, node_id: &NodeId) -> Result<bool, NetworkError> {
+ let node_metadata = self.node_metadata(node_id)?;
+ let collapsed = node_metadata.persistent_metadata.collapsed.unwrap_or_else(|| self.implementation_name(node_id) == "Merge");
+ Ok(collapsed)
+ }
</file context>
2 participants
discussion : https://discord.com/channels/@me/1461994742291234997/1473618001944252530 from DM with @Keavon