Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Code-analysis diffPainscore total: 7251.68 → 7248.97 (-2.71) 🆕 New findings (10)
✅ Resolved (15)
📈 Painscore deltas (top movers)
|
🧪 UI test report — ✅ all greenSuites
📊 Coverage (unit)
⏱ 10 slowest test cases
|
![chip-peanut-bot[bot]](https://avatars.githubusercontent.com/in/2915219?s=60&v=4){kind=small}
There was a problem hiding this comment.
Chip review — no blocking findings — this is not an approval
Clean review: the scroll-jack deletion removes the page-freezing listeners and CTA scale state without disturbing the remaining footer visibility or CTA entrance behavior.
Checked clean
- Confirmed the detached worktree HEAD and merge base match the supplied head and base SHAs.
- Traced the removed scroll, wheel, and touch handlers plus body overflow writes; no production remnants of the scroll-jack identifiers or CTA scale custom property remain.
- Verified the SendInSeconds server-rendered slot still resolves to the same section and CTA, with only classless wrapper elements removed.
- Checked Hero CTA transforms and hover/entrance CSS; translate and rotate behavior remains while only the deleted scale input is removed.
- Exact-head required CI is green, including unit, typecheck, eslint, format, ds-lint, native-export, and ci-success; advisory ds-shots and Deploy-Preview were still in progress when checked.
- The focused Jest command could not run locally because this detached worktree has no node_modules; the exact-head unit check completed successfully in CI.
- Reviewed churn and history for the changed landing-page files and found no cross-component consumer that still depends on the removed wrapper, prop, selector, or custom property.
Security review: did not run — this change has no security, privacy or money surface, so it was not asked. This review is one reviewer short.
Third opinion by claude-opus: 0 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: 61d97fd203a2 · Context: repo · Took 8m
🖼 Visual diff — 4 screens moved5 of 66 shots changed · 61 identical · baseline
new screens (1)
job summary · before/after/diff images — artifact Fixture screenshots, no backend. Advisory — this check never blocks a merge. Posted from the default branch by ds-shots-comment.yml; the report it renders is untrusted data. |
There was a problem hiding this comment.
Chip review — no blocking findings — this is not an approval
Clean: the deletion removes the landing-page scroll lock, input interception, and CTA scale plumbing while preserving the independent footer-visibility behavior and the surviving CTA entrance and hover motion. The regression tests exercise the old freeze window and the removed wrapper and scale contracts; completed exact-head checks are green.
Checked clean
- Pinned checkout and merge base: HEAD matches 5f7c801 and its merge base with the supplied head is b7f62ae.
- Correctness and failure paths: traced removal of the body overflow writes, scroll pinning, wheel and touch interception, virtual-delta state, and hero scale prop while confirming the footer-driven hero visibility effect remains independent.
- Layout and animation: reviewed both removed wrapper elements, the SendInSecondsCTA structure, and the CTA transform and keyframe edits; no runtime buttonScale, --cta-scale, sticky-button-target, or scroll-jack state remnants remain outside regression fixtures.
- Regression coverage: the new tests place the legacy target inside the old freeze band, assert body scrolling and wheel and touch defaults remain untouched, and guard the direct slot, section anchor, CTA motion, and scale-property contracts.
- Exact-head CI: completed ci-success, unit, typecheck, eslint, format, native-export, ds-lint, analyze, report, review, human-authors, bot-approval, and related completed checks are green; ds-shots and Deploy Preview were still in progress when checked.
- Security, privacy, and money: the diff deletes client-side presentation behavior and introduces no new trust boundary, data flow, authorization, credential, or amount handling.
- Slop and history: reviewed both commits in the range, checked whitespace and repository-wide remnants, and found no actionable duplication, dead runtime code, misleading abstraction, or architecture drift in the changed behavior.
Security review: did not run — this change has no security, privacy or money surface, so it was not asked. This review is one reviewer short.
Third opinion by claude-opus: 0 finding(s), marked with the model name. It answers only product truth, missing tests and the cross-repo contract, so treat its findings as advice.
Exact head: 5f7c801362a5 · Context: repo · Took 10m
|
Superseded by #3018 (all four folded into one PR per Konrad); branch kept until that merges. |
This branch was successfully deployed
1 participant
Summary
The "Send in seconds" fold froze the page. On the way down, once the CTA reached the sticky-bar band,
LandingPageClientsetdocument.body.style.overflow = 'hidden', pinnedwindow.scrollY, and swallowedwheelandtouchmovewithpreventDefaultuntil the reader had pushed 500px of virtual delta into growing the button to 1.5x. This deletes that machinery. Scrolling is now ordinary scrolling everywhere on the landing page.Part of the PWA-sunset landing work (TASK-21788), split out as its own PR because it is a straight deletion and touches no migration surface.
What changed
LandingPageClient.tsx: removedisScrollFrozen,buttonScale,animationComplete,shrinkingPhase,hasGrown, their mirror refs,frozenScrollY/virtualScrollY/touchStartY,handleScrollDelta, and thescroll/wheel/touchstart/touchmoveeffect with itsdocument.body.style.overflowwrites.buttonVisibleand its footer-visibility effect stay — that is the separate fade-out on the footer.hero.tsx: dropped thebuttonScaleprop, its default, and--cta-scalefromgetCtaStyle. The--cta-x/--cta-y/--cta-rentrance and the hover transform are untouched.sendInSeconds.tsx: removed the<div id="sticky-button-target">wrapper — the only thing that read it was the deleted handler.id="send-in-seconds"on the section stays, and fold 10 keeps its.cta-motion .cta-enterentrance.globals.css: dropped the now-unsettablescale(var(--cta-scale, 1))from.cta-motionand thecta-enterkeyframe (no-op; see Divergences).__tests__/LandingPageClient.scrollJack.test.tsxand__tests__/ctaScrollJackRemnants.test.tsx.How verified
pnpm test -- src/components/LandingPage→ 7 suites, 27 tests, all green (includes the two new suites and the pre-existing Footer / SEOFooter / CurrencySelect ones).#sticky-button-targetwithgetBoundingClientRectstubbed to{top: 600, bottom: 660}, which is inside the band the old handler froze on at jsdom's 768px viewport. Run against the pre-deletion component (copied out ofHEADinto a scratch file), its tests fail: body overflow becomeshidden,wheelandtouchmovecome backdefaultPrevented, and the hero receives abuttonScale. Against this branch all four pass.#sticky-button-targetwas absent from a slot that never contained it, so it could not fail either way. It now asserts the thing this PR actually removed — the anonymous<div ref={sendInSecondsRef}>aroundsendInSecondsSlot— by checking the slot'sparentElementis the render container (LandingPageClientreturns a fragment). Re-adding<div>{sendInSecondsSlot}</div>to the component makes exactly that test fail, and reverting makes it pass again.ctaScrollJackRemnants.test.tsxsplit the fold-10 block in two. Oneitholds only the scroll-jack regression assertions (#send-in-secondspresent,#sticky-button-targetnull,.cta-motionpresent); a second, separately nameditholds the/sendhref and the.cta-enterentrance, so PR 2's "GET THE APP" rewrite of fold 10 can retarget or delete that one without touching the regression guard or reading as a scroll-jack regression.pnpm exec eslint --quieton the six touched files → clean, exit 0.pnpm exec tsc --noEmit→ 6 errors, all pre-existingTS2307in untouched files, none in this diff (see Divergences hotfix: sdk version update #3).pnpm exec prettier --writeon the touched files, then re-run clean.next dev --webpack -p 3053in the worktree + Playwright 1.58.2 chromium, on/and/pt-brat 1440x900 and 390x844 — four combinations, all clean:getComputedStyle(document.body).overflowisvisibleanddocument.body.style.overflowis empty both while fold 10 is centred in the viewport and after scrolling past it; same for<html>.wheel(deltaY 400) andtouchmoveat fold 10 both come backdefaultPrevented === false.mouse.wheelsequence at fold 10 moves the page 1472 / 1860 / 1499 / 2021 px on the four combinations — the page never pins.#sticky-button-targetis absent from the live DOM;scrollWidth - clientWidthis 0 (no horizontal overflow)..cta-enterentrance still fires after the keyframe edit: replaying the class gives a runningCSSAnimationnamedcta-enter, duration 450ms, computed transformmatrix(0.999914, 0.0130896, -0.0130896, 0.999914, 0, 4)— the 0.75deg rotation and 4px lift of thefromframe, resolving correctly now thatscale(var(--cta-scale, 1))is gone..cta-motionelement's centre X equals#send-in-seconds's centre X to 0.00px on all four combinations (720.00 vs 720.00 at 1440, 195.00 vs 195.00 at 390).#418/#423, nopageerrorfrom application code. The only console errors are dev-server infrastructure (webpack-hmrwebsocket handshake failures, andERR_CONNECTION_REFUSEDon the run where earlyoom killed the server mid-pass — that combination was re-run clean afterwards).run/shots-scrolljack/fold10-{en,pt-br}-{1440x900,390x844}.png, raw measurements inrun/shots-scrolljack/report.jsonandreport-ptbr-390.json.Divergences
Also removed the dead
--cta-scalefromglobals.css. The brief scoped the deletion to the components. Oncehero.tsxstops writing the property, nothing insrc,e2e,public,scriptsordocssets it, so.cta-motion'sscale(var(--cta-scale, 1))and the identical call in thecta-enterkeyframe were permanentlyscale(1). Both removed — a no-op at runtime that stops the CSS from carrying a variable no component can set.Removed the
#sticky-button-targetwrapper and the ref wrapper around the send-in-seconds slot. The brief made the first conditional on a clean grep; the grep is clean, and the<div ref={sendInSecondsRef}>inLandingPageClientexisted only to feed the freeze handler. Both were plain class-less block elements around block / inline-block children, so the layout is unchanged.pnpm typecheckis not clean on this worktree, for reasons this PR cannot fix. It reports exactly 6TS2307"Cannot find module" errors and nothing else:web-vitals(src/utils/web-vitals-shim.tsx2, its test),@capacitor/app-launcherand@capgo/capacitor-in-app-review(src/utils/app-review.ts),capacitor-native-settings(src/utils/native-settings.ts). None are in a file this PR touches, and none are of any other error class. This is the documented symlinked-node_modulesworktree artifact — the native-only packages are absent from the shared store the worktree links to. CI installs properly and should be clean.Browser verification proved fold-10 centring geometrically instead of by screenshot diff against
origin/dev. The review asked for a pixel diff of the send-in-seconds fold againstorigin/dev; everything else on that list was run as asked. The diff was replaced by a direct measurement in the same run — section centre X vs.cta-motioncentre X, equal to 0.00px on all four route x width combinations — which answers the question exactly rather than by proxy and does not need a secondnext devonorigin/dev. This box cannot afford one: three sibling TASK-21788 sessions compile in parallel and earlyoom SIGTERM'd the single dev server four times during the pass (next-serverbadness ~1100 at VmRSS 2.1-2.5 GiB). Two host-level tweaks, neither a repo change, were needed to keep one server alive:fs.inotify.max_user_watches92678 -> 524288 (the ENOSPC watcher errors were puttingnext devinto an endless config-changed restart loop) and a temporary 3 GB swapfile at/swap-verify.img, which should be removed once the parallel runs finish.Flag-off impact
The scroll-jack ran in both flag states, so this is the one landing change in the set that is visible with
pwa-sunsetoff: the page no longer freezes at fold 10 and the CTA no longer grows. That is the intent of the ticket. Nothing else about the flag-off page changes — same DOM, same copy, same entrance animations, same sticky mobile bar.