refacto: replace hardcoded widget keys with type-safe enums#2915
refacto: replace hardcoded widget keys with type-safe enums#2915
Conversation
9clg6
requested review from
dab246,
hoangdat,
nqhhdev and
tddang-linagora
as code owners
WalkthroughThis pull request centralizes widget key definitions by adding a new lib/presentation/widget_keys/ barrel and five enum-based registries (ChatKeys, DialogKeys, LinkPreviewKeys, NavigationKeys, SettingsKeys) that expose Key and ValueKey getters. It replaces scattered hard-coded Key/ValueKey literals and removed local key constants across many widgets, pages, dialogs, and integration tests with references to these centralized enums. No behavioral or control-flow changes beyond key indirection are introduced. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/widgets/layouts/adaptive_layout/app_adaptive_scaffold.dart (1)
20-20: Route the default scaffold key throughNavigationKeystoo.Line 20 still duplicates
'scaffoldWithNestedNavigation'as a rawValueKey, whilelib/widgets/layouts/adaptive_layout/app_adaptive_scaffold_body_view.dart:32-45already reads the same selector fromNavigationKeys. That leaves two sources of truth for the scaffold key inside the same refactor and makes a future rename easy to miss here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/widgets/layouts/adaptive_layout/app_adaptive_scaffold.dart` at line 20, The constructor in AppAdaptiveScaffold currently uses a hard-coded ValueKey('scaffoldWithNestedNavigation') in the super call; change it to reuse the canonical key from NavigationKeys (the same key used in AppAdaptiveScaffoldBodyView) by replacing the raw ValueKey with NavigationKeys.scaffoldWithNestedNavigation (or the matching identifier in NavigationKeys) so there is a single source of truth for the scaffold key.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/widgets/layouts/adaptive_layout/app_adaptive_scaffold.dart`:
- Line 20: The constructor in AppAdaptiveScaffold currently uses a hard-coded
ValueKey('scaffoldWithNestedNavigation') in the super call; change it to reuse
the canonical key from NavigationKeys (the same key used in
AppAdaptiveScaffoldBodyView) by replacing the raw ValueKey with
NavigationKeys.scaffoldWithNestedNavigation (or the matching identifier in
NavigationKeys) so there is a single source of truth for the scaffold key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4ee703cb-fe48-4ece-8fb5-80a0de99a3a1
📒 Files selected for processing (31)
integration_test/extensions/default_flows.dartintegration_test/robots/home_robot.dartintegration_test/robots/setting/settings_privacy_and_security_robot.dartlib/pages/bootstrap/init_client_dialog.dartlib/pages/bootstrap/tom_bootstrap_dialog.dartlib/pages/chat/chat.dartlib/pages/chat/chat_scroll_view.dartlib/pages/chat/send_file_dialog/send_file_dialog.dartlib/pages/chat_adaptive_scaffold/chat_adaptive_scaffold_builder.dartlib/pages/chat_details/chat_details.dartlib/pages/chat_draft/draft_chat.dartlib/pages/chat_list/chat_list_view.dartlib/pages/chat_profile_info/chat_profile_info_details.dartlib/pages/settings_dashboard/settings/settings_view.dartlib/pages/settings_dashboard/settings_profile/settings_profile_view.dartlib/pages/settings_dashboard/settings_security/settings_security_view.dartlib/presentation/mixins/chat_details_tab_mixin.dartlib/presentation/mixins/media_viewer_app_bar_mixin.dartlib/presentation/widget_keys/chat_keys.dartlib/presentation/widget_keys/dialog_keys.dartlib/presentation/widget_keys/link_preview_keys.dartlib/presentation/widget_keys/navigation_keys.dartlib/presentation/widget_keys/settings_keys.dartlib/presentation/widget_keys/widget_keys.dartlib/utils/dialog/twake_dialog.dartlib/widgets/layouts/adaptive_layout/adaptive_scaffold_appbar_style.dartlib/widgets/layouts/adaptive_layout/app_adaptive_scaffold.dartlib/widgets/layouts/adaptive_layout/app_adaptive_scaffold_body_view.dartlib/widgets/layouts/enum/adaptive_destinations_enum.dartlib/widgets/twake_components/twake_preview_link/twake_link_preview.dartlib/widgets/twake_components/twake_preview_link/twake_link_preview_item.dart
💤 Files with no reviewable changes (1)
- lib/pages/chat_list/chat_list_view.dart
|
This PR has been deployed to https://linagora.github.io/twake-on-matrix/2915 |
hoangdat
left a comment
hoangdat
left a comment
There was a problem hiding this comment.
Did you run one round again of E2E test to ensure nothing break?
No, I only ran a single random test, because it's E2E and it would take me a really long time to run everything. And in reality, I didn't change anything functional, only structural, so there's little chance that it would have an impact on proper functioning. |
9clg6
force-pushed
the
refacto/widget-keys-enums
branch
from
7667851 to
17472d2
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/presentation/widget_keys/dialog_keys.dart (1)
3-8: Add/keep key-focused integration smoke for dialog flows in CI.Since runtime key values are derived from enum names, a member rename can silently break finder-based E2E selectors. A lightweight smoke on bootstrap/init/confirm-dialog paths helps catch that early.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/presentation/widget_keys/dialog_keys.dart` around lines 3 - 8, Add a small CI integration smoke test that asserts the dialog-related enum names are available as runtime keys and that the bootstrap/init/confirm dialog flows can be found by those keys; specifically create a test (e.g., DialogFlowSmokeTest or dialog_keys_smoke_test) which boots the relevant widget tree, then uses finder by Key(ValueKey(DialogKeys.showConfirmAlertDialog.name)), Key(ValueKey(DialogKeys.bootstrapBreakpointMobile.name))/WebAndDesktop, and Key(ValueKey(DialogKeys.initClientBreakpointMobile.name))/WebAndDesktop to locate the widgets, tap the confirm dialog trigger, and verify the dialog appears; this ensures any future renames of DialogKeys (enum DialogKeys and its members showConfirmAlertDialog, bootstrapBreakpointMobile, bootstrapBreakpointWebAndDesktop, initClientBreakpointMobile, initClientBreakpointWebAndDesktop) break CI so they are caught early.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/presentation/widget_keys/dialog_keys.dart`:
- Around line 3-8: Add a small CI integration smoke test that asserts the
dialog-related enum names are available as runtime keys and that the
bootstrap/init/confirm dialog flows can be found by those keys; specifically
create a test (e.g., DialogFlowSmokeTest or dialog_keys_smoke_test) which boots
the relevant widget tree, then uses finder by
Key(ValueKey(DialogKeys.showConfirmAlertDialog.name)),
Key(ValueKey(DialogKeys.bootstrapBreakpointMobile.name))/WebAndDesktop, and
Key(ValueKey(DialogKeys.initClientBreakpointMobile.name))/WebAndDesktop to
locate the widgets, tap the confirm dialog trigger, and verify the dialog
appears; this ensures any future renames of DialogKeys (enum DialogKeys and its
members showConfirmAlertDialog, bootstrapBreakpointMobile,
bootstrapBreakpointWebAndDesktop, initClientBreakpointMobile,
initClientBreakpointWebAndDesktop) break CI so they are caught early.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e036e681-ca6f-42dd-a1ad-e9a1d8c5ecd3
📒 Files selected for processing (31)
integration_test/extensions/default_flows.dartintegration_test/robots/home_robot.dartintegration_test/robots/setting/settings_privacy_and_security_robot.dartlib/pages/bootstrap/init_client_dialog.dartlib/pages/bootstrap/tom_bootstrap_dialog.dartlib/pages/chat/chat.dartlib/pages/chat/chat_scroll_view.dartlib/pages/chat/send_file_dialog/send_file_dialog.dartlib/pages/chat_adaptive_scaffold/chat_adaptive_scaffold_builder.dartlib/pages/chat_details/chat_details.dartlib/pages/chat_draft/draft_chat.dartlib/pages/chat_list/chat_list_view.dartlib/pages/chat_profile_info/chat_profile_info_details.dartlib/pages/settings_dashboard/settings/settings_view.dartlib/pages/settings_dashboard/settings_profile/settings_profile_view.dartlib/pages/settings_dashboard/settings_security/settings_security_view.dartlib/presentation/mixins/chat_details_tab_mixin.dartlib/presentation/mixins/media_viewer_app_bar_mixin.dartlib/presentation/widget_keys/chat_keys.dartlib/presentation/widget_keys/dialog_keys.dartlib/presentation/widget_keys/link_preview_keys.dartlib/presentation/widget_keys/navigation_keys.dartlib/presentation/widget_keys/settings_keys.dartlib/presentation/widget_keys/widget_keys.dartlib/utils/dialog/twake_dialog.dartlib/widgets/layouts/adaptive_layout/adaptive_scaffold_appbar_style.dartlib/widgets/layouts/adaptive_layout/app_adaptive_scaffold.dartlib/widgets/layouts/adaptive_layout/app_adaptive_scaffold_body_view.dartlib/widgets/layouts/enum/adaptive_destinations_enum.dartlib/widgets/twake_components/twake_preview_link/twake_link_preview.dartlib/widgets/twake_components/twake_preview_link/twake_link_preview_item.dart
💤 Files with no reviewable changes (1)
- lib/pages/chat_list/chat_list_view.dart
🚧 Files skipped from review as they are similar to previous changes (16)
- lib/presentation/mixins/media_viewer_app_bar_mixin.dart
- lib/pages/chat_draft/draft_chat.dart
- lib/pages/chat_adaptive_scaffold/chat_adaptive_scaffold_builder.dart
- lib/pages/chat/chat.dart
- lib/widgets/layouts/adaptive_layout/app_adaptive_scaffold.dart
- integration_test/robots/home_robot.dart
- lib/pages/chat/send_file_dialog/send_file_dialog.dart
- lib/pages/chat/chat_scroll_view.dart
- lib/widgets/twake_components/twake_preview_link/twake_link_preview.dart
- lib/pages/bootstrap/tom_bootstrap_dialog.dart
- lib/pages/chat_details/chat_details.dart
- lib/presentation/widget_keys/widget_keys.dart
- lib/pages/settings_dashboard/settings/settings_view.dart
- integration_test/extensions/default_flows.dart
- lib/pages/settings_dashboard/settings_security/settings_security_view.dart
- lib/presentation/widget_keys/navigation_keys.dart
Do we have a tasks list of the actions needed to be able to run e2e on CI? |
Not that I know of |
|
IMO, Your current implementation has a few structural issues that will become problematic at scale:
Suggestion:
import 'package:flutter/foundation.dart';
mixin NamespacedKey on Enum {
String get namespace;
String get fullName => '$namespace.$name';
Key get key => Key(fullName);
ValueKey<String> get valueKey => ValueKey(fullName);
}=> Then refactor enums with namespace enum ChatKeys with NamespacedKey {
leaveChatButton,
actionsMobileAndTablet,
actionsWebAndDesktop,
composerTypeAhead,
mediaPickerTypeAhead,
draftComposerTypeAhead,
draftMediaPickerTypeAhead,
sendFileDialogTypeAhead,
eventListCenter,
invitationSelectionMobileAndTablet,
invitationSelectionWebAndDesktop,
forwardSelectionMobileAndTablet,
forwardSelectionWebAndDesktop;
@override
String get namespace => 'chat';
}
...=> Debugging, testing, and tracking are extremely easy. |
|
Thanks for the review! I agree on the duplicated logic point — the key/valueKey getters are copy-pasted across all 5 enums, which is not ideal. I'll extract a mixin to fix that: For the namespacing part though, I'm not convinced it's worth it here. The enum member names are already descriptive enough (leaveChatButton, composerTypeAhead, etc.), and the enum type itself (ChatKeys.x vs NavigationKeys.x) already provides namespacing at the Dart level. I'll push the mixin refactor without the namespace. WDYT? |
IMO, Enums only provide |
4 participants
Ticket
No ticket, refacto: hardcoded widget Key strings scattered across 20+ files with inconsistent conventions and duplicated values in tests.
Root cause
N/A — refactoring, not a bug.
Solution
Created 5 typed enums (NavigationKeys, SettingsKeys, DialogKeys, ChatKeys, LinkPreviewKeys) in lib/presentation/widget_keys/ that auto-derive key strings from enum member names
Migrated all ~45 hardcoded Key('...') / ValueKey('...') declarations to use these enums
Updated integration test robots to reference enums instead of duplicating raw strings
Removed dead key reference ConnectPageListView from default_flows.dart
Added integration_test/test_bundle.dart to .gitignore (auto-generated file)
Impact description
Type safety: typos in key names are now compile-time errors instead of silent test failures
Single source of truth: no more string duplication between widgets and tests
IDE support: autocomplete and refactoring work out of the box
Consistency: all keys now follow camelCase convention (derived from enum names)
No functional change — widget behavior is identical.
Test recommendations
dart analyze lib/ passes with 0 issues
flutter test test/widget/message/twake_link_preview_item_test.dart — 7/7 pass
Run full integration test suite to confirm key matching still works
Pre-merge
No.
Resolved
No visual change — pure refactoring. No screenshots needed.
Summary by CodeRabbit
Refactor
Tests
Chores