Skip to content

fix(modular): fix CoordinatorModular edgecase cascading dispose and prevent duplicate definitions.#91

Merged
definev merged 3 commits into
mainfrom
fix/route-module-missing-edge-case
Mar 2, 2026
Merged

fix(modular): fix CoordinatorModular edgecase cascading dispose and prevent duplicate definitions.#91
definev merged 3 commits into
mainfrom
fix/route-module-missing-edge-case

Conversation

@definev
Copy link
Copy Markdown
Owner

@definev definev commented Mar 2, 2026

No description provided.

@vercel
Copy link
Copy Markdown

vercel Bot commented Mar 2, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
zenrouter Ready Ready Preview, Comment Mar 2, 2026 3:52am

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Mar 2, 2026

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (51a65d8) to head (9585752).
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #91   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           16        16           
  Lines          610       610           
=========================================
  Hits           610       610           
Flag Coverage Δ
zenrouter 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR targets nested CoordinatorModular edge cases in zenrouter_core, aiming to (1) avoid double execution of module setup when coordinators are nested as modules, (2) enable retrieving nested modules via getModule, and (3) cascade disposal to child coordinators.

Changes:

  • Bump zenrouter_core version from 2.0.0 to 2.0.1.
  • Add recursive module lookup via _allModules and update getModule to support nested modules.
  • Add disposal cascading + adjust layout/converter delegation to avoid double execution for nested coordinator-modules, with new/updated tests covering these cases.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

File Description
packages/zenrouter_core/pubspec.yaml Patch version bump for the core package.
packages/zenrouter_core/lib/src/coordinator/modular.dart Implements recursive module registry, cascaded dispose, and avoids double-calling define hooks for nested coordinator-modules.
packages/zenrouter/test/mixin/layout_test.dart Adds assertions ensuring CoordinatorModular.dispose() cascades to child coordinators.
packages/zenrouter/test/coordinator/nested_modular_test.dart New regression test ensuring nested modular coordinators don’t double-include paths or re-run define hooks.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +86 to +91
for (final module in _modules.values.whereType<CoordinatorCore>()) {
module.dispose();
}
_modules.clear();
_allModules.clear();
super.dispose();
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In dispose(), _modules is cleared before calling super.dispose(). Since CoordinatorModular.paths depends on _modules, clearing it first means CoordinatorCore.dispose() will no longer see module paths and therefore won’t remove listeners / dispose those paths. Fix by calling super.dispose() before clearing _modules/_allModules, or by snapshotting the current paths/module paths before clearing and disposing them explicitly.

Suggested change
for (final module in _modules.values.whereType<CoordinatorCore>()) {
module.dispose();
}
_modules.clear();
_allModules.clear();
super.dispose();
// Ensure CoordinatorCore.dispose() can still see all module paths via `paths`.
super.dispose();
// Then dispose any modules that are also CoordinatorCore instances.
for (final module in _modules.values.whereType<CoordinatorCore>()) {
module.dispose();
}
// Finally, clear module registries to release references.
_modules.clear();
_allModules.clear();

Copilot uses AI. Check for mistakes.
@definev definev merged commit 20ac420 into main Mar 2, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants