Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive set of string manipulation nodes to the node graph, including functionality for capitalization, joining, padding, repeating, and searching. It also updates the UI to support these nodes, specifically adding a property editor for string capitalization with joiner presets, and refines the string_slice node to use integer types. Review feedback pointed out a bug in the unescaping logic for string separators where the order of replacements could lead to incorrect results, and suggested improvements to the capitalization behavior when word-splitting is disabled to ensure consistency and standard formatting.
There was a problem hiding this comment.
4 issues found across 8 files
Confidence score: 3/5
- There is concrete user-impact risk in
node-graph/nodes/gcore/src/logic.rs: Unicode case conversion keeps only the first character fromto_uppercase()/to_lowercase(), which can produce incorrect text for some characters/locales. editor/src/messages/portfolio/document/node_graph/node_properties.rshas a UI logic inconsistency whereUseJoinercan be hidden whenJoinerInputis exposed, andnode-graph/nodes/gcore/src/logic.rsalso has ause_joinerbehavior mismatch forLowerCase/UpperCase; together these are moderate regressions but likely localized.- The PR title format issue in
node-graph/nodes/gcore/Cargo.tomlis process-related rather than runtime functionality, so it should be fixed but does not by itself indicate breakage. - Pay close attention to
node-graph/nodes/gcore/src/logic.rsandeditor/src/messages/portfolio/document/node_graph/node_properties.rs- text transformation correctness and joiner control visibility/behavior need validation before merge.
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="node-graph/nodes/gcore/Cargo.toml">
<violation number="1" location="node-graph/nodes/gcore/Cargo.toml:31">
P1: Custom agent: **PR title enforcement**
PR title does not follow the required "New nodes" dedicated format. Rename it to `New nodes: 'Name1', 'Name2', and 'Name3'` (with actual node names in single quotes).</violation>
</file>
<file name="editor/src/messages/portfolio/document/node_graph/node_properties.rs">
<violation number="1" location="editor/src/messages/portfolio/document/node_graph/node_properties.rs:1628">
P2: `UseJoiner` control is conditionally hidden when `JoinerInput` is exposed, because both controls are gated on `joiner_value` being non-exposed.</violation>
</file>
<file name="node-graph/nodes/gcore/src/logic.rs">
<violation number="1" location="node-graph/nodes/gcore/src/logic.rs:222">
P2: `use_joiner` is documented as applying to capitalization conversion, but `LowerCase`/`UpperCase` ignore `joiner` when it is enabled.
(Based on your team's feedback about aligning behavior with comments and TODO expectations.) [FEEDBACK_USED]</violation>
<violation number="2" location="node-graph/nodes/gcore/src/logic.rs:234">
P2: Unicode case conversion is lossy here because only the first character of `to_uppercase()`/`to_lowercase()` is kept.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
3 issues found across 16 files (changes from recent commits).
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="node-graph/nodes/text/src/lib.rs">
<violation number="1" location="node-graph/nodes/text/src/lib.rs:156">
P3: `first.clone()` is unnecessary — `first` is already an owned `String` and `+` consumes it, reusing its buffer. The clone wastes an allocation.</violation>
<violation number="2" location="node-graph/nodes/text/src/lib.rs:799">
P2: `regex_find` has an empty category `""` while every other regex node uses `"Text: Regex"`. This will cause the node to be miscategorized in the UI.</violation>
</file>
<file name="node-graph/nodes/math/Cargo.toml">
<violation number="1" location="node-graph/nodes/math/Cargo.toml:12">
P1: Custom agent: **PR title enforcement**
PR title violates the PR title rule’s "Special Commit Types → New nodes" format: new-node PRs must be titled as `New node: 'Node Name'` or `New nodes: 'Name1', 'Name2', and 'Name3'`, not `Add new string processing nodes`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Still a bit more work to do to get this landed in the next day or so.