Ref: Introduce minor UX related improvements i.e. localization, navigation locations#293
Open
Marty-Byrde wants to merge 27 commits into
Open
Ref: Introduce minor UX related improvements i.e. localization, navigation locations#293Marty-Byrde wants to merge 27 commits into
Marty-Byrde wants to merge 27 commits into
Conversation
Previously the width of the grid-cell that shows the practice question, practice progress had a max-width of 4/5 on large screens. Now the max-w is slightly smaller on larger screens, namely to 50% of the viewport width.
This way the auto-generated locale-files and the `.json` files that are manually edited are always in sync when changes are committed.
The reason for this change is that previously the edit-course page showed the generic breadcrumb which showed the actual course id. Since the course-id is not a good breadcrumb the name is used instead, thus a custom Breadcrumbs were defined in the new layout to override the generic breadcrumbs.
The reason for this change is that previously users able to delete all answer-options when creating a drag-drop question. To prevent this edge-case a min constraint was introduces to the respective schema to disallow this case.
The reason for this change is that this way the general errors for choice- and drag-drop answer errors also include an error-icon, just like field-errors using the `Field` component do.
This way users on mobile devices are also able to access tooltips. At the same time this allows users on desktop screens to also toggle tooltips.
This way fields for e.g. descriptions can be rendered as a Textarea instead of a single-line input. This can be done by setting the `variant` to `textarea`.
This way the descriptions fields used in the `GeneralSection` and the `CourseContentDialog` are now rendering a textarea instead of a single-line input.
Previously the `stripZodDefaultValues` utility function stripped all effects from a given array when stripping it from default values. To prevent potential issues because of it in the future, the stripped checks are now re-applied.
Previously the `schemaDefaults` utility function logged "unknown schema type" when a property was of type `any`, `unknown` or `undefined`. However, since all these types are instantiated as `undefined` it is not worth logging.
Contributor
WalkthroughAdds translation sync to pre-commit and npm scripts, introduces a textarea variant for the shared Field, adds pinnable Tooltip state, converts multiple breadcrumb components to server-side i18n, adds an edit layout that fetches a course, tightens Zod array handling and strip-default logic, and adjusts several UI layout/props. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
This development build script shows all build errors and not just a single one.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![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: 3
🧹 Nitpick comments (5)
src/app/[locale]/courses/edit/[id]/layout.tsx (1)
14-32: InconsistentBreadcrumbItemwrapping pattern.The "Home" link (line 16-18) and
BreadcrumbPage(line 30) are not wrapped inBreadcrumbItem, while "Courses" and "Edit" are. For consistent styling and semantic structure, all breadcrumb entries should be wrapped inBreadcrumbItem.♻️ Proposed fix for consistent structure
<Breadcrumb> <BreadcrumbList> - <BreadcrumbLink asChild> - <Link href={`/`}>Home</Link> - </BreadcrumbLink> + <BreadcrumbItem> + <BreadcrumbLink asChild> + <Link href='/'>Home</Link> + </BreadcrumbLink> + </BreadcrumbItem> <BreadcrumbSeparator /> <BreadcrumbItem> <BreadcrumbLink asChild> - <Link href={`/courses`}>Courses</Link> + <Link href='/courses'>Courses</Link> </BreadcrumbLink> </BreadcrumbItem> <BreadcrumbSeparator /> <BreadcrumbItem>Edit</BreadcrumbItem> <BreadcrumbSeparator /> - <BreadcrumbPage>{course.name}</BreadcrumbPage> + <BreadcrumbItem> + <BreadcrumbPage>{course.name}</BreadcrumbPage> + </BreadcrumbItem> </BreadcrumbList> </Breadcrumb>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[locale]/courses/edit/[id]/layout.tsx around lines 14 - 32, The breadcrumb markup is inconsistent: wrap the "Home" link and the final page title so every entry uses BreadcrumbItem like the others. Update the JSX around Breadcrumb/BreadcrumbList to place <BreadcrumbItem> around the Home <BreadcrumbLink asChild><Link href={`/`}>Home</Link></BreadcrumbLink> and around <BreadcrumbPage>{course.name}</BreadcrumbPage>, keeping BreadcrumbSeparator placements the same and preserving existing BreadcrumbLink/BreadcrumbPage components.src/schemas/QuestionSchema.ts (1)
114-114: Translation key reuse fromSingleChoicenamespace inDragDropschema.The validation correctly enforces a minimum of one answer, but uses
schemas.Question.SingleChoice.answers.min_answer_countfor a drag-drop question. For consistency withgetMultipleChoiceAnswerSchema(line 68), which has its own key (schemas.Question.MultipleChoice.answers.min_constraint), consider either:
- Adding a dedicated
DragDroptranslation key, or- Using a shared key under
schemas.Question.Shared.answers.min_answer_countThe current approach works but may confuse translators or future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/schemas/QuestionSchema.ts` at line 114, The DragDrop answer schema is reusing the SingleChoice translation key; update the validation in getDragDropAnswerSchema to reference a DragDrop-specific or shared key instead (e.g., 'schemas.Question.DragDrop.answers.min_answer_count' or 'schemas.Question.Shared.answers.min_answer_count') and add the matching translation entry(s) to the locale files so translators see the correct scope; ensure the t(...) call in the .min(...) validator is updated to the chosen key.src/components/courses/create/(create-question)/CreateQuestionDialog.tsx (1)
467-467: Consider fixing the type parameter while modifying this line.This line uses
<FieldError<ChoiceQuestion>insideDragDropQuestionAnswers, but should useDragDropQuestionfor type consistency:♻️ Suggested fix
- <FieldError<ChoiceQuestion> field='answers' errors={errors} showIcon /> + <FieldError<DragDropQuestion> field='answers' errors={errors} showIcon />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/courses/create/`(create-question)/CreateQuestionDialog.tsx at line 467, The FieldError generic is using the wrong type: change the type parameter from ChoiceQuestion to DragDropQuestion for the FieldError used inside DragDropQuestionAnswers in CreateQuestionDialog.tsx; locate the FieldError component instance (FieldError<ChoiceQuestion> field='answers' errors={errors} showIcon) and replace the generic so it becomes FieldError<DragDropQuestion>, ensuring the type aligns with the DragDropQuestionAnswers component and related form data types.src/components/Shared/form/Field.tsx (1)
100-116: MultipleAnycasts reduce type safety in event handlers.The casts on lines 102, 108, and 112 are pragmatic workarounds for the discriminated union, but they suppress type checking. Consider narrowing the event handler types based on variant:
♻️ Optional: Type-narrowed event handlers
onFocus={(e) => { setIsFocused(true) - props.onFocus?.(e as Any) + ;(props as { onFocus?: (e: typeof e) => void }).onFocus?.(e) }} onBlur={(e) => { setIsFocused(false) - props.onBlur?.(e as Any) + ;(props as { onBlur?: (e: typeof e) => void }).onBlur?.(e) field.onBlur() }}This preserves type inference while avoiding the broad
Anyescape hatch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Shared/form/Field.tsx` around lines 100 - 116, The event handlers in the ControlledComponent use broad Any casts (props.onFocus?.(e as Any), props.onBlur?.(e as Any), and ControlledComponent props spread) which suppress type checking; update the handlers to narrow the event types instead: derive the correct React event type from ControlledComponent (e.g., React.FocusEvent<HTMLInputElement> or the component's input type) and type the local onFocus/onBlur callback parameters accordingly, replace the Any casts with properly typed parameters when calling props.onFocus and props.onBlur, and ensure fieldOnChange uses the matching ChangeEvent type so you can remove the Any casts and preserve type safety around ControlledComponent, setIsFocused, field.onBlur, and fieldOnChange.src/components/Shared/Tooltip.tsx (1)
19-24: Effect may skip necessary updates due to staleisPinnedreference.The effect reads
isPinnedbut excludes it from dependencies (with eslint-disable). Whenconfig.openchanges but the closure still holds a staleisPinnedvalue, theconfig.open === isPinnedcheck may incorrectly returntrue, skipping the update.Consider storing the previous
config.openvalue instead:♻️ Suggested fix using previous value tracking
- useEffect(() => { - if (config.open === undefined || config.open === isPinned) return - - // eslint-disable-next-line react-hooks/set-state-in-effect - setPinned(config.open) - }, [config.open]) + const prevConfigOpen = useRef(config.open) + useEffect(() => { + if (config.open !== undefined && config.open !== prevConfigOpen.current) { + setPinned(config.open) + } + prevConfigOpen.current = config.open + }, [config.open])This requires importing
useRefas well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Shared/Tooltip.tsx` around lines 19 - 24, The effect currently reads isPinned but omits it from the dependency array, causing stale-closure bugs; change the effect to track the previous config.open using a useRef (import useRef) — e.g., create prevOpenRef, compare prevOpenRef.current to config.open and only run setPinned(config.open) when they differ (and update prevOpenRef.current = config.open afterwards); update the effect dependencies to include config.open (and remove the eslint-disable) and keep using setPinned and isPinned as the state references inside Tooltip.tsx.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.husky/pre-commit:
- Around line 4-5: The pre-commit hook currently runs "yarn sync-translations"
then stages only "git add src/i18n/locales/*.ts", but the
prepare-i18n_ally-locales.ts script also writes JSON files; update the hook so
it stages JSON as well (e.g., change the git add to include both ts and json
like "git add src/i18n/locales/*.{ts,json}" or add an additional "git add
src/i18n/locales/*.json" line) to ensure generated locale JSON files are not
omitted from commits.
In `@src/components/courses/`(hamburger-menu)/CourseActionMenu.tsx:
- Around line 149-153: The Link in CourseActionMenu uses the wrong route and
doesn't guard against a null share_key; change the navigation to use the
gatherShareToken pattern (the same helper used elsewhere in this component) to
build the path /courses/{share_token}/results and disable the DropdownMenuItem
when gatherShareToken returns null/undefined. Update the item that currently
links to `/checks/${share_key}/results` to call gatherShareToken (or the local
equivalent), render the Link only when a token exists, and apply the same
disabled styling/prop used by other menu items so the menu entry is
non-interactive when the token is missing.
In `@src/components/Shared/Tooltip.tsx`:
- Around line 36-38: The TooltipTrigger currently passes onClick with asChild
which toggles setPinned even when interactive children handle their own clicks;
update TooltipTrigger usage in Tooltip.tsx so the onClick is either removed or
only attached for non-interactive children: detect interactive children (e.g.,
elements with role/button, onClick/onPress/onPointerDown props, or known
components like Toggle) and avoid merging the onClick into those, or wrap
children in a non-interactive wrapper element and attach the toggle handler
there; reference TooltipTrigger, setPinned and children to locate the code and
implement conditional attachment or a dedicated non-interactive wrapper to
prevent conflicting handlers.
---
Nitpick comments:
In `@src/app/`[locale]/courses/edit/[id]/layout.tsx:
- Around line 14-32: The breadcrumb markup is inconsistent: wrap the "Home" link
and the final page title so every entry uses BreadcrumbItem like the others.
Update the JSX around Breadcrumb/BreadcrumbList to place <BreadcrumbItem> around
the Home <BreadcrumbLink asChild><Link href={`/`}>Home</Link></BreadcrumbLink>
and around <BreadcrumbPage>{course.name}</BreadcrumbPage>, keeping
BreadcrumbSeparator placements the same and preserving existing
BreadcrumbLink/BreadcrumbPage components.
In `@src/components/courses/create/`(create-question)/CreateQuestionDialog.tsx:
- Line 467: The FieldError generic is using the wrong type: change the type
parameter from ChoiceQuestion to DragDropQuestion for the FieldError used inside
DragDropQuestionAnswers in CreateQuestionDialog.tsx; locate the FieldError
component instance (FieldError<ChoiceQuestion> field='answers' errors={errors}
showIcon) and replace the generic so it becomes FieldError<DragDropQuestion>,
ensuring the type aligns with the DragDropQuestionAnswers component and related
form data types.
In `@src/components/Shared/form/Field.tsx`:
- Around line 100-116: The event handlers in the ControlledComponent use broad
Any casts (props.onFocus?.(e as Any), props.onBlur?.(e as Any), and
ControlledComponent props spread) which suppress type checking; update the
handlers to narrow the event types instead: derive the correct React event type
from ControlledComponent (e.g., React.FocusEvent<HTMLInputElement> or the
component's input type) and type the local onFocus/onBlur callback parameters
accordingly, replace the Any casts with properly typed parameters when calling
props.onFocus and props.onBlur, and ensure fieldOnChange uses the matching
ChangeEvent type so you can remove the Any casts and preserve type safety around
ControlledComponent, setIsFocused, field.onBlur, and fieldOnChange.
In `@src/components/Shared/Tooltip.tsx`:
- Around line 19-24: The effect currently reads isPinned but omits it from the
dependency array, causing stale-closure bugs; change the effect to track the
previous config.open using a useRef (import useRef) — e.g., create prevOpenRef,
compare prevOpenRef.current to config.open and only run setPinned(config.open)
when they differ (and update prevOpenRef.current = config.open afterwards);
update the effect dependencies to include config.open (and remove the
eslint-disable) and keep using setPinned and isPinned as the state references
inside Tooltip.tsx.
In `@src/schemas/QuestionSchema.ts`:
- Line 114: The DragDrop answer schema is reusing the SingleChoice translation
key; update the validation in getDragDropAnswerSchema to reference a
DragDrop-specific or shared key instead (e.g.,
'schemas.Question.DragDrop.answers.min_answer_count' or
'schemas.Question.Shared.answers.min_answer_count') and add the matching
translation entry(s) to the locale files so translators see the correct scope;
ensure the t(...) call in the .min(...) validator is updated to the chosen key.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9818ae40-ce4f-48c1-86c5-cea522d09f7a
📒 Files selected for processing (13)
.husky/pre-commitpackage.jsonsrc/app/[locale]/courses/[share_token]/practice/page.tsxsrc/app/[locale]/courses/edit/[id]/layout.tsxsrc/components/Shared/Tooltip.tsxsrc/components/Shared/form/Field.tsxsrc/components/courses/(hamburger-menu)/CourseActionMenu.tsxsrc/components/courses/create/(create-question)/CreateQuestionDialog.tsxsrc/components/courses/create/(sections)/CourseContentDialog.tsxsrc/components/courses/create/(sections)/GeneralSection.tsxsrc/schemas/QuestionSchema.tssrc/schemas/utils/schemaDefaults.tssrc/schemas/utils/stripZodDefaultValues.ts
💤 Files with no reviewable changes (1)
- src/schemas/utils/schemaDefaults.ts
This way these custom breadcrumbs align with the other breadcrumbs shown on other pages, which all start from the root page.
The `GenericBreadcrumb` component now displays the translation for a given page-segement-key or if it does not exist it shows the segment (key) as before.
The reason for this is that with the introduction of the new `content` stage the index of the remaining stages was increased by one. This meant that the `Jumpback` button redirected users to the wrong stage when clicked. This issue is now resolved.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/Shared/StopwatchTime.tsx (1)
15-27:⚠️ Potential issue | 🟡 MinorDefault delimiter remains English for German output
When no
delimiterprop is passed, Line 14 still defaults to' and ', so German duration units can render with an English conjunction (mixed-language UI).💡 Suggested fix
-export function StopwatchTime({ start: rawStartDate, delimiter = ' and ' }: { start: Date; delimiter?: string }) { +export function StopwatchTime({ start: rawStartDate, delimiter }: { start: Date; delimiter?: string }) { const currentLocale = useCurrentLocale() + const effectiveDelimiter = delimiter ?? (currentLocale === 'de' ? ' und ' : ' and ') @@ - { zero: true, delimiter, locale: currentLocale === 'de' ? de : enUS }, + { zero: true, delimiter: effectiveDelimiter, locale: currentLocale === 'de' ? de : enUS }, ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Shared/StopwatchTime.tsx` around lines 15 - 27, The code uses a fixed/default English delimiter so German output can include an English conjunction; update the StopwatchTime component to compute a locale-aware delimiter (e.g., compute a localDelimiter from the existing delimiter prop or fallback to ' und ' when currentLocale === 'de' and ' and ' otherwise) and pass that computed localDelimiter into the formatDuration call in computeDifference (references: currentLocale, delimiter, computeDifference, formatDuration). Ensure you use the computed localDelimiter everywhere formatDuration is called so German durations render with the correct conjunction.
🧹 Nitpick comments (4)
src/components/courses/[share_token]/practice/PracticeBreadcrumbs.tsx (2)
53-53: Avoid runtime string manipulation on translated values.Applying
.toLowerCase().replace(/ /g, '-')to a translated string is fragile across locales—different languages may have varying spacing, casing rules, or special characters. Consider either:
- Adding a separate i18n key for the slug/display format
- Using the raw
categoryvalue for display when it's_none_/undefined🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/courses/`[share_token]/practice/PracticeBreadcrumbs.tsx at line 53, The breadcrumb currently transforms the translated label with .toLowerCase().replace(/ /g, '-') which is fragile; update the BreadcrumbPage rendering logic (where BreadcrumbPage is used and the variable category is checked) to stop mutating the i18n string—either introduce and use a new i18n key for the slug/display form (e.g., a separate practice_category_all_slug key) or display the raw category value when category === '_none_' || category === undefined and leave the translation untouched (use t('practice_category_all_label') only for display without toLowerCase/replace).
40-41: Consider making "practice" a navigable link for consistency.The "practice" breadcrumb item is plain text while other items are links. If there's a practice landing/overview page, consider wrapping this in a
BreadcrumbLinkfor consistent navigation UX.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/courses/`[share_token]/practice/PracticeBreadcrumbs.tsx around lines 40 - 41, The "practice" breadcrumb is currently rendered as plain text (BreadcrumbItem with t('practice')); change it to a navigable item by wrapping it in a BreadcrumbLink (e.g., <BreadcrumbItem><BreadcrumbLink href={...}>{t('practice')}</BreadcrumbLink></BreadcrumbItem>) so it matches other breadcrumb items in PracticeBreadcrumbs.tsx and points to the practice landing/overview route for the current share_token context; ensure the href/route generation uses the same token/params logic as the other breadcrumb links in this component.src/components/Shared/Breadcrumb/GenericBreadcrumb.tsx (1)
44-52: Dynamic segment translation may fall back to raw keys for unknown paths.Using
t(segment as Any)assumes every URL segment has a corresponding i18n key inShared.Breadcrumbs. If a segment (e.g., a dynamic ID like a UUID) lacks a translation key, the behavior depends on the i18n library configuration—it may display the raw key string or an empty value.Consider adding a fallback mechanism to handle untranslated segments gracefully:
💡 Suggested fallback pattern
- <BreadcrumbPage>{t(segment as Any)}</BreadcrumbPage> + <BreadcrumbPage>{t(segment as Any) || segment}</BreadcrumbPage> ) : ( <BreadcrumbLink asChild> - <Link href={'/' + pages.slice(0, i + 1).join('/')}>{t(segment as Any)}</Link> + <Link href={'/' + pages.slice(0, i + 1).join('/')}>{t(segment as Any) || segment}</Link>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Shared/Breadcrumb/GenericBreadcrumb.tsx` around lines 44 - 52, The breadcrumb currently calls t(segment as Any) for every URL segment (in GenericBreadcrumb.tsx inside the pages?.map loop, around BreadcrumbPage/BreadcrumbLink and isCurrentPage), which can render raw keys for dynamic segments; change the rendering to first check whether a translation exists (or if the segment looks dynamic like a UUID/ID) and if not use a safe fallback such as a humanized/capitalized version of the segment (or pass a defaultValue to the i18n call). Update the map rendering to use that existence-check + fallback approach for both BreadcrumbPage and BreadcrumbLink so untranslated segments show a readable label instead of the raw key.src/components/courses/create/(sections)/SettingsSection.tsx (1)
41-41: Replace hard-coded stage indices with shared stage constants.Line 41 introduces another literal stage index (
4). BecauseCardStageJumpButtonpasses this directly (src/components/Shared/CardStageJumpButton.tsx:13-27) andsetStageis unbounded (src/hooks/Shared/MultiStage/MultiStageStore.ts:30-44), future stage reordering can silently break navigation.Proposed refactor
+const COURSE_CREATE_STAGE = { + QUESTIONS: 3, + SETTINGS: 4, +} as const + export default function SettingsSection({ jumpBackButtons, className, ...config }: { jumpBackButtons?: boolean; className?: string } & Omit<UseFormProps<CourseSettings>, 'resolver' | 'defaultValues'>) { @@ - {jumpBackButtons && <CardStageJumpButton targetStage={4} />} + {jumpBackButtons && <CardStageJumpButton targetStage={COURSE_CREATE_STAGE.SETTINGS} />} @@ - {jumpBackButtons && <CardStageJumpButton targetStage={3} />} + {jumpBackButtons && <CardStageJumpButton targetStage={COURSE_CREATE_STAGE.QUESTIONS} />} @@ - {jumpBackButtons && <CardStageJumpButton targetStage={3} />} + {jumpBackButtons && <CardStageJumpButton targetStage={COURSE_CREATE_STAGE.QUESTIONS} />}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/courses/create/`(sections)/SettingsSection.tsx at line 41, SettingsSection currently uses a hard-coded stage index (4) when rendering <CardStageJumpButton targetStage={4} />, which can break navigation if stages are reordered; replace the literal with a shared stage constant (e.g., import and use a named constant like COURSE_SETTINGS_STAGE or STAGES.Settings) so the jump target is driven by the same enum used by MultiStageStore and setStage, update the import in SettingsSection to reference that shared stage constant, and ensure any other usages of the numeric literal (including in CardStageJumpButton calls) are migrated to the same shared constant to keep navigation stable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/courses/`[share_token]/practice/PracticeBreadcrumbs.tsx:
- Around line 33-37: Breadcrumb item currently links to the edit page which may
be inaccessible to practice participants; update the Link href in the
PracticeBreadcrumbs component (the BreadcrumbLink containing Link with
courseId/courseName) to point to a view-only route such as
`/courses/${courseId}` or the practice landing route like
`/courses/${courseId}/practice` instead of `/courses/${courseId}/edit`, and if
your app requires, add a conditional to render the edit link only when the
current user has edit permission.
In `@src/components/results/practice/PracticeResultsBreadcrumbs.tsx`:
- Around line 29-31: The breadcrumb components are nested incorrectly in
PracticeResultsBreadcrumbs: currently BreadcrumbPage wraps BreadcrumbItem;
update the JSX so BreadcrumbItem is the outer wrapper and BreadcrumbPage is
inside it (i.e., wrap BreadcrumbPage with BreadcrumbItem and keep the translated
label t('results') inside BreadcrumbPage) to match the pattern used by
PracticeBreadcrumbs and shadcn/ui.
---
Outside diff comments:
In `@src/components/Shared/StopwatchTime.tsx`:
- Around line 15-27: The code uses a fixed/default English delimiter so German
output can include an English conjunction; update the StopwatchTime component to
compute a locale-aware delimiter (e.g., compute a localDelimiter from the
existing delimiter prop or fallback to ' und ' when currentLocale === 'de' and '
and ' otherwise) and pass that computed localDelimiter into the formatDuration
call in computeDifference (references: currentLocale, delimiter,
computeDifference, formatDuration). Ensure you use the computed localDelimiter
everywhere formatDuration is called so German durations render with the correct
conjunction.
---
Nitpick comments:
In `@src/components/courses/`[share_token]/practice/PracticeBreadcrumbs.tsx:
- Line 53: The breadcrumb currently transforms the translated label with
.toLowerCase().replace(/ /g, '-') which is fragile; update the BreadcrumbPage
rendering logic (where BreadcrumbPage is used and the variable category is
checked) to stop mutating the i18n string—either introduce and use a new i18n
key for the slug/display form (e.g., a separate practice_category_all_slug key)
or display the raw category value when category === '_none_' || category ===
undefined and leave the translation untouched (use
t('practice_category_all_label') only for display without toLowerCase/replace).
- Around line 40-41: The "practice" breadcrumb is currently rendered as plain
text (BreadcrumbItem with t('practice')); change it to a navigable item by
wrapping it in a BreadcrumbLink (e.g., <BreadcrumbItem><BreadcrumbLink
href={...}>{t('practice')}</BreadcrumbLink></BreadcrumbItem>) so it matches
other breadcrumb items in PracticeBreadcrumbs.tsx and points to the practice
landing/overview route for the current share_token context; ensure the
href/route generation uses the same token/params logic as the other breadcrumb
links in this component.
In `@src/components/courses/create/`(sections)/SettingsSection.tsx:
- Line 41: SettingsSection currently uses a hard-coded stage index (4) when
rendering <CardStageJumpButton targetStage={4} />, which can break navigation if
stages are reordered; replace the literal with a shared stage constant (e.g.,
import and use a named constant like COURSE_SETTINGS_STAGE or STAGES.Settings)
so the jump target is driven by the same enum used by MultiStageStore and
setStage, update the import in SettingsSection to reference that shared stage
constant, and ensure any other usages of the numeric literal (including in
CardStageJumpButton calls) are migrated to the same shared constant to keep
navigation stable.
In `@src/components/Shared/Breadcrumb/GenericBreadcrumb.tsx`:
- Around line 44-52: The breadcrumb currently calls t(segment as Any) for every
URL segment (in GenericBreadcrumb.tsx inside the pages?.map loop, around
BreadcrumbPage/BreadcrumbLink and isCurrentPage), which can render raw keys for
dynamic segments; change the rendering to first check whether a translation
exists (or if the segment looks dynamic like a UUID/ID) and if not use a safe
fallback such as a humanized/capitalized version of the segment (or pass a
defaultValue to the i18n call). Update the map rendering to use that
existence-check + fallback approach for both BreadcrumbPage and BreadcrumbLink
so untranslated segments show a readable label instead of the raw key.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9f2dff58-8146-4235-8b49-b1a7d060e424
📒 Files selected for processing (13)
package.jsonsrc/app/[locale]/courses/[share_token]/practice/page.tsxsrc/components/Shared/Breadcrumb/GenericBreadcrumb.tsxsrc/components/Shared/StopwatchTime.tsxsrc/components/courses/(hamburger-menu)/CourseActionMenu.tsxsrc/components/courses/[share_token]/practice/PracticeBreadcrumbs.tsxsrc/components/courses/create/(sections)/QuestionsSection.tsxsrc/components/courses/create/(sections)/SettingsSection.tsxsrc/components/results/practice/PracticeResultsBreadcrumbs.tsxsrc/i18n/locales/de.jsonsrc/i18n/locales/de.tssrc/i18n/locales/en.jsonsrc/i18n/locales/en.ts
✅ Files skipped from review due to trivial changes (5)
- src/components/courses/(hamburger-menu)/CourseActionMenu.tsx
- src/i18n/locales/en.ts
- src/i18n/locales/de.json
- src/i18n/locales/en.json
- src/i18n/locales/de.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- src/app/[locale]/courses/[share_token]/practice/page.tsx
The reason for this change is that previously the editor-pane (`EditorContent`) in some cases causes its container to grow outside of its max-height constraint. By removing an obsolete outer wrapper and moving the constraints directly to the editor pane these issues are resolved.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/tiptap-examples/RichTextEditor.tsx (1)
134-156: The'content'growth option has no effect.The JSDoc documents
growthas accepting'content' | 'fill', but only'fill'has explicit handling (Line 195). The'content'option behaves identically toundefined. Consider either:
- Simplifying to
growth?: 'fill'or a boolean likefillContainer?: boolean- Or documenting
'content'as the explicit default for clarityThis is a minor API design consideration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/tiptap-examples/RichTextEditor.tsx` around lines 134 - 156, The growth prop currently accepts 'content' | 'fill' but only 'fill' is handled, so simplify the API by replacing growth with a boolean fillContainer prop: update the JSDoc and the RichTextEditor signature (remove size?: 'sm'|'md'|'lg' growth?: 'content'|'fill' and add fillContainer?: boolean), change any defaulting to use fillContainer (previously growth === 'fill'), and update rendering logic that checks growth to instead check fillContainer; also update all callers and tests to pass fillContainer where they previously passed growth:'fill' or leave undefined for content behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/tiptap-examples/RichTextEditor.tsx`:
- Line 214: The new default max-h-[30dvh] on RichTextEditor (in the className
applied where EditorContent is rendered) restricts CourseContentDialog
unexpectedly; update either the default or the dialog usage: in
CourseContentDialog pass growth='fill' and provide an editorPaneClassname that
overrides the height (e.g., matching ContentRenderer's max-h-[58dvh]) when
instantiating RichTextEditor, or remove/relax the hardcoded max-h-[30dvh] in
RichTextEditor so edit forms are not forced to 30dvh; modify the instantiation
in CourseContentDialog (the place that renders <RichTextEditor ... />) or adjust
the className logic in RichTextEditor to honor an editorPaneClassname prop.
---
Nitpick comments:
In `@src/components/tiptap-examples/RichTextEditor.tsx`:
- Around line 134-156: The growth prop currently accepts 'content' | 'fill' but
only 'fill' is handled, so simplify the API by replacing growth with a boolean
fillContainer prop: update the JSDoc and the RichTextEditor signature (remove
size?: 'sm'|'md'|'lg' growth?: 'content'|'fill' and add fillContainer?:
boolean), change any defaulting to use fillContainer (previously growth ===
'fill'), and update rendering logic that checks growth to instead check
fillContainer; also update all callers and tests to pass fillContainer where
they previously passed growth:'fill' or leave undefined for content behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dc6fafd1-6bc4-4bf0-b452-3e6ce1cdf3a0
📒 Files selected for processing (8)
src/components/courses/(hamburger-menu)/CourseActionMenu.tsxsrc/components/courses/contents/[courseId]/ContentRenderer.tsxsrc/components/courses/contents/[courseId]/ContentsPageBreadcrumbs.tsxsrc/components/tiptap-examples/RichTextEditor.tsxsrc/i18n/locales/de.jsonsrc/i18n/locales/de.tssrc/i18n/locales/en.jsonsrc/i18n/locales/en.ts
✅ Files skipped from review due to trivial changes (2)
- src/components/courses/(hamburger-menu)/CourseActionMenu.tsx
- src/i18n/locales/en.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/i18n/locales/de.json
- src/i18n/locales/de.ts
- src/i18n/locales/en.json
Marty-Byrde
changed the title
Marty-Byrde
added
Done
Previously (1b4d162), tooltips were updated in a way that users can pin all tooltips when the trigger is clicked. However, this change meant that any element wrapped by the `Tooltip` would pin the respective tooltip when clicked. This was not the intended behavior. Instead the idea was to pin tooltips on click for specific elements like info-icons. Hence, a `pinnable` argument was introduced which when set to true enables this tooltip-pin functionality.
The reason for this change is that because the examination and practice results were unified in the `/[locale]/results` directory the share-key is no longer needed. Hence the old drag-drop that was not removed in the previous migration is now removed.
The reason for this change is that an element wrapped in `BreadcrumbPage` doesn't need to use the `BreadcrumbItem`
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/Shared/form/Field.tsx (1)
77-92: Consider narrowing types to reduceas Anycasts.The
as Anycasts work correctly but bypass type safety. Since TypeScript doesn't narrow discriminated unions after destructuring, this is a known limitation.💡 Alternative approach using type guards
You could create a helper to narrow the props:
const isTextareaVariant = (v: string, p: unknown): p is TextareaFieldProps => v === 'textarea' // Then in fieldOnChange: if (isTextareaVariant(variant, props)) { // TypeScript knows props is TextareaFieldProps here }However, given the current usage and the number of cast locations, the pragmatic
as Anyapproach is acceptable for this component.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Shared/form/Field.tsx` around lines 77 - 92, The code uses a blanket cast "as Any" inside fieldOnChange which bypasses TypeScript safety; replace that with a narrow type guard helper (e.g., isTextareaVariant or isInputNumber) that inspects variant and props to refine props to InputFieldProps or TextareaFieldProps before accessing type-specific members, then use the narrowed type when calling field.onChange (references: fieldOnChange, variant, props, InputFieldProps, TextareaFieldProps); implement the guard(s) near this component and use them in the custom onChange branch and the number-input branch so you can remove the unsafe casts while preserving the same behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/Shared/Tooltip.tsx`:
- Around line 37-39: The TooltipTrigger currently always includes
aria-label='toggle tooltip' which is incorrect when pinnable is false; update
the TooltipTrigger element so the aria-label is only set when pinnable is true
(e.g., pass aria-label conditionally based on the pinnable prop or omit it
otherwise) and keep the existing onClick behavior using pinnable ? () =>
setPinned(prev => !prev) : undefined; reference TooltipTrigger, pinnable,
setPinned and children to locate the change.
---
Nitpick comments:
In `@src/components/Shared/form/Field.tsx`:
- Around line 77-92: The code uses a blanket cast "as Any" inside fieldOnChange
which bypasses TypeScript safety; replace that with a narrow type guard helper
(e.g., isTextareaVariant or isInputNumber) that inspects variant and props to
refine props to InputFieldProps or TextareaFieldProps before accessing
type-specific members, then use the narrowed type when calling field.onChange
(references: fieldOnChange, variant, props, InputFieldProps,
TextareaFieldProps); implement the guard(s) near this component and use them in
the custom onChange branch and the number-input branch so you can remove the
unsafe casts while preserving the same behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eddf0699-2840-464d-ab87-8ce859637548
📒 Files selected for processing (4)
src/components/Shared/Tooltip.tsxsrc/components/Shared/form/Field.tsxsrc/components/courses/(hamburger-menu)/CourseActionMenu.tsxsrc/components/results/practice/PracticeResultsBreadcrumbs.tsx
💤 Files with no reviewable changes (1)
- src/components/courses/(hamburger-menu)/CourseActionMenu.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/results/practice/PracticeResultsBreadcrumbs.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Note
This pull request mainly improves the authoring and practice experience through small but meaningful UX and localization refinements, fixes a navigation regression in the jump-back flow, and adds a few maintenance updates to keep translations, schema handling, and development workflows more reliable.
Summary
Fixes
Chores
Added translation synchronization to the pre-commit workflow so generated locale files and manually maintained JSON files stay aligned.
Improved stripDefaultValues handling so array effects are preserved and re-applied correctly, avoiding future schema-related issues.
Added a dedicated development build script that exposes all build errors instead of only the first one.
Refactors
Improved course editing breadcrumbs by introducing a dedicated edit/[id] layout so course names are shown instead of raw IDs.
Tightened drag-drop question validation by preventing authors from removing all answer options.
Improved form error feedback by enabling icons in more FormFieldError usages.
Expanded the Field component with textarea support and applied it to description fields for a better editing experience.
Improved tooltip accessibility by allowing toggling, especially benefiting mobile users.
Localized several UI areas, including generic breadcrumbs, practice breadcrumbs, practice results breadcrumbs, content page breadcrumbs, and stopwatch duration formatting.
Refined breadcrumb behavior so custom practice breadcrumbs align better with the rest of the app by starting from the root.
Cleaned up minor internal behavior, including removing unnecessary schema logging and improving the DOM structure of the RichTextEditor to better respect layout constraints.
Added dummy exam results in KnowCMenu for internal/testing-related purposes.
Styles
Changelog
Changelog
Fixes:
jumpBackButtonstage indecies (eccc8a37)The reason for this is that with the introduction of the new
contentstage the index of the remaining stages was increased by one. This meant that theJumpbackbutton redirected users to the wrong stage when clicked. This issue is now resolved.Chores:
Added translation synchronization to
pre-commit(d4e3df6b)This way the auto-generated locale-files and the
.jsonfiles that are manually edited are always in sync when changes are committed.Resolved stripping of array effects in
stripDefaultValues(66b74687)Previously the
stripZodDefaultValuesutility function stripped all effects from a given array when stripping it from default values. To prevent potential issues because of it in the future, the stripped checks are now re-applied.Added development build script (
63da9040)This development build script shows all build errors and not just a single one.
Refactors:
Added dummy exam results to
KnowCMenu(19d77f3c)Added
edit/[id]layout to render specificBreadcrumbs(54c9de92)The reason for this change is that previously the edit-course page showed the generic breadcrumb which showed the actual course id. Since the course-id is not a good breadcrumb the name is used instead, thus a custom Breadcrumbs were defined in the new layout to override the generic breadcrumbs.
Added minimum drag-drop answer constraint (
d96ff4d2)The reason for this change is that previously users able to delete all answer-options when creating a drag-drop question. To prevent this edge-case a min constraint was introduces to the respective schema to disallow this case.
Enabled
showIconinFormFieldErrorusages (4a3d5ed5)The reason for this change is that this way the general errors for choice- and drag-drop answer errors also include an error-icon, just like field-errors using the
Fieldcomponent do.Improved tooltip accessibility by allowing toggling (
1b4d1626)This way users on mobile devices are also able to access tooltips. At the same time this allows users on desktop screens to also toggle tooltips.
Added textarea support to
Fieldcomponent (ff90fc8d)This way fields for e.g. descriptions can be rendered as a Textarea instead of a single-line input. This can be done by setting the
varianttotextarea.Applied
Fieldvarianttextareafor descriptions (601b3dac)This way the descriptions fields used in the
GeneralSectionand theCourseContentDialogare now rendering a textarea instead of a single-line input.Removed log statement in
schemaDefaults(58d59f9a)Previously the
schemaDefaultsutility function logged "unknown schema type" when a property was of typeany,unknownorundefined. However, since all these types are instantiated asundefinedit is not worth logging.Localized
PracticeBreadcrumbstexts (4b368856)Localized
StopWatchduration format based on locale (ea02cfe2)Extended
PracticeBreadcrumbsto start from root (9a3c110e)This way these custom breadcrumbs align with the other breadcrumbs shown on other pages, which all start from the root page.
Localized
GenericBreadcrumbpage segments (4b5e4703)The
GenericBreadcrumbcomponent now displays the translation for a given page-segement-key or if it does not exist it shows the segment (key) as before.Localized
PracticeResultsBreadcrumbstexts (3dd06745)Localized
ContentPageBreadcrumbstexts (7a39842e)Improved DOM structure of
RichTextEditor(cd68b4aa)The reason for this change is that previously the editor-pane (
EditorContent) in some cases causes its container to grow outside of its max-height constraint. By removing an obsolete outer wrapper and moving the constraints directly to the editor pane these issues are resolved.Styles:
b8e45eeb)Previously the width of the grid-cell that shows the practice question, practice progress had a max-width of 4/5 on large screens. Now the max-w is slightly smaller on larger screens, namely to 50% of the viewport width.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Accessibility
Localization