Skip to content

Redesign receive screen#448

Open
dewabisma wants to merge 10 commits intobeast/redesign-home-screenfrom
beast/redesign-receive-screen
Open

Redesign receive screen#448
dewabisma wants to merge 10 commits intobeast/redesign-home-screenfrom
beast/redesign-receive-screen

Conversation

@dewabisma
Copy link
Copy Markdown
Collaborator

Summary

  • Remove receive sheet
  • Created receive screen
  • Update toaster styling
  • Standardize circular loader
  • Created segmented controls component
  • QR code with quantus logo inside it

FWIW, QR code can be scanned, please give it a try from the screenshot to verify again.

Screenshots

  • Receive screen QR tab
Simulator Screenshot - iPhone 8 - 2026-04-13 at 18 45 45
  • Receive screen address tab
Simulator Screenshot - iPhone 8 - 2026-04-13 at 18 45 48
  • Receive screen address tab clicking copy
Simulator Screenshot - iPhone 8 - 2026-04-13 at 18 45 51

It's crazy how we keep adding loader on the fly

Some are still left as is because of specific usage requirements but I changed mostly to use single reusable loader
@dewabisma dewabisma requested a review from n13 April 13, 2026 10:55
@n13
Copy link
Copy Markdown
Collaborator

n13 commented Apr 14, 2026

Here's my review of PR #448Redesign receive screen.


Overall

Solid PR. The new receive screen with tabbed QR/Address views is a nice upgrade over the bottom sheet, the SegmentedControls component is well-built and reusable, and standardizing the Loader across the app is a great cleanup. A few things worth addressing before merge:


Issues

1. DRY: Custom mapIndexed duplicates collection package

segmented_controls.dart adds a private _IterableIndexed extension with mapIndexed, but the codebase already depends on collection: ^1.19.1 and uses its mapIndexed in activity_screen.dart and activity_section.dart:

import 'package:collection/collection.dart';

The custom extension should be dropped in favor of importing from collection.

2. DRY: _buildChecksum is duplicated

QrCodeTab._buildChecksum() and _AddressTabState._buildChecksum() are nearly identical — same FutureBuilder, same addPostFrameCallback side-effect, same loading/empty states. This should be extracted into a shared widget (e.g. _ChecksumFutureBuilder) and used in both tabs.

3. addPostFrameCallback to set parent state during build

Both checksum builders call setChecksum(snapshot.data!) inside addPostFrameCallback during build. This is fragile — it creates a coupling where a child's build triggers a parent setState, which can cause extra rebuild cycles. Consider resolving the checksum in _loadAccountData instead:

Future<void> _loadAccountData() async {
  final account = (await _settingsService.getActiveAccount())!;
  final checksum = await _checksumService.getHumanReadableName(account.account.accountId);
  setState(() {
    _accountId = account.account.accountId;
    _checksum = checksum;
  });
}

Then pass _checksum directly to the tabs instead of a Future + a setter callback. This eliminates the FutureBuilder + postFrameCallback pattern entirely.

4. Hardcoded colors bypass the theme system

Several hardcoded hex colors across the new files:

File Colors
segmented_controls.dart 0xFF191919, 0xFF1C1C1C, 0xFF363636
receive_screen.dart 0xFF3D3D3D, 0xFF0A0A0A, 0xFF0C1C14, 0xFF1A3226

The codebase has AppColorsV2 with proper lerp support. If these are new semantic colors, they should be added to the theme. Otherwise they'll break if a light theme is ever introduced.

5. Hardcoded SizedBox(width: 186) in address tab

In _buildItem, the address text is constrained to exactly 186px wide. This will likely clip or look wrong on smaller devices (e.g. iPhone SE) and waste space on larger ones. Consider using Flexible or Expanded instead.

6. Icon size change in quantus_icon_button.dart is global

The change from iconSize = 20 to iconSize = 16 for non-small buttons affects every QuantusIconButton in the app, not just the receive screen. Worth verifying this doesn't regress other screens (home, settings, etc.).


