fix: catch AbortErrors in storybook tests and patch @reatom/core#17
fix: catch AbortErrors in storybook tests and patch @reatom/core#17Guria wants to merge 6 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (19)
💤 Files with no reviewable changes (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR enhances test infrastructure and Storybook reliability: it isolates the connect logger from tests, captures and reports abort errors during Storybook tests, adds a pre-navigation callback, gates route loader execution to active routes, hardens mobile test interactions and story metadata, and simplifies an unread-count atom. ChangesTest Infrastructure and Component Reliability
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Fallow audit reportFound 1 finding. Details
Generated by fallow. |
There was a problem hiding this comment.
Pull request overview
This PR reduces noisy AbortError output in Storybook browser tests and improves routing/loader performance by patching @reatom/core to avoid evaluating non-matching route loaders on URL changes.
Changes:
- Add a Storybook test-time guard to collect and fail on unexpected Reatom
AbortErrors, and fix Vitest detection in the browser context. - Patch
@reatom/core@1001.0.0to avoid proactively evaluating loaders for non-matching routes and to avoid forcing loader evaluation inisSomeLoaderPending. - Apply several test/stability/accessibility tweaks (timer input labeling, mobile master-detail navigation waits, disable connect logger in test tasks, simplify unread count atom).
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/setup.ts | Enables connectLogger by default in dev unless explicitly disabled. |
| src/pages/timer/ui/TimerPage.tsx | Adds accessible label for the custom duration input. |
| src/pages/timer/testing.ts | Updates test locator to target the labeled timer input. |
| src/entities/conversation/model/unreadCount.ts | Removes redundant connect hook from unread count async atom. |
| src/app/integration/Connections.stories.tsx | Stabilizes mobile navigation test by waiting for list visibility after back navigation. |
| src/app/integration/Articles.stories.tsx | Stabilizes mobile navigation test by waiting for list visibility after back navigation. |
| .storybook/setupStorybookUrl.ts | Adds a pre-navigation hook so auth/setup runs before urlAtom.go(). |
| .storybook/preview.tsx | Integrates abort error guard + fixes Vitest detection + adds per-story abort error assertion. |
| .storybook/abortErrorGuard.ts | Adds global onReject call hook to collect unexpected abort errors during Storybook tests. |
| patches/@reatom%2Fcore@1001.0.0.patch | Patches Reatom routing internals to avoid non-matching loader evaluation and pending checks. |
| package.json | Registers the @reatom/core patch via patchedDependencies. |
| bun.lock | Records patchedDependencies entry for Bun. |
| .config/mise/conf.d/tasks-test.toml | Disables connect logger in test-related tasks to reduce noisy output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Reatom routing control flow — urlAtom proactively triggers all | ||
| // registered route loaders on every URL change, and nested loaders | ||
| // await their parents which can cause concurrent re-evaluation. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a12c6cf666
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (url !== newUrl) { | ||
| _enqueue(() => { | ||
| - for (const [, routeAtom] of Object.entries(urlAtom.routes)) routeAtom.loader(); | ||
| + for (const [, routeAtom] of Object.entries(urlAtom.routes)) if (routeAtom()) routeAtom.loader(); |
There was a problem hiding this comment.
When navigating away from a route with an in-flight loader, this guard skips calling that route's loader after routeAtom() becomes null. That removes the proactive recomputation that used to enter the loader, see the unmatch state, and trigger the withAbort/reject path; with the new isSomeLoaderPending short-circuit, unmatched loaders are also no longer kept subscribed via pending(). As a result, slow requests for the previous route can continue and fulfill into route.loader.data() after the route is no longer active instead of being aborted on navigation.
Useful? React with 👍 / 👎.
Guria
force-pushed
the
fix/abort-error-guard-and-reatom-patch
branch
2 times, most recently
from
e9c0966 to
6598873
Compare
Add abort error detection guard to storybook test harness that fails tests when non-route-loader AbortErrors are detected. Route loader aborts (unmatch/concurrent) are expected Reatom routing lifecycle and are filtered. Patch @reatom/core to reduce unnecessary route loader evaluations: - urlAtom init/set loops now only trigger loaders for matching routes - isSomeLoaderPending only checks pending state for matching routes These optimizations prevent isSomeLoaderPending (used by GlobalLoader) from forcing ALL route loaders to evaluate, which was causing every non-matching route to produce an unmatch AbortError on every URL change. Other fixes: - Remove redundant withConnectHook from conversationUnreadCountAtom that caused a concurrent abort (double-fetch on first connection) - Set auth before URL navigation in storybook setup to prevent concurrent loader re-evaluations from auth state changes - Fix timer input accessibility (add aria-label) - Fix mobile master-detail tests to wait for list after goBack - Disable connectLogger during test runs (VITE_CONNECT_LOGGER per-task) - Fix broken import.meta.env.VITEST detection in browser tests
Guria
force-pushed
the
fix/abort-error-guard-and-reatom-patch
branch
from
6598873 to
878a1f4
Compare
- split Connections, Chat, and Articles integration stories by route mode - keep Storybook AbortError guard strict by default - add explicit helper for expected matched-route loader teardown aborts - restore isolated multi-navigation coverage for Articles - reduce cross-story/test abort attribution noise without hiding real loader misuse
Problem
Storybook tests produce many
AbortErrormessages in the connect logger output that go undetected. These indicate issues in the routing/async lifecycle that should be tracked.Additionally,
@reatom/corehas a performance issue whereurlAtomproactively evaluates ALL registered route loaders on every URL change — even for non-matching routes. Combined withisSomeLoaderPending(used byGlobalLoader) readingroute.loader.pending()for all routes, this forces every non-matching route loader to evaluate and produce unnecessaryunmatchAbortErrors.Patch:
@reatom/core@1001.0.0Three optimizations in
patches/@reatom%2Fcore@1001.0.0.patch:urlAtominit hook — only triggerrouteAtom.loader()whenrouteAtom()returns truthy (route matches)urlAtomset handler — same match check in the_enqueuecompute loopisSomeLoaderPending— checkroute.match()before readingroute.loader.pending(), preventing forced loader evaluation for non-matching routesThese changes are safe because:
Other fixes
.storybook/abortErrorGuard.ts) —addGlobalExtension+withCallHookon every.onRejectaction to collect AbortErrors during tests. Route loader aborts are filtered (expected lifecycle).conversationUnreadCountAtom— removed redundantwithConnectHookthat caused a concurrent abort (double-fetch on first connection)urlAtom.go()to prevent concurrent loader re-evaluationsaria-label="Custom duration"to timer inputgoBack()connectLoggerin tests — disabled via per-taskVITE_CONNECT_LOGGER=falseimport.meta.env.VITEST(alwaysundefinedin browser context) → use__vitest_worker__Result
Summary by CodeRabbit
New Features
Bug Fixes
Accessibility
Refactor
Tests