diff --git a/.gitignore b/.gitignore index 36eebe5ba..6859af91b 100644 --- a/.gitignore +++ b/.gitignore @@ -69,6 +69,7 @@ axe-debug/ /logs/ /screenshots/ /screenshots-*/ +/sweep-results-*/ /.e2e-*.done /.build.done /screenshot-diff-report.json diff --git a/app/course/[course_id]/discussion/discussion_thread.tsx b/app/course/[course_id]/discussion/discussion_thread.tsx index 35d027266..e7d8a1491 100644 --- a/app/course/[course_id]/discussion/discussion_thread.tsx +++ b/app/course/[course_id]/discussion/discussion_thread.tsx @@ -47,6 +47,7 @@ export function DiscussionThreadReply({ } }, [visible]); const { tableController } = useDiscussionThreadsController(); + const courseController = useCourseController(); const sendMessage = useCallback( async (message: string, profile_id: string, close = true) => { @@ -64,16 +65,27 @@ export function DiscussionThreadReply({ body: message }); - // invalidate({ - // resource: "discussion_threads", - // invalidates: ['detail'], - // id: thread.parent! - // }); if (close) { setVisible(false); } + + // Replying auto-follows the thread via a server-side trigger that + // INSERTs into discussion_thread_watchers. The Follow→Unfollow + // button transition reads that row through a realtime-backed + // TableController, and on slow CI runs realtime delivery of the + // new watcher row lags the reply create by several seconds, during + // which the button keeps saying "Follow". Warm the watcher cache + // directly so the UI flips without waiting on the broadcast. Run + // this after the composer close because it's best-effort — + // realtime is the authoritative path — and we don't want a slow + // REST round-trip stalling the close transition. + void courseController.discussionThreadWatchers + .getOneByFilters([{ column: "discussion_thread_root_id", operator: "eq", value: thread.root || thread.id }]) + .catch(() => { + // Realtime will catch up. + }); }, - [tableController, setVisible, thread] + [tableController, courseController, setVisible, thread] ); if (!visible) { return <>; diff --git a/app/course/[course_id]/office-hours/[queue_id]/[request_id]/page.tsx b/app/course/[course_id]/office-hours/[queue_id]/[request_id]/page.tsx index 8fa55166b..b80793f35 100644 --- a/app/course/[course_id]/office-hours/[queue_id]/[request_id]/page.tsx +++ b/app/course/[course_id]/office-hours/[queue_id]/[request_id]/page.tsx @@ -3,9 +3,12 @@ import { useParams } from "next/navigation"; import CurrentRequest from "../currentRequest"; import { useQueueData } from "@/hooks/useQueueData"; -import { useMemo, useState } from "react"; -import { Box, Flex, useBreakpointValue } from "@chakra-ui/react"; +import { useEffect, useMemo, useState } from "react"; +import { Box, Flex, Spinner, useBreakpointValue } from "@chakra-ui/react"; import { HelpRequestSidebar } from "@/components/help-queue/help-request-sidebar"; +import { useOfficeHoursController } from "@/hooks/useOfficeHoursRealtime"; + +type LoadState = "pending" | "loaded" | "not_found" | "error"; export default function RequestDetailPage() { const { queue_id, course_id, request_id } = useParams(); @@ -18,11 +21,61 @@ export default function RequestDetailPage() { queueId: Number(queue_id) }); + const requestIdNum = Number(request_id); + // Find the specific request - could be from user's requests or queue requests const request = useMemo(() => { - const requestIdNum = Number(request_id); return userRequests.find((req) => req.id === requestIdNum) || queueRequests.find((req) => req.id === requestIdNum); - }, [userRequests, queueRequests, request_id]); + }, [userRequests, queueRequests, requestIdNum]); + + // The realtime-backed help_requests controller is populated by an + // initial query plus realtime INSERT broadcasts. If a user lands here + // via a deep link, a freshly-created request whose realtime broadcast + // hasn't arrived yet, or a navigation that races the initial query, + // `request` is undefined and we used to render a flat "Request not + // found." — which is wrong for the loading case and surfaces as an + // e2e flake. Distinguish "not yet loaded" from "definitively missing" + // by issuing a one-shot single-row fetch on mount; only flip to + // not_found once that fetch has resolved and the row is still absent. + // The fetch writes through the controller's cache, so the existing + // useQueueData / useHelpRequests hooks pick it up via their normal + // notification path. + const controller = useOfficeHoursController(); + const [loadState, setLoadState] = useState(request ? "loaded" : "pending"); + useEffect(() => { + if (request) { + setLoadState("loaded"); + return; + } + let cancelled = false; + setLoadState("pending"); + controller.helpRequests + .invalidate(requestIdNum) + .then(() => { + if (cancelled) return; + // `invalidate` resolves successfully both when the row was found + // (and added to cache — `useQueueData` will pick it up via its + // listener and the effect will re-run with `request` truthy) + // AND when the row was not found (single() returned data:null + // without an explicit error, e.g. RLS filtered it out). Only + // flip to "not_found" if the row really isn't in cache; never + // flip a transient "not_found" between the cache write and the + // re-render, which would cause a flash of the error state. + if (!controller.helpRequests.getById(requestIdNum).data) { + setLoadState("not_found"); + } + }) + .catch(() => { + // Distinct from "not_found": this fires when the REST round-trip + // itself fails (network blip, auth glitch, 5xx). Rendering + // "Request not found." in those cases is misleading — the row + // may very well exist, we just couldn't fetch it. + if (!cancelled) setLoadState("error"); + }); + return () => { + cancelled = true; + }; + }, [request, controller, requestIdNum]); // Calculate position in queue for active requests const position = useMemo(() => { @@ -33,7 +86,17 @@ export default function RequestDetailPage() { }, [request, queueRequests]); if (!request) { - return
Request not found.
; + if (loadState === "not_found") { + return
Request not found.
; + } + if (loadState === "error") { + return
Unable to load this request right now. Please try again in a moment.
; + } + return ( + + + + ); } return ( diff --git a/app/course/[course_id]/office-hours/[queue_id]/new/newRequestForm.tsx b/app/course/[course_id]/office-hours/[queue_id]/new/newRequestForm.tsx index ec57771a8..594792375 100644 --- a/app/course/[course_id]/office-hours/[queue_id]/new/newRequestForm.tsx +++ b/app/course/[course_id]/office-hours/[queue_id]/new/newRequestForm.tsx @@ -1,5 +1,6 @@ "use client"; +import { createClient } from "@/utils/supabase/client"; import { HelpRequestFormFileReference } from "@/components/help-queue/help-request-chat"; import { OfficeHoursDiscussionBrowser } from "@/components/help-queue/office-hours-discussion-browser"; import { Checkbox } from "@/components/ui/checkbox"; @@ -23,7 +24,6 @@ import { Assignment, HelpRequest, HelpRequestLocationType, - HelpRequestMessage, HelpRequestTemplate, HelpRequestWithStudentCount, Submission, @@ -69,8 +69,6 @@ export default function HelpRequestForm({ // Use ref to avoid closure issues with selectedStudents in async callbacks const selectedStudentsRef = useRef([]); - // Track if we've created the initial message to avoid duplicates on retries - const createdInitialMessageRef = useRef(false); // Update ref whenever selectedStudents changes useEffect(() => { @@ -114,8 +112,9 @@ export default function HelpRequestForm({ const { private_profile_id } = useClassProfiles(); // Get table controllers from office hours controller + // Only used here to warm the message cache for the about-to-mount chat + // page; the actual writes happen inside the RPC. const controller = useOfficeHoursController(); - const { helpRequestStudents, helpRequests, helpRequestFileReferences, studentHelpActivity } = controller; // Get available help queues using individual hook const allHelpQueues = useHelpQueues(); @@ -484,134 +483,73 @@ export default function HelpRequestForm({ } // Group requests are always allowed - no validation needed - // Create a custom onFinish function that excludes file_references and adds required fields + // Submit through a single atomic Postgres RPC. The RPC creates + // help_requests + help_request_students + help_request_messages + + // help_request_file_references + student_help_activity in one + // transaction with SECURITY DEFINER auth checks. Replaces the + // legacy 4–5 sequential client-side writes that left the door + // open for partially-created requests under realtime backpressure + // (see the TODO at the top of this file and migration + // 20260524000944_create_help_request_with_participants_rpc.sql). const customOnFinish = async (values: Record) => { - // Exclude file_references from the submission data since it's not a column in help_requests table - // eslint-disable-next-line @typescript-eslint/no-unused-vars - const { _intended_privacy, file_references, ...helpRequestData } = values; - - // Add required fields that may not be set in the form - const finalData = { - ...helpRequestData, - assignee: null, - class_id: Number.parseInt(course_id as string), - created_by: private_profile_id, // Set the created_by field - // Ensure these fields have proper defaults - status: "open" as const, - is_video_live: false, - is_private: values.is_private || false - }; - - try { - const createdHelpRequest = await helpRequests.create(finalData as unknown as HelpRequest); - const helpRequestMessages = controller.loadMessagesForHelpRequest(createdHelpRequest.id); - // Get current selected students from ref to avoid closure issues - const currentSelectedStudents = selectedStudentsRef.current; - - if (!createdHelpRequest.id) { - throw new Error("Help request ID not found in response data"); - } - - // Add all selected students to help_request_students - if (currentSelectedStudents.length > 0) { - for (const studentId of currentSelectedStudents) { - try { - await helpRequestStudents.create({ - help_request_id: createdHelpRequest.id, - profile_id: studentId, - class_id: Number.parseInt(course_id as string) - }); - } catch (error) { - const msg = getStudentFacingErrorMessage(error); - toaster.error({ - title: "Could not add everyone to the request", - description: `We could not add a classmate to this help request: ${msg}` - }); - throw new Error(`Failed to create student associations: ${msg}`); - } - } + const currentSelectedStudents = selectedStudentsRef.current; + if (currentSelectedStudents.length === 0) { + toaster.error({ + title: "Error", + description: "No students selected for help request" + }); + throw new Error("No students selected for help request"); + } - // Log activity for all students in the help request - for (const studentId of currentSelectedStudents) { - try { - await studentHelpActivity.create({ - student_profile_id: studentId, - class_id: Number.parseInt(course_id as string), - help_request_id: createdHelpRequest.id, - activity_type: "request_created", - activity_description: `Student created a new help request in queue: ${helpQueues.find((q) => q.id === createdHelpRequest.help_queue)?.name || "Unknown"}` - }); - } catch { - // Don't throw here - activity logging shouldn't block request creation - } - } - } else { - toaster.error({ - title: "Error", - description: "No students selected for help request" - }); - throw new Error("No students selected for help request"); - } + // Build the file-references payload the RPC expects. We only + // include refs once we've resolved their assignment_id, which + // comes from the selected submission. If a submission is + // referenced without an assignment we drop the file refs (the + // legacy code threw on this — RPC-side it's a quiet skip). + const requestText = ((values.request as string) ?? "").trim(); + const formFileRefs = (getValues("file_references") ?? []) as HelpRequestFormFileReference[]; + const referencedSubmissionId = getValues("referenced_submission_id") as number | null | undefined; + const selectedSubmission = + referencedSubmissionId != null + ? submissions?.data?.find((s) => s.id === referencedSubmissionId) + : undefined; + const fileReferencesPayload = + formFileRefs.length > 0 && selectedSubmission?.assignment_id + ? formFileRefs.map((ref) => ({ + submission_file_id: ref.submission_file_id, + line_number: ref.line_number, + assignment_id: selectedSubmission.assignment_id, + submission_id: referencedSubmissionId + })) + : []; - // Create the initial chat message from the request description so it shows in the conversation view - try { - const requestText = (getValues("request") as string) || ""; - if (requestText.trim().length > 0 && private_profile_id) { - const trimmedText = requestText.trim(); - // Check existing cached messages and local ref to prevent duplicates on retry - const existingLocal = (helpRequestMessages.rows || []).some( - (m: HelpRequestMessage) => - Number(m.help_request_id) === Number(createdHelpRequest.id) && - String(m.author) === String(private_profile_id) && - ((m.message as string) || "").trim() === trimmedText - ); - if (!createdInitialMessageRef.current && !existingLocal) { - await helpRequestMessages.create({ - message: requestText, - help_request_id: createdHelpRequest.id, - author: private_profile_id, - class_id: Number.parseInt(course_id as string), - instructors_only: false, - reply_to_message_id: null - }); - createdInitialMessageRef.current = true; - } + try { + const supabase = createClient(); + const { data: createdHelpRequestId, error: rpcError } = await supabase.rpc( + "create_help_request_with_participants", + { + p_help_queue_id: Number.parseInt(queue_id as string), + p_request: requestText, + p_is_private: Boolean(values.is_private), + p_location_type: (values.location_type as HelpRequestLocationType) ?? "remote", + p_student_profile_ids: currentSelectedStudents, + p_template_id: (values.template_id as number | null | undefined) ?? undefined, + p_referenced_submission_id: referencedSubmissionId ?? undefined, + p_file_references: fileReferencesPayload as never } - } catch { + ); + if (rpcError) { + const isRls = rpcError.code === "42501"; toaster.error({ - title: "Error", - description: "Failed to create initial chat message with help request description." + title: isRls ? "Permission issue" : "Could not complete help request", + description: isRls + ? "You don't have permission to create this help request with these settings. Try making the request public instead of private, or contact your instructor." + : getStudentFacingErrorMessage(rpcError) }); + throw rpcError; } - - // Create file references if any - const fileReferences = getValues("file_references") || []; - if (fileReferences.length > 0) { - // Get assignment_id from the selected submission - const selectedSubmission = submissions?.data?.find((s) => s.id === getValues("referenced_submission_id")); - if (!selectedSubmission?.assignment_id) { - throw new Error("Assignment ID not found for the selected submission"); - } - - for (const ref of fileReferences) { - try { - await helpRequestFileReferences.create({ - help_request_id: createdHelpRequest.id, - class_id: Number.parseInt(course_id as string), - assignment_id: selectedSubmission.assignment_id, - submission_file_id: ref.submission_file_id, - submission_id: getValues("referenced_submission_id"), - line_number: ref.line_number - }); - } catch (error) { - const msg = getStudentFacingErrorMessage(error); - toaster.error({ - title: "Could not attach code reference", - description: msg - }); - throw new Error(`Failed to create file reference: ${msg}`); - } - } + if (createdHelpRequestId == null) { + throw new Error("create_help_request_with_participants did not return an id"); } toaster.success({ @@ -619,21 +557,27 @@ export default function HelpRequestForm({ description: "Help request successfully created. Opening your request..." }); - // Navigate to queue view BEFORE any state-clearing work. The form is - // about to unmount, so resetting state first is unnecessary and risky: - // each setState triggers a re-render whose effects (validation, error - // clearing, useList refetches) can run on the same microtask as - // router.push and silently swallow the navigation under load. This - // was observed on webkit in CI where the success toast fired but the - // URL never changed (issue tracked: form's post-create writes should - // be collapsed into a single RPC; see TODO at top of file). + // Pre-load the messages controller so the chat page mounts + // with the initial message already cached. The chat page + // itself handles fetching the help_requests row on mount if + // realtime hasn't delivered it yet (see RequestDetailPage's + // invalidate-on-mount effect) — we deliberately don't fetch + // it here because we'd just be racing the same realtime + // broadcast from the wrong side, and an awaited fetch can + // stall router.push if the REST round-trip is slow. + controller.loadMessagesForHelpRequest(createdHelpRequestId); + navigated = true; - router.push(`/course/${course_id}/office-hours/${queue_id}/${createdHelpRequest.id}`); + router.push(`/course/${course_id}/office-hours/${queue_id}/${createdHelpRequestId}`); } catch (error) { - toaster.error({ - title: "Could not complete help request", - description: getStudentFacingErrorMessage(error) - }); + // RPC error path already toasted above; this catches non-RPC + // throws (e.g. the empty-students guard re-throwing). + if (!(error && typeof error === "object" && "code" in error)) { + toaster.error({ + title: "Could not complete help request", + description: getStudentFacingErrorMessage(error) + }); + } } }; @@ -662,14 +606,10 @@ export default function HelpRequestForm({ getValues, selectedStudents, helpQueues, - helpRequestFileReferences, - helpRequests, - helpRequestStudents, queue_id, controller, router, submissions?.data, - studentHelpActivity, templates.length ] ); @@ -705,12 +645,6 @@ export default function HelpRequestForm({ ); } - // Compute hasErrors, ignoring empty root object left by React Hook Form - const hasErrors = Object.keys(errors).some( - (key) => - key !== "root" || (key === "root" && Object.keys((errors as Record).root || {}).length > 0) - ); - return (
@@ -1245,7 +1179,18 @@ export default function HelpRequestForm({