Repository navigation
fix(DropdownMenu): a failed sheet preload is handled and retried, never an unhandled rejection - #8478
Conversation
…er an unhandled rejection A sheet menu preloads its action list at rest with a fire-and-forget import. When that import failed (a dropped connection) the rejection went unhandled, an error in the console, and the rejected promise stayed cached, so every later open failed too. In tests, the SSR file could end while the preload was still loading: the environment tore down mid-import, and the run failed on unhandled errors with every test passing. The preload now handles its own failure, and a failed load is not kept, so the next preload or open loads the list again. The SSR test waits for the sheet modules before it ends. A unit test fails a preload once and checks there is no unhandled rejection and that the next call loads again.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix itself is sound, but please correct the changeset before merging. Handling the preload's rejection and dropping the failed load both work, and the new unit test fails without either part. The changeset (and the matching comments in lazyMenuBottomSheet.ts) promises that the next open loads the actions again, yet Chromium 149 and Firefox 151 do not fetch a module again once its native import() has failed, so in builds that load the action list as a native ES module chunk the open still fails until a reload. Please narrow the changeset to what holds everywhere (no unhandled rejection, and the failed load is no longer kept), or make the open path survive a failed import.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [fix] A bottom-sheet DropdownMenu whose sheet preload fails (a dropped connection) no longer reports an unhandled promise rejection, and its next open loads the sheet's actions again instead of failing until a reload. (#8478) |
There was a problem hiding this comment.
This promises that the next open loads the actions again, but clearing actionListLoad only lets the next import() call happen: Chromium 149 and Firefox 151 reject a second import() of a module URL whose fetch failed without fetching it again, so in a build that loads the list as a native ES module chunk the open still fails until a reload. Suggest stating what holds everywhere, for example "no longer reports an unhandled promise rejection, and no longer keeps the failed load, so a module loader that retries loads the actions on the next open". The comments in lazyMenuBottomSheet.ts at lines 18-19, 59-60 and 69-71 say the same.
There was a problem hiding this comment.
Done in 3962530: the changeset and the three comments now promise only what holds everywhere, no unhandled rejection and no kept failure, so a module loader that retries loads the actions on the next open; the unit test's title says it asks the loader again.
…in; it does not promise a fresh load A browser does not fetch a native module again once its import failed, so clearing the failed load only lets the next open ask the loader again: a loader that retries then loads the actions. The changeset, the three comments and the unit test's title now say that.
PR Analysis Report
Component Size
Sizes are gzip. Import counts the JavaScript the import loads eagerly; CSS counts the package stylesheet rules referenced by the modules it can load from its own package, lazily loaded ones included. Bundle Size SummaryGzip figures measure each package's entry file, not the code an import pulls in.
Accessibility AuditStatus: No accessibility violations detected. Visual RegressionStatus: No visual change across 4 compared shot(s). Generated by PR Enrichment workflow | View full report |
There was a problem hiding this comment.
This is ready to merge: the preload's rejection is now handled, the failed load is dropped so a module loader that retries recovers on the first open, and the changeset now claims exactly that. The new unit test fails on main and with either half of the fix reverted, and DropdownMenu.ssr.test.tsx no longer fails on teardown when run alone. One optional wording nit on the header comment of lazyMenuBottomSheet.ts: after a failed open, React's lazy() keeps that failure for menus already mounted, so "the next preload or the first open" would be exact.
[Automated review]
| * The action list preloads at rest; a failed load is not kept, so the next | ||
| * preload or open asks the module loader again (a loader that retries then |
There was a problem hiding this comment.
Non-blocking: this holds after a failed preload, but after a failed open React's lazy() keeps that failure, so another bottom-sheet menu that is already mounted opens straight into it without asking the loader again (as on the base). "the next preload or the first open asks the module loader again" would match the comment on preloadMenuBottomSheetActionList below.
There was a problem hiding this comment.
This looks ready to merge: the preload's rejection is now handled and the failed load is dropped, so a module loader that retries recovers on the first open, and the new unit test fails on main and with either half of the fix reverted. The changeset claims exactly that, and nothing for native ES module imports, which Chromium and Firefox do not fetch again after a failure. One optional cleanup: the description still lists a DropdownMenu.ssr.test.tsx change, but after merging main that file is main's own version (#8492 already waits for the sheet modules), so that bullet and its run counts no longer describe this diff.
[Automated review]
cixzhang
added
the
ci:run
…t, not an empty catch (#9069) Follows #8478. ## Maintainer impact #8478 stopped a failed preload of a bottom-sheet DropdownMenu's action list from raising an unhandled rejection, using `loadActionList().catch(() => {})`. An empty handler hides every error in that chain, not only the expected failed load, so a real defect there would go unnoticed. What led to it: a sweep of recent changes for error handlers that catch and default. ## Intended invariant A sheet preload's failure is its result, never a rejection and never swallowed: `preloadMenuBottomSheetActionList` resolves `{isLoaded: true}`, or `{isLoaded: false, error}` with the loader's error. As before, a failed load is not kept, so the next open loads the list. ## Change - `lazyMenuBottomSheet.ts`: the preload returns that result (an `ActionListPreload` type, internal to DropdownMenu). - `DropdownMenu.tsx`: its effect starts the preload with `void`; after a failure, the first open loads the list. - `lazyMenuBottomSheet.test.ts`: checks the result, no unhandled rejection, and a retry on the next call. - A `[chore]` changeset: callers see no change. Options considered: - Keep the empty handler: it hides errors other than the expected one. - Log the failure: a console line is no contract, and the first open already surfaces a real failure through `lazy()`. - Return the failure as the result (chosen): the expected failure becomes part of the contract, and nothing is dropped. Prior art: no open pull request touches `preloadMenuBottomSheetActionList`, `lazyMenuBottomSheet.ts` or an `ActionListPreload` type (searched by those names and "sheet preload"). ## Evidence - On main fd0aba8, the updated `lazyMenuBottomSheet.test.ts` fails; it passes here. - The rest of DropdownMenu's tests pass unchanged, within `pnpm test:ui` (16,454 passed). ## Scope - [x] No intended public API, product behavior, visual direction, or policy change. - [x] Product changes discovered during the work were removed or split. - [x] Public text and artifacts contain no internal Meta context. ## Testing `pnpm exec vitest run packages/core/src/DropdownMenu/lazyMenuBottomSheet.test.ts`: fails on main, passes here. Local gates at 45046fe: build, typecheck, lint, `pnpm test:ui` (16,454), `pnpm test:node` (all but #8520's two known `cache.test.mjs` failures, and two of main's load-sensitive cases, `ci-scope-pr-diff.test.mjs` and `component-size.test.mjs`: 30 s timeouts at box load 70–85; they pass alone on main fd0aba8 and this head), CLI (47), Storybook, the DropdownMenu and Menu accessibility specs in Chromium (228), Prettier, changeset coverage, and the knowledge, changeset and repo checks.
User impact
A bottom-sheet DropdownMenu preloads its action list at rest. When that load fails (a dropped or flaky connection), the page gets an unhandled promise rejection in the console. The rejected load also stays cached, so every later open of the menu fails to show its rows until a reload. In this repository's CI, the same unhandled rejection fails the UI test shard that runs
DropdownMenu.ssr.test.tsxwhile every test passes, as on #8353, #8229 and #8211.Problem and solution fit
preloadMenuBottomSheetActionListstarts the load withvoid loadActionList(): nothing handles a rejection, andactionListLoad ??=keeps the rejected promise.The fix handles the preload's failure and drops the failed load, so the next preload or open asks the loader again. The SSR test waits for the sheet modules before it ends.
Expected behavior and authority
The sheet loads on demand (#7709) and preloads at rest so the first open shows its actions at once (#8392). A preload is an optimization: its failure surfaces no error and blocks no retry.
Smallest restoration
loadActionListclears its cached load when the import rejects.preloadMenuBottomSheetActionListhandles the rejection.DropdownMenu.ssr.test.tsxawaits the two sheet modules after unmounting.No API, rendering or timing change.
Evidence
DropdownMenu.ssr.test.tsxrun alone fails 5 of 6 runs with threeEnvironmentTeardownErrors each, and the new unit test records the unhandled rejection.lazyMenuBottomSheet.test.tsfails the list's import once, then checks there is no unhandled rejection and that the next call imports it again. Keeping the rejected load cached fails it ("expected 1 to be 2").Scope
Testing
node scripts/release/changeset-coverage.mjs pr --base e9b3f3edacfails at 4d8e178 and passes here ([fix],@astryxdesign/core);pnpm check:changesets: valid.pnpm exec vitest run --project ui packages/core/src/DropdownMenu/lazyMenuBottomSheet.test.ts: fails on main, passes here.pnpm exec vitest run --project ui packages/core/src/DropdownMenu/DropdownMenu.ssr.test.tsx, alone, 8 times: 8 of 8 (main: 1 of 6).pnpm exec vitest run packages/core/src/DropdownMenu: 10 files, 312 tests.tsc -p packages/core/tsconfig.json --noEmit, eslint and prettier on the changed files: clean.