test(website): keep upcoming-events coverage when nothing is scheduled - #15160
Conversation
The upcoming list is derived from the wall clock, so it empties out once the newest configured event ends. Every assertion about those rows then scaled to zero and the section's behaviour silently stopped being tested. Cover the rows in a component test that stubs the derived list, so row markup, localization, and link targets stay verified whatever the date. Keep the e2e clock-honest and have it assert the list container renders even with no rows. Also correct the comment on the derivation: the events islands re-evaluate it in the browser on hydration, so classification is not fixed at build time.
The list is re-derived on hydration, so an event drops out on the first page load after it ends -- not the instant it ends.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughEvent classification now uses a module-load timestamp during server rendering and browser hydration. Upcoming-event tests cover rendered links, localization, security attributes, and empty configured event lists. ChangesEvent behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: ⚪ Minimal · up to This PR strengthens upcoming-events test coverage without changing production behavior, and no actionable merge-blocking risk remains after normal checks and review. Possibly related PRs
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
🌐 Website E2ETip All tests passed.
🔗 Website PreviewWebsite Preview: https://comfy-website-preview-pr-15160.vercel.app This commit: https://website-frontend-rdlix8sfw-comfyui.vercel.app Last updated: 2026-08-12T22:45:13Z for |
![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
🤖 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 `@apps/website/src/templates/events/UpcomingEventsSection.test.ts`:
- Around line 42-45: Extend the tests around the mocked upcomingEvents data to
cover an empty list, rendering the relevant section with no events and asserting
that the list remains attached. Keep the existing two-event coverage unchanged
and use the existing render/query helpers and list selector.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a84e0dea-7df7-457f-8c79-d91b8a7f1121
📒 Files selected for processing (3)
apps/website/e2e/events.spec.tsapps/website/src/data/events.tsapps/website/src/templates/events/UpcomingEventsSection.test.ts
Read the stubbed list through a getter so a test can empty it, and assert the list element survives an empty derivation. That is the state the live page is in whenever the schedule runs dry, and the e2e only reaches it by accident of the calendar.
dosubot
Bot
added
the
size:S
github-actions
Bot
added
the
risk:R2
mobeenabdullah
left a comment
mobeenabdullah
left a comment
There was a problem hiding this comment.
LGTM. Tests are behavioral, not change-detectors: they assert the link branch (streamed to /events/{id} with no target, external outbound with rel=noopener noreferrer), that the list survives an empty derivation, and zh-CN localization including asserting the English string is absent. vi.mock + importOriginal with a getter stubs only upcomingEvents and leaves the rest real, and satisfies ComfyEvent keeps the fixtures from drifting. Vitest picks up the new path and the happy-dom docblock overrides the node default. The events.ts change is a rename plus a comment fix, and the corrected comment is right: hydration re-derives, so classification is not build-fixed.
One thing: finding #1 in your description is a real user-facing bug (post-hydration content shift plus Vue hydration warnings on /events), and there's no open issue for it. PR bodies disappear into the squash commit, so please file it before merge.
3 participants
PR Created by the Glary-Bot Agent
Summary
The "Upcoming events" list on
/eventsis derived from the wall clock, so it empties out once the newest configured event ends — which silently reduced every assertion about those rows to a no-op. This restores that coverage in a form that does not depend on the calendar.Changes
UpcomingEventsSection.test.tsstubs the derivedupcomingEventslist (vi.mock+importOriginal) and covers what the e2e can no longer see: one row per event, localized title/blurb/location/date, and the streamed-vs-external link branch (/events/{id}vs the external href withtarget=_blank+rel), including the zh-CN event-page href.events.spec.tsnow asserts the list container renders even when there are no rows, so a zero-row page is still a real assertion rather than a vacuoustoHaveCount(0).BUILD_NOW→NOW): the events islands re-evaluate it in the browser on hydration, so classification is not fixed at build time as the old comment claimed.Review Focus
Why prod is empty is not a bug in the fetch/render logic. The data is not remote — it is the hard-coded array in
src/data/events.ts. The only dynamic input is the clock:deriveUpcomingEventskeeps events whose end (start + 1h default) is still in the future. The newest configured event,video-model-showdown, ended2026-08-12T18:00Z, and nothing is scheduled after it, soupcomingEventsis[]and theulrenders as<!--[--><!--]-->. A locally builtdist/events/index.htmlreproduces the reported prod markup byte-for-byte. The page needs a content refresh, not a code fix.Two findings worth separate follow-ups (deliberately not changed here):
<UpcomingEventsSection client:visible />ships the full events array plusnew Date()to the client, so the upcoming/past split is re-derived from the visitor's clock at hydration. Freezing the browser clock to2026-08-01against the current build makes 2 rows appear where the build rendered 0, along with a burst of VueHydration text/children mismatchwarnings — i.e. real visitors on a build older than an event's end get a visible post-hydration content shift on/events. Making it deterministic per-deploy is a behaviour change (a finished livestream would linger as "upcoming" until the next deploy), so it needs a product call.Follows up #15150, which stopped the CI failure by guarding the one unguarded test; this addresses the coverage hole that guard left behind.
Screenshots
Both taken against a local production build. The first is today's real clock (reproduces prod); the second freezes the browser clock to
2026-08-01, showing the render logic itself is healthy.Verification
pnpm test:unit(website): 42 files, 399 tests passing, including the 3 new ones.pnpm typecheck(astro check): 0 errors, 0 warnings.--type-aware, and eslint clean on the changed files (also via pre-commit hooks).playwright test events.spec.ts --project=desktop --project=mobile: 7 passed, 2 skipped (the two time-dependent tests, correctly skipped with zero upcoming events).ulwith adivfails the new e2e assertion — which previously passed silently.Screenshots