Skip to content

fix(useExpansibleController): add dispose #481

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(useExpansibleController): add dispose #481

wants to merge 1 commit into from
+36 −2

Conversation

@Reprevise
Copy link
Contributor

@Reprevise Reprevise commented Nov 4, 2025 •
edited by coderabbitai bot
Loading

The useExpansibleController hook wasn't disposing the ExpansibleController.

Summary by CodeRabbit

  • Bug Fixes

    • Improved resource cleanup and disposal handling for expansion tile controllers to prevent memory leaks.

  • Tests

    • Added test coverage for controller disposal behavior to ensure proper lifecycle management.

@coderabbitai
Copy link

coderabbitai bot commented Nov 4, 2025 •
edited
Loading

Walkthrough

The PR refactors the _ExpansibleControllerHookState class by renaming the internal controller field to _controller, updating the build method to return the private field, and adding a dispose() override to properly clean up the controller. A new test verifies disposal behavior by confirming that accessing a disposed controller throws an error.

Changes

Cohort / File(s) Summary
Expansion Tile Controller Refactoring
packages/flutter_hooks/lib/src/expansion_tile_controller.dart		 Renamed internal field from controller to _controller for clarity, updated build() to return the private field, and added dispose() override to call _controller.dispose() before calling super.dispose() for proper lifecycle management.
Disposal Behavior Test
packages/flutter_hooks/test/use_expansible_controller_test.dart		 Added widget test that verifies ExpansibleController disposal by mounting a widget with useExpansibleController, replacing it with a SizedBox, and asserting that adding a listener to the disposed controller throws a FlutterError.

Sequence Diagram(s)

sequenceDiagram
    participant Widget as Widget Tree
    participant Hook as useExpansibleController Hook
    participant State as _ExpansibleControllerHookState
    participant Controller as ExpansibleController

    Widget->>Hook: Create/Initialize
    Hook->>State: Create state
    State->>Controller: Create controller
    State->>Widget: Return controller via build()
    
    Note over Widget,Controller: Hook Active
    Widget->>Controller: Use (e.g., add listener)
    
    Note over Widget,Controller: Widget Unmounted/Replaced
    Widget->>State: Cleanup triggered
    State->>State: dispose() called
    State->>Controller: dispose()
    Controller->>Controller: Mark as disposed
    State->>State: super.dispose()
    
    Note over Controller: Any further access throws FlutterError
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • The changes follow a straightforward resource management pattern with field renaming and lifecycle cleanup
  • The dispose implementation is standard and calls the appropriate parent method
  • Test case is clear and directly validates the expected error behavior
  • Key area to verify: Ensure the dispose call happens at the correct lifecycle point and that super.dispose() is called after controller cleanup

Possibly related PRs

  • useExpansibleController #473: Modifies the same expansion_tile_controller hook implementation with renaming and hook lifecycle updates that align with these disposal and field naming changes.

Poem

🐰 A controller's journey, from birth to rest,
With _controller renamed—now more finessed!
Dispose it clean when the widget's gone,
Throw errors for those who linger on.
Tests ensure our cleanup's strong! ✨

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix(useExpansibleController): add dispose' accurately describes the main change—adding disposal to the useExpansibleController hook, which matches the core objective of the PR.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 352aeb3 and 47ac2c2.

📒 Files selected for processing (2)
  • packages/flutter_hooks/lib/src/expansion_tile_controller.dart (1 hunks)
  • packages/flutter_hooks/test/use_expansible_controller_test.dart (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-02-19T13:45:17.310Z 
Learnt from: benthillerkus
Repo: rrousselGit/flutter_hooks PR: 460
File: packages/flutter_hooks/lib/src/tree_sliver_controller.dart:19-28
Timestamp: 2025-02-19T13:45:17.310Z
Learning: TreeSliverController in Flutter does not have or require a dispose method. Its lifecycle is managed automatically by Flutter's widget tree. The controller only provides methods for tree manipulation (collapseAll, collapseNode, expandAll, expandNode, toggleNode, getActiveIndexFor, getNodeFor) and doesn't hold resources requiring manual cleanup.

Applied to files:

  • packages/flutter_hooks/lib/src/expansion_tile_controller.dart
  • packages/flutter_hooks/test/use_expansible_controller_test.dart
📚 Learning: 2025-02-19T13:45:17.310Z 
Learnt from: benthillerkus
Repo: rrousselGit/flutter_hooks PR: 460
File: packages/flutter_hooks/lib/src/tree_sliver_controller.dart:19-28
Timestamp: 2025-02-19T13:45:17.310Z
Learning: TreeSliverController in Flutter does not have or require a dispose method. It can be safely used without explicit cleanup.

Applied to files:

  • packages/flutter_hooks/lib/src/expansion_tile_controller.dart
  • packages/flutter_hooks/test/use_expansible_controller_test.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: flutter (flutter_hooks, master)
  • GitHub Check: flutter (flutter_hooks, stable)
🔇 Additional comments (4)
packages/flutter_hooks/lib/src/expansion_tile_controller.dart (3)

30-30: LGTM! Private field follows best practices.

Making the controller field private ensures proper encapsulation and prevents external access to the internal state.

36-36: LGTM! Build method correctly returns the private field.

The build method maintains the same public API while using the renamed private field.

38-42: LGTM! Disposal implementation is correct.

The dispose method properly cleans up the ExpansibleController (which extends ChangeNotifier) before calling super.dispose(). This follows Flutter conventions and fulfills the documentation promise at line 3 that the controller "will be disposed automatically."

packages/flutter_hooks/test/use_expansible_controller_test.dart (1)

82-108: LGTM! Test correctly verifies disposal behavior.

The test appropriately validates that the ExpansibleController is disposed when the hook is disposed. Using addListener() to verify disposal is the standard approach for testing ChangeNotifier disposal, as it throws a FlutterError when called on a disposed instance.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@Reprevise