[LUM-718] perf: flatten inner view hierarchies (MarkdownSegmentView, AssistantProgressView)#23982
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
devin-ai-integration
Bot
force-pushed
the
devin/LUM-718-1775576262
branch
from
bd5f6c4 to
693ee88
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![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: 693ee88531
ℹ️ 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".
| .fill(VColor.surfaceOverlay.opacity(0.6)) | ||
| .stroke(VColor.borderBase, lineWidth: 0.5) |
There was a problem hiding this comment.
The new background builder chains .stroke(...) directly after .fill(...) on RoundedRectangle, but in SwiftUI fill returns some View, so this chain is not valid shape API usage and will fail when compiling the macOS client target. This makes the change a build blocker for environments that build clients/macos; use a separate overlay/background composition instead of fill().stroke().
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is a false positive. The project targets macOS 14+ (confirmed by usage of ViewThatFits, onGeometryChange, and other macOS 14+ APIs throughout the codebase). In macOS 14 / iOS 17, Apple added Shape.fill(_:).stroke(_:lineWidth:) as a first-class chaining API — see Apple Developer Documentation: Shape.stroke(_:lineWidth:). The .fill().stroke() chain compiles and works correctly on macOS 14+.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
devin-ai-integration
Bot
force-pushed
the
devin/LUM-718-1775576262
branch
from
fe6ecd3 to
6a9a0ce
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Spin Dump Comparison: main vs PR devin/LUM-718 _PaddingLayout Nesting Depth (bubbleChrome → inner view) ┌───────────────────────────────────────┬────────────┬───────────┬───────────┐ Key Findings
|
Reduce per-cell layout depth by flattening redundant modifier chains and
view wrappers in chat bubble inner views:
- Remove unnecessary Group wrappers around switch statements in
CompactPermissionChip and ChatBubbleToolStatusView.compactPermissionChip
(ViewBuilder handles switches natively since Swift 5.3)
- Remove single-child VStack wrapper around stepTitle Text in StepDetailRow
- Flatten nested VStack > VStack in stepDetailContent technical details
(both had identical spacing: VSpacing.xs)
- Combine .background() + .clipShape() + .overlay() into single
.background {} with fill+stroke in outputBlock
- Replace VStack > HStack > Spacer with HStack + .frame(alignment:) in
ChatBubbleToolStatusView trailing status
- Merge two .padding() calls into single EdgeInsets in CodeBlockView header
Estimated depth reduction: ~8-10 layout nodes per cell.
Co-Authored-By: Jason Zhou <jasonczhou3@gmail.com>
… corners The .clipShape(RoundedRectangle) is needed to clip scrollable content (e.g., 500+ line outputs in the ScrollView) to the rounded background. Without it, text near corners visually bleeds past the rounded border. Co-Authored-By: Jason Zhou <jasonczhou3@gmail.com>
The stroke was inside .background {} which gets clipped by .clipShape(),
halving the visible border width (0.25pt instead of 0.5pt). Move the
stroke to .overlay() after .clipShape() so the full border is drawn on
top, matching the existing pattern at line 758-762.
Co-Authored-By: Jason Zhou <jasonczhou3@gmail.com>
…copy buttons Pre-existing issue: VCopyButton instances used .opacity(isHovered ? 1 : 0) without .allowsHitTesting or .accessibilityHidden guards. When opacity is 0, the button was still tappable and visible to VoiceOver. Added both guards to the two VCopyButton instances in CodeBlockView (language header and no-language overlay). Co-Authored-By: Jason Zhou <jasonczhou3@gmail.com>
devin-ai-integration
Bot
force-pushed
the
devin/LUM-718-1775576262
branch
from
d76ad62 to
19cee44
Compare
|
@codex review |
|
@devin review |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…AssistantProgressView) (#23982) * [LUM-718] perf: flatten inner view hierarchies to reduce layout depth Reduce per-cell layout depth by flattening redundant modifier chains and view wrappers in chat bubble inner views: - Remove unnecessary Group wrappers around switch statements in CompactPermissionChip and ChatBubbleToolStatusView.compactPermissionChip (ViewBuilder handles switches natively since Swift 5.3) - Remove single-child VStack wrapper around stepTitle Text in StepDetailRow - Flatten nested VStack > VStack in stepDetailContent technical details (both had identical spacing: VSpacing.xs) - Combine .background() + .clipShape() + .overlay() into single .background {} with fill+stroke in outputBlock - Replace VStack > HStack > Spacer with HStack + .frame(alignment:) in ChatBubbleToolStatusView trailing status - Merge two .padding() calls into single EdgeInsets in CodeBlockView header Estimated depth reduction: ~8-10 layout nodes per cell. Co-Authored-By: Jason Zhou <jasonczhou3@gmail.com> * [LUM-718] Restore .clipShape() to prevent content overflow at rounded corners The .clipShape(RoundedRectangle) is needed to clip scrollable content (e.g., 500+ line outputs in the ScrollView) to the rounded background. Without it, text near corners visually bleeds past the rounded border. Co-Authored-By: Jason Zhou <jasonczhou3@gmail.com> * [LUM-718] Separate stroke into overlay to prevent clipping The stroke was inside .background {} which gets clipped by .clipShape(), halving the visible border width (0.25pt instead of 0.5pt). Move the stroke to .overlay() after .clipShape() so the full border is drawn on top, matching the existing pattern at line 758-762. Co-Authored-By: Jason Zhou <jasonczhou3@gmail.com> * [LUM-718] Add hit testing and accessibility guards to opacity-hidden copy buttons Pre-existing issue: VCopyButton instances used .opacity(isHovered ? 1 : 0) without .allowsHitTesting or .accessibilityHidden guards. When opacity is 0, the button was still tappable and visible to VoiceOver. Added both guards to the two VCopyButton instances in CodeBlockView (language header and no-language overlay). Co-Authored-By: Jason Zhou <jasonczhou3@gmail.com> --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: Jason Zhou <jasonczhou3@gmail.com>
1 participant
Reduces per-cell layout depth (~6-8 nodes) in chat bubble inner views by removing redundant wrapper views and merging modifier chains across
AssistantProgressView,ChatBubbleToolStatusView, andMarkdownSegmentView— targeting thePaddingLayout → PaddingLayout → ZStackLayoutnesting hotspots identified in spin dumps. Patterns applied: merge consecutive.padding()into singleEdgeInsets, remove unnecessaryGroup/VStackwrappers, flatten nestedVStack > VStack, replaceHStack + Spacerwith.frame(alignment:), and use pre-shaped.background(RoundedRectangle.fill(color))instead of.background(color).