Feat: Learning Record For Residents#1169
Open
gtonye wants to merge 10 commits into
Open
Conversation
- Integrated funnel functionality in DigitalTranscriptEntryPage, allowing for a streamlined user experience when adding achievements. - Added back navigation handling that commits session rows before navigating back. - Updated DigitalTranscriptWysiwygEntry to support single-row sessions for funnel mode. - Improved UI components to conditionally render based on funnel state, enhancing usability. - Introduced new helper functions for managing session states and entries.
…funnel support - Refactored AchievementForm and AchievementFormMetadata to support funnel step navigation and error handling. - Updated AchievementRow to conditionally render based on funnel state, integrating save and cancel functionalities. - Enhanced AchievementsRecordPreview to include download capabilities for funnel mode. - Introduced new props and handlers for managing funnel interactions across digital transcript components. - Improved UI consistency and user experience in the digital transcript entry flow.
…I improvements - Updated AchievementForm and AchievementRow to integrate funnel navigation and save functionalities. - Enhanced DigitalTranscriptEntryPage with alert dialogs for unsaved changes and streamlined navigation. - Improved UI consistency across digital transcript components, including the addition of new props for funnel handling. - Refactored ConfidenceSegmentedControl and LearningRecordDocument to support funnel-specific logic and display. - Introduced new badge components for better visual feedback on progress within the funnel.
…siveness - Changed table class to 'table-fixed' for better layout control. - Adjusted column widths for 'Completed', 'Questions answered', and 'Added on' headers to ensure consistent display across different screen sizes. - Enhanced cell alignment for better visual consistency in the SavedEntriesSection.
…rovements - Added extra_hosts configuration in docker-compose for improved local development. - Updated Nginx configuration to use dynamic upstream variables for API and frontend proxying. - Enhanced ConfirmDialog component to accept custom button class names for better styling flexibility. - Refactored useTranscriptDraft hook to include additional fields for transcript entries. - Improved AchievementForm and related components with new props for better funnel integration and UI consistency. - Introduced sorting functionality in DigitalTranscriptHome for better user experience when viewing achievements.
Give residents a clearer home experience that surfaces in-progress achievements, reduces duplicate resume CTAs, and aligns copy and title display across the learning record flow. Co-authored-by: Cursor <cursoragent@cursor.com>
…ort to main - Add missing digital-transcript files dropped during cherry-pick (digitalTranscriptRoutes, learningRecordPrototypes, learningRecordResidentName, TranscriptResumePreview, AchievementFormCategories, AchievementFormCategoryCard, AchievementFormSectionHeader, AchievementPreview, TopSkillsTagField) - Fix import paths in ResidentHomeDashboard (useTourContext, targetToStepIndexMap) - Add missing imports for DigitalTranscriptHome/EntryPage in app-routes.tsx - Remove demo calendar data fallback in favour of real API data - Wrap .card-block in @layer components so Tailwind utilities can override bg colour - Restore nginx to Docker container names (server/frontend) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ge persistence Replace all localStorage reads/writes for entries and drafts with API calls to the learning-record endpoints. The hook now fetches on mount, autosaves drafts via debounced PUT, and persists committed entries through POST/PUT/DELETE. Session UI state (expanded row, etc.) remains in React state only.
Introduces LearningRecordAccess feature flag end-to-end: DB migration, backend route gating via featureRoute, frontend enum/auth/sidebar/admin toggle, and dedicated learning-record-routes file with routes moved out of knowledge-routes.
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds learning record storage, feature access, resident home/dashboard UI, and the digital transcript editor/preview/export flow. ChangesLearning record rollout
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 29
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/migrations/00073_add_learning_record_feature_flag.sql`:
- Line 10: The DELETE statement violates SQLFluff LT14; reformat the statement
so the DELETE keyword and the WHERE clause are on separate lines to satisfy the
linter: locate the DELETE targeting public.feature_flags where name =
'learning_record' and split it into a DELETE FROM public.feature_flags line
followed by a separate WHERE name = 'learning_record' line so the clause is on
its own line.
In `@backend/src/database/learning_record.go`:
- Around line 29-31: The update/delete DB calls that use result :=
db.Model(entry)... (e.g., the update path where result is assigned and similar
blocks around lines handling deletion and bulk operations) only check
result.Error; change these to also check result.RowsAffected and treat
RowsAffected == 0 as a not-found/unauthorized outcome: after the DB call (the
result variable from db.Model(entry).Where(...).Updates(entry) and the
corresponding delete calls), if result.Error != nil return the error, else if
result.RowsAffected == 0 return a not-found/unauthorized error (same error
type/status your handlers use for missing ownership), so that non-matching
ownership filters do not return success.
- Line 31: The code is using GORM's Updates(entry) which omits zero-value fields
(so clearing fields fails); change the call that currently uses Updates(entry)
in learning_record.go to use UpdateColumns(entry) on the same model/DB chain (or
alternatively construct a map[string]interface{} and pass that to Updates) so
zero/empty values are written; update the call site where Updates(entry) is
invoked to use UpdateColumns(entry) (or a map) to ensure zero-value fields are
persisted.
In `@backend/src/handlers/learning_record_handler.go`:
- Around line 47-50: The code currently parses path IDs with strconv.Atoi (via
r.PathValue("id")) and then allows negative values to be cast to uint, producing
invalid large IDs; update both parsing sites (the blocks using strconv.Atoi and
newInvalidIdServiceError) to reject non-positive IDs by checking if id <= 0 and
returning newInvalidIdServiceError(errOrCustom, "entry ID") (or a clear
invalid-id error) before casting to uint; ensure the uint conversion only occurs
after the positive check so functions using the resulting uint get a valid ID.
- Around line 33-39: The create and update handlers are allowing clients to set
the draft flag; force published state by overriding the entry's draft field
before persisting. In handleCreateLearningRecordEntry (before calling
srv.Db.CreateLearningRecordEntry) set the LearningRecordEntry's IsDraft (or
is_draft) to false/unset so clients cannot submit drafts to the published
endpoint; do the same in the corresponding update handler (the code path that
calls srv.Db.UpdateLearningRecordEntry / UpdateLearningRecordEntry) to ensure
incoming payloads cannot toggle entries back to draft.
In `@backend/src/models/learning_record.go`:
- Around line 20-31: The Scan method on StringSlice currently swallows
unexpected DB value types by defaulting to "[]"; update StringSlice.Scan to
return a descriptive error in the default branch (including the concrete type of
value) instead of silently unmarshaling "[]", and handle nil explicitly if
needed (either treat nil as empty array or return an error) so callers see type
mismatches; reference the StringSlice.Scan function and its switch on
value.(type) to locate and change the default behavior to return
fmt.Errorf("unexpected type for StringSlice: %T", value) (or similar).
In `@frontend/package.json`:
- Around line 49-52: Remove the unused html2pdf.js dependency from
frontend/package.json: confirm there are no runtime imports (e.g., check
frontend/src/utils/downloadLearningRecordPdf.ts uses only html2canvas and
jspdf), remove the "html2pdf.js" entry from the dependencies, then run the
package manager (yarn/npm install) to update node_modules and regenerate the
lockfile; optionally run a repo-wide search for "html2pdf" to verify nothing
else references it before committing.
In `@frontend/src/api/learningRecord.ts`:
- Around line 116-177: The API functions (apiGetEntries, apiCreateEntry,
apiUpdateEntry, apiDeleteEntry, apiGetDraft, apiUpsertDraft, apiDeleteDraft)
currently swallow failures; add error logging/telemetry whenever resp.success is
false or resp.type/data are not as expected by calling console.error or your
error-tracking helper with a clear message plus the full resp and relevant
identifiers (e.g., backendId, clientId, entry.id) so failures are recorded; for
apiGetDraft also log when b or b.client_id is missing and include b.updated_at
when present; keep behavior otherwise (return null/false/empty) but ensure every
early-return on failed response logs diagnostic details.
- Line 46: Validate the incoming challenge_toggle before casting: replace the
unsafe cast used for q4Toggle by checking that (b.challenge_toggle) is strictly
one of the allowed TranscriptQ4Toggle values ('yes' or 'notReally') or null, and
only then assign it to q4Toggle; if the value is anything else, set q4Toggle to
null (or another safe default). Implement a small type-guard function (e.g.,
isTranscriptQ4Toggle) and use it when mapping b.challenge_toggle to q4Toggle to
prevent silently accepting invalid backend values.
- Line 161: The fallback to new Date().toISOString() when converting a backend
record (variable b) to the frontend model in the call to toDraftFrontend(b,
b.updated_at ?? new Date().toISOString()) is misleading; instead pass a
null/undefined sentinel so the frontend knows updated_at is unknown (or log the
missing value), e.g. call toDraftFrontend with b.updated_at as-is (no
current-time fallback) or explicitly pass null and update
toDraftFrontend/signature handling if needed to accept nullable updated_at;
locate the call in frontend/src/api/learningRecord.ts and adjust the call site
and any dependent handling in toDraftFrontend to treat missing updated_at as
unknown rather than current time.
In `@frontend/src/components/dashboard/ContinueLearningSection.tsx`:
- Around line 78-91: The mapping currently hardcodes statusLabel from the array
index and sets progressPercent to 0; update the map to derive progressPercent
from the actual item field (e.g., item.progress, item.progress_percent, or
item.user_progress) and compute statusLabel from that value (e.g., 0 -> "Not
started yet", 0<progress<100 -> "In progress" or "Your progress", 100 ->
"Completed"); if OpenContentItem contains no progress field, return no
progressPercent (undefined/null) and do not render progress UI elsewhere; keep
resolveContentHref usage and the existing id/title/contentType fields unchanged
and add defensive checks for missing progress properties.
In `@frontend/src/components/dashboard/DiscoverContentSection.tsx`:
- Around line 167-182: The handleClick function lacks error handling around the
helpful link tracking API call (API.put in the block guarded by
item.helpfulLinkId); wrap that await API.put<{ url: string }, object>(...) call
in a try-catch, log the caught error (use existing logger or console.error) and
still fall back to item.href if request/response fails, then proceed to open the
link (window.open) or navigate(item.href); optionally, call the existing
toast/notification utility in the catch to surface persistent failures to the
user.
In `@frontend/src/components/dashboard/UpcomingClassSessionCard.tsx`:
- Around line 103-108: The inProgress check uses UTC `now` against raw session
times causing timezone mismatch; update it to compare times in the same timezone
as the converted start/end (use the already computed `start` and `end` from
toZonedTime or convert `now` with `toZonedTime(now, timezone)`) so the
`inProgress` boolean is determined by comparing like-for-like zoned Date objects
(refer to the `toZonedTime` usage and the `inProgress` variable that reads
`nextSession.start`/`end`).
In `@frontend/src/hooks/useTranscriptDraft.ts`:
- Around line 73-80: The autosave currently fire-and-forgets apiUpsertDraft in
useTranscriptDraft (using hydrated, draft and DRAFT_AUTOSAVE_MS) so failures are
silent; add an autosave error state (e.g., autosaveError via useState) and
replace the void call with a promise chain/async handling that sets
autosaveError on .catch or when the API indicates failure and clears it on
success, log the error for debugging, and expose autosaveError from
useTranscriptDraft so the UI can surface a warning to users when autosave fails.
- Line 43: The autosave debounce constant DRAFT_AUTOSAVE_MS in
useTranscriptDraft.ts is too low (400ms) and should be increased to reduce API
traffic; update the DRAFT_AUTOSAVE_MS value to a more conservative interval
(recommend 1000–2000ms, e.g., 1500) and ensure any debounce logic that
references DRAFT_AUTOSAVE_MS (in the useTranscriptDraft hook) continues to use
the constant so autosave timing is centralized, then run related tests or manual
verification to confirm autosave behavior.
- Around line 96-102: The startFreshDraft function currently fires
apiDeleteDraft(prev.id) without awaiting it causing a race; make startFreshDraft
async and await apiDeleteDraft(prev.id) (inside a try/catch) before creating the
fresh draft with createEmptyDraft and calling setDraft, and on delete failure
log or surface the error and decide whether to abort creating the new draft;
update any callers to handle the now-async startFreshDraft accordingly and keep
using draftRef/current references (draftRef, apiDeleteDraft, createEmptyDraft,
setDraft) to locate the changes.
- Around line 104-126: upsertCommittedEntry currently calls
apiUpdateEntry/apiCreateEntry without error handling, causing silent failures
and data loss; wrap the async API calls in try/catch, only mutate
entryBackendIds.current, call setEntries, and invoke dispatchEntrySessionUpdated
on successful API responses, and on catch show a user-facing error (e.g.,
toast/notification) and keep the local state untouched (or roll back any
optimistic changes); ensure references to apiUpdateEntry, apiCreateEntry,
entryBackendIds.current, setEntries, and dispatchEntrySessionUpdated are updated
accordingly and maintain trimming via TOP_SKILLS_MAX before sending.
In `@frontend/src/loaders/routeLoaders.ts`:
- Around line 70-80: The mapping that builds featuredLibraries unnecessarily
uses nullish coalescing for the description field (lib.description ??
undefined); update the mapping inside the featuredLibraries assignment to assign
description directly from lib.description (i.e., description: lib.description)
since OpenContentItem already permits undefined, removing the redundant ??
undefined in the mapping for clarity.
In `@frontend/src/pages/learning/ResidentOnlineCourses.tsx`:
- Around line 85-94: Compute a single, clamped progress number and reuse it for
both the label and the bar width instead of calling
Math.floor(course.course_progress) twice; e.g., create a local variable (e.g.,
progressRaw = Number(course.course_progress) || 0; progressClamped = Math.max(0,
Math.min(100, Math.floor(progressRaw)))) and then render {progressClamped}% for
the label and style={{ width: `${progressClamped}%` }} for the bar (use the same
variable in the JSX where course.course_progress is currently used).
In `@frontend/src/pages/student/digital-transcript/AchievementRow.tsx`:
- Around line 95-156: In AchievementRow (the button span, the chevron container
span, and the progress bar inner span) replace the hardcoded color '`#556830`'
with theme/tailwind tokens (e.g., use a text token like text-primary or
text-primary-600 for the hover text and a background token like bg-primary-600
for the progress filled state) by updating the cn(...) class strings where
'`#556830`' appears (the title group-hover, the chevron group-hover, and the
complete ? progress class). Ensure you pick the appropriate dark/light variants
if needed (e.g., dark:bg-primary-600) and keep the existing conditional logic
(complete ? ... : ...) and aria attributes unchanged.
In
`@frontend/src/pages/student/digital-transcript/DigitalTranscriptEntryPage.tsx`:
- Around line 10-13: The code calls readLearningRecordExportRows without
importing it and with the wrong signature; import readLearningRecordExportRows
from the transcriptEntrySessionStorage module and call it with the required
TranscriptEntry[] argument (the committed entries array you already manage in
DigitalTranscriptEntryPage, e.g., committedEntries or the state/prop that holds
saved TranscriptEntry items) instead of zero args; ensure the import name
matches the exported symbol and that the caller passes the correct variable of
type TranscriptEntry[] when invoking readLearningRecordExportRows.
In `@frontend/src/pages/student/digital-transcript/DigitalTranscriptHome.tsx`:
- Around line 303-349: The TableCell elements are being wrapped by Button/Link
which creates invalid table markup; change the structure so TableCell elements
are direct children of TableRow and do not have Button or Link as their parent.
Move the Link (using editHref) inside the appropriate TableCell content (e.g.,
wrap the title span or all inner content) or add an onClick/onKeyDown on
TableRow to navigate to editHref while keeping TableCell nodes intact; update
TableRow (and getEntryDisplayTitle usage) to provide keyboard accessibility
(handle Enter/Space) and ensure aria attributes/role are set if you make the row
interactive.
In `@frontend/src/pages/student/digital-transcript/ReflectionStepField.tsx`:
- Around line 53-56: The conditional in handleChange is redundant; always call
onChange with next.slice(0, nudge.maxLength) which will return the original
string when shorter and truncate when longer. Update the handleChange function
to remove the if-else and invoke onChange(next.slice(0, nudge.maxLength)) so you
only reference handleChange, onChange, and nudge.maxLength.
- Line 121: The onChange inline handler calls v.trim() twice; compute const
trimmed = v.trim() once and use it in the condition and when setting topSkills
to avoid duplicate work and improve readability (update the onChange handler
that currently sets topSkills: v.trim() ? [v.trim()] : [] to use the single
trimmed variable).
In `@frontend/src/pages/student/digital-transcript/TranscriptResumePreview.tsx`:
- Line 91: Update the user-facing heading in the TranscriptResumePreview
component: replace the incorrect phrase "Achievements logs of {name}" with a
grammatically correct variant such as "Achievement record for {name}" or
"Achievement records for {name}" (use the chosen wording wherever the heading
string is rendered and ensure it still interpolates the name variable).
- Around line 76-79: The className string in TranscriptResumePreview's JSX uses
legacy Tailwind important-prefix syntax ('!px-0 !pt-0') when computing classes
via cn; update this to Tailwind v4 suffix important syntax by replacing those
tokens with 'px-0! pt-0!' so the conditional branch that uses isPaper continues
to apply the same important styles using the new suffix form inside the cn call.
In `@frontend/src/pages/student/ResidentHome.tsx`:
- Line 106: Remove the redundant null-coalescing when passing featuredLibraries
to ResidentHomeDashboard: since featuredLibraries is obtained from
useLoaderData() and the loader (routeLoaders.ts) always includes
featuredLibraries (defaulting to []), change the prop from
featuredLibraries={featuredLibraries ?? []} to simply
featuredLibraries={featuredLibraries}; update the JSX in ResidentHome (the prop
passed to ResidentHomeDashboard) accordingly and run tests/lint to ensure no
further references rely on the fallback.
In `@frontend/src/pages/student/ResidentHomeDashboard.tsx`:
- Around line 349-351: The useMemo for incompleteEntry currently includes an
unnecessary dependency hasIncompleteEntry; update the dependency array for the
useMemo that defines incompleteEntry to only include the actual inputs used by
findIncompleteAchievementEntry — learningRecordEntries, entrySessionTick, and
learningRecordFormVariant — so remove hasIncompleteEntry from the dependencies
of the useMemo (the useMemo call creating incompleteEntry that invokes
findIncompleteAchievementEntry).
In `@frontend/src/routes/learning-record-routes.tsx`:
- Around line 12-35: The routes for 'learning-record-funnel' and
'learning-record-categories' both render the same components
(DigitalTranscriptHome and DigitalTranscriptEntryPage); either consolidate them
into a single route group or add an explanatory comment; specifically, in the
route definitions where path: 'learning-record-funnel' and path:
'learning-record-categories' reference DigitalTranscriptHome and
DigitalTranscriptEntryPage, decide whether to merge into one path (removing the
duplicate group) or keep both and add a clear comment above each route
explaining why both exist (e.g., A/B/variant testing or future divergence) so
future readers understand the intent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a35cf888-81e7-4616-9b9b-3bf25e21ff12
⛔ Files ignored due to path filters (1)
frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (70)
backend/migrations/00072_create_learning_record_tables.sqlbackend/migrations/00073_add_learning_record_feature_flag.sqlbackend/src/database/learning_record.gobackend/src/handlers/learning_record_handler.gobackend/src/handlers/server.gobackend/src/models/feature_flags.gobackend/src/models/learning_record.goconfig/dev.nginx.confdocker-compose.ymlfrontend/package.jsonfrontend/src/api/learningRecord.tsfrontend/src/auth/useAuth.tsfrontend/src/components/UnlockEdTour.tsxfrontend/src/components/dashboard/ContinueLearningSection.tsxfrontend/src/components/dashboard/DiscoverContentSection.tsxfrontend/src/components/dashboard/IncompleteEntryReminder.tsxfrontend/src/components/dashboard/UpcomingClassSessionCard.tsxfrontend/src/components/learning-record/PrintShareHelpLink.tsxfrontend/src/components/navigation/Sidebar.tsxfrontend/src/components/shared/ConfirmDialog.tsxfrontend/src/components/ui/alert-dialog.tsxfrontend/src/data/faqData.tsfrontend/src/data/learningRecordResidentCopy.tsfrontend/src/hooks/useTranscriptDraft.tsfrontend/src/loaders/routeLoaders.tsfrontend/src/pages/admin/FeatureControl.tsxfrontend/src/pages/learning/ResidentOnlineCourses.tsxfrontend/src/pages/student/ResidentHome.tsxfrontend/src/pages/student/ResidentHomeDashboard.tsxfrontend/src/pages/student/digital-transcript/AchievementForm.tsxfrontend/src/pages/student/digital-transcript/AchievementFormActions.tsxfrontend/src/pages/student/digital-transcript/AchievementFormCategories.tsxfrontend/src/pages/student/digital-transcript/AchievementFormCategoryCard.tsxfrontend/src/pages/student/digital-transcript/AchievementFormMetadata.tsxfrontend/src/pages/student/digital-transcript/AchievementFormSectionHeader.tsxfrontend/src/pages/student/digital-transcript/AchievementPreview.tsxfrontend/src/pages/student/digital-transcript/AchievementRow.tsxfrontend/src/pages/student/digital-transcript/AchievementsRecordPreview.tsxfrontend/src/pages/student/digital-transcript/AddAchievementRow.tsxfrontend/src/pages/student/digital-transcript/ConfidenceSegmentedControl.tsxfrontend/src/pages/student/digital-transcript/DigitalTranscriptEntryPage.tsxfrontend/src/pages/student/digital-transcript/DigitalTranscriptHome.tsxfrontend/src/pages/student/digital-transcript/DigitalTranscriptShell.tsxfrontend/src/pages/student/digital-transcript/DigitalTranscriptWysiwygEntry.tsxfrontend/src/pages/student/digital-transcript/LearningRecordDocument.tsxfrontend/src/pages/student/digital-transcript/LearningRecordDocumentNarrative.tsxfrontend/src/pages/student/digital-transcript/LearningRecordExportContent.tsxfrontend/src/pages/student/digital-transcript/ReflectionStepField.tsxfrontend/src/pages/student/digital-transcript/ReflectionTextField.tsxfrontend/src/pages/student/digital-transcript/TopSkillsTagField.tsxfrontend/src/pages/student/digital-transcript/TranscriptResumePreview.tsxfrontend/src/pages/student/digital-transcript/ViewAllAchievementsSheet.tsxfrontend/src/pages/student/digital-transcript/confidenceLevelVisual.tsfrontend/src/pages/student/digital-transcript/digitalTranscriptRoutes.tsfrontend/src/pages/student/digital-transcript/entryTitleDisplay.tsfrontend/src/pages/student/digital-transcript/learningRecordButtons.tsfrontend/src/pages/student/digital-transcript/learningRecordDocumentModel.tsfrontend/src/pages/student/digital-transcript/learningRecordPrototypes.tsfrontend/src/pages/student/digital-transcript/learningRecordResidentName.tsfrontend/src/pages/student/digital-transcript/learningRecordTableSort.tsfrontend/src/pages/student/digital-transcript/transcriptEntrySessionStorage.tsfrontend/src/pages/student/digital-transcript/transcriptReflectionConfig.tsfrontend/src/routes/app-routes.tsxfrontend/src/routes/index.tsxfrontend/src/routes/knowledge-routes.tsxfrontend/src/routes/learning-record-routes.tsxfrontend/src/styles/globals.cssfrontend/src/types/digital-transcript.tsfrontend/src/types/user.tsfrontend/src/utils/downloadLearningRecordPdf.ts
💤 Files with no reviewable changes (1)
- frontend/src/routes/knowledge-routes.tsx
| ON CONFLICT (name) DO NOTHING; | ||
|
|
||
| -- +goose Down | ||
| DELETE FROM public.feature_flags WHERE name = 'learning_record'; |
There was a problem hiding this comment.
Split DELETE and WHERE onto separate lines to satisfy SQLFluff.
Line 10 violates LT14 and can fail SQL lint gates.
Proposed fix
-DELETE FROM public.feature_flags WHERE name = 'learning_record';
+DELETE FROM public.feature_flags
+WHERE name = 'learning_record';🧰 Tools
🪛 SQLFluff (4.2.1)
[error] 10-10: The 'WHERE' keyword should always start a new line.
(LT14)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/migrations/00073_add_learning_record_feature_flag.sql` at line 10,
The DELETE statement violates SQLFluff LT14; reformat the statement so the
DELETE keyword and the WHERE clause are on separate lines to satisfy the linter:
locate the DELETE targeting public.feature_flags where name = 'learning_record'
and split it into a DELETE FROM public.feature_flags line followed by a separate
WHERE name = 'learning_record' line so the clause is on its own line.
Source: Linters/SAST tools
Comment on lines
+29
to
+31
| result := db.Model(entry). | ||
| Where("id = ? AND user_id = ?", entry.ID, entry.UserID). | ||
| Updates(entry) |
There was a problem hiding this comment.
Missing match checks allow false-success updates/deletes.
Line 29-Line 31 (and similarly Line 39-Line 40, Line 83-Line 84) only check result.Error; when no row matches ownership filters, handlers still return success. Please treat RowsAffected == 0 as not-found/unauthorized for these mutations.
Suggested fix
func (db *DB) UpdateLearningRecordEntry(entry *models.LearningRecordEntry) error {
- result := db.Model(entry).
+ result := db.Model(&models.LearningRecordEntry{}).
Where("id = ? AND user_id = ?", entry.ID, entry.UserID).
Updates(entry)
if result.Error != nil {
return newUpdateDBError(result.Error, "learning_record_entries")
}
+ if result.RowsAffected == 0 {
+ return newUpdateDBError(gorm.ErrRecordNotFound, "learning_record_entries")
+ }
return nil
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/database/learning_record.go` around lines 29 - 31, The
update/delete DB calls that use result := db.Model(entry)... (e.g., the update
path where result is assigned and similar blocks around lines handling deletion
and bulk operations) only check result.Error; change these to also check
result.RowsAffected and treat RowsAffected == 0 as a not-found/unauthorized
outcome: after the DB call (the result variable from
db.Model(entry).Where(...).Updates(entry) and the corresponding delete calls),
if result.Error != nil return the error, else if result.RowsAffected == 0 return
a not-found/unauthorized error (same error type/status your handlers use for
missing ownership), so that non-matching ownership filters do not return
success.
| func (db *DB) UpdateLearningRecordEntry(entry *models.LearningRecordEntry) error { | ||
| result := db.Model(entry). | ||
| Where("id = ? AND user_id = ?", entry.ID, entry.UserID). | ||
| Updates(entry) |
There was a problem hiding this comment.
Struct Updates(entry) drops zero-value writes.
Line 31 uses struct updates, so empty strings/zero numbers/false values are skipped by GORM. This breaks legitimate “clear this field” edits in learning record entries.
Suggested fix
- result := db.Model(&models.LearningRecordEntry{}).
- Where("id = ? AND user_id = ?", entry.ID, entry.UserID).
- Updates(entry)
+ result := db.Model(&models.LearningRecordEntry{}).
+ Where("id = ? AND user_id = ?", entry.ID, entry.UserID).
+ Select("*").
+ Omit("id", "user_id", "created_at").
+ Updates(entry)📝 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
| Updates(entry) | |
| result := db.Model(&models.LearningRecordEntry{}). | |
| Where("id = ? AND user_id = ?", entry.ID, entry.UserID). | |
| Select("*"). | |
| Omit("id", "user_id", "created_at"). | |
| Updates(entry) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/database/learning_record.go` at line 31, The code is using GORM's
Updates(entry) which omits zero-value fields (so clearing fields fails); change
the call that currently uses Updates(entry) in learning_record.go to use
UpdateColumns(entry) on the same model/DB chain (or alternatively construct a
map[string]interface{} and pass that to Updates) so zero/empty values are
written; update the call site where Updates(entry) is invoked to use
UpdateColumns(entry) (or a map) to ensure zero-value fields are persisted.
Comment on lines
+33
to
+39
| func (srv *Server) handleCreateLearningRecordEntry(w http.ResponseWriter, r *http.Request, log sLog) error { | ||
| var entry models.LearningRecordEntry | ||
| if err := json.NewDecoder(r.Body).Decode(&entry); err != nil { | ||
| return newJSONReqBodyServiceError(err) | ||
| } | ||
| entry.UserID = r.Context().Value(ClaimsKey).(*Claims).UserID | ||
| if err := srv.Db.CreateLearningRecordEntry(&entry); err != nil { |
There was a problem hiding this comment.
/entries handlers should force published state.
Line 33-Line 39 and Line 55-Line 57 let clients control is_draft. That allows draft-state payloads through published-entry endpoints, which breaks API semantics and can hide entries from the list query.
Suggested fix
func (srv *Server) handleCreateLearningRecordEntry(w http.ResponseWriter, r *http.Request, log sLog) error {
var entry models.LearningRecordEntry
if err := json.NewDecoder(r.Body).Decode(&entry); err != nil {
return newJSONReqBodyServiceError(err)
}
entry.UserID = r.Context().Value(ClaimsKey).(*Claims).UserID
+ entry.IsDraft = false func (srv *Server) handleUpdateLearningRecordEntry(w http.ResponseWriter, r *http.Request, log sLog) error {
@@
entry.ID = uint(id)
entry.UserID = r.Context().Value(ClaimsKey).(*Claims).UserID
+ entry.IsDraft = falseAlso applies to: 55-57
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/handlers/learning_record_handler.go` around lines 33 - 39, The
create and update handlers are allowing clients to set the draft flag; force
published state by overriding the entry's draft field before persisting. In
handleCreateLearningRecordEntry (before calling
srv.Db.CreateLearningRecordEntry) set the LearningRecordEntry's IsDraft (or
is_draft) to false/unset so clients cannot submit drafts to the published
endpoint; do the same in the corresponding update handler (the code path that
calls srv.Db.UpdateLearningRecordEntry / UpdateLearningRecordEntry) to ensure
incoming payloads cannot toggle entries back to draft.
Comment on lines
+47
to
+50
| id, err := strconv.Atoi(r.PathValue("id")) | ||
| if err != nil { | ||
| return newInvalidIdServiceError(err, "entry ID") | ||
| } |
There was a problem hiding this comment.
Reject non-positive path IDs before uint conversion.
Line 47-Line 50 and Line 66-Line 69 accept negative integers, then cast to uint, producing invalid large IDs. Validate id > 0 before using it.
Suggested fix
id, err := strconv.Atoi(r.PathValue("id"))
if err != nil {
return newInvalidIdServiceError(err, "entry ID")
}
+if id <= 0 {
+ return newBadRequestServiceError(nil, "entry ID must be a positive integer")
+}Also applies to: 66-69
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/src/handlers/learning_record_handler.go` around lines 47 - 50, The
code currently parses path IDs with strconv.Atoi (via r.PathValue("id")) and
then allows negative values to be cast to uint, producing invalid large IDs;
update both parsing sites (the blocks using strconv.Atoi and
newInvalidIdServiceError) to reject non-positive IDs by checking if id <= 0 and
returning newInvalidIdServiceError(errOrCustom, "entry ID") (or a clear
invalid-id error) before casting to uint; ensure the uint conversion only occurs
after the positive check so functions using the resulting uint get a valid ID.
Comment on lines
+76
to
+79
| className={cn( | ||
| 'space-y-2 border-b border-gray-100 pb-5 dark:border-border', | ||
| isPaper && '!px-0 !pt-0 pb-4' | ||
| )} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find tailwind version info
rg -n --hidden -S '"tailwindcss"|' package.json **/package.json 2>/dev/null || true
rg -n --hidden -S 'tailwindcss@' **/package-lock.json **/pnpm-lock.yaml **/yarn.lock 2>/dev/null || true
rg -n --hidden -S 'tailwindcss' frontend/package.json frontend/**/package.json 2>/dev/null || true
# Check the file in question
sed -n '60,95p' frontend/src/pages/student/digital-transcript/TranscriptResumePreview.tsx
# Search for other usages of the important modifier syntax
rg -n --hidden -S "'!px-0|!px-0|!pt-0|px-0!|pt-0!" frontend/src 2>/dev/null || trueRepository: UnlockedLabs/UnlockEdv2
Length of output: 7656
🌐 Web query:
Tailwind v4 important modifier syntax px-0! vs !px-0
💡 Result:
In Tailwind CSS v4, the correct syntax for the important modifier is to place the exclamation mark at the end of the utility class (e.g., px-0!) [1][2][3]. This change brings Tailwind's syntax in line with native CSS, where the !important rule is placed after the declaration [2]. While the older syntax of placing the exclamation mark at the beginning (e.g., !px-0) is supported in v4 for backwards compatibility, the trailing ! syntax is the recommended convention [4][5][3]. Key details regarding the important modifier in v4: - Consistency with native CSS: Placing the ! suffix at the end of the class makes it easier to reason about, especially when using variants [1][2]. - Usage: Use utility-name! to mark a specific utility as !important [1][4]. - @apply directives: The !important keyword is no longer supported within @apply directives [1][4]. Instead, you should append the ! suffix to each individual utility class used inside the @apply block (e.g., @apply flex! items-center!) [1]. - Global important: If you need to make all utilities !important globally, you can add an important flag to your Tailwind CSS import in your CSS file (e.g., @import "tailwindcss" important;) [6]. Note that in v4, this global flag can also cause utilities used within @apply to automatically receive !important [7][8].
Citations:
- 1: Add initial codemod tooling tailwindlabs/tailwindcss#14434
- 2: https://stackoverflow.com/questions/75664539/tailwind-css-class-precedence-is-not-respected
- 3: https://github.com/dxos/dxos/blob/e4919fd46f87a8107cac7af0def56ea6136edc1f/TAILWIND_V4_UTILITY_CHANGES.md
- 4: PostCSS + TailwindCSS 4 throws error on important! and ! modifiers tailwindlabs/tailwindcss#18570
- 5: Compatibility issue with hardcoded styles. tailwindlabs/tailwindcss#17124
- 6: Add support for
importantin v4 tailwindlabs/tailwindcss#14448 - 7: Using important automatically applies it to utilites set via @apply in v4 tailwindlabs/tailwindcss#18400
- 8: [v4] Breaking change: When using the important flag, @apply now also make all of the declarations !important tailwindlabs/tailwindcss#17448
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate Tailwind config files
fd -t f -e js -e ts -e cjs -e mjs -e json -e yml -e yaml tailwind.config . 2>/dev/null || true
fd -t f 'tailwind.config.*' . || true
# Print tailwind config if found (small)
for f in $(fd -t f -a 'tailwind.config.*' .); do
echo "---- $f ----"
# limit to first ~120 lines
sed -n '1,140p' "$f"
done
# Scan for important modifier usage patterns across frontend/src
rg -n --hidden -S "'!px-|\"!px-|!pt-|px-0!|pt-0!|!important" frontend/src || trueRepository: UnlockedLabs/UnlockEdv2
Length of output: 2464
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find tailwind config and print if any (likely small)
fd -t f 'tailwind.config.*' . || true
for f in $(fd -t f 'tailwind.config.*' . 2>/dev/null); do
echo "---- tailwind config: $f ----"
sed -n '1,220p' "$f"
done
# Inspect cn implementation (commonly in utils/cn.ts or similar)
fd -t f -e ts -e tsx -e js -e jsx -e mjs -e cjs 'cn.*' frontend/src || true
fd -t f -e ts -e tsx -e js -e jsx -e mjs -e cjs 'utils.*' frontend/src/utils || true
# Directly inspect the component file for imports
sed -n '1,140p' frontend/src/pages/student/digital-transcript/TranscriptResumePreview.tsx
# Also locate cn import path
rg -n "from ['\"].*cn['\"]" frontend/src/pages/student/digital-transcript/TranscriptResumePreview.tsx || trueRepository: UnlockedLabs/UnlockEdv2
Length of output: 4057
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find tailwind config files (any location)
find . -type f -name 'tailwind.config.*' 2>/dev/null || true
find . -type f -name 'postcss.config.*' 2>/dev/null || true
find . -type f -name 'tailwind.css' 2>/dev/null || true
# Locate cn implementation
fd -t f -e ts -e tsx -e js -e jsx 'utils.*' frontend/src/lib || true
fd -t f -e ts -e tsx -e js -e jsx 'utils.*' frontend/src || true
# Print likely cn file(s)
for f in $(rg -l --hidden "export const cn|export function cn|function cn" frontend/src/lib/utils.ts frontend/src/lib/utils.tsx frontend/src/lib/utils.* 2>/dev/null || true); do
echo "---- $f ----"
sed -n '1,220p' "$f"
done
# If not found via rg above, search broadly but limit to first few results
rg -n --hidden "from 'tailwind-merge'|tailwind-merge|twMerge" frontend/src/lib/utils.* frontend/src/lib -S || true
rg -n --hidden "export (const|function) cn" frontend/src -S || true
# Also quickly inspect the imported utils file referenced in TranscriptResumePreview
# (We know it's '`@/lib/utils`'; try to find that path)
fd -t f -e ts -e tsx -e js -e jsx 'lib/utils.*' frontend/src -a || trueRepository: UnlockedLabs/UnlockEdv2
Length of output: 1056
🌐 Web query:
Tailwind CSS v4 important modifier backwards compatibility: does '!px-0' still work or is it only suffix 'px-0!'?
💡 Result:
In Tailwind CSS v4, the recommended syntax for the important modifier is to place the '!' suffix at the end of the utility class (e.g., 'px-0!') [1][2][3][4]. Regarding backwards compatibility, the legacy syntax using the '!' prefix (e.g., '!px-0') is still supported in v4 [5][6]. However, this legacy usage is considered deprecated [4], and you should migrate to the trailing '!' syntax for all new code [7][4].
Citations:
- 1: https://stackoverflow.com/questions/75664539/tailwind-css-class-precedence-is-not-respected
- 2: https://josedacosta.github.io/tailwindcss-obfuscator/reference/tailwind-patterns
- 3: https://tailwindcss.com/docs/styling-with-utility-classes
- 4: https://tailwindcss.com/docs/upgrade-guide
- 5: PostCSS + TailwindCSS 4 throws error on important! and ! modifiers tailwindlabs/tailwindcss#18570
- 6: [v4] Support old important modifier position tailwindlabs/tailwindcss#13103
- 7: https://github.com/dxos/dxos/blob/e4919fd46f87a8107cac7af0def56ea6136edc1f/TAILWIND_V4_UTILITY_CHANGES.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Scan for legacy prefix and recommended suffix forms across the whole repo
rg -n --hidden -S "'!px-|!pt-|!m[trblxy]?-|!p[adx]?-|!space|!ring|!text|!border|px-0!|pt-0!|p[adx]?-[0-9-]+!" . || true
# Narrower targeted scans
rg -n --hidden -S "!px-[0-9]|!pt-[0-9]|px-[0-9]!|pt-[0-9]!" frontend/src || true
# Check exact tailwind-merge version from yarn.lock
rg -n --hidden -S "tailwind-merge@" frontend/yarn.lock || true
rg -n --hidden -S "tailwind-merge@\\^3" frontend/yarn.lock || trueRepository: UnlockedLabs/UnlockEdv2
Length of output: 4275
🌐 Web query:
tailwind-merge v3 important modifier syntax '!prefix' vs 'suffix' Tailwind CSS v4
💡 Result:
In tailwind-merge v3, which was released to support Tailwind CSS v4, the important modifier syntax transitioned from a prefix (!prefix) to a suffix (suffix!) [1][2][3]. Key details regarding this change include: * Preferred Syntax: Tailwind CSS v4 utilizes the suffix syntax (e.g., underline!) as the standard way to mark a utility as important [3]. * Backwards Compatibility: While the suffix position is preferred, tailwind-merge v3 maintains support for the legacy prefix syntax (e.g., !underline) to ensure backward compatibility [3]. * Usage in tailwind-merge v3: You can use the suffix syntax directly in your function calls, and tailwind-merge will correctly handle the conflicts [4][5]. For example: twMerge('p-3! p-4! p-5') // → 'p-4! p-5' twMerge('right-2! -inset-x-1!') // → '-inset-x-1!' [4][5] This change aligns tailwind-merge with the official Tailwind CSS v4 syntax, which moved the important modifier to the end of the class name [3]. Upgrading to tailwind-merge v3 is recommended when using Tailwind CSS v4 to ensure full compatibility and proper handling of these syntax updates [1][2].
Citations:
- 1: https://github.com/dcastil/tailwind-merge/releases/tag/v3.0.0
- 2: https://newreleases.io/project/github/dcastil/tailwind-merge/release/v3.0.0
- 3: Tailwind CSS v4 support dcastil/tailwind-merge#513
- 4: https://github.com/dcastil/tailwind-merge/blob/v3.3.1/docs/features.md
- 5: https://github.com/dcastil/tailwind-merge/blob/v3.5.0/docs/features.md
Tailwind v4 important modifier: migrate to suffix syntax for consistency
!px-0 !pt-0 is the deprecated legacy prefix form; Tailwind v4 + tailwind-merge v3 still support it, but the v4-recommended form is suffix (px-0! pt-0!).
Suggested fix
- isPaper && '!px-0 !pt-0 pb-4'
+ isPaper && 'px-0! pt-0! pb-4'📝 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
| className={cn( | |
| 'space-y-2 border-b border-gray-100 pb-5 dark:border-border', | |
| isPaper && '!px-0 !pt-0 pb-4' | |
| )} | |
| className={cn( | |
| 'space-y-2 border-b border-gray-100 pb-5 dark:border-border', | |
| isPaper && 'px-0! pt-0! pb-4' | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/pages/student/digital-transcript/TranscriptResumePreview.tsx`
around lines 76 - 79, The className string in TranscriptResumePreview's JSX uses
legacy Tailwind important-prefix syntax ('!px-0 !pt-0') when computing classes
via cn; update this to Tailwind v4 suffix important syntax by replacing those
tokens with 'px-0! pt-0!' so the conditional branch that uses isPaper continues
to apply the same important styles using the new suffix form inside the cn call.
| : 'text-[#556830] dark:text-primary' | ||
| )} | ||
| > | ||
| Achievements logs of {name} |
There was a problem hiding this comment.
Fix user-facing heading grammar.
“Achievements logs of {name}” reads incorrectly; use singular “log” (or “Achievement records”) for polished UI copy.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/pages/student/digital-transcript/TranscriptResumePreview.tsx` at
line 91, Update the user-facing heading in the TranscriptResumePreview
component: replace the incorrect phrase "Achievements logs of {name}" with a
grammatically correct variant such as "Achievement record for {name}" or
"Achievement records for {name}" (use the chosen wording wherever the heading
string is rendered and ensure it still interpolates the name variable).
| hasIncompleteEntry={hasDraft} | ||
| topUserContent={topUserContent} | ||
| topFacilityContent={topFacilityContent} | ||
| featuredLibraries={featuredLibraries ?? []} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Unnecessary null coalescing for featuredLibraries.
Line 106 uses featuredLibraries ?? [] when passing the prop to ResidentHomeDashboard, but featuredLibraries is destructured from useLoaderData() (line 47) which always returns an object from the loader. The loader at routeLoaders.ts (line 87) always includes featuredLibraries in the returned JSON, even if empty (featuredLibrariesRaw defaults to [] when the API call fails). The ?? operator adds no protection here.
♻️ Minor cleanup
- featuredLibraries={featuredLibraries ?? []}
+ featuredLibraries={featuredLibraries}📝 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
| featuredLibraries={featuredLibraries ?? []} | |
| featuredLibraries={featuredLibraries} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/pages/student/ResidentHome.tsx` at line 106, Remove the
redundant null-coalescing when passing featuredLibraries to
ResidentHomeDashboard: since featuredLibraries is obtained from useLoaderData()
and the loader (routeLoaders.ts) always includes featuredLibraries (defaulting
to []), change the prop from featuredLibraries={featuredLibraries ?? []} to
simply featuredLibraries={featuredLibraries}; update the JSX in ResidentHome
(the prop passed to ResidentHomeDashboard) accordingly and run tests/lint to
ensure no further references rely on the fallback.
Comment on lines
+349
to
+351
| const incompleteEntry = useMemo(() => { | ||
| return findIncompleteAchievementEntry(learningRecordEntries, learningRecordFormVariant); | ||
| }, [hasIncompleteEntry, learningRecordEntries, entrySessionTick, learningRecordFormVariant]); |
There was a problem hiding this comment.
Incorrect dependency in useMemo for incompleteEntry.
The useMemo hook includes hasIncompleteEntry as a dependency (line 351), but hasIncompleteEntry is a prop passed from the parent and isn't used inside the findIncompleteAchievementEntry function. The function only needs learningRecordEntries, entrySessionTick, and learningRecordFormVariant. Including hasIncompleteEntry could cause unnecessary recomputation or, worse, could mask bugs if the parent's hasIncompleteEntry becomes stale relative to the actual learningRecordEntries.
🐛 Fix the dependencies
const incompleteEntry = useMemo(() => {
return findIncompleteAchievementEntry(learningRecordEntries, learningRecordFormVariant);
- }, [hasIncompleteEntry, learningRecordEntries, entrySessionTick, learningRecordFormVariant]);
+ }, [learningRecordEntries, entrySessionTick, learningRecordFormVariant]);📝 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 incompleteEntry = useMemo(() => { | |
| return findIncompleteAchievementEntry(learningRecordEntries, learningRecordFormVariant); | |
| }, [hasIncompleteEntry, learningRecordEntries, entrySessionTick, learningRecordFormVariant]); | |
| const incompleteEntry = useMemo(() => { | |
| return findIncompleteAchievementEntry(learningRecordEntries, learningRecordFormVariant); | |
| }, [learningRecordEntries, entrySessionTick, learningRecordFormVariant]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/pages/student/ResidentHomeDashboard.tsx` around lines 349 - 351,
The useMemo for incompleteEntry currently includes an unnecessary dependency
hasIncompleteEntry; update the dependency array for the useMemo that defines
incompleteEntry to only include the actual inputs used by
findIncompleteAchievementEntry — learningRecordEntries, entrySessionTick, and
learningRecordFormVariant — so remove hasIncompleteEntry from the dependencies
of the useMemo (the useMemo call creating incompleteEntry that invokes
findIncompleteAchievementEntry).
Comment on lines
+12
to
+35
| { | ||
| path: 'learning-record-funnel', | ||
| element: <DigitalTranscriptHome />, | ||
| errorElement: <Error />, | ||
| handle: { title: 'Learning Record' } | ||
| }, | ||
| { | ||
| path: 'learning-record-funnel/entry', | ||
| element: <DigitalTranscriptEntryPage />, | ||
| errorElement: <Error />, | ||
| handle: { title: 'Add your achievement' } | ||
| }, | ||
| { | ||
| path: 'learning-record-categories', | ||
| element: <DigitalTranscriptHome />, | ||
| errorElement: <Error />, | ||
| handle: { title: 'Learning Record' } | ||
| }, | ||
| { | ||
| path: 'learning-record-categories/entry', | ||
| element: <DigitalTranscriptEntryPage />, | ||
| errorElement: <Error />, | ||
| handle: { title: 'Add your achievement' } | ||
| }, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Two route groups render identical components.
Both learning-record-funnel (lines 12-23) and learning-record-categories (lines 24-35) route groups render the same components (DigitalTranscriptHome and DigitalTranscriptEntryPage). If these are intended for variant testing or future differentiation, consider adding a comment explaining the rationale. If they're truly identical, consider consolidating to a single route group.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/routes/learning-record-routes.tsx` around lines 12 - 35, The
routes for 'learning-record-funnel' and 'learning-record-categories' both render
the same components (DigitalTranscriptHome and DigitalTranscriptEntryPage);
either consolidate them into a single route group or add an explanatory comment;
specifically, in the route definitions where path: 'learning-record-funnel' and
path: 'learning-record-categories' reference DigitalTranscriptHome and
DigitalTranscriptEntryPage, decide whether to merge into one path (removing the
duplicate group) or keep both and add a clear comment above each route
explaining why both exist (e.g., A/B/variant testing or future divergence) so
future readers understand the intent.
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
3 participants
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.
Pre-Submission PR Checklist
Description of the change
This PR adds backend persistence for the Learning Record feature allowing residents to log
Backend
/api/learning-record/Frontend
How to test
Screenshot(s)
Additional context
Technical design decision — column-per-question schema
The questionnaire is stored in a dedicated table with each question as its own column, rather than a single JSONB column containing the full response payload. This prioritizes maintainability through migrations: