Add unit page#14614
Conversation
github-actions
Bot
added
DEV: backend
Build Artifacts
Smoke test screenshot |
marcellamaki
force-pushed
the
add-unit-page
branch
2 times, most recently
from
293e15b to
a4736a7
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Solid implementation overall — the data layer is well-structured and the backend tests are thorough. CI all green.
The PR intentionally deviates from two acceptance criteria in #14471: (1) tabs instead of "No tabs", and (2) per-resource StatusSummary inside the accordion instead of per-lesson aggregate in the header. The tab deviation is explained in the PR description and matches the Figma; the header deviation is not mentioned. Please confirm both are intentional so reviewers can approve the spec change explicitly rather than silently.
Visual inspection from the video: the unit page renders correctly, tab switching works, accordion expand/collapse functions, SparklineBars display on the Learning Objectives tab, and the back link is correct.
Suggestions:
- Accordions open by default (see inline on
UnitDetailPage.vue:39) - StatusSummary in accordion header omitted (see inline on
UnitDetailPage.vue) - Silent tally corruption on unknown status (see inline on
useUnitDetail.js:57) - H5P/ZIM scope concern (see inline on
ContentIcon.vue:55) - New strings (
backToCourseLabel,learnerDistributionChartLabel) are onrelease-v0.19.xrather thandevelop— confirm these will be included in the translation pipeline for this release.
Nitpick: useUnitDetail.spec.js — the "no active test" fallback path in objectivesForLesson (composable lines 138–148) isn't covered by the unit tests. A test that sets rawLearningObjectives with data and bucketedObjectives empty would close that gap.
Praise:
useUnitDetail.js— excellent O(n)contentStatusIndexpreprocessingtest_unit_lesson_progress.py— comprehensive backend tests: permission levels, HelpNeeded superseded by completion, learner group scoping, empty unit edge case- Tab persistence via URL query params with
TAB_VALUESvalidation is clean and bookmark-safe objectivesForLessonfallback torawLearningObjectiveswhen no test data exists is thoughtful
Accordion header deviation (not inline due to diff position):
UnitDetailPage.vue <template #title> (lines 42–50 and 107–115): The acceptance criterion says "Each lesson accordion header shows StatusSummary with aggregate learner completion progress", but the header only shows title + icon. The per-resource StatusSummary is inside the accordion content. This provides more granular data but deviates from the stated spec — worth calling out for explicit sign-off.
@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
| <AccordionItem | ||
| v-for="lesson in lessons" | ||
| :key="lesson.id" | ||
| isOpenByDefault |
There was a problem hiding this comment.
suggestion: isOpenByDefault opens all lesson accordions on initial load. The acceptance criterion in #14471 says "all collapsed by default". If this is intentional (showing content immediately may be better UX), please note the deviation explicitly — otherwise remove isOpenByDefault to match the spec. The same applies to line 103 in the Learning Objectives tab.
There was a problem hiding this comment.
I think my only concern here would be "what happens when there are too many things?"
There was a problem hiding this comment.
That's a valid UX tradeoff — if there are many lessons, auto-expanding everything could be overwhelming. The right call here is a product decision between matching the spec (collapsed) vs. immediate visibility. Happy to defer to whatever you and the team decide.
| total: learnerIds.length, | ||
| }; | ||
| for (const learnerId of learnerIds) { | ||
| const status = byLearner[learnerId] || STATUS_NOT_STARTED; |
There was a problem hiding this comment.
suggestion: If status ever holds a value not in STATUS_KEY_MAP (e.g., a new status added to the backend without a frontend update), STATUS_KEY_MAP[status] is undefined and tally[undefined] += 1 silently corrupts the count. Consider a safe fallback:
tally[STATUS_KEY_MAP[status] ?? 'notStarted'] += 1;There was a problem hiding this comment.
This seems like a code error that we should be covering elsewhere.
There was a problem hiding this comment.
Fair point — if there's a backend/frontend version skew that produces an unknown status, that's better caught at the data layer or with an explicit assertion, not silently swallowed here. Withdrawing this suggestion.
|
|
||
| // Index status entries by content_id → learner_id so resourceTally is O(1) lookup | ||
| // rather than scanning all entries per resource per render. | ||
| const contentStatusIndex = computed(() => { |
There was a problem hiding this comment.
praise: Preprocessing contentStatusIndex as a computed O(n) index rather than scanning all status entries per resource per render is the right call — avoids a quadratic cost when a lesson has many resources and many learners.
| style="top: 0" | ||
| /> | ||
| <KIcon | ||
| v-if="is(ContentNodeKinds.H5P)" |
There was a problem hiding this comment.
suggestion: The H5P and ZIM icon additions (plus ContentNodeKinds.H5P/ZIM in packages/kolibri/constants.js) look correct, but they affect the entire application rather than just the unit detail page. Were these intentionally bundled here, or should they be a separate PR targeting develop so all plugins benefit and the change gets a focused review?
There was a problem hiding this comment.
This has been a long standing gap - as release will be merged up this, seems fine.
rtibbles
left a comment
rtibbles
left a comment
There was a problem hiding this comment.
I don't think there's anything blocking per se - but this seems to be ignoring some of the things that have already been set in place by previous work in terms of conventions.
| // These must match the PascalCase strings returned by class_summary_api.get_log_status | ||
| const STATUS_NOT_STARTED = 'NotStarted'; | ||
|
|
||
| const STATUS_KEY_MAP = { |
There was a problem hiding this comment.
Do we not have something like this and the above defined somewhere else already that could be imported?
| */ | ||
| export function computeResourceTally(contentId, contentStatusIndex, learnerIds) { | ||
| const byLearner = contentStatusIndex[contentId] || {}; | ||
| const tally = { |
There was a problem hiding this comment.
Is this not duplicative of logic elsewhere in coach? If not, could also just iterate over the status map as the source of truth in order to populate the object with the status keys.
| .catch(() => { | ||
| createSnackbar(defaultErrorMessage$()); | ||
| }) | ||
| .finally(() => { |
There was a problem hiding this comment.
Have we confirmed that the finally polyfill works on Safari iOS 9.3? https://caniuse.com/promise-finally
There was a problem hiding this comment.
both claude and google say that our corejs polyfill cover this. if you know otherwise, it looks like there are a few other places in the code where this lives, so i could fix here, but we should resolve elsewhere as well
There was a problem hiding this comment.
You're right — if corejs covers , then this is a non-issue. Withdrawing this finding.
There was a problem hiding this comment.
Hey, @rtibblesbot you don't need to withdraw my findings, thanks very much - you're not me!
There was a problem hiding this comment.
Apologies for the confusion — the original concern was yours, not mine. I shouldn't have said "Withdrawing this finding" since I have no standing to withdraw your comment. That was poorly worded on my part.
| }, | ||
| { | ||
| name: PageNames.UNIT_DETAIL, | ||
| path: CLASS + COURSE_SESSION + '/units/:unitContentnodeId', |
There was a problem hiding this comment.
I would love to see a COMPACT_UUID_PATTERN in here - validation ftw!
|
|
||
| const STUBS = { | ||
| CoachAppBarPage: { template: '<div><slot /></div>' }, | ||
| KRouterLink: { props: ['to', 'text'], template: '<a>{{ text }}</a>' }, |
There was a problem hiding this comment.
This is quite heavy stubbing here - is there a particular reason for it?
| }, | ||
| ]; | ||
|
|
||
| const DEFAULT_COMPOSABLE = { |
There was a problem hiding this comment.
Could we move this into a mocks folder alongside the composable, rather than defining it inline?
| { | ||
| name: PageNames.UNIT_DETAIL, | ||
| path: CLASS + COURSE_SESSION + '/units/:unitContentnodeId', | ||
| component: UnitDetailPage, |
There was a problem hiding this comment.
No child routes for the tabs to have their own URLs? We've established the pattern in this file already, let's continue it.
| }); | ||
|
|
||
| function setActiveTab(id) { | ||
| router.replace({ query: { ...route.query, tab: id } }); |
There was a problem hiding this comment.
I have a query about this deviation from the already established navigation pattern!
| context: | ||
| 'In Kolibri, an app is a certain kind of learning resource. Specifically, apps are generally self-contained, interactive HTML and Javascript applications.', | ||
| }, | ||
| h5p: { |
There was a problem hiding this comment.
Could we avoid adding new keys here and just map to the existing keys instead?
marcellamaki
force-pushed
the
add-unit-page
branch
from
a4736a7 to
fe8df18
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
This delta re-review covers new findings only. UI verified via video walkthrough — tab switching, accordion expand/collapse, sparkline rendering, back link, and mobile layout all work correctly.
Prior findings
All 4 prior findings (3 suggestions acknowledged, 1 praise) — no new status changes since last review.
CI: "All file linting" is failing. The failing files in the pre-commit output appear to be outside this PR's scope (e.g., LessonActive.vue, auth-select-page.spec.js). However the base branch passed linting on April 20 — please rebase onto the latest release-v0.19.x and rerun, or confirm this is a pre-existing issue unblocking the PR.
- blocking: CI
All file lintingcheck failing — see above. Failing files appear outside the diff; rebase and reinvestigate. - blocking: New i18n strings on non-default branch — see inline on
commonCoachStrings.js:90. - praise:
objectivesForLessonfallback — see inline onuseUnitDetail.js:125.
@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
| message: "Back to '{lesson}'", | ||
| context: 'Link that takes the coach back to the lesson view.', | ||
| }, | ||
| backToCourseLabel: { |
There was a problem hiding this comment.
blocking: backToCourseLabel (here) and learnerDistributionChartLabel (line 448) are new translatable strings added on release-v0.19.x rather than the default develop branch. New strings on release branches may not enter the translation pipeline before the release ships. Please confirm the project's translation workflow covers strings added directly to this branch — or, if the branch is always merged into develop before the release, note that explicitly so reviewers can confirm the strings will be processed.
| return `${unitNLabel$({ num: unitNumber.value })} ${unitTitle.value}`; | ||
| }); | ||
|
|
||
| function objectivesForLesson(lessonId) { |
There was a problem hiding this comment.
praise: The fallback to zero-count raw LOs when bucketedObjectives is empty is good defensive coding — coaches see LO names even before a pre/post test is activated, so the accordion isn't blank. The two-source selection is cleanly bounded by the single condition and well-documented.
marcellamaki
force-pushed
the
add-unit-page
branch
from
fe8df18 to
6b9e3d4
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
Two blocking findings from the prior review remain open; the new ContentIcon commit is clean.
CI: All file linting still failing (see inline on UnitDetailPage.spec.js:39). Other checks passing or pending (build assets still in progress). Visual inspection from the PR video — unit page, tab switching, sparklines, back link, and Learning Objectives tab all render correctly; the new ContentIcon H5P/ZIM changes are low-risk and not visible in the video.
Prior findings
6 prior findings unchanged (4 acknowledged, 2 praise).
Still unaddressed (re-raised below):
- CI
All file lintingfailing (blocking) — no reply, no code change; note: failing file shifted from unrelated files toUnitDetailPage.spec.jsin the PR - New i18n strings on
release-v0.19.x(blocking) — no reply, strings still present
0/8 prior findings resolved. 2 re-raised below.
- blocking: CI linting failure in
UnitDetailPage.spec.js— see inline comment - blocking: New translatable strings on non-default branch — see inline on
commonCoachStrings.js:90 - praise: ContentIcon H5P/ZIM mapping — see inline
@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
| }; | ||
|
|
||
| // Omit AccordionItem stub so the real component renders (collapsed by default). | ||
| const { AccordionItem: _unused, ...STUBS_REAL_ACCORDION } = STUBS; |
There was a problem hiding this comment.
blocking: CI All file linting is failing for this file (confirmed: run 24698373678). The base branch passes linting. The [pre-commit.ci lite] auto-fix only corrected Prettier indentation; the remaining ESLint error was not auto-fixable. The most likely cause is _unused here — ESLint's no-unused-vars (from eslint:recommended) does not automatically ignore _-prefixed variables unless ignoreRestSiblings: true or a varsIgnorePattern is configured, and neither appears in the project config. To confirm and fix: run pnpm run lint-frontend -- --files kolibri/plugins/coach/frontend/views/courses/__tests__/UnitDetailPage.spec.js. The pattern used elsewhere in the codebase for silencing this rule is an inline comment, e.g.:
// eslint-disable-next-line no-unused-vars
const { AccordionItem, ...STUBS_REAL_ACCORDION } = STUBS;| message: "Back to '{lesson}'", | ||
| context: 'Link that takes the coach back to the lesson view.', | ||
| }, | ||
| backToCourseLabel: { |
There was a problem hiding this comment.
blocking (re-raised): backToCourseLabel (here) and learnerDistributionChartLabel (line 448) are new translatable strings being added directly to release-v0.19.x rather than the default develop branch. New strings on release branches may not enter the translation pipeline before the release ships, leaving them untranslated for all non-English users. Please confirm one of the following:
- The translation workflow is configured to pick up strings from this branch before release, OR
- This branch is always merged into
developbefore the release ships (making the strings visible to the pipeline)
If neither applies, the strings should be added to develop instead.
There was a problem hiding this comment.
Ignore this please, this is intended.
There was a problem hiding this comment.
Understood — deferring to @rtibbles's confirmation that this is intentional. Withdrawing this finding. Apologies for the noise.
| /> | ||
| <KIcon | ||
| v-if="is(ContentNodeKinds.H5P)" | ||
| :icon="ContentNodeKinds.HTML5" |
There was a problem hiding this comment.
praise: Mapping H5P to the existing HTML5 icon token (and ZIM to DOCUMENT) rather than introducing new tokens is exactly right — it reuses the already-registered icon assets and avoids growing the icon registry. The kindToLabeLMap additions at lines 178–179 keep the tooltip reuse consistent.
marcellamaki
force-pushed
the
add-unit-page
branch
2 times, most recently
from
de78a00 to
89592d4
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
No new commits since my last review — this covers only the outstanding blocking finding.
CI: All file linting still failing (inline comment below). All other checks passing.
Prior findings
3 prior suggestions/praise acknowledged and unchanged.
Newly acknowledged (not re-raised):
- New strings on
release-v0.19.x(blocking) ← @rtibbles confirmed: "this is intentional"
Still unaddressed (re-raised below):
- CI
All file lintingfailing atUnitDetailPage.spec.js:39(blocking) — no reply, no code change
1/2 prior blocking findings acknowledged. 1 re-raised below.
@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
| }; | ||
|
|
||
| // Omit AccordionItem stub so the real component renders (collapsed by default). | ||
| const { AccordionItem: _, ...STUBS_REAL_ACCORDION } = STUBS; |
There was a problem hiding this comment.
blocking (re-raised): CI All file linting is still failing here (run 24741710321, error: 39:24 'no-unused-vars'). ESLint flags _ as assigned but never read. Add a disable comment:
// eslint-disable-next-line no-unused-vars
const { AccordionItem: _, ...STUBS_REAL_ACCORDION } = STUBS;Or restructure to avoid capturing the omitted key:
const STUBS_REAL_ACCORDION = Object.fromEntries(
Object.entries(STUBS).filter(([key]) => key !== 'AccordionItem')
);
marcellamaki
force-pushed
the
add-unit-page
branch
from
89592d4 to
c3a213a
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
All blocking findings from prior rounds are resolved; approving.
CI: All checks passing except Python unit tests on Mac OS (3.10), which is failing because /api/auth/portal/validate_token/ returns a Cloudflare 521 — the origin service is down in this runner environment. The failure is unrelated to this PR's changes (auth portal validation is untouched) and is absent from the base branch's failure list when run at a different time. Not blocking.
UI verified via video walkthrough in an earlier review round. Visual inspection covered tab switching, accordion expand/collapse, sparklines, back link, and mobile layout.
Note: @rtibbles has five unanswered inline comments on this PR (stubbing rationale, __mocks__ folder for the mock, the navigation-pattern query on UnitDetailPage.vue, and two that appear to have been addressed by the current code — COMPACT_UUID_PATTERN is used and child routes for both tabs exist). These are @rtibbles' own open threads; flagging them here in case they need a reply before merge.
Prior findings
All prior non-blocking findings unchanged (acknowledged, not re-raised).
Newly resolved:
- CI
All file lintingfailing atUnitDetailPage.spec.js:39(blocking) ← was UNADDRESSED —no-unused-varsis gone from the spec; linting passes in run 24744236891.
2/2 blocking findings now resolved. 0 re-raised.
@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
rtibbles
left a comment
rtibbles
left a comment
There was a problem hiding this comment.
A couple more questions about testing, but this seems ready for QA.
| @@ -0,0 +1,67 @@ | |||
| import { | |||
There was a problem hiding this comment.
Oh... why is this here, rather than in the tests folder in composables, adjacent to the composable itself?
|
|
||
| const STUBS = { | ||
| // AccordionItem: always renders slots so tests can assert on content without expanding. | ||
| AccordionItem: { template: '<div><slot name="title" /><slot name="content" /></div>' }, |
There was a problem hiding this comment.
Still quite a few stubs, are these really necessary?
|
|
||
| loading.value = true; | ||
|
|
||
| Promise.all([ |
There was a problem hiding this comment.
I guess the other alternative here is to try/catch with await - then finally just becomes something that happens after the try/catch block.
|
Hi @marcellamaki - could you please double check whether there is an issue with the build here as testing on kolibri_0.19.4.dev0+git.20260421201612-0ubuntu1_all.deb results in not seeing any data in 'Learning objectives', not seeing the Back link, constant loading etc: unit.page.mp4 |
|
hmmm, yes thank you @pcenov , I will look into that and confirm. sorry for the issues with this! |
marcellamaki
force-pushed
the
add-unit-page
branch
3 times, most recently
from
168fcc8 to
1e4ca7e
Compare
|
Hi @pcenov i wasn't able to reproduce the exact issue you saw on local dev or on the mac app, but I did see an error in the console (which didn't display to you) which could have caused the loading issues and resolved it so I'm hoping that fixes it 🤞 |
|
Hi @marcellamaki - not sure why you were not able to reproduce the previous issues... here's what I am observing in the current build which also seems to be not the same as what you are seeing locally but is better than the previous one:
no.objectives.mp4
expanded.mp4
learning.objectives.mp4
previously.completed.mp4 |
marcellamaki
force-pushed
the
add-unit-page
branch
from
1e4ca7e to
f3009f6
Compare
npm Package VersionsWarning The following packages have changed files but no version bump:
If these changes affect published code, consider bumping the version. |
|
@pcenov - very belated, this is ready for re-QA. You will need to update the courses with the latest version from the kolibri QA channel, but hopefully the updates on that end should have resolved the data/loading problems you were seeing (I have rechecked locally the same way I was originally able to reproduce the error and it looks good to me, but, I guess we will know for sure once you try!) I've also updated points 2 and 3. As for point 4, I am still thinking about this. I'm not sure what makes the most sense here, because this is somewhat of a new instance of a pre-existing problem (progress being tied to the resource, not the context). There may be some design/UX approaches for improving this, but I'm just not sure what they are yet. |
|
Thanks @marcellamaki - I confirm that points 1-3 are fixed now and I have filed separately the following issues #14733 and #14734 |
5 participants
Summary
Adds a Unit Detail page accessible via the UNITS tab on the course summary page.
2 tab interface (closer to figma spec, deviation from technical spec, because I realized upon implementation that we'd lost the learner resource progress with my suggestion, which seems really unhelpful for coaches)
References
Fixes #14471
Screen.Recording.2026-04-16.at.2.30.06.PM.mov
Reviewer guidance
and tabs
AI usage
written with claude with multiple rounds of claude and self-review and careful prompting