Add ability to side-panel preview course materials#14907
Conversation
github-actions
Bot
added
APP: Coach
Build Artifacts
Smoke test screenshot |
68cc693 to
012c1c4
Compare
marcellamaki
left a comment
marcellamaki
left a comment
There was a problem hiding this comment.
Added just a couple of notes
| // Side panel routes accessible from the course summary page | ||
| COURSE_SUMMARY_ASSIGN: 'COURSE_SUMMARY_ASSIGN', | ||
| COURSE_SUMMARY_ASSIGN_COURSE_DETAILS: 'COURSE_SUMMARY_ASSIGN_COURSE_DETAILS', | ||
| COURSE_SUMMARY_ASSIGN_COURSE_PREVIEW: 'COURSE_SUMMARY_ASSIGN_COURSE_PREVIEW', |
There was a problem hiding this comment.
This might be a bit redundant? but following the existing pattern for course routing as I suspect there was good reason for it :)
| if (previewContentId.value) { | ||
| router.push(overrideRoute(route, { query: { previewContentId: undefined } })); | ||
| } else if (currentTopicId.value !== route.params.courseId) { | ||
| // topic.value isn't reset when a new topic starts fetching, so while |
There was a problem hiding this comment.
This is an edge case I wouldn't have caught on my own (Claude did). "We" redrafted the comment a few times. Not sure if it's still helpful and clear enough -- happy to revise
🔵 Review postedLast updated: 2026-07-21 20:26 UTC |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #14907 adds a browsable course-resource preview side panel. Implementation is clean and matches the linked scope; the tree navigation (remount :key, _topicFetchId stale-guard, no-trust-of-topic.value-while-loading) is carefully reasoned. Findings below are all non-blocking.
CI passing. Manual QA was required for these UI changes but did not run in this pipeline, so the rendered UI has not been visually verified — not approving on UI grounds.
- suggestion — New logic-bearing components ship with no tests; the
goBack/route-derivation state machine is the part most likely to regress silently (inline). - suggestion —
CourseResourcePreview:label="contentNode.kind"is dead and would be an untranslated enum if rendered (inline). - suggestion — Shared
SidePanelModalheader tweak is unrelated to this feature and silently affects the search-filters back state (inline). - suggestion — Topic title (content text) lacks
dir="auto"for RTL (inline). - nitpick — Back during an in-flight topic fetch exits the whole preview (inline).
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran a phased review pipeline over the pull request diff:
- Classified the diff to select review passes (core, frontend, backend) and whether manual QA was required
- Core review pass checked correctness, design, architecture, testing, completeness, and DRY/SRP/Rule-of-Three principles
- Specialized frontend/backend review passes applied framework-specific lenses where those files changed
- For UI changes: manual QA and an accessibility audit against a live dev server, when available
- Checked CI status and linked issue acceptance criteria
- Synthesized one review from those passes and chose the verdict from the findings, CI status, and QA evidence
| ); | ||
| } | ||
|
|
||
| function goBack() { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: goBack and the getTopicRoute/getResourceRoute/exitPreview route derivation carry the branching logic most likely to regress silently, but no *.spec.js is touched in this branch. views/courses/__tests__/ already has the pattern (CourseSummaryPage.spec.js). A smoke test plus a focused test of goBack's three paths (resource→topic, topic→parent, root→exit) and the stale-loading fallback would be high value. AGENTS.md lists testing as required.
| // loading it may still hold the previous topic's (stale) data — | ||
| // ignore it and fall back to exiting preview rather than navigating | ||
| // to the wrong parent | ||
| const parentId = loading.value ? undefined : topic.value?.parent; |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: When loading is true, parentId is forced to undefined, so a back-click during an in-flight topic fetch drops the user out of preview entirely rather than up one level. The comment documents this as deliberate and it's a rare race, so take-it-or-leave-it — but disabling the back button while loading would be more forgiving than repurposing it into "exit".
| <KCircularLoader v-if="loading && !contentNode" /> | ||
| <div v-else-if="contentNode"> | ||
| <h2 class="resource-title"> | ||
| <KLabeledIcon :label="contentNode.kind"> |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: KLabeledIcon renders label only as the fallback of its default slot; since this usage supplies a default slot ({{ contentNode.title }}), :label is never rendered — it's dead. And contentNode.kind is a raw enum ('video', 'exercise'), an untranslated string were it ever shown. Drop the :label binding.
| v-if="topic" | ||
| class="topic-header" | ||
| > | ||
| <h2 class="topic-title"> |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: topic.title is channel/DB content, not an app string, so it isn't auto-flipped for RTL. Add dir="auto" so the browser picks direction from the text itself: <h2 class="topic-title" dir="auto">. (The resource title in CourseResourcePreview gets this for free via KLabeledIcon.)
| class="header-content" | ||
| :style="{ | ||
| flexDirection: closeButtonIconType === 'close' ? 'row' : 'row-reverse', | ||
| ...(closeButtonIconType === 'back' |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: This shared-component tweak is unrelated to the course-preview flow, which uses courses/sidePanel/SidePanelLayout, not this modal. The only consumer setting closeButtonIconType='back' is SearchFiltersPanel (category back state), so this silently alters that panel's layout. If intentional, worth splitting out and confirming the search-filters back state still renders correctly; if it's leftover from an earlier iteration, drop it.
|
Hi @marcellamaki, Noticed the following issues:
video.with.description.mp4
bookmarks.mp4Improvement suggestion:
whynobreadcrumb.mp4 |
|
Thank you @pcenov -- I'll address these! The video bit is particularly strange. Good find 😂 |
012c1c4 to
eceb429
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #14907 — delta re-review. 1 of 5 prior findings resolved (dir="auto" on the topic title); 4 still open (all suggestion/nitpick). No new findings. CI pending; manual QA was required but did not run, so the UI is not visually verified.
Still-open findings (see inline comments):
- CoursePreviewSidePanel.vue:226 — no
*.spec.jsforgoBack's three paths and the stale-loading fallback;views/courses/__tests__/already has the pattern. - CourseResourcePreview.vue:7 — dead
:labelbinding on a raw enum; drop it. - SidePanelModal/index.vue:33 — shared-component style change is unrelated to this flow; it only affects
SearchFiltersPanel's back state. - CoursePreviewSidePanel.vue:234 — nitpick: back during in-flight fetch exits preview rather than going up a level (documented as deliberate).
Prior-finding status
RESOLVED — CoursePreviewSidePanel.vue:26 — topic.title needs dir="auto" for RTL
UNADDRESSED — CoursePreviewSidePanel.vue:226 — no spec for goBack/route derivation
UNADDRESSED — CoursePreviewSidePanel.vue:234 — loading fallback drops user out of preview
UNADDRESSED — CourseResourcePreview.vue:7 — dead :label binding on raw enum
UNADDRESSED — SidePanelModal/index.vue:33 — unrelated shared-component change
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| ); | ||
| } | ||
|
|
||
| function goBack() { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: Still no *.spec.js in the branch. goBack and the getTopicRoute/getResourceRoute/exitPreview derivation carry the branching most likely to regress silently. A smoke test plus a focused test of goBack's three paths (resource→topic, topic→parent, root→exit) and the stale-loading fallback would be high value. views/courses/__tests__/ already has the pattern (CourseSummaryPage.spec.js); AGENTS.md lists testing as required.
| // loading it may still hold the previous topic's (stale) data — | ||
| // ignore it and fall back to exiting preview rather than navigating | ||
| // to the wrong parent | ||
| const parentId = loading.value ? undefined : topic.value?.parent; |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: When loading is true, parentId is forced to undefined, so a back-click during an in-flight topic fetch drops the user out of preview rather than up one level. Documented as deliberate and a rare race — take-it-or-leave-it. Disabling the back button while loading would be more forgiving.
| <KCircularLoader v-if="loading && !contentNode" /> | ||
| <div v-else-if="contentNode"> | ||
| <h2 class="resource-title"> | ||
| <KLabeledIcon :label="contentNode.kind"> |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: KLabeledIcon renders label only as the fallback of its default slot; this usage supplies a default slot, so :label never renders. And contentNode.kind is a raw enum (video, exercise) — an untranslated string were it ever shown. Drop the :label binding.
| class="header-content" | ||
| :style="{ | ||
| flexDirection: closeButtonIconType === 'close' ? 'row' : 'row-reverse', | ||
| ...(closeButtonIconType === 'back' |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: This shared-component tweak is unrelated to the course-preview flow, which renders via courses/sidePanel/SidePanelLayout and does not wrap SidePanelModal — so it has zero effect on this feature. The only consumer that sets closeButtonIconType='back' is SearchFiltersPanel, so this silently alters that panel's category back-state header (adds justify-content: flex-end; gap: 8px). If intentional, split it out and confirm search-filters still renders correctly; if leftover, drop it.
eceb429 to
d1eb69a
Compare
github-actions
Bot
added
DEV: renderers
Implement rtibblesbot cleanup fixes
d1eb69a to
d7ba770
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #14907 — delta re-review. All five prior findings resolved (dir="auto" persists, spec covers the three goBack paths + stale-loading fallback, dead :label replaced by KLabeledIcon+title, the unrelated SidePanelModal change is gone, loading fallback now hides the back button rather than exiting preview). One new edge-case suggestion below on the error path; not blocking.
CI pending. Manual QA was required for these UI changes but did not run this pass — the rendered UI is not visually verified, so this is not an approval on UI grounds.
- suggestion —
goBackHandlercan dereference a nulltopicon the fetch-error path (see inline).
Prior-finding status
RESOLVED — CoursePreviewSidePanel.vue:26 — topic.title needs dir="auto" for RTL
RESOLVED — CoursePreviewSidePanel.vue:226 — no spec for goBack/route derivation
RESOLVED — CoursePreviewSidePanel.vue:234 — loading fallback drops user out of preview
RESOLVED — CourseResourcePreview.vue:7 — dead :label binding on raw enum
RESOLVED — SidePanelModal/index.vue:33 — unrelated shared-component change
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| if (previewContentId.value) { | ||
| router.push(overrideRoute(route, { query: { previewContentId: undefined } })); | ||
| } else if (currentTopicId.value !== route.params.courseId) { | ||
| router.push(getTopicRoute(topic.value.parent)); |
There was a problem hiding this comment.
suggestion: goBack guards the loading window but not the error window. useFetch sets loading=false and leaves data/topic untouched when fetchMethod rejects (packages/kolibri-common/composables/useFetch.js), and error isn't surfaced here. So on direct navigation to a deep topic URL where fetchTree rejects (previewTopicId set, no prior successful fetch), topic.value stays null, loading is false, and the back button renders — clicking it hits getTopicRoute(topic.value.parent) → Cannot read properties of null (reading 'parent'). Guard this branch on topic.value (falling back to exitPreview()), and consider rendering error so the panel doesn't silently show a blank body. Low-probability path, not blocking.
|
Course details workflow:
Potential issue with direct navigation to a bookmarked page (preview of a single resource), "back" button apparently moves well up the tree with one difference (screenshot below):
Maybe a question for @jtamiace?
I couldn't replicate this with any Flexbox resources included in the 2 courses. Will keep an 👁️ Of the 2 issues reported by @pcenov, bookmarking resources during preview is now working correctly ✔️, but it is still not possible to use the back ⬅️ button if the video has a description (one must first press |
4 participants
Summary
After some slack discussion with Richard and Alex, this implements a "course content previewing" option based on the existing side panel lesson and quiz content previewing/selection experience. Within a course, specific subsections of content cannot be selected, thus no checkboxes -- it's just about seeing the content and being able to access everything a learner will see. Pre-post test assessments are not included.
References
Fixes #14149
Screen.Recording.2026-07-14.at.6.56.02.PM.mov
Reviewer guidance
Assign course workflow AND Course details workflow should be tested (although they share components, so it's unlikely that there would be significant UI differences but there might be some data bugs I suppose!)
one extra manual QA note: I had one instance on load where flexbook content didn't load and I didn't have my console open for an error 🤦♀️ . I haven't been able to reproduce it. Worth seeing if this pops up.
AI usage
Implementation done collaboratively with Claude (with moments for human in the loop feedback on the workflow from both me and Richard via slack discussions to do some decision making). Two separate rounds of human/self-review of the code. 1 upon initial completion, and now 1 ~2 weeks later when marking issue as ready for review.