Skip to content

cmux-next iOS lists: follow-ups from the #27664 reviews - #30681

Merged
lawrencecchen merged 6 commits into
feat-cmux-nextfrom
ios-lists-followups
Nov 2, 2026
Merged

lawrencecchen merged 6 commits into
feat-cmux-nextfrom
ios-lists-followups

Conversation

@azooz2003-bit

Copy link
Copy Markdown
Collaborator

Summary

Follow-ups left by the reviews of #27664 (landed as 5e4c938), in the cmux-next iOS chat transcript and agent tab cards.

  1. Very tall images re-decoded in a loop. SentAttachments.fill scaled only the short side, so a 1290x20000 screenshot kept a 600x9300 thumbnail (about 22 MB). A card whose last turns carried two of them passed the 32 MB bound, each landing evicted the other, resident() never held for both, and AgentCardRasterizer.frame re-queued and decoded again at 60 Hz.
    • A thumbnail now also cuts its long side to the middle 2 x side that its fill frame shows (SentAttachments.thumbnail): 600x1200 px, 2.9 MB.
    • The rasterizer holds the thumbnails a waiting card already has (AgentChatPreview.PictureHold) and lends them to the cache while it draws, so an eviction cannot race the draw.
    • The bubble's own .task, which ImageRenderer starts during the draw, finds the lent thumbnail and does not decode it again.
    • A decode lands directly in the holds waiting for it, so with many cards queued a fresh decode is not lost before the card's turn (from this PR's review).
  2. Native-cell measurements were dropped. ChatNativeRowCell.preferredLayoutAttributesFitting returned attributes without ChatRowAttributes.shown, and the layout keeps only measurements that carry it. A native row that is not exact went back to its estimate on the next change above it. The native cell now carries the state it was configured with, and keeps a measurement only at the width its layout was made for.
  3. aRowsHeightForAStateIsTheOneItSettledAt now requires a kept height (knownHeight != nil) for both the open and closed states, so it fails if measurements are dropped.
  4. measuresSettle is per row. While a disclosure change waits for the list's reload, only rows whose disclosures (ChatRowLookup.fold) it moves are held. Another row's growth, such as a streaming answer, is kept.

Fixes, tests, red/green

Command for every red/green below (aws-m4pro-6, simulator cmux-listperf-1, Xcode 26.6):
xcodebuild test -scheme CmuxMobileKit-Package -destination "id=<cmux-listperf-1>,arch=arm64" -enableCodeCoverage NO CODE_SIGNING_ALLOWED=NO -only-testing:CmuxMobileAgentChatTests/SentAttachmentThumbnailsTests -only-testing:CmuxMobileAgentChatTests/NativeRowFoldTests -only-testing:CmuxMobileWorkspacesTests/AgentCardPictureTests

Red commit 0139cca holds the tests only (plus test hooks: SentAttachmentThumbnails.evict(attachmentID:), and AgentCardRasterizer.frame made internal).

Fix Test Red (0139cca) Green
1, long-side cap SentAttachmentThumbnailsTests.twoVeryTallImagesSettleUnderTheBound (two 600x9000 images, 32 MB bound) not resident; decodes 62 -> 82 while settled; 21.6 MB kept; thumbnail not in memory passes
1, rasterizer holds AgentCardPictureTests.anEvictedThumbnailIsDecodedOnceThenTheCardIsDrawn 2 decodes (the held image was decoded again) 1 decode; card drawn with both images
1, hold filled on landing SentAttachmentThumbnailsTests.aWaitingPictureHoldsWhatIsDecodedForIt red at d11d586: not resident, 3 decodes green at ee1dc9c: 2 decodes
2 NativeRowFoldTests.aNativeRowKeepsTheHeightItsCellMeasured nothing kept; row back to its estimate (30 pt off) passes
3 NativeRowFoldTests.aRowsHeightForAStateIsTheOneItSettledAt (strengthened) passes (measurements of SwiftUI rows were already kept) passes
4 NativeRowFoldTests.anotherRowsGrowthIsKeptWhileADisclosureIsPending the other row's growth was dropped passes

The review subagent's P3s are fixed in d11d586:

  • A card whose key no longer needs drawing drops its hold.
  • When the scaled copy cannot be made, the fallback draws the whole image scaled into the crop.
  • A native measurement is kept only at its layout's width.
  • A reused native cell clears shown before a configure that may return early.

