Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces a server-time endpoint, refactors exam pages into a composable Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client Component
participant Storage as Component State
participant Server as Backend Server
participant Select as RTT Selector
Client->>Storage: Initialize sampling (SAMPLE_COUNT=7)
loop Sample Loop (up to 7 times)
Client->>Server: GET /api/v0/main-frontend/time (with timestamp)
Server-->>Client: RFC3339 time + RTT measured
Client->>Storage: Store sample with RTT
end
Client->>Select: Select sample with lowest RTT
Select-->>Client: Best sample (finalOffsetMs)
Client->>Client: Calculate offset magnitude
alt Offset ≥ 2 minutes
Client->>Client: Determine severity (mild vs severe)
Client->>Client: Format duration & timezone info
Client->>Client: Render warning with details
else Offset < 2 minutes
Client->>Client: Return null (no warning)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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. 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: 1
🧹 Nitpick comments (4)
services/main-frontend/src/components/course-material/exams/ExamClockSkewWarning.tsx (2)
183-212:cancelledis only checked after all samples complete — remaining requests fire after unmount.The
cancelledflag preventssetEstimatefrom being called, but it is checked outside the loop. If the component unmounts mid-sampling, all remainingawait sampleClockOffset()calls still execute. Adding a guard inside the loop stops unnecessary requests early.♻️ Proposed fix
for (let i = 0; i < SAMPLE_COUNT; i += 1) { + if (cancelled) { + break + } try { const sample = await sampleClockOffset() if (sample) { samples.push(sample) } } catch { // Ignore individual sample failures and use the rest. } }Pair this with an
AbortControllerpassed tofetchCurrentServerTime(see thebackend.tscomment) to also cancel the in-flight HTTP request whencancelledbecomes true.🤖 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/exams/ExamClockSkewWarning.tsx` around lines 183 - 212, The sampling loop in runSampling currently only checks the cancelled flag after all SAMPLE_COUNT calls, so unmounting won't stop remaining sampleClockOffset() calls; move a cancelled check inside the for loop (before each await sampleClockOffset()) to break early, and propagate an AbortController to sampleClockOffset() / ultimately fetchCurrentServerTime so in-flight HTTP requests are aborted when cancelled; after the loop keep the existing guard before calling setEstimate(chooseEstimateFromSamples(samples)).
183-212: Coding guideline: server requests should go throughuseQueryor a hook wrapping it.As per coding guidelines, components that send server requests should use
useQuery,useToastMutation, or hooks wrapping those. The sequential multi-sample measurement pattern here doesn't map cleanly touseQuery(the repeated no-cache requests, custom aggregation, and silent error handling all require custom logic). The most guideline-compliant approach would be to extract the entire sampling logic into auseClockSkewEstimatecustom hook, keeping this component as a pure renderer. As per coding guidelines: "If component sends requests to server, use either useQuery, or useToastMutation hooks, or use hooks wrapping those hooks."🤖 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/exams/ExamClockSkewWarning.tsx` around lines 183 - 212, The component currently performs server requests inside useEffect via the runSampling async function calling sampleClockOffset and then setEstimate with chooseEstimateFromSamples; extract this entire sampling flow into a new custom hook (e.g., useClockSkewEstimate) that encapsulates the SAMPLE_COUNT loop, no-cache/retry behavior, silent error handling, aggregation logic, and returns the final estimate (and loading/state) so the ExamClockSkewWarning component becomes a pure renderer that only calls useClockSkewEstimate() instead of performing requests directly in useEffect.services/main-frontend/src/services/course-material/backend.ts (2)
487-497: Consider accepting anAbortSignalto support request cancellation.
ExamClockSkewWarningfires 7 sequential requests on mount but cannot abort in-flight ones on unmount — only a closure flag guards post-completion state updates. Passing anAbortSignalwould enable proper HTTP-level cancellation.♻️ Proposed signature change
-export const fetchCurrentServerTime = async (): Promise<string> => { - const response = await axios.get("/api/v0/main-frontend/time", { - responseType: "json", - headers: { - "Cache-Control": "no-cache", - Pragma: "no-cache", - }, - }) - - return validateResponse(response, isString) -} +export const fetchCurrentServerTime = async (signal?: AbortSignal): Promise<string> => { + const response = await axios.get("/api/v0/main-frontend/time", { + responseType: "json", + signal, + headers: { + "Cache-Control": "no-cache", + Pragma: "no-cache", + }, + }) + + return validateResponse(response, isString) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/src/services/course-material/backend.ts` around lines 487 - 497, Add an optional AbortSignal parameter to fetchCurrentServerTime (e.g., fetchCurrentServerTime(signal?: AbortSignal)): update the axios.get call to include the signal in the request config so in-flight HTTP requests can be cancelled, and thread the signal through any callers (like ExamClockSkewWarning) so they pass their AbortController.signal; keep the existing response handling via validateResponse(response, isString) unchanged but ensure callers abort on unmount to cancel requests.
487-497:fetchCurrentServerTimeis in the wrong service file.This function calls
/api/v0/main-frontend/timebut lives inservices/course-material/backend.ts. The module boundary is already blurred by the directaxiosimport (bypassingcourseMaterialClient). Placing it in a dedicatedmain-frontendservice module would better reflect the dependency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/src/services/course-material/backend.ts` around lines 487 - 497, The fetchCurrentServerTime function is placed in services/course-material/backend.ts but calls the main-frontend API directly; move it into a new or existing main-frontend service module (e.g., services/main-frontend/backend.ts) and export it from there as fetchCurrentServerTime, remove the direct axios import from the course-material module, and update any callers to import fetchCurrentServerTime from the main-frontend service; if your codebase has a mainFrontend client abstraction (e.g., mainFrontendClient or validateResponse/isString helpers), use that client/validators in the new module so the course-material service no longer depends on the main-frontend API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shared-module/packages/common/src/locales/en/main-frontend.json`:
- Line 460: The current translation key exam-clock-warning-duration returns
grammatically wrong strings for singular/zero values; change the locales to use
i18next plurals (e.g., add keys exam-clock-hours_one / exam-clock-hours_other
and exam-clock-minutes_one / exam-clock-minutes_other) and update the consumer
(the component that calls t('exam-clock-warning-duration')) to assemble the
message by calling t('exam-clock-hours', { count: hours }) and
t('exam-clock-minutes', { count: minutes }) and skip any zero parts (only
include non-zero hour/minute segments) so you get correct singular/plural and no
"0 hours" output.
---
Nitpick comments:
In
`@services/main-frontend/src/components/course-material/exams/ExamClockSkewWarning.tsx`:
- Around line 183-212: The sampling loop in runSampling currently only checks
the cancelled flag after all SAMPLE_COUNT calls, so unmounting won't stop
remaining sampleClockOffset() calls; move a cancelled check inside the for loop
(before each await sampleClockOffset()) to break early, and propagate an
AbortController to sampleClockOffset() / ultimately fetchCurrentServerTime so
in-flight HTTP requests are aborted when cancelled; after the loop keep the
existing guard before calling setEstimate(chooseEstimateFromSamples(samples)).
- Around line 183-212: The component currently performs server requests inside
useEffect via the runSampling async function calling sampleClockOffset and then
setEstimate with chooseEstimateFromSamples; extract this entire sampling flow
into a new custom hook (e.g., useClockSkewEstimate) that encapsulates the
SAMPLE_COUNT loop, no-cache/retry behavior, silent error handling, aggregation
logic, and returns the final estimate (and loading/state) so the
ExamClockSkewWarning component becomes a pure renderer that only calls
useClockSkewEstimate() instead of performing requests directly in useEffect.
In `@services/main-frontend/src/services/course-material/backend.ts`:
- Around line 487-497: Add an optional AbortSignal parameter to
fetchCurrentServerTime (e.g., fetchCurrentServerTime(signal?: AbortSignal)):
update the axios.get call to include the signal in the request config so
in-flight HTTP requests can be cancelled, and thread the signal through any
callers (like ExamClockSkewWarning) so they pass their AbortController.signal;
keep the existing response handling via validateResponse(response, isString)
unchanged but ensure callers abort on unmount to cancel requests.
- Around line 487-497: The fetchCurrentServerTime function is placed in
services/course-material/backend.ts but calls the main-frontend API directly;
move it into a new or existing main-frontend service module (e.g.,
services/main-frontend/backend.ts) and export it from there as
fetchCurrentServerTime, remove the direct axios import from the course-material
module, and update any callers to import fetchCurrentServerTime from the
main-frontend service; if your codebase has a mainFrontend client abstraction
(e.g., mainFrontendClient or validateResponse/isString helpers), use that
client/validators in the new module so the course-material service no longer
depends on the main-frontend API.
shared-module/packages/common/src/locales/en/main-frontend.json
Outdated
Show resolved
Hide resolved
- Adjusted server time calculation to include a 2-hour and 3-minute offset for better accuracy. - Wrapped the clock skew warning component in a Centered layout for improved presentation in exam pages. - Updated localization strings for clarity and consistency in clock skew warnings, including new formats for time differences. - Refactored ExamClockSkewWarning component to streamline time formatting and improve user feedback on clock discrepancies.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
services/headless-lms/server/src/controllers/main_frontend/time.rs (1)
58-59: Test doesn't catch the time-offset bug — assert the returned time is close to now.The test only checks that the body is a valid RFC3339 string. It would have caught the
+2h3mbug if it also verified the parsed timestamp is within a few seconds ofUtc::now().Proposed addition
let body: String = test::read_body_json(resp).await; - assert!(chrono::DateTime::parse_from_rfc3339(&body).is_ok()); + let parsed = chrono::DateTime::parse_from_rfc3339(&body) + .expect("body should be valid RFC3339"); + let diff = (Utc::now() - parsed.with_timezone(&Utc)) + .num_seconds() + .abs(); + assert!(diff < 5, "Server time should be within 5s of actual UTC, but diff was {diff}s");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/headless-lms/server/src/controllers/main_frontend/time.rs` around lines 58 - 59, The test currently only checks RFC3339 formatting; update it to also parse the timestamp and assert it's close to now: use chrono::DateTime::parse_from_rfc3339(&body) to get a FixedOffset DateTime, convert to Utc via .with_timezone(&chrono::Utc), get chrono::Utc::now(), compute the absolute difference with chrono::Duration::seconds (e.g. 5 seconds) and assert the diff is less than that threshold; keep the existing format assertion but add this proximity check to catch time-offset bugs (referencing the variables body and resp and functions parse_from_rfc3339 and Utc::now).services/main-frontend/src/app/org/[organizationSlug]/(course-material)/exams/[id]/EndExamButton.tsx (1)
21-29: Mutation variable wrapping is unnecessarily indirect.
examIdis already in scope; threading it through{ id: string }adds noise.♻️ Suggested simplification
- const endExamMutation = useToastMutation( - ({ id }: { id: string }) => endExamTime(id), - { notify: true, method: "POST" }, - { onSuccess: onEnded }, - ) - - const handleEndExam = () => { - endExamMutation.mutate({ id: examId }) - } + const endExamMutation = useToastMutation( + () => endExamTime(examId), + { notify: true, method: "POST" }, + { onSuccess: onEnded }, + ) + + const handleEndExam = () => { + endExamMutation.mutate() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/src/app/org/`[organizationSlug]/(course-material)/exams/[id]/EndExamButton.tsx around lines 21 - 29, The mutation currently wraps examId in an unnecessary object; simplify by changing the mutation initializer to call endExamTime with the outer-scope examId directly (e.g., pass () => endExamTime(examId) into useToastMutation) and update handleEndExam to call endExamMutation.mutate() with no payload. Update references to endExamMutation, useToastMutation, handleEndExam, and endExamTime accordingly to remove the { id: string } wrapper.services/main-frontend/src/app/org/[organizationSlug]/(course-material)/exams/ExamClockSkewWarning.tsx (3)
72-92: Consolidate per-line eslint-disable comments into a single block disable.Lines 62-70 already demonstrate the cleaner pattern. The per-property disables here add significant visual noise.
♻️ Suggested diff
+/* eslint-disable i18next/no-literal-string */ const formatDateTimeInTimezone = ( timestampMs: number, timeZone: string, language: string, ): string => { return new Intl.DateTimeFormat(language, { timeZone, - // eslint-disable-next-line i18next/no-literal-string year: "numeric", - // eslint-disable-next-line i18next/no-literal-string month: "2-digit", - // eslint-disable-next-line i18next/no-literal-string day: "2-digit", - // eslint-disable-next-line i18next/no-literal-string hour: "2-digit", - // eslint-disable-next-line i18next/no-literal-string minute: "2-digit", - // eslint-disable-next-line i18next/no-literal-string second: "2-digit", }).format(new Date(timestampMs)) } const formatTimeInTimezone = (timestampMs: number, timeZone: string, language: string): string => { return new Intl.DateTimeFormat(language, { timeZone, - // eslint-disable-next-line i18next/no-literal-string hour: "2-digit", - // eslint-disable-next-line i18next/no-literal-string minute: "2-digit", - // eslint-disable-next-line i18next/no-literal-string second: "2-digit", }).format(new Date(timestampMs)) } +/* eslint-enable i18next/no-literal-string */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/src/app/org/`[organizationSlug]/(course-material)/exams/ExamClockSkewWarning.tsx around lines 72 - 92, The object in formatDateTimeInTimezone has repeated per-line eslint-disable-next-line comments; replace them with a single block disable around the entire options object (e.g., add /* eslint-disable i18next/no-literal-string */ before the DateTimeFormat options and re-enable with /* eslint-enable i18next/no-literal-string */ after) so the code is cleaner and matches the pattern used in the nearby lines.
189-218: Seven sequential HTTP requests on mount could noticeably delay the warning on slow connections.Each
sampleClockOffsetcall isawait-ed sequentially, so total latency is ~7×RTT. On a slow mobile connection (e.g. 500 ms RTT), the warning could take 3-4 seconds to appear. Consider running a few samples in parallel (e.g. 3 batches of 2-3) or displaying an initial estimate after the first successful sample and refining it as more arrive.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/src/app/org/`[organizationSlug]/(course-material)/exams/ExamClockSkewWarning.tsx around lines 189 - 218, The current useEffect/runSampling loop awaits SAMPLE_COUNT sequential sampleClockOffset calls causing high latency; change runSampling to issue multiple sampleClockOffset calls in parallel (e.g., create an array of Promise calls and use Promise.allSettled or implement a small concurrency/batching strategy) so multiple samples are collected concurrently, still filtering out failed results before calling chooseEstimateFromSamples, and ensure cancelled is checked before calling setEstimate; alternatively set an initial estimate from the first successful resolved sample (call setEstimate with that single sample) and then refine with chooseEstimateFromSamples as more parallel samples complete.
220-303:useMemois recalculated on every tick fromuseTime()even when there is no skew.When
estimateisnullor the offset is below threshold, the memo still runs on everynowupdate only to returnnull. You could short-circuit before the memo or split the null-check out:+ if (!estimate || Math.abs(estimate.finalOffsetMs) < 2 * MINUTE_MS) { + return null + } + const renderedWarning = useMemo(() => { - if (!estimate) { - return null - } - const absOffsetMs = Math.abs(estimate.finalOffsetMs) - if (absOffsetMs < 2 * MINUTE_MS) { - return null - } + const absOffsetMs = Math.abs(estimate.finalOffsetMs) ...This avoids formatting work and JSX creation on every tick for the common no-skew path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/src/app/org/`[organizationSlug]/(course-material)/exams/ExamClockSkewWarning.tsx around lines 220 - 303, The useMemo for renderedWarning is being re-run on every tick because now from useTime() is in its dependency list even when there's no skew; move the cheap null/threshold check out of the memo so that renderedWarning is short-circuited before useMemo (or create two branches: if !estimate or absOffsetMs < 2 * MINUTE_MS return null immediately), and only call useMemo when you know you need the heavy formatting/JSX (i.e., when estimate exists and absOffsetMs >= 2 * MINUTE_MS), updating refs to renderedWarning, useMemo, estimate, now, and MINUTE_MS accordingly so the expensive computations and JSX creation are avoided on every tick.services/main-frontend/src/app/org/[organizationSlug]/(course-material)/exams/ExamRunningSection.tsx (1)
49-79: Move static CSS class definitions to module scope.These Emotion
csscalls don't depend on any props or state—they only reference theme constants. Currently they are re-evaluated on every render, including the frequent timer-driven re-renders (every 5 s vianow). Extracting them to module scope avoids unnecessary work.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/src/app/org/`[organizationSlug]/(course-material)/exams/ExamRunningSection.tsx around lines 49 - 79, warningMessageClass, infoMessageClass and messageTextClass are currently defined inside the ExamRunningSection component and get recreated on every render; move these Emotion css() declarations to module scope (top-level of the file) so they are evaluated once. Ensure the moved definitions still reference the same theme constants (baseTheme, primaryFont) and keep their names unchanged so usages inside ExamRunningSection (e.g., className={warningMessageClass}) require no further edits.services/main-frontend/src/app/org/[organizationSlug]/(course-material)/exams/[id]/ExamGradingView.tsx (1)
27-59: Use theme colors instead of hardcoded hex values.Several colors here have direct equivalents in
baseTheme.colors:#f5f6f7→clear[100],#535a66→gray[500],#ffffff→clear[50].♻️ Suggested diff
+import { baseTheme, headingFont } from "@/shared-module/common/styles" -import { headingFont } from "@/shared-module/common/styles" ... className={css` display: flex; flex-direction: column; - background: `#f5f6f7`; + background: ${baseTheme.colors.clear[100]}; font-family: ${headingFont}; font-size: 18px; padding: 8px; margin: 10px; `} ... className={css` - color: `#535a66`; + color: ${baseTheme.colors.gray[500]}; font-size: 16px; padding-top: 1rem; `} ... className={css` - background: `#ffffff`; - color: `#535a66`; + background: ${baseTheme.colors.clear[50]}; + color: ${baseTheme.colors.gray[500]}; padding: 10px; `}As per coding guidelines, "If a component uses colors, use colors from the theme if possible."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/src/app/org/`[organizationSlug]/(course-material)/exams/[id]/ExamGradingView.tsx around lines 27 - 59, Replace hardcoded hex colors in ExamGradingView's styled blocks with the theme tokens: use baseTheme.colors.clear[100] instead of `#f5f6f7`, baseTheme.colors.gray[500] instead of `#535a66`, and baseTheme.colors.clear[50] instead of `#ffffff`; update the three css calls around the outer wrapper, the feedback label block, and the inner feedback container (the blocks referencing grade and justification) to reference these theme values (ensure baseTheme is imported or the component accesses the theme object used across the app and use template interpolation like ${baseTheme.colors.clear[100]} etc.).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/headless-lms/server/src/controllers/main_frontend/time.rs`:
- Around line 14-16: The get_current_time handler is adding a hardcoded offset
to Utc::now() which causes false clock-skew detections; remove the added
Duration arithmetic so server_time is computed directly from
Utc::now().to_rfc3339_opts(SecondsFormat::Millis, true) and then remove the
now-unused Duration import at the top of the file to keep imports clean; update
references in get_current_time and ensure no other code relies on that
artificial offset.
In
`@services/main-frontend/src/app/org/`[organizationSlug]/(course-material)/exams/ExamInfoHeader.tsx:
- Around line 27-66: Replace the three hard-coded hex colors inside
ExamInfoHeader.tsx with theme tokens from baseTheme: import baseTheme and use
baseTheme color tokens for the container background and text colors instead of
"#f6f6f6", "#333333", and "#353535"; specifically update the outer wrapper's
background (currently "#f6f6f6") to a semantic theme value (e.g.,
baseTheme.colors.background or baseTheme.colors.gray[...]), change the title
color (currently "#333333") to baseTheme's primary/text color (e.g.,
baseTheme.colors.textPrimary), and change the subtitle/secondary text color
(currently "#353535") to baseTheme's secondary/textMuted token (e.g.,
baseTheme.colors.textSecondary); keep the same CSS blocks that reference
primaryFont and respondToOrLarger.sm and only swap the literals for baseTheme
references.
- Around line 74-78: ExamData currently types starts_at and ends_at as
non-nullable string but ExamInfoHeader (in component ExamInfoHeader) uses
conditionals on examData.starts_at / examData.ends_at; update the ExamData type
definition so starts_at and ends_at are string | null (matching the Exam
interface) so the ternary branches remain meaningful, and ensure any other
usages of ExamData handle the nullable values (e.g., pass null-safe values into
humanReadableDateTime and t) rather than removing the conditionals in
ExamInfoHeader.
In
`@services/main-frontend/src/app/org/`[organizationSlug]/(course-material)/exams/ExamPageShell.tsx:
- Around line 167-179: When enrollmentTag === "StudentCanViewGrading" but
renderGradingView is not provided, ExamPageShell currently falls through to the
generic Spinner; change the branch to handle that case explicitly: if
enrollmentTag === "StudentCanViewGrading" and renderGradingView is falsy, return
a minimal fallback (e.g., CenteredClockSkewWarning + ExamInfoHeader with
examData) and optionally call console.warn or a logger to note the missing
renderGradingView prop; keep the original rendering when renderGradingView is
present and retain the Spinner only for non-enrolled/non-started states.
- Around line 126-129: The onStart currently calls enrollInExam directly and
provides no user-visible error; replace that direct call with a toast-enabled
mutation using useToastMutation so failures show a toast and success triggers
refresh: create a mutation via useToastMutation with mutationFn calling
enrollInExam(examId, mode === "testexam") and onSuccess invoking
handleRefresh(), then change ExamStartBanner's onStart to await the mutation's
mutateAsync() instead of calling enrollInExam/handleRefresh directly so errors
are surfaced via toast and success refreshes the page.
In
`@services/main-frontend/src/components/course-material/exams/TestExamTeacherTools.tsx`:
- Around line 1-106: Delete the unused duplicate component file that defines the
TestExamTeacherTools React component (the file in
services/main-frontend/src/components/course-material/exams/TestExamTeacherTools.tsx)
because it is byte-for-byte identical to the TestExamTeacherTools used by the
testexam route and is not imported anywhere; remove this file and ensure no
imports reference TestExamTeacherTools from that location (the app continues to
use the TestExamTeacherTools implementation from the testexam/[id] route).
In `@shared-module/packages/common/src/locales/en/main-frontend.json`:
- Around line 461-462: Remove the redundant key "exam-clock-warning-duration"
(it is unused) and update the two variant keys
"exam-clock-warning-duration-hours-only" and
"exam-clock-warning-duration-minutes-only" to use proper singular/plural forms
(e.g. a pluralization/template mechanism so they resolve to "{{hours}} hour"
when hours==1 and "{{hours}} hours" otherwise, and similarly for minutes) so
that "1 hours" / "1 minutes" are not produced; keep the existing
"exam-clock-warning-duration-hours-minutes" key as-is.
---
Duplicate comments:
In
`@services/main-frontend/src/app/org/`[organizationSlug]/(course-material)/exams/testexam/[id]/TestExamTeacherTools.tsx:
- Around line 1-106: This file duplicates the TestExamTeacherTools component
found in
services/main-frontend/src/components/course-material/exams/TestExamTeacherTools.tsx;
remove the duplication by keeping a single canonical implementation and either
delete this copy or turn it into a thin re-export (export { default } from "…")
pointing to the canonical module; ensure the canonical symbol
TestExamTeacherTools remains the default export, update any imports in the
codebase to use the canonical module, and run the build/tests to confirm no
import paths break.
In `@shared-module/packages/common/src/locales/en/main-frontend.json`:
- Around line 463-464: The new duration keys produce incorrect singular forms;
update the JSON keys "exam-clock-warning-duration-hours-only" and
"exam-clock-warning-duration-minutes-only" to use i18next plural variants (e.g.
add "_one" and "_other" entries) so "1 hour" and "2 hours" (similarly for
minutes) render correctly, mirror the pattern used for
"exam-clock-warning-duration" (use "{{count}} hour" for _one and "{{count}}
hours" for _other), and ensure the component passes count: hours / count:
minutes when calling t().
---
Nitpick comments:
In `@services/headless-lms/server/src/controllers/main_frontend/time.rs`:
- Around line 58-59: The test currently only checks RFC3339 formatting; update
it to also parse the timestamp and assert it's close to now: use
chrono::DateTime::parse_from_rfc3339(&body) to get a FixedOffset DateTime,
convert to Utc via .with_timezone(&chrono::Utc), get chrono::Utc::now(), compute
the absolute difference with chrono::Duration::seconds (e.g. 5 seconds) and
assert the diff is less than that threshold; keep the existing format assertion
but add this proximity check to catch time-offset bugs (referencing the
variables body and resp and functions parse_from_rfc3339 and Utc::now).
In
`@services/main-frontend/src/app/org/`[organizationSlug]/(course-material)/exams/[id]/EndExamButton.tsx:
- Around line 21-29: The mutation currently wraps examId in an unnecessary
object; simplify by changing the mutation initializer to call endExamTime with
the outer-scope examId directly (e.g., pass () => endExamTime(examId) into
useToastMutation) and update handleEndExam to call endExamMutation.mutate() with
no payload. Update references to endExamMutation, useToastMutation,
handleEndExam, and endExamTime accordingly to remove the { id: string } wrapper.
In
`@services/main-frontend/src/app/org/`[organizationSlug]/(course-material)/exams/[id]/ExamGradingView.tsx:
- Around line 27-59: Replace hardcoded hex colors in ExamGradingView's styled
blocks with the theme tokens: use baseTheme.colors.clear[100] instead of
`#f5f6f7`, baseTheme.colors.gray[500] instead of `#535a66`, and
baseTheme.colors.clear[50] instead of `#ffffff`; update the three css calls around
the outer wrapper, the feedback label block, and the inner feedback container
(the blocks referencing grade and justification) to reference these theme values
(ensure baseTheme is imported or the component accesses the theme object used
across the app and use template interpolation like
${baseTheme.colors.clear[100]} etc.).
In
`@services/main-frontend/src/app/org/`[organizationSlug]/(course-material)/exams/ExamClockSkewWarning.tsx:
- Around line 72-92: The object in formatDateTimeInTimezone has repeated
per-line eslint-disable-next-line comments; replace them with a single block
disable around the entire options object (e.g., add /* eslint-disable
i18next/no-literal-string */ before the DateTimeFormat options and re-enable
with /* eslint-enable i18next/no-literal-string */ after) so the code is cleaner
and matches the pattern used in the nearby lines.
- Around line 189-218: The current useEffect/runSampling loop awaits
SAMPLE_COUNT sequential sampleClockOffset calls causing high latency; change
runSampling to issue multiple sampleClockOffset calls in parallel (e.g., create
an array of Promise calls and use Promise.allSettled or implement a small
concurrency/batching strategy) so multiple samples are collected concurrently,
still filtering out failed results before calling chooseEstimateFromSamples, and
ensure cancelled is checked before calling setEstimate; alternatively set an
initial estimate from the first successful resolved sample (call setEstimate
with that single sample) and then refine with chooseEstimateFromSamples as more
parallel samples complete.
- Around line 220-303: The useMemo for renderedWarning is being re-run on every
tick because now from useTime() is in its dependency list even when there's no
skew; move the cheap null/threshold check out of the memo so that
renderedWarning is short-circuited before useMemo (or create two branches: if
!estimate or absOffsetMs < 2 * MINUTE_MS return null immediately), and only call
useMemo when you know you need the heavy formatting/JSX (i.e., when estimate
exists and absOffsetMs >= 2 * MINUTE_MS), updating refs to renderedWarning,
useMemo, estimate, now, and MINUTE_MS accordingly so the expensive computations
and JSX creation are avoided on every tick.
In
`@services/main-frontend/src/app/org/`[organizationSlug]/(course-material)/exams/ExamRunningSection.tsx:
- Around line 49-79: warningMessageClass, infoMessageClass and messageTextClass
are currently defined inside the ExamRunningSection component and get recreated
on every render; move these Emotion css() declarations to module scope
(top-level of the file) so they are evaluated once. Ensure the moved definitions
still reference the same theme constants (baseTheme, primaryFont) and keep their
names unchanged so usages inside ExamRunningSection (e.g.,
className={warningMessageClass}) require no further edits.
services/headless-lms/server/src/controllers/main_frontend/time.rs
Outdated
Show resolved
Hide resolved
...ices/main-frontend/src/app/org/[organizationSlug]/(course-material)/exams/ExamInfoHeader.tsx
Outdated
Show resolved
Hide resolved
...ices/main-frontend/src/app/org/[organizationSlug]/(course-material)/exams/ExamInfoHeader.tsx
Outdated
Show resolved
Hide resolved
services/main-frontend/src/app/org/[organizationSlug]/(course-material)/exams/ExamPageShell.tsx
Show resolved
Hide resolved
services/main-frontend/src/app/org/[organizationSlug]/(course-material)/exams/ExamPageShell.tsx
Outdated
Show resolved
Hide resolved
services/main-frontend/src/components/course-material/exams/TestExamTeacherTools.tsx
Outdated
Show resolved
Hide resolved
shared-module/packages/common/src/locales/en/main-frontend.json
Outdated
Show resolved
Hide resolved
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 (1)
shared-module/packages/common/src/locales/uk/main-frontend.json (1)
439-451:⚠️ Potential issue | 🟡 MinorAdd missing Ukrainian translations for exam clock-skew warning and other new features.
The Ukrainian locale is missing 67 keys present in the English locale, including all 15
exam-clock-warning-*keys required for the clock-skew warning feature. Additionally missing are keys for email templates, chapter locking, user settings, and verification flows. All these keys must be translated and added toshared-module/packages/common/src/locales/uk/main-frontend.json.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared-module/packages/common/src/locales/uk/main-frontend.json` around lines 439 - 451, The Ukrainian locale file is missing 67 keys (including all 15 exam-clock-warning-* entries) and other new keys used across email templates, chapter locking, user settings, and verification flows; fix by copying the missing keys from the English main-frontend.json into shared-module/packages/common/src/locales/uk/main-frontend.json and provide accurate Ukrainian translations for each (preserve ICU/HTML placeholders like {{ starts-at }}, {{ ends-at }}, {{ time-minutes }}, and tags like <1>…</1>), specifically ensure all keys named exam-clock-warning-1 through exam-clock-warning-15 plus the new email/template, chapter-lock, user-setting, and verification keys present in EN are added and translated while keeping interpolation tokens and pluralization formats identical.
🧹 Nitpick comments (4)
services/main-frontend/src/app/org/[organizationSlug]/(course-material)/exams/[id]/ExamGradingView.tsx (2)
20-49: Hoist static CSS class definitions to module scope.All four
css\...`` calls are static (they depend only on theme tokens, which are module-level constants) but are re-created on every render. Emotion caches by content hash, so class names stay stable, but the template literal is still evaluated on each render. Moving them outside the component is the idiomatic Emotion CSS pattern used elsewhere in the codebase.♻️ Proposed refactor (excerpt)
+const cardClass = css` + display: flex; + flex-direction: column; + background: ${baseTheme.colors.clear[200]}; + border: 1px solid ${baseTheme.colors.clear[300]}; + border-radius: 8px; + padding: 1rem 1.25rem; + margin: 1rem 0; + font-family: ${headingFont}; + font-size: 1.125rem; + color: ${baseTheme.colors.gray[700]}; +` +const pointsClass = css` + font-family: ${primaryFont}; +` +const feedbackLabelClass = css` + font-family: ${primaryFont}; + color: ${baseTheme.colors.gray[600]}; + font-size: 1rem; + padding-top: 1rem; +` +const feedbackBoxClass = css` + background: ${baseTheme.colors.primary[100]}; + color: ${baseTheme.colors.gray[600]}; + padding: 1rem; + border-radius: 6px; + border: 1px solid ${baseTheme.colors.clear[300]}; + margin-top: 0.5rem; + font-family: ${primaryFont}; +` export default function ExamGradingView({ examData }: ExamGradingViewProps) { const { t } = useTranslation() if (examData.enrollment_data.tag !== "StudentCanViewGrading") { return null } const { gradings } = examData.enrollment_data - const cardClass = css`...` - const pointsClass = css`...` - const feedbackLabelClass = css`...` - const feedbackBoxClass = css`...`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/src/app/org/`[organizationSlug]/(course-material)/exams/[id]/ExamGradingView.tsx around lines 20 - 49, The four Emotion css definitions (cardClass, pointsClass, feedbackLabelClass, feedbackBoxClass) are currently created inside the component on every render; move these const declarations out to module scope (above the component function) so they are only evaluated once, keeping the same identifiers and using the same theme token references so existing usage in ExamGradingView remains unchanged.
52-68: Preferfilter+mapover returningfalsefrommap, and destructure the tuple for clarity.Returning
!grade[0].hidden && (...)frommapyieldsfalsefor hidden items, which React silently discards but which can confuse readers and static analysis. Afilter+mapsplit is cleaner. The tuple indicesgrade[0]/grade[1]are also opaque; naming the parts improves readability.♻️ Proposed refactor
- {gradings.map( - (grade) => - !grade[0].hidden && ( - <div key={grade[0].id} className={cardClass}> - <div> - {t("label-name")}: {grade[1].name} - </div> - <div className={pointsClass}> - {t("points")}: {grade[0].score_given} / {grade[1].score_maximum} - </div> - <div className={feedbackLabelClass}> - {t("label-feedback")}: - <div className={feedbackBoxClass}>{grade[0].justification}</div> - </div> - </div> - ), - )} + {gradings + .filter(([gradeRecord]) => !gradeRecord.hidden) + .map(([gradeRecord, exerciseInfo]) => ( + <div key={gradeRecord.id} className={cardClass}> + <div> + {t("label-name")}: {exerciseInfo.name} + </div> + <div className={pointsClass}> + {t("points")}: {gradeRecord.score_given} / {exerciseInfo.score_maximum} + </div> + <div className={feedbackLabelClass}> + {t("label-feedback")}: + <div className={feedbackBoxClass}>{gradeRecord.justification}</div> + </div> + </div> + ))}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/src/app/org/`[organizationSlug]/(course-material)/exams/[id]/ExamGradingView.tsx around lines 52 - 68, The current JSX uses gradings.map returning "!grade[0].hidden && (...)" and accesses tuple indices grade[0]/grade[1]; change this to first filter out hidden entries and then map over the remaining tuples while destructuring the tuple into meaningful names (e.g., [submission, spec] or [gradeRecord, gradeSpec]) and use those names when building the elements that use cardClass, pointsClass, feedbackLabelClass, and feedbackBoxClass so no false values are emitted from map and tuple accesses are clearer.services/main-frontend/src/app/org/[organizationSlug]/(course-material)/exams/ExamClockSkewWarning.tsx (1)
71-103: Use block-leveleslint-disableinformatDateTimeInTimezoneandformatTimeInTimezonefor consistency.
getDateStringInTimezonealready uses the cleaner block-level/* eslint-disable/enable */approach (lines 61–69). Repeating// eslint-disable-next-line i18next/no-literal-stringseven times in two sibling functions adds noise without adding information.♻️ Proposed refactor
+/* eslint-disable i18next/no-literal-string */ const formatDateTimeInTimezone = ( timestampMs: number, timeZone: string, language: string, ): string => { return new Intl.DateTimeFormat(language, { timeZone, - // eslint-disable-next-line i18next/no-literal-string year: "numeric", - // eslint-disable-next-line i18next/no-literal-string month: "2-digit", - // eslint-disable-next-line i18next/no-literal-string day: "2-digit", - // eslint-disable-next-line i18next/no-literal-string hour: "2-digit", - // eslint-disable-next-line i18next/no-literal-string minute: "2-digit", - // eslint-disable-next-line i18next/no-literal-string second: "2-digit", }).format(new Date(timestampMs)) } const formatTimeInTimezone = (timestampMs: number, timeZone: string, language: string): string => { return new Intl.DateTimeFormat(language, { timeZone, - // eslint-disable-next-line i18next/no-literal-string hour: "2-digit", - // eslint-disable-next-line i18next/no-literal-string minute: "2-digit", - // eslint-disable-next-line i18next/no-literal-string second: "2-digit", }).format(new Date(timestampMs)) } +/* eslint-enable i18next/no-literal-string */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/src/app/org/`[organizationSlug]/(course-material)/exams/ExamClockSkewWarning.tsx around lines 71 - 103, The two functions formatDateTimeInTimezone and formatTimeInTimezone repeat many single-line eslint-disable comments; replace those with a block-level disable/enable around each function (or around both together) using /* eslint-disable i18next/no-literal-string */ before the function(s) and /* eslint-enable i18next/no-literal-string */ after to match getDateStringInTimezone's style, keeping all Intl.DateTimeFormat options intact and removing the per-line // eslint-disable-next-line comments.services/main-frontend/src/app/org/[organizationSlug]/(course-material)/exams/ExamPageShell.tsx (1)
124-128: Hoist identicalpadding-top: 2remsnippets to a module-level constant.The same single-rule CSS block appears three times inside the render path. Extracting it avoids repeated string construction and keeps the JSX readable.
♻️ Proposed refactor
+const paddingTopClass = css` + padding-top: 2rem; +` // …then replace each occurrence: - <div - className={css` - padding-top: 2rem; - `} - /> + <div className={paddingTopClass} />Also applies to: 175-177, 193-197
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/src/app/org/`[organizationSlug]/(course-material)/exams/ExamPageShell.tsx around lines 124 - 128, The repeated single-rule CSS block "padding-top: 2rem" used in ExamPageShell's JSX should be hoisted to a module-level constant; add a constant (e.g., const paddingTop2 = css`padding-top: 2rem;`) at the top of the file and replace the inline css`padding-top: 2rem;` occurrences inside the ExamPageShell render with className={paddingTop2} (update all three instances around the existing snippets shown). Ensure the constant is exported or local as appropriate and remove the duplicated inline css blocks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shared-module/packages/common/src/locales/en/main-frontend.json`:
- Around line 459-461: Remove the five unused locale keys from the
exam-clock-warning block: delete "exam-clock-warning-direction-ahead",
"exam-clock-warning-direction-behind", "exam-clock-warning-duration",
"exam-clock-warning-title", and "exam-clock-warning-title-severe" from the JSON
so only the keys actually used by ExamClockSkewWarning.tsx (e.g.,
"exam-clock-warning-difference-fast"/"-slow" and the specific duration variants
"-hours-minutes", "-hours-only", "-minutes-only") remain; repeat this cleanup
across all locale files to avoid dead strings in every locale.
---
Outside diff comments:
In `@shared-module/packages/common/src/locales/uk/main-frontend.json`:
- Around line 439-451: The Ukrainian locale file is missing 67 keys (including
all 15 exam-clock-warning-* entries) and other new keys used across email
templates, chapter locking, user settings, and verification flows; fix by
copying the missing keys from the English main-frontend.json into
shared-module/packages/common/src/locales/uk/main-frontend.json and provide
accurate Ukrainian translations for each (preserve ICU/HTML placeholders like {{
starts-at }}, {{ ends-at }}, {{ time-minutes }}, and tags like <1>…</1>),
specifically ensure all keys named exam-clock-warning-1 through
exam-clock-warning-15 plus the new email/template, chapter-lock, user-setting,
and verification keys present in EN are added and translated while keeping
interpolation tokens and pluralization formats identical.
---
Duplicate comments:
In
`@services/main-frontend/src/app/org/`[organizationSlug]/(course-material)/exams/ExamPageShell.tsx:
- Around line 133-136: The onStart callback calls enrollInExam(examId, mode ===
"testexam") directly which can fail silently; wrap this server call with the
useToastMutation (or a hook that uses it) so failures surface to the user and
success/failure toasts are shown. Replace the direct await enrollInExam(...) in
the onStart handler with a mutation created via useToastMutation({ mutationFn:
() => enrollInExam(examId, mode === "testexam"), onSuccess: () =>
handleRefresh(), onError: (err) => /* show error toast */ }) and invoke that
mutation in onStart, ensuring the button state still toggles only after mutation
resolution. Ensure you reference enrollInExam, onStart, handleRefresh and
useToastMutation when making the change.
- Around line 190-207: The ExamPageShell currently lets enrollmentTag ===
"StudentCanViewGrading" fall through to the enrollmentTag !==
"EnrolledAndStarted" check and render a perpetual <Spinner>, so add an explicit
branch in ExamPageShell to handle when enrollmentTag === "StudentCanViewGrading"
but renderGradingView is missing: detect (enrollmentTag ===
"StudentCanViewGrading" && !renderGradingView) and return a clear fallback UI
(e.g., a message/empty state or a non-blocking placeholder) or log/report the
missing prop instead of falling through to the Spinner; update the conditional
logic around enrollmentTag and renderGradingView to ensure the
StudentCanViewGrading path always returns a definitive UI.
In `@shared-module/packages/common/src/locales/en/main-frontend.json`:
- Around line 463-464: The translation keys
"exam-clock-warning-duration-hours-only" and
"exam-clock-warning-duration-minutes-only" produce incorrect singular forms;
change their values to use i18next plural suffixes (e.g.
"exam-clock-warning-duration-hours-only_one"/"_other" and similarly for minutes)
or use the pluralized format "{{count}} hour" / "{{count}} hours" and then
update call sites that use t() (where these keys are referenced) to pass the
count parameter (t('exam-clock-warning-duration-hours-only', { count: hours })
and t('exam-clock-warning-duration-minutes-only', { count: minutes })) so
i18next will select the correct singular/plural form.
---
Nitpick comments:
In
`@services/main-frontend/src/app/org/`[organizationSlug]/(course-material)/exams/[id]/ExamGradingView.tsx:
- Around line 20-49: The four Emotion css definitions (cardClass, pointsClass,
feedbackLabelClass, feedbackBoxClass) are currently created inside the component
on every render; move these const declarations out to module scope (above the
component function) so they are only evaluated once, keeping the same
identifiers and using the same theme token references so existing usage in
ExamGradingView remains unchanged.
- Around line 52-68: The current JSX uses gradings.map returning
"!grade[0].hidden && (...)" and accesses tuple indices grade[0]/grade[1]; change
this to first filter out hidden entries and then map over the remaining tuples
while destructuring the tuple into meaningful names (e.g., [submission, spec] or
[gradeRecord, gradeSpec]) and use those names when building the elements that
use cardClass, pointsClass, feedbackLabelClass, and feedbackBoxClass so no false
values are emitted from map and tuple accesses are clearer.
In
`@services/main-frontend/src/app/org/`[organizationSlug]/(course-material)/exams/ExamClockSkewWarning.tsx:
- Around line 71-103: The two functions formatDateTimeInTimezone and
formatTimeInTimezone repeat many single-line eslint-disable comments; replace
those with a block-level disable/enable around each function (or around both
together) using /* eslint-disable i18next/no-literal-string */ before the
function(s) and /* eslint-enable i18next/no-literal-string */ after to match
getDateStringInTimezone's style, keeping all Intl.DateTimeFormat options intact
and removing the per-line // eslint-disable-next-line comments.
In
`@services/main-frontend/src/app/org/`[organizationSlug]/(course-material)/exams/ExamPageShell.tsx:
- Around line 124-128: The repeated single-rule CSS block "padding-top: 2rem"
used in ExamPageShell's JSX should be hoisted to a module-level constant; add a
constant (e.g., const paddingTop2 = css`padding-top: 2rem;`) at the top of the
file and replace the inline css`padding-top: 2rem;` occurrences inside the
ExamPageShell render with className={paddingTop2} (update all three instances
around the existing snippets shown). Ensure the constant is exported or local as
appropriate and remove the duplicated inline css blocks.
shared-module/packages/common/src/locales/en/main-frontend.json
Outdated
Show resolved
Hide resolved
- Added new styling for exam details, including start and end times, duration, grading options, and language. - Introduced localization strings for new labels and actions related to exam management. - Improved layout and presentation of exam details for better user experience. - Integrated edit functionality for exam details with a dialog component.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/main-frontend/src/app/manage/exams/`[id]/page.tsx:
- Around line 268-288: The TextField is currently uncontrolled so clearing state
via setNewCourse("") won't update the visible input; make the TextField
controlled by adding the value prop bound to newCourse (value={newCourse}) and
keep the existing onChange that calls setNewCourse, ensuring after calling
setCourseMutation.mutate(...) and setNewCourse("") the input is visually
cleared; verify this change around the TextField, setNewCourse, newCourse, and
the Button handler that calls setCourseMutation.mutate({ examId:
getExam.data.id, courseId: newCourse }).
- Around line 163-184: The "Edit Exam" button can be clicked before
organizationId resolves causing EditExamDialog to get an empty organizationId;
modify the Button and opening logic to guard on organizationId: set the Button's
disabled prop to !organizationId (or !organizationId || !getExam.isSuccess) and
update the onClick to only call setEditExamFormOpen(true) when organizationId is
defined; ensure EditExamDialog is only rendered/opened with a non-empty
organizationId (examId/getExam.data can remain as-is) so submissions never send
an empty organizationId to the backend.
---
Duplicate comments:
In `@shared-module/packages/common/src/locales/en/main-frontend.json`:
- Around line 467-468: The two locale entries
"exam-clock-warning-duration-hours-only" and
"exam-clock-warning-duration-minutes-only" produce incorrect grammar for
singular (e.g., "1 hours"); change each to use i18next plural keys by adding a
plural variant (e.g., keep the base key as "{{count}} hour" and add
"exam-clock-warning-duration-hours-only_plural" as "{{count}} hours", and do the
same for minutes) and update consumers to pass count: hours / count: minutes
instead of hours / minutes so i18next will pick the correct singular/plural
form.
- Around line 463-465: Remove the unused localization keys from the JSON: delete
"exam-clock-warning-direction-ahead", "exam-clock-warning-direction-behind",
"exam-clock-warning-duration", "exam-clock-warning-title", and
"exam-clock-warning-title-severe" (also duplicates at the other reported
locations), but first grep for usages from the ExamClockSkewWarning component
and other code to confirm they are not referenced; ensure only the actual keys
used by ExamClockSkewWarning.tsx (the -fast/-slow and hours/minutes variants)
remain and update any automated locale validation tests or snapshot fixtures if
they expect the removed keys.
# Conflicts: # shared-module/packages/common/src/locales/fi/main-frontend.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
services/headless-lms/server/src/controllers/main_frontend/time.rs (1)
58-60: Minor: redundant double-parse of the RFC3339 string in tests.Line 58 parses to check
is_ok(), then line 60 re-parses the same string unconditionally to get the value. Combine into one parse with.expect(...)to eliminate the redundancy.♻️ Proposed simplification
- let body: String = test::read_body_json(resp).await; - assert!(chrono::DateTime::parse_from_rfc3339(&body).is_ok()); - - let parsed = chrono::DateTime::parse_from_rfc3339(&body).unwrap(); + let body: String = test::read_body_json(resp).await; + let parsed = chrono::DateTime::parse_from_rfc3339(&body) + .expect("body should be a valid RFC3339 string");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/headless-lms/server/src/controllers/main_frontend/time.rs` around lines 58 - 60, Tests currently parse the RFC3339 string twice (assert!(...is_ok()) then parse_from_rfc3339(...).unwrap()); replace the two-step parse with a single call that both asserts and returns the value by using chrono::DateTime::parse_from_rfc3339(&body).expect("failed to parse RFC3339 from body") and assign that to parsed (remove the earlier assert!), so the variable parsed is produced in one step in time.rs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/main-frontend/src/app/manage/exams/`[id]/page.tsx:
- Around line 284-296: The onClick handler clears the input immediately by
calling setNewCourse("") right after setCourseMutation.mutate(...), which loses
the user's value if the mutation fails; move the clearing logic into the
mutation's onSuccess callback on setCourseMutation so the input is only reset
after a successful response — update the setCourseMutation configuration (the
mutate call or its mutation definition) to include an onSuccess that calls
setNewCourse("") and remove the synchronous setNewCourse("") from the Button
onClick handler.
---
Duplicate comments:
In `@services/headless-lms/server/src/controllers/main_frontend/time.rs`:
- Around line 14-28: get_current_time already correctly uses Utc::now() and sets
no-cache headers, so no functional change is required; ensure the now-unused
Duration import has been removed and confirm the function get_current_time in
time.rs remains as shown (including token = skip_authorize() and
token.authorized_ok). If there are any leftover duplicate review comments or
metadata markers like [duplicate_comment], remove them from the PR description
only — no code edits needed.
---
Nitpick comments:
In `@services/headless-lms/server/src/controllers/main_frontend/time.rs`:
- Around line 58-60: Tests currently parse the RFC3339 string twice
(assert!(...is_ok()) then parse_from_rfc3339(...).unwrap()); replace the
two-step parse with a single call that both asserts and returns the value by
using chrono::DateTime::parse_from_rfc3339(&body).expect("failed to parse
RFC3339 from body") and assign that to parsed (remove the earlier assert!), so
the variable parsed is produced in one step in time.rs.
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 by CodeRabbit
New Features
Documentation