Repository navigation
fix(frontend): separate Upcoming Scheduled Purchases actions from the savings figure - #1868
Conversation
… savings figure `.upcoming-card` is a flex row laid out with `justify-content: space-between` and no `gap`, and `.upcoming-actions` had no CSS rule at all. Once a realistic plan name fills the row the space-between free space reaches zero: the savings block ends up pixel-adjacent to the button block, "Est. monthly savings" wraps to two lines, and the two buttons stack. Measured in Chromium, the gap between the savings block and the buttons is 0px from 780px to 1024px, and the gap between View Details and Cancel is 0px at every width including 1600px. Text flush against a solid-background button is what the report describes as the buttons rendering on top of the figure. Give the card a `gap`, pin the savings block at its natural width so a long plan name squeezes the info block instead of the figure, and lay the action block out as a flex row with its own gap. The regression spec measures rendered bounding boxes in Chromium rather than DOM structure, because jsdom does not resolve stylesheets into layout and the jest suite stays green while the collision is visible. It fails on the pre-fix stylesheet at 1024px (separation, label wrap) and 1600px (button gap). Closes #1776.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 48 minutes Limit details: You’ve used the included review currently available. Your 73 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. You’re in a promotional period — use the checkbox below to run this review for free:
On-demand reviews are free for the next 30 days. After that, they cost $0.25 per reviewed file. How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
cristim
added
priority/p1
|
@coderabbitai full review The push at 19:30Z hit the review limit before this PR was opened, so this head has never been reviewed. The stated countdown has elapsed. Requesting explicitly because the auto-review on push was consumed by the throttle rather than deferred. |
|
🧠 Learnings used
|
1 participant
Closes #1776.
What the measurements actually show
Reproduced in Chromium against the production webpack bundle, sweeping 1600px down to 320px in 20px
steps, with fixture rows covering a 60-char plan name, the
$0figure from the report,$1,234,567.89, and a short baseline row.There are two defects behind one report, and they have different reach. The savings/actions
collision is width- and content-dependent: it appears from roughly 780px to 1024px with a realistic
plan name, and never on rows with short names. The button/button collision is content-independent
and present at every width including 1600px. The issue says "at all widths", which is true only
of the second one, so a fix aimed at the sentence would have addressed one and left the other.
Correction to the report's stated mechanism
There is no z-order overlap anywhere. Overlapping area is 0 at every width and every fixture
row. Verified per text node with
Range.getClientRects()(text never escapes its own box), andcross-checked that no global
word-break/overflow-wraprule defeats flexbox'smin-width: autoprotection.
What users see is zero separation: bold green text sharing a pixel boundary with a solid-
background button reads as an overlap. Flagging this explicitly because a reviewer comparing the
issue's "render on top of" against a diff that adds gaps would otherwise reasonably think the wrong
thing was fixed. The spec still asserts overlap area is zero in addition to the minimum gap, so a
genuine overlap would also be caught.
Root cause
All three in
frontend/src/styles/plans.css:.upcoming-cardisdisplay: flex; justify-content: space-betweenwith nogap, so oncethe three blocks fill the row the distributed free space reaches zero and they abut.
.upcoming-savingshad no shrink guard, so it was squeezed until "Est. monthly savings" wrappedto two lines, extending the block right to the button edge.
.upcoming-actionshad no CSS rule at all, despitedashboard.ts:497setting the class. Itsbuttons were bare inline-blocks with no separator, touching at every width, and the block shrank
freely and wrapped the buttons into a two-row stack that pushed into the savings text.
The fix
Three declarations, no markup or TypeScript change:
gapfixes the collapse at its source rather than special-casing a width.flex-shrink: 0on thesavings block makes the info block, which holds wrappable prose, absorb the squeeze instead.
display: flexis required forgapto apply to the button block at all. Sizes come from existingspacing tokens.
Ablation, so nothing dead ships. Each declaration removed in turn, rebuilt, spec re-run:
.upcoming-card { gap }.upcoming-savings { flex-shrink: 0 }.upcoming-actions { display: flex; gap }.upcoming-actions { flex-shrink: 0 }(a 4th I had written)The fourth changed nothing even under a dense sweep (1600px to 380px in 20px steps): with the
savings block pinned, the info block absorbs all shrink across the entire range where the card is a
flex row. It guarded an unreachable state, so it was removed before committing.
The
@media (max-width: 768px)rule that setsgap: 1remon.upcoming-cardis untouched. It isnow redundant with the new default (
--cudly-sp-4is 1rem) but remains correct, and removing itwould be a drive-by change.
Regression spec
frontend/tests-e2e/upcoming-purchases-layout.spec.ts, following the #1864 precedent: real bundlein Chromium, measuring
getBoundingClientRect()geometry rather than class names or DOM structure,because jsdom does not resolve stylesheets into layout and the jest suite stays green while the
collision is visible. Each card is measured in a single
evaluateso all boxes come from one layoutpass; measuring locators one at a time produced a false failure when the Home charts reflowed
mid-assertion.
Reverting only
plans.cssto the parent commit, with the committed spec unchanged:Post-fix: 4 passed.
The one spec that passes in both states is a deliberate non-vacuity guard, not a weak test. It
asserts the fixture actually renders two buttons per row, so a fixture regression that dropped the
Cancel buttons could not make the spacing assertions pass trivially.
A vacuous assertion found in this spec and fixed
The label line-count assertion originally used
Element.getClientRects().length. That returns asingle rect for a block box regardless of how many lines the text wraps to, so it passed on the
pre-fix stylesheet and proved nothing. It now uses a
Rangeover the label's contents and failspre-fix as shown above.
Boundary cases
Long savings figure (
$1,234,567.89), zero figure ($0, the value in the report), 60-char planname, 320px (column layout, 16px vertical separation, no horizontal card overflow), 1600px, and the
768px breakpoint with 780px either side of it. Mobile touch targets are unaffected: the 44x44
minimum comes from
@media (pointer: coarse)on the barebuttonselector inresponsive.css,which these buttons already match, and nothing here alters button box sizing.
Verification
Run from
frontend/:npm run buildexit 0;npm testexit 0 (90 suites, 2858 passed, 1 skipped);npm run lintexit 0, 0 errors;npm run typecheckexit 0; full Playwright suite 36 passed, 0failed.
.github/workflows/frontend-e2e.ymlrunsnpm run test:e2e, so the new spec gates on CI.Not verified
docs/ui-ux-review.md, cited as the source in the issue, is not in the repo at any commit, sothere is no screenshot or viewport to match against. The 780-1024px band is derived from this
fixture's plan-name length and shifts with different content; the button/button 0px gap does not.
canCancelUpcomingPurchase()hides Cancel for rows the session didnot create, but the e2e fixture session is an admin whose
admin:*short-circuits the gate.Producing that row needs overriding both
/api/auth/me/permissionsand the group list on/api/auth/me. A one-button action block is strictly narrower than a two-button one and so cannotreintroduce the collision, so all three fixture rows render two buttons.