fix(modules): warn on config.modules.* keys that match no registered module - #4529
Conversation
…module
isModuleActive() fails open: any config.modules.{name} entry that isn't
exactly { activated: false } resolves to active. A mis-cased key, a
typo'd module name, or the wrong property (e.g. display instead of
activated) silently leaves the module - and its routes - active with no
signal that the deactivation attempt did nothing.
Adds a dev-mode-only warnUnknownModuleKeys() that scans config.modules
once at router boot and warns per offending key: unknown key (wrong
case/typo) or a known key with no activated property. No-op in
production, no activation-semantics change (isModuleActive is
untouched).
Rejected: flipping the tasks module's shipped default to inactive, and
fail-closed template routes - both change shipped defaults and would
need downstream migration notes.
Closes #4480
…arg build - registeredModuleNames was built only from optionalModules, missing invitations (gated via adminChildModules/accountChildModules) — would have false-positive warned on a legitimately deactivated module. Now unions all four isModuleActive-gated registries. - replace the manual hasWarned flag with lodash-es once() (already a repo dependency). - pass registeredModuleNames as a thunk so the list is only built on the (single) call that actually runs, not on every getRouter() call. - add a regression test asserting invitations is present in the names passed to warnUnknownModuleKeys.
…mistake
Address Phase-0 review findings on the config.modules.* warn:
- config.modules.{name} is dual-purpose — useCoreStore.refreshNav reads
config.modules[routeName].display to hide a nav item without touching
activation. Both warning messages now name that pattern explicitly so
intentional display-only usage isn't misread as broken config.
- activated is always inert on a CORE module (isModuleActive hardcodes
true regardless of config) — skip the "no activated property" check
entirely for core module keys instead of telling the developer to add
a property that would do nothing.
- document the nonRoutedModuleNames allowlist as intentionally narrow
(stack-owned, not downstream-extensible) rather than a silent gap.
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
WalkthroughThe change adds development-time validation for module configuration keys. The app router supplies registered names from route-gated and configuration-only modules. Tests cover warning conditions, production suppression, scan deduplication, core modules, and router integration. ChangesModule key validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AppRouter
participant ModuleWarningHelper
participant ModuleConfig
participant Console
AppRouter->>ModuleWarningHelper: pass registered route and analytics module names
ModuleWarningHelper->>ModuleConfig: inspect configured module keys
ModuleWarningHelper->>Console: warn for unknown or malformed keys in development
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4529 +/- ##
=======================================
Coverage 99.57% 99.57%
=======================================
Files 36 36
Lines 1411 1427 +16
Branches 439 448 +9
=======================================
+ Hits 1405 1421 +16
Misses 6 6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/modules/app/app.router.js`:
- Around line 131-139: Remove the hard-coded 'analytics' entry from
nonRoutedModuleNames in the core router. Add a registry or configuration seam
that optional modules use to register non-routed configuration names, and have
the router consume that registered set while preserving the existing dev-mode
unknown-module validation.
- Around line 143-149: Document each changed arrow function with a one-line
JSDoc description plus required `@param` and `@returns` tags: add the lazy
module-name provider documentation in src/modules/app/app.router.js lines
143-149, and document the mock delegation functions in
src/modules/app/tests/app.router.unit.tests.js lines 12-15, 73-76, and 641-644.
Describe each function’s inputs and returned iterable or delegated result.
In `@src/modules/app/tests/app.router.unit.tests.js`:
- Around line 606-614: Add an expectation for the configuration-only module name
`analytics` in the module-name assertions within the relevant router unit test,
alongside the existing optional module checks. Keep the regression guard
covering the child-module registry entries unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b6635c96-63d0-4f0e-8109-bd949c86c177
📒 Files selected for processing (4)
src/lib/helpers/modules.jssrc/lib/helpers/tests/modules.unit.tests.jssrc/modules/app/app.router.jssrc/modules/app/tests/app.router.unit.tests.js
…sting it CodeRabbit flagged app.router.js (core module) hardcoding the optional module name 'analytics' in a nonRoutedModuleNames allowlist, violating the "no optional module names in core module code" rule (CLAUDE.md). config.modules.analytics.activated was never read anywhere in the codebase (confirmed repo-wide) — vestigial config predating this PR. Removing it from both default config files eliminates the false- positive source at the root instead of special-casing it, so core router code no longer references any optional module name at all. Also names + documents the lazy registeredModuleNames() provider (previously an anonymous inline arrow) per CodeRabbit review.
…-hide intent Adversarial review before merge found the unknown-key warn treated config.modules.* as a single module-name namespace, but useCoreStore.refreshNav separately keys the same object by Vue Router ROUTE name (PascalCase, e.g. 'Tasks') for nav-hiding — a legitimate, unrelated pattern. Warning "typo" on every real display-only override would have been a false positive on adoption. Now discriminates by intent: - entry has "activated" → activation intent → must match a registered MODULE name, else warn (still catches the original #4480 bug: a mis-cased/typo'd module name with activated:false). - entry has no "activated" → nav-hide intent → must match a mounted ROUTE name (the same list refreshNav consults, threaded from app.router.js's getRouter()) OR a module name (coincidental-match case), else warn. Also softens both messages to cover dead leftover config, not just typos (downstream repos may still carry the vestigial `analytics` key this PR removed stack-side), and updates MIGRATIONS.md.
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 23 minutes. |
1 participant
Summary
isModuleActive()fails open — anyconfig.modules.{name}entry that isn't exactly{ activated: false }resolves to active. Adds a dev-mode-onlywarnUnknownModuleKeys()that scansconfig.modulesonce at router boot and warns per offending key: a key that matches no registered module (wrong case / typo), or a key that matches a module but has noactivatedproperty (e.g.displayset instead). No-op in production;isModuleActiveitself is unchanged.Chosen shape (a) only — dev-mode warn. Rejected: (b) flipping the
tasksreference module's shipped default to inactive, and (c) fail-closed template routes — both change shipped defaults and would need downstream migration notes; no activation-semantics change was in scope.Scope
src/lib/helpers/modules.js(newwarnUnknownModuleKeys),src/modules/app/app.router.js(one call site ingetRouter())none—isModuleActivebehavior is byte-identical; this only adds a diagnosticlowValidation
npm run lintnpm run test:unit(150 files / 2547 tests green)npm run builddisplaywithoutactivated), typo'd name, correctactivated: false, production no-op, once-per-load, and the core-module exemption (activatedis always inert on a core module, so it's skipped rather than mis-guided)Guardrails check
Notes for reviewers
console.warn, no-op in production, no change to route mounting or auth.config.modules.{name}is a dual-purpose namespace (also read byuseCoreStore.refreshNavfor a per-route nav-display override) — both warning messages name that pattern explicitly so intentionaldisplay-only usage isn't misread as broken config, rather than trying to fully disambiguate it.Summary by CodeRabbit