fix(card): bulletproof share-asset capture — faithful shadows + deterministic ready gate#2308
Merged
Conversation
…ministic ready gate The card launched today; the share asset users post had two capture bugs. 1) Square shadows. html-to-image renders CSS box-shadow on a rounded element as a SQUARE block, so the captured PNG showed square shadows behind the rounded peanut.me/<user> pill and the card itself. Replace those box-shadows with offset black sibling elements that share the border-radius — they capture as faithful rounded shadows. (Hero stickers use filter:drop-shadow, which captures fine — left as is.) 2) Blank card. PixelatedCardFace paints its pixelated hand into a <canvas> appended asynchronously, so a capture firing before the canvas mounted snapshotted a blank pink card — and the capture SUCCEEDED, so nothing reached Sentry. The bounded waitForAssetReady wait (PR #2302) can time out under load. Deterministic fix: PixelatedCardFace fires onReady once the hand canvas mounts; it threads up through ShareAssetD3 to the Share/Save buttons, which stay disabled until the asset signals ready. waitForAssetReady stays as a belt-and-suspenders fallback. Also wires the /dev/share-builder "Save image" button to the real capture path and adds an e2e regression guard that decodes the captured PNG and asserts the card centre (the hand's territory) is not entirely background — proving the card face actually renders.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 15 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds an ChangesShare asset onReady gating and capture regression test
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
Contributor
Code-analysis diffPainscore total: 5843.8 → 5844.93 (+1.13) 🆕 New findings (29)
…and 9 more. ✅ Resolved (29)
…and 9 more. |
Contributor
🧪 UI test report — ✅ all greenSuites
📊 Coverage (unit)
⏱ 10 slowest test cases
|
CI typecheck couldn't resolve 'sharp' (not a declared dependency). Decode + sample the captured PNG in-page via an <img> + <canvas>.getImageData inside page.evaluate instead — same not-empty assertion, zero new dependencies, so the supply-chain min-release-age gate stays untouched.
![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)
291-302: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winIgnore stale hand-raster completions before firing
onReady.Line 296 starts async work, but Line 302 always unblocks capture even if this
nodehas already been replaced. After the builder rerolls or toggles animation, an olderrasterImg()completion can flipassetReadyback totruebefore the current card has appended its canvas.node.firstChildalso doesn't stop duplicate raster jobs during rerenders before the first append. Guard the callback with an in-flight/still-connected check beforeappendChild()/onReady().Suggested fix
const PixelatedHand: FC<{ onReady?: () => void }> = ({ onReady }) => ( <div ref={(node) => { - if (!node || node.firstChild) return + if (!node || node.firstChild || node.dataset.handRasterState === 'loading') return + node.dataset.handRasterState = 'loading' const handRatio = 560 / 471 // hand display w/h const rasterW = handRatio > 1 ? HAND_RASTER_PX : Math.max(1, Math.round(HAND_RASTER_PX * handRatio)) const rasterH = handRatio > 1 ? Math.max(1, Math.round(HAND_RASTER_PX / handRatio)) : HAND_RASTER_PX rasterImg(ASSET_CARD_HAND, rasterW, rasterH, (canvas) => { + if (!node.isConnected || node.firstChild) return canvas.style.width = '100%' canvas.style.height = '100%' canvas.style.imageRendering = 'pixelated' node.appendChild(canvas) + node.dataset.handRasterState = 'ready' // Card face is now painted — let capture surfaces unblock. onReady?.() }) }}🤖 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 291 - 302, The hand raster callback in PixelatedCardFace can still fire after the original DOM node has been replaced, causing stale completions to call onReady too early. Add a guard in the ref callback and rasterImg completion path to ensure the node is still the current connected element before appending the canvas or unblocking capture. Use the existing ref closure around node and the PixelatedCardFace rasterImg callback to ignore outdated jobs and only call onReady for the active card face.
🤖 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 291-302: The hand raster callback in PixelatedCardFace can still
fire after the original DOM node has been replaced, causing stale completions to
call onReady too early. Add a guard in the ref callback and rasterImg completion
path to ensure the node is still the current connected element before appending
the canvas or unblocking capture. Use the existing ref closure around node and
the PixelatedCardFace rasterImg callback to ignore outdated jobs and only call
onReady for the active card face.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4b9c7a6e-d5db-4cff-afa9-c5698e528359
📒 Files selected for processing (8)
e2e/flows/share-asset-capture.spec.tssrc/app/(mobile-ui)/dev/share-builder/page.tsxsrc/components/Card/BadgeSkipCelebration.tsxsrc/components/Card/CardUnlockDrawer.tsxsrc/components/Card/share-asset/PixelatedCardFace.tsxsrc/components/Card/share-asset/ShareAssetActions.tsxsrc/components/Card/share-asset/ShareAssetD3.tsxsrc/components/Card/share-asset/shareAsset.types.ts
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.
Why
The card launched today (
main= prod). The share asset users capture-and-post had two capture bugs, plus a test gap.1. Square-shadow bug
html-to-imagerenders CSSbox-shadowon a rounded element as a square block, so the captured PNG showed square shadows behind:peanut.me/<user>username pill (rounded-full) inShareAssetD3rounded-3xl) inPixelatedCardFaceFix: replace each
box-shadowwith an offset black sibling element that shares the sameborder-radius, shifted by the shadow distance and layered behind.html-to-imagerenders these faithfully (rounded, not square). The pill's rotation now lives on a wrapper so the shadow tracks the tilt.Hero stickers use
filter: drop-shadow(...)— those capture fine and are left untouched.2. Deterministic blank-card gate
PixelatedCardFacepaints its pixelated hand into a<canvas>appended asynchronously (new Image().onload → appendChild). A capture firing before that canvas mounted snapshotted a blank pink card — and the capture succeeded, so nothing reached Sentry. The boundedwaitForAssetReadywait (#2302) can time out under load → still blank.Fix (deterministic):
PixelatedCardFacefires anonReadycallback once the hand canvas mounts. It threads upScaledPixelatedCardFace/ShareAssetD3/ScaledShareAsset(via the existing prop spreads) toShareAssetActions, which disables Share/Save until the asset signals ready — a capture can never fire before the card face paints.waitForAssetReadystays as a belt-and-suspenders fallback.3. Test surface + regression guard
/dev/share-builder"Save image" button (previously a dummy<Button>with noonClick) to the realcaptureShareAsset+downloadBlobpath, gated on the sameonReadysignal.e2e/flows/share-asset-capture.spec.ts: loads/dev/share-builder, waits for the Save button to enable (proves the ready gate releases only after the canvas mounts), clicks Save, intercepts the downloaded PNG, decodes it withsharp, and samples the centre of the card (the hand's territory — away from the top-left logo and bottom-left number). It fails if that region is entirely background (card-pink#FF90E8/ asset-blue#90A8ED), i.e. the hand never rendered.Files
src/components/Card/share-asset/PixelatedCardFace.tsx— card offset-shadow sibling;onReadyfired fromPixelatedHandafter the canvas appends.src/components/Card/share-asset/ShareAssetD3.tsx— pill offset-shadow sibling; forwardsonReadytoPixelatedCardFace.src/components/Card/share-asset/shareAsset.types.ts—onReady?: () => voidonShareAssetD3Props.src/components/Card/share-asset/ShareAssetActions.tsx—ready?prop gates both buttons.src/components/Card/BadgeSkipCelebration.tsx,CardUnlockDrawer.tsx— trackassetReady, passonReady→ready.src/app/(mobile-ui)/dev/share-builder/page.tsx— wired Save button +onReadygating.e2e/flows/share-asset-capture.spec.ts— new regression guard.Verification
pnpm prettier --check✅ ·npm run typecheck✅ ·npm test✅ (103 suites, 1602 passed, 3 skipped)(mobile-ui)layout redirects unauthenticated users to/setup; the spec needs the authenticated harness, which CI provides) — relies on CI for the browser run.