Minor / Nits

  • Fractional padding EdgeInsets.symmetric(horizontal: 17.73, vertical: 7.09) in the toaster looks like raw Figma values. Rounding to whole pixels (18, 7) avoids sub-pixel rendering artifacts.
  • Force-unwrap (await _settingsService.getActiveAccount())! — will crash if there's no active account. Pre-existing issue from the old sheet, but a good opportunity to add a null check.
  • The assert in the SegmentedControls constructor only fires in debug mode. If you want a runtime guarantee, consider a regular if check or documenting the constraint.

What looks great

  • Generic SegmentedControls<T> with smooth AnimatedPositioned pill — nice reusable component.
  • Loader standardization across 10+ screens is a great cleanup.
  • Toaster improvements (textColor, bottom position, showCopyToaster using checkmark + success color) are polished touches.
  • Clean migration from bottom sheet to full screen with proper navigation.

- remove fragile postFrame
- Add color tokens to theme
- remove duplicate map index implementation
@n13
Copy link
Copy Markdown
Collaborator

n13 commented Apr 17, 2026

Here's my review of PR #448 — Redesign receive screen.

Note: this PR targets beast/redesign-home-screen (not main), so it's the second link in a stack — review scope is just the delta on top of the home-screen branch (~22 files, +541/−258).

Overall

Solid follow-up. Three of the four major issues from the previous review were addressed cleanly in 05b20f4 - feat: resolve review issues, and the standardized Loader rollout is consistent across 9 V2 screens. A few items from last round are still open, plus a handful of new ones to consider before merge.

Resolved since last review

  • addPostFrameCallback during build: gone — checksum now resolved in _loadAccountData and passed as a plain String to both tabs. Much cleaner.
  • Duplicate mapIndexed: the private extension was dropped; package:collection/collection.dart is now imported in segmented_controls.dart. Matches the pattern in activity_screen.dart and activity_section.dart.
  • Hardcoded colors: new theme tokens added — segmentedControlPill, surfaceDeep, copyButtonCopiedBg, copyButtonCopiedBorder, textLabel, plus updated toasterBackground/toasterBorder. All wired through copyWith and lerp. Nice.
  • Duplicate _buildChecksum: no longer applies once the postFrame pattern was removed.

Still open from previous review

