fix: loaders not being collected from async components for repeated n… #714
Conversation
WalkthroughAdds exported symbol LOADER_SET_PROMISES_KEY, augments Vue Router RouteMeta to store per-record loader Promise arrays, updates the navigation guard to collect/await/clear those promises across repeated navigations and error paths, and adds tests covering lazy-loaded route navigation behavior. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the ✨ Finishing Touches
🧪 Generate unit tests
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 |
minht11
force-pushed
the
fix/repeated-routes-loaders
branch
from
4557771 to
aa03f3a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/data-loaders/navigation-guard.ts (1)
160-171: Bug: clearing the wrong meta + not cleared on error
- You set
to.meta[LOADER_SET_PROMISES_KEY] = undefinedinstead of clearingrecord.meta[...].- If module import rejects, promises remain and will be re-awaited on every nav.
Fix by clearing per-record promises and doing it in a
finallyso it also runs on errors:- return Promise.all(lazyLoadingPromises).then(() => { - // group all the loaders in a single set - for (const record of to.matched) { - // merge the whole set of loaders - for (const loader of record.meta[LOADER_SET_KEY]!) { - to.meta[LOADER_SET_KEY]!.add(loader) - to.meta[LOADER_SET_PROMISES_KEY] = undefined - } - } - // we return nothing to remove the value to allow the navigation - // same as return true - }) + return Promise.all(lazyLoadingPromises) + .then(() => { + // group all the loaders in a single set + for (const record of to.matched) { + for (const loader of record.meta[LOADER_SET_KEY]!) { + to.meta[LOADER_SET_KEY]!.add(loader) + } + } + // we return nothing to remove the value to allow the navigation + // same as return true + }) + .finally(() => { + // clear per-record in-flight promises regardless of success/failure + for (const record of to.matched) { + record.meta[LOADER_SET_PROMISES_KEY] = undefined + } + })
🧹 Nitpick comments (1)
src/data-loaders/symbols.ts (1)
7-12: Nit: debug label consistencyConsider pluralizing/aligning the debug string:
Symbol('loaderSetPromises')orSymbol('loadersPromises')to better reflect the array of promises and matchLOADER_SET_KEY.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/data-loaders/meta-extensions.ts(2 hunks)src/data-loaders/navigation-guard.spec.ts(1 hunks)src/data-loaders/navigation-guard.ts(3 hunks)src/data-loaders/symbols.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/data-loaders/navigation-guard.spec.ts (3)
playground/src/router.ts (1)
router(5-8)src/data-loaders/symbols.ts (1)
LOADER_SET_KEY(5-5)tests/data-loaders/loaders.ts (2)
useDataOne(7-7)useDataTwo(8-8)
src/data-loaders/meta-extensions.ts (1)
src/data-loaders/symbols.ts (1)
LOADER_SET_PROMISES_KEY(11-11)
src/data-loaders/navigation-guard.ts (1)
src/data-loaders/symbols.ts (1)
LOADER_SET_PROMISES_KEY(11-11)
🔇 Additional comments (6)
src/data-loaders/meta-extensions.ts (2)
11-12: Correct: type-only importType-only import of the symbol for use in interface computed keys is right.
74-79: RouteMeta augmentation looks goodKeying by the symbol with
Promise<void>[]matches the producer sites.src/data-loaders/navigation-guard.ts (3)
17-18: Import is correctRe-export path via meta-extensions is fine.
149-151: Good: persist per-record in-flight promisesStoring promises on
record.meta[...]enables awaiting across repeated navigations.
153-157: Good: await prior in-flight loads on repeat navPushing stored promises ensures loaders are collected when async modules are still loading.
src/data-loaders/navigation-guard.spec.ts (1)
184-196: Make async component loading deterministic
Relying onawait Promise.resolve()can let the dynamic import finish before the second navigation, making the test flaky. Use a deferred promise to keep the import “in-flight” across both navigations:--- src/data-loaders/navigation-guard.spec.ts @@ - router.addRoute({ - component: () => - import('../../tests/data-loaders/ComponentWithLoader.vue'), - }) + let resolveImport!: (mod: unknown) => void + const pendingImport = new Promise<unknown>(r => (resolveImport = r)) + router.addRoute({ + component: () => pendingImport, + }) @@ - void router.push('/fetch') - // simulate repeated navigation while the async component is loading - await Promise.resolve() - await router.push('/fetch') + const nav1 = router.push('/fetch') + const nav2 = router.push('/fetch') + // now resolve the import and let both navigations finish + resolveImport(await import('../../tests/data-loaders/ComponentWithLoader.vue')) + await Promise.all([nav1, nav2])
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/data-loaders/navigation-guard.ts (1)
154-154: Remove dev log or gate it behind NODE_ENV.Avoid console noise in production.
- console.log('REUSE', record.path) + // if needed during debugging: + // if (process.env.NODE_ENV !== 'production') { + // // eslint-disable-next-line no-console + // console.debug('[data-loaders] reusing pending loader promises for', record.path) + // }src/data-loaders/navigation-guard.spec.ts (1)
28-29: Confirm API surface for LOADER_SET_PROMISES_KEY.Importing this from the public entry makes it part of the public API. If intended only for tests, consider marking it
@internal(and excluding viastripInternal) or importing from an internal path.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/data-loaders/navigation-guard.spec.ts(2 hunks)src/data-loaders/navigation-guard.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/data-loaders/navigation-guard.spec.ts (2)
src/data-loaders/symbols.ts (2)
LOADER_SET_KEY(5-5)LOADER_SET_PROMISES_KEY(11-11)tests/data-loaders/loaders.ts (2)
useDataOne(7-7)useDataTwo(8-8)
src/data-loaders/navigation-guard.ts (1)
src/data-loaders/symbols.ts (1)
LOADER_SET_PROMISES_KEY(11-11)
🔇 Additional comments (2)
src/data-loaders/navigation-guard.ts (1)
17-17: Import looks correct.Symbol is used consistently with meta augmentation.
src/data-loaders/navigation-guard.spec.ts (1)
182-205: Test covers the regression well.The repeated navigation while the async component is in-flight is exercised and final assertions are precise, including cleanup of LOADER_SET_PROMISES_KEY.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/data-loaders/navigation-guard.ts (1)
160-172: Deduplicate cleanup with finally; ensure symmetric teardown.Clear LOADER_SET_PROMISES_KEY in a finally block to avoid duplication and guarantee cleanup even if the merge step throws.
- return Promise.all(lazyLoadingPromises) - .then(() => { - // group all the loaders in a single set - for (const record of to.matched) { - // merge the whole set of loaders - for (const loader of record.meta[LOADER_SET_KEY]!) { - to.meta[LOADER_SET_KEY]!.add(loader) - } - record.meta[LOADER_SET_PROMISES_KEY] = undefined - } - // we return nothing to remove the value to allow the navigation - // same as return true - }) - .catch((error) => { - // If error happens while collecting loaders, reset them - // so on next navigation we can try again - for (const record of to.matched) { - record.meta[LOADER_SET_KEY] = undefined - record.meta[LOADER_SET_PROMISES_KEY] = undefined - } - - throw error - }) + return Promise.all(lazyLoadingPromises) + .then(() => { + // group all the loaders in a single set + for (const record of to.matched) { + for (const loader of record.meta[LOADER_SET_KEY]!) { + to.meta[LOADER_SET_KEY]!.add(loader) + } + } + // return nothing to allow the navigation + }) + .catch((error) => { + // If error happens while collecting loaders, reset them + for (const record of to.matched) { + record.meta[LOADER_SET_KEY] = undefined + } + throw error + }) + .finally(() => { + // always clear in-flight bookkeeping + for (const record of to.matched) { + record.meta[LOADER_SET_PROMISES_KEY] = undefined + } + })Also applies to: 173-182
🧹 Nitpick comments (2)
src/data-loaders/navigation-guard.spec.ts (2)
182-205: Make the “repeated navigation while loading” test deterministic.The dynamic import may resolve before the second push, making the test flaky. Gate the import with a controllable promise so the first navigation is guaranteed in-flight when the second starts.
it('collects all loaders from lazy loaded pages with repeated navigation', async () => { setupApp({ isSSR: false }) const router = getRouter() - router.addRoute({ + let release!: () => void + const gate = new Promise<void>((r) => (release = r)) + router.addRoute({ name: '_test', path: '/fetch', - component: () => - import('../../tests/data-loaders/ComponentWithLoader.vue'), + component: async () => { + await gate + return import('../../tests/data-loaders/ComponentWithLoader.vue') + }, }) - void router.push('/fetch') - // simulate repeated navigation while the async component is loading - await Promise.resolve() - await router.push('/fetch') + void router.push('/fetch') + // repeated navigation while the async component is still loading + const p = router.push('/fetch') + release() + await p
206-236: Strengthen failure-path assertions.Optionally assert that the record metadata was reset after the failed first navigation before retrying.
-const firstNavPromise = router.push('/fetch') +const firstNavPromise = router.push('/fetch') await expect(firstNavPromise).rejects.toThrow(Error) +// verify cleanup after failure +const rec = getRouter().getRoutes().find(r => r.name === '_test')! +expect(rec.meta[LOADER_SET_PROMISES_KEY]).toBeUndefined() +// LOADER_SET_KEY is recomputed later; ensure it's cleared too +expect(rec.meta[LOADER_SET_KEY]).toBeUndefined() + await router.push('/fetch')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/data-loaders/navigation-guard.spec.ts(2 hunks)src/data-loaders/navigation-guard.ts(2 hunks)
🔇 Additional comments (4)
src/data-loaders/navigation-guard.spec.ts (1)
28-29: Importing LOADER_SET_PROMISES_KEY for assertions looks fine.Good to validate the cleanup behavior explicitly.
src/data-loaders/navigation-guard.ts (3)
17-18: New symbol import is correct.Matches its usage below.
149-151: Persist per-record in-flight promises — OK.This enables awaiting async component loaders across repeated navigations.
153-157: Reusing in-flight promises on repeated navigations — OK.Minimal overhead; acceptable for correctness.
…avigations fix LOADER_SET_PROMISES_KEY being not reset
improve tests handle error while collecting loaders
minht11
force-pushed
the
fix/repeated-routes-loaders
branch
from
5c14ca8 to
4e90e60
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/data-loaders/navigation-guard.ts (1)
149-151: Move cleanup to finally block to prevent permanent failures.If async import rejects, the rejected promise remains in
LOADER_SET_PROMISES_KEYand causesPromise.allto keep rejecting on subsequent navigations.- return Promise.all(lazyLoadingPromises) - .then(() => { - // group all the loaders in a single set - for (const record of to.matched) { - // merge the whole set of loaders - for (const loader of record.meta[LOADER_SET_KEY]!) { - to.meta[LOADER_SET_KEY]!.add(loader) - } - record.meta[LOADER_SET_PROMISES_KEY] = undefined - } - // we return nothing to remove the value to allow the navigation - // same as return true - }) + return Promise.all(lazyLoadingPromises) + .then(() => { + // group all the loaders in a single set + for (const record of to.matched) { + // merge the whole set of loaders + for (const loader of record.meta[LOADER_SET_KEY]!) { + to.meta[LOADER_SET_KEY]!.add(loader) + } + } + // we return nothing to remove the value to allow the navigation + // same as return true + }) + .finally(() => { + // always clear in-flight bookkeeping, even on failures + for (const record of to.matched) { + record.meta[LOADER_SET_PROMISES_KEY] = undefined + } + })Also applies to: 153-156, 168-168
🧹 Nitpick comments (1)
src/data-loaders/navigation-guard.ts (1)
173-182: Redundant cleanup in catch block.With cleanup moved to
finally, theLOADER_SET_PROMISES_KEYreset here becomes redundant..catch((error) => { // If error happens while collecting loaders, reset them // so on next navigation we can try again for (const record of to.matched) { record.meta[LOADER_SET_KEY] = undefined - record.meta[LOADER_SET_PROMISES_KEY] = undefined } throw error })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/data-loaders/meta-extensions.ts(2 hunks)src/data-loaders/navigation-guard.spec.ts(2 hunks)src/data-loaders/navigation-guard.ts(2 hunks)src/data-loaders/symbols.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/data-loaders/meta-extensions.ts
- src/data-loaders/symbols.ts
- src/data-loaders/navigation-guard.spec.ts
Closes #567
With repeated navigations on the same time, while async module is still being loaded, subsequent navigations wouldn't have any loaders.
Added test which fails without the fix.
Summary by CodeRabbit