Skip to content

chore: post-refactor housekeeping + P1 fixes (#59, #71, #72)#75

Merged
variablefate merged 5 commits intomainfrom
chore/post-refactor-housekeeping
Apr 19, 2026
Merged

chore: post-refactor housekeeping + P1 fixes (#59, #71, #72)#75
variablefate merged 5 commits intomainfrom
chore/post-refactor-housekeeping

Conversation

@variablefate
Copy link
Copy Markdown
Owner

Housekeeping + three deferred P1 fixes from the #65/#66/#67 refactor arc. Split into focused commits for easier review.

Scope

Code/docs

P1 fixes

  • 40a2e8b fix: show placeholder in SATS mode when BTC price is unavailable (#72) — Adds formatSatsOrPlaceholder helper in :common/fiat/FiatFareFormatting. Applied to formatFareAmount (RideTabComponents) + formatFareUsd (DriverSelectionScreen), both of which previously fell back silently to $X.XX USD when BitcoinPriceService hadn't populated a price. Verified that HistoryStatsCard.totalSpentDisplay and RideHistoryEntry.formatFareDisplay do NOT have this bug (their SATS branch reads fareSats directly). Unit test pins the contract. Closes bug: formatFareAmount SATS mode silently falls back to USD when price unavailable #72.

  • 3c0a63e perf: reduce recomposition cost in DriverMode offer cards (#71) — Five-part fix:

    1. RelativeTimeText leaf composable isolates the 1Hz ticker from card bodies. Route math, earnings, and findBestCommonFiatMethod no longer re-run per second.
    2. Derived values (pickupDistanceKm, pickupDurationMin, earnings strings, payment-match state) wrapped in keyed remember blocks. Earnings logic factored into EarningsDisplay + computeEarningsDisplay; payment-match into a PaymentMatchDisplay sealed class.
    3. items { } lambdas hoisted via remember(offer, callback) { ... }; items(key = { it.eventId }) added so LazyList stays aligned.
    4. RideOfferData, BroadcastRideOfferData, FiatFare, RouteResult, FollowedDriver annotated @Immutable (pure value types whose List<T> fields cause default-unstable inference).
    5. Same remember(deps) pattern applied to HistoryEntryCard.fareDisplay / pickupDisplay / dropoffDisplay, HistoryStatsCard.totalSpentDisplay, and DriverCard.distanceMiles.

    Not addressed in this PR: Map<String, RouteResult> / List<T> params at the OfferInbox / RoadflareFollowerList boundary remain intrinsically unstable. Fixing that would require adding kotlinx.collections.immutable (new cross-cutting dep) or wrapper @Immutable classes for every collection param — filed as follow-up observation below. Closes perf: Reduce recomposition cost in DriverMode offer cards #71.

  • 035c51c fix: contextual cancel-warning text for fiat/RoadFlare rides (#59)CancelDialogStack now takes a PaymentPath parameter. Cashu HTLC rides (SAME_MINT / CROSS_MINT) keep the existing "driver can still claim the fare / no refund guaranteed" text. Fiat / NO_PAYMENT rides get new text: "Cancel This Ride? / Your driver has been notified and may already be on the way." Trigger condition (preimageShared || pinVerified) unchanged — pinVerified still surfaces the dialog for fiat rides, just with honest language. Closes Cancel warning incorrectly mentions payment claiming for fiat/RoadFlare rides #59.

Follow-up observations

None filed as GitHub issues yet — noting here so they're not lost:

  • Map/List collection stability at component boundaries (OfferInbox, RoadflareFollowerList, FollowedDriverList). Options: add kotlinx.collections.immutable dependency, or define @Immutable wrapper data classes. Gate on whether the Compose profiler shows measurable cost from those parents' recompositions after this PR lands — if the leaf ticker + inner remember blocks are the dominant cost, the collection-stability work may not be needed.

Closes

Test plan

  • ./gradlew :common:testDebugUnitTest passes (adds SatsPlaceholderTest)
  • ./gradlew :rider-app:assembleDebug :drivestr:assembleDebug :roadflare-rider:assembleDebug — all three apps assemble clean
  • Manual: SATS-mode fare surfaces show — sats placeholder on a cold start (before BTC price fetch), not a $X.XX string
  • Manual: Cancel a Cashu ride mid-flight after PIN → "Payment Already Sent" dialog with refund-guarantee language
  • Manual: Cancel a fiat/RoadFlare ride mid-flight after PIN → "Cancel This Ride?" dialog without claim language
  • Manual: Offer inbox with 5+ broadcast requests visible → ticker updates per second without the full card subtree recomposing (inspect via Layout Inspector / Compose recomposition counts)

🤖 Generated with Claude Code

variablefate and others added 5 commits April 18, 2026 08:21
Updates the Quick Implementation Status table for the #65/#66/#67 arc
that landed in PRs #70, #69, #68, #73, and adds two new subsections to
Key Files Reference:

- Coordinators (:common/coordinator/) — catalogs the six extracted
  coordinators, notes the SDK-grade posture, flags them as synthetic
  hotspots warranting extra care on first bug fixes.
- Screen Components (Issue #67) — catalogs the per-module components/
  packages that now own the decomposed Compose screens.

Also annotates the Ride State Management entries to note that ride
protocol logic has moved out of the ViewModels into the coordinators
(accept* methods are now delegates).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
In SATS display mode, `formatFareAmount` (RideTabComponents) and
`formatFareUsd` (DriverSelectionScreen) silently fell back to a `$X.XX`
USD string when `BitcoinPriceService.btcPriceUsd` was null. To the
user — who had explicitly toggled SATS — the fare surface looked
identical to USD mode, so tapping the currency toggle (USD → SATS →
USD) produced no visible change and the click target appeared broken.
Cold starts, relay outages, and network blips all hit this path.

Replaces both fallbacks with `formatSatsOrPlaceholder`, a new helper
in :common/fiat/FiatFareFormatting that returns `"— sats"` (em-dash)
when the sats value is null. The placeholder is visually distinct from
any `$X.XX` formatting — users can see their toggle was applied and
understand conversion is pending instead of mistaking the output for
a dollar amount.

`HistoryStatsCard.totalSpentDisplay` and `RideHistoryEntry.formatFareDisplay`
do not have this bug — their SATS branches read a known-present `fareSats`
field directly and never cross-format into USD.

Closes #72

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses the five named hotspots from Issue #71's third-pass review of #68:

1. **Ticker hoisted to leaf.** `RelativeTimeText(timestampSeconds, ...)` is a new
   private composable that owns the 1Hz ticker's state. `RideOfferCard` and
   `BroadcastRideRequestCard` now render it once in their header; the ticker's
   per-second recomposition is scoped to the `Text` itself instead of cascading
   through the whole card body (route math, earnings, payment-method lookup).

2. **Derived values wrapped in `remember`.** The nullable route unboxing
   (`pickupDistanceKm`, `pickupDurationMin`, etc.), the authoritative-USD-fare
   extraction, the earnings `$/hr` + `$/mi` computations, and the RoadFlare
   payment-method `findBestCommonFiatMethod` scan all now run only when their
   real inputs change. Earnings logic factored into `computeEarningsDisplay()` +
   `EarningsDisplay` so both cards share one `remember` block instead of duplicating
   the keying. Payment-match state factored into a `PaymentMatchDisplay` sealed
   class so the allocation-heavy method lookup runs once per offer/methods change.

3. **`items { }` lambdas hoisted.** Each accept/decline lambda in `OfferInbox` and
   `RoadflareFollowerList` is `remember(offer, callback) { ... }` so child cards
   see the same lambda identity across recompositions. `items(..., key = { it.eventId })`
   added so LazyList stays aligned with underlying event IDs. The fiat-offer
   acceptance guard (Issue #46 interception) factored into `handleOfferAccept()`
   to avoid duplicating the allocation + scan logic across two call sites.

4. **Data classes annotated `@Immutable`.** `RideOfferData`, `BroadcastRideOfferData`,
   `FiatFare`, `RouteResult`, and `FollowedDriver` all carry `List<T>` fields or
   nested lists that cause the Compose compiler to infer unstable-by-default. Each
   is a pure value type (parsed from a Nostr event, never mutated) passed through
   Compose UI, so the annotation is safe and unlocks child skipping for the offer
   cards and follower-list rows once all other inputs stabilise.

5. **HistoryComponents and DriverCard.** Applied the same `remember(deps)` pattern
   to `HistoryEntryCard.fareDisplay` / `pickupDisplay` / `dropoffDisplay`, the
   `HistoryStatsCard.totalSpentDisplay` reducer, and `DriverCard.distanceMiles` —
   these shared the hotspot pattern even though they're not on the 1Hz critical path.

Not fixed in this PR: Map/List parameter stability at the OfferInbox /
RoadflareFollowerList boundary. kotlin.collections.Map is intrinsically unstable;
fixing that requires either adding `kotlinx.collections.immutable` (a cross-cutting
dependency) or defining `@Immutable` wrapper classes for every Map/List param in
the UiState. Out of scope for a single-issue perf pass — filed as a follow-up
observation in the PR body.

Closes #71

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The rider-side cancel warning dialog (shown when cancelling after PIN
verification) always said "Your payment has already been authorized to
the driver. If you cancel now, the driver can still claim the fare."
— but that only applies to Cashu HTLC rides (SAME_MINT / CROSS_MINT
payment paths). Fiat payment rides (Zelle, Venmo, Cash App, cash) and
RoadFlare bitcoin/fiat rides have no escrow for the driver to claim,
so the escrow-claim language was misleading.

`CancelDialogStack` now takes the ride's `PaymentPath` and branches:

- **Cashu HTLC (SAME_MINT / CROSS_MINT):** unchanged — "Payment Already
  Sent / driver can still claim the fare / cancelling does not
  guarantee a refund."
- **Fiat / NO_PAYMENT:** "Cancel This Ride? / Your driver has been
  notified and may already be on the way. Cancelling now may
  inconvenience them." No refund/claim language since there was no
  payment authorization.

Trigger condition (`preimageShared || pinVerified`) is unchanged —
`pinVerified` still surfaces the dialog for fiat rides, just with the
correct text.

Closes #59

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
First-pass review feedback on PR #75. Two fixes bundled:

**1. Stabilize per-offer callbacks at `DriverModeScreen`.**

The #71 changes added `remember(offer, onAcceptOffer) { { onAcceptOffer(offer) } }`
wrappers on child-side lambdas in `OfferInbox` / `RoadflareFollowerList` to
enable `items { }` child skipping. But the parent passed fresh
`{ viewModel.acceptOffer(it) }` lambdas per recompose, so the child `remember`
invalidated every frame — strictly worse than inline lambdas.

Hoist six per-offer/per-broadcast callbacks to `remember(viewModel) { ... }`
at the screen level. `viewModel` is the only key needed (ViewModel instances
outlive recomposition), so these stay identity-stable across frames and the
child-side `remember`s now actually hold. Unblocks the intended child-skip
path in the offer inbox.

**2. Replace `Triple` destructuring with direct `val` assignments in `CancelDialogStack`.**

Positional destructuring (`val (title, body, footer) = ...`) communicates
nothing about what each position means and allocates a `Triple` on every
recomposition of the dialog. Replace with three explicit `val title: String`
/ `val body: String` / `val footer: String?` declarations assigned inside
the if/else — clearer and no allocation.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@variablefate
Copy link
Copy Markdown
Owner Author

Code review — First pass

Scanned the PR with five parallel agents (CLAUDE.md compliance, shallow bug scan, git-history review, prior-PR feedback, code-comment compliance). Surfaced five findings. Using the verify-and-fix filter (improve-codebase / lower-entropy / fix-bug; any yes → advance):

Advanced + fixed in 5e43ae3:

  1. Child-side remember(offer, onAccept) { ... } wrappers defeated by unstable parent lambdas. DriverModeScreen was passing fresh { viewModel.acceptOffer(it) } per recompose at DriverModeScreen.kt#L575-L577 and #L592-L596. That meant the child remembers added in 3c0a63e invalidated every frame — strictly worse than inline lambdas. Fixed by hoisting six per-offer callbacks to remember(viewModel) { ... } before the Crossfade, so child-side remember(offer, onAccept) blocks now hold across recompositions.
  2. Triple destructuring in CancelDialogStack. Positional val (title, body, footer) = Triple(...) is opaque and allocates per recompose. Replaced with three explicit vals assigned inside the if/else.

Verified false-positive (no change needed):

  1. formatFareAmount SATS→USD fallback change — this IS the fix requested by #72 acceptance criteria ("not a $X.XX string"). Not a regression.
  2. computeEarningsDisplay dropped pickupDistanceKm null guard — false positive. pickupDistanceKm and pickupDurationMin both project from the same pickupRoute?, so they're null-equivalent by construction. The ?: 0.0 fallback is dead defensive code; earnings math is unchanged.
  3. @Immutable on RouteResult with List<Maneuver> — verified. Maneuver is a data class with all-val primitive fields, so the contract holds.

Build green on 5e43ae3 (:common:testDebugUnitTest + three :assembleDebug targets).

🤖 Generated with Claude Code

@variablefate variablefate marked this pull request as ready for review April 18, 2026 15:55
@variablefate
Copy link
Copy Markdown
Owner Author

Code review — Pass 2 (convergence)

Pass 2 focused on the 5e43ae3 review-fix commit. No new issues:

  • remember(viewModel) { ... } is an appropriate stabilization key — Hilt ViewModels are NavBackStackEntry-scoped and identity-stable across recomposition.
  • Both CancelDialogStack if/else branches assign all three vals; definite-assignment holds, footer: String? nullability preserved.

Converged. Marking ready-for-review.

🤖 Generated with Claude Code

@variablefate
Copy link
Copy Markdown
Owner Author

Code review — Third pass

Fresh scan against tip 5e43ae3 with five parallel agents (CLAUDE.md compliance, shallow bug scan, git-history review, prior-PR feedback, code-comment compliance).

New findings: none.
Advanced: none.
Verified false / not applicable: n/a — nothing surfaced.
Fixed: n/a.

Specific things spot-checked this pass:

Converged. Keeping PR ready-for-review.

🤖 Generated with Claude Code

@variablefate variablefate merged commit 87706d0 into main Apr 19, 2026
@variablefate variablefate deleted the chore/post-refactor-housekeeping branch April 19, 2026 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant