fix(flashcard): clone Date object in getFlashcardStats to prevent side-effect mutation (#1196) - #1526
Open
suhaniiz wants to merge 5 commits into
Open
fix(flashcard): clone Date object in getFlashcardStats to prevent side-effect mutation (#1196)#1526suhaniiz wants to merge 5 commits into
suhaniiz wants to merge 5 commits into
Conversation
Contributor
Author
|
Hey! @KaranUnique Just a heads-up regarding issue #1196 (Side-effect mutation of Date object in getFlashcardStats query). The issue was listed as closed on GitHub, but the original code was still mutating the now Date reference in-place in controllers/flashcard.controller.js. I’ve opened this PR which properly clones now before running .setHours(0, 0, 0, 0). The fix is tested and ready for review whenever you get a chance! |
📝 WalkthroughWalkthroughChangesFlashcard date handling
Job processing
Authentication API paths
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant getJobs
participant AdzunaAPI
participant JobCache
Client->>getJobs: Request jobs with role and country
getJobs->>getJobs: Validate country and build cache key
getJobs->>JobCache: Read cached jobs
getJobs->>AdzunaAPI: Fetch jobs for validated country
AdzunaAPI-->>getJobs: Return job results
getJobs->>JobCache: Upsert job results
getJobs-->>Client: Return jobs
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 3
🤖 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 `@backend/controllers/jobController.js`:
- Around line 94-100: Update the invalid-country test in
jobCache.boundedKeys.unit.test.js to expect an HTTP 400 response for
country=USA!!, and verify that no cache write is performed instead of expecting
a backend|in cache key.
- Around line 7-15: Normalize ADZUNA_COUNTRY at initialization by trimming it,
validating it against SUPPORTED_COUNTRIES, and falling back to "in" when invalid
or blank. Update getJobs and refreshJobCache to use this single validated
effective country so both paths share identical configuration behavior.
- Around line 108-109: Update backend/controllers/jobController.js lines 108-109
to pass the requested or session role through normalizeRole and fall back to the
default role when normalization fails; retain the normalized role for cacheKey
generation. At lines 137-141, normalize every stored role with normalizeRole and
skip entries that fail normalization before invoking Adzuna.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 68f08077-c2ec-4d8c-9486-8a0631f64d67
📒 Files selected for processing (3)
backend/controllers/flashcardController.jsbackend/controllers/jobController.jsfrontend/src/utils/apiPaths.js
Comment on lines
+7
to
+15
| const ADZUNA_COUNTRY = (process.env.ADZUNA_COUNTRY || "in").toLowerCase(); | ||
| const CACHE_TTL_MS = 24 * 60 * 60 * 1000; | ||
|
|
||
| // The Jobs feature is optional: without Adzuna credentials it stays dormant | ||
| // instead of crashing the server or spamming failed API calls. | ||
| // Supported Adzuna country codes | ||
| const SUPPORTED_COUNTRIES = new Set([ | ||
| "gb", "us", "in", "ca", "au", "de", "fr", "br", "za", | ||
| "at", "be", "ch", "es", "it", "nl", "nz", "pl", "ru", "sg" | ||
| ]); | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use one validated effective default country.
If ADZUNA_COUNTRY is invalid or contains whitespace, getJobs returns HTTP 400 even when the client does not send country. refreshJobCache instead falls back to "in", so the API and refresh paths use different configuration behavior.
Trim and validate the configured value at initialization. Use the same validated fallback in both paths.
🤖 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 `@backend/controllers/jobController.js` around lines 7 - 15, Normalize
ADZUNA_COUNTRY at initialization by trimming it, validating it against
SUPPORTED_COUNTRIES, and falling back to "in" when invalid or blank. Update
getJobs and refreshJobCache to use this single validated effective country so
both paths share identical configuration behavior.
Comment on lines
+94
to
+100
| // Validate country parameter if explicitly provided | ||
| let country = req.query.country ? String(req.query.country).toLowerCase() : ADZUNA_COUNTRY; | ||
| if (!SUPPORTED_COUNTRIES.has(country)) { | ||
| return res.status(400).json({ | ||
| message: `Invalid or unsupported country code: "${req.query.country}". Supported codes are: ${Array.from(SUPPORTED_COUNTRIES).join(", ")}`, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Update the invalid-country unit test.
backend/tests/jobCache.boundedKeys.unit.test.js expects country=USA!! to fall back to backend|in. This handler now returns HTTP 400 before it creates a cache key. The existing test will fail.
Change the test to assert the HTTP 400 response and that no cache write occurs.
🤖 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 `@backend/controllers/jobController.js` around lines 94 - 100, Update the
invalid-country test in jobCache.boundedKeys.unit.test.js to expect an HTTP 400
response for country=USA!!, and verify that no cache write is performed instead
of expecting a backend|in cache key.
Comment on lines
+108
to
+109
| const role = req.query.role || latestSession?.role || "software engineer"; | ||
| const cacheKey = `${role.toLowerCase()}|${country}`; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Restore role normalization in both cache-write paths.
Both paths bypass normalizeRole. Raw roles can create unbounded cache keys and bypass the existing bounded-key contract.
backend/controllers/jobController.js#L108-L109: normalize the requested or session role, then use the default role when normalization fails.backend/controllers/jobController.js#L137-L141: normalize each stored role and skip values that fail normalization before calling Adzuna.
📍 Affects 1 file
backend/controllers/jobController.js#L108-L109(this comment)backend/controllers/jobController.js#L137-L141
🤖 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 `@backend/controllers/jobController.js` around lines 108 - 109, Update
backend/controllers/jobController.js lines 108-109 to pass the requested or
session role through normalizeRole and fall back to the default role when
normalization fails; retain the normalized role for cacheKey generation. At
lines 137-141, normalize every stored role with normalizeRole and skip entries
that fail normalization before invoking Adzuna.
11 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📝 Pull Request Description
Related Issue
Closes #1196
Summary
In
controllers/flashcard.controller.js, callingnow.setHours(0, 0, 0, 0)mutated the originalnowDate object in-place. As a result,nowwas prematurely modified to midnight, causing any subsequent checks or parallel references to evaluate against the start of the day rather than the actual current timestamp.This fix creates a clone (
const startOfDay = new Date(now)) before applying.setHours(0, 0, 0, 0), preservingnowas the accurate current timestamp throughout the request.Type of Change
How Has This Been Tested?
Describe the testing steps performed.
GET /api/flashcards/statsand loggednowbefore and afterstartOfDaycalculation to verifynowremains unmutated.dueCountandreviewedTodayMongoDB queries return accurate calculations.Screenshots (if applicable)
N/A (Backend logic fix)
Checklist
Looks good to me. Ready to merge.