Conversation
Co-authored-by: logbie <1138960+logbie@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthroughThe PR adds a performance optimization to the string replacement function by checking if the target substring exists before performing the replacement, avoiding unnecessary memory allocation when the substring is not found. The changelog documents this change. ChangesString Replacement Optimization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| ## 2026-04-22 - [Avoid string allocation on single-part split] | ||
| **Learning:** Calling `.split(delimiter)` on a reference-counted string (`Arc<str>`) and `.map()`ing the results into `Arc::from(s)` unconditionally creates a new allocation for every chunk. If the delimiter doesn't exist, the entire string is re-allocated unnecessarily. | ||
| **Action:** When iterating over a split of a reference-counted string, explicitly check if `s.len() == text.len() && !text.is_empty()`. If it is, use `Arc::clone(&text)` to return another reference to the existing string, bypassing the allocation. | ||
| ## 2025-05-05 - Avoid Unnecessary String Allocations in Replace |
There was a problem hiding this comment.
🟡 Wrong year in bolt.md entry (2025 instead of 2026)
The new bolt.md entry is dated 2025-05-05 but all 16 other entries in the file use the year 2026 (from 2026-01-03 through 2026-04-22). The current date is 2026-05-05, so this is clearly a typo. Additionally, the entry is missing the square bracket format used by every other entry (e.g., ## 2026-04-22 - [Avoid string allocation on single-part split]) and is missing the blank line separator that precedes every other entry.
| ## 2025-05-05 - Avoid Unnecessary String Allocations in Replace | |
| ## 2026-05-05 - [Avoid Unnecessary String Allocations in Replace] |
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/stdlib/text.rs (1)
282-289: ⚡ Quick winAdd a test for the
old-not-found fast path.The new early-return branch (lines 284–286) has no unit test. The existing
test_replaceandtest_replace_multipletests both exercise the slow path. A test covering the fast-path return — including verifying the identity of the returnedArc— would close the gap and prevent future regressions.✅ Proposed test
+ #[test] + fn test_replace_not_found_returns_original() { + let original: Arc<str> = Arc::from("hello world"); + let result = native_replace(vec![ + Value::Text(Arc::clone(&original)), + Value::Text(Arc::from("xyz")), + Value::Text(Arc::from("rust")), + ]) + .unwrap(); + // Value must be unchanged + assert_eq!(result, Value::Text(Arc::from("hello world"))); + // Arc should be the same allocation (fast path), not a new copy + if let Value::Text(returned) = result { + assert!(Arc::ptr_eq(&original, &returned)); + } else { + panic!("Expected Value::Text"); + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/stdlib/text.rs` around lines 282 - 289, Add a unit test that exercises the early-return fast path in native_replace: create an Arc<str> original (e.g. "hello world"), call native_replace with old set to a substring not present (e.g. "xyz") and new set to anything, unwrap the Result, assert the returned Value equals Value::Text(Arc::from("hello world")), and then assert Arc::ptr_eq(&original, &returned) to verify the same allocation (fast-path) was returned; place this test alongside test_replace/test_replace_multiple in the same test module and name it e.g. test_replace_not_found_returns_original.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.jules/bolt.md:
- Line 64: Update the changelog heading "## 2025-05-05 - Avoid Unnecessary
String Allocations in Replace" to match the other entries by correcting the year
to 2026 and adding the square-bracketed title convention; e.g. change it to "##
2026-05-05 - [Avoid Unnecessary String Allocations in Replace]" so the date and
heading format are consistent with the rest of the file.
---
Nitpick comments:
In `@src/stdlib/text.rs`:
- Around line 282-289: Add a unit test that exercises the early-return fast path
in native_replace: create an Arc<str> original (e.g. "hello world"), call
native_replace with old set to a substring not present (e.g. "xyz") and new set
to anything, unwrap the Result, assert the returned Value equals
Value::Text(Arc::from("hello world")), and then assert Arc::ptr_eq(&original,
&returned) to verify the same allocation (fast-path) was returned; place this
test alongside test_replace/test_replace_multiple in the same test module and
name it e.g. test_replace_not_found_returns_original.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 093f2796-3364-49e1-bfad-f4f25df0f96e
📒 Files selected for processing (2)
.jules/bolt.mdsrc/stdlib/text.rs
| ## 2026-04-22 - [Avoid string allocation on single-part split] | ||
| **Learning:** Calling `.split(delimiter)` on a reference-counted string (`Arc<str>`) and `.map()`ing the results into `Arc::from(s)` unconditionally creates a new allocation for every chunk. If the delimiter doesn't exist, the entire string is re-allocated unnecessarily. | ||
| **Action:** When iterating over a split of a reference-counted string, explicitly check if `s.len() == text.len() && !text.is_empty()`. If it is, use `Arc::clone(&text)` to return another reference to the existing string, bypassing the allocation. | ||
| ## 2025-05-05 - Avoid Unnecessary String Allocations in Replace |
There was a problem hiding this comment.
Wrong year and inconsistent heading format in changelog entry.
Two issues:
- The date
2025-05-05is one year behind all other entries (which are all 2026-xx-xx) and behind the current date (2026-05-05). - The heading omits the square-bracket title convention used by every other entry in this file.
📝 Proposed fix
-## 2025-05-05 - Avoid Unnecessary String Allocations in Replace
+## 2026-05-05 - [Avoid Unnecessary String Allocations in Replace]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## 2025-05-05 - Avoid Unnecessary String Allocations in Replace | |
| ## 2026-05-05 - [Avoid Unnecessary String Allocations in Replace] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.jules/bolt.md at line 64, Update the changelog heading "## 2025-05-05 -
Avoid Unnecessary String Allocations in Replace" to match the other entries by
correcting the year to 2026 and adding the square-bracketed title convention;
e.g. change it to "## 2026-05-05 - [Avoid Unnecessary String Allocations in
Replace]" so the date and heading format are consistent with the rest of the
file.
There was a problem hiding this comment.
Pull request overview
Adds a small runtime optimization to the WFL stdlib text replace native to avoid allocating a new String when no replacement is needed, and records the learning in the Jules Bolt log.
Changes:
- Added a
contains()fast-path tonative_replaceto returnArc::clone(&text)whenoldis not present. - Updated
.jules/bolt.mdwith a new entry describing the optimization rationale.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/stdlib/text.rs |
Adds a fast path in native_replace to skip allocation when the substring is not found. |
.jules/bolt.md |
Documents the replace allocation-avoidance optimization as a Bolt learning/action item. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Optimization: avoid string allocation if the substring to replace is not found | ||
| if !text.contains(old.as_ref()) { | ||
| return Ok(Value::Text(Arc::clone(&text))); | ||
| } |
| ## 2026-04-22 - [Avoid string allocation on single-part split] | ||
| **Learning:** Calling `.split(delimiter)` on a reference-counted string (`Arc<str>`) and `.map()`ing the results into `Arc::from(s)` unconditionally creates a new allocation for every chunk. If the delimiter doesn't exist, the entire string is re-allocated unnecessarily. | ||
| **Action:** When iterating over a split of a reference-counted string, explicitly check if `s.len() == text.len() && !text.is_empty()`. If it is, use `Arc::clone(&text)` to return another reference to the existing string, bypassing the allocation. | ||
| ## 2025-05-05 - Avoid Unnecessary String Allocations in Replace |
💡 What: Added a fast-path
.contains()check tonative_replaceinsrc/stdlib/text.rs.🎯 Why: In Rust,
str::replacealways allocates a newStringeven if the target substring is not found. By checking.contains()first, we can avoid this allocation and instead return a cheapArc::cloneof the original string when no replacement is needed.📊 Impact: Eliminates O(N) memory allocation and copy operations for
replacecalls where the target substring does not exist in the text.🔬 Measurement: Can be verified by running micro-benchmarks on
str::replacevsstr::contains+str::replace. The fast path significantly reduces latency when the substring is missing. Tested correctness withcargo test.PR created automatically by Jules for task 4698546564048008536 started by @logbie
Summary by CodeRabbit