Feat: Introduce knowledge retainment score to KnowledgeCheckCard#258
Feat: Introduce knowledge retainment score to KnowledgeCheckCard#258Marty-Byrde wants to merge 20 commits into
Conversation
This way the description is no longer automatically shown when the input is focussed but instead only when the user hovers the info-icon. This way users are only shown the description when they decide that they need further help / information. Thus. preventing layout shifts for experienced users that do not need these descriptions.
This way indicators like webkit indicators for number and date inputs automatically use the respective color-mode scheme. Previously, the color-scheme was set based on the user's color-mode preference and not by the currently selected color-mode.
Previously the disabled state of the `Input` component in the dark-mode was very intense, making the disabled input stand out. At the same time, the strong disabled state made it hard to recognize the `Input`. By lowering the opacity in the dark-mode the color intensity for the disabled state is reduced.
This way both the `input` and `base` variant only show their hover and focus styles when they are enabled, thus not showing them when disabled.
Previously the `outline` variant used the foreground color which was not suitable as the ring color of the button. The button now uses the `ring-input` color when enabled and a less dominant ring color when disabled.
WalkthroughThis pull request introduces a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 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 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 |
Marty-Byrde
changed the title
KnowledgeCheckCard
Marty-Byrde
changed the title
KnowledgeCheckCard
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}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/Shared/form/Field.tsx (2)
179-193: Remove dead code functions:RenderInlineDescription,RenderAbsoluteError, andRenderAbsoluteDescription.These three functions (lines 179–193, 195–208, 210–222) are no longer called after the refactor from inline descriptions to the Tooltip approach. They should be removed to keep the codebase clean.
Additionally, the
showDescriptionvariable (line 53) and relatedkeepAnimationContainerSizelogic (line 57) are now only used for layout preservation during error/description transitions. Consider whether this logic is still necessary given that descriptions are now displayed via tooltip rather than inline.
195-222: Remove unused functionsRenderAbsoluteErrorandRenderAbsoluteDescription.These functions are defined at lines 195–222 but are never called anywhere in this file or the codebase. They appear to have been replaced by
RenderInlineErrorandRenderInlineDescription(which handle animations differently) and should be deleted.
🤖 Fix all issues with AI agents
In `@src/components/checks/DegradingScoreIndicator.tsx`:
- Line 59: In DegradingScoreIndicator.tsx update the invalid OKLCH color token
used in the conditional class for level '20-40'—replace the malformed chroma
"0.0.08" with "0.08" in the bg-[oklch(...)] string so the class becomes
bg-[oklch(88%_0.08_45)] (and likewise update the dark:bg value only if it
contains a similar typo); locate the conditional that checks level === '20-40'
and fix the OKLCH chroma formatting to a valid decimal.
- Line 30: The user-facing string assigned to scoreMessage in
DegradingScoreIndicator (scoreMessage = 'Most of the Knokwledge gained by this
check is retained') has a typo; update that assignment in
DegradingScoreIndicator.tsx to correct "Knokwledge" to "Knowledge" so the
message reads "Most of the Knowledge gained by this check is retained".
In `@src/components/shadcn/button.tsx`:
- Line 21: The class string in the Button component contains a typo: the token
"disabled:disabled:ring-neutral-500" is duplicated; update that token to
"dark:disabled:ring-neutral-500" so it matches the surrounding dark-mode pattern
(replace the erroneous "disabled:disabled:ring-neutral-500" in the class list
within the Button/component JSX).
🧹 Nitpick comments (5)
src/i18n/locales/de.json (1)
186-189: Consider localizing “Co-Author” for German consistency.
Everything else is German; swapping “Co-Author” → “Co‑Autor” (or preferred term) would keep locale consistency.src/i18n/locales/de.ts (1)
186-189: Consider localizing “Co-Author” for German consistency.
Same suggestion as the JSON locale: a German term (e.g., “Co‑Autor”) would match the rest of the locale strings.src/components/checks/DegradingScoreIndicator.tsx (2)
20-20: Clamping condition may be clearer with explicit undefined check.The current check
knowledgeCheckScore && knowledgeCheckScore > 100works correctly but relies on0being falsy (which is fine since 0 doesn't need clamping). However, for clarity and consistency with line 12, consider using an explicit undefined check.♻️ Suggested improvement
- if (knowledgeCheckScore && knowledgeCheckScore > 100) knowledgeCheckScore = 100 + if (knowledgeCheckScore !== undefined && knowledgeCheckScore > 100) knowledgeCheckScore = 100
22-50: Consider using i18n for user-facing score messages.The
scoreMessagestrings are hardcoded in English. Since the codebase uses i18n (as seen inKnowledgeCheckCard.tsx), these messages should be translated for consistency.src/components/Shared/form/Field.tsx (1)
53-57: Potentially obsolete logic after refactor.The
showDescriptionvariable (line 53) andkeepAnimationContainerSizelogic (lines 56-57) were designed for the inline description display which has been replaced with tooltips.
showDescriptionis now only used in thekeepAnimationContainerSizecalculation, but since descriptions are no longer rendered inline, this logic may no longer serve its original purpose of preventing layout shifts.Consider reviewing whether this logic is still needed or can be simplified.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/lib/checks/computeKnowledgeRetainment.ts`:
- Around line 3-12: computeKnowledgeRetainment currently returns a random score
and ignores difficulty; update it to be deterministic by replacing the
randomness with a proper computation that uses difficulty and explicit user
data: change the signature of computeKnowledgeRetainment to accept the required
data (e.g., practiceHistory: Array<{score:number,timestamp:number}>,
lastExam?:{score:number,timestamp:number}) along with difficulty (currently
Pick<KnowledgeCheck,'difficulty'>), compute a decay-based retainment score using
time-since-last-practice/exam and weighted scores (apply higher decay for higher
difficulty and combine recent practice/exam scores into a 0–100 value), clamp to
0–100 and return an integer; alternatively, if these data are not yet available,
make the function return null/undefined or throw a clear error so the UI can be
gated until proper data is passed.
🧹 Nitpick comments (2)
src/components/checks/KnowledgeCheckCard.tsx (2)
17-20: Stabilize retainmentScore computation to avoid UI jitter.If the retainment score is time/random-based, computing it on every render will cause visible changes. Memoize it per check to keep the badge stable during re-renders.
Proposed update
- const retainmentScore = computeKnowledgeRetainment({ difficulty: check.difficulty }) + const retainmentScore = React.useMemo( + () => computeKnowledgeRetainment({ difficulty: check.difficulty }), + [check.difficulty], + )
38-41: Remove the empty JSX expression.The
{}renders nothing and is a leftover artifact.Proposed cleanup
- <div className='absolute top-0 left-0 z-10 opacity-65'> - {} + <div className='absolute top-0 left-0 z-10 opacity-65'> <DegradingScoreIndicator className='min-w-10 rounded-tl-md rounded-br-md p-1.5 px-3 text-sm' knowledgeCheckScore={retainmentScore} /> </div>
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/checks/[share_token]/practice/PracticeProgress.tsx (1)
14-14: Guard against division by zero whenquestionsis empty.If
practiceQuestionsis an empty array,results.length / questions.lengthevaluates toNaNorInfinity, which may cause unexpected rendering behavior.Proposed fix
- <Progress value={(results.length / questions.length) * 100} className='h-5' indicatorClasses='dark:bg-green-700 bg-green-500' /> + <Progress value={questions.length > 0 ? (results.length / questions.length) * 100 : 0} className='h-5' indicatorClasses='dark:bg-green-700 bg-green-500' />
🤖 Fix all issues with AI agents
In `@database/drizzle/migrations/0016_explicit-has-done-result-types.sql`:
- Line 1: The migration tries to add a NOT NULL enum column to
User_has_done_KnowledgeCheck which will fail if rows already exist; instead
implement a two-step migration: first ALTER TABLE User_has_done_KnowledgeCheck
ADD COLUMN `type` enum('practice','examination') NULL (or with a safe DEFAULT),
then run a backfill UPDATE on User_has_done_KnowledgeCheck to set `type` for all
existing rows (choose the correct value logic or a default such as 'practice'),
and finally ALTER TABLE ... MODIFY `type` enum('practice','examination') NOT
NULL to enforce non-nullability; reference the table name
User_has_done_KnowledgeCheck and column `type` when locating changes.
In `@database/practice/index.ts`:
- Around line 24-36: The existence check on db_userHasDoneKnowledgeCheck is
missing a filter for the record type, so add a condition to the where clause to
require db_userHasDoneKnowledgeCheck.type === 'practice' (e.g., using
eq(db_userHasDoneKnowledgeCheck.type, 'practice')) alongside the existing
eq(...) conditions in the query that populates the update variable; this will
ensure savePracticeResults (or the surrounding logic that calls
insertPracticeResults/updatePracticeResults) only treats matching practice
records as updates while keeping formatDatetime(props.startedAt) and the
userId/knowledgeCheckId checks unchanged.
In `@database/practice/update.ts`:
- Around line 29-45: The UPDATE is missing a filter for the record type, so add
a condition to the WHERE clause to restrict to practice records: include
eq(db_userHasDoneKnowledgeCheck.type, 'practice') inside the and(...) used in
the update on db_userHasDoneKnowledgeCheck (the same block that checks
eq(...knowledgeCheckId, values.knowledgeCheckId), eq(...startedAt,
formatDatetime(startedAt)), and eq(...userId, userId)) so only practice entries
are updated.
In `@src/hooks/checks/`[share_token]/practice/PracticeStore.ts:
- Line 79: The call to savePracticeResults in PracticeStore is fire-and-forget
and lacks error handling; update the call site to await savePracticeResults(...)
inside a try/catch (or attach .catch) so failures are detected, log or surface
the error (e.g., processLogger/error handler) and surface a user-facing fallback
such as a toast or retry path; reference the savePracticeResults invocation and
the surrounding function that triggers it and ensure you handle both async
rejection and provide a user notification on failure.
- Around line 74-82: The storeAnswer updater is creating an update object with
the wrong property name (`answers`) so the state field `answerResults` never
changes and savePracticeResults is passed the wrong value; modify the update
creation in storeAnswer (the function named storeAnswer) to set `answerResults`
(not `answers`) and then pass `update.answerResults` into savePracticeResults
(preserving checkId and startedAt) so PracticeState.answerResults is updated
correctly and the saved payload is accurate.
🧹 Nitpick comments (3)
database/practice/select.ts (1)
6-14: Consider using authenticated user ID instead of accepting it as a parameter.
getCheckPracticeResultsByUserIdacceptsuserIdas a parameter. If callers can supply arbitrary user IDs, this could allow fetching other users' practice data. Consider derivinguserIdfromrequireAuthentication()similar to the insert/update functions.database/practice/index.ts (1)
18-38: Redundant authentication calls across insert/update/save functions.
requireAuthentication()is called insavePracticeResults, then again in bothinsertPracticeResultsandupdatePracticeResults. Consider refactoring to passuserIdto the child functions or extracting a shared internal implementation.database/practice/insert.ts (1)
28-35: Consider returning a success indicator or the inserted record.Unlike
updatePracticeResultswhich returns{ success: boolean }, this function returns nothing. Callers cannot confirm the insert succeeded without catching exceptions. Consider returning a consistent response shape.Proposed fix
- await db.insert(db_userHasDoneKnowledgeCheck).values({ + const result = await db.insert(db_userHasDoneKnowledgeCheck).values({ userId, startedAt: formatDatetime(startedAt), type: 'practice', results: JSON.stringify(results), finishedAt: formatDatetime(new Date(Date.now())), ...values, }) + + return { success: result[0].affectedRows === 1 }
Marty-Byrde
force-pushed
the
feat/knowledgeCheck-degrading-score
branch
from
7cc6c78 to
92ee07c
Compare
Marty-Byrde
force-pushed
the
feat/knowledgeCheck-degrading-score
branch
from
92ee07c to
76133e2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/checks/KnowledgeCheckCard.tsx (1)
70-83:⚠️ Potential issue | 🟠 Major
Math.random()in render causes hydration mismatch and inconsistent UI.Using
Math.random()directly in the render path (line 77) will:
- Produce different values between SSR and client hydration in Next.js, causing React hydration errors.
- Generate a new random value on every re-render.
- Show a different score than the
retainmentScorealready computed at line 18.The
eslint-disablecomment suppresses the lint warning but doesn't fix the underlying issue.🐛 Proposed fix: Pass retainmentScore as a prop to Footer
- <Footer updatedAt={check.updatedAt} /> + <Footer updatedAt={check.updatedAt} retainmentScore={retainmentScore} /> </Card> ) } -function Footer({ updatedAt }: { updatedAt?: Date }) { +function Footer({ updatedAt, retainmentScore }: { updatedAt?: Date; retainmentScore: number }) { const t = useScopedI18n('Components.KnowledgeCheckCard') const currentLocale = useCurrentLocale() return ( <div className='relative -mt-6 -mb-1 flex flex-row-reverse justify-between border-t border-neutral-400/80 px-4 pt-3 text-xs text-neutral-500/70 dark:border-neutral-700 dark:text-neutral-500'> <div className='absolute top-0.5 left-2'> - {/* eslint-disable-next-line react-hooks/purity */} - <DegradingScoreIndicator knowledgeCheckScore={+(Math.random() * 100).toFixed(0)} /> + <DegradingScoreIndicator knowledgeCheckScore={retainmentScore} /> </div>
🧹 Nitpick comments (1)
src/components/checks/KnowledgeCheckCard.tsx (1)
37-40: Remove empty JSX expression.Line 38 contains an empty
{}which serves no purpose and should be removed.🧹 Proposed fix
<div className='absolute top-0 left-0 z-10 opacity-65'> - {} <DegradingScoreIndicator className='min-w-10 rounded-tl-md rounded-br-md p-1.5 px-3 text-sm' knowledgeCheckScore={retainmentScore} /> </div>
Tried out a variant in which the `DegradingScoreIndicator` is positioned in the top left corner of the `KnowledgeCheckCard`.
Tried out another variant in which the `DegradingScoreIndicator` is positioned in the left corner of the footer of the `KnowledgeCheckCard`.
Marty-Byrde
force-pushed
the
feat/knowledgeCheck-degrading-score
branch
from
76133e2 to
da282a6
Compare
…ledgeCheck-degrading-score
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/app/`[locale]/checks/[share_token]/attempt-not-possible/page.tsx:
- Around line 18-24: The page currently renders only the `reason` from
`isExaminationAllowed(check, user)`, which is undefined when the exam is
allowed; update the call to destructure both `allowed` and `reason` (e.g., `{
allowed, reason } = await isExaminationAllowed(...)`) and then branch: if
`allowed` is true perform a redirect to the examination entry (using your app
routing/redirect helper) or render a suitable fallback (e.g., a message or
link), otherwise render the existing `PageHeading` and the `reason`; ensure you
modify the logic in the component that returns the JSX (page.tsx) so it never
renders an empty <p> when `reason` is undefined.
In `@src/components/checks/DegradingScoreIndicator.tsx`:
- Around line 24-47: The switch in DegradingScoreIndicator sets user-facing
strings directly to scoreMessage (cases '95-100'...'0-20'), bypassing i18n;
update it to use the component's useScopedI18n (or the project's equivalent) to
return localized labels instead of hardcoded English strings, i.e., import/use
the hook in DegradingScoreIndicator, replace each literal assigned to
scoreMessage with a call to the i18n lookup key (e.g.,
t('degradingScore.almost100') or similar scoped keys), and ensure keys exist in
the locale files so the UI uses translated messages.
- Around line 8-50: The component DegradingScoreIndicator currently treats
undefined as 0 and may show "0" or an empty tooltip; normalize the incoming
knowledgeCheckScore at the top of the function before computing level by: if
knowledgeCheckScore is undefined, keep a sentinel (e.g., null/undefined) and do
not map it to a numeric level so the UI can render an explicit empty
state/placeholder; otherwise clamp numeric values to the range 0–100 (set
negatives to 0 and values >100 to 100) and then compute level; update uses of
level/scoreMessage (the switch and any render logic) to show a clear empty
message/tooltip when the score was undefined instead of showing 0.
In `@src/components/Shared/form/Field.tsx`:
- Around line 111-131: The tooltip trigger (the motion.div wrapping InfoIcon
inside Tooltip) is not keyboard-accessible; change the trigger to a focusable
element (preferably a semantic <button> or add tabIndex={0} and role="button")
and wire keyboard focus handlers (onFocus/onBlur and onKeyDown for Enter/Space)
to call setIsHovered so the Tooltip appears for keyboard users; also add an
accessible label (aria-label or aria-describedby using the same description
variable) to the trigger so screen readers can read the help text, and preserve
the existing disabled styling logic that uses data-[disabled] and props.type ===
'checkbox' positioning.
🧹 Nitpick comments (1)
src/lib/checks/[share_token]/isExaminationAllowed.ts (1)
7-10: Consider removing the optionalreasonfield fromAllowedReturn.The
reasonfield onAllowedReturnis marked optional but is never populated whenallowed: trueis returned (line 28). If there's no use case for providing a reason on successful authorization, removing it would make the type more precise.type AllowedReturn = { allowed: true - reason?: string }
| switch (level) { | ||
| case '95-100': { | ||
| scoreMessage = 'Almost 100% of the Knowledge is most likely still retained' | ||
| break | ||
| } | ||
| case '80-95': { | ||
| scoreMessage = 'Most of the Knowledge gained by this check is retained' | ||
| break | ||
| } | ||
| case '60-80': { | ||
| scoreMessage = 'Big Chunks of Knowledge are still retained' | ||
| break | ||
| } | ||
| case '40-60': { | ||
| scoreMessage = 'While some Knowledge parts are retained, a large part might have been forgotten' | ||
| break | ||
| } | ||
| case '20-40': { | ||
| scoreMessage = 'Most of the Knowledge delivered by this check is no longer retained' | ||
| break | ||
| } | ||
| case '0-20': { | ||
| scoreMessage = 'The contents and knowledge delivered by this check are no longer retained' | ||
| break |
There was a problem hiding this comment.
Localize user-facing score messages.
These strings bypass i18n, while the rest of the card uses localized labels. Please wire them through useScopedI18n (or equivalent) to keep translations consistent.
🤖 Prompt for AI Agents
In `@src/components/checks/DegradingScoreIndicator.tsx` around lines 24 - 47, The
switch in DegradingScoreIndicator sets user-facing strings directly to
scoreMessage (cases '95-100'...'0-20'), bypassing i18n; update it to use the
component's useScopedI18n (or the project's equivalent) to return localized
labels instead of hardcoded English strings, i.e., import/use the hook in
DegradingScoreIndicator, replace each literal assigned to scoreMessage with a
call to the i18n lookup key (e.g., t('degradingScore.almost100') or similar
scoped keys), and ensure keys exist in the locale files so the UI uses
translated messages.
|
🎉 This PR is included in version 3.4.0-canary.7 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
The reason for this change is that this way the `DegradingScoreIndicator` does not overlap with the role.
By aligning the action buttons and user role with the `DegradingScoreIndicator` makes it visually more appealing.
1 participant
Important
Note that while the pull request already introduces the concept of showing a degrading knowledge score, its visual representation is not yet ready for production.
Tip
This is what the degrading-score-indicator looks like:
Summary
DegradingScoreIndicatorcomponent that renders a knowledge retainment score.Summary by CodeRabbit