Skip to content

fix(ci): multiline match + empty-allowlist tolerance in flexframe lint (addresses #27554 reviews) #27556

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ashleeradka merged 1 commit into from
Apr 22, 2026
Merged

fix(ci): multiline match + empty-allowlist tolerance in flexframe lint (addresses #27554 reviews) #27556

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 19 additions & 2 deletions clients/scripts/check-flexframe.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,16 @@ fi
# (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 \
#
# `-U --multiline-dotall` enables multiline matching so `.frame(` wrapped
# across lines (opening paren on one line, `maxWidth:` on the next) is
# caught. Without this, code formatted as `.frame(\n maxWidth: …\n)`
# bypasses the lint silently — a real escape already present at
# ChatLoadingSkeleton.swift before this PR. ripgrep reports only the
# START line of a multiline match, so the comment filter (which only
# inspects that line) stays correct: the outer `.frame(` never itself
# starts with `//`.
RAW_HITS=$(rg -U --multiline-dotall -n --no-heading "$PATTERN" "${SCAN_DIRS[@]}" 2>/dev/null \
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep multiline matches as a single violation record

rg -U --multiline-dotall -n emits one output line for each line participating in a multiline match, not one line per match. In this script that means a single call like .frame(\n maxWidth: ...) is split into multiple normalized keys (for example ...|.frame( and ...|maxWidth: ...), so NEW_COUNT over-reports and the allowlist no longer follows its documented “one entry per occurrence” contract. This makes baseline management brittle and can let match-fragment churn slip through unintentionally; parse only the match start (e.g. --vimgrep/JSON) or collapse multiline hits before normalize.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

@ashleeradka ashleeradka Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declining as non-blocking. Reasoning: the lint's correctness invariant is 'any new violation fails CI', and the current implementation holds that — a multi-line match produces N allowlist entries that all must be present for the match to count as allowlisted. If any line of the match changes, the changed line is flagged as a new violation (correctly, not silently). The cost of the fragment-dedup fix (parsing rg --json with jq or building a consecutive-line heuristic that has to distinguish 'two adjacent single-line matches' from 'one multi-line match') outweighs the benefit (one logical violation = one allowlist entry). Current multi-line instances in the scanned dirs: 1 (ChatLoadingSkeleton). Will revisit if the codebase grows enough multi-line instances that allowlist verbosity becomes a real friction. Thanks for the review.

| grep -vE '^[^:]+:[0-9]+:[[:space:]]*//' \
|| true)

Expand Down Expand Up @@ -131,9 +140,17 @@ HEADER
fi

# Load the allowlist (strip comments + blank lines), preserving multiplicity.
#
# `grep -v` exits 1 when no lines match — under `set -euo pipefail` that
# would abort the script if the allowlist is ever header-only (e.g. after
# a full cleanup or `--update-baseline` with zero observed violations).
# `|| true` tolerates that edge case so a clean state stays clean.
ALLOWLIST_ENTRIES=""
if [[ -f "$ALLOWLIST_FILE" ]]; then
ALLOWLIST_ENTRIES=$(grep -vE '^([[:space:]]*#|[[:space:]]*$)' "$ALLOWLIST_FILE" | sort)
ALLOWLIST_RAW=$(grep -vE '^([[:space:]]*#|[[:space:]]*$)' "$ALLOWLIST_FILE" || true)
if [[ -n "$ALLOWLIST_RAW" ]]; then
ALLOWLIST_ENTRIES=$(printf '%s\n' "$ALLOWLIST_RAW" | sort)
fi
fi

# New violations = observed - allowlist (multiset difference preserved by `comm -23`).
Expand Down
2 changes: 2 additions & 0 deletions clients/scripts/flexframe-allowlist.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,13 @@ clients/macos/vellum-assistant/Features/Chat/ChatEmptyStateView.swift|.frame(max
clients/macos/vellum-assistant/Features/Chat/ChatEmptyStateView.swift|.frame(maxWidth: VSpacing.chatBubbleMaxWidth)
clients/macos/vellum-assistant/Features/Chat/ChatErrorToastView.swift|.frame(maxWidth: .infinity)
clients/macos/vellum-assistant/Features/Chat/ChatErrorToastView.swift|.frame(maxWidth: .infinity, alignment: .leading)
clients/macos/vellum-assistant/Features/Chat/ChatLoadingSkeleton.swift|.frame(
clients/macos/vellum-assistant/Features/Chat/ChatLoadingSkeleton.swift|.frame(maxWidth: .infinity, alignment: .trailing)
clients/macos/vellum-assistant/Features/Chat/ChatLoadingSkeleton.swift|.frame(maxWidth: VSpacing.chatBubbleMaxWidth * 0.45, alignment: .trailing)
clients/macos/vellum-assistant/Features/Chat/ChatLoadingSkeleton.swift|.frame(maxWidth: VSpacing.chatBubbleMaxWidth * 0.65)
clients/macos/vellum-assistant/Features/Chat/ChatLoadingSkeleton.swift|.frame(maxWidth: VSpacing.chatBubbleMaxWidth, alignment: .leading)
clients/macos/vellum-assistant/Features/Chat/ChatLoadingSkeleton.swift|.frame(maxWidth: VSpacing.chatColumnMaxWidth, alignment: .leading)
clients/macos/vellum-assistant/Features/Chat/ChatLoadingSkeleton.swift|maxWidth: VSpacing.chatBubbleMaxWidth * assistantLineWidths[idx],
clients/macos/vellum-assistant/Features/Chat/ChatView.swift|.frame(maxWidth: .infinity, maxHeight: .infinity)
clients/macos/vellum-assistant/Features/Chat/ChatView.swift|.frame(maxWidth: .infinity, maxHeight: .infinity, alignment: .top)
clients/macos/vellum-assistant/Features/Chat/ChatView.swift|.frame(maxWidth: .infinity, maxHeight: .infinity, alignment: .top)
Expand Down