Fix mark-all-present switch requiring two clicks after modal cancel#14552
Fix mark-all-present switch requiring two clicks after modal cancel#14552akolson wants to merge 2 commits intolearningequality:release-v0.19.xfrom
Conversation
github-actions
bot
added
APP: Coach
Build Artifacts
Smoke test screenshot |
akolson
force-pushed
the
attendance-mark-all-double-click
branch
3 times, most recently
from
83941b7 to
0018f86
Compare
KSwitch optimistically toggles its internal state on click. When the "mark all present" modal was cancelled, allPresent stayed false (unchanged), so Vue never re-rendered the switch — leaving it visually on. The next click then fired @change(false) (a no-op) instead of reopening the modal. Introduce pendingMarkAll ref set true when the modal opens and false when cancelled or confirmed. The switch binds to markAllSwitchValue (allPresent || pendingMarkAll), so cancelling creates a real true→false prop change that forces the KSwitch to re-render back to unchecked. Adds a regression test reproducing the exact cancel→single-click sequence. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
akolson
force-pushed
the
attendance-mark-all-double-click
branch
from
0018f86 to
6abb4ae
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Clean, minimal fix for a subtle KSwitch reactive-update problem.
CI: All checks passing (Frontend tests, linting, Python unit tests).
Visual: No screenshots provided — behavioral fix; see suggestion below. Dev server unavailable at review time.
- suggestion: A short screencast or GIF showing the fixed sequence (switch click → modal opens → Go back → switch is unchecked → single click reopens modal) would help future readers and manual QA. The regression test fully validates the behavior, but a visual recording reduces reviewer friction for this type of interaction bug.
- praise: See inline comments.
@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
| // user has clicked "mark all present" and is seeing the confirmation modal (pendingMarkAll). | ||
| // When the modal is canceled, pendingMarkAll resets to false, which changes this computed | ||
| // from true → false and gives Vue a real prop change to re-render the KSwitch correctly. | ||
| const markAllPresent = computed(() => allPresent.value || pendingMarkAll.value); |
There was a problem hiding this comment.
praise: The block comment (lines 99–102) is exactly the right level of detail — it explains the problem, the mechanism, and the cancellation invariant in a way that makes the computed immediately comprehensible without reading surrounding code. Future readers will thank you.
| expect(buttons.length).toBe(2); | ||
| }); | ||
|
|
||
| it('re-opens modal on a single click after "Go back" was clicked in the mark-all modal', async () => { |
There was a problem hiding this comment.
praise: The regression test mirrors the bug report's step sequence precisely (open modal → Go back → assert switch is unchecked → assert one click reopens modal). Testing behavior rather than implementation details, and the inline comment correctly documents the pre-fix failure mode for future readers.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Good housekeeping commit — string migration is correct and CI still passes.
Prior findings
Unaddressed (re-raised below):
- Screencast/GIF for the fixed interaction sequence (suggestion) — no reply, no code change
Stable (not re-raised):
- Praise on
useAttendanceForm.js:103(block comment quality) — still applies - Praise on spec regression test structure — still applies
1/3 prior findings resolved. 1 re-raised below.
CI passing.
Visual: dev server unavailable; no screenshots in PR. New commit is spec-only — no new UI surface to capture.
- suggestion: The prior suggestion about adding a screencast or GIF of the fixed interaction sequence (switch click → modal opens → Go back → switch unchecked → one click reopens modal) remains open. The regression test covers it functionally, but a visual recording would reduce friction for anyone manually QA-ing this kind of interaction bug on future visits.
@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
| import { coreString } from 'kolibri/uiText/commonCoreStrings'; | ||
| // eslint-disable-next-line import/named | ||
| import useSnackbar, { useSnackbarMock } from 'kolibri/composables/useSnackbar'; | ||
| import { attendanceStrings } from 'kolibri-common/strings/attendanceStrings'; |
There was a problem hiding this comment.
praise: Replacing all hard-coded English strings with coreString()/attendanceStrings.*$() calls is thorough and correct — verified each mapping against the source definitions in attendanceStrings.js and commonCoreStrings.js. The tests now survive string updates without false failures.
2 participants
Summary
After clicking Go back in the Mark all N learners as present confirmation modal, the Mark all learners present switch required two clicks to re-open the modal instead of one.
Root cause:
KSwitchoptimistically toggles its internal checked state when clicked.handleMarkAllChange(true)was called but instead of updating the actual attendance state, a modal was shown. The:valuebinding (allPresent) stayedfalseboth before the click and after the cancel — so Vue saw no prop change, never re-rendered the switch, and it remained visually on. The next click then fired@change(false)(a no-op), and only the second click fired@change(true)to re-open the modal.Fix: Introduced a
pendingMarkAllref inuseAttendanceForm.jsthat is set totruewhen the modal opens and back tofalsewhen the modal is cancelled or confirmed. The switch now binds tomarkAllPresent = computed(() => allPresent.value || pendingMarkAll.value). When the modal is cancelled,pendingMarkAllflipsfalse, which changes the computed fromtrue → false— giving Vue a real reactive update to re-render theKSwitchback to its unchecked state.A regression test was added to
AttendancePages.spec.jsthat reproduces the exact sequence: open modal → click Go back → verify switch is unchecked → verify one click re-opens the modal.References
Fixes #14416
Reviewer guidance
Coach > Class home > class > Mark attendanceReproduce the original bug:
With this fix, one click after cancelling the modal is sufficient to re-open it.
AI usage
Implemented with Claude Code. The root cause analysis, fix, and regression test were AI-generated. I reviewed the generated code for correctness and unnecessary complexity, verified the approach against existing test patterns, and confirmed all 26 tests pass.