Open
Conversation
- Introduced new atoms for managing draft stages in course plans using Jotai. - Implemented functions to add and remove months from stages, ensuring contiguous stage management. - Enhanced the CoursePlanSchedulePage component to utilize the new state management and display month labels. - Added corresponding tests for the new functionality in scheduleStageTransforms. - Updated localization strings for new actions related to month management in course plans.
- Introduced a new handleFinalizeSchedule function to streamline the finalization process of course schedules. - Updated button states to include checks for finalizeMutation.isPending, ensuring proper user feedback during operations. - Refactored test cases to utilize parseISO for date handling, improving date management in schedule stage transformations.
github-actions
bot
added
dependencies
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:
📝 WalkthroughWalkthroughAdds a Course Designer feature across backend (DB migrations, Rust models, Actix controllers, SQLx queries), frontend (React UI, hooks, state, API client, routes, tests, locales), TypeScript bindings/guards, ESLint config tweak, and a motion dependency. Changes
Sequence DiagramsequenceDiagram
participant User
participant FE as Frontend (React)
participant API as HTTP API (Actix)
participant Models as Rust Models
participant DB as Postgres
User->>FE: create plan / request suggestions / edit schedule
FE->>API: HTTP POST/PUT to /course-plans and schedule endpoints
API->>Models: call model functions (create_plan, build_schedule_suggestion, replace_schedule, finalize)
Models->>DB: INSERT / SELECT / UPDATE (plans, stages, tasks, events)
DB-->>Models: rows / IDs
Models-->>API: domain objects / DTOs
API-->>FE: JSON responses
FE->>FE: update Jotai atoms and UI state
User->>FE: save / finalize
FE->>API: PUT /course-plans/{id}/schedule or POST /finalize
API->>Models: transactional updates and event inserts
Models->>DB: transactional writes
DB-->>Models: confirmation
Models-->>API: updated plan/details
API-->>FE: updated data (navigate to workspace)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 16
🧹 Nitpick comments (15)
services/headless-lms/models/.sqlx/query-c8d0042fd40cd20e963f96eb070e84714eeb8653571856fb8a013a87cff93980.json (1)
3-3: Make member inserts idempotent for retry safety.Line 3 uses a plain INSERT, so duplicate requests can fail instead of becoming a no-op. Consider adding conflict handling.
Proposed update
- "query": "\nINSERT INTO course_designer_plan_members (course_designer_plan_id, user_id)\nVALUES ($1, $2)\n", + "query": "\nINSERT INTO course_designer_plan_members (course_designer_plan_id, user_id)\nVALUES ($1, $2)\nON CONFLICT DO NOTHING\n",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/headless-lms/models/.sqlx/query-c8d0042fd40cd20e963f96eb070e84714eeb8653571856fb8a013a87cff93980.json` at line 3, The INSERT into course_designer_plan_members should be made idempotent to avoid duplicate key errors on retries: change the query that currently does INSERT INTO course_designer_plan_members (course_designer_plan_id, user_id) VALUES ($1, $2) to include conflict handling, e.g. INSERT ... ON CONFLICT (course_designer_plan_id, user_id) DO NOTHING (or ON CONFLICT DO NOTHING if a suitable unique constraint exists) so duplicate inserts become no-ops.services/main-frontend/src/app/manage/course-plans/coursePlanQueryKeys.ts (1)
1-4: Add a short doc comment for this exported query-key API.Line 1 exposes a shared cache-key surface; a brief note about intent/stability helps prevent accidental key-shape changes.
As per coding guidelines, "Require consistent, minimal function documentation... Public interfaces should have clearer and more comprehensive documentation."Proposed update
+/** + * Stable React Query keys for course designer plans. + * Keep key shapes unchanged unless cache invalidation strategy is intentionally updated. + */ export const coursePlanQueryKeys = { list: () => ["course-designer-plans"] as const, detail: (planId: string) => ["course-designer-plan", planId] as const, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/src/app/manage/course-plans/coursePlanQueryKeys.ts` around lines 1 - 4, Add a short doc comment above the exported coursePlanQueryKeys object describing that it is the shared cache-key surface used for course plan React Query cache keys, noting its intent (stable, public API) and warning not to change the key shape lightly; mention the two helpers (list and detail(planId: string)) and their return shapes so callers know they produce immutable tuple keys. Ensure the comment sits immediately above the coursePlanQueryKeys export and refers to the list and detail helpers by name.services/main-frontend/src/app/manage/course-plans/[id]/schedule/scheduleAtoms.ts (1)
8-36: Consider cleaning up atom families when plans are unmounted to prevent memory accumulation.
atomFamilycaches atoms indefinitely by default. Each plan ID creates entries in all four families. If users navigate across many plans in a session, stale atoms accumulate. You can callatomFamily.remove(planId)in a cleanup effect when the plan view unmounts.This is unlikely to be a practical issue given typical usage patterns, but worth keeping in mind.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/src/app/manage/course-plans/`[id]/schedule/scheduleAtoms.ts around lines 8 - 36, The atom families draftStagesAtomFamily, addMonthToStageAtomFamily, removeMonthFromStageAtomFamily, and scheduleWizardStepAtomFamily are cached indefinitely and should be removed when a plan view unmounts to avoid memory growth; update the plan-level component's cleanup (useEffect return or equivalent unmount handler) to call draftStagesAtomFamily.remove(planId), addMonthToStageAtomFamily.remove(planId), removeMonthFromStageAtomFamily.remove(planId), and scheduleWizardStepAtomFamily.remove(planId) for the current planId so the per-plan atoms are released when the view is torn down.services/headless-lms/migrations/20260223110000_add_course_designer_planning_tables.up.sql (1)
86-124: Consider adding an index oncourse_designer_plan_idfor the stages table.PostgreSQL does not automatically create indexes on foreign key columns. The
course_designer_plan_stages_plan_stage_uniqueconstraint covers(plan_id, stage, deleted_at)which will help some queries, but a dedicated index on(course_designer_plan_id, deleted_at)would optimize the common pattern of fetching all stages for a plan.The same consideration applies to
course_designer_plan_stage_tasks.course_designer_plan_stage_idandcourse_designer_plan_events.course_designer_plan_id(though the events table already has an index coveringcourse_designer_plan_id).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/headless-lms/migrations/20260223110000_add_course_designer_planning_tables.up.sql` around lines 86 - 124, Add explicit b-tree indexes to speed lookups by plan and soft-delete filtering: create an index on course_designer_plan_stages (course_designer_plan_id, deleted_at) to optimize fetching stages for a plan (alongside the existing UNIQUE constraint course_designer_plan_stages_plan_stage_unique), add an index on course_designer_plan_stage_tasks (course_designer_plan_stage_id) to speed task lookups by stage, and ensure course_designer_plan_events has an index on course_designer_plan_id (or add one if missing) to optimize event queries; implement these as CREATE INDEX statements in the migration after the table definitions.services/main-frontend/src/services/backend/courseDesigner.ts (1)
87-128: Add brief docs for exported API wrappers.These are public service functions; short intent-level docs would make endpoint purpose and usage clearer for maintainers.
As per coding guidelines, "Require consistent, minimal function documentation... Public interfaces should have clearer and more comprehensive documentation."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/src/services/backend/courseDesigner.ts` around lines 87 - 128, Add brief JSDoc-style docs to each exported API wrapper (createCourseDesignerPlan, listCourseDesignerPlans, getCourseDesignerPlan, generateCourseDesignerScheduleSuggestion, saveCourseDesignerSchedule, finalizeCourseDesignerSchedule) that state the intent/purpose, describe parameters (e.g., planId, payload) and the returned type, and note any side effects (HTTP method and endpoint) or errors thrown; keep each comment 1–3 lines and placed immediately above the function so maintainers can quickly understand usage and expected return values.services/headless-lms/models/src/course_designer_plans.rs (1)
116-812: Add short rustdoc comments for the new public model APIs.This module introduces a broad public surface (
fixed_stage_order,validate_schedule_input,build_schedule_suggestion, CRUD/scheduling functions), but intent-level docs are mostly missing. Please add concise docs for each public function.As per coding guidelines, "Require consistent, minimal function documentation... Public interfaces should have clearer and more comprehensive documentation."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/headless-lms/models/src/course_designer_plans.rs` around lines 116 - 812, Add concise Rustdoc comments for each public function to clarify intent, parameters and returns: add /// one-line summaries and short param/return notes above fixed_stage_order, validate_schedule_input, build_schedule_suggestion, create_plan, list_plans_for_user, get_plan_for_user, get_plan_members_for_user, get_plan_stages_for_user, get_plan_details_for_user, replace_schedule_for_user, finalize_schedule_for_user, and no_gap_between; keep them minimal (single-sentence intent plus brief param/return or error behavior), reference relevant types (e.g. CourseDesignerStage, CourseDesignerScheduleStageInput, CourseDesignerCourseSize, PgConnection) and mention error conditions (ModelResult/ModelError) where applicable to match project docs guidelines.services/main-frontend/src/app/manage/course-plans/[id]/schedule/components/steps/SetupStep.tsx (1)
54-116: Form inputs should usereact-hook-formper coding guidelines.This component presents two controlled inputs (
select,input[type="month"]) with validation logic (empty check onstartsOnMonth) but bypassesreact-hook-form. As per coding guidelines, any form must useuseFormfromreact-hook-formintegrated withuseToastMutation/useQueryhooks. As a plain wizard step it may not need full mutation wiring here, but the controlled inputs and validation logic should still go throughreact-hook-form.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/src/app/manage/course-plans/`[id]/schedule/components/steps/SetupStep.tsx around lines 54 - 116, Replace the manual controlled inputs in the SetupStep component with react-hook-form: create a useForm inside SetupStep, register the "courseSize" select and "startsOnMonth" month input (use register with required validation for startsOnMonth), replace direct value/onChange usage with the register bindings and read current values from formState or getValues, and wire the Continue button to handleSubmit to call the existing onContinue prop only when the form is valid (respecting isGeneratingSuggestion to disable as before); keep onBack unchanged and still call onCourseSizeChange/onStartsOnMonthChange inside a form watch/handleSubmit or a useEffect if parent needs immediate updates.services/main-frontend/src/app/manage/course-plans/[id]/schedule/components/MonthBlock.tsx (2)
20-20:forwardRefis deprecated in React 19 — passrefas a plain prop instead.The project uses React 19.2.1 where
forwardRefis deprecated andrefis a regular prop. Motion's own docs note: "React 19: React 19 can pass ref via props:const Component = (props) => { return <div ref={props.ref} /> }". React 19 now allows passingrefdirectly as a standard prop, makingforwardRefunnecessary.♻️ Proposed refactor
-import { forwardRef } from "react" +import { ComponentPropsWithRef } from "react" interface MonthBlockProps { month: StageMonth reduceMotion: boolean layoutId: string + ref?: ComponentPropsWithRef<"div">["ref"] } -const MonthBlock = forwardRef<HTMLDivElement, MonthBlockProps>(function MonthBlock( - { month, reduceMotion, layoutId }, - ref, -) { +function MonthBlock({ month, reduceMotion, layoutId, ref }: MonthBlockProps) { const MonthIcon = MONTH_ICONS[month.date.getMonth()] return ( <motion.div ref={ref} ... > ... </motion.div> ) -}) +} export default MonthBlockAlso applies to: 80-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/src/app/manage/course-plans/`[id]/schedule/components/MonthBlock.tsx at line 20, The component currently imports and uses forwardRef; update the MonthBlock component to accept ref as a normal prop instead: remove the forwardRef import, change the component signature (e.g., MonthBlock = (props) => or destructure including ref) so it receives props.ref, and pass props.ref (or the destructured ref) to the DOM element(s) that previously received the forwarded ref; also update any export that wrapped the component with forwardRef to export the plain component and remove the forwardRef import/usage. Ensure all internal references to the forwarded ref (within MonthBlock) use the prop name you chose and adjust any call sites that passed refs accordingly.
39-72: Hardcoded hex colors should usebaseThemevalues.All color values in the styles are raw hex literals (
#d9dde4,white,#415167,#6a7686,#2d7b4f) rather thanbaseThemetokens. As per coding guidelines: "If a component uses colors, use colors from the theme if possible."♻️ Example fix (adjust tokens to match closest theme values)
+import { baseTheme } from "@/shared-module/common/styles" const stageMonthBlockStyles = css` min-width: 84px; border-radius: 12px; - border: 1px solid `#d9dde4`; + border: 1px solid ${baseTheme.colors.gray[300]}; - background: white; + background: ${baseTheme.colors.clear[100]}; ... ` const stageMonthBlockMonthStyles = css` ... - color: `#415167`; + color: ${baseTheme.colors.gray[700]}; ... ` const stageMonthBlockYearStyles = css` ... - color: `#6a7686`; + color: ${baseTheme.colors.gray[500]}; ... ` const stageMonthBlockIconStyles = css` ... - color: `#2d7b4f`; + color: ${baseTheme.colors.green[600]}; ... `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/manage/course-plans/`[id]/schedule/components/MonthBlock.tsx around lines 39 - 72, Replace hardcoded hex color literals in stageMonthBlockStyles, stageMonthBlockMonthStyles, stageMonthBlockYearStyles, and stageMonthBlockIconStyles with theme tokens from baseTheme: import or reference baseTheme (or theme.colors) and swap `#d9dde4` → baseTheme.colors.border (or neutral variant), white → baseTheme.colors.background or baseTheme.colors.surface, `#415167` → baseTheme.colors.textPrimary, `#6a7686` → baseTheme.colors.textSecondary, and `#2d7b4f` → baseTheme.colors.success/green (or closest green token); update the CSS template strings to use those tokens so all color values come from the theme rather than hardcoded hexes.services/main-frontend/src/app/manage/course-plans/[id]/schedule/components/ScheduleWizardPage.tsx (1)
36-42: Use theme tokens instead of raw surface/border colors.
sectionStylesandwizardStepCardStylesstill use hard-codedwhite/#d9dde4. Switching these tobaseThemetokens keeps the component consistent with theming.♻️ Suggested patch
const sectionStyles = css` - background: white; - border: 1px solid `#d9dde4`; + background: ${baseTheme.colors.clear[100]}; + border: 1px solid ${baseTheme.colors.gray[200]}; border-radius: 12px; padding: 1rem; margin-bottom: 1rem; ` const wizardStepCardStyles = css` - background: white; + background: ${baseTheme.colors.clear[100]}; border-radius: 16px;As per coding guidelines "If a component uses colors, use colors from the theme if possible."
Also applies to: 45-46
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/src/app/manage/course-plans/`[id]/schedule/components/ScheduleWizardPage.tsx around lines 36 - 42, sectionStyles and wizardStepCardStyles use hard-coded colors (white and `#d9dde4`); replace those literals with the theme tokens from baseTheme (surface/background and border colors) so the component follows theming. Locate the consts sectionStyles and wizardStepCardStyles in ScheduleWizardPage.tsx and swap the background and border values to use baseTheme (or theme) tokens (e.g., surface/base and border/neutral tokens used elsewhere in the app) instead of "white" and "#d9dde4"; ensure imports for baseTheme or theme hooks are added if missing and update both occurrences mentioned in the review.services/main-frontend/src/app/manage/course-plans/[id]/schedule/components/StageCard.tsx (2)
14-19: Use theme color token for the stage card background.
#fbfcfdshould be replaced with a theme color token to keep the component compatible with centralized theming.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/manage/course-plans/`[id]/schedule/components/StageCard.tsx around lines 14 - 19, Replace the hardcoded background color in stageCardStyles with a theme token: change background: `#fbfcfd` to a baseTheme color (for example background: baseTheme.colors.gray[50] or baseTheme.colors.white) so the StageCard component uses centralized theming; ensure baseTheme is referenced/imported the same way as the existing border color usage in stageCardStyles in StageCard.tsx.
26-28: Replace raw@mediawith shared responsive helper.This file uses a direct media query; please align it with the shared
respondToOrLargerhelper used across the frontend style system.As per coding guidelines "If a component uses media queries, use the respondToOrLarger function."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/src/app/manage/course-plans/`[id]/schedule/components/StageCard.tsx around lines 26 - 28, The component uses a raw media query (`@media` (max-width: 900px) { flex-direction: column; }) inside StageCard; replace it by importing and using the shared respondToOrLarger helper and apply the same rule via that helper (e.g., wrap the flex-direction: column rule with respondToOrLarger(...) for the same breakpoint) in the StageCard styled component so the style system is consistent across the frontend.services/main-frontend/src/app/manage/course-plans/[id]/schedule/scheduleStageTransforms.ts (1)
21-23: Avoid hard-coded stage count in timeline validation.
buildMonthTimelinechecksstages.length !== 5while the rest of the module already relies on shared stage constants. Tie this check to the shared constant to prevent drift.♻️ Suggested patch
- if (stages.length !== 5) { + if (stages.length !== STAGE_ORDER.length) { return null }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/src/app/manage/course-plans/`[id]/schedule/scheduleStageTransforms.ts around lines 21 - 23, In buildMonthTimeline replace the hard-coded stages.length !== 5 check with a reference to the shared stage constant used elsewhere (e.g., import and compare against the shared STAGES array length or a STAGE_COUNT constant) so the validation uses the canonical source of truth; update the check from stages.length !== 5 to stages.length !== <shared constant or STAGES.length> and import the constant at the top of the module to prevent future drift (refer to buildMonthTimeline and the stages variable for location).services/main-frontend/src/app/manage/course-plans/[id]/schedule/components/steps/ScheduleEditorStep.tsx (1)
36-40: Use a theme token for validation error color.
validationErrorStylesuses a hard-coded color (#8d2323). Please switch it to abaseThemecolor token for consistent theming.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/manage/course-plans/`[id]/schedule/components/steps/ScheduleEditorStep.tsx around lines 36 - 40, validationErrorStyles currently hard-codes color "#8d2323"; update it to use the theme token instead (e.g., use baseTheme.colors.danger or the project’s equivalent token) so the component follows theming guidelines: replace the color value in validationErrorStyles with the baseTheme color token, and add the necessary import for baseTheme (or Theme) at the top of the file so the symbol validationErrorStyles references a theme color rather than a hex literal.services/main-frontend/src/app/manage/course-plans/[id]/schedule/hooks/useScheduleWizardController.ts (1)
33-77: Add short intent docs for helpers and the exported controller hookThis new module exposes multiple utility functions and a public hook/type without minimal intent-level documentation. Please add concise doc comments (especially for the exported hook API) to clarify responsibilities and assumptions.
As per coding guidelines, "Require consistent, minimal function documentation... Public interfaces should have clearer and more comprehensive documentation."
Also applies to: 77-268
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/src/app/manage/course-plans/`[id]/schedule/hooks/useScheduleWizardController.ts around lines 33 - 77, Add concise intent doc comments for each helper and the public hook: document what todayMonthValue() returns and its format, monthToStartsOnDate(month) behavior and empty-string edge case, and the mapping functions atomStepToId(), stepIdToAtomStep(), and stepIndex() (describe input types ScheduleWizardStep / ScheduleWizardStepId and expected outputs). Also add a clear doc comment for the exported useScheduleWizardController(planId: string) hook describing its responsibility, returned values/side effects, assumptions about planId, and any invariants (e.g., step id mapping). Reference the symbols todayMonthValue, monthToStartsOnDate, atomStepToId, stepIdToAtomStep, stepIndex, useScheduleWizardController, ScheduleWizardStep, and ScheduleWizardStepId in the comments.
🤖 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/models/src/course_designer_plans.rs`:
- Around line 485-492: The public persistence function replace_schedule_for_user
currently accepts stages without validating schedule invariants; add validation
inside this model API to enforce ordering, no gaps, and no overlaps before
writing. In replace_schedule_for_user (and any helper it calls) validate the
stages slice (CourseDesignerScheduleStageInput elements) for contiguous timeline
(start/end ordering or sequence indices), ensure each stage's start is >=
previous end (or indices are strictly increasing with no gaps), and return a
ModelResult error if invariants fail; perform these checks immediately after
beginning the transaction and before any DB mutations so other callers cannot
persist invalid schedules.
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/schedule/components/MonthBlock.tsx:
- Around line 100-101: The month labels are currently rendered with
format(month.date, "MMMM") / format(month.date, "yyyy") which ignores the app's
active language; update MonthBlock.tsx to use the app i18n locale: import and
call useTranslation() (or reuse existing i18n) to get i18n.language, map that
language to the corresponding date-fns locale object (e.g.,
dateLocaleForCurrentLanguage) and pass it as the third argument to format, e.g.,
format(month.date, "MMMM", { locale: dateLocaleForCurrentLanguage }) and
format(month.date, "yyyy", { locale: dateLocaleForCurrentLanguage }) so month
and year are localized for en/fi/uk/sv.
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/schedule/components/ScheduleWizardProgress.tsx:
- Around line 135-137: The valueLabel passed to the progress (currently `Step
${currentStepIndex + 1} of ${SCHEDULE_WIZARD_STEPS.length}`) is hard-coded
English and must be localized: import and call the useTranslation hook in the
ScheduleWizardProgress component, add a translation key (e.g.
"scheduleWizard.progress" with placeholders for current and total) and use
t('scheduleWizard.progress', { current: currentStepIndex + 1, total:
SCHEDULE_WIZARD_STEPS.length }) to produce the localized string, then pass that
result into the `valueLabel` prop so `aria-valuetext` is translated for screen
readers.
- Around line 106-116: Replace the custom srOnlyStyles definition and its usage
in ScheduleWizardProgress with react-aria's VisuallyHidden: remove the
srOnlyStyles constant, import { VisuallyHidden } from "react-aria", and wrap the
progress label block that currently uses srOnlyStyles (where you reference
progressBar.progressBarProps and render t("course-plans-wizard-progress-label")
with progressBar.progressBarProps["aria-valuetext"]) inside <VisuallyHidden> so
the same accessible text is rendered without the custom CSS.
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/schedule/components/steps/NameStep.tsx:
- Around line 21-35: The focus styles are nested incorrectly in the styled block
for input[type="text"] in NameStep.tsx: the nested :focus becomes a descendant
selector instead of targeting the input itself; update the nested selector
inside the input[type="text"] block from :focus to &:focus so the focus border
and box-shadow are applied to the input element itself (locate the CSS block
referencing input[type="text"] in NameStep.tsx and change the nested focus
selector accordingly).
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/schedule/components/steps/SetupStep.tsx:
- Around line 29-44: The focus styles inside the CSS block for the selectors
input[type="month"], select are written as :focus which becomes a descendant
selector and won't apply to the element itself; update the nested selector to
use &:focus so the focus ring (border-color ${baseTheme.colors.green[500]} and
box-shadow ${baseTheme.colors.green[100]}) is applied to the focused
input/select element in SetupStep.tsx.
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/schedule/hooks/useScheduleWizardController.ts:
- Around line 85-89: The schedule wizard initializes courseSize to "medium" and
resets step incorrectly; update useScheduleWizardController to hydrate state
from the fetched plan once the query result is available: when plan data is
loaded, call setCourseSize(plan.courseSize) (or map the plan field to
CourseDesignerCourseSize), setPlanName(plan.name), and
setInitializedFromQuery(plan.id) as needed; determine the initial wizard step by
checking plan content (if plan.name or setup exists but plan.stages is empty,
set the step to "setup" instead of "name") and avoid overwriting these hydrated
values on subsequent renders—use the initializedFromQuery flag to run this
hydration only once.
- Around line 123-134: The hook useScheduleWizardController currently updates
atoms when planQuery.data.stages has items but does not clear the
draftStagesAtomFamily(planId) when stages is empty; update the else branch to
explicitly reset the draft stages for the current plan (call the same setter
used above, e.g. setDraftStages([]) or the atom reset for
draftStagesAtomFamily(planId)) in addition to setting the wizard step/direction
so stale stage data cannot leak into an empty plan.
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/schedule/scheduleConstants.ts:
- Around line 3-9: Change SCHEDULE_STAGE_ORDER to be readonly at the type level
so it cannot be mutated: replace the mutable CourseDesignerStage[] declaration
with a readonly type (e.g. export const SCHEDULE_STAGE_ORDER: readonly
CourseDesignerStage[] = ["Analysis", "Design", "Development", "Implementation",
"Evaluation"]; or use export const SCHEDULE_STAGE_ORDER =
["Analysis","Design","Development","Implementation","Evaluation"] as const if
you need a tuple of literal types). Update the SCHEDULE_STAGE_ORDER declaration
(and keep the same symbol name) so downstream code sees an immutable sequence.
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/schedule/scheduleMappers.ts:
- Around line 74-82: The mapper currently always returns an entry per
SCHEDULE_STAGE_ORDER even when a stage has no months; update the logic in the
function that maps SCHEDULE_STAGE_ORDER to stage cards (referencing
SCHEDULE_STAGE_ORDER, byStage and getStageMonths in scheduleMappers.ts) to skip
stages with no months — e.g. compute months = stageInput ?
getStageMonths(stageInput) : [] and only emit the card when months.length > 0
(filter out empty-month results) so empty stage cards are not produced.
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/schedule/scheduleValidation.ts:
- Around line 31-56: validateScheduleStages currently only checks length, ranges
and contiguity but not duplicate or missing stage identities; update
validateScheduleStages to verify the set of stage IDs in the incoming stages
array (inspect each stage.stage) contains exactly the expected distinct IDs (no
duplicates and no missing ones) by building a Set of stage.stage values and
comparing its size and membership against SCHEDULE_STAGE_COUNT or the canonical
list of expected stage IDs, and return explicit validation issues (e.g.,
"duplicate_stage" with the offending stage id or "missing_stage" with the
missing id(s)) when mismatches are found before proceeding with range/contiguity
checks.
In
`@services/main-frontend/src/app/manage/course-plans/components/CoursePlanCard.tsx`:
- Around line 12-16: The component CoursePlanCard uses hard-coded colors
(border: `#d9dde4`, background: white, and text color `#5d6776`) — replace those
literals with theme tokens by reading from the app theme (e.g.,
props.theme.colors or useTheme()) inside the styled component or style block;
update the border to theme.colors.border (or equivalent token like
colors.gray[200]), background to theme.colors.background/white, and the text
color to theme.colors.textSecondary (or the matching token), and ensure useTheme
is imported/typed if necessary so the styled component references theme tokens
instead of hex values.
In
`@services/main-frontend/src/app/manage/course-plans/components/CoursePlansListPage.tsx`:
- Around line 46-55: The toast mutation call for createPlanMutation (the
useToastMutation invocation) is missing user-facing labels so success/error
toasts show empty headers; update the options object passed to useToastMutation
for createPlanMutation to include translated strings for successHeader and
errorHeader (and optionally successMessage/errorMessage) using the app's i18n
helper (e.g., t("...")) before calling createCourseDesignerPlan, so that
onSuccess/onError toasts display meaningful text and then continue to invalidate
queries and route to coursePlanScheduleRoute(plan.id).
In `@services/main-frontend/src/app/manage/course-plans/coursePlanRoutes.ts`:
- Around line 1-4: coursePlanScheduleRoute currently interpolates planId
directly which can break routes for IDs with reserved URL characters; update the
function to encode the dynamic segment using encodeURIComponent(planId) and add
a one- or two-line JSDoc/comment above the exported function describing its
intent (e.g., "Return the schedule route for a course plan by id") so callers
know it's a URL helper; reference the function name coursePlanScheduleRoute and
the parameter planId when making the change.
In `@services/main-frontend/src/services/backend/courseDesigner.ts`:
- Around line 69-71: CreateCourseDesignerPlanRequest (and any other interfaces
in this file that currently declare name?: string | null) mark the name field as
required (name: string | null) to match the generated contract and avoid sending
invalid payloads; locate all occurrences of name?: string | null (e.g.,
CreateCourseDesignerPlanRequest) and change them to name: string | null, then
update any call sites or tests that rely on omitting name to provide an explicit
string or null value.
In `@shared-module/packages/common/src/locales/fi/main-frontend.json`:
- Around line 290-291: The Finnish strings in the course-plans wizard are
inconsistent: replace "Työn alkamiskuukausi" (key
course-plans-wizard-starts-on-month-label) and "Nimeä projekti" (key
course-plans-wizard-step-name) to use the same term as the rest of the wizard
(e.g. "Kurssin alkamiskuukausi" and "Nimeä kurssi") so all course-plans-* keys
consistently use "kurssi"; if instead the flow truly refers to a project/work,
update the other course-plans keys to match that term across locales.
---
Nitpick comments:
In
`@services/headless-lms/migrations/20260223110000_add_course_designer_planning_tables.up.sql`:
- Around line 86-124: Add explicit b-tree indexes to speed lookups by plan and
soft-delete filtering: create an index on course_designer_plan_stages
(course_designer_plan_id, deleted_at) to optimize fetching stages for a plan
(alongside the existing UNIQUE constraint
course_designer_plan_stages_plan_stage_unique), add an index on
course_designer_plan_stage_tasks (course_designer_plan_stage_id) to speed task
lookups by stage, and ensure course_designer_plan_events has an index on
course_designer_plan_id (or add one if missing) to optimize event queries;
implement these as CREATE INDEX statements in the migration after the table
definitions.
In
`@services/headless-lms/models/.sqlx/query-c8d0042fd40cd20e963f96eb070e84714eeb8653571856fb8a013a87cff93980.json`:
- Line 3: The INSERT into course_designer_plan_members should be made idempotent
to avoid duplicate key errors on retries: change the query that currently does
INSERT INTO course_designer_plan_members (course_designer_plan_id, user_id)
VALUES ($1, $2) to include conflict handling, e.g. INSERT ... ON CONFLICT
(course_designer_plan_id, user_id) DO NOTHING (or ON CONFLICT DO NOTHING if a
suitable unique constraint exists) so duplicate inserts become no-ops.
In `@services/headless-lms/models/src/course_designer_plans.rs`:
- Around line 116-812: Add concise Rustdoc comments for each public function to
clarify intent, parameters and returns: add /// one-line summaries and short
param/return notes above fixed_stage_order, validate_schedule_input,
build_schedule_suggestion, create_plan, list_plans_for_user, get_plan_for_user,
get_plan_members_for_user, get_plan_stages_for_user, get_plan_details_for_user,
replace_schedule_for_user, finalize_schedule_for_user, and no_gap_between; keep
them minimal (single-sentence intent plus brief param/return or error behavior),
reference relevant types (e.g. CourseDesignerStage,
CourseDesignerScheduleStageInput, CourseDesignerCourseSize, PgConnection) and
mention error conditions (ModelResult/ModelError) where applicable to match
project docs guidelines.
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/schedule/components/MonthBlock.tsx:
- Line 20: The component currently imports and uses forwardRef; update the
MonthBlock component to accept ref as a normal prop instead: remove the
forwardRef import, change the component signature (e.g., MonthBlock = (props) =>
or destructure including ref) so it receives props.ref, and pass props.ref (or
the destructured ref) to the DOM element(s) that previously received the
forwarded ref; also update any export that wrapped the component with forwardRef
to export the plain component and remove the forwardRef import/usage. Ensure all
internal references to the forwarded ref (within MonthBlock) use the prop name
you chose and adjust any call sites that passed refs accordingly.
- Around line 39-72: Replace hardcoded hex color literals in
stageMonthBlockStyles, stageMonthBlockMonthStyles, stageMonthBlockYearStyles,
and stageMonthBlockIconStyles with theme tokens from baseTheme: import or
reference baseTheme (or theme.colors) and swap `#d9dde4` → baseTheme.colors.border
(or neutral variant), white → baseTheme.colors.background or
baseTheme.colors.surface, `#415167` → baseTheme.colors.textPrimary, `#6a7686` →
baseTheme.colors.textSecondary, and `#2d7b4f` → baseTheme.colors.success/green (or
closest green token); update the CSS template strings to use those tokens so all
color values come from the theme rather than hardcoded hexes.
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/schedule/components/ScheduleWizardPage.tsx:
- Around line 36-42: sectionStyles and wizardStepCardStyles use hard-coded
colors (white and `#d9dde4`); replace those literals with the theme tokens from
baseTheme (surface/background and border colors) so the component follows
theming. Locate the consts sectionStyles and wizardStepCardStyles in
ScheduleWizardPage.tsx and swap the background and border values to use
baseTheme (or theme) tokens (e.g., surface/base and border/neutral tokens used
elsewhere in the app) instead of "white" and "#d9dde4"; ensure imports for
baseTheme or theme hooks are added if missing and update both occurrences
mentioned in the review.
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/schedule/components/StageCard.tsx:
- Around line 14-19: Replace the hardcoded background color in stageCardStyles
with a theme token: change background: `#fbfcfd` to a baseTheme color (for example
background: baseTheme.colors.gray[50] or baseTheme.colors.white) so the
StageCard component uses centralized theming; ensure baseTheme is
referenced/imported the same way as the existing border color usage in
stageCardStyles in StageCard.tsx.
- Around line 26-28: The component uses a raw media query (`@media` (max-width:
900px) { flex-direction: column; }) inside StageCard; replace it by importing
and using the shared respondToOrLarger helper and apply the same rule via that
helper (e.g., wrap the flex-direction: column rule with respondToOrLarger(...)
for the same breakpoint) in the StageCard styled component so the style system
is consistent across the frontend.
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/schedule/components/steps/ScheduleEditorStep.tsx:
- Around line 36-40: validationErrorStyles currently hard-codes color "#8d2323";
update it to use the theme token instead (e.g., use baseTheme.colors.danger or
the project’s equivalent token) so the component follows theming guidelines:
replace the color value in validationErrorStyles with the baseTheme color token,
and add the necessary import for baseTheme (or Theme) at the top of the file so
the symbol validationErrorStyles references a theme color rather than a hex
literal.
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/schedule/components/steps/SetupStep.tsx:
- Around line 54-116: Replace the manual controlled inputs in the SetupStep
component with react-hook-form: create a useForm inside SetupStep, register the
"courseSize" select and "startsOnMonth" month input (use register with required
validation for startsOnMonth), replace direct value/onChange usage with the
register bindings and read current values from formState or getValues, and wire
the Continue button to handleSubmit to call the existing onContinue prop only
when the form is valid (respecting isGeneratingSuggestion to disable as before);
keep onBack unchanged and still call onCourseSizeChange/onStartsOnMonthChange
inside a form watch/handleSubmit or a useEffect if parent needs immediate
updates.
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/schedule/hooks/useScheduleWizardController.ts:
- Around line 33-77: Add concise intent doc comments for each helper and the
public hook: document what todayMonthValue() returns and its format,
monthToStartsOnDate(month) behavior and empty-string edge case, and the mapping
functions atomStepToId(), stepIdToAtomStep(), and stepIndex() (describe input
types ScheduleWizardStep / ScheduleWizardStepId and expected outputs). Also add
a clear doc comment for the exported useScheduleWizardController(planId: string)
hook describing its responsibility, returned values/side effects, assumptions
about planId, and any invariants (e.g., step id mapping). Reference the symbols
todayMonthValue, monthToStartsOnDate, atomStepToId, stepIdToAtomStep, stepIndex,
useScheduleWizardController, ScheduleWizardStep, and ScheduleWizardStepId in the
comments.
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/schedule/scheduleAtoms.ts:
- Around line 8-36: The atom families draftStagesAtomFamily,
addMonthToStageAtomFamily, removeMonthFromStageAtomFamily, and
scheduleWizardStepAtomFamily are cached indefinitely and should be removed when
a plan view unmounts to avoid memory growth; update the plan-level component's
cleanup (useEffect return or equivalent unmount handler) to call
draftStagesAtomFamily.remove(planId), addMonthToStageAtomFamily.remove(planId),
removeMonthFromStageAtomFamily.remove(planId), and
scheduleWizardStepAtomFamily.remove(planId) for the current planId so the
per-plan atoms are released when the view is torn down.
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/schedule/scheduleStageTransforms.ts:
- Around line 21-23: In buildMonthTimeline replace the hard-coded stages.length
!== 5 check with a reference to the shared stage constant used elsewhere (e.g.,
import and compare against the shared STAGES array length or a STAGE_COUNT
constant) so the validation uses the canonical source of truth; update the check
from stages.length !== 5 to stages.length !== <shared constant or STAGES.length>
and import the constant at the top of the module to prevent future drift (refer
to buildMonthTimeline and the stages variable for location).
In `@services/main-frontend/src/app/manage/course-plans/coursePlanQueryKeys.ts`:
- Around line 1-4: Add a short doc comment above the exported
coursePlanQueryKeys object describing that it is the shared cache-key surface
used for course plan React Query cache keys, noting its intent (stable, public
API) and warning not to change the key shape lightly; mention the two helpers
(list and detail(planId: string)) and their return shapes so callers know they
produce immutable tuple keys. Ensure the comment sits immediately above the
coursePlanQueryKeys export and refers to the list and detail helpers by name.
In `@services/main-frontend/src/services/backend/courseDesigner.ts`:
- Around line 87-128: Add brief JSDoc-style docs to each exported API wrapper
(createCourseDesignerPlan, listCourseDesignerPlans, getCourseDesignerPlan,
generateCourseDesignerScheduleSuggestion, saveCourseDesignerSchedule,
finalizeCourseDesignerSchedule) that state the intent/purpose, describe
parameters (e.g., planId, payload) and the returned type, and note any side
effects (HTTP method and endpoint) or errors thrown; keep each comment 1–3 lines
and placed immediately above the function so maintainers can quickly understand
usage and expected return values.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
services/main-frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (52)
eslint.config.jsservices/headless-lms/migrations/20260223110000_add_course_designer_planning_tables.down.sqlservices/headless-lms/migrations/20260223110000_add_course_designer_planning_tables.up.sqlservices/headless-lms/models/.sqlx/query-0627e79ff4710afe3c10d4375b1ba0067b035a45be213eb80e7301361d1d4429.jsonservices/headless-lms/models/.sqlx/query-0ea8cc60a82bfb68f0dfc1717c77610c998036b5359ddf9f5ac311862967823d.jsonservices/headless-lms/models/.sqlx/query-2194bc23a7b6ed52a4205be56b8033004eca24a7b62694f2a138a38a9e4cc519.jsonservices/headless-lms/models/.sqlx/query-2dadd4e83ccd5f82e12d01dfe372e250309efb4539de0b36e081e153fb936877.jsonservices/headless-lms/models/.sqlx/query-413005008c2b7eb57fbb4cbe35f9a400118ab5eb91e327f4ab293134e86621c1.jsonservices/headless-lms/models/.sqlx/query-48335dfa0348b619bc7f835e4a3b3aa05b6ba9345a44ceed558692513654702d.jsonservices/headless-lms/models/.sqlx/query-6ac247dd9f8f71bf96a105b23ffb2622486ef4c22bec890635b623a2fa21c37b.jsonservices/headless-lms/models/.sqlx/query-92a1fcd6a7140887b2349bc5112b0d3e7469e388a07a31ef6a1b919e24acd7ce.jsonservices/headless-lms/models/.sqlx/query-a01d1b5421f9a4767af3e65457a5b03e3fe4d697687f1438eb516441b2f053c1.jsonservices/headless-lms/models/.sqlx/query-a5c698b800871fe0093d58d2d6fc01ddbe00c5aa81bdf66d390f4d923d48ec82.jsonservices/headless-lms/models/.sqlx/query-bf61544e2c1844e3001b89360a78a3c6b0237f1b87a6e3761d7dbddf3c53b0ca.jsonservices/headless-lms/models/.sqlx/query-c8d0042fd40cd20e963f96eb070e84714eeb8653571856fb8a013a87cff93980.jsonservices/headless-lms/models/.sqlx/query-d108ba195f2512ce64f4300dacdfbb409d90598e2d0e465cc99eacfce14711e1.jsonservices/headless-lms/models/.sqlx/query-d1615df3085611c8a68aa4669bb431b6e5b27544883dc99441f9cfbc810fe887.jsonservices/headless-lms/models/src/course_designer_plans.rsservices/headless-lms/models/src/lib.rsservices/headless-lms/server/src/controllers/main_frontend/course_designer.rsservices/headless-lms/server/src/controllers/main_frontend/mod.rsservices/headless-lms/server/src/ts_binding_generator.rsservices/main-frontend/package.jsonservices/main-frontend/src/app/manage/course-plans/[id]/schedule/__tests__/scheduleStageTransforms.test.tsservices/main-frontend/src/app/manage/course-plans/[id]/schedule/components/MonthBlock.tsxservices/main-frontend/src/app/manage/course-plans/[id]/schedule/components/ScheduleWizardPage.tsxservices/main-frontend/src/app/manage/course-plans/[id]/schedule/components/ScheduleWizardProgress.tsxservices/main-frontend/src/app/manage/course-plans/[id]/schedule/components/StageCard.tsxservices/main-frontend/src/app/manage/course-plans/[id]/schedule/components/steps/NameStep.tsxservices/main-frontend/src/app/manage/course-plans/[id]/schedule/components/steps/ScheduleEditorStep.tsxservices/main-frontend/src/app/manage/course-plans/[id]/schedule/components/steps/SetupStep.tsxservices/main-frontend/src/app/manage/course-plans/[id]/schedule/hooks/useScheduleWizardController.tsservices/main-frontend/src/app/manage/course-plans/[id]/schedule/page.tsxservices/main-frontend/src/app/manage/course-plans/[id]/schedule/scheduleAtoms.tsservices/main-frontend/src/app/manage/course-plans/[id]/schedule/scheduleConstants.tsservices/main-frontend/src/app/manage/course-plans/[id]/schedule/scheduleMappers.tsservices/main-frontend/src/app/manage/course-plans/[id]/schedule/scheduleStageTransforms.tsservices/main-frontend/src/app/manage/course-plans/[id]/schedule/scheduleValidation.tsservices/main-frontend/src/app/manage/course-plans/components/CoursePlanCard.tsxservices/main-frontend/src/app/manage/course-plans/components/CoursePlanList.tsxservices/main-frontend/src/app/manage/course-plans/components/CoursePlansListPage.tsxservices/main-frontend/src/app/manage/course-plans/coursePlanQueryKeys.tsservices/main-frontend/src/app/manage/course-plans/coursePlanRoutes.tsservices/main-frontend/src/app/manage/course-plans/page.tsxservices/main-frontend/src/services/backend/courseDesigner.tsshared-module/packages/common/src/bindings.guard.tsshared-module/packages/common/src/bindings.tsshared-module/packages/common/src/locales/ar/main-frontend.jsonshared-module/packages/common/src/locales/en/main-frontend.jsonshared-module/packages/common/src/locales/fi/main-frontend.jsonshared-module/packages/common/src/locales/sv/main-frontend.jsonshared-module/packages/common/src/locales/uk/main-frontend.json
Comment on lines
+485
to
+492
| pub async fn replace_schedule_for_user( | ||
| conn: &mut PgConnection, | ||
| plan_id: Uuid, | ||
| user_id: Uuid, | ||
| name: Option<String>, | ||
| stages: &[CourseDesignerScheduleStageInput], | ||
| ) -> ModelResult<CourseDesignerPlanDetails> { | ||
| let mut tx = conn.begin().await?; |
Contributor
There was a problem hiding this comment.
Enforce schedule invariants inside the model write API.
Line 485 defines a public persistence function, but stages are not validated here. Right now invariants are enforced only in the controller path, so any other caller can persist invalid order/gaps/overlaps; downstream finalization currently checks only count.
💡 Proposed fix
pub async fn replace_schedule_for_user(
conn: &mut PgConnection,
plan_id: Uuid,
user_id: Uuid,
name: Option<String>,
stages: &[CourseDesignerScheduleStageInput],
) -> ModelResult<CourseDesignerPlanDetails> {
+ validate_schedule_input(stages)?;
let mut tx = conn.begin().await?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/headless-lms/models/src/course_designer_plans.rs` around lines 485 -
492, The public persistence function replace_schedule_for_user currently accepts
stages without validating schedule invariants; add validation inside this model
API to enforce ordering, no gaps, and no overlaps before writing. In
replace_schedule_for_user (and any helper it calls) validate the stages slice
(CourseDesignerScheduleStageInput elements) for contiguous timeline (start/end
ordering or sequence indices), ensure each stage's start is >= previous end (or
indices are strictly increasing with no gaps), and return a ModelResult error if
invariants fail; perform these checks immediately after beginning the
transaction and before any DB mutations so other callers cannot persist invalid
schedules.
Comment on lines
+100
to
+101
| <span className={stageMonthBlockMonthStyles}>{format(month.date, "MMMM")}</span> | ||
| <span className={stageMonthBlockYearStyles}>{format(month.date, "yyyy")}</span> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the MonthBlock.tsx file and examine it
find . -name "MonthBlock.tsx" -type fRepository: rage/secret-project-331
Length of output: 159
🏁 Script executed:
# Search for StageMonth type definition and related types
rg -n "type StageMonth|interface StageMonth" --type tsRepository: rage/secret-project-331
Length of output: 180
🏁 Script executed:
# Look for where month.label is set or constructed
rg -n "\.label" --type ts -B2 -A2 | head -100Repository: rage/secret-project-331
Length of output: 9260
🏁 Script executed:
# Check if the app has i18n/locale configuration
find . -name "*i18n*" -o -name "*locale*" -o -name "*translation*" | grep -E "\.(ts|tsx|js|json)$" | head -20Repository: rage/secret-project-331
Length of output: 720
🏁 Script executed:
# Read MonthBlock.tsx to see the context around lines 100-101
cat -n services/main-frontend/src/app/manage/course-plans/[id]/schedule/components/MonthBlock.tsx | head -150Repository: rage/secret-project-331
Length of output: 3115
🏁 Script executed:
# Read the StageMonth type definition and surrounding code
cat -n services/main-frontend/src/app/manage/course-plans/[id]/schedule/scheduleMappers.ts | head -100Repository: rage/secret-project-331
Length of output: 2782
🏁 Script executed:
# Check if the component uses useTranslation
rg -n "useTranslation\|i18n\|locale" services/main-frontend/src/app/manage/course-plans/[id]/schedule/components/MonthBlock.tsxRepository: rage/secret-project-331
Length of output: 49
🏁 Script executed:
# Look for how month data is created in scheduleMappers
rg -n "StageMonth\|label" services/main-frontend/src/app/manage/course-plans/[id]/schedule/scheduleMappers.ts -A2 -B2Repository: rage/secret-project-331
Length of output: 49
🏁 Script executed:
# Check if date-fns locales are being used elsewhere in the app
rg -n "date-fns.*locale\|from \"date-fns/locale" --type ts --type tsxRepository: rage/secret-project-331
Length of output: 94
🏁 Script executed:
# Check i18n configuration in main-frontend
cat -n services/main-frontend/types/i18next.d.ts | head -50Repository: rage/secret-project-331
Length of output: 823
🏁 Script executed:
# Look for locale/language configuration in the app
rg -n "language\|locale\|i18n\|useTranslation" services/main-frontend/src -l | head -15Repository: rage/secret-project-331
Length of output: 49
🏁 Script executed:
# Check if format() is used with locales elsewhere in the app
rg -n "format\(.+,\s+[a-z]" services/main-frontend/src --type ts --type tsx | head -20Repository: rage/secret-project-331
Length of output: 94
🏁 Script executed:
# Fix the rg command to use proper file extensions
rg -n "date-fns.*locale|from \"date-fns/locale" --type ts -g "*.tsx"Repository: rage/secret-project-331
Length of output: 49
🏁 Script executed:
# Check if the app is locale-aware by checking i18n initialization
rg -n "i18next\|useTranslation\|i18n.use" services/main-frontend/src -g "*.ts" -g "*.tsx" | head -20Repository: rage/secret-project-331
Length of output: 49
🏁 Script executed:
# Check the main-frontend locales directory
find services/main-frontend -type d -name "locales"Repository: rage/secret-project-331
Length of output: 49
🏁 Script executed:
# Look for language/locale configuration files
find services/main-frontend -type f \( -name "i18n.*" -o -name "locale*" -o -name "lang*" \) | head -20Repository: rage/secret-project-331
Length of output: 134
🏁 Script executed:
# Check for i18n configuration in main-frontend more broadly
find services/main-frontend/src -type f -name "*i18n*" -o -name "*config*" | grep -v node_modules | head -10Repository: rage/secret-project-331
Length of output: 49
🏁 Script executed:
# Look at the language helpers file
cat -n services/main-frontend/src/utils/course-material/languageHelpers.tsRepository: rage/secret-project-331
Length of output: 952
🏁 Script executed:
# Search for how date-fns format is used in main-frontend
rg "format\(" services/main-frontend/src -g "*.tsx" -g "*.ts" -B2 -A2 | head -80Repository: rage/secret-project-331
Length of output: 8112
🏁 Script executed:
# Check if there's any locale provider or config at the app root level
cat services/main-frontend/src/app/layout.tsx | head -100Repository: rage/secret-project-331
Length of output: 1675
🏁 Script executed:
# Check if date-fns locales are imported anywhere in the codebase
rg "from \"date-fns/locale" services/main-frontend/src -g "*.ts" -g "*.tsx" -B2 -A2Repository: rage/secret-project-331
Length of output: 49
🏁 Script executed:
# Check how i18n language code maps to date-fns locales
rg -n "i18n.language\|languageCode" services/main-frontend/src -g "*.ts" -g "*.tsx" | head -20Repository: rage/secret-project-331
Length of output: 49
🏁 Script executed:
# Look for any utility that converts i18n language to date-fns locale
rg -n "getLocale\|toLocale\|dateLocale" services/main-frontend/src -g "*.ts" -g "*.tsx"Repository: rage/secret-project-331
Length of output: 49
🏁 Script executed:
# Check the shared module for any locale utilities
find shared-module -name "*locale*" -o -name "*language*" | grep -E "\.(ts|tsx|js)$" | head -20Repository: rage/secret-project-331
Length of output: 49
🏁 Script executed:
# Check if there's any conversion from i18n language to date-fns locale anywhere
rg -n "locale\|i18n\|language" shared-module/packages/common/src -g "*.ts" -g "*.tsx" | grep -i "date\|locale\|i18n" | head -30Repository: rage/secret-project-331
Length of output: 49
🏁 Script executed:
# Look at the getDir function since it's in layout.tsx
rg -n "getDir" shared-module/packages/common -g "*.ts" -g "*.tsx" -B5 -A10Repository: rage/secret-project-331
Length of output: 6196
🏁 Script executed:
# Check how other parts of the app handle date localization
rg -n "format.*MMMM\|format.*yyyy" services/main-frontend/src -g "*.ts" -g "*.tsx" -B2 -A2 | head -40Repository: rage/secret-project-331
Length of output: 49
🏁 Script executed:
# Look at the learning context - check the time.ts file mentioned
cat -n shared-module/packages/common/src/utils/time.ts | head -50Repository: rage/secret-project-331
Length of output: 2146
🏁 Script executed:
# Check how the app currently handles locale/language for i18n
rg -n "i18n.language\|useTranslation" services/main-frontend/src/app/manage/course-plans -g "*.ts" -g "*.tsx" -B2 -A2 | head -40Repository: rage/secret-project-331
Length of output: 49
🏁 Script executed:
# Check if MonthBlock is used with i18n provider context
cat -n services/main-frontend/src/app/manage/course-plans/[id]/schedule/components/MonthBlock.tsx | tail -10Repository: rage/secret-project-331
Length of output: 417
🏁 Script executed:
# Verify what month.label contains - check if it's same as the format calls
rg -n "month.label\|StageMonth" services/main-frontend/src/app/manage/course-plans -g "*.ts" -g "*.tsx" -B2 -A2 | head -50Repository: rage/secret-project-331
Length of output: 49
🏁 Script executed:
# Check the coding guidelines - verify if .tsx files must use useTranslation
cat -n services/main-frontend/src/app/manage/course-plans/[id]/schedule/components/MonthBlock.tsxRepository: rage/secret-project-331
Length of output: 3115
🏁 Script executed:
# Check if there's a date-fns locale mapping for the app's supported languages
rg -n "date-fns\|locale" services/main-frontend/src -g "*.ts" -g "*.tsx" | grep -i "en\|fi\|uk\|sv" | head -20Repository: rage/secret-project-331
Length of output: 49
🏁 Script executed:
# Look for other usages of month names or date formatting in schedule components
find services/main-frontend/src/app/manage/course-plans/[id]/schedule -name "*.tsx" -type fRepository: rage/secret-project-331
Length of output: 819
🏁 Script executed:
# Check if there's any locale provider or context setup for the schedule page
cat -n services/main-frontend/src/app/manage/course-plans/[id]/schedule/page.tsx 2>/dev/null | head -50Repository: rage/secret-project-331
Length of output: 430
🏁 Script executed:
# Verify what exact format string is used for month.label
rg -n "label.*format\|format.*label" services/main-frontend/src/app/manage/course-plans/[id]/schedule/scheduleMappers.ts -B2 -A2Repository: rage/secret-project-331
Length of output: 49
🏁 Script executed:
# Check if month names are in the translation files
find services/main-frontend -path "*locales*" -name "*.json" | head -5Repository: rage/secret-project-331
Length of output: 49
🏁 Script executed:
# Look at what's in the translation files
find services/main-frontend -path "*locales*" -name "*.json" -type fRepository: rage/secret-project-331
Length of output: 49
🏁 Script executed:
# Check the structure of the locales directory
find services/main-frontend -type d -name "locales" -exec find {} -type f \;Repository: rage/secret-project-331
Length of output: 49
🏁 Script executed:
# Verify if the schedule component page or wizard uses i18n
cat -n services/main-frontend/src/app/manage/course-plans/[id]/schedule/components/ScheduleWizardPage.tsx | head -40Repository: rage/secret-project-331
Length of output: 1587
🏁 Script executed:
# Check if there's any utility that handles date-fns locale conversion
rg -n "locale\|enUS\|fi\|uk\|sv" shared-module/packages/common/src -g "*.ts" -g "*.tsx" | grep -i "date\|locale" | head -30Repository: rage/secret-project-331
Length of output: 49
🏁 Script executed:
# Check if there are any locale imports from date-fns in the entire shared module
find shared-module/packages/common/src -type f \( -name "*.ts" -o -name "*.tsx" \) -exec grep -l "date-fns/locale" {} \;Repository: rage/secret-project-331
Length of output: 49
🏁 Script executed:
# Look at the schedule editor step to see if it uses i18n
cat -n services/main-frontend/src/app/manage/course-plans/[id]/schedule/components/steps/ScheduleEditorStep.tsx | head -50Repository: rage/secret-project-331
Length of output: 1617
🏁 Script executed:
# Check what languages are actually supported and if there's a mapping
rg -n "SUPPORTED_LANGUAGES\|en\|fi\|uk\|sv" shared-module/packages/common/src/hooks/useLanguage.tsx -B2 -A2Repository: rage/secret-project-331
Length of output: 49
🏁 Script executed:
# Check if date-fns locale is set globally anywhere
rg -n "setDefaultOptions\|setDefaultLocale\|date-fns" services/main-frontend/src -g "*.ts" -g "*.tsx" | head -30Repository: rage/secret-project-331
Length of output: 49
🏁 Script executed:
# Look for date-fns usage in the shared module
rg -n "from \"date-fns" shared-module/packages/common/src -g "*.ts" -g "*.tsx" | head -20Repository: rage/secret-project-331
Length of output: 372
🏁 Script executed:
# Check if the issue is acknowledged anywhere in the codebase
rg -n "locale\|localization.*date\|i18n.*date" services/main-frontend/src -g "*.ts" -g "*.tsx" | head -20Repository: rage/secret-project-331
Length of output: 49
🏁 Script executed:
# Examine how the app handles language changes - check i18n setup
find services/main-frontend/src -type f -name "*i18n*" -o -name "*init*" | grep -E "\.(ts|tsx|js)$"Repository: rage/secret-project-331
Length of output: 49
🏁 Script executed:
# Final check: Look for any date-fns locale package installation
cat package.json 2>/dev/null | grep date-fnsRepository: rage/secret-project-331
Length of output: 49
🏁 Script executed:
# Check if there are any locale strings for months in the i18n translation files
find shared-module -path "*locales*" -name "*.json" -type f -exec grep -l "january\|february\|January\|February" {} \;Repository: rage/secret-project-331
Length of output: 49
🏁 Script executed:
# Look at the actual scheduleMappers file again to confirm the label format
sed -n '44,60p' services/main-frontend/src/app/manage/course-plans/[id]/schedule/scheduleMappers.tsRepository: rage/secret-project-331
Length of output: 545
🏁 Script executed:
# Check if MonthBlock is the only place where month names are displayed
rg -n "format.*MMMM\|format.*MMM" services/main-frontend/src -g "*.ts" -g "*.tsx"Repository: rage/secret-project-331
Length of output: 49
Use date-fns locales to localize month names based on the app's active language.
format(month.date, "MMMM") renders the full month name in the default locale (en-US), ignoring the app's active language setting. Since the app supports multiple locales (en, fi, uk, sv), month names should be localized.
Import the appropriate date-fns locale and pass it as the third argument to format(), e.g., format(month.date, "MMMM", { locale: dateLocaleForCurrentLanguage }). Alternatively, per the coding guidelines for .tsx files, use the useTranslation hook to integrate with the app's i18n system for all visible user text, and apply the corresponding date-fns locale based on i18n.language.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/schedule/components/MonthBlock.tsx
around lines 100 - 101, The month labels are currently rendered with
format(month.date, "MMMM") / format(month.date, "yyyy") which ignores the app's
active language; update MonthBlock.tsx to use the app i18n locale: import and
call useTranslation() (or reuse existing i18n) to get i18n.language, map that
language to the corresponding date-fns locale object (e.g.,
dateLocaleForCurrentLanguage) and pass it as the third argument to format, e.g.,
format(month.date, "MMMM", { locale: dateLocaleForCurrentLanguage }) and
format(month.date, "yyyy", { locale: dateLocaleForCurrentLanguage }) so month
and year are localized for en/fi/uk/sv.
Comment on lines
+106
to
+116
| const srOnlyStyles = css` | ||
| position: absolute; | ||
| width: 1px; | ||
| height: 1px; | ||
| padding: 0; | ||
| margin: -1px; | ||
| overflow: hidden; | ||
| clip-path: inset(50%); | ||
| white-space: nowrap; | ||
| border: 0; | ||
| ` |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l "services/main-frontend/src/app/manage/course-plans/[id]/schedule/components/ScheduleWizardProgress.tsx"Repository: rage/secret-project-331
Length of output: 173
🏁 Script executed:
sed -n '100,120p' "services/main-frontend/src/app/manage/course-plans/[id]/schedule/components/ScheduleWizardProgress.tsx"Repository: rage/secret-project-331
Length of output: 464
🏁 Script executed:
sed -n '210,220p' "services/main-frontend/src/app/manage/course-plans/[id]/schedule/components/ScheduleWizardProgress.tsx"Repository: rage/secret-project-331
Length of output: 302
🏁 Script executed:
head -50 "services/main-frontend/src/app/manage/course-plans/[id]/schedule/components/ScheduleWizardProgress.tsx"Repository: rage/secret-project-331
Length of output: 1207
🏁 Script executed:
grep -n "VisuallyHidden" "services/main-frontend/src/app/manage/course-plans/[id]/schedule/components/ScheduleWizardProgress.tsx"Repository: rage/secret-project-331
Length of output: 49
🌐 Web query:
react-aria VisuallyHidden v3.45.0 usage pattern progressbar ARIA
💡 Result:
VisuallyHidden in React Aria is meant for visually hiding text while keeping it available to screen readers (it’s not display:none). It’s absolutely positioned, so ensure it has a position: relative/absolute ancestor to avoid unwanted scrollbars. [1]
Recommended patterns with an ARIA progressbar
A) If you’re building a custom progress bar with useProgressBar
- Spread
progressBarPropson the element that represents the ARIA progressbar. - If you don’t render a visible label, you must provide
aria-labeloraria-labelledbyin the props you pass touseProgressBar. [2] - If you do have a label but want it hidden visually, spread
labelPropsonto the label element and wrap it inVisuallyHidden.
import {useProgressBar, VisuallyHidden} from 'react-aria';
function MyProgressBar(props: {label: string; value: number}) {
let {progressBarProps, labelProps} = useProgressBar(props);
return (
<div {...progressBarProps}>
<VisuallyHidden>
<span {...labelProps}>{props.label}</span>
</VisuallyHidden>
{/* your visual track/fill here */}
{/* optional visible value text (already localized/formatted) */}
<span>{progressBarProps['aria-valuetext']}</span>
</div>
);
}useProgressBar also provides the formatted value via progressBarProps['aria-valuetext'] so you can display it visually while keeping AT output consistent. [2]
B) If you’re using React Aria Components <ProgressBar>
- Put a
<Label />inside<ProgressBar>(it’s semantically associated automatically). [3] - If you have no visual label, pass
aria-label/aria-labelledby. [3]
import {ProgressBar, Label} from 'react-aria-components';
import {VisuallyHidden} from 'react-aria';
<ProgressBar value={80}>
<VisuallyHidden><Label>Loading…</Label></VisuallyHidden>
</ProgressBar>Version note (re: “v3.45.0”)
“3.45.0” is a React Spectrum monorepo release (dated October 2, 2025) and doesn’t mean every individual React Aria docs page shows “3.45.0” for each package. [4] The patterns above are the intended React Aria usage per the official docs. [1][2][3]
Sources: [1] React Aria VisuallyHidden docs, [2] React Aria useProgressBar docs, [3] React Aria Components ProgressBar docs, [4] React Spectrum “October 2, 2025 Release – V3” page.
🏁 Script executed:
find . -type f -name "*.tsx" -o -name "*.ts" | xargs grep -l "export.*VisuallyHidden" | head -5Repository: rage/secret-project-331
Length of output: 5970
🏁 Script executed:
grep -r "from.*react-aria" "services/main-frontend/src" -n | grep VisuallyHidden | head -5Repository: rage/secret-project-331
Length of output: 513
🏁 Script executed:
rg "VisuallyHidden" --type tsx --type ts -A 3 -B 1 | head -100Repository: rage/secret-project-331
Length of output: 94
🏁 Script executed:
sed -n '205,219p' "services/main-frontend/src/app/manage/course-plans/[id]/schedule/components/ScheduleWizardProgress.tsx"Repository: rage/secret-project-331
Length of output: 459
Use VisuallyHidden from react-aria instead of custom sr-only CSS.
Replace the srOnlyStyles definition (lines 106–116) and its usage with the VisuallyHidden component from react-aria, per the coding guidelines. Import VisuallyHidden from "react-aria" and wrap the progress label text:
import { VisuallyHidden } from "react-aria"
// Remove srOnlyStyles definition
// In render:
<VisuallyHidden>
<div {...progressBar.progressBarProps}>
{t("course-plans-wizard-progress-label")}: {progressBar.progressBarProps["aria-valuetext"]}
</div>
</VisuallyHidden>This applies to line 214 where the custom CSS class is currently applied.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/schedule/components/ScheduleWizardProgress.tsx
around lines 106 - 116, Replace the custom srOnlyStyles definition and its usage
in ScheduleWizardProgress with react-aria's VisuallyHidden: remove the
srOnlyStyles constant, import { VisuallyHidden } from "react-aria", and wrap the
progress label block that currently uses srOnlyStyles (where you reference
progressBar.progressBarProps and render t("course-plans-wizard-progress-label")
with progressBar.progressBarProps["aria-valuetext"]) inside <VisuallyHidden> so
the same accessible text is rendered without the custom CSS.
Comment on lines
+135
to
+137
| // eslint-disable-next-line i18next/no-literal-string -- step counter, not user-facing copy | ||
| valueLabel: `Step ${currentStepIndex + 1} of ${SCHEDULE_WIZARD_STEPS.length}`, | ||
| }) |
Contributor
There was a problem hiding this comment.
Localize the progress valueLabel for screen readers.
valueLabel is hard-coded in English, so aria-valuetext is not translated.
♻️ Suggested patch
const progressBar = useProgressBar({
label: t("course-plans-wizard-progress-label"),
value: currentStepIndex + 1,
minValue: 1,
maxValue: SCHEDULE_WIZARD_STEPS.length,
- // eslint-disable-next-line i18next/no-literal-string -- step counter, not user-facing copy
- valueLabel: `Step ${currentStepIndex + 1} of ${SCHEDULE_WIZARD_STEPS.length}`,
+ valueLabel: t("course-plans-wizard-progress-step-count", {
+ current: currentStepIndex + 1,
+ total: SCHEDULE_WIZARD_STEPS.length,
+ }),
})As per coding guidelines "**/*.tsx: Make sure to use the useTranslation` hook for all text that is visible to the user."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // eslint-disable-next-line i18next/no-literal-string -- step counter, not user-facing copy | |
| valueLabel: `Step ${currentStepIndex + 1} of ${SCHEDULE_WIZARD_STEPS.length}`, | |
| }) | |
| valueLabel: t("course-plans-wizard-progress-step-count", { | |
| current: currentStepIndex + 1, | |
| total: SCHEDULE_WIZARD_STEPS.length, | |
| }), | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/schedule/components/ScheduleWizardProgress.tsx
around lines 135 - 137, The valueLabel passed to the progress (currently `Step
${currentStepIndex + 1} of ${SCHEDULE_WIZARD_STEPS.length}`) is hard-coded
English and must be localized: import and call the useTranslation hook in the
ScheduleWizardProgress component, add a translation key (e.g.
"scheduleWizard.progress" with placeholders for current and total) and use
t('scheduleWizard.progress', { current: currentStepIndex + 1, total:
SCHEDULE_WIZARD_STEPS.length }) to produce the localized string, then pass that
result into the `valueLabel` prop so `aria-valuetext` is translated for screen
readers.
Comment on lines
+21
to
+35
| input[type="text"] { | ||
| padding: 0.65rem 0.85rem; | ||
| border-radius: 10px; | ||
| border: 1px solid ${baseTheme.colors.gray[300]}; | ||
| font-size: 1rem; | ||
| transition: | ||
| border-color 0.2s, | ||
| box-shadow 0.2s; | ||
|
|
||
| :focus { | ||
| outline: none; | ||
| border-color: ${baseTheme.colors.green[500]}; | ||
| box-shadow: 0 0 0 3px ${baseTheme.colors.green[100]}; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Same :focus without & bug as SetupStep.
input[type="text"] { :focus { ... } } compiles to a descendant selector in Emotion; the focus ring is never applied to the input itself.
🐛 Proposed fix
input[type="text"] {
...
- :focus {
+ &:focus {
outline: none;
border-color: ${baseTheme.colors.green[500]};
box-shadow: 0 0 0 3px ${baseTheme.colors.green[100]};
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| input[type="text"] { | |
| padding: 0.65rem 0.85rem; | |
| border-radius: 10px; | |
| border: 1px solid ${baseTheme.colors.gray[300]}; | |
| font-size: 1rem; | |
| transition: | |
| border-color 0.2s, | |
| box-shadow 0.2s; | |
| :focus { | |
| outline: none; | |
| border-color: ${baseTheme.colors.green[500]}; | |
| box-shadow: 0 0 0 3px ${baseTheme.colors.green[100]}; | |
| } | |
| } | |
| input[type="text"] { | |
| padding: 0.65rem 0.85rem; | |
| border-radius: 10px; | |
| border: 1px solid ${baseTheme.colors.gray[300]}; | |
| font-size: 1rem; | |
| transition: | |
| border-color 0.2s, | |
| box-shadow 0.2s; | |
| &:focus { | |
| outline: none; | |
| border-color: ${baseTheme.colors.green[500]}; | |
| box-shadow: 0 0 0 3px ${baseTheme.colors.green[100]}; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/schedule/components/steps/NameStep.tsx
around lines 21 - 35, The focus styles are nested incorrectly in the styled
block for input[type="text"] in NameStep.tsx: the nested :focus becomes a
descendant selector instead of targeting the input itself; update the nested
selector inside the input[type="text"] block from :focus to &:focus so the focus
border and box-shadow are applied to the input element itself (locate the CSS
block referencing input[type="text"] in NameStep.tsx and change the nested focus
selector accordingly).
Comment on lines
+12
to
+16
| border: 1px solid #d9dde4; | ||
| border-radius: 12px; | ||
| padding: 1rem; | ||
| background: white; | ||
| ` |
Contributor
There was a problem hiding this comment.
Replace hard-coded colors with theme tokens.
Line 12, Line 15, and Line 28 use literal colors (#d9dde4, white, #5d6776). Please switch to theme color values for consistency and design-system alignment.
As per coding guidelines, "If a component uses colors, use colors from the theme if possible."
Also applies to: 27-29
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/main-frontend/src/app/manage/course-plans/components/CoursePlanCard.tsx`
around lines 12 - 16, The component CoursePlanCard uses hard-coded colors
(border: `#d9dde4`, background: white, and text color `#5d6776`) — replace those
literals with theme tokens by reading from the app theme (e.g.,
props.theme.colors or useTheme()) inside the styled component or style block;
update the border to theme.colors.border (or equivalent token like
colors.gray[200]), background to theme.colors.background/white, and the text
color to theme.colors.textSecondary (or the matching token), and ensure useTheme
is imported/typed if necessary so the styled component references theme tokens
instead of hex values.
Comment on lines
46
to
55
| const createPlanMutation = useToastMutation( | ||
| () => createCourseDesignerPlan({}), | ||
| { notify: true, method: "POST" }, | ||
| { | ||
| onSuccess: async (plan) => { | ||
| await queryClient.invalidateQueries({ queryKey: coursePlanQueryKeys.list() }) | ||
| router.push(coursePlanScheduleRoute(plan.id)) | ||
| }, | ||
| }, | ||
| ) |
Contributor
There was a problem hiding this comment.
Missing toast notification labels (successHeader/errorHeader).
useToastMutation is called with { notify: true, method: "POST" } but no successHeader, successMessage, errorHeader, or errorMessage. This will display empty headers in the success and error toasts shown to the user after plan creation. Add translated strings for at least the header fields.
🛠️ Suggested fix
const createPlanMutation = useToastMutation(
() => createCourseDesignerPlan({}),
- { notify: true, method: "POST" },
+ {
+ notify: true,
+ method: "POST",
+ successHeader: t("message-created-successfully"),
+ errorHeader: t("error-title"),
+ },
{📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const createPlanMutation = useToastMutation( | |
| () => createCourseDesignerPlan({}), | |
| { notify: true, method: "POST" }, | |
| { | |
| onSuccess: async (plan) => { | |
| await queryClient.invalidateQueries({ queryKey: coursePlanQueryKeys.list() }) | |
| router.push(coursePlanScheduleRoute(plan.id)) | |
| }, | |
| }, | |
| ) | |
| const createPlanMutation = useToastMutation( | |
| () => createCourseDesignerPlan({}), | |
| { | |
| notify: true, | |
| method: "POST", | |
| successHeader: t("message-created-successfully"), | |
| errorHeader: t("error-title"), | |
| }, | |
| { | |
| onSuccess: async (plan) => { | |
| await queryClient.invalidateQueries({ queryKey: coursePlanQueryKeys.list() }) | |
| router.push(coursePlanScheduleRoute(plan.id)) | |
| }, | |
| }, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/main-frontend/src/app/manage/course-plans/components/CoursePlansListPage.tsx`
around lines 46 - 55, The toast mutation call for createPlanMutation (the
useToastMutation invocation) is missing user-facing labels so success/error
toasts show empty headers; update the options object passed to useToastMutation
for createPlanMutation to include translated strings for successHeader and
errorHeader (and optionally successMessage/errorMessage) using the app's i18n
helper (e.g., t("...")) before calling createCourseDesignerPlan, so that
onSuccess/onError toasts display meaningful text and then continue to invalidate
queries and route to coursePlanScheduleRoute(plan.id).
Comment on lines
1
to
4
| export const coursePlanScheduleRoute = (planId: string) => { | ||
| // eslint-disable-next-line i18next/no-literal-string | ||
| return `/manage/course-plans/${planId}/schedule` | ||
| } |
Contributor
There was a problem hiding this comment.
Encode the dynamic path segment and add a minimal helper doc.
Line 3 interpolates planId as-is, so values containing reserved URL characters can produce malformed routes. Also, this exported helper should include a short intent-level doc.
Proposed update
+/** Returns the schedule route for a course plan id. */
export const coursePlanScheduleRoute = (planId: string) => {
// eslint-disable-next-line i18next/no-literal-string
- return `/manage/course-plans/${planId}/schedule`
+ return `/manage/course-plans/${encodeURIComponent(planId)}/schedule`
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/main-frontend/src/app/manage/course-plans/coursePlanRoutes.ts`
around lines 1 - 4, coursePlanScheduleRoute currently interpolates planId
directly which can break routes for IDs with reserved URL characters; update the
function to encode the dynamic segment using encodeURIComponent(planId) and add
a one- or two-line JSDoc/comment above the exported function describing its
intent (e.g., "Return the schedule route for a course plan by id") so callers
know it's a URL helper; reference the function name coursePlanScheduleRoute and
the parameter planId when making the change.
Comment on lines
+69
to
+71
| export interface CreateCourseDesignerPlanRequest { | ||
| name?: string | null | ||
| } |
Contributor
There was a problem hiding this comment.
name optionality drifts from the generated contract and can send invalid payloads.
Line 70, Line 83, and Line 88 allow omitting name ({} by default), while generated bindings/guards define name as required string | null. This mismatch can cause request validation failures and type drift across modules.
Proposed fix
export interface CreateCourseDesignerPlanRequest {
- name?: string | null
+ name: string | null
}
export interface SaveCourseDesignerScheduleRequest {
- name?: string | null
+ name: string | null
stages: Array<CourseDesignerScheduleStageInput>
}
export const createCourseDesignerPlan = async (
- payload: CreateCourseDesignerPlanRequest = {},
+ payload: CreateCourseDesignerPlanRequest = { name: null },
): Promise<CourseDesignerPlan> => {Also applies to: 82-85, 87-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/main-frontend/src/services/backend/courseDesigner.ts` around lines
69 - 71, CreateCourseDesignerPlanRequest (and any other interfaces in this file
that currently declare name?: string | null) mark the name field as required
(name: string | null) to match the generated contract and avoid sending invalid
payloads; locate all occurrences of name?: string | null (e.g.,
CreateCourseDesignerPlanRequest) and change them to name: string | null, then
update any call sites or tests that rely on omitting name to provide an explicit
string or null value.
Comment on lines
+290
to
+291
| "course-plans-wizard-starts-on-month-label": "Työn alkamiskuukausi", | ||
| "course-plans-wizard-step-name": "Nimeä projekti", |
Contributor
There was a problem hiding this comment.
Inconsistent Finnish terminology within the same wizard: projekti/työ vs. kurssi
Line 290 uses "Työn alkamiskuukausi" ("work start month") and Line 291 uses "Nimeä projekti" ("name the project"), while Line 293 uses "Kurssin koko ja alkamiskuukausi" ("course size and start month"). All three are labels within the same wizard flow, but two different Finnish terms (työ/projekti vs. kurssi) are used to refer to the same subject being configured. This is inconsistent terminology that may confuse Finnish-speaking users.
If the wizard is about a course plan (as the key namespace course-plans-* implies), consider aligning all three to use kurssi:
- "course-plans-wizard-starts-on-month-label": "Työn alkamiskuukausi",
+ "course-plans-wizard-starts-on-month-label": "Kurssin alkamiskuukausi",
"course-plans-wizard-step-name": "Nimeä projekti",
+ "course-plans-wizard-step-name": "Nimeä kurssisuunnitelma",If projekti/työ is intentional (e.g. this wizard refers to a development project, not the student-facing course), please verify consistency against the other locale files (en, sv, etc.).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "course-plans-wizard-starts-on-month-label": "Työn alkamiskuukausi", | |
| "course-plans-wizard-step-name": "Nimeä projekti", | |
| "course-plans-wizard-starts-on-month-label": "Kurssin alkamiskuukausi", | |
| "course-plans-wizard-step-name": "Nimeä kurssisuunnitelma", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shared-module/packages/common/src/locales/fi/main-frontend.json` around lines
290 - 291, The Finnish strings in the course-plans wizard are inconsistent:
replace "Työn alkamiskuukausi" (key course-plans-wizard-starts-on-month-label)
and "Nimeä projekti" (key course-plans-wizard-step-name) to use the same term as
the rest of the wizard (e.g. "Kurssin alkamiskuukausi" and "Nimeä kurssi") so
all course-plans-* keys consistently use "kurssi"; if instead the flow truly
refers to a project/work, update the other course-plans keys to match that term
across locales.
- Removed deprecated course plan route functions from coursePlanRoutes.ts. - Updated imports in relevant components to use new route functions from shared-module. - Ensured consistency in route management for course plans across the application.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (5)
services/main-frontend/src/app/manage/course-plans/coursePlanRoutes.ts (1)
1-14: Prior feedback aboutencodeURIComponentand minimal function docs still applies.The previous review already flagged that
planIdshould be encoded and that these exported helpers should have brief doc comments. Those suggestions remain valid.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/src/app/manage/course-plans/coursePlanRoutes.ts` around lines 1 - 14, The three route helpers (coursePlanHubRoute, coursePlanScheduleRoute, coursePlanWorkspaceRoute) must encode the planId and include brief docs: update each function to call encodeURIComponent(planId) when constructing the path to prevent unsafe characters, and add a one-line JSDoc or single-line comment above each exported function describing what route it returns and the expected parameter (planId: string).services/main-frontend/src/services/backend/courseDesigner.ts (1)
88-104:⚠️ Potential issue | 🟠 MajorRequest DTO optionality drifts from the generated contract.
Line 88, Line 101, and task DTOs later in the file allow omitted fields, while generated bindings define these as required nullable fields (
...: T | null). This can create cross-module type drift and invalid payload shapes.💡 Proposed fix
export interface CreateCourseDesignerPlanRequest { - name?: string | null + name: string | null } export interface SaveCourseDesignerScheduleRequest { - name?: string | null + name: string | null stages: Array<CourseDesignerScheduleStageInput> } export const createCourseDesignerPlan = async ( - payload: CreateCourseDesignerPlanRequest = {}, + payload: CreateCourseDesignerPlanRequest = { name: null }, ): Promise<CourseDesignerPlan> => { export interface CreateCourseDesignerStageTaskRequest { title: string - description?: string | null + description: string | null } export interface UpdateCourseDesignerStageTaskRequest { - title?: string | null - description?: string | null - is_completed?: boolean + title: string | null + description: string | null + is_completed: boolean | null }Also applies to: 106-108, 174-195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/src/services/backend/courseDesigner.ts` around lines 88 - 104, The DTOs currently make fields optional (using ?) which drifts from the generated contract; update the request interfaces (CreateCourseDesignerPlanRequest, CourseDesignerScheduleSuggestionRequest, SaveCourseDesignerScheduleRequest and the other affected task DTOs around lines referenced) so each property is declared as required but nullable (e.g., name: string | null) instead of optional, ensuring the types match the generated bindings and produce valid payload shapes across modules.services/headless-lms/models/src/course_designer_plans.rs (1)
570-576:⚠️ Potential issue | 🟠 MajorValidate schedule invariants inside
replace_schedule_for_userbefore persisting.This write API accepts
stagesbut does not callvalidate_schedule_input, so invalid ordering/gaps/overlaps can still be stored through this path.💡 Proposed fix
pub async fn replace_schedule_for_user( conn: &mut PgConnection, plan_id: Uuid, user_id: Uuid, name: Option<String>, stages: &[CourseDesignerScheduleStageInput], ) -> ModelResult<CourseDesignerPlanDetails> { + validate_schedule_input(stages)?; let mut tx = conn.begin().await?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/headless-lms/models/src/course_designer_plans.rs` around lines 570 - 576, The replace_schedule_for_user function accepts stages but doesn't validate them before persisting; call the existing validate_schedule_input routine at the start of replace_schedule_for_user (using the provided stages/possibly name if the validator requires it) and return the appropriate ModelResult error if validation fails so invalid ordering/gaps/overlaps are rejected before any DB updates. Ensure you reference validate_schedule_input and CourseDesignerScheduleStageInput and perform validation prior to any calls that modify or upsert the plan/schedule.services/main-frontend/src/app/manage/course-plans/components/CoursePlanCard.tsx (1)
12-16: Replace literal colors with theme tokens in card styles.
border,background, and meta text color are hard-coded. Please switch these to shared theme color tokens for design-system consistency.As per coding guidelines, "If a component uses colors, use colors from the theme if possible."
Also applies to: 28-29
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/src/app/manage/course-plans/components/CoursePlanCard.tsx` around lines 12 - 16, The card's styles in the CoursePlanCard styled component use hard-coded colors for border, border-radius, padding, background and the meta text color; replace the literal color values (`#d9dde4` and white and the meta text color) with the design-system theme tokens (e.g., theme.colors.surface/border/background or equivalent tokens your app exposes) and update the meta text style (the MetaText or .meta style referenced around lines 28-29) to use the theme token for muted/text-secondary instead of a hard-coded hex; ensure you import/use the theme object or styled-system token names already used across the codebase so colors are consistent with the shared theme.services/main-frontend/src/app/manage/course-plans/components/CoursePlansListPage.tsx (1)
46-49:⚠️ Potential issue | 🟡 MinorAdd toast headers for create-plan mutation notifications.
The mutation still enables notifications without explicit
successHeader/errorHeader, so toast titles may be blank or non-descriptive.Suggested fix
const createPlanMutation = useToastMutation( () => createCourseDesignerPlan({}), - { notify: true, method: "POST" }, + { + notify: true, + method: "POST", + successHeader: t("message-created-successfully"), + errorHeader: t("error-title"), + }, { onSuccess: async (plan) => { await queryClient.invalidateQueries({ queryKey: coursePlanQueryKeys.list() }) router.push(coursePlanHubRoute(plan.id)) }, }, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/src/app/manage/course-plans/components/CoursePlansListPage.tsx` around lines 46 - 49, The createPlanMutation call enables notifications but doesn't set toast titles; update the useToastMutation call for createPlanMutation so the options object (the { notify: true, method: "POST" } argument) includes explicit successHeader and errorHeader strings (e.g., "Plan created" and "Failed to create plan") so toasts for createCourseDesignerPlan have descriptive titles; modify the options passed to useToastMutation in CoursePlansListPage.tsx rather than changing createCourseDesignerPlan itself.
🧹 Nitpick comments (3)
services/main-frontend/src/app/manage/course-plans/[id]/schedule/components/ScheduleWizardPage.tsx (3)
65-67:stepTransitionDirectionis a pass-through — usestepdirectly.This function returns its argument unchanged. It can be inlined at the call-site (Line 145:
key={controller.ui.step}) unless there's a planned extension.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/src/app/manage/course-plans/`[id]/schedule/components/ScheduleWizardPage.tsx around lines 65 - 67, The function stepTransitionDirection currently just returns its argument; remove the dead wrapper and inline the value where used (e.g., replace uses like key={stepTransitionDirection(controller.ui.step)} with key={controller.ui.step}) and delete the stepTransitionDirection definition to avoid an unnecessary indirection; if any other call sites rely on it, update them to use the raw ScheduleWizardStepId value directly (e.g., controller.ui.step).
37-63: Use theme colors instead of hardcoded values.
sectionStylesuses hardcodedwhiteand#d9dde4;wizardStepCardStylesalso uses hardcodedwhite. PreferbaseTheme.colorsfor consistency.Proposed fix
const sectionStyles = css` - background: white; - border: 1px solid `#d9dde4`; + background: ${baseTheme.colors.clear[100]}; + border: 1px solid ${baseTheme.colors.gray[200]}; border-radius: 12px; padding: 1rem; margin-bottom: 1rem; ` const wizardStepCardStyles = css` - background: white; + background: ${baseTheme.colors.clear[100]}; border-radius: 16px;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/manage/course-plans/`[id]/schedule/components/ScheduleWizardPage.tsx around lines 37 - 63, Replace hardcoded color literals in sectionStyles and wizardStepCardStyles with theme color tokens: swap "white" for baseTheme.colors.gray[0] (or the theme's primary background token) and replace "#d9dde4" with an appropriate border token such as baseTheme.colors.gray[200]; update the border-left green literal to use baseTheme.colors.green[500] (already used) if any other hardcoded colors exist. Modify the CSS blocks for sectionStyles and wizardStepCardStyles to reference baseTheme.colors (...) so all background, border and shadow color usages come from the theme.
77-93: Consider adding an exhaustive default to guard against future stage additions.All current
CourseDesignerStagevariants are handled, but adefault: { const _exhaustive: never = stage; return _exhaustive; }guard would turn future enum extensions into compile-time errors rather than silentundefinedreturns.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/src/app/manage/course-plans/`[id]/schedule/components/ScheduleWizardPage.tsx around lines 77 - 93, The switch in the useCallback function stageLabel handling CourseDesignerStage should include an exhaustive default to prevent returning undefined when new stages are added; update the switch inside stageLabel (the function referenced as stageLabel and the type CourseDesignerStage) to add a default branch that asserts exhaustiveness (e.g., const _exhaustive: never = stage; return _exhaustive;) so the compiler will error if a new variant is introduced without updating this function.
🤖 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/models/.sqlx/query-a9bb85cad044304914c9a02a0c62026592b75f642db016d66743365ad09d401f.json`:
- Line 3: The ORDER BY clause currently uses s.id which yields non-deterministic
stage ordering; change it to order stages by the enum's natural sequence by
replacing "ORDER BY s.id, t.order_number, t.id" with an expression that maps the
course_designer_stage enum to its fixed order (for example: ORDER BY
array_position(ARRAY['analysis','design','development','implementation','evaluation']::text[],
s.course_designer_stage), t.order_number, t.id), referencing the
course_designer_plan_stages table and its course_designer_stage column so tasks
follow the intended stage sequence.
In `@services/headless-lms/models/src/course_designer_plans.rs`:
- Around line 606-617: The precondition branch that checks locked_plan.status
and returns ModelError (the if matching CourseDesignerPlanStatus::InProgress |
Completed | Archived that returns ModelError::new) must explicitly roll back the
active transaction before the early return; locate the transaction guard
variable used when the transaction was started (e.g., tx or tx_guard) and call
its rollback method (e.g., tx.rollback().await? or tx.rollback() as appropriate)
before returning the Err to ensure the transaction is not left open.
- Around line 845-847: The code currently sets completed_at and completed_by
whenever new_completed is true, overwriting existing audit fields; change this
to only set completed_at = Some(now) and completed_by = Some(user_id) when the
completion state actually transitions from not completed to completed (i.e.,
old_completed == false && new_completed == true). Use the prior/computed old
completion flag (e.g., an existing completed/was_completed value from the
record) to decide: if old_completed && new_completed then preserve existing
completed_at and completed_by; if !new_completed then set both to None; if
!old_completed && new_completed set them to Some(now)/Some(user_id). Ensure you
reference and update the same variables shown (new_completed, completed_at,
completed_by, user_id, now) when implementing this check.
- Around line 806-820: Replace the dynamic sqlx::query(...) call with the
compile-time checked sqlx::query! (or query_as! if you want a mapped struct)
macro: use sqlx::query!(r#"SELECT t.title, t.description, t.is_completed FROM
... WHERE t.id = $3 AND s.course_designer_plan_id = $1 AND t.deleted_at IS
NULL"#, plan_id, user_id, task_id) (or query_as! with your target struct) and
keep the .fetch_optional(&mut *conn).await? call; ensure the selected column
names and their Rust types (title, description, is_completed) match the
macro-inferred types or adjust to query_as! with a struct if needed so the
models crate uses compile-time SQL checking instead of sqlx::query.
- Around line 749-760: The current pattern computing order_number by querying
MAX(order_number) then inserting is race-prone; wrap the read+insert in a DB
transaction and serialize per-stage (use either SELECT MAX(order_number) ... FOR
UPDATE inside the transaction after acquiring a per-stage lock via
pg_advisory_xact_lock(stage_id) or lock the tasks table/rows) so concurrent
creators cannot compute the same value; update the code around stage_id,
order_number and the creation of CourseDesignerPlanStageTask to start a
transaction on conn, call pg_advisory_xact_lock(stage_id) (or LOCK TABLE
course_designer_plan_stage_tasks), re-query MAX(order_number) and then insert,
and commit the transaction.
In
`@services/headless-lms/server/src/controllers/main_frontend/course_designer.rs`:
- Around line 56-60: The ExtendStageRequest struct accepts an unbounded u32 for
months; add an explicit upper-bound validation (e.g., months > 0 && months <=
120) before using the value to prevent overflow or nonsensical dates. Implement
the check in the controller handler that receives ExtendStageRequest (the
endpoint/method handling extension requests) or add an impl Validate/validate()
on ExtendStageRequest that returns an error for out-of-range values, and ensure
the handler maps that to a 400 Bad Request with a clear message when months is
invalid.
In `@services/main-frontend/src/app/manage/course-plans/`[id]/page.tsx:
- Around line 26-42: The effect handling plan navigation (useEffect reading
planQuery.data and plan.status) only redirects for specific statuses and leaves
other/unknown statuses hanging on the Spinner; update the effect to handle
unknown statuses by logging the unexpected plan.status and performing a safe
fallback (e.g., router.replace to a fallback route or
coursePlanWorkspaceRoute(planId) and/or set an error state to render a
user-facing message) so the user is not left on a permanent spinner; include
references to planQuery.data, plan.status, router.replace,
coursePlanScheduleRoute, and coursePlanWorkspaceRoute when implementing the
fallback.
- Around line 44-47: The component currently returns a Spinner for both loading
and error states causing query errors to be swallowed; update the planQuery
handling so that when planQuery.isError is true it renders the shared
ErrorBanner component instead of Spinner, keep Spinner for planQuery.isLoading,
and add the import for ErrorBanner from
"@/shared-module/common/components/ErrorBanner"; locate the conditional using
planQuery.isLoading and planQuery.isError in page.tsx and replace the error
branch to render <ErrorBanner /> (optionally provide the error message from
planQuery.error) while leaving the loading branch to render <Spinner
variant="medium" />.
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/workspace/components/CoursePlanWorkspacePage.tsx:
- Around line 215-230: The two buttons in CoursePlanWorkspacePage both call
advanceMutation.mutate() but represent different intents; update the UI to call
distinct handlers or mutations — e.g., implement a startNextPhaseEarly
handler/mutation (or call advanceMutation.mutate({early: true})) for the
"course-plans-start-next-phase-early" button and a completePhaseAndProceed
handler/mutation for the "course-plans-mark-phase-done-proceed" button, then
wire those handlers into the Button onClick props instead of both calling
advanceMutation.mutate(); ensure the new handlers reference the existing
advanceMutation logic or create new mutations with clear names
(startNextPhaseEarly, completePhaseAndProceed) so intent-specific behavior is
executed.
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/workspace/components/WorkspaceStageSection.tsx:
- Around line 168-175: The task input currently uses only a placeholder and
needs an accessible label: update the input in WorkspaceStageSection (the
element using taskInputStyles, value={newTaskTitle}, onChange={setNewTaskTitle},
onKeyDown calling handleAddTask) to include an explicit accessible name by
either adding an aria-label (e.g., aria-label="New task title") or by adding a
linked <label> element for the control; ensure the label text is localized via
t(...) if appropriate and that the label/aria-label accurately describes the
control for screen readers.
- Around line 113-145: Replace the ad-hoc useState flow for the task input
(newTaskTitle, setNewTaskTitle, and handleAddTask) with react-hook-form:
initialize useForm in this component, register the task title input, and wire
submission through handleSubmit to call createMutation.mutate with the trimmed
title; on success call reset() (instead of setNewTaskTitle("")) and still call
onInvalidate via createMutation's onSuccess. Also refactor the similar block
referenced at lines 167-184 the same way (replace local state and manual
handlers with useForm registration, handleSubmit → createMutation.mutate, and
reset on success) so both task input flows follow the react-hook-form +
useToastMutation pattern.
---
Duplicate comments:
In `@services/headless-lms/models/src/course_designer_plans.rs`:
- Around line 570-576: The replace_schedule_for_user function accepts stages but
doesn't validate them before persisting; call the existing
validate_schedule_input routine at the start of replace_schedule_for_user (using
the provided stages/possibly name if the validator requires it) and return the
appropriate ModelResult error if validation fails so invalid
ordering/gaps/overlaps are rejected before any DB updates. Ensure you reference
validate_schedule_input and CourseDesignerScheduleStageInput and perform
validation prior to any calls that modify or upsert the plan/schedule.
In
`@services/main-frontend/src/app/manage/course-plans/components/CoursePlanCard.tsx`:
- Around line 12-16: The card's styles in the CoursePlanCard styled component
use hard-coded colors for border, border-radius, padding, background and the
meta text color; replace the literal color values (`#d9dde4` and white and the
meta text color) with the design-system theme tokens (e.g.,
theme.colors.surface/border/background or equivalent tokens your app exposes)
and update the meta text style (the MetaText or .meta style referenced around
lines 28-29) to use the theme token for muted/text-secondary instead of a
hard-coded hex; ensure you import/use the theme object or styled-system token
names already used across the codebase so colors are consistent with the shared
theme.
In
`@services/main-frontend/src/app/manage/course-plans/components/CoursePlansListPage.tsx`:
- Around line 46-49: The createPlanMutation call enables notifications but
doesn't set toast titles; update the useToastMutation call for
createPlanMutation so the options object (the { notify: true, method: "POST" }
argument) includes explicit successHeader and errorHeader strings (e.g., "Plan
created" and "Failed to create plan") so toasts for createCourseDesignerPlan
have descriptive titles; modify the options passed to useToastMutation in
CoursePlansListPage.tsx rather than changing createCourseDesignerPlan itself.
In `@services/main-frontend/src/app/manage/course-plans/coursePlanRoutes.ts`:
- Around line 1-14: The three route helpers (coursePlanHubRoute,
coursePlanScheduleRoute, coursePlanWorkspaceRoute) must encode the planId and
include brief docs: update each function to call encodeURIComponent(planId) when
constructing the path to prevent unsafe characters, and add a one-line JSDoc or
single-line comment above each exported function describing what route it
returns and the expected parameter (planId: string).
In `@services/main-frontend/src/services/backend/courseDesigner.ts`:
- Around line 88-104: The DTOs currently make fields optional (using ?) which
drifts from the generated contract; update the request interfaces
(CreateCourseDesignerPlanRequest, CourseDesignerScheduleSuggestionRequest,
SaveCourseDesignerScheduleRequest and the other affected task DTOs around lines
referenced) so each property is declared as required but nullable (e.g., name:
string | null) instead of optional, ensuring the types match the generated
bindings and produce valid payload shapes across modules.
---
Nitpick comments:
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/schedule/components/ScheduleWizardPage.tsx:
- Around line 65-67: The function stepTransitionDirection currently just returns
its argument; remove the dead wrapper and inline the value where used (e.g.,
replace uses like key={stepTransitionDirection(controller.ui.step)} with
key={controller.ui.step}) and delete the stepTransitionDirection definition to
avoid an unnecessary indirection; if any other call sites rely on it, update
them to use the raw ScheduleWizardStepId value directly (e.g.,
controller.ui.step).
- Around line 37-63: Replace hardcoded color literals in sectionStyles and
wizardStepCardStyles with theme color tokens: swap "white" for
baseTheme.colors.gray[0] (or the theme's primary background token) and replace
"#d9dde4" with an appropriate border token such as baseTheme.colors.gray[200];
update the border-left green literal to use baseTheme.colors.green[500] (already
used) if any other hardcoded colors exist. Modify the CSS blocks for
sectionStyles and wizardStepCardStyles to reference baseTheme.colors (...) so
all background, border and shadow color usages come from the theme.
- Around line 77-93: The switch in the useCallback function stageLabel handling
CourseDesignerStage should include an exhaustive default to prevent returning
undefined when new stages are added; update the switch inside stageLabel (the
function referenced as stageLabel and the type CourseDesignerStage) to add a
default branch that asserts exhaustiveness (e.g., const _exhaustive: never =
stage; return _exhaustive;) so the compiler will error if a new variant is
introduced without updating this function.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
services/headless-lms/models/.sqlx/query-115e8920adf206f53197a387a640b11307ac593db2e46ebad8846338f5da34ae.jsonservices/headless-lms/models/.sqlx/query-312d1160a49c0f40c4abcc13327c2a95f1c1454590c5629950969a32ce5fa70b.jsonservices/headless-lms/models/.sqlx/query-33ef238557577f0455e7464e6ef8811f5b60f8f9f80545a14916a8964d3b56b1.jsonservices/headless-lms/models/.sqlx/query-9275233ba053dccf57b81e7caf1c1da3597b86b3265b880f7a9517805e28fe43.jsonservices/headless-lms/models/.sqlx/query-a1608b5e88488bef17d4766885c8bceb0796b17c7f2b06c55f3f37d2ee1f6476.jsonservices/headless-lms/models/.sqlx/query-a9bb85cad044304914c9a02a0c62026592b75f642db016d66743365ad09d401f.jsonservices/headless-lms/models/src/course_designer_plans.rsservices/headless-lms/server/src/controllers/main_frontend/course_designer.rsservices/headless-lms/server/src/ts_binding_generator.rsservices/main-frontend/src/app/manage/course-plans/[id]/page.tsxservices/main-frontend/src/app/manage/course-plans/[id]/schedule/components/ScheduleWizardPage.tsxservices/main-frontend/src/app/manage/course-plans/[id]/workspace/components/CoursePlanWorkspacePage.tsxservices/main-frontend/src/app/manage/course-plans/[id]/workspace/components/WorkspaceStageSection.tsxservices/main-frontend/src/app/manage/course-plans/[id]/workspace/page.tsxservices/main-frontend/src/app/manage/course-plans/components/CoursePlanCard.tsxservices/main-frontend/src/app/manage/course-plans/components/CoursePlansListPage.tsxservices/main-frontend/src/app/manage/course-plans/coursePlanRoutes.tsservices/main-frontend/src/services/backend/courseDesigner.tsshared-module/packages/common/src/bindings.guard.tsshared-module/packages/common/src/bindings.tsshared-module/packages/common/src/locales/en/main-frontend.json
✅ Files skipped from review due to trivial changes (2)
- services/headless-lms/models/.sqlx/query-115e8920adf206f53197a387a640b11307ac593db2e46ebad8846338f5da34ae.json
- services/headless-lms/models/.sqlx/query-9275233ba053dccf57b81e7caf1c1da3597b86b3265b880f7a9517805e28fe43.json
| @@ -0,0 +1,89 @@ | |||
| { | |||
| "db_name": "PostgreSQL", | |||
| "query": "\nSELECT\n t.id,\n t.created_at,\n t.updated_at,\n t.course_designer_plan_stage_id,\n t.title,\n t.description,\n t.order_number,\n t.is_completed,\n t.completed_at,\n t.completed_by_user_id,\n t.is_auto_generated,\n t.created_by_user_id\nFROM course_designer_plan_stage_tasks t\nJOIN course_designer_plan_stages s ON s.id = t.course_designer_plan_stage_id AND s.deleted_at IS NULL\nJOIN course_designer_plan_members m ON m.course_designer_plan_id = s.course_designer_plan_id\n AND m.user_id = $2\n AND m.deleted_at IS NULL\nWHERE s.course_designer_plan_id = $1\n AND t.deleted_at IS NULL\nORDER BY s.id, t.order_number, t.id\n", | |||
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find SQL migration files in the repository
fd -e sql -type f | head -20Repository: rage/secret-project-331
Length of output: 237
🏁 Script executed:
#!/bin/bash
# Find SQL migration files in the repository
fd '\.sql$' -type f | head -30Repository: rage/secret-project-331
Length of output: 237
🏁 Script executed:
#!/bin/bash
# Search for migrations directory and SQL files
find . -name "*.sql" | head -30Repository: rage/secret-project-331
Length of output: 2640
🏁 Script executed:
#!/bin/bash
# Search for course_designer_plan_stages table definition
rg "course_designer_plan_stages" ./services/headless-lms/migrations/ -A 10 -B 2Repository: rage/secret-project-331
Length of output: 9241
🏁 Script executed:
#!/bin/bash
# Get the full migration file to see all table definitions
cat ./services/headless-lms/migrations/20260223110000_add_course_designer_planning_tables.up.sqlRepository: rage/secret-project-331
Length of output: 12028
ORDER BY s.id does not preserve stage ordering — should use the stage enum's natural order instead.
The course_designer_plan_stages table does not have an order_number column. Stages are identified by a fixed-value course_designer_stage ENUM ('analysis', 'design', 'development', 'implementation', 'evaluation'), which has an inherent order. Ordering by s.id (a UUID) produces arbitrary, non-deterministic stage sequence. Callers iterating by stage would silently see tasks in random order rather than the intended workflow sequence.
🐛 Proposed fix
-ORDER BY s.id, t.order_number, t.id
+ORDER BY s.stage, t.order_number, t.idThe stage enum column follows the declaration order: analysis → design → development → implementation → evaluation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "query": "\nSELECT\n t.id,\n t.created_at,\n t.updated_at,\n t.course_designer_plan_stage_id,\n t.title,\n t.description,\n t.order_number,\n t.is_completed,\n t.completed_at,\n t.completed_by_user_id,\n t.is_auto_generated,\n t.created_by_user_id\nFROM course_designer_plan_stage_tasks t\nJOIN course_designer_plan_stages s ON s.id = t.course_designer_plan_stage_id AND s.deleted_at IS NULL\nJOIN course_designer_plan_members m ON m.course_designer_plan_id = s.course_designer_plan_id\n AND m.user_id = $2\n AND m.deleted_at IS NULL\nWHERE s.course_designer_plan_id = $1\n AND t.deleted_at IS NULL\nORDER BY s.id, t.order_number, t.id\n", | |
| "query": "\nSELECT\n t.id,\n t.created_at,\n t.updated_at,\n t.course_designer_plan_stage_id,\n t.title,\n t.description,\n t.order_number,\n t.is_completed,\n t.completed_at,\n t.completed_by_user_id,\n t.is_auto_generated,\n t.created_by_user_id\nFROM course_designer_plan_stage_tasks t\nJOIN course_designer_plan_stages s ON s.id = t.course_designer_plan_stage_id AND s.deleted_at IS NULL\nJOIN course_designer_plan_members m ON m.course_designer_plan_id = s.course_designer_plan_id\n AND m.user_id = $2\n AND m.deleted_at IS NULL\nWHERE s.course_designer_plan_id = $1\n AND t.deleted_at IS NULL\nORDER BY s.stage, t.order_number, t.id\n", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/headless-lms/models/.sqlx/query-a9bb85cad044304914c9a02a0c62026592b75f642db016d66743365ad09d401f.json`
at line 3, The ORDER BY clause currently uses s.id which yields
non-deterministic stage ordering; change it to order stages by the enum's
natural sequence by replacing "ORDER BY s.id, t.order_number, t.id" with an
expression that maps the course_designer_stage enum to its fixed order (for
example: ORDER BY
array_position(ARRAY['analysis','design','development','implementation','evaluation']::text[],
s.course_designer_stage), t.order_number, t.id), referencing the
course_designer_plan_stages table and its course_designer_stage column so tasks
follow the intended stage sequence.
Comment on lines
+606
to
+617
| if matches!( | ||
| locked_plan.status, | ||
| CourseDesignerPlanStatus::InProgress | ||
| | CourseDesignerPlanStatus::Completed | ||
| | CourseDesignerPlanStatus::Archived | ||
| ) { | ||
| return Err(ModelError::new( | ||
| ModelErrorType::PreconditionFailed, | ||
| "Cannot edit schedule for a plan that has already started or is closed.".to_string(), | ||
| None, | ||
| )); | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Explicitly roll back before precondition early-returns in active transactions.
A transaction is started at Line 577, but this precondition path returns without an explicit rollback. Please roll back before returning errors in these tx-guard branches.
💡 Proposed fix
if matches!(
locked_plan.status,
CourseDesignerPlanStatus::InProgress
| CourseDesignerPlanStatus::Completed
| CourseDesignerPlanStatus::Archived
) {
+ tx.rollback().await?;
return Err(ModelError::new(
ModelErrorType::PreconditionFailed,
"Cannot edit schedule for a plan that has already started or is closed.".to_string(),
None,
));
}As per coding guidelines services/headless-lms/models/src/**/*.rs: If a transaction is started it should be committed or rolled back before the transaction goes out of scope.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if matches!( | |
| locked_plan.status, | |
| CourseDesignerPlanStatus::InProgress | |
| | CourseDesignerPlanStatus::Completed | |
| | CourseDesignerPlanStatus::Archived | |
| ) { | |
| return Err(ModelError::new( | |
| ModelErrorType::PreconditionFailed, | |
| "Cannot edit schedule for a plan that has already started or is closed.".to_string(), | |
| None, | |
| )); | |
| } | |
| if matches!( | |
| locked_plan.status, | |
| CourseDesignerPlanStatus::InProgress | |
| | CourseDesignerPlanStatus::Completed | |
| | CourseDesignerPlanStatus::Archived | |
| ) { | |
| tx.rollback().await?; | |
| return Err(ModelError::new( | |
| ModelErrorType::PreconditionFailed, | |
| "Cannot edit schedule for a plan that has already started or is closed.".to_string(), | |
| None, | |
| )); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/headless-lms/models/src/course_designer_plans.rs` around lines 606 -
617, The precondition branch that checks locked_plan.status and returns
ModelError (the if matching CourseDesignerPlanStatus::InProgress | Completed |
Archived that returns ModelError::new) must explicitly roll back the active
transaction before the early return; locate the transaction guard variable used
when the transaction was started (e.g., tx or tx_guard) and call its rollback
method (e.g., tx.rollback().await? or tx.rollback() as appropriate) before
returning the Err to ensure the transaction is not left open.
Comment on lines
+749
to
+760
| let max_order: Option<i32> = sqlx::query_scalar!( | ||
| r#" | ||
| SELECT MAX(order_number)::INTEGER | ||
| FROM course_designer_plan_stage_tasks | ||
| WHERE course_designer_plan_stage_id = $1 AND deleted_at IS NULL | ||
| "#, | ||
| stage_id | ||
| ) | ||
| .fetch_one(&mut *conn) | ||
| .await?; | ||
| let order_number = max_order.unwrap_or(0) + 1; | ||
| let task: CourseDesignerPlanStageTask = sqlx::query_as!( |
Contributor
There was a problem hiding this comment.
order_number assignment is race-prone under concurrent task creation.
Line 751 reads MAX(order_number) and Line 760 inserts later without serialization. Concurrent requests can produce duplicate order numbers for the same stage.
💡 Proposed fix
pub async fn create_stage_task_for_user(
conn: &mut PgConnection,
@@
) -> ModelResult<CourseDesignerPlanStageTask> {
+ let mut tx = conn.begin().await?;
@@
- )
- .fetch_optional(&mut *conn)
+ )
+ .fetch_optional(&mut *tx)
.await?
.flatten();
@@
+ sqlx::query!(
+ r#"
+SELECT id
+FROM course_designer_plan_stages
+WHERE id = $1
+FOR UPDATE
+"#,
+ stage_id
+ )
+ .fetch_one(&mut *tx)
+ .await?;
+
let max_order: Option<i32> = sqlx::query_scalar!(
@@
- .fetch_one(&mut *conn)
+ .fetch_one(&mut *tx)
.await?;
@@
- .fetch_one(&mut *conn)
+ .fetch_one(&mut *tx)
.await?;
+ tx.commit().await?;
Ok(task)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/headless-lms/models/src/course_designer_plans.rs` around lines 749 -
760, The current pattern computing order_number by querying MAX(order_number)
then inserting is race-prone; wrap the read+insert in a DB transaction and
serialize per-stage (use either SELECT MAX(order_number) ... FOR UPDATE inside
the transaction after acquiring a per-stage lock via
pg_advisory_xact_lock(stage_id) or lock the tasks table/rows) so concurrent
creators cannot compute the same value; update the code around stage_id,
order_number and the creation of CourseDesignerPlanStageTask to start a
transaction on conn, call pg_advisory_xact_lock(stage_id) (or LOCK TABLE
course_designer_plan_stage_tasks), re-query MAX(order_number) and then insert,
and commit the transaction.
Comment on lines
+806
to
+820
| let row = sqlx::query( | ||
| r#" | ||
| SELECT t.title, t.description, t.is_completed | ||
| FROM course_designer_plan_stage_tasks t | ||
| JOIN course_designer_plan_stages s ON s.id = t.course_designer_plan_stage_id AND s.deleted_at IS NULL | ||
| JOIN course_designer_plan_members m ON m.course_designer_plan_id = s.course_designer_plan_id | ||
| AND m.user_id = $2 AND m.deleted_at IS NULL | ||
| WHERE t.id = $3 AND s.course_designer_plan_id = $1 AND t.deleted_at IS NULL | ||
| "#, | ||
| ) | ||
| .bind(plan_id) | ||
| .bind(user_id) | ||
| .bind(task_id) | ||
| .fetch_optional(&mut *conn) | ||
| .await?; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Verify the sqlx::query usage at the specified lines
cat -n services/headless-lms/models/src/course_designer_plans.rs | sed -n '800,825p'Repository: rage/secret-project-331
Length of output: 1221
🏁 Script executed:
# Check for other sqlx::query usage in the file
rg -nP '\bsqlx::query\s*\(' services/headless-lms/models/src/course_designer_plans.rsRepository: rage/secret-project-331
Length of output: 97
🏁 Script executed:
# Look for the function context to understand if this is a dynamic query
cat -n services/headless-lms/models/src/course_designer_plans.rs | sed -n '795,840p'Repository: rage/secret-project-331
Length of output: 1970
Use SQLx macros instead of dynamic sqlx::query().
Line 806 uses sqlx::query() in the models crate, which violates the guideline requiring all SQL queries to use SQLx macros for compile-time checking. Replace with query!() or query_as!() as appropriate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/headless-lms/models/src/course_designer_plans.rs` around lines 806 -
820, Replace the dynamic sqlx::query(...) call with the compile-time checked
sqlx::query! (or query_as! if you want a mapped struct) macro: use
sqlx::query!(r#"SELECT t.title, t.description, t.is_completed FROM ... WHERE
t.id = $3 AND s.course_designer_plan_id = $1 AND t.deleted_at IS NULL"#,
plan_id, user_id, task_id) (or query_as! with your target struct) and keep the
.fetch_optional(&mut *conn).await? call; ensure the selected column names and
their Rust types (title, description, is_completed) match the macro-inferred
types or adjust to query_as! with a struct if needed so the models crate uses
compile-time SQL checking instead of sqlx::query.
Comment on lines
+845
to
+847
| let now = Utc::now(); | ||
| let completed_at = if new_completed { Some(now) } else { None }; | ||
| let completed_by = if new_completed { Some(user_id) } else { None }; |
Contributor
There was a problem hiding this comment.
Preserve completion audit fields unless completion state actually transitions.
Line 846/Line 847 overwrite completed_at and completed_by_user_id whenever new_completed is true, including title/description-only edits on already completed tasks.
💡 Proposed fix
-SELECT t.title, t.description, t.is_completed
+SELECT t.title, t.description, t.is_completed, t.completed_at, t.completed_by_user_id
@@
- let (current_title, current_description, current_completed) = match row {
+ let (
+ current_title,
+ current_description,
+ current_completed,
+ current_completed_at,
+ current_completed_by,
+ ) = match row {
Some(r) => (
r.get::<String, _>("title"),
r.get::<Option<String>, _>("description"),
r.get::<bool, _>("is_completed"),
+ r.get::<Option<DateTime<Utc>>, _>("completed_at"),
+ r.get::<Option<Uuid>, _>("completed_by_user_id"),
),
@@
- let now = Utc::now();
- let completed_at = if new_completed { Some(now) } else { None };
- let completed_by = if new_completed { Some(user_id) } else { None };
+ let now = Utc::now();
+ let (completed_at, completed_by) = match (current_completed, new_completed) {
+ (false, true) => (Some(now), Some(user_id)),
+ (true, false) => (None, None),
+ (true, true) => (current_completed_at, current_completed_by),
+ (false, false) => (None, None),
+ };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/headless-lms/models/src/course_designer_plans.rs` around lines 845 -
847, The code currently sets completed_at and completed_by whenever
new_completed is true, overwriting existing audit fields; change this to only
set completed_at = Some(now) and completed_by = Some(user_id) when the
completion state actually transitions from not completed to completed (i.e.,
old_completed == false && new_completed == true). Use the prior/computed old
completion flag (e.g., an existing completed/was_completed value from the
record) to decide: if old_completed && new_completed then preserve existing
completed_at and completed_by; if !new_completed then set both to None; if
!old_completed && new_completed set them to Some(now)/Some(user_id). Ensure you
reference and update the same variables shown (new_completed, completed_at,
completed_by, user_id, now) when implementing this check.
Comment on lines
26
to
42
| useEffect(() => { | ||
| if (!planQuery.data) { | ||
| return | ||
| } | ||
| const { plan } = planQuery.data | ||
| if (plan.status === "Draft" || plan.status === "Scheduling") { | ||
| router.replace(coursePlanScheduleRoute(planId)) | ||
| return | ||
| } | ||
| if ( | ||
| plan.status === "ReadyToStart" || | ||
| plan.status === "InProgress" || | ||
| plan.status === "Completed" | ||
| ) { | ||
| router.replace(coursePlanWorkspaceRoute(planId)) | ||
| } | ||
| }, [planQuery.data, planId, router]) |
Contributor
There was a problem hiding this comment.
Unhandled plan statuses leave the user on a permanent spinner.
If plan.status doesn't match any of the five known values (e.g., a new status is added server-side), the useEffect silently does nothing and the component renders a Spinner forever. Consider logging or redirecting to a fallback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/main-frontend/src/app/manage/course-plans/`[id]/page.tsx around
lines 26 - 42, The effect handling plan navigation (useEffect reading
planQuery.data and plan.status) only redirects for specific statuses and leaves
other/unknown statuses hanging on the Spinner; update the effect to handle
unknown statuses by logging the unexpected plan.status and performing a safe
fallback (e.g., router.replace to a fallback route or
coursePlanWorkspaceRoute(planId) and/or set an error state to render a
user-facing message) so the user is not left on a permanent spinner; include
references to planQuery.data, plan.status, router.replace,
coursePlanScheduleRoute, and coursePlanWorkspaceRoute when implementing the
fallback.
Comment on lines
+44
to
+47
| if (planQuery.isLoading || planQuery.isError) { | ||
| return <Spinner variant="medium" /> | ||
| } | ||
| return <Spinner variant="medium" /> |
Contributor
There was a problem hiding this comment.
Query errors are silently swallowed — show ErrorBanner instead of Spinner.
When planQuery.isError is true, the user sees an infinite spinner with no way to understand what went wrong. Use ErrorBanner (already imported pattern in the codebase) for error states.
Proposed fix
- if (planQuery.isLoading || planQuery.isError) {
+ if (planQuery.isError) {
+ return <ErrorBanner variant="readOnly" error={planQuery.error} />
+ }
+ if (planQuery.isLoading) {
return <Spinner variant="medium" />
}This requires adding the import:
import ErrorBanner from "@/shared-module/common/components/ErrorBanner"As per coding guidelines, "For loading and error states, use the Spinner and ErrorBanner components from the shared module."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/main-frontend/src/app/manage/course-plans/`[id]/page.tsx around
lines 44 - 47, The component currently returns a Spinner for both loading and
error states causing query errors to be swallowed; update the planQuery handling
so that when planQuery.isError is true it renders the shared ErrorBanner
component instead of Spinner, keep Spinner for planQuery.isLoading, and add the
import for ErrorBanner from "@/shared-module/common/components/ErrorBanner";
locate the conditional using planQuery.isLoading and planQuery.isError in
page.tsx and replace the error branch to render <ErrorBanner /> (optionally
provide the error message from planQuery.error) while leaving the loading branch
to render <Spinner variant="medium" />.
...n-frontend/src/app/manage/course-plans/[id]/workspace/components/CoursePlanWorkspacePage.tsx
Outdated
Show resolved
Hide resolved
Comment on lines
+113
to
+145
| const [newTaskTitle, setNewTaskTitle] = useState("") | ||
|
|
||
| const createMutation = useToastMutation( | ||
| (title: string) => createCourseDesignerStageTask(planId, stage.id, { title: title.trim() }), | ||
| { notify: true, method: "POST" }, | ||
| { | ||
| onSuccess: () => { | ||
| setNewTaskTitle("") | ||
| onInvalidate() | ||
| }, | ||
| }, | ||
| ) | ||
|
|
||
| const updateMutation = useToastMutation( | ||
| ({ taskId, is_completed }: { taskId: string; is_completed: boolean }) => | ||
| updateCourseDesignerStageTask(planId, taskId, { is_completed }), | ||
| { notify: false }, | ||
| { onSuccess: onInvalidate }, | ||
| ) | ||
|
|
||
| const deleteMutation = useToastMutation( | ||
| (taskId: string) => deleteCourseDesignerStageTask(planId, taskId), | ||
| { notify: true, method: "DELETE" }, | ||
| { onSuccess: onInvalidate }, | ||
| ) | ||
|
|
||
| const handleAddTask = () => { | ||
| const title = newTaskTitle.trim() | ||
| if (!title) { | ||
| return | ||
| } | ||
| createMutation.mutate(title) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Locate and examine the WorkspaceStageSection.tsx file
fd WorkspaceStageSection.tsx --exec wc -l {}Repository: rage/secret-project-331
Length of output: 175
🏁 Script executed:
# Read the file content around the specified lines with context
fd WorkspaceStageSection.tsx --exec cat -n {} | sed -n '100,200p'Repository: rage/secret-project-331
Length of output: 3710
🏁 Script executed:
# Check if react-hook-form is imported or used anywhere in the component
fd WorkspaceStageSection.tsx --exec rg -n 'useForm|from.*react-hook-form|<form' {}Repository: rage/secret-project-331
Length of output: 49
🏁 Script executed:
# Look for similar patterns in other components in the same directory
fd '\.tsx$' services/main-frontend/src/app/manage/course-plans/ --exec rg -l 'useForm|newTask' {}Repository: rage/secret-project-331
Length of output: 169
🏁 Script executed:
# Check for other similar input+button patterns in the workspace directory
fd '\.tsx$' services/main-frontend/src/app/manage/course-plans/[id]/workspace/components/ --exec rg -l 'useState.*Title|useState.*Name|onChange.*setNew' {}Repository: rage/secret-project-331
Length of output: 169
🏁 Script executed:
# Look for useForm usage in similar components in the course-plans directory
fd '\.tsx$' services/main-frontend/src/app/manage/course-plans/ --exec rg -l 'useForm' {} | head -10Repository: rage/secret-project-331
Length of output: 49
Use react-hook-form for the task input flow instead of local ad-hoc state.
This is a form interaction (input + submit action), but it is managed manually via useState. Please migrate to useForm and wire submission through your existing useToastMutation flow.
Per coding guidelines for **/*.tsx: form implementations must use useForm from react-hook-form, integrated with useToastMutation and useQuery hooks.
Also applies to: 167-184
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/workspace/components/WorkspaceStageSection.tsx
around lines 113 - 145, Replace the ad-hoc useState flow for the task input
(newTaskTitle, setNewTaskTitle, and handleAddTask) with react-hook-form:
initialize useForm in this component, register the task title input, and wire
submission through handleSubmit to call createMutation.mutate with the trimmed
title; on success call reset() (instead of setNewTaskTitle("")) and still call
onInvalidate via createMutation's onSuccess. Also refactor the similar block
referenced at lines 167-184 the same way (replace local state and manual
handlers with useForm registration, handleSubmit → createMutation.mutate, and
reset on success) so both task input flows follow the react-hook-form +
useToastMutation pattern.
Comment on lines
+168
to
+175
| <input | ||
| type="text" | ||
| className={taskInputStyles} | ||
| placeholder={t("course-plans-task-placeholder")} | ||
| value={newTaskTitle} | ||
| onChange={(e) => setNewTaskTitle(e.target.value)} | ||
| onKeyDown={(e) => e.key === "Enter" && handleAddTask()} | ||
| /> |
Contributor
There was a problem hiding this comment.
Add an explicit accessible label to the task input.
The text input currently relies on placeholder text only. Add aria-label or an associated <label> so the control is properly announced to assistive tech.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/workspace/components/WorkspaceStageSection.tsx
around lines 168 - 175, The task input currently uses only a placeholder and
needs an accessible label: update the input in WorkspaceStageSection (the
element using taskInputStyles, value={newTaskTitle}, onChange={setNewTaskTitle},
onKeyDown calling handleAddTask) to include an explicit accessible name by
either adding an aria-label (e.g., aria-label="New task title") or by adding a
linked <label> element for the control; ensure the label text is localized via
t(...) if appropriate and that the label/aria-label accurately describes the
control for screen readers.
- Adjusted import path for coursePlanQueryKeys to ensure correct module resolution. - Improved accessibility by adding aria-label to the CheckBox component in WorkspaceStageSection for better screen reader support.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
services/main-frontend/src/app/manage/course-plans/[id]/workspace/components/WorkspaceStageSection.tsx (2)
168-175: Task input is missing an explicit accessible label.This issue was flagged in a previous review and remains unaddressed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/src/app/manage/course-plans/`[id]/workspace/components/WorkspaceStageSection.tsx around lines 168 - 175, The text input in WorkspaceStageSection (the <input> bound to value={newTaskTitle} with onChange={setNewTaskTitle} and onKeyDown invoking handleAddTask) lacks an accessible label; add one by either rendering a visually-hidden <label> associated via id/htmlFor or adding an aria-label/aria-labelledby attribute that uses the localized string (t("course-plans-task-placeholder") or a new i18n key like t("course-plans-task-label")), ensure the input keeps its id if using a label and that any label text is localized and descriptive so screen readers can announce the field before users type.
113-145: Usereact-hook-formfor the task-add input instead of ad-hocuseState.This issue was flagged in a previous review and remains unaddressed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/src/app/manage/course-plans/`[id]/workspace/components/WorkspaceStageSection.tsx around lines 113 - 145, Replace the ad-hoc useState/newTaskTitle pattern in WorkspaceStageSection with react-hook-form: import and call useForm<{title:string}>(), register the input as register('title'), replace the standalone handleAddTask with a handleSubmit wrapper that reads data.title, trims and calls createMutation.mutate(data.title), and remove setNewTaskTitle/newTaskTitle usage; also call form reset() inside createMutation's onSuccess (instead of setNewTaskTitle("")) so the input clears after successful creation; keep existing createMutation/updateMutation/deleteMutation logic and names (createMutation, updateMutation, deleteMutation) intact.
🧹 Nitpick comments (2)
services/main-frontend/src/app/manage/course-plans/[id]/schedule/components/ScheduleWizardPage.tsx (2)
65-67: Inline the no-opstepTransitionDirectionhelper.
stepTransitionDirectioncurrently returnsstepunchanged, so it adds indirection without behavior. Inlining the key improves clarity.🧹 Proposed cleanup
-function stepTransitionDirection(step: ScheduleWizardStepId): string { - return step -} - export default function ScheduleWizardPage() { @@ - key={stepTransitionDirection(controller.ui.step)} + key={controller.ui.step}Also applies to: 145-145
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/src/app/manage/course-plans/`[id]/schedule/components/ScheduleWizardPage.tsx around lines 65 - 67, Remove the no-op helper function stepTransitionDirection and replace its usages with the step value directly: delete both definitions of stepTransitionDirection in ScheduleWizardPage.tsx and update any call sites to use the step (of type ScheduleWizardStepId) instead of calling stepTransitionDirection(step); ensure any type expectations are met where the function was used (e.g., if callers expected a string, adjust to use the step value or cast/convert appropriately).
37-55: Use theme color tokens instead of hardcoded colors.Line 38, Line 39, and Line 46 introduce hardcoded color values (
white,#d9dde4). Please switch these tobaseThemetokens for consistency and easier theming.🎨 Proposed refactor
const sectionStyles = css` - background: white; - border: 1px solid `#d9dde4`; + background: ${baseTheme.colors.clear[100]}; + border: 1px solid ${baseTheme.colors.gray[200]}; border-radius: 12px; padding: 1rem; margin-bottom: 1rem; ` const wizardStepCardStyles = css` - background: white; + background: ${baseTheme.colors.clear[100]}; border-radius: 16px; padding: 2rem 2.25rem; margin-bottom: 1rem;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/manage/course-plans/`[id]/schedule/components/ScheduleWizardPage.tsx around lines 37 - 55, Replace hardcoded color strings in the style blocks: in sectionStyles, replace background: white with background: ${baseTheme.colors.white} and replace border: 1px solid `#d9dde4` with border: 1px solid ${baseTheme.colors.gray[200]}; in wizardStepCardStyles, replace background: white with background: ${baseTheme.colors.white}. Update only the CSS string literals for sectionStyles and wizardStepCardStyles so they use baseTheme color tokens (baseTheme.colors.white and baseTheme.colors.gray[200]) instead of literal "white" and "#d9dde4".
🤖 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/course-plans/`[id]/workspace/components/WorkspaceStageSection.tsx:
- Around line 90-95: formatDateRange currently calls toLocaleDateString()
without a locale, so dates follow the browser/system locale; update
formatDateRange(startsOn: string, endsOn: string) to accept a locale parameter
(e.g., locale: string) and use Intl.DateTimeFormat(locale, options).format(...)
or pass the locale into toLocaleDateString(locale, options), then propagate the
app locale from useTranslation().i18n.language at the call site in the
WorkspaceStageSection component so the rendered range uses the active i18n
language.
- Around line 19-25: The CSS for cardStyles currently uses a hardcoded
"background: white"; update it to use the theme color instead by replacing the
literal with the baseTheme color reference (e.g., use baseTheme.colors.white or
the appropriate white token from baseTheme) so the rule becomes background:
${baseTheme.colors.white}; keep the rest of cardStyles (border, border-radius,
padding, margin-bottom) unchanged and ensure the interpolation uses the existing
baseTheme import.
- Around line 189-199: WorkspaceStageSection currently passes
updateMutation.isPending and deleteMutation.isPending to every WorkspaceTaskRow,
disabling all rows when any task is in flight; fix this by tracking in-flight
task IDs locally in WorkspaceStageSection (e.g., a Set or separate
pendingUpdateId/pendingDeleteId state) and update that state when starting and
finishing a mutation (use updateMutation's onMutate/onSettled or wrap
mutateAsync with setting/clearing the task id in try/finally); then pass per-row
flags to WorkspaceTaskRow (e.g., isUpdating={pendingUpdateIds.has(task.id)} and
isDeleting={pendingDeleteIds.has(task.id)}) and keep the existing
onToggle/onDelete handlers but ensure they set the pending id before calling
updateMutation.mutate / deleteMutation.mutate and clear it in the mutation
callbacks.
---
Duplicate comments:
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/workspace/components/WorkspaceStageSection.tsx:
- Around line 168-175: The text input in WorkspaceStageSection (the <input>
bound to value={newTaskTitle} with onChange={setNewTaskTitle} and onKeyDown
invoking handleAddTask) lacks an accessible label; add one by either rendering a
visually-hidden <label> associated via id/htmlFor or adding an
aria-label/aria-labelledby attribute that uses the localized string
(t("course-plans-task-placeholder") or a new i18n key like
t("course-plans-task-label")), ensure the input keeps its id if using a label
and that any label text is localized and descriptive so screen readers can
announce the field before users type.
- Around line 113-145: Replace the ad-hoc useState/newTaskTitle pattern in
WorkspaceStageSection with react-hook-form: import and call
useForm<{title:string}>(), register the input as register('title'), replace the
standalone handleAddTask with a handleSubmit wrapper that reads data.title,
trims and calls createMutation.mutate(data.title), and remove
setNewTaskTitle/newTaskTitle usage; also call form reset() inside
createMutation's onSuccess (instead of setNewTaskTitle("")) so the input clears
after successful creation; keep existing
createMutation/updateMutation/deleteMutation logic and names (createMutation,
updateMutation, deleteMutation) intact.
---
Nitpick comments:
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/schedule/components/ScheduleWizardPage.tsx:
- Around line 65-67: Remove the no-op helper function stepTransitionDirection
and replace its usages with the step value directly: delete both definitions of
stepTransitionDirection in ScheduleWizardPage.tsx and update any call sites to
use the step (of type ScheduleWizardStepId) instead of calling
stepTransitionDirection(step); ensure any type expectations are met where the
function was used (e.g., if callers expected a string, adjust to use the step
value or cast/convert appropriately).
- Around line 37-55: Replace hardcoded color strings in the style blocks: in
sectionStyles, replace background: white with background:
${baseTheme.colors.white} and replace border: 1px solid `#d9dde4` with border: 1px
solid ${baseTheme.colors.gray[200]}; in wizardStepCardStyles, replace
background: white with background: ${baseTheme.colors.white}. Update only the
CSS string literals for sectionStyles and wizardStepCardStyles so they use
baseTheme color tokens (baseTheme.colors.white and baseTheme.colors.gray[200])
instead of literal "white" and "#d9dde4".
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
services/main-frontend/src/app/manage/course-plans/[id]/page.tsxservices/main-frontend/src/app/manage/course-plans/[id]/schedule/components/ScheduleWizardPage.tsxservices/main-frontend/src/app/manage/course-plans/[id]/workspace/components/CoursePlanWorkspacePage.tsxservices/main-frontend/src/app/manage/course-plans/[id]/workspace/components/WorkspaceStageSection.tsxservices/main-frontend/src/app/manage/course-plans/components/CoursePlanCard.tsxservices/main-frontend/src/app/manage/course-plans/components/CoursePlansListPage.tsxshared-module/packages/common/src/utils/routes.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- services/main-frontend/src/app/manage/course-plans/components/CoursePlansListPage.tsx
- services/main-frontend/src/app/manage/course-plans/[id]/workspace/components/CoursePlanWorkspacePage.tsx
- services/main-frontend/src/app/manage/course-plans/[id]/page.tsx
- services/main-frontend/src/app/manage/course-plans/components/CoursePlanCard.tsx
Comment on lines
+19
to
+25
| const cardStyles = css` | ||
| background: white; | ||
| border: 1px solid ${baseTheme.colors.gray[200]}; | ||
| border-radius: 12px; | ||
| padding: 1.25rem; | ||
| margin-bottom: 1rem; | ||
| ` |
Contributor
There was a problem hiding this comment.
Replace hardcoded background: white with a theme color.
Per coding guidelines for **/*.tsx, colors should come from baseTheme where possible.
🎨 Proposed fix
const cardStyles = css`
- background: white;
+ background: ${baseTheme.colors.clear[100]};
border: 1px solid ${baseTheme.colors.gray[200]};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/manage/course-plans/`[id]/workspace/components/WorkspaceStageSection.tsx
around lines 19 - 25, The CSS for cardStyles currently uses a hardcoded
"background: white"; update it to use the theme color instead by replacing the
literal with the baseTheme color reference (e.g., use baseTheme.colors.white or
the appropriate white token from baseTheme) so the rule becomes background:
${baseTheme.colors.white}; keep the rest of cardStyles (border, border-radius,
padding, margin-bottom) unchanged and ensure the interpolation uses the existing
baseTheme import.
Comment on lines
90
to
95
| function formatDateRange(startsOn: string, endsOn: string): string { | ||
| const s = new Date(startsOn) | ||
| const e = new Date(endsOn) | ||
| // eslint-disable-next-line i18next/no-literal-string | ||
| return `${s.toLocaleDateString()} – ${e.toLocaleDateString()}` | ||
| } |
Contributor
There was a problem hiding this comment.
toLocaleDateString() ignores the app's i18n locale.
formatDateRange calls toLocaleDateString() with no arguments, so the formatted date follows the browser's system locale rather than the active i18n locale. This can produce inconsistent output across users whose browser locale differs from the language they selected in the app.
Consider passing the current locale from useTranslation().i18n.language through to formatDateRange, or use the Intl.DateTimeFormat API with an explicit locale.
♻️ Proposed fix
-function formatDateRange(startsOn: string, endsOn: string): string {
+function formatDateRange(startsOn: string, endsOn: string, locale: string): string {
const s = new Date(startsOn)
const e = new Date(endsOn)
// eslint-disable-next-line i18next/no-literal-string
- return `${s.toLocaleDateString()} – ${e.toLocaleDateString()}`
+ return `${s.toLocaleDateString(locale)} – ${e.toLocaleDateString(locale)}`
}Then at the call site:
+ const { t, i18n } = useTranslation()
...
- {formatDateRange(stage.planned_starts_on, stage.planned_ends_on)}
+ {formatDateRange(stage.planned_starts_on, stage.planned_ends_on, i18n.language)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/workspace/components/WorkspaceStageSection.tsx
around lines 90 - 95, formatDateRange currently calls toLocaleDateString()
without a locale, so dates follow the browser/system locale; update
formatDateRange(startsOn: string, endsOn: string) to accept a locale parameter
(e.g., locale: string) and use Intl.DateTimeFormat(locale, options).format(...)
or pass the locale into toLocaleDateString(locale, options), then propagate the
app locale from useTranslation().i18n.language at the call site in the
WorkspaceStageSection component so the rendered range uses the active i18n
language.
Comment on lines
+189
to
+199
| stage.tasks.map((task) => ( | ||
| <WorkspaceTaskRow | ||
| key={task.id} | ||
| task={task} | ||
| onToggle={(is_completed) => updateMutation.mutate({ taskId: task.id, is_completed })} | ||
| onDelete={() => deleteMutation.mutate(task.id)} | ||
| isUpdating={updateMutation.isPending} | ||
| isDeleting={deleteMutation.isPending} | ||
| /> | ||
| )) | ||
| )} |
Contributor
There was a problem hiding this comment.
Shared isUpdating/isDeleting flags disable all task rows when any single task is in flight.
updateMutation.isPending and deleteMutation.isPending are passed identically to every WorkspaceTaskRow. If the user toggles task A, every checkbox in the list is disabled while the request is in flight. Same for deletions. This prevents any parallel interaction and creates a confusing UX.
Track the in-flight task ID in local state and only disable the specific row(s) involved.
🐛 Proposed fix
In WorkspaceStageSection, track which task IDs have pending operations:
+ const [updatingId, setUpdatingId] = useState<string | null>(null)
+ const [deletingId, setDeletingId] = useState<string | null>(null)
const updateMutation = useToastMutation(
- ({ taskId, is_completed }: { taskId: string; is_completed: boolean }) =>
- updateCourseDesignerStageTask(planId, taskId, { is_completed }),
+ ({ taskId, is_completed }: { taskId: string; is_completed: boolean }) => {
+ setUpdatingId(taskId)
+ return updateCourseDesignerStageTask(planId, taskId, { is_completed })
+ },
{ notify: false },
- { onSuccess: onInvalidate },
+ { onSuccess: () => { setUpdatingId(null); onInvalidate() } },
)
const deleteMutation = useToastMutation(
- (taskId: string) => deleteCourseDesignerStageTask(planId, taskId),
+ (taskId: string) => {
+ setDeletingId(taskId)
+ return deleteCourseDesignerStageTask(planId, taskId)
+ },
{ notify: true, method: "DELETE" },
- { onSuccess: onInvalidate },
+ { onSuccess: () => { setDeletingId(null); onInvalidate() } },
)Then pass per-task flags to each row:
<WorkspaceTaskRow
key={task.id}
task={task}
onToggle={(is_completed) => updateMutation.mutate({ taskId: task.id, is_completed })}
onDelete={() => deleteMutation.mutate(task.id)}
- isUpdating={updateMutation.isPending}
- isDeleting={deleteMutation.isPending}
+ isUpdating={updatingId === task.id}
+ isDeleting={deletingId === task.id}
/>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/workspace/components/WorkspaceStageSection.tsx
around lines 189 - 199, WorkspaceStageSection currently passes
updateMutation.isPending and deleteMutation.isPending to every WorkspaceTaskRow,
disabling all rows when any task is in flight; fix this by tracking in-flight
task IDs locally in WorkspaceStageSection (e.g., a Set or separate
pendingUpdateId/pendingDeleteId state) and update that state when starting and
finishing a mutation (use updateMutation's onMutate/onSettled or wrap
mutateAsync with setting/clearing the task id in try/finally); then pass per-row
flags to WorkspaceTaskRow (e.g., isUpdating={pendingUpdateIds.has(task.id)} and
isDeleting={pendingDeleteIds.has(task.id)}) and keep the existing
onToggle/onDelete handlers but ensure they set the pending id before calling
updateMutation.mutate / deleteMutation.mutate and clear it in the mutation
callbacks.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
shared-module/packages/common/src/bindings.guard.ts (1)
926-931: ReuseisCourseDesignerStageforactive_stagechecksLine 926 and Line 1071 duplicate stage literals already covered by
isCourseDesignerStage. Reusing the helper avoids drift when stage values evolve.♻️ Suggested change
- (typedObj["active_stage"] === null || - typedObj["active_stage"] === "Analysis" || - typedObj["active_stage"] === "Design" || - typedObj["active_stage"] === "Development" || - typedObj["active_stage"] === "Implementation" || - typedObj["active_stage"] === "Evaluation") && + (typedObj["active_stage"] === null || + isCourseDesignerStage(typedObj["active_stage"]) as boolean) &&Also applies to: 1071-1076
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared-module/packages/common/src/bindings.guard.ts` around lines 926 - 931, Replace the repeated literal checks against typedObj["active_stage"] with a call to the existing helper isCourseDesignerStage to avoid duplicating stage values; specifically, where the code currently tests (typedObj["active_stage"] === null || ... === "Analysis" || "Design" || "Development" || "Implementation" || "Evaluation"), call isCourseDesignerStage(typedObj["active_stage"]) (and keep the null check if intended) and do the same replacement for the other duplicated block that mirrors those literals; update any boolean logic to preserve existing semantics (e.g., maintain the null allowance) and ensure imports/visibility for isCourseDesignerStage are correct.services/headless-lms/models/.sqlx/query-1c4c29506309533e35b2b10c53a1689d959edd5bc49b443efcb3dd36a8909165.json (1)
3-3: Limit row-lock scope for the joinedFOR UPDATEquery.On Line 3, bare
FOR UPDATElocks rows from bothcourse_designer_plansandcourse_designer_plan_membersin PostgreSQL. Since only the plan columns are returned, useFOR UPDATE OF pto lock only the plan rows and reduce lock contention. Make this change in the source Rust SQL, then regenerate this.sqlxartifact.Proposed SQL adjustment
SELECT p.id, p.created_at, p.updated_at, p.created_by_user_id, p.name, p.status AS "status: CourseDesignerPlanStatus", p.active_stage AS "active_stage: CourseDesignerStage", p.last_weekly_stage_email_sent_at FROM course_designer_plans p JOIN course_designer_plan_members m ON m.course_designer_plan_id = p.id AND m.user_id = $2 AND m.deleted_at IS NULL WHERE p.id = $1 AND p.deleted_at IS NULL -FOR UPDATE +FOR UPDATE OF p🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/headless-lms/models/.sqlx/query-1c4c29506309533e35b2b10c53a1689d959edd5bc49b443efcb3dd36a8909165.json` at line 3, The SQL query currently issues a bare FOR UPDATE which locks both course_designer_plans (alias p) and course_designer_plan_members (alias m); update the source Rust SQL so the final clause reads FOR UPDATE OF p to limit locking to the plan rows only, then re-run the sqlx generation step to regenerate the .sqlx artifact so the JSON reflects the change.
🤖 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/models/.sqlx/query-43792942081e488f680acdc89a3290f1b46fea2c4f5fc0ea754ab797376b40fa.json`:
- Line 3: The UPDATE query for course_designer_plan_stages currently always
assigns actual_started_at ($3) on any status change; change it to preserve
existing values by only setting actual_started_at when it's NULL (e.g., use
COALESCE(actual_started_at, $3) or a CASE expression) so the original start
timestamp is not overwritten. Update the query in the JSON entry named "query"
that updates course_designer_plan_stages (the statement using parameters $1..$4
and setting status and actual_started_at) to conditionally assign
actual_started_at only when absent, leaving existing timestamps intact.
- Line 3: The two UPDATE statements in the course_designer_plan_stages SQL (the
queries updating status and actual_started_at) are executed with
.execute().await? without validating the result; modify the call sites that run
these statements to capture the execution result, check rows_affected() and
enforce exactly one row was changed, and if rows_affected() == 0 return/raise a
domain-level error (e.g., NotFound or InvalidState) and if >1 treat as an
invariant violation; update the functions that call these queries to propagate
that error instead of silently succeeding.
In `@shared-module/packages/common/src/bindings.guard.ts`:
- Around line 5366-5428: Guards currently require fields like
name/title/description/is_completed to be present (possibly null) which rejects
payloads where frontend omits optional properties; update the guard predicates
(e.g., isCreateCourseDesignerPlanRequest,
isCreateCourseDesignerStageTaskRequest, isSaveCourseDesignerScheduleRequest,
isUpdateCourseDesignerStageTaskRequest, isExtendStageRequest) to treat undefined
as acceptable for those optional fields—i.e., change checks from
(typedObj["field"] === null || typeof typedObj["field"] === "string") to
(typedObj["field"] === undefined || typedObj["field"] === null || typeof
typedObj["field"] === "string") or equivalent for booleans/numbers, and ensure
array/stage validation (isSaveCourseDesignerScheduleRequest) still validates
present elements with isCourseDesignerScheduleStageInput; alternatively, if you
prefer shifting contract, update the shared request types to make those
properties optional to match the frontend instead of changing guards.
---
Nitpick comments:
In
`@services/headless-lms/models/.sqlx/query-1c4c29506309533e35b2b10c53a1689d959edd5bc49b443efcb3dd36a8909165.json`:
- Line 3: The SQL query currently issues a bare FOR UPDATE which locks both
course_designer_plans (alias p) and course_designer_plan_members (alias m);
update the source Rust SQL so the final clause reads FOR UPDATE OF p to limit
locking to the plan rows only, then re-run the sqlx generation step to
regenerate the .sqlx artifact so the JSON reflects the change.
In `@shared-module/packages/common/src/bindings.guard.ts`:
- Around line 926-931: Replace the repeated literal checks against
typedObj["active_stage"] with a call to the existing helper
isCourseDesignerStage to avoid duplicating stage values; specifically, where the
code currently tests (typedObj["active_stage"] === null || ... === "Analysis" ||
"Design" || "Development" || "Implementation" || "Evaluation"), call
isCourseDesignerStage(typedObj["active_stage"]) (and keep the null check if
intended) and do the same replacement for the other duplicated block that
mirrors those literals; update any boolean logic to preserve existing semantics
(e.g., maintain the null allowance) and ensure imports/visibility for
isCourseDesignerStage are correct.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
services/headless-lms/models/.sqlx/query-1c4c29506309533e35b2b10c53a1689d959edd5bc49b443efcb3dd36a8909165.jsonservices/headless-lms/models/.sqlx/query-1fbf00a624ec709faa946fa91da6458b3ca008d637740412ca17affb621e7d97.jsonservices/headless-lms/models/.sqlx/query-2fdb3b7acfb839af3ec2404bf705b969d9187c36675e684d01cc15e51e9586e0.jsonservices/headless-lms/models/.sqlx/query-3f9c38734f5c4903ce95b8247a5bb54ca82e4a12997d85481853597412a5a1a6.jsonservices/headless-lms/models/.sqlx/query-43792942081e488f680acdc89a3290f1b46fea2c4f5fc0ea754ab797376b40fa.jsonservices/headless-lms/models/.sqlx/query-68e1c24a5c81cccbf11f5f2ff58798754bf56630aa5dd80b39a56eb6bcb8d055.jsonservices/headless-lms/models/.sqlx/query-7b125a8722a471495e4117e5153bc3806fb100ea5a471f69243923eadd4a538c.jsonservices/headless-lms/models/.sqlx/query-84e83c10a293ef3252c5a0c5ef3ed19fed04c47c2a27d08584c09f42bc237cd0.jsonservices/headless-lms/models/.sqlx/query-98d2024e476b3d7164a4d0d111116edf60beb1c982c0e8453ae9bf976573f8fa.jsonservices/headless-lms/models/.sqlx/query-f27e0fc53bbc8a048eb2a4e01196f464667d18ff01a63b9fc291b478eb7e875e.jsonshared-module/packages/common/src/bindings.guard.tsshared-module/packages/common/src/bindings.ts
| @@ -0,0 +1,41 @@ | |||
| { | |||
| "db_name": "PostgreSQL", | |||
| "query": "\nUPDATE course_designer_plan_stages\nSET status = $2, actual_started_at = $3\nWHERE course_designer_plan_id = $1 AND stage = $4 AND deleted_at IS NULL\n", | |||
Contributor
There was a problem hiding this comment.
Preserve actual_started_at instead of overwriting it on every status change.
Line 3 currently rewrites actual_started_at for any status transition, which can erase the original start timestamp and weaken audit/data integrity.
Proposed SQL fix
-UPDATE course_designer_plan_stages
-SET status = $2, actual_started_at = $3
+UPDATE course_designer_plan_stages
+SET
+ status = $2,
+ actual_started_at = CASE
+ WHEN $2 = 'in_progress'::course_designer_plan_stage_status
+ THEN COALESCE(actual_started_at, $3)
+ ELSE actual_started_at
+ END
WHERE course_designer_plan_id = $1 AND stage = $4 AND deleted_at IS NULL🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/headless-lms/models/.sqlx/query-43792942081e488f680acdc89a3290f1b46fea2c4f5fc0ea754ab797376b40fa.json`
at line 3, The UPDATE query for course_designer_plan_stages currently always
assigns actual_started_at ($3) on any status change; change it to preserve
existing values by only setting actual_started_at when it's NULL (e.g., use
COALESCE(actual_started_at, $3) or a CASE expression) so the original start
timestamp is not overwritten. Update the query in the JSON entry named "query"
that updates course_designer_plan_stages (the statement using parameters $1..$4
and setting status and actual_started_at) to conditionally assign
actual_started_at only when absent, leaving existing timestamps intact.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the query hash in Rust code - this sqlx query file hash should be referenced
QUERY_HASH="43792942081e488f680acdc89a3290f1b46fea2c4f5fc0ea754ab797376b40fa"
echo "=== Searching for references to this query hash ==="
rg -n "$QUERY_HASH" --type=rust
echo -e "\n=== Searching for course_designer_plan_stages callsites ==="
rg -n -B3 -A3 "course_designer_plan_stages" --type=rust | head -100
echo -e "\n=== Searching for actual_started_at updates ==="
rg -n -B5 -A5 "actual_started_at" --type=rust | head -150Repository: rage/secret-project-331
Length of output: 118
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check if .sqlx directory exists ==="
find . -type d -name ".sqlx" 2>/dev/null | head -20
echo -e "\n=== Search for course_designer_plan_stages in all files ==="
rg -n "course_designer_plan_stages" --type=rust | head -50
echo -e "\n=== Look for Rust files in services/headless-lms/models ==="
find services/headless-lms/models -name "*.rs" -type f 2>/dev/null | head -30
echo -e "\n=== Search for rows_affected pattern in Rust ==="
rg -n "rows_affected\s*\(" --type=rust | head -50Repository: rage/secret-project-331
Length of output: 4209
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Check file size ==="
wc -l services/headless-lms/models/src/course_designer_plans.rs
echo -e "\n=== Look for UPDATE statements with actual_started_at ==="
rg -n -B5 -A10 "actual_started_at" services/headless-lms/models/src/course_designer_plans.rsRepository: rage/secret-project-331
Length of output: 4340
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Context around first UPDATE (lines 970-1005) ==="
sed -n '970,1005p' services/headless-lms/models/src/course_designer_plans.rs
echo -e "\n=== Context around second UPDATE (lines 1230-1270) ==="
sed -n '1230,1270p' services/headless-lms/models/src/course_designer_plans.rsRepository: rage/secret-project-331
Length of output: 2213
Validate rows_affected() for both UPDATE statements to course_designer_plan_stages.
These UPDATE queries can match zero rows if the plan/stage doesn't exist or is soft-deleted. Both execute at lines 987–997 and 1249–1259 use .execute().await? without checking the result, allowing silent state transition failures.
Enforce that exactly one row is affected, or explicitly handle zero rows as a domain error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/headless-lms/models/.sqlx/query-43792942081e488f680acdc89a3290f1b46fea2c4f5fc0ea754ab797376b40fa.json`
at line 3, The two UPDATE statements in the course_designer_plan_stages SQL (the
queries updating status and actual_started_at) are executed with
.execute().await? without validating the result; modify the call sites that run
these statements to capture the execution result, check rows_affected() and
enforce exactly one row was changed, and if rows_affected() == 0 return/raise a
domain-level error (e.g., NotFound or InvalidState) and if >1 treat as an
invariant violation; update the functions that call these queries to propagate
that error instead of silently succeeding.
Comment on lines
+5366
to
+5428
| export function isCreateCourseDesignerPlanRequest(obj: unknown): obj is CreateCourseDesignerPlanRequest { | ||
| const typedObj = obj as CreateCourseDesignerPlanRequest | ||
| return ( | ||
| (typedObj !== null && | ||
| typeof typedObj === "object" || | ||
| typeof typedObj === "function") && | ||
| (typedObj["name"] === null || | ||
| typeof typedObj["name"] === "string") | ||
| ) | ||
| } | ||
|
|
||
| export function isCreateCourseDesignerStageTaskRequest(obj: unknown): obj is CreateCourseDesignerStageTaskRequest { | ||
| const typedObj = obj as CreateCourseDesignerStageTaskRequest | ||
| return ( | ||
| (typedObj !== null && | ||
| typeof typedObj === "object" || | ||
| typeof typedObj === "function") && | ||
| typeof typedObj["title"] === "string" && | ||
| (typedObj["description"] === null || | ||
| typeof typedObj["description"] === "string") | ||
| ) | ||
| } | ||
|
|
||
| export function isExtendStageRequest(obj: unknown): obj is ExtendStageRequest { | ||
| const typedObj = obj as ExtendStageRequest | ||
| return ( | ||
| (typedObj !== null && | ||
| typeof typedObj === "object" || | ||
| typeof typedObj === "function") && | ||
| typeof typedObj["months"] === "number" | ||
| ) | ||
| } | ||
|
|
||
| export function isSaveCourseDesignerScheduleRequest(obj: unknown): obj is SaveCourseDesignerScheduleRequest { | ||
| const typedObj = obj as SaveCourseDesignerScheduleRequest | ||
| return ( | ||
| (typedObj !== null && | ||
| typeof typedObj === "object" || | ||
| typeof typedObj === "function") && | ||
| (typedObj["name"] === null || | ||
| typeof typedObj["name"] === "string") && | ||
| Array.isArray(typedObj["stages"]) && | ||
| typedObj["stages"].every((e: any) => | ||
| isCourseDesignerScheduleStageInput(e) as boolean | ||
| ) | ||
| ) | ||
| } | ||
|
|
||
| export function isUpdateCourseDesignerStageTaskRequest(obj: unknown): obj is UpdateCourseDesignerStageTaskRequest { | ||
| const typedObj = obj as UpdateCourseDesignerStageTaskRequest | ||
| return ( | ||
| (typedObj !== null && | ||
| typeof typedObj === "object" || | ||
| typeof typedObj === "function") && | ||
| (typedObj["title"] === null || | ||
| typeof typedObj["title"] === "string") && | ||
| (typedObj["description"] === null || | ||
| typeof typedObj["description"] === "string") && | ||
| (typedObj["is_completed"] === null || | ||
| typedObj["is_completed"] === false || | ||
| typedObj["is_completed"] === true) | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Frontend request interfaces (optional vs required) ==="
rg -n --type=ts 'interface (CreateCourseDesignerPlanRequest|CreateCourseDesignerStageTaskRequest|SaveCourseDesignerScheduleRequest|UpdateCourseDesignerStageTaskRequest)' \
services/main-frontend/src/services/backend/courseDesigner.ts -A8 -B1
echo
echo "=== Shared bindings request interfaces ==="
rg -n --type=ts 'interface (CreateCourseDesignerPlanRequest|CreateCourseDesignerStageTaskRequest|SaveCourseDesignerScheduleRequest|UpdateCourseDesignerStageTaskRequest)' \
shared-module/packages/common/src/bindings.ts -A8 -B1
echo
echo "=== Generated guards for these requests ==="
rg -n --type=ts 'export function is(CreateCourseDesignerPlanRequest|CreateCourseDesignerStageTaskRequest|SaveCourseDesignerScheduleRequest|UpdateCourseDesignerStageTaskRequest)' \
shared-module/packages/common/src/bindings.guard.ts -A18 -B1Repository: rage/secret-project-331
Length of output: 4993
Align Course Designer request guards with optional frontend request payloads
Frontend request interfaces define fields as optional (e.g., CreateCourseDesignerPlanRequest.name?: string | null), but shared bindings require them as present-nullable (e.g., name: string | null). Guards validate against the shared bindings signature, rejecting omitted fields. For example, line 107 of courseDesigner.ts uses createCourseDesignerPlan = async (payload: CreateCourseDesignerPlanRequest = {}), sending an empty object, but the shared type and guards require name to be present (though nullable). This causes a contract mismatch: the frontend can send partial payloads, but the shared guards and bindings expect complete records.
Align the request type definitions (either make shared bindings match frontend optionality, or ensure frontend always provides nullable values rather than omitting fields) and verify guard behavior handles both cases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shared-module/packages/common/src/bindings.guard.ts` around lines 5366 -
5428, Guards currently require fields like name/title/description/is_completed
to be present (possibly null) which rejects payloads where frontend omits
optional properties; update the guard predicates (e.g.,
isCreateCourseDesignerPlanRequest, isCreateCourseDesignerStageTaskRequest,
isSaveCourseDesignerScheduleRequest, isUpdateCourseDesignerStageTaskRequest,
isExtendStageRequest) to treat undefined as acceptable for those optional
fields—i.e., change checks from (typedObj["field"] === null || typeof
typedObj["field"] === "string") to (typedObj["field"] === undefined ||
typedObj["field"] === null || typeof typedObj["field"] === "string") or
equivalent for booleans/numbers, and ensure array/stage validation
(isSaveCourseDesignerScheduleRequest) still validates present elements with
isCourseDesignerScheduleStageInput; alternatively, if you prefer shifting
contract, update the shared request types to make those properties optional to
match the frontend instead of changing guards.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
shared-module/packages/common/src/locales/fi/main-frontend.json (1)
399-402:⚠️ Potential issue | 🟡 MinorAlign wizard terminology to one concept (
kurssivs.työ/projekti).These labels still mix terms inside the same
course-planswizard flow, which can feel inconsistent for Finnish users. Consider using one domain term consistently (likelykurssi/kurssisuunnitelma) across these keys.💡 Suggested wording alignment
- "course-plans-wizard-starts-on-month-label": "Työn alkamiskuukausi", + "course-plans-wizard-starts-on-month-label": "Kurssin alkamiskuukausi", - "course-plans-wizard-step-name": "Nimeä projekti", + "course-plans-wizard-step-name": "Nimeä kurssisuunnitelma",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared-module/packages/common/src/locales/fi/main-frontend.json` around lines 399 - 402, The Finnish strings in the course-plans wizard mix domain terms ("työ"/"projekti") with "kurssi" — update the translations for the keys course-plans-wizard-starts-on-month-label, course-plans-wizard-step-name, course-plans-wizard-step-schedule, and course-plans-wizard-step-size-and-date to consistently use one term (preferably "kurssi" or "kurssisuunnitelma"); for example replace "Nimeä projekti" with "Nimeä kurssi" and change any "Työn" or "projekti" occurrences to the chosen term so the wizard reads consistently in Finnish.
🧹 Nitpick comments (9)
services/main-frontend/src/app/manage/course-plans/[id]/workspace/components/StageSummaryHeader.tsx (2)
18-18: UnuseduseTranslationimport.The
useTranslationhook is imported but never used in this component. All text is passed as pre-translated props from the parent.-import { useTranslation } from "react-i18next"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/src/app/manage/course-plans/`[id]/workspace/components/StageSummaryHeader.tsx at line 18, Remove the unused import of useTranslation from StageSummaryHeader.tsx; locate the import line containing "useTranslation" in the StageSummaryHeader component and delete it (or remove only useTranslation from the import list) since all text is provided via props and the hook is not referenced anywhere in the component.
56-58: Replace hardcodedwhiteandrgbacolors with theme values.Per coding guidelines, colors should come from
baseTheme. Consider usingbaseTheme.colors.clear[100]for white backgrounds and theme-based shadow utilities if available.- background: white; + background: ${baseTheme.colors.clear[100]}; border: 1px solid ${baseTheme.colors.gray[200]}; - box-shadow: 0 12px 25px rgba(15, 23, 42, 0.08); + box-shadow: 0 12px 25px ${baseTheme.colors.gray[900]}14;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/manage/course-plans/`[id]/workspace/components/StageSummaryHeader.tsx around lines 56 - 58, The CSS in StageSummaryHeader uses hardcoded colors ("white" and an rgba box-shadow); replace those with theme values from baseTheme: use baseTheme.colors.clear[100] instead of "white" for the background and switch the box-shadow to either the project’s theme shadow utility or derive it from baseTheme colors (e.g., using baseTheme.colors.gray[...] for shadow color) within the same styled component or style block in StageSummaryHeader so all colors come from baseTheme.services/main-frontend/src/app/manage/course-plans/[id]/workspace/components/CompactPhaseStatusWidget.tsx (2)
82-85: Simplify pluralization using i18next's built-in count handling.i18next supports automatic pluralization with a single key. The current approach with separate keys (
course-plans-task-remainingvscourse-plans-tasks-remaining) is redundant.- const tasksText = - tasksRemainingCount === 1 - ? t("course-plans-task-remaining", { count: 1 }) - : t("course-plans-tasks-remaining", { count: tasksRemainingCount }) + const tasksText = t("course-plans-tasks-remaining", { count: tasksRemainingCount })This requires the locale file to use standard i18next plural keys (e.g.,
course-plans-tasks-remaining_oneandcourse-plans-tasks-remaining_other).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/src/app/manage/course-plans/`[id]/workspace/components/CompactPhaseStatusWidget.tsx around lines 82 - 85, Replace the manual singular/plural branching in CompactPhaseStatusWidget by using i18next's count handling: change the tasksText construction that checks tasksRemainingCount to a single call to t with the same key and a count option (use the existing t function, the tasksText variable and tasksRemainingCount), and update the locale to provide plural forms for "course-plans-tasks-remaining" (e.g., _one and _other) so i18next can resolve singular vs plural automatically.
15-15: Replace hardcodedwhitewith theme color.- background: white; + background: ${baseTheme.colors.clear[100]};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/manage/course-plans/`[id]/workspace/components/CompactPhaseStatusWidget.tsx at line 15, The component CompactPhaseStatusWidget currently uses a hardcoded CSS rule "background: white"; update that to use the design system theme instead (e.g., access theme via useTheme() or the project's theme context and replace the hardcoded value with the theme token such as theme.colors.background / theme.palette.background.paper or the equivalent token your app uses). Locate the background declaration in CompactPhaseStatusWidget.tsx (the styled block or inline style that contains "background: white") and swap it for the theme-based value so colors come from the centralized theme rather than a literal "white".services/main-frontend/src/app/manage/course-plans/[id]/workspace/components/PlanOverviewPanel.tsx (2)
92-93: Replace hardcodedwhiteandtransparentwith theme colors.Multiple style definitions use hardcoded colors. Consider using theme values consistently.
const overviewStageRowCurrentStyles = css` - background: ${baseTheme.colors.green[50]}; + background: ${baseTheme.colors.green[50]};Line 30 (
heroBlockStyles), Line 127 (overviewNodePlannedStyles), and Line 194 (overviewStatusPlannedStyles) also use hardcodedtransparentwhich is fine, butbackground: whiteat similar components in this PR should usebaseTheme.colors.clear[100].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/manage/course-plans/`[id]/workspace/components/PlanOverviewPanel.tsx around lines 92 - 93, Replace hardcoded color values in PlanOverviewPanel styles with theme tokens: change any occurrences of background: white to use baseTheme.colors.clear[100] and replace hardcoded transparent/white color usages in the style objects (notably heroBlockStyles, overviewNodePlannedStyles, overviewStatusPlannedStyles and the styles around lines 92–93) to use baseTheme color tokens (e.g., baseTheme.colors.clear[100] for white equivalents and appropriate baseTheme.colors.* for transparent/foreground) so all component colors come from the theme.
77-77: Replace hardcoded hex color with theme value.- background: `#e5e7eb`; + background: ${baseTheme.colors.gray[200]};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/manage/course-plans/`[id]/workspace/components/PlanOverviewPanel.tsx at line 77, In PlanOverviewPanel replace the hardcoded background: "#e5e7eb" with the app theme value: read the theme inside the PlanOverviewPanel component (e.g. via useTheme() or props.theme depending on your styling system) and use the appropriate token (for example theme.colors.gray[200] or theme.palette.background.paper) instead of the hex literal so the component follows the theme; update the style/CSS-in-JS declaration where background: "#e5e7eb" appears to reference the theme variable.services/main-frontend/src/app/manage/course-plans/[id]/workspace/components/CoursePlanWorkspacePage.tsx (2)
353-386: Consider extracting stage descriptions to a lookup object.The deeply nested ternary chain is hard to read and maintain. A map-based approach would be cleaner.
+const STAGE_DESCRIPTION_KEYS: Record<CourseDesignerStage, string[]> = { + Analysis: [ + "course-plans-stage-description-analysis-1", + "course-plans-stage-description-analysis-2", + "course-plans-stage-description-analysis-3", + "course-plans-stage-description-analysis-4", + "course-plans-stage-description-analysis-5", + ], + Design: [ + "course-plans-stage-description-design-1", + // ... etc + ], + // ... other stages +} - const stageDescriptionItems = - currentStage === "Analysis" - ? [...] - : currentStage === "Design" - ? [...] - : // ... etc + const stageDescriptionItems = currentStage + ? (STAGE_DESCRIPTION_KEYS[currentStage] ?? []).map((key) => t(key as any)) + : []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/main-frontend/src/app/manage/course-plans/`[id]/workspace/components/CoursePlanWorkspacePage.tsx around lines 353 - 386, Replace the nested ternary that builds stageDescriptionItems with a lookup object keyed by stage names: create a const (e.g., stageDescriptionMap) mapping "Analysis","Design","Development","Implementation","Evaluation" to arrays of translation keys or pre-translated strings, then set stageDescriptionItems = (stageDescriptionMap[currentStage] || []). If you store keys, map them through t (e.g., (stageDescriptionMap[currentStage] || []).map(k => t(k))); update references to currentStage and t inside CoursePlanWorkspacePage to use the new stageDescriptionMap for clarity and maintainability.
85-85: Replace hardcodedwhitewith theme color.- background: white; + background: ${baseTheme.colors.clear[100]};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/manage/course-plans/`[id]/workspace/components/CoursePlanWorkspacePage.tsx at line 85, The CSS in CoursePlanWorkspacePage currently uses a hardcoded "background: white"; replace that with the theme token instead: read the theme (e.g., via useTheme() or styled-system/theme) and use the appropriate background color token (for example theme.colors.background or theme.palette.background.paper) in the component's style or styled-component where "background: white" appears so the page uses the app theme rather than a hardcoded white.services/main-frontend/src/app/manage/course-plans/[id]/workspace/components/StageNavigationBar.tsx (1)
30-30: Replace hardcodedwhitewith theme color.- background: white; + background: ${baseTheme.colors.clear[100]};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/manage/course-plans/`[id]/workspace/components/StageNavigationBar.tsx at line 30, The component StageNavigationBar currently uses a hardcoded background: white; in its styles—replace that literal with the theme token instead: import or access the app theme inside StageNavigationBar (e.g., via useTheme(), ThemeProvider props, or the styled-components theme) and substitute background: white with the appropriate theme color token (for example theme.palette.background.paper or theme.colors.background / theme.colors.surface depending on your design system) so the component uses the centralized theme value.
🤖 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/course-plans/`[id]/schedule/components/StageCard.tsx:
- Around line 13-29: Replace the hardcoded background color and raw media query
in stageCardStyles and stageRowStyles with theme tokens and the
respondToOrLarger helper: swap background: `#fbfcfd` for the appropriate token
from baseTheme (e.g., baseTheme.colors.[desiredToken]) and
import/respondToOrLarger from your shared styles, then replace the "@media
(max-width: 900px) { flex-direction: column; }" block with the respondToOrLarger
breakpoint call to apply the column layout at the intended breakpoint; update
imports to include respondToOrLarger and ensure stageCardStyles and
stageRowStyles reference the theme token and helper instead of raw literals.
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/workspace/components/CoursePlanWorkspacePage.tsx:
- Around line 298-306: The lastEditedText construction uses new
Date(plan.updated_at).toLocaleDateString(undefined, ...) which can render in the
browser default locale; change it to use the app locale (i18n.language) like the
phase end date does so formatting is consistent. Update the lastEditedText logic
(where lastEditedText and plan.updated_at are used in CoursePlanWorkspacePage)
to pass i18n.language into toLocaleDateString (or otherwise derive locale from
useTranslation().i18n.language) so both dates use the same locale.
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/workspace/components/StageNavigationBar.tsx:
- Around line 115-116: In StageNavigationBar, guard against activeStage not
being found: compute the index via stages.indexOf(activeStage) into activeIndex,
then if activeIndex is -1 treat it as "not started" (e.g., set
activeIndexForProgress = 0 or directly set progressPercent = 0) before
calculating progressPercent; ensure the calculation uses the non-negative
activeIndex (or early-return 0) so progressPercent can never be negative. Use
the existing variables activeIndex, progressPercent, stages, and activeStage to
locate and update the logic.
---
Duplicate comments:
In `@shared-module/packages/common/src/locales/fi/main-frontend.json`:
- Around line 399-402: The Finnish strings in the course-plans wizard mix domain
terms ("työ"/"projekti") with "kurssi" — update the translations for the keys
course-plans-wizard-starts-on-month-label, course-plans-wizard-step-name,
course-plans-wizard-step-schedule, and course-plans-wizard-step-size-and-date to
consistently use one term (preferably "kurssi" or "kurssisuunnitelma"); for
example replace "Nimeä projekti" with "Nimeä kurssi" and change any "Työn" or
"projekti" occurrences to the chosen term so the wizard reads consistently in
Finnish.
---
Nitpick comments:
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/workspace/components/CompactPhaseStatusWidget.tsx:
- Around line 82-85: Replace the manual singular/plural branching in
CompactPhaseStatusWidget by using i18next's count handling: change the tasksText
construction that checks tasksRemainingCount to a single call to t with the same
key and a count option (use the existing t function, the tasksText variable and
tasksRemainingCount), and update the locale to provide plural forms for
"course-plans-tasks-remaining" (e.g., _one and _other) so i18next can resolve
singular vs plural automatically.
- Line 15: The component CompactPhaseStatusWidget currently uses a hardcoded CSS
rule "background: white"; update that to use the design system theme instead
(e.g., access theme via useTheme() or the project's theme context and replace
the hardcoded value with the theme token such as theme.colors.background /
theme.palette.background.paper or the equivalent token your app uses). Locate
the background declaration in CompactPhaseStatusWidget.tsx (the styled block or
inline style that contains "background: white") and swap it for the theme-based
value so colors come from the centralized theme rather than a literal "white".
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/workspace/components/CoursePlanWorkspacePage.tsx:
- Around line 353-386: Replace the nested ternary that builds
stageDescriptionItems with a lookup object keyed by stage names: create a const
(e.g., stageDescriptionMap) mapping
"Analysis","Design","Development","Implementation","Evaluation" to arrays of
translation keys or pre-translated strings, then set stageDescriptionItems =
(stageDescriptionMap[currentStage] || []). If you store keys, map them through t
(e.g., (stageDescriptionMap[currentStage] || []).map(k => t(k))); update
references to currentStage and t inside CoursePlanWorkspacePage to use the new
stageDescriptionMap for clarity and maintainability.
- Line 85: The CSS in CoursePlanWorkspacePage currently uses a hardcoded
"background: white"; replace that with the theme token instead: read the theme
(e.g., via useTheme() or styled-system/theme) and use the appropriate background
color token (for example theme.colors.background or
theme.palette.background.paper) in the component's style or styled-component
where "background: white" appears so the page uses the app theme rather than a
hardcoded white.
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/workspace/components/PlanOverviewPanel.tsx:
- Around line 92-93: Replace hardcoded color values in PlanOverviewPanel styles
with theme tokens: change any occurrences of background: white to use
baseTheme.colors.clear[100] and replace hardcoded transparent/white color usages
in the style objects (notably heroBlockStyles, overviewNodePlannedStyles,
overviewStatusPlannedStyles and the styles around lines 92–93) to use baseTheme
color tokens (e.g., baseTheme.colors.clear[100] for white equivalents and
appropriate baseTheme.colors.* for transparent/foreground) so all component
colors come from the theme.
- Line 77: In PlanOverviewPanel replace the hardcoded background: "#e5e7eb" with
the app theme value: read the theme inside the PlanOverviewPanel component (e.g.
via useTheme() or props.theme depending on your styling system) and use the
appropriate token (for example theme.colors.gray[200] or
theme.palette.background.paper) instead of the hex literal so the component
follows the theme; update the style/CSS-in-JS declaration where background:
"#e5e7eb" appears to reference the theme variable.
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/workspace/components/StageNavigationBar.tsx:
- Line 30: The component StageNavigationBar currently uses a hardcoded
background: white; in its styles—replace that literal with the theme token
instead: import or access the app theme inside StageNavigationBar (e.g., via
useTheme(), ThemeProvider props, or the styled-components theme) and substitute
background: white with the appropriate theme color token (for example
theme.palette.background.paper or theme.colors.background / theme.colors.surface
depending on your design system) so the component uses the centralized theme
value.
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/workspace/components/StageSummaryHeader.tsx:
- Line 18: Remove the unused import of useTranslation from
StageSummaryHeader.tsx; locate the import line containing "useTranslation" in
the StageSummaryHeader component and delete it (or remove only useTranslation
from the import list) since all text is provided via props and the hook is not
referenced anywhere in the component.
- Around line 56-58: The CSS in StageSummaryHeader uses hardcoded colors
("white" and an rgba box-shadow); replace those with theme values from
baseTheme: use baseTheme.colors.clear[100] instead of "white" for the background
and switch the box-shadow to either the project’s theme shadow utility or derive
it from baseTheme colors (e.g., using baseTheme.colors.gray[...] for shadow
color) within the same styled component or style block in StageSummaryHeader so
all colors come from baseTheme.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
services/main-frontend/src/app/manage/course-plans/[id]/schedule/components/StageCard.tsxservices/main-frontend/src/app/manage/course-plans/[id]/schedule/components/steps/ScheduleEditorStep.tsxservices/main-frontend/src/app/manage/course-plans/[id]/workspace/components/CompactPhaseStatusWidget.tsxservices/main-frontend/src/app/manage/course-plans/[id]/workspace/components/CoursePlanWorkspacePage.tsxservices/main-frontend/src/app/manage/course-plans/[id]/workspace/components/PlanOverviewPanel.tsxservices/main-frontend/src/app/manage/course-plans/[id]/workspace/components/StageNavigationBar.tsxservices/main-frontend/src/app/manage/course-plans/[id]/workspace/components/StageSummaryHeader.tsxservices/main-frontend/src/app/manage/course-plans/[id]/workspace/components/WorkspaceStageSection.tsxshared-module/packages/common/src/locales/en/main-frontend.jsonshared-module/packages/common/src/locales/fi/main-frontend.json
🚧 Files skipped from review as they are similar to previous changes (2)
- services/main-frontend/src/app/manage/course-plans/[id]/schedule/components/steps/ScheduleEditorStep.tsx
- shared-module/packages/common/src/locales/en/main-frontend.json
Comment on lines
+13
to
+29
| import { baseTheme } from "@/shared-module/common/styles" | ||
|
|
||
| const stageCardStyles = css` | ||
| border: 1px solid ${baseTheme.colors.gray[200]}; | ||
| border-radius: 14px; | ||
| background: #fbfcfd; | ||
| padding: 1rem; | ||
| ` | ||
|
|
||
| const stageRowStyles = css` | ||
| display: flex; | ||
| gap: 1rem; | ||
| width: 100%; | ||
|
|
||
| @media (max-width: 900px) { | ||
| flex-direction: column; | ||
| } |
Contributor
There was a problem hiding this comment.
Use theme tokens + respondToOrLarger instead of raw CSS literals.
This component uses a hardcoded background color and a raw @media query. Please switch both to shared style primitives for consistency.
♻️ Suggested fix
import { CourseDesignerStage } from "@/services/backend/courseDesigner"
import Button from "@/shared-module/common/components/Button"
import { baseTheme } from "@/shared-module/common/styles"
+import { respondToOrLarger } from "@/shared-module/common/styles/respond"
const stageCardStyles = css`
border: 1px solid ${baseTheme.colors.gray[200]};
border-radius: 14px;
- background: `#fbfcfd`;
+ background: ${baseTheme.colors.clear[100]};
padding: 1rem;
`
const stageRowStyles = css`
display: flex;
+ flex-direction: column;
gap: 1rem;
width: 100%;
- `@media` (max-width: 900px) {
- flex-direction: column;
+ ${respondToOrLarger.md} {
+ flex-direction: row;
}
`As per coding guidelines, **/*.tsx: "If a component uses colors, use colors from the theme if possible." and "If a component uses media queries, use the respondToOrLarger function."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import { baseTheme } from "@/shared-module/common/styles" | |
| const stageCardStyles = css` | |
| border: 1px solid ${baseTheme.colors.gray[200]}; | |
| border-radius: 14px; | |
| background: #fbfcfd; | |
| padding: 1rem; | |
| ` | |
| const stageRowStyles = css` | |
| display: flex; | |
| gap: 1rem; | |
| width: 100%; | |
| @media (max-width: 900px) { | |
| flex-direction: column; | |
| } | |
| import { baseTheme } from "@/shared-module/common/styles" | |
| import { respondToOrLarger } from "@/shared-module/common/styles/respond" | |
| const stageCardStyles = css` | |
| border: 1px solid ${baseTheme.colors.gray[200]}; | |
| border-radius: 14px; | |
| background: ${baseTheme.colors.clear[100]}; | |
| padding: 1rem; | |
| ` | |
| const stageRowStyles = css` | |
| display: flex; | |
| flex-direction: column; | |
| gap: 1rem; | |
| width: 100%; | |
| ${respondToOrLarger.md} { | |
| flex-direction: row; | |
| } | |
| ` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/schedule/components/StageCard.tsx
around lines 13 - 29, Replace the hardcoded background color and raw media query
in stageCardStyles and stageRowStyles with theme tokens and the
respondToOrLarger helper: swap background: `#fbfcfd` for the appropriate token
from baseTheme (e.g., baseTheme.colors.[desiredToken]) and
import/respondToOrLarger from your shared styles, then replace the "@media
(max-width: 900px) { flex-direction: column; }" block with the respondToOrLarger
breakpoint call to apply the column layout at the intended breakpoint; update
imports to include respondToOrLarger and ensure stageCardStyles and
stageRowStyles reference the theme token and helper instead of raw literals.
Comment on lines
+298
to
+306
| const lastEditedText = plan.updated_at | ||
| ? t("course-plans-last-edited", { | ||
| time: new Date(plan.updated_at).toLocaleDateString(undefined, { | ||
| day: "numeric", | ||
| month: "short", | ||
| year: "numeric", | ||
| }), | ||
| }) | ||
| : null |
Contributor
There was a problem hiding this comment.
**Inconsistent locale usage in date formatting.**Line 300 uses toLocaleDateString(undefined, ...) which "depends on the implementation, the default locale, and the default time zone", while line 310 correctly uses i18n.language. This inconsistency means the "last edited" date may display in the browser's locale while the phase end date uses the app's selected locale.
const lastEditedText = plan.updated_at
? t("course-plans-last-edited", {
- time: new Date(plan.updated_at).toLocaleDateString(undefined, {
+ time: new Date(plan.updated_at).toLocaleDateString(i18n.language, {
day: "numeric",
month: "short",
year: "numeric",
}),
})
: null📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const lastEditedText = plan.updated_at | |
| ? t("course-plans-last-edited", { | |
| time: new Date(plan.updated_at).toLocaleDateString(undefined, { | |
| day: "numeric", | |
| month: "short", | |
| year: "numeric", | |
| }), | |
| }) | |
| : null | |
| const lastEditedText = plan.updated_at | |
| ? t("course-plans-last-edited", { | |
| time: new Date(plan.updated_at).toLocaleDateString(i18n.language, { | |
| day: "numeric", | |
| month: "short", | |
| year: "numeric", | |
| }), | |
| }) | |
| : null |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/workspace/components/CoursePlanWorkspacePage.tsx
around lines 298 - 306, The lastEditedText construction uses new
Date(plan.updated_at).toLocaleDateString(undefined, ...) which can render in the
browser default locale; change it to use the app locale (i18n.language) like the
phase end date does so formatting is consistent. Update the lastEditedText logic
(where lastEditedText and plan.updated_at are used in CoursePlanWorkspacePage)
to pass i18n.language into toLocaleDateString (or otherwise derive locale from
useTranslation().i18n.language) so both dates use the same locale.
Comment on lines
+115
to
+116
| const activeIndex = stages.indexOf(activeStage) | ||
| const progressPercent = stages.length > 0 ? ((activeIndex + 0.5) / stages.length) * 100 : 0 |
Contributor
There was a problem hiding this comment.
Guard against negative progress percentage when activeStage is not found.
If activeStage is not in the stages array, indexOf returns -1, resulting in a negative progress percentage.
const activeIndex = stages.indexOf(activeStage)
- const progressPercent = stages.length > 0 ? ((activeIndex + 0.5) / stages.length) * 100 : 0
+ const progressPercent =
+ stages.length > 0 && activeIndex >= 0 ? ((activeIndex + 0.5) / stages.length) * 100 : 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@services/main-frontend/src/app/manage/course-plans/`[id]/workspace/components/StageNavigationBar.tsx
around lines 115 - 116, In StageNavigationBar, guard against activeStage not
being found: compute the index via stages.indexOf(activeStage) into activeIndex,
then if activeIndex is -1 treat it as "not started" (e.g., set
activeIndexForProgress = 0 or directly set progressPercent = 0) before
calculating progressPercent; ensure the calculation uses the non-negative
activeIndex (or early-return 0) so progressPercent can never be negative. Use
the existing variables activeIndex, progressPercent, stages, and activeStage to
locate and update the logic.
- Updated CoursePlanWorkspacePage to allow extending the current stage with a specified number of months. - Modified PlanOverviewPanel to include a dialog for adjusting the schedule, enabling users to select the number of months to extend. - Added new localization strings for the schedule adjustment feature in main-frontend.json.
- Introduced a responsive grid layout for the CoursePlanWorkspacePage to improve the organization of components. - Added new styles for various sections including header, instructions, tasks, workspace, and chatbot areas. - Integrated BreakFromCentered component for better layout management. - Updated localization strings to enhance accessibility for screen readers, including aria-labels for workspace and assistant components.
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
Tests
Chores