Production-readiness audit remediation#21
Merged
harrisonk0 merged 8 commits intomainfrom Mar 22, 2026
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Owner
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2db80f218
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
components/WeeklyMarksPage.tsx
Outdated
Comment on lines
+265
to
+267
| if (snapshot.length === 0) { | ||
| showToast('No changes to save.', 'info'); | ||
| return; |
There was a problem hiding this comment.
When a user makes edits and then restores every value back to its original state, isDirty can still be true (it is set on input events, not recomputed from data). In that case buildWeeklyMarksSnapshot returns an empty list, but this branch only shows a toast and returns, so the page remains stuck in an unsaved state (Save Marks button and leave-confirm prompts persist despite no actual changes). Before this commit, the no-op save path still cleared dirty state via the shared success path.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Fixed in 6d7a9c7. The no-op save path now clears isDirty before returning, so reverting edits back to the original state no longer leaves the page stuck in an unsaved state.
![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 (11)
components/DatePicker.tsx (1)
20-26: Consider removing commented-out code.The commented code blocks explaining the removed
showPicker()functionality could be replaced with a brief inline comment. The git history preserves the original implementation if needed later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/DatePicker.tsx` around lines 20 - 26, Remove the stale commented-out code related to the removed showPicker() API: delete the commented declarations and function for inputRef, handleIconClick and the showPicker() call in DatePicker.tsx, and replace them with a single concise inline comment noting that programmatic opening via showPicker was removed; reference the identifiers inputRef, handleIconClick, and showPicker in the comment so future readers know what was intentionally removed.scripts/check-db-contract.mjs (1)
81-83: Consider adding error handling for signOut failures.If
signOut()fails (e.g., network issue), any error thrown would mask the original test failure. Consider wrapping in a try-catch to log but not re-throw signOut errors.🛡️ Proposed defensive signOut
} finally { - await supabase.auth.signOut(); + try { + await supabase.auth.signOut(); + } catch (signOutError) { + console.warn('Failed to sign out during cleanup:', signOutError.message); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/check-db-contract.mjs` around lines 81 - 83, The finally block currently awaits supabase.auth.signOut() without guarding against failures, which could mask an earlier test error; wrap the await supabase.auth.signOut() call in a try-catch inside the existing finally, await it inside the try, and in the catch log the sign-out error (e.g., console.error or existing logger) but do not re-throw so the original test error is preserved; specifically modify the finally that calls supabase.auth.signOut() to a try { await supabase.auth.signOut(); } catch (err) { /* log sign-out failure with context */ }.tests/e2e/02-section-settings-workflow.md (1)
17-23: Add an explicit post-cleanup persistence check.After restoring the original value, include a save + reload confirmation step so cleanup guarantees are verifiable, not implied.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/02-section-settings-workflow.md` around lines 17 - 23, Update the "Restore the original meeting day value before leaving the page." step in tests/e2e/02-section-settings-workflow.md to explicitly persist and verify the cleanup: after resetting the meeting day back to the original value perform the save action (e.g., trigger the test's save/submit flow and wait for any success indicator), then reload the page (or navigate away and back) and assert the meeting day equals the original value; ensure the new assertions replace the current implied cleanup so the test fails if the restore does not actually persist.docs/04-deployment.md (1)
34-37: Optional docs polish: reduce repeated “Verify” openings in checklist items.This is readable as-is, but varying the leading verb in adjacent steps would make the checklist flow smoother.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/04-deployment.md` around lines 34 - 37, The checklist repeats the verb "Verify" for consecutive items (the lines starting "Verify sign-in works for manually provisioned users.", "Verify the production users have the expected roles in `profiles`.", "Verify `settings` has seeded rows for both `company` and `junior`.", and "Smoke-test auth, section settings persistence, member CRUD, marks entry, and dashboard behavior against the live backend."); update these items to use varied leading verbs (e.g., "Confirm sign-in works...", "Ensure production users have the expected roles in `profiles`.", "Check `settings` contains seeded rows for both `company` and `junior`.", and keep or slightly reword the final "Smoke-test..." line) so the checklist reads more smoothly while preserving the same checks and scope..github/workflows/ci-infrastructure.yml (1)
33-60: Consider gating live-Supabase smoke steps so core CI still runs without secrets.Right now, missing live secrets fail the job before typecheck/unit/build. If fork PRs or secretless validation are expected, conditionally run the live contract/e2e steps and keep static/unit checks unconditional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci-infrastructure.yml around lines 33 - 60, Make the live-Supabase steps conditional so forks and secretless CI still run static checks: add an if expression that requires all four secrets to be present to each of the live steps ("Check required Supabase client and smoke-test env vars", "Smoke-test Supabase client startup and runtime config", "Run client-visible database contract smoke check", "Install Playwright browser dependencies", and "Run browser smoke tests against live Supabase data"); e.g. use if: ${{ secrets.VITE_SUPABASE_URL != '' && secrets.VITE_SUPABASE_ANON_KEY != '' && secrets.E2E_TEST_EMAIL != '' && secrets.E2E_TEST_PASSWORD != '' }} so the remaining steps (typecheck, test:run, build) run unconditionally.services/db.test.ts (1)
209-255: Tighten thememberswrite-path assertions in these tests.These cases currently prove routing (
memberstable, no RPC) but not the actual write parameters. They would still pass ifupdateBoy()skippedupdate(...), or if either method used the wrongeq(...)filters.🔍 Tighten the assertion surface
return { from, rpc, + memberUpdate, + memberUpdateEqId, + memberUpdateEqSection, + memberDeleteEqId, + memberDeleteEqSection, get rpcResponse() {expect(supabaseMock.from).toHaveBeenCalledWith('members'); + expect(supabaseMock.memberUpdate).toHaveBeenCalledWith({ + name: 'Alex Updated', + school_year: '11', + section: 'company', + squad: 3, + is_squad_leader: true, + }); + expect(supabaseMock.memberUpdateEqId).toHaveBeenCalledWith('id', 'member-1'); + expect(supabaseMock.memberUpdateEqSection).toHaveBeenCalledWith('section', 'company'); expect(supabaseMock.rpc).not.toHaveBeenCalled(); expect(supabaseMock.from.mock.calls.filter(([table]) => table === 'marks')).toHaveLength(1);expect(supabaseMock.from).toHaveBeenCalledTimes(1); expect(supabaseMock.from).toHaveBeenCalledWith('members'); + expect(supabaseMock.memberDeleteEqId).toHaveBeenCalledWith('id', 'member-1'); + expect(supabaseMock.memberDeleteEqSection).toHaveBeenCalledWith('section', 'junior'); expect(supabaseMock.rpc).not.toHaveBeenCalled();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/db.test.ts` around lines 209 - 255, The tests currently only assert routing to 'members' and absence of RPC but not the exact write arguments; update the two tests to assert the concrete write parameters: for the updateBoy test, verify that supabaseMock.from('members').update was called once with the expected partial payload (only allowed member fields — e.g., name/squad/year/is_squad_leader mapping from the input) and that the returned query used eq('id', 'member-1') and eq('section', 'company'); for the deleteBoyById test, verify that supabaseMock.from('members').delete was called with no payload (or called once) and that its query used eq('id', 'member-1') and eq('section', 'junior'); keep the existing checks that rpc was not called. Ensure you reference the chain starting from supabaseMock.from so you assert .update/.delete call arguments and subsequent .eq filters used by updateBoy and deleteBoyById.components/weeklyMarksSavePlan.ts (1)
67-67: Type assertion could be made safer.The cast
(markState as JuniorMarkState | undefined)assumes the caller provides the correct type for junior section. If a company-stylenumberis accidentally passed, accessing.uniformand.behaviourwill returnundefined, which is handled by the fallback, but consider adding a runtime type guard for defense-in-depth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/weeklyMarksSavePlan.ts` at line 67, The current cast of markState to JuniorMarkState when creating juniorState is unsafe; add a runtime type guard (e.g., isJuniorMarkState) that checks markState is an object and has string properties "uniform" and "behaviour", then use that guard instead of a blind cast when computing juniorState so you only read .uniform and .behaviour when safe, falling back to { uniform: '', behaviour: '' } otherwise; update the code that references juniorState to rely on this guarded assignment.services/db.ts (2)
13-18: Duplicate type definition.
WeeklyMarksSnapshotEntryis also defined incomponents/weeklyMarksSavePlan.ts(line 7). Consider importing from a single source to avoid drift.♻️ Suggested consolidation
Either:
- Export from
weeklyMarksSavePlan.tsand import here, or- Move shared types to a dedicated types file (e.g.,
types.tsor a newtypes/marks.ts)-export type WeeklyMarksSnapshotEntry = { - memberId: string; - mark: Mark | null; -}; +import { WeeklyMarksSnapshotEntry } from '../components/weeklyMarksSavePlan';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/db.ts` around lines 13 - 18, Duplicate type WeeklyMarksSnapshotEntry/WeeklyMarksSaveEntry exists; remove the local definition and import the canonical WeeklyMarksSnapshotEntry from the existing declaration (the one in components/weeklyMarksSavePlan.ts) or move both to a shared types module (e.g., types/marks.ts) and import from there, then re-export WeeklyMarksSaveEntry as an alias of the imported WeeklyMarksSnapshotEntry to preserve existing exports; update any imports that referenced the removed local type to use the single exported symbol.
121-129: Consider simplifying the mark transformation.The IIFE pattern for extracting and removing
dateis functional but adds cognitive overhead. A helper function or destructuring at the call site could improve readability.♻️ Suggested simplification
p_snapshot: snapshot.map(({ memberId, mark }) => ({ memberId, - mark: mark - ? (() => { - const { date: _date, ...storedMark } = toStoredMark(mark, section); - return storedMark; - })() - : null, + mark: mark ? toStoredMarkWithoutDate(mark, section) : null, })),Where
toStoredMarkWithoutDateis a simple helper:const toStoredMarkWithoutDate = (mark: Mark, section: Section) => { const { date: _date, ...rest } = toStoredMark(mark, section); return rest; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/db.ts` around lines 121 - 129, The p_snapshot mapping uses an IIFE to call toStoredMark(mark, section) and strip its date field which is hard to read; create a small helper (e.g., toStoredMarkWithoutDate(mark, section)) that calls toStoredMark, destructures out date and returns the rest, then replace the IIFE in the p_snapshot map with a call to that helper for clarity and reuse; reference p_snapshot, toStoredMark, mark, and section when making the change.components/weeklyMarksSavePlan.test.ts (2)
124-139: Consider expanding test coverage fornormalizeEditableMarksForSave.The current tests cover company marks and absent junior marks, but the implementation has additional branches (junior section with valid uniform/behaviour scores, empty string → 0 conversion). Additional test cases would strengthen confidence in the normalization logic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/weeklyMarksSavePlan.test.ts` around lines 124 - 139, Add tests inside the existing describe('normalizeEditableMarksForSave') suite to cover the junior-mark branches: one test where a junior mark includes numeric uniformScore and behaviourScore (e.g., uniformScore: '2', behaviourScore: '3') and assert those are preserved and converted to numbers, and another test where uniformScore/behaviourScore are empty strings and should be normalized to 0 (instead of ''). Use the function normalizeEditableMarksForSave and mirror the existing test style to assert expected normalized output for both cases.
141-156: Consider adding edge case tests forareMarkListsEqual.Currently only tests order-insensitivity. Consider adding tests for: differing list lengths (should return false), marks with same date but different scores, and junior marks with
uniformScore/behaviourScorefields.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/weeklyMarksSavePlan.test.ts` around lines 141 - 156, Add edge-case tests for areMarkListsEqual: add a test asserting false for differing list lengths, a test asserting false for two marks with the same date but different score values (e.g., score 5 vs 6), and a test covering junior-mark objects that use uniformScore/behaviourScore fields (both equal should return true, differing should return false). Place these new it() blocks alongside the existing order-insensitivity test and reference the areMarkListsEqual function in each assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/03-getting-started.md`:
- Around line 59-71: The docs currently state that only `npm run test:e2e`
requires the E2E credentials, but `npm run check:db-contract` also reads
`.env`/`.env.local` and requires the E2E_TEST_* variables for the auth check;
update the paragraph describing `npm run check:db-contract` to explicitly state
it requires E2E_TEST_* (the same test user/role/seeded data as `npm run
test:e2e`), and clarify that both `npm run check:db-contract` and `npm run
test:e2e` consume E2E_TEST_* env vars so developers must set them before running
the pre-ship step.
In `@package.json`:
- Line 10: The package.json scripts "check:db-contract" (and similarly
"test:e2e") use the Node CLI flag --env-file-if-exists which requires Node
v22.9.0+, causing CI (Node 20) failures; fix by either removing the unknown flag
and switching to --env-file (and ensure required env files are present),
updating the CI Node runtime to >=22.9.0, or wrap env-file loading in a small
bootstrap script that conditionally loads .env/.env.local before invoking
scripts so the CLI doesn’t rely on --env-file-if-exists.
In `@tests/e2e/smoke.e2e.ts`:
- Around line 111-121: In the finally block make the restore authoritative: use
openSectionSettings to navigate, await restoreSelect.inputValue and if different
await restoreSelect.selectOption and then await the Save button click
(page.getByRole('button', { name: 'Save' }).click), and explicitly wait for the
save to complete (e.g., await a network response or a visible confirmation)
before exiting; do not swallow errors—replace console.error with rethrowing the
restoreError so the test fails fast if cleanup fails.
---
Nitpick comments:
In @.github/workflows/ci-infrastructure.yml:
- Around line 33-60: Make the live-Supabase steps conditional so forks and
secretless CI still run static checks: add an if expression that requires all
four secrets to be present to each of the live steps ("Check required Supabase
client and smoke-test env vars", "Smoke-test Supabase client startup and runtime
config", "Run client-visible database contract smoke check", "Install Playwright
browser dependencies", and "Run browser smoke tests against live Supabase
data"); e.g. use if: ${{ secrets.VITE_SUPABASE_URL != '' &&
secrets.VITE_SUPABASE_ANON_KEY != '' && secrets.E2E_TEST_EMAIL != '' &&
secrets.E2E_TEST_PASSWORD != '' }} so the remaining steps (typecheck, test:run,
build) run unconditionally.
In `@components/DatePicker.tsx`:
- Around line 20-26: Remove the stale commented-out code related to the removed
showPicker() API: delete the commented declarations and function for inputRef,
handleIconClick and the showPicker() call in DatePicker.tsx, and replace them
with a single concise inline comment noting that programmatic opening via
showPicker was removed; reference the identifiers inputRef, handleIconClick, and
showPicker in the comment so future readers know what was intentionally removed.
In `@components/weeklyMarksSavePlan.test.ts`:
- Around line 124-139: Add tests inside the existing
describe('normalizeEditableMarksForSave') suite to cover the junior-mark
branches: one test where a junior mark includes numeric uniformScore and
behaviourScore (e.g., uniformScore: '2', behaviourScore: '3') and assert those
are preserved and converted to numbers, and another test where
uniformScore/behaviourScore are empty strings and should be normalized to 0
(instead of ''). Use the function normalizeEditableMarksForSave and mirror the
existing test style to assert expected normalized output for both cases.
- Around line 141-156: Add edge-case tests for areMarkListsEqual: add a test
asserting false for differing list lengths, a test asserting false for two marks
with the same date but different score values (e.g., score 5 vs 6), and a test
covering junior-mark objects that use uniformScore/behaviourScore fields (both
equal should return true, differing should return false). Place these new it()
blocks alongside the existing order-insensitivity test and reference the
areMarkListsEqual function in each assertion.
In `@components/weeklyMarksSavePlan.ts`:
- Line 67: The current cast of markState to JuniorMarkState when creating
juniorState is unsafe; add a runtime type guard (e.g., isJuniorMarkState) that
checks markState is an object and has string properties "uniform" and
"behaviour", then use that guard instead of a blind cast when computing
juniorState so you only read .uniform and .behaviour when safe, falling back to
{ uniform: '', behaviour: '' } otherwise; update the code that references
juniorState to rely on this guarded assignment.
In `@docs/04-deployment.md`:
- Around line 34-37: The checklist repeats the verb "Verify" for consecutive
items (the lines starting "Verify sign-in works for manually provisioned
users.", "Verify the production users have the expected roles in `profiles`.",
"Verify `settings` has seeded rows for both `company` and `junior`.", and
"Smoke-test auth, section settings persistence, member CRUD, marks entry, and
dashboard behavior against the live backend."); update these items to use varied
leading verbs (e.g., "Confirm sign-in works...", "Ensure production users have
the expected roles in `profiles`.", "Check `settings` contains seeded rows for
both `company` and `junior`.", and keep or slightly reword the final
"Smoke-test..." line) so the checklist reads more smoothly while preserving the
same checks and scope.
In `@scripts/check-db-contract.mjs`:
- Around line 81-83: The finally block currently awaits supabase.auth.signOut()
without guarding against failures, which could mask an earlier test error; wrap
the await supabase.auth.signOut() call in a try-catch inside the existing
finally, await it inside the try, and in the catch log the sign-out error (e.g.,
console.error or existing logger) but do not re-throw so the original test error
is preserved; specifically modify the finally that calls supabase.auth.signOut()
to a try { await supabase.auth.signOut(); } catch (err) { /* log sign-out
failure with context */ }.
In `@services/db.test.ts`:
- Around line 209-255: The tests currently only assert routing to 'members' and
absence of RPC but not the exact write arguments; update the two tests to assert
the concrete write parameters: for the updateBoy test, verify that
supabaseMock.from('members').update was called once with the expected partial
payload (only allowed member fields — e.g., name/squad/year/is_squad_leader
mapping from the input) and that the returned query used eq('id', 'member-1')
and eq('section', 'company'); for the deleteBoyById test, verify that
supabaseMock.from('members').delete was called with no payload (or called once)
and that its query used eq('id', 'member-1') and eq('section', 'junior'); keep
the existing checks that rpc was not called. Ensure you reference the chain
starting from supabaseMock.from so you assert .update/.delete call arguments and
subsequent .eq filters used by updateBoy and deleteBoyById.
In `@services/db.ts`:
- Around line 13-18: Duplicate type
WeeklyMarksSnapshotEntry/WeeklyMarksSaveEntry exists; remove the local
definition and import the canonical WeeklyMarksSnapshotEntry from the existing
declaration (the one in components/weeklyMarksSavePlan.ts) or move both to a
shared types module (e.g., types/marks.ts) and import from there, then re-export
WeeklyMarksSaveEntry as an alias of the imported WeeklyMarksSnapshotEntry to
preserve existing exports; update any imports that referenced the removed local
type to use the single exported symbol.
- Around line 121-129: The p_snapshot mapping uses an IIFE to call
toStoredMark(mark, section) and strip its date field which is hard to read;
create a small helper (e.g., toStoredMarkWithoutDate(mark, section)) that calls
toStoredMark, destructures out date and returns the rest, then replace the IIFE
in the p_snapshot map with a call to that helper for clarity and reuse;
reference p_snapshot, toStoredMark, mark, and section when making the change.
In `@tests/e2e/02-section-settings-workflow.md`:
- Around line 17-23: Update the "Restore the original meeting day value before
leaving the page." step in tests/e2e/02-section-settings-workflow.md to
explicitly persist and verify the cleanup: after resetting the meeting day back
to the original value perform the save action (e.g., trigger the test's
save/submit flow and wait for any success indicator), then reload the page (or
navigate away and back) and assert the meeting day equals the original value;
ensure the new assertions replace the current implied cleanup so the test fails
if the restore does not actually persist.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f59dd80-992e-41bc-84d7-d6e05ddadb94
📒 Files selected for processing (34)
.github/workflows/ci-infrastructure.ymlARCHITECTURE.mdApp.tsxREADME.mdcomponents/AccountSettingsPage.tsxcomponents/BoyMarksPage.tsxcomponents/DatePicker.tsxcomponents/Header.tsxcomponents/HomePage.tsxcomponents/SettingsPage.tsxcomponents/WeeklyMarksPage.tsxcomponents/weeklyMarksDateChange.test.tscomponents/weeklyMarksDateChange.tscomponents/weeklyMarksSavePlan.test.tscomponents/weeklyMarksSavePlan.tsdocs/03-getting-started.mddocs/04-deployment.mddocs/06-data-and-services.mddocs/09-database-and-migrations.mddocs/10-database-security-model.mdhooks/useAppData.tshooks/useAuthAndRole.tshooks/useToastNotifications.tspackage.jsonscripts/check-db-contract.mjsservices/db.test.tsservices/db.tsservices/dbModel.tsservices/dbSyncPlan.test.tsservices/dbSyncPlan.tsservices/settings.test.tsservices/settings.tstests/e2e/02-section-settings-workflow.mdtests/e2e/smoke.e2e.ts
💤 Files with no reviewable changes (6)
- components/SettingsPage.tsx
- components/HomePage.tsx
- components/AccountSettingsPage.tsx
- hooks/useAuthAndRole.ts
- services/dbSyncPlan.ts
- services/dbSyncPlan.test.ts
| "dev": "vite", | ||
| "build": "vite build", | ||
| "preview": "vite preview", | ||
| "check:db-contract": "node --env-file-if-exists=.env --env-file-if-exists=.env.local scripts/check-db-contract.mjs", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Node version:"
node -v
echo
echo "Checking Node help for env-file flags:"
node --help | rg -n -- '--env-file|env-file-if-exists' || true
echo
echo "Inspecting affected npm scripts:"
jq -r '.scripts["check:db-contract"], .scripts["test:e2e"]' package.jsonRepository: harrisonk0/BB-Manager
Length of output: 506
🏁 Script executed:
cat .github/workflows/ci-infrastructure.yml | head -50Repository: harrisonk0/BB-Manager
Length of output: 1634
🌐 Web query:
Node.js --env-file-if-exists flag introduction version release notes
💡 Result:
--env-file-if-exists was introduced in Node.js v22.9.0 (added as a new CLI flag; same behavior as --env-file but it doesn’t error if the file is missing) [1][2].
Sources: Node.js CLI docs (shows “Added in: v22.9.0”) [1] and the Node.js v22.9.0 release notes (lists “src: add --env-file-if-exists flag”) [2].
Remove --env-file-if-exists flag or upgrade CI Node version to 22.9.0+.
The --env-file-if-exists flag was introduced in Node.js v22.9.0, but CI uses Node 20. Both check:db-contract and test:e2e scripts will fail when run in CI. Either downgrade to --env-file (which errors on missing files and requires manual env setup), upgrade CI to Node 22.9.0 or later, or restructure scripts to handle missing env files gracefully.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 10, The package.json scripts "check:db-contract" (and
similarly "test:e2e") use the Node CLI flag --env-file-if-exists which requires
Node v22.9.0+, causing CI (Node 20) failures; fix by either removing the unknown
flag and switching to --env-file (and ensure required env files are present),
updating the CI Node runtime to >=22.9.0, or wrap env-file loading in a small
bootstrap script that conditionally loads .env/.env.local before invoking
scripts so the CLI doesn’t rely on --env-file-if-exists.
Owner
Author
There was a problem hiding this comment.
Not taking this change. The repo CI already passed with these scripts on the current runner, so this is not an actual compatibility failure in this PR. If the runner is later pinned to a Node version that does not support --env-file-if-exists, I would switch to a bootstrap wrapper instead of changing behavior speculatively now.
There was a problem hiding this comment.
@harrisonk0, understood — if CI is already passing on the current runner, the flag is supported and the concern doesn't apply here. Your plan to switch to a bootstrap wrapper if the runner is ever pinned to an older version is a sensible fallback. No further action needed.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors marks persistence to RPC-driven snapshot saves, adds a Node smoke-check ( Changes
Sequence Diagram(s)sequenceDiagram
actor User as User (Browser)
participant UI as WeeklyMarksPage
participant Logic as weeklyMarksSavePlan
participant DB as Supabase RPC
participant Auth as Supabase Auth
User->>UI: Edit marks or click Save
UI->>Logic: buildWeeklyMarksSnapshot({boys, marks, selectedDate, attendance})
Logic->>Logic: normalizeEditableMarksForSave / areMarkListsEqual
Logic-->>UI: snapshot (changes-only) or empty
alt Snapshot Empty
UI-->>User: Show "No changes to save"
else Snapshot Has Changes
UI->>DB: RPC save_weekly_marks_snapshot(snapshot)
DB->>Auth: resolve current_app_role()
Auth-->>DB: role
DB-->>UI: RPC success
UI->>UI: fetchBoyById() to reload marks
UI-->>User: Show success toast
end
alt Change week with unsaved changes
User->>UI: Click week nav
UI->>UI: requestDateChange(nextDate)
UI->>Logic: shouldConfirmWeeklyMarksDateChange({currentDate,nextDate,isDirty})
Logic-->>UI: true
UI-->>User: Show confirmation modal
User->>UI: Confirm change
UI->>UI: apply pendingDate, reset editedMarks
end
sequenceDiagram
participant CI as CI Pipeline
participant Script as check-db-contract.mjs
participant Supabase as Supabase Project
participant Auth as Auth Service
participant DB as Database Tables
CI->>Script: npm run check:db-contract (loads .env/.env.local)
Script->>Supabase: create anon client (VITE_SUPABASE_URL, VITE_SUPABASE_ANON_KEY)
Script->>Auth: signInWithPassword(E2E_TEST_EMAIL, E2E_TEST_PASSWORD)
Auth-->>Script: session + user
Script->>Supabase: rpc('current_app_role')
Supabase-->>Script: role
Script->>DB: select section, meeting_day from settings where section IN ('company','junior')
DB-->>Script: rows (expect exactly 2)
Script->>Script: validate meeting_day ∈ [0..6]
Script->>Auth: signOut (finally)
Script-->>CI: exit success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Owner
Author
|
Addressed in 6d7a9c7:
Not taking a few review suggestions:
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
components/WeeklyMarksPage.tsx (1)
203-219:⚠️ Potential issue | 🟡 Minor
isDirtyis still sticky after reverting to the original snapshot.The no-op save path clears the flag only after the user clicks Save. If they edit back to the original values and then change weeks,
requestDateChange()still seesisDirty === trueand opens the discard modal even thoughbuildWeeklyMarksSnapshot()is empty. Use the snapshot itself to drive both the modal gate and the Save FAB.Also applies to: 257-268
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/WeeklyMarksPage.tsx` around lines 203 - 219, requestDateChange currently relies on the boolean isDirty which stays true even if the user reverts edits; change the gating logic to compute the snapshot via buildWeeklyMarksSnapshot() and use its emptiness instead of isDirty when calling shouldConfirmWeeklyMarksDateChange and when rendering/enabling the Save FAB. Specifically, update requestDateChange to pass the computed snapshot (or a boolean derived from it) into shouldConfirmWeeklyMarksDateChange and only call setPendingDate/setIsDateChangeConfirmOpen when the snapshot indicates real changes; likewise, drive the Save FAB enabled/visible state from buildWeeklyMarksSnapshot() emptiness rather than the isDirty flag so reverting edits clears the modal gate and save affordance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/check-db-contract.mjs`:
- Line 80: The log line prints a user's email (signInData.user.email) which
leaks PII; update the console.log that uses signInData.user.email and role to
either remove the email or redact it (e.g., only include role or a constant like
"[redacted]") so the message becomes something like "Database contract smoke
check passed for [role]" or "Database contract smoke check passed for [redacted]
(role)"; modify the console.log call in scripts/check-db-contract.mjs
accordingly to stop emitting the email.
In `@services/db.ts`:
- Around line 25-59: buildMemberMarksPatch assumes mark.date is unique but
validateMarksForSection currently doesn’t enforce this, causing duplicate
upserts; add a uniqueness guard that detects duplicate mark.date values and
rejects (throw an error) before patch construction. Specifically, in the call
path that uses buildMemberMarksPatch (e.g., saveBoyMarks) and inside
services/dbModel.ts.validateMarksForSection, validate the incoming marks array
for duplicate dates (by building a Set of dates or a Map) and throw a clear
validation error if any date appears more than once so buildMemberMarksPatch
only ever receives unique-dated marks.
In `@tests/e2e/smoke.e2e.ts`:
- Around line 43-48: The network waiter in saveSettingsAndWait is too permissive
(accepts both POST and PATCH) and should only detect the seeded-row update path;
update the predicate passed to page.waitForResponse in saveSettingsAndWait to
only match PATCH requests whose URL includes '/rest/v1/settings' (remove POST
from the allowed methods) so the test fails on insert/upsert regressions; keep
the rest of the wait logic and the saveResponsePromise usage intact.
- Around line 115-137: The finally block for restoring the 'Weekly Meeting Day'
(working with meetingDaySelect, restoreSelect, saveSettingsAndWait,
openSectionSettings, originalValue, nextValue) must not throw directly; instead
capture any error from the try body and any error from the cleanup, then after
the finally rethrow appropriately so the original test failure isn't lost.
Concretely, introduce two local error holders (e.g., originalError and
restoreError), assign originalError if the main try throws, assign restoreError
if the cleanup fails (but do not throw inside finally), and after the finally
rethrow either the originalError (if present) or the restoreError, or combine
them into a single error message so both failures are preserved.
---
Duplicate comments:
In `@components/WeeklyMarksPage.tsx`:
- Around line 203-219: requestDateChange currently relies on the boolean isDirty
which stays true even if the user reverts edits; change the gating logic to
compute the snapshot via buildWeeklyMarksSnapshot() and use its emptiness
instead of isDirty when calling shouldConfirmWeeklyMarksDateChange and when
rendering/enabling the Save FAB. Specifically, update requestDateChange to pass
the computed snapshot (or a boolean derived from it) into
shouldConfirmWeeklyMarksDateChange and only call
setPendingDate/setIsDateChangeConfirmOpen when the snapshot indicates real
changes; likewise, drive the Save FAB enabled/visible state from
buildWeeklyMarksSnapshot() emptiness rather than the isDirty flag so reverting
edits clears the modal gate and save affordance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e4bb035-19a6-4f5f-a077-359d7e50a99b
📒 Files selected for processing (11)
components/DatePicker.tsxcomponents/WeeklyMarksPage.tsxcomponents/weeklyMarksSavePlan.test.tscomponents/weeklyMarksSavePlan.tsdocs/03-getting-started.mdscripts/check-db-contract.mjsservices/db.test.tsservices/db.tstests/e2e/02-section-settings-workflow.mdtests/e2e/smoke.e2e.tstypes.ts
✅ Files skipped from review due to trivial changes (4)
- types.ts
- tests/e2e/02-section-settings-workflow.md
- services/db.test.ts
- components/weeklyMarksSavePlan.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/03-getting-started.md
- components/weeklyMarksSavePlan.test.ts
Owner
Author
|
Addressed in 9b94f69:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/WeeklyMarksPage.tsx (1)
350-350:⚠️ Potential issue | 🟡 MinorRemove double semicolon.
continue;;has an extraneous semicolon.🔧 Fix
- stats[squad] = { present: 0, total: 0, percentage: 0 }; - continue;; + stats[squad] = { present: 0, total: 0, percentage: 0 }; + continue;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/WeeklyMarksPage.tsx` at line 350, There is an extraneous semicolon after a continue statement ("continue;;") in WeeklyMarksPage.tsx; replace "continue;;" with a single "continue;" (or simply "continue" if in a context where semicolons are omitted) to remove the double semicolon and restore correct syntax.
🧹 Nitpick comments (2)
tests/e2e/smoke.e2e.ts (1)
128-142: Consider guardingopenSectionSettingsagainst already-open state.If the main test body completes successfully, Section Settings will already be open when the
finallyblock runs. CallingopenSectionSettings(page)again at line 130 clicks the button, which could toggle the modal closed if the UI treats it as a toggle, potentially causing the subsequent assertions to fail spuriously.If the UI button always opens (never toggles), this is fine. Otherwise, consider checking whether the heading is already visible before clicking:
🛡️ Optional guard
} finally { try { + const settingsHeading = page.getByRole('heading', { name: 'Section Settings' }); + if (!(await settingsHeading.isVisible())) { + await openSectionSettings(page); + } - await openSectionSettings(page); const restoreSelect = page.getByLabel('Weekly Meeting Day');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/smoke.e2e.ts` around lines 128 - 142, The finally block can call openSectionSettings(page) even when the Section Settings modal is already open, which may toggle it closed; update the cleanup to first check whether the settings modal is already visible (e.g., query for the Section Settings heading or dialog role) and only call openSectionSettings(page) when it is not visible, then proceed to read/restore Weekly Meeting Day via restoreSelect and saveSettingsAndWait; reference openSectionSettings(page), the restoreSelect (page.getByLabel('Weekly Meeting Day')), saveSettingsAndWait, and restoreError to keep existing error capture.components/WeeklyMarksPage.tsx (1)
45-45: Remove deadisDirtystate.The
isDirtystate value is never read—hasPendingChangesderived from the snapshot is used everywhere instead. ThesetIsDirtycalls (lines 104, 163, 195, 214, 274, 285) cause unnecessary re-renders without affecting behavior. Consider removing this state entirely along with allsetIsDirtycalls.♻️ Remove dead state and its setter calls
- const [, setIsDirty] = useState(false); // Retained for local input change tracking; navigation gating is derived from the snapshot.Then remove all
setIsDirty(...)calls throughout the file (lines 104, 163, 195, 214, 274, 285).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/WeeklyMarksPage.tsx` at line 45, Remove the dead isDirty state and its setter to avoid unnecessary renders: delete the useState declaration "const [, setIsDirty] = useState(false);" in the WeeklyMarksPage component and remove every call to setIsDirty(...) inside that component (these calls occur in handlers that already rely on hasPendingChanges derived from the snapshot). Ensure no other code references isDirty or setIsDirty afterwards; if there are, replace those references to use hasPendingChanges or the appropriate snapshot-derived value instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@components/WeeklyMarksPage.tsx`:
- Line 350: There is an extraneous semicolon after a continue statement
("continue;;") in WeeklyMarksPage.tsx; replace "continue;;" with a single
"continue;" (or simply "continue" if in a context where semicolons are omitted)
to remove the double semicolon and restore correct syntax.
---
Nitpick comments:
In `@components/WeeklyMarksPage.tsx`:
- Line 45: Remove the dead isDirty state and its setter to avoid unnecessary
renders: delete the useState declaration "const [, setIsDirty] =
useState(false);" in the WeeklyMarksPage component and remove every call to
setIsDirty(...) inside that component (these calls occur in handlers that
already rely on hasPendingChanges derived from the snapshot). Ensure no other
code references isDirty or setIsDirty afterwards; if there are, replace those
references to use hasPendingChanges or the appropriate snapshot-derived value
instead.
In `@tests/e2e/smoke.e2e.ts`:
- Around line 128-142: The finally block can call openSectionSettings(page) even
when the Section Settings modal is already open, which may toggle it closed;
update the cleanup to first check whether the settings modal is already visible
(e.g., query for the Section Settings heading or dialog role) and only call
openSectionSettings(page) when it is not visible, then proceed to read/restore
Weekly Meeting Day via restoreSelect and saveSettingsAndWait; reference
openSectionSettings(page), the restoreSelect (page.getByLabel('Weekly Meeting
Day')), saveSettingsAndWait, and restoreError to keep existing error capture.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5d73d910-f704-44f5-8104-462dd2741c30
📒 Files selected for processing (6)
components/WeeklyMarksPage.tsxscripts/check-db-contract.mjsservices/db.test.tsservices/dbModel.test.tsservices/dbModel.tstests/e2e/smoke.e2e.ts
✅ Files skipped from review due to trivial changes (2)
- scripts/check-db-contract.mjs
- services/db.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- services/dbModel.ts
Owner
Author
|
Addressed the final three review items in commit
Validation after the change: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
components/WeeklyMarksPage.tsx (1)
288-288:todaycan become stale if the page stays open past midnight.The empty dependency array means
todayis computed once on mount. If a user keeps the page open across midnight,isPastDate(line 381) will use a stale value. TheisLockedeffect (lines 66-70) also won't re-run since it only depends onselectedDate.This is a low-probability edge case, but for consistency you could derive
todayinline or refresh it on visibility change.♻️ Optional: derive inline or refresh on focus
- const today = useMemo(() => new Date().toISOString().split('T')[0], []); + const today = new Date().toISOString().split('T')[0];Or, if you want to avoid recomputing on every render, consider refreshing when the document regains focus.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/WeeklyMarksPage.tsx` at line 288, The computed `today` value is created once with useMemo and can become stale across midnight; change its usage so it is computed fresh when needed (e.g., derive `today` inline instead of via `useMemo`) or update it on visibility/focus so dependent logic like `isPastDate` and the `isLocked` effect that depends on `selectedDate` re-evaluates; specifically, replace the `const today = useMemo(...)` pattern in WeeklyMarksPage with either an inline `new Date().toISOString().split('T')[0]` where `today` is used (so `isPastDate` always sees current date) or add a small effect that updates `today` on `visibilitychange`/`focus` and use that state variable so `isPastDate` and the `isLocked` effect re-run when the day changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/smoke.e2e.ts`:
- Around line 95-105: The test currently calls the save_weekly_marks_snapshot
RPC (detected via saveResponsePromise) and writes a new live snapshot but never
restores the original marks, leaving persistent dated fixtures; after the Save
Marks step (after saveResponse.ok() assertion) call the same RPC to write the
original/currentValue back (use page.request.post or the app UI flow) and await
a matching response like the existing saveResponsePromise pattern (POST to
'/rest/v1/rpc/save_weekly_marks_snapshot') to ensure the cleanup completed
before the test exits; locate the save logic around
saveResponsePromise/scoreInput and add a restore call that sends the original
value for marksDate/values and waits for the response.
---
Nitpick comments:
In `@components/WeeklyMarksPage.tsx`:
- Line 288: The computed `today` value is created once with useMemo and can
become stale across midnight; change its usage so it is computed fresh when
needed (e.g., derive `today` inline instead of via `useMemo`) or update it on
visibility/focus so dependent logic like `isPastDate` and the `isLocked` effect
that depends on `selectedDate` re-evaluates; specifically, replace the `const
today = useMemo(...)` pattern in WeeklyMarksPage with either an inline `new
Date().toISOString().split('T')[0]` where `today` is used (so `isPastDate`
always sees current date) or add a small effect that updates `today` on
`visibilitychange`/`focus` and use that state variable so `isPastDate` and the
`isLocked` effect re-run when the day changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 31e772af-82b5-4dc1-ad48-4e5732911b02
📒 Files selected for processing (2)
components/WeeklyMarksPage.tsxtests/e2e/smoke.e2e.ts
Owner
Author
|
Addressed the latest two real review findings in commit
I did not act on the older repeated comments in that review because they were already resolved on Validation after the change:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/e2e/smoke.e2e.ts (1)
126-141: Consider adding anensureWeeklyMarksPageOpenhelper for consistency.The section settings test uses
ensureSectionSettingsOpenfor idempotent navigation in the restore path, but the weekly marks restore unconditionally callsopenWeeklyMarksPage. While this works (since clicking the nav button is likely idempotent), anensure*variant would be more explicit and consistent with the settings pattern.♻️ Optional consistency improvement
+const ensureWeeklyMarksPageOpen = async (page: Page) => { + const heading = page.getByRole('heading', { name: 'Weekly Marks' }); + + if (!(await heading.isVisible())) { + await openWeeklyMarksPage(page); + } +};Then in the restore block:
try { - await openWeeklyMarksPage(page); + await ensureWeeklyMarksPageOpen(page); await dateInput.fill(marksDate);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/smoke.e2e.ts` around lines 126 - 141, Add an idempotent navigation helper ensureWeeklyMarksPageOpen and use it in the restore/finally path instead of calling openWeeklyMarksPage unconditionally; implement ensureWeeklyMarksPageOpen to detect whether the weekly marks view is already active (or verify by selector used in openWeeklyMarksPage) and only click the nav button when needed, then update the finally block to call ensureWeeklyMarksPageOpen(page) before interacting with dateInput, restoreScoreInput and saveWeeklyMarksAndWait to mirror the ensureSectionSettingsOpen pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2e/smoke.e2e.ts`:
- Around line 126-141: Add an idempotent navigation helper
ensureWeeklyMarksPageOpen and use it in the restore/finally path instead of
calling openWeeklyMarksPage unconditionally; implement ensureWeeklyMarksPageOpen
to detect whether the weekly marks view is already active (or verify by selector
used in openWeeklyMarksPage) and only click the nav button when needed, then
update the finally block to call ensureWeeklyMarksPageOpen(page) before
interacting with dateInput, restoreScoreInput and saveWeeklyMarksAndWait to
mirror the ensureSectionSettingsOpen pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 73ae1ec4-6297-41d1-b76d-29258c7c303c
📒 Files selected for processing (2)
components/WeeklyMarksPage.tsxtests/e2e/smoke.e2e.ts
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.
This PR replaces #20 and contains the full set of fixes from the production-readiness audit.
Scope:
save_weekly_marks_snapshotfunction and align the repo docs/tests with that repairValidation already run:
npm run test:runnpm run typechecknpm run buildnpm run check:db-contractnpm run test:e2eNotes:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Chores