Testing

  • iOS simulator, full CmuxMobileKit-Package suite at ee1dc9c, aws-m4pro-6: TEST SUCCEEDED. 1,904 Swift Testing tests in 13 test runs, all passed.
  • macOS swift test in CmuxMobileKit at ee1dc9c, aws-m4pro-5: 1,598 tests, 4 failures. All 4 are the known host-only PreviewInsufficientSpace failures (LinkFilePreviewStreamTests x3, TerminalFloodTests x1); nothing else failed.
  • python3 scripts/cmux-next/prepush-gate.py: no new violations.
  • Correctness review (subagent) of ac20cd3: SHIP. The P2 and P3s above are fixed; one P3 was not taken, see below.

Observed / Not run

  • Observed: the red and green runs above, both full suites, and the gate.
  • Not taken from the review: P3 Disambiguate Sentry environment between production and dev #2 said the lent check in SentAttachmentThumbnails.start is dead because the bubble's .task runs after the draw. A diagnostic run showed otherwise: the decode started inside ImageRenderer's synchronous draw (2 decodes at draw time). The check is what turns anEvictedThumbnailIsDecodedOnceThenTheCardIsDrawn green, so it stays.
  • Not run: no on-device or tagged-build dogfood, and no perf trace of the tab grid with real tall screenshots. The loop is covered by the unit tests only.
  • AgentCardPictureTests uses SentAttachmentThumbnails.shared. Its decode-count assertion could be disturbed by another suite decoding into the shared cache at the same moment; its suite is .serialized, and it passed in every run here.

No capture: no visible change apart from tall images in agent cards no longer re-decoding. The thumbnails draw the same pixels: the crop keeps everything the bubble's fill frame shows.

Changelog

Fixed: an agent tab card showing two very tall screenshots no longer re-decodes its images in a loop.

🤖 Generated with Claude Code

azooz2003-bit and others added 4 commits November 1, 2026 01:04
…s (red)

- SentAttachmentThumbnailsTests.twoVeryTallImagesSettleUnderTheBound: two 600x9000
  images under the 32 MB bound evict each other and re-decode forever.
- AgentCardPictureTests.anEvictedThumbnailIsDecodedOnceThenTheCardIsDrawn: a thumbnail
  the card already had is decoded again when the cache lets it go while the card waits.
- NativeRowFoldTests.aNativeRowKeepsTheHeightItsCellMeasured: a native cell's
  measurement is dropped (no ChatRowAttributes.shown).
- NativeRowFoldTests.anotherRowsGrowthIsKeptWhileADisclosureIsPending: every row's
  measurement is dropped while any disclosure change is pending.
- aRowsHeightForAStateIsTheOneItSettledAt now requires a kept height.

Test hooks only (SentAttachmentThumbnails.evict(attachmentID:), AgentCardRasterizer.frame
internal); no behavior change.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Very tall images: a thumbnail scaled only its short side, so a 1290x20000
  screenshot kept 600x9300 px (~22 MB); two in a card's last turns passed the
  32 MB bound, each landing evicted the other and the rasterizer re-queued and
  re-decoded them on every frame. A thumbnail now also cuts its long side to the
  middle 2x side its fill frame shows (600x1200, 2.9 MB), and the rasterizer holds
  the thumbnails a waiting card already has (AgentChatPreview.PictureHold) and
  lends them to the cache while it draws, so an eviction cannot race the draw and
  the bubble's own .task (started by ImageRenderer) does not decode them again.
- Native-cell measurements: ChatNativeRowCell's sizing pass returned attributes
  without ChatRowAttributes.shown, so the layout dropped every native measurement
  and a non-exact native row fell back to its estimate. The native cell carries
  the state it was configured with, as ChatRowCell does.
- measuresSettle is per row: while a disclosure change waits for the list's
  reload, only rows whose disclosures (ChatRowLookup.fold) it moves are held;
  other rows' growth (a streaming answer) is kept.

Red (0139cca) / green (this commit), aws-m4pro-6, iOS simulator cmux-listperf-1:
xcodebuild test -scheme CmuxMobileKit-Package
  -only-testing:CmuxMobileAgentChatTests/SentAttachmentThumbnailsTests
  -only-testing:CmuxMobileAgentChatTests/NativeRowFoldTests
  -only-testing:CmuxMobileWorkspacesTests/AgentCardPictureTests
