Implement MVP Feedbot #195
Conversation
WalkthroughAdds an LLM hint API and UI integration with provider-aware LLM calls, caching, rate-limits, and usage tracking; new DB tables/types for hint feedback, LLM usage, and video meeting users; expanded metrics RPC/Edge function and Grafana dashboard; Pyret REPL UI and tests; admin Supabase client helper; Chime attendee tracking; various scripts, tests, config, and docs updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Results Page
participant API as /api/llm-hint
participant SB as Supabase (RLS + Service)
participant LLM as Provider (OpenAI/Azure/Anthropic)
User->>UI: Click "Get Hint"
UI->>API: POST { testId } with session cookie
API->>SB: Authenticate user, load grader_result_tests(+relations)
alt Not found/unauthorized
API-->>UI: 404/401
else Has cached result
API-->>UI: 200 { cached: true, response }
else No cache
API->>API: checkRateLimits()
API->>LLM: Generate hint (model/provider/account)
alt No response/error
API-->>UI: 500/429 with message
else Success
API->>SB: Update grader_result_tests.extra_data.llm.result
API->>SB: Insert llm_inference_usage (tokens/labels)
API-->>UI: 200 { cached: false, response }
end
end
sequenceDiagram
autonumber
participant SIS as SIS API
participant FN as course-import-sis Edge
participant Ret as fetchWithRetry
participant Sentry as Sentry
FN->>Ret: fetch(url, opts, retries)
loop attempts ≤ maxRetries
Ret->>SIS: HTTP request
alt 2xx
SIS-->>Ret: 200 OK
Ret-->>FN: Response
else 429/503 with Retry-After
SIS-->>Ret: 429/503
Ret->>Sentry: breadcrumb: retry-after/backoff
Ret->>Ret: wait(exponential+jitter or Retry-After)
else 5xx/408
SIS-->>Ret: error
Ret->>Sentry: breadcrumb: retry
Ret->>Ret: wait(backoff)
else 4xx (not 408/429)
SIS-->>Ret: 4xx
Ret-->>FN: throw UserVisibleError
end
end
sequenceDiagram
autonumber
participant Chime as AWS Chime SNS
participant Callback as live-meeting-callback
participant Wrapper as _shared/ChimeWrapper
participant SB as Supabase (Service)
Chime->>Callback: POST SNS event
Callback->>Callback: Validate Authorization
alt Unauthorized
Callback-->>Chime: 401
else Authorized
Callback->>Wrapper: processSNSMessage(body.body)
alt AttendeeJoined
Wrapper->>SB: upsert video_meeting_session_users (joined_at)
else AttendeeLeft
Wrapper->>SB: update record set left_at
else MeetingStarted/Ended
Wrapper->>SB: update help_requests / sessions
end
Callback-->>Chime: 200 OK
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
app/api/llm-hint/route.ts (6)
125-133: Reduce duplicate generations under race conditionsTwo concurrent requests can both miss the cache and double-generate. Consider re-checking after generation or performing a conditional update (only when llm_hint_result is null) via a single update with a WHERE json path predicate.
160-166: Guard missing Supabase service env and fall back safelyIf service-role env vars are missing, the update will fail; explicitly check and surface a 500 or fall back to user client update (if RLS allows).
- const serviceSupabase = createServiceClient(process.env.SUPABASE_URL!, process.env.SUPABASE_SERVICE_ROLE_KEY!); + if (!process.env.SUPABASE_URL || !process.env.SUPABASE_SERVICE_ROLE_KEY) { + console.error("Service role env vars missing; cannot cache LLM hint."); + } + const serviceSupabase = + process.env.SUPABASE_URL && process.env.SUPABASE_SERVICE_ROLE_KEY + ? createServiceClient(process.env.SUPABASE_URL, process.env.SUPABASE_SERVICE_ROLE_KEY) + : null;- const { error: updateError } = await serviceSupabase + const { error: updateError } = serviceSupabase + ? await serviceSupabase .from("grader_result_tests") .update({ extra_data: updatedExtraData }) .eq("id", testId); + : { error: null };
119-124: Validate prompt size to avoid context-length errorsVery large prompts will 400/422 from the API. Consider rejecting or truncating prompts above a safe threshold (e.g., ~32k tokens equivalent) and returning a 413 with guidance.
172-177: Return model info for observabilityInclude model name in the response to aid debugging when OPENAI_MODEL changes.
- return NextResponse.json({ + return NextResponse.json({ success: true, response: aiResponse, usage: completion.usage, + model: completion.model, cached: false });
12-15: Use 503 for missing upstream configurationMissing OPENAI_API_KEY is a temporary upstream config issue; 503 better signals retry semantics than 500.
- return NextResponse.json({ error: "OpenAI API key missing" }, { status: 500 }); + return NextResponse.json({ error: "OpenAI API key missing" }, { status: 503 });
178-182: Prefer structured logging (Sentry) over console.errorGiven @sentry/nextjs is present, capture exceptions for better traceability.
app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/layout.tsx (1)
63-83: Avoid duplicating icon selection logic across filesBoth this file and results/page.tsx implement “robot if hint else status icon.” Consider extracting a small helper to keep behavior in sync.
app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx (2)
75-87: Use Button’s loading prop for consistent UX and accessibilityLeverage the existing loading UI pattern used elsewhere in the app.
- <Button onClick={handleGetHint} disabled={isLoading} colorPalette="blue" variant="outline" size="sm"> - {isLoading ? ( + <Button onClick={handleGetHint} loading={isLoading} disabled={isLoading} colorPalette="blue" variant="outline" size="sm"> + {isLoading ? ( <> <FaSpinner className="animate-spin" /> Getting Feedbot Response... </> ) : ( <> <FaRobot /> Get Feedbot Response </> )} </Button>
444-452: Consider consistent icon rendering in the Status cellUsing Chakra’s here (as in the sidebar list) would align styling and theming; emojis can differ across platforms.
- return result.score === result.max_score ? "✅" : "❌"; + return result.score === result.max_score ? <Text color="fg.success">✔︎</Text> : <Text color="fg.error">✘</Text>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
app/api/llm-hint/route.ts(1 hunks)app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/layout.tsx(2 hunks)app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx(6 hunks)package.json(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-30T00:20:04.201Z
Learnt from: Ari-S-123
PR: pawtograder/platform#168
File: app/course/[course_id]/manage/course/due-date-extensions/layout.tsx:52-69
Timestamp: 2025-08-30T00:20:04.201Z
Learning: Chakra UI v3 Button component supports the `asChild` prop, which allows rendering the Button as another component (like NextLink) while maintaining Button styling and functionality. This is documented in the official Chakra UI docs and is the recommended pattern for creating button-styled links.
Applied to files:
app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx
🧬 Code graph analysis (2)
app/api/llm-hint/route.ts (2)
tests/e2e/TestingUtils.ts (1)
supabase(11-11)utils/supabase/client.ts (1)
createClient(4-9)
app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx (1)
components/ui/markdown.tsx (1)
Markdown(67-81)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy
🔇 Additional comments (6)
package.json (1)
97-97: Confirmed OpenAI SDK is server-only
Verified imports showimport OpenAI from "openai"only inapp/api/llm-hint/route.ts(a server-side route). No client-side imports detected.app/api/llm-hint/route.ts (1)
45-68: Access control path looks correctOwnership, group membership, and instructor/grader role checks are coherent and scoped to the submission’s class.
app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/layout.tsx (1)
442-444: Nice precedence: show robot when an LLM hint existsClear visual affordance for hint-capable tests; keeps the existing iconMap as fallback.
app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx (3)
129-137: Good: Hint flow defers API cost until clicked and hides the raw promptOn-demand generation + caching matches the stated MVP.
390-394: Nice reuse: unified formatting for lint outputSwitch to format_basic_output is clean and keeps markdown/text handling consistent.
97-126: Default to hint when present; allow fallback to raw outputThe display precedence is sensible and keeps the student view focused.
...course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Looks like a great first pass! I think that collecting feedback on usefulness is important, even for MVP... here is a prompt for an LLM:
Add a new supabase migration to this PR, introducing a new table: assignment_hint_feedback, columns: class_id int8, submission_id int8, hint text, boolean useful, text comment, uuid created_by. class is FK to classes, submission FK to submissions, created_by FK to profiles. RLS on read is authorize_for_submission(submission_id), on insert is that PLUS authorizeforprofile(created_by). Add a simple react component that allows for submitting feedback after viewing a hint. Examine past migrations to ensure that the columns/types etc match up and use the same format.
…; other papercuts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
app/api/llm-hint/route.ts (1)
20-26: Harden payload parsing; return 400 on bad JSON and non-integer IDsCurrent code throws 500 on invalid JSON and accepts 0/strings inconsistently. Coerce and validate.
- const { testId } = await request.json(); - - Sentry.setTag("testId", testId); - - if (!testId || typeof testId !== "number" || testId < 0 || !Number.isInteger(testId)) { - return NextResponse.json({ error: "testId must be a non-negative integer" }, { status: 400 }); - } + const body = (await request.json().catch(() => ({}))) as { testId?: unknown }; + const testId = Number((body as any)?.testId); + Sentry.setTag("testId", String(testId)); + if (!Number.isInteger(testId) || testId <= 0) { + return NextResponse.json({ error: "Invalid testId (must be a positive integer)" }, { status: 400 }); + }app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx (1)
503-507: Corrected: preserves markdown for instructor-only outputsUsing output_format fixes the earlier rendering bug.
🧹 Nitpick comments (7)
utils/supabase/DatabaseTypes.d.ts (1)
12-23: Future-proof LLM extra_data type and make hide_score ergonomic
- Allow boolean for hide_score; code uses string comparison now.
- Keep "v1" but permit "feedbot-v1" to align with earlier evolution suggestion.
export type GraderResultTestExtraData = { llm?: { prompt: string; result?: string; model?: string; temperature?: number; max_tokens?: number; - type: "v1"; + type: "v1" | "feedbot-v1"; }; - hide_score?: string; + hide_score?: boolean | "true" | "false"; icon?: string; };app/api/llm-hint/route.ts (3)
125-131: Guard service-role env vars to avoid silent misconfig writesPrevent null client use; fail fast with 500 and Sentry.
- const serviceSupabase = createServiceClient(process.env.SUPABASE_URL!, process.env.SUPABASE_SERVICE_ROLE_KEY!); + if (!process.env.SUPABASE_URL || !process.env.SUPABASE_SERVICE_ROLE_KEY) { + Sentry.captureMessage("SUPABASE_URL or SERVICE_ROLE_KEY missing", "error"); + return NextResponse.json({ error: "Server misconfiguration" }, { status: 500 }); + } + const serviceSupabase = createServiceClient(process.env.SUPABASE_URL, process.env.SUPABASE_SERVICE_ROLE_KEY);
116-123: Preserve/initialize llm.type when writing backIf autograder didn’t set type, default it for forward compatibility.
const updatedExtraData: GraderResultTestExtraData = { ...extraData, llm: { ...extraData.llm, + type: extraData.llm?.type ?? "v1", result: aiResponse } };
147-152: Include cached flag and minimal metadata consistentlyGood shape. Consider echoing model and testId for client-side telemetry.
return NextResponse.json({ success: true, response: aiResponse, usage: completion.usage, - cached: false + cached: false, + model: apiParams.model, + testId });app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx (3)
58-61: Remove console.log to satisfy lint and avoid noisy client logsUse Sentry breadcrumb if needed, or drop it.
- if (data.cached) { - console.log("Feedbot response was retrieved from cache"); - } + // if (data.cached) { /* optionally show a UI affordance */ }
124-131: Nice UX: cancel in-flight fetch on unmountAvoids setting state on unmounted component if the user navigates away.
- <LLMHintButton testId={testId} onHintGenerated={setHintContent} /> + <LLMHintButton testId={testId} onHintGenerated={setHintContent} />Outside this range, implement inside LLMHintButton:
const handleGetHint = async () => { + const controller = new AbortController(); + const signal = controller.signal; setIsLoading(true); setError(null); try { - const response = await fetch("/api/llm-hint", { + const response = await fetch("/api/llm-hint", { method: "POST", headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ testId }) + body: JSON.stringify({ testId }), + signal }); @@ } finally { setIsLoading(false); } }; +useEffect(() => { + return () => controller.abort(); +}, []);
423-427: Handle boolean hide_score in addition to stringAligns with widened type and avoids surprises.
- (r.extra_data as GraderResultTestExtraData)?.hide_score !== "true" && + !( + (r.extra_data as GraderResultTestExtraData)?.hide_score === true || + (r.extra_data as GraderResultTestExtraData)?.hide_score === "true" + ) &&
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
app/api/llm-hint/route.ts(1 hunks)app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/layout.tsx(2 hunks)app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx(7 hunks)utils/supabase/DatabaseTypes.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/layout.tsx
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-30T00:20:04.201Z
Learnt from: Ari-S-123
PR: pawtograder/platform#168
File: app/course/[course_id]/manage/course/due-date-extensions/layout.tsx:52-69
Timestamp: 2025-08-30T00:20:04.201Z
Learning: Chakra UI v3 Button component supports the `asChild` prop, which allows rendering the Button as another component (like NextLink) while maintaining Button styling and functionality. This is documented in the official Chakra UI docs and is the recommended pattern for creating button-styled links.
Applied to files:
app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx
📚 Learning: 2025-08-30T20:00:05.568Z
Learnt from: dbp
PR: pawtograder/platform#195
File: app/api/llm-hint/route.ts:134-146
Timestamp: 2025-08-30T20:00:05.568Z
Learning: In the OpenAI Node.js SDK, timeout is not a parameter of the chat.completions.create() method itself. Instead, timeout should be configured either at the client level when creating the OpenAI instance, or as an options parameter (second argument) to the create method, not within the main request parameters.
Applied to files:
app/api/llm-hint/route.ts
📚 Learning: 2025-08-23T18:05:42.129Z
Learnt from: jon-bell
PR: pawtograder/platform#173
File: supabase/migrations/20250823074648_create_system_notification_function.sql:296-327
Timestamp: 2025-08-23T18:05:42.129Z
Learning: In PostgreSQL/Supabase, SECURITY DEFINER functions run with the privileges of the function owner. According to PostgreSQL documentation, superusers and roles with the BYPASSRLS attribute always bypass the row security system, and table owners normally bypass row security as well. In Supabase, SECURITY DEFINER functions are typically owned by privileged roles (like the service role) that have superuser privileges, so they bypass RLS policies entirely. Therefore, INSERT policies are not needed for operations performed within SECURITY DEFINER functions owned by privileged roles.
Applied to files:
app/api/llm-hint/route.ts
📚 Learning: 2025-08-30T20:02:29.865Z
Learnt from: dbp
PR: pawtograder/platform#195
File: app/api/llm-hint/route.ts:134-146
Timestamp: 2025-08-30T20:02:29.865Z
Learning: The `user` parameter in OpenAI's chat.completions.create() method is deprecated, though it remains valid for attribution and abuse tracking purposes as of August 2025.
Applied to files:
app/api/llm-hint/route.ts
🧬 Code graph analysis (2)
app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx (2)
utils/supabase/DatabaseTypes.d.ts (1)
GraderResultTestExtraData(12-23)components/ui/markdown.tsx (1)
Markdown(67-81)
app/api/llm-hint/route.ts (1)
utils/supabase/DatabaseTypes.d.ts (1)
GraderResultTestExtraData(12-23)
🪛 GitHub Actions: Lint
app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx
[warning] 59-59: ESLint: Unexpected console statement. (no-console)
app/api/llm-hint/route.ts
[error] 82-82: ESLint: Unexpected any. Specify a different type. (no-explicit-any)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy
🔇 Additional comments (3)
app/api/llm-hint/route.ts (1)
66-71: Support legacy key ‘llm_hint_prompt’ for backward compatibilityBack-compat to avoid breaking autograders still sending
llm_hint_prompt.- const extraData = testResult.extra_data as GraderResultTestExtraData | null; - if (!extraData?.llm?.prompt) { + const extraData = testResult.extra_data as GraderResultTestExtraData | null; + const prompt = + extraData?.llm?.prompt ?? + // legacy field to match existing autograder output + (extraData as any)?.llm_hint_prompt; + if (!prompt) { return NextResponse.json({ error: "No LLM hint prompt found" }, { status: 400 }); }And update subsequent calls to use
promptinstead ofextraData.llm.prompt.app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx (2)
440-447: Status cell logic LGTMRobot icon precedence over pass/fail reads clean and matches spec.
489-495: Prop typing is sound; structural match is sufficientPassing the full test row where only output fields are consumed is acceptable here.
I did this, with some refinement. It, along with other changes, reflected in current state of this branch. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx (1)
771-776: Potential markdown loss: preferformatwhen present on test outputsSome test-output rows expose
format(notoutput_format). Use a small normalizer to support both; avoids regressions where markdown renders as plain text.- <CardBody> - {format_basic_output({ - output: output.output, - output_format: output.output_format as "text" | "markdown" - })} - </CardBody> + <CardBody>{format_test_output(output)}</CardBody>Add this helper near
format_output:function format_test_output(output: { output: string | null; output_format?: "text" | "markdown" | null; format?: "text" | "markdown" | null; }) { return format_basic_output({ output: output.output, output_format: output.output_format ?? output.format ?? null }); }
🧹 Nitpick comments (10)
utils/supabase/DatabaseTypes.d.ts (1)
12-23: Type shape for extra_data.hide_score should be boolean-friendlyDownstream code compares to the string "true". To avoid brittle string checks, type this as a boolean (optionally accept the legacy string) to ease adoption.
export type GraderResultTestExtraData = { llm?: { prompt: string; result?: string; model?: string; temperature?: number; max_tokens?: number; type: "v1"; }; - hide_score?: string; + // Prefer boolean; keep "true" for backward compat with existing payloads. + hide_score?: boolean | "true"; icon?: string; };app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx (7)
60-62: Remove console.log to satisfy lint (no-console) and keep UI cleanUse a debug logger or gate behind NODE_ENV, or simply drop it.
- if (data.cached) { - console.log("Feedbot response was retrieved from cache"); - } + // Optionally surface a subtle UI cue if needed.
158-163: Fix lint: unused catch variableThe variable isn’t referenced; use a bare catch.
- } catch (err) { + } catch { // No existing feedback is fine } finally {
249-253: Fix lint: unused catch variableSame here; remove the unused identifier.
- } catch (err) { + } catch { setError("An error occurred while submitting feedback"); } finally {
274-277: Escape quotes in JSX text to satisfy react/no-unescaped-entitiesRender curly quotes instead of raw ".
- {comment && ( - <Text fontSize="sm" color="green.600" mt={1}> - "{comment}" - </Text> - )} + {comment && ( + <Text fontSize="sm" color="green.600" mt={1}> + “{comment}” + </Text> + )}
73-85: Optional: use Chakra Spinner instead of icon + Tailwind class
className="animate-spin"depends on Tailwind. Chakra’s Spinner avoids that dependency drift.- <Button onClick={handleGetHint} disabled={isLoading} colorPalette="blue" variant="outline" size="sm"> - {isLoading ? ( - <> - <FaSpinner className="animate-spin" /> - Getting Feedbot Response... - </> - ) : ( + <Button onClick={handleGetHint} disabled={isLoading} colorPalette="blue" variant="outline" size="sm"> + {isLoading ? ( + <HStack gap={2}> + <Spinner size="xs" /> + <span>Getting Feedbot Response...</span> + </HStack> + ) : ( <> <FaRobot /> Get Feedbot Response </> )} </Button>
692-715: Status cell: add accessible label for the robot iconScreen readers won’t announce meaning of FaRobot. Add sr-only text.
- if (extraData?.llm?.prompt || extraData?.llm?.result) { - return <FaRobot />; + if (extraData?.llm?.prompt || extraData?.llm?.result) { + return ( + <HStack> + <FaRobot aria-label="LLM hint available" /> + <Text srOnly>LLM hint available</Text> + </HStack> + ); }
391-399: Nice UX win: prevent double-submitDisable the button while request in-flight (you already do), and consider debouncing additional clicks with an AbortController to cancel stale requests if unmounted.
supabase/migrations/20250830163212_grader_result_tests_hint_feedback.sql (2)
40-42: Unique constraint is good; consider composite for typical lookupsYou already have single-column indexes plus a unique pair. If common queries filter by (grader_result_tests_id, created_by), the unique constraint’s underlying index suffices; you could drop one of the single-column indexes to reduce write overhead.
10-13: Data minimization (optional): avoid storing full hint when not neededIf you only need to audit the exact text later, consider also storing a hash and optionally truncating very long hints to reduce storage and PII risk. Not a blocker.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx(7 hunks)supabase/migrations/20250830163212_grader_result_tests_hint_feedback.sql(1 hunks)utils/supabase/DatabaseTypes.d.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-30T00:20:04.201Z
Learnt from: Ari-S-123
PR: pawtograder/platform#168
File: app/course/[course_id]/manage/course/due-date-extensions/layout.tsx:52-69
Timestamp: 2025-08-30T00:20:04.201Z
Learning: Chakra UI v3 Button component supports the `asChild` prop, which allows rendering the Button as another component (like NextLink) while maintaining Button styling and functionality. This is documented in the official Chakra UI docs and is the recommended pattern for creating button-styled links.
Applied to files:
app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx
🧬 Code graph analysis (1)
app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx (2)
utils/supabase/DatabaseTypes.d.ts (2)
GraderResultTestsHintFeedback(25-35)GraderResultTestExtraData(12-23)utils/supabase/client.ts (1)
createClient(4-9)
🪛 GitHub Actions: Lint
app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx
[warning] 61-61: Step: npm run lint. ESLint: Unexpected console statement. (no-console)
[error] 145-145: Step: npm run lint. ESLint: Unexpected any. Specify a different type. (no-explicit-any)
[error] 158-158: Step: npm run lint. ESLint: 'err' is defined but never used. (no-unused-vars)
[error] 201-201: Step: npm run lint. ESLint: Unexpected any. Specify a different type. (no-explicit-any)
[error] 222-222: Step: npm run lint. ESLint: Unexpected any. Specify a different type. (no-explicit-any)
[error] 249-249: Step: npm run lint. ESLint: 'err' is defined but never used. (no-unused-vars)
[error] 276-276: Step: npm run lint. ESLint: React: Unescaped quote detected. (react/no-unescaped-entities)
[error] 276-276: Step: npm run lint. ESLint: React: Unescaped quote detected. (react/no-unescaped-entities)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy
🔇 Additional comments (2)
utils/supabase/DatabaseTypes.d.ts (1)
25-35: New hint-feedback row type looks correctFields align with the migration (ids, FK types, and booleans). No issues.
supabase/migrations/20250830163212_grader_result_tests_hint_feedback.sql (1)
52-58: No change required: policies reference the correct function
The helper is defined aspublic.authorizeforprofile(profile_id)in your migrations, so calls toauthorizeforprofile(...)are accurate—there’s noauthorize_for_profilevariant to rename.Likely an incorrect or invalid review comment.
...course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx
Outdated
Show resolved
Hide resolved
...course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx
Outdated
Show resolved
Hide resolved
...course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx
Outdated
Show resolved
Hide resolved
…de, add a test, add usage logging
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
app/api/llm-hint/route.ts (1)
86-95: Harden body parsing and validate testId strictly (> 0)Avoid 500s on bad JSON and reject non-integer/<=0 IDs.
- const { testId } = await request.json(); + const body = (await request.json().catch(() => ({}))) as { testId?: unknown }; + const testId = Number(body?.testId); @@ - if (!testId || typeof testId !== "number" || testId < 0 || !Number.isInteger(testId)) { - return NextResponse.json({ error: "testId must be a non-negative integer" }, { status: 400 }); + if (!Number.isInteger(testId) || testId <= 0) { + return NextResponse.json({ error: "Invalid testId" }, { status: 400 }); }
🧹 Nitpick comments (17)
tests/e2e/TestingUtils.ts (3)
128-129: Type the user-scoped client and fail fast on missing envs.Keeps typings consistent with the rest of the file and surfaces misconfigured test envs early.
- const userSupabase = createClient(process.env.NEXT_PUBLIC_SUPABASE_URL!, process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY!); + if (!process.env.NEXT_PUBLIC_SUPABASE_URL || !process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY) { + throw new Error("Missing NEXT_PUBLIC_SUPABASE_URL or NEXT_PUBLIC_SUPABASE_ANON_KEY"); + } + const userSupabase = createClient<Database>( + process.env.NEXT_PUBLIC_SUPABASE_URL, + process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY + );
131-145: Small DRY: extract a shared generateMagicLink(..) util to reuse here and in loginAsUser.Both places construct magic links via admin.generateLink. A tiny helper reduces duplication and keeps flows in sync if Supabase API changes.
124-151: Route/api/llm-hintuses cookie-based auth; Bearer token header won’t work
It callssupabase.auth.getUser()without checking theAuthorizationheader, so tests using only an access token will be unauthorized. Return the full session and add a helper to set the standardsb-<projectRef>-auth-tokencookies (e.g.getAuthSessionForUser+setSupabaseAuthCookies(page, session)) for stable e2e flows.supabase/functions/_shared/SupabaseTypes.d.ts (3)
2325-2424: Optional: de-dup hint payloadsIf the same hint can be shown multiple times, consider recording a stable reference alongside the text (e.g., hint_hash) to simplify aggregation and reduce storage drift. Keep full text for snapshotting if desired.
3716-3807: Harden llm_inference_usage with constraints and indexesNon-negative token counts and common-query indexes will protect data quality and keep reporting fast.
Recommended DB changes (then regenerate types if defaults change):
ALTER TABLE public.llm_inference_usage ALTER COLUMN created_at SET DEFAULT now(); ALTER TABLE public.llm_inference_usage ADD CONSTRAINT llm_inference_usage_input_tokens_nonneg CHECK (input_tokens >= 0), ADD CONSTRAINT llm_inference_usage_output_tokens_nonneg CHECK (output_tokens >= 0); CREATE INDEX IF NOT EXISTS idx_llm_usage_class_submission ON public.llm_inference_usage (class_id, submission_id); CREATE INDEX IF NOT EXISTS idx_llm_usage_grader_result_test ON public.llm_inference_usage (grader_result_test_id); CREATE INDEX IF NOT EXISTS idx_llm_usage_created_by_recent ON public.llm_inference_usage (created_by, created_at DESC);
3716-3807: Optional: tighten provider values Replace the plainstringtype forproviderwith aLLMProviderenum (e.g.'openai' | 'anthropic' | 'azure') insupabase/functions/_shared/SupabaseTypes.d.tsand introduce a matching Postgres enum or domain in the migration (20250830163212_grader_result_tests_hint_feedback.sql) to prevent typos without breaking existing rows.utils/supabase/SupabaseTypes.d.ts (1)
3716-3807: Remove redundant FK suggestion; composite index remains optional
Thellm_inference_usage_created_by_fkeyconstraint already exists in20250830163212_grader_result_tests_hint_feedback.sql, referencingauth.users(id), and individual indexes onclass_id,submission_id,grader_result_test_id, andcreated_atare present.
• You may still add a composite index—e.g.(class_id, submission_id, grader_result_test_id, created_at DESC)—in a follow-up migration if query patterns warrant it.app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx (3)
76-88: Use Button.loading and Chakra Spinner; drop Tailwind classRemoves reliance on Tailwind and improves a11y with aria-live.
- <Button onClick={handleGetHint} disabled={isLoading} colorPalette="blue" variant="outline" size="sm"> - {isLoading ? ( - <> - <FaSpinner className="animate-spin" /> - Getting Feedbot Response... - </> - ) : ( - <> - <FaRobot /> - Get Feedbot Response - </> - )} - </Button> + <Button + onClick={handleGetHint} + loading={isLoading} + colorPalette="blue" + variant="outline" + size="sm" + aria-live="polite" + > + <FaRobot /> + {isLoading ? "Generating..." : "Get Feedbot Response"} + </Button>
122-148: Fix stale dependency in feedback fetch effectInclude private_profile_id to avoid missing/incorrect preload when profile context changes.
- }, [testId, classId]); + }, [testId, classId, private_profile_id]);
127-133: Type Supabase queries; remove castsUse table generics to drop unknown→cast hops and get field checking.
- const { data: feedback, error: fetchError } = await supabase - .from("grader_result_tests_hint_feedback") + const { data: feedback, error: fetchError } = await supabase + .from<GraderResultTestsHintFeedback>("grader_result_tests_hint_feedback") .select("*") @@ - const { data: updatedFeedback, error: updateError } = await supabase - .from("grader_result_tests_hint_feedback") + const { data: updatedFeedback, error: updateError } = await supabase + .from<GraderResultTestsHintFeedback>("grader_result_tests_hint_feedback") .update({ useful: useful, comment: comment.trim() || null }) .eq("id", existingFeedback.id) .select() .single(); @@ - const { data: newFeedback, error: insertError } = await supabase - .from("grader_result_tests_hint_feedback") + const { data: newFeedback, error: insertError } = await supabase + .from<GraderResultTestsHintFeedback>("grader_result_tests_hint_feedback") .insert({ class_id: classId, grader_result_tests_id: testId, submission_id: submissionId, hint: hintText, useful: useful, comment: comment.trim() || null, created_by: private_profile_id }) .select() .single();Also applies to: 160-168, 181-194
tests/e2e/llm-hint.test.tsx (3)
24-33: Skip each provider test independentlyCurrently the suite is skipped if either key is missing. Prefer per-test skips so OpenAI-only or Anthropic-only environments still run what they can.
Example:
test("should work with anthropic", async ({ request }) => { test.skip(!process.env.ANTHROPIC_API_KEY_e2e_test, "No Anthropic key"); ... }); test("should work with openai", async ({ request }) => { test.skip(!process.env.OPENAI_API_KEY_e2e_test, "No OpenAI key"); ... });
130-138: Use a widely-available OpenAI model for stabilitygpt-4.1-nano may not exist/route in all environments. Default to a known-small model.
- model: "gpt-4.1-nano", + model: process.env.OPENAI_MODEL || "gpt-4o-mini",
167-191: Consider setting the Supabase auth token signature cookieSome setups require sb--auth-token.sig alongside the main cookie. If flakiness appears, add a matching .sig cookie.
- Cookie: `${cookieName}=${encodeURIComponent(JSON.stringify(sessionData))}` + Cookie: [ + `${cookieName}=${encodeURIComponent(JSON.stringify(sessionData))}`, + `${cookieName}.sig=dummy-signature` + ].join("; ")app/api/llm-hint/route.ts (4)
152-162: Default provider should match PR scope; add Sentry tags for model/provider/accountAligns with “targets the OpenAI API” and improves observability.
- const modelName = extraData.llm.model || process.env.OPENAI_MODEL || "gpt-4o-mini"; - const providerName = extraData.llm.provider || "anthropic"; - const accountName = extraData.llm.account; + const modelName = extraData.llm.model || process.env.OPENAI_MODEL || "gpt-4o-mini"; + const providerName = extraData.llm.provider || "openai"; + const accountName = extraData.llm.account; + Sentry.setTag("llm_provider", providerName); + Sentry.setTag("llm_model", modelName); + if (accountName) Sentry.setTag("llm_account", accountName);
163-167: Simplify: skip ChatPromptTemplate, call model directlyThe template doesn’t use variables; calling the model is clearer and faster.
- const prompt = await getPrompt(extraData.llm); - const chain = prompt.pipe(chatModel); - const response = await chain.invoke({ - input: extraData.llm.prompt - }); + const response = await chatModel.invoke(extraData.llm.prompt);
208-223: Capture token usage even when provider omits itDefault to null and let DB defaults or nullable columns handle absence; current zeros might mask missing data.
- const inputTokens = response.usage_metadata?.input_tokens || 0; - const outputTokens = response.usage_metadata?.output_tokens || 0; + const inputTokens = response.usage_metadata?.input_tokens ?? null; + const outputTokens = response.usage_metadata?.output_tokens ?? null;
189-206: Return updated extra_data on write success (optional)Selecting the updated row helps troubleshooting divergence if caching logic expands later.
- const { error: updateError } = await serviceSupabase + const { error: updateError } = await serviceSupabase .from("grader_result_tests") .update({ extra_data: updatedExtraData }) .eq("id", testId); + // Consider chaining .select('extra_data').single() if you need to verify persistence
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
app/api/llm-hint/route.ts(1 hunks)app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx(8 hunks)package.json(2 hunks)supabase/functions/_shared/SupabaseTypes.d.ts(2 hunks)supabase/migrations/20250830163212_grader_result_tests_hint_feedback.sql(1 hunks)tests/e2e/TestingUtils.ts(2 hunks)tests/e2e/llm-hint.test.tsx(1 hunks)utils/supabase/DatabaseTypes.d.ts(1 hunks)utils/supabase/SupabaseTypes.d.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- package.json
- supabase/migrations/20250830163212_grader_result_tests_hint_feedback.sql
- utils/supabase/DatabaseTypes.d.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-30T20:00:05.568Z
Learnt from: dbp
PR: pawtograder/platform#195
File: app/api/llm-hint/route.ts:134-146
Timestamp: 2025-08-30T20:00:05.568Z
Learning: In the OpenAI Node.js SDK, timeout is not a parameter of the chat.completions.create() method itself. Instead, timeout should be configured either at the client level when creating the OpenAI instance, or as an options parameter (second argument) to the create method, not within the main request parameters.
Applied to files:
app/api/llm-hint/route.ts
📚 Learning: 2025-08-23T18:05:42.129Z
Learnt from: jon-bell
PR: pawtograder/platform#173
File: supabase/migrations/20250823074648_create_system_notification_function.sql:296-327
Timestamp: 2025-08-23T18:05:42.129Z
Learning: In PostgreSQL/Supabase, SECURITY DEFINER functions run with the privileges of the function owner. According to PostgreSQL documentation, superusers and roles with the BYPASSRLS attribute always bypass the row security system, and table owners normally bypass row security as well. In Supabase, SECURITY DEFINER functions are typically owned by privileged roles (like the service role) that have superuser privileges, so they bypass RLS policies entirely. Therefore, INSERT policies are not needed for operations performed within SECURITY DEFINER functions owned by privileged roles.
Applied to files:
app/api/llm-hint/route.ts
📚 Learning: 2025-08-30T20:02:29.865Z
Learnt from: dbp
PR: pawtograder/platform#195
File: app/api/llm-hint/route.ts:134-146
Timestamp: 2025-08-30T20:02:29.865Z
Learning: The `user` parameter in OpenAI's chat.completions.create() method is deprecated, though it remains valid for attribution and abuse tracking purposes as of August 2025.
Applied to files:
app/api/llm-hint/route.ts
📚 Learning: 2025-08-30T00:20:04.201Z
Learnt from: Ari-S-123
PR: pawtograder/platform#168
File: app/course/[course_id]/manage/course/due-date-extensions/layout.tsx:52-69
Timestamp: 2025-08-30T00:20:04.201Z
Learning: Chakra UI v3 Button component supports the `asChild` prop, which allows rendering the Button as another component (like NextLink) while maintaining Button styling and functionality. This is documented in the official Chakra UI docs and is the recommended pattern for creating button-styled links.
Applied to files:
app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx
🧬 Code graph analysis (4)
tests/e2e/llm-hint.test.tsx (2)
utils/supabase/DatabaseTypes.d.ts (5)
Course(48-48)Course(914-920)Assignment(28-28)RubricPart(662-662)RubricCheck(664-664)tests/e2e/TestingUtils.ts (7)
TestingUser(20-29)createClass(31-71)createUsersInClass(396-600)insertAssignment(890-1148)insertPreBakedSubmission(603-820)supabase(11-11)getAuthTokenForUser(126-151)
tests/e2e/TestingUtils.ts (1)
utils/supabase/client.ts (1)
createClient(4-9)
app/api/llm-hint/route.ts (3)
utils/supabase/DatabaseTypes.d.ts (1)
GraderResultTestExtraData(12-25)tests/e2e/TestingUtils.ts (1)
supabase(11-11)utils/supabase/client.ts (1)
createClient(4-9)
app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx (3)
utils/supabase/DatabaseTypes.d.ts (2)
GraderResultTestsHintFeedback(27-27)GraderResultTestExtraData(12-25)hooks/useClassProfiles.tsx (1)
useClassProfiles(24-30)utils/supabase/client.ts (1)
createClient(4-9)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy
🔇 Additional comments (2)
tests/e2e/TestingUtils.ts (1)
811-813: LGTM on the logging lint exception.Keeping console.error for test diagnostics is reasonable here.
app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx (1)
418-420: Unable to extract the Row fields forgrader_result_outputautomatically. Please manually inspectutils/supabase/DatabaseTypes.d.tsunderDatabase["public"]["Tables"]["grader_result_output"]["Row"]and confirm whether the field is namedformatoroutput_format.
...course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx
Outdated
Show resolved
Hide resolved
New Features Embedded interactive Pyret REPL in grader results with expandable panels and instructor-only mode. UI/UX Loading spinner, clear error states, collapsible behavior, and “No output” fallback when results are empty. Tests Comprehensive end-to-end suite covering student/instructor flows, multi-REPL scenarios, error handling, and visual snapshots. Chores Added REPL runtime dependency; tooling/dev config updates (local dev shells, env handling, gitignore, .envrc); admin client and test-utils adjustments; small CLI/script improvements; added assert helper. Documentation Expanded README with local Supabase setup instructions.
…vents from chime SDK New Features Auto-accepts matching SIS invitations and backfills pending invites. Adds video meeting participant tracking and richer video-meeting metrics. Expands Prometheus metrics and adds a Grafana dashboard plus metrics documentation. Bug Fixes Hides already-accepted invitations from enrollment views and related exports. Chores Improves SIS API reliability with retries and reduces noisy error reporting.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
scripts/DatabaseSeedingUtils.ts (2)
3926-3927: Nix the bunny reference in the prompt.Per team preference, drop the rabbit joke from the seeded prompt.
- "... not to be ignored in favor of LLM-powered static analysis tools. Also, it's a rabbit.", + "... not to be ignored in favor of LLM-powered static analysis tools.",Also applies to: 3950-3951, 3974-3975
3912-3980: DRY up the repeated prompt/config.Factor the repeated prompt into a const and reuse; keeps tests consistent and easier to tweak.
- const graderResultTestInserts = graderResultData.flatMap((graderResult, index) => [ + const LLM_HINT_PROMPT = + "You are an end-to-end test case for an AI CS tutor. Your friend/enemy is 'CodeRabbit', a static analysis tool. Tell a joke about how testing is so important, and not to be ignored in favor of LLM-powered static analysis tools."; + const graderResultTestInserts = graderResultData.flatMap((graderResult, index) => [ ... - llm: { - prompt: - "You are an end-to-end test case for an AI CS tutor. Your friend/enemy is 'CodeRabbit', a static analysis tool. Tell a joke about how testing is so important, and not to be ignored in favor of LLM-powered static analysis tools. Also, it's a rabbit.", + llm: { + type: "v1", + prompt: LLM_HINT_PROMPT, model: "gpt-4o-mini", ... - llm: { - prompt: - "You are an end-to-end test case for an AI CS tutor. Your friend/enemy is 'CodeRabbit', a static analysis tool. Tell a joke about how testing is so important, and not to be ignored in favor of LLM-powered static analysis tools. Also, it's a rabbit.", + llm: { + type: "v1", + prompt: LLM_HINT_PROMPT, model: "gpt-4o-mini", ... - llm: { - prompt: - "You are an end-to-end test case for an AI CS tutor. Your friend/enemy is 'CodeRabbit', a static analysis tool. Tell a joke about how testing is so important, and not to be ignored in favor of LLM-powered static analysis tools. Also, it's a rabbit.", + llm: { + type: "v1", + prompt: LLM_HINT_PROMPT, model: "claude-3-haiku-20240307",supabase/migrations/20250830163212_grader_result_tests_hint_feedback.sql (1)
31-39: Index created_at for feedback table.Add created_at index to speed recency queries and housekeeping.
create index "idx_grader_result_tests_hint_feedback_created_by" on "public"."grader_result_tests_hint_feedback" using btree ("created_by"); +create index "idx_grader_result_tests_hint_feedback_created_at" on "public"."grader_result_tests_hint_feedback" using btree ("created_at");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
app/api/llm-hint/route.ts(1 hunks)app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx(8 hunks)scripts/DatabaseSeedingUtils.ts(2 hunks)supabase/migrations/20250830163212_grader_result_tests_hint_feedback.sql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/api/llm-hint/route.ts
- app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy
🔇 Additional comments (4)
scripts/DatabaseSeedingUtils.ts (2)
3951-3954: Validate Azure model/deployment mapping.Using model: "gpt-4o-mini" with provider: "azure" may require a deployment name rather than an OpenAI model ID. Confirm your route maps this correctly, or seed a deployment-like identifier here to avoid 400s in e2e.
3924-3932: Verify LLM extra_data schema matches seed
Ensure theGraderResultTestExtraDatatype inutils/supabase/DatabaseTypes.d.tsincludes all fields used in your seed (prompt,model,account,provider,temperature,max_tokens,type, and optionalresult). If any are missing, update either the type or the seeding code so they align.supabase/migrations/20250830163212_grader_result_tests_hint_feedback.sql (2)
45-51: RLS read scope looks good.Allowing graders (via authorizeforclassgrader) and submitters (via authorize_for_submission) to read feedback is appropriate.
156-166: Confirm identity domain choice for created_by across tables.Feedback uses profiles.id; usage uses auth.users.id. If intentional (UI actor vs auth actor), great—just confirm joins and API routes account for both domains.
supabase/migrations/20250830163212_grader_result_tests_hint_feedback.sql
Show resolved
Hide resolved
supabase/migrations/20250830163212_grader_result_tests_hint_feedback.sql
Outdated
Show resolved
Hide resolved
supabase/migrations/20250830163212_grader_result_tests_hint_feedback.sql
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
components/github/unlink-account.tsx (1)
41-41: Fix lint error: remove console.log.ESLint blocks the pipeline (Unexpected console statement).
Apply:
- onCancel={async () => { - console.log("Canceled"); - }} + onCancel={() => {}}supabase/functions/notification-queue-processor/index.ts (2)
480-494: Do not archive on SMTP send failures; allow retry via VTCurrently, standard notifications are archived even when SMTP delivery fails, which drops emails on transient outages. This is inconsistent with the digest path (which correctly avoids archiving on failure) and risks silent data loss.
Apply this diff to archive only on success or on clearly non-retryable reasons:
} finally { - // ALWAYS archive the message, regardless of success or failure - await archiveMessage(adminSupabase, notification.msg_id, scope); - - // Log to Sentry if email was not sent successfully - if (!emailSent) { - scope.setContext("email_not_sent", { - reason: skipReason, - msg_id: notification.msg_id, - notification_id: notification.message.id, - class_id: notification.message.class_id, - user_id: notification.message.user_id - }); - Sentry.captureMessage(`Email not sent: ${skipReason}`, scope); - } + // Archive only on success or non-retryable issues; otherwise let VT expire for retry + const nonRetryableReasons = new Set([ + "invalid_notification_body", + "invalid_email_template", + "no_valid_recipient", + "internal_test_email" + ]); + const shouldArchive = + emailSent || (skipReason !== null && nonRetryableReasons.has(skipReason)); + if (shouldArchive) { + await archiveMessage(adminSupabase, notification.msg_id, scope); + } + + // Log to Sentry if email was not sent successfully + if (!emailSent) { + scope.setContext("email_not_sent", { + reason: skipReason, + msg_id: notification.msg_id, + notification_id: notification.message.id, + class_id: notification.message.class_id, + user_id: notification.message.user_id + }); + Sentry.captureMessage(`Email not sent: ${skipReason}`, scope); + } }
824-829: Remove default edge secret to prevent accidental exposureFalling back to a hardcoded secret is a security footgun. Require EDGE_FUNCTION_SECRET to be set and fail closed.
- const expectedSecret = Deno.env.get("EDGE_FUNCTION_SECRET") || "some-secret-value"; - if (secret !== expectedSecret) { + const expectedSecret = Deno.env.get("EDGE_FUNCTION_SECRET"); + if (!expectedSecret) { + return new Response(JSON.stringify({ error: "Server misconfigured: missing EDGE_FUNCTION_SECRET" }), { + headers: { "Content-Type": "application/json" }, + status: 500 + }); + } + if (secret !== expectedSecret) { return new Response(JSON.stringify({ error: "Invalid secret" }), { - headers: { "Content-Type": "application/json" } + headers: { "Content-Type": "application/json" }, + status: 403 }); }app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx (1)
1-1: Remove staleGraderResultTestDatacast and console calls
- The cast to
GraderResultTestDatais still present on line 988; remove or replace it with a valid type.console.log/console.errorcalls remain at lines 100, 105, and 596; swap these out for the proper logging mechanism.
♻️ Duplicate comments (3)
supabase/functions/_shared/SupabaseTypes.d.ts (1)
2325-2424: Rename FK column to singular: grader_result_tests_id → grader_result_test_idKeep this consistent with
grader_result_test_outputand the rest of the schema. Update the DB, then regenerate types (this file and utils/supabase).DB patch:
ALTER TABLE public.grader_result_tests_hint_feedback RENAME COLUMN grader_result_tests_id TO grader_result_test_id; ALTER TABLE public.grader_result_tests_hint_feedback RENAME CONSTRAINT grader_result_tests_hint_feedback_grader_result_tests_id_fkey TO grader_result_tests_hint_feedback_grader_result_test_id_fkey;Type diff (will be produced by codegen, shown for clarity):
- grader_result_tests_id: number; + grader_result_test_id: number; @@ - grader_result_tests_id: number; + grader_result_test_id: number; @@ - grader_result_tests_id?: number; + grader_result_test_id?: number; @@ - foreignKeyName: "grader_result_tests_hint_feedback_grader_result_tests_id_fkey"; - columns: ["grader_result_tests_id"]; + foreignKeyName: "grader_result_tests_hint_feedback_grader_result_test_id_fkey"; + columns: ["grader_result_test_id"];Run to catch lingering references:
#!/bin/bash rg -nP -C2 '\bgrader_result_tests_id\b|grader_result_test_id\b'utils/supabase/SupabaseTypes.d.ts (1)
2325-2424: Rename FK column: use grader_result_test_id (singular) for consistency and to avoid broken joins.Everywhere else (e.g., grader_result_test_output, llm_inference_usage) uses grader_result_test_id. This block exposes grader_result_tests_id (plural). Align the column and FK name, then regenerate Supabase types.
Apply within this block:
- grader_result_tests_id: number; + grader_result_test_id: number; ... - grader_result_tests_id: number; + grader_result_test_id: number; ... - grader_result_tests_id?: number; + grader_result_test_id?: number; ... - foreignKeyName: "grader_result_tests_hint_feedback_grader_result_tests_id_fkey"; - columns: ["grader_result_tests_id"]; + foreignKeyName: "grader_result_tests_hint_feedback_grader_result_test_id_fkey"; + columns: ["grader_result_test_id"];app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx (1)
212-260: Use a single upsert to avoid duplicate rows and racesCurrent read-then-update/insert can race and create duplicates. Prefer an atomic upsert keyed on (grader_result_tests_id, created_by). Add a unique index in SQL and simplify the client code.
Client change:
- if (existingFeedback) { - // Update existing feedback - const { data: updatedFeedback, error: updateError } = await supabase - .from("grader_result_tests_hint_feedback") - .update({ - useful: usefulToSave, - comment: commentToSave.trim() || null - }) - .eq("id", existingFeedback.id) - .select() - .single(); - ... - } else { - // Insert new feedback - const { data: newFeedback, error: insertError } = await supabase - .from("grader_result_tests_hint_feedback") - .insert({ - class_id: classId, - grader_result_tests_id: testId, - submission_id: submissionId, - hint: hintText, - useful: usefulToSave, - comment: commentToSave.trim() || null, - created_by: private_profile_id - }) - .select() - .single(); - ... - } + const { data: upserted, error: upsertError } = await supabase + .from("grader_result_tests_hint_feedback") + .upsert( + { + class_id: classId, + grader_result_tests_id: testId, + submission_id: submissionId, + hint: hintText, + useful: usefulToSave, + comment: commentToSave.trim() || null, + created_by: private_profile_id + }, + { onConflict: "grader_result_tests_id,created_by" } + ) + .select() + .single(); + if (upsertError) { + setError("Failed to save feedback: " + upsertError.message); + return; + } + setExistingFeedback(upserted);DB migration (separate file):
- create unique index on grader_result_tests_hint_feedback(grader_result_tests_id, created_by).
- ensure an UPDATE policy lets the same user revise their own row.
🧹 Nitpick comments (65)
components/github/unlink-account.tsx (3)
24-39: Harden unlink flow: handle API errors, correct comments, pass explicit params.Improves reliability and avoids misleading comments.
onConfirm={async () => { - // retrieve all identities linked to a user + // retrieve all identities linked to a user const identities = await supabase.auth.getUserIdentities(); - // find the google identity - const githubIdentity = identities.data?.identities.find((identity) => identity.provider === "github"); + // surface API error early + if (identities.error) { + throw new Error(identities.error.message); + } + const allIdentities = identities.data?.identities ?? []; + // prevent locking the user out by unlinking the only identity + if (allIdentities.length <= 1) { + throw new Error("You must add another sign-in method before unlinking GitHub."); + } + // find the GitHub identity + const githubIdentity = allIdentities.find((i) => i.provider === "github"); if (!githubIdentity) { throw new Error("GitHub identity not found"); } - // unlink the google identity - const { error } = await supabase.auth.unlinkIdentity(githubIdentity); + // unlink the GitHub identity + const { error } = await supabase.auth.unlinkIdentity({ + provider: "github", + identity_id: githubIdentity.identity_id, + }); if (error) { throw new Error(error.message); } }}
16-21: Align button copy with action.“Sign out” ≠ unlink. Use consistent wording.
trigger={ - <Button colorPalette="gray"> - <BsGithub /> Sign out of GitHub + <Button colorPalette="gray"> + <BsGithub /> Unlink GitHub </Button> }
3-11: Avoid recreating the Supabase client on every render.Memoize for stability and perf.
-import { Button, VStack } from "@chakra-ui/react"; +import { Button, VStack } from "@chakra-ui/react"; +import { useMemo } from "react"; @@ - const supabase = createBrowserClient( - process.env.NEXT_PUBLIC_SUPABASE_URL || "", - process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY || "" - ); + const supabase = useMemo( + () => + createBrowserClient( + process.env.NEXT_PUBLIC_SUPABASE_URL || "", + process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY || "" + ), + [] + );supabase/functions/notification-queue-processor/index.ts (3)
608-619: Set Nodemailer TLS mode based on port (465 vs 587) to avoid handshake errorsUsing secure: true unconditionally (except Inbucket) will break common 587/STARTTLS setups.
- const isInbucketEmail = Deno.env.get("SMTP_PORT") === "54325"; - const transporter = nodemailer.createTransport({ + const isInbucketEmail = Deno.env.get("SMTP_PORT") === "54325"; + const port = parseInt(Deno.env.get("SMTP_PORT") || "465", 10); + const secure = isInbucketEmail ? false : port === 465; // TLS on 465, STARTTLS otherwise + const transporter = nodemailer.createTransport({ pool: false, host: Deno.env.get("SMTP_HOST") || "", - port: parseInt(Deno.env.get("SMTP_PORT") || "465"), - secure: isInbucketEmail ? false : true, // use TLS + port, + secure, ignoreTLS: isInbucketEmail, auth: { user: Deno.env.get("SMTP_USER") || "", pass: Deno.env.get("SMTP_PASSWORD") || "" } });
341-349: Redact recipient PII in logsAvoid logging full email addresses. Minimal redaction still preserves operability.
- console.log(`Sending email to ${recipient} with subject ${subject}`); + const domain = recipient?.split("@")[1] || "redacted"; + console.log(`Sending email to *@${domain} with subject ${subject}`);
603-606: Optional: add one-shot Sentry breadcrumb for visibility without noiseKeep console.log, but also emit a single info-level Sentry message per run so ops can discover prolonged deferrals.
if (!Deno.env.get("SMTP_HOST") || Deno.env.get("SMTP_HOST") === "") { - // eslint-disable-next-line no-console - console.log("No SMTP host found, deferring email processing"); + // eslint-disable-next-line no-console + console.log("No SMTP host found, deferring email processing"); + // Emit once-per-run breadcrumb + try { + // @ts-ignore module-scope flag (add at top-level) + if (!globalThis.__SMTP_MISSING_REPORTED__) { + // Level "info" to avoid alert noise + Sentry.captureMessage("SMTP misconfigured (host missing) — deferring email processing", { level: "info" }); + // @ts-ignore + globalThis.__SMTP_MISSING_REPORTED__ = true; + } + } catch {} // Do not archive; allow messages to become visible again after VT expires. return false; }Add this module-scope flag once near the top of the file:
// module-scope throttle for SMTP missing notice // @ts-ignore (globalThis as any).__SMTP_MISSING_REPORTED__ = (globalThis as any).__SMTP_MISSING_REPORTED__ ?? false;supabase/migrations/20250829200228_invitations-sync-to-sis-id-on-create.sql (3)
5-10: SECURITY DEFINER hardening: set a safer search_pathFor SECURITY DEFINER functions, explicitly include pg_catalog first to avoid any chance of object shadowing. Current code sets only public.
-SECURITY DEFINER -SET search_path = public +SECURITY DEFINER +SET search_path = pg_catalog, public
38-41: Avoid logging PII; downgrade verbosityRAISE NOTICE includes sis_user_id and will spam logs. Drop PII and use LOG/DEBUG instead.
- RAISE NOTICE 'Auto-accepted invitation % for user % (SIS ID: %) - user already has role % in class %', - NEW.id, v_user_id, NEW.sis_user_id, NEW.role, NEW.class_id; + RAISE LOG 'Auto-accepted invitation % for user % - user already has role % in class %', + NEW.id, v_user_id, NEW.role, NEW.class_id;- RAISE NOTICE 'Created role and auto-accepted invitation % for user % (SIS ID: %) - created role % in class %', - NEW.id, v_user_id, NEW.sis_user_id, NEW.role, NEW.class_id; + RAISE LOG 'Created role and auto-accepted invitation % for user % - created role % in class %', + NEW.id, v_user_id, NEW.role, NEW.class_id;Also applies to: 72-75
162-169: Migration output styleSome runners choke on result-returning SELECTs in migrations. Consider converting to RAISE NOTICE with the counts to avoid producing a result set.
app/course/[course_id]/manage/course/enrollments/enrollmentsTable.tsx (5)
172-179: Excluding 'accepted' invites at fetch time: confirm intent and align UI.
.neq("status", "accepted")hides accepted invitations everywhere (table, CSV, counts). If this is to avoid duplication once a user enrolls, great—then remove the “Accepted” status option from the Status filter to prevent a dead option. If you still need to view accepted invites, consider keeping them in the query and deduping in the merge layer instead.
988-995: Remove “Accepted” from Status filter options to match the dataset.Accepted invites are no longer fetched, so this option becomes misleading.
options={[ { label: "Enrolled", value: "Enrolled" }, { label: "Pending", value: "pending" }, - { label: "Accepted", value: "accepted" }, { label: "Cancelled", value: "cancelled" }, { label: "Expired", value: "Expired" } ]}
956-973: Header “select all” shows checked when there are zero rows.Avoid marking the header checkbox as checked for an empty table.
- <Checkbox.Root - checked={checkedBoxes.size === getRowModel().rows.length} + <Checkbox.Root + checked={getRowModel().rows.length > 0 && checkedBoxes.size === getRowModel().rows.length}
85-86: Specify radix in parseInt to avoid edge cases.Explicit base 10 is safer and consistent.
- const { error } = await supabase.from("user_roles").delete().eq("id", parseInt(userRoleId)); + const { error } = await supabase.from("user_roles").delete().eq("id", parseInt(userRoleId, 10));- .eq("class_id", parseInt(course_id as string)) + .eq("class_id", parseInt(course_id as string, 10))- class_id: parseInt(course_id as string), + class_id: parseInt(course_id as string, 10),Also applies to: 176-177, 1250-1252
172-179: Optional: add an index to support the new query shape.If invitations is large, consider a composite index to speed
.eq(class_id)+ order by created + status filtering:
- Postgres:
CREATE INDEX CONCURRENTLY IF NOT EXISTS invitations_class_created_status_idx ON invitations (class_id, created_at DESC, status);supabase/functions/gradebook-column-recalculate/GradebookProcessor.ts (1)
241-243: Tagging is good; set Sentry level and fingerprint to reduce noiseKeep the provider in tags, but mark the message as a warning and add a fingerprint so events group cleanly.
Apply this diff:
- newScope.setTag("dependency_provider", dependencyProvider); - Sentry.captureMessage(`Dependency source not found`, newScope); + newScope.setTag("dependency_provider", dependencyProvider); + newScope.setFingerprint(["dependency-source-not-found", dependencyProvider]); + Sentry.captureMessage("Dependency source not found", "warning");utils/utils.ts (1)
19-21: Nice addition; consider a null/undefined-specialized helper for common casesA dedicated helper improves call-site types when checking defined-ness.
Add alongside
assert:export function assert(cond: unknown, msg?: string): asserts cond { if (!cond) throw new Error(msg ?? "Assertion failed"); } + +// Asserts value is not null/undefined and narrows its type. +export function assertDefined<T>( + value: T, + msg?: string +): asserts value is NonNullable<T> { + if (value === null || value === undefined) { + throw new Error(msg ?? "Expected value to be defined"); + } +}flake.nix (2)
25-31: Only override Playwright host on LinuxThis avoids surprising behavior on macOS while keeping Linux CI/dev deterministic.
shellHook = '' export PLAYWRIGHT_BROWSERS_PATH=${pkgs.playwright.browsers} export PLAYWRIGHT_SKIP_VALIDATE_HOST_REQUIREMENTS=true - export PLAYWRIGHT_HOST_PLATFORM_OVERRIDE="ubuntu-24.04" - echo "Nix playwrite version: ${pkgs.playwright.version}" + if [[ "$(uname -s)" == "Linux" ]]; then + export PLAYWRIGHT_HOST_PLATFORM_OVERRIDE="ubuntu-24.04" + fi + echo "Nix playwright version: ${pkgs.playwright.version}" '';
29-29: Typo: “playwrite” → “playwright”Minor spelling fix in the echo.
- echo "Nix playwrite version: ${pkgs.playwright.version}" + echo "Nix playwright version: ${pkks.playwright.version}"README.md (3)
21-21: Clarify the “staging backend + local frontend” quick-startSuggest making it explicit that no local DB is required for this path.
-The quickest way to get started with development is to use our staging environment as a backend and run the frontend locally. For features requiring database schema changes or to run end-to-end tests, see [Setting up Supabase locally](#setting-up-supabase-locally) below. +The quickest way to get started is to run the frontend locally against our staging backend (no local DB required). For features requiring database schema changes or to run end-to-end tests, see [Setting up Supabase locally](#setting-up-supabase-locally).
42-54: Tighten setup wording and add a key-handling cautionMinor edits for clarity and a note to avoid committing the service role key.
-1. Install dependencies: `npm install` -2. Start Supabase locally: `npx supabase start` - This will start all Supabase services in Docker containers. -3. Reset the database: `npx supabase db reset` - This applies all database migrations and seeds the database with initial data. -4. Update your `.env.local` file: After running `npx supabase start`, you'll see output with local service URLs and keys. Update your `.env.local` file with these values: +1. Install dependencies: `npm install`. +2. Start Supabase locally: `npx supabase start` — starts all Supabase services in Docker. +3. Reset the database: `npx supabase db reset` — applies all migrations and seeds initial data. +4. Update your `.env.local`: After `npx supabase start`, copy the printed local URLs/keys: - `SUPABASE_URL` - Set to the local API URL (typically `http://127.0.0.1:54321`) - `NEXT_PUBLIC_SUPABASE_ANON_KEY` - Set to the "anon key" from the output - - `SUPABASE_SERVICE_ROLE_KEY` - Set to the "service_role key" (required for e2e tests) + - `SUPABASE_SERVICE_ROLE_KEY` - Set to the "service_role key" (required for E2E tests). Do not commit this value. - `NEXT_PUBLIC_SUPABASE_URL` - Should match `SUPABASE_URL` -5. Build the application: `npm run build` -6. Start Supabase Edge Functions: `npx supabase functions serve` -7. Start the development server: `npm start` -8. Run end-to-end tests (optional): `npx playwright test --ui` +5. Build the application: `npm run build`. +6. Start Supabase Edge Functions: `npx supabase functions serve` (in a separate terminal). +7. Start the app: `npm start`. +8. (Optional) Run end-to-end tests: `npx playwright test --ui`.
55-60: Minor prose polishLight wording tweaks; no content changes.
-Once the local Supabase setup is complete, you can develop with the full local stack. The Supabase dashboard will be available at `http://localhost:54323` where you can view and manage your local database, auth users, storage, and more. You can also access the local email interface at `http://localhost:54324` to view captured emails from development (useful for testing authentication flows). +Once setup is complete, you can develop against the full local stack. The Supabase dashboard is at `http://localhost:54323` (DB, auth, storage, etc.). The local email viewer is at `http://localhost:54324` (useful for testing auth flows).tests/e2e/pyret-repl.test.tsx (6)
272-286: Replace fixed sleeps with event-driven waits to reduce flakinessAvoid
waitForTimeout(3000)and poll for a concrete readiness signal from the REPL container or a known iframe root.- await expect(page.getByText("Initializing REPL...")).toBeVisible(); - await page.waitForTimeout(3000); + await expect(page.getByText("Initializing REPL...")).toBeVisible(); + await page.waitForFunction(() => { + const el = document.querySelector('[id^="pyret-repl-region-"]'); + return !!el && el.children.length > 0; + }, { timeout: 15000 });
292-303: DRY up repeated REPL-open logic with a helperThe “open + wait for children” sequence is duplicated; extract a helper to keep tests concise and consistent.
+async function openReplByIndex(page: Page, index = 0) { + const toggle = page.getByRole("button", { name: /Interactive Pyret REPL/i }).nth(index); + await toggle.click(); + await page.waitForFunction(() => { + const el = document.querySelector('[id^="pyret-repl-region-"]'); + return !!el && el.children.length > 0; + }, { timeout: 15000 }); +}Then reuse:
- const secondReplToggle = page.getByRole("button", { name: /Interactive Pyret REPL/i }).nth(1); - await secondReplToggle.click(); - await page.waitForTimeout(3000); - const newReplContainer = page.locator('[id^="pyret-repl-region-"]').first(); - await expect(newReplContainer).toBeVisible(); - await page.waitForFunction(() => { - const replElement = document.querySelector('[id^="pyret-repl-region-"]'); - return replElement && replElement.children.length > 0; - }); + await openReplByIndex(page, 1);
350-367: Make error-path deterministic; remove blind 10s sleepPoll for either “ready” or “error” state with a reasonable timeout, then retry once if error.
- await page.waitForTimeout(10000); - - const hasLoadedSuccessfully = await page.locator('[id^="pyret-repl-region-"]').isVisible(); - const hasErrorMessage = await page.getByText(/Failed to load REPL|Error loading/i).isVisible(); + const hasLoadedSuccessfully = await page + .locator('[id^="pyret-repl-region-"]') + .waitFor({ state: "visible", timeout: 15000 }) + .then(() => true) + .catch(() => false); + const hasErrorMessage = hasLoadedSuccessfully + ? false + : await page.getByText(/Failed to load REPL|Error loading/i).isVisible();
381-400: Count visible REPLs explicitly
.count()returns total nodes, not only visible. Use:visibleto assert the number opened.- await expect(replContainers.filter({ hasText: /./ }).or(replContainers)).toHaveCount(expectedOpenReplCount); - - const visibleCount = await replContainers.count(); - expect(visibleCount).toBeGreaterThanOrEqual(expectedOpenReplCount); + const visibleRepls = page.locator('[id^="pyret-repl-region-"]:visible'); + await expect(visibleRepls).toHaveCount(expectedOpenReplCount);
53-253: Promote insertPyretSubmission to shared seeding utilsThis bespoke seeder duplicates patterns from DatabaseSeedingUtils. Move it there (or reuse existing helpers) to centralize schema knowledge and reduce drift.
259-268: Selector robustness: prefer test IDs over visible text
getByRole(..., { name: "Hidden Test (Instructor Only)" })and similar will break on copy changes. Prefer stabledata-testidhooks.supabase/migrations/20250830021125_enhance_metrics.sql (5)
257-266: Late token limit: return a scalar, not a rowset expression
SELECT late_tokens_per_student FROM classes WHERE id = class_record.idreturns one value; OK. EnsureNULLis handled to keep JSON shape stable.- 'late_tokens_per_student_limit', ( - SELECT late_tokens_per_student FROM "public"."classes" - WHERE id = class_record.id - ), + 'late_tokens_per_student_limit', + COALESCE(( + SELECT late_tokens_per_student FROM "public"."classes" WHERE id = class_record.id + ), 0),
354-362: Unique index with nullable column may not prevent duplicates
chime_attendee_idis nullable; Postgres allows multiple NULLs in a unique index. If you want “one row per session per attendee when attendee ID is present,” keep this index; if you want to also prevent duplicates whenchime_attendee_id IS NULL, add a partial unique index for the NULL case keyed byprivate_profile_id.CREATE UNIQUE INDEX IF NOT EXISTS video_meeting_session_users_session_profile_null_attendee_unique_idx ON public.video_meeting_session_users (video_meeting_session_id, private_profile_id) WHERE chime_attendee_id IS NULL;
382-389: RLS + GRANT ALL to authenticated is confusing; narrow privilegesWith RLS enabled and no SELECT policy, granting ALL to
authenticatedis misleading. Consider granting minimal table privileges and relying on RLS for access.-REVOKE ALL ON TABLE "public"."video_meeting_session_users" FROM "anon"; -GRANT ALL ON TABLE "public"."video_meeting_session_users" TO "authenticated"; -GRANT ALL ON TABLE "public"."video_meeting_session_users" TO "service_role"; +REVOKE ALL ON TABLE "public"."video_meeting_session_users" FROM "anon","authenticated"; +GRANT ALL ON TABLE "public"."video_meeting_session_users" TO "service_role";
23-67: Workflow “failed” vs “timeout” criteria overlap
workflow_runs_failedandworkflow_runs_timeoutshare the same base predicate; onlytimeoutadds the 30-minute window. If “failed” should mean explicit error, consider sourcing fromworkflow_run_erroror add a failure flag to disambiguate.
24-66: Use array aggregation to avoid repeated jsonb concatenation
result := result || jsonb_build_array(class_metrics)in a loop copies arrays repeatedly. For many classes, use a singleSELECT jsonb_agg(...)with a subquery for better performance.supabase/functions/metrics/index.ts (4)
214-221: Timestamp unit checkYou’re emitting
Date.now()(ms). Prometheus text exposition allows an optional timestamp in milliseconds, but this is often omitted entirely. If your Prometheus setup expects seconds, useMath.floor(Date.now() / 1000), or drop timestamps and let the scraper assign them.
91-101: Type rpc result instead of double castingUse Supabase generics to avoid
as unknown asand catch drift between RPC and interface at compile time.- const { data: metricsData, error: metricsError } = (await supabase.rpc("get_all_class_metrics")) as unknown as { - data: ClassMetrics[]; - error: Error; - }; + const { data: metricsData, error: metricsError } = await supabase.rpc<ClassMetrics[]>( + "get_all_class_metrics" + );
223-231: Escape label values once; avoid repeated string concatenation
generateMetricappends strings in a tight loop. Minor, but building an array and.join("")is faster and cleaner.- const generateMetric = (name: string, help: string, type: string, field: keyof ClassMetrics) => { - output += `# HELP ${name} ${help} -# TYPE ${name} ${type} -`; - for (const metric of metrics) { - output += `${name}{class_id="${metric.class_id}",class_name="${escapeLabel(metric.class_name)}",class_slug="${escapeLabel(metric.class_slug)}"} ${metric[field]} ${timestamp}\n`; - } - output += "\n"; - }; + const generateMetric = (name: string, help: string, type: string, field: keyof ClassMetrics) => { + const header = `# HELP ${name} ${help}\n# TYPE ${name} ${type}\n`; + const lines = metrics.map((m) => + `${name}{class_id="${m.class_id}",class_name="${escapeLabel(m.class_name)}",class_slug="${escapeLabel(m.class_slug)}"} ${m[field]} ${timestamp}\n` + ); + output += header + lines.join("") + "\n"; + };
540-578: Auth: constant-time compare fallback is fine; add early return for empty tokenIf
METRICS_TOKENis set but empty, treat as misconfig; fail fast to avoid accidental exposure.- const metricsToken = Deno.env.get("METRICS_TOKEN"); - if (metricsToken) { + const metricsToken = Deno.env.get("METRICS_TOKEN"); + if (metricsToken !== undefined) { + if (!metricsToken) { + return new Response("Server misconfiguration: METRICS_TOKEN empty", { status: 500 }); + }supabase/functions/_shared/SupabaseTypes.d.ts (3)
2325-2424: Optional: prevent duplicate feedback rows per test/userIf business rules allow only one feedback per (grader_result_test_id, created_by), add a unique constraint.
ALTER TABLE public.grader_result_tests_hint_feedback ADD CONSTRAINT uq_hint_feedback_per_user UNIQUE (grader_result_test_id, created_by);
3716-3807: Add FK for created_by and sanity checks on token countsStrengthen data integrity for usage auditing.
ALTER TABLE public.llm_inference_usage ADD CONSTRAINT llm_inference_usage_created_by_fkey FOREIGN KEY (created_by) REFERENCES public.profiles(id); ALTER TABLE public.llm_inference_usage ADD CONSTRAINT ck_llm_tokens_nonnegative CHECK (input_tokens >= 0 AND output_tokens >= 0);Optionally consider enums for
provider(e.g., 'openai','azure','anthropic') to reduce typos.
7102-7162: De-dupe attendees per sessionTo avoid multiple rows for the same user in a session, add a uniqueness constraint on (video_meeting_session_id, private_profile_id).
ALTER TABLE public.video_meeting_session_users ADD CONSTRAINT uq_session_user UNIQUE (video_meeting_session_id, private_profile_id);utils/supabase/SupabaseTypes.d.ts (2)
3716-3807: Add created_by FK and helpful indexes for common queries.Link created_by to profiles for integrity; index hot paths.
Migration snippet:
alter table public.llm_inference_usage add constraint llm_inference_usage_created_by_fkey foreign key (created_by) references public.profiles(id) on update cascade on delete restrict; create index if not exists idx_llm_usage_submission_id on public.llm_inference_usage(submission_id); create index if not exists idx_llm_usage_grader_result_test_id on public.llm_inference_usage(grader_result_test_id); create index if not exists idx_llm_usage_class_created_at on public.llm_inference_usage(class_id, created_at desc);
7101-7162: Add indices/constraints to streamline “currently in meeting” queries and data hygiene.
- Partial index for active attendees.
- Optional uniqueness to prevent multiple active rows per user per session.
- Sanity check on joined_at/left_at ordering.
create index concurrently if not exists idx_vmsu_active on public.video_meeting_session_users (video_meeting_session_id) where left_at is null; create unique index concurrently if not exists uq_vmsu_one_active_per_user on public.video_meeting_session_users (video_meeting_session_id, private_profile_id) where left_at is null; alter table public.video_meeting_session_users add constraint vmsu_join_before_leave check (left_at is null or joined_at <= left_at);app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx (3)
232-258: Drop unnecessary casts from Supabase resultsWith a typed Supabase client, these are already correctly typed.
- if (updatedFeedback) { - setExistingFeedback(updatedFeedback as unknown as GraderResultTestsHintFeedback); - } + if (updatedFeedback) { + setExistingFeedback(updatedFeedback); + } ... - if (newFeedback) { - setExistingFeedback(newFeedback as unknown as GraderResultTestsHintFeedback); - } + if (newFeedback) { + setExistingFeedback(newFeedback); + }
594-601: Fix lint: avoid console.error in client codeThis triggers the no-console rule and leaks noisy logs. Use Sentry (already imported) and surface UI error.
- console.error("Failed to initialize Pyret REPL:", e); - setError(e?.message || "Failed to load REPL"); + Sentry.captureException(e, { tags: { operation: "pyret_repl_init", testId: String(testId) } }); + setError((e?.message as string) || "Failed to load REPL");
171-171: Make debounce timer type DOM/Node-safeNodeJS.Timeout can conflict in browser builds. Use ReturnType.
- const debounceTimeoutRef = useRef<NodeJS.Timeout>(); + const debounceTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null); ... - if (debounceTimeoutRef.current) { - clearTimeout(debounceTimeoutRef.current); - } + if (debounceTimeoutRef.current) clearTimeout(debounceTimeoutRef.current); ... - if (debounceTimeoutRef.current) { - clearTimeout(debounceTimeoutRef.current); - } + if (debounceTimeoutRef.current) clearTimeout(debounceTimeoutRef.current); ... - if (debounceTimeoutRef.current) { - clearTimeout(debounceTimeoutRef.current); - } + if (debounceTimeoutRef.current) clearTimeout(debounceTimeoutRef.current);Also applies to: 285-287, 303-305, 315-317
scripts/GenerateMagicLink.ts (1)
5-5: Avoid logging environment URLs in scriptsThis log is unnecessary and can clutter CI output.
-console.log(process.env.NEXT_PUBLIC_SUPABASE_URL);supabase/functions/live-meeting-callback/index.ts (2)
21-24: Auth header parsing: support Bearer and avoid brittle equalityMany integrations send “Authorization: Bearer ”. Normalize before compare.
- const auth = req.headers.get("Authorization"); - if (auth !== Deno.env.get("AWS_CHIME_EVENT_AUTH_TOKEN")) { + const authHeader = req.headers.get("Authorization") ?? ""; + const token = authHeader.startsWith("Bearer ") ? authHeader.slice(7) : authHeader; + if (token !== Deno.env.get("AWS_CHIME_EVENT_AUTH_TOKEN")) { return new Response("Unauthorized", { status: 401 }); }
13-17: Harden JSON parsing of nested bodyIf body.body is malformed JSON, JSON.parse will throw and return a 500. Return a 400 instead.
- if (typeof body.body === "string") { - body.body = JSON.parse(body.body); - } - await chimeUtils.processSNSMessage(body.body); + if (typeof body.body === "string") { + try { + body.body = JSON.parse(body.body); + } catch (e) { + return new Response("Bad Request: invalid JSON body", { status: 400 }); + } + } + await chimeUtils.processSNSMessage(body.body);utils/supabase/client.ts (1)
19-26: Prevent accidental client-side usage of the service-role clientAdd a guard so this can’t be invoked from the browser by mistake.
export const createAdminClient = <DB>() => { + if (typeof window !== "undefined") { + throw new Error("createAdminClient must not be called in the browser"); + } const supabaseUrl = process.env.SUPABASE_URL ?? process.env.NEXT_PUBLIC_SUPABASE_URL; const supabaseServiceRoleKey = process.env.SUPABASE_SERVICE_ROLE_KEY; assert(supabaseUrl, "SUPABASE_URL or NEXT_PUBLIC_SUPABASE_URL is required"); assert(supabaseServiceRoleKey, "SUPABASE_SERVICE_ROLE_KEY is required"); return supabaseCreateClient<DB>(supabaseUrl, supabaseServiceRoleKey); };tests/e2e/TestingUtils.ts (1)
165-169: Guard against missing hashed_token; fall back to action_linkSupabase may return an
action_link. Use it ifhashed_tokenisn’t present.- const magicLink = `/auth/magic-link?token_hash=${magicLinkData.properties?.hashed_token}`; + const tokenHash = magicLinkData.properties?.hashed_token; + const magicLink = + tokenHash != null + ? `/auth/magic-link?token_hash=${tokenHash}` + : magicLinkData.properties?.action_link!;docs/grafana-dashboard-pawtograder-metrics.json (1)
59-81: Bind panels to the selected datasource variablePanels don’t specify a datasource, so they rely on Grafana’s instance default. Add
"datasource": "$datasource"(or the structured form) to each panel for portability.Example for the “Active Classes” panel:
"pluginVersion": "8.0.0", + "datasource": "$datasource", "targets": [ { "expr": "count(pawtograder_active_students_total{class_slug=~\"$class_slug\", class_name=~\"$class_name\"})", "refId": "A" } ],supabase/functions/_shared/ChimeWrapper.ts (4)
53-56: Validate required env for admin Supabase clientAvoid creating a client with empty strings; fail fast with a clear error (and Sentry) if env is missing.
- const adminSupabase = createClient<Database>( - Deno.env.get("SUPABASE_URL") || "", - Deno.env.get("SUPABASE_SERVICE_ROLE_KEY") || "" - ); + const supabaseUrl = Deno.env.get("SUPABASE_URL"); + const serviceKey = Deno.env.get("SUPABASE_SERVICE_ROLE_KEY"); + if (!supabaseUrl || !serviceKey) { + const err = new Error("Missing SUPABASE_URL or SUPABASE_SERVICE_ROLE_KEY"); + Sentry.captureException(err, scope); + throw err; + } + const adminSupabase = createClient<Database>(supabaseUrl, serviceKey);
118-128: Use a stricter, non-greedy regex for externalMeetingId parsingPrevents accidental over-capture if IDs contain dashes.
- const match = message.detail.externalMeetingId.match(/video-meeting-session-(.*)-(.*)-(.*)/); + const match = message.detail.externalMeetingId.match(/video-meeting-session-(\d+)-(\d+)-(\d+)/);Apply similarly in the other parsing sites.
67-73: Pass AWS credentials to Chime client here as wellYou do this in getActiveMeeting; mirror it to avoid relying on ambient creds.
- const chime = new ChimeSDKMeetings({ - region: "us-east-1" - }); + const chime = new ChimeSDKMeetings({ + region: "us-east-1", + credentials: { + accessKeyId: Deno.env.get("AWS_ACCESS_KEY_ID")!, + secretAccessKey: Deno.env.get("AWS_SECRET_ACCESS_KEY")! + } + });
250-253: Guard NotificationsConfiguration ARNIf
AWS_CHIME_SQS_QUEUE_ARNis unset, Chime meeting creation will fail at runtime; check and surface a clear error.- NotificationsConfiguration: { - SqsQueueArn: Deno.env.get("AWS_CHIME_SQS_QUEUE_ARN")! - } + NotificationsConfiguration: (() => { + const arn = Deno.env.get("AWS_CHIME_SQS_QUEUE_ARN"); + if (!arn) throw new Error("AWS_CHIME_SQS_QUEUE_ARN is not set"); + return { SqsQueueArn: arn }; + })()docs/metrics.md (9)
176-183: Prometheus counter naming: add_totalsuffix and keep type-specific examples separateCounters should end with
_total. Also avoid mixing counter/gauge guidance inline; show one example per type.- generateMetric( - "pawtograder_your_new_metric_name", - "Description of what this metric measures", - "counter", // or "gauge" - see metric types below - "your_new_metric_name" - ); + generateMetric( + "pawtograder_your_new_metric_name_total", + "Description of what this metric measures", + "counter", + "your_new_metric_name" + );Optionally add a separate gauge example below without
_total.
166-171: Prefer nullish coalescing to avoid masking valid falsy valuesUse
??instead of||so0stays0and onlynull/undefinedcoalesce.- your_new_metric_name: classData.your_new_metric_name || 0 + your_new_metric_name: classData.your_new_metric_name ?? 0
191-215: Invalid JSON due to comments; switch to JSONC (or remove comments) to prevent copy/paste breakageGrafana expects strict JSON. Either mark this as JSONC in docs or remove inline comments.
-```json +```jsonc { "fieldConfig": { "defaults": { "color": {"mode": "thresholds"}, "thresholds": { "steps": [ {"color": "green", "value": null}, {"color": "yellow", "value": 10}, {"color": "red", "value": 50} ] }, "unit": "short" } }, "gridPos": {"h": 8, "w": 8, "x": 0, "y": NEXT_Y_POSITION}, "id": NEXT_ID, "targets": [{ "expr": "sum by (class_slug, class_name) (pawtograder_your_new_metric_name{class_slug=~\"$class_slug\", class_name=~\"$class_name\"})", "legendFormat": "{{class_name}}" }], "title": "Your Metric Title", - "type": "timeseries" // or "bargauge", "stat", "piechart" + "type": "timeseries" // Note: remove comments when pasting into Grafana JSON }
296-299: Add fenced language and legacy no-cache headersThis fixes markdownlint (MD040) and avoids proxy caching edge cases.
-``` -Cache-Control: no-cache, no-store, must-revalidate -``` +```http +Cache-Control: no-cache, no-store, must-revalidate +Pragma: no-cache +Expires: 0 +```
271-279: Document explicit GRANT/REVOKE and search_path hardening for the SECURITY DEFINER RPCYou mention service_role-only, but showing the exact statements helps avoid accidental public execute.
The metrics RPC function uses `SECURITY DEFINER` and runs as `service_role` to bypass RLS. This is necessary for system-wide metrics collection. When you delete and recreate it, make sure to include all the latest policies and permissions. + +Example hardening snippet to include in the migration: + +```sql +-- Ensure stable search path +ALTER FUNCTION public.get_all_class_metrics() SET search_path = public; + +-- Lock down execute privileges +REVOKE ALL ON FUNCTION public.get_all_class_metrics() FROM PUBLIC; +GRANT EXECUTE ON FUNCTION public.get_all_class_metrics() TO service_role; +```If you want, I can add this to the migration.
99-105: Add Content-Type guidance for Prometheus exposition formatPrometheus scrapers expect the text format header; worth calling out explicitly.
- TypeScript interface for type safety - Prometheus-compatible output format +- Sets `Content-Type: text/plain; version=0.0.4; charset=utf-8` - Configurable authentication - Comprehensive metric labeling (class_id, class_name, class_slug)
334-336: Tweak bearer token placeholder to avoid secret scannersUsing angle brackets reduces false positives from tools like gitleaks while keeping the example clear.
-curl -H "Authorization: Bearer YOUR_TOKEN" \ +curl -H "Authorization: Bearer <METRICS_TOKEN>" \ http://localhost:54321/functions/v1/metrics
221-234: Cardinality caution: avoid mutable/high-cardinality labels where possibleIncluding
class_namecan cause series churn if names change. Prefer stable identifiers likeclass_id/class_slug; keepclass_nameoptional.
129-141: Optional: prefer deriving time windows in PromQL instead of embedding in metric namesIf feasible, export raw counters/gauges (e.g., totals with timestamps) and compute 24h/7d windows in Grafana/Prometheus. This reduces schema churn and complexity in SQL.
Also applies to: 150-161, 173-183
supabase/functions/course-import-sis/index.ts (2)
50-56: Consider making retry configuration more discoverableThe retry parameters have sensible defaults, but consider documenting the expected ranges or adding JSDoc comments to help future maintainers understand the appropriate values for
maxRetriesandbaseDelayMs./** * Fetch with retry and exponential backoff for SIS API calls + * @param url - The URL to fetch + * @param options - Standard fetch options + * @param maxRetries - Maximum number of retry attempts (default: 3, recommended range: 1-5) + * @param baseDelayMs - Base delay in milliseconds for exponential backoff (default: 1000, recommended range: 500-5000) + * @param scope - Optional Sentry scope for logging */ async function fetchWithRetry(
178-180: Consider preserving the original error type in the final error messageWhen all retries fail, the error message could indicate whether it was a network error, timeout, or HTTP error to help with debugging.
throw new UserVisibleError( - `SIS API request failed after ${maxRetries + 1} attempts: ${lastError?.message || "Unknown error"}` + `SIS API request failed after ${maxRetries + 1} attempts: ${lastError?.message || "Unknown error"}${lastError?.message?.includes("SIS API error") ? "" : " (network or timeout error)"}` );
...course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx
Show resolved
Hide resolved
...course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx
Outdated
Show resolved
Hide resolved
...course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx
Outdated
Show resolved
Hide resolved
supabase/migrations/20250829200228_invitations-sync-to-sis-id-on-create.sql
Show resolved
Hide resolved
supabase/migrations/20250829200228_invitations-sync-to-sis-id-on-create.sql
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (8)
scripts/DatabaseSeedingUtils.ts (1)
3918-3939: Add required llm.type discriminator to match API contract.Seeded tests’ extra_data.llm objects are missing type: "v1", which the UI/types expect. This also aligns with the PR contract and avoids downstream routing/e2e issues.
Apply this diff in each llm block:
extra_data: { llm: { + type: "v1", prompt: "You are an end-to-end test case for an AI CS tutor. Your friend/enemy is 'CodeRabbit', a static analysis tool. Tell a joke about how testing is so important, and not to be ignored in favor of LLM-powered static analysis tools. Also, it's a rabbit.", model: "gpt-4o-mini", account: "e2e_test", provider: "openai", temperature: 1, max_tokens: 100 } }Repeat the same addition for the Azure and Anthropic test blocks.
Also applies to: 3942-3963, 3966-3987
app/api/llm-hint/route.ts (2)
55-63: Add request timeouts to all model clients.External calls can hang; set a sane timeout (configurable) on Azure/OpenAI/Anthropic LangChain clients.
return new AzureChatOpenAI({ model, temperature: temperature || 0.85, maxTokens: maxTokens, maxRetries: maxRetries || 2, + timeout: Number(process.env.LLM_REQUEST_TIMEOUT_MS ?? 30_000), azureOpenAIApiKey: process.env[key_env_name], azureOpenAIApiInstanceName: instanceName, azureOpenAIApiDeploymentName: model, azureOpenAIApiVersion: "2024-05-01-preview" }); return new ChatOpenAI({ model, apiKey: process.env[key_env_name], temperature: temperature || 0.85, maxTokens: maxTokens, - maxRetries: maxRetries || 2 + maxRetries: maxRetries || 2, + timeout: Number(process.env.LLM_REQUEST_TIMEOUT_MS ?? 30_000) }); return new ChatAnthropic({ model, apiKey: process.env[key_env_name], temperature: temperature || 0.85, maxTokens: maxTokens, - maxRetries: maxRetries || 2 + maxRetries: maxRetries || 2, + timeout: Number(process.env.LLM_REQUEST_TIMEOUT_MS ?? 30_000) });Also applies to: 69-75, 81-87
177-187: Harden payload parsing and integer validation for testId (400 on bad JSON).Avoid failing with 500 when JSON parse throws; coerce and validate integer semantics.
- const { testId } = await request.json(); + const body = (await request.json().catch(() => ({}))) as { testId?: unknown }; + const testId = Number(body?.testId); @@ - if (!testId || typeof testId !== "number" || testId < 0 || !Number.isInteger(testId)) { - throw new UserVisibleError("testId must be a non-negative integer", 400); + if (!Number.isInteger(testId) || testId <= 0) { + throw new UserVisibleError("testId must be a positive integer", 400); }app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx (5)
35-37: Import missing PyretReplConfig type.Fix TS error by importing the type used in PyretRepl props.
-import { GraderResultTestExtraData, GraderResultTestsHintFeedback } from "@/utils/supabase/DatabaseTypes"; +import { GraderResultTestExtraData, GraderResultTestsHintFeedback, PyretReplConfig } from "@/utils/supabase/DatabaseTypes";
540-541: Use output_format, not format, to preserve markdown rendering.The GraderResultOutput shape uses output_format; using format here can break markdown.
-return format_basic_output({ output: output.output, output_format: output.format as "text" | "markdown" }); +return format_basic_output({ output: output.output, output_format: output.output_format as "text" | "markdown" });
991-992: Fix removed type alias: use GraderResultTestExtraData.
GraderResultTestDatawas removed; this cast breaks type checking.- const extraData = result.extra_data as GraderResultTestData | undefined; + const extraData = result.extra_data as GraderResultTestExtraData | undefined;
1029-1048: Return JSX from map; instructor-only output currently never renders.The callback has braces but no return.
- {hasInstructorOutput && - result.grader_result_test_output.map((output) => { - const hiddenExtraData = output.extra_data as GraderResultTestExtraData | undefined; - <CardRoot key={output.id} m={2}> + {hasInstructorOutput && + result.grader_result_test_output.map((output) => { + const hiddenExtraData = output.extra_data as GraderResultTestExtraData | undefined; + return ( + <CardRoot key={output.id} m={2}> <CardHeader bg="bg.muted" p={2}> <Heading size="md">Instructor-Only Output</Heading> </CardHeader> <CardBody> {format_basic_output({ output: output.output, output_format: output.output_format as "text" | "markdown" })} </CardBody> {hiddenExtraData?.pyret_repl && ( <Box mt={3}> <PyretRepl testId={result.id} config={hiddenExtraData.pyret_repl} hidden /> </Box> )} - </CardRoot>; + </CardRoot> + ); })}
218-253: Avoid read-then-insert race for hint feedback; use upsert with unique index.Two clients can race and create duplicates. Prefer a single upsert on (grader_result_tests_id, created_by).
UI change:
- if (existingFeedback) { - // Update existing feedback - const { data: updatedFeedback, error: updateError } = await supabase - .from("grader_result_tests_hint_feedback") - .update({ - useful: usefulToSave, - comment: commentToSave.trim() || null - }) - .eq("id", existingFeedback.id) - .select() - .single(); - ... - } else { - // Insert new feedback - const { data: newFeedback, error: insertError } = await supabase - .from("grader_result_tests_hint_feedback") - .insert({ - class_id: classId, - grader_result_tests_id: testId, - submission_id: submissionId, - hint: hintText, - useful: usefulToSave, - comment: commentToSave.trim() || null, - created_by: private_profile_id - }) - .select() - .single(); - ... - } + const { data: upserted, error: upsertError } = await supabase + .from("grader_result_tests_hint_feedback") + .upsert( + { + class_id: classId, + grader_result_tests_id: testId, + submission_id: submissionId, + hint: hintText, + useful: usefulToSave, + comment: commentToSave.trim() || null, + created_by: private_profile_id + }, + { onConflict: "grader_result_tests_id,created_by" } + ) + .select() + .single(); + if (upsertError) { + setError("Failed to save feedback: " + upsertError.message); + return; + } + setExistingFeedback(upserted as GraderResultTestsHintFeedback);DB migration follow-up (separate PR/migration): add unique index on (grader_result_tests_id, created_by) and ensure an UPDATE policy allows authors to modify their own feedback.
Also applies to: 240-251
🧹 Nitpick comments (6)
scripts/DatabaseSeedingUtils.ts (2)
1806-1816: Guard against data explosions in large classes (seed ergonomics).The 75%/95% probabilities plus 1–3/1–4 submissions per entity can produce very large volumes with big cohorts, slowing tests. Consider gating by env or adding a hard cap per assignment to keep CI stable.
Example tweak:
- Add SEED_MAX_SUBMISSIONS_PER_ASSIGNMENT env and clamp numSubmissions based on it.
Also applies to: 1823-1830
3841-3848: Removeanyin batch maps to satisfy lint and improve safety.These maps still rely on
(submission: any, index)and similar casts. Prefer typed shapes or infer from Supabase insert responses to avoid runtime shape bugs.Example:
-const submissionFileInserts = submissionData.map((submission: any, index) => ({ +const submissionFileInserts = submissionData.map((submission: { id: number }, index) => ({ name: "sample.java", contents: sampleJavaCode, class_id: class_id, submission_id: submission.id, profile_id: chunk[index].student?.private_profile_id, assignment_group_id: chunk[index].group?.id }));Also applies to: 3864-3874, 4006-4011
app/api/llm-hint/route.ts (2)
282-293: Normalize LangChain response content to string to avoid array/block cases.
response.contentcan be string or array of blocks. Normalize before storing/returning to prevent “object rendered as [object Object]” issues.- // Extract token usage from the response + // Extract token usage from the response const inputTokens = response.usage_metadata?.input_tokens || 0; const outputTokens = response.usage_metadata?.output_tokens || 0; - // Store the result in the database + // Normalize content to string + const toText = (c: unknown) => + Array.isArray(c) + ? c.map((b: any) => (typeof b === "string" ? b : b?.text ?? "")).filter(Boolean).join("\n") + : (c as string); + + // Store the result in the database const updatedExtraData: GraderResultTestExtraData = { ...extraData, llm: { ...extraData.llm, - result: response.content as string + result: toText(response.content) } }; @@ - return NextResponse.json({ + return NextResponse.json({ success: true, - response: response.content as string, + response: toText(response.content), cached: false });Also applies to: 349-353
15-23: De-duplicate UserVisibleError – import shared type.You already have a shared UserVisibleError in supabase/functions/_shared/HandlerUtils.ts. Prefer importing it to avoid drift.
If cross-imports from serverless functions are undesirable, consider a shared utilities module under utils/.
app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx (2)
174-175: Use cross-runtime-safe timeout type in the client component.
NodeJS.Timeoutin a "use client" component can cause type friction. PreferReturnType<typeof setTimeout>.- const debounceTimeoutRef = useRef<NodeJS.Timeout>(); + const debounceTimeoutRef = useRef<ReturnType<typeof setTimeout>>();Also applies to: 287-301
597-601: Drop console.error in client; rely on Sentry only.Keep lint clean and avoid noisy logs in prod bundles.
- console.error("Failed to initialize Pyret REPL:", e); setError(e?.message || "Failed to load REPL");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
app/api/llm-hint/route.ts(1 hunks)app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx(9 hunks)scripts/DatabaseSeedingUtils.ts(6 hunks)utils/supabase/DatabaseTypes.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- utils/supabase/DatabaseTypes.d.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-30T20:00:05.568Z
Learnt from: dbp
PR: pawtograder/platform#195
File: app/api/llm-hint/route.ts:134-146
Timestamp: 2025-08-30T20:00:05.568Z
Learning: In the OpenAI Node.js SDK, timeout is not a parameter of the chat.completions.create() method itself. Instead, timeout should be configured either at the client level when creating the OpenAI instance, or as an options parameter (second argument) to the create method, not within the main request parameters.
Applied to files:
app/api/llm-hint/route.ts
📚 Learning: 2025-08-23T18:05:42.129Z
Learnt from: jon-bell
PR: pawtograder/platform#173
File: supabase/migrations/20250823074648_create_system_notification_function.sql:296-327
Timestamp: 2025-08-23T18:05:42.129Z
Learning: In PostgreSQL/Supabase, SECURITY DEFINER functions run with the privileges of the function owner. According to PostgreSQL documentation, superusers and roles with the BYPASSRLS attribute always bypass the row security system, and table owners normally bypass row security as well. In Supabase, SECURITY DEFINER functions are typically owned by privileged roles (like the service role) that have superuser privileges, so they bypass RLS policies entirely. Therefore, INSERT policies are not needed for operations performed within SECURITY DEFINER functions owned by privileged roles.
Applied to files:
app/api/llm-hint/route.ts
📚 Learning: 2025-08-30T20:02:29.865Z
Learnt from: dbp
PR: pawtograder/platform#195
File: app/api/llm-hint/route.ts:134-146
Timestamp: 2025-08-30T20:02:29.865Z
Learning: The `user` parameter in OpenAI's chat.completions.create() method is deprecated, though it remains valid for attribution and abuse tracking purposes as of August 2025.
Applied to files:
app/api/llm-hint/route.ts
📚 Learning: 2025-08-30T00:20:04.201Z
Learnt from: Ari-S-123
PR: pawtograder/platform#168
File: app/course/[course_id]/manage/course/due-date-extensions/layout.tsx:52-69
Timestamp: 2025-08-30T00:20:04.201Z
Learning: Chakra UI v3 Button component supports the `asChild` prop, which allows rendering the Button as another component (like NextLink) while maintaining Button styling and functionality. This is documented in the official Chakra UI docs and is the recommended pattern for creating button-styled links.
Applied to files:
app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx
🧬 Code graph analysis (3)
app/api/llm-hint/route.ts (2)
supabase/functions/_shared/HandlerUtils.ts (1)
UserVisibleError(281-288)utils/supabase/DatabaseTypes.d.ts (2)
GraderResultTestExtraData(24-39)LLMRateLimitConfig(18-22)
app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx (3)
utils/supabase/DatabaseTypes.d.ts (3)
GraderResultTestsHintFeedback(41-41)GraderResultTestExtraData(24-39)PyretReplConfig(12-16)hooks/useClassProfiles.tsx (1)
useClassProfiles(24-30)utils/supabase/client.ts (1)
createClient(6-17)
scripts/DatabaseSeedingUtils.ts (1)
hooks/useSubmission.tsx (2)
submission(114-116)submission(124-129)
🪛 GitHub Actions: Lint
app/api/llm-hint/route.ts
[error] 103-103: Error: Unexpected any. Specify a different type. @typescript-eslint/no-explicit-any
[error] 105-105: Error: Unexpected any. Specify a different type. @typescript-eslint/no-explicit-any
[error] 148-148: Error: Unexpected any. Specify a different type. @typescript-eslint/no-explicit-any
app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx
[warning] 599-599: Warning: Unexpected console statement. no-console
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy
🔇 Additional comments (1)
app/api/llm-hint/route.ts (1)
319-329: Keep usinguser.idforcreated_by. Thellm_inference_usagetable’screated_bycolumn is defined as a foreign key toauth.users.id, so supplyinguser.idis correct.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/e2e/TestingUtils.ts (1)
8-9: Resolve prior client import; add typing + env fallbacks.Good fix using supabase-js directly. Add type param and fall back to SUPABASE_* like your helper does to avoid undefined NEXT_PUBLIC_* in CI.
-import { createClient } from "@supabase/supabase-js"; +import { createClient as supabaseJsCreateClient } from "@supabase/supabase-js"; @@ - const userSupabase = createClient(process.env.NEXT_PUBLIC_SUPABASE_URL!, process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY!); + const supabaseUrl = process.env.SUPABASE_URL ?? process.env.NEXT_PUBLIC_SUPABASE_URL!; + const supabaseAnonKey = process.env.SUPABASE_ANON_KEY ?? process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY!; + const userSupabase = supabaseJsCreateClient<Database>(supabaseUrl, supabaseAnonKey);Also applies to: 126-130
🧹 Nitpick comments (10)
package.json (1)
41-47: Pin AI lib versions to reduce breakage risk.LangChain and OpenAI SDKs ship frequent breaking changes across minors. Consider pinning exact versions (no caret) for reproducible CI and snapshots.
- "@langchain/anthropic": "^0.3.26", - "@langchain/core": "^0.3.72", - "@langchain/openai": "^0.6.9", - "openai": "^5.16.0", + "@langchain/anthropic": "0.3.26", + "@langchain/core": "0.3.72", + "@langchain/openai": "0.6.9", + "openai": "5.16.0",Also applies to: 104-104
tests/e2e/TestingUtils.ts (4)
166-167: URL-encode token hash in magic-link.Defensive encoding avoids rare parsing issues.
- const magicLink = `/auth/magic-link?token_hash=${magicLinkData.properties?.hashed_token}`; + const magicLink = `/auth/magic-link?token_hash=${encodeURIComponent(magicLinkData.properties?.hashed_token ?? "")}`;
242-253: Prefer Admin API over querying users table.Use supabase.auth.admin.getUserByEmail to avoid RLS and schema coupling.
- const { data: existingUserData, error: getUserError } = await supabase - .from("users") - .select("*") - .eq("email", resolvedEmail) - .single(); + const { data: existingUserData, error: getUserError } = await supabase.auth.admin.getUserByEmail(resolvedEmail); if (getUserError) { throw new Error(`Failed to get existing user: ${getUserError.message}`); } - userId = existingUserData.user_id; + userId = (existingUserData.user?.id ?? (existingUserData as unknown as { id: string }).id)!;
2264-2272: Pattern likely off-by-one “assignment-assignment-*”.If slugs look like “assignment-1”, drop the duplicate “assignment-”.
- score_expression: "mean(gradebook_columns('assignment-assignment-*'))", + score_expression: "mean(gradebook_columns('assignment-*'))",
1728-1747: Pass objects directly to the JSONB column instead of stringifyingThe
dependenciescolumn is JSONB—providing the JS object (ornull) lets Supabase store it as JSONB for proper querying.- dependencies: finalDependencies ? JSON.stringify(finalDependencies) : null, + dependencies: finalDependencies ?? null,jest.setup.js (1)
11-12: Tiny nit: punctuation and clarity.-// If you delete this file, remove `setupFilesAfterEnv` from `jest.config.js` +// If you delete this file, remove `setupFilesAfterEnv` from `jest.config.js`.jest.config.js (1)
9-11: Node env + jest-dom mismatch.If any unit tests render React, switch to jsdom or mark those files with
@jest-environment jsdom. Otherwise, keep Node for route tests only.- testEnvironment: 'node', + testEnvironment: 'node', // use jsdom if you add DOM-based unit testsOr split configs via Jest “projects” to run Node and jsdom suites separately.
tests/unit/llm-hint.test.ts (3)
550-597: Reduce provider test duplication with table-driven cases.Parametrize OpenAI/Azure/Anthropic cases to cut boilerplate and speed up maintenance.
// e.g. describe.each([{provider:'openai', ...}, {provider:'azure', ...}, ...])(...)Also applies to: 616-672, 674-711
314-345: Cooldown math assertion is brittle.Hard-coding “3 more minute(s)” depends on test start time. Consider stubbing Date.now for stability.
// jest.spyOn(Date, 'now').mockReturnValue(FIXED_TS)
573-597: Add test for per-account keys across providers.You cover OpenAI account-scoped key; mirror for Azure/Anthropic (e.g., AZURE_OPENAI_KEY_account1, ANTHROPIC_API_KEY_account1).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
jest.config.js(1 hunks)jest.setup.js(1 hunks)package.json(5 hunks)tests/e2e/TestingUtils.ts(4 hunks)tests/unit/llm-hint.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/unit/llm-hint.test.ts (2)
utils/supabase/client.ts (1)
createClient(6-17)utils/supabase/DatabaseTypes.d.ts (1)
LLMRateLimitConfig(18-22)
tests/e2e/TestingUtils.ts (1)
utils/supabase/client.ts (2)
createAdminClient(19-26)createClient(6-17)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy
🔇 Additional comments (4)
package.json (1)
15-17: Nice: added unit-test scripts.Jest scripts are wired cleanly and won’t collide with the existing Playwright e2e tasks.
tests/e2e/TestingUtils.ts (1)
432-444: Drop RLS warning: this batch‐read uses the service_role client (createAdminClient) which bypasses RLS, and thepublic.userstable (withuser_id,email) is defined in your migrations—no need to switch toauth.admin.listUsers.Likely an incorrect or invalid review comment.
tests/unit/llm-hint.test.ts (2)
782-803: Great: usage tracking assertions hit the important fields.
805-821: Good resilience checks on DB failure paths.Continuing after partial failures and capturing to Sentry matches the PR’s intent.
Also applies to: 823-840
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
scripts/DatabaseSeedingUtils.ts (1)
3968-3976: Add missing llm.type = "v1" to match Feedbot contract.Without the discriminator, routing/e2e assertions can fail.
Apply this diff:
extra_data: { llm: { + type: "v1", prompt: "You are an end-to-end test case for an AI CS tutor. Your friend/enemy is 'CodeRabbit', a static analysis tool. Tell a joke about how testing is so important, and not to be ignored in favor of LLM-powered static analysis tools. Also, it's a rabbit.", model: "gpt-4o-mini", account: "e2e_test", provider: "openai", temperature: 1, max_tokens: 100 } }extra_data: { llm: { + type: "v1", prompt: "You are an end-to-end test case for an AI CS tutor. Your friend/enemy is 'CodeRabbit', a static analysis tool. Tell a joke about how testing is so important, and not to be ignored in favor of LLM-powered static analysis tools. Also, it's a rabbit.", model: "gpt-4o-mini", account: "e2e_test", provider: "azure", temperature: 1, max_tokens: 100 } }extra_data: { llm: { + type: "v1", prompt: "You are an end-to-end test case for an AI CS tutor. Your friend/enemy is 'CodeRabbit', a static analysis tool. Tell a joke about how testing is so important, and not to be ignored in favor of LLM-powered static analysis tools. Also, it's a rabbit.", model: "claude-3-haiku-20240307", account: "e2e_test", provider: "anthropic", temperature: 1, max_tokens: 100 } }Also applies to: 3992-4000, 4016-4024
🧹 Nitpick comments (10)
scripts/DatabaseSeedingUtils.ts (4)
1819-1829: Cap seeded submission multiplicity to avoid runaway volume.At scale, 75%×(1–3) per group and 95%×(1–4) per student can explode row counts and throttle pipelines. Consider env- or config-driven rates and per-assignment caps.
Also applies to: 1833-1843
2326-2364: Harden keying: skip items with neither student nor group.Edge case safeguard to avoid a key like “profile_undefined”.
Apply this diff:
submissionData.forEach((submission) => { + // Skip malformed entries defensively + if (!submission.student && !submission.group) { + return; + } // Create a unique key for each assignment + student/group combination const profileKey = submission.group ? `assignment_${submission.assignment.id}_group_${submission.group.id}` : `assignment_${submission.assignment.id}_profile_${submission.student?.private_profile_id}`;
3879-3886: Prefer null over undefined for DB optional FKs.Supabase handles nulls explicitly; using ?? null avoids implicit undefined serialization differences.
Apply this diff:
- profile_id: chunk[index].student?.private_profile_id, - assignment_group_id: chunk[index].group?.id + profile_id: chunk[index].student?.private_profile_id ?? null, + assignment_group_id: chunk[index].group?.id ?? null
4044-4044: Avoid any; return a typed submission_id.Keeps types clean and avoids masking with a broad cast. Also consider renaming the inner “submissionData” variable to “insertedSubmissions” to reduce shadowing.
Apply this diff:
- submission_id: (submissionData[index] as any).id, + submission_id: (submissionData[index] as { id: number }).id,tests/unit/llm-hint.test.ts (6)
133-171: Make Supabase query builder mocks less brittle (per-table builders; consistent return types)Right now a single shared builder is reused and methods like eq/in alternate between chainable and promise-returning across tests. This makes order-dependent, fragile stubbing. Prefer returning a fresh, table-scoped builder and keep method return shapes consistent.
Example approach (add near the mocks setup):
type QueryResult<T = unknown> = Promise<{ data?: T; count?: number; error: unknown | null }>; const makeBuilder = (): MockQueryBuilder => { const builder: any = {}; builder.select = jest.fn().mockReturnValue(builder); builder.eq = jest.fn().mockReturnValue(builder); builder.neq = jest.fn().mockReturnValue(builder); builder.order = jest.fn().mockReturnValue(builder); builder.limit = jest.fn().mockReturnValue(builder); builder.single = jest.fn<QueryResult>().mockResolvedValue({ data: null, error: null }); builder.insert = jest.fn<QueryResult>().mockResolvedValue({ error: null }); builder.update = jest.fn().mockReturnValue({ eq: jest.fn<QueryResult>().mockResolvedValue({ error: null }) }); builder.in = jest.fn<QueryResult>().mockResolvedValue({ count: 0, error: null }); return builder as MockQueryBuilder; }; const usageBuilder = makeBuilder(); const graderResultTestsBuilder = makeBuilder(); const submissionsBuilder = makeBuilder(); (mockCreateServiceClient as any).mockReturnValue({ from: jest.fn((table?: string) => { if (table === "llm_inference_usage") return usageBuilder; if (table === "grader_result_tests") return graderResultTestsBuilder; if (table === "submissions") return submissionsBuilder; return makeBuilder(); }) });Then, in tests, stub the specific builder (e.g., submissionsBuilder, usageBuilder) instead of the shared one.
318-347: Stabilize cooldown tests: freeze time and avoid hard-coded IDs
- Freeze Date.now to eliminate minute-boundary flakiness in remaining-time math.
- Use mockTestResult.grader_results.submissions.assignment_id and .id instead of literal 1s for clarity.
Minimal patch inside the "Cooldown Rate Limiting" describe:
describe("Cooldown Rate Limiting", () => { + let nowSpy: jest.SpyInstance<number, []>; + beforeAll(() => { + nowSpy = jest.spyOn(Date, "now").mockReturnValue(new Date("2025-01-01T12:00:00Z").getTime()); + }); + afterAll(() => { + nowSpy.mockRestore(); + }); it("should return 429 when cooldown period has not elapsed", async () => { @@ - .eq("submissions.assignment_id", 1) - .neq("submission_id", 1) + .eq("submissions.assignment_id", mockTestResult.grader_results.submissions.assignment_id) + .neq("submission_id", mockTestResult.grader_results.submissions.id)Also applies to: 349-395
571-578: Relax provider constructor assertions to avoid needless brittlenessFuture provider config changes (e.g., extra options) will break strict toHaveBeenCalledWith. Match only the required fields.
Apply:
-expect(mockChatOpenAI).toHaveBeenCalledWith({ - model: "gpt-4o-mini", - apiKey: "test-openai-key", - temperature: 0.85, - maxTokens: undefined, - maxRetries: 2 -}); +expect(mockChatOpenAI).toHaveBeenCalledWith(expect.objectContaining({ + model: "gpt-4o-mini", + apiKey: "test-openai-key", + temperature: expect.any(Number), + maxTokens: undefined, + maxRetries: 2 +})); -expect(mockChatOpenAI).toHaveBeenCalledWith({ - model: "gpt-4", - apiKey: "test-account-key", - temperature: 0.5, - maxTokens: 1000, - maxRetries: 2 -}); +expect(mockChatOpenAI).toHaveBeenCalledWith(expect.objectContaining({ + model: "gpt-4", + apiKey: "test-account-key", + temperature: 0.5, + maxTokens: 1000, + maxRetries: 2 +})); -expect(mockAzureChatOpenAI).toHaveBeenCalledWith({ - model: "gpt-4o-mini", - temperature: 0.85, - maxTokens: undefined, - maxRetries: 2, - azureOpenAIApiKey: "test-azure-key", - azureOpenAIApiInstanceName: "test.openai.azure.com", - azureOpenAIApiDeploymentName: "gpt-4o-mini", - azureOpenAIApiVersion: "2024-05-01-preview" -}); +expect(mockAzureChatOpenAI).toHaveBeenCalledWith(expect.objectContaining({ + model: "gpt-4o-mini", + azureOpenAIApiKey: "test-azure-key", + azureOpenAIApiInstanceName: "test.openai.azure.com", + azureOpenAIApiDeploymentName: "gpt-4o-mini", + azureOpenAIApiVersion: expect.any(String) +})); -expect(data.error).toBe("Azure OpenAI endpoint and key are required, must set env var AZURE_OPENAI_KEY"); +expect(data.error).toContain("Azure OpenAI"); -expect(mockChatAnthropic).toHaveBeenCalledWith({ - model: "claude-3-sonnet-20240229", - apiKey: "test-anthropic-key", - temperature: 0.85, - maxTokens: undefined, - maxRetries: 2 -}); +expect(mockChatAnthropic).toHaveBeenCalledWith(expect.objectContaining({ + model: "claude-3-sonnet-20240229", + apiKey: "test-anthropic-key" +}));Also applies to: 597-604, 635-645, 676-678, 695-700
752-766: Also assert chain input and token-default semantics in the “no usage metadata” caseValidate we send the expected prompt into the chain and that usage defaults to zeros.
Apply:
const response = await POST(request); const data = await response.json(); expect(response.status).toBe(200); expect(data.success).toBe(true); expect(data.response).toBe("Test AI response"); +expect(mockChain.invoke).toHaveBeenCalledWith({ input: "Test prompt" }); +expect(serviceQueryBuilder.insert).toHaveBeenCalledWith(expect.objectContaining({ + input_tokens: 0, + output_tokens: 0 +}));
223-248: Add a test for non-integer numeric testIdRoute enforces Number.isInteger; cover decimals.
Add within “Authentication and Authorization”:
it("should return 400 when testId is a non-integer number", async () => { const request = new NextRequest("http://localhost:3000/api/llm-hint", { method: "POST", body: JSON.stringify({ testId: 1.5 }) }); const response = await POST(request); const data = await response.json(); expect(response.status).toBe(400); expect(data.error).toBe("testId must be a non-negative integer"); });
176-185: Guard against env leakage across testsSnapshot/restore process.env to isolate tests that delete keys.
Apply:
-// Ensure env vars are set for provider tests (avoid bleed from other tests) -process.env.OPENAI_API_KEY = process.env.OPENAI_API_KEY || "test-openai-key"; +// Ensure env vars are set for provider tests (avoid bleed from other tests) +const __envBackup = { ...process.env }; +process.env.OPENAI_API_KEY = process.env.OPENAI_API_KEY || "test-openai-key"; @@ process.env.ANTHROPIC_API_KEY = process.env.ANTHROPIC_API_KEY || "test-anthropic-key"; + +afterEach(() => { + process.env = { ...__envBackup }; +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
app/api/llm-hint/route.ts(1 hunks)jest.config.js(1 hunks)jest.setup.js(1 hunks)scripts/DatabaseSeedingUtils.ts(11 hunks)tests/unit/llm-hint.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- jest.config.js
- app/api/llm-hint/route.ts
- jest.setup.js
🧰 Additional context used
🧬 Code graph analysis (2)
tests/unit/llm-hint.test.ts (2)
utils/supabase/DatabaseTypes.d.ts (1)
LLMRateLimitConfig(18-22)app/api/llm-hint/route.ts (1)
POST(197-391)
scripts/DatabaseSeedingUtils.ts (1)
tests/e2e/TestingUtils.ts (1)
TestingUser(21-30)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy
🔇 Additional comments (8)
scripts/DatabaseSeedingUtils.ts (5)
101-108: Type alias for SubmissionDataItem — LGTMFits downstream usage and keeps group/student mutually optional.
664-674: Confirm intent: grade only latest, but create workflow events/errors for all.This will inflate events/errors vs. graded rows. If that’s intentional for realistic history, all good; otherwise filter earlier to save inserts.
1858-2077: Workflow events generator ordering and batching — LGTM; verify future timestamps.Chronological requested → in_progress → completed is solid. If due_date is in the future, requestedAt will also be future; ensure any UI/queries that use now() tolerates that.
2366-2377: Type-safe ID extraction for grading — LGTMMapping to submissionIds via the alias keeps types consistent with the new SubmissionDataItem.
3902-3912: Grader result seed defaults — LGTMReasonable defaults and formats for downstream renderers.
tests/unit/llm-hint.test.ts (3)
27-35: Solid coverage and scenario breadthGreat job covering auth, caching, rate limits, providers, persistence, and observability in one suite.
789-810: Nice persistence assertionsValidations for writing both the result and usage are on point.
882-893: Sentry context checks look goodTags and user context assertions match route behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/DatabaseSeedingUtils.ts (1)
3039-3067: Final Grade references a non-existent column slugscore_expression uses gradebook_columns('average-assignments'), but you created slug "average.hw". This will break the column computation.
Apply:
- score_expression: - "gradebook_columns('average-lab-assignments') * 0.4 + gradebook_columns('average-assignments') * 0.5 + gradebook_columns('participation') * 0.1", + score_expression: + "gradebook_columns('average-lab-assignments') * 0.4 + gradebook_columns('average.hw') * 0.5 + gradebook_columns('participation') * 0.1",
♻️ Duplicate comments (3)
scripts/DatabaseSeedingUtils.ts (1)
3927-4045: Add missing llm.type for Azure and Anthropic seeded testsOpenAI test includes type: "v1", but Azure/Anthropic omit it, breaking the contract noted in the PR.
Apply:
{ score: 3, max_score: 5, name: "Azure test", @@ extra_data: { llm: { + type: "v1", prompt: "You are an end-to-end test case for an AI CS tutor. Your friend/enemy is 'CodeRabbit', a static analysis tool. Tell a joke about how testing is so important, and not to be ignored in favor of LLM-powered static analysis tools. Also, it's a rabbit.", model: "gpt-4o-mini", account: "e2e_test", provider: "azure", temperature: 1, max_tokens: 100 } } }, { score: 3, max_score: 5, name: "Anthropic test", @@ extra_data: { llm: { + type: "v1", prompt: "You are an end-to-end test case for an AI CS tutor. Your friend/enemy is 'CodeRabbit', a static analysis tool. Tell a joke about how testing is so important, and not to be ignored in favor of LLM-powered static analysis tools. Also, it's a rabbit.", model: "claude-3-haiku-20240307", account: "e2e_test", provider: "anthropic", temperature: 1, max_tokens: 100 } } }app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx (2)
1034-1054: Map callback now returns JSX — regression fixed.Instructor-only outputs render again.
208-275: Use a single upsert to avoid duplicates; add DB unique index.The read-then-insert can race and create multiple rows per user/test. Prefer one upsert on (grader_result_tests_id, created_by) and rely on a unique index.
- if (existingFeedback) { - // Update existing feedback - const { data: updatedFeedback, error: updateError } = await supabase - .from("grader_result_tests_hint_feedback") - .update({ - useful: usefulToSave, - comment: commentToSave.trim() || null - }) - .eq("id", existingFeedback.id) - .select() - .single(); - if (updateError) { - setError("Failed to save feedback: " + updateError.message); - return; - } - if (updatedFeedback) { - setExistingFeedback(updatedFeedback as unknown as GraderResultTestsHintFeedback); - } - } else { - // Insert new feedback - const { data: newFeedback, error: insertError } = await supabase - .from("grader_result_tests_hint_feedback") - .insert({ - class_id: classId, - grader_result_tests_id: testId, - submission_id: submissionId, - hint: hintText, - useful: usefulToSave, - comment: commentToSave.trim() || null, - created_by: private_profile_id - }) - .select() - .single(); - if (insertError) { - setError("Failed to save feedback: " + insertError.message); - return; - } - if (newFeedback) { - setExistingFeedback(newFeedback as unknown as GraderResultTestsHintFeedback); - } - } + const { data: upserted, error: upsertError } = await supabase + .from("grader_result_tests_hint_feedback") + .upsert( + { + class_id: classId, + grader_result_tests_id: testId, + submission_id: submissionId, + hint: hintText, + useful: usefulToSave, + comment: commentToSave.trim() || null, + created_by: private_profile_id + }, + { onConflict: "grader_result_tests_id,created_by" } + ) + .select() + .single(); + if (upsertError) { + setError("Failed to save feedback: " + upsertError.message); + return; + } + setExistingFeedback(upserted);DB migration (separate file):
- create unique index on grader_result_tests_hint_feedback(grader_result_tests_id, created_by).
- ensure UPDATE policy so authors can revise their own feedback.
🧹 Nitpick comments (13)
scripts/DatabaseSeedingUtils.ts (8)
1819-1846: Submission explosion: sanity-check batch/throughput settings1–3 group submissions and 1–4 individual submissions per student can grow fast. Confirm DEFAULT_RATE_LIMITS and Supabase quotas are sized for typical e2e runs to avoid throttling.
2326-2364: Avoid per-bucket sort; pick max ID in O(n)Sorting each bucket to find the latest submission is O(k log k) per key. A single pass to keep the max submission_id is simpler and faster.
Apply:
- submissionsByAssignmentAndProfile.forEach((submissions) => { - if (submissions.length > 0) { - // Sort by submission_id in descending order and take the first (highest ID = latest) - const latestSubmission = submissions.sort((a, b) => b.submission_id - a.submission_id)[0]; - latestSubmissions.push(latestSubmission); - } - }); + submissionsByAssignmentAndProfile.forEach((submissions) => { + if (submissions.length > 0) { + let latest = submissions[0]; + for (let i = 1; i < submissions.length; i++) { + if (submissions[i].submission_id > latest.submission_id) latest = submissions[i]; + } + latestSubmissions.push(latest); + } + });
2366-2644: Cap awarded points to check limitsThe synthetic allocation ignores rubric check maxima, which can yield per-check points > check.points and confuse consumers.
Minimal fix:
- const pointsAwarded = checkPointAllocations.get(check.id) || 0; + const planned = checkPointAllocations.get(check.id) || 0; + const pointsAwarded = Math.min(planned, check.points);If totals need to remain ~90, scale allocations before capping to reduce drift.
3879-3886: Minor: remove unused fields in seeded submission filesprofile_id/assignment_group_id are already represented on the submission; unless needed for queries, consider omitting to reduce noise.
3165-3167: Drop stray debug log"Column inserted" is noisy during large seeds.
Apply:
- console.log("Column inserted");
2702-2709: Likely typo: target count for regradesMath.max(..., 1000) makes the intention unclear; slice caps it anyway. Use min to cap upper bound or remove the 1000.
Apply one of:
- const numRegradeRequests = Math.max(Math.max(1, Math.floor(submissionData.length * 0.2)), 1000); + const numRegradeRequests = Math.max(1, Math.floor(submissionData.length * 0.2));or
- const numRegradeRequests = Math.max(Math.max(1, Math.floor(submissionData.length * 0.2)), 1000); + const numRegradeRequests = Math.min(Math.max(1, Math.floor(submissionData.length * 0.2)), 1000);
1814-1818: Unused isRecentlyDueisRecentlyDue is computed but never used. Remove to keep seed payloads lean unless it’s for planned logic.
Apply:
- const isRecentlyDue = new Date(assignment.due_date) < now; + // const isRecentlyDue = new Date(assignment.due_date) < now; @@ - submissionsToCreate.push({ - assignment: { ...assignment }, - group, - isRecentlyDue - }); + submissionsToCreate.push({ assignment: { ...assignment }, group }); @@ - submissionsToCreate.push({ - assignment: { ...assignment }, - student, - isRecentlyDue - }); + submissionsToCreate.push({ assignment: { ...assignment }, student });Also applies to: 1823-1829, 1833-1843
3039-3067: Expressions contain single/double quoted nested quotesThey look correct, but these strings are brittle. Consider extracting long expressions to constants to avoid accidental escaping errors.
Also applies to: 3540-3671
app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx (5)
47-129: Harden fetch flow: add AbortController and surface Retry-After on 429.
- Prevent setState on unmounted by aborting in-flight requests.
- If 429, read Retry-After and show a next-at timestamp.
Apply:
+ const abortRef = useRef<AbortController | null>(null); const handleGetHint = async () => { + abortRef.current?.abort(); + abortRef.current = new AbortController(); setIsLoading(true); setError(null); try { const response = await fetch("/api/llm-hint", { method: "POST", headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ testId }) + body: JSON.stringify({ testId }), + signal: abortRef.current.signal }); @@ - case 429: - errorMessage = "Rate limit exceeded."; + case 429: { + const ra = response.headers.get("Retry-After"); + errorMessage = ra ? `Rate limit exceeded. Try again in ${ra}s.` : "Rate limit exceeded."; break; - } + }And cleanup:
+ useEffect(() => () => abortRef.current?.abort(), []);Also applies to: 133-151
168-179: Timer type: prefer ReturnType for browser compatibility.NodeJS.Timeout can conflict in the browser build.
- const debounceTimeoutRef = useRef<NodeJS.Timeout>(); + const debounceTimeoutRef = useRef<ReturnType<typeof setTimeout>>();
177-191: Guard for missing profile id before querying.If private_profile_id is undefined, the read will fail RLS later; short-circuit and keep the form disabled.
- useEffect(() => { + useEffect(() => { + if (!private_profile_id) { + setIsLoading(false); + return; + } const fetchExistingFeedback = async () => {
239-241: Unnecessary casts; rely on typed Supabase client.With Database generics, select().single() returns typed rows; remove unknown/as casts.
- setExistingFeedback(updatedFeedback as unknown as GraderResultTestsHintFeedback); + setExistingFeedback(updatedFeedback);- setExistingFeedback(newFeedback as unknown as GraderResultTestsHintFeedback); + setExistingFeedback(newFeedback);Also applies to: 263-265
565-576: Dispose REPL instance on collapse/unmount to avoid leaks.If makeEmbed returns a disposable, call it when collapsing or unmounting.
- if (prev) { + if (prev) { // Collapsing: reset embed so it will re-initialize on next expand - embedRef.current = null; + try { + // @ts-expect-error optional cleanup + embedRef.current?.destroy?.(); + // Fallback names if API differs + embedRef.current?.dispose?.(); + } catch {} + embedRef.current = null; if (containerRef.current) { containerRef.current.innerHTML = ""; } }- return () => { - cancelled = true; - }; + return () => { + cancelled = true; + try { + // @ts-expect-error optional cleanup + embedRef.current?.destroy?.(); + embedRef.current?.dispose?.(); + } catch {} + embedRef.current = null; + containerRef.current && (containerRef.current.innerHTML = ""); + };Also applies to: 610-613
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx(9 hunks)scripts/DatabaseSeedingUtils.ts(11 hunks)supabase/migrations/20250830163212_grader_result_tests_hint_feedback.sql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- supabase/migrations/20250830163212_grader_result_tests_hint_feedback.sql
🧰 Additional context used
🧬 Code graph analysis (2)
app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx (3)
utils/supabase/DatabaseTypes.d.ts (3)
GraderResultTestsHintFeedback(41-41)GraderResultTestExtraData(24-39)PyretReplConfig(12-16)hooks/useClassProfiles.tsx (1)
useClassProfiles(24-30)utils/supabase/client.ts (1)
createClient(6-17)
scripts/DatabaseSeedingUtils.ts (1)
tests/e2e/TestingUtils.ts (1)
TestingUser(21-30)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy
🔇 Additional comments (10)
scripts/DatabaseSeedingUtils.ts (2)
101-108: Type export looks goodThe SubmissionDataItem shape aligns with downstream usage in this file.
664-674: Grade only latest submissions — LGTMCreating events/errors for all submissions while grading only latest is clear and intentional.
app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx (8)
39-42: Types import looks correct now (PyretReplConfig present).This resolves the earlier missing-type issue.
411-419: Verify Button’s loading prop.Ensure "@/components/ui/button" uses loading (not isLoading) and disables clicks while pending.
543-545: Correct formatter mapping for grader_result_output.Switch to format uses output.format → basic formatter. Looks good.
776-799: Nice adoption of theme tokens for error states.bg.error/border.error/fg.error improve consistency across light/dark modes.
Also applies to: 782-792, 786-787
913-916: Good: unified basic formatter for lint output.Avoids Markdown loss and keeps rendering consistent.
953-976: Status cell logic reads well; robot icon when LLM present.Small UX win; immediately communicates Feedbot involvement.
1019-1032: Composition is clean: TestResultOutput + optional PyretRepl.Props passed correctly, including extraData cast.
469-497: No sanitization change needed: Markdown component already escapes HTML by default.ReactMarkdown disables raw HTML unless you add a
rehypeRawplugin, and ourMarkdownonly usesrehypeKatexandrehypeHighlight, so any HTML in LLM output is escaped/ignored by default (github.com, contentful.com).Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (7)
app/api/llm-hint/route.ts (3)
197-207: Harden body parsing and coerce testId to an integer (duplicate of earlier feedback)Avoid falsy/truthy checks and strings slipping through; coerce and validate.
- const { testId } = await request.json(); + const body = (await request.json().catch(() => ({}))) as { testId?: unknown }; + const testId = Number((body as any).testId); @@ - if (!testId || typeof testId !== "number" || testId < 0 || !Number.isInteger(testId)) { - throw new UserVisibleError("testId must be a non-negative integer", 400); + if (!Number.isInteger(testId) || testId <= 0) { + throw new UserVisibleError("testId must be a positive integer", 400); }
74-83: Add per-request timeouts to prevent hung external callsWithout timeouts, requests can hang and tie up server resources.
return new AzureChatOpenAI({ model, temperature: temperature || 0.85, maxTokens: maxTokens, maxRetries: maxRetries || 2, + timeout: Number(process.env.LLM_REQUEST_TIMEOUT_MS) || 30_000, azureOpenAIApiKey: process.env[key_env_name], @@ return new ChatOpenAI({ model, apiKey: process.env[key_env_name], temperature: temperature || 0.85, maxTokens: maxTokens, - maxRetries: maxRetries || 2 + maxRetries: maxRetries || 2, + timeout: Number(process.env.LLM_REQUEST_TIMEOUT_MS) || 30_000 }); @@ return new ChatAnthropic({ model, apiKey: process.env[key_env_name], temperature: temperature || 0.85, maxTokens: maxTokens, - maxRetries: maxRetries || 2 + maxRetries: maxRetries || 2, + timeout: Number(process.env.LLM_REQUEST_TIMEOUT_MS) || 30_000 });Also applies to: 89-95, 101-107
68-83: Azure instance name parsing is wrong; yields invalid endpoint
split("/")[2]returns the full hostname (e.g., my-res.openai.azure.com). The SDK expects just the resource name (my-res).- const instanceName = process.env.AZURE_OPENAI_ENDPOINT?.split("/")[2]; + const endpoint = process.env.AZURE_OPENAI_ENDPOINT!; + const instanceName = new URL(endpoint).hostname.split(".")[0]; - if (!process.env.AZURE_OPENAI_ENDPOINT || !process.env[key_env_name]) { + if (!endpoint || !process.env[key_env_name]) { throw new UserVisibleError(`Azure OpenAI endpoint and key are required, must set env var ${key_env_name}`, 500); }supabase/functions/metrics/index.ts (1)
271-306: Counter vs gauge: snapshot totals must be gauges (not counters).These are point-in-time counts that can decrease. Switch to gauges. Keep counters only for truly monotonic series (e.g., append-only LLM usage/token sums).
- generateMetric("pawtograder_workflow_runs_total", "Total number of workflow runs per class", "counter", "workflow_runs_total"); - generateMetric("pawtograder_workflow_runs_completed", "Total number of completed workflow runs per class", "counter", "workflow_runs_completed"); - generateMetric("pawtograder_workflow_runs_failed", "Total number of failed workflow runs per class", "counter", "workflow_runs_failed"); + generateMetric("pawtograder_workflow_runs_total", "Total number of workflow runs per class", "gauge", "workflow_runs_total"); + generateMetric("pawtograder_workflow_runs_completed", "Total number of completed workflow runs per class", "gauge", "workflow_runs_completed"); + generateMetric("pawtograder_workflow_runs_failed", "Total number of failed workflow runs per class", "gauge", "workflow_runs_failed"); - generateMetric("pawtograder_workflow_errors_total", "Total number of workflow errors per class", "counter", "workflow_errors_total"); + generateMetric("pawtograder_workflow_errors_total", "Total number of workflow errors per class", "gauge", "workflow_errors_total"); - generateMetric("pawtograder_workflow_runs_timeout", "Total number of workflow runs that timed out per class", "counter", "workflow_runs_timeout"); + generateMetric("pawtograder_workflow_runs_timeout", "Total number of workflow runs that timed out per class", "gauge", "workflow_runs_timeout"); - generateMetric("pawtograder_submissions_total", "Total number of active submissions per class", "counter", "submissions_total"); + generateMetric("pawtograder_submissions_total", "Total number of active submissions per class", "gauge", "submissions_total"); - generateMetric("pawtograder_submission_reviews_total", "Total number of completed submission reviews per class", "counter", "submission_reviews_total"); + generateMetric("pawtograder_submission_reviews_total", "Total number of completed submission reviews per class", "gauge", "submission_reviews_total"); - generateMetric("pawtograder_submission_comments_total", "Total number of submission comments (all types) per class", "counter", "submission_comments_total"); + generateMetric("pawtograder_submission_comments_total", "Total number of submission comments (all types) per class", "gauge", "submission_comments_total"); - generateMetric("pawtograder_regrade_requests_total", "Total number of regrade requests per class", "counter", "regrade_requests_total"); + generateMetric("pawtograder_regrade_requests_total", "Total number of regrade requests per class", "gauge", "regrade_requests_total"); - generateMetric("pawtograder_discussion_threads_total", "Total number of discussion threads per class", "counter", "discussion_threads_total"); + generateMetric("pawtograder_discussion_threads_total", "Total number of discussion threads per class", "gauge", "discussion_threads_total"); - generateMetric("pawtograder_help_requests_total", "Total number of help requests per class", "counter", "help_requests_total"); + generateMetric("pawtograder_help_requests_total", "Total number of help requests per class", "gauge", "help_requests_total"); - generateMetric("pawtograder_help_request_messages_total", "Total number of help request messages per class", "counter", "help_request_messages_total"); + generateMetric("pawtograder_help_request_messages_total", "Total number of help request messages per class", "gauge", "help_request_messages_total"); - generateMetric("pawtograder_late_token_usage_total", "Total number of late tokens used per class", "counter", "late_token_usage_total"); + generateMetric("pawtograder_late_token_usage_total", "Total number of late tokens used per class", "gauge", "late_token_usage_total"); - generateMetric("pawtograder_video_meeting_sessions_total", "Total number of video meeting sessions per class", "counter", "video_meeting_sessions_total"); + generateMetric("pawtograder_video_meeting_sessions_total", "Total number of video meeting sessions per class", "gauge", "video_meeting_sessions_total"); - generateMetric("pawtograder_video_meeting_participants_total", "Total number of video meeting participants per class", "counter", "video_meeting_participants_total"); + generateMetric("pawtograder_video_meeting_participants_total", "Total number of video meeting participants per class", "gauge", "video_meeting_participants_total"); - generateMetric("pawtograder_hint_feedback_total", "Total number of hint feedback responses per class", "counter", "hint_feedback_total"); + generateMetric("pawtograder_hint_feedback_total", "Total number of hint feedback responses per class", "gauge", "hint_feedback_total"); - generateMetric("pawtograder_hint_feedback_useful_total", "Number of hint feedback responses marked as useful per class", "counter", "hint_feedback_useful_total"); + generateMetric("pawtograder_hint_feedback_useful_total", "Number of hint feedback responses marked as useful per class", "gauge", "hint_feedback_useful_total"); - generateMetric("pawtograder_hint_feedback_with_comments", "Number of hint feedback responses with written comments per class", "counter", "hint_feedback_with_comments"); + generateMetric("pawtograder_hint_feedback_with_comments", "Number of hint feedback responses with written comments per class", "gauge", "hint_feedback_with_comments");If workflow_run_error is append-only, you can keep workflow_errors_total as a counter; otherwise, gauge is safer.
Also applies to: 369-373, 395-399, 415-419, 423-427, 437-441, 451-455, 475-479, 499-503, 513-517, 525-529, 615-619, 621-625, 639-643
supabase/migrations/20250830163212_grader_result_tests_hint_feedback.sql (2)
8-14: Standardize FK column name: grader_result_tests_id → grader_result_test_id.Avoid pluralization inconsistency; it leaks into FK/index names and unique constraints.
- "grader_result_tests_id" bigint not null, + "grader_result_test_id" bigint not null, ... -alter table "public"."grader_result_tests_hint_feedback" add constraint "grader_result_tests_hint_feedback_grader_result_tests_id_fkey" foreign key ("grader_result_tests_id") references "public"."grader_result_tests"("id") on delete cascade; +alter table "public"."grader_result_tests_hint_feedback" add constraint "grader_result_tests_hint_feedback_grader_result_test_id_fkey" foreign key ("grader_result_test_id") references "public"."grader_result_tests"("id") on delete cascade; ... -create index "idx_grader_result_tests_hint_feedback_grader_result_tests_id" on "public"."grader_result_tests_hint_feedback" using btree ("grader_result_tests_id"); +create index "idx_grader_result_tests_hint_feedback_grader_result_test_id" on "public"."grader_result_tests_hint_feedback" using btree ("grader_result_test_id"); ... -alter table "public"."grader_result_tests_hint_feedback" add constraint "grader_result_tests_hint_feedback_unique_per_user_test" unique ("grader_result_tests_id", "created_by"); +alter table "public"."grader_result_tests_hint_feedback" add constraint "grader_result_tests_hint_feedback_unique_per_user_test" unique ("grader_result_test_id", "created_by"); ... -comment on column "public"."grader_result_tests_hint_feedback"."grader_result_tests_id" is 'Foreign key to grader_result_tests table'; +comment on column "public"."grader_result_tests_hint_feedback"."grader_result_test_id" is 'Foreign key to grader_result_tests table';Also applies to: 25-25, 34-35, 41-41, 85-85
103-106: Enforce non-negative tokens and explicit defaults.Prevents bad data and keeps counters monotonic.
- "input_tokens" integer not null, - "output_tokens" integer not null, - "tags" jsonb not null default '{}' + "input_tokens" integer not null default 0, + "output_tokens" integer not null default 0, + "tags" jsonb not null default '{}'::jsonb, + constraint "llm_inference_usage_tokens_nonneg" check (input_tokens >= 0 and output_tokens >= 0)Also applies to: 156-167
supabase/functions/_shared/SupabaseTypes.d.ts (1)
2325-2424: Fix inconsistent FK column name: grader_result_tests_id → grader_result_test_idThis table is the lone outlier using the pluralized FK; elsewhere the singular form is used (e.g., grader_result_test_output, llm_inference_usage). Standardize now to avoid a long tail of inconsistencies and bugs.
Apply this type diff after updating the DB and regenerating types:
- grader_result_tests_id: number; + grader_result_test_id: number; @@ - grader_result_tests_id: number; + grader_result_test_id: number; @@ - grader_result_tests_id?: number; + grader_result_test_id?: number; @@ - foreignKeyName: "grader_result_tests_hint_feedback_grader_result_tests_id_fkey"; - columns: ["grader_result_tests_id"]; + foreignKeyName: "grader_result_tests_hint_feedback_grader_result_test_id_fkey"; + columns: ["grader_result_test_id"]; isOneToOne: false; referencedRelation: "grader_result_tests"; referencedColumns: ["id"];DB patch to run first (then re-run codegen):
ALTER TABLE public.grader_result_tests_hint_feedback RENAME COLUMN grader_result_tests_id TO grader_result_test_id; ALTER TABLE public.grader_result_tests_hint_feedback RENAME CONSTRAINT grader_result_tests_hint_feedback_grader_result_tests_id_fkey TO grader_result_tests_hint_feedback_grader_result_test_id_fkey;Quick repo scan to catch usages:
#!/bin/bash rg -nP -C2 '\bgrader_result_tests_id\b|\bgrader_result_test_id\b'Also, confirm the intended table name aligns with earlier request (“assignment_hint_feedback”). If the shift to test-level is intentional, all client code and docs should reflect it.
🧹 Nitpick comments (9)
app/api/llm-hint/route.ts (3)
200-201: Remove console.log or gate it behind NODE_ENVUse Sentry breadcrumbs instead; avoid noisy logs in production.
- // eslint-disable-next-line no-console - console.log("testId", testId); + Sentry.addBreadcrumb({ level: "info", message: "llm-hint request received", data: { testId } });
265-266: Validate service-role env before useFail fast with a user-visible error if required env vars are missing.
- const serviceSupabase = createServiceClient(process.env.SUPABASE_URL!, process.env.SUPABASE_SERVICE_ROLE_KEY!); + if (!process.env.SUPABASE_URL || !process.env.SUPABASE_SERVICE_ROLE_KEY) { + throw new UserVisibleError("Server misconfiguration: missing SUPABASE_URL or SUPABASE_SERVICE_ROLE_KEY", 500); + } + const serviceSupabase = createServiceClient(process.env.SUPABASE_URL, process.env.SUPABASE_SERVICE_ROLE_KEY);
276-286: Add Sentry tags for provider/model for observabilityMakes debugging and dashboards easier.
const modelName = extraData.llm.model || process.env.OPENAI_MODEL || "gpt-4o-mini"; const providerName = extraData.llm.provider || "openai"; const accountName = extraData.llm.account; + Sentry.setTags({ llm_provider: providerName, llm_model: modelName, llm_account: accountName ?? "default" });supabase/functions/metrics/index.ts (1)
115-123: RPC return type: guard against jsonb array vs object shape drift.get_all_class_metrics() returns jsonb; supabase-js will hand you a JSON value. Add a type guard to verify it’s an array before mapping to ClassMetrics to avoid 500s on unexpected shapes.
- const { data: metricsData, error: metricsError } = (await supabase.rpc("get_all_class_metrics")) as unknown as { - data: ClassMetrics[]; - error: Error; - }; + const { data: rawData, error: metricsError } = await supabase.rpc("get_all_class_metrics"); + const metricsData = Array.isArray(rawData) ? rawData : [];supabase/migrations/20250830163212_grader_result_tests_hint_feedback.sql (4)
46-58: RLS looks reasonable; add created_at index for recent-window queries.hint_feedback_recent_7d filters by created_at; add an index to avoid scans on growth.
+-- Improves 7d window queries used by metrics +create index "idx_grader_result_tests_hint_feedback_created_at" + on "public"."grader_result_tests_hint_feedback" using btree ("created_at");Also applies to: 61-71, 74-80
141-153: Service-role insert policy: good. Consider omitting policy entirely.Security definer/service role bypasses RLS; you can drop the insert policy for a stricter default-deny posture if only service writes.
256-270: Type mismatch cast may defeat indexes (class_id int8 vs int4).The ::integer casts on user_roles.class_id suggest a type mismatch with classes.id (bigint). Consider aligning column types to bigint to keep index usage, or create compatible indexes/casts.
Also applies to: 272-292
377-381: Verify source for “discussion posts” metric.This counts discussion_threads created in 7d, but the help text says “posts.” If there’s a separate posts/comments table, switch to it.
supabase/functions/_shared/SupabaseTypes.d.ts (1)
3716-3810: llm_inference_usage: add created_by FK (and consider enum for provider)created_by is a free string here; most tables FK this to profiles for referential integrity and easier joins. Was omitting the FK intentional?
If not intentional, add:
ALTER TABLE public.llm_inference_usage ADD CONSTRAINT llm_inference_usage_created_by_fkey FOREIGN KEY (created_by) REFERENCES public.profiles(id);Optional: constrain provider via enum to prevent typos:
DO $$ BEGIN CREATE TYPE public.llm_provider AS ENUM ('openai','anthropic','azure_openai'); EXCEPTION WHEN duplicate_object THEN END $$; ALTER TABLE public.llm_inference_usage ALTER COLUMN provider TYPE public.llm_provider USING provider::public.llm_provider;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
app/api/llm-hint/route.ts(1 hunks)jest.config.js(1 hunks)package.json(6 hunks)supabase/functions/_shared/SupabaseTypes.d.ts(3 hunks)supabase/functions/metrics/index.ts(3 hunks)supabase/migrations/20250830163212_grader_result_tests_hint_feedback.sql(1 hunks)tests/unit/llm-hint.test.ts(1 hunks)utils/supabase/SupabaseTypes.d.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- utils/supabase/SupabaseTypes.d.ts
- tests/unit/llm-hint.test.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-30T20:00:05.597Z
Learnt from: dbp
PR: pawtograder/platform#195
File: app/api/llm-hint/route.ts:134-146
Timestamp: 2025-08-30T20:00:05.597Z
Learning: In the OpenAI Node.js SDK, timeout is not a parameter of the chat.completions.create() method itself. Instead, timeout should be configured either at the client level when creating the OpenAI instance, or as an options parameter (second argument) to the create method, not within the main request parameters.
Applied to files:
app/api/llm-hint/route.ts
📚 Learning: 2025-08-23T18:05:42.129Z
Learnt from: jon-bell
PR: pawtograder/platform#173
File: supabase/migrations/20250823074648_create_system_notification_function.sql:296-327
Timestamp: 2025-08-23T18:05:42.129Z
Learning: In PostgreSQL/Supabase, SECURITY DEFINER functions run with the privileges of the function owner. According to PostgreSQL documentation, superusers and roles with the BYPASSRLS attribute always bypass the row security system, and table owners normally bypass row security as well. In Supabase, SECURITY DEFINER functions are typically owned by privileged roles (like the service role) that have superuser privileges, so they bypass RLS policies entirely. Therefore, INSERT policies are not needed for operations performed within SECURITY DEFINER functions owned by privileged roles.
Applied to files:
app/api/llm-hint/route.tssupabase/migrations/20250830163212_grader_result_tests_hint_feedback.sql
📚 Learning: 2025-08-30T20:02:29.900Z
Learnt from: dbp
PR: pawtograder/platform#195
File: app/api/llm-hint/route.ts:134-146
Timestamp: 2025-08-30T20:02:29.900Z
Learning: The `user` parameter in OpenAI's chat.completions.create() method is deprecated, though it remains valid for attribution and abuse tracking purposes as of August 2025.
Applied to files:
app/api/llm-hint/route.ts
📚 Learning: 2025-08-29T02:47:03.315Z
Learnt from: Ari-S-123
PR: pawtograder/platform#168
File: app/course/[course_id]/manage/course/due-date-extensions/tables/rosterTokensTable.tsx:0-0
Timestamp: 2025-08-29T02:47:03.315Z
Learning: The assignment_groups_members table already has RLS enabled and proper policies in place that restrict access based on class authorization. Users should not suggest adding RLS when it already exists.
Applied to files:
supabase/migrations/20250830163212_grader_result_tests_hint_feedback.sql
🧬 Code graph analysis (2)
app/api/llm-hint/route.ts (3)
supabase/functions/_shared/SupabaseTypes.d.ts (1)
Database(3-9158)supabase/functions/_shared/HandlerUtils.ts (1)
UserVisibleError(281-288)utils/supabase/DatabaseTypes.d.ts (2)
GraderResultTestExtraData(24-39)LLMRateLimitConfig(18-22)
supabase/functions/_shared/SupabaseTypes.d.ts (2)
utils/supabase/SupabaseTypes.d.ts (1)
Json(1-1)utils/supabase/DatabaseTypes.d.ts (1)
Json(3-3)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy
🔇 Additional comments (4)
package.json (3)
15-17: LGTM on new test scriptsThe added Jest scripts are standard and useful.
41-41: Ensure these server-only libs never hit client bundlesDouble-check no client-side imports of @langchain/* or openai to avoid bloating the browser bundle.
Also applies to: 44-46, 104-104
139-139: Remove unused ts-jest dependency
ts-jest isn’t referenced in any Jest config or code (only in package.json and package-lock.json); drop"ts-jest": "^29.4.1"from package.json and regenerate your lockfile.supabase/functions/_shared/SupabaseTypes.d.ts (1)
7104-7165: LGTM: video_meeting_session_users types and relationships look consistentMatches existing naming and FK patterns; no issues spotted.
supabase/migrations/20250830163212_grader_result_tests_hint_feedback.sql
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
app/api/llm-hint/route.ts (5)
385-389: Return normalized text (not rawresponse.content).You already normalize via
toText; use it for HTTP response too for consistent shape across providers.return NextResponse.json({ success: true, - response: response.content as string, + response: resultText, cached: false });
68-83: Fix Azure instanceName parsing (currently yields an invalid endpoint).Parse the resource name from the endpoint hostname; the current split("/")[2] returns the full host and breaks Azure requests.
if (provider === "azure") { - const instanceName = process.env.AZURE_OPENAI_ENDPOINT?.split("/")[2]; + const endpoint = process.env.AZURE_OPENAI_ENDPOINT!; + const instanceName = new URL(endpoint).hostname.split(".")[0]; const key_env_name = account ? `AZURE_OPENAI_KEY_${account}` : "AZURE_OPENAI_KEY"; if (!process.env.AZURE_OPENAI_ENDPOINT || !process.env[key_env_name]) { throw new UserVisibleError(`Azure OpenAI endpoint and key are required, must set env var ${key_env_name}`, 500); } return new AzureChatOpenAI({ model, temperature: temperature || 0.85, maxTokens: maxTokens, maxRetries: maxRetries || 2, azureOpenAIApiKey: process.env[key_env_name], azureOpenAIApiInstanceName: instanceName, azureOpenAIApiDeploymentName: model, azureOpenAIApiVersion: "2024-05-01-preview" });
74-83: Add explicit request timeouts to all model clients.Prevents hung lambdas/route handlers; use a sane default or env override. LangChain clients accept
timeout.return new AzureChatOpenAI({ model, temperature: temperature || 0.85, maxTokens: maxTokens, maxRetries: maxRetries || 2, + timeout: parseInt(process.env.LLM_REQUEST_TIMEOUT ?? "30000", 10), azureOpenAIApiKey: process.env[key_env_name], azureOpenAIApiInstanceName: instanceName, azureOpenAIApiDeploymentName: model, azureOpenAIApiVersion: "2024-05-01-preview" }); @@ return new ChatOpenAI({ model, apiKey: process.env[key_env_name], temperature: temperature || 0.85, maxTokens: maxTokens, - maxRetries: maxRetries || 2 + maxRetries: maxRetries || 2, + timeout: parseInt(process.env.LLM_REQUEST_TIMEOUT ?? "30000", 10) }); @@ return new ChatAnthropic({ model, apiKey: process.env[key_env_name], temperature: temperature || 0.85, maxTokens: maxTokens, - maxRetries: maxRetries || 2 + maxRetries: maxRetries || 2, + timeout: parseInt(process.env.LLM_REQUEST_TIMEOUT ?? "30000", 10) });Also applies to: 89-95, 101-107
112-121: Fix prompt template to accept{input}(current chain may throw on extra inputs).Template lacks an
{input}slot while callers pass{ input: ... }.if (!input.prompt) { throw new UserVisibleError("LLM prompt is required", 400); } - return ChatPromptTemplate.fromMessages([["human", input.prompt]]); + return ChatPromptTemplate.fromMessages([["human", "{input}"]]);
199-207: Harden payload parsing; coerce and validatetestId. Also drop stray console.log.Prevents treating strings/NaN as valid IDs; clearer 400 on bad input.
- const { testId } = await request.json(); - // eslint-disable-next-line no-console - console.log("testId", testId); + const body = (await request.json().catch(() => ({}))) as { testId?: unknown }; + const testId = Number((body as any)?.testId); Sentry.setTag("testId", testId); - - if (!testId || typeof testId !== "number" || testId < 0 || !Number.isInteger(testId)) { - throw new UserVisibleError("testId must be a non-negative integer", 400); - } + if (!Number.isInteger(testId) || testId <= 0) { + throw new UserVisibleError("Invalid testId", 400); + }
🧹 Nitpick comments (7)
app/api/llm-hint/route.ts (7)
133-147: UsemaybeSingle()for cooldown lookup (avoid error on 0 rows).
.single()with no rows returns an error; you only need the latest or none..order("created_at", { ascending: false }) - .limit(1) - .single(); + .maybeSingle();
276-286: Tag model/provider/account in Sentry for observability.Adds quick pivots in Sentry without digging into context.
const modelName = extraData.llm.model || process.env.OPENAI_MODEL || "gpt-4o-mini"; const providerName = extraData.llm.provider || "openai"; const accountName = extraData.llm.account; + Sentry.setTag("llm_provider", providerName); + Sentry.setTag("llm_model", modelName); + Sentry.setTag("llm_account", accountName ?? "default");
297-300: Capture token usage in Sentry context.Helpful for cost/limit investigations.
const inputTokens = response.usage_metadata?.input_tokens || 0; const outputTokens = response.usage_metadata?.output_tokens || 0; + Sentry.setContext("llm_usage", { input_tokens: inputTokens, output_tokens: outputTokens });
256-263: Mitigate race that can double-generate and double-charge.Two concurrent requests can both miss the cache and produce two inferences before the first write lands. Consider:
- Re-checking cache immediately before invoking provider, and again before write.
- Adding a unique partial index or a guard on
llm_inference_usage(grader_result_test_id)if one-shot is a hard invariant.- Moving “check+write” into a small SECURITY DEFINER RPC to serialize per-test writes, or using Postgres advisory locks keyed by
testId.Also applies to: 328-345, 351-365
13-13: Typo: renameGradrResultTestWithGraderResultstoGraderResultTestWithGraderResults.Minor polish; improves readability and searchability.
-type GradrResultTestWithGraderResults = GetResult< +type GraderResultTestWithGraderResults = GetResult< @@ -async function checkRateLimits( - testResult: GradrResultTestWithGraderResults, +async function checkRateLimits( + testResult: GraderResultTestWithGraderResults,Also applies to: 122-126
85-88: Validate/sanitizeaccountwhen deriving env var names.Protects against surprising names and misconfig (restrict to
[A-Z0-9_]+, fallback to default).Would you like a tiny helper like
assertValidAccountName()added next togetChatModel?Also applies to: 97-100
31-43: De-duplicateUserVisibleErroracross the codebase.If feasible, move this to a shared
utils/errors.tsto keep status-code semantics consistent between routes and functions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/api/llm-hint/route.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-30T20:00:05.597Z
Learnt from: dbp
PR: pawtograder/platform#195
File: app/api/llm-hint/route.ts:134-146
Timestamp: 2025-08-30T20:00:05.597Z
Learning: In the OpenAI Node.js SDK, timeout is not a parameter of the chat.completions.create() method itself. Instead, timeout should be configured either at the client level when creating the OpenAI instance, or as an options parameter (second argument) to the create method, not within the main request parameters.
Applied to files:
app/api/llm-hint/route.ts
📚 Learning: 2025-08-23T18:05:42.129Z
Learnt from: jon-bell
PR: pawtograder/platform#173
File: supabase/migrations/20250823074648_create_system_notification_function.sql:296-327
Timestamp: 2025-08-23T18:05:42.129Z
Learning: In PostgreSQL/Supabase, SECURITY DEFINER functions run with the privileges of the function owner. According to PostgreSQL documentation, superusers and roles with the BYPASSRLS attribute always bypass the row security system, and table owners normally bypass row security as well. In Supabase, SECURITY DEFINER functions are typically owned by privileged roles (like the service role) that have superuser privileges, so they bypass RLS policies entirely. Therefore, INSERT policies are not needed for operations performed within SECURITY DEFINER functions owned by privileged roles.
Applied to files:
app/api/llm-hint/route.ts
📚 Learning: 2025-08-30T20:02:29.900Z
Learnt from: dbp
PR: pawtograder/platform#195
File: app/api/llm-hint/route.ts:134-146
Timestamp: 2025-08-30T20:02:29.900Z
Learning: The `user` parameter in OpenAI's chat.completions.create() method is deprecated, though it remains valid for attribution and abuse tracking purposes as of August 2025.
Applied to files:
app/api/llm-hint/route.ts
🧬 Code graph analysis (1)
app/api/llm-hint/route.ts (3)
supabase/functions/_shared/HandlerUtils.ts (1)
UserVisibleError(281-288)utils/supabase/DatabaseTypes.d.ts (2)
GraderResultTestExtraData(24-39)LLMRateLimitConfig(18-22)utils/supabase/client.ts (1)
createClient(6-17)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy
…put can be used to explain feedbot
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx (1)
222-266: Race-safe persistence: replace read-then-insert with single upsertThe current check-then-insert can create duplicates under concurrency and break maybeSingle(); prefer an atomic upsert on a unique key.
- if (existingFeedback) { - // Update existing feedback - const { data: updatedFeedback, error: updateError } = await supabase - .from("grader_result_tests_hint_feedback") - .update({ - useful: usefulToSave, - comment: commentToSave.trim() || null - }) - .eq("id", existingFeedback.id) - .select() - .single(); - if (updateError) { - setError("Failed to save feedback: " + updateError.message); - return; - } - if (updatedFeedback) { - setExistingFeedback(updatedFeedback as unknown as GraderResultTestsHintFeedback); - } - } else { - // Insert new feedback - const { data: newFeedback, error: insertError } = await supabase - .from("grader_result_tests_hint_feedback") - .insert({ - class_id: classId, - grader_result_tests_id: testId, - submission_id: submissionId, - hint: hintText, - useful: usefulToSave, - comment: commentToSave.trim() || null, - created_by: private_profile_id - }) - .select() - .single(); - if (insertError) { - setError("Failed to save feedback: " + insertError.message); - return; - } - if (newFeedback) { - setExistingFeedback(newFeedback as unknown as GraderResultTestsHintFeedback); - } - } + const { data: upserted, error: upsertError } = await supabase + .from("grader_result_tests_hint_feedback") + .upsert( + { + id: existingFeedback?.id, // allows update when present + class_id: classId, + grader_result_tests_id: testId, + submission_id: submissionId, + hint: hintText, + useful: usefulToSave, + comment: commentToSave.trim() || null, + created_by: private_profile_id + }, + { onConflict: "grader_result_tests_id,created_by" } + ) + .select() + .maybeSingle(); + if (upsertError) { + setError("Failed to save feedback: " + upsertError.message); + return; + } + if (upserted) { + setExistingFeedback(upserted as unknown as GraderResultTestsHintFeedback); + }Follow-up (outside this file): ensure a unique index on (grader_result_tests_id, created_by) and an UPDATE policy for owners.
🧹 Nitpick comments (5)
app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx (5)
133-145: Use Chakra Spinner instead of icon + Tailwind classFaSpinner + "animate-spin" assumes Tailwind; use the Chakra Spinner you already import for consistency/accessibility.
<Button onClick={handleGetHint} disabled={isLoading} colorPalette="blue" variant="outline" size="sm"> {isLoading ? ( <> - <FaSpinner className="animate-spin" /> + <Spinner size="xs" /> Getting Feedbot Response... </> ) : (
43-51: Add AbortController to prevent stale requests and cancel on unmountPrevents multiple in-flight calls and avoids setting state after unmount.
function LLMHintButton({ testId, onHintGenerated }: { testId: number; onHintGenerated: (hint: string) => void }) { const [isLoading, setIsLoading] = useState(false); const [error, setError] = useState<string | null>(null); + const abortRef = useRef<AbortController | null>(null); const handleGetHint = async () => { setIsLoading(true); setError(null); try { - const response = await fetch("/api/llm-hint", { + // Cancel any previous request + abortRef.current?.abort(); + abortRef.current = new AbortController(); + const response = await fetch("/api/llm-hint", { method: "POST", headers: { "Content-Type": "application/json" }, - body: JSON.stringify({ - testId - }) + body: JSON.stringify({ testId }), + signal: abortRef.current.signal }); @@ } finally { setIsLoading(false); + abortRef.current = null; } }; + useEffect(() => { + return () => abortRef.current?.abort(); + }, []);Also applies to: 52-61, 126-131
178-178: Use cross-platform timeout typeNodeJS.Timeout can be wrong in browser typings; use ReturnType.
- const debounceTimeoutRef = useRef<NodeJS.Timeout>(); + const debounceTimeoutRef = useRef<ReturnType<typeof setTimeout>>();
180-206: Guard fetch until profile id is availableAvoids a pointless round-trip with created_by = undefined and simplifies loading state.
useEffect(() => { const fetchExistingFeedback = async () => { try { const supabase = createClient(); @@ } finally { setIsLoading(false); } }; - fetchExistingFeedback(); -}, [testId, classId, private_profile_id]); + if (private_profile_id) { + fetchExistingFeedback(); + } else { + setIsLoading(false); + } +}, [testId, classId, private_profile_id]);
464-468: Render LLM section if either prompt or stored result existsCurrently it only renders when a prompt exists; stored results without prompt would be hidden.
- const storedHintResult = extraData?.llm?.result; - const displayHint = hintContent || storedHintResult; - const hasLLMPrompt = extraData?.llm?.prompt; + const storedHintResult = extraData?.llm?.result; + const displayHint = hintContent || storedHintResult; + const hasLLM = Boolean(displayHint || extraData?.llm?.prompt); @@ - {hasLLMPrompt && ( + {hasLLM && (Also applies to: 474-528
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx (3)
utils/supabase/DatabaseTypes.d.ts (3)
GraderResultTestsHintFeedback(41-41)GraderResultTestExtraData(24-39)PyretReplConfig(12-16)hooks/useClassProfiles.tsx (1)
useClassProfiles(24-30)utils/supabase/client.ts (1)
createClient(6-17)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy
🔇 Additional comments (5)
app/course/[course_id]/assignments/[assignment_id]/submissions/[submissions_id]/results/page.tsx (5)
558-560: LGTM: fixed format vs output_format mismatchformat_output now adapts GraderResultOutput.format to the basic formatter. This resolves the markdown loss noted earlier.
1049-1069: LGTM: instructor-only outputs now return from mapThe missing return was fixed; instructor-only blocks render correctly.
186-191: Verified correct table, index, and RLS policies for grader_result_tests_hint_feedback; no changes required.
507-507: No sanitization changes required
ReactMarkdown, used without the rehypeRaw plugin or allowDangerousHtml flag, escapes all HTML by default, preventing XSS.
413-417: Buttonloadingprop is supported
Confirmed thatcomponents/ui/button.tsxuses theloadingprop to render a spinner and disable the button. No changes required.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (7)
supabase/functions/_shared/SupabaseTypes.d.ts (1)
2325-2424: Standardize FK: grader_result_tests_id → grader_result_test_id (singular).Matches the rest of the schema (e.g., grader_result_test_output, llm_inference_usage). Keeps API consistent and avoids confusion.
Apply in this file after DB rename/codegen:
- grader_result_tests_id: number; + grader_result_test_id: number; @@ - grader_result_tests_id: number; + grader_result_test_id: number; @@ - grader_result_tests_id?: number; + grader_result_test_id?: number; @@ - foreignKeyName: "grader_result_tests_hint_feedback_grader_result_tests_id_fkey"; - columns: ["grader_result_tests_id"]; + foreignKeyName: "grader_result_tests_hint_feedback_grader_result_test_id_fkey"; + columns: ["grader_result_test_id"];DB patch (then regenerate types):
ALTER TABLE public.grader_result_tests_hint_feedback RENAME COLUMN grader_result_tests_id TO grader_result_test_id; ALTER TABLE public.grader_result_tests_hint_feedback RENAME CONSTRAINT grader_result_tests_hint_feedback_grader_result_tests_id_fkey TO grader_result_tests_hint_feedback_grader_result_test_id_fkey;supabase/functions/metrics/index.ts (2)
285-321: Counters vs gauges: snapshot “totals” should be gauges (unless truly cumulative)These appear to be point-in-time counts from RPCs and can decrease; Prometheus counters must be strictly monotonic. Switch to gauge for these, or change the source to emit cumulative counters from an append-only log. (Same feedback as earlier review.)
-# TYPE pawtograder_workflow_runs_total counter +# TYPE pawtograder_workflow_runs_total gauge ... -# TYPE pawtograder_workflow_errors_total counter +# TYPE pawtograder_workflow_errors_total gauge ... -# TYPE pawtograder_submissions_total counter +# TYPE pawtograder_submissions_total gauge ... -# TYPE pawtograder_submission_reviews_total counter +# TYPE pawtograder_submission_reviews_total gauge ... -# TYPE pawtograder_submission_comments_total counter +# TYPE pawtograder_submission_comments_total gauge ... -# TYPE pawtograder_regrade_requests_total counter +# TYPE pawtograder_regrade_requests_total gauge ... -# TYPE pawtograder_help_requests_total counter +# TYPE pawtograder_help_requests_total gauge ... -# TYPE pawtograder_help_request_messages_total counter +# TYPE pawtograder_help_request_messages_total gauge ... -# TYPE pawtograder_late_token_usage_total counter +# TYPE pawtograder_late_token_usage_total gauge ... -# TYPE pawtograder_video_meeting_sessions_total counter +# TYPE pawtograder_video_meeting_sessions_total gauge ... -# TYPE pawtograder_video_meeting_participants_total counter +# TYPE pawtograder_video_meeting_participants_total gauge ... -# TYPE pawtograder_llm_inference_total counter +# TYPE pawtograder_llm_inference_total gauge ... -# TYPE pawtograder_llm_input_tokens_total counter +# TYPE pawtograder_llm_input_tokens_total gauge ... -# TYPE pawtograder_llm_output_tokens_total counter +# TYPE pawtograder_llm_output_tokens_total gauge ... -# TYPE pawtograder_hint_feedback_total counter +# TYPE pawtograder_hint_feedback_total gauge ... -# TYPE pawtograder_hint_feedback_useful_total counter +# TYPE pawtograder_hint_feedback_useful_total gauge ... -# TYPE pawtograder_hint_feedback_with_comments counter +# TYPE pawtograder_hint_feedback_with_comments gaugeIf any of these are truly cumulative across scrapes and only reset on data loss, keep them as counters and ignore this change for those specific metrics.
Also applies to: 383-389, 409-415, 429-435, 437-443, 451-457, 465-471, 491-495, 513-519, 527-533, 541-545, 573-579, 591-597, 629-641
12-18: Metric name mismatch: “failed” is actually queued/unstartedPrior review noted the SQL selects backlog (requested_at set, not started). Rename to “queued” and use a gauge.
interface ClassMetrics { - workflow_runs_failed: number; + workflow_runs_queued: number; workflow_runs_in_progress: number; workflow_errors_total: number; workflow_runs_timeout: number; } // mapping - workflow_runs_failed: classData.workflow_runs_failed || 0, + workflow_runs_queued: classData.workflow_runs_queued || 0, // emission - generateMetric("pawtograder_workflow_runs_failed", - "Total number of failed workflow runs per class", - "counter","workflow_runs_failed"); + generateMetric("pawtograder_workflow_runs_queued", + "Current number of queued/unstarted workflow runs per class", + "gauge","workflow_runs_queued");Be sure to update the RPC and docs/dashboard accordingly.
Also applies to: 154-160, 299-303
utils/supabase/SupabaseTypes.d.ts (1)
2325-2424: Rename column to singular: grader_result_tests_id → grader_result_test_id (and regenerate types).Keeps schema consistent (e.g., llm_inference_usage uses grader_result_test_id) and fixes broken joins/TS surfaces. This was flagged earlier and still persists.
Apply after fixing the migration and re-running codegen:
- grader_result_tests_id: number; + grader_result_test_id: number; ... - grader_result_tests_id: number; + grader_result_test_id: number; ... - grader_result_tests_id?: number; + grader_result_test_id?: number; ... - foreignKeyName: "grader_result_tests_hint_feedback_grader_result_tests_id_fkey"; - columns: ["grader_result_tests_id"]; + foreignKeyName: "grader_result_tests_hint_feedback_grader_result_test_id_fkey"; + columns: ["grader_result_test_id"];#!/bin/bash # Verify no lingering pluralized usages and that singular exists everywhere. rg -nP '\bgrader_result_tests_id\b' rg -nP '\bgrader_result_test_id\b' # Inspect migrations defining/renaming the column, FK, indexes, unique constraints. fd -t f supabase/migrations | xargs rg -nP 'grader_result_tests_hint_feedback|grader_result_tests_id|grader_result_test_id|_fkey|unique|idx_' # Grep app for payload/selectors still using the pluralized name. rg -nP '\bgrader_result_tests_id\b' app/ tests/ supabase/functions/supabase/migrations/20250830163212_grader_result_tests_hint_feedback.sql (3)
8-14: Standardize FK column name to singular: grader_result_test_id.Use the singular form consistently across column, FKs, indexes, unique constraint, and comments to avoid query confusion and align with existing conventions.
- "grader_result_tests_id" bigint not null, + "grader_result_test_id" bigint not null, ... -alter table "public"."grader_result_tests_hint_feedback" add constraint "grader_result_tests_hint_feedback_grader_result_tests_id_fkey" foreign key ("grader_result_tests_id") references "public"."grader_result_tests"("id") on delete cascade; +alter table "public"."grader_result_tests_hint_feedback" add constraint "grader_result_tests_hint_feedback_grader_result_test_id_fkey" foreign key ("grader_result_test_id") references "public"."grader_result_tests"("id") on delete cascade; ... -create index "idx_grader_result_tests_hint_feedback_grader_result_tests_id" on "public"."grader_result_tests_hint_feedback" using btree ("grader_result_tests_id"); +create index "idx_grader_result_tests_hint_feedback_grader_result_test_id" on "public"."grader_result_tests_hint_feedback" using btree ("grader_result_test_id"); ... -alter table "public"."grader_result_tests_hint_feedback" add constraint "grader_result_tests_hint_feedback_unique_per_user_test" unique ("grader_result_tests_id", "created_by"); +alter table "public"."grader_result_tests_hint_feedback" add constraint "grader_result_tests_hint_feedback_unique_per_user_test" unique ("grader_result_test_id", "created_by"); ... -comment on column "public"."grader_result_tests_hint_feedback"."grader_result_tests_id" is 'Foreign key to grader_result_tests table'; +comment on column "public"."grader_result_tests_hint_feedback"."grader_result_test_id" is 'Foreign key to grader_result_tests table';Also applies to: 25-26, 34-41, 85-85
103-105: Enforce non-negative token counts (and set sane defaults).Prevent bad writes with CHECKs and defaults; keeps metrics reliable.
- "input_tokens" integer not null, - "output_tokens" integer not null, - "tags" jsonb not null default '{}' + "input_tokens" integer not null default 0, + "output_tokens" integer not null default 0, + "tags" jsonb not null default '{}'::jsonb, + constraint "llm_inference_usage_tokens_nonneg" check (input_tokens >= 0 and output_tokens >= 0)
213-219: Metric is queued, not failed — rename the key (and update consumers).This WHERE counts queued/unstarted runs; rename to workflow_runs_queued for accuracy.
- 'workflow_runs_failed', ( + 'workflow_runs_queued', ( SELECT COUNT(*) FROM "public"."workflow_events_summary" WHERE class_id = class_record.id AND requested_at IS NOT NULL AND completed_at IS NULL AND in_progress_at IS NULL ),
🧹 Nitpick comments (14)
supabase/functions/_shared/SupabaseTypes.d.ts (3)
2325-2424: Add uniqueness and supporting indexes for feedback.Prevents duplicate feedback rows and speeds typical lookups.
-- one open feedback per (submission,test,student) CREATE UNIQUE INDEX IF NOT EXISTS grader_result_tests_hint_feedback_uni ON public.grader_result_tests_hint_feedback (submission_id, grader_result_test_id, created_by); -- helpers CREATE INDEX IF NOT EXISTS grader_result_tests_hint_feedback_by_submission ON public.grader_result_tests_hint_feedback (submission_id); CREATE INDEX IF NOT EXISTS grader_result_tests_hint_feedback_by_test ON public.grader_result_tests_hint_feedback (grader_result_test_id); CREATE INDEX IF NOT EXISTS grader_result_tests_hint_feedback_by_class_created ON public.grader_result_tests_hint_feedback (class_id, created_at DESC);
3716-3810: Indexes + defaults for LLM usage metrics.Likely hot paths: by submission/test, time-window per class, tag filtering.
-- query accelerators CREATE INDEX IF NOT EXISTS llm_inference_usage_by_submission ON public.llm_inference_usage (submission_id); CREATE INDEX IF NOT EXISTS llm_inference_usage_by_test ON public.llm_inference_usage (grader_result_test_id); CREATE INDEX IF NOT EXISTS llm_inference_usage_by_class_time ON public.llm_inference_usage (class_id, created_at DESC); CREATE INDEX IF NOT EXISTS llm_inference_usage_by_provider_model ON public.llm_inference_usage (provider, model); CREATE INDEX IF NOT EXISTS llm_inference_usage_tags_gin ON public.llm_inference_usage USING gin ((tags)); -- make tags safe to query without COALESCE ALTER TABLE public.llm_inference_usage ALTER COLUMN tags SET DEFAULT '{}'::jsonb;
7104-7165: Prevent duplicate “open” attendance rows per session and user.Guarantees at most one active join (left_at IS NULL), plus helpful indexes.
CREATE UNIQUE INDEX IF NOT EXISTS video_meeting_session_users_one_open_row ON public.video_meeting_session_users (video_meeting_session_id, private_profile_id) WHERE left_at IS NULL; CREATE INDEX IF NOT EXISTS video_meeting_session_users_by_session ON public.video_meeting_session_users (video_meeting_session_id); CREATE INDEX IF NOT EXISTS video_meeting_session_users_by_class_time ON public.video_meeting_session_users (class_id, joined_at DESC);supabase/functions/metrics/index.ts (5)
782-795: Make token comparison constant-time.every() short-circuits; switch to an XOR accumulator over the digests.
- const expectedHash = await crypto.subtle.digest("SHA-256", expectedBytes); - const providedHash = await crypto.subtle.digest("SHA-256", providedBytes); - return new Uint8Array(expectedHash).every((byte, i) => byte === new Uint8Array(providedHash)[i]); + const expectedHash = new Uint8Array(await crypto.subtle.digest("SHA-256", expectedBytes)); + const providedHash = new Uint8Array(await crypto.subtle.digest("SHA-256", providedBytes)); + let diff = 0; + for (let i = 0; i < expectedHash.length; i++) { + diff |= expectedHash[i] ^ providedHash[i]; + } + return diff === 0;
279-281: Reduce label cardinality (class_name, assignment_title, model/account)High-cardinality labels can bloat Prometheus and slow queries. Prefer stable IDs; move human-readable names into separate “info” metrics or Grafana lookups.
Also applies to: 666-677, 684-686, 693-697, 702-705, 719-723, 735-737, 749-752
265-273: Optional: omit explicit timestampsLet Prometheus assign scrape time unless you need custom timestamps. Simpler output and fewer millisecond skews.
-const timestamp = Math.floor(Date.now()); // Use milliseconds since epoch +// Prefer letting Prometheus assign scrape time; include timestamps only if required. +const timestamp: number | undefined = undefined; ... -pawtograder_info{version="1.0.0"} 1 ${timestamp} +pawtograder_info{version="1.0.0"} 1(And drop “${timestamp}” from emitted samples when undefined.)
268-271: Version should be dynamicHard-coding “1.0.0” makes it easy to drift. Read from env or embed a commit SHA.
-# TYPE pawtograder_info gauge -pawtograder_info{version="1.0.0"} 1 ${timestamp} +# TYPE pawtograder_info gauge +const version = Deno.env.get("APP_VERSION") ?? Deno.env.get("GIT_SHA") ?? "unknown"; +pawtograder_info{version="${version}"} 1 ${timestamp}
275-276: Tighten type for metric “type”Use a string literal union to prevent typos at call sites.
-const generateMetric = (name: string, help: string, type: string, field: keyof ClassMetrics) => { +const generateMetric = (name: string, help: string, type: "counter" | "gauge", field: keyof ClassMetrics) => {utils/supabase/SupabaseTypes.d.ts (1)
7104-7165: Add check constraint for left_at ≥ joined_at
Migrations already include indexes (video_meeting_session_users_video_meeting_session_id_idx,video_meeting_session_users_private_profile_id_idx) and a unique active session–profile index (video_meeting_session_users_session_profile_active_unique_idx), but there’s no check enforcingleft_at >= joined_at. Add:ALTER TABLE public.video_meeting_session_users ADD CONSTRAINT video_meeting_session_users_left_after_join_chk CHECK (left_at IS NULL OR left_at >= joined_at);supabase/migrations/20250830163212_grader_result_tests_hint_feedback.sql (5)
31-38: Add index on created_at for recent-window analytics.You query last 7 days frequently; add an index on created_at for grader_result_tests_hint_feedback.
create index "idx_grader_result_tests_hint_feedback_created_by" on "public"."grader_result_tests_hint_feedback" using btree ("created_by"); +create index "idx_grader_result_tests_hint_feedback_created_at" on "public"."grader_result_tests_hint_feedback" using btree ("created_at");Also applies to: 535-539
148-153: Drop insert policy; rely on service role’s RLS bypass for system writes.Service/privileged roles bypass RLS under SECURITY DEFINER and don’t need an insert policy. Removing this reduces surface area and accidental grants.
- create policy "System can insert LLM usage data" - on "public"."llm_inference_usage" - for insert - with check (auth.role() = 'service_role'); + -- Omit insert policies so only the service role (RLS bypass) can write.
257-270: Avoid downcasting class IDs to integer in comparisons.Casting class_record.id to integer risks truncation/mismatch if ids exceed 32-bit. Compare without downcast (Postgres can implicitly handle bigint=int) or cast user_roles.class_id up to bigint.
- WHERE ur.class_id = class_record.id::integer + WHERE ur.class_id = class_record.id ... - WHERE ur.class_id = class_record.id::integer + WHERE ur.class_id = class_record.idIf user_roles.class_id is actually integer, consider migrating it to bigint for consistency.
Also applies to: 271-293
92-106: Clarify jsonb default literal.Use '{}'::jsonb for explicit typing; avoids accidental text default if types change.
- "tags" jsonb not null default '{}' + "tags" jsonb not null default '{}'::jsonb
156-167: Consider adding comments for new CHECKs/defaults.Document token constraints in column comments so downstream consumers understand invariants.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
supabase/functions/_shared/SupabaseTypes.d.ts(5 hunks)supabase/functions/metrics/index.ts(5 hunks)supabase/migrations/20250830163212_grader_result_tests_hint_feedback.sql(1 hunks)utils/supabase/SupabaseTypes.d.ts(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-23T18:05:42.129Z
Learnt from: jon-bell
PR: pawtograder/platform#173
File: supabase/migrations/20250823074648_create_system_notification_function.sql:296-327
Timestamp: 2025-08-23T18:05:42.129Z
Learning: In PostgreSQL/Supabase, SECURITY DEFINER functions run with the privileges of the function owner. According to PostgreSQL documentation, superusers and roles with the BYPASSRLS attribute always bypass the row security system, and table owners normally bypass row security as well. In Supabase, SECURITY DEFINER functions are typically owned by privileged roles (like the service role) that have superuser privileges, so they bypass RLS policies entirely. Therefore, INSERT policies are not needed for operations performed within SECURITY DEFINER functions owned by privileged roles.
Applied to files:
supabase/migrations/20250830163212_grader_result_tests_hint_feedback.sql
📚 Learning: 2025-08-29T02:47:03.315Z
Learnt from: Ari-S-123
PR: pawtograder/platform#168
File: app/course/[course_id]/manage/course/due-date-extensions/tables/rosterTokensTable.tsx:0-0
Timestamp: 2025-08-29T02:47:03.315Z
Learning: The assignment_groups_members table already has RLS enabled and proper policies in place that restrict access based on class authorization. Users should not suggest adding RLS when it already exists.
Applied to files:
supabase/migrations/20250830163212_grader_result_tests_hint_feedback.sql
🧬 Code graph analysis (2)
utils/supabase/SupabaseTypes.d.ts (1)
supabase/functions/_shared/SupabaseTypes.d.ts (1)
Json(1-1)
supabase/functions/_shared/SupabaseTypes.d.ts (1)
utils/supabase/SupabaseTypes.d.ts (1)
Json(1-1)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy
🔇 Additional comments (6)
supabase/functions/_shared/SupabaseTypes.d.ts (2)
2325-2424: Verified RLS policies and table referencesRLS is enabled on grader_result_tests_hint_feedback with SELECT using authorize_for_submission(submission_id) (and authorizeforclassgrader), and INSERT/UPDATE/DELETE policies enforce authorize_for_submission(submission_id) AND authorizeforprofile(created_by); UI code correctly queries grader_result_tests_hint_feedback.
8858-8861: No action—LLM metrics RPCs already SECURITY DEFINER and locked to service_roleThese functions are defined with SECURITY DEFINER, owned by postgres, and GRANT-EXECUTE only to the service_role (with authenticated/anon revoked), so they run in the intended privileged context and don’t require additional authorizeforclass checks.
utils/supabase/SupabaseTypes.d.ts (3)
8858-8862: Secured get_assignment_llm_metrics as requested. Function is defined with SECURITY DEFINER, owned by postgres, REVOKE ALL from anon/authenticated and GRANT EXECUTE to service_role.
8870-8874: Confirm and lock down get_llm_tags_breakdown RPC
- Verify a migration defines
public.get_llm_tags_breakdownwithSECURITY DEFINER(owned by service_role) and that defaultEXECUTEprivileges are revoked and only intended roles are granted. I didn’t find a corresponding migration undersupabase/migrations.
3716-3810: Remove redundant FK/index suggestions: thellm_inference_usage_created_by_fkeyand btree indexes onsubmission_id,grader_result_test_id,class_id, andcreated_atare already created in 20250830163212_grader_result_tests_hint_feedback.sql.Likely an incorrect or invalid review comment.
supabase/migrations/20250830163212_grader_result_tests_hint_feedback.sql (1)
169-190: Good SECURITY DEFINER hardening and execution gating.Explicit search_path + role gate + limited GRANTs look solid.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
supabase/migrations/20250901182633_fix-create-invitation.sql (5)
122-124: Time zone hygiene nit: use UTC consistently.If the rest of the system uses UTC timestamps, prefer
timezone('utc', now())for deterministic behavior across deployments.- v_expires_at := NOW() + INTERVAL '30 days'; + v_expires_at := timezone('utc', now()) + INTERVAL '30 days';
126-158: Use resolved inviter id in insert (if adopting earlier refactor).- p_invited_by, + v_invited_by_effective,
100-121: Lifecycle consideration: cleanup pre-created profiles for expired invites.Two profiles are created per invite. Consider a scheduled cleanup (or a trigger) to delete orphaned profiles when an invitation expires and was never accepted.
If you want, I can draft a small SQL function + cron trigger to prune these safely.
Also applies to: 126-158
164-175: Comment reads well; add note about section validation and inviter resolution if implemented.-) IS 'Creates a course invitation with pre-created profiles for a user identified by sis_user_id. Public profiles get random names while private profiles use the actual user name. Supports sis_managed flag to control whether invitations can be expired by SIS sync.'; +) IS 'Creates a course invitation with pre-created profiles for a user identified by sis_user_id. Public profiles get random names while private profiles use the actual user name. Validates that provided section ids belong to the class. Resolves inviter from param or auth.uid(). Supports sis_managed flag to control whether invitations can be expired by SIS sync.';
16-31: Resolvep_invited_byinside the function body
Verified that the only RPC caller (InvitationUtils.ts) passesp_invited_byexplicitly, so changing its default toNULLand usingCOALESCEis safe. Apply:- p_invited_by uuid DEFAULT auth.uid(), + p_invited_by uuid DEFAULT NULL, @@ DECLARE v_public_profile_id uuid; v_private_profile_id uuid; v_invitation_id bigint; + v_invited_by_effective uuid; @@ BEGIN + -- Resolve caller explicitly to avoid NULLs in service contexts + v_invited_by_effective := COALESCE(p_invited_by, auth.uid());And update the INSERT at line 150 to use
v_invited_by_effectiveinstead ofp_invited_by.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
app/api/llm-hint/route.ts(1 hunks)supabase/migrations/20250901182633_fix-create-invitation.sql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/api/llm-hint/route.ts
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: deploy
🔇 Additional comments (3)
supabase/migrations/20250901182633_fix-create-invitation.sql (3)
100-121: Avoid double-inserting the public profile after adopting the conflict-safe loop.If you implement the
ON CONFLICTloop above, delete this public-profile INSERT to prevent creating two rows.- INSERT INTO public.profiles ( - name, - class_id, - is_private_profile - ) VALUES ( - v_public_name, - p_class_id, - false - ) RETURNING id INTO v_public_profile_id;
45-47: Confirm auth helper naming
Ensure the functionpublic.authorizeforclassinstructor(p_class_id)actually exists; if the canonical helper is namedpublic.authorize_for_class_instructor(p_class_id bigint), update the call accordingly to avoid runtime errors.
4-14: Drop signature covers the sole overload. The only existingcreate_invitationis defined with the same nine-parameter signature (defaults don’t alter types), so theDROP FUNCTION …fully removes it before recreation.
This adds support for autograders providing custom LLM-backed feedback to parts of assignments.
It is both minimal -- it requires no database change, and minimal UI changes -- and flexibly autograder-agnostic: any autograder can easily plug into the mechanism, using it for individual problems, for entire assignments, or for really anything where executing a prompt on-demand can be useful for student. It's also essentially a direct port of the existing https://github.com/NUFeedBot/feedbot-server architecture -- and takes from that the one-shot nature -- there is no ability to get additional rounds of feedback, or refinement on the results, though that's certainly something we could explore in the future. There also isn't currently any rate limited implemented -- while there is natural rate limiting based on the fact that everything is on-demand, it may be desirable to put overall caps per student, or per assignment, and display an error if they are hit -- this would fit in pretty well with the existing architecture.
To use it, autograder should add:
To a test result to turn it into an LLM test result. This changes the UI to:
Once you click the button, an api call is made to openai, which sends the full prompt from
llm_hint_prompt(exactly as written, no additions or changes), and the response is put in.Note, importantly, that the prompt is never displayed to the user, though it is stored (permanently), in case it is helpful for debugging, research, etc.
The response, in addition to being rendered, is stored in
llm.resultinextra_data, and on future page loads this is rendered rather than the button.Note that while the intention is that test results that use this feature always have
scoreandmax_scoreof 0, since we intentionally did not alter any of the totalling logic, we still display the point values in both of the summary tables, in case the scores are not 0/0.One final note -- currently this points against the OpenAI API -- we should change this to the AzureOpenAI API -- that's a couple line change though.
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Database & Metrics
Documentation
Tests