fix: catch AbortErrors in storybook tests and patch @reatom/core#17
Merged
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (39)
WalkthroughThis PR extends Storybook test infrastructure with abort-error tracking, adds comprehensive integration story coverage for Articles/Chat/Connections flows, implements route-aware optimizations, removes test utilities, and consolidates test configuration across multiple foundation layers. ChangesTesting Infrastructure and Integration Coverage
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
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 reportNo GitHub PR/MR findings. 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.
Comment on lines
+28
to
+30
| // 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
3 times, most recently
from
6598873 to
878a1f4
Compare
Uses the pkg.pr.new preview build from reatom/reatom#1294 which fixes the root cause: onReject no longer fires for abort rejections (aborts route to onSettle instead). Drops the local @reatom/core patch. AbortError guard (.storybook/abortErrorGuard.ts) hooks every `.onReject` action via addGlobalExtension + withCallHook. The global beforeEach drains collected errors after each story test and throws on unexpected AbortErrors. assertNoRouteLoaderAbortErrors proves the fix: navigation teardowns must NOT surface loader AbortErrors on onReject. This is a positive regression test — fails loudly against unpatched @reatom/core. Other changes in this branch: - Split Connections/Chat/Articles stories by route mode (detail, direct-url, list-request, list, navigation) to isolate abort attribution per story - Route loader contract tests (reatomRouteContracts.test.ts) - connectLogger: on by default in dev, off in test env - conversationUnreadCountAtom: removed redundant withConnectHook causing double-fetch - Storybook URL setup: auth before urlAtom.go() to prevent loader re-evaluations - Timer: aria-label on custom duration input - Vitest detection: __vitest_worker__ instead of broken import.meta.env.VITEST - Actor: use tryTo instead of hopeThat for negative filter check (fixes soft-error leak); remove unused blur method 330/330 tests pass. Fallow health/dead-code/dupes clean. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Guria
force-pushed
the
fix/abort-error-guard-and-reatom-patch
branch
from
dfb065b to
be5311b
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
Tests
Chores