Add picture password confirm modal#14691
Add picture password confirm modal#14691akolson wants to merge 10 commits intolearningequality:developfrom
Conversation
After a successful picture password submission, show a confirmation
modal displaying the learner's name and icon sequence before completing
sign-in. The user must tap Yes to proceed or No to discard the session
and return to the grid.
- Refactor useUser.login() to always return { data, error } and update
all callers (SignInPage/index.vue, SettingUpKolibri.vue)
- Add PicturePasswordConfirmModal with custom overlay, KFocusTrap, focus
trap via window focus listener, themed action buttons with hover states
- Add confirmation strings to picturePasswordStrings (isThisYou,
yesConfirmAction, noGoBackAction, yourPasswordIs)
- Add tests for modal show, no premature redirect, confirm, and cancel
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Validates picture password credentials server-side without creating a session, returning the learner's full name so the confirm modal can display it. The real login only happens when the user taps the confirm button, preventing a page refresh while the modal is open from redirecting an unauthenticated user to a secure area. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
github-actions
Bot
added
DEV: backend
npm Package VersionsWarning The following packages have changed files but no version bump:
If these changes affect published code, consider bumping the version. |
akolson
changed the title
prevalidate is passed in the POST body, not as a query param. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Solid two-phase login implementation with clean cancel semantics; a few items to fix before merge.
Frontend tests and browser smoke test pass. Python unit tests still pending. No screenshots in the PR body, and the dev server is unavailable for local generation — see blocking finding below.
Blocking
- Missing
aria-labelledbyonrole="dialog"— see inline comment onPicturePasswordConfirmModal.vue:7 - No screenshots; this flow requires facility setup that makes it non-trivial to verify visually without them — please attach screenshots of: (1) the modal with learner name displayed, and (2) the grid cleared after cancel
Suggestion
KFocusTrapis wired without@shouldFocusFirstEl/@shouldFocusLastElhandlers — see inline onPicturePasswordConfirmModal.vue:4- RTL centering bug: RTLCSS will flip
left: 50%→right: 50%on.modal-card, off-centering the modal in RTL — see inline onPicturePasswordConfirmModal.vue:210
Nitpick
/deep/ buttonis the deprecated Vue 2 deep selector; prefer::v-deep(button)— see inline onPicturePasswordConfirmModal.vue:262prevalidate: falseis always included in the POST body for normal logins — see inline onuseUser.js:83
Praise
- Clean prevalidate design — see inline on
PictureSignInPage.vue:156 - No-DELETE assertion in cancel test — see inline on
PictureSignInPage.spec.js:180
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Reviewed the pull request diff checking for:
- Correctness: bugs, edge cases, undocumented behavior, resource leaks, hardcoded values
- Design: unnecessary complexity, naming, readability, comment accuracy, redundant state
- Architecture: duplicated concerns, minimal interfaces, composition over inheritance
- Testing: behavior-based assertions, mocks only at hard boundaries, accurate coverage
- Completeness: missing dependencies, unupdated usages, i18n, accessibility, security
- Principles: DRY (same reason to change), SRP, Rule of Three (no premature abstraction)
- Checked CI status and linked issue acceptance criteria
- For UI changes: inspected screenshots for layout, visual completeness, and consistency
| <KFocusTrap> | ||
| <div | ||
| ref="modalCard" | ||
| role="dialog" |
There was a problem hiding this comment.
blocking: The role="dialog" element has no accessible name. WCAG 4.1.2 requires that UI components, including dialogs, have a programmatically determinable name. Without aria-labelledby (or aria-label), screen readers announce the dialog without context, leaving users uncertain what they are confirming.
Add an id to the <h1> and reference it:
<h1 id="confirm-modal-title" class="modal-title" ...>
{{ isThisYou$() }}
</h1><div
ref="modalCard"
role="dialog"
aria-modal="true"
aria-labelledby="confirm-modal-title"
...
>| <template> | ||
|
|
||
| <div class="modal-overlay"> | ||
| <KFocusTrap> |
There was a problem hiding this comment.
suggestion: KFocusTrap is used without the @shouldFocusFirstEl / @shouldFocusLastEl handlers that complete the focus loop. Without them, the sentinel <div> elements trap a tab key press but don't redirect focus back into the modal — the real trap is the handleFocusEscape listener registered on window.
See packages/kolibri/components/language-switcher/LanguageSwitcherModal.vue:4-5 for the expected pattern. Either add the handlers here (and call confirmBtn.$el.focus() / modalCard.value.focus() in response), or remove KFocusTrap entirely and rely solely on handleFocusEscape.
| .modal-card { | ||
| position: absolute; | ||
| top: 50%; | ||
| left: 50%; |
There was a problem hiding this comment.
suggestion: RTLCSS will flip left: 50% → right: 50% in RTL mode. Combined with transform: translate(-50%, -50%), the modal won't be centered: the right edge would land at the midpoint of the overlay, then translate shifts the card further left, putting it off-screen on narrow viewports.
Fix by preventing the flip:
.modal-card {
position: absolute;
top: 50%;
/* rtl:ignore */
left: 50%;
...
transform: translate(-50%, -50%);
}Alternatively, use flexbox centering on .modal-overlay (display: flex; align-items: center; justify-content: center) which is symmetric and doesn't need RTL handling.
| .btn-bg { | ||
| border-radius: 8px; | ||
|
|
||
| /deep/ button { |
There was a problem hiding this comment.
nitpick: /deep/ is the deprecated Vue 2 deep-selector syntax. The current preferred form is ::v-deep(button) (or ::v-deep button for broader compat). Some toolchains warn on /deep/.
There was a problem hiding this comment.
Our current linting rules forbid this. So we maya have to use it until updated
There was a problem hiding this comment.
Understood — if the project's linting rules require , that's the right call. Withdrawing this finding.
| os, | ||
| }, | ||
| const response = await client({ | ||
| data: { ...sessionPayload, active: true, browser, os, prevalidate }, |
There was a problem hiding this comment.
nitpick: prevalidate is always spread into the request body, so every normal login now sends prevalidate: false. The backend ignores the falsy value, but it adds noise to the payload. Consider only including the field when truthy:
data: { ...sessionPayload, active: true, browser, os, ...(prevalidate && { prevalidate }) },There was a problem hiding this comment.
This doesn't seem seem to apply after the current changes, I think?
There was a problem hiding this comment.
You're right — this was resolved. In round 2 I noted it as "Newly resolved": prevalidate is now passed as a query param only when truthy, so it's no longer included in the body for normal logins. Apologies for not replying directly in this thread.
| } | ||
| } | ||
|
|
||
| function handleCancel() { |
There was a problem hiding this comment.
praise: Because prevalidation doesn't create a server session, cancel is a pure UI reset — no DELETE request, no error-state cleanup needed. This is a clean design choice that keeps the cancel path simple and reliable.
| expect(checkbox(star())).not.toBeChecked(); | ||
| expect(checkbox(moon())).not.toBeChecked(); | ||
| }); | ||
| expect(client).not.toHaveBeenCalledWith(expect.objectContaining({ method: 'delete' })); |
There was a problem hiding this comment.
praise: Explicitly asserting that no DELETE is issued on cancel is thoughtful — it documents the architectural intent (prevalidate = no session to clean up) and would catch a regression if someone added session teardown to the cancel path.
Build Artifacts
Smoke test screenshot |
LianaHarris360
left a comment
LianaHarris360
left a comment
There was a problem hiding this comment.
I reviewed and tested everything locally, works as expected. The prevalidate flag was a good addition. Noted a few things to address, but nothing major stood out.
| ) | ||
| if serializer.is_valid(): | ||
| user = serializer.validated_data["user"] | ||
| if request.data.get("prevalidate"): |
There was a problem hiding this comment.
Hmm, seem like this would be true for any value that's not empty, could we extend this to be if request.data.get("prevalidate") is True instead
There was a problem hiding this comment.
Noting that we might need to move back to to passing the prevalidate as a query param based on Blaine's comment here so this may change to literal check of the value "true" as all query params values are considered strings.
|
|
||
| function handleCancel() { | ||
| showConfirmModal.value = false; | ||
| wrongSequence.value = true; |
There was a problem hiding this comment.
I understand why wrongSequence was reused to clear the grid selection here, but since future animations are expected to triggered by wrongSequence, they will also be triggered when handleCancel is called, which isn't the intended behavior.
To prevent that, perhaps we should have a separate clearSelection event (or prop?) on PicturePasswordGrid that resets the icon selection without implying an error.
| const err = await login(sessionPayload); | ||
| if (err) { | ||
| const { error } = await login(sessionPayload); | ||
| if (error) { |
There was a problem hiding this comment.
I think we should also reset submittedPicturePassword and confirmedLearnerName within this if block.
Clears all modal state when handleConfirm receives a login error, not just showConfirmModal and wrongSequence. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
wrongSequence carries error semantics and may trigger animations in future. Cancel is not an error, so introduce a separate clearSelection prop that resets the icon selection without implying a wrong attempt. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
?prevalidate=true is a flag that controls how the endpoint functions, not data being POSTed — treating it as a variation of the destination address rather than payload is more semantically accurate. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Three prior findings remain unaddressed; a couple of new items below.
CI: 6/9 checks completed at review time, 3 still running.
Visual inspection: compared implementation against design spec from issue #14428 — yellow header, learner name, icon sequence, cancel/confirm buttons all match. Manual QA not possible (no dev server running).
Prior findings
1 prior finding unchanged (acknowledged — /deep/ selector, per project linting rules; I withdrew this finding in my previous review).
Newly resolved:
prevalidatealways sent in request body (nitpick) ← was UNADDRESSED — now passed as query param only when truthy:params: prevalidate ? { prevalidate } : undefined
Still unaddressed (re-raised below):
- Missing
aria-labelledbyonrole="dialog"(blocking) — no reply, no code change KFocusTrapwithout focus-loop handlers (suggestion) — no reply, no code change- RTLCSS flips
left: 50%in RTL (suggestion) — no reply, no code change
1/5 prior findings resolved. 3 re-raised below.
Findings:
- blocking:
PicturePasswordConfirmModal.vue:7—aria-labelledbystill missing (re-raised) - suggestion:
PicturePasswordConfirmModal.vue:4—KFocusTrapfocus-loop handlers (re-raised) - suggestion:
PicturePasswordConfirmModal.vue:210— RTLCSSleft: 50%(re-raised) - suggestion:
useUser.js:77—login()can returnundefinedfor unrecognized errors - suggestion:
PictureSignInPage.spec.js:183— missing test forhandleConfirmerror path - praise:
PicturePasswordGrid.vue:215— cleanclearSelection/clearSelectionHandledpattern
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews deterministically via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
- Checked CI status and linked issue acceptance criteria
| <KFocusTrap> | ||
| <div | ||
| ref="modalCard" | ||
| role="dialog" |
There was a problem hiding this comment.
blocking: (Re-raised — unaddressed.)
role="dialog" still has no accessible name. WCAG 4.1.2 requires dialogs to have a programmatically determinable name; without aria-labelledby, screen readers announce the dialog without context.
Add an id to the <h1> and reference it:
<h1 id="confirm-modal-title" class="modal-title" ...>
{{ isThisYou$() }}
</h1><div
ref="modalCard"
role="dialog"
aria-modal="true"
aria-labelledby="confirm-modal-title"
...
>| <template> | ||
|
|
||
| <div class="modal-overlay"> | ||
| <KFocusTrap> |
There was a problem hiding this comment.
suggestion: (Re-raised — unaddressed.)
KFocusTrap is used without @shouldFocusFirstEl / @shouldFocusLastEl handlers that complete the focus cycle. The sentinel <div> elements trap Tab but don't redirect focus back into the modal — focus containment relies solely on the handleFocusEscape listener on window.
See packages/kolibri/components/language-switcher/LanguageSwitcherModal.vue:4-5 for the expected pattern: add the event handlers and call confirmBtn.$el.focus() / modalCard.value.focus() in response, or remove KFocusTrap entirely and rely on handleFocusEscape alone.
| .modal-card { | ||
| position: absolute; | ||
| top: 50%; | ||
| left: 50%; |
There was a problem hiding this comment.
suggestion: (Re-raised — unaddressed.)
RTLCSS will flip left: 50% → right: 50% in RTL mode. Combined with transform: translate(-50%, -50%), the card will not be centered — the right edge lands at 50% from the container's right, then the transform shifts it further left.
Fix by preventing the flip:
/* rtl:ignore */
left: 50%;Alternatively, center via flexbox on .modal-overlay (display: flex; align-items: center; justify-content: center), which is direction-agnostic and needs no RTL override.
| * @param {boolean} [enableRedirect=true] - When false, suppresses the post-login redirect, | ||
| * allowing the caller to handle navigation manually. | ||
| */ | ||
| async function login(sessionPayload, prevalidate = false, enableRedirect = true) { |
There was a problem hiding this comment.
suggestion: When CatchErrors returns nothing (unrecognized server error), handleApiError is called and login() falls off the catch block, implicitly returning undefined. Callers that destructure the result (const { data, error } = await login(...)) will throw a TypeError, caught by their own catch blocks — which then show a generic snackbar while handleApiError may have already shown an error UI.
Consider adding an explicit return in the else branch:
} else {
handleApiError({ error });
return { data: null, error: null };
}| expect(client).not.toHaveBeenCalledWith(expect.objectContaining({ method: 'delete' })); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
suggestion: No test covers the path where the real login call inside handleConfirm returns an error (e.g., the picture password is revoked between prevalidation and confirmation). The suite covers prevalidation failure and cancel, but not this path.
Suggested test: mock login to return { data: { full_name: MOCK_LEARNER_NAME }, error: null } on the first call (prevalidate) and { data: null, error: LoginErrors.INVALID_CREDENTIALS } on the second (confirm), then verify the modal is dismissed and wrongSequence fires.
| ); | ||
|
|
||
| watch( | ||
| () => props.clearSelection, |
There was a problem hiding this comment.
praise: The clearSelection / clearSelectionHandled round-trip is clean — parent signals intent, child handles its own internal state reset and reports completion. Good encapsulation; mirrors the established wrongSequence / wrongSequenceHandled pattern.
- Add aria-labelledby on role="dialog" pointing to the h1 title (WCAG 4.1.2) - Replace absolute/transform centering with flexbox on the overlay to prevent RTLCSS from flipping left: 50% in RTL layouts - Wire @shouldFocusFirstEl / @shouldFocusLastEl on KFocusTrap so the tab loop is completed via the component rather than a window listener - Focus the modal title h1 on mount so screen readers announce the dialog immediately; prevent backdrop clicks from stealing focus via @mousedown.prevent on the overlay - Add test for confirm-login failure path (credentials revoked between prevalidation and confirmation) Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Good work resolving all four prior unaddressed findings — the aria-labelledby, RTL, and KFocusTrap fixes are solid. One new issue introduced in this round needs attention.
CI: all 13 checks pass. Visual inspection: code-only review; this commit is accessibility/focus fixes only, and design-spec conformance from round 2 remains valid. Manual QA not possible (no dev server running).
Prior findings
Newly resolved:
aria-labelledbyonrole="dialog"(blocking) ← was UNADDRESSED —aria-labelledby="confirm-modal-title"added to dialog div,idadded toh1KFocusTrapfocus-loop handlers (suggestion) ← was UNADDRESSED —@shouldFocusFirstEl/@shouldFocusLastEladded- RTL centering via
left: 50%(suggestion) ← was UNADDRESSED — replaced with direction-agnostic flexbox handleConfirmerror path test (suggestion) ← was UNADDRESSED — test added
Still unaddressed (re-raised below):
login()returnsundefinedin unrecognized-error branch (suggestion) — no reply, no code change
4/5 prior findings resolved. 1 re-raised below.
Blocking:
onMountednow focuses the heading instead of a confirm/cancel action, violating the acceptance criterion — see inline onPicturePasswordConfirmModal.vue:138
Suggestion (re-raised):
packages/kolibri/composables/useUser.js:119— Theelsebranch callshandleApiErrorwithout returning. Callers destructureconst { data, error } = await login(...)— when this branch executes,login()returnsundefinedand the destructuring throwsTypeError, caught by callers' owncatchblocks as a generic snackbar (whilehandleApiErrormay already be showing error UI). Add an explicit fallback:return { data: null, error: null };
Praise:
- Flexbox centering on
.modal-overlay— see inline onPicturePasswordConfirmModal.vue:197
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews deterministically via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
- Checked CI status and linked issue acceptance criteria
| return yourPasswordIs$({ labels }); | ||
| }); | ||
|
|
||
| onMounted(() => { |
There was a problem hiding this comment.
blocking: onMounted now focuses modalTitle (the h1 heading) instead of a confirm or cancel action. The acceptance criterion for this issue requires "initial focus lands on the confirm or cancel action" — focusing a heading element with tabindex="-1" does not satisfy this.
The previous code focused the confirm button (confirmBtn.value.$el.focus()), which met the AC. Restore that, or focus the cancel button if preferred:
onMounted(() => {
confirmBtn.value.$el.focus();
});Note: @shouldFocusFirstEl can continue to target modalTitle if you want the heading announced when the user cycles focus, but the initial mount should land on an interactive control per the AC.
| top: 0; | ||
| left: 0; | ||
| z-index: 24; | ||
| display: flex; |
There was a problem hiding this comment.
praise: Flexbox centering on .modal-overlay is a clean, direction-agnostic fix — align-items: center; justify-content: center centers the card symmetrically in both LTR and RTL without any rtl:ignore overrides or transform hacks.
AC requires initial focus to land on an interactive control, not a heading. Keep @shouldFocusFirstEl targeting the title for screen reader announcement when cycling focus. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
One prior blocking finding fully resolved; one new observation below.
CI: all checks pass (Raspberry Pi image build pending — not merge-blocking). Visual inspection: the only change in this push is the onMounted focus line — no visual modifications introduced. Design-spec match was confirmed in round 3; this round is out of scope for a new visual comparison.
Prior findings
1 prior finding from round 4.
Newly resolved:
- Initial
onMountedfocus targetsmodalTitle(h1 heading) instead of confirm/cancel button (blocking) ← was UNADDRESSED in round 4 — code changed atPicturePasswordConfirmModal.vue:138
1/1 prior findings resolved. 0 re-raised below.
New findings:
- suggestion:
@shouldFocusFirstElhandler focuses the heading instead of the cancel button — see inline comment - praise: clean one-line fix restoring focus to the confirm button on mount
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews deterministically via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
- Checked CI status and linked issue acceptance criteria
| @mousedown.prevent | ||
| > | ||
| <KFocusTrap | ||
| @shouldFocusFirstEl="modalTitle.focus()" |
There was a problem hiding this comment.
suggestion: shouldFocusFirstEl fires when Tab wraps forward past the last interactive element (the confirm button), signalling "redirect to the first interactive element." The LanguageSwitcherModal reference pattern calls languageItem[0].focus() — i.e., the first focusable control — in its focusFirstEl handler. Here the handler focuses modalTitle (the h1 with tabindex="-1"), so a screen reader user cycling Tab from the confirm button would hear "Is this you?" (heading announcement) before Tab advances to the cancel button.
Fix: add a ref to the cancel KIconButton and focus its $el in this handler:
<KIconButton
ref="cancelBtn"
icon="close"
...
/>@shouldFocusFirstEl="cancelBtn.$el.focus()"There was a problem hiding this comment.
I think it makes more sense to focus the modal title on open modal. That way the user has context of the on what the dialog is about. Focusing the cancel button distorts the navigation order and makes it difficult to makes sense of the open dialog
There was a problem hiding this comment.
Understood — if also fires on initial modal open (not just Tab wrap-around), then focusing the title to announce the dialog heading is a reasonable choice for orienting screen reader users. Withdrawing this finding.
| return yourPasswordIs$({ labels }); | ||
| }); | ||
|
|
||
| onMounted(() => { |
There was a problem hiding this comment.
praise: Clean one-line fix — restoring confirmBtn.value.$el.focus() on mount correctly satisfies the acceptance criterion (initial focus lands on the confirm action) without any added complexity.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
One suggestion carried over from prior rounds; no new code to review since round 5.
CI passing. Visual inspection carried forward from round 3 (design-spec match confirmed). No dev server available for manual QA.
Prior findings
14 prior findings unchanged (resolved or acknowledged).
Newly contested (not re-raised — non-blocking):
@shouldFocusFirstElfocuses heading (h1) instead of cancel button (suggestion from round 5) — author replied: prefers heading focus to give context when Tab wraps
Still unaddressed (re-raised below):
login()returnsundefinedin unrecognized-error branch (suggestion, originally raised round 3) — no reply, no code change
0/1 prior round-5 findings newly resolved. 1 re-raised below.
- suggestion (re-raised):
useUser.js:120—handleApiErrorcall without explicit return; see inline comment
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews deterministically via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
- Checked CI status and linked issue acceptance criteria
| } | ||
| return { data: null, error: loginError }; | ||
| } else { | ||
| handleApiError({ error }); |
There was a problem hiding this comment.
suggestion (re-raised — still unaddressed): The else branch calls handleApiError and falls off with an implicit undefined return. Both new callers in PictureSignInPage.vue destructure the result (const { data, error } = await login(...) at line 112, and const { error } = await login(...) at line 144) — when this branch runs, the destructuring throws TypeError: Cannot destructure property of undefined, which each caller's catch block catches and displays as a generic snackbar, while handleApiError may already have shown a server-error message.
Add an explicit return after the handleApiError call:
} else {
handleApiError({ error });
return { data: null, error: null };
}
There was a problem hiding this comment.
@akolson You were misled here by @rtibblesbot. Returning anything in this branch is dead code because handleApiError has a default shouldThrow = true, meaning it will log the error then re-throw it, and the return will never happen.
There was a problem hiding this comment.
You're right — I missed that defaults to , which re-throws the error before the is ever reached, making it dead code. Withdrawing this finding.
There was a problem hiding this comment.
Thanks @bjester for catching this! Not sure how i missed this one 😞 ! Will rectifiy
Prevents TypeError when callers destructure the return value after handleApiError runs on an unexpected server error. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
All prior findings resolved, acknowledged, or contested — the one remaining open suggestion is closed in this push.
CI: all checks pass. Visual inspection: useUser.js is the only file changed; no UI modifications. Design-spec conformance from prior rounds remains valid.
Prior findings
15 prior findings unchanged (14 resolved or acknowledged; 1 contested — @shouldFocusFirstEl focuses heading for context on open, author preference, not re-raised).
Newly resolved:
login()returnsundefinedin unrecognized-error branch (suggestion, raised rounds 3–6) ← RESOLVED —return { data: null, error: null }added atuseUser.js:121
1/1 open finding resolved. 0 new findings.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews deterministically via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
- Checked CI status and linked issue acceptance criteria
| return { data: null, error: loginError }; | ||
| } else { | ||
| handleApiError({ error }); | ||
| return { data: null, error: null }; |
There was a problem hiding this comment.
praise: Minimal, correct fix — handleApiError already handles the error UI, so returning { data: null, error: null } gives callers a consistent destructurable shape without adding noise.
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
All prior findings resolved, acknowledged, or contested — clean to merge.
CI passing (frontend tests path-skipped on this single-line commit, as expected; prior frontend CI runs covered the .vue changes). No UI modifications in this commit; design-spec conformance from round 3 remains valid.
Prior findings
All 15 prior findings remain resolved or acknowledged — no changes since round 7 approval.
The login() undefined suggestion (rounds 2–6, resolved round 7) was contested post-approval: author noted handleApiError re-throws before the return is reached for most errors; bot agreed and withdrew. This commit removes the return.
One observation for the record (no action needed): handleApiError (packages/kolibri/utils/appError.js:45–49) has one early-return path for disconnection errors that does not throw, so login() returns undefined in that case. Both callers (prevalidate at line 112 and handleConfirm at line 144 in PictureSignInPage.vue) handle this via defensive try/catch blocks — a TypeError from destructuring undefined is caught and shown as a snackbar alongside the heartbeat disconnection overlay. This was the pre-PR behavior for all logins and is acceptable.
0 new findings.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Compared the current PR state against findings from a prior review:
- Retrieved prior bot reviews deterministically via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Reviewed the pull request diff checking for correctness, design, architecture, testing, completeness, and adherence to DRY/SRP principles
- Checked CI status and linked issue acceptance criteria
4 participants
Summary
PicturePasswordConfirmModal— displays the learner's name and icon sequence after a correct picture password entry, with confirm and cancel actionsprevalidateflag toPOST /api/auth/session/that validates credentials without creating a session, returning{ full_name }— prevents a page refresh while the modal is open from redirecting to a secure arealogin()inuseUser.jswith explicitprevalidateandenableRedirectparameters, keeping frontend-only concerns out of the server payloadChecklist
General
Accessibility and i18n
References
Closes #14428
Reviewer guidance
AI usage
I used Claude Code to implement the feature, prompting it to follow existing patterns in the codebase. I reviewed and directed the generated code throughout, making decisions on API design (prevalidate as a body param,
login()signature) and removing unnecessary complexity at each step. I verified all tests pass after each change.