Repository navigation
test(unit): wait for the RelayStaff toast; cut Settings work in App.profile - #1973
Conversation
kalvinnchau
left a comment
There was a problem hiding this comment.
🤖 One test-ordering regression to address before merging.
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating Rocket, Mantis and Groot coverage.
One test-determinism issue is described inline. The sidebar case still exercises the unscoped-to-scoped navigation transition, with representative section-button interaction retained elsewhere; the toast assertion still requires visibility.
Reviewed head ae0ea65bf9128f1106e4efa17f0830dca8bfd867 against base 36a283b2386473130430890faa3b06c6e45d4a6b. Source-only review of both changed test files, relevant callers/cache lifecycle, PR description and commit metadata; no attachments in the description. No tests or app were run by this review.
Existing hosted evidence: Actions run 41051665992 reports CI required successful. Its full-file artifacts report 23 App.profile tests and 136 RelayStaff tests passing; the two flake shards cover ten combined iterations of 24 selected tests each, with 135 other tests skipped per iteration. The missing synchronization is identified from source; the failing ordering was not reproduced. These passing runs do not prove that all flakes are eliminated, and the author’s local before/after timing claims were not independently reproduced.
…rofile Fixes the RelayStaff row (section 7) and the App.profile row (section 13) of the 23-30 Oct CI flake sweep note. The InboxPage and MemberAdministration rows are left out because the code does not confirm their causes; see the end of this message. Before editing, I re-checked origin/main (7166858) and the open PRs. No main commit since this branch's base touches these files. #1730, #1587 and #1556 edit App.profile.test.tsx, but none changes :261 or :590 (#1730 adds more work to :590). No open PR touches RelayStaff.test.tsx. Nothing was dropped as already covered. RelayStaff.test.tsx:1903 "enforcement error finished while closed reaches reopened detail" - CI: 39982451663 (job 120512955775), 40045357815 (120725704072), 40217567203 (121241926901), 40538472709 (122310064282), 40547430636 (122341694882), 40881478558 (123473832890), 40923691896 (123623781223). - Cause: in the logs of 123623781223 and 123473832890, toBeVisible gets the buzz-toast-title element, so the toast was in the DOM but hidden. Sonner inserts a toast with data-mounted="false" and its CSS sets opacity 0. A passive effect then sets data-mounted at default priority, so the attribute is applied in a later commit. The test asserted at once after awaiting an unrelated button. - Change: wait for the toast with waitFor(... toBeVisible()). Visibility is still asserted. "Native enforcement detail" renders inline and stays a direct check. - Verified: a temporary probe held Sonner's insertion timer and released it just before the assertion, as in CI. The old assertion then fails 2/2 with the CI message ("Received element is not visible", buzz-toast-title). The new one passes 2/2. Without the probe, the file passes 136/136. App.profile.test.tsx :261 and :590 (5 s timeouts) - CI: main 40812491297 (job 123245850311). :261 took 5438 ms and :590 5138 ms. The whole file took 65 s there, against 35 s locally, so the runner was about 3.7x slower than this machine. - Cause: a CPU profile shows most of each test is jsdom style resolution. Each Settings open costs about 450 ms here, mostly Base UI's tabbable scan and TabsIndicator measuring through getComputedStyle. The first role/name query inside the dialog after each render costs another 110-190 ms. Two parts of the work were not needed for what the tests assert: - :261 found the Notifications section button with a dialog-wide role query and clicked it. That button calls openSettingsSection (App.tsx), which opens the section through services.navigation.open with the selected community's scope. The "keeps the same unsent conversation" journey at :657 already clicks the section buttons by role. - :590 opened Settings, waited for the profile and closed it before holding the refresh and reopening. A probe showed the session's profile cache already held the viewer's kind-0 profile ("Community name") before that first visit. The first visit filled nothing that the reopen read. - Change: :261 makes the button's navigation itself: it opens Notifications with the scope openSettingsSection builds from services.communities.snapshot(). The first open stays unscoped, as before, so the test still covers the switch from unscoped to scoped Settings with a page sidebar draft behind it. It then still waits for the Notifications heading and closes with the Close Settings button. :590 drops the first visit, opens once with the refresh held, and is renamed "opens profile settings from the session cache while refreshing". All of its assertions are unchanged. No timeout was added or raised. - Verified: three full-file runs each, before and after, under the same local load. :261 went from 1607-1671 ms to 1290-1326 ms (about -20%), and :590 from 1448-1657 ms to 723-744 ms (about -53%). The file passes 23/23. Making ProfileSettings' cachedProfile return null makes the new :590 fail (no Display name while loading), so it still guards the cache seed. The scope costs nothing measurable: alternating full-file runs gave :261 1320-1326 ms with it and 1315-1326 ms without. - Remaining gap: at 3.7x, :261 would still take about 4.8 s, close to the budget. The rest of its time is product rendering of the Settings dialog in jsdom, which a test-only change cannot cut. This change reduces that risk but does not remove it. Left out: InboxPage.test.tsx "reopens archived DMs durably..." - CI: 40916517646 (job 123598080429), 5047 ms for "reply"; 39996979872 (job 120562947760), 5055 ms for "message". - The note blames two full mounts and plans a split at view.unmount() with a shared setup. Timing laps do not support that. The first mount takes 80-240 ms and the remount 20-30 ms. About 65% of the 1.35-1.6 s test runs after the remount, in seven filter-menu changes. A split at the unmount would still leave the second test with nearly all of today's work. Left out: MemberAdministration.test.tsx "reuses mounted presence... (agent: false)" (:1390 at the failing base) - CI: 40725623425 (job 122947973880), "expected true to be false" on listeners.has(target) after typing "Carl". - The note's plan waits for the listener to be removed. The code shows that the member filter and the presence unsubscribe both complete synchronously with the keystroke's commit, so there is nothing to wait for. One mechanism does reproduce the exact signature: the filter matches "carl" against Morgan's npub, and test keys are random. A temporary probe with a key whose npub contains "carl" fails 2/2 at that line. A waitFor would not fix that case, because the row rightly stays. Its odds (about 5e-5 per run) make it an unlikely explanation for this hit, so the cause is still unconfirmed. Checks: tsc --noEmit, and biome check on the two changed files. Vitest ran only App.profile.test.tsx and RelayStaff.test.tsx. Hosted CI has not run on this commit. A green run will not show that these flakes are gone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Addresses the two review comments on PR #1973 (kalvinnchau and wesbillman) on "opens profile settings from the session cache while refreshing" in App.profile.test.tsx. setup() waits for relay readiness, but the viewer's profile cache is filled by ProfileButton's separate background read. ProfileSettings seeds its loaded state from that cache once, on mount, and a later cache fill does not update the mounted component. The first Settings visit that the previous commit removed had been the barrier that proved the read was complete: if the background read lands after Settings mounts, the held refresh leaves it loading and the Display name query fails. Keep the single open, and before installing the refresh spy waitFor the session cache to return the fixture kind-0 event for the viewer, matched by event id. No assertion changed and no timeout was added or raised. Checks: vitest on App.profile.test.tsx (23/23), tsc --noEmit, and biome check on the changed file. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Addresses the two review comments on PR #1973 (kalvinnchau and wesbillman) on "opens profile settings from the session cache while refreshing" in App.profile.test.tsx. setup() waits for relay readiness, but the viewer's profile cache is filled by ProfileButton's separate background read. ProfileSettings seeds its loaded state from that cache once, on mount, and a later cache fill does not update the mounted component. The first Settings visit that the previous commit removed had been the barrier that proved the read was complete: if the background read lands after Settings mounts, the held refresh leaves it loading and the Display name query fails. Keep the single open, and before installing the refresh spy waitFor the session cache to return the fixture kind-0 event for the viewer, matched by event id. No assertion changed and no timeout was added or raised. Checks: vitest on App.profile.test.tsx (23/23), tsc --noEmit, and biome check on the changed file. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
ebb9c61 to
49de06e
Compare
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Carl, an automated reviewer, commenting via Wes's GitHub account.
No blockers at 49de06e. Test-only, one file, CI green.
:590: the newwaitForreadssession.profiles.event(viewer), the same sourcecachedProfileinProfileSettings.tsxseeds from, so it restores the completion barrier the removed first visit provided. If the cache never fills, the test fails loudly rather than passing vacuously.:261: drivingnavigation.openwith the scopeopenSettingsSectionbuilds still covers the unscoped to scoped transition behind the sidebar draft. Real section-button clicks remain at:738/:741. The cost is that this test now mirrorsopenSettingsSectionby hand and won't notice if that function changes.
Non-blocking cleanup before merge:
- The title and body still describe the
RelayStaff.test.tsxfix, but the diff no longer touches that file (#2064 already landed it onmain). The body's ":590 ... cache already held the profile before that first visit" also predates the explicit wait. A squash merge would carry the stale title into history. :630still says "Reopening uses the cached value immediately", but the test no longer reopens.
Summary
Fixes two Vitest flakes from the 23–30 Oct CI flake sweep. The
RelayStafftoast check asserted before Sonner had shown the toast. TwoApp.profiletests ran close to the 5 s budget on slow runners. No product code changes, and no timeouts were added or raised. This is one of six independent PRs split from theflakey-testsbranch. Each is cut frommainand can merge in any order.RelayStaff.test.tsx:1903"enforcement error finished while closed reaches reopened detail"toBeVisiblereceives thebuzz-toast-titleelement. The toast was in the DOM but hidden. Sonner inserts a toast withdata-mounted="false", and its CSS sets opacity 0. A passive effect setsdata-mountedat default priority, so the attribute lands in a later commit. The test asserted immediately after awaiting an unrelated button.waitFor(... toBeVisible()), so visibility is still asserted. "Native enforcement detail" renders inline and stays a direct check.buzz-toast-title), and the new one passes 2/2. Without the probe, the file passes 136/136.App.profile.test.tsx:261and:590(5 s timeouts):261took 5438 ms and:590took 5138 ms. The whole file took 65 s there, against 35 s locally, so that runner was about 3.7× slower than this machine.TabsIndicatormeasuring throughgetComputedStyle. Two pieces of that work were not needed for what the tests assert::261found the Notifications section button with a dialog-wide role query and clicked it. That button callsopenSettingsSection(App.tsx), which opens the section throughservices.navigation.openwith the selected community's scope. The "keeps the same unsent conversation" journey at:657already clicks the section buttons by role.:590opened Settings, waited for the profile, and closed it before holding the refresh and reopening. A probe showed that the session's profile cache already held the viewer's kind-0 profile before that first visit, so the first visit filled nothing the reopen read.:261performs the button's navigation itself. It opens Notifications with the scope thatopenSettingsSectionbuilds fromservices.communities.snapshot(). The first open stays unscoped, so the test still covers the switch from unscoped to scoped Settings with a page sidebar draft behind it, and it still waits for the Notifications heading and closes with the Close Settings button.:590drops the first visit, opens once with the refresh held, and is renamed "opens profile settings from the session cache while refreshing". All of its assertions are unchanged.:261went from 1607–1671 ms to 1290–1326 ms (about −20%), and:590from 1448–1657 ms to 723–744 ms (about −53%). The file passes 23/23. IfProfileSettings'cachedProfileis made to return null, the new:590fails (no Display name while loading), so it still guards the cache seed.:261would still take about 4.8 s, close to the budget. The rest of its time is jsdom rendering of the product's Settings dialog, which a test-only change cannot cut. This PR reduces that risk but does not remove it.App.profile.test.tsx, but none changes:261or:590. fix(composer): synchronize effort picker entrance #1730 adds more work to:590.Left out (causes not confirmed)
InboxPage.test.tsx"reopens archived DMs durably…" (40916517646, 39996979872): the planned split atview.unmount()would not help. Timing laps show that about 65% of the test runs after the remount, in seven filter-menu changes.MemberAdministration.test.tsx"reuses mounted presence… (agent: false)" (40725623425): the filter and the presence unsubscribe both complete synchronously with the keystroke's commit, so there is nothing to wait for. A random-key npub containing "carl" reproduces the signature, but at about 5e-5 odds per run, so the cause is still unconfirmed.Verification scope
tsc --noEmitandbiome checkran on the two changed files. Vitest ran onlyApp.profile.test.tsxandRelayStaff.test.tsx.Remaining gaps
🤖 Generated with Claude Code