(ci) E2E supabase moves to local runner, more parallelism#734
Merged
Conversation
Splits deploy.yml into deploy-preview and e2e-local jobs. The Coolify preview no longer gates the test run — E2E spins up a clean local Supabase + Edge Functions + prod Next build on the runner and executes the Playwright suite against it, so a slow or failing preview deploy doesn't hold up feedback. Verified locally: full enrollments.test.tsx passes against the local stack with the same setup the workflow performs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Runner is 24-core with tmpfs-backed FS and cache; bumping workers uses the cores we actually have. .next/cache restore keeps the Next build fast across runs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
WalkthroughReplaces the Coolify preview deploy with a self-contained e2e-local GitHub Actions job that provisions Supabase and serves the Next.js app on the runner; adds deterministic per-run Supabase project wiring, Playwright/Playwright-worker tuning, pervasive dotenv quieting, E2E synchronization fixes, TableController catch-up APIs, UI/gradebook stability changes, and a value-change-gated gradebook recalculation SQL migration. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as GitHub Runner
participant Workflow as deploy.yml (e2e-local job)
participant SupabaseCLI as Supabase CLI
participant Supabase as Supabase Docker Stack
participant EdgeFn as Edge Functions Gateway
participant NextApp as Next.js Prod Server
participant Playwright as Playwright/Browser
Runner->>Workflow: start e2e-local job (per-run SUPABASE_PROJECT)
Workflow->>SupabaseCLI: init project, write supabase/config.toml
SupabaseCLI-->>Supabase: start containers
Workflow->>Supabase: poll REST readiness & seed partitions
Workflow->>EdgeFn: serve Edge Functions
EdgeFn-->>Workflow: gateway readiness
Workflow->>NextApp: start Next.js prod server
NextApp-->>Workflow: server ready (polled)
Workflow->>Playwright: run tests using local BASE_URL and Supabase keys
Playwright-->>Workflow: tests pass/fail
Workflow->>Runner: on failure tail logs, upload `server-logs` artifact
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Poem
🚥 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. 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/deploy.yml (1)
7-8:⚠️ Potential issue | 🟠 Major
pull_request_target+ checkout of PR head SHA exposes secrets to untrusted code.CodeQL correctly flags this pattern. Both the
deploy-previewande2e-localjobs usepull_request_targetwith PR head SHA checkout (lines 7–8, 77–79), bute2e-localmaterially expands the exposure: it passesTEST_PASSWORD,ARGOS_TOKEN, andGITHUB_TOKEN(lines 194–196) directly into steps that execute PR-author-controlled code vianpx playwright test(line 205) andnpm run build(line 169). Anyone opening a PR can modify Playwright specs or build configuration to exfiltrate these secrets.Two recommended hardenings:
- Gate the
e2e-localjob on a label or membership check (run automatically only for collaborators; require a maintainer label for external PRs), or- Move privileged-context jobs to
pull_request(un-privileged) and use a separate workflow triggered on trusted refs to publish artifacts.Confirm this trade-off is intentional given the secrets now in scope.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/deploy.yml around lines 7 - 8, The workflow uses pull_request_target which checks out PR head SHA and exposes secrets; specifically the jobs named "deploy-preview" and "e2e-local" run in privileged context and "e2e-local" passes TEST_PASSWORD, ARGOS_TOKEN and GITHUB_TOKEN into steps that run PR-controlled code ("npx playwright test" and "npm run build"). Fix by removing secret exposure: either gate the "e2e-local" job behind a collaborator/label check (require a maintainer label or membership condition before using secrets) or change the trigger from pull_request_target to pull_request for jobs that run PR code and move any privileged artifact publishing into a separate workflow that runs only on trusted refs; ensure "deploy-preview" and "e2e-local" no longer pass TEST_PASSWORD/ARGOS_TOKEN/GITHUB_TOKEN into steps that execute PR-authored commands.
🧹 Nitpick comments (1)
.github/workflows/deploy.yml (1)
197-205: Minor:EDGE_FUNCTION_SECRET/END_TO_END_SECRETduplicated between.env.localand inline export.The same throwaway values appear at lines 154–155 (in
.env.local) and again at lines 203–204 (exported for the Playwright step). If one ever changes (or rotates to a real secret) the other will silently drift. A single source of truth — e.g. defining them once at the job-levelenv:block and consuming them in both the heredoc and Playwright — keeps the two in sync.Not blocking, just easy to flush before merge.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/deploy.yml around lines 197 - 205, The duplicate throwaway secrets EDGE_FUNCTION_SECRET and END_TO_END_SECRET are exported inline in the Playwright step and also defined in .env.local; remove the inline exports and instead declare these two variables once in the job-level env block so the heredoc (the block that sources .env.local / eval "$(npx supabase status -o env)") and the Playwright step both read the same values; update the Playwright step to rely on the job env (remove export EDGE_FUNCTION_SECRET and export END_TO_END_SECRET lines) so there is a single source of truth for EDGE_FUNCTION_SECRET and END_TO_END_SECRET.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/deploy.yml:
- Around line 73-75: Replace the hard-coded STORAGE_MIGRATION env and the
literal migration values used in the INSERT into
supabase_migrations.schema_migrations with logic that derives the migration
filename, version, and name at runtime: find the migration file matching
*_binary_submission_files.sql (e.g., define STORAGE_MIGRATION_PATH by globbing
supabase/migrations/*_binary_submission_files.sql), set STORAGE_MIGRATION as its
basename, then split into VERSION (prefix before first underscore) and NAME
(remainder without .sql) and use $VERSION/$NAME in the schema_migrations INSERT;
also add a brief comment in the special migration file
(20260217000000_binary_submission_files.sql) pointing back to this workflow so
future contributors know it’s special.
- Around line 177-180: The readiness loop incorrectly treats "000" as a
successful HTTP code; update the curl invocation in the loop(s) to use curl -sf
-o /dev/null -w '%{http_code}' (or keep -s and add -f) and change the test to
only accept 2xx responses (e.g., grep -qE '^2[0-9]{2}$'), and after the for-loop
add a failure branch that exits non-zero or errors if no healthy response was
seen so the step fails instead of proceeding; apply this change to both
occurrences of the curl loop shown (the loop checking
http://127.0.0.1:54321/functions/v1/ and the other analogous loop).
- Around line 158-164: The GitHub App private key written to .env.local is being
generated as a multi-line quoted value which Supabase's functions serve
--env-file cannot parse; update the workflow block that writes
GITHUB_PRIVATE_KEY_STRING so it produces a single-line value with escaped
newline sequences (e.g., replace actual newlines with "\n" and wrap the entire
key in quotes) before appending to .env.local so the resulting line matches the
format used in .env.local.staging and can be parsed by supabase CLI.
---
Outside diff comments:
In @.github/workflows/deploy.yml:
- Around line 7-8: The workflow uses pull_request_target which checks out PR
head SHA and exposes secrets; specifically the jobs named "deploy-preview" and
"e2e-local" run in privileged context and "e2e-local" passes TEST_PASSWORD,
ARGOS_TOKEN and GITHUB_TOKEN into steps that run PR-controlled code ("npx
playwright test" and "npm run build"). Fix by removing secret exposure: either
gate the "e2e-local" job behind a collaborator/label check (require a maintainer
label or membership condition before using secrets) or change the trigger from
pull_request_target to pull_request for jobs that run PR code and move any
privileged artifact publishing into a separate workflow that runs only on
trusted refs; ensure "deploy-preview" and "e2e-local" no longer pass
TEST_PASSWORD/ARGOS_TOKEN/GITHUB_TOKEN into steps that execute PR-authored
commands.
---
Nitpick comments:
In @.github/workflows/deploy.yml:
- Around line 197-205: The duplicate throwaway secrets EDGE_FUNCTION_SECRET and
END_TO_END_SECRET are exported inline in the Playwright step and also defined in
.env.local; remove the inline exports and instead declare these two variables
once in the job-level env block so the heredoc (the block that sources
.env.local / eval "$(npx supabase status -o env)") and the Playwright step both
read the same values; update the Playwright step to rely on the job env (remove
export EDGE_FUNCTION_SECRET and export END_TO_END_SECRET lines) so there is a
single source of truth for EDGE_FUNCTION_SECRET and END_TO_END_SECRET.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3429ae23-6056-4e97-8c32-8bc27fdaee8c
📒 Files selected for processing (2)
.github/workflows/deploy.ymlplaywright.config.ts
Replaces the dummy GitHub App key + missing Upstash/Sentry config in the local-Supabase E2E run with the real GHA secrets, so autograder flows that talk to GitHub work the same as on the deploy preview. Also captures Supabase container logs and the Edge Functions / Next server logs as a "server-logs" artifact (kept 14 days) so post-mortems on failed runs aren't limited to the tail printed inline. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Temporary; remove before merge. pull_request_target runs the workflow from the base branch, so it can't exercise changes on a feature branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previous run got OOM-killed at the runner level — backing off concurrency to give Postgres + Edge Functions + Next + 8 chromium workers more headroom. Also restructures the e2e-local job so 'supabase start' runs in the background while 'playwright install' and 'next build' execute. This is safe because the local supabase JWT keys are deterministic, so .env.local can be written (and the build can begin inlining NEXT_PUBLIC_*) before the database is up. A poll-with-PID-fallback waits for the API to answer before tests run. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Modern supabase CLI applies the storage.objects RLS migration cleanly on a fresh start, so the move-aside / superuser-apply / replay dance is gone. Also backgrounds 'playwright install --with-deps' so it overlaps with 'next build' (saves another ~30-90s). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dotenv@17 prints '◇ injected env (n) from .env.local // tip: ...' banners on every load. Pass quiet: true at every call site to drop them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
.github/workflows/deploy.yml (3)
145-150:⚠️ Potential issue | 🔴 Critical
GITHUB_PRIVATE_KEY_STRINGshould be written as single-line escaped\n, not multiline quoted PEM.The current write path can produce multiline env values that are fragile with
supabase functions serve --env-file.🔧 Safer serialization for `.env.local`
- { - printf 'GITHUB_PRIVATE_KEY_STRING="' - printf '%s' "${GITHUB_PRIVATE_KEY_STRING}" - printf '"\n' - } >> .env.local + KEY_ESCAPED=$(printf '%s' "${GITHUB_PRIVATE_KEY_STRING}" | awk '{printf "%s\\n",$0}') + printf 'GITHUB_PRIVATE_KEY_STRING="%s"\n' "$KEY_ESCAPED" >> .env.localDoes `supabase functions serve --env-file` officially support multiline quoted env values (e.g., PEM blocks), or should PEMs be single-line with escaped \n?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/deploy.yml around lines 145 - 150, The current deploy.yml block appends a multiline PEM into .env.local (the printf block writing GITHUB_PRIVATE_KEY_STRING) which produces literal newlines; instead serialize the key as a single-line string with escaped "\n" sequences so tools like supabase functions serve --env-file can parse it. Modify the GITHUB_PRIVATE_KEY_STRING write step to replace real newlines with the two-character sequence backslash+n (e.g., piping the key through a replace/sed/awk transformation) and append that escaped string to .env.local rather than a quoted multiline PEM.
229-241:⚠️ Potential issue | 🔴 CriticalReadiness checks are still false-positive on HTTP
000and don’t fail hard on timeout.Line 230 and Line 239 treat any 3-digit code as ready;
000matches and causes premature exit. This can start Playwright against dead services.✅ Tighten health checks
- name: Serve Edge Functions run: | nohup npx supabase functions serve --env-file .env.local \ > /tmp/edge-functions.log 2>&1 & echo $! > /tmp/edge-functions.pid - for i in $(seq 1 60); do - if curl -s -o /dev/null -w '%{http_code}' http://127.0.0.1:54321/functions/v1/ | grep -qE '^[0-9]{3}$'; then break; fi - sleep 1 - done + ok=0 + for i in $(seq 1 60); do + if curl -sf -o /dev/null http://127.0.0.1:54321/functions/v1/; then ok=1; break; fi + sleep 1 + done + [ "$ok" -eq 1 ] || { echo "Edge Functions failed readiness"; exit 1; } - name: Start Next.js prod server run: | nohup env PORT=3001 npm run start > /tmp/next-server.log 2>&1 & echo $! > /tmp/next-server.pid - for i in $(seq 1 60); do - if curl -s -o /dev/null -w '%{http_code}' http://localhost:3001/ | grep -qE '^[0-9]{3}$'; then break; fi - sleep 1 - done + ok=0 + for i in $(seq 1 60); do + if curl -sf -o /dev/null http://localhost:3001/; then ok=1; break; fi + sleep 1 + done + [ "$ok" -eq 1 ] || { echo "Next.js failed readiness"; exit 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/deploy.yml around lines 229 - 241, The readiness loops use curl -w '%{http_code}' with grep -qE '^[0-9]{3}$' which accepts 000; update both health checks (the curl condition used before starting Playwright and the one in the "Start Next.js prod server" step) to explicitly assert a valid success status (e.g., check for ^2[0-9]{2}$ or ^(2|3)[0-9]{2}$) and after the for-loop add a failing branch that exits non-zero if the service never became ready (so the job fails instead of continuing). Target the curl checks and add an explicit post-loop test to echo a helpful message and exit 1 when the service did not return a 2xx/3xx within the timeout.
9-10:⚠️ Potential issue | 🔴 CriticalCritical:
pull_request_targetis executing untrusted PR code with secrets available.Line 9 enables privileged context, and Lines 27/86 check out the PR head SHA. Later steps run project scripts with secrets injected (for example Lines 116-125), which is a direct secret-exfiltration path from forked PRs.
🔒 Safer pattern
on: - pull_request_target: + pull_request: workflow_dispatch:- - uses: actions/checkout@v4 - with: - ref: ${{ github.event.pull_request.head.sha || github.sha }} + - uses: actions/checkout@v4If
pull_request_targetmust stay for trusted tasks, gate secret-using jobs:if: github.event_name != 'pull_request_target' || github.event.pull_request.head.repo.fork == falseAlso applies to: 25-27, 84-87, 116-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/deploy.yml around lines 9 - 10, The workflow currently uses pull_request_target which runs untrusted PR code with repository secrets; change to avoid exposing secrets by either using pull_request instead of pull_request_target or gate any jobs/steps that use secrets with the conditional if: github.event_name != 'pull_request_target' || github.event.pull_request.head.repo.fork == false, and ensure any checkout actions (actions/checkout) do not fetch and run the PR head when secrets are required—only checkout the base or use a safe path for trusted tasks; also update steps that run project scripts with secrets (the script-running steps and any actions that use secrets) to only run under the gated condition so forked PRs cannot access secrets.
🧹 Nitpick comments (1)
.github/workflows/deploy.yml (1)
35-37: Pin Supabase CLI version instead oflatestfor deterministic CI.Using
version: latestmakes runs non-reproducible and can break E2E unexpectedly after upstream releases. Pin to the current stable version 2.92.1.📌 Suggested change
- uses: supabase/setup-cli@v1 with: - version: latest + version: 2.92.1Also applies to: 109-111
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/deploy.yml around lines 35 - 37, Replace the non-deterministic Supabase CLI pin in the GitHub Actions workflow: change the "version: latest" value passed to the supabase/setup-cli@v1 step to the fixed stable version "2.92.1" wherever it appears (the occurrences around the supabase/setup-cli@v1 steps shown in the diff), ensuring both places use the exact "2.92.1" pin for reproducible CI runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/deploy.yml:
- Around line 7-8: Remove the temporary branch trigger entry marked by
TODO(remove-before-merge); delete the list item referencing the job
"ci/parallel-e2e-local-supabase" from the workflow so the deploy.yml no longer
triggers on that branch name, ensuring the workflow only uses permanent
triggers.
---
Duplicate comments:
In @.github/workflows/deploy.yml:
- Around line 145-150: The current deploy.yml block appends a multiline PEM into
.env.local (the printf block writing GITHUB_PRIVATE_KEY_STRING) which produces
literal newlines; instead serialize the key as a single-line string with escaped
"\n" sequences so tools like supabase functions serve --env-file can parse it.
Modify the GITHUB_PRIVATE_KEY_STRING write step to replace real newlines with
the two-character sequence backslash+n (e.g., piping the key through a
replace/sed/awk transformation) and append that escaped string to .env.local
rather than a quoted multiline PEM.
- Around line 229-241: The readiness loops use curl -w '%{http_code}' with grep
-qE '^[0-9]{3}$' which accepts 000; update both health checks (the curl
condition used before starting Playwright and the one in the "Start Next.js prod
server" step) to explicitly assert a valid success status (e.g., check for
^2[0-9]{2}$ or ^(2|3)[0-9]{2}$) and after the for-loop add a failing branch that
exits non-zero if the service never became ready (so the job fails instead of
continuing). Target the curl checks and add an explicit post-loop test to echo a
helpful message and exit 1 when the service did not return a 2xx/3xx within the
timeout.
- Around line 9-10: The workflow currently uses pull_request_target which runs
untrusted PR code with repository secrets; change to avoid exposing secrets by
either using pull_request instead of pull_request_target or gate any jobs/steps
that use secrets with the conditional if: github.event_name !=
'pull_request_target' || github.event.pull_request.head.repo.fork == false, and
ensure any checkout actions (actions/checkout) do not fetch and run the PR head
when secrets are required—only checkout the base or use a safe path for trusted
tasks; also update steps that run project scripts with secrets (the
script-running steps and any actions that use secrets) to only run under the
gated condition so forked PRs cannot access secrets.
---
Nitpick comments:
In @.github/workflows/deploy.yml:
- Around line 35-37: Replace the non-deterministic Supabase CLI pin in the
GitHub Actions workflow: change the "version: latest" value passed to the
supabase/setup-cli@v1 step to the fixed stable version "2.92.1" wherever it
appears (the occurrences around the supabase/setup-cli@v1 steps shown in the
diff), ensuring both places use the exact "2.92.1" pin for reproducible CI runs.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9e5e56e9-4532-4ba3-b1a2-8a585cd494bf
📒 Files selected for processing (32)
.github/workflows/deploy.ymlplaywright.config.tsscripts/AddUserToClassAsInstructor.tsscripts/BenchmarkCourseSSR.tsscripts/CreateTestUsers.tsscripts/ExportMissingUsers.tsscripts/FixGitHubTeamMembership.tsscripts/GenerateMagicLink.tsscripts/GetUserFromCanvas.tsscripts/SeedCourseAssignments.tsscripts/SeedDB.tsscripts/UnlinkGitHubIdentity.tstests/e2e/TestingUtils.tstests/e2e/discussion-threads.test.tsxtests/e2e/enrollments.test.tsxtests/e2e/gradebook-calculations.test.tsxtests/e2e/gradebook-whatif.test.tsxtests/e2e/gradebook.test.tsxtests/e2e/grading.test.tsxtests/e2e/lab-sections.test.tsxtests/e2e/leaderboard.test.tsxtests/e2e/llm-hint.test.tsxtests/e2e/manual-grading-scores.test.tsxtests/e2e/office-hours.test.tsxtests/e2e/pseudonymous-grading.test.tsxtests/e2e/regrade-deadline.test.tsxtests/e2e/rubric-editor.test.tsxtests/e2e/student-dashboard-total-score.test.tsxtests/e2e/survey-assignment-grading.test.tsxtests/e2e/surveys.test.tsxtests/e2e/timezone-preferences.test.tsxtests/manual/gradebook.ts
✅ Files skipped from review due to trivial changes (30)
- tests/e2e/pseudonymous-grading.test.tsx
- tests/e2e/grading.test.tsx
- scripts/GetUserFromCanvas.ts
- tests/e2e/manual-grading-scores.test.tsx
- tests/e2e/lab-sections.test.tsx
- tests/e2e/gradebook-whatif.test.tsx
- scripts/GenerateMagicLink.ts
- scripts/UnlinkGitHubIdentity.ts
- tests/e2e/regrade-deadline.test.tsx
- tests/e2e/enrollments.test.tsx
- scripts/BenchmarkCourseSSR.ts
- tests/e2e/gradebook-calculations.test.tsx
- tests/e2e/student-dashboard-total-score.test.tsx
- scripts/SeedCourseAssignments.ts
- tests/manual/gradebook.ts
- tests/e2e/llm-hint.test.tsx
- tests/e2e/leaderboard.test.tsx
- scripts/ExportMissingUsers.ts
- tests/e2e/office-hours.test.tsx
- tests/e2e/discussion-threads.test.tsx
- scripts/SeedDB.ts
- scripts/FixGitHubTeamMembership.ts
- playwright.config.ts
- tests/e2e/rubric-editor.test.tsx
- tests/e2e/gradebook.test.tsx
- tests/e2e/timezone-preferences.test.tsx
- tests/e2e/survey-assignment-grading.test.tsx
- tests/e2e/surveys.test.tsx
- scripts/AddUserToClassAsInstructor.ts
- scripts/CreateTestUsers.ts
The Coolify branch preview is being retired until the deployment target moves into the same k8s cluster as the e2e runner; until then there's no point spending CI minutes on it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Chakra v3 dialogs flip data-state to "closed" the instant the dismiss fires, but the element keeps a visible bounding box during the exit animation. Playwright's toBeVisible / toBeHidden read the bounding box, so on webkit (slower exit transition) the assertion races and times out even though the dialog functionally closed — exactly the failure we hit on the e2e-local CI run. Asserting the data-state attribute removes the animation race entirely. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Last commit changed two assertions that weren't actually broken. The alertdialog at due-dates:405 unmounts on dismiss, so its locator resolves to "element not found" — toHaveAttribute(data-state) fails because there's no element. The gradebook addDialog (toBeHidden) was fine before. Only the regular Chakra dialog at due-dates:377 keeps the element in the DOM during exit animation, which is the only place data-state=closed is the right tool. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
finalize_submission_early commits the state change before the client calls reviewAssignments.refetchAll(). If the refetch fails (network blip, RLS hiccup, etc.) the outer catch was showing "Could not finalize submission" — the user thinks it didn't work and retries an already-finalized submission. Show the success toast immediately after the RPC succeeds, then run the refetch in its own try/catch with a non-fatal warning toast on failure so the post-success cache refresh can't masquerade as a finalize error. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
.github/workflows/deploy.yml (2)
7-8:⚠️ Potential issue | 🟡 MinorRemove the temporary branch trigger before merge.
This still leaves the workflow wired to a one-off branch name.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/deploy.yml around lines 7 - 8, Remove the temporary branch trigger entry that references the one-off branch name so the workflow is not tied to a specific branch; locate the commented TODO and the list item "- ci/parallel-e2e-local-supabase" in the deploy.yml workflow and delete that list item (or replace it with the intended stable trigger/job name) so the workflow uses the normal branch triggers instead of the temporary branch-specific job.
18-20:⚠️ Potential issue | 🟠 MajorScope the concurrency group to the PR, not just the base branch.
With
pull_request_target,github.refresolves to the base branch. Every PR againststaginglands in the same concurrency group and cancels each other's E2E runs. Key this off the PR number when present.Suggested fix
concurrency: - group: e2e-local-${{ github.ref }} + group: e2e-local-${{ github.event.pull_request.number || github.ref }} cancel-in-progress: true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/deploy.yml around lines 18 - 20, The concurrency group currently uses github.ref which with pull_request_target resolves to the base branch causing PRs against the same branch to collide; change the concurrency group expression to prefer the PR number when present (github.event.pull_request.number) and fall back to github.ref for non-PR contexts so each PR gets a unique group (update the concurrency.group key in the workflow to use a conditional expression referencing github.event.pull_request.number and github.ref).
🧹 Nitpick comments (7)
components/ui/markdown-file-preview.tsx (1)
631-650: Pre-awaitclose looks good — minor heads-up on the error path.The synchronous
close()makes sense givensubmission_file_comments.create()does an optimistic insert, and the comment block does a great job documenting the CI-flake rationale. One small thing worth thinking about (no need to block on it): ifcreate()rejects, the popover is already gone, so the user sees only the toaster and loses whatever check/sub-option/message they had typed. For transient PostgREST hiccups that's probably fine, but for "real" failures (e.g.releasedReviewGraderBlocked) re‑entering the annotation from scratch isn't great. A future improvement could be to roll back the optimistic row and re‑open the popover with the prior state on error — happy to leave that for a follow‑up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ui/markdown-file-preview.tsx` around lines 631 - 650, The popover is closed before awaiting submissionController.submission_file_comments.create(), so on rejection the user loses their typed annotation; update the error path to detect non-transient errors (use getStudentFacingErrorMessage(e, { releasedReviewGraderBlocked: ... }) or inspect the exception) and on such errors roll back the optimistic insert and re-open the popover with the previous form state so the user can retry/edit, while still showing toaster.error; tie this logic to the same submission flow that calls close() and the submission_file_comments.create() call so it restores the exact values the user had entered when re-opening.components/ui/code-file.tsx (1)
1188-1209: Mirror change LGTM.This matches the
MarkdownLineActionPopupchange and the inline comment captures the "chromium >60s insert" CI flake nicely. Same optional follow‑up applies as incomponents/ui/markdown-file-preview.tsx(preserving draft state / rolling back the optimistic row whencreate()rejects) — not blocking.One tiny nit while you're in here: the
allowEmptyMessageprop onMessageInputat line 1143 isselectedCheckOption.check && !selectedCheckOption.check.is_comment_required, which evaluates to a truthyRubricCheckobject when the check exists rather than a boolean. The markdown twin (line 591) already uses the boolean formselectedCheckOption.check ? !... : true. Worth aligning ifMessageInput's prop is typed asboolean, but unrelated to this PR so feel free to defer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/ui/code-file.tsx` around lines 1188 - 1209, The allowEmptyMessage prop passed to MessageInput uses the expression selectedCheckOption.check && !selectedCheckOption.check.is_comment_required which yields a RubricCheck object (truthy) instead of a boolean; update the expression in components/ui/code-file.tsx (where MessageInput is rendered) to return a boolean consistent with the markdown twin, e.g. use a conditional that evaluates to true/false (selectedCheckOption.check ? !selectedCheckOption.check.is_comment_required : true) or an explicit boolean coercion so allowEmptyMessage is always a boolean.app/course/[course_id]/office-hours/[queue_id]/new/newRequestForm.tsx (1)
603-616: LGTM — navigate-first ordering is a sensible fix for the webkit/CI race.Reordering
router.pushahead of any post-create state work avoids the cascade ofsetState→ effect re-runs (validation,useListrefetches, error clearing) that could land in the same microtask as the navigation and quietly chomp it — much like how something burrowing in the carrot patch can leave you with no clue where the route went. The inline rationale is clear and ties back to the file-top TODO about consolidating the post-create writes into a single RPC. Removingresetfrom the destructure and theuseCallbackdeps is consistent.One tiny copy nit if you feel like polishing while you're here: line 605's toast says "Redirecting to queue view..." but the destination is the individual request page (
/.../{queue_id}/{createdHelpRequest.id}), not the queue listing. Easy to tweak, totally optional.✏️ Optional toast copy tweak
toaster.success({ title: "Success", - description: "Help request successfully created. Redirecting to queue view..." + description: "Help request successfully created. Opening your request..." });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/course/`[course_id]/office-hours/[queue_id]/new/newRequestForm.tsx around lines 603 - 616, The success toast copy is slightly misleading: it says "Redirecting to queue view..." while the code navigates to the individual request page via router.push(`/course/${course_id}/office-hours/${queue_id}/${createdHelpRequest.id}`); update the toaster.success description to reflect the actual destination (e.g., "Redirecting to request view..." or "Redirecting to your request...") so the message matches the navigation performed by router.push and the createdHelpRequest.id route.app/course/[course_id]/manage/gradebook/GradebookPopoverProvider.tsx (1)
161-161: Optional: reuseclose()for consistent teardown after submit.
handleFormSuccessonly flipsisOpen, leavingselected,anchorRect, andtargetRef.currentstale until the nextopenAt. Functionally harmless today (rendering is gated byisOpen && selected), but reusingclosekeeps all dismissal paths converging on the same teardown — easier to reason about if a future change readsselected/anchorRectwhile closed.♻️ Proposed change
- const handleFormSuccess = useCallback(() => setIsOpen(false), []); + const handleFormSuccess = useCallback(() => close(), [close]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/course/`[course_id]/manage/gradebook/GradebookPopoverProvider.tsx at line 161, The handler handleFormSuccess currently calls setIsOpen(false) but leaves selected, anchorRect and targetRef.current stale; change handleFormSuccess to call the existing close() function so all teardown paths (close, openAt, etc.) converge and clear selected/anchorRect/targetRef consistently—update the handleFormSuccess definition to invoke close() instead of setIsOpen(false).supabase/migrations/20260424140000_gradebook_cell_recalc_value_changed_gating.sql (2)
134-134:changed_rowsis collected but never read.
changed_rowsis declared and populated viaarray_append(...)inside the loop, but it's never referenced after the loop or returned in the result jsonb. Either drop it or include it in the return payload (e.g., for logging/observability). Currently it's just dead state.♻️ Suggested cleanup
updated_count integer := 0; all_rows_to_enqueue jsonb[] := ARRAY[]::jsonb[]; - changed_rows jsonb[]; BEGINupdated_count := updated_count + 1; - changed_rows := array_append(changed_rows, to_jsonb(updated_row));Also applies to: 180-180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260424140000_gradebook_cell_recalc_value_changed_gating.sql` at line 134, The variable changed_rows is being collected via array_append inside the loop but never used afterward; either remove the changed_rows declaration and all array_append calls to eliminate dead state, or include changed_rows in the function's output JSON by adding it to the returned result payload (e.g., add changed_rows to the result jsonb object returned at the end of the procedure). Locate the changed_rows declaration and its array_append calls in the loop and update accordingly, and if choosing to return it ensure the return construction (the result jsonb) includes a field for changed_rows so the collected rows are observable.
188-203: O(n²) dedup scan in the inner loop.For each changed row you
unnest(all_rows_to_enqueue)and scan the full accumulator. Under heavy batches (which is the same E2E load that caused the prior cascade revert mentioned in the header comment) this becomes quadratic. ASELECT count(*) FROM unnest(...) ...based check has the same shape; consider tracking atext[]or localHSTORE/temp table keyed onclass_id||gradebook_id||student_id||is_privateto dedup in O(1) per insertion, or just defer dedup toenqueue_gradebook_row_recalculation_batchif it already handles duplicates.This is unlikely to bite for typical interactive batches, but worth keeping in mind for the bulk-import path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260424140000_gradebook_cell_recalc_value_changed_gating.sql` around lines 188 - 203, The current dedup logic in the loop unnesting all_rows_to_enqueue for every updated_row (using all_rows_to_enqueue and updated_row) is O(n²); replace it with an O(1) membership check by maintaining a local set of keys (e.g. a text[] seen_keys or an hstore/temp table) keyed by a stable composite string like class_id||'|'||gradebook_id||'|'||student_id||'|'||is_private, check that set before appending to all_rows_to_enqueue, and update the set when you append, or alternatively remove this in-loop dedup entirely and rely on enqueue_gradebook_row_recalculation_batch to handle duplicates if it already does so.tests/e2e/lab-sections.test.tsx (1)
147-154: Barecatchwill mask non-row-count failures.The empty
catch {}swallows every error from the assertion block — including unrelated failures like page crashes, navigation away, or context closure — and then unconditionally reloads. If something genuinely broken happens, you'll spend the 30s timeout chasing it on a reloaded page instead of seeing the real error from the 8s attempt.Consider narrowing to the specific timeout case, or at minimum logging the swallowed error so CI triage isn't blind:
♻️ Optional refactor
try { await expect(page.getByRole("row")).toHaveCount(21, { timeout: 8_000 }); - } catch { + } catch (err) { + // Recovery path: stale SSR cache race; reload once and re-wait. + // eslint-disable-next-line no-console + console.warn("Row count not reached in 8s, reloading once:", err); await page.reload(); await expect(page.getByText("Loading lab sections...")).toBeHidden(); await expect(page.getByRole("button", { name: "Create Lab Section" })).toBeVisible(); await expect(page.getByRole("row")).toHaveCount(21, { timeout: 30_000 }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/lab-sections.test.tsx` around lines 147 - 154, The bare catch around the initial expect of page.getByRole("row").toHaveCount(21) swallows all failures; change it to catch the specific timeout case (e.g., check error.name === "TimeoutError" or error instanceof playwright.errors.TimeoutError) and only perform page.reload() and the retry flow when it is that timeout error, otherwise rethrow (or at minimum log the caught error before rethrowing) so unrelated failures (page crash/navigation/context closed) surface; update the catch block that wraps expect(page.getByRole("row")).toHaveCount to inspect the caught error and conditionally run page.reload(), the subsequent expects (page.getByText("Loading lab sections..."), page.getByRole("button", { name: "Create Lab Section" }), and the final toHaveCount retry) only when appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hooks/useDiscussionThreadRootController.tsx`:
- Around line 217-218: The background call to threadRealTimeController.start()
can reject and is currently ignored; update the call site to append a .catch
handler that logs the error (use the module's existing logger if available,
otherwise console.error) and then calls threadRealTimeController.close() to
clean up the broken controller so the thread doesn't keep a failed realtime
instance.
In `@tests/e2e/lab-sections.test.tsx`:
- Line 96: The locator page.locator('[data-part="content"][data-scope="menu"]')
is matching multiple menus and causing Playwright strict-mode failures; change
the selector used with toHaveAttribute("data-state", "closed") to specifically
target the Course Settings menu (for example, resolve the trigger's
aria-controls id and use that id in the selector or scope via an ancestor
selector tied to the Course Settings container) so only the intended element is
matched; additionally, replace the broad bare catch surrounding the
count/assertion retry with a narrow catch that only handles Playwright's
ExpectError (or otherwise only handles the specific recoverable assertion error)
so other exceptions (timeouts/crashes) are not swallowed and reloaded.
---
Duplicate comments:
In @.github/workflows/deploy.yml:
- Around line 7-8: Remove the temporary branch trigger entry that references the
one-off branch name so the workflow is not tied to a specific branch; locate the
commented TODO and the list item "- ci/parallel-e2e-local-supabase" in the
deploy.yml workflow and delete that list item (or replace it with the intended
stable trigger/job name) so the workflow uses the normal branch triggers instead
of the temporary branch-specific job.
- Around line 18-20: The concurrency group currently uses github.ref which with
pull_request_target resolves to the base branch causing PRs against the same
branch to collide; change the concurrency group expression to prefer the PR
number when present (github.event.pull_request.number) and fall back to
github.ref for non-PR contexts so each PR gets a unique group (update the
concurrency.group key in the workflow to use a conditional expression
referencing github.event.pull_request.number and github.ref).
---
Nitpick comments:
In `@app/course/`[course_id]/manage/gradebook/GradebookPopoverProvider.tsx:
- Line 161: The handler handleFormSuccess currently calls setIsOpen(false) but
leaves selected, anchorRect and targetRef.current stale; change
handleFormSuccess to call the existing close() function so all teardown paths
(close, openAt, etc.) converge and clear selected/anchorRect/targetRef
consistently—update the handleFormSuccess definition to invoke close() instead
of setIsOpen(false).
In `@app/course/`[course_id]/office-hours/[queue_id]/new/newRequestForm.tsx:
- Around line 603-616: The success toast copy is slightly misleading: it says
"Redirecting to queue view..." while the code navigates to the individual
request page via
router.push(`/course/${course_id}/office-hours/${queue_id}/${createdHelpRequest.id}`);
update the toaster.success description to reflect the actual destination (e.g.,
"Redirecting to request view..." or "Redirecting to your request...") so the
message matches the navigation performed by router.push and the
createdHelpRequest.id route.
In `@components/ui/code-file.tsx`:
- Around line 1188-1209: The allowEmptyMessage prop passed to MessageInput uses
the expression selectedCheckOption.check &&
!selectedCheckOption.check.is_comment_required which yields a RubricCheck object
(truthy) instead of a boolean; update the expression in
components/ui/code-file.tsx (where MessageInput is rendered) to return a boolean
consistent with the markdown twin, e.g. use a conditional that evaluates to
true/false (selectedCheckOption.check ?
!selectedCheckOption.check.is_comment_required : true) or an explicit boolean
coercion so allowEmptyMessage is always a boolean.
In `@components/ui/markdown-file-preview.tsx`:
- Around line 631-650: The popover is closed before awaiting
submissionController.submission_file_comments.create(), so on rejection the user
loses their typed annotation; update the error path to detect non-transient
errors (use getStudentFacingErrorMessage(e, { releasedReviewGraderBlocked: ...
}) or inspect the exception) and on such errors roll back the optimistic insert
and re-open the popover with the previous form state so the user can retry/edit,
while still showing toaster.error; tie this logic to the same submission flow
that calls close() and the submission_file_comments.create() call so it restores
the exact values the user had entered when re-opening.
In
`@supabase/migrations/20260424140000_gradebook_cell_recalc_value_changed_gating.sql`:
- Line 134: The variable changed_rows is being collected via array_append inside
the loop but never used afterward; either remove the changed_rows declaration
and all array_append calls to eliminate dead state, or include changed_rows in
the function's output JSON by adding it to the returned result payload (e.g.,
add changed_rows to the result jsonb object returned at the end of the
procedure). Locate the changed_rows declaration and its array_append calls in
the loop and update accordingly, and if choosing to return it ensure the return
construction (the result jsonb) includes a field for changed_rows so the
collected rows are observable.
- Around line 188-203: The current dedup logic in the loop unnesting
all_rows_to_enqueue for every updated_row (using all_rows_to_enqueue and
updated_row) is O(n²); replace it with an O(1) membership check by maintaining a
local set of keys (e.g. a text[] seen_keys or an hstore/temp table) keyed by a
stable composite string like
class_id||'|'||gradebook_id||'|'||student_id||'|'||is_private, check that set
before appending to all_rows_to_enqueue, and update the set when you append, or
alternatively remove this in-loop dedup entirely and rely on
enqueue_gradebook_row_recalculation_batch to handle duplicates if it already
does so.
In `@tests/e2e/lab-sections.test.tsx`:
- Around line 147-154: The bare catch around the initial expect of
page.getByRole("row").toHaveCount(21) swallows all failures; change it to catch
the specific timeout case (e.g., check error.name === "TimeoutError" or error
instanceof playwright.errors.TimeoutError) and only perform page.reload() and
the retry flow when it is that timeout error, otherwise rethrow (or at minimum
log the caught error before rethrowing) so unrelated failures (page
crash/navigation/context closed) surface; update the catch block that wraps
expect(page.getByRole("row")).toHaveCount to inspect the caught error and
conditionally run page.reload(), the subsequent expects (page.getByText("Loading
lab sections..."), page.getByRole("button", { name: "Create Lab Section" }), and
the final toHaveCount retry) only when appropriate.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0fbd26ac-2ea9-43ff-89ef-e44ef0587e5e
📒 Files selected for processing (20)
.github/workflows/deploy.ymlapp/course/[course_id]/assignments/[assignment_id]/finalizeSubmissionEarly.tsxapp/course/[course_id]/discussion/[root_id]/page.tsxapp/course/[course_id]/manage/gradebook/GradebookPopoverProvider.tsxapp/course/[course_id]/manage/gradebook/gradebookTable.tsxapp/course/[course_id]/office-hours/[queue_id]/new/newRequestForm.tsxcomponents/ui/code-file.tsxcomponents/ui/markdown-file-preview.tsxhooks/useCourseController.tsxhooks/useDiscussionThreadRootController.tsxlib/TableController.tsplaywright.config.tssupabase/migrations/20260424140000_gradebook_cell_recalc_value_changed_gating.sqltests/e2e/TestingUtils.tstests/e2e/discussion-threads.test.tsxtests/e2e/gradebook.test.tsxtests/e2e/grading.test.tsxtests/e2e/lab-sections.test.tsxtests/e2e/office-hours.test.tsxtests/e2e/pseudonymous-grading.test.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/e2e/office-hours.test.tsx
- playwright.config.ts
- tests/e2e/grading.test.tsx
- tests/e2e/TestingUtils.ts
- tests/e2e/pseudonymous-grading.test.tsx
- tests/e2e/gradebook.test.tsx
After 21f7f34 made ColumnDefs stable across `gradebookDataEpoch` ticks (reading score values via `scoreMapsRef` instead of `scoreMaps` to avoid the column-header DOM-detach race during column reorder), TanStack Table's sorted/filtered row models — which it memoizes by `(data, columns)` reference — never re-run their accessor closures on score updates. A user sorted by Participation would see stale row order after editing a cell; a filter like "score > 80%" would show stale membership. Fix: re-create the `studentProfiles` array (used as `data:` to `useReactTable`) per `gradebookDataEpoch` tick. New array reference invalidates TanStack's row-model memos and forces accessors to re-run against the live `scoreMapsRef`, without rebuilding any ColumnDef and without disturbing the column-header DOM. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR is ready to merge; future CI signal comes via pull_request_target on the PR itself, not a branch-name push. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Round 2 of PR review fixes (commit 76460b7 review): - deploy.yml concurrency group: was keyed on github.ref, which under pull_request_target resolves to the base branch and would collide for PRs against the same target. Now keys on pull_request.number when present, falling back to github.ref for non-PR contexts. - GradebookPopoverProvider.handleFormSuccess: call close() so successful submission tears down `selected`/`anchorRect`/`targetRef` like every other dismiss path does, not just `setIsOpen(false)`. - useDiscussionThreadRootController: add .catch on the background start() — was silently swallowing rejections, leaving threads with a half-initialized realtime controller and no diagnostics. - lab-sections.test.tsx beforeEach: scope the menu data-state assertion to the Course Settings menu by hasText filter — bare selector matched both Course Settings and the UserMenu in dynamicCourseNav, tripping Playwright strict mode. Also narrow the row-count catch to the toHaveCount timeout case so unrelated failures (page crash, nav, etc.) don't get masked by a reload retry. - newRequestForm toast copy: now matches the destination — was saying "Redirecting to queue view" but actually goes to the individual request page. - code-file.tsx allowEmptyMessage: coerce to boolean (was a truthy RubricCheck object), aligning with the markdown twin. - gradebook batch recalc: drop unused `changed_rows` accumulator. New migration; behaviour unchanged. Skipped explicit "future work" items from the review: - code-file/markdown rollback-on-error (re-open popover with prior state) - Migration O(n²) dedup scan over `all_rows_to_enqueue` Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
hooks/useDiscussionThreadRootController.tsx (1)
252-259: Remove redundantthreadRealTimeController.close()call in cleanup.
controllersRef.current.threadController.close()(line 254) already closes the realtime controller at line 170 viaDiscussionThreadsController.close(). The direct close at line 256 is redundant, and there's also a third close in the.catchhandler at line 223. While idempotent, this pattern creates maintenance risk if teardown logic later becomes non-idempotent.Let
DiscussionThreadsController.close()own the realtime controller teardown and remove the direct close from the cleanup function.♻️ Suggested cleanup
return () => { if (controllersRef.current) { controllersRef.current.threadController.close(); controllersRef.current.tableController.close(); - controllersRef.current.threadRealTimeController.close(); controllersRef.current = null; } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useDiscussionThreadRootController.tsx` around lines 252 - 259, The cleanup return currently calls controllersRef.current.threadController.close(), controllersRef.current.tableController.close(), and controllersRef.current.threadRealTimeController.close(); remove the direct call to controllersRef.current.threadRealTimeController.close() so that DiscussionThreadsController.close() (invoked by threadController.close()) is the single owner of realtime teardown; keep the threadController.close() and tableController.close() calls and the controllersRef.current = null assignment, and ensure no other code relies on explicit realtime controller closing in this cleanup path..github/workflows/deploy.yml (1)
272-278: Server logs won't upload on cancellation (e.g., job timeout) — exactly when you'd most want them.
Stop background servicesruns onalways()and populatesserver-logs/, but this upload step gates on!cancelled(), so a timeout-cancelled run (the 75-minute cap) drops the supabase container logs on the floor. Switching toif: always()here matches the teardown step and keeps the artifact available for cancellation post-mortems. The existingplaywright-reportupload at the bottom can stay as-is since Playwright won't have produced a useful report on cancellation.🔧 Suggested change
- name: Upload server logs - if: ${{ !cancelled() }} + if: always() uses: actions/upload-artifact@v4 with: name: server-logs path: server-logs/ retention-days: 14🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/deploy.yml around lines 272 - 278, The "Upload server logs" GitHub Actions step currently uses the condition if: ${{ !cancelled() }}, which prevents uploading server-logs/ when a run is cancelled (e.g., timeout); update that step (the job step named "Upload server logs" that uses actions/upload-artifact@v4 with name: server-logs and path: server-logs/) to use if: always() so the artifact is uploaded even on cancelled runs and matches the "Stop background services" teardown behavior.tests/e2e/lab-sections.test.tsx (1)
155-167: Consider using Playwright'smatcherResultproperty instead of message substring matching for better resilience to future upgrades.The current approach of checking
message.includes("toHaveCount")works correctly on supported Playwright versions, but couples the test to the error message format. Playwright'sexpect()assertion errors expose structured metadata viaerror.matcherResult, which includes the actual vs. expected counts. This structured data survives minor version upgrades better than message string patterns.Note: The error thrown by
expect(...)timeouts is a standard Error class, notTimeoutError(which is reserved for action-level timeouts likelocator.click({ timeout })). You can distinguish timeout failures by checkingerror.message.includes('Timeout')or accessing thematcherResultproperty directly for programmatic handling.Not blocking — the current code is correct on supported Playwright versions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/lab-sections.test.tsx` around lines 155 - 167, Replace the brittle message substring check with inspection of the assertion's structured metadata: in the catch block for the expect(page.getByRole("row")).toHaveCount(...) call, treat the error as any and check for err.matcherResult (and optionally err.matcherResult.matcherName === "toHaveCount") to decide whether to swallow and retry; rethrow for any error without matcherResult or with a different matcherName so real failures (page crash, navigation failure) still fail the test. Use the same locator and expectation symbols (page.getByRole, toHaveCount) to locate the code to change.app/course/[course_id]/manage/gradebook/GradebookPopoverProvider.tsx (1)
161-194: Optional:handleFormSuccessis a no-op wrapper aroundclose.Since
closeis already memoized with[]deps (stable across renders),handleFormSuccess = useCallback(() => close(), [close])is functionally identical to passingclosedirectly. Happy to keep it for the explanatory comment, but the indirection isn't doing any work.♻️ Optional simplification
- // Reuse close() so submission-success teardown matches every other dismiss - // path — keeps `selected`, `anchorRect`, and `targetRef` from going stale. - const handleFormSuccess = useCallback(() => close(), [close]); - return ( <GradebookPopoverContext.Provider value={contextValue}> {children} {/* Fixed-positioned invisible anchor that the popover uses as its trigger/reference */} <Popover.Root open={isOpen} onOpenChange={(d) => setIsOpen(d.open)} closeOnInteractOutside={false} positioning={{ placement: "bottom", strategy: "fixed" }} > <Popover.Trigger asChild> <Box position="fixed" top={anchorRect ? `${anchorRect.top + anchorRect.height}px` : "-10000px"} left={anchorRect ? `${anchorRect.left}px` : "-10000px"} width={anchorRect ? `${anchorRect.width}px` : "0px"} height="0px" zIndex={10000} pointerEvents="none" aria-hidden /> </Popover.Trigger> {isOpen && selected ? ( <Popover.Positioner> <Box ref={contentRef} zIndex={90000}> <SelectedPopoverContent columnId={selected.columnId} studentId={selected.studentId} - onSuccess={handleFormSuccess} + // Reuse close() so success teardown matches every other dismiss path. + onSuccess={close} /> </Box> </Popover.Positioner> ) : null} </Popover.Root> </GradebookPopoverContext.Provider> );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/course/`[course_id]/manage/gradebook/GradebookPopoverProvider.tsx around lines 161 - 194, handleFormSuccess is just a no-op wrapper around the already-stable close, so simplify by removing handleFormSuccess and pass close directly as the onSuccess prop to SelectedPopoverContent (replace onSuccess={handleFormSuccess} with onSuccess={close}); if you prefer to keep an alias for clarity, ensure the alias is the same reference (const handleFormSuccess = close) rather than useCallback, and update any references to handleFormSuccess accordingly so there is no unnecessary memoization layer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/deploy.yml:
- Around line 258-269: The Stop background services step is unguarded and may
kill unrelated processes using stale /tmp PIDs (edge-functions.pid,
next-server.pid); ensure PID files are namespaced and validated before killing
by either clearing those PID files at the start of the job (or before the steps
that write them), or switch to run-specific PID paths (e.g. include
github.run_id in the filenames) and validate a PID belongs to the expected
process before calling kill (for example check the process command/owner matches
the expected edge-functions/next-server) so the kill in the "Stop background
services" step only targets our own processes.
In `@tests/e2e/gradebook.test.tsx`:
- Around line 128-141: The helper waitForGradebookRecalculationsIdle currently
inspects only the first pulse; change it to operate on the full set by using
page.locator(".gradebook-cell-pulse") (e.g., const pulses =
page.locator(".gradebook-cell-pulse")), early-return if pulses.count() === 0,
and otherwise wait until the set drains by polling via page.waitForFunction that
returns true when document.querySelectorAll(".gradebook-cell-pulse") is empty or
every pulse element is not visible (use getComputedStyle/offsetParent checks).
Update the function body to reference pulses and remove .first() so the helper
only returns when all recalculation pulses have disappeared or the timeout
elapses.
---
Nitpick comments:
In @.github/workflows/deploy.yml:
- Around line 272-278: The "Upload server logs" GitHub Actions step currently
uses the condition if: ${{ !cancelled() }}, which prevents uploading
server-logs/ when a run is cancelled (e.g., timeout); update that step (the job
step named "Upload server logs" that uses actions/upload-artifact@v4 with name:
server-logs and path: server-logs/) to use if: always() so the artifact is
uploaded even on cancelled runs and matches the "Stop background services"
teardown behavior.
In `@app/course/`[course_id]/manage/gradebook/GradebookPopoverProvider.tsx:
- Around line 161-194: handleFormSuccess is just a no-op wrapper around the
already-stable close, so simplify by removing handleFormSuccess and pass close
directly as the onSuccess prop to SelectedPopoverContent (replace
onSuccess={handleFormSuccess} with onSuccess={close}); if you prefer to keep an
alias for clarity, ensure the alias is the same reference (const
handleFormSuccess = close) rather than useCallback, and update any references to
handleFormSuccess accordingly so there is no unnecessary memoization layer.
In `@hooks/useDiscussionThreadRootController.tsx`:
- Around line 252-259: The cleanup return currently calls
controllersRef.current.threadController.close(),
controllersRef.current.tableController.close(), and
controllersRef.current.threadRealTimeController.close(); remove the direct call
to controllersRef.current.threadRealTimeController.close() so that
DiscussionThreadsController.close() (invoked by threadController.close()) is the
single owner of realtime teardown; keep the threadController.close() and
tableController.close() calls and the controllersRef.current = null assignment,
and ensure no other code relies on explicit realtime controller closing in this
cleanup path.
In `@tests/e2e/lab-sections.test.tsx`:
- Around line 155-167: Replace the brittle message substring check with
inspection of the assertion's structured metadata: in the catch block for the
expect(page.getByRole("row")).toHaveCount(...) call, treat the error as any and
check for err.matcherResult (and optionally err.matcherResult.matcherName ===
"toHaveCount") to decide whether to swallow and retry; rethrow for any error
without matcherResult or with a different matcherName so real failures (page
crash, navigation failure) still fail the test. Use the same locator and
expectation symbols (page.getByRole, toHaveCount) to locate the code to change.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d963e2c7-579e-4c6e-a976-9edf555cce22
📒 Files selected for processing (14)
.github/workflows/deploy.ymlapp/course/[course_id]/assignments/[assignment_id]/finalizeSubmissionEarly.tsxapp/course/[course_id]/manage/gradebook/GradebookPopoverProvider.tsxapp/course/[course_id]/manage/gradebook/gradebookTable.tsxapp/course/[course_id]/office-hours/[queue_id]/new/newRequestForm.tsxcomponents/ui/code-file.tsxhooks/useDiscussionThreadRootController.tsxsupabase/migrations/20260424150000_gradebook_cell_recalc_lock_old_row.sqlsupabase/migrations/20260424160000_gradebook_cell_recalc_drop_dead_state.sqltests/e2e/TestingUtils.tstests/e2e/due-dates.test.tsxtests/e2e/gradebook.test.tsxtests/e2e/grading.test.tsxtests/e2e/lab-sections.test.tsx
…edundancies Round 3 of PR review fixes (commit eaac2ed review): - deploy.yml: namespace PID/log files under $RUN_TMP=/tmp/e2e-${run_id}-${run_attempt} to prevent the cleanup `kill $(cat ...pid)` from targeting a stale PID that the OS had recycled to an unrelated process on a persistent self-hosted runner. - deploy.yml: `Upload server logs` → `if: always()` so timeout-cancelled runs (the case where these logs are most useful) still upload. - gradebook.test.tsx: `waitForGradebookRecalculationsIdle` waits for the *full* `.gradebook-cell-pulse` set to drain via page.waitForFunction, not just `.first()` — partial-idle was masquerading as fully-idle. - useDiscussionThreadRootController cleanup: drop the redundant direct `threadRealTimeController.close()` — `threadController.close()` already closes the realtime controller (DiscussionThreadsController: 168). Idempotent today, but the chain is brittle if teardown ever becomes non-idempotent. - GradebookPopoverProvider: inline `close` as `onSuccess` directly; `handleFormSuccess` was a no-op `useCallback(() => close(), [close])`. - lab-sections.test.tsx: narrow the row-count `catch` by inspecting `err.matcherResult.name === "toHaveCount"` instead of substring-matching the message string — the message format isn't part of Playwright's API contract. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Folds three iterative migrations into a single CREATE OR REPLACE of both
wrappers with the final body:
- 20260424140000_gradebook_cell_recalc_value_changed_gating.sql
(original — value-changed gating, missing FOR UPDATE, dead changed_rows)
- 20260424150000_gradebook_cell_recalc_lock_old_row.sql
(added FOR UPDATE on the old_row read in both wrappers)
- 20260424160000_gradebook_cell_recalc_drop_dead_state.sql
(dropped the unread changed_rows accumulator from the batch wrapper)
Final 20260424140000 contains both fixes inline; the two follow-ups are
deleted. Header explains the bug, the value-changed gating, and the
concurrent-edits race that the FOR UPDATE prevents. Behaviour is identical
to applying all three iteratively. Local schema_migrations cleaned.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Will be removed again immediately before merge. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
.github/workflows/deploy.yml (2)
7-8:⚠️ Potential issue | 🟡 MinorRemove the temporary branch trigger before merging.
The
ci/parallel-e2e-local-supabasebranch entry is still marked temporary and keeps this workflow trigger scope wider than intended.Suggested cleanup
push: branches: - main - staging - # TODO(remove-before-merge): one final pre-merge validation lap. - - ci/parallel-e2e-local-supabase🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/deploy.yml around lines 7 - 8, Remove the temporary branch trigger entry "ci/parallel-e2e-local-supabase" from the workflow triggers (the list that currently has the TODO comment) so the workflow no longer runs for that branch; locate the TODO(remove-before-merge) line and delete the "- ci/parallel-e2e-local-supabase" item (and the TODO comment) to narrow the trigger scope before merging.
9-10:⚠️ Potential issue | 🔴 Critical
pull_request_target+ PR head checkout + secrets is a privileged execution path.Line 9 uses
pull_request_target, while Line 54 checks out PR head code and later runs build/tests with repository secrets. That combination can execute untrusted code in a privileged context unless fork access is explicitly blocked.Minimal in-workflow guard
e2e-local: + if: github.event_name != 'pull_request_target' || github.event.pull_request.head.repo.fork == false runs-on: pawtograder-e2eAlso applies to: 52-55, 87-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/deploy.yml around lines 9 - 10, The workflow uses pull_request_target together with checking out PR head and running steps that access repository secrets (referenced as pull_request_target and actions/checkout@v3 plus uses of secrets), which allows untrusted PR code to run with elevated privileges; either change the trigger from pull_request_target to pull_request so the checkout of PR code runs in the PR context, or keep pull_request_target but do NOT checkout the PR head nor expose secrets — instead checkout the merge commit or the base repo and run only safe actions; alternatively add a guard (e.g., if: github.repository == github.event.pull_request.head.repo.full_name) before the checkout/secret-using steps to ensure the PR comes from the same repo and avoid using secrets for forked PRs.tests/e2e/gradebook.test.tsx (1)
128-143:⚠️ Potential issue | 🟡 MinorAvoid early-return race in recalculation-idle helper.
If no pulse exists at the exact first check, the function exits immediately even if pulses appear right after; that can reintroduce the same flake window.
Suggested fix
async function waitForGradebookRecalculationsIdle(page: Page, timeoutMs = 30_000) { - const pulses = page.locator(".gradebook-cell-pulse"); - if ((await pulses.count()) === 0) return; - await page.waitForFunction(() => document.querySelectorAll(".gradebook-cell-pulse").length === 0, { - timeout: timeoutMs - }); + await page.waitForFunction( + () => { + const win = window as Window & { __gradebookPulseIdleSince?: number }; + const activeCount = document.querySelectorAll(".gradebook-cell-pulse").length; + if (activeCount > 0) { + win.__gradebookPulseIdleSince = undefined; + return false; + } + const now = performance.now(); + if (win.__gradebookPulseIdleSince == null) { + win.__gradebookPulseIdleSince = now; + return false; + } + return now - win.__gradebookPulseIdleSince >= 150; + }, + { polling: "raf", timeout: timeoutMs } + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/gradebook.test.tsx` around lines 128 - 143, The helper waitForGradebookRecalculationsIdle currently returns immediately if pulses.count() === 0, which creates a race where pulses appearing just after the check are missed; remove the early return and instead always wait for the DOM to reach zero pulses by using page.waitForFunction(() => document.querySelectorAll(".gradebook-cell-pulse").length === 0, { timeout: timeoutMs }) so the function blocks until the full set drains (use the existing pulses locator/selector ".gradebook-cell-pulse" and the waitForFunction call in waitForGradebookRecalculationsIdle).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/course/`[course_id]/manage/gradebook/GradebookPopoverProvider.tsx:
- Around line 60-61: The checks for auto-calculated flags treat undefined as
truthy because they use !== null; update the null checks in
GradebookPopoverProvider (the expressions that set isAutoCalculated and
showWarning, which reference column?.score_expression and column?.external_data)
to guard against both null and undefined (e.g., use != null-style checks or
explicit !== undefined && !== null) so loading/undefined values don't
incorrectly flip the flags; change both the isAutoCalculated and showWarning
expressions accordingly.
In `@hooks/useDiscussionThreadRootController.tsx`:
- Around line 220-224: The startup failure path currently calls
threadRealTimeController.close() without handling its Promise; update the catch
handler for threadRealTimeController.start() to await or explicitly handle the
async close() result and surface any errors (e.g., call
threadRealTimeController.close().catch(err => processLogger/error handler) or
use an async IIFE to await it), ensuring any close failures are logged alongside
the original start error; reference the threadRealTimeController.start() catch
block and threadRealTimeController.close() when applying the change.
- Around line 207-209: The early return in the effect inside
useDiscussionThreadRootController when courseController?.client is falsy leaves
a stale controller in state; update that no-client branch to explicitly clear
the discussion controller state (e.g., call the hook's setter like
setDiscussionController(null) or dispatch the appropriate clear action) so the
controller is reset when courseController.client becomes unavailable, instead of
returning early and keeping the old controller.
---
Duplicate comments:
In @.github/workflows/deploy.yml:
- Around line 7-8: Remove the temporary branch trigger entry
"ci/parallel-e2e-local-supabase" from the workflow triggers (the list that
currently has the TODO comment) so the workflow no longer runs for that branch;
locate the TODO(remove-before-merge) line and delete the "-
ci/parallel-e2e-local-supabase" item (and the TODO comment) to narrow the
trigger scope before merging.
- Around line 9-10: The workflow uses pull_request_target together with checking
out PR head and running steps that access repository secrets (referenced as
pull_request_target and actions/checkout@v3 plus uses of secrets), which allows
untrusted PR code to run with elevated privileges; either change the trigger
from pull_request_target to pull_request so the checkout of PR code runs in the
PR context, or keep pull_request_target but do NOT checkout the PR head nor
expose secrets — instead checkout the merge commit or the base repo and run only
safe actions; alternatively add a guard (e.g., if: github.repository ==
github.event.pull_request.head.repo.full_name) before the checkout/secret-using
steps to ensure the PR comes from the same repo and avoid using secrets for
forked PRs.
In `@tests/e2e/gradebook.test.tsx`:
- Around line 128-143: The helper waitForGradebookRecalculationsIdle currently
returns immediately if pulses.count() === 0, which creates a race where pulses
appearing just after the check are missed; remove the early return and instead
always wait for the DOM to reach zero pulses by using page.waitForFunction(() =>
document.querySelectorAll(".gradebook-cell-pulse").length === 0, { timeout:
timeoutMs }) so the function blocks until the full set drains (use the existing
pulses locator/selector ".gradebook-cell-pulse" and the waitForFunction call in
waitForGradebookRecalculationsIdle).
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5e4c5fa5-e869-4bb0-80c8-b3c56ea8bcd1
📒 Files selected for processing (6)
.github/workflows/deploy.ymlapp/course/[course_id]/manage/gradebook/GradebookPopoverProvider.tsxhooks/useDiscussionThreadRootController.tsxsupabase/migrations/20260424140000_gradebook_cell_recalc_value_changed_gating.sqltests/e2e/gradebook.test.tsxtests/e2e/lab-sections.test.tsx
✅ Files skipped from review due to trivial changes (1)
- supabase/migrations/20260424140000_gradebook_cell_recalc_value_changed_gating.sql
…ync close CodeRabbit findings on commit 7d76254: - GradebookPopoverProvider SelectedPopoverContent: `!== null` mis-flags loading state as auto-calculated because `column` can be `undefined` while the controller is hydrating. Switch to `!= null` so both null and undefined are filtered out. - useDiscussionThreadRootController effect: when `courseController?.client` becomes unavailable mid-lifecycle, the early return left the previous controller in context. Consumers then render with a stale, already- closed reference. Call setController(null) before the early return. - useDiscussionThreadRootController start() failure path: close() is async; calling it without chaining .catch surfaces teardown failures as unhandled rejections. Wrap with `void close().catch(...)`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Run 25024552721 on a0893f1 went green (431 passed, 0 hard fails); restore pull_request_target as the only PR-driven trigger. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/deploy.yml (1)
264-277: Optional: remove run-scoped temp dir during teardown.
$RUN_TMPis run-unique (good), but on persistent self-hosted runners these directories can accumulate over time. Consider cleaning it at the end of teardown.♻️ Optional cleanup diff
- name: Stop background services if: always() run: | kill "$(cat $RUN_TMP/edge-functions.pid 2>/dev/null)" 2>/dev/null || true kill "$(cat $RUN_TMP/next-server.pid 2>/dev/null)" 2>/dev/null || true # Capture full Supabase container logs while the stack is still up. mkdir -p server-logs for c in $(docker ps --filter "label=com.supabase.cli.project=${SUPABASE_PROJECT}" --format '{{.Names}}'); do docker logs "$c" > "server-logs/${c}.log" 2>&1 || true done cp -f $RUN_TMP/edge-functions.log server-logs/edge-functions.log 2>/dev/null || true cp -f $RUN_TMP/next-server.log server-logs/next-server.log 2>/dev/null || true npx supabase stop --no-backup || true + rm -rf "$RUN_TMP" || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/deploy.yml around lines 264 - 277, The teardown step "Stop background services" leaves the run-unique temp dir ($RUN_TMP) behind on self-hosted runners; after capturing logs and stopping supabase (in the block that runs always), add a safe cleanup of the temp directory (e.g., test for existence and run rm -rf "$RUN_TMP" || true) at the end of the step so artifacts don't accumulate; reference the existing commands mkdir -p server-logs, cp -f $RUN_TMP/edge-functions.log, cp -f $RUN_TMP/next-server.log and npx supabase stop to place the cleanup after those operations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/deploy.yml:
- Around line 264-277: The teardown step "Stop background services" leaves the
run-unique temp dir ($RUN_TMP) behind on self-hosted runners; after capturing
logs and stopping supabase (in the block that runs always), add a safe cleanup
of the temp directory (e.g., test for existence and run rm -rf "$RUN_TMP" ||
true) at the end of the step so artifacts don't accumulate; reference the
existing commands mkdir -p server-logs, cp -f $RUN_TMP/edge-functions.log, cp -f
$RUN_TMP/next-server.log and npx supabase stop to place the cleanup after those
operations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 69005ecc-f407-4c3f-8915-e3faca81e14a
📒 Files selected for processing (3)
.github/workflows/deploy.ymlapp/course/[course_id]/manage/gradebook/GradebookPopoverProvider.tsxhooks/useDiscussionThreadRootController.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- app/course/[course_id]/manage/gradebook/GradebookPopoverProvider.tsx
- hooks/useDiscussionThreadRootController.tsx
jon-bell
added a commit
that referenced
this pull request
Apr 28, 2026
CI run 25064577029 surfaced 6 a11y-smoke failures (chromium + webkit × 3) after Jonathan's commit 5a4366d cleaned up dynamicCourseNav so the responsive mobile/desktop twins don't both expose `id="primary-nav"` — the hidden subtree now gets `aria-hidden={true}`. That's the right fix for the original "two navs with the same id" concern raised in PR review. But my `assertPageHasLandmarks` helper queried `nav, [role="navigation"]` via raw DOM CSS selector, so it still found the aria-hidden subtree and then called `toHaveAccessibleName(/\S/)` on it — which returns "" because aria-hidden removes the element from the a11y tree. Switch to Playwright's `getByRole("main")` / `getByRole("navigation")` which respect the accessibility tree and skip aria-hidden subtrees, matching what screen readers actually see. Verified locally before pushing: 10/10 a11y-smoke pass. Triage of the other 8 CI failures (an agent ran in parallel): zero real regressions on this branch. 7 are pre-existing flakes flagged in prior commit messages (due-dates × 2, gradebook column reorder webkit, enrollments dup row, lab-sections × 2, survey-assignment-grading realtime lag). 1 (office-hours waitForURL) was flagged when the test was added in the staging-merge PR #734 — it's a router.push race in newRequestForm.tsx, separate surface owner. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 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
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.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements