Skip to content

Fix loading race conditions #1667

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nygrenh merged 2 commits into from
Mar 6, 2026
Merged

Fix loading race conditions #1667

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e60d07b
Refactor ExamPageShell to replace useHydrateAtoms with useEffect for …
nygrenh Mar 6, 2026
f7ff1af
Fix race conditions when loading exercises
nygrenh Mar 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 7 additions & 11 deletions ...es/main-frontend/src/app/org/[organizationSlug]/(course-material)/exams/ExamPageShell.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import { css } from "@emotion/css"
import { isPast } from "date-fns"
import { useAtomValue, useSetAtom } from "jotai"
import { useHydrateAtoms } from "jotai/utils"
import React, { useCallback, useEffect, useMemo } from "react"
import { useTranslation } from "react-i18next"

Expand Down Expand Up @@ -66,16 +65,13 @@ export default function ExamPageShell({
[examId, mode],
)

useHydrateAtoms(
useMemo(
() =>
[
[organizationSlugAtom, organizationSlug],
[viewParamsAtom, viewParams],
] as const,
[organizationSlug, viewParams],
),
)
const setOrganizationSlug = useSetAtom(organizationSlugAtom)
const setViewParams = useSetAtom(viewParamsAtom)

useEffect(() => {
setOrganizationSlug(organizationSlug)
setViewParams(viewParams)
}, [organizationSlug, viewParams, setOrganizationSlug, setViewParams])

const courseMaterialState = useAtomValue(courseMaterialAtom)
const triggerRefetch = useSetAtom(refetchViewAtom)
Expand Down
18 changes: 17 additions & 1 deletion ...al/ContentRenderer/moocfi/ExerciseBlock/PeerOrSelfReviewView/PeerOrSelfReviewViewImpl.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,10 +192,26 @@ const PeerOrSelfReviewViewImpl: React.FC<React.PropsWithChildren<PeerOrSelfRevie
}

// Uses isFetching instead of isPending because we want there to be a visual indication when the refresh button is clicked
if (query.isFetching || !query.data) {
if (query.isFetching) {
return <Spinner variant="medium" />
}

if (!query.data) {
return (
<div>
<ErrorBanner variant={"readOnly"} error={t("error-loading-exercise")} />
<button
className={cx(exerciseButtonStyles)}
onClick={() => {
void query.refetch()
}}
>
{t("button-text-try-again")}
</button>
</div>
)
}

if (!peerOrSelfReviewData?.answer_to_review?.course_material_exercise_tasks) {
return (
<div>
Expand Down
21 changes: 20 additions & 1 deletion ...in-frontend/src/components/course-material/ContentRenderer/moocfi/ExerciseBlock/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ const ExerciseBlock: React.FC<
const courseMaterialState = useAtomValue(courseMaterialAtom)
const showExercise =
Boolean(courseMaterialState.examData?.id) ||
courseMaterialState.status === "loading" ||
(loginState.signedIn ? !!courseMaterialState.settings : true)
const [postThisStateToIFrame, dispatch] = useReducer(
exerciseBlockPostThisStateToIFrameReducer,
Expand Down Expand Up @@ -301,9 +302,27 @@ const ExerciseBlock: React.FC<
if (getCourseMaterialExercise.isError) {
return <ErrorBanner variant={"readOnly"} error={getCourseMaterialExercise.error} />
}
if (getCourseMaterialExercise.isLoading || !getCourseMaterialExercise.data) {
if (getCourseMaterialExercise.isLoading) {
return <Spinner variant={"medium"} />
}
if (!getCourseMaterialExercise.data) {
return (
<div>
<ErrorBanner
variant={"readOnly"}
error={t("error-loading-exercise", { defaultValue: "Error loading exercise" })}
/>
<button
className={cx(exerciseButtonStyles)}
onClick={() => {
void getCourseMaterialExercise.refetch()
}}
>
{t("button-text-try-again", { defaultValue: "Try again" })}
</button>
</div>
)
}
Comment on lines +305 to +325
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Use isFetching instead of isLoading for consistent refetch feedback.

When the user clicks the Retry button, refetch() is called. However, React Query's isLoading is only true during the initial load, not during refetches—only isFetching becomes true during refetch operations. This means the spinner won't display while the retry is in progress, leaving users without visual feedback.

This is inconsistent with PeerOrSelfReviewViewImpl.tsx (lines 195-197 in context snippet 2), which correctly uses isFetching with an explicit comment explaining: "Uses isFetching instead of isPending because we want there to be a visual indication when the refresh button is clicked".

Proposed fix 
-  if (getCourseMaterialExercise.isLoading) {
+  if (getCourseMaterialExercise.isFetching) {
     return <Spinner variant={"medium"} />
   }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (getCourseMaterialExercise.isLoading) {
return <Spinner variant={"medium"} />
}
if (!getCourseMaterialExercise.data) {
return (
<div>
<ErrorBanner
variant={"readOnly"}
error={t("error-loading-exercise", { defaultValue: "Error loading exercise" })}
/>
<button
className={cx(exerciseButtonStyles)}
onClick={() => {
void getCourseMaterialExercise.refetch()
}}
>
{t("button-text-try-again", { defaultValue: "Try again" })}
</button>
</div>
)
}
if (getCourseMaterialExercise.isFetching) {
return <Spinner variant={"medium"} />
}
if (!getCourseMaterialExercise.data) {
return (
<div>
<ErrorBanner
variant={"readOnly"}
error={t("error-loading-exercise", { defaultValue: "Error loading exercise" })}
/>
<button
className={cx(exerciseButtonStyles)}
onClick={() => {
void getCourseMaterialExercise.refetch()
}}
>
{t("button-text-try-again", { defaultValue: "Try again" })}
</button>
</div>
)
}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In
`@services/main-frontend/src/components/course-material/ContentRenderer/moocfi/ExerciseBlock/index.tsx`
around lines 305 - 325, Replace the use of React Query's isLoading with
isFetching in the render gating for getCourseMaterialExercise so the Spinner
(Spinner component) shows during refetches triggered by
getCourseMaterialExercise.refetch(); specifically, change checks that reference
getCourseMaterialExercise.isLoading to use getCourseMaterialExercise.isFetching
(and keep the existing ErrorBanner + retry button behavior) so clicking the
retry button gives a visual loading indicator while the fetch is in progress.


const courseInstanceId = courseMaterialState.instance?.id
const chapterStatus = chapterId
Expand Down
2 changes: 2 additions & 0 deletions shared-module/packages/common/src/locales/en/main-frontend.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,7 @@
"button-text-signed-in": "Signed in",
"button-text-submit": "Submit",
"button-text-submit-and-reset": "Submit and reset",
"button-text-try-again": "Try again",
"button-text-update": "Update",
"button-text-upload-image": "Upload image",
"button-text-verify": "Verify",
Expand Down Expand Up @@ -426,6 +427,7 @@
"error-field-value-between": "{{field}} should be a value between {{lower}} and {{upper}}.",
"error-hide-details": "Hide details",
"error-issue": "Issue: {{issue}}",
"error-loading-exercise": "Error loading exercise.",
"error-loading-organizations": "Error loading organizations.",
"error-message": "Message: {{message}}",
"error-min-length": "{{field}} must be at least {{count}} characters.",
Expand Down
14 changes: 7 additions & 7 deletions shared-module/packages/common/src/locales/en/shared-module.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,23 +35,23 @@
"dialog-title-prompt": "Enter value",
"download-csv": "Download CSV",
"dropdown-menu": "Dropdown menu",
"dynamic-loading-slow-warning": "Loading a part of the application is taking longer than expected.",
"dynamic-loading-very-slow-warning": "This may be due to network issues. If loading does not finish soon, please reload the page.",
"dynamic-loading-possible-causes-title": "If this keeps spinning, possible causes include:",
"dynamic-loading-cause-client-boundary": "A Server/Client boundary issue (for example, a missing \"use client\" on a parent client component).",
"dynamic-loading-cause-export-mismatch": "The dynamically imported module doesn’t export what the loader expects (default vs named export mismatch).",
"dynamic-loading-cause-hydration-script": "The chunk downloaded but the client runtime did not finish running or hydrating (for example due to blocked scripts, extensions, or a strict Content Security Policy).",
"dynamic-loading-cause-runtime-error": "The component throws during initialization or mount (sometimes only visible in production logs).",
"dynamic-loading-cause-suspense-forever": "The component suspends forever (waiting on a promise or data fetch that never resolves), so the fallback stays on screen.",
"dynamic-loading-cause-hydration-script": "The chunk downloaded but the client runtime did not finish running or hydrating (for example due to blocked scripts, extensions, or a strict Content Security Policy).",
"dynamic-loading-fallback-title": "We were unable to load this part of the page.",
"dynamic-loading-fallback-reason": "Reason: {{reason}}",
"dynamic-loading-detected-import-rejected": "We tried to load this part of the app, but the import failed.",
"dynamic-loading-detected-invalid-export": "We loaded the module, but it did not export a usable React component.",
"dynamic-loading-detected-resolved-no-commit": "We loaded the code, but the component has not finished rendering yet.",
"dynamic-loading-detected-render-error": "The component code loaded, but an error occurred while rendering it.",
"dynamic-loading-detected-resolved-no-commit": "We loaded the code, but the component has not finished rendering yet.",
"dynamic-loading-detected-resolved-no-commit-details": "Import resolved; waiting for first React commit (mount).",
"dynamic-loading-fallback-reason": "Reason: {{reason}}",
"dynamic-loading-fallback-title": "We were unable to load this part of the page.",
"dynamic-loading-possible-causes-title": "If this keeps spinning, possible causes include:",
"dynamic-loading-reload": "Reload page",
"dynamic-loading-retrying": "Retrying to load this part of the app ({{attempt}}/{{max}})…",
"dynamic-loading-slow-warning": "Loading a part of the application is taking longer than expected.",
"dynamic-loading-very-slow-warning": "This may be due to network issues. If loading does not finish soon, please reload the page.",
"editable": "Editable",
"email": "Email",
"email-templates": "Email templates",
Expand Down
Loading