fix: consistent param validation, index hygiene, async cleanup, a11y, and test coverage - #334
Conversation
… and test coverage for contest module
- Added validateParams middleware + contestIdParamSchema, applied to
DELETE /reminders/:contestId and POST /reminders/:contestId/notified.
Both now enforce the same positive-integer contract as POST
/reminders instead of a manual parseInt(...) that permissively
accepted strings like '12abc'.
- Removed the redundant standalone contestId index on Contest — the
compound unique {platform, contestId} index already covers every
query pattern in the codebase. Documented the rationale in
server/modules/contests/INDEXES.md and added a migration script
(dropRedundantContestIndex.js) since a schema change alone never
drops an existing production index.
- useContests now guards against stale/superseded async responses via
a per-fetch sequence token, and invalidates in-flight requests on
unmount, matching the cancellation pattern already used elsewhere
(bell, notifier).
- ContestReminderBell now has a dynamic aria-label (including the
due-soon count) instead of relying on title alone, and marks its
decorative icon/badge aria-hidden.
- Added 28 new tests (46 backend total, 28 frontend total) covering
the validation contract, reminder lifecycle edge cases, repository
idempotency/joins/cleanup, the stale-response race condition, and
bell accessibility (accessible name, count, keyboard focus) — using
the project's existing test tooling (node:test, vitest), no new
testing stack introduced.
Closes kunalverma2512#279
|
@ida-jemi is attempting to deploy a commit to the Kunal Verma's projects Team on Vercel. A member of the Team first needs to authorize it. |
🎉 Welcome to CodeLens — Thank You for Your Contribution!Hey @ida-jemi! 👋 We are genuinely excited to have you here. Every single PR — big or small — makes CodeLens better, and yours is no exception. Take a moment to review the checklist below to help us merge your work quickly and smoothly. ✅ Before Requesting a Review
💬 Join Our Community Channel — This is MandatoryBeing part of our communication channel is compulsory for all contributors, not optional. Why join? This is where all important announcements, PR review updates, contribution discussions, and maintainer decisions happen in real time. Contributors who are not in the channel regularly miss critical context and updates, which often leads to duplicated or misaligned work. Staying connected here is what keeps the community strong and your contributions impactful. We are rooting for you! If you have any questions, drop them in the channel or comment right here on this PR. Let's build something great together. 🚀✨ |
|
Warning Review limit reached
Next review available in: 44 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds route-level contest ID validation, stale-request protection, reminder accessibility updates, redundant contest index cleanup, and frontend and backend test coverage. ChangesFrontend contest behavior
Backend contest validation and index management
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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: 4
🧹 Nitpick comments (2)
server/modules/contests/validation.test.js (1)
13-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct
validateParamsmiddleware tests.These tests validate schemas only. Add tests that verify an invalid parameter returns
400without callingnext, and that a valid string parameter reachesnextas a numericreq.params.contestId.🤖 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 `@server/modules/contests/validation.test.js` around lines 13 - 65, Add direct validateParams middleware tests alongside the existing schema tests: verify an invalid contestId produces a 400 response without invoking next, and verify a valid string contestId invokes next with req.params.contestId converted to a number. Reuse the existing contest validation schemas and middleware setup symbols rather than testing schema.safeParse alone.frontend/src/hooks/useContests.test.js (1)
114-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen or reframe the post-unmount test; it cannot currently detect a regression.
This test unmounts the hook, then resolves the pending fetch, and asserts nothing about
result.currentor about any spy. React 18 removed the "Can't perform a React state update on an unmounted component" warning: We've removed a warning when you call setState on an unmounted component. An update to an unmounted component is now a silent no-op with no re-render, no warning, and no thrown error, whether or not thecontestsRequestId/remindersRequestIdguard inuseContests.js(Lines 72-80) exists.Since
result.currentonly reflects the last re-render, and no re-render occurs for a truly unmounted tree, this test passes identically whether or not the requestId invalidation logic is removed. It does not verify the guard it claims to test.Consider one of these:
- Add a
console.errorspy assertion to make the "no warning/no crash" claim explicit in the test, even though it can't discriminate the guard's presence.- Rely on the existing stale-response race test (Lines 86-112) as the actual regression check for this guard, and reword this test's description to state it only verifies "no crash on late resolution after unmount," not "the response was ignored."
🧪 Example of making the smoke-test intent explicit
it("does not apply a fetch response that resolves after unmount", async () => { let resolveFetch; mockGetUpcomingCodeforcesContests.mockReturnValue( new Promise((resolve) => { resolveFetch = resolve; }) ); + const errorSpy = vi.spyOn(console, "error").mockImplementation(() => {}); const { unmount } = renderHook(() => useContests()); unmount(); // Should not throw / cause an act() warning even though state would // otherwise be updated after unmount. await act(async () => { resolveFetch({ data: { data: [CONTEST_A] } }); }); + + expect(errorSpy).not.toHaveBeenCalled(); + errorSpy.mockRestore(); });Since React 18+ no-ops these updates unconditionally, this doesn't restore the test's ability to catch a removed guard; it only documents intent. The real regression coverage for the requestId contract is the stale-response race test above it.
🤖 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 `@frontend/src/hooks/useContests.test.js` around lines 114 - 128, Reframe the test description and comments around the unmount flow in useContests to state that it only verifies late resolution causes no crash or warning, rather than claiming it validates response suppression. Keep the existing stale-response race test as coverage for the contestsRequestId/remindersRequestId guard, and optionally add an explicit console.error spy assertion if retaining the no-warning claim.
🤖 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 `@server/modules/contests/INDEXES.md`:
- Around line 9-13: Update the index inventory to include Contest.js’s
standalone { platform: 1 } index and justify its platform-only query coverage,
or remove that index from both the schema and deployed databases. Correct the {
platform: 1, contestId: 1 } entry so its explanation only claims support for
platform-prefixed queries, not phase-filtered queries, and revise the phase
index description accordingly.
In `@server/modules/contests/validation.js`:
- Around line 39-50: Update the contestId validation in addReminderSchema and
contestIdParamSchema to reject booleans, arrays, and other non-string/non-number
inputs before numeric coercion, while preserving integer and positive-value
checks. Add regression tests covering boolean and array contestId values in the
add-reminder body.
In `@server/scripts/dropRedundantContestIndex.js`:
- Around line 5-15: Update the migration documentation and the script’s Usage
comment to consistently use the repository-root command `node
server/scripts/dropRedundantContestIndex.js`, or explicitly state that the
command must be run from the server directory; ensure both documented locations
follow the same convention.
- Around line 19-27: Update the migration around the standalone index lookup to
first verify that a unique compound index on `{ platform: 1, contestId: 1 }`
exists. Abort before `collection.dropIndex(standalone.name)` when the
replacement index is missing, while preserving the existing drop behavior when
both indexes are present.
---
Nitpick comments:
In `@frontend/src/hooks/useContests.test.js`:
- Around line 114-128: Reframe the test description and comments around the
unmount flow in useContests to state that it only verifies late resolution
causes no crash or warning, rather than claiming it validates response
suppression. Keep the existing stale-response race test as coverage for the
contestsRequestId/remindersRequestId guard, and optionally add an explicit
console.error spy assertion if retaining the no-warning claim.
In `@server/modules/contests/validation.test.js`:
- Around line 13-65: Add direct validateParams middleware tests alongside the
existing schema tests: verify an invalid contestId produces a 400 response
without invoking next, and verify a valid string contestId invokes next with
req.params.contestId converted to a number. Reuse the existing contest
validation schemas and middleware setup symbols rather than testing
schema.safeParse alone.
🪄 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 Plus
Run ID: 02b9bdba-dcc9-4873-93e4-c8a194767806
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
frontend/package.jsonfrontend/src/components/contests/ContestCountdown.test.jsxfrontend/src/components/contests/ContestReminderBell.jsxfrontend/src/components/contests/ContestReminderBell.test.jsxfrontend/src/hooks/useContests.jsfrontend/src/hooks/useContests.test.jsserver/models/Contest.jsserver/modules/contests/INDEXES.mdserver/modules/contests/controller.jsserver/modules/contests/repository.test.jsserver/modules/contests/routes.jsserver/modules/contests/service.test.jsserver/modules/contests/validation.jsserver/modules/contests/validation.test.jsserver/scripts/dropRedundantContestIndex.js
…guard destructive index migration, fix docs
- contestId validation now rejects booleans and arrays before numeric
coercion (z.coerce.number() alone treats true as 1 and [42] as 42
via JS Number() semantics).
- dropRedundantContestIndex.js now refuses to drop any standalone
index unless the compound unique {platform, contestId} replacement
index is confirmed present, preventing a drifted database from
losing lookup coverage and the uniqueness guarantee.
- Removed the also-redundant standalone platform index (no query
filters on platform alone), and corrected INDEXES.md to accurately
describe what each index covers and use one consistent invocation
path for the migration script.
- Added direct validateParams middleware tests, not just schema-level
ones.
- Reframed the useContests unmount test to accurately state what it
verifies, since React 18 already silently no-ops post-unmount
setState regardless of the requestId guard's presence.
Addresses CodeRabbit review feedback on this PR
…solve react-hooks/set-state-in-effect Unrelated to issue kunalverma2512#279's scope, but blocking the required Frontend Lint & Test CI check on this PR. Matches the setTimeout-deferral pattern already used by the other two branches in the same effect — no behavior change, just no longer calls setState synchronously during the effect body.
Date.now() was called directly in the render body, which the new react-hooks/purity lint rule flags as an impure call. Moved to useState's lazy initializer + a periodic refresh via useEffect, matching the existing 'now' pattern already used in useContests.js. This was the actual blocking error in the Frontend Lint & Test CI check — the other similarly-worded 'Error: Calling setState...' log lines are warning-severity, not the build-failing error.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
1 participant
📌 Pull Request Summary
🔗 Related Issue
Closes #279
📝 Description
Provide a clear and concise summary of the changes made in this pull request.
Changes Made
validateParamsZod middleware andcontestIdParamSchema, applied toDELETE /reminders/:contestIdandPOST /reminders/:contestId/notified, both now enforce the same positive-integer contract asPOST /remindersat the route boundary, replacing a manualparseInt(...)that only rejectedNaNand would silently accept permissive strings like"12abc".contestIdindex on theContestmodel, the existing compound unique index{ platform: 1, contestId: 1 }already covers every current query pattern via its leftmost-prefix behavior. Documented the full rationale inserver/modules/contests/INDEXES.mdand addedserver/scripts/dropRedundantContestIndex.js, since a schema change alone never drops an already-existing index from a deployed database.useContestsnow guards against stale/superseded async responses using a per-fetch sequence token (incremented on every new fetch, and again on unmount), so a slower earlier request can never overwrite a newer one's state, and no response can write state after unmount, matching the cancellation pattern the bell and notifier already used.ContestReminderBellnow has a dynamicaria-label(including the due-soon count when applicable) instead of relying ontitlealone, and marks its decorative icon and count badgearia-hiddenso they aren't redundantly announced.node:testfor backend,vitestfor frontend, no new testing stack introduced): the validation contract (valid/zero/negative/decimal/non-numeric/"12abc"cases), reminder add/remove/mark-notified lifecycle edge cases (missing contest, non-upcoming phase), repository idempotency/sorting/joins/stale-cleanup, theuseContestsstale-response race condition and unmount safety, and bell accessibility (accessible name, due-soon count, keyboard focus).Motivation
This resolves the 5 Low-severity findings consolidated from the production review of #269. None of these individually blocks a single-instance release, but together they close real gaps: split validation logic between layers with inconsistent error contracts, a database index adding write overhead without ever being selected by the query planner, a React hook that could apply stale data after a slower request resolves late, an icon-only control with no reliable accessible name for screen readers, and a substantial feature area (contest tracker + reminders) shipped without any automated coverage protecting its lifecycle/async/validation behavior.
🚀 Type of Change
Select all that apply:
🧪 Testing
Verification
Test Details
validation.test.jsruns bothaddReminderSchemaandcontestIdParamSchemathrough a shared table of 9 cases each, valid integers, valid numeric strings, zero, negatives, decimals, non-numeric strings, the specific"12abc"permissive-parsing case, missing values, and empty strings.service.test.jswith 6 new tests coveringaddReminderthrowing 404 for a missing contest, 400 for aFINISHEDcontest, 400 for aSYSTEM_TESTcontest (not justFINISHED), successful add persisting correctly, plusremoveReminder/markReminderNotifieddelegating with correct arguments.repository.test.jscoversaddReminder's upsert-with-$setOnInsertidempotency,getUpcomingContests's filter/sort shape,getActiveReminderContests's join behavior (including dropping reminders for contests that are no longer upcoming), andpruneStaleReminders's deletion scope.useContests.test.jsincludes a genuine race-condition test, a slow initial fetch is made to resolve after a fastrefetch()call, and asserts the late response does not overwrite the newer data, plus an unmount test confirming a late-resolving response doesn't throw or apply.ContestReminderBell.test.jsxqueries by accessible role/name (getByRole("link", { name: ... })), the same mechanism assistive tech uses, verifies the due-soon count is included and correctly filtered (excludes >24h-out and already-started contests), confirms decorative elements arearia-hidden, and verifies real keyboard tab-focus reaches the control.npm testinserver/) and all 28 frontend tests (npm testinfrontend/) pass locally.📸 Screenshots / Demo (If Applicable)
N/A - this PR is backend validation/indexing logic, a React hook's async-safety internals, and accessibility attributes, not new UI. No visible design changes.
✅ Checklist
📚 Additional Notes
This is the 4th and final issue from the #269 production review - #276/#280, #277/#284, and #278/#286 are already merged. The index removal requires an explicit one-time run of
node server/scripts/dropRedundantContestIndex.jsagainst any already-deployed database (staging/production) after this merges, since Mongoose'sautoIndexonly ever adds missing indexes, never drops ones no longer declared in the schema, flagging this for whoever handles the deploy. Test coverage here is intentionally proportionate rather than exhaustive per the issue's own guidance ("avoid introducing a second testing stack," "prioritize behavior over implementation details"), full Playwright E2E flows and an exhaustiveUpcomingContestsListstate matrix were left as natural follow-ups for the existing Playwright setup at the repo root, rather than duplicated here.Summary by CodeRabbit
Accessibility
Bug Fixes
Tests
Maintenance