fix: essay hand-in linking, URL-safe code encodings, multi-device e2e tests#132
Merged
Conversation
…overage - Persist onboarding role across OTP re-login so returning students/teachers aren't re-prompted to choose a role. - Use URL-safe base64 (new src/utils/urlSafeBase64.ts) for rubric preview share links, essay share codes, essay submission codes, and student share codes, fixing links that broke when the encoded payload contained '+', '/' or '='. - Add .seb config download for students on essay assignments requiring Safe Exam Browser (src/utils/sebConfig.ts). - Show a handed-in self-assessment as an attachment in GradeStudent. - Fix EssayImportModal's "From database" tab being permanently hidden because GradeStudent never passes a teacherKey; fetching submissions only needs rubricId + studentId. Imported essays are now linked via rubricId/studentId and visible in both normal and comparative grading views. - Update DocsPage to describe the essay submission/import flow. - Add e2e coverage: 19-essay-import.spec.ts (offline paste-code import) and 20-essay-import-db.spec.ts (Supabase-backed "From database" import, scoped to the correct rubric/student). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Addresses the PR coverage report flagging src/utils/sebConfig.ts at 0%. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds essay import workflows (from submission codes or Supabase), Safe Exam Browser (SEB) configuration generation and download, centralized URL-safe base64 encoding for share codes, student self-assessment rendering in the grading view, and supporting documentation and localization across multiple languages. ChangesEssay Workflows: Import, SEB, and Self-Assessment
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
e2e/pages/StudentEssayPage.ts (1)
134-145:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the URL-safe encoder for
buildEssayCodetoo.This file now builds submission codes with URL-safe base64, but
buildEssayCode()still emits plain base64 into/#/essay/${code}. That leaves the E2E route helper vulnerable to the same/,+, and=path issues this PR is fixing, so the tests no longer model production behavior on assignment links.Suggested change
export function buildEssayCode(overrides: TestEssayAssignment = {}): string { const assignment = { rubricId: 'test-rubric-id', studentId: 'test-student-id', teacherKey: 'test-teacher-key', title: 'E2E Test Essay', readOnlyAfterSubmit: true, createdAt: new Date().toISOString(), ...overrides, }; - return btoa(encodeURIComponent(JSON.stringify(assignment))); + return encodeUrlSafeBase64(JSON.stringify(assignment)); }Also applies to: 149-153
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/pages/StudentEssayPage.ts` around lines 134 - 145, buildEssayCode currently uses btoa(encodeURIComponent(...)) and emits plain base64 into /#/essay/${code}, which can produce /, +, and = characters; change buildEssayCode to use the URL-safe base64 encoder used elsewhere in the codebase (the same function used for submission codes) so the returned code is base64url-safe. Update the buildEssayCode implementation (and the similar helper around lines 149-153) to call the project's URL-safe encoder (e.g., urlSafeBase64Encode or existing encodeBase64Url function) on the JSON stringified assignment instead of btoa, so links like /#/essay/${code} match production behavior.src/components/__tests__/EssayImportModal.test.tsx (1)
313-326:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore
fetchin a guaranteed cleanup path.These tests only call
vi.unstubAllGlobals()on the happy path. If an assertion fails before the last line, the mockedfetchleaks into the remaining cases and makes the suite order-dependent.Also applies to: 337-348, 370-383
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/__tests__/EssayImportModal.test.tsx` around lines 313 - 326, The test stubs the global fetch via vi.stubGlobal('fetch', fetchMock) but only calls vi.unstubAllGlobals() on the happy path, which can leak the mock if assertions throw; restore globals in a guaranteed cleanup path by moving vi.unstubAllGlobals() into a shared afterEach hook (e.g., add afterEach(() => vi.unstubAllGlobals())) or wrap the test body in try/finally and call vi.unstubAllGlobals() in finally; apply the same fix for the other tests that stub fetch (the tests around the "imports a submission..." cases that create fetchMock and call vi.stubGlobal).src/components/Essay/EssayImportModal.tsx (2)
30-40:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftUpdate the fetch contract to pass rubric/student scope instead of an empty teacher key.
The modal now treats
teacherKeyas optional, butonFetchSubmissionsstill only accepts ateacherKey: stringand Line 80 calls it with''when the key is missing. That leaves the fetcher without therubricId/studentIdscope this PR says should drive DB imports, so the callback can only guess or fall back to broader queries.Proposed direction
interface Props { rubricId: string; studentId: string; studentName: string; teacherKey?: string; onImport: (attachment: Omit<Attachment, 'id' | 'addedAt'>) => void; onClose: () => void; - onFetchSubmissions?: (teacherKey: string) => Promise<DbSubmission[]>; + onFetchSubmissions?: (params: { + rubricId: string; + studentId: string; + teacherKey?: string; + }) => Promise<DbSubmission[]>; onGetSignedUrl?: (storagePath: string) => Promise<string | null>; onDeleteSubmission?: (id: string, storagePath: string) => Promise<{ success: boolean; error?: string }>; } @@ - const rows = await onFetchSubmissions(teacherKey ?? ''); + const rows = await onFetchSubmissions({ rubricId, studentId, teacherKey });Also applies to: 75-87
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Essay/EssayImportModal.tsx` around lines 30 - 40, Props currently types onFetchSubmissions as onFetchSubmissions?: (teacherKey: string) => Promise<DbSubmission[]> and the component calls it with '' when teacherKey is missing, which loses rubric/student scope; change the contract to accept the scope (e.g. onFetchSubmissions?: (scope: { rubricId: string; studentId: string; teacherKey?: string }) => Promise<DbSubmission[]>) and update the Props interface and the call site inside the EssayImportModal component to pass { rubricId, studentId, teacherKey } (or omit teacherKey when undefined); adjust any consumers accordingly to use rubricId/studentId for DB queries instead of relying on an empty teacherKey.
139-145:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCheck
res.okbefore importing the fetched HTML.A signed URL fetch that returns 403/404 still reaches
res.text(), so the modal will import the error page as the essay attachment instead of surfacingdbError.Suggested fix
const res = await fetch(url); + if (!res.ok) { + throw new Error(`Download failed: ${res.status}`); + } const html = await res.text();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Essay/EssayImportModal.tsx` around lines 139 - 145, The fetch result is not checked so a 403/404 error page can be imported; update the block in EssayImportModal where you call fetch(url) to check res.ok before calling res.text() and before constructing the dataUrl/filename; if !res.ok, throw or return an error containing res.status/res.statusText (or call the component's error handler) so the dbError is surfaced instead of importing the error HTML; ensure this uses the existing variables res, url, html, and onImport (i.e., call res.text() only after res.ok, and only call onImport when the response is successful).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/specs/20-essay-import-db.spec.ts`:
- Around line 66-127: Test seeds use fixed IDs (db-essay-*) which cause
duplicate-key conflicts when tests run in parallel or retry; update the setup to
generate per-run unique IDs for buildClass/buildRubric/buildStudent and the
local variables assignmentId/submissionId/otherAssignmentId/otherSubmissionId
and storagePath by appending or replacing with a run-unique suffix (e.g.,
timestamp/UUID/test-run id). Change all insertRow and uploadEssayHtml calls to
use those generated IDs so the same symbols (buildClass, buildRubric,
buildStudent, assignmentId, otherAssignmentId, submissionId, otherSubmissionId,
storagePath, otherStoragePath) refer to unique-per-run values.
In `@src/components/Essay/EssayImportModal.tsx`:
- Line 83: The modal is using hardcoded English strings (e.g., the setDbError
message) instead of using the i18n translator; update EssayImportModal to import
and call useTranslation() and replace all user-facing literals in this component
(including error messages, button labels, headings, paragraphs and any inline
text currently hardcoded) with t('locale.key') calls tying each string to an
appropriate locale key; ensure you create/choose unique locale keys for each
string, pass any variables via t's interpolation where needed, and remove direct
string literals so every UI string in EssayImportModal is fetched through t(...)
(e.g., replace the setDbError('Failed to load submissions...') with
setDbError(t('essayImport.failedToLoadSubmissions')) and similarly for other
messages in this component).
In `@src/pages/DocsPage.tsx`:
- Around line 720-721: Replace the hardcoded SEB user-facing strings in the
DocsPage component with translated keys using the useTranslation hook (e.g.,
call const { t } = useTranslation() at the top of DocsPage) and swap the inline
text for t('docs.seb.enable'), t('docs.seb.downloadConfig'), etc.; add
corresponding keys and English values to src/locales/en.json (suggested keys:
"docs.seb.enable", "docs.seb.downloadConfig" and any additional lines referenced
around the other block at 747-759) so all strings render via t(...) instead of
hardcoded English.
In `@src/pages/GradeStudent.tsx`:
- Around line 1374-1382: Remove hardcoded English fallbacks from the
GradeStudent JSX by calling the translation function without inline English
defaults and ensuring the corresponding keys exist in the English locale;
specifically update the t(...) calls using keys
gradeStudent.self_assessment_title, gradeStudent.self_assessed_on, and
gradeStudent.student_reflection in GradeStudent.tsx to not pass literal English
fallback strings (remove the second-argument defaultValue strings like 'Student
self-assessment' and 'Self-assessed {{date}}' and the 'Student reflection'
literal), and if those keys are missing add them to src/locales/en.json so
t(key) resolves through the locale file.
- Around line 414-415: The visibility boolean hasSelfAssessment currently only
checks selfAssessmentEntries and sr.selfAssessmentReflection; include
sr.selfAssessedAt so saved-but-empty self-assessments are treated as present.
Update the expression that defines hasSelfAssessment (used in GradeStudent.tsx)
to also consider a truthy sr.selfAssessedAt (e.g., OR sr.selfAssessedAt) so any
saved self-assessment, even with no entries or reflection, is shown.
- Line 1380: The self-assessment date is being formatted with
toLocaleDateString() using the environment/browser locale; update
GradeStudent.tsx to use the app locale from i18n by calling useTranslation()
(add const { t, i18n } = useTranslation() near the top where hooks are
initialized) and change the formatting of sr.selfAssessedAt to pass the app
locale (e.g., use i18n.language) into toLocaleDateString so the date renders
using the app-selected locale.
In `@src/utils/essayShareCode.ts`:
- Around line 13-16: The encoder currently returns a bare teacherKey for
Supabase-backed assignments (in encodeEssayAssignment), while
decodeEssayAssignment only parses JSON payloads causing non-round-trip behavior;
make encodeEssayAssignment always emit the full JSON payload (i.e., remove the
special-case that returns safe.teacherKey and instead return
encodeUrlSafeBase64(JSON.stringify(safe)) for all inputs) so that
decodeEssayAssignment can reliably decode every encoded assignment, or
alternatively split the short-code path into a separate function and update
callers to use the appropriate API; update references to encodeEssayAssignment
and decodeEssayAssignment accordingly.
In `@src/utils/sebConfig.ts`:
- Around line 13-29: The quitURL and URL filter expression drop
window.location.pathname causing wrong base path on non-root deployments; update
the quitURL string and the URLFilterRules expression to include
window.location.pathname (e.g. build quitURL as
`${window.location.origin}${window.location.pathname}`#/seb-done`` and change the
expression from `${window.location.origin}.*` to
`${window.location.origin}${window.location.pathname}.*`) so SEB preserves the
app base path; modify the entries named quitURL and URLFilterRules (expression)
in src/utils/sebConfig.ts accordingly.
- Around line 39-42: The filename sanitization in downloadSebConfig only
replaces whitespace and leaves unsafe characters (e.g. / \ : ? " *) that can
break downloads; update the sanitization of fileNameBase in downloadSebConfig
to: remove or replace all characters except a safe whitelist (letters, numbers,
dash, underscore), collapse consecutive separators, trim leading/trailing
separators, lowercase the result, and fall back to a safe default if the result
is empty so saveAs is always given a stable, filesystem-safe name.
- Around line 4-35: The buildSebConfigXml function inserts startUrl and
window.location.origin into the plist without escaping XML special characters,
which can produce malformed XML if values contain &, <, >, " or '. Add an
XML-escaping helper (e.g., escapeXml) and call it for startUrl and for every
usage of window.location.origin (including the quitURL and URLFilterRules
expression) so all interpolated URLs are sanitized before being inserted into
the returned string; update references inside buildSebConfigXml to use the
escaped values.
---
Outside diff comments:
In `@e2e/pages/StudentEssayPage.ts`:
- Around line 134-145: buildEssayCode currently uses
btoa(encodeURIComponent(...)) and emits plain base64 into /#/essay/${code},
which can produce /, +, and = characters; change buildEssayCode to use the
URL-safe base64 encoder used elsewhere in the codebase (the same function used
for submission codes) so the returned code is base64url-safe. Update the
buildEssayCode implementation (and the similar helper around lines 149-153) to
call the project's URL-safe encoder (e.g., urlSafeBase64Encode or existing
encodeBase64Url function) on the JSON stringified assignment instead of btoa, so
links like /#/essay/${code} match production behavior.
In `@src/components/__tests__/EssayImportModal.test.tsx`:
- Around line 313-326: The test stubs the global fetch via
vi.stubGlobal('fetch', fetchMock) but only calls vi.unstubAllGlobals() on the
happy path, which can leak the mock if assertions throw; restore globals in a
guaranteed cleanup path by moving vi.unstubAllGlobals() into a shared afterEach
hook (e.g., add afterEach(() => vi.unstubAllGlobals())) or wrap the test body in
try/finally and call vi.unstubAllGlobals() in finally; apply the same fix for
the other tests that stub fetch (the tests around the "imports a submission..."
cases that create fetchMock and call vi.stubGlobal).
In `@src/components/Essay/EssayImportModal.tsx`:
- Around line 30-40: Props currently types onFetchSubmissions as
onFetchSubmissions?: (teacherKey: string) => Promise<DbSubmission[]> and the
component calls it with '' when teacherKey is missing, which loses
rubric/student scope; change the contract to accept the scope (e.g.
onFetchSubmissions?: (scope: { rubricId: string; studentId: string; teacherKey?:
string }) => Promise<DbSubmission[]>) and update the Props interface and the
call site inside the EssayImportModal component to pass { rubricId, studentId,
teacherKey } (or omit teacherKey when undefined); adjust any consumers
accordingly to use rubricId/studentId for DB queries instead of relying on an
empty teacherKey.
- Around line 139-145: The fetch result is not checked so a 403/404 error page
can be imported; update the block in EssayImportModal where you call fetch(url)
to check res.ok before calling res.text() and before constructing the
dataUrl/filename; if !res.ok, throw or return an error containing
res.status/res.statusText (or call the component's error handler) so the dbError
is surfaced instead of importing the error HTML; ensure this uses the existing
variables res, url, html, and onImport (i.e., call res.text() only after res.ok,
and only call onImport when the response is successful).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cafd098e-7786-4992-afd5-ffcd00a0a309
📒 Files selected for processing (23)
e2e/pages/StudentEssayPage.tse2e/specs/19-essay-import.spec.tse2e/specs/20-essay-import-db.spec.tsplaywright.config.tssrc/components/Essay/EssayAssignmentModal.tsxsrc/components/Essay/EssayImportModal.tsxsrc/components/__tests__/EssayImportModal.test.tsxsrc/locales/de.jsonsrc/locales/en.jsonsrc/locales/es.jsonsrc/locales/fr.jsonsrc/locales/nl.jsonsrc/pages/DocsPage.tsxsrc/pages/GradeStudent.tsxsrc/pages/StudentEssayPage.tsxsrc/services/database/StorageSync.tssrc/utils/essayShareCode.tssrc/utils/essaySubmissionCode.tssrc/utils/rubricImport.tssrc/utils/sebConfig.test.tssrc/utils/sebConfig.tssrc/utils/studentShareCode.tssrc/utils/urlSafeBase64.ts
| const rows = await onFetchSubmissions(teacherKey ?? ''); | ||
| setSubmissions(rows); | ||
| } catch { | ||
| setDbError('Failed to load submissions. Make sure you are connected to the database.'); |
Contributor
There was a problem hiding this comment.
Move the modal copy through t(...) instead of hardcoded English.
This component still renders most of its user-facing text as literals, even though the repo requires all UI strings to come from useTranslation and locale keys. That leaves the new import flow partially untranslated and out of sync with the locale files.
Also applies to: 95-107, 135-145, 164-165, 194-260, 320-324, 337-381, 407-475, 493-533
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/Essay/EssayImportModal.tsx` at line 83, The modal is using
hardcoded English strings (e.g., the setDbError message) instead of using the
i18n translator; update EssayImportModal to import and call useTranslation() and
replace all user-facing literals in this component (including error messages,
button labels, headings, paragraphs and any inline text currently hardcoded)
with t('locale.key') calls tying each string to an appropriate locale key;
ensure you create/choose unique locale keys for each string, pass any variables
via t's interpolation where needed, and remove direct string literals so every
UI string in EssayImportModal is fetched through t(...) (e.g., replace the
setDbError('Failed to load submissions...') with
setDbError(t('essayImport.failedToLoadSubmissions')) and similarly for other
messages in this component).
Source: Coding guidelines
Comment on lines
+720
to
721
| 'Enable "Require Safe Exam Browser (SEB)" to lock the assignment to the SEB app — download the .seb config from the assignment modal to distribute to students.', | ||
| ]} |
Contributor
There was a problem hiding this comment.
Localize the newly added SEB docs copy instead of hardcoding English text.
These newly introduced user-facing strings are hardcoded in JSX, so they won’t translate with locale changes.
🌐 Suggested patch
function EssaysTab() {
+ const { t } = useTranslation();
return (
<div>
<FeatureSection icon={FileText} title="Essay assignments" color="`#6366f1`">
@@
<FeatureList
items={[
@@
- 'Enable "Require Safe Exam Browser (SEB)" to lock the assignment to the SEB app — download the .seb config from the assignment modal to distribute to students.',
+ t('docs.essays.require_seb_help'),
]}
/>
@@
- <p style={{ color: 'var(--text-muted)', fontSize: '0.9rem', lineHeight: 1.6, marginBottom: 16 }}>
- Students visit the essay URL or enter the submission code in the Student Portal. After an optional
- email verification gate, they write and submit. Drafts are auto-saved to localStorage every few
- seconds. After submission, teachers open the linked rubric's grading view and click{' '}
- <strong>Import Essay</strong> to pull the submission from the database (or paste the student's
- submission code if no database is connected) — the essay text is then added as an attachment,
- visible in both the normal and comparative grading views.
- </p>
- <p style={{ color: 'var(--text-muted)', fontSize: '0.9rem', lineHeight: 1.6 }}>
- If an assignment requires Safe Exam Browser and the student opens the link outside of SEB,
- submission is blocked and a banner offers a <strong>Download .seb config</strong> button so the
- student can get the right config file and reopen the assignment inside SEB.
- </p>
+ <p style={{ color: 'var(--text-muted)', fontSize: '0.9rem', lineHeight: 1.6, marginBottom: 16 }}>
+ {t('docs.essays.submission_flow_main_prefix')}
+ <strong>{t('docs.essays.import_essay_cta')}</strong>
+ {t('docs.essays.submission_flow_main_suffix')}
+ </p>
+ <p style={{ color: 'var(--text-muted)', fontSize: '0.9rem', lineHeight: 1.6 }}>
+ {t('docs.essays.submission_flow_seb_prefix')}
+ <strong>{t('docs.essays.download_seb_cta')}</strong>
+ {t('docs.essays.submission_flow_seb_suffix')}
+ </p>
</FeatureSection>
</div>
);
}As per coding guidelines, “All user-visible strings must use the useTranslation hook and a key from src/locales/en.json. Never hardcode English text in JSX.”
Also applies to: 747-759
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pages/DocsPage.tsx` around lines 720 - 721, Replace the hardcoded SEB
user-facing strings in the DocsPage component with translated keys using the
useTranslation hook (e.g., call const { t } = useTranslation() at the top of
DocsPage) and swap the inline text for t('docs.seb.enable'),
t('docs.seb.downloadConfig'), etc.; add corresponding keys and English values to
src/locales/en.json (suggested keys: "docs.seb.enable",
"docs.seb.downloadConfig" and any additional lines referenced around the other
block at 747-759) so all strings render via t(...) instead of hardcoded English.
Source: Coding guidelines
Comment on lines
13
to
+16
| if (safe.supabaseUrl) { | ||
| return safe.teacherKey; | ||
| } | ||
| return btoa(encodeURIComponent(JSON.stringify(safe))); | ||
| return encodeUrlSafeBase64(JSON.stringify(safe)); |
Contributor
There was a problem hiding this comment.
encodeEssayAssignment and decodeEssayAssignment no longer round-trip Supabase assignments.
Lines 13-15 still emit a bare teacherKey, but Lines 24-26 only decode JSON payloads. Any caller that treats these as a paired encode/decode API will now get null for Supabase-backed assignments. Either keep this helper self-invertible or split the short-code path into a separate API so callers cannot mix the two formats accidentally.
Also applies to: 22-25
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/essayShareCode.ts` around lines 13 - 16, The encoder currently
returns a bare teacherKey for Supabase-backed assignments (in
encodeEssayAssignment), while decodeEssayAssignment only parses JSON payloads
causing non-round-trip behavior; make encodeEssayAssignment always emit the full
JSON payload (i.e., remove the special-case that returns safe.teacherKey and
instead return encodeUrlSafeBase64(JSON.stringify(safe)) for all inputs) so that
decodeEssayAssignment can reliably decode every encoded assignment, or
alternatively split the short-code path into a separate function and update
callers to use the appropriate API; update references to encodeEssayAssignment
and decodeEssayAssignment accordingly.
- sebConfig: XML-escape startUrl/origin-derived strings, preserve pathname in quitURL and the URL filter (needed for non-root base paths), and fully slugify the .seb file name with an "essay" fallback. - EssayImportModal: check res.ok before reading the downloaded essay body. - GradeStudent: include selfAssessedAt (and trim reflection) when deciding whether to show the self-assessment block, drop hardcoded i18n defaultValue fallbacks, and format the self-assessed date with i18n.language. - EssayImportModal.test: restore stubbed fetch via afterEach so a failing assertion can't leak the mock into later tests; add coverage for the new res.ok check. - 20-essay-import-db.spec: give all seeded Supabase rows a per-run unique suffix so retries/parallel runs don't collide on primary keys.
Fixes the format:check failure in CI caused by the new res.ok test.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/utils/sebConfig.test.ts (1)
28-52:⚠️ Potential issue | 🟡 MinorDecouple
saveAscall-index assertions indownloadSebConfigtestsLine 42 and Line 49 rely on
vi.mocked(saveAs).mock.calls[1]/[2]without clearing the shared mock betweenitblocks, making the tests order-dependent and brittle.Proposed fix
import { describe, it, expect, vi } from 'vitest'; +import { beforeEach } from 'vitest'; @@ describe('downloadSebConfig', () => { + beforeEach(() => { + vi.mocked(saveAs).mockClear(); + }); + it('saves a .seb file with a slugified, lowercased file name', () => { downloadSebConfig('https://example.com/#/essay/abc123', 'Essay For Alice'); @@ it('strips diacritics and punctuation from the file name', () => { downloadSebConfig('https://example.com/#/essay/abc123', "Café — Élève's Essay!"); - const [, filename] = vi.mocked(saveAs).mock.calls[1]; + const [, filename] = vi.mocked(saveAs).mock.calls[0]; expect(filename).toBe('cafe-eleve-s-essay.seb'); }); @@ it('falls back to "essay" when the name has no usable characters', () => { downloadSebConfig('https://example.com/#/essay/abc123', '!!!'); - const [, filename] = vi.mocked(saveAs).mock.calls[2]; + const [, filename] = vi.mocked(saveAs).mock.calls[0]; expect(filename).toBe('essay.seb'); }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/sebConfig.test.ts` around lines 28 - 52, Tests for downloadSebConfig rely on positional indices into vi.mocked(saveAs).mock.calls across multiple it blocks, making them order-dependent; fix by isolating the mock between tests—add a beforeEach/afterEach that clears or resets the saveAs mock (e.g., call vi.mocked(saveAs).mockClear() or vi.clearAllMocks() before each it) so each test can assert calls[0] independently, or alternatively change the assertions to read the last call (e.g., vi.mocked(saveAs).mock.calls.at(-1)) when referencing downloadSebConfig results.src/components/Essay/EssayImportModal.tsx (2)
106-107:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFormat these dates with the active UI locale.
These
toLocaleDateString()/toLocaleString()calls use the browser default, so a user who switches the app language can still get filenames and timestamps in a different locale. Since this component already readsuseTranslation(), passi18n.languagethrough the date formatters.💡 Minimal fix
- const { t } = useTranslation(); + const { t, i18n } = useTranslation(); ... - const dateStr = new Date(submission.submittedAt).toLocaleDateString(); + const dateStr = new Date(submission.submittedAt).toLocaleDateString(i18n.language); ... - const dateStr = new Date(sub.submittedAt).toLocaleDateString(); + const dateStr = new Date(sub.submittedAt).toLocaleDateString(i18n.language); ... - {meta.wordCount} words · submitted {new Date(meta.submittedAt).toLocaleString()} + {meta.wordCount} words · submitted {new Date(meta.submittedAt).toLocaleString(i18n.language)} ... - {sub.wordCount} words · {new Date(sub.submittedAt).toLocaleString()} + {sub.wordCount} words · {new Date(sub.submittedAt).toLocaleString(i18n.language)}Also applies to: 142-145, 218-219, 420-422
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Essay/EssayImportModal.tsx` around lines 106 - 107, The date/time formatting calls in EssayImportModal (e.g., the filename creation where dateStr = new Date(submission.submittedAt).toLocaleDateString() and other toLocaleDateString()/toLocaleString() uses) should be passed the active UI locale from useTranslation; update the component to read const { i18n } = useTranslation() and call new Date(...).toLocaleDateString(i18n.language) or toLocaleString(i18n.language) (optionally keeping existing options objects) for all occurrences (including the filename construction, the timestamp displays around lines 142-145, 218-219, and 420-422) so formatting respects the app language. Ensure you only change the date formatting calls to include i18n.language and do not alter other logic.
162-174:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle rejected deletes the same way you handle
{ success: false }.
onDeleteSubmissionis awaited outside atry/finally, so a thrown network/DB error leavesdeletingIdstuck and skips the existing error UI altogether. Wrap this call so the busy state is always cleared and rejected promises surface throughdbError.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/Essay/EssayImportModal.tsx` around lines 162 - 174, The handleDelete callback currently awaits onDeleteSubmission outside of error handling, so a thrown/rejected promise can leave deletingId set and skip the error UI; modify handleDelete (the async function passed to useCallback) to wrap the await in try/catch/finally: after setDeletingId(sub.id) call onDeleteSubmission inside try, handle the { success: false } path by calling setDbError(result.error) as before, catch any thrown errors and call setDbError(err?.message ?? String(err)), and always call setDeletingId(null) in finally so the busy state is cleared.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/__tests__/EssayImportModal.test.tsx`:
- Around line 215-217: The test teardown currently calls vi.unstubAllGlobals()
but doesn't restore spies, causing a leaked spy on window.confirm if a test
fails before calling mockRestore(); add a call to vi.restoreAllMocks() in the
same afterEach teardown so all spies (e.g. the vi.spyOn(window, 'confirm') uses)
are restored automatically rather than relying on individual mockRestore()
calls.
---
Outside diff comments:
In `@src/components/Essay/EssayImportModal.tsx`:
- Around line 106-107: The date/time formatting calls in EssayImportModal (e.g.,
the filename creation where dateStr = new
Date(submission.submittedAt).toLocaleDateString() and other
toLocaleDateString()/toLocaleString() uses) should be passed the active UI
locale from useTranslation; update the component to read const { i18n } =
useTranslation() and call new Date(...).toLocaleDateString(i18n.language) or
toLocaleString(i18n.language) (optionally keeping existing options objects) for
all occurrences (including the filename construction, the timestamp displays
around lines 142-145, 218-219, and 420-422) so formatting respects the app
language. Ensure you only change the date formatting calls to include
i18n.language and do not alter other logic.
- Around line 162-174: The handleDelete callback currently awaits
onDeleteSubmission outside of error handling, so a thrown/rejected promise can
leave deletingId set and skip the error UI; modify handleDelete (the async
function passed to useCallback) to wrap the await in try/catch/finally: after
setDeletingId(sub.id) call onDeleteSubmission inside try, handle the { success:
false } path by calling setDbError(result.error) as before, catch any thrown
errors and call setDbError(err?.message ?? String(err)), and always call
setDeletingId(null) in finally so the busy state is cleared.
In `@src/utils/sebConfig.test.ts`:
- Around line 28-52: Tests for downloadSebConfig rely on positional indices into
vi.mocked(saveAs).mock.calls across multiple it blocks, making them
order-dependent; fix by isolating the mock between tests—add a
beforeEach/afterEach that clears or resets the saveAs mock (e.g., call
vi.mocked(saveAs).mockClear() or vi.clearAllMocks() before each it) so each test
can assert calls[0] independently, or alternatively change the assertions to
read the last call (e.g., vi.mocked(saveAs).mock.calls.at(-1)) when referencing
downloadSebConfig results.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 40f0204a-0828-4226-b12f-e401815597ff
📒 Files selected for processing (6)
e2e/specs/20-essay-import-db.spec.tssrc/components/Essay/EssayImportModal.tsxsrc/components/__tests__/EssayImportModal.test.tsxsrc/pages/GradeStudent.tsxsrc/utils/sebConfig.test.tssrc/utils/sebConfig.ts
- EssayImportModal: format submission dates/timestamps with i18n.language instead of the browser default locale. - EssayImportModal.test: restore spies (e.g. window.confirm) in the database-tab afterEach, not just stubbed globals. - sebConfig.test: clear the saveAs mock between downloadSebConfig tests so call-index assertions aren't order-dependent.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/__tests__/EssayImportModal.test.tsx (1)
215-217:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPast review comment remains unaddressed: restore spies in teardown.
The shared teardown still only calls
vi.unstubAllGlobals()but doesn't restore spies. Multiple tests in this describe block usevi.spyOn(window, 'confirm')and rely on manualmockRestore()calls (lines 361, 414, 425, 438). If a test fails or throws before reaching itsmockRestore(), the spy leaks into subsequent tests.🔧 Add spy restoration to teardown
afterEach(() => { + vi.restoreAllMocks(); vi.unstubAllGlobals(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/__tests__/EssayImportModal.test.tsx` around lines 215 - 217, The afterEach teardown only calls vi.unstubAllGlobals() and must also restore spies to avoid leaks from window.confirm spies; update the afterEach in EssayImportModal.test.tsx to call vi.restoreAllMocks() (or vi.resetAllMocks() if you prefer resetting behavior) in addition to vi.unstubAllGlobals() so that any vi.spyOn(window, 'confirm') used in tests is automatically restored even if a test fails, and you can then remove redundant manual mockRestore() calls if desired.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/components/__tests__/EssayImportModal.test.tsx`:
- Around line 215-217: The afterEach teardown only calls vi.unstubAllGlobals()
and must also restore spies to avoid leaks from window.confirm spies; update the
afterEach in EssayImportModal.test.tsx to call vi.restoreAllMocks() (or
vi.resetAllMocks() if you prefer resetting behavior) in addition to
vi.unstubAllGlobals() so that any vi.spyOn(window, 'confirm') used in tests is
automatically restored even if a test fails, and you can then remove redundant
manual mockRestore() calls if desired.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e012b65b-e5eb-4b62-afa5-4a097fca7d74
📒 Files selected for processing (1)
src/components/__tests__/EssayImportModal.test.tsx
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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This branch bundles two areas of work:
Essay & sharing fixes
src/utils/urlSafeBase64.ts) — fixes links that broke when the encoded payload contained+,/, or=..sebconfig download for students on essay assignments that require Safe Exam Browser (src/utils/sebConfig.ts).EssayImportModal's "From database" tab being permanently hidden because GradeStudent never passed ateacherKey— fetching submissions only needsrubricId+studentId. Imported essays are now linked viarubricId/studentIdand visible in both normal and comparative grading views.DocsPageto describe the essay submission/import flow.Multi-device / essay e2e coverage (Phase 2.1)
secondSupabasePageduplicate-user bug and LWW test expectations, plus closing its context on setup failure.19-essay-import.spec.ts— offline paste-code essay import flow.20-essay-import-db.spec.ts— Supabase-backed "From database" import, scoped to the correct rubric/student (requiresnpm run db:start && npm run e2e:supabase, not yet run against a live stack).Test plan
npm run typecheckpassesnpm run lint— 0 errors (pre-existing warnings only)npx vitest run— 1283/1283 tests passe2e/specs/19-essay-import.spec.tspasses on chromium, firefox, webkitnpm run db:start && npm run e2e:supabase— run20-essay-import-db.spec.tsand the multi-device sync specs against a local Supabase stack🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Localization
Tests