Skip to content

REOPEN-F7 follow-up: empty-label test counts layouts through the measure seam, no test-only read in source (#32616 review P2) - #32799

Merged
lawrencecchen merged 1 commit into
feat-cmux-nextfrom
fix/reopen-f7-label-measure-seam
Nov 3, 2026
Merged

lawrencecchen merged 1 commit into
feat-cmux-nextfrom
fix/reopen-f7-label-measure-seam

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #32616 (REOPEN-F7, empty labels need no TextKit layout), which cmux-bot merged at 0b92974 before its review P2 was fixed. This PR is that fix, with no product change:

  • Review P2 (REVIEW.md section 8): LabelText.lastLaidOutString, a test-only read in production source, is removed.
  • LabelText.measure takes its TextKit layout as a parameter (layOut, type LabelText.Layout, default laidOutMetrics). anEmptyLabelLaysNoTextOut counts layouts through it.
  • Review P3s: one emptyLine(font:) builder instead of two copies; the test covers a font with nonzero leading (Times New Roman 14) and no font.

Testing

  • EmptyLabelMeasureTests (CmuxNextDesignTests):
    • anEmptyLabelLaysNoTextOut: 0 layouts for an empty text at an unbounded width over 8 fonts and 4 shapes; then 1 layout each for a text and for an empty text at a set width.
    • anEmptyLabelLaysOutAsTextKitWould: the fast result equals TextKit at scales 1-3.
  • Red: scratch commit 33e7f76 (the seam and this test, no fast path), compile job 30eb160d passed. Fleet step 06fe0512040c855ed90c899f failed as expected: layouts → 32 == 0 and layouts → 34 == 2. The parity test passed.
  • Green: the same change at c1e340c (the REOPEN-F7: an empty label's size needs no TextKit layout (sidebar rows at window build) #32616 branch). Fleet step 748d04ad5fa647e33d935cf5 passed both tests. Compile job 8c0726cbe2c0466855a15cd2 passed (CmuxNext, app, cmux-tui). This head is c1e340c cherry-picked onto the feat tip, with the same diff in these two files. The compile and green at this head are below.
  • At this head 8fedf1a: compile job c6925fa2300771e49145a460 passed (CmuxNext, app, cmux-tui). Both tests pass in steps bee43816 (aws-m4pro-9) and 3554f3fe (aws-m4pro-6). Then the test process ends with a failure after about 665 s (xctest signal 6, after Contacts CoreData XPC errors), and the lane marks the suite failed.
  • Base finding, not from this PR: the same suite at the feat tip 73ba3f9, without this change, shows the same pattern (step d823cd6b, Blacksmith run 41565867427): both tests pass, then exit 1 after 665 s. At c1e340c on the older base a14bdc2, the lane passed (step 748d04ad).

Review

A review subagent gave SHIP at c1e340c (comment 6592935500 on #32616). It noted that layOut is a parameter whose default is the production function, and that the parameter adds no stored or debug-only state (P3, no change needed).

Observed / Not run

  • Observed: the fleet runs above.
  • Not run: no pixel check (the measured values do not change; the parity test checks them).

Changelog

none

…s layout seam (review P2)

Review of #32616 (REVIEW.md section 8): lastLaidOutString was a test-only
read in production source. It is removed. LabelText.measure now takes
its TextKit layout as a parameter (layOut, default laidOutMetrics), the
seam a measure already has; anEmptyLabelLaysNoTextOut passes a counting
layout and expects no layout for an empty text at an unbounded width,
over 8 fonts (one with nonzero leading, and no font) and 4 shapes, and
one layout each for text and for an empty text at a set width. The
empty line is built in one place (emptyLine), not twice (review P3).

(cherry picked from commit c1e340c)
@lawrencecchen lawrencecchen added the risk:low Risk tier (docs/ci/risk-tiers.md) label Nov 3, 2026
@github-actions

github-actions Bot commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

cmux status (one comment, edited in place; each line is updated by the job that owns it)

Review 8fedf1a: HQ feed card · gallery before/after runs · checks

App of this head (app.zip, kept 1 day)

gh workflow run reload-build.yml -R manaflow-ai/cmux --ref feat-cmux-next -f tag=pr32799 -f ref=fix/reopen-f7-label-measure-seam -f platform=macos -f backend_mode=local

Shipped in:

@github-actions github-actions Bot added risk:normal Risk tier (docs/ci/risk-tiers.md) and removed risk:low Risk tier (docs/ci/risk-tiers.md) labels Nov 3, 2026
@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Base finding (not from this PR): the Swift test lane on feat-cmux-next 73ba3f9 ends with a failure about 665 s after the selected tests pass. Base step: d823cd6b71adc026df5cf8ce, https://github.com/manaflow-ai/cmuxterm-hq/actions/runs/41565867427. On the owned Macs it is xctest signal 6 after Contacts CoreData XPC errors (steps bee43816, 3554f3fe). The same tests pass in all of these runs.

@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Review (subagent opus): SHIP at 8fedf1a

  • Same change as reviewed: at this head, both files have the same blobs as c1e340c (LabelText.swift 30ee426, EmptyLabelMeasureTests.swift 0da779d). The base blobs equal REOPEN-F7: an empty label's size needs no TextKit layout (sidebar rows at window build) #32616's merged state (0b92974 = ecce1e0 = 73ba3f9). Feat has not changed these files since 73ba3f9.
  • Callers: git grep at 8fedf1a and at the feat tip finds one production caller, ChromeLabel.metrics(width:) (ChromeLabel.swift:194). It uses the default layOut, so its behavior is the same. lastLaidOutString has no remaining reference.
  • Correctness: the fast path is unchanged. emptyLine(font:) builds the same string as the two copies it replaces. The @MainActor Layout type agrees with the module's .defaultIsolation(MainActor.self). The count test now checks the negative cases too (text, and empty text at a set width, give 2 layouts). The parity test adds Times New Roman 14 (leading) and nil.
  • P3, no change: layOut is an injected function parameter. It adds no stored or debug-only state, so REVIEW.md section 8 is met.
  • Lane red: I agree it is a base finding. Both tests pass at this head (bee43816, 3554f3fe). The same 665 s xctest exit shows at the feat tip without this change (d823cd6b). A change to label measurement cannot cause a Contacts CoreData XPC abort.

@lawrencecchen lawrencecchen added the ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds label Nov 3, 2026
@lawrencecchen
lawrencecchen merged commit 4bad31b into feat-cmux-next Nov 3, 2026
265 of 286 checks passed
@lawrencecchen
lawrencecchen deleted the fix/reopen-f7-label-measure-seam branch November 3, 2026 03:13
@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Correction to my base-finding comment above: the failure about 665 s after the tests passed was not a worker fault. It was the test-bundle run-loop guard holding xctest's main thread, fixed on feat-cmux-next by #33023 / #33013 (ecfbb44) and #33276.

This branch was previously deployed

1 inactive deployment
ci-routing-agent — 8fedf1a0 Deployed Nov 3, 2026 by lawrencecchen via wait for same-key tree publisher #103344
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds risk:normal Risk tier (docs/ci/risk-tiers.md)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant