Skip to content

opt.: no Tag Switcher on desktop#932

Merged
lollipopkit merged 2 commits intomainfrom
lollipopkit/issue931
Oct 8, 2025
Merged

opt.: no Tag Switcher on desktop#932
lollipopkit merged 2 commits intomainfrom
lollipopkit/issue931

Conversation

@lollipopkit
Copy link
Copy Markdown
Owner

@lollipopkit lollipopkit commented Oct 8, 2025

Fixes #931

Summary by CodeRabbit

  • New Features

    • Desktop top bar now shows live server connection status and uses a responsive leading widget across breakpoints.
  • Bug Fixes

    • Avatar initials safely handle empty names, falling back to “?”.
  • Chores

    • Added a new participant identifier to the participant list.
    • Updated fl_lib to v1.0.351 and removed the local override.
    • Minor wiki submodule cleanup.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Oct 8, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

Refactors the server top bar to a ConsumerWidget with breakpoint-aware leading content and aggregated connection status; adds a Github participant ID; guards avatar initial extraction; updates fl_lib git ref and removes a local dependency override; deletes a submodule-commit line in the wiki.

Changes

Cohort / File(s) Summary of changes
Top bar UI
lib/view/page/server/tab/top_bar.dart
Converted _TopBar from StatelessWidget to ConsumerWidget; replaced build signature to use WidgetRef; added breakpoint-aware padding and a leading widget (mobile: settings-like button; desktop: aggregated "X/Y connected" text by watching servers' connection providers); removed mobile-only early return and adjusted layout.
Settings list (avatar)
lib/view/page/setting/seq/srv_seq.dart
Safely extract avatar initial from spi.name with fallback '?' when name is empty.
Data IDs
lib/data/res/github_id.dart
Added 'HHXXYY123' to GithubIds.static const participants.
Dependencies
pubspec.yaml
Updated fl_lib git ref from v1.0.349 to v1.0.351; commented-out/removed the local dependency_overrides entry for ../fl_lib.
Wiki submodule pointer
flutter_server_box.wiki
Removed a line referencing a subproject commit hash (deleted the submodule commit pointer line).

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant UI as TopBar (ConsumerWidget)
  participant Breakpoints as BreakpointService
  participant Riverpod as Providers

  User->>UI: Open Server Tab
  UI->>Breakpoints: isMobile?
  Breakpoints-->>UI: true/false

  UI->>Riverpod: watch(serverOrder)
  Riverpod-->>UI: [serverIds]

  loop per serverId
    UI->>Riverpod: watch(conn[serverId])
    Riverpod-->>UI: conn state
  end

  alt Desktop
    UI-->>User: Render leading: "X/Y connected" (aggregated)
  else Mobile
    UI-->>User: Render leading: settings button
  end

  UI-->>User: Render TagSwitcher and trailing controls
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

  • opt.: no Tag Switcher on desktop #931 — Top bar changes touch TagSwitcher layout and removed the mobile-only early return; likely relevant to the "no Tag Switcher on desktop" objective.

Poem

A twitch of whiskers, nibble bright,
I watched the top bar find its light.
New ID hops into the row,
A friendly "?" for names that go.
Dependencies fresh, I drum my feet — hooray, progress is sweet! 🥕🐇

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Linked Issues Check ⚠️ Warning The linked issue [#931] requests removing the Tag Switcher on desktop, but the provided diffs mainly modify top_bar to add breakpoint-aware leading content while explicitly "maintaining existing TagSwitcher usage and layout," so the change does not implement the stated objective. Other files changed in the PR do not address this issue and no conditional removal of the TagSwitcher for desktop is present in the summaries. Update top_bar to conditionally hide the TagSwitcher on non-mobile breakpoints (using the computed isMobile) and include a short note or UI screenshot demonstrating the TagSwitcher is removed on desktop, or else adjust the PR to match the implemented behavior.
Out of Scope Changes Check ⚠️ Warning Several unrelated changes appear in this PR that are out of scope for removing the Tag Switcher on desktop [#931], including adding a participant ID in lib/data/res/github_id.dart, an avatar fallback change in lib/view/page/setting/seq/srv_seq.dart, a dependency/version change and commented-out override in pubspec.yaml, and a trivial wiki line deletion. These edits are not required to implement the linked issue and should be separated or documented. Split unrelated edits into their own PRs or add explanations in this PR for why each non-UI change is included, and revert any accidental changes so reviewers can focus on the Tag Switcher change.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title concisely states the intended change (removing the Tag Switcher on desktop) and aligns with the PR objective Fixes #931; it is short and focused. The diff shows responsive top_bar changes related to breakpoints, so the title is related, although summaries indicate the TagSwitcher may still be present on desktop, so the title is partially but acceptably descriptive.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

📜 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 19443a6 and f6d5dc0.

📒 Files selected for processing (1)
  • lib/view/page/setting/seq/srv_seq.dart (1 hunks)

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

Copy link
Copy Markdown
Contributor

@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: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
lib/view/page/server/tab/top_bar.dart (1)

56-65: TagSwitcher still renders on desktop—breaks PR objective.

Even in the desktop branch we still place TagSwitcher(...).expanded() in the row, so the tag switcher remains visible. That contradicts “no Tag Switcher on desktop” and keeps the old UI. Please gate the widget behind the isMobile check (e.g., only include it in children when isMobile is true, otherwise insert a Spacer() or nothing).

A minimal adjustment could look like:

-        children: [
-          leading,
-          SizedBox(width: isMobile ? 30 : 16),
-          TagSwitcher(...).expanded(),
-        ],
+        children: [
+          leading,
+          if (isMobile) ...[
+            const SizedBox(width: 30),
+            TagSwitcher(
+              tags: tags,
+              onTagChanged: onTagChanged,
+              initTag: initTag,
+              singleLine: true,
+              reversed: true,
+            ).expanded(),
+          ],
+        ],

Feel free to adjust spacing for the desktop branch as needed.

🧹 Nitpick comments (1)
lib/view/page/setting/seq/srv_seq.dart (1)

116-128: Handle full grapheme clusters for initials.

String.fromCharCode(spi.name.runes.first) chops multi-code-point graphemes (e.g., emoji flags). Prefer the characters extension (bundled with Flutter) to extract the first visible character cleanly.

You can update the snippet like:

-    final String name;
-    if (spi.name.isNotEmpty) {
-      name = String.fromCharCode(spi.name.runes.first);
-    } else {
-      name = '?';
-    }
+    final name = spi.name.characters.isNotEmpty ? spi.name.characters.first : '?';

Make sure to import package:characters/characters.dart; it’s part of Flutter’s SDK, so no extra dependency needed.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3307fca and 19443a6.

⛔ Files ignored due to path filters (1)
  • pubspec.lock is excluded by !**/*.lock
📒 Files selected for processing (5)
  • flutter_server_box.wiki (0 hunks)
  • lib/data/res/github_id.dart (1 hunks)
  • lib/view/page/server/tab/top_bar.dart (1 hunks)
  • lib/view/page/setting/seq/srv_seq.dart (1 hunks)
  • pubspec.yaml (2 hunks)
💤 Files with no reviewable changes (1)
  • flutter_server_box.wiki
🧰 Additional context used
📓 Path-based instructions (2)
pubspec.yaml

📄 CodeRabbit inference engine (CLAUDE.md)

Use hive_ce instead of hive for Hive integration

Files:

  • pubspec.yaml
lib/view/**/*.dart

📄 CodeRabbit inference engine (CLAUDE.md)

lib/view/**/*.dart: Prefer widgets/utilities from fl_lib for common UI (e.g., CustomAppBar, context.showRoundDialog, Input, Btnx.cancelOk)
Prefer using libL10n strings before adding new ones to project l10n
Split UI into build, actions, and utils; use extension on to separate concerns

Files:

  • lib/view/page/setting/seq/srv_seq.dart
  • lib/view/page/server/tab/top_bar.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). (1)
  • GitHub Check: check

@lollipopkit lollipopkit merged commit bb3e3b4 into main Oct 8, 2025
1 of 2 checks passed
@lollipopkit lollipopkit deleted the lollipopkit/issue931 branch October 8, 2025 13:21
@coderabbitai coderabbitai bot mentioned this pull request Dec 7, 2025
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.

opt.: no Tag Switcher on desktop

1 participant