Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions clients/macos/AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -301,17 +301,21 @@ DM Mono is not used in the SwiftUI-facing palette anymore (removed when the type
- **No-op backgrounds**: Never add invisible backgrounds like `.background(Capsule().fill(Color.clear))` — they create layout wrappers with zero visual effect.
- **No animated insertions in chat `LazyVStack`**: ANY animated insertion/removal in a `LazyVStack` triggers `motionVectors` — an O(n) `sizeThatFits` measurement over ALL children that defeats lazy loading and causes multi-minute hangs. The chat message list uses `.transaction { $0.animation = nil }` to suppress all insertion animations. Do NOT remove that modifier or wrap content mutations in `withAnimation` that flows into the `LazyVStack`. See [`.transaction` docs](https://developer.apple.com/documentation/swiftui/view/transaction(_:)) and [WWDC23: Demystify SwiftUI performance](https://developer.apple.com/videos/play/wwdc2023/10160/).
- **Geometry observations must not drive state that changes the observed layout**: if a subtree is size-constrained (e.g., `.frame(height:)`, `.clipped()`) and an `onGeometryChange` or `GeometryReader` inside it writes the measured height/width into `@State` that gates the same constraint, you get a feedback loop — the observed value is the *clamped* value, so the decision to clamp flips off, the frame is removed, the child re-measures larger, the decision flips back on, and the layout oscillates or settles incorrectly. Derive layout-gating decisions from the model (content counts, text length, attachment types) or from a container-level geometry source that is *not* inside the constrained subtree. See [`onGeometryChange` docs](https://developer.apple.com/documentation/swiftui/view/ongeometrychange(for:of:action:)).
- **No `.frame(maxWidth:)`, `.frame(maxHeight:)`, or `.frame(maxWidth:maxHeight:alignment:)` inside LazyVStack/LazyHStack/LazyVGrid cell hierarchy**: These create [`_FlexFrameLayout`](https://developer.apple.com/documentation/swiftui/view/frame(minwidth:idealwidth:maxwidth:minheight:idealheight:maxheight:alignment:)) whose `placement()` queries each child's explicit alignment via [`ViewDimensions.subscript`](https://developer.apple.com/documentation/swiftui/viewdimensions). Nested FlexFrames recurse O(depth × children) per layout pass. **This applies to ALL `maxWidth`/`maxHeight` values, not just `.infinity`** — bounded values like `.frame(maxWidth: 360)` still create `_FlexFrameLayout` and trigger the alignment cascade. The difference is magnitude, not kind. See [WWDC23: Demystify SwiftUI performance](https://developer.apple.com/videos/play/wwdc2023/10160/). Safe alternatives:
- **`.widthCap(N)`** — uses `WidthCapLayout` ([Layout protocol](https://developer.apple.com/documentation/swiftui/layout), O(1)), caps width without creating `_FlexFrameLayout`. See `WidthCapLayout.swift`. Used in 10+ places in the codebase (PRs #24589, #26007, #26092).
- **No `_FlexFrameLayout` inside LazyVStack/LazyHStack/LazyVGrid cell hierarchy**: ANY parameter on the [flexible frame overload](https://developer.apple.com/documentation/swiftui/view/frame(minwidth:idealwidth:maxwidth:minheight:idealheight:maxheight:alignment:)) — `minWidth`, `minHeight`, `maxWidth`, `maxHeight`, `idealWidth`, `idealHeight` — creates `_FlexFrameLayout`, whose `placeSubviews` queries each child's explicit alignment via [`ViewDimensions.subscript`](https://developer.apple.com/documentation/swiftui/viewdimensions). Nested FlexFrames recurse O(depth × children) per layout pass. **This applies to ALL values, not just `.infinity`** — bounded values like `.frame(maxWidth: 360)` or `.frame(minHeight: 100)` still create `_FlexFrameLayout` and trigger the alignment cascade. The [fixed frame overload](https://developer.apple.com/documentation/swiftui/view/frame(width:height:alignment:)) (`.frame(width:)`, `.frame(height:)`) creates `_FrameLayout` instead — a different internal type. See [WWDC23: Demystify SwiftUI performance](https://developer.apple.com/videos/play/wwdc2023/10160/). Safe alternatives:
- **`.widthCap(N)`** — uses `WidthCapLayout` ([Layout protocol](https://developer.apple.com/documentation/swiftui/layout), O(1)), caps width without creating `_FlexFrameLayout`. See `WidthCapLayout.swift`.
- **`.fixedWidth(N)`** — uses `FixedWidthLayout` ([Layout protocol](https://developer.apple.com/documentation/swiftui/layout), O(1)), sets a definite width without creating `_FrameLayout`. See `FixedWidthLayout.swift`.
- **`.topAlignedMinHeight(N)`** — uses `TopAlignedMinHeightLayout` ([Layout protocol](https://developer.apple.com/documentation/swiftui/layout), O(1)), minimum height with top alignment without creating `_FlexFrameLayout`. See `TopAlignedMinHeightLayout.swift`.
- **`.bottomAlignedMinHeight(N)`** — uses `BottomAlignedMinHeightLayout` ([Layout protocol](https://developer.apple.com/documentation/swiftui/layout), O(1)), minimum height with bottom alignment without creating `_FlexFrameLayout`. See `BottomAlignedMinHeightLayout.swift`.
- `.frame(width: exactWidth)` — [`_FrameLayout`](https://developer.apple.com/documentation/swiftui/view/frame(width:height:alignment:)), safe for `sizeThatFits` (O(1)), but `placeSubviews` still queries child alignment via `commonPlacement → ViewDimensions[guide]`. **Not safe as a cascade barrier** — use `.fixedWidth()` instead when the child subtree contains a `LazyVStack` or deep view hierarchy.
- `HStack { content; Spacer(minLength: 0) }` — leading alignment without queries.
- `HStack { Spacer(minLength: 0); content }` — trailing alignment without queries.
- [`.containerRelativeFrame(.horizontal)`](https://developer.apple.com/documentation/swiftui/view/containerrelativeframe(_:alignment:)) — width constraint without FlexFrame.

Never trade `HStack+Spacer` for `.frame(alignment:)` in lazy containers — fewer layout nodes is not worth O(n) recursive alignment queries per node.

**Enforced mechanically in CI**: [`clients/scripts/check-flexframe.sh`](../scripts/check-flexframe.sh) fails the build on new `.frame(maxWidth:)` / `.frame(maxHeight:)` inside `Features/Chat/`, `Features/Home/`, and `Features/MainWindow/`. Known intentional leaves are listed in [`clients/scripts/flexframe-allowlist.txt`](../scripts/flexframe-allowlist.txt) with rationale. The manual audit process missed regressions twice (#25947 wrong call on bounded `maxWidth`, #26220 deferred leaves) before the lint existed — prefer fixing the code over adding allowlist entries; the allowlist is a last resort.
**Why Layout protocol wrappers are safe**: custom [`Layout`](https://developer.apple.com/documentation/swiftui/layout) implementations use [`LayoutSubview.place(at:anchor:proposal:)`](https://developer.apple.com/documentation/swiftui/layoutsubview/place(at:anchor:proposal:)) for positioning, which resolves the anchor from the child's known size as a `UnitPoint` — no alignment guide queries. The [default `explicitAlignment`](https://developer.apple.com/documentation/swiftui/layout/explicitalignment(of:in:proposal:subviews:cache:)-8cl0p) merges all subviews' guides recursively; overriding it to return `nil` tells ancestors "no explicit value; use default positioning", blocking the cascade in O(1).

**Enforced mechanically in CI**: [`clients/scripts/check-flexframe.sh`](../scripts/check-flexframe.sh) fails the build on new `.frame(minWidth:)` / `.frame(minHeight:)` / `.frame(maxWidth:)` / `.frame(maxHeight:)` / `.frame(idealWidth:)` / `.frame(idealHeight:)` inside `Features/Chat/`, `Features/Home/`, and `Features/MainWindow/`. Known intentional usages are listed in [`clients/scripts/flexframe-allowlist.txt`](../scripts/flexframe-allowlist.txt). Prefer fixing the code over adding allowlist entries; the allowlist is a last resort.

**Leaf wrapper exception (O(0) cascade)**: `_FlexFrameLayout` wrapping a view with no descendants — `Text`, `Image`, `VIconView`, `RoundedRectangle`, etc. — has nothing to cascade into. The alignment query bottoms out immediately. Documented allowlist case: [`QueuedMessageRow.swift:55`](vellum-assistant/Features/Chat/QueuedMessageRow.swift) `.frame(maxWidth: .infinity, alignment: .leading)` around a leaf `Text` with `.lineLimit(1).truncationMode(.tail)` — a configuration `HStack + Spacer` breaks cleanly (truncation stops working because the Text takes intrinsic width first). If you must wrap a leaf, prefer `HStack + Spacer` or `.widthCap` anyway; use the allowlist only when those break required semantics.

Expand Down
4 changes: 3 additions & 1 deletion clients/macos/SCROLL_STRATEGY.md
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,9 @@ When `pinnedLatestTurnAnchorMessageId` is set, the newest turn is carved out int

The section is flipped as a single unit (cancelling the outer ScrollView flip), so the visual order matches source order: anchor at the visual top, response below, the spacer fills the rest, sentinel at the bottom.

The section's height is bound to the scroll viewport as a minimum — not a fixed size. A zero-width `Color.clear` probe in the section's `.background` uses `containerRelativeFrame(.vertical, alignment: .top) { length, _ in max(0, length - VSpacing.md * 2) }` to measure the scroll container's visible height, and `onGeometryChange` mirrors the result into a local `@State` that drives the VStack's `.frame(minHeight:, alignment: .top)`. The `max(0, …)` clamp keeps the probe non-negative during transient zero-height layout passes. This replaces the previous `LatestTurnSpacerCalculator` + `viewportHeight` `@State` pipeline (which lagged one frame and caused the anchor row to briefly clip on every Enter keystroke in the composer).
The section's height is bound to the scroll viewport as a minimum — not a fixed size. A zero-width `Color.clear` probe in the section's `.background` uses `containerRelativeFrame(.vertical, alignment: .top) { length, _ in max(0, length - VSpacing.md * 2) }` to measure the scroll container's visible height, and `onGeometryChange` mirrors the result into a local `@State` that drives the VStack's `.topAlignedMinHeight()`. The `max(0, …)` clamp keeps the probe non-negative during transient zero-height layout passes.

**Why `TopAlignedMinHeightLayout` instead of `.frame(minHeight:, alignment: .top)`**: `.frame(minHeight:, alignment:)` creates `_FlexFrameLayout`, whose `placeSubviews` queries `explicitAlignment` on every descendant — O(n × depth) cascade through the response cluster. `TopAlignedMinHeightLayout` (Layout protocol) achieves the same sizing and top-alignment via `place(at:anchor:)` and returns `nil` from `explicitAlignment`, stopping the cascade in O(1). See `TopAlignedMinHeightLayout.swift` and AGENTS.md.

### Tall-response behavior

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -473,13 +473,18 @@ private struct PinnedLatestTurnSection: View {
// equals visual order: anchor at top, response below, spacer
// fills remaining viewport, sentinel marks the latest edge.
//
// `.frame(minHeight:)` (not a fixed `containerRelativeFrame` on
// the VStack) gives the section a viewport-sized floor while
// letting it grow when anchor + response exceeds the viewport.
// Without the floor, the `Spacer` below cannot keep the anchor
// pinned to the visual top while a short response is streaming.
// Without growth, a tall assistant response is capped at the
// viewport and the newest content becomes unreachable by scroll.
// `TopAlignedMinHeightLayout` gives the section a viewport-sized
// floor while letting it grow when anchor + response exceeds the
// viewport. Without the floor, the `Spacer` below cannot keep the
// anchor pinned to the visual top while a short response streams.
// Without growth, a tall response is capped at the viewport and
// the newest content becomes unreachable by scroll.
//
// `.frame(minHeight:alignment: .top)` achieves the same sizing
// but creates `_FlexFrameLayout`, whose `placeSubviews` queries
// `explicitAlignment` on every descendant — O(n × depth) cascade.
// `TopAlignedMinHeightLayout` returns `nil` from
// `explicitAlignment`, stopping the cascade in O(1).
VStack(alignment: .leading, spacing: 0) {
contentView.transcriptRow(
row: anchorRow,
Expand All @@ -495,7 +500,7 @@ private struct PinnedLatestTurnSection: View {

contentView.latestEdgeSentinel(isFlipped: false)
}
.frame(minHeight: viewportMinHeight, alignment: .top)
.topAlignedMinHeight(viewportMinHeight)
.background(viewportMinHeightProbe)
.flipped()
}
Expand Down
54 changes: 35 additions & 19 deletions clients/scripts/check-flexframe.sh
Comment thread
devin-ai-integration[bot] marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,29 @@ set -euo pipefail

# FlexFrame guardrail script
#
# Detects `.frame(maxWidth:)` / `.frame(maxHeight:)` usages in performance-sensitive
# chat/window directories. These modifiers create `_FlexFrameLayout`, which queries
# `explicitAlignment` on descendants — cascading O(depth × children) per layout pass
# and causing multi-second hangs in LazyVStack-backed hierarchies.
# Detects `.frame(maxWidth:)` / `.frame(maxHeight:)` / `.frame(minWidth:)` /
# `.frame(minHeight:)` / `.frame(idealWidth:)` / `.frame(idealHeight:)` usages
# in performance-sensitive chat/window directories.
# These modifiers create `_FlexFrameLayout`, which queries `explicitAlignment`
# on descendants — cascading O(depth × children) per layout pass and causing
# multi-second hangs in LazyVStack-backed hierarchies.
#
# See clients/macos/AGENTS.md (section "No `.frame(maxWidth:)` ... in LazyVStack/
# ALL parameters on the flexible frame overload — minWidth, minHeight, maxWidth,
# maxHeight, idealWidth, idealHeight — create `_FlexFrameLayout`. The fixed
# overload (width:, height:)
# creates `_FrameLayout` instead. See:
# https://developer.apple.com/documentation/swiftui/view/frame(minwidth:idealwidth:maxwidth:minheight:idealheight:maxheight:alignment:)
#
# See clients/macos/AGENTS.md (section "No `_FlexFrameLayout` ... in LazyVStack/
# LazyHStack/LazyVGrid cell hierarchy") for the rule and safe alternatives.
#
# Safe alternatives:
# - .widthCap(N) — O(1) width cap via WidthCapLayout
# - .frame(width: N) — _FrameLayout, no alignment query
# - .fixedWidth(N) — O(1) fixed width via FixedWidthLayout
# - .topAlignedMinHeight(N) — O(1) min-height with top alignment
# - .bottomAlignedMinHeight(N) — O(1) min-height with bottom alignment
# - .frame(width: N) / .frame(height: N) — _FrameLayout (but not safe as cascade barrier)
# - HStack { content; Spacer(minLength: 0) } / Spacer + content — alignment without FlexFrame
# - BottomAlignedMinHeightLayout — vertical equivalent
#
# Historical context: this cascade has been fixed 9+ times in chat-surface code
# (PRs #24019, #24091, #24584, #24589, #25844, #25947, #26007, #26053, #26092, #26220).
Expand All @@ -39,7 +49,7 @@ for arg in "$@"; do
case "$arg" in
--update-baseline) UPDATE_BASELINE=1 ;;
-h|--help)
sed -n '2,25p' "$0" | sed 's/^# \?//'
sed -n '2,40p' "$0" | sed 's/^# \?//'
exit 0 ;;
*) echo "Unknown argument: $arg" >&2; exit 1 ;;
esac
Expand All @@ -53,10 +63,13 @@ SCAN_DIRS=(
"clients/macos/vellum-assistant/Features/MainWindow/"
)

# Matches .frame(maxWidth: ...) or .frame(maxHeight: ...) — any value.
# Matches .frame(maxWidth: ...) / .frame(maxHeight: ...) / .frame(minWidth: ...)
# / .frame(minHeight: ...) / .frame(idealWidth: ...) / .frame(idealHeight: ...)
# — any value. ALL six parameters live on the flexible frame overload and
# create `_FlexFrameLayout`.
# Rust-regex compatible (no lookaround) so it works with ripgrep's default
# engine; we strip comment-only lines in a second pass below.
PATTERN='\.frame\(\s*max(Width|Height)\s*:'
PATTERN='\.frame\(\s*(max|min|ideal)(Width|Height)\s*:'

cd "$REPO_ROOT"

Expand Down Expand Up @@ -102,7 +115,7 @@ fi
if [[ "$UPDATE_BASELINE" == "1" ]]; then
{
cat <<'HEADER'
# FlexFrame allowlist — intentional `.frame(maxWidth:)` / `.frame(maxHeight:)` usages.
# FlexFrame allowlist — intentional `.frame(max/minWidth:)` / `.frame(max/minHeight:)` usages.
#
# Each line is `<path>|<trimmed-line-content>` for one occurrence. Line numbers
# are intentionally omitted so entries survive unrelated line drift.
Expand All @@ -119,7 +132,8 @@ if [[ "$UPDATE_BASELINE" == "1" ]]; then
# and no animated transition in the parent).
#
# Adding a new entry: BEFORE allowlisting, first try a safe alternative:
# .widthCap(N), .frame(width: N), HStack+Spacer, BottomAlignedMinHeightLayout.
# .widthCap(N), .fixedWidth(N), .topAlignedMinHeight(N),
# .bottomAlignedMinHeight(N), HStack+Spacer.
# If and only if none of those preserve required semantics (truncation, exact
# alignment, fill-parent for a modal root), add the entry and a one-line note
# in the PR description explaining why. The default answer is "use a safe
Expand All @@ -128,7 +142,7 @@ if [[ "$UPDATE_BASELINE" == "1" ]]; then
# Regenerate this file after an intentional bulk refactor with:
# bash clients/scripts/check-flexframe.sh --update-baseline
#
# See clients/macos/AGENTS.md §§ "No `.frame(maxWidth:)` ... in LazyVStack/
# See clients/macos/AGENTS.md §§ "No `_FlexFrameLayout` ... in LazyVStack/
# LazyHStack/LazyVGrid cell hierarchy" for the underlying rule.
HEADER
if [[ -n "$OBSERVED_NORMALIZED" ]]; then
Expand Down Expand Up @@ -224,16 +238,18 @@ print_new_violations() {
if [[ "$NEW_COUNT" -gt 0 ]]; then
echo "=== flexframe lint: $NEW_COUNT new violation(s) ==="
echo
echo " .frame(maxWidth:) / .frame(maxHeight:) create _FlexFrameLayout, which queries"
echo " explicitAlignment on descendants and cascades O(depth × children) per layout"
echo " pass. This causes multi-second hangs in LazyVStack-backed chat hierarchies."
echo " .frame(max/min/idealWidth:) / .frame(max/min/idealHeight:)"
echo " create _FlexFrameLayout, which queries explicitAlignment on descendants and"
echo " cascades O(depth × children) per layout pass. This causes multi-second hangs"
echo " in LazyVStack-backed chat hierarchies."
echo
echo " Safe alternatives (see clients/macos/AGENTS.md §§ 'No .frame(maxWidth:) ...'):"
echo " Safe alternatives (see clients/macos/AGENTS.md §§ 'No _FlexFrameLayout ...'):"
echo " .widthCap(N) — O(1) width cap"
echo " .frame(width: N) — _FrameLayout, no alignment query"
echo " .fixedWidth(N) — O(1) fixed width"
echo " .topAlignedMinHeight(N) — O(1) min-height, top-aligned"
echo " .bottomAlignedMinHeight(N) — O(1) min-height, bottom-aligned"
echo " HStack { content; Spacer(minLength: 0) } — leading alignment, no FlexFrame"
echo " HStack { Spacer(minLength: 0); content } — trailing alignment, no FlexFrame"
echo " BottomAlignedMinHeightLayout — vertical fill, no FlexFrame"
echo
echo " If none of the above preserve the required semantics (e.g. single-line Text"
echo " truncation, modal-root fill-parent), add an entry to:"
Expand Down
Loading