Conversation
![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 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.
| let separator = if separator_escaping { | ||
| separator.replace("\\n", "\n").replace("\\r", "\r").replace("\\t", "\t").replace("\\0", "\0").replace("\\\\", "\\") | ||
| } else { | ||
| separator | ||
| }; |
There was a problem hiding this comment.
The chained replace() calls for unescaping the separator string are buggy. The order of replacements matters, and the current order can lead to incorrect results. For example, an input of "\\\\n" (meant to be a literal backslash followed by 'n') will be incorrectly converted to a newline character.
A more robust and efficient approach is to iterate through the string once and handle escape sequences manually. This avoids ordering issues and correctly handles all cases, including unrecognized escape sequences.
let separator = if separator_escaping {
let mut unescaped = String::with_capacity(separator.len());
let mut chars = separator.chars();
while let Some(c) = chars.next() {
if c == '\\' {
match chars.next() {
Some('n') => unescaped.push('\n'),
Some('r') => unescaped.push('\r'),
Some('t') => unescaped.push('\t'),
Some('0') => unescaped.push('\0'),
Some('\\') => unescaped.push('\\'),
Some(other) => {
unescaped.push('\\');
unescaped.push(other);
}
None => unescaped.push('\\'),
}
} else {
unescaped.push(c);
}
}
unescaped
} else {
separator
};| StringCapitalization::CapitalCase | StringCapitalization::HeadlineCase => { | ||
| let mut capitalize_next = true; | ||
| string | ||
| .chars() | ||
| .map(|c| { | ||
| if c.is_whitespace() || c == '_' || c == '-' { | ||
| capitalize_next = true; | ||
| c | ||
| } else if capitalize_next { | ||
| capitalize_next = false; | ||
| c.to_uppercase().next().unwrap_or(c) | ||
| } else { | ||
| c | ||
| } | ||
| }) | ||
| .collect() | ||
| } |
There was a problem hiding this comment.
The implementation for CapitalCase and HeadlineCase when use_joiner is disabled is inconsistent with the documentation and the behavior when use_joiner is enabled. It only uppercases the first letter of each word but doesn't lowercase the rest. This can lead to unexpected results (e.g., hELLO becomes HELLO instead of Hello).
To make the behavior consistent, you should also lowercase the other letters in each word. This would also make HeadlineCase a reasonable alias for CapitalCase in this branch, as implementing true headline casing without the titlecase crate is complex.
| StringCapitalization::CapitalCase | StringCapitalization::HeadlineCase => { | |
| let mut capitalize_next = true; | |
| string | |
| .chars() | |
| .map(|c| { | |
| if c.is_whitespace() || c == '_' || c == '-' { | |
| capitalize_next = true; | |
| c | |
| } else if capitalize_next { | |
| capitalize_next = false; | |
| c.to_uppercase().next().unwrap_or(c) | |
| } else { | |
| c | |
| } | |
| }) | |
| .collect() | |
| } | |
| StringCapitalization::CapitalCase | StringCapitalization::HeadlineCase => { | |
| let mut capitalize_next = true; | |
| string | |
| .chars() | |
| .map(|c| { | |
| if c.is_whitespace() || c == '_' || c == '-' { | |
| capitalize_next = true; | |
| c | |
| } else if capitalize_next { | |
| capitalize_next = false; | |
| c.to_uppercase().next().unwrap_or(c) | |
| } else { | |
| c.to_lowercase().next().unwrap_or(c) | |
| } | |
| }) | |
| .collect() | |
| } |
| StringCapitalization::CamelCase => { | ||
| let mut capitalize_next = false; | ||
| string | ||
| .chars() | ||
| .map(|c| { | ||
| if c.is_whitespace() || c == '_' || c == '-' { | ||
| capitalize_next = true; | ||
| c | ||
| } else if capitalize_next { | ||
| capitalize_next = false; | ||
| c.to_uppercase().next().unwrap_or(c) | ||
| } else { | ||
| c.to_lowercase().next().unwrap_or(c) | ||
| } | ||
| }) | ||
| .collect() | ||
| } |
There was a problem hiding this comment.
The !use_joiner implementation for CamelCase is non-standard and may be confusing. It lowercases the first word and then capitalizes the first letter of subsequent words (e.g., HELLO WORLD becomes hello World), which is not typical camelCase.
Given that !use_joiner implies preserving separators, a more predictable behavior might be to simply lowercase the first letter of the entire string, which is a common variation of camel case.
StringCapitalization::CamelCase => {
let mut chars = string.chars();
match chars.next() {
Some(first) => first.to_lowercase().to_string() + chars.as_str(),
None => String::new(),
}
}![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
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.
| @@ -28,6 +28,9 @@ dyn-any = { workspace = true } | |||
| glam = { workspace = true } | |||
There was a problem hiding this comment.
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).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/nodes/gcore/Cargo.toml, line 31:
<comment>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).</comment>
<file context>
@@ -28,6 +28,9 @@ dyn-any = { workspace = true }
glam = { workspace = true }
log = { workspace = true }
serde_json = { workspace = true }
+convert_case = { workspace = true }
+titlecase = { workspace = true }
+unicode-segmentation = { workspace = true }
</file context>
|
|
||
| // Joiner row: the UseJoiner checkbox is drawn in the assist area, followed by the Joiner text input | ||
| let mut joiner_widgets = start_widgets(ParameterWidgetsInfo::new(node_id, JoinerInput::INDEX, false, context)); | ||
| if let Some(joiner) = joiner_value { |
There was a problem hiding this comment.
P2: UseJoiner control is conditionally hidden when JoinerInput is exposed, because both controls are gated on joiner_value being non-exposed.
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_properties.rs, line 1628:
<comment>`UseJoiner` control is conditionally hidden when `JoinerInput` is exposed, because both controls are gated on `joiner_value` being non-exposed.</comment>
<file context>
@@ -1588,6 +1590,101 @@ pub(crate) fn exposure_properties(node_id: NodeId, context: &mut NodePropertiesC
+
+ // Joiner row: the UseJoiner checkbox is drawn in the assist area, followed by the Joiner text input
+ let mut joiner_widgets = start_widgets(ParameterWidgetsInfo::new(node_id, JoinerInput::INDEX, false, context));
+ if let Some(joiner) = joiner_value {
+ let joiner_is_empty = joiner.is_empty();
+ joiner_widgets.extend_from_slice(&[
</file context>
| // When the joiner is disabled, apply only character-level casing while preserving the string's existing structure | ||
| if !use_joiner { | ||
| return match capitalization { | ||
| StringCapitalization::LowerCase => string.to_lowercase(), |
There was a problem hiding this comment.
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.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/nodes/gcore/src/logic.rs, line 222:
<comment>`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.) </comment>
<file context>
@@ -59,6 +85,208 @@ fn string_slice(_: impl Ctx, string: String, start: f64, end: f64) -> String {
+ // When the joiner is disabled, apply only character-level casing while preserving the string's existing structure
+ if !use_joiner {
+ return match capitalization {
+ StringCapitalization::LowerCase => string.to_lowercase(),
+ StringCapitalization::UpperCase => string.to_uppercase(),
+ StringCapitalization::CapitalCase | StringCapitalization::HeadlineCase => {
</file context>
| c | ||
| } else if capitalize_next { | ||
| capitalize_next = false; | ||
| c.to_uppercase().next().unwrap_or(c) |
There was a problem hiding this comment.
P2: Unicode case conversion is lossy here because only the first character of to_uppercase()/to_lowercase() is kept.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/nodes/gcore/src/logic.rs, line 234:
<comment>Unicode case conversion is lossy here because only the first character of `to_uppercase()`/`to_lowercase()` is kept.</comment>
<file context>
@@ -59,6 +85,208 @@ fn string_slice(_: impl Ctx, string: String, start: f64, end: f64) -> String {
+ c
+ } else if capitalize_next {
+ capitalize_next = false;
+ c.to_uppercase().next().unwrap_or(c)
+ } else {
+ c
</file context>
1 participant
Still a bit more work to do to get this landed in the next day or so.