ci(macos): lint rule for .frame(maxWidth:) / .frame(maxHeight:) regressions#27554
Conversation
…ssions Adds a fast ripgrep-based guard that fails CI when a new `.frame(maxWidth:)` or `.frame(maxHeight:)` is introduced inside `Features/Chat/` or `Features/MainWindow/`. These modifiers create `_FlexFrameLayout`, which cascades `explicitAlignment` queries through descendants and has caused multi-second hangs in LazyVStack-backed chat surfaces 9+ times (PRs #24019, #24091, #24584, #24589, #25844, #25947, #26007, #26053, #26092, #26220). The manual audit process missed regressions twice — this lint enforces the AGENTS.md:277-286 rule mechanically. Tracked in LUM-1116. Content-hash allowlist (`clients/scripts/flexframe-allowlist.txt`) seeded with the 170 existing occurrences so the check passes on current main. Entries are keyed on `<path>|<trimmed-line>` so unrelated line drift doesn't break them.
![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: f9fca266d1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # (lines whose first non-whitespace is `//` or `///`). AGENTS.md-style | ||
| # warnings like `// ⚠️ No .frame(maxWidth:) in LazyVStack cells` would | ||
| # otherwise false-positive. | ||
| RAW_HITS=$(rg -n --no-heading "$PATTERN" "${SCAN_DIRS[@]}" 2>/dev/null \ |
There was a problem hiding this comment.
The main scan uses rg -n --no-heading "$PATTERN" without -U/--multiline, so it only matches when .frame( and maxWidth/maxHeight are on the same line. A Swift call formatted as .frame( on one line and maxWidth: on the next line is silently missed, which lets new FlexFrame regressions bypass this guardrail entirely in the exact directories this lint is meant to protect.
Useful? React with 👍 / 👎.
| # Load the allowlist (strip comments + blank lines), preserving multiplicity. | ||
| ALLOWLIST_ENTRIES="" | ||
| if [[ -f "$ALLOWLIST_FILE" ]]; then | ||
| ALLOWLIST_ENTRIES=$(grep -vE '^([[:space:]]*#|[[:space:]]*$)' "$ALLOWLIST_FILE" | sort) |
There was a problem hiding this comment.
With set -euo pipefail, this assignment exits the script when the allowlist has only comments/blank lines because grep -vE ... returns status 1 for "no matches." That means once all violations are cleaned up (or after generating a header-only baseline), normal lint runs fail before comparison logic, turning a fully-clean state into a CI failure.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| # Load the allowlist (strip comments + blank lines), preserving multiplicity. | ||
| ALLOWLIST_ENTRIES="" | ||
| if [[ -f "$ALLOWLIST_FILE" ]]; then | ||
| ALLOWLIST_ENTRIES=$(grep -vE '^([[:space:]]*#|[[:space:]]*$)' "$ALLOWLIST_FILE" | sort) |
There was a problem hiding this comment.
🟡 Missing || true on allowlist grep causes script crash when all allowlist entries are removed
Under set -euo pipefail (line 2), the grep -vE on line 136 exits with code 1 when it finds no non-comment/non-blank lines (i.e., the allowlist file exists but all actual entries have been removed, leaving only the header comments). With pipefail, the pipeline grep ... | sort inherits grep's non-zero exit, and set -e terminates the script with no error message. This silently crashes the CI job.
Every other grep in the script that can legitimately match zero lines is protected with || true (clients/scripts/check-flexframe.sh:75, clients/scripts/check-flexframe.sh:128, clients/scripts/check-flexframe.sh:185), but this one is missing the guard. The bug will manifest when the team eventually fixes all allowlisted violations and removes the entries — the lint CI job will start failing on every PR, with no diagnostic output.
| ALLOWLIST_ENTRIES=$(grep -vE '^([[:space:]]*#|[[:space:]]*$)' "$ALLOWLIST_FILE" | sort) | |
| ALLOWLIST_ENTRIES=$(grep -vE '^([[:space:]]*#|[[:space:]]*$)' "$ALLOWLIST_FILE" | sort || true) |
Was this helpful? React with 👍 or 👎 to provide feedback.
| flexframe-lint: | ||
| name: FlexFrame Lint | ||
| needs: [changes] | ||
| if: needs.changes.outputs.clients == 'true' || github.event_name == 'workflow_dispatch' | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 5 | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 | ||
|
|
||
| - name: Ensure ripgrep is available | ||
| run: | | ||
| if ! command -v rg >/dev/null 2>&1; then | ||
| sudo apt-get update | ||
| sudo apt-get install -y ripgrep | ||
| fi | ||
| rg --version | head -1 | ||
|
|
||
| - name: FlexFrame guard | ||
| run: bash clients/scripts/check-flexframe.sh |
There was a problem hiding this comment.
🚩 notify-macos job doesn't track flexframe-lint failures
The notify-macos job in ci-main-macos.yaml has needs: [changes, test, build] and only checks needs.test.result and needs.build.result for status computation (ci-main-macos.yaml:327-348). The new flexframe-lint job is not included in the needs list, so if it fails on main, the Slack notification will report success (assuming test/build pass). GitHub's workflow-level status will still show the failure, but the team won't get a Slack alert for it.
Was this helpful? React with 👍 or 👎 to provide feedback.
#27556) Addresses two Codex review findings on #27554: P1: Main scan now uses `rg -U --multiline-dotall` so `.frame(` wrapped across lines (opening paren on one line, `maxWidth:` on the next) is caught. The prior single-line regex missed a real instance at ChatLoadingSkeleton.swift:58-61 — now allowlisted. P2: `grep -v` against the allowlist is now guarded with `|| true` so a header-only allowlist (possible after a bulk cleanup or fresh `--update-baseline` with zero violations) doesn't abort the script under `set -euo pipefail`. Tests: - dry-run passes (172 allowlisted, 0 new) - injected multi-line violation caught and exits 1 - injected single-line violation still caught (P1 regression check) - empty allowlist + no violations = exit 0 (P2 new path) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses Devin review finding on #27554: the notify-macos job's status computation only checked needs.test.result and needs.build.result — if flexframe-lint fails on main, the Slack notification would still report success. Add flexframe-lint to needs[] and to all three status branches (failure / cancelled / skipped). Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Summary
clients/scripts/check-flexframe.sh) that fails when a new.frame(maxWidth:)or.frame(maxHeight:)is introduced insideclients/macos/vellum-assistant/Features/Chat/orFeatures/MainWindow/(includingSidebar/andPanels/subdirectories).clients/macos/**(nopreviewlabel gate — feedback in under a minute) and on main-branch CI.Why this rule exists
.frame(maxWidth:)and.frame(maxHeight:)(any value, including bounded likemaxWidth: 360) create_FlexFrameLayout, whoseplacement()queries each child'sexplicitAlignmentviaViewDimensions.subscript. Nested FlexFrames recurse O(depth × children) per layout pass — in LazyVStack-backed chat hierarchies that's an O(n)motionVectorscascade over every row on every layout. The same cascade hits non-lazy hierarchies under transitions (see VELLUM-ASSISTANT-MACOS-66 / VELLUM-ASSISTANT-MACOS-KE).This has been fixed manually 9+ times:
The rule is documented in
clients/macos/AGENTS.md:277-286but has been enforced only by human vigilance. LUM-1116 Phase 3 tracks converting the rule to a mechanical check — this PR.How the lint works
rg -n '\.frame\(\s*max(Width|Height)\s*:'acrossFeatures/Chat/+Features/MainWindow/. Comment-only lines (//and///) are filtered so prose like// ⚠️ No .frame(maxWidth:) in LazyVStack cellsdoesn't false-positive.<path>|<trimmed-line-content>— line numbers are intentionally NOT part of the key so entries survive unrelated line drift above them.comm -23(multiset-preserving) yields new occurrences vs.clients/scripts/flexframe-allowlist.txt.file:line:contentfor each new violation, a summary of safe alternatives, a pointer to AGENTS.md, and instructions for allowlisting if unavoidable.Safe alternatives (also shown in the error message)
.widthCap(N)— O(1) width cap viaWidthCapLayout(used in 10+ places per fix: eliminate _FlexFrameLayout anti-pattern from chat cell hierarchy #24589, perf: eliminate remaining FlexFrame anti-patterns from LazyVStack cell views (Batch 3) #26007, perf: cap ForEach item count and eliminate cell-level FlexFrames (LUM-945) #26092).frame(width: N)—_FrameLayout, no alignment queryHStack { content; Spacer(minLength: 0) }— leading alignment without FlexFrameHStack { Spacer(minLength: 0); content }— trailing alignment without FlexFrameBottomAlignedMinHeightLayout— vertical fill equivalentScope (intentionally conservative)
Only
Features/Chat/andFeatures/MainWindow/are linted. These are the directories where the cascade has caused user-visible hangs. A follow-up PR can widen scope to additional feature modules once this proves its value.The lint does NOT fire on
.frame(maxWidth: .infinity)with no trailing arguments at the source level — actually it does, but boundedmaxWidth:/maxHeight:is where the wrong-usage pattern is mechanically clearest. We deliberately do NOT try to lint forms like.padding(.horizontal).background(Color.clear.frame(maxWidth: .infinity))because distinguishing leaf-vs-subtree usage via regex is unreliable; the 170-entry allowlist already documents the legitimate leaf cases.Adding to the allowlist
If a safe alternative genuinely can't preserve the required semantics (e.g. single-line
Texttruncation whereHStack+Spacerbreaks.lineLimit(1).truncationMode(.tail)— see #26220'sQueuedMessageRow.swift), add the<path>|<trimmed-line>entry toclients/scripts/flexframe-allowlist.txtAND explain why in the PR description. The file's top-of-file comment documents the typical "leaf Text / Image / VIconView" rationale. Bulk regeneration after an intentional refactor:bash clients/scripts/check-flexframe.sh --update-baseline.Files changed
clients/scripts/check-flexframe.sh(new) — lint script, bash 3.2 compatible, shellcheck cleanclients/scripts/flexframe-allowlist.txt(new) — 170 seed entries with documenting header.github/workflows/pr-macos.yaml— addsflexframe-lintjob (ubuntu-latest, unconditional, ~30s).github/workflows/ci-main-macos.yaml— addsflexframe-lintjob to main-branch CINo Swift source files are modified.
Verification
bash clients/scripts/check-flexframe.shlocally against the current worktree:flexframe lint: OK (170 allowlisted, 0 new)→ exit 0..frame(maxWidth: 100)to a Chat/ view, confirmed the lint reported the exactfile:line:contentand exited 1, then reverted.shellcheckclean; YAML validates.Original prompt
Click to expand
Add a CI lint check that fails on new
.frame(maxWidth:)/.frame(maxHeight:)usages insideclients/macos/vellum-assistant/Features/Chat/andclients/macos/vellum-assistant/Features/MainWindow/(including subdirectories — Sidebar/, Panels/).Rationale (details for the PR description, not for the implementation):
.frame(maxWidth:)/.frame(maxHeight:)creates_FlexFrameLayoutwhich triggersexplicitAlignmentcascades through descendants. Fixed 9 times in this codebase across PRs Fix LazyVStack cell hangs: definite scroll heights + cached coloredOutput #24019, Fix ScrollView.frame(maxHeight:) anti-pattern in 5 LazyVStack cell files #24091, fix: remove FlexFrameLayout ancestors that cascade into LazyVStack measureEstimates #24584, fix: eliminate _FlexFrameLayout anti-pattern from chat cell hierarchy #24589, perf: eliminate FlexFrameLayout anti-patterns causing 2s+ LazyVStack hangs #25844, fix(chat): Remove remaining FlexFrame anti-patterns from LazyVStack cell views #25947 (wrong call), perf: eliminate remaining FlexFrame anti-patterns from LazyVStack cell views (Batch 3) #26007, fix: replace FlexFrame with Layout protocol to eliminate main-thread hang (LUM-944) #26053, perf: cap ForEach item count and eliminate cell-level FlexFrames (LUM-945) #26092, perf: replace FlexFrame pair in QueuedMessagesDrawer with widthCap (LUM-1011) #26220. Tracked in LUM-1116..frame(maxWidth:)/.frame(maxHeight:)in LazyVStack/LazyHStack/LazyVGrid cell hierarchies. In practice, the same cascade hits non-lazy hierarchies under transitions too (see LUM-1116 / VELLUM-ASSISTANT-MACOS-66 / VELLUM-ASSISTANT-MACOS-KE)..widthCap(N),.frame(width:),HStack { content; Spacer(minLength: 0) },BottomAlignedMinHeightLayout.Start with a conservative scope (Chat/ + MainWindow/); use an allowlist for existing intentional violations; the lint must pass on current main; do not modify Swift source; PR title:
ci(macos): lint rule for .frame(maxWidth:) / .frame(maxHeight:) regressions; link LUM-1116.