1. Hardcoded SizedBox(width: 186) in address tab

  Widget _buildItem(BuildContext context, String label, String value, {isCheckphrase = false, required bool isCopied}) {
    final valueTextStyle = isCheckphrase
        ? context.themeText.smallParagraph?.copyWith(color: context.colors.checksum)
        : context.themeText.smallParagraph?.copyWith(fontFamily: AppTextTheme.fontFamilySecondary);

    return Row(
      mainAxisAlignment: MainAxisAlignment.spaceBetween,
      crossAxisAlignment: CrossAxisAlignment.center,
      children: [
        SizedBox(
          width: 186,
          child: Column(

Still hardcoded to 186 px, which will cause weird wrapping on small phones (iPhone SE) and waste space on iPads. Replace with Expanded and drop the redundant Spacer() (see #4 below).

2. Force-unwrap on getActiveAccount()

(await _settingsService.getActiveAccount())! at line 40 will crash if no active account. The try/catch will catch the TypeError and surface an error toaster, so this works, but the more idiomatic V2 pattern (used by activity_screen.dart, pos_qr_screen.dart, home_screen.dart) is to ref.watch(activeAccountProvider) and let the AsyncValue.when handle loading/error/null cases. Converting ReceiveScreen to a ConsumerStatefulWidget would also remove the bespoke _loadAccountData + setState plumbing entirely.

3. Fractional padding in toaster

      padding: const EdgeInsets.symmetric(horizontal: 17.73, vertical: 7.09),
      ...
          const SizedBox(width: 8.86),

Still raw Figma values. Round to whole pixels (18, 7, 9) — sub-pixel rendering causes blurry borders on lower-DPR screens.

New issues

4. Race condition in _triggerCopied (no Timer cancellation)

  void _triggerCopied({required bool isAddress}) {
    setState(() { ... });
    Future.delayed(const Duration(seconds: 2), () {
      if (mounted) {
        setState(() {
          if (isAddress) {
            _addressCopied = false;
          ...

If the user taps copy twice within 2 s, the first Future.delayed fires and turns the indicator off prematurely (1 s after the second tap). Store a Timer? per item and .cancel() the previous one before scheduling a new one — pos_qr_screen.dart already does exactly this with _startTimer?.cancel() and _timeoutTimer?.cancel().

5. mainAxisAlignment: spaceBetween + Spacer() is redundant

    return Row(
      mainAxisAlignment: MainAxisAlignment.spaceBetween,
      crossAxisAlignment: CrossAxisAlignment.center,
      children: [
        SizedBox(
          width: 186,
          child: Column( ... ),
        ),

        const Spacer(),

        _copyButton(isCopied: isCopied),
      ],
    );

Once Spacer() consumes all remaining space, spaceBetween does nothing. Pick one. Combined with #1, the right shape is Expanded(child: Column(...)) + drop both spaceBetween and Spacer.

6. Missing type annotation on isCheckphrase

  Widget _buildItem(BuildContext context, String label, String value, {isCheckphrase = false, required bool isCopied}) {

isCheckphrase defaults to dynamic. Should be bool isCheckphrase = false — the analyzer usually flags this with strict-inference.

7. DRY: Share button duplicated between QR and Address tabs

Both tabs end with the same QuantusButton.simple for Share with identical padding/icon/iconPlacement and a trailing SizedBox(height: 24). Extract a _ShareButton({required VoidCallback onTap}) (or just a Widget _shareButton(...) helper) and reuse. Per the project's DRY rule this is worth doing while the file is still fresh.

8. DRY: button padding constant

padding: const EdgeInsets.symmetric(vertical: 20, horizontal: 16) appears 3× in this file (Copy, two Share buttons). Hoist to a single const _kActionButtonPadding.

9. DRY: 267 magic number

          Container(
            ...
            width: 267,
            height: 267,
            child: QrImageView(
              ...
              size: 267,

Three appearances of 267, and 64 for the embedded Q. If anyone changes one and not the others the QR will break. Lift to local consts (_kQrSize, _kQrLogoSize).

10. else if (_selectedTab == ReceiveTab.address) should be else

          if (_accountId == null || _checksum == null)
            const Expanded(child: Center(child: Loader()))
          else if (_selectedTab == ReceiveTab.qrCode)
            QrCodeTab(accountId: _accountId!, onShare: _share, checksum: _checksum!)
          else if (_selectedTab == ReceiveTab.address)
            AddressTab(accountId: _accountId!, onShare: _share, checksum: _checksum!),

ReceiveTab only has two values, so the second else if is dead-defensive — and worse, if the enum ever gains a third value the screen silently renders nothing. Make it a plain else so the compiler/exhaustiveness check stays meaningful.

11. Loader regresses tablet sizing

class Loader extends StatelessWidget {
  final Color? color;
  const Loader({super.key, this.color});

  @override
  Widget build(BuildContext context) {
    return SizedBox(
      height: 16,
      width: 16,
      child: CircularProgressIndicator(strokeWidth: 2, color: color ?? context.colors.textSecondary),
    );
  }
}

The standardization is great, but Toaster and the rest of the V2 system already adapt with context.isTablet ? X : Y. The previous pos_qr_screen "Waiting for payment" loader was 18 × 18, full-screen auth_wrapper loader was a default CircularProgressIndicator (~36 × 36). They're now all 16 × 16, which will look small on iPad and especially small as the centered loader on the home/auth/activity screens.

Suggest two sizes (or a size param), e.g.:

class Loader extends StatelessWidget {
  final Color? color;
  final double? size;
  const Loader({super.key, this.color, this.size});

  @override
  Widget build(BuildContext context) {
    final s = size ?? (context.isTablet ? 22 : 16);
    return SizedBox(
      height: s, width: s,
      child: CircularProgressIndicator(strokeWidth: 2, color: color ?? context.colors.textSecondary),
    );
  }
}

Then full-screen call sites can use Loader(size: 28).

12. textLabel (0xFF787878) sits very close to textMuted (0xFF888888)

Only 16/256 grey-step apart. Worth checking with the designer whether these are actually two semantic tokens or one — keeping near-duplicate tokens makes the system harder to maintain. If they really are different, a comment on textLabel explaining its intended use vs textMuted would help.

Nits

  • InkWell wrapping _buildItem lives inside a Container with borderRadius: 24 but no Material/ClipRRect — splash will leak past the rounded corners. Use Material(color: Colors.transparent, child: InkWell(borderRadius: BorderRadius.circular(24), ...)) or wrap each row's own rounded clip.
  • assert(items.length >= 2, ...) in SegmentedControls only fires in debug. If you really want the runtime guarantee, throw — otherwise it's just docs.
  • home_screen.dart _circleIconButton cleanup (dropping the colors parameter) is a nice tidy-up but technically unrelated to the receive redesign.

What looks great

  • Generic SegmentedControls<T> with the smooth AnimatedPositioned pill and AnimatedDefaultTextStyle is a polished, reusable component.
  • Loader rollout removes nine ad-hoc CircularProgressIndicator usages — exactly the cleanup the codebase needed.
  • New theme tokens were added with full copyWith + lerp plumbing — easy to extend later.
  • The "tap-to-copy with check-mark feedback" pattern in AddressTab is a nice UX upgrade over the old sheet's separate copy buttons.

Overall happy to approve once #1, #2, #4, #6, #10 are sorted — the rest are polish/DRY items that can land in this PR or a follow-up at the team's call.

Related context: babysit PR review approach

@n13
Copy link
Copy Markdown
Collaborator

n13 commented Apr 20, 2026

Re-review of PR #448 — Redesign receive screen

3 new commits since my last review (two merges + 7b184ec "feat: resolve PR review issues"). Ran the same checks — CI format + code read.

Previous review follow-up — mostly resolved

# Issue Status
1 Hardcoded SizedBox(width: 186) Fixed → Expanded + SizedBox(width: 32) separator
2 Force-unwrap on getActiveAccount()! Still open (see below)
3 Fractional padding (17.73 / 7.09 / 8.86) Fixed → 18 / 7 / 9
4 Race condition in _triggerCopied Fixed → Timer? _resetTimer + .cancel() + dispose()
5 spaceBetween + Spacer() redundancy Fixed
6 Missing bool type annotation on isCheckphrase Fixed
7 DRY: Share button duplicated Fixed → extracted _ShareButton
8 DRY: button padding constant Fixed → _actionButtonPadding top-level const
9 DRY: 267 magic number Partially fixed — qrSize = 267.0 used in 3 places, but Size(64, 64) for the embedded Q logo is still raw
10 else if dead-defensive Fixed → plain else
11 Loader regresses tablet sizing Fixed — added size param with context.isTablet ? 24 : 16 default
12 textLabel vs textMuted duplication Not addressed (minor / design call)

The _ShareButton + _actionButtonPadding extraction and the Timer fix are particularly clean.

Still open

1. Force-unwrap on getActiveAccount()! — not addressed

final account = (await _settingsService.getActiveAccount())!;

The surrounding try/catch now catches the TypeError and surfaces a toaster, so this works — but the more idiomatic V2 pattern (ref.watch(activeAccountProvider) with AsyncValue.when) is still worth doing. Not a blocker though.

2. CI formatting failures — blocker for eventual merge to main

dart format . --line-length=120 --set-exit-if-changed (the exact CI command from .github/workflows/ci.yaml) reports 4 files changed:

  • mobile-app/lib/v2/components/toaster.dart
  • mobile-app/lib/v2/screens/accounts/edit_account_view.dart
  • mobile-app/lib/v2/screens/home/home_screen.dart
  • mobile-app/lib/v2/screens/settings/settings_screen.dart

CI only runs on PRs targeting main/master, so this PR's stack-internal CI won't flag them, but they'll block the merge of the v3 stack into main. Easy fix: melos exec -- dart format . --line-length=120 (what CI runs).

3. Embedded QR logo size Size(64, 64) still a magic number

Small residual DRY issue — if the logo ever needs resizing in relation to qrSize, it'd help to have _kQrLogoSize = 64.0 (or compute it as a fraction).

Nits

  • InkWell inside _buildItem's rounded Container still has no Material/ClipRRect — splash can leak past corners on tap. Low priority.

Verdict

Not approving yet — blocked on CI format. The follow-up is otherwise solid: 9 of 12 flagged items cleanly resolved, the Timer race fix is exactly right, and the _ShareButton / padding-constant extraction show good DRY discipline. Once dart format is run on the 4 files above, I'd approve. The force-unwrap and embedded-logo-size items are non-blocking and can follow in a later PR.

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.

2 participants