Skip to content

fix(paywalls): clamp negative margins/padding to zero#3559

Merged
facumenzella merged 3 commits into
mainfrom
facundo/clamp-negative-margins
Jun 11, 2026
Merged

fix(paywalls): clamp negative margins/padding to zero#3559
facumenzella merged 3 commits into
mainfrom
facundo/clamp-negative-margins

Conversation

@facumenzella

@facumenzella facumenzella commented Jun 8, 2026

Copy link
Copy Markdown
Member

Description

Android does not support negative padding, and the RevenueCatUI SDK applies component margins via Compose's Modifier.padding(...) as well. The Paywalls dashboard can send negative top / bottom / leading / trailing values, which render incorrectly on Android.

This clamps negative values to 0 at the single conversion choke point, Padding.toPaddingValues(), so both margins and padding are protected. Because clamping happens before the value is stored in component styles/state, any downstream consumers that compute from the resulting PaddingValues (e.g. ButtonComponentView's margin.calculateTopPadding()) get clamped values for free.

When clamping occurs, a warning is logged so the dashboard misconfiguration is visible. The warning is opt-in via a warnIfNegative flag that only StyleFactory passes (once per paywall build); the conversion itself is pure, so hot recompute paths (derivedStateOf) clamp silently and never spam the same warning on tab/package/size/variable changes.

Why this layer

  • toPaddingValues() is the one place every margin and padding flows through (both StyleFactory base styles and per-component partial overrides).
  • Not clamping in the Padding model itself: that is the serialized source of truth, and clamping there would corrupt round-trips.

Scope note

The ask was about margins specifically, but this choke point covers both margins and padding. There are zero intentional negative padding/margin values anywhere in source or JSON fixtures (only 0 and positives), and negative padding is unsupported on Android regardless, so clamping both is correct behavior rather than scope creep, and avoids a future footgun where a new margin call site forgets to clamp.

Tests

  • Added PaddingTest next to the conversion (the existing PaddingTests lives in :purchases and tests the model, not the conversion). Covers positive pass-through, all-negative clamping, mixed, and clamping with warnIfNegative enabled. Runs under Robolectric so android.util.Log (used by the warning) is available.
  • Added a StackComponentView preview with negative padding and margin so the clamped rendering is visually validated via Emerge snapshots.
AI session context

AI Context

Metadata

  • PR: 3559
  • Branch: facundo/clamp-negative-margins
  • Author / human owner: facumenzella
  • Agent(s): Claude Code (Opus 4.8, 1M context)
  • Session source: current conversation
  • Generated: 2026-06-10
  • Context document version: 3

Goal

Clamp negative spacing values sent by the Paywalls dashboard to zero on Android, since Android does not support negative padding and margins are applied via Modifier.padding(...). Focus on margins.

Initial Prompt

"Apparently we don't support negative padding in android, but the dashboard can send it. Can we clamp those? Margins specifically."

Important Follow-up Prompts

  • "open a draft and let's take a look" — open a draft PR.
  • "let's add the debug log" + "and let's add the preview" — address reviewer suggestions.
  • Pointed at Cursor Bugbot comment re: repeated warnings on state refresh — gate the warning so it only fires once at build time, not on recompose.

Agent Contribution

  • Located the conversion choke point Padding.toPaddingValues() and mapped all margin/padding call sites (StyleFactory once-per-build, per-component derivedStateOf recompute paths, view application sites).
  • Verified no intentional negative padding/margin exists in Kotlin source or JSON fixtures.
  • Implemented clamping via coerceAtLeast(0.0) on all four axes (unconditional).
  • Added an opt-in warnIfNegative flag; only StyleFactory's 17 conversion sites pass it, so the warning fires once per paywall build and the derivedStateOf paths stay silent.
  • Wrote unit tests (PaddingTest) under Robolectric; added a StackComponentView negative-padding preview.
  • Ran unit tests + detekt (passing), rebased onto main, replied to all review threads.

Human Decisions

  • Directed scope toward margins; agent confirmed (with evidence) that clamping at the shared conversion covers both safely.
  • Reviewer (tonidero) approved and suggested logging + a negative-padding preview. Cursor Bugbot flagged repeated warnings on recompose. All addressed.

Key Implementation Decisions

  • Decision: Clamp negatives in Padding.toPaddingValues(); gate the warning behind an opt-in warnIfNegative flag used only by StyleFactory.
    • Rationale: Single choke point for clamping; logging only at the once-per-build ingest point avoids re-emitting the warning when partial overrides recompute in derivedStateOf.
    • Rejected: Logging unconditionally inside toPaddingValues() (spams on state changes, per Cursor); clamping in the Padding model (corrupts serialized round-trips); clamping at the ~20 .padding(...) application sites (footgun); a process-wide dedupe set (mutable global state).

Files / Symbols Touched

  • ui/revenuecatui/.../components/ktx/Padding.kt
    • Why: clamp negatives (always) + opt-in warning
    • Symbols: Padding.toPaddingValues(warnIfNegative: Boolean = false)
    • Review relevance: clamping is unconditional; warnIfNegative only gates the log
  • ui/revenuecatui/.../components/style/StyleFactory.kt
    • Why: pass warnIfNegative = true at the 17 once-per-build conversion sites
    • Review relevance: confirm only build-time sites opt in (not derivedStateOf)
  • ui/revenuecatui/.../components/ktx/PaddingTest.kt
    • Why: positive, all-negative, mixed, and warnIfNegative coverage; Robolectric for Log
    • Symbols: PaddingTest
  • ui/revenuecatui/.../components/stack/StackComponentView.kt
    • Why: preview validating clamped negative padding/margin
    • Symbols: StackComponentView_Preview_NegativePaddingAndMarginClamped

Dependencies / Config / Migrations

  • None.

Validation

  • Commands run:
    • ./gradlew :ui:revenuecatui:testDefaultsBc8DebugUnitTest --tests "...ktx.PaddingTest": BUILD SUCCESSFUL
    • detekt (pre-commit): passed
  • Manual verification:
    • Not run (no on-device rendering performed locally).
  • CI:
    • Emerge snapshot: 1 added (the new preview), 0 modified, 0 errored — confirms no existing snapshot changed. New snapshot needs approval in the Emerge dashboard.
    • Other CI green on prior commit; re-running on the latest commit.

Validation Gaps

  • Local Paparazzi verification unavailable: ui/revenuecatui/src/test/snapshots is git-ignored/absent in this checkout; snapshots run on CI / the snapshots repo.
  • The opt-in warning does not fire for negative values that come only from a partial/conditional override (those convert on the recompute path, which intentionally stays silent). Base component padding/margin still logs once at build.

Review Focus

  • Confirm clamping both margins and padding (vs margins-only) is acceptable.
  • Confirm only StyleFactory (build-time) opts into warnIfNegative, not derivedStateOf.
  • Review the Emerge snapshot of the new preview to confirm clamped rendering looks correct, then approve it.

Risks / Reviewer Notes

  • Risk: clamping padding (not just margin) could change rendering if a paywall intentionally used negative padding.
    • Evidence: grep of source and JSON fixtures found only 0/positive values; negative padding is unsupported on Android.
    • Mitigation: CI Emerge/Paparazzi snapshots surface any visual change (0 modified observed).

Non-goals / Out of Scope

  • No change to the serialized Padding model or backend contract.
  • No iOS changes.

Omitted Context

  • Raw transcript, unrelated exploration, sensitive details, repetitive attempts, and chain-of-thought-style content were omitted.

@facumenzella facumenzella changed the title fix: clamp negative margins/padding to zero fix(paywalls): clamp negative margins/padding to zero Jun 8, 2026
@codecov

codecov Bot commented Jun 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.35%. Comparing base (0f2ad15) to head (0279786).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3559   +/-   ##
=======================================
  Coverage   80.35%   80.35%           
=======================================
  Files         377      377           
  Lines       15417    15417           
  Branches     2140     2140           
=======================================
  Hits        12389    12389           
  Misses       2171     2171           
  Partials      857      857           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@facumenzella
facumenzella marked this pull request as ready for review June 8, 2026 16:43
@facumenzella
facumenzella requested a review from a team as a code owner June 8, 2026 16:43

@tonidero tonidero left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A couple of suggestions, but I think this is ok!

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 7b790375df90b639e6e74334b68e8197c68ebeb7. Configure here.

@emerge-tools

emerge-tools Bot commented Jun 9, 2026

Copy link
Copy Markdown

📸 Snapshot Test

1 added, 591 unchanged

Name Added Removed Modified Renamed Unchanged Errored Approval
TestPurchasesUIAndroidCompatibility Paparazzi
com.revenuecat.testpurchasesuiandroidcompatibility.paparazzi
0 0 0 0 257 0 N/A
TestPurchasesUIAndroidCompatibility
com.revenuecat.testpurchasesuiandroidcompatibility
1 0 0 0 334 0 ✅ Approved

🛸 Powered by Emerge Tools

facumenzella and others added 3 commits June 10, 2026 17:29
Android does not support negative padding, and the SDK applies margins via
Compose's Modifier.padding(...) too. When the dashboard sends negative
top/bottom/leading/trailing values, they render incorrectly.

Clamp negatives to zero at the single conversion choke point
(Padding.toPaddingValues), so both margins and padding are protected and any
downstream consumers that compute from the resulting PaddingValues get clamped
values for free.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses review feedback:
- Log a warning in Padding.toPaddingValues when negative values are received,
  to surface the dashboard misconfiguration.
- Add a StackComponentView preview with negative padding/margin so the clamped
  rendering is visually validated (Emerge snapshots @Preview functions).

PaddingTest now runs under Robolectric so android.util.Log is available.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses Cursor Bugbot: `toPaddingValues` is also called from `derivedStateOf`
blocks, so logging inside it re-emitted the same warning whenever a partial
override recomputed (tab/package/window-size/variable changes), even though the
dashboard values had not changed.

Gate the warning behind an opt-in `warnIfNegative` flag that only StyleFactory
passes (once per paywall build). Clamping still happens unconditionally on every
call, so the recompute path stays correct and silent.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@facumenzella
facumenzella force-pushed the facundo/clamp-negative-margins branch from 7b79037 to 0279786 Compare June 10, 2026 15:30
@facumenzella
facumenzella added this pull request to the merge queue Jun 11, 2026
Merged via the queue into main with commit 7f895a3 Jun 11, 2026
37 checks passed
@facumenzella
facumenzella deleted the facundo/clamp-negative-margins branch June 11, 2026 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants