fix(clock): stop tick-driven re-renders in registration widget - #145
Conversation
|
Warning Review limit reached
More reviews will be available in 56 minutes and 34 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR replaces Redux-based clock management with a provider-based pattern using ClockProvider and useClockSelector. The withWidgetProviders HOC now supplies live clock context to wrapped components, eliminating the need for Redux clock updates. Dependencies on openstack-uicore-foundation and spark-md5 are updated. ChangesClock Provider Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/registration-form/index.js (1)
90-97: 💤 Low valueGuard against partial-null sales window dates in
isTicketCurrentlyAvailable.The current logic treats “open-ended” only when both
sales_start_dateandsales_end_datearenull; if exactly one isnull, the comparison(nowUtc >= tt.sales_start_date && nowUtc <= tt.sales_end_date)can mis-evaluate due to JSnullcoercion. The repo fixtures/tests show the both-nullcase (sales_start_date: null+sales_end_date: null) but no partial-null example, so either handle the partial-null case explicitly or assert upstream that these fields are always paired. (src/components/registration-form/index.js:90-97; e2e/fixtures.js; src/components/ticket-dropdown/tests/ticket-dropdown.test.js)🤖 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/registration-form/index.js` around lines 90 - 97, The isTicketCurrentlyAvailable helper currently only treats both sales_start_date and sales_end_date null as open-ended and then uses a two-sided comparison which misbehaves if one side is null; update isTicketCurrentlyAvailable to explicitly handle partial-null windows: keep the prepaid check (tt.sub_type === TICKET_TYPE_SUBTYPE_PREPAID) and the both-null case, then if sales_start_date is null treat the ticket as available when nowUtc <= tt.sales_end_date, if sales_end_date is null treat it as available when nowUtc >= tt.sales_start_date, otherwise use the existing two-sided check (nowUtc >= tt.sales_start_date && nowUtc <= tt.sales_end_date); reference the function isTicketCurrentlyAvailable and the fields tt.sales_start_date / tt.sales_end_date to locate the change.src/utils/__tests__/withWidgetProviders.test.js (1)
83-85: ⚡ Quick winStrengthen the clock assertion to catch unit regressions.
year >= 2024is too permissive; assert proximity to current epoch seconds instead so ms/sec mismatches fail loudly.Suggested test hardening
- const year = useClockSelector((nowUtc) => - nowUtc ? new Date(nowUtc * 1000).getUTCFullYear() : null - ); - return <div data-testid="year">{year ?? 'null'}</div>; + const nowUtc = useClockSelector((value) => value); + return <div data-testid="now-utc">{nowUtc ?? 'null'}</div>; @@ - expect(Number(getByTestId('year').textContent)).toBeGreaterThanOrEqual(2024); + const renderedNowUtc = Number(getByTestId('now-utc').textContent); + expect(Number.isFinite(renderedNowUtc)).toBe(true); + expect(Math.abs(renderedNowUtc - Math.floor(Date.now() / 1000))).toBeLessThanOrEqual(5);Also applies to: 97-97
🤖 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/utils/__tests__/withWidgetProviders.test.js` around lines 83 - 85, The test currently asserts a loose year check (year >= 2024) for the value returned by useClockSelector; tighten this by asserting the returned epoch seconds (nowUtc) or computed year is close to the current time to catch ms/sec unit regressions: in the test(s) that call useClockSelector (the lines around the useClockSelector call), assert nowUtc is non-null and that Math.abs(nowUtc - Math.floor(Date.now()/1000)) is within a small delta (e.g. 2–5 seconds) or equivalently compute year from Date.now() and assert the computed year equals the selector year; update both occurrences (around lines with useClockSelector at 83 and 97) to use this proximity check.
🤖 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.
Inline comments:
In `@src/utils/withWidgetProviders.js`:
- Line 36: The now prop currently uses Math.round(Date.now() / 1000) which can
advance the epoch-second by ~0.5s; change the conversion to use
Math.floor(Date.now() / 1000) so epoch seconds never jump forward prematurely.
Locate the now={...} assignment in withWidgetProviders.js (the now prop passed
into the widget/provider wrapper) and replace the Math.round call with
Math.floor to ensure floor semantics for sale-window checks.
---
Nitpick comments:
In `@src/components/registration-form/index.js`:
- Around line 90-97: The isTicketCurrentlyAvailable helper currently only treats
both sales_start_date and sales_end_date null as open-ended and then uses a
two-sided comparison which misbehaves if one side is null; update
isTicketCurrentlyAvailable to explicitly handle partial-null windows: keep the
prepaid check (tt.sub_type === TICKET_TYPE_SUBTYPE_PREPAID) and the both-null
case, then if sales_start_date is null treat the ticket as available when nowUtc
<= tt.sales_end_date, if sales_end_date is null treat it as available when
nowUtc >= tt.sales_start_date, otherwise use the existing two-sided check
(nowUtc >= tt.sales_start_date && nowUtc <= tt.sales_end_date); reference the
function isTicketCurrentlyAvailable and the fields tt.sales_start_date /
tt.sales_end_date to locate the change.
In `@src/utils/__tests__/withWidgetProviders.test.js`:
- Around line 83-85: The test currently asserts a loose year check (year >=
2024) for the value returned by useClockSelector; tighten this by asserting the
returned epoch seconds (nowUtc) or computed year is close to the current time to
catch ms/sec unit regressions: in the test(s) that call useClockSelector (the
lines around the useClockSelector call), assert nowUtc is non-null and that
Math.abs(nowUtc - Math.floor(Date.now()/1000)) is within a small delta (e.g. 2–5
seconds) or equivalently compute year from Date.now() and assert the computed
year equals the selector year; update both occurrences (around lines with
useClockSelector at 83 and 97) to use this proximity check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b00b062c-176b-430a-b8e8-6241d725aa7b
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (10)
package.jsonsrc/actions.jssrc/components/purchase-complete/index.jssrc/components/registration-form/__tests__/registration-form.test.jssrc/components/registration-form/index.jssrc/components/registration-modal/__tests__/registration-modal.test.jssrc/components/registration-modal/index.jssrc/reducer.jssrc/utils/__tests__/withWidgetProviders.test.jssrc/utils/withWidgetProviders.js
💤 Files with no reviewable changes (2)
- src/reducer.js
- src/actions.js
The widget's Clock component dispatched UPDATE_CLOCK to Redux every second, so every connected component re-rendered on every tick — including TicketDropdownComponent, which only depends on the filtered list of available tickets. Replace the internal Redux clock with uicore 4.2.31's ClockProvider and switch consumers to useClockSelector so the form only re-renders when the derived value actually changes: - registration-form: allowedTicketTypes recomputes every tick but only commits a new array when a sale window opens/closes (shallowEqual) - purchase-complete: isActive only flips when the summit transitions active/inactive Also drops the UPDATE_CLOCK action, reducer case, and nowUtc state field, and renames withReduxProvider to withWidgetProviders since it now hosts both the Redux Provider and the ClockProvider.
17b110d to
b1b1295
Compare
| <WrappedComponent {...this.props} /> | ||
| <ClockProvider | ||
| timezone={summitData?.time_zone_id || 'UTC'} | ||
| now={Math.round(Date.now() / 1000)} |
uicore 4.2.31's compiled output (notably company-input-v2) imports spark-md5, declared as a peer dep. CI's clean install can't resolve it, so webpack-dev-server emits "Module not found" and the widget renders the error overlay instead of the React tree — every e2e test failed looking for elements the React app never mounted. Locally the dep was hoisted from elsewhere so the issue stayed hidden until rebuilding node_modules from the lockfile.
|
|
||
| // Stub the uicore clock-context so the selector runs once against a fixed timestamp | ||
| jest.mock('openstack-uicore-foundation/lib/components/clock-context', () => ({ | ||
| useClockSelector: (selector) => selector(1000000), |
There was a problem hiding this comment.
The 1000000 seed (≈ Unix epoch 1970-01-12) means isTicketCurrentlyAvailable returns false for every ticket with a real sales window, so allowedTicketTypes is always [] in this suite.
Missing: a test that supplies a ticket fixture with sales_start_date <= 1000000 <= sales_end_date (or bumps the seed to a realistic value and matches the fixture), then asserts the ticket appears in the rendered component. Without it, a bug in the new isTicketCurrentlyAvailable helper — the core of what this PR optimizes — would go undetected.
| // active/inactive transitions (typically zero times per session). | ||
| const isActive = useClockSelector( | ||
| useCallback( | ||
| (nowUtc) => summit.start_date <= nowUtc && summit.end_date >= nowUtc, |
There was a problem hiding this comment.
No __tests__/ directory exists for PurchaseComplete. The isActive value is now derived from useClockSelector, but neither branch is exercised: the active-summit CTA path (isActive === true) and the "event will start on…" path (isActive === false) are both untested.
Missing: mount PurchaseComplete with a mocked useClockSelector that returns a timestamp inside summit.start_date / summit.end_date and assert the active CTA renders; then one where the timestamp is outside the window and assert the "event will start" copy renders.
| apiBaseUrl="https://api.test.com" | ||
| getAccessToken={() => 'token'} | ||
| summitData={{ time_zone_id: 'UTC' }} | ||
| /> |
There was a problem hiding this comment.
This test confirms the clock is live at initial mount. Missing: a test that re-renders Wrapped with a different summitData.time_zone_id prop and asserts useClockSelector still resolves a valid timestamp — covering the WithWidgetProviders PureComponent re-render path where a fresh timezone and now value propagate to ClockProvider. That path is the one flagged for risk in the review.
Per review: Math.floor is the standard truncate-to-second; Math.round could nudge the seed up by ~0.5s on average. Functionally tiny but floor is the convention.
Per review: the 1970-shaped clock seed used everywhere else in the suite makes every realistic sales window evaluate to false, so a bug in the new isTicketCurrentlyAvailable helper would not surface here. Makes the clock seed and TicketTypeComponent's last props settable per test, then adds a regression that mixes in-window, expired, always-open and prepaid fixtures and asserts only the expected ones reach the rendered allowedTicketTypes prop.
Per review: PurchaseComplete had no test directory and the new useClockSelector-derived isActive boolean was unexercised. Adds one test per branch — seed inside summit.start_date/end_date asserts the active CTA path renders, seed outside asserts the "event will start on…" copy renders.
…zone Per review: the existing test only confirms the clock is live at initial mount. Adds a rerender with a different summitData.time_zone_id and asserts useClockSelector still resolves a valid year — exercising the PureComponent re-render that propagates a fresh timezone/now into ClockProvider, the path flagged as the risk in review.
2 participants
ref: https://app.clickup.com/t/86b8dm4da
Summary
The widget's Clock component dispatched UPDATE_CLOCK to Redux every second, causing every connected component to re-render on every tick — including TicketDropdownComponent, which only depends on the filtered list of available tickets.
Replace the internal Redux clock with uicore 4.2.31's ClockProvider and switch consumers to useClockSelector so the form only re-renders when the derived value actually changes.
registration-form:allowedTicketTypesrecomputes every tick but only commits a new array when a sale window opens/closes (shallowEqual)purchase-complete:isActiveonly flips when the summit transitions active/inactiveUPDATE_CLOCKaction, reducer case, andnowUtcstate fieldwithReduxProvider→withWidgetProviderssince it now hosts both the Redux Provider and the ClockProvideropenstack-uicore-foundationto4.2.31(deps + peerDeps)The widget remains self-contained: each instance mounts its own ClockProvider. If the host already provides one, React context shadowing keeps the widget's own active — no conflict.
Summary by CodeRabbit
openstack-uicore-foundationto version 4.2.31spark-md5v3.0.2 as a dependency