red: twoVeryTallImagesSettleUnderTheBound (decodes 62 -> 82, 21.6 MB kept),
anEvictedThumbnailIsDecodedOnceThenTheCardIsDrawn (2 decodes),
aNativeRowKeepsTheHeightItsCellMeasured (nothing kept, back to estimate -30 pt),
anotherRowsGrowthIsKeptWhileADisclosureIsPending (growth dropped); green: all 16 pass.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ve measurements

From the correctness review of ac20cd3:
- The hold is a reference (AgentChatPreview.PictureHold / SentAttachmentHold), so
  the cache can fill it (next commit); a test that a waiting picture keeps what is
  decoded for it (aWaitingPictureHoldsWhatIsDecodedForIt, red here: 3 decodes).
- A card whose key no longer needs drawing drops its hold (it pinned up to its
  thumbnails until the tab closed).
- A thumbnail whose scaled copy could not be made draws the whole image scaled
  into its crop, not a zoomed-in middle.
- A native cell keeps its measurement only at the width its layout was made for,
  always returns it with the state it drew, and a reused cell clears its state
  before a configure that may return early.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A waiting card's hold took a thumbnail only on the card's next turn in the
rasterizer's queue; with many image cards waiting, the byte bound could let a
fresh decode go before then, and it was decoded again (the eviction ping-pong at
queue scale, review of ac20cd3 P2). SentAttachmentThumbnails registers a hold
for each decode it waits on and fills it when the decode lands.

Red (previous commit) / green (this commit), aws-m4pro-6, iOS simulator:
xcodebuild test -only-testing:CmuxMobileAgentChatTests/SentAttachmentThumbnailsTests
aWaitingPictureHoldsWhatIsDecodedForIt: red (not resident, 3 decodes) -> green.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@azooz2003-bit azooz2003-bit added the risk:normal Risk tier (docs/ci/risk-tiers.md) label Nov 1, 2026
@github-actions

github-actions Bot commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

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

Review 6758f50: 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=pr30681 -f ref=ios-lists-followups -f platform=macos -f backend_mode=local

Shipped in:

@blacksmith-sh

This comment has been minimized.

azooz2003-bit and others added 2 commits November 2, 2026 11:53
…et go when the grid moves

Review of #30681 (P2): every waiting card held the thumbnails it had until its
picture was drawn, outside the cache's 32 MB bound and after the grid moved or
hid (about 175 MB with 30 cards waiting), with no release on a memory warning.
The card waiting now stays first in the queue until its decodes land, so only
one card holds (at most six cropped thumbnails); the hold is let go when the
grid stops resting, on a memory warning, when the card is dropped or undrawable.

Also: the decode-count test counts decodes for its own attachments only
(SentAttachmentThumbnails.decodes(attachmentID:)), so another suite sharing the
cache cannot move it, and its ids are unique per run (it failed 1 of 3
iterations when a previous iteration's settled keys were reused).

Tests (iOS 26.5 simulator, aws-m4pro-6): new
onlyTheCardWaitingHoldsAndAMovingGridOrMemoryWarningLetsGo (does not compile
without the change: no single hold existed); AgentCardPictureTests and
SentAttachmentThumbnailsTests -test-iterations 5 all green; full
CmuxMobileKit-Package suite green.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cursor

cursor Bot commented Nov 2, 2026

Copy link
Copy Markdown

Bugbot needs on-demand usage enabled

Bugbot uses usage-based billing for this team and requires on-demand usage to be enabled.

A team admin can enable on-demand usage in the Cursor dashboard.

@azooz2003-bit

Copy link
Copy Markdown
Collaborator Author

Review (Claude subagents, read-only, correctness first): SHIP at 6758f50.

  • At 0953544 (the four fixes plus a clean merge of feat-cmux-next): SHIP. Tall thumbnails keep the centred crop the bubble shows (checked tall, wide and 288 px); hold and lent keyed like the cache; native heights kept only at the layout's width; per-row measuresSettle covers every disclosure key. One P2: held images were not bounded and outlived the grid.
  • That P2 fixed in 6758f50: one waiting card holds at a time (it stays first until its decodes land), released when the grid moves, on a memory warning, on drop or an undrawable key. No starvation, no re-decode loop, the two-tall-images anti-loop guarantee kept. The decode-count test counts only its own attachments with per-run ids.

iOS 26.5 simulator (aws-m4pro-6): full CmuxMobileKit-Package suite green; AgentCardPictureTests and SentAttachmentThumbnailsTests 5/5 iterations.
P3s left (order only, memory warning does not purge the cache itself, | in ids for test counts).

@azooz2003-bit azooz2003-bit 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 2, 2026
@lawrencecchen

Copy link
Copy Markdown
Contributor

Merge service note: this PR has ready-to-land, but the merge round leaves it out with "no comment whose first line is "Review (): SHIP at 6758f50"". The receipt from 20:14Z has markdown bold and a final period on its first line (**SHIP at …**.), so it does not match. The reviewer can post the same receipt again with a plain first line, exactly:

Review (Claude subagents, read-only, correctness first): SHIP at 6758f5003ca2bd8477873809b97db9996c8eb481

The red release-notary / Guard status checks are a base finding (fixed on feat by #32548), so they do not hold this PR.

@lawrencecchen

Copy link
Copy Markdown
Contributor

Review (claude-lead-review-subagent): SHIP at 6758f50

Read-only review of the diff from merge-base d9d62ba to 6758f50 (9 files). I did not build or run anything. Correctness came first, then the CLAUDE.md rules.

Checked and found correct:

  • SentAttachments.thumbnail and crop: the visible part of a 200x150 (4:3) or 96x96 scaledToFill frame along the long side is at most 4/3 of the short side, so a centered crop to 2 x side keeps every pixel the bubble shows. This holds for portrait and landscape images and also when the short side is below side (no upscale). The full-screen preview uses the file bytes, not the thumbnail, so the crop does not lose content there. The fallback (scaled ?? image) draws into the same centered frame. UIGraphicsImageRenderer off the main thread is safe.
  • resident(_:holding:) and landed: a decode that lands goes into each live hold that waits for it. The hold keeps only the keys that turns draws. Undecodable keys (settled == false) are skipped, so the waiting card cannot stall on them. Weak waiters are cleared on landing.
  • AgentCardRasterizer.frame: the waiting card stays first until its decodes land. Only one hold exists at a time. The hold is released on want with an undrawable key, on drop, on setIdle(false), on a memory warning and before the draw (the local hold keeps the images alive through drawing). The ahead path inserts a card that is not resident at the front. I found no ping-pong and no starvation path, because every decode completes.
  • drawing lends the images only during the synchronous ImageRenderer pass and restores lent with defer. start and image both look in lent.
  • ChatNativeRowCell.preferredLayoutAttributesFitting: shown comes from the layout item as configured (the same pattern as ChatRowCell). It is cleared before the early return in configureNative. It is set only when layoutWidth (environment width + 40) matches the attributes width. Because ChatRowAttributes.isEqual compares shown, a measurement at the same height still reaches shouldInvalidateLayout, which records it and returns false. Exact native rows still ignore measured.
  • measureSettles(item): ChatRowLookup.fold covers every disclosure key prefix the rows read (tool-, group-, thought-, diff-, code-). So a row whose fold the pending change does not move draws the state of its item, and its measurement can be kept.
  • The tests assert the changed behavior, and the red evidence in the description matches what the code paths do. No try!, no blocking call on the main thread, no new font literals, no animations and no new user-facing strings.

Non-blocking notes (P3):

  1. SentAttachmentThumbnails.swift:39 decodesByAttachment is a counter for tests only. It lives in production, grows with every distinct attachment id and is never trimmed, and it splits the key string on each decode. It is small, but you could bound it or put it behind DEBUG.
  2. AgentChatPreview.picturesResident(_:) (the overload with no hold) and SentAttachmentThumbnails.resident(_:) have no production callers now (REVIEW.md, "remove the API your change made unreachable"). Also, each call of resident(_:) registers a throwaway hold in waiting while a decode runs. That is harmless, but it is extra work.
  3. New test hooks in production source: evict(attachmentID:), holdsThumbnails, and frame() made internal. This follows the existing decodes/renders pattern, but REVIEW.md section 8 discourages it.
  4. While a card waits, other queued cards wait behind it for one decode duration. This is intended (it bounds the hold to one card), but a tall image can delay later cards by the decode time.

Checks: the only reds are nightly-guards / Guard status and release-notary, the known base finding (fixed on feat by #32548). The PR causes no red of its own.

@lawrencecchen
lawrencecchen merged commit 48fc806 into feat-cmux-next Nov 2, 2026
138 of 141 checks passed
@lawrencecchen
lawrencecchen deleted the ios-lists-followups branch November 2, 2026 22:38

This branch was previously deployed

1 inactive deployment
ci-routing-agent — 6758f500 Deployed Nov 2, 2026 by azooz2003-bit via cmux-tui tree preflight #102479
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.

2 participants