chore: merge 'main-hotfix' into 'main' - #2673
Merged
Merged
Conversation
The timezone Combobox handler treated the emitted payload as an option object and read `opt.value`. frappe-ui's Combobox declares `defineModel<string | null>()`, so `update:modelValue` emits the value string — `opt.value` is undefined. Selecting a timezone therefore cleared `liveClass.timezone`. Since the field is required, saving then failed validation with a 500. Read the emitted string directly.
The mobile nav was the desk sidebar squeezed into a fixed strip: every configured link became an icon-only button, nine of them across 390px, with the overflow in a floating box anchored to the bottom-right corner. Nothing said what any of them did. Replace it with a curated bar — four primary destinations plus More — where every tab carries its label, and put the overflow in a sectioned, searchable bottom sheet. A signed-out visitor gets a static five that covers everything they can reach, so no More button and no wait on the admin-configured links, which used to arrive late and sometimes not at all. The bar is a sibling in normal flow rather than `fixed`, so main ends where the bar begins. The `pb-10` it replaces never worked: Chromium drops a flex column's bottom padding from the scrollable area, so the last row stayed under the bar at the end of the scroll. Also h-screen to h-dvh across the layouts. 100vh is the URL-bar-retracted viewport, so on a phone it puts the bottom of the app below the visible area. This one cannot be proven in emulation — 100vh, 100dvh and innerHeight are all equal there — so it needs a real device. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every list page put its rows in a scroll box of their own, which held the title and filters still above them and left the page itself with no scroll range at all — measured at 390px, main#scrollContainer reported 0. A browser only retracts its URL bar when the root scroller moves, so that band of the viewport stayed unreachable however far you swiped, which is the "can't scroll to the end on mobile" report from the earlier round. The scroll box moves up to the page body, so the title and filters travel with the rows they belong to. Only the app header above and the footer below stay put, at every width. On a phone there is no box at all: the body grows and MobileLayout's scroll container does the work. The footer needs both `sticky bottom-0` and `mt-auto` on a phone. Sticky holds it once the rows overflow and pass under it; without mt-auto a short list leaves the column with slack nothing claims, and the strip stops wherever the last row ended — on Certified Members, 323px above the tab bar. It sits on bg-surface-elevation-1, the Espresso token for a surface content scrolls beneath. The filters are not pinned, deliberately. LayoutHeader is already `sticky top-0` in that same scroller, so a second one lands underneath it and loses its first 49px; the offset that would clear it is the app header's own content height, which is exactly the measured offset that drifted and opened a band last time. They get a rule underneath on a phone instead, where they run the full width and the rows begin straight under them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The strip spans the row on a phone but the pills inside it sized to their labels, leaving the grey track showing past the last tab — 327px of pills in a 350px track. `grow` shares the slack out so each pill takes what its label needs plus a share of the rest, rather than forcing equal columns, which truncates Certificates and Schedule at 390px. Desktop keeps sizing to content. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A textarea is inline-block by default, so the title sat on its parent's line box and carried that box's descender with it: 5px under the field belonging to no rule and no gap, which read as an uneven space before Instructor notes. Measured 29px below the title against 24px above it, where both are the same space-y-6. Making it a block leaves the 24px margin as the only thing between them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
get_courses and get_batches hardcoded a page length (30 and 20) while createListResource sends limit_page_length and then advances start by that same client-side number. Any page size other than the one the endpoint happened to use made the next page overlap the previous one, so Load More repeated rows — and the footer offers 24, 60 and 120. Read the client's value through a shared resolve_page_length() instead. It is bounded: both endpoints allow guests, so an unbounded page size would be a cheap way to ask for the whole table. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The page aborted its count request but not its list request, so a slow response for filters the user had already moved on from repainted the list seconds later. It also needs a page-local `reloading` flag: `list.loading` belongs to the resource rather than to a request, so the aborted fetch's tail cleared it for the reload that replaced it and the empty state flashed. Same contract Courses already had. The ListView stub in the ResponsiveListView tests was a single div, so the `overflow-y-hidden` wrapper frappe-ui really puts around its rows did not exist in the test DOM and no assertion about it could ever fail. It is now a copy of both of frappe-ui's wrappers, class lists included, with a test that the cards never scroll inside a box of their own — the page body owns the only scroll box. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three things were wrong with what the footer said. The page size was ignored. get_courses and get_batches hardcoded a length (30 and 20) while createListResource sends limit_page_length and then advances start by that same client-side number, so Load More overlapped the previous page. Read the client's value through a bounded resolve_page_length() instead — bounded because both endpoints allow guests and an unbounded size is a cheap way to ask for the whole table. The app sets require_type_annotated_api_methods, so the new argument is annotated: without it frappe rejects the call before the function runs and neither list loads at all. Featured courses were then added on top of that. They lead the list and come from a second query, and were prepended to an already-full page: asking for 24 returned 31. They are part of the same sequence the caller pages through, so they now come out of the same page budget, and the second page starts where the first stopped instead of repeating whatever the extras pushed past the end. That read stops at the end of the window too: nothing caps how many courses a site marks featured, and this is a list guests can ask for. And Courses and Batches had no total at all — just a loaded count — since their tabs filter on `enrolled`, `created` and `live`, which are not fields, and frappe.client.get_count cannot evaluate them. Both now have a count endpoint that resolves the same pseudo-filters the list does, and the pages ask for it alongside the list, cancelling the outstanding one first for the same reason the list does. Both count in the database: the batch tabs turn on the time of day, which the query settles only to the day, so the difference — today's batches that have already begun — is a second COUNT rather than a pass over every row the filters match, which would have made an anonymous request cost as much as the site has batches. Counting the batches turned up a real bug in the list itself: Upcoming and Archived are settled in Python against the wall clock, comparing `str(start_time)` with nowtime() — and str() of a timedelta drops the leading zero, so "9:00:00" sorted above "14:30:00" and a batch that began at nine that morning still counted as upcoming at half past two. Compared as times now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The four course tabs rendered into a bounded tab panel that scrolled inside itself: TabsRoot clips, TabsContent scrolls. On a phone that is a second scroll box nested in MobileLayout's #scrollContainer, so the page had no scroll range of its own and the URL bar could never retract — Settings had two nested scrollers. Release the panel on Overview and Settings, the two tabs that are ordinary documents. The editor keeps its bounded panel, which its `flex-1 min-h-0` grid needs to fill, and the dashboard is unchanged. The tablist also overflowed horizontally at 390px. Drop the per-tab icon and shorten "Course editor" to "Editor" on a phone, and tighten the tablist gap below 640px. The editor itself put a 30% outline aside beside a 70% editing surface; below `md` that left neither room to work. Hide the aside on a phone and open the same outline from a Chapters pill into a sheet, with a lesson stepper — prev, "2 / 5", next — above the editing surface. The stepper derives its bounds from the outline, which is already loaded, rather than from the LessonForm child, so the buttons do not flicker out on every hop while the child remounts. The remaining chrome earns its space at 390px: Student View keeps its 36x36 hit area but drops its label, the "how to edit" help goes, and publish moves into the ... menu beside it, where it keeps a written label — an icon-only globe never reads as "publish". The desk is untouched. Measured in Chrome at 390x844. Overview before: the nested panel scrolled 1315 and the page's range was 0; after: one scroller, #scrollContainer, 1297. Settings before: two scrollers (1683 + a 208 aside), page range 0; after: one, 1889. Tablist before: overflowed; after: fits. Prev/Next walk 1 -> 2 -> 1, the sheet opens, and picking a lesson dismisses it and moves the stepper to 2 / 5. At 1280 the scrollers, labels and aside are the same elements as before the change. Ported from feat/mobile-redesign (7ec2eff23, 81d895dd9, e3cbb9c5e); the files had drifted too far to cherry-pick, and the port drops that branch's preview-mode plumbing — student view is a route of its own since #2629, not a mode of this editor. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
LessonForm was `mx-10 px-20`, so at 390px its container was 310 wide and the content well 150 — the lesson title wrapped to four lines. It never overflowed, which is why sweeps passed it. Drop to `px-4` on a phone and keep the desk spacing from `sm` up; the well measures 358 and the title sits on one line. The inline "Include in preview" row costs a phone more than it earns, so it collapses behind a Lesson details chip and opens in a sheet. CourseForm's two panes each kept their own `overflow-y-auto`. Stacked below `md` that made the aside a second scroll box, so its settings did not continue the scroll the details started. Everything responsive here hangs off `md`, the width at which the columns actually split — a narrower cutoff would leave a band where the panes are stacked but still scroll independently. Ported from feat/mobile-redesign (f1c1e56cd, e3cbb9c5e). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The signed-out bottom bar was a hardcoded five. That list exists for a good reason — the admin-configured sidebar links arrive asynchronously, and matching against them left a guest looking at nothing but the More button until the settings call resolved, sometimes forever — but it also meant LMS Settings could not take a destination away. Turn Batches or Jobs off and a visitor still saw the tab, and could still open it. Static stays the starting set, not the final one: the tabs are filtered once `get_sidebar_settings` actually answers, by the same lowercased, underscored key that `filterLinksToShow` already uses to drop a link from the More sheet. Nothing is hidden while the call is unresolved, and a label the settings say nothing about — Log in — is left alone. That endpoint has two settled shapes, and they mean opposite things. The usual one is an object of flags. But to a guest, with guest access off, it returns a bare `[]`, and reading that as "no flags matched, keep everything" left every destination on the bar for a visitor who may not open a single one of them. An empty list is an answer, not a silence: it now leaves only Log in, which goes to Frappe's own /login rather than into the SPA. Greptile on #2630. Regression tests in mobileNav.test.ts cover the switched-off flag, both unresolved shapes, the two settled shapes and that they are not conflated, and the int-or-string the endpoint may carry; removing either guard fails them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fix: make the LMS usable on a phone: tab bar, one scroller per list, honest counts
ci: stop building frappe when packaging lms assets
The app had one header component and no shared body, so every page decided for itself what a title, a filter and an action button look like below `sm` — and mostly decided nothing, because the markup predates anyone opening the app on a phone. Four components, none of which a page has to think about: PageHeader is the whole header. A breadcrumb trail on a desk; on a phone a single back link, because a trail plus a badge does not fit 390px beside the actions. Both come out of the `breadcrumbs` array a page already passes — last crumb is the title, the one before it is where back goes — so adopting it costs a caller nothing, and the destination is a real route rather than `router.back()`, which cannot name where it is going and drops a deep-linked visitor out of the app. PageBody is what sits under it: a name, a filter strip, the content, a footer. The filter strip owns the gap and one width for everything in it, so a page drops in a search box, a dropdown and a toggle and gets a row that lines up. A control with an intrinsic width says so with `!w-fit` — a child selector outranks a class on the child, hence the bang. HeaderButton is labelled on a desk and a 36x36 icon on a phone. All three sit on one inset. The header, the footer and the skeletons that stand in for them were below while every page body is , so on a phone the chrome was indented 8px less than the content it frames: a page title started left of its own heading, and an action button ended right of the search box under it. One value now, at every width. TabbedDetailPage is the tabbed shell, taking its tabs as configuration. Bodies come from per-tab `#tab-body-<key>` slots rather than one generic slot: overriding a single tab through a shared slot would strip the default doc binding from all the others. ResponsiveListView gains `kind: 'actions'`, so a column of row actions draws unlabelled at the end of a phone card instead of as a field captioned "Actions". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three things about the bottom bar. It read the session once, at setup, off a plain destructure of the store, so logging in or out left it showing the other session's tabs until a full reload. It takes the ref now, and answers from the `user_id` cookie first so the bar is settled before the first paint — then corrects itself from the session resource, because the cookie is an ordinary client-side one and can be stale while `sid` is fine. The tabs shrank on press. On a bar you tap constantly a transient scale reads as a wobble; the active state is the affordance, so the animation goes. And the active state could never reach Profile. `isActive` matched `activeFor` against the leaf route name, but Profile is a parent that redirects to ProfileAbout, so the leaf is never 'Profile'. It matches against the whole `matched` chain now, which is what the desk sidebar means by activeFor anyway. Logging out also assigned to `isLoggedIn` after it became a const. It is a computed off the session user, so the store settles it on its own — and esbuild refuses to write a const, which meant the dev server would not start at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ListPage moves onto PageHeader and PageBody, which retires ListPageHeader and ListPageBody — they had split a page's own contents down the middle, one holding the title and filters and the other the scroll box and the footer, so a page had to nest them in the right order to get either. The filter strip loses its sub-slots with them. Search, tabs and toggles each carried their own width rule, which is how the same three controls came out three different sizes across these pages; there is one `filters` slot now and the strip sets the width. Four pages join that were never list pages in the code, only on screen: the submissions lists for assignments, quizzes and programming exercises, and the job applications list. Each had hand-rolled a title row, a filter grid, a Load More button and a count line — all of which the shell already does — and each gains a page-size control and the phone card layout it never had. Two of them had a single breadcrumb, which leaves the phone header with nothing to go back to, so they take their parent list as a crumb. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…hell BatchDetail died before painting. Its tab list was a `ref([])` filled by a watcher, and frappe-ui's Tabs reads `props.tabs[0].label` for its default value, so the first paint indexed an empty array. CourseDetail could not fail the same way only because its tab list is a literal — which is the argument for not patching the watcher but taking the tab list out of the pages entirely. Both move onto TabbedDetailPage. The dashboards and the tables under those tabs follow: number cards stack rather than shrink, and the student, progress and assessment tables become cards below `sm` instead of a row that scrolls sideways. Jobs takes the same header and the same buttons. LayoutHeader goes with them — it was a slot shell that only ever had one caller worth the indirection, and PageHeader is that caller's whole job. One bug surfaced: every course inside a batch linked to a course that does not exist. BatchCourses routed with `row.name`, a Batch Course child row's hash, where the route wants `row.course`. Declared, not hidden: ProgramForm's members list loses desk column-drag resize, and FeedbackModal's desk rows lose ~0.2rem of padding. ProgramForm's *courses* list is deliberately not converted — its rows come from vuedraggable inside ListRows, and converting would silently delete drag-to-reorder. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ten pages had pasted LayoutHeader's class list into an inline `<header>`
of their own, so the same markup existed eleven times and a fix to the
real one reached none of them. None had a phone back link, because the
header they copied predates it. A grep for `<header` or `<Breadcrumbs`
under src/pages comes back empty now.
Data Import is the awkward one. It renders frappe-ui's own DataImport,
which hard-codes that same header with no slot and no prop to turn it
off, and the package exports only the wrapper — so composing the steps
under our own header is not available without vendoring them. The page
suppresses frappe-ui's header instead and draws the app's. Its step
indicator is rendered twice, inside that header above `lg` and as a
`lg:hidden` copy below it, so hiding the header alone would leave a desk
with no steps: the in-page copy is shown at every width instead.
Statistics, Profile and Home are not filtered lists and keep their own
bodies, but take the shared header like everything else. NotFound and
PersonaForm stay as they are — both are full-screen by design.
The student lesson view gets the treatment the editor already had — at
390px it had 14 elements wider than the viewport, from an outline aside
stretching the grid; it has none now. The quiz is usable on a phone
taking it and authoring it, and the profile stops drawing a second header
under the first.
Two bugs surfaced. QuizSubmission read `.quiz` off a doc that had not
loaded, which only worked because a `v-if` on Breadcrumbs hid the whole
header until it did; the computed guards itself now. QuizForm ran a field
key through the translator as `key: __('question_detail')`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The shells above only reach as far as the page. These are the parts underneath that were still sized for a mouse: the quiz's own controls and its in-video prompt, the lesson sidebar, the feedback modal, the video preview field, and the home page's top row. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three files have now failed a full run with "Test timed out in 5000ms" and passed on their own in under a second. The default is wall-clock, and a full run mounts 60-odd suites at once, so the mount-heavy ones lose the race on a loaded machine. Raise it to 20s: it costs nothing when they pass, and a timeout is not an assertion. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two things review caught on BatchDetail, both real. The batch resource read `props.batchName` once, into `params` and into its `cache` key. Nothing re-reads either, and the router reuses this component when you go straight from one batch to another — the command palette does exactly that — so setup does not run again and the page kept showing the batch you arrived on. It takes `makeParams` and a watcher on the prop now. The cache key is gone rather than made reactive: read once, a reload would have filed the new batch's data under the old batch's entry. Nothing else reads that key. This predates the branch — develop has the same resource, unchanged — but the page is rewritten here, so it is fixed here. Second, publishing had moved entirely into the ... menu, which is what broke `batch_creation.cy.js`: the spec clicks a button reading "Publish" and there was no longer one. CourseDetail never made that move — it keeps a desk-width button and offers the menu entry only on a phone, where an icon-only globe would give no hint what it does. Batches now does the same, which is the arrangement the two pages were supposed to share. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The slot picker rendered "9:30 AM - 10:30 AM" with nothing saying which zone that was, while the batch card directly above it showed one. Labelling those times with the batch's timezone — the first attempt at this — states something false. Evaluator slots are system time by construction: validate_if_existing_requests and mark_eval_as_completed compare them against nowtime(), and create_event hands Google Calendar a naive datetime it reads as system time. Calling a 10:00 system slot "10:00 Europe/Berlin" contradicts the calendar invite generated from the same booking. Evaluator Schedule is also a child of Course Evaluator, shared across every batch that evaluator serves, so one Monday 10:00 row would carry two different zones at once. So convert instead of relabel. get_schedule resolves a display timezone (batch, else a paid-certificate course's own, else system) and returns both clocks per slot: the system values a booking submits, and display_* values for rendering. Conversion is display-only — submitting the converted clock would break validate_slot, past-slot rejection, completion marking and the calendar event at once. Grouping moves after conversion. An evaluator's Monday 09:00 in Asia/Kolkata is Sunday 20:30 in America/Los_Angeles, so a Monday schedule row legitimately produces Sunday slots, and one rendered day can hold slots stored on two different dates. The date a booking submits therefore comes from the slot, never from the day it was rendered under. Doing it server-side means the offset resolves at the slot's actual instant rather than at midnight, and there is no second JS implementation to drift out of step with the confirmation email. The availability editor now states the system timezone, since that is the clock an evaluator is authoring in. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The email restated a booking in the system clock while the picker it was made from rendered the batch's, so a learner who booked "6:30 AM Berlin" was told "10:00 AM". It converts the same instant into the same zone the picker used, and the template loses its own parentheses — the formatted zone already carries "(GMT+2:00)", so it read "(Europe/Berlin (GMT+2:00) time)". LMS Certificate Request.timezone goes back to the system zone and is now derived rather than accepted from the client: it records the zone the stored wall clock is in, and the stored clock is system time. Existing rows already hold that value, so nothing needs backfilling. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Batch, live class and evaluation timezones are free-text Data fields:
new records hold an IANA name picked from get_country_timezone_info,
older rows predate that control and can hold anything ("IST (GMT+5:30)").
Rendering the raw value read differently from one record to the next, and
a bare "Asia/Kolkata" leaves a learner to work out what that means for
them. formatTimezone renders "Asia/Kolkata (GMT+5:30)" and echoes
anything Intl does not recognise as a zone, mirroring
lms.lms.utils.format_timezone so an email reads like the screen.
No abbreviation lookup: no single locale yields abbreviations for every
zone (en-IN gives IST but degrades New York to GMT-4; en-US the
reverse), so only the offset is portable.
A batch spanning a DST transition has two offsets, so the card and
overlay label the one in force at the next class a learner could attend
rather than one pinned to the start date. Dates are anchored at local
midday, since `new Date('2026-09-06')` is UTC midnight — the previous day
in every westward zone, and enough to report the wrong side of a
transition.
Booked evaluations show the zone stored on the record. That is the system
zone their stored clock is in, and it is what the calendar invite says.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nt-view fix(mobile): one shared page header and tab shell, and a Batch page that stops crashing
Conversion can push a slot's end past midnight while its start stays on the day it is rendered under: 17:00-19:00 Asia/Kolkata is 23:30-01:30 in Pacific/Auckland — one system day, two display days. The end's converted date was computed and discarded, so the picker rendered "23:30 - 01:30" under a single date and said nothing about which day the slot finishes. Slots now carry display_end_date. The picker marks those buttons with a "+1" and spells the end date out in the accessible label, so the marker is not the only place the information exists. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…bels feat(evaluations): show slot times in the batch's timezone
A lesson whose video came from the `youtube` field or a `{{ YouTubeVideo }}`
macro rendered a bare <iframe class="youtube-video">, while the EditorJS embed
block rendered <div class="video-player" data-plyr-provider="youtube">. Watch
tracking in Lesson.vue only sees document.querySelectorAll('video') and live
Plyr instances, so the iframe was invisible to it: no timeupdate, no watch
duration, isVideoComplete never fired.
The consequence was worse than missing statistics. hasVideoListener was false
for those lessons, so shouldStartDwellTimer({hasVideo: false, enforceVideo: 1})
returned true and the dwell timer marked the lesson complete on a timer with
the video never played. Combined with the sequential-lesson gate, a student
could unlock a whole gated course by idling on each video lesson.
Both authoring paths now render the same Plyr-wrapped .video-player, so
enforce_video_completion actually gates them: hasVideoListener becomes true,
the dwell timer is cleared, and completion only comes from real playback via
isVideoComplete or the ended/statechange listeners. shouldAttachVideoFallback
is true too, so a broken embed re-enables dwell rather than locking the student
out.
Reuses extractYoutubeID from lessonMacros rather than adding a second URL
parser. That also fixes the old field path, which built .../embed/watch?v=ID
from a standard watch URL — a dead embed. A falsy id now renders nothing: an
empty-id Plyr would count as a video, suppress the dwell timer and leave the
lesson permanently uncompletable.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… ARIA
The locked row in both outlines named itself with aria-label on the element
returned by `:is="lesson.locked ? 'div' : ..."`. A div has an implicit
role="generic", which prohibits aria-label, so the name is not guaranteed to
be exposed; aria-disabled is likewise not a global attribute and does not
apply to a generic. Both attributes were introduced by this branch, in
ChapterRow.vue and StudentLessonSidebar.vue.
The lock glyph is now aria-hidden (it keeps its title for sighted hover) and
sits next to <span class="sr-only">{{ __('Locked') }}</span>, so the state is
real text content, exposed regardless of role and using no ARIA at all. That
also collapses the name from string concatenation into one translatable token.
role="listitem" was the other candidate and is wrong for ChapterRow: its rows
render inside vuedraggable and a headlessui DisclosurePanel, both plain divs,
with no list or role="list" ancestor, so it would trade this violation for
aria-required-parent. A <button disabled> would lie about interactivity on a
deliberately inert row, leave the tab order, and is an invalid parent for the
div the row contains when allowEdit is true.
Pattern follows Settings/Mobile/SettingsRow.vue and Helpdesk's
EmailNotifications/Notification.vue.
The tests located the row by [aria-disabled="true"]; they now key off the
locked row's own styling and assert the accessible name is text.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…second The countdown text sat in a role="status" element. role="status" implies aria-live="polite" and aria-atomic="true", so a string changing every second queued three complete announcements inside the three seconds before navigation — and polite announcements queue rather than interrupt, so the countdown crowded out the one sentence explaining why the lesson is locked. A screen-reader user got a fragment of the explanation, a burst of numbers, and then a silent route change. The visible counter is now aria-hidden (sighted users see it unchanged) and a single sr-only live region carries one complete sentence, set after a tick so the region registers a change rather than initial content. No control was added and nothing changed visually: the countdown UI stays as specified. The residual WCAG 2.2.1 question — that the navigation itself is automatic with no way to pause or cancel it — is unresolved and is a product decision, recorded in the handover. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rand it
Plyr's YouTube provider does not decorate the element it is given, it replaces
it: `player.media = replaceElement(container, player.media)` (plyr.mjs:5901,
i.e. `oldChild.parentNode.replaceChild(newChild, oldChild)`), and the
replacement carries no `video-player` class. So after init the node Vue created
and still tracks is detached from the document.
`Lesson.vue` reuses `LessonContent` across lessons with no `:key`, and the
`v-if` wrapper is truthy for any two consecutive `youtube`-field lessons, so
Vue patched the *detached* node while the previous lesson's Plyr subtree stayed
in the DOM. Two consequences, both worse than the bug 469741dfc fixed:
- lesson B showed lesson A's video;
- `enablePlyr()` scans `getElementsByClassName('video-player')` and found
nothing, so `plyrSources` was empty, `hasVideoListener` false, and
`shouldStartDwellTimer` returned true — the lesson auto-completed on the
dwell timer despite `enforce_video_completion`, which is exactly what
469741dfc set out to prevent. The old bare `<iframe :src>` never hit this,
because nothing detached it.
Keyed at both levels: `LessonContent` on the lesson name in `Lesson.vue` (the
structural fix — remounting discards the DOM Plyr injected behind Vue's back,
matching the `:key="lesson.data.name"` already used on the sibling component at
line 308), and the player wrapper on the embed id so the component is correct in
isolation. The instructor-notes mount is keyed for the same reason.
The regression was invisible to the existing tests because the Plyr mock only
recorded construction. It now mirrors `replaceElement`, and the new case walks
an A -> B lesson transition: it fails on the parent commit with lesson A's embed
id still in the document.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
49a07b97d set the announcement inside `nextTick()`, a microtask, so the `role="status"` region was inserted and populated within the same frame. Screen readers generally treat that as the region's *initial content* and announce nothing — leaving the silently-redirected student with no explanation, which was the whole point of that commit. The test could not tell the difference, because it only proved the DOM text changed after a tick. The region is now rendered unconditionally, outside the `v-if="redirect"` block, so it is in the accessibility tree from first paint, and the text lands on a 100ms timeout. The timeout is cleared alongside the interval. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…it is locked get_lesson now sends not_found alongside locked for a lesson number that resolves to nothing, because the gate answers that case with the same redirecting payload a real locked lesson gets. Without somewhere to put it the panel read "This lesson is locked / Finish the earlier lessons to unlock this one" under a "Lesson not found" breadcrumb - advice that cannot be followed. Kept out of the commits that build the panel: each of them rewrites this markup in turn, so the copy only survives here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The centred empty-state panel treated a withheld lesson as a whole empty page. It is a notice about one thing that did not happen, so it reads as one: the banner Helpdesk uses for the same job, copied from desk/src/components/erpnext-integration/ERPNextIntegrationSettings.vue:93-124 - tinted row, 28px icon well, two-line text column, trailing button - and sat at the top of the lesson column where a page notice belongs. The surface token is NOT the one that file names. Helpdesk's installed frappe-ui is still the v1 line, where bg-surface-amber-1 resolved to amber 100; this app runs the v2 tokens, where the same class is amber 50 - a paler colour under an unchanged name, which is how the accent ramp re-base in tailwind/migrate-tokens -v2.js lands on anything copied across the two lines. The v2 warning surface is bg-surface-amber-2, which is what frappe-ui's own Alert uses for theme="yellow", so the banner uses that with Alert's ink-amber-6 icon. The draining progress bar goes with it. A banner has no room for it and the seconds still read as a countdown next to the button that skips the wait, which the bar never offered. Announcement behaviour is unchanged: the reason is spoken once, the counter stays aria-hidden, and the button and counter both appear only when there is somewhere to send the student. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
can_access_quiz walked a flat set of (course, lesson) placements and ran the whole authorization chain per entry: can_modify_course, get_membership, then get_locked_lessons, which itself re-reads the setting, the membership, the ordered lesson rows and the progress rows. Every check but the last is course-level, so a quiz embedded in several lessons of one course repeated all of it for a lock set that cannot differ between them. Grouping the placements by course runs those checks once per course and keeps the per-lesson test where it belongs. Same answer: the loop already granted on the first placement that qualified, and course-level checks cannot disagree between two lessons of the same course. Raised in review of #2674. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…y called Two things kept this test green locally and red on CI. _create_batch links a Course Evaluator, which in turn links a User, and both helpers default to frappe@example.com — a record nothing in this suite seeds. Both are now built from self.author, created in setUp, so the test depends on no record outside its own fixtures. submit_quiz is whitelisted with `results: str | None`, so frappe coerces the argument through pydantic. The test passed a bare list, which is not a string; every other caller in the repo passes json.dumps(...). It now does too, which is also how the endpoint is reached over HTTP. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
feat: enforce lesson completion
…earch to one The index held courses, batches and jobs, so those were the only things the palette could ever find. Quizzes, assignments and programs are now indexed too; quizzes and programs carry no prose of their own, so the title doubles as the content field the index requires. No patch is needed to pick them up: after_migrate already rebuilds the index in the background. search_sqlite takes an optional category, which is what lets the palette narrow a search to one type. SQLiteSearch binds filter values but interpolates the column name, so the category is looked up in CATEGORY_DOCTYPES and the doctype constant is what reaches the query; an unknown or non-string category is rejected at the boundary. Courses, batches and jobs keep their existing visibility rules. LMS Program is filtered through frappe.get_list, which honours the permission_query_conditions hook it registers in hooks.py — an unpublished program is confirmed absent for a student, and that is pinned by a test rather than argued, because the palette leans on the hook instead of scoping programs by hand and nothing else here would notice if the registration went away. LMS Quiz and LMS Assignment cannot lean on the same thing. Both grant `read` to LMS Student and register no permission_query_conditions hook, so frappe.get_list filters nothing: a student got all 272 quizzes and all 40 assignments on the dev bench, and an assignment's indexed content is its full question text. They are scoped by hand — a moderator sees all, an instructor sees their own courses', nobody else sees any — and a test pins that frappe itself would allow the read, so the scope is not quietly dropped later as redundant. Search asks for title matches only. The index matches descriptions too, and searching "cour" returned Sequential Gating Demo, Digital Marketing Essentials and Financial Accounting Fundamentals — not unrelated, since every one of those blurbs contains the word "course", but a row renders its title and nothing else, so the matched word was invisible and the hit read as noise. On the dev site that takes "cour" from 53 matches across 10 titles to 6 across 4. Description-only matches are gone with it; that is the trade, and it is one line to put back. Course Instructor is no longer indexed. Its title is overwritten with its parent course's title, so under a title-only search it bought nothing except a second row per course that deduplicated away again. Searching a course by its instructor's name goes with it, which is the deliberate trade for a search that only matches what the row actually shows. `get_instructor_info` went too: two queries per result to attach `author_info`, which nothing in the frontend has ever read. Groups come back in the documented order rather than in whatever order the first row of each doctype happened to appear. Rebuilding the index afterwards found something worth knowing. Every doctype's distinct indexed count now equals its row count in the database; before, one course held 52 index rows and the file was 1.8 MB against 713 KB now. frappe only clears the table when it builds into a temporary database, which it does only when no index exists (sqlite_search.py:369). A rebuild over an existing index therefore re-inserts every document on top of the old rows, and both after_migrate and the scheduler rebuild in place — so the index duplicates itself on every migrate. That is a framework bug, not one this app can fix; `remove_duplicates` is what has been keeping the palette's own results honest, so it stays. Verified by rebuilding the index on test-lms.localhost: LMS Quiz (3) and LMS Assignment (2) appear. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Arrowing through results threw `Cannot set properties of undefined`. Every result was built with `isActive: false`, so `findIndex` returned -1 and the handler assigned to `allItems[-1]`. Enter was dead for the same reason: it looked for an active row that never existed, which left clicking as the only way to open a hit. Active state is now a single index held by the parent instead of a flag copied onto every row. Alongside that: - Route hits through a doctype map. Every doctype but LMS Course fell through to the batch route, so a job hit opened /batches/JOB-0001. A doctype with no mapping now drops its row rather than pointing at a page that cannot load it. - Keep the jump-to list up until the query is long enough to search. The results pane took over at one character while the search only ran from three, so the dialog went blank for exactly the two keystrokes that begin every search. - Bind the keys to the input rather than to window. The listener was added on mount and never removed, so every mount left one behind and a single keypress ran the handler once per palette ever mounted. - Push instead of replace, so the page you arrived from survives on the back stack, and drop a response whose query is no longer current. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tings
The jump-to list was three hardcoded links to list pages, so the only way
to reach anything else was to already know its name. It now lists the
categories the user may see; opening one narrows the search to that type
through the `category` argument, and the row inside it is the way out to
that category's own page. Backspace on an empty query and Esc step back
out, so Backspace stays an ordinary edit while there is still something
to delete.
Which categories are offered comes from the sidebar rather than from a
rule of the palette's own. An earlier `adminOnly` flag got Programs wrong
in both directions — it carried no flag at all, so the palette offered
Programs to a guest while the sidebar hid it outright
(`if (!userResource.data) return false`) and hid it from a student with
no enrolled or published programs. Two copies of a visibility rule was
the mistake: a category is offered when the sidebar is offering its page
to this user, so Quizzes and Assignments stay
instructor/moderator/evaluator only and Programs follows its own
condition, without either rule being written down twice. None of this is
access control — `get_grouped_results` withholds the records — but a row
that offers a page the sidebar is hiding is still telling the user
something untrue.
Typing a section's name offers that section's page above the hits, so
"cour" leads with Courses and lands on the course list on Enter, instead
of only ever offering individual courses. Matching compares the
translated label; comparing the untranslated one worked in English by
luck and nowhere else.
Settings joins as an account row for moderators, matching the gate
UserDropdown already applies. It is hidden unless the settings dialog is
actually mounted — the desktop sidebar owns it, and on a phone nothing
listens to isSettingsOpen, so the row would otherwise do nothing at all.
`routeForSearchHit` learned the three new doctypes at the same time.
`toGroups` drops any hit it cannot route, so until it did, every quiz,
assignment and program the server returned was thrown away in the
browser: opening Quizzes and typing a quiz's exact title showed "No
results found", and always would have.
Also fixed here:
- Scope survived a close, so the palette reopened silently filtered to
whatever category was last opened.
- Escape inside a category backed out *and* closed the dialog, because
reka-ui listens for it at the document level unless it is stopped.
- Clicking a category destroyed the focused button and left focus
nowhere, so the keyboard stopped working until the input was clicked.
- The stale-response guard compared against the current query, which
cannot tell an old request from a new one when both queries have since
been replaced. It is a per-request token now.
- A failed search rendered as "No results found", hiding an outage.
- Arrowing to the top row of a group scrolled the row into view but left
its heading clipped above the fold, so the list appeared to have no
heading at all. The first row of a group now scrolls the group.
- The esc chip is a word, not a glyph, and `size-5` fixed a square that
cropped it, so it gets a chip that grows with its text.
The suites' `__` stub now mirrors src/translation.js, which returns a
`{format}` object rather than a string for a message with placeholders;
an identity stub would have let a real `.format is not a function` pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
"No results found" was tied to `search.loading`, so each keystroke's request blanked it: the results area collapsed to nothing, the dialog resized around it, and everything came back when the response landed — once per letter, which is what made typing against an empty result set look like the palette was tearing itself apart. The message now follows whether a search has come back at all, not whether one is in flight. Once it says nothing matched it stays until results actually arrive, and it no longer appears before the first response, which used to call a search that had not happened yet empty. The new suite mocks createResource as a reactive object on purpose: the bug only exists when `loading` is reactive, so the other palette suites' plain-object stub cannot reproduce it, and per the test spec a file that stubs a module differently stays its own file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Programs.vue splits by role: a student gets the read-only ProgramDetail page, but a moderator or instructor gets a list whose cards open the ProgramForm modal. The palette sent everyone to ProgramDetail, so an author searching for a program landed on the page they cannot edit from. Route builders now take the visitor as context, gated on the same test Programs.vue gates its own card click on, read_only_mode included. ProgramForm and AssignmentForm are child routes rendered as a modal over their list, and both pages reach them through openFormRoute so that the form's close pops back to the list. The palette pushed them bare, which left no marker and degraded that close into a replace; it now stamps the entry the same way. QuizForm stays an ordinary push — it is a top-level route the quiz list reaches with a plain row link. The outage case in commandPaletteSearch swapped submit() for one that throws and never put it back, so every later search in the file inherited the failure; it is restored per test now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Statistics is a sidebar page with no records behind it, so it cannot be a searchable category — there is nothing to narrow a search to. Rows like it navigate instead of drilling in: Statistics, Certifications, Programming Exercises and Home, each still gated on the sidebar offering its page. The list is written out rather than derived from the sidebar, because a sidebar `to` is not always a route name — Contact Us carries a URL or a mailto address, and pushing either as a route name lands nowhere. Settings only ever appeared in the pre-search browse list, so typing its name put the palette into search mode, matched nothing, and reported "No results found" for a row that was sitting one keystroke behind. Matching now runs over every row the palette offers by name, Account included. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The palette imported its icons as components from lucide-vue-next while the rest of the app — including this component's own search glyph and keyboard chips — names them with the `lucide-*` classes the frappe-ui vite plugin generates from lucide-static. Two conventions for one thing, and the imported one is the fragile half. It shipped broken because of that. `House` is lucide's name for the home icon from 0.400 onward; this bench has lucide-vue-next 0.383, where it is still `Home`, so the browser's strict ESM refused the whole module and the palette died on load with `doesn't provide an export named: 'House'`. Nothing caught it: vitest's interop hands back `undefined` for a missing named export rather than throwing, so all 140 suites stayed green against a component that could not load at all. The class form has no such gap — a name is resolved by the plugin against lucide-static at build time, not by a version-pinned export list — and it drops eleven imports. `stroke-1.5` goes with them; the generated classes carry their own stroke, which is why nothing else in the app sets it. commandPaletteIcons.test.ts pins each name against lucide-static's own icon files, so a name that generates nothing fails the suite instead of rendering an empty box. Proved by pointing a row at `lucide-nonesuch` and watching it go red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The bar floats over the tab strip and masks the 30% of it that sits above the outline, so the two have to be exactly as tall as each other: the bar's own bottom border stands in for the strip's there, and the outline's start border runs down from it. They matched until the page shell gave the tab buttons `text-p-base` (line-height 1.5) while the bar's label kept `text-base-medium` (1.15). Same 14px, but 4.9px less box, so the bar's border landed short of the strip's and the start border broke between the two.
Course Instructor rows used to be indexed and then rewritten to look like their parent course, which left `id` naming the child row while `doctype` and `name` named the course. They are no longer indexed, but every learning.db built before that still holds them, frozen at whatever `published` said when they were written, and `remove_doc` deletes by `LMS Course:<name>` so nothing reaches them. Visibility is checked before `remove_duplicates` runs, so an unpublished course kept surfacing through its twin until the index was rebuilt. Drop any row whose `id` does not name the document its `doctype` and `name` claim. That is exact — the index writes `id` as `doctype:name` — and a no-op for every row written since. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`get_authored_names` scoped by `course`, which is optional on both LMS Quiz and LMS Assignment — the quiz form never asks for one — so an instructor could not find their own courseless quiz. A Batch Evaluator is listed in AUTHORING_ROLES but instructs no courses, so the scope withheld everything from them. Match on the owner as well, which is what the docstring already promised. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`title_only` makes frappe select the raw `content` column rather than a snippet of it, so every hit carried a whole course description or assignment question — up to 100 of them per keystroke, to a UI that draws a title and a relative date. Project each row down to what the palette renders. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`getSidebarLinks()` is only half of what the sidebar draws: AppSidebar filters its result again against `get_sidebar_settings`, the per-site on/off flags. The palette read the first half only, so a site with Jobs switched off was still offered a Jobs row that led to a page the sidebar was hiding. Filter by the flags too, through `isLinkEnabled` — the phone bar's reading of the same label-to-key convention AppSidebar uses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The handler was bound to the `<input>`, so tabbing to a result button — the only other thing in the dialog that takes focus — left the arrows and Enter doing nothing. Bind it to the panel above both. Enter reaching a focused result button is left to that button, or it would open whichever row is highlighted instead of the one the user tabbed to, and arrowing pulls focus back to the input so the caret and the highlight never disagree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`showsErrorState` also required the list to be empty, and a query that spells a section name — "cour", "sett" — fills one row from `matchingSections`. That was enough to swallow the message, so a failing search read as a search that simply found little. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Only a landing response was checked against the token; nothing bumped it when the search stopped being wanted. Narrowing to a category left the root request in flight to fill the category with rows from outside it, and unmounting left a debounced call to reach the server for a dialog that was gone. frappe-ui's `debounce` returns a bare function with no `.cancel()`, so a scheduled call cannot be cleared. Arm it with a token instead: a scope change, a close, a query too short to search, and unmounting each move `searchToken` past what the pending tick is holding, and the tick asks for nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Moving from one valid query to another leaves `isSearching` true, so the query watcher's clear branch never runs and the previous rows stay on screen for the debounce plus the replacement request. Enter takes the top row, and `activeIndex` has just reset to -1, so it opened a hit belonging to a query that was no longer in the box. Clearing the rows on every keystroke is not the fix — that is the blink "stop the empty state blinking on every keystroke" removed, where the results area collapsed and the dialog resized once per letter. The rows stay visible and stop being *selectable* instead: the response records which query it answered, and while that is not the query on screen the hits are marked stale, skipped by the active-row counter, left out of `flatItems`, and refused by `run` so a click cannot take them either. They render dimmed and `disabled`, because a row that silently ignores a click is worse than one that says it is not ready. Section rows are computed from the current query, so they stay live throughout. "No results found" now counts what is drawn rather than what can be selected. Keying it to the selectable rows reintroduced the same blink from the other side: with a stale set excluded, a query that had already found nothing showed the message, then blanked it for every in-flight keystroke after. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…align fix(courses): give the Chapters bar the tab strip's height
feat: improved command palette
chore: merge develop into main-hotfix
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
7 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.
Automated weekly release