⚡ Bolt: [Performance] Avoid string array allocations in CollapseToSingleLine - #129
Conversation
…gleLine Optimized `CollapseToSingleLine` in `ShortcutDisplay.cs` by replacing `string.Split()` with `AsSpan().SplitAny()`. This eliminates `string[]` allocations and intermediate strings, using a `StringBuilder` directly instead, thereby reducing GC overhead while preserving the exact semantic behavior.
|
👋 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. |
📝 WalkthroughWalkthrough
ChangesShortcut display formatting
Windows build targeting
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 8✅ Passed checks (8 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…o local variable' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> Signed-off-by: Anthony Thompson <github@trackdub.com>
…26522984034040118
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The separator set is currently defined twice (once via
IndexOfAny(' ', ' ', ' ')and again via explicit!= ' ' && != ' ' && != ' 'checks in the loop); consider centralizing this (e.g., with a smallIsSeparatorhelper) so any future change to the delimiter set can’t accidentally get applied in only one place.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The separator set is currently defined twice (once via `IndexOfAny('
', '
', ' ')` and again via explicit `!= '
' && != '
' && != ' '` checks in the loop); consider centralizing this (e.g., with a small `IsSeparator` helper) so any future change to the delimiter set can’t accidentally get applied in only one place.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a41bb9c2f9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| <NuGetAuditMode>direct</NuGetAuditMode> | ||
| <PlatformTarget>$(Platform)</PlatformTarget> | ||
| <LocalCmdPalToolkit>$(MSBuildThisFileDirectory)..\PowerToys\src\modules\cmdpal\extensionsdk\Microsoft.CommandPalette.Extensions.Toolkit\Microsoft.CommandPalette.Extensions.Toolkit.csproj</LocalCmdPalToolkit> | ||
| <EnableWindowsTargeting>true</EnableWindowsTargeting> |
There was a problem hiding this comment.
On non-Windows machines, this opts every project—including the MSIX host, Run plugin, and tests—into Windows targeting, even though the repository limits Linux validation to Core/Suggest with -p:EnableWindowsTargeting=true. It also modifies a protected build file outside the scope of this display-allocation optimization; remove the property and pass it only on the supported Core/Suggest build command.
AGENTS.md reference: AGENTS.md:L128-L129
Useful? React with 👍 / 👎.
2 participants
💡 What: Replaced
string.Split(['\r', '\n', '\t'], StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries)inShortcutDisplay.CollapseToSingleLinewith a span-based approach (span.SplitAny(...)) writing to aStringBuilder.🎯 Why: The previous implementation allocated an array of strings as well as intermediate strings for each newline or tab separated token. Since this formatting method can be called repeatedly during UI rendering/catalog processing, reducing object allocations lowers garbage collector pressure and enhances overall responsiveness.
📊 Impact: Reduces
string[]andstringallocations to zero for strings containing no tabs or newlines (thanks to a fast-pathIndexOfAnycheck), and to just a singleStringBuilderand final string allocation for multi-line inputs.🔬 Measurement: Verify memory allocations via a profiler (like dotMemory or Visual Studio Diagnostics Tools) when the command catalog/shortcut UI populates – there should be a notable drop in Gen 0 allocations sourced from
CollapseToSingleLine. Ensure unit tests pass and behavior remains identical.PR created automatically by Jules for task 11926522984034040118 started by @mta-babel
Summary by cubic
Optimize ShortcutDisplay.CollapseToSingleLine by replacing string.Split with a span-based scan that writes directly to a StringBuilder, preserving output while cutting allocations and GC during UI/catalog rendering. Also enables Windows targeting in Directory.Build.props to fix Linux CI and addresses a CodeQL “useless assignment to local variable” warning.
Written for commit a41bb9c. Summary will update on new commits.