Conversation
654e402 to
ce44a4e
Compare
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Source review clear: no actionable blockers found. Reviewed head ce44a4e30f9bf3f5f8dc533ec890bd00a300b693 against base d21f43364fe9b7bdf2643779dab819fd9e8c16bb.
- Contract preserved: all seven migrated browser specs reuse the existing stateless Vite worker fixture while retaining Playwright's fresh per-test browser contexts. Browser-owned routes, storage, fixture globals, identities, native-bridge mocks and held operations remain isolated. The unchanged server helper owns a separate temporary optimizer cache and cleans it up on close or creation failure. This matches the repository's documented stateless-sharing boundary.
- Coverage preserved: reviewed the complete base/head test functions beyond reindentation, including assertions, relative navigation, pending-operation cleanup and terminal scope/write fencing. No cases or assertions are removed in the inspected diff. The direct-Vite guard is narrowed; both functional engines, zero retries and isolated measurement selection remain unchanged. Independent reviews of the migrated specs were integrated before this verdict.
- Validation caveat: source/metadata-only review; I did not execute PR code or run tests. In the existing exact-head CI run, all four functional browser shards, JavaScript and native/tool-integration jobs passed, but measurement setup failed with
Input required and not supplied: path; measurements were skipped andCI requiredis red. This is not merge readiness or current passing measurement evidence. The historical local timing results do not establish a stable end-to-end hosted CI speedup.
Production web/native behavior and live relay integration were not independently runtime-validated by this test-only source review. This is a non-approving review comment, not a GitHub approval.
ce44a4e to
6cedf25
Compare
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Reviewed head 813831a89dd758102e37fb7421e485fc18c0c9be against its reported base f0c6ac892fa3b5296e983ec7b02999dada9c9b6f, and checked integration with current main c3e36a1f1ed4a25441cda20a1e1f53a2e6340d26.
Changes requested: one P2 integration/coverage defect. Main now includes #69’s { alpha: 20, beta: 1 } notification dataset. A conflict-free three-way merge leaves the new fractional-bottom regression with only two short Beta rows, so it no longer exercises a scroll-end boundary. This needs a test-local overflowing setup and an explicit overflow precondition, not reverting the suite-wide fixture savings. See the inline finding.
No other actionable findings in the refreshed stateless-server migration, tokenized CSS repair, external-link synchronization, or signer completion-order test. The production CSS preserves strict fully-visible reading eligibility; the test migration retains fresh browser contexts and existing behavior assertions.
Validation: source/contract review, independent CSS/integration review, and clean exact-base git diff --check. I did not execute PR code or rerun broad suites locally; a collaborator reported focused signer tests/mutation results, which I have not independently verified and am not counting as validation evidence. GitHub reports all current-head checks successful, including both browser engines and the required aggregate. Its synthetic merge still uses the older base, not the newer main integration described above. The documented local-only WebKit checks and packaged-native acceptance are not established by that hosted result. Historical subset timing gains are not proof of an overall CI speedup.
Exit criterion: preserve an overflowing fractional-last-row control at 1440px and 640px on the integrated branch, with fail-without-clearance/pass-with-clearance evidence in Chromium and WebKit. No production redesign or unrelated fixes requested.
| await row.evaluate((element) => { | ||
| element.style.paddingBottom = "0.5px"; | ||
| }); | ||
| for (const width of [1440, 640]) { |
There was a problem hiding this comment.
[P2] Keep this regression overflowing after merging current main
Main c3e36a1 includes #69, which sets this suite to historyCounts: { alpha: 20, beta: 1 }. A three-way merge combines that option with this block without conflicts. The mention branch then has one short fixture Beta row plus the incoming row, so the 950px-high viewport does not exercise the overflowing-list boundary. end() accepts a zero bottom distance, and full visibility can pass even with .feed’s new bottom clearance removed. The newly added regression therefore stops guarding the clipping defect when this lands.
Give just this geometry scenario enough test-local history to overflow (or an equivalent focused overflowing setup), assert scrollHeight > clientHeight before end() at both widths, and preserve the strict ratio-1 check. Keep #69’s small dataset for the other notification cases. Verify the integrated control fails without the clearance and passes with it in both engines. This is an integration coverage issue; I did not find a defect in the CSS fix itself.
Co-authored-by: Amp <amp@ampcode.com> Signed-off-by: Charlie Croom <ccroom@squareup.com> Amp-Thread-ID: https://ampcode.com/threads/T-01a0a149-b278-7550-acc4-356168b0c4b5
…/browser/mentions.spec.mjs tests/browser/terminal-renderer.spec.mjs Co-authored-by: Amp <amp@ampcode.com> Signed-off-by: Charlie Croom <ccroom@squareup.com> Amp-Thread-ID: https://ampcode.com/threads/T-01a0a149-b278-7550-acc4-356168b0c4b5
Co-authored-by: Amp <amp@ampcode.com> Signed-off-by: Charlie Croom <ccroom@squareup.com> Amp-Thread-ID: https://ampcode.com/threads/T-01a0a149-b278-7550-acc4-356168b0c4b5
Co-authored-by: Amp <amp@ampcode.com> Signed-off-by: Charlie Croom <ccroom@squareup.com> Amp-Thread-ID: https://ampcode.com/threads/T-01a0a149-b278-7550-acc4-356168b0c4b5
Co-authored-by: Amp <amp@ampcode.com> Signed-off-by: Charlie Croom <ccroom@squareup.com> Amp-Thread-ID: https://ampcode.com/threads/T-01a0a149-b278-7550-acc4-356168b0c4b5
Co-authored-by: Amp <amp@ampcode.com> Signed-off-by: Charlie Croom <ccroom@squareup.com> Amp-Thread-ID: https://ampcode.com/threads/T-01a0a149-b278-7550-acc4-356168b0c4b5
Co-authored-by: Amp <amp@ampcode.com> Signed-off-by: Charlie Croom <ccroom@squareup.com> Amp-Thread-ID: https://ampcode.com/threads/T-01a0a149-b278-7550-acc4-356168b0c4b5
Co-authored-by: Amp <amp@ampcode.com> Signed-off-by: Charlie Croom <ccroom@squareup.com> Amp-Thread-ID: https://ampcode.com/threads/T-01a0a149-b278-7550-acc4-356168b0c4b5
813831a to
2ad3678
Compare
|
🤖 Closing at Charlie’s request to focus the test-optimization review on #81 → #83. Keeping this branch intact. This is not a claim that all changes are superseded: the fractional row-clipping fix and other correctness/test-ordering repairs documented here are not included in that stack and remain available to recover separately. |
September 16 — preserve the regression after #69
🤖 Current head incorporates merged main through #68. Full CI passed, including JavaScript, native/tool integration, Windows, measurements, all four browser shards and
CI required. DCO and security checks also pass.Addresses Wes's integration-coverage finding: only the mention geometry scenario adds 20 historical Beta messages, without live delivery. All other notification cases retain #69's small fixtures. The regression asserts
scrollHeight > clientHeightbefore scrolling at 1440px and 640px and retains strict ratio-1 containment. The fractional reflow input is now 0.125px: the previous half-pixel input rounded away the clipping with this history. No browser cases, production behavior, retries or tolerances changed in this follow-up.Validation on integrated main: all 14 cases in the complete notification file pass across Chromium/WebKit (49.6s);
bin/pnpm check, pre-push types/10 related tests and all eight DCO trailers pass. Removing the production clearance caused read acknowledgment to fail in both engines. A second temporary control disabled clearance after acknowledgment so the geometry checks could run: both widths failed ratio-1 containment in both engines, and both pass with clearance restored. Fault injection and diagnostic logging were removed. Renewed reviewer approval remains required.Command:
bin/pnpm test:browser tests/browser/notifications.spec.mjs --project chromium --project webkit --no-deps.September 15 — historical CI fixes and verification
🤖 Current-head CI passed at the repaired head: JavaScript, Rust/tool integration, Windows, measurements, all four browser shards and
CI required. Hosted DCO, Semgrep and zizmor also pass. The branch is rebased onto main's merged design foundations (#59), with updated design expectations preserved and merge conflicts resolved. Formal reviewer approval remains required.Current hosted browser-job durations, including setup and artifact work: Chromium 7m04s / 7m48s; WebKit 8m21s / 9m34s. JavaScript took 1m34s and measurements 3m59s. These are current-head observations, not a controlled before/after comparison or evidence of a further speedup.
The new full runs exposed three defects beyond the original pnpm-download HTTP 500 failure. These are addressed in separate commits:
Current local verification:
bin/pnpm checkpasses, including all design guards. All 42 cases in the complete notifications, message-navigation, messages and external-links browser files pass in Chromium and WebKit (2.7m). Fractional containment failed before repair in both engines, including the added narrow-width assertion; both widths pass after repair. The successful narrow WebKit state was rendered and inspected. All 1,553 Vitest tests passed on the preceding head, with no subsequent JS production or unit-test changes; pre-push reran TypeScript and 10 related tests successfully. Reversed signer completion timed out before its test repair and passes afterward. Diff checks and all seven commit sign-offs pass. No sleeps, forced clicks, timeout increases or retry-configuration changes were added.Verification correction: the preceding run passed all browser, measurement and native jobs but failed the design spacing guard on the initial raw
1pxpadding. The final fix uses existing--space-halfand preserves bottom padding across the narrow-screen media query. The fullpnpm checkcommand now passes locally; no guard was bypassed or weakened.Browser command:
bin/pnpm test:browser tests/browser/notifications.spec.mjs tests/browser/message-navigation.spec.mjs tests/browser/messages.spec.mjs tests/browser/external-links.spec.mjs --project=chromium --project=webkit --no-deps. Unit command:bin/pnpm exec vitest run.Separate local gap: the earlier seven-file source-fixture run passed 17/18 cases. The macOS WebKit emoji-composer containment assertion failed by 0.234px; main's exact, unmodified emoji test failed identically in all three repetitions. That separate input-layout issue is unchanged and remains unresolved; the migrated emoji journey passed in hosted Linux CI. Full local native/measurement scan remains deferred.
Inspected 640px WebKit notification state after fractional reflow (synthetic fixture data)
Historical timing evidence below applies only to its linked snapshots. The CI repairs add necessary correctness work and do not claim a new speedup.
Purpose
Continue the browser-cost work in #63 by reusing its stateless Vite worker fixture across agents, avatar loading, messages, mentions, reactions, emoji, and terminal-renderer journeys. Reuse the server, optimizer, and transformed modules; retain a fresh Playwright context and browser-side fixture state for every test. Custom mutable middleware servers are not migrated.
Targets
maindirectly after #63 merged, independent of the remaining PRs. The fixture migration changes no production behavior; the subsequent CI repairs above add tokenized conversation scroll space and repair test ordering. No dependencies, PR-template changes, browser cases added/removed, or assertion weakening. Most diff lines are Biome reindentation after removing the server-only outertry/finally; reviewing with whitespace hidden is useful. Pending-operation cleanup inside the tests is retained. The cache guard no longer permits emoji to recreate a private Vite server directly.Local timing evidence
Baseline: #63 head. Changed snapshot: this head.
Same worktree and machine: macOS arm64, pinned Node 24.18.0 / pnpm 11.8.0 / Playwright 1.60.0, two workers, Chromium and WebKit. Identical 54 case identities across ten complete source-fixture files, including three existing fixture consumers. Baseline followed by two changed-snapshot runs, not simultaneous. One baseline sample: this is evidence, not a confidence interval or a hosted-CI guarantee.
Wall time decreased 17–18%. Summed test durations overlap across workers and are not wall time. Dependency installation, browser installation, native build and shell startup are outside the Playwright measurement; Vite setup/optimization is included but not separately instrumented. No optimizer cache is persisted between these runs.
Reproduction command (run once on each snapshot; repeat the changed snapshot):
Original migration snapshot — coverage and verification
expect-rooted call in all seven migrated browser files against the base: identical, including arguments and matcher chains. No replacement coverage is needed because no behavior assertions move or disappear.git diff --checkpass; hooks were not bypassed.just scan) and hosted CI. Draft for measurement, not a claim of integration readiness. Current repository rules requireCI required,DCO Check, and code-owner review.Hosted CI evidence
CI run 34896980229 passed at this head, including native checks, JavaScript, all browser lanes,
CI requiredand DCO. Verified all 364 unique functional identities match #63's executed set, including colocated workflow journeys; 93/89 cases per engine, plus five isolated measurement cases. No failed, skipped or retried browser cases. The measurement artifacts retain 640/80 histories and the existing budgets; Chromium warm switching measured 35.1–44.3ms. WebKit evidence records three instances of the existing exact ResizeObserver warning, with no new allowance.Functional journeys step durations, excluding separate CI setup/native-build/cache steps (Playwright-owned frontend builds remain included):
This run does not demonstrate an overall CI speedup. The slowest functional step increased from 445s to 489s, despite the local subset improvement. Allocation changed, so same-numbered shards are not equivalent selections. Across the seven migrated files, rounded summed case times changed from 82.8s to 62.0s in Chromium and 74.2s to 88.7s in WebKit; combined 157.0s to 150.7s. Runner variance and different execution placement prevent attributing these differences solely to server reuse. The Vitest step remained 61s; the isolated measurement step took 114s. Compare future merged-main runs before claiming a stable end-to-end reduction. Full local scan and code-owner review remain separate requirements.
🤖 Prepared with Amp for Charlie Croom.