Skip to content

Conversation

@Quwaysim
Copy link
Contributor

@Quwaysim Quwaysim commented Aug 17, 2025

Description

Fixes #395
This is a QoL refactor for WnFilledButton where we had label and icon params as Widget. This often leads to mistake as sometimes devs pass icon to label and vice-versa

Type of Change

  • Major refactor happened at lib/ui/core/ui/wn_filled_button.dart.
  • I updated the new usages around in approx 10 files

What to expect

There is no .icon or .child variant of the WnFilledButton anymore. For Regular filled buttons, either with prefix or suffix icons, use WnFilledButton at lib/ui/core/ui/wn_filled_button.dart. This changed was guided by the buttons components page on Figma.

For IconButtons, please use CustomIconButton at lib/shared/custom_icon_button.dart. It can be updated to fully support al usage types/variants.

Benefits

Consistency and no/less confusion, I think :D :D . Simply specify what you want, and the position comes up just right.

  • ✨ New feature (non-breaking change which adds functionality)
  • 🛠️ Bug fix (non-breaking change which fixes an issue)
  • ❌ Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 Code refactor
  • ✅ Build configuration change
  • 📝 Documentation
  • 🗑️ Chore

Checklist

  • Run just precommit to ensure that formatting and linting are correct
  • Updated the CHANGELOG.md file with your changes

Summary by CodeRabbit

  • New Features

    • Buttons across chat, contacts, profile and settings now use a unified label-plus-optional-trailing-icon API, standardizing actions like Search Chat, Add/Remove Contact, Add to Group, Send Message, QR flows, and dialog actions.
  • Style

    • Improved icon sizing, spacing and color consistency for clearer, more uniform visuals.
  • Refactor

    • Consolidated multiple button variants into a single consistent implementation and updated call sites to the new label/suffix pattern.

@Quwaysim Quwaysim linked an issue Aug 17, 2025 that may be closed by this pull request
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Aug 17, 2025

Walkthrough

Consolidated WnFilledButton API: renamed title→label, removed .child/.icon ctors, added prefixIcon/suffixIcon/labelTextStyle, and migrated many call sites from Row-based children to label + optional prefix/suffix icons; visual/layout behavior preserved except for a few loading-to-disabled adjustments.

Changes

Cohort / File(s) Summary
Core button API
lib/ui/core/ui/wn_button.dart, lib/ui/core/ui/wn_filled_button.dart
Unified WnFilledButton into a single constructor with label: String, optional prefixIcon/suffixIcon, labelTextStyle; removed .child/.icon named ctors; updated internal Row composition and imports (flutter_svg, gap).
Auth flow
lib/ui/auth_flow/...
Replaced title/child usages with label (and optional suffixIcon) in InfoScreen, CreateProfileScreen, LoginScreen, WelcomeScreen; no control-flow changes.
Chat / Chat info
lib/ui/chat/chat_info/*, lib/ui/chat/chat_management/*, lib/ui/chat/invite/*
Migrated .icon/.child usages to WnFilledButton(label: ..., suffixIcon: ...); preserved onPressed/loading/gating logic; removed inline Row/Text/Gap compositions.
Contact list & group flows
lib/ui/contact_list/*, lib/ui/contact_list/widgets/*
Replaced Row-based button content with label+suffixIcon; standardized icon assets via AssetsPaths; some in-button spinner UIs replaced by disabling (onPressed=null).
Settings & profile screens
lib/ui/settings/**, lib/ui/settings/profile/*
Replaced child/icon usages with label, suffixIcon, and labelTextStyle; updated flutter_svg import path and explicit icon sizing/tinting at call sites.
Misc UI call sites
various files under lib/ui/...
Bulk rename titlelabel across many call sites; preserved handlers and behavior.

Sequence Diagram(s)

(omitted — changes are API/markup refactors without new control-flow interactions)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Assessment against linked issues

Objective Addressed Explanation
Add icon position argument to buttons (#395)

Assessment against linked issues: Out-of-scope changes

Code Change Explanation
Removed inline progress indicator and replaced with disabled/onPressed=null semantics (lib/ui/contact_list/start_chat_bottom_sheet.dart) This alters the visible loading affordance (spinner removed) rather than adding icon-positioning functionality requested in #395.

Suggested reviewers

  • erskingardner
  • codeswot
  • untreu2

Poem

I twitch my nose and tidy each line,
Labels sit straight and icons align.
Prefix or suffix — now both can play,
No swapped-up props to lead us astray.
A rabbit cheers for buttons made fine. 🐇✨

Tip

🔌 Remote MCP (Model Context Protocol) integration is now available!

Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats.


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 91b5bd8 and 161496f.

⛔ Files ignored due to path filters (1)
  • assets/svgs/ic_arrows_vertical.svg is excluded by !**/*.svg
📒 Files selected for processing (1)
  • lib/ui/core/ui/wn_button.dart (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • lib/ui/core/ui/wn_button.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: Flutter CI
✨ Finishing Touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch 395-add-icon-position-arg-to-buttons

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
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
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: 1

🔭 Outside diff range comments (5)
lib/ui/chat/chat_info/widgets/member_action_buttons.dart (1)

111-118: Don’t use ref.watch in event handlers; use ref.read inside _toggleContact

ref.watch should be used in build; calling it inside _toggleContact risks assertion failures and unintended rebuild coupling. Compute the contact status with ref.read in the handler.

Update _toggleContact to avoid watch:

Future<void> _toggleContact() async {
  setState(() => _isAddingContact = true);

  try {
    final contactsNotifier = ref.read(contactsProvider.notifier);

    // Use read here to avoid watch outside build:
    final contactsState = ref.read(contactsProvider);
    final contacts = contactsState.contactModels ?? [];
    final bool isCurrentlyContact = contacts.any(
      (c) => c.publicKey.toLowerCase() == widget.user.publicKey.toLowerCase(),
    );

    if (isCurrentlyContact) {
      await contactsNotifier.removeContactByHex(widget.user.publicKey);
      if (mounted) ref.showSuccessToast('${widget.user.displayName} removed from contacts');
    } else {
      await contactsNotifier.addContactByHex(widget.user.publicKey);
      if (mounted) ref.showSuccessToast('${widget.user.displayName} added to contacts');
    }
  } catch (e) {
    if (mounted) ref.showErrorToast('Failed to update contact: $e');
  } finally {
    if (mounted) setState(() => _isAddingContact = false);
  }
}

Keep _isContact() with ref.watch for build-time reactivity; use ref.read in actions.

Also applies to: 120-151

lib/ui/settings/general_settings_screen.dart (1)

247-249: Logic bug: multiple accounts check is off by one

“Multiple accounts” should be > 1, not > 2. With exactly two accounts, this path won’t execute as intended.

Apply this fix:

-    final hasMultipleAccounts = accounts.length > 2;
+    final hasMultipleAccounts = accounts.length > 1;
lib/ui/contact_list/start_chat_bottom_sheet.dart (1)

152-160: Avoid ref.watch inside _isContact when called from actions

ref.watch should be confined to build. Here _isContact() is also used from _toggleContact (an event handler). Use ref.read to get a stable snapshot in the handler.

Either:

  • Keep _isContact() with ref.watch for build usage only, and in _toggleContact compute using ref.read (preferred), or
  • Add a separate helper using ref.read and call that in _toggleContact.

Example adjustment for _toggleContact:

final contactsState = ref.read(contactsProvider);
final contacts = contactsState.contactModels ?? [];
final bool isCurrentlyContact = contacts.any(
  (c) => c.publicKey.toLowerCase() == widget.contact.publicKey.toLowerCase(),
);

Also applies to: 167-171

lib/ui/settings/app_settings/app_settings_screen.dart (1)

37-37: Typo in dialog title

"Delete app app data" has a duplicated word.

Apply this diff:

-            title: 'Delete app app data',
+            title: 'Delete app data',
lib/ui/core/ui/wn_filled_button.dart (1)

4-14: Preserve WnButton API: forward style and onLongPress

Currently, callers can’t customize style or onLongPress through WnFilledButton. Forward these to avoid breaking the base API.

Apply this diff:

   WnFilledButton({
     super.key,
     super.size,
     super.visualState,
+    super.style,
     this.loading = false,
     this.prefixIcon,
     this.suffixIcon,
     this.titleTextStyle,
     required String title,
     required super.onPressed,
+    super.onLongPress,
   }) : super(
🧹 Nitpick comments (20)
lib/ui/auth_flow/info_screen.dart (2)

124-130: Add explicit size to the suffix icon for visual consistency.

In other call sites within this PR, suffix icons are explicitly sized. Do the same here to avoid size discrepancies across devices/themes.

-                suffixIcon: SvgPicture.asset(
+                suffixIcon: SvgPicture.asset(
                   AssetsPaths.icArrowRight,
+                  width: 18.w,
+                  height: 18.w,
                   colorFilter: ColorFilter.mode(
                     context.colors.primaryForeground,
                     BlendMode.srcIn,
                   ),
                 ),

121-123: Consider decoupling "loading" from provider loading state.

If you only want the spinner when the user taps the button, pass _isLoading to loading and keep isButtonDisabled for disabling. This avoids showing a spinner due to unrelated provider work.

-                loading: isButtonDisabled,
+                loading: _isLoading,
lib/ui/settings/profile/share_profile_qr_scan_screen.dart (1)

114-126: API update looks good; add semantics label for accessibility.

The migration to title + suffixIcon is correct. Consider adding a semanticsLabel to improve screen reader support.

-                            suffixIcon: SvgPicture.asset(
+                            suffixIcon: SvgPicture.asset(
                               AssetsPaths.icQrCode,
                               height: 18.w,
                               width: 18.w,
+                              semanticsLabel: 'QR code',
                               colorFilter: ColorFilter.mode(
                                 context.colors.primaryForeground,
                                 BlendMode.srcIn,
                               ),
                             ),
lib/ui/chat/chat_info/dm_chat_info.dart (3)

198-214: Set both width and height on the suffix icon for consistent sizing.

Other screens set both; doing the same avoids layout variance and keeps icons square.

                 suffixIcon: SvgPicture.asset(
                   AssetsPaths.icSearch,
-                  width: 14.w,
+                  width: 14.w,
+                  height: 14.w,
                   colorFilter: ColorFilter.mode(
                     context.colors.secondaryForeground,
                     BlendMode.srcIn,
                   ),
                 ),

216-232: Good: loading state disables action; add height to icon for consistency.

The onPressed gating with isContactLoading is correct. For consistency, specify height as well.

                 suffixIcon: SvgPicture.asset(
                   isContact ? AssetsPaths.icRemoveUser : AssetsPaths.icAddUser,
-                  width: 14.w,
+                  width: 14.w,
+                  height: 14.w,
                   colorFilter: ColorFilter.mode(
                     isContact
                         ? context.colors.secondaryForeground
                         : context.colors.primaryForeground,
                     BlendMode.srcIn,
                   ),
                 ),

244-256: Also add height to the Add to Group icon.

Keeps sizing uniform with other buttons in this screen.

                 suffixIcon: SvgPicture.asset(
                   AssetsPaths.icAdd,
-                  width: 14.w,
+                  width: 14.w,
+                  height: 14.w,
                   colorFilter: ColorFilter.mode(
                     context.colors.secondaryForeground,
                     BlendMode.srcIn,
                   ),
                 ),
lib/ui/settings/profile/share_profile_screen.dart (1)

178-190: Migration looks correct; consider adding a semantics label.

The move to title + suffixIcon is consistent with the new API. Optional: add a semanticsLabel for screen readers.

                               suffixIcon: SvgPicture.asset(
                                 AssetsPaths.icScan,
                                 height: 18.w,
                                 width: 18.w,
+                                semanticsLabel: 'Scan',
                                 colorFilter: ColorFilter.mode(
                                   context.colors.primaryForeground,
                                   BlendMode.srcIn,
                                 ),
                               ),
lib/ui/chat/chat_info/widgets/member_action_buttons.dart (2)

80-96: Prevent duplicate DM group creation while loading

If WnFilledButton doesn’t internally disable taps during loading, this can double-trigger group creation. Safer to gate onPressed by loading state.

Apply this diff if WnFilledButton does not disable on loading:

-      onPressed: _createOrOpenDirectMessageGroup,
+      onPressed: _isLoading ? null : _createOrOpenDirectMessageGroup,

156-171: Disable tap while loading and align disabled icon state

You set loading: _isLoading but still allow onPressed. If the button doesn’t internally guard, taps can queue while loading. Also consider ensuring suffixIcon reflects disabled state (muted color or opacity) if the button doesn’t handle it.

Apply this diff if needed:

-      onPressed: _toggleContact,
+      onPressed: _isLoading ? null : _toggleContact,

Optional: localize the user-visible strings per guidelines.

// title: isContact ? context.l10n.removeContact : context.l10n.addContact,
lib/ui/settings/general_settings_screen.dart (2)

224-229: Confirm if manual titleTextStyle override is necessary for destructive button

If WnFilledButton’s destructive variant already enforces the correct label color/contrast, overriding with titleTextStyle may be redundant and couples this button to typography internals.

If defaults are correct, remove the explicit style to simplify:

-                    titleTextStyle: WnButtonSize.small.textStyle().copyWith(
-                      color: context.colors.solidNeutralWhite,
-                    ),

333-344: Specify SVG icon size for consistent layout

This suffixIcon lacks width/height, unlike other usages in this PR. Explicit sizing avoids layout jitter across devices.

Apply this diff:

-                      suffixIcon: SvgPicture.asset(
-                        AssetsPaths.icArrowsVertical,
-                        colorFilter: ColorFilter.mode(
+                      suffixIcon: SvgPicture.asset(
+                        AssetsPaths.icArrowsVertical,
+                        width: 14.w,
+                        height: 14.w,
+                        colorFilter: ColorFilter.mode(
                           context.colors.primary,
                           BlendMode.srcIn,
                         ),
                       ),
lib/ui/contact_list/start_chat_bottom_sheet.dart (1)

261-275: Show progress on Add/Remove Contact and compute isContact once

  • Add loading: _isAddingContact for feedback consistency (you already do this for Start Chat).
  • Optional: compute _isContact() once to avoid double provider reads in one build.

Apply this minimal UX fix:

                         WnFilledButton(
                           size: WnButtonSize.small,
                           visualState: WnButtonVisualState.secondary,
                           onPressed: _isAddingContact ? null : _toggleContact,
+                          loading: _isAddingContact,
                           title: _isContact() ? 'Remove Contact' : 'Add Contact',

Optional refactor (outside this hunk): compute once at the top of the buttons section and reuse:

final bool isContact = _isContact();
// then use: title: isContact ? ... : ...
// and icon: isContact ? AssetsPaths.icRemoveUser : AssetsPaths.icAddUser

Also consider muted icon color when disabled, unless WnFilledButton handles this internally.

lib/ui/contact_list/widgets/profile_ready_card.dart (3)

4-4: Standardize flutter_svg import

Other files import from flutter_svg/flutter_svg.dart. Align this file for consistency and IDE symbol resolution.

-import 'package:flutter_svg/svg.dart';
+import 'package:flutter_svg/flutter_svg.dart';

82-94: Localize user-facing string

Per guidelines, prefer AppLocalizations for strings.

// title: context.l10n.shareYourProfile,

97-108: Localize user-facing string

Similarly, localize “Search For Friends”.

// title: context.l10n.searchForFriends,
lib/ui/settings/app_settings/app_settings_screen.dart (2)

51-59: Migration to new WnFilledButton API looks good; minor nit on explicit text color

The conversion to title/titleTextStyle is correct. Given WnFilledButton sets foregroundColor via visualState, you may be able to omit titleTextStyle here if the visual state already yields white for destructive buttons.


260-276: Clean adoption of suffixIcon; consider centralizing icon sizing

Usage aligns with the new API and reads clearly. Consider centralizing the 18.w icon size in the button size spec (e.g., a size.iconSize getter), so icon sizing stays consistent across usages and sizes.

lib/ui/core/ui/wn_filled_button.dart (3)

75-80: Disable callbacks while loading to reflect disabled state and prevent re-taps

Ignoring pointer events works, but setting callbacks to null ensures the button uses disabled styling while loading and guards long-press too.

Apply this diff:

   return FilledButton(
     style: effectiveStyle,
-    onPressed: onPressed,
-    onLongPress: onLongPress,
-    child: !loading ? child : loadingIndicator,
+    onPressed: loading ? null : onPressed,
+    onLongPress: loading ? null : onLongPress,
+    child: loading ? loadingIndicator : child,
   );

46-46: Use FilledButtonTheme instead of ElevatedButtonTheme

You’re building a FilledButton; merging the FilledButtonTheme is more appropriate than ElevatedButtonTheme and will pick up correct theme overrides.

Apply this diff:

-    final theme = Theme.of(context).elevatedButtonTheme;
+    final theme = Theme.of(context).filledButtonTheme;
@@
-        .merge(theme.style);
+        .merge(theme.style);

Also applies to: 73-73


68-71: Avoid redundant text color sources

You set both ButtonStyle.textStyle.color and an explicit Text style (via titleTextStyle or size.textStyle). Consider relying on visualState.foregroundColor (and not setting a color on the inner Text unless explicitly requested) to avoid conflicts.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 09e3295 and 33cd9df.

⛔ Files ignored due to path filters (2)
  • assets/svgs/ic_arrows_vertical.svg is excluded by !**/*.svg
  • pubspec.lock is excluded by !**/*.lock
📒 Files selected for processing (12)
  • lib/ui/auth_flow/info_screen.dart (1 hunks)
  • lib/ui/chat/chat_info/dm_chat_info.dart (3 hunks)
  • lib/ui/chat/chat_info/widgets/group_member_bottom_sheet.dart (2 hunks)
  • lib/ui/chat/chat_info/widgets/member_action_buttons.dart (2 hunks)
  • lib/ui/contact_list/start_chat_bottom_sheet.dart (1 hunks)
  • lib/ui/contact_list/widgets/profile_ready_card.dart (2 hunks)
  • lib/ui/core/ui/wn_filled_button.dart (2 hunks)
  • lib/ui/settings/app_settings/app_settings_screen.dart (4 hunks)
  • lib/ui/settings/general_settings_screen.dart (4 hunks)
  • lib/ui/settings/profile/edit_profile_screen.dart (2 hunks)
  • lib/ui/settings/profile/share_profile_qr_scan_screen.dart (1 hunks)
  • lib/ui/settings/profile/share_profile_screen.dart (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.dart

📄 CodeRabbit Inference Engine (.cursor/rules/flutter.mdc)

**/*.dart: Always declare the type of each variable and function (parameters and return value). Avoid using 'any'. Create necessary types.
Don't leave blank lines within a function.
One export per file.
Use PascalCase for classes.
Use camelCase for variables, functions, and methods.
Use underscores_case for file and directory names.
Use UPPERCASE for environment variables. Avoid magic numbers and define constants.
Start each function with a verb.
Use verbs for boolean variables. Example: isLoading, hasError, canDelete, etc.
Use complete words instead of abbreviations and correct spelling, except for standard and well-known abbreviations (API, URL, i, j, err, ctx, req, res, next).
Write short functions with a single purpose. Less than 20 instructions.
Name functions with a verb and something else. If it returns a boolean, use isX or hasX, canX, etc. If it doesn't return anything, use executeX or saveX, etc.
Avoid nesting blocks by early checks and returns, or extraction to utility functions.
Use higher-order functions (map, filter, reduce, etc.) to avoid function nesting. Use arrow functions for simple functions (less than 3 instructions). Use named functions for non-simple functions.
Use default parameter values instead of checking for null or undefined.
Reduce function parameters using RO-RO: use an object to pass multiple parameters and to return results. Declare necessary types for input arguments and output.
Use a single level of abstraction in functions.
Don't abuse primitive types and encapsulate data in composite types.
Avoid data validations in functions and use classes with internal validation.
Prefer immutability for data. Use readonly for data that doesn't change. Use 'as const' for literals that don't change.
Declare interfaces to define contracts.
Write small classes with a single purpose. Less than 200 instructions, less than 10 public methods, less than 10 properties.
Use exceptions to handle errors you don't expect. If you catch an exception, it sh...

Files:

  • lib/ui/settings/profile/share_profile_qr_scan_screen.dart
  • lib/ui/chat/chat_info/widgets/member_action_buttons.dart
  • lib/ui/settings/profile/share_profile_screen.dart
  • lib/ui/chat/chat_info/widgets/group_member_bottom_sheet.dart
  • lib/ui/settings/profile/edit_profile_screen.dart
  • lib/ui/contact_list/widgets/profile_ready_card.dart
  • lib/ui/settings/app_settings/app_settings_screen.dart
  • lib/ui/settings/general_settings_screen.dart
  • lib/ui/auth_flow/info_screen.dart
  • lib/ui/contact_list/start_chat_bottom_sheet.dart
  • lib/ui/chat/chat_info/dm_chat_info.dart
  • lib/ui/core/ui/wn_filled_button.dart
🧠 Learnings (4)
📚 Learning: 2025-08-08T13:39:00.500Z
Learnt from: CR
PR: parres-hq/whitenoise_flutter#0
File: .cursor/rules/flutter.mdc:0-0
Timestamp: 2025-08-08T13:39:00.500Z
Learning: Applies to **/*.dart : Use Riverpod to manage state. Use StreamProviders to watch for state changes that come from the rust api. See keepAlive if you need to keep the state alive.

Applied to files:

  • lib/ui/chat/chat_info/widgets/member_action_buttons.dart
  • lib/ui/settings/app_settings/app_settings_screen.dart
  • lib/ui/settings/general_settings_screen.dart
📚 Learning: 2025-08-08T13:39:00.500Z
Learnt from: CR
PR: parres-hq/whitenoise_flutter#0
File: .cursor/rules/flutter.mdc:0-0
Timestamp: 2025-08-08T13:39:00.500Z
Learning: Applies to **/*.dart : Use flutter_rust_bridge to access core functionality of the app.

Applied to files:

  • lib/ui/contact_list/widgets/profile_ready_card.dart
  • lib/ui/settings/app_settings/app_settings_screen.dart
  • lib/ui/settings/general_settings_screen.dart
📚 Learning: 2025-08-08T13:39:00.500Z
Learnt from: CR
PR: parres-hq/whitenoise_flutter#0
File: .cursor/rules/flutter.mdc:0-0
Timestamp: 2025-08-08T13:39:00.500Z
Learning: Applies to **/*.dart : Use ThemeData to manage themes.

Applied to files:

  • lib/ui/contact_list/widgets/profile_ready_card.dart
  • lib/ui/settings/app_settings/app_settings_screen.dart
  • lib/ui/settings/general_settings_screen.dart
📚 Learning: 2025-08-08T13:39:00.500Z
Learnt from: CR
PR: parres-hq/whitenoise_flutter#0
File: .cursor/rules/flutter.mdc:0-0
Timestamp: 2025-08-08T13:39:00.500Z
Learning: Applies to **/*.dart : Use the whitenoise rust crate (via flutter_rust_bridge) as the source of all data and only way to trigger changes in our data model.

Applied to files:

  • lib/ui/settings/app_settings/app_settings_screen.dart
  • lib/ui/settings/general_settings_screen.dart
🔇 Additional comments (8)
lib/ui/auth_flow/info_screen.dart (1)

120-131: Migration to new WnFilledButton API looks correct.

Using title + suffixIcon with loading/null onPressed aligns with the new API and intention of the PR.

lib/ui/settings/profile/edit_profile_screen.dart (2)

270-281: Dialog button migration LGTM.

Switch to title-based WnFilledButton with the secondaryWarning visual state and small size is consistent with the new API.


270-281: No remaining WnFilledButton.icon/child usages found

A project-wide search shows only commented-out references in lib/ui/chat/chat_info/group_chat_info.dart; there are no active calls to WnFilledButton.icon(...) or old-style child: parameters in WnFilledButton(...). No further action is needed.

lib/ui/contact_list/start_chat_bottom_sheet.dart (1)

278-291: LGTM on Add to Group button migration

Migration to title + suffixIcon reads clean and consistent with the new API.

lib/ui/chat/chat_info/widgets/group_member_bottom_sheet.dart (2)

269-283: LGTM on “Add to Another Group” button migration

Usage aligns well with the new WnFilledButton API; sizing/tinting is consistent with other buttons.


287-310: LGTM on “Remove From Group” destructive styling

Clear label styling via titleTextStyle and destructive icon tint fits the warning context. Nice.

lib/ui/settings/app_settings/app_settings_screen.dart (1)

5-5: New imports are appropriate for SVG asset usage

Adding flutter_svg and assets paths is correct given the new suffixIcon usage.

Also applies to: 21-21

lib/ui/core/ui/wn_filled_button.dart (1)

1-83: No active references to removed constructors
A repository-wide search for WnFilledButton.icon/.child only returned commented-out lines in lib/ui/chat/chat_info/group_chat_info.dart. There are no live usages of the deprecated variants. ✅

Comment on lines 20 to 35
if (prefixIcon != null) ...[
prefixIcon,
size == WnButtonSize.small ? Gap(8.w) : Gap(12.w),
],
FittedBox(
fit: BoxFit.scaleDown,
child: Text(
title,
style: titleTextStyle ?? size.textStyle(),
),
),
if (suffixIcon != null) ...[
size == WnButtonSize.small ? Gap(8.w) : Gap(12.w),
suffixIcon,
],
],
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix null-safety and loosen icon types (SvgPicture? → Widget?)

  • prefixIcon/suffixIcon are nullable but are inserted into a List without null assertion — this won’t compile.
  • Restricting to SvgPicture is unnecessary; callers may want to pass Icon, Image, or any Widget.

Switch to Widget? and assert non-null at insertion sites.

Apply this diff:

-             if (prefixIcon != null) ...[
-               prefixIcon,
+             if (prefixIcon != null) ...[
+               prefixIcon!,
                size == WnButtonSize.small ? Gap(8.w) : Gap(12.w),
              ],
@@
-             if (suffixIcon != null) ...[
+             if (suffixIcon != null) ...[
                size == WnButtonSize.small ? Gap(8.w) : Gap(12.w),
-               suffixIcon,
+               suffixIcon!,
              ],
@@
-  final SvgPicture? prefixIcon;
-  final SvgPicture? suffixIcon;
+  final Widget? prefixIcon;
+  final Widget? suffixIcon;

Also applies to: 40-42

🤖 Prompt for AI Agents
In lib/ui/core/ui/wn_filled_button.dart around lines 20-35 and 40-42,
prefixIcon/suffixIcon are currently typed as SvgPicture? and are being inserted
into a List<Widget> without null-assertion which breaks null-safety and
unnecessarily restricts icon types; change their types to Widget? (allow any
Widget such as Icon or Image), and in the widget tree assert non-null when
expanding into the children list (e.g., use the existing if (prefixIcon != null)
... pattern but insert prefixIcon! and suffixIcon!), keeping the gap logic
intact so the List<Widget> only contains non-null Widgets.

Copy link
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: 1

🧹 Nitpick comments (5)
lib/ui/contact_list/widgets/profile_ready_card.dart (2)

85-97: Suffix icon has no explicit size; add dimensions to avoid layout/visual inconsistencies

Without width/height, SVGs will render at their intrinsic size, which may not match the button spec (especially for small buttons). Recommend setting an explicit size.

Apply:

           suffixIcon: SvgPicture.asset(
             AssetsPaths.icQrCode,
+            width: 18.w,
+            height: 18.w,
             colorFilter: ColorFilter.mode(
               context.colors.primary,
               BlendMode.srcIn,
             ),
           ),

Additionally, consider localizing the title string via AppLocalizations to avoid introducing new hard-coded UI strings.

I can propose the exact localization keys/usages if you share your l10n conventions or ARB keys.


100-111: Set explicit size for the suffix icon and prefer localization for the button title

Same sizing concern as above; specify dimensions for consistency with your design tokens.

Apply:

           suffixIcon: SvgPicture.asset(
             AssetsPaths.icAddUser,
+            width: 18.w,
+            height: 18.w,
             colorFilter: ColorFilter.mode(
               context.colors.primaryForeground,
               BlendMode.srcIn,
             ),
           ),

Also, replace the hard-coded 'Search For Friends' with a localized string.

Happy to draft the l10n changes if you provide the localization keys or preferred naming.

lib/ui/settings/app_settings/app_settings_screen.dart (3)

50-58: Avoid overriding title text color for destructive buttons; let the component handle contrast

If WnFilledButton already applies the correct foreground color for destructive, overriding titleTextStyle per call is unnecessary and risks drift across states (pressed/disabled).

Apply:

-                  child: WnFilledButton(
+                  child: WnFilledButton(
                     visualState: WnButtonVisualState.destructive,
                     size: WnButtonSize.small,
                     onPressed: () => Navigator.of(dialogContext).pop(true),
-                    title: 'Delete',
-                    titleTextStyle: WnButtonSize.small.textStyle().copyWith(
-                      color: context.colors.solidNeutralWhite,
-                    ),
+                    title: 'Delete',
                   ),

Also consider localizing 'Delete' per your l10n scheme.


263-279: Set button size explicitly to match the large style; avoid manual text color overrides

You’re using WnButtonSize.large.textStyle() for the title, but the button’s size isn’t set. If the component’s default isn’t large, typography and paddings may be inconsistent. Prefer setting size: WnButtonSize.large and letting the component manage colors for the destructive variant.

Apply:

                           WnFilledButton(
                             title: 'Delete All Data',
-                            titleTextStyle: WnButtonSize.large.textStyle().copyWith(
-                              color: context.colors.solidNeutralWhite,
-                            ),
                             visualState: WnButtonVisualState.destructive,
+                            size: WnButtonSize.large,
                             onPressed: () => _deleteAllData(context, ref),
                             suffixIcon: SvgPicture.asset(
                               AssetsPaths.icDelete,
                               height: 18.w,
                               width: 18.w,
                               colorFilter: ColorFilter.mode(
                                 context.colors.solidNeutralWhite,
                                 BlendMode.srcIn,
                               ),
                             ),
                           ),

Also, replace the hard-coded 'Delete All Data' with a localized string to comply with your localization guideline.

Confirm that WnFilledButton’s default foreground for destructive is already white; if not, prefer a theme-level fix rather than per-call overrides.


5-5: Unify flutter_svg import paths across the repo

Pubspec.yaml declares

flutter_svg: ^2.1.0

which supports the new entrypoint

import 'package:flutter_svg/svg.dart';

However, we still have legacy imports of

import 'package:flutter_svg/flutter_svg.dart';

in both code and tests. Please migrate all of them to the new svg.dart entrypoint for consistency.

Affected files (examples):

  • test/ui/core/ui/wn_callout_test.dart
  • test/ui/contact_list/.../*.dart
  • lib/ui/contact_list/chat_list_screen.dart
  • lib/ui/contact_list/start_chat_bottom_sheet.dart
  • lib/ui/settings/general_settings_screen.dart
  • …and any other files importing package:flutter_svg/flutter_svg.dart

Ensuring a single import style will avoid mixed entrypoints and potential build/runtime issues.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 33cd9df and 623579f.

⛔ Files ignored due to path filters (1)
  • pubspec.lock is excluded by !**/*.lock
📒 Files selected for processing (5)
  • lib/ui/chat/chat_info/widgets/group_member_bottom_sheet.dart (2 hunks)
  • lib/ui/contact_list/widgets/profile_ready_card.dart (2 hunks)
  • lib/ui/core/ui/wn_button.dart (1 hunks)
  • lib/ui/settings/app_settings/app_settings_screen.dart (3 hunks)
  • lib/ui/settings/general_settings_screen.dart (2 hunks)
✅ Files skipped from review due to trivial changes (1)
  • lib/ui/core/ui/wn_button.dart
🚧 Files skipped from review as they are similar to previous changes (2)
  • lib/ui/chat/chat_info/widgets/group_member_bottom_sheet.dart
  • lib/ui/settings/general_settings_screen.dart
🧰 Additional context used
📓 Path-based instructions (1)
**/*.dart

📄 CodeRabbit Inference Engine (.cursor/rules/flutter.mdc)

**/*.dart: Always declare the type of each variable and function (parameters and return value). Avoid using 'any'. Create necessary types.
Don't leave blank lines within a function.
One export per file.
Use PascalCase for classes.
Use camelCase for variables, functions, and methods.
Use underscores_case for file and directory names.
Use UPPERCASE for environment variables. Avoid magic numbers and define constants.
Start each function with a verb.
Use verbs for boolean variables. Example: isLoading, hasError, canDelete, etc.
Use complete words instead of abbreviations and correct spelling, except for standard and well-known abbreviations (API, URL, i, j, err, ctx, req, res, next).
Write short functions with a single purpose. Less than 20 instructions.
Name functions with a verb and something else. If it returns a boolean, use isX or hasX, canX, etc. If it doesn't return anything, use executeX or saveX, etc.
Avoid nesting blocks by early checks and returns, or extraction to utility functions.
Use higher-order functions (map, filter, reduce, etc.) to avoid function nesting. Use arrow functions for simple functions (less than 3 instructions). Use named functions for non-simple functions.
Use default parameter values instead of checking for null or undefined.
Reduce function parameters using RO-RO: use an object to pass multiple parameters and to return results. Declare necessary types for input arguments and output.
Use a single level of abstraction in functions.
Don't abuse primitive types and encapsulate data in composite types.
Avoid data validations in functions and use classes with internal validation.
Prefer immutability for data. Use readonly for data that doesn't change. Use 'as const' for literals that don't change.
Declare interfaces to define contracts.
Write small classes with a single purpose. Less than 200 instructions, less than 10 public methods, less than 10 properties.
Use exceptions to handle errors you don't expect. If you catch an exception, it sh...

Files:

  • lib/ui/settings/app_settings/app_settings_screen.dart
  • lib/ui/contact_list/widgets/profile_ready_card.dart
🧠 Learnings (2)
📚 Learning: 2025-08-08T13:39:00.500Z
Learnt from: CR
PR: parres-hq/whitenoise_flutter#0
File: .cursor/rules/flutter.mdc:0-0
Timestamp: 2025-08-08T13:39:00.500Z
Learning: Applies to **/*.dart : Use Riverpod to manage state. Use StreamProviders to watch for state changes that come from the rust api. See keepAlive if you need to keep the state alive.

Applied to files:

  • lib/ui/settings/app_settings/app_settings_screen.dart
  • lib/ui/contact_list/widgets/profile_ready_card.dart
📚 Learning: 2025-08-08T13:39:00.500Z
Learnt from: CR
PR: parres-hq/whitenoise_flutter#0
File: .cursor/rules/flutter.mdc:0-0
Timestamp: 2025-08-08T13:39:00.500Z
Learning: Applies to **/*.dart : Use flutter_rust_bridge to access core functionality of the app.

Applied to files:

  • lib/ui/settings/app_settings/app_settings_screen.dart
  • lib/ui/contact_list/widgets/profile_ready_card.dart

import 'package:flutter_riverpod/flutter_riverpod.dart';
import 'package:flutter_screenutil/flutter_screenutil.dart';
import 'package:flutter_svg/flutter_svg.dart';
import 'package:flutter_svg/svg.dart';
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Confirm flutter_svg import path change is compatible with your installed version

The project-wide switch to package:flutter_svg/svg.dart is fine if you’re on flutter_svg ≥2.x and consistently using the new entrypoint. Please verify the dependency version and that there are no remaining mixed imports.

Run this to verify:


🏁 Script executed:

#!/bin/bash
# Check flutter_svg dependency version
rg -n "flutter_svg:" pubspec.yaml -C2

# Find all import paths and ensure consistency
rg -nP "package:flutter_svg/(flutter_svg|svg)\.dart" -C1

Length of output: 13045


Ensure consistent flutter_svg entrypoint across your codebase

  • Verified pubspec.yaml uses flutter_svg: ^2.1.0 (≥2.x), so the new svg.dart entrypoint is supported.
  • However, there are still numerous imports using the old path
    package:flutter_svg/flutter_svg.dart in both lib/ and test/ files.

Please update all of these to the new entrypoint:

import 'package:flutter_svg/svg.dart';

You can locate remaining legacy imports with:

rg -l "package:flutter_svg/flutter_svg.dart"

After refactoring, rerun the above command to confirm no old imports remain.

🤖 Prompt for AI Agents
In lib/ui/contact_list/widgets/profile_ready_card.dart around line 4, the file
currently imports the old flutter_svg entrypoint; replace any occurrence of
"package:flutter_svg/flutter_svg.dart" with the new entrypoint
"package:flutter_svg/svg.dart" to match the pubspec >=2.x requirement, apply the
same replacement across all lib/ and test/ files (use rg -l
"package:flutter_svg/flutter_svg.dart" to find them), update any affected import
statements in tests and widgets, and re-run the ripgrep command to confirm no
legacy imports remain.

Copy link
Contributor

@josefinalliende josefinalliende left a comment

Choose a reason for hiding this comment

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

Thanks for working on this one! 🙌

My intuition was that we could have one prop for the SVG icon and another for its position, with a default (probably "right" cause most icons are used as suffix). Buut I also like the current approach, since the prop name makes the position clear.

I really like how this is going, it’s a big improvement that we’re no longer using reversed text and icon props 🚀

beaver-buttonsr

),
),
],
title: 'Setup Profile',
Copy link
Contributor

@josefinalliende josefinalliende Aug 17, 2025

Choose a reason for hiding this comment

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

[sug] "label" for me is more intuitive for button text than "title"... WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not bad, let's go with label.... Will make a push soon.

Copy link
Contributor

@josefinalliende josefinalliende left a comment

Choose a reason for hiding this comment

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

I’m not 💯% sure, but I noticed some font size differences between master and this branch. The left screenshot is from master, and the right one is from this branch on the emulator. It seems that the text is smaller now.
Screenshot 2025-08-17 at 11 26 14 PM

josefinalliende

This comment was marked as resolved.

Copy link
Contributor

@josefinalliende josefinalliende left a comment

Choose a reason for hiding this comment

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

Amazing. Checked locally every screen that was in files changed list and they look great. Just left some small doubts related to some font size changes in some screens. If @vladimir-krstic is ok with the font sizes, then I give my ✅ !

@vladimir-krstic
Copy link

Amazing. Checked locally every screen that was in files changed list and they look great. Just left some small doubts related to some font size changes in some screens. If @vladimir-krstic is ok with the font sizes, then I give my ✅ !

Can't verify from screenshots but they do look odd. Button text font specs should be:
Font: Manrope
Weight: SemiBold (600)
Height: 16px
Line Height: 20px
Letter Spacing: 0.2px

@Quwaysim
Copy link
Contributor Author

Thanks for working on this one! 🙌

My intuition was that we could have one prop for the SVG icon and another for its position, with a default (probably "right" cause most icons are used as suffix). Buut I also like the current approach, since the prop name makes the position clear.

I really like how this is going, it’s a big improvement that we’re no longer using reversed text and icon props 🚀

beaver-buttonsr

Thanks.

For using position though, I prefer prefix and suffix or leading and trailing since it aligns better with the way things are done in Flutter IMO.

@Quwaysim
Copy link
Contributor Author

Thanks @josefinalliende @vladimir-krstic, resolving that now

Copy link
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

🔭 Outside diff range comments (1)
lib/ui/chat/chat_info/widgets/group_member_bottom_sheet.dart (1)

260-261: Row does not support spacing; insert an explicit Gap between children

Flutter’s Row has no spacing named parameter. This will not compile on stable. Use a Gap between the Flexible children (you already import gap).

Apply this diff:

-              Row(
-                spacing: 6.w,
-                children: [
+              Row(
+                children: [
                   Flexible(
                     child: SendMessageButton(widget.member),
                   ),
+                  Gap(6.w),
                   Flexible(
                     child: AddToContactButton(widget.member),
                   ),
                 ],
               ),
♻️ Duplicate comments (2)
lib/ui/settings/network/add_relay_bottom_sheet.dart (1)

8-8: Switch flutter_svg import to the new entrypoint for consistency

Use the updated entrypoint to align with the rest of the repo and flutter_svg ≥2.x guidance.

Apply:

-import 'package:flutter_svg/flutter_svg.dart';
+import 'package:flutter_svg/svg.dart';
lib/ui/core/ui/wn_filled_button.dart (1)

16-35: Fix null-safety and allow any widget for icons (SvgPicture? → Widget?)

prefixIcon/suffixIcon are nullable fields but inserted into a List<Widget> without !, which violates null-safety. Also, restricting these to SvgPicture is unnecessarily tight; callers may want to pass Icon, Image, or any Widget.

Apply this diff:

          children: [
-            if (prefixIcon != null) ...[
-              prefixIcon,
+            if (prefixIcon != null) ...[
+              prefixIcon!,
               size == WnButtonSize.small ? Gap(8.w) : Gap(12.w),
             ],
             FittedBox(
               fit: BoxFit.scaleDown,
               child: Text(
                 label,
                 style: titleTextStyle ?? size.textStyle(),
               ),
             ),
-            if (suffixIcon != null) ...[
+            if (suffixIcon != null) ...[
               size == WnButtonSize.small ? Gap(8.w) : Gap(12.w),
-              suffixIcon,
+              suffixIcon!,
             ],
           ],
        ),
      );

   final bool loading;
-  final SvgPicture? prefixIcon;
-  final SvgPicture? suffixIcon;
+  final Widget? prefixIcon;
+  final Widget? suffixIcon;
   final TextStyle? titleTextStyle;

Also applies to: 39-42

🧹 Nitpick comments (11)
lib/ui/settings/developer/developer_settings_screen.dart (1)

127-127: Localize button labels

Consider moving hard-coded labels ('Clear Cache', 'Reload Contacts') to AppLocalizations for i18n consistency.

Also applies to: 136-136

lib/ui/auth_flow/login_screen.dart (1)

231-231: Localize the 'Login' label

Recommend sourcing this from AppLocalizations to support translations.

lib/ui/chat/invite/chat_invite_screen.dart (1)

74-74: Localize action labels

Please consider moving 'Decline' and 'Accept' to AppLocalizations to maintain i18n consistency across the app.

Also applies to: 85-85

lib/ui/settings/network/widgets/relay_tile.dart (1)

80-80: Localize dialog button text

'Cancel' and 'Remove Relay' should come from AppLocalizations to support translations.

Also applies to: 87-87

lib/ui/chat/chat_management/add_to_group_screen.dart (1)

218-218: Localize CTA text

Consider moving 'Add to Group' to AppLocalizations for i18n.

lib/ui/settings/profile/switch_profile_bottom_sheet.dart (1)

104-119: Avoid repeated npub→hex conversions by leveraging the cache

_isActiveAccount currently recomputes/awaits hex conversion per tile even though _precomputeProfileHexes already maintains _pubkeyToHex. Use the cache first, and populate it on miss. This reduces rebuild cost and avoids redundant async work.

Apply this diff:

-  Future<bool> _isActiveAccount(ContactModel profile) async {
-    if (_activeAccountHex == null) return false;
-
-    try {
-      // Convert profile's npub key to hex for comparison
-      String profileHex = profile.publicKey;
-      if (profile.publicKey.startsWith('npub1')) {
-        profileHex = await hexPubkeyFromNpub(npub: profile.publicKey);
-      }
-
-      return profileHex == _activeAccountHex;
-    } catch (e) {
-      // If conversion fails, try direct comparison as fallback
-      return profile.publicKey == _activeAccountHex;
-    }
-  }
+  Future<bool> _isActiveAccount(ContactModel profile) async {
+    if (_activeAccountHex == null) return false;
+    final original = profile.publicKey;
+
+    // Fast-path using the precomputed cache
+    final cached = _pubkeyToHex[original];
+    if (cached != null) {
+      return cached == _activeAccountHex;
+    }
+
+    try {
+      final hex = original.startsWith('npub1')
+          ? await hexPubkeyFromNpub(npub: original)
+          : original;
+      // Cache for subsequent calls
+      _pubkeyToHex[original] = hex;
+      return hex == _activeAccountHex;
+    } catch (_) {
+      // Fallback to original and cache the result to avoid repeated work
+      _pubkeyToHex[original] = original;
+      return original == _activeAccountHex;
+    }
+  }
lib/ui/contact_list/group_chat_details_sheet.dart (1)

176-210: Speed up keypackage checks by parallelizing I/O

_fetchKeyPackage is awaited sequentially for each contact. Given this is all I/O, you can parallelize with Future.wait to reduce overall latency. Consider a bounded concurrency pool if the upstream can’t handle bursts.

Example implementation:

Future<Map<String, List<ContactModel>>> _filterContactsByKeyPackage(
  List<ContactModel> contacts,
) async {
  final activeAccountData = await ref.read(activeAccountProvider.notifier).getActiveAccountData();
  if (activeAccountData == null) {
    throw Exception('No active account found');
  }

  final withKey = <ContactModel>[];
  final withoutKey = <ContactModel>[];

  // Fire requests in parallel
  final results = await Future.wait(
    contacts.map((contact) async {
      try {
        final pubkey = await publicKeyFromString(publicKeyString: contact.publicKey);
        final keyPackage = await fetchKeyPackage(
          pubkey: pubkey,
          nip65Relays: activeAccountData.nip65Relays,
        );
        return (contact, hasKey: keyPackage != null);
      } catch (_) {
        return (contact, hasKey: false);
      }
    }),
    eagerError: false,
  );

  for (final r in results) {
    if (r.hasKey) {
      withKey.add(r.$1);
    } else {
      withoutKey.add(r.$1);
    }
  }

  return {
    'withKeyPackage': withKey,
    'withoutKeyPackage': withoutKey,
  };
}

If you need to limit concurrency, you can chunk contacts or use a simple semaphore.

lib/ui/auth_flow/create_profile_screen.dart (1)

61-71: Avoid calling ref.listen inside build to prevent multiple subscriptions

Placing ref.listen in build risks creating a new listener on every rebuild. Move it to initState (ConsumerState provides ref there) and remove the build-time listener.

Proposed change (initState addition):

@override
void initState() {
  super.initState();

  WidgetsBinding.instance.addPostFrameCallback((_) async {
    // existing metadata-loading logic unchanged…
  });

  // Move this from build to initState
  ref.listen<AccountState>(accountProvider, (previous, next) {
    if (next.metadata?.displayName != null &&
        next.metadata!.displayName!.isNotEmpty &&
        _displayNameController.text.isEmpty) {
      _displayNameController.text = next.metadata!.displayName!;
      if (mounted) {
        setState(() {
          _isLoadingDisplayName = false;
        });
      }
    }
  });
}

Then remove the ref.listen block from build.

lib/ui/chat/chat_info/widgets/group_member_bottom_sheet.dart (1)

275-289: Suffix icon migration reads well; verify icon sizing consistency

The trailing icon implementation using suffixIcon is correct. Quick check: ensure the 14.w x 13.h sizing is intentional vs. other small buttons (many use 18x18). If it's by design for this specific button, all good.

lib/ui/contact_list/start_chat_bottom_sheet.dart (1)

261-275: Avoid double provider reads and show loading for better UX

  • _isContact() is called twice (label and icon), each calling ref.watch(...). Cache it once per build to avoid redundant provider reads.
  • Show a spinner when _isAddingContact is true using the loading prop (you already disable onPressed).

Apply this diff to the button:

                         WnFilledButton(
                           size: WnButtonSize.small,
                           visualState: WnButtonVisualState.secondary,
-                          onPressed: _isAddingContact ? null : _toggleContact,
-                          label: _isContact() ? 'Remove Contact' : 'Add Contact',
+                          loading: _isAddingContact,
+                          onPressed: _isAddingContact ? null : _toggleContact,
+                          label: isContact ? 'Remove Contact' : 'Add Contact',
                           suffixIcon: SvgPicture.asset(
-                            _isContact() ? AssetsPaths.icRemoveUser : AssetsPaths.icAddUser,
+                            isContact ? AssetsPaths.icRemoveUser : AssetsPaths.icAddUser,
                             width: 18.w,
                             height: 18.w,
                             colorFilter: ColorFilter.mode(
                               context.colors.primary,
                               BlendMode.srcIn,
                             ),
                           ),
                         ),

And add this once at the start of build (or just before rendering the buttons):

// At the start of build(), before returning the Column:
final bool isContact = _isContact();
lib/ui/settings/app_settings/app_settings_screen.dart (1)

5-5: Use a consistent flutter_svg import across the app

This file imports package:flutter_svg/svg.dart while many others use package:flutter_svg/flutter_svg.dart. Both paths can work depending on flutter_svg version, but mixing them is brittle and surprises tooling. Prefer a single import path project-wide.

If you choose the canonical one used elsewhere in the repo, apply:

-import 'package:flutter_svg/svg.dart';
+import 'package:flutter_svg/flutter_svg.dart';
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 623579f and 5bbd0ff.

📒 Files selected for processing (26)
  • lib/ui/auth_flow/create_profile_screen.dart (1 hunks)
  • lib/ui/auth_flow/info_screen.dart (1 hunks)
  • lib/ui/auth_flow/login_screen.dart (1 hunks)
  • lib/ui/auth_flow/welcome_screen.dart (1 hunks)
  • lib/ui/chat/chat_info/dm_chat_info.dart (3 hunks)
  • lib/ui/chat/chat_info/widgets/group_member_bottom_sheet.dart (3 hunks)
  • lib/ui/chat/chat_info/widgets/member_action_buttons.dart (2 hunks)
  • lib/ui/chat/chat_management/add_to_group_screen.dart (1 hunks)
  • lib/ui/chat/invite/chat_invite_screen.dart (2 hunks)
  • lib/ui/contact_list/group_chat_details_sheet.dart (1 hunks)
  • lib/ui/contact_list/group_welcome_invitation_sheet.dart (2 hunks)
  • lib/ui/contact_list/new_group_chat_sheet.dart (1 hunks)
  • lib/ui/contact_list/start_chat_bottom_sheet.dart (1 hunks)
  • lib/ui/contact_list/widgets/profile_ready_card.dart (2 hunks)
  • lib/ui/contact_list/widgets/share_invite_button.dart (1 hunks)
  • lib/ui/core/ui/wn_filled_button.dart (2 hunks)
  • lib/ui/settings/app_settings/app_settings_screen.dart (3 hunks)
  • lib/ui/settings/developer/developer_settings_screen.dart (2 hunks)
  • lib/ui/settings/general_settings_screen.dart (2 hunks)
  • lib/ui/settings/network/add_relay_bottom_sheet.dart (1 hunks)
  • lib/ui/settings/network/widgets/relay_tile.dart (1 hunks)
  • lib/ui/settings/profile/connect_profile_bottom_sheet.dart (2 hunks)
  • lib/ui/settings/profile/edit_profile_screen.dart (4 hunks)
  • lib/ui/settings/profile/share_profile_qr_scan_screen.dart (1 hunks)
  • lib/ui/settings/profile/share_profile_screen.dart (1 hunks)
  • lib/ui/settings/profile/switch_profile_bottom_sheet.dart (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
  • lib/ui/auth_flow/info_screen.dart
  • lib/ui/settings/general_settings_screen.dart
  • lib/ui/settings/profile/share_profile_qr_scan_screen.dart
  • lib/ui/chat/chat_info/widgets/member_action_buttons.dart
  • lib/ui/chat/chat_info/dm_chat_info.dart
  • lib/ui/settings/profile/share_profile_screen.dart
  • lib/ui/settings/profile/edit_profile_screen.dart
🧰 Additional context used
📓 Path-based instructions (1)
**/*.dart

📄 CodeRabbit Inference Engine (.cursor/rules/flutter.mdc)

**/*.dart: Always declare the type of each variable and function (parameters and return value). Avoid using 'any'. Create necessary types.
Don't leave blank lines within a function.
One export per file.
Use PascalCase for classes.
Use camelCase for variables, functions, and methods.
Use underscores_case for file and directory names.
Use UPPERCASE for environment variables. Avoid magic numbers and define constants.
Start each function with a verb.
Use verbs for boolean variables. Example: isLoading, hasError, canDelete, etc.
Use complete words instead of abbreviations and correct spelling, except for standard and well-known abbreviations (API, URL, i, j, err, ctx, req, res, next).
Write short functions with a single purpose. Less than 20 instructions.
Name functions with a verb and something else. If it returns a boolean, use isX or hasX, canX, etc. If it doesn't return anything, use executeX or saveX, etc.
Avoid nesting blocks by early checks and returns, or extraction to utility functions.
Use higher-order functions (map, filter, reduce, etc.) to avoid function nesting. Use arrow functions for simple functions (less than 3 instructions). Use named functions for non-simple functions.
Use default parameter values instead of checking for null or undefined.
Reduce function parameters using RO-RO: use an object to pass multiple parameters and to return results. Declare necessary types for input arguments and output.
Use a single level of abstraction in functions.
Don't abuse primitive types and encapsulate data in composite types.
Avoid data validations in functions and use classes with internal validation.
Prefer immutability for data. Use readonly for data that doesn't change. Use 'as const' for literals that don't change.
Declare interfaces to define contracts.
Write small classes with a single purpose. Less than 200 instructions, less than 10 public methods, less than 10 properties.
Use exceptions to handle errors you don't expect. If you catch an exception, it sh...

Files:

  • lib/ui/auth_flow/login_screen.dart
  • lib/ui/contact_list/new_group_chat_sheet.dart
  • lib/ui/settings/network/widgets/relay_tile.dart
  • lib/ui/chat/chat_management/add_to_group_screen.dart
  • lib/ui/contact_list/group_chat_details_sheet.dart
  • lib/ui/settings/developer/developer_settings_screen.dart
  • lib/ui/settings/profile/connect_profile_bottom_sheet.dart
  • lib/ui/auth_flow/create_profile_screen.dart
  • lib/ui/contact_list/group_welcome_invitation_sheet.dart
  • lib/ui/settings/profile/switch_profile_bottom_sheet.dart
  • lib/ui/contact_list/widgets/share_invite_button.dart
  • lib/ui/auth_flow/welcome_screen.dart
  • lib/ui/chat/invite/chat_invite_screen.dart
  • lib/ui/chat/chat_info/widgets/group_member_bottom_sheet.dart
  • lib/ui/contact_list/widgets/profile_ready_card.dart
  • lib/ui/settings/app_settings/app_settings_screen.dart
  • lib/ui/settings/network/add_relay_bottom_sheet.dart
  • lib/ui/core/ui/wn_filled_button.dart
  • lib/ui/contact_list/start_chat_bottom_sheet.dart
🧠 Learnings (2)
📚 Learning: 2025-08-08T13:39:00.500Z
Learnt from: CR
PR: parres-hq/whitenoise_flutter#0
File: .cursor/rules/flutter.mdc:0-0
Timestamp: 2025-08-08T13:39:00.500Z
Learning: Applies to **/*.dart : Use Riverpod to manage state. Use StreamProviders to watch for state changes that come from the rust api. See keepAlive if you need to keep the state alive.

Applied to files:

  • lib/ui/contact_list/widgets/profile_ready_card.dart
  • lib/ui/settings/app_settings/app_settings_screen.dart
📚 Learning: 2025-08-08T13:39:00.500Z
Learnt from: CR
PR: parres-hq/whitenoise_flutter#0
File: .cursor/rules/flutter.mdc:0-0
Timestamp: 2025-08-08T13:39:00.500Z
Learning: Applies to **/*.dart : Use flutter_rust_bridge to access core functionality of the app.

Applied to files:

  • lib/ui/contact_list/widgets/profile_ready_card.dart
  • lib/ui/settings/app_settings/app_settings_screen.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: Flutter CI
🔇 Additional comments (28)
lib/ui/settings/developer/developer_settings_screen.dart (2)

126-131: WnFilledButton migration to label — looks correct

Both button usages correctly adopt the new label API. No functional concerns from this change.

Also applies to: 135-139


126-139: Deprecated WnFilledButton API Fully Removed

I’ve scanned for all active uses of WnFilledButton.icon / .child constructors and any title: parameters—no live call sites remain. The only references are commented-out snippets in:

  • lib/ui/chat/chat_info/group_chat_info.dart (lines 162, 174)

These can be deleted at your discretion. The migration to label/prefixIcon/suffixIcon is complete.

lib/ui/auth_flow/login_screen.dart (1)

229-233: WnFilledButton label migration — good

The update to label: 'Login' aligns with the new API. Loading/disable logic remains intact.

lib/ui/chat/invite/chat_invite_screen.dart (1)

73-82: Button API migration looks correct

Both 'Decline' and 'Accept' buttons are updated to label and preserve previous behaviors (secondary/destructive/loader/disable). Looks good.

Also applies to: 84-118

lib/ui/settings/network/widgets/relay_tile.dart (1)

78-83: Dialog buttons migrated to label correctly

The cancel and destructive actions keep their visual states and sizes; migration is accurate.

Also applies to: 85-90

lib/ui/chat/chat_management/add_to_group_screen.dart (1)

217-221: CTA migrated to label — looks good

The bottom action now uses label while preserving loading/enablement logic. No issues spotted.

lib/ui/settings/profile/switch_profile_bottom_sheet.dart (2)

213-215: WnFilledButton(title → label) migration looks good

The call site correctly uses label and keeps behavior intact.


213-215: All deprecated WnFilledButton APIs have been removed

I ran the scan across the Dart codebase and confirmed there are no active usages of:

  • WnFilledButton(title: …)
  • WnFilledButton.icon
  • WnFilledButton.child

The only occurrences of WnFilledButton.icon are in commented-out legacy snippets (e.g. in lib/ui/chat/chat_info/group_chat_info.dart), so no further changes are needed here.

lib/ui/contact_list/widgets/share_invite_button.dart (1)

55-56: WnFilledButton(title → label) migration looks good

Correctly maps the dynamic text to label and preserves loading/disabled behavior.

lib/ui/contact_list/group_chat_details_sheet.dart (1)

301-304: WnFilledButton(title → label) migration looks good

Label text and loading/enablement logic remain consistent with prior behavior.

lib/ui/auth_flow/create_profile_screen.dart (1)

205-218: WnFilledButton(title → label) migration looks good

Keeps the same disabled/loading behavior while adopting the new API.

lib/ui/settings/profile/connect_profile_bottom_sheet.dart (2)

42-64: WnFilledButton(title → label) migration looks good

The semantics and disabled/loading behavior are preserved; visualState remains secondary.


67-82: WnFilledButton(title → label) migration looks good

Reads authProvider.isLoading for loading state, consistent with prior logic.

lib/ui/auth_flow/welcome_screen.dart (1)

96-99: No lingering legacy WnFilledButton APIs found
Scanned the entire codebase—there are no active WnFilledButton.child or .icon constructors, nor any title: named arguments on WnFilledButton. All call sites now correctly use label/prefixIcon/suffixIcon.

lib/ui/settings/network/add_relay_bottom_sheet.dart (1)

249-252: WnFilledButton API migration (label) is correct

The call site update from title to label preserves behavior and matches the new API.

lib/ui/contact_list/new_group_chat_sheet.dart (1)

208-209: WnFilledButton label migration LGTM

The change to label: 'Continue' is consistent with the new API, with no behavior changes.

lib/ui/contact_list/group_welcome_invitation_sheet.dart (1)

83-84: Button text param rename is correct and behavior-preserving

Both Decline and Accept now use label as expected. No other logic changes—good.

Also applies to: 93-94

lib/ui/contact_list/widgets/profile_ready_card.dart (3)

4-4: Good move to the new flutter_svg entrypoint

Thanks for switching to package:flutter_svg/svg.dart; this matches the repo-wide migration.


85-97: Clean refactor to label + suffixIcon — clearer API and easier to scan

Replacing the old child variant with explicit label and suffixIcon improves readability and reduces misuse risk. ColorFilter choice also matches the secondary visual state.


100-111: Second button migration also looks good

Explicit label + suffixIcon with default visual state is appropriate here; the icon color aligns with a primary button.

lib/ui/chat/chat_info/widgets/group_member_bottom_sheet.dart (3)

161-167: Migration to label param looks correct

The cancel button correctly uses the new label API with the small secondary visual state. No functional issues spotted.


169-174: Destructive confirm button wiring is sound

Using loading: _isRemoving plus disabling onPressed while removing aligns with the new API and avoids duplicate actions. Good update.


292-316: Secondary warning + custom textStyle is coherent

Applying WnButtonVisualState.secondaryWarning and overriding the label color via titleTextStyle keeps the text/icon red while retaining the secondary-warning container — consistent with the rest of the design system. Looks good.

lib/ui/contact_list/start_chat_bottom_sheet.dart (2)

278-291: LGTM: clean migration to suffixIcon

“Add to Group” correctly adopts the new API with a trailing icon and secondary style. No issues noted.


293-297: Good: loading state and label stay in sync

Start Chat button properly toggles both the loading indicator and the label. This avoids accidental re-taps during group creation.

lib/ui/settings/app_settings/app_settings_screen.dart (2)

41-46: Dialog actions: migration to label looks correct

Cancel/Confirm buttons in the dialog use the new API properly. The small sizes and visual states match expected semantics.


263-279: Destructive CTA migration is correct; style override is appropriate

“Delete All Data” correctly uses label and a trailing icon. Overriding text color to white via titleTextStyle aligns with the destructive visual state and button theme.

lib/ui/core/ui/wn_filled_button.dart (1)

68-71: Confirm precedence of text style vs. visual state color

buildButton sets textStyle: size.textStyle().copyWith(color: visualState.foregroundColor(context)), while the label Text uses titleTextStyle ?? size.textStyle(). The Text style will take precedence over the ButtonStyle.textStyle. This is fine, but ensure this is intentional since call sites (e.g., destructive secondary with red label) rely on the override.

No changes requested; just a heads-up.

Copy link
Contributor

@josefinalliende josefinalliende left a comment

Choose a reason for hiding this comment

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

Cool! 😎 So I checked the code of the screenshots that I left earlier and just one sheet needs small change to match the font sie vlad mentioned, the others are ok! I left the code suggestions, just 2 lines.

This was referenced Aug 18, 2025
Quwaysim and others added 2 commits August 18, 2025 13:06
Co-authored-by: Josefina Alliende <mariajosefinaalliende@gmail.com>
Co-authored-by: Josefina Alliende <mariajosefinaalliende@gmail.com>
Copy link
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: 2

🔭 Outside diff range comments (1)
lib/ui/contact_list/start_chat_bottom_sheet.dart (1)

162-171: Riverpod misuse: _isContact() uses watch; calling it in an event handler can assert at runtime.

_toggleContact calls _isContact(), which uses ref.watch. watch must only be used during build. Use ref.read in handlers.

     try {
-      final contactsNotifier = ref.read(contactsProvider.notifier);
-      final isCurrentlyContact = _isContact();
+      final contactsNotifier = ref.read(contactsProvider.notifier);
+      final contactsState = ref.read(contactsProvider);
+      final List<ContactModel> contacts = contactsState.contactModels ?? const <ContactModel>[];
+      final bool isCurrentlyContact = contacts.any(
+        (c) => c.publicKey.toLowerCase() == widget.contact.publicKey.toLowerCase(),
+      );

Optionally, consider removing _isContact() entirely after caching isContact in build to avoid divergent logic paths.

🧹 Nitpick comments (2)
lib/ui/contact_list/start_chat_bottom_sheet.dart (2)

97-102: Avoid side-effects inside setState.

Calling ref.showErrorToast(...) inside setState is unnecessary and can surprise. Do the state updates in setState, then call the toast outside.

-        setState(() {
-          _isLoadingKeyPackage = false;
-          _isLoadingKeyPackageError = true;
-          ref.showErrorToast('Error loading contact: $e');
-        });
+        setState(() {
+          _isLoadingKeyPackage = false;
+          _isLoadingKeyPackageError = true;
+        });
+        ref.showErrorToast('Error loading contact: $e');

294-297: Optional: rely on loading indicator and keep label stable.

Given loading: _isCreatingGroup already shows progress, consider keeping the label constant to avoid text jank during state transitions.

-                          label: _isCreatingGroup ? 'Creating Chat...' : 'Start Chat',
+                          label: 'Start Chat',
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 5bbd0ff and 4d75390.

📒 Files selected for processing (1)
  • lib/ui/contact_list/start_chat_bottom_sheet.dart (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.dart

📄 CodeRabbit Inference Engine (.cursor/rules/flutter.mdc)

**/*.dart: Always declare the type of each variable and function (parameters and return value). Avoid using 'any'. Create necessary types.
Don't leave blank lines within a function.
One export per file.
Use PascalCase for classes.
Use camelCase for variables, functions, and methods.
Use underscores_case for file and directory names.
Use UPPERCASE for environment variables. Avoid magic numbers and define constants.
Start each function with a verb.
Use verbs for boolean variables. Example: isLoading, hasError, canDelete, etc.
Use complete words instead of abbreviations and correct spelling, except for standard and well-known abbreviations (API, URL, i, j, err, ctx, req, res, next).
Write short functions with a single purpose. Less than 20 instructions.
Name functions with a verb and something else. If it returns a boolean, use isX or hasX, canX, etc. If it doesn't return anything, use executeX or saveX, etc.
Avoid nesting blocks by early checks and returns, or extraction to utility functions.
Use higher-order functions (map, filter, reduce, etc.) to avoid function nesting. Use arrow functions for simple functions (less than 3 instructions). Use named functions for non-simple functions.
Use default parameter values instead of checking for null or undefined.
Reduce function parameters using RO-RO: use an object to pass multiple parameters and to return results. Declare necessary types for input arguments and output.
Use a single level of abstraction in functions.
Don't abuse primitive types and encapsulate data in composite types.
Avoid data validations in functions and use classes with internal validation.
Prefer immutability for data. Use readonly for data that doesn't change. Use 'as const' for literals that don't change.
Declare interfaces to define contracts.
Write small classes with a single purpose. Less than 200 instructions, less than 10 public methods, less than 10 properties.
Use exceptions to handle errors you don't expect. If you catch an exception, it sh...

Files:

  • lib/ui/contact_list/start_chat_bottom_sheet.dart
🪛 GitHub Actions: CI
lib/ui/contact_list/start_chat_bottom_sheet.dart

[error] 269-269: avoid_redundant_argument_values: The value of the argument is redundant because it matches the default value.


[error] 285-285: avoid_redundant_argument_values: The value of the argument is redundant because it matches the default value.

Copy link
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

♻️ Duplicate comments (1)
lib/ui/core/ui/wn_filled_button.dart (1)

20-35: Fix nullable icon insertion and loosen icon types to Widget? (compile-time + API flexibility)

  • Fields are nullable but are inserted into a List without null assertion. Because these are instance fields, Dart won't promote them to non-null in the collection-if scope — this can fail to compile.
  • Restricting to SvgPicture unnecessarily couples the button to one icon type; callers often need Icon, Image, or any Widget.

Apply the following changes:

  • Use Widget? for prefix/suffix to accept any widget.
  • Use null assertions when inserting them into children.
   children: [
-    if (prefixIcon != null) ...[
-      prefixIcon,
+    if (prefixIcon != null) ...[
+      prefixIcon!,
       size == WnButtonSize.small ? Gap(8.w) : Gap(12.w),
     ],
@@
-    if (suffixIcon != null) ...[
+    if (suffixIcon != null) ...[
       size == WnButtonSize.small ? Gap(8.w) : Gap(12.w),
-      suffixIcon,
+      suffixIcon!,
     ],
   ],
-  final SvgPicture? prefixIcon;
-  final SvgPicture? suffixIcon;
+  final Widget? prefixIcon;
+  final Widget? suffixIcon;
   final TextStyle? labelTextStyle;

Also applies to: 40-42

🧹 Nitpick comments (3)
lib/ui/chat/chat_info/widgets/group_member_bottom_sheet.dart (1)

275-289: Prefer localized strings and confirm icon a11y intent

  • Consider moving hard-coded labels ("Add to Another Group", "Remove From Group") to AppLocalizations to match project standards for i18n.
  • If the suffix icons are decorative, keep them silent for screen readers (SvgPicture has semanticsLabel; keep it null or consider wrapping with ExcludeSemantics if needed).

If you want, I can prepare a quick patch wiring these strings into AppLocalizations and confirming semantics behavior for the SVGs.

Also applies to: 292-316

lib/ui/core/ui/wn_filled_button.dart (2)

26-30: Avoid overriding the button’s state-driven text color

The FilledButton.styleFrom already sets textStyle with the correct state-driven color via visualState. Passing a concrete TextStyle to Text (size.textStyle()) can override that and desync the label color from hover/disabled states.

Let the ButtonStyle drive defaults and only apply labelTextStyle when explicitly provided.

-                style: labelTextStyle ?? size.textStyle(),
+                style: labelTextStyle,

Also applies to: 68-70


75-80: Optionally disable callbacks while loading for consistent semantics

You already gate interactions with ignorePointer: loading and swap the child with a loading indicator. Disabling callbacks too makes semantics/ink ripple behavior consistent with “disabled” Material state.

-    return FilledButton(
-      style: effectiveStyle,
-      onPressed: onPressed,
-      onLongPress: onLongPress,
-      child: !loading ? child : loadingIndicator,
-    );
+    return FilledButton(
+      style: effectiveStyle,
+      onPressed: loading ? null : onPressed,
+      onLongPress: loading ? null : onLongPress,
+      child: !loading ? child : loadingIndicator,
+    );
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 4d75390 and 7510b12.

📒 Files selected for processing (4)
  • lib/ui/chat/chat_info/widgets/group_member_bottom_sheet.dart (3 hunks)
  • lib/ui/core/ui/wn_filled_button.dart (2 hunks)
  • lib/ui/settings/app_settings/app_settings_screen.dart (3 hunks)
  • lib/ui/settings/general_settings_screen.dart (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • lib/ui/settings/general_settings_screen.dart
  • lib/ui/settings/app_settings/app_settings_screen.dart
🧰 Additional context used
📓 Path-based instructions (1)
**/*.dart

📄 CodeRabbit Inference Engine (.cursor/rules/flutter.mdc)

**/*.dart: Always declare the type of each variable and function (parameters and return value). Avoid using 'any'. Create necessary types.
Don't leave blank lines within a function.
One export per file.
Use PascalCase for classes.
Use camelCase for variables, functions, and methods.
Use underscores_case for file and directory names.
Use UPPERCASE for environment variables. Avoid magic numbers and define constants.
Start each function with a verb.
Use verbs for boolean variables. Example: isLoading, hasError, canDelete, etc.
Use complete words instead of abbreviations and correct spelling, except for standard and well-known abbreviations (API, URL, i, j, err, ctx, req, res, next).
Write short functions with a single purpose. Less than 20 instructions.
Name functions with a verb and something else. If it returns a boolean, use isX or hasX, canX, etc. If it doesn't return anything, use executeX or saveX, etc.
Avoid nesting blocks by early checks and returns, or extraction to utility functions.
Use higher-order functions (map, filter, reduce, etc.) to avoid function nesting. Use arrow functions for simple functions (less than 3 instructions). Use named functions for non-simple functions.
Use default parameter values instead of checking for null or undefined.
Reduce function parameters using RO-RO: use an object to pass multiple parameters and to return results. Declare necessary types for input arguments and output.
Use a single level of abstraction in functions.
Don't abuse primitive types and encapsulate data in composite types.
Avoid data validations in functions and use classes with internal validation.
Prefer immutability for data. Use readonly for data that doesn't change. Use 'as const' for literals that don't change.
Declare interfaces to define contracts.
Write small classes with a single purpose. Less than 200 instructions, less than 10 public methods, less than 10 properties.
Use exceptions to handle errors you don't expect. If you catch an exception, it sh...

Files:

  • lib/ui/chat/chat_info/widgets/group_member_bottom_sheet.dart
  • lib/ui/core/ui/wn_filled_button.dart
🔇 Additional comments (1)
lib/ui/chat/chat_info/widgets/group_member_bottom_sheet.dart (1)

165-166: Migration to label-only API looks correct

The updated WnFilledButton(label: ...) usage for Cancel/Remove actions aligns with the new API and preserves the original behavior.

Also applies to: 173-174

@josefinalliende
Copy link
Contributor

josefinalliende commented Aug 19, 2025

After #518 is merged and after rebase, this checks should pass. Sorry for the inconvenience 😓 @Quwaysim

Copy link
Contributor

@josefinalliende josefinalliende left a comment

Choose a reason for hiding this comment

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

✅ LGTM! Approved!

key: const ValueKey('buttons'),
children: [
WnFilledButton(
size: WnButtonSize.large,
Copy link
Contributor

Choose a reason for hiding this comment

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

Oopps, so just by leaving no size was ok! Good catch!

Copy link
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

🧹 Nitpick comments (4)
lib/ui/chat/chat_info/dm_chat_info.dart (1)

201-201: Localize user-visible button labels

Replace hard-coded strings ('Search Chat', 'Remove Contact'/'Add Contact', 'Add to Group') with AppLocalizations to comply with guidelines and enable i18n.

If you share your l10n keys (or preferred phrasing), I can generate the exact replacements for these labels.

Also applies to: 220-220, 247-247

lib/ui/chat/chat_info/widgets/group_member_bottom_sheet.dart (3)

275-289: Match suffixIcon tint to the button’s secondary visual state

The secondary button variant typically uses secondaryForeground for its content color. Here the suffixIcon uses primary, which diverges from similar usage elsewhere (e.g., DMChatInfo). Consider aligning the tint for consistency.

Apply this minimal diff:

           suffixIcon: SvgPicture.asset(
             AssetsPaths.icChatInvite,
             width: 14.w,
             height: 13.h,
             colorFilter: ColorFilter.mode(
-              context.colors.primary,
+              context.colors.secondaryForeground,
               BlendMode.srcIn,
             ),
           ),

Please confirm the intended spec for secondary buttons’ icon/text color. If the design system expects primary here, disregard this suggestion.


292-316: Avoid manual labelTextStyle override if the variant already encodes warning/destructive styling

If WnButtonVisualState.secondaryWarning is responsible for destructive color/weight, the explicit labelTextStyle override may be redundant and risks diverging from theme updates.

Proposed simplification:

         WnFilledButton(
           onPressed: () async {
             final result = await _openRemoveFromGroupDialog();
             if (result == true && context.mounted) {
               Navigator.pop(context);
             }
           },
           size: WnButtonSize.small,
           visualState: WnButtonVisualState.secondaryWarning,
-          label: 'Remove From Group',
-          labelTextStyle: context.textTheme.bodyMedium?.copyWith(
-            color: context.colors.destructive,
-            fontWeight: FontWeight.w600,
-            fontSize: 14.sp,
-          ),
+          label: 'Remove From Group',
           suffixIcon: SvgPicture.asset(
             AssetsPaths.icRemoveOutlined,
             width: 14.w,
             height: 13.h,
             colorFilter: ColorFilter.mode(
               context.colors.destructive,
               BlendMode.srcIn,
             ),
           ),
         ),

If the variant does not set the text color as desired, keeping the override is fine—just confirm it’s intentional.


166-166: Localize button labels in this sheet

Use AppLocalizations instead of hard-coded strings ('Cancel', both instances of 'Remove From Group', 'Add to Another Group') to follow the project’s localization guideline.

I can draft the l10n ARB entries and replacements if you provide the desired keys/namespaces.

Also applies to: 174-174, 279-279, 301-301

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 7510b12 and 91b5bd8.

📒 Files selected for processing (11)
  • lib/ui/auth_flow/create_profile_screen.dart (1 hunks)
  • lib/ui/auth_flow/login_screen.dart (1 hunks)
  • lib/ui/chat/chat_info/dm_chat_info.dart (3 hunks)
  • lib/ui/chat/chat_info/widgets/group_member_bottom_sheet.dart (3 hunks)
  • lib/ui/chat/chat_management/add_to_group_screen.dart (1 hunks)
  • lib/ui/chat/invite/chat_invite_screen.dart (2 hunks)
  • lib/ui/contact_list/group_welcome_invitation_sheet.dart (2 hunks)
  • lib/ui/contact_list/start_chat_bottom_sheet.dart (1 hunks)
  • lib/ui/settings/network/add_relay_bottom_sheet.dart (1 hunks)
  • lib/ui/settings/profile/edit_profile_screen.dart (4 hunks)
  • lib/ui/settings/profile/share_profile_screen.dart (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
  • lib/ui/settings/network/add_relay_bottom_sheet.dart
  • lib/ui/chat/invite/chat_invite_screen.dart
  • lib/ui/chat/chat_management/add_to_group_screen.dart
  • lib/ui/contact_list/group_welcome_invitation_sheet.dart
  • lib/ui/auth_flow/create_profile_screen.dart
  • lib/ui/settings/profile/share_profile_screen.dart
  • lib/ui/auth_flow/login_screen.dart
  • lib/ui/settings/profile/edit_profile_screen.dart
  • lib/ui/contact_list/start_chat_bottom_sheet.dart
🧰 Additional context used
📓 Path-based instructions (1)
**/*.dart

📄 CodeRabbit Inference Engine (.cursor/rules/flutter.mdc)

**/*.dart: Always declare the type of each variable and function (parameters and return value). Avoid using 'any'. Create necessary types.
Don't leave blank lines within a function.
One export per file.
Use PascalCase for classes.
Use camelCase for variables, functions, and methods.
Use underscores_case for file and directory names.
Use UPPERCASE for environment variables. Avoid magic numbers and define constants.
Start each function with a verb.
Use verbs for boolean variables. Example: isLoading, hasError, canDelete, etc.
Use complete words instead of abbreviations and correct spelling, except for standard and well-known abbreviations (API, URL, i, j, err, ctx, req, res, next).
Write short functions with a single purpose. Less than 20 instructions.
Name functions with a verb and something else. If it returns a boolean, use isX or hasX, canX, etc. If it doesn't return anything, use executeX or saveX, etc.
Avoid nesting blocks by early checks and returns, or extraction to utility functions.
Use higher-order functions (map, filter, reduce, etc.) to avoid function nesting. Use arrow functions for simple functions (less than 3 instructions). Use named functions for non-simple functions.
Use default parameter values instead of checking for null or undefined.
Reduce function parameters using RO-RO: use an object to pass multiple parameters and to return results. Declare necessary types for input arguments and output.
Use a single level of abstraction in functions.
Don't abuse primitive types and encapsulate data in composite types.
Avoid data validations in functions and use classes with internal validation.
Prefer immutability for data. Use readonly for data that doesn't change. Use 'as const' for literals that don't change.
Declare interfaces to define contracts.
Write small classes with a single purpose. Less than 200 instructions, less than 10 public methods, less than 10 properties.
Use exceptions to handle errors you don't expect. If you catch an exception, it sh...

Files:

  • lib/ui/chat/chat_info/widgets/group_member_bottom_sheet.dart
  • lib/ui/chat/chat_info/dm_chat_info.dart
🔇 Additional comments (5)
lib/ui/chat/chat_info/dm_chat_info.dart (3)

198-214: Migration to new WnFilledButton API (Search Chat) looks correct

Clean swap to label + suffixIcon; tinting matches the secondary variant. onPressed flow preserved.


216-242: Add/Remove Contact button refactor is solid

  • Correctly uses loading to disable onPressed while pending.
  • Visual state and suffixIcon color vary with isContact as expected.

244-257: Add to Group migration is consistent and correct

Label/suffixIcon usage aligns with the new API; tinting matches the secondary variant.

lib/ui/chat/chat_info/widgets/group_member_bottom_sheet.dart (2)

165-166: Dialog “Cancel” button migration is correct

Switched to label with secondary visual state; handler unchanged.


173-174: Dialog “Remove From Group” migration is correct

Uses label and preserves loading/disabled state wiring via _isRemoving.

Copy link
Contributor

@josefinalliende josefinalliende left a comment

Choose a reason for hiding this comment

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

✅ Go go go! 🚀

@Quwaysim Quwaysim merged commit f064371 into master Aug 20, 2025
2 checks passed
@Quwaysim Quwaysim deleted the 395-add-icon-position-arg-to-buttons branch August 20, 2025 08:50
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.

Add icon position arg to buttons

4 participants