fix(submissions): fix unhandled 404s in assessment/submission routes - #8552
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves UX and correctness for invalid/missing assessment/submission routes by ensuring they redirect to the Not Found page (instead of rendering blank pages or generic fetch errors), and by adding an index redirect for the /submissions/:id parent route.
Changes:
- Add an index route under
:submissionIdthat redirects/submissions/:idto/submissions/:id/edit. - Introduce
redirectToNotFoundIfMissingto opt-in redirect-to-404 behavior for missing-record 404s. - Apply the helper to submission attempt, assessment show, and submission logs fetch paths to avoid “blank page” / generic error states.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| client/app/routers/course/assessments/submissions.tsx | Adds an index redirect under :submissionId to avoid rendering an empty outlet. |
| client/app/bundles/course/assessment/submission/actions/logs.ts | Redirects missing submission-log requests to Not Found when backend returns 404. |
| client/app/bundles/course/assessment/submission/actions/index.js | Redirects missing submission attempts to Not Found on 404 instead of leaving an empty page state. |
| client/app/bundles/course/assessment/pages/AssessmentShow/index.tsx | Redirects missing assessments to Not Found rather than showing Preload’s generic fetch error UI. |
| client/app/api/ErrorHandling.ts | Adds an opt-in helper to redirect on missing-record 404 responses. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Two dead ends in the submission and assessment routes:
/submissions/:idrendered a blank page. The:submissionIdroute has children but no elementof its own. React Router still creates a match branch for a parent route, so an exact
/submissions/5matched it and rendered a default<Outlet />with nothing inside — it never fellthrough to the not-found route.
A submission or assessment ID that doesn't exist showed no 404. The backend returns 404, but the
frontend swallowed it:
submissions/:id/edit)fetchSubmissionisLoadingoff, leaving the page to render against an empty storeassessments/:id)fetchAssessmentviaPreloadsubmissions/:id/logs)fetchLogsviaPreloadThe existing response interceptor only redirects on the 404 whose body says "component not found"
(a disabled course component). A missing record raises
ActiveRecord::RecordNotFound, whose responsedoesn't match, so nothing redirected.
Change
An index route under
:submissionIdredirects toedit, matching the pattern already used formarketplace/listings.A
redirectToNotFoundIfMissinghelper inapi/ErrorHandling.tsredirects to the not-found page on a404 and returns whether it did, so callers can skip their own error handling. It is applied at the
three fetches above.
Note this covers more than a typo'd ID. Resources are loaded through their parent —
through: :coursein
Course::Assessment::Controller,through: :assessmentinCourse::Assessment::Submission::Controller— so CanCanCan scopes the find to@course.assessments/@assessment.submissions. An assessment that exists in another courseraises
RecordNotFoundbefore authorization ever runs, and so 404s rather than 403s. That is theright split, and it means the not-found path leaks no existence information across courses.
Why the helper is opt-in
The obvious alternative is to redirect on every 404 inside
redirectIfMatchesErrorIn, in theresponse interceptor. That would break callers that treat a 404 as a normal outcome —
sendOldSessionsin the video submission actions uses it to detect a session the backend has already dropped. Keeping
it opt-in also leaves the existing local handling in
RubricPlaygroundPagecoherent.Preload's ownonErrorDowas not usable for the twoPreloadpages: it receiveserror.response?.data, not the status, so the check has to sit at the call site.