Skip to content

Collapse stale composer height on tab return - #132

Closed
hamzamerzic wants to merge 4 commits into
mainfrom
fix/composer-height-foreground-20260722
Closed

Collapse stale composer height on tab return#132
hamzamerzic wants to merge 4 commits into
mainfrom
fix/composer-height-foreground-20260722

Conversation

@hamzamerzic

Copy link
Copy Markdown
Collaborator

Summary

  • centralize composer textarea sizing across typed input, voice input, restores, and send cleanup
  • reconcile programmatic empty-value commits so a previously multiline composer cannot retain stale inline height
  • reconcile textarea geometry before measuring the composer on visibilitychange and pageshow

Production evidence

The originating production chat reproduced an empty composer forced to about 290px after returning to the tab; the foreground reconciliation collapsed it to the expected one-row geometry (about 50px including the composer shell). No production deployment is performed by this PR.

Verification

  • npm test — 1,808 tests passed (1,761 lib + 47 hooks)
  • npm run build
  • git diff --check

@hamzamerzic
hamzamerzic force-pushed the fix/composer-height-foreground-20260722 branch from 13bd87a to 9913806 Compare July 22, 2026 16:45
@hamzamerzic

Copy link
Copy Markdown
Collaborator Author

Review of exact head 9913806ce40b854416bc092eca65c8a57b794c1f: blocking pending a deterministic browser regression and green E2E.

The implementation direction looks sound. I traced every composer geometry/value path I could find: native typing, restored drafts/prompts, controlled voice transcripts, permission-error copy, failed-send restoration, authoritative failed-attempt cleanup, both queue/fresh send clears, hidden retained-pane reactivation, visibilitychange, and pageshow. Centralizing the write/read/class toggle in composerTextareaSizing.js, running controlled-value sizing in a layout effect, and synchronously resetting on send are coherent. The foreground path does one write→scrollHeight layout read per mounted chat and the existing observer/rAF work coalesces; given the shell's bounded retained-view model and infrequent foreground event, I don't see that as a merge blocker.

Blocker — the new browser test never exercises that foreground path. In CI job 88989203592, tests/frontend.spec.mjs:111 failed on both the original attempt and retry at the assertion that the first page became hidden; 297 other tests passed. Headless Chromium keeps both pages' document.visibilityState as visible when context.newPage() opens. I independently reproduced visible → visible → visible before opening the second page, after opening it, and after bringToFront(). Consequently the test never reaches bringToFront(), never fires the product's visible-edge handler, and does not prove the reported repair.

Please make this lifecycle transition deterministic. This same spec already demonstrates the repository's accepted technique around line 1434: override the document's configurable visibilityState getter, dispatch the real visibilitychange, then override to visible and dispatch again. For this regression, assert the stale 280px/tall state remains after the hidden edge, then assert it collapses only after the visible edge. A separate PageTransitionEvent('pageshow') assertion would cover the second installed trigger. Blur/focus is not an adequate substitute because this implementation does not listen to those events.

Also, this branch is now two commits behind main, including #117, which overlaps ChatView.jsx and ChatInputBar.jsx. Please update it and rerun the combined exact-head checks before merge.

Non-blocking cleanup: ChatInputBar.jsx still says handleTextareaChange owns chat__pill--tall, and the queue-send comment says that handler re-evaluates the class. Both became stale when this PR moved ownership to resizeComposerTextarea/the parent layout effect.

My exact-head checks: frontend unit/hook suite 1,808 passed, production build passed (matching CI's index-BMTT1d24.js and Shell-CzIIlovG.js), and git diff --check passed. I am not approving this SHA while its only browser-level regression is deterministically red before the trigger under test.

@hamzamerzic

Copy link
Copy Markdown
Collaborator Author

Two review rounds complete on exact head 9913806ce40b854416bc092eca65c8a57b794c1f. Pass 1 checked every value owner—typing, voice, draft restore, failed-send restore, queued/direct send cleanup, visibility return, and pageshow—against the centralized sizing boundary. Pass 2 checked hidden retained panes, zero scrollHeight, layout-effect timing, ResizeObserver interaction, tall-class cleanup, and the 280px cap. Focused tests pass 5/5 and a production build passes. No finding; recommendation: merge once hosted CI is green.

@hamzamerzic

Copy link
Copy Markdown
Collaborator Author

Review fix pushed on exact head 06e67b63: the browser regression no longer relies on headless Chromium backgrounding a second page. It now drives deterministic hidden→visible visibilitychange edges, proves the stale 280px/tall geometry remains while hidden, proves collapse on the visible edge, and separately covers the bfcache pageshow trigger. Stale ownership comments were corrected and current main was merged cleanly. Focused sizing/voice tests: 7 passed; production build and pre-push frontend gate pass. The local disposable E2E helper could not run because this container has no Docker, so hosted E2E is the required final proof.

@hamzamerzic

Copy link
Copy Markdown
Collaborator Author

Latest exact 06e67b6 fixes the E2E visibility-state protocol, but it still does not contain the production recurrence fix at 721b8039. Its helper resets height to auto and then trusts scrollHeight even when the authoritative value is empty; real Chromium can report the stale/capped 280px height during multi-pane foregrounding, while the headless fixture reports ~31px and passes. The production correction makes empty a semantic one-line state (no geometry read), passes authoritative text explicitly for pre-React typing/voice/restore paths, and keeps hidden-pane reconciliation. Please reconcile that commit into this PR and add the hostile empty+scrollHeight=280 regression; do not merge 06e as-is.

@hamzamerzic

Copy link
Copy Markdown
Collaborator Author

Newest local improvement is now folded into this same PR rather than packaged separately. Exact head 40e3b02f treats an empty controlled value as a semantic one-line state, so transient Chromium scrollHeight=280 during pane/foreground layout cannot re-expand a blank composer. The authoritative value is threaded through layout/restore/foreground sizing while typing and voice remain centralized under the controlled composer owner (the cherry-pick conflicts were resolved without reintroducing duplicate sizing paths). Focused tests: 6 sizing + 2 voice; production build and pre-push frontend gate pass. Hosted CI has restarted on this final head.

@hamzamerzic

Copy link
Copy Markdown
Collaborator Author

Independent exact-head verdict for 40e3b02fdb446a9464a964009b9edd242c21764f: approve / no functional blocker found. (GitHub will not record an approval because this review account is also the PR author.) This head now includes the production semantic correction from 721b8039: an authoritative empty composer bypasses scrollHeight entirely and resets to intrinsic one-line geometry, while non-empty typing, restore, voice, send cleanup, hidden-pane reactivation, visibilitychange, and pageshow share one sizing owner. I reproduced the prior head returning {height: 280px, tall: true} for an empty textarea with Chrome-like stale scrollHeight=280; this head returns intrinsic auto/not-tall and covers authoritative text before DOM value commit. The foreground E2E drives deterministic hidden→visible and bfcache pageshow triggers; the originating production run additionally exercised two focused chat panes through reload/tab return at 31px inputs / 50px pills. Focused sizing/voice tests passed 8/8, production build and diff check passed locally, the served production bundle was fresh, and all exact-head hosted jobs—including backend and E2E—are green. The branch still needs the normal update-to-current-main check after the separate #139 revert, but I do not see a composer/scroll reason to hold this tree.

@hamzamerzic

Copy link
Copy Markdown
Collaborator Author

Landed on protected main through reviewed integration #147 (merge 226106c). The integrated patch matches this PR's reviewed head; closing the superseded source PR to keep the queue truthful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant