Add movie-ranker: Swiss tournament ranking game for Hindi/Telugu movies#752
Open
avinashpullela-cmyk wants to merge 2 commits into
Open
Add movie-ranker: Swiss tournament ranking game for Hindi/Telugu movies#752avinashpullela-cmyk wants to merge 2 commits into
avinashpullela-cmyk wants to merge 2 commits into
Conversation
Reads movies from Google Sheets, runs head-to-head Swiss tournament comparisons, and writes final rankings back to a new Rankings tab. - Express API: init, match (GET next pair), match (POST result), standings, sync - Swiss pairing: sort by score+Buchholz, avoid rematches, handle bye - Persistent state in state.json (resume across sessions) - Clean dark UI with keyboard shortcuts (←/→/Space) - Hindi/Telugu language badges, per-round progress bar - Google Sheets integration via service account
Contributor
|
Welcome to career-ops, @avinashpullela-cmyk! Thanks for your first PR. A few things to know:
We'll review your PR soon. Join our Discord if you have questions. |
📝 WalkthroughWalkthroughThis PR introduces a complete full-stack movie ranking tournament application. It combines a Swiss-style pairing algorithm, Google Sheets integration for data import/export, a persistent Express backend managing tournament state through REST endpoints, and an interactive single-page frontend where users select movie winners to build rankings. ChangesMovie Tournament Ranker
Sequence DiagramsequenceDiagram
participant Client as Browser Client
participant Server as Express Server
participant Sheets as Google Sheets
Client->>Server: GET /api/status
alt Tournament not initialized
Client->>Server: POST /api/init
Server->>Sheets: readMovies() from Hindi/Telugu tabs
Sheets-->>Server: [movies]
Server->>Server: generateRoundPairs() with bye handling
Server-->>Client: initialized, round 1 starts
else Tournament in progress
Server-->>Client: current round, remaining pairs
end
Client->>Server: GET /api/match
Server-->>Client: pair with movie cards and stats
Client->>Server: POST /api/match with choice (left/right/skip)
Server->>Server: update score/wins/losses, opponent history
alt Round complete
Server->>Server: advance round, generateRoundPairs() again
end
Server-->>Client: match recorded, next pair available or roundComplete
Client->>Server: GET /api/standings
Server-->>Client: [rankings with Buchholz tiebreaker]
Client->>Server: POST /api/sync
Server->>Sheets: writeRankings() to Rankings tab
Sheets-->>Server: sync complete
Server-->>Client: success
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
…ugu movies" This reverts commit 395f7ff.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
movie-ranker/swiss.js (1)
75-76:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Add null checks when retrieving movies by ID.
Lines 75-76 and 99-100 in
server.jsuse.find()to locate movies by pair IDs, but never validate thatmaormbexist before accessing their properties. If the state is corrupted or IDs mismatch, this will throw "Cannot read properties of undefined."This comment applies to
movie-ranker/server.jslines 75-76 and 99-100, not this file directly, but the pairing contract depends on ID stability.Also applies to: 99-100
🤖 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 `@movie-ranker/swiss.js` around lines 75 - 76, The code uses .find() to locate movies by ID and then accesses properties on the results (variables ma and mb) without null checks; update the lookup logic in server.js where movies are resolved (the code that uses .find(...) to set ma and mb) to validate that both ma and mb are non-null before accessing their properties, and handle the failure case by returning an appropriate error response or skipping the pairing (e.g., log the missing ID with processLogger or send a 400/404 and do not proceed with comparisons). Ensure you reference and guard the specific identifiers ma and mb and the functions/handlers that perform the pair resolution so no property is accessed on undefined.
🤖 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 `@movie-ranker/package.json`:
- Around line 1-14: Update package.json to include optional metadata: add an
"engines" field specifying the supported Node.js range (e.g., "engines":
{"node": ">=18 <20"}), add a "license" field with the project's license
identifier (e.g., "MIT" or other), and add a "repository" object or string with
the repo URL (e.g., "repository":
{"type":"git","url":"https://github.com/yourorg/yourrepo.git"}). Place these
properties alongside existing top-level keys such as "name", "version",
"description", and "scripts" so tools and registries can read them.
- Around line 9-13: Update the dependency ranges in package.json for the three
packages: bump "dotenv" to 17.4.2, "express" to 5.2.1, and "googleapis" to
172.0.0; then regenerate the lockfile (npm install or npm ci) and run your test
suite and manual smoke tests to catch breaking changes (pay special attention to
Express major-version changes—review middleware/error-handler signatures and any
usage of app.use/router in code referencing express). Ensure
package-lock.json/yarn.lock is committed and adjust any code that fails under
express@5 (middleware signature or router behaviors) before merging.
In `@movie-ranker/public/index.html`:
- Line 424: The setHTML function uses appEl.innerHTML with untrusted Google
Sheets data leading to XSS; fix by sanitizing all movie fields before they reach
the client (preferred) or by avoiding innerHTML and rendering via text nodes/DOM
methods. Specifically, in server.js sanitize movie properties (e.g., name, year,
etc.) using a library like DOMPurify (createDOMPurify from isomorphic-dompurify)
when loading the sheet so setHTML/renderBattle/renderRoundComplete receive safe
strings, or refactor setHTML to clear appEl and rebuild DOM with
createElement/textContent and update renderBattle/renderRoundComplete to append
text nodes instead of injecting HTML.
- Around line 407-408: The two nav buttons (ids "nav-battle" and
"nav-standings") lack an explicit type and therefore default to type="submit";
update the button elements in index.html that call switchView('battle') and
switchView('standings') to include type="button" so they do not submit any
enclosing form (add the attribute to both the button with id="nav-battle" and
id="nav-standings").
- Around line 527-534: The api helper (function api) should validate HTTP status
codes before returning parsed JSON: after fetch, check res.ok and if false,
attempt to parse an error payload (try res.json(), fallback to res.text()) and
then throw a new Error containing the HTTP status, statusText, method/path
context and the parsed error body so callers can distinguish network/server/app
errors; when res.ok is true, still safely parse JSON but surface a clear error
if parsing fails. Ensure you update the api function to perform these res.ok
checks and enrich thrown errors with method/path and response body.
- Around line 588-628: The busy flag is released too early in choose() because
the finally block runs immediately; remove the unconditional finally and instead
explicitly clear busy only in the correct places: after handling the
round-complete branch (set busy = false after renderRoundComplete/result
handling), and in the non-round-complete branch keep busy true until the
scheduled setTimeout finishes—move busy = false into the setTimeout callback
after await loadNextMatch(); also ensure error paths (the result.error branch
and any catch around the api call) set busy = false before invoking showError or
rethrowing. Use the function name choose, variables busy, currentMatch,
loadNextMatch, and the setTimeout callback to locate where to apply these
changes.
In `@movie-ranker/server.js`:
- Around line 56-59: The bye-handling logic for incrementing a movie's score and
wins (currently repeated where byeId is checked) should be extracted into a
small helper function (e.g., applyBye or handleBye) that accepts the byeId and
the movies array (or uses the same movies closure) and encapsulates finding the
movie and doing m.score++ and m.wins++; replace both duplicated blocks (the one
using byeId, movies and the other occurrence) with calls to that helper to
ensure consistent behavior and reduce duplication.
- Around line 109-110: Extract the mutual-opponent registration into a helper
function (e.g., registerOpponents or addMutualOpponent) that takes two player
objects and ensures each other's id is pushed only if not already present;
replace the duplicated blocks that currently check
winner.opponents/includes(loser.id) and loser.opponents/includes(winner.id) (and
the analogous skip-case where the same two lines appear) with calls to this
helper to centralize the logic and avoid duplication.
- Around line 19-23: The load function currently swallows any errors when
reading/parsing STATE_FILE; update the catch in load() (around fs.readFileSync
and JSON.parse) to log the caught error (including error.message and the
STATE_FILE path) to the console (e.g., console.error) before returning null so
parsing/file read failures are visible for debugging.
- Around line 163-166: The POST handler for '/api/reset' uses fs.existsSync and
fs.unlinkSync which can throw and crash the request; wrap the deletion in a
try-catch around the fs.unlinkSync call (after checking exists), and on error
log/record the error and respond with res.status(500).json({ success: false,
error: err.message }) while returning res.json({ success: true }) only when
deletion succeeds (or when the file didn't exist); reference the
app.post('/api/reset') handler, STATE_FILE, fs.existsSync, fs.unlinkSync, and
res.json.
- Around line 99-100: The lookups for ma and mb (const ma = s.movies.find(m =>
m.id === pair.a); const mb = s.movies.find(m => m.id === pair.b);) can return
undefined and later property access will crash; add defensive null checks right
after those finds in the same route/handler that uses them (same pattern as GET
`/api/match`): if either ma or mb is falsy, handle gracefully (e.g., return a
400/404 response with a clear error message or skip processing the pair) instead
of proceeding to access their properties.
- Around line 75-76: The lookup using s.movies.find(...) for pair.a and pair.b
can return undefined (ma/mb) and later property access on ma/mb will crash;
update the code around the ma and mb assignments (the s.movies.find calls for
pair.a and pair.b) to guard against missing movies by checking if ma and mb are
truthy before accessing their properties — if either is undefined, handle
gracefully (e.g., return a 400/500 error response or skip the pair and log the
inconsistency) so the request does not throw, and include clear logging that
mentions pair.a and pair.b IDs to aid debugging.
In `@movie-ranker/sheets.js`:
- Around line 20-21: The code currently hard-codes sheet tabs in the two calls
to sheets.spreadsheets.values.get (ranges "'Hindi Movies'!A:B" and "'Telugu
Movies'!A:B"), which reduces flexibility; change this to read a configurable
list (e.g., from an environment variable like MOVIE_SHEETS or a config file)
that contains sheet names, build the range strings dynamically using
SPREADSHEET_ID and each sheet name, and iterate over that list to call
sheets.spreadsheets.values.get for each sheet; update any surrounding logic that
assumes exactly two sheets to work with a variable-length array.
- Around line 6-14: In getAuth, wrap the JSON.parse(fs.readFileSync(keyPath,
'utf8')) call in a try-catch so invalid JSON in the service account key file
doesn't throw and crash the server; read the file into a string (e.g., raw =
fs.readFileSync(keyPath, 'utf8')), parse inside try, and on error throw a new,
clear Error that includes the keyPath and the underlying parse error message so
callers can handle it gracefully (keep the rest of the GoogleAuth construction
using the parsed credentials).
- Line 28: The id generation uses lang[0].toLowerCase() in the template for id
(id: `${lang[0].toLowerCase()}_${i}`) which will break or create collisions if
lang is empty; update the logic where ids are created (the id template
expression) to defensively pick a fallback when lang is falsy or empty (e.g.,
use lang.charAt(0).toLowerCase() with a fallback like 'u' or sanitize/use the
full lang string or a prefix such as `${(lang && lang[0]) ?
lang[0].toLowerCase() : 'u'}_${i}`) so IDs remain stable and unique even when
lang is an empty string. Ensure the chosen fallback is consistent across calls
to avoid collisions.
In `@movie-ranker/swiss.js`:
- Around line 3-45: generateRoundPairs currently assumes movies is a valid array
of well-formed objects; add upfront defensive validation in generateRoundPairs
to (1) return {pairs:[], byeId:null} for falsy/non-array or empty input, (2)
sanitize/filter the input array into a local variable (e.g., validated or
sanitizedMovies) by removing null/non-object items and ensuring each item has an
id (string), score (number, default 0), buchholz (number, default 0) and
opponents (array, default []), and (3) use these sanitized items for the rest of
the logic (sorted, used, pairing loops) so that calls like .sort and .includes
cannot throw on missing properties; if you prefer, throw a clear TypeError when
required fields (id) are missing instead of filtering.
---
Outside diff comments:
In `@movie-ranker/swiss.js`:
- Around line 75-76: The code uses .find() to locate movies by ID and then
accesses properties on the results (variables ma and mb) without null checks;
update the lookup logic in server.js where movies are resolved (the code that
uses .find(...) to set ma and mb) to validate that both ma and mb are non-null
before accessing their properties, and handle the failure case by returning an
appropriate error response or skipping the pairing (e.g., log the missing ID
with processLogger or send a 400/404 and do not proceed with comparisons).
Ensure you reference and guard the specific identifiers ma and mb and the
functions/handlers that perform the pair resolution so no property is accessed
on undefined.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 17c8b361-2eca-4ae5-bd55-3364839983cc
📒 Files selected for processing (7)
movie-ranker/.env.examplemovie-ranker/.gitignoremovie-ranker/package.jsonmovie-ranker/public/index.htmlmovie-ranker/server.jsmovie-ranker/sheets.jsmovie-ranker/swiss.js
Comment on lines
+1
to
+14
| { | ||
| "name": "movie-ranker", | ||
| "version": "1.0.0", | ||
| "description": "Swiss tournament movie ranking game", | ||
| "type": "module", | ||
| "scripts": { | ||
| "start": "node server.js" | ||
| }, | ||
| "dependencies": { | ||
| "dotenv": "^16.3.1", | ||
| "express": "^4.18.2", | ||
| "googleapis": "^128.0.0" | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider adding optional package metadata.
The manifest is missing optional but recommended fields:
"engines": to specify the required Node.js version range"license": to declare the project's license"repository": to link to the source repository
While these don't affect functionality, they improve project documentation and compatibility verification.
🤖 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 `@movie-ranker/package.json` around lines 1 - 14, Update package.json to
include optional metadata: add an "engines" field specifying the supported
Node.js range (e.g., "engines": {"node": ">=18 <20"}), add a "license" field
with the project's license identifier (e.g., "MIT" or other), and add a
"repository" object or string with the repo URL (e.g., "repository":
{"type":"git","url":"https://github.com/yourorg/yourrepo.git"}). Place these
properties alongside existing top-level keys such as "name", "version",
"description", and "scripts" so tools and registries can read them.
Comment on lines
+9
to
+13
| "dependencies": { | ||
| "dotenv": "^16.3.1", | ||
| "express": "^4.18.2", | ||
| "googleapis": "^128.0.0" | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check latest versions from npm registry and security advisories from GitHub
echo "=== Checking latest versions from npm ==="
for pkg in dotenv express googleapis; do
echo "Package: $pkg"
curl -s "https://registry.npmjs.org/$pkg/latest" | jq -r '.version' 2>/dev/null || echo " (lookup failed)"
done
echo ""
echo "=== Checking for security advisories ==="
gh api graphql -f query='
{
dotenv: securityVulnerabilities(first: 5, ecosystem: NPM, package: "dotenv") {
nodes {
advisory { summary severity publishedAt }
vulnerableVersionRange
firstPatchedVersion { identifier }
}
}
express: securityVulnerabilities(first: 5, ecosystem: NPM, package: "express") {
nodes {
advisory { summary severity publishedAt }
vulnerableVersionRange
firstPatchedVersion { identifier }
}
}
googleapis: securityVulnerabilities(first: 5, ecosystem: NPM, package: "googleapis") {
nodes {
advisory { summary severity publishedAt }
vulnerableVersionRange
firstPatchedVersion { identifier }
}
}
}
' | jq '.'Repository: santifer/career-ops
Length of output: 2513
Update npm dependency versions (security advisories don’t flag current ranges, but staying current is recommended)
- Latest npm versions:
dotenv17.4.2 (currently ^16.3.1),express5.2.1 (currently ^4.18.2),googleapis172.0.0 (currently ^128.0.0). - Security advisories for the versions in use: no advisory returned for
dotenv;googleapisadvisory shown applies to< 39.1.0(so 128.0.0 is outside);expressadvisory returned for< 4.22.0is marked “Withdrawn” (so no clear active issue for 4.18.2 from this data). - Upgrade still makes sense to reduce future exposure and pull in fixes/compatibility updates—especially
expressmajor-version jumps.
🤖 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 `@movie-ranker/package.json` around lines 9 - 13, Update the dependency ranges
in package.json for the three packages: bump "dotenv" to 17.4.2, "express" to
5.2.1, and "googleapis" to 172.0.0; then regenerate the lockfile (npm install or
npm ci) and run your test suite and manual smoke tests to catch breaking changes
(pay special attention to Express major-version changes—review
middleware/error-handler signatures and any usage of app.use/router in code
referencing express). Ensure package-lock.json/yarn.lock is committed and adjust
any code that fails under express@5 (middleware signature or router behaviors)
before merging.
Comment on lines
+407
to
+408
| <button id="nav-battle" class="active" onclick="switchView('battle')">⚔️ Battle</button> | ||
| <button id="nav-standings" onclick="switchView('standings')">📊 Standings</button> |
There was a problem hiding this comment.
Add explicit type="button" to navigation buttons.
These buttons are missing the type attribute. Browsers default to type="submit", which could cause unexpected behavior if the buttons were ever placed inside a form.
🔧 Proposed fix
- <button id="nav-battle" class="active" onclick="switchView('battle')">⚔️ Battle</button>
- <button id="nav-standings" onclick="switchView('standings')">📊 Standings</button>
+ <button type="button" id="nav-battle" class="active" onclick="switchView('battle')">⚔️ Battle</button>
+ <button type="button" id="nav-standings" onclick="switchView('standings')">📊 Standings</button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <button id="nav-battle" class="active" onclick="switchView('battle')">⚔️ Battle</button> | |
| <button id="nav-standings" onclick="switchView('standings')">📊 Standings</button> | |
| <button type="button" id="nav-battle" class="active" onclick="switchView('battle')">⚔️ Battle</button> | |
| <button type="button" id="nav-standings" onclick="switchView('standings')">📊 Standings</button> |
🧰 Tools
🪛 HTMLHint (1.9.2)
[warning] 407-407: The type attribute must be present on elements.
(button-type-require)
[warning] 408-408: The type attribute must be present on
elements.(button-type-require)
🤖 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 `@movie-ranker/public/index.html` around lines 407 - 408, The two nav buttons
(ids "nav-battle" and "nav-standings") lack an explicit type and therefore
default to type="submit"; update the button elements in index.html that call
switchView('battle') and switchView('standings') to include type="button" so
they do not submit any enclosing form (add the attribute to both the button with
id="nav-battle" and id="nav-standings").
Comment on lines
+527
to
+534
| async function api(method, path, body) { | ||
| const res = await fetch(path, { | ||
| method, | ||
| headers: body ? { 'Content-Type': 'application/json' } : {}, | ||
| body: body ? JSON.stringify(body) : undefined, | ||
| }); | ||
| return res.json(); | ||
| } |
There was a problem hiding this comment.
API helper doesn't validate HTTP status codes.
The api function returns res.json() without checking res.ok. If the backend returns a 4xx/5xx error with an HTML error page (common in production), the JSON parse will fail and throw an unhelpful error. Callers won't be able to distinguish between network errors, server errors, and application errors.
🛡️ Proposed fix
async function api(method, path, body) {
const res = await fetch(path, {
method,
headers: body ? { 'Content-Type': 'application/json' } : {},
body: body ? JSON.stringify(body) : undefined,
});
+ if (!res.ok) {
+ const text = await res.text();
+ let errorMsg;
+ try {
+ const json = JSON.parse(text);
+ errorMsg = json.error || `HTTP ${res.status}: ${res.statusText}`;
+ } catch {
+ errorMsg = `HTTP ${res.status}: ${res.statusText}`;
+ }
+ return { error: errorMsg };
+ }
return res.json();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async function api(method, path, body) { | |
| const res = await fetch(path, { | |
| method, | |
| headers: body ? { 'Content-Type': 'application/json' } : {}, | |
| body: body ? JSON.stringify(body) : undefined, | |
| }); | |
| return res.json(); | |
| } | |
| async function api(method, path, body) { | |
| const res = await fetch(path, { | |
| method, | |
| headers: body ? { 'Content-Type': 'application/json' } : {}, | |
| body: body ? JSON.stringify(body) : undefined, | |
| }); | |
| if (!res.ok) { | |
| const text = await res.text(); | |
| let errorMsg; | |
| try { | |
| const json = JSON.parse(text); | |
| errorMsg = json.error || `HTTP ${res.status}: ${res.statusText}`; | |
| } catch { | |
| errorMsg = `HTTP ${res.status}: ${res.statusText}`; | |
| } | |
| return { error: errorMsg }; | |
| } | |
| return res.json(); | |
| } |
🤖 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 `@movie-ranker/public/index.html` around lines 527 - 534, The api helper
(function api) should validate HTTP status codes before returning parsed JSON:
after fetch, check res.ok and if false, attempt to parse an error payload (try
res.json(), fallback to res.text()) and then throw a new Error containing the
HTTP status, statusText, method/path context and the parsed error body so
callers can distinguish network/server/app errors; when res.ok is true, still
safely parse JSON but surface a clear error if parsing fails. Ensure you update
the api function to perform these res.ok checks and enrich thrown errors with
method/path and response body.
Comment on lines
+163
to
+166
| app.post('/api/reset', (req, res) => { | ||
| if (fs.existsSync(STATE_FILE)) fs.unlinkSync(STATE_FILE); | ||
| res.json({ success: true }); | ||
| }); |
There was a problem hiding this comment.
Add error handling for file deletion failure.
If fs.unlinkSync fails (e.g., permission errors, file in use), the unhandled exception will crash the request. Wrap in try-catch for robustness.
🛡️ Proposed fix
app.post('/api/reset', (req, res) => {
- if (fs.existsSync(STATE_FILE)) fs.unlinkSync(STATE_FILE);
- res.json({ success: true });
+ try {
+ if (fs.existsSync(STATE_FILE)) fs.unlinkSync(STATE_FILE);
+ res.json({ success: true });
+ } catch (e) {
+ res.status(500).json({ error: `Failed to reset: ${e.message}` });
+ }
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| app.post('/api/reset', (req, res) => { | |
| if (fs.existsSync(STATE_FILE)) fs.unlinkSync(STATE_FILE); | |
| res.json({ success: true }); | |
| }); | |
| app.post('/api/reset', (req, res) => { | |
| try { | |
| if (fs.existsSync(STATE_FILE)) fs.unlinkSync(STATE_FILE); | |
| res.json({ success: true }); | |
| } catch (e) { | |
| res.status(500).json({ error: `Failed to reset: ${e.message}` }); | |
| } | |
| }); |
🤖 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 `@movie-ranker/server.js` around lines 163 - 166, The POST handler for
'/api/reset' uses fs.existsSync and fs.unlinkSync which can throw and crash the
request; wrap the deletion in a try-catch around the fs.unlinkSync call (after
checking exists), and on error log/record the error and respond with
res.status(500).json({ success: false, error: err.message }) while returning
res.json({ success: true }) only when deletion succeeds (or when the file didn't
exist); reference the app.post('/api/reset') handler, STATE_FILE, fs.existsSync,
fs.unlinkSync, and res.json.
Comment on lines
+6
to
+14
| function getAuth() { | ||
| const keyPath = process.env.GOOGLE_SERVICE_ACCOUNT_KEY; | ||
| if (!keyPath) throw new Error('GOOGLE_SERVICE_ACCOUNT_KEY not set in .env — see setup instructions'); | ||
| if (!fs.existsSync(keyPath)) throw new Error(`Service account key file not found: ${keyPath}`); | ||
| return new google.auth.GoogleAuth({ | ||
| credentials: JSON.parse(fs.readFileSync(keyPath, 'utf8')), | ||
| scopes: ['https://www.googleapis.com/auth/spreadsheets'], | ||
| }); | ||
| } |
There was a problem hiding this comment.
Wrap JSON.parse in try-catch to prevent server crashes.
If the service account key file contains invalid JSON, JSON.parse will throw an uncaught exception that crashes the server. This should be caught and converted to a user-friendly error message.
🔒 Proposed fix
function getAuth() {
const keyPath = process.env.GOOGLE_SERVICE_ACCOUNT_KEY;
if (!keyPath) throw new Error('GOOGLE_SERVICE_ACCOUNT_KEY not set in .env — see setup instructions');
if (!fs.existsSync(keyPath)) throw new Error(`Service account key file not found: ${keyPath}`);
+ let credentials;
+ try {
+ credentials = JSON.parse(fs.readFileSync(keyPath, 'utf8'));
+ } catch (e) {
+ throw new Error(`Invalid JSON in service account key file: ${e.message}`);
+ }
return new google.auth.GoogleAuth({
- credentials: JSON.parse(fs.readFileSync(keyPath, 'utf8')),
+ credentials,
scopes: ['https://www.googleapis.com/auth/spreadsheets'],
});
}🤖 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 `@movie-ranker/sheets.js` around lines 6 - 14, In getAuth, wrap the
JSON.parse(fs.readFileSync(keyPath, 'utf8')) call in a try-catch so invalid JSON
in the service account key file doesn't throw and crash the server; read the
file into a string (e.g., raw = fs.readFileSync(keyPath, 'utf8')), parse inside
try, and on error throw a new, clear Error that includes the keyPath and the
underlying parse error message so callers can handle it gracefully (keep the
rest of the GoogleAuth construction using the parsed credentials).
Comment on lines
+20
to
+21
| sheets.spreadsheets.values.get({ spreadsheetId: SPREADSHEET_ID, range: "'Hindi Movies'!A:B" }), | ||
| sheets.spreadsheets.values.get({ spreadsheetId: SPREADSHEET_ID, range: "'Telugu Movies'!A:B" }), |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoff
Hard-coded tab names limit flexibility.
The sheet names 'Hindi Movies' and 'Telugu Movies' are hard-coded. Consider making these configurable via environment variables or a config file to support additional languages or different sheet structures without code changes.
🤖 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 `@movie-ranker/sheets.js` around lines 20 - 21, The code currently hard-codes
sheet tabs in the two calls to sheets.spreadsheets.values.get (ranges "'Hindi
Movies'!A:B" and "'Telugu Movies'!A:B"), which reduces flexibility; change this
to read a configurable list (e.g., from an environment variable like
MOVIE_SHEETS or a config file) that contains sheet names, build the range
strings dynamically using SPREADSHEET_ID and each sheet name, and iterate over
that list to call sheets.spreadsheets.values.get for each sheet; update any
surrounding logic that assumes exactly two sheets to work with a variable-length
array.
Comment on lines
+3
to
+45
| export function generateRoundPairs(movies) { | ||
| const sorted = [...movies].sort((a, b) => | ||
| b.score !== a.score ? b.score - a.score : | ||
| b.buchholz !== a.buchholz ? b.buchholz - a.buchholz : | ||
| a.id.localeCompare(b.id) | ||
| ); | ||
|
|
||
| const pairs = []; | ||
| const used = new Set(); | ||
|
|
||
| for (let i = 0; i < sorted.length; i++) { | ||
| if (used.has(sorted[i].id)) continue; | ||
|
|
||
| let matched = false; | ||
|
|
||
| // First pass: prefer opponents not yet faced | ||
| for (let j = i + 1; j < sorted.length; j++) { | ||
| if (used.has(sorted[j].id)) continue; | ||
| if (!sorted[i].opponents.includes(sorted[j].id)) { | ||
| pairs.push({ a: sorted[i].id, b: sorted[j].id, completed: false, result: null }); | ||
| used.add(sorted[i].id); | ||
| used.add(sorted[j].id); | ||
| matched = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| // Second pass: allow rematch if necessary | ||
| if (!matched) { | ||
| for (let j = i + 1; j < sorted.length; j++) { | ||
| if (!used.has(sorted[j].id)) { | ||
| pairs.push({ a: sorted[i].id, b: sorted[j].id, completed: false, result: null }); | ||
| used.add(sorted[i].id); | ||
| used.add(sorted[j].id); | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| const byeId = sorted.find(m => !used.has(m.id))?.id ?? null; | ||
| return { pairs, byeId }; | ||
| } |
There was a problem hiding this comment.
Add input validation to prevent runtime errors.
The function assumes movies is a valid array and that each movie object contains the required fields (id, score, buchholz, opponents). Invalid or missing data will cause runtime exceptions when accessing these properties.
🛡️ Proposed defensive validation
export function generateRoundPairs(movies) {
+ if (!Array.isArray(movies)) {
+ throw new Error('movies must be an array');
+ }
const sorted = [...movies].sort((a, b) =>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export function generateRoundPairs(movies) { | |
| const sorted = [...movies].sort((a, b) => | |
| b.score !== a.score ? b.score - a.score : | |
| b.buchholz !== a.buchholz ? b.buchholz - a.buchholz : | |
| a.id.localeCompare(b.id) | |
| ); | |
| const pairs = []; | |
| const used = new Set(); | |
| for (let i = 0; i < sorted.length; i++) { | |
| if (used.has(sorted[i].id)) continue; | |
| let matched = false; | |
| // First pass: prefer opponents not yet faced | |
| for (let j = i + 1; j < sorted.length; j++) { | |
| if (used.has(sorted[j].id)) continue; | |
| if (!sorted[i].opponents.includes(sorted[j].id)) { | |
| pairs.push({ a: sorted[i].id, b: sorted[j].id, completed: false, result: null }); | |
| used.add(sorted[i].id); | |
| used.add(sorted[j].id); | |
| matched = true; | |
| break; | |
| } | |
| } | |
| // Second pass: allow rematch if necessary | |
| if (!matched) { | |
| for (let j = i + 1; j < sorted.length; j++) { | |
| if (!used.has(sorted[j].id)) { | |
| pairs.push({ a: sorted[i].id, b: sorted[j].id, completed: false, result: null }); | |
| used.add(sorted[i].id); | |
| used.add(sorted[j].id); | |
| break; | |
| } | |
| } | |
| } | |
| } | |
| const byeId = sorted.find(m => !used.has(m.id))?.id ?? null; | |
| return { pairs, byeId }; | |
| } | |
| export function generateRoundPairs(movies) { | |
| if (!Array.isArray(movies)) { | |
| throw new Error('movies must be an array'); | |
| } | |
| const sorted = [...movies].sort((a, b) => | |
| b.score !== a.score ? b.score - a.score : | |
| b.buchholz !== a.buchholz ? b.buchholz - a.buchholz : | |
| a.id.localeCompare(b.id) | |
| ); | |
| const pairs = []; | |
| const used = new Set(); | |
| for (let i = 0; i < sorted.length; i++) { | |
| if (used.has(sorted[i].id)) continue; | |
| let matched = false; | |
| // First pass: prefer opponents not yet faced | |
| for (let j = i + 1; j < sorted.length; j++) { | |
| if (used.has(sorted[j].id)) continue; | |
| if (!sorted[i].opponents.includes(sorted[j].id)) { | |
| pairs.push({ a: sorted[i].id, b: sorted[j].id, completed: false, result: null }); | |
| used.add(sorted[i].id); | |
| used.add(sorted[j].id); | |
| matched = true; | |
| break; | |
| } | |
| } | |
| // Second pass: allow rematch if necessary | |
| if (!matched) { | |
| for (let j = i + 1; j < sorted.length; j++) { | |
| if (!used.has(sorted[j].id)) { | |
| pairs.push({ a: sorted[i].id, b: sorted[j].id, completed: false, result: null }); | |
| used.add(sorted[i].id); | |
| used.add(sorted[j].id); | |
| break; | |
| } | |
| } | |
| } | |
| } | |
| const byeId = sorted.find(m => !used.has(m.id))?.id ?? null; | |
| return { pairs, byeId }; | |
| } |
🤖 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 `@movie-ranker/swiss.js` around lines 3 - 45, generateRoundPairs currently
assumes movies is a valid array of well-formed objects; add upfront defensive
validation in generateRoundPairs to (1) return {pairs:[], byeId:null} for
falsy/non-array or empty input, (2) sanitize/filter the input array into a local
variable (e.g., validated or sanitizedMovies) by removing null/non-object items
and ensuring each item has an id (string), score (number, default 0), buchholz
(number, default 0) and opponents (array, default []), and (3) use these
sanitized items for the rest of the logic (sorted, used, pairing loops) so that
calls like .sort and .includes cannot throw on missing properties; if you
prefer, throw a clear TypeError when required fields (id) are missing instead of
filtering.
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
2 participants
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.
Reads movies from Google Sheets, runs head-to-head Swiss tournament comparisons, and writes final rankings back to a new Rankings tab.
What does this PR do?
Related issue
Type of change
Checklist
node test-all.mjsand all tests passQuestions? Join the Discord for faster feedback.
Summary by CodeRabbit
Release Notes