fix(card): render the share asset's hand crisp, not pixelated#2516
Open
0xkkonrad wants to merge 3 commits into
Open
fix(card): render the share asset's hand crisp, not pixelated#25160xkkonrad wants to merge 3 commits into
0xkkonrad wants to merge 3 commits into
Conversation
The share asset hides nothing — the card number is already "????" and the Visa mark is dropped — so pixelating the hand only made the one image users actually post look low-fidelity. Pixelation belongs to the closed-beta tease, which is exactly what `blurAll` already marks, so the hand now follows that switch instead of being unconditional. Keeps the <img> capture path untouched (the pre-pixelated PNG swaps for the vector source, same element, same box): the peanut logo on this card face is already an <img src=…svg> that survives html-to-image, and a local capture through html-to-image confirms the hand does too — the blank-share-asset bug came from a runtime <canvas>, not from an SVG-sourced <img>.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesCard hand rendering and readiness
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Contributor
Code-analysis diffPainscore total: 6268.09 → 6268.99 (+0.9) 🆕 New findings (10)
✅ Resolved (10)
|
Contributor
🧪 UI test report — ✅ all greenSuites
📊 Coverage (unit)
⏱ 10 slowest test cases
|
The hand stopped being a <canvas> two commits before this branch, but three docblocks still described it as one — and PixelatedCardFace's header still claimed a 620×391 native size that CARD_W/CARD_H outgrew (they're 760×479). Restating layout constants in prose is what let it drift, so the header now points at shareAssetLayout instead of repeating the numbers. Also drops the conditional spread for imageRendering in favour of a plain ternary — same output, one less thing to parse.
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/Card/share-asset/PixelatedCardFace.tsx (1)
302-308: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake
onReadyidempotent.
onLoadtriggers a parent re-render, and React later reattaches the inline callback ref with the same completed image DOM node. SinceonReadyis documented to fire once, guard both theonLoadand cachedimg.completepaths with a component-leveluseRefflag before calling it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Card/share-asset/PixelatedCardFace.tsx` around lines 302 - 308, Make the onReady callback in PixelatedCardFace idempotent by adding a component-level useRef flag that tracks whether readiness has been signaled. Guard both the onLoad handler and the cached-image img.complete path in the ref callback, setting the flag before invoking onReady so re-renders and ref reattachments cannot trigger it more than once.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/components/Card/share-asset/PixelatedCardFace.tsx`:
- Around line 302-308: Make the onReady callback in PixelatedCardFace idempotent
by adding a component-level useRef flag that tracks whether readiness has been
signaled. Guard both the onLoad handler and the cached-image img.complete path
in the ref callback, setting the flag before invoking onReady so re-renders and
ref reattachments cannot trigger it more than once.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 37d9cf2c-9545-474b-aa14-144689aa85d6
📒 Files selected for processing (4)
src/components/Card/share-asset/PixelatedCardFace.tsxsrc/components/Card/share-asset/ShareAssetActions.tsxsrc/components/Card/share-asset/captureShareAsset.tssrc/components/Card/share-asset/shareAsset.types.ts
Missed in the previous sweep — same rot, same reason: the hand has been an <img> since before this branch.
Contributor
Author
Valid observation, declining for this PR. Reasons, in order:
The docblock wording is the actual source of the ambiguity: "fires once the hand No other findings from this review; nothing else open. |
0xkkonrad
marked this pull request as ready for review
July 25, 2026 09:59
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The card share asset rendered its hand through a pre-pixelated PNG, so the one image users actually post looked low-fidelity. Nothing on that asset is hidden — the card number is already
????and the Visa mark is dropped viahideVisa— so the pixelation had nothing left to obscure. It was only costing fidelity.Pixelation belongs to the closed-beta tease, which is exactly what the existing
blurAllprop already marks. The hand now follows that switch instead of being unconditional:ShareAssetD3, noblurAll) → crisppeanut-card-hand.svg,image-rendering: pixelateddropped.blurAll) → pre-pixelated PNG, unchanged:AddCardEntryScreen,CardEligibilityCheckScreen,BadgeSkipCelebration,/shhhhhLP hero.Two files, +23/−17.
Risks / breaking changes
None cross-repo. Frontend-only, no API surface touched.
The one risk worth naming is the capture path.
PixelatedCardFacecarries a comment about the launch-day "blank share asset" bug, so the swap was made to preserve that fix rather than reopen it:<img>. It is not returned to a runtime<canvas>— that was the actual cause of the blank card (canvas.toDataURL()returns empty on iOS Safari for an SVG-sourced canvas), and nothing here reintroduces it.<img>. The peanut logo on this same card face is already<img src=…svg>and surviveshtml-to-imagetoday.html-to-imageatpixelRatio: 2produces a 1560×1000 PNG with the hand fully present (screenshot 3 below).onReadystill fires off the hand'sonLoad/complete, so the Share/Save capture gate is unchanged.QA
/dev/share-builder— the card's hand renders crisp; badges, burst and@usernamepill are untouched./cardeligibility check, add-card entry, badge-skip celebration,/shhhhh— hand still pixelated, whole card still reads as one consistent tease.Local gate:
prettier --check .clean ·typecheckclean ·npm test162/162 suites, 2143 tests ·next buildcompiles.Design notes / accepted trade-offs
Recorded so the next reader doesn't have to re-derive them.
CardFace.tsx— the in-app card — renders this same SVG bottom-right ath-[90%], commented as matching the finalised Rain card art. The share asset renders it top-right at 74% of card width, overflowing the top edge, rotated −15°. While pixelated that read as an abstract blur; crisp, it's a recognisable hand in a placement that disagrees with the canonical art. Reviewed and kept — re-anchoring is a design change, not a cleanup, and belongs in its own PR.PixelatedCardFaceeven though pixelation is now the non-default. A rename touches 5 consumers plus tests; not worth widening a 2-file diff.CardFace.tsxandPixelatedCardFace.tsxare two independent implementations of the same card artwork. This change makes them diverge visually rather than converge. Out of blast radius — follow-up.blurAll→ source mapping. Nothing stops a future refactor from routing the hand back through a<canvas>and silently reviving the blank-share-asset bug, which is the expensive failure mode here. This code doesn't move money or mutate shared state, so it isn't a hard gate — but it is the real coverage gap.Screenshots
ShareAssetD3, after — native 1200×900 at 80%, usernameedcriptofi, 6 badgestop:-40 right:-20 560×471 rotate(-15deg)); only fidelity changes, the hand does not movehtml-to-image— what a user's shared PNG actually is<ShareAssetD3 />component with fixed props, rendered via a throwaway route outside the(mobile-ui)auth gate (deleted; not in this diff). The API harness was not running, so the component was driven directly rather than by a seeded user.PixelatedCardFace's exact markup and geometry in a standalone page, in order to show both hand sources side by side — the pixelated variant cannot otherwise be rendered next to the crisp one.html-to-imagecapture path fromcaptureShareAsset, run against Fetch Chain details from SDK #2's crisp face.Assets branch
pr-assets-2516is deleted post-merge.Summary by CodeRabbit