Skip to content

TW-2942: Refactor connection status handling in chat components#2943

Open
nqhhdev wants to merge 2 commits intomainfrom
TW-2942-UI-change-connection-warning
Open

TW-2942: Refactor connection status handling in chat components#2943
nqhhdev wants to merge 2 commits intomainfrom
TW-2942-UI-change-connection-warning

Conversation

@nqhhdev
Copy link
Member

@nqhhdev nqhhdev commented Mar 19, 2026

Ticket

Related issue

Resolved

Attach screenshots or videos demonstrating the changes

  • Web:
  • Android:
  • IOS:
Simulator Screenshot - iPhone 17 - 2026-03-19 at 12 30 31 Simulator Screenshot - iPhone 17 - 2026-03-19 at 12 30 34 Screenshot 2026-03-26 at 09 43 55

Summary by CodeRabbit

  • Improvements

    • Repositioned the connection-status indicator into app headers for more consistent visibility.
    • Reduced UI update frequency for connection status (debounced updates) and improved rendering transitions.
  • Localization

    • Added new status messages: "Updating..." and "Waiting for response".
    • Updated fallback error/status text to "Waiting for response" for better user feedback.

@coderabbitai
Copy link

coderabbitai bot commented Mar 19, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 90693c80-358a-44aa-8059-d046f5d3eff8

📥 Commits

Reviewing files that changed from the base of the PR and between 576320a and 7e9ce76.

📒 Files selected for processing (1)
  • lib/widgets/connection_status_header.dart

Walkthrough

The PR refactors how the connection status UI is composed and surfaced: ConnectionStatusHeader is removed from ChatViewBody and ChatListBodyView and instead accepted and propagated as an optional connectionStatusHeaderWidget through TwakeHeaderTwakeAppBar, allowing the app bar title to include the connection status. ConnectionStatusHeader's API changed to accept a connectedWidget instead of a controller, its UI now uses an AnimatedSwitcher and debounced sync-status updates (100ms Timer), and status text styling/localization calls were adjusted. Two new localization keys (updating, waitingForResponse) were added and a fallback error message now uses waitingForResponse.

Possibly related PRs

Suggested reviewers

  • hoangdat
  • 9clg6
  • dab246
  • tddang-linagora
🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The pull request description is incomplete. While it includes the ticket reference (#2942), screenshots demonstrating the changes, and platform placeholders, it lacks critical sections: Root cause, Solution, Impact description, Test recommendations, and Pre-merge verification. Add comprehensive sections explaining the refactoring approach, architectural improvements, testing strategy, and any pre-merge considerations to meet the template requirements.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main refactoring effort: reorganizing connection status handling across chat components by moving the ConnectionStatusHeader from multiple locations to a centralized header structure.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch TW-2942-UI-change-connection-warning

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.

@github-actions
Copy link
Contributor

This PR has been deployed to https://linagora.github.io/twake-on-matrix/2943

@nqhhdev nqhhdev changed the base branch from sprint/march26 to main March 26, 2026 04:24
@nqhhdev nqhhdev force-pushed the TW-2942-UI-change-connection-warning branch from 02ed6bb to 576320a Compare March 26, 2026 04:45
@nqhhdev nqhhdev marked this pull request as ready for review March 26, 2026 04:45
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (1)
lib/widgets/connection_status_header.dart (1)

72-90: Consider simplifying the nested ternary.

The nested ternary on lines 77-80 is functional but slightly hard to read. A minor readability improvement would be to use a local variable.

♻️ Optional refactor for readability
     child: hide
-        ? (widget.connectedWidget != null
-              ? widget.connectedWidget!
-              : const SizedBox.shrink())
+        ? widget.connectedWidget ?? const SizedBox.shrink()
         : Text(
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@lib/widgets/connection_status_header.dart` around lines 72 - 90, Replace the
nested ternary inside the AnimatedSwitcher child with a clearly named local
variable to improve readability: compute a Widget (e.g., childWidget) by using
hide ? (widget.connectedWidget ?? const SizedBox.shrink()) :
Text(status.toLocalizedString(context), key: ValueKey(status.status), maxLines:
1, overflow: TextOverflow.ellipsis, style:
Theme.of(context).textTheme.labelMedium?.copyWith(color:
LinagoraSysColors.material().secondary)), then pass that variable into
AnimatedSwitcher; this keeps the logic identical while avoiding the nested
ternary in the child property.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@assets/l10n/intl_en.arb`:
- Line 3144: Remove the unused localization key "updating" from the intl_en.arb
resource (or, if it's intentionally reserved for a near-term change, add a short
comment/TODO elsewhere documenting its planned use and create a follow-up PR);
specifically, delete the "updating": "Updating..." entry from the ARB file (or
confirm and document its intended consumer so reviewers know it’s deliberate).

In `@lib/utils/localized_exception_extension.dart`:
- Around line 47-48: The fallback localized message in the toLocalizedString
extension is incorrect: change the return value used for unknown/unhandled
exceptions from L10n.of(context)!.waitingForResponse back to the original error
fallback L10n.of(context)!.oopsSomethingWentWrong (and keep the
Logs().w('Something went wrong: ', this); call). Locate the toLocalizedString
implementation in localized_exception_extension.dart and replace the
waitingForResponse fallback with oopsSomethingWentWrong so error displays in
SnackBar (chat.dart) and other UI (story_view.dart,
public_room_bottom_sheet.dart) show the correct “something went wrong” message.

---

Nitpick comments:
In `@lib/widgets/connection_status_header.dart`:
- Around line 72-90: Replace the nested ternary inside the AnimatedSwitcher
child with a clearly named local variable to improve readability: compute a
Widget (e.g., childWidget) by using hide ? (widget.connectedWidget ?? const
SizedBox.shrink()) : Text(status.toLocalizedString(context), key:
ValueKey(status.status), maxLines: 1, overflow: TextOverflow.ellipsis, style:
Theme.of(context).textTheme.labelMedium?.copyWith(color:
LinagoraSysColors.material().secondary)), then pass that variable into
AnimatedSwitcher; this keeps the logic identical while avoiding the nested
ternary in the child property.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 362e5cd1-57e8-451b-89a2-8d62439f2205

📥 Commits

Reviewing files that changed from the base of the PR and between acfa947 and 576320a.

📒 Files selected for processing (9)
  • assets/l10n/intl_en.arb
  • lib/pages/chat/chat_app_bar_title.dart
  • lib/pages/chat/chat_view_body.dart
  • lib/pages/chat_list/chat_list_body_view.dart
  • lib/pages/chat_list/chat_list_header.dart
  • lib/utils/localized_exception_extension.dart
  • lib/widgets/app_bars/twake_app_bar.dart
  • lib/widgets/connection_status_header.dart
  • lib/widgets/twake_components/twake_header.dart
💤 Files with no reviewable changes (2)
  • lib/pages/chat/chat_view_body.dart
  • lib/pages/chat_list/chat_list_body_view.dart

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.

1 participant