Add shared ViewerToolbar and EmbeddedReadCard across content viewers - #15038
Add shared ViewerToolbar and EmbeddedReadCard across content viewers#15038rtibbles wants to merge 4 commits into
Conversation
github-actions
Bot
added
DEV: renderers
🔵 Review postedLast updated: 2026-07-30 21:41 UTC |
e1b1c76 to
f69fe2c
Compare
Build Artifacts
Smoke test screenshot |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15038 consolidates four bespoke viewer headers into a shared ViewerToolbar and adds EmbeddedReadCard — a clean, well-tested refactor that also improves RTL correctness. CI passing. Manual QA exercised all four toolbars (PDF/EPUB/HTML5/BloomPub) live with no PR-attributable console or a11y issues, but the EmbeddedReadCard active/mobile-embedded card state could not be rendered on this branch, so its interactive card path is unverified.
Heads up: the PR head advanced to f69fe2c after these passes ran; the three newest commits were not reviewed. Verdict is COMMENT, not approval.
Findings (see inline):
- important —
PdfRendererIndexnon-embedded initial fit readsthis.$el.clientWidthon a now-display:contentsroot → scale 0 → pages render at hardcoded 1.0 instead of fit-to-width. - suggestion — shared
BaseToolbarrestyle ripples intoLessonMasteryBar(untouched consumer). - suggestion —
EmbeddedReadCardcarries real click/keyboard logic but has no test. - suggestion — unrelated epubjs bootstrap changes (
global.ePubdrop,openAs) bundled into this PR. - nitpick — new components use Options API against the AGENTS.md
setup()convention; unusedcommonCoreStringsmixin inEpubRendererIndex.
@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
| }" | ||
| :style="{ backgroundColor: $themeTokens.text }" | ||
| @changeFullscreen="isInFullscreen = $event" | ||
| <EmbeddedReadCard |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
important: Making <EmbeddedReadCard> the template root changes what this.$el points at. In the normal non-embedded case the card renders in passthrough mode (.embedded-read-card-passthrough → display: contents), which generates no layout box, so this.$el.clientWidth is 0. The unchanged initial-fit line (this.scale = this.$el.clientWidth / (this.firstPageWidth * this.screenSizeMultiplier)) therefore computes scale = 0; PdfPage receives :scale="scale || 1", so every non-embedded PDF renders at a hardcoded 1.0 instead of fit-to-container, with no resize recompute to correct it. jsdom always reports clientWidth 0, so tests won't catch this, and QA didn't scale-check. Suggest reading the inner container that has a real box in both states, e.g. this.$refs.pdfContainer.$el.clientWidth (already used at the sidebar branch). The mobileEmbedded watcher below is fine — the card is a real box when active.
There was a problem hiding this comment.
Confirmed and fixed. Scale now reads the viewer element via a viewerWidth() helper.
Not $refs.pdfContainer as suggested: it sits inside the v-else gated on firstPageHeight === null, so at the initial-fit line the DOM has not re-rendered and the ref is still null. $refs.pdfViewer.$el is the element $el resolved to before the card wrapped the template.
Reproducing it took some care — three things mask it:
- the
showSideBarwatcher recomputes scale frompdfContainer, and fires whenever the value changes from itsfalseinitial mounted()restorespdf_scalefrom localStorage, so an earlier good run poisons the next measurement- reopening the same hash URL does not remount the component
It only bites where showSideBar computes false, so the watcher never fires: small viewport, embedded, or an empty outline array. At 700px, scale is 0 and the page collapses to a blank sliver. With the fix, scale is 1.017 and the page fits.
No test — jsdom reports clientWidth 0 either way, so broken and fixed are indistinguishable there.
| padding-right: 24px; | ||
| padding-left: 24px; | ||
| z-index: 1; | ||
| min-height: 48px; |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: BaseToolbar is shared; its only other consumer is LessonMasteryBar.vue, which this PR doesn't touch. Changing min-height (56→48px), padding (24→16px), and the box-shadow → %dropshadow-2dp reshapes the lesson mastery bar as a side effect. If intended for both, fine — otherwise scope the new dimensions to ViewerToolbar. Worth a QA glance at the lesson attempt view.
| }, | ||
| }, | ||
| methods: { | ||
| emitRead() { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: EmbeddedReadCard isn't purely presentational — emitRead guards on active, onSpace early-returns and preventDefault()s, and role/tabindex/aria-label toggle by state. ViewerToolbar ships a spec but this component has none. Add at least a smoke test plus the active/inactive emit branches and the space-key handling.
| * non-interactive, so the entire card acts as a single click target. | ||
| */ | ||
| export default { | ||
| name: 'EmbeddedReadCard', |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: AGENTS.md says new components must use setup() (Composition API); both new files (EmbeddedReadCard, ViewerToolbar) use Options API. Consistent with the surrounding legacy viewer code, so low-priority — flagging only against the stated convention since these will be the template other viewers copy.
| beforeMount() { | ||
| global.ePub = Epub; | ||
| this.book = new Epub(this.epubURL); | ||
| this.book = new Epub(this.epubURL, { openAs: 'epub' }); |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: Dropping global.ePub = Epub (+ its destroyed cleanup) and adding { openAs: 'epub' } are epubjs bootstrap behavior changes unrelated to the shared-toolbar scope. Removing the global looks safe (no other references), but openAs alters how books open. Consider splitting into its own commit and noting the rationale.
There was a problem hiding this comment.
Keeping both in this commit.
openAs is not a behavior change for our URLs. epubURL resolves to .../<hash>.epub, and epubjs determineType returns EPUB for a .epub extension (book.js:430), which is what open(url, openAs) would have derived anyway. It pins the type instead of re-sniffing the extension.
The global was write-only on our side, with no other references.
There was a problem hiding this comment.
That all holds up — pinning openAs: 'epub' yields the same type determineType would derive from the .epub extension, so it's a no-op for your URLs, and a write-only global is safe to drop. Rationale noted; no concern with keeping them together. Thanks for the detail.
| SearchButton, | ||
| LoadingError, | ||
| }, | ||
| mixins: [commonCoreStrings], |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: commonCoreStrings is mixed in but never used here — the "READ" label lives inside EmbeddedReadCard. Drop the import and mixin unless a use is coming.
| }); | ||
| }); | ||
|
|
||
| describe('custom components', () => { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: These custom components tests pin the new "components own their class" behavior precisely (first-render class preserved, safe-html not force-added, authored class forwarded via $attrs) — exactly the behavior-level coverage that guards against the earlier class-overwrite bug.
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15038 — delta re-review at f69fe2c7. No new commits and no thread replies since the last review, so every prior finding stands unchanged (code is identical). CI passing; manual QA found no blocking issues (the active EmbeddedReadCard and the SafeHTML .table-container change couldn't be exercised on this branch — author follow-ups).
7 prior findings; 0 resolved, 6 still open, 1 acknowledged (praise).
Still open:
- important —
PdfRendererIndex.vue: with<EmbeddedReadCard>now the template root, the non-embedded case renders thedisplay:contentspassthrough root, sothis.$el.clientWidthis 0. Thecreated()initial-fit (this.scale = this.$el.clientWidth / …) computes 0 and:scale="scale || 1"renders standard PDFs at native 1.0 instead of fit-to-width, with no resize watcher to recover. Measurethis.$refs.pdfContainer.$el.clientWidth(the box-bearing ref already used in theshowSideBarwatcher); themobileEmbeddedwatcher hits the same zero. - suggestion —
EmbeddedReadCard.vue: logic-bearing (active-guardedreademit, enter/space handling withpreventDefault, conditional role/tabindex/aria-label) but has no spec, unlike its siblingViewerToolbar. Add coverage for the emit branches and keyboard activation. - suggestion —
BaseToolbar.vue: the shared restyle (min-height 56→48, padding 24→16, dropshadow,z-index:1) also lands on the untouchedLessonMasteryBar. Confirm intended for both, or scope the new dimensions toViewerToolbar; worth a QA glance at the lesson attempt view. - suggestion —
EpubRendererIndex.vue: droppingglobal.ePub = Epub(+ itsdestroyedcleanup) and adding{ openAs: 'epub' }are epubjs bootstrap changes unrelated to the shared-toolbar scope; consider splitting into their own commit with rationale. - nitpick —
EpubRendererIndex.vue:commonCoreStringsmixed in but unused now that the READ label lives inEmbeddedReadCard. - nitpick — both new shared components (
ViewerToolbar,EmbeddedReadCard) use the Options API against AGENTS.md'ssetup()rule; consistent with surrounding legacy viewers, so low-priority, but these are the template others will copy.
Prior-finding status
UNADDRESSED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:3 — non-embedded initial fit reads $el.clientWidth on display:contents root → scale 0
UNADDRESSED — packages/kolibri-common/components/EmbeddedReadCard.vue:77 — real click/keyboard logic but no test
UNADDRESSED — packages/kolibri-common/components/BaseToolbar.vue:41 — restyle ripples into untouched LessonMasteryBar
UNADDRESSED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:472 — unrelated epubjs global.ePub/openAs changes bundled in
UNADDRESSED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:245 — unused commonCoreStrings mixin
UNADDRESSED — packages/kolibri-common/components/EmbeddedReadCard.vue:51 — new components use Options API vs setup() convention
ACKNOWLEDGED — packages/kolibri-common/components/SafeHTML/tests/SafeHTML.spec.js:191 — praise (no action)
@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 via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Ran the same phased review passes as a first review (core, frontend/backend lenses, manual QA when required)
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
f69fe2c to
908bea4
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15038 — delta re-review at 908bea40.
All 7 prior findings resolved/acknowledged; 3 new findings on newly changed code (1 important, 2 suggestions). CI passing. Manual QA was required (UI changes) but did not run in this pass — the embedded/mobile card state is not reproducible on this branch, so the two SafeHTML visual changes and the EmbeddedReadCard active path are unverified visually.
- important —
EpubRendererIndex.vue: thesideBarOpenwatcher can dereference an unmounted$refs.topBar. See inline. - suggestion —
SafeHTML/style.scss: the.table-containerfull-bleed removal is global to all SafeHTML tables. See inline. - suggestion —
SafeHTML/index.js: the newembedded-content-viewerwrapper branch has no spec. See inline.
Prior-finding status
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue — non-embedded initial fit read $el.clientWidth on display:contents root (now viewerWidth() via $refs.pdfViewer.$el)
RESOLVED — packages/kolibri-common/components/BaseToolbar.vue — shared-dimension side effect on LessonMasteryBar (dimensions now scoped to ViewerToolbar)
RESOLVED — packages/kolibri-common/components/EmbeddedReadCard.vue — no tests (spec added, active/inactive + space/enter)
RESOLVED — packages/kolibri-common/components/EmbeddedReadCard.vue:53 — Options API vs AGENTS.md (now setup())
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue — commonCoreStrings imported-but-unused (removed)
ACKNOWLEDGED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:468 — openAs:'epub' + dropped global.ePub (author rationale accepted; no-op for .epub URLs)
ACKNOWLEDGED — packages/kolibri-common/components/SafeHTML/tests/SafeHTML.spec.js:191 — praise: components-own-their-class tests (still present)
@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
| <LoadingScreen v-else-if="!loaded" /> | ||
|
|
||
| <TopBar | ||
| v-if="!mobileEmbedded" |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
important: Gating TopBar with v-if="!mobileEmbedded" means $refs.topBar can be undefined when the sideBarOpen watcher runs. Path I can see statically: embedded + small viewport, tap the card → fullscreen (isInFullscreen true → mobileEmbedded false → TopBar mounts), open the TOC sidebar, press Esc — Esc both exits fullscreen (isInFullscreen false → mobileEmbedded true → TopBar unmounts) and fires @keyup.esc="closeSideBar". In the watcher's $nextTick, this.$refs.topBar.focusOnTocButton() (line 441) then throws a TypeError. The if (this.$refs.tocButton) guards inside TopBar.focusOn* don't cover this — the throw is on the absent topBar ref one level up. Can a sidebar be open (or transition closed) while mobileEmbedded is true? If so, guard the caller (this.$refs.topBar && this.$refs.topBar.focusOn…) or close open sidebars when leaving fullscreen.
| width: calc(100% + 32px); | ||
| padding: 0 16px; | ||
| margin-left: -16px; | ||
| width: 100%; |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: .table-container is the wrapper for every SafeHtmlTable, so dropping the calc(100% + 32px) / negative-margin full-bleed changes table layout for all SafeHTML consumers (QTI etc.), not just the embedded card this PR targets. The old trick let wide tables scroll edge-to-edge past the container padding. If the change is needed so embedded tables don't overflow the card, reasonable — please confirm the full-bleed removal is intended everywhere rather than a side effect of the embedded work.
| mapChildren(node.childNodes), | ||
| ); | ||
| // Wrap embedded ContentViewers in a layout container | ||
| if (component === 'ContentViewer') { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: This new embedded-content-viewer wrapper is a new structural branch in the render path with no accompanying spec, unlike the sibling custom-component and object-tag cases that are tested. A small render assertion (the ContentViewer element ends up inside .embedded-content-viewer) would lock the behavior.
| } | ||
| }, | ||
| // The component root is EmbeddedReadCard, which is `display: contents` | ||
| // when inactive and so has no box to measure; the viewer element always |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: The display: contents measurement pitfall is documented exactly where a future reader would trip on it — this comment captures the subtle failure mode behind the earlier scale-0 bug and should prevent a regression.
908bea4 to
06653b2
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15038 — Delta re-review at 06653b2531. All prior findings resolved; no new findings. CI passing.
Verdict is COMMENT rather than APPROVE only because manual QA was required for this UI change but did not run in this pass — the UI was not visually verified here.
Prior-finding status
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:438 — sideBarOpen watcher could deref unmounted $refs.topBar
RESOLVED — packages/kolibri-common/components/SafeHTML/style.scss — global .table-container full-bleed removal (reverted; scoped to embedded-content-viewer)
RESOLVED — packages/kolibri-common/components/SafeHTML/index.js:166 — embedded-content-viewer wrapper had no spec (spec added)
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue — non-embedded initial fit read $el.clientWidth on display:contents root
RESOLVED — packages/kolibri-common/components/BaseToolbar.vue — shared-dimension side effect on LessonMasteryBar
RESOLVED — packages/kolibri-common/components/EmbeddedReadCard.vue — no tests / Options API vs setup()
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue — commonCoreStrings imported-but-unused
ACKNOWLEDGED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:468 — openAs:'epub' + dropped global.ePub (author rationale accepted)
ACKNOWLEDGED — packages/kolibri-common/components/SafeHTML/tests/SafeHTML.spec.js:191 — praise (no action)
ACKNOWLEDGED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:496 — praise on display:contents documentation (no action)
@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
06653b2 to
e1ea8c7
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15038 — delta re-review. All prior findings resolved or acknowledged; no new findings on the newly changed code. CI passing.
Commenting rather than approving because manual QA was required for this PR but did not run this pass — the active/mobile EmbeddedReadCard interactive state remains author-screenshot-verified only, not independently confirmed here.
Prior-finding status
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:3 — EmbeddedReadCard as template root changed this.$el / initial fit read clientWidth on display:contents root
RESOLVED — packages/kolibri-common/components/BaseToolbar.vue — restyle scoped to ViewerToolbar, does not ripple into LessonMasteryBar
RESOLVED — packages/kolibri-common/components/EmbeddedReadCard.vue — real click/keyboard logic now covered by spec
RESOLVED — packages/kolibri-common/components/EmbeddedReadCard.vue:53 — now uses setup() per AGENTS.md convention
RESOLVED — packages/kolibri-common/components/ViewerToolbar.vue — now uses setup() per convention
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue — commonCoreStrings imported-but-unused removed
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:22 — sideBarOpen watcher $refs.topBar deref guarded when TopBar unmounts
RESOLVED — packages/kolibri-common/components/SafeHTML/style.scss — full-bleed removal scoped to embedded-content-viewer
RESOLVED — packages/kolibri-common/components/SafeHTML/index.js:166 — embedded-content-viewer wrapper now covered by spec
ACKNOWLEDGED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:470 — openAs:'epub' + dropped global.ePub (author rationale accepted)
ACKNOWLEDGED — packages/kolibri-common/components/SafeHTML/tests/SafeHTML.spec.js:191 — components-own-their-class tests (praise, no action)
ACKNOWLEDGED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:496 — display:contents documentation (praise, no action)
@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
marcellamaki
left a comment
marcellamaki
left a comment
There was a problem hiding this comment.
So, just to be sure I'm following. The EmbeddedReadCard is a wrapper that turns the read-kind file preview into a button. clicking on it, at any place, opens it into a full screen? and from there, the learner can exit full screen, and they are essentially returned to where they were in the broader article context. so, it's.... almost but not precisely like a lightbox?
marcellamaki
left a comment
marcellamaki
left a comment
There was a problem hiding this comment.
Richard and I discussed a bit more in person and looked at the figma spec together, and confirmed that my understanding of the expected behavior here aligned with both what i understood from the code and saw in the spec. therefore, 
e1ea8c7 to
a7ca005
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15038 — delta re-review of a7ca0059.
All 12 prior findings resolved or acknowledged. CI green (55 pass / 17 skipped). Five new findings below, none blocking — an a11y gap on the active card, a layout artifact in the mobile-embedded EPUB state, and three nitpicks. Note that manual QA did not run for this review, so nothing here is visually verified; @marcellamaki's approval on this SHA stands.
Prior-finding status
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:3 — $el on a display:contents root / initial fit measurement
RESOLVED — packages/kolibri-common/components/BaseToolbar.vue — shared restyle rippling into LessonMasteryBar
RESOLVED — packages/kolibri-common/components/EmbeddedReadCard.vue — click/keyboard logic had no spec
RESOLVED — packages/kolibri-common/components/EmbeddedReadCard.vue:53 — Options API vs setup() convention
RESOLVED — packages/kolibri-common/components/ViewerToolbar.vue — Options API vs setup() convention
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue — commonCoreStrings imported but unused
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:22 — sideBarOpen watcher could deref an unmounted $refs.topBar
RESOLVED — packages/kolibri-common/components/SafeHTML/style.scss — global .table-container full-bleed removal
RESOLVED — packages/kolibri-common/components/SafeHTML/index.js:166 — embedded-content-viewer wrapper had no spec
ACKNOWLEDGED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:470 — openAs: 'epub' + dropped global.ePub
ACKNOWLEDGED — packages/kolibri-common/components/SafeHTML/tests/SafeHTML.spec.js:191 — praise
ACKNOWLEDGED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:496 — praise on the display:contents comment
@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
| @keydown.enter="emitRead" | ||
| @keydown.space="onSpace" | ||
| > | ||
| <div :class="active ? 'embedded-read-card-content' : 'embedded-read-card-content-passthrough'"> |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: The active card is a single pointer target, but its content stays in the tab order. pointer-events: none (line 99) covers mouse and touch; it does not remove descendants from the tab order or the a11y tree.
For the EPUB card this is reachable: TopBar is v-if'd off and the nav/bottom bars are v-show'd off, but the epubjs rendition is an <iframe> inside .epubjs-parent, which is implicitly focusable. A keyboard user tabbing past the card lands inside book content that is presented to AT as an opaque role="button" labelled "READ", and any Enter/Space pressed there bubbles back to the card's @keydown handlers and fires read.
inert on this wrapper when active covers pointer, keyboard and AT in one attribute, and lets you drop pointer-events: none:
<div
:class="active ? 'embedded-read-card-content' : 'embedded-read-card-content-passthrough'"
:inert="active || null"
>| .epub-viewer-content { | ||
| position: relative; | ||
| height: 100%; | ||
| height: calc(100% - #{$top-bar-height}); |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: Content height still subtracts the toolbar (and bottom bar) when neither is rendered. In the mobileEmbedded state TopBar is v-if="!mobileEmbedded" (line 22) and BottomBar is v-show="!mobileEmbedded", but $top-bar-height is still subtracted here and .navigation-and-epubjs still reserves bottom: $bottom-bar-height — roughly 100px of the 25vh card is dead space. This is the blank band under the "Introduction" box in the PR's own mobile-EPUB screenshot.
Relatedly, .top-bar-component { flex-shrink: 0 } (line 869) is a no-op: .epub-viewer is position: relative with no display: flex. Making .epub-viewer a column flex container and giving this rule flex: 1; min-height: 0 would let the layout self-adjust when the chrome is absent, and drop the hardcoded coupling to ViewerToolbar's min-height.
| }, | ||
| }, | ||
| watch: { | ||
| mobileEmbedded() { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: This measures width synchronously; the sibling showSideBar watcher (line ~312) defers with $nextTick precisely because the grid has to re-render before clientWidth is meaningful.
On the exit-fullscreen transition mobileEmbedded flips to true and this runs before Vue re-renders EmbeddedReadCard into its active state — at which point the card adds padding: 16px, so viewerWidth() returns ~32px more than the box the pages actually land in and scale comes out slightly too large. Wrapping the body in this.$nextTick(...) matches the sibling watcher and measures post-layout.
| bottom: 0, | ||
| }; | ||
| } | ||
| if (this.embedded) { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: Neither html5_viewer nor bloompub_viewer declares css_selectors or allow_object_tag on its ContentRendererHook (only epub and pdf do), so neither can be selected by kolibri.canHandleElement() and embedded is always false here — this branch is currently dead. It also makes it easy to miss that BloomPubRendererIndex.containerStyle did not get the same branch despite the two files being otherwise near-identical. If the embedded path is intended for these viewers, the hooks need the selector declarations and bloompub needs the matching branch; if not, the embedded plumbing could be left out of both until then.
|
|
||
| // Dimensions and elevation are scoped here rather than on the shared | ||
| // BaseToolbar, whose other consumer is sized for its own layout. | ||
| .viewer-toolbar { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: .viewer-toolbar overrides .base-toolbar at equal specificity — .base-toolbar sets min-height: 3.5rem and padding-left/right: 24px, both compile to one class plus a scope attribute, so the winner is stylesheet source order. It resolves in ViewerToolbar's favour today only because BaseToolbar is injected first as a dependency, which isn't something this component controls. .base-toolbar.viewer-toolbar { … } makes it deterministic for the same cost.
Also: ref="fullscreenButton" (line 17) is read by nothing here or in any consumer (it was already unused on the old epub TopBar it came from) — worth dropping unless a focusOnFullscreenButton follow-up is planned.
a7ca005 to
a257ebd
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15038 — delta re-review of a257ebd835.
16 of 16 prior findings resolved or acknowledged; none still open. Two new suggestions and three nitpicks below, none blocking. CI passing; manual QA did not run in this pass, so nothing here is visually verified.
- suggestion — the SafeHTML table-style "move" changes rendered output for every consumer (inline on
SafeHtmlTable.vue) - suggestion —
showSideBarwatcher lost its ref guard on the way toviewerWidth()(inline onPdfRendererIndex.vue) - nitpick ×3 — hardcoded focus-outline colour;
inertandscrollbar-gutterboth sit above the browserslist floor (inline)
Prior-finding status
RESOLVED — packages/kolibri-common/components/EmbeddedReadCard.vue:16 — active card content stays in tab order / a11y tree
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:875 — content height subtracts chrome that isn't rendered; flex-shrink no-op
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:282 — mobileEmbedded watcher measured width synchronously
RESOLVED — kolibri/plugins/html5_viewer/frontend/views/Html5AppRendererIndex.vue:122 — dead embedded branch
RESOLVED — packages/kolibri-common/components/ViewerToolbar.vue:77 — equal-specificity override vs .base-toolbar; unused ref="fullscreenButton"
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:3 — EmbeddedReadCard as template root changes this.$el
RESOLVED — packages/kolibri-common/components/BaseToolbar.vue — shared with LessonMasteryBar.vue
RESOLVED — packages/kolibri-common/components/EmbeddedReadCard.vue — not purely presentational (emitRead/onSpace guards)
RESOLVED — packages/kolibri-common/components/EmbeddedReadCard.vue:57 — new components must use setup()
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:470 — dropped global.ePub + { openAs: 'epub' }
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue — unused commonCoreStrings mixin
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:22 — $refs.topBar undefined when TopBar is v-if'd off
RESOLVED — packages/kolibri-common/components/SafeHTML/style.scss — .table-container full-bleed rules dropped
RESOLVED — packages/kolibri-common/components/SafeHTML/index.js:166 — embedded-content-viewer wrapper had no coverage
ACKNOWLEDGED — packages/kolibri-common/components/SafeHTML/tests/SafeHTML.spec.js:191 — praise, custom-component class tests
ACKNOWLEDGED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue — praise, display: contents measurement pitfall documented
@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
| <style scoped> | ||
|
|
||
| /* Negative margins cancel the extra width, so wide tables scroll full-bleed. */ | ||
| .table-container { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: The commit is framed as relocating styles, but this changes what renders for every SafeHTML table.
On develop the two rules competed: style.scss (global, unscoped) set width: calc(100% + 32px); padding: 0 16px; margin-left: -16px at specificity (0,1,0), while the scoped rule here compiled to .table-container[data-v-…] at (0,2,0) and won with just margin: 1em 0. So margin-left: -16px never applied — the container was 32px wider than its parent with no negative offset, overflowing to the trailing edge. Merging both into margin: 1em -16px makes the full-bleed offset live for the first time, for QTI assessment items and safe_html5 alike, not just the embedded card this PR targets.
Almost certainly the original intent, but worth saying so in the commit message and eyeballing a wide QTI table — nothing in SafeHtmlTable.spec.js can catch it (jsdom applies neither scoped nor imported stylesheets). The symmetric margin is also an RTL improvement: no RTLCSS flip needed where margin-left did.
| @@ -281,11 +320,7 @@ | |||
| }, | |||
| showSideBar() { | |||
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: The previous revision guarded on this.$refs.pdfContainer && this.$refs.pdfContainer.$el before measuring. viewerWidth() (line ~502) has no equivalent bail-out — when neither $refs.recycleList nor $refs.pdfViewer resolves it returns 0, and this assigns scale = 0. :scale="scale || 1" then silently renders pages at native 1.0 — the failure mode this branch already fixed once, and equally invisible in jsdom. A null firstPageWidth yields NaN, which propagates into itemHeight and the scroller sizing.
The two sibling call sites both wrap the assignment in if (this.firstPageWidth); this one doesn't. I couldn't construct a reachable path (showSideBar only changes from the created() chain and toggleSideBar(), and $refs.pdfViewer exists in both), so this is defensiveness rather than a demonstrated bug — but the guard was cheap and the failure is silent.
| } | ||
|
|
||
| .table-container:focus-visible { | ||
| outline: 3px solid rgb(51, 172, 245) !important; |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: rgb(51, 172, 245) is a verbatim carry-over from style.scss, but it's now the only raw colour literal left in the repo's own SCSS/Vue, and AGENTS.md bans them. The token is $themeTokens.focusOutline (see kolibri/plugins/media_player/frontend/views/VideoPlayer.vue:23, bound via a CSS custom property). This component already reads this.$themePalette/this.$themeBrand in tableStyle, so binding it is a one-liner while the line is being touched.
The rule isn't dead despite .table-container having no tabindex — Chrome makes overflow: auto regions keyboard-focusable.
| > | ||
| <div | ||
| :class="active ? 'embedded-read-card-content' : 'embedded-read-card-content-passthrough'" | ||
| :inert="active" |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: inert is Chrome 102+ / Safari 15.5+ / Firefox 112+; packages/browserslist-config-kolibri/index.js targets Chrome >= 49, Safari >= 11.1, iOS >= 10, Samsung >= 5. Below those, only the pointer-events: none fallback applies — that covers pointer but leaves the wrapped viewer in the tab order and the a11y tree, exactly the gap the attribute closes. The comment on the fallback is honest about this; flagging only so the residual scope is explicit (a wicg-inert polyfill would be scope creep).
The Vue 2 side is correct: inert is in isBooleanAttr, so :inert="active" adds/removes the attribute rather than rendering inert="false", and the new specs pin that.
| .pdf-container { | ||
| position: relative; | ||
| overflow-y: auto; | ||
| scrollbar-gutter: stable; |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: scrollbar-gutter is Chrome 94+ / Firefox 97+ / Safari 18.2+, also above the browserslist floor. Where unsupported the comment's promise doesn't hold and measurement reverts to today's behaviour — graceful rather than broken, and moot on iOS where overlay scrollbars take no width. Worth knowing the fit is best-effort on older WebViews.
| .epub-viewer { | ||
| position: relative; | ||
| // Counter-balance the padding to avoid unnecessary scroll | ||
| display: flex; |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: The flex-column conversion removes the coupling rather than special-casing it — flex: 1; min-height: 0 on .epub-viewer-content deletes the calc(100% - $top-bar-height) hardcode that duplicated ViewerToolbar's min-height in a second file, and turns the pre-existing .top-bar-component { flex-shrink: 0 } from a no-op into a load-bearing rule. The layout now self-adjusts when TopBar is v-if'd off.
a257ebd to
bd79df2
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15038 — 23 of 23 prior findings resolved or acknowledged; none still open, and no new findings in this delta (5 added / 2 removed lines across two files).
Commenting rather than approving only because manual QA was required for this PR and did not run — no UI verification was performed in this round. CI passing.
Prior-finding status
RESOLVED — packages/kolibri-common/components/SafeHTML/SafeHtmlTable.vue:107 — full-bleed margin merge undocumented in the commit message
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:323 — showSideBar watcher lacked the sibling firstPageWidth guard
RESOLVED — packages/kolibri-common/components/SafeHTML/SafeHtmlTable.vue:112 — raw rgb(51, 172, 245) literal instead of $themeTokens.focusOutline
ACKNOWLEDGED — packages/kolibri-common/components/EmbeddedReadCard.vue:18 — inert below the browserslist floor (informational)
ACKNOWLEDGED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:743 — scrollbar-gutter below the browserslist floor (informational)
ACKNOWLEDGED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:858 — praise on the flex-column conversion
RESOLVED — packages/kolibri-common/components/EmbeddedReadCard.vue:16 — active card left descendants in the tab order / a11y tree
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:875 — content height subtracted absent chrome in mobileEmbedded
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:282 — mobileEmbedded watcher measured before re-render
RESOLVED — kolibri/plugins/html5_viewer/frontend/views/Html5AppRendererIndex.vue:122 — embedded branch unreachable / bloompub divergence
RESOLVED — packages/kolibri-common/components/ViewerToolbar.vue:77 — source-order-dependent override of .base-toolbar; unused fullscreenButton ref
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:3 — $el measured on the display:contents passthrough root
RESOLVED — packages/kolibri-common/components/BaseToolbar.vue — restyle rippling into the untouched LessonMasteryBar
RESOLVED — packages/kolibri-common/components/EmbeddedReadCard.vue — click/keyboard logic had no spec
RESOLVED — packages/kolibri-common/components/EmbeddedReadCard.vue:53 — Options API vs the setup() convention
RESOLVED — packages/kolibri-common/components/ViewerToolbar.vue — Options API vs the setup() convention
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue — commonCoreStrings imported but unused
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:22 — sideBarOpen watcher could deref an unmounted $refs.topBar
RESOLVED — packages/kolibri-common/components/SafeHTML/style.scss — global .table-container full-bleed removal
RESOLVED — packages/kolibri-common/components/SafeHTML/index.js:166 — embedded-content-viewer wrapper had no spec
ACKNOWLEDGED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:470 — openAs: 'epub' + dropped global.ePub (author rationale accepted)
ACKNOWLEDGED — packages/kolibri-common/components/SafeHTML/tests/SafeHTML.spec.js:191 — praise on the components-own-their-class tests
ACKNOWLEDGED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:496 — praise on the display: contents comment
@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
|
Looks good to me, noticed the following minor issues:
epub.-.menu.not.closing.on.second.click.mp4
|
bd79df2 to
ab3e2b6
Compare
github-actions
Bot
added
the
DEV: backend
|
All four of these issues should be resolved now. |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15038 — delta re-review of ab3e2b61 (rebase onto develop, plus safe_html5_viewer enabled by default and amendments folded into d797bb8a).
All 23 prior findings resolved or acknowledged; 4 new findings below, all on newly changed code. CI still pending on this SHA, and manual QA did not run — this is a code-only review, so nothing here is a statement about how the viewers actually render.
- important — stale
:embeddedbinding left behind afterViewerToolbardropped the prop (inline). - important — epub search side bar has no reachable close affordance from where focus lands (inline).
- suggestion —
.pdf-viewer { height: 100% }doesn't resolve for the coach preview panel'smax-height-only container (inline). - nitpick — unexplained visual/behavioural epub changes folded into the integration commit (inline).
Enabling safe_html5_viewer is correctly scoped: SafeHTML5AppAsset.presets is (KPUB_ZIP,) only, so no contention with HTML5AppAsset, and kolibri/plugins/utils/__init__.py:465 auto-enables it on upgrade.
Prior-finding status
RESOLVED — packages/kolibri-common/components/SafeHTML/SafeHtmlTable.vue:104 — full-bleed margin merge undocumented in the commit message
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:321 — showSideBar watcher lacked the sibling firstPageWidth guard
RESOLVED — packages/kolibri-common/components/SafeHTML/SafeHtmlTable.vue — raw rgb(51, 172, 245) literal instead of a theme token
RESOLVED — packages/kolibri-common/components/EmbeddedReadCard.vue — active card left descendants in the tab order / a11y tree
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue — content height subtracted absent chrome in mobileEmbedded
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:282 — mobileEmbedded watcher measured before re-render
RESOLVED — kolibri/plugins/html5_viewer/frontend/views/Html5AppRendererIndex.vue — embedded branch unreachable / bloompub divergence
RESOLVED — packages/kolibri-common/components/ViewerToolbar.vue — source-order-dependent override of .base-toolbar
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:3 — $el measured on the display:contents passthrough root
RESOLVED — packages/kolibri-common/components/BaseToolbar.vue — restyle rippling into the untouched LessonMasteryBar
RESOLVED — packages/kolibri-common/components/EmbeddedReadCard.vue — click/keyboard logic had no spec
RESOLVED — packages/kolibri-common/components/EmbeddedReadCard.vue:57 — Options API vs the setup() convention
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue — commonCoreStrings imported but unused
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:22 — sideBarOpen watcher could deref an unmounted $refs.topBar
RESOLVED — packages/kolibri-common/components/SafeHTML/style.scss — global .table-container full-bleed removal
RESOLVED — packages/kolibri-common/components/SafeHTML/index.js:166 — embedded-content-viewer wrapper had no spec
ACKNOWLEDGED — packages/kolibri-common/components/EmbeddedReadCard.vue:18 — inert below the browserslist floor (informational)
ACKNOWLEDGED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:744 — scrollbar-gutter below the browserslist floor (informational)
ACKNOWLEDGED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:444 — openAs: 'epub' + dropped global.ePub (author rationale accepted)
ACKNOWLEDGED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:837 — praise on the flex-column conversion
ACKNOWLEDGED — packages/kolibri-common/components/SafeHTML/tests/SafeHTML.spec.js:191 — praise on the components-own-their-class tests
ACKNOWLEDGED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue — praise on the display: contents comment
@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
| <ViewerToolbar | ||
| class="pdf-controls-container" | ||
| :isInFullscreen="isInFullscreen" | ||
| :embedded="embedded" |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
important: ViewerToolbar no longer declares an embedded prop (dropped in this revision along with the labelled fullscreen button), and BaseToolbar declares no props either — so this binding falls through to the DOM and renders a stray embedded="true" attribute on .base-toolbar. This is the only surviving reference (grep -rn "<ViewerToolbar" -A6 across kolibri/plugins and packages returns just this one), so the refactor is one deletion away from complete.
| </div> | ||
| </FocusLock> | ||
|
|
||
| <FocusLock |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
important: Removing the in-FocusLock SearchButton copy leaves keyboard users with no reachable way to close the search side bar from where focus lands.
- The
sideBarOpenwatcher calls$refs.searchSideBar.focusOnInput(), so focus starts in the search<input>. - That input has
@keyup.esc.stop(SearchSideBar.vue:15), which stops the Esc keyup before it reaches@keyup.esc="closeSideBar"on.epub-viewer-content(line 38). - The focus lock is active while
searchSideBarIsOpen, so Tab can't reach theTopBarSearchButton.
Net: from the initial focus position neither Esc nor Tab-to-a-close-control works — the user has to Tab onto the submit button or a result first, then press Esc. TOC and settings are fine (Esc bubbles; SettingsSideBar has its own close button); search is the exception.
Cheapest fix is dropping .stop from SearchSideBar.vue:15 so Esc closes from the input as elsewhere; alternatively give SearchSideBar a titlebar close button matching SettingsSideBar. Worth checking why the .stop is there — it predates this PR (#14009) and carries no comment.
| @extend %dropshadow-2dp; | ||
|
|
||
| position: relative; | ||
| height: 100%; |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: height: 100% paired with display: block on the passthrough root restores sizing for consumers whose .content-viewer carries a definite height (ContentPage.vue:415, ContentArea.vue:65), but not where the consumer sets only max-height:
kolibri/plugins/coach/.../PreviewSelectedResources/PreviewContent.vue:94
/deep/ .content-viewer { position: relative; max-height: 500px; }
With height: auto on the containing block a percentage height computes to auto (CSS 2.1 §10.5), so .pdf-viewer sizes to content and — the wrapper's overflow being visible — is no longer clipped at 500px the way it was on develop, where .content-viewer and .pdf-viewer were the same element. Strictly better than the previous display: contents state, so not a regression against the last reviewed SHA, but the coach resource-preview panel is worth a QA glance.
| .bottom-bar { | ||
| height: 54px; | ||
| padding: 8px 8px 0; | ||
| padding: 8px 40px 0; |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: d797bb8a ("Integrate shared viewer chrome across viewers") also carries changes that aren't toolbar integration and aren't mentioned in the body: this 8px 8px 0 → 8px 40px 0 absorption of the padding: 0 40px rule deleted from EpubRendererIndex's scoped block (right call — two same-specificity scoped rules whose winner depended on bundle order — but note the resulting vertical padding is the child's 8px 0, not the parent's 0 0); size="small" dropped from TocButton/SettingsButton/SearchButton, enlarging all three; and the handleMouseDown guard replacing the in-FocusLock button copies, which is behavioural. 9593597d shows your own bar for explaining a style merge — a sentence in d797bb8a's body would match it.
ab3e2b6 to
d69cf2e
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15038 — delta re-review at d69cf2e1. All 27 prior findings are resolved or acknowledged; 2 new low-severity notes on the delta (both inline).
CI on this SHA is still running (8 complete, no failures). Manual QA did not run in this pass, so none of the layout changes are visually verified here — the .embedded-read-card-passthrough flex change in particular can only be confirmed in a browser.
Prior-finding status
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:35 — stale :embedded binding on ViewerToolbar
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:72 — search side bar had no reachable close affordance from initial focus
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:736 — .pdf-viewer { height: 100% } unresolved in a max-height-only container
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/BottomBar.vue:103 — ride-along epub changes undocumented in the integration commit
RESOLVED — packages/kolibri-common/components/SafeHTML/SafeHtmlTable.vue:104 — full-bleed margin merge undocumented
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:321 — showSideBar watcher lacked the firstPageWidth guard
RESOLVED — packages/kolibri-common/components/SafeHTML/SafeHtmlTable.vue — raw rgb(51, 172, 245) instead of a theme token
RESOLVED — packages/kolibri-common/components/EmbeddedReadCard.vue — active card left descendants in the tab order / a11y tree
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue — content height subtracted absent chrome in mobileEmbedded
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:282 — mobileEmbedded watcher measured before re-render
RESOLVED — kolibri/plugins/html5_viewer/frontend/views/Html5AppRendererIndex.vue — dead embedded branch / bloompub divergence
RESOLVED — packages/kolibri-common/components/ViewerToolbar.vue — source-order-dependent .base-toolbar override
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:3 — $el measured on the display: contents passthrough root
RESOLVED — packages/kolibri-common/components/BaseToolbar.vue — restyle rippling into LessonMasteryBar
RESOLVED — packages/kolibri-common/components/EmbeddedReadCard.vue — click/keyboard logic had no spec
RESOLVED — packages/kolibri-common/components/EmbeddedReadCard.vue:57 — Options API vs the setup() convention
RESOLVED — packages/kolibri-common/components/ViewerToolbar.vue — Options API vs the setup() convention
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue — commonCoreStrings imported but unused
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:22 — sideBarOpen watcher could deref an unmounted $refs.topBar
RESOLVED — packages/kolibri-common/components/SafeHTML/style.scss — global .table-container full-bleed removal
RESOLVED — packages/kolibri-common/components/SafeHTML/index.js:166 — embedded-content-viewer wrapper had no spec
ACKNOWLEDGED — packages/kolibri-common/components/EmbeddedReadCard.vue:18 — inert below the browserslist floor (informational)
ACKNOWLEDGED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:744 — scrollbar-gutter below the browserslist floor (informational)
ACKNOWLEDGED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:444 — openAs: 'epub' + dropped global.ePub
ACKNOWLEDGED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:837 — praise on the flex-column conversion
ACKNOWLEDGED — packages/kolibri-common/components/SafeHTML/tests/SafeHTML.spec.js:191 — praise on the components-own-their-class tests
ACKNOWLEDGED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue — praise on the display: contents comment
@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
| // The root carries the consumer's class (e.g. `.content-viewer`), so it must | ||
| // stay a real box for that sizing to apply. Flex column, not block, so the | ||
| // viewer fills a `max-height`-only consumer too. | ||
| .embedded-read-card-passthrough { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: This root is used for every non-mobile-embedded render of both the PDF and EPUB viewers, so block → flex column changes the containing-block model on all of their surfaces, not just the max-height: 500px coach preview that motivated it.
The two viewers also fill differently: .pdf-viewer uses flex: 1 (PdfRendererIndex.vue:735) while .epub-viewer keeps height: calc(100vh - 64px) (EpubRendererIndex.vue:839) and relies on default flex-shrink to be clamped. They agree in the 500px panel, but diverge in the consumers that set no height at all — ContentItem.vue:180 and .embedded-content-viewer (SafeHTML/style.scss:133): the EPUB stays at 100vh - 64px, the PDF sizes to content.
This is read off the CSS, not a browser — jsdom computes none of it, so no spec can guard it. Worth eyeballing the PDF and EPUB in the coach resource-preview panel, the channel renderer, and a SafeHTML embed before merge.
| class="d-tc search-input" | ||
| type="search" | ||
| :aria-label="$tr('enterSearchQuery')" | ||
| @keyup.esc.stop |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: Two small things on dropping .stop:
8ccb97c4's body enumerates three ride-along epub changes but not this fourth behavioural one — one more bullet would complete the set.- This is
type="search", where Blink/WebKit natively clear the field on Escape, so one press plausibly both clearssearchQueryand closes the side bar.SearchSideBarisv-show-ed (EpubRendererIndex.vue:77) so it stays mounted — reopening would show an empty input next to the previous result count. I haven't confirmed Chrome's current Esc-clear behaviour in a browser; can you check what reopening search after Esc looks like?
d69cf2e to
b4d5950
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15038 — delta re-review of b4d59504. 28 of 29 prior findings resolved or acknowledged; 1 still open (inline on SearchSideBar.vue), plus one new suggestion on the flex: 1 removal.
The only content delta since the last review is the removal of flex: 1 from .pdf-viewer (plus a fourth ride-along bullet in the integration commit body). That removal is correct — flex-basis: 0 would have overridden .pdf-embedded's height: 25vh. The residual question is what now sizes the viewer in the one max-height-only consumer.
CI on this SHA: 70 pass, 14 pending, 0 fail (Frontend tests and All file linting green). Manual QA did not run for this delta, so nothing here is visually verified — both open points are browser checks.
@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
Prior-finding status
UNADDRESSED — kolibri/plugins/epub_viewer/frontend/views/SearchSideBar.vue:15 — type="search" native Esc-clear on reopen
RESOLVED — packages/kolibri-common/components/EmbeddedReadCard.vue:106 — passthrough block → flex column; PDF/EPUB fill divergence
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:35 — stale :embedded binding on ViewerToolbar
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:72 — search side bar had no reachable close affordance from initial focus
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:736 — .pdf-viewer { height: 100% } unresolved in a max-height-only container
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/BottomBar.vue:103 — ride-along epub changes undocumented in the integration commit
RESOLVED — packages/kolibri-common/components/SafeHTML/SafeHtmlTable.vue:104 — full-bleed margin merge undocumented
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:321 — showSideBar watcher lacked the firstPageWidth guard
RESOLVED — packages/kolibri-common/components/SafeHTML/SafeHtmlTable.vue — raw rgb(51, 172, 245) instead of a theme token
RESOLVED — packages/kolibri-common/components/EmbeddedReadCard.vue — active card left descendants in the tab order / a11y tree
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue — content height subtracted absent chrome in mobileEmbedded
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:282 — mobileEmbedded watcher measured before re-render
RESOLVED — kolibri/plugins/html5_viewer/frontend/views/Html5AppRendererIndex.vue — dead embedded branch / bloompub divergence
RESOLVED — packages/kolibri-common/components/ViewerToolbar.vue — source-order-dependent .base-toolbar override
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:3 — $el measured on the display: contents passthrough root
RESOLVED — packages/kolibri-common/components/BaseToolbar.vue — restyle rippling into LessonMasteryBar
RESOLVED — packages/kolibri-common/components/EmbeddedReadCard.vue — click/keyboard logic had no spec
RESOLVED — packages/kolibri-common/components/EmbeddedReadCard.vue:57 — Options API vs the setup() convention
RESOLVED — packages/kolibri-common/components/ViewerToolbar.vue — Options API vs the setup() convention
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue — commonCoreStrings imported but unused
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:22 — sideBarOpen watcher could deref an unmounted $refs.topBar
RESOLVED — packages/kolibri-common/components/SafeHTML/style.scss — global .table-container full-bleed removal
RESOLVED — packages/kolibri-common/components/SafeHTML/index.js:166 — embedded-content-viewer wrapper had no spec
ACKNOWLEDGED — packages/kolibri-common/components/EmbeddedReadCard.vue:18 — inert below the browserslist floor (informational)
ACKNOWLEDGED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:744 — scrollbar-gutter below the browserslist floor (informational)
ACKNOWLEDGED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:444 — openAs: 'epub' + dropped global.ePub
ACKNOWLEDGED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:837 — praise on the flex-column conversion
ACKNOWLEDGED — packages/kolibri-common/components/SafeHTML/tests/SafeHTML.spec.js:191 — praise on the components-own-their-class tests
ACKNOWLEDGED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue — praise on the display: contents comment
| @extend %dropshadow-2dp; | ||
|
|
||
| position: relative; | ||
| // No `flex: 1` — its `flex-basis: 0` would beat `.pdf-embedded`'s height. |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: With flex: 1 gone, .pdf-viewer's only sizing input in a max-height-only consumer is height: 100%. The one such consumer is PreviewContent.vue:94 (/deep/ .content-viewer { max-height: 500px }), where the flex container's main size is indefinite — so the percentage computes to auto and the item is content-sized. What's left doing the work is default flex-shrink: 1 plus min-height: 0 clamping back to the 500px max-height, which only "fills" when the intrinsic content box exceeds 500px, and doesn't make the post-flex size definite for the height: 100% chain below (.full-height-container → .scroller-height).
Worth a browser check: does the coach resource-preview panel still show a scrollable, correctly-clipped PDF rather than a page overflowing the 500px panel? If it does, the EmbeddedReadCard.vue:104-105 comment ("Flex column, not block, so the viewer fills a max-height-only consumer too") is now slightly ahead of the code — the mechanism is shrink-to-max-height, not grow-to-fill.
The removal itself is right for the embedded case; this is only about whether the case that motivated the flex column still holds. The comment on this line is the right thing to have written — flex: 1 looks like the obvious declaration here and the reason it isn't is invisible from the surrounding code.
There was a problem hiding this comment.
Checked it in the coach resource-preview panel. Still scrolls and clips correctly: .content-viewer 500 → .pdf-viewer 500 → .full-height-container 500 → #pdf-container 452, scrollHeight 203303.
Toggling flex: 1 back on the live element gives identical numbers, so the removal is neutral here — this consumer never depended on it.
The comment was wrong though. The flex column does nothing for PDF; EPUB is what needs it, and by shrinking rather than filling: .epub-viewer's height: calc(100vh - 64px) measures 656px under a block passthrough at a 720px viewport, 500px under the flex column. Reworded to "so a taller viewer shrinks to a max-height-only consumer instead of overflowing it".
There was a problem hiding this comment.
The measured chain settles it — 500 → 500 → 500 with #pdf-container at 452 and a 203303 scrollHeight is a correctly clipped, scrollable panel, and identical numbers with flex: 1 toggled back on the live element confirms this consumer never leaned on it. Good catch that EPUB is the real motivating case: calc(100vh - 64px) resolving to 656px under a block passthrough at a 720px viewport is precisely the overflow the flex column absorbs, and 500px under it is shrink-to-fit doing the work. The reworded comment now matches the mechanism. Resolved.
| type="search" | ||
| :aria-label="$tr('enterSearchQuery')" | ||
| @keyup.esc.stop | ||
| > |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick (still open): The commit-body half of this is resolved — 5e1253c5 now enumerates the ride-along epub changes. Still unanswered: on type="search", Blink/WebKit natively clear the field on Escape. Does one Esc now both empty searchQuery and close the v-show-ed side bar, leaving an empty input beside a stale result count on reopen? Purely a browser check — the new SearchSideBar.spec.js pins the Esc-propagates half, which is the part a test can hold.
There was a problem hiding this comment.
Confirmed, and fixed. In Chromium one Esc does both: keydown → native clear → input (so v-model writes '' to searchQuery) → search → keyup reaches the ancestor's closeSideBar. The bar is v-showed, so reopening left an empty input beside the previous result count and list.
@keydown.esc.prevent on the input suppresses the native clear while the keyup still propagates, so the close affordance gained by dropping @keyup.esc.stop survives. Verified both halves against real event dispatch. The spec asserts defaultPrevented on the keydown — jsdom has no native clear to assert against.
There was a problem hiding this comment.
Thanks for running the real event dispatch — that chain (keydown → native clear → input → search, keyup still bubbling) is exactly the ordering that made this invisible from the source alone. @keydown.esc.prevent is the right lever: the native search-cancel is a keydown default action, so preventing it kills the clear without touching the keyup that reaches closeSideBar, and the close affordance you gained by dropping @keyup.esc.stop stays intact. Asserting defaultPrevented is the most a jsdom spec can pin here. Resolved.
b4d5950 to
03b66a8
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15038 — delta re-review of 03b66a8 (17 lines across 3 files since b4d5950).
All 30 prior findings resolved or acknowledged; 2 new, non-blocking findings below, both first-raise observations on branch code earlier rounds didn't cover. CI on 03b66a8 is green on Frontend tests, All file linting, and Python tests (stage 1) (Morango integration jobs still running). Manual QA did not run this round, so nothing here is visually verified — the embedded-fullscreen point below wants one look in a browser before merge.
- suggestion — embedded PDF keeps its light backdrop and hairline border in fullscreen (inline,
PdfRendererIndex.vue:215) - nitpick —
:fullscreen { padding: 0 }is now a no-op after the flex-column conversion (inline,EpubRendererIndex.vue:847)
Prior-finding status
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/SearchSideBar.vue:17 — type="search" native Esc-clear on reopen
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/SearchSideBar.vue:15 — .stop vs .prevent on the Esc handler
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:735 — what sizes .pdf-viewer after flex: 1 removal
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:736 — .pdf-viewer { height: 100% } unresolved in a max-height-only container
RESOLVED — packages/kolibri-common/components/EmbeddedReadCard.vue:106 — passthrough block -> flex column; PDF/EPUB fill divergence
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:35 — stale :embedded binding on ViewerToolbar
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:3 — $el measured on the display: contents passthrough root
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:282 — mobileEmbedded watcher measured before re-render
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:321 — showSideBar watcher lacked the firstPageWidth guard
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:72 — search side bar had no reachable close affordance from initial focus
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:22 — sideBarOpen watcher could deref an unmounted $refs.topBar
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue — content height subtracted absent chrome in mobileEmbedded
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue — commonCoreStrings imported but unused
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/BottomBar.vue:103 — ride-along epub changes undocumented in the integration commit
RESOLVED — packages/kolibri-common/components/SafeHTML/SafeHtmlTable.vue:104 — full-bleed margin merge undocumented
RESOLVED — packages/kolibri-common/components/SafeHTML/SafeHtmlTable.vue — raw rgb(51, 172, 245) instead of a theme token
RESOLVED — packages/kolibri-common/components/SafeHTML/style.scss — global .table-container full-bleed removal
RESOLVED — packages/kolibri-common/components/SafeHTML/index.js:166 — embedded-content-viewer wrapper had no spec
RESOLVED — packages/kolibri-common/components/EmbeddedReadCard.vue — active card left descendants in the tab order / a11y tree
RESOLVED — packages/kolibri-common/components/EmbeddedReadCard.vue — click/keyboard logic had no spec
RESOLVED — packages/kolibri-common/components/EmbeddedReadCard.vue:57 — Options API vs the setup() convention
RESOLVED — packages/kolibri-common/components/ViewerToolbar.vue — Options API vs the setup() convention
RESOLVED — packages/kolibri-common/components/ViewerToolbar.vue — source-order-dependent .base-toolbar override
RESOLVED — packages/kolibri-common/components/BaseToolbar.vue — restyle rippling into LessonMasteryBar
RESOLVED — kolibri/plugins/html5_viewer/frontend/views/Html5AppRendererIndex.vue — dead embedded branch / bloompub divergence
ACKNOWLEDGED — packages/kolibri-common/components/EmbeddedReadCard.vue:18 — inert below the browserslist floor (informational)
ACKNOWLEDGED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:744 — scrollbar-gutter below the browserslist floor (informational)
ACKNOWLEDGED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:444 — openAs: 'epub' + dropped global.ePub
ACKNOWLEDGED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:837 — praise on the flex-column conversion
ACKNOWLEDGED — packages/kolibri-common/components/SafeHTML/tests/SafeHTML.spec.js:191 — praise on the components-own-their-class tests
ACKNOWLEDGED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue — praise on the display: contents comment
@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
| }, | ||
| pdfViewerStyle() { | ||
| const style = { | ||
| backgroundColor: this.embedded ? this.$themeTokens.surface : this.$themeTokens.text, |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: The backdrop keys off embedded alone, not embedded && !isInFullscreen. mobileEmbedded is false in fullscreen (line 211), so entering fullscreen from an embedded PDF — the flow EmbeddedReadCard's @read="$refs.pdfViewer.toggleFullscreen()" drives — lands on $themeTokens.surface instead of the $themeTokens.text backdrop a standalone fullscreen PDF gets, keeps the 1px solid fineLine hairline at the screen edge (line 219), and still matches .pdf-embedded, so /deep/ .pdf-page { margin: 2px auto } is the only thing separating pages. White pages on a white surface with a 2px gutter is where this could read badly — the dark backdrop is what gives page boundaries on develop, and CoreFullscreen's .normalize-fullscreen only forces sizing (CoreFullscreen.vue:82-87), so nothing resets the colour.
This is read off the CSS, not a browser. Worth one look at fullscreen-from-embedded (both the desktop toolbar button and the mobile card tap) before merge; if it's intended, the comment on line 218 is the natural place to say so.
| @@ -838,27 +847,24 @@ | |||
| padding: 0; | |||
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: These two selectors existed to cancel .epub-viewer's padding: 32px 24px, which the flex-column conversion above removed. With no padding to cancel, both are dead — as is .epub-viewer.small { padding: 0 } if it's still around for the same reason.
| cursor: pointer; | ||
| } | ||
|
|
||
| // The root carries the consumer's class (e.g. `.content-viewer`), so it must |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: Reworking this comment to the mechanism the measurements actually showed (shrink-to-fit, not fill) rather than leaving a plausible-but-wrong rationale in the code is the right call — same discipline as the @keydown.esc.prevent comment in SearchSideBar.vue:9.
|
Not quite ready for QA rereview. |
03b66a8 to
4decdb2
Compare
npm Package VersionsWarning The following packages have changed files but no version bump:
If these changes affect published code, consider bumping the version. |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15038 — delta re-review of 4decdb23 (14 lines across the two renderer indexes). All prior findings are resolved or acknowledged; 4 new nitpicks below, none blocking.
Both open findings from the last round are fixed: the embedded PDF now gets the dark backdrop and drops its hairline border in fullscreen, and the dead .epub-viewer:fullscreen { padding: 0 } pair is gone along with the padding it cancelled. CI on this SHA is green on Frontend tests and All file linting; the only pending checks are the Morango integration jobs. Manual QA did not run in this pass, so nothing here is visually verified — hence COMMENT rather than APPROVE.
The new nitpicks are first-raises on branch code earlier rounds didn't reach (the .pdf-embedded class binding, the kpub preset label, some diff noise in the html5 viewer, and three unreachable ref guards in TopBar). All inline.
Prior-finding status
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:215 — embedded PDF kept its light backdrop and hairline border in fullscreen
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:847 — :fullscreen { padding: 0 } a no-op after the flex-column conversion
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/SearchSideBar.vue:17 — type="search" native Esc-clear on reopen
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/SearchSideBar.vue:15 — .stop vs .prevent on the Esc handler
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:735 — what sizes .pdf-viewer after flex: 1 removal
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:736 — .pdf-viewer { height: 100% } unresolved in a max-height-only container
RESOLVED — packages/kolibri-common/components/EmbeddedReadCard.vue:106 — passthrough block → flex column; PDF/EPUB fill divergence
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:35 — stale :embedded binding on ViewerToolbar
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:3 — $el measured on the display: contents passthrough root
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:282 — mobileEmbedded watcher measured before re-render
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:321 — showSideBar watcher lacked the firstPageWidth guard
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:72 — search side bar had no reachable close affordance from initial focus
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:22 — sideBarOpen watcher could deref an unmounted $refs.topBar
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue — content height subtracted absent chrome in mobileEmbedded
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue — commonCoreStrings imported but unused
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/BottomBar.vue:103 — ride-along epub changes undocumented in the integration commit
RESOLVED — packages/kolibri-common/components/SafeHTML/SafeHtmlTable.vue:104 — full-bleed margin merge undocumented
RESOLVED — packages/kolibri-common/components/SafeHTML/SafeHtmlTable.vue — raw rgb(51, 172, 245) instead of a theme token
RESOLVED — packages/kolibri-common/components/SafeHTML/style.scss — global .table-container full-bleed removal
RESOLVED — packages/kolibri-common/components/SafeHTML/index.js:166 — embedded-content-viewer wrapper had no spec
RESOLVED — packages/kolibri-common/components/EmbeddedReadCard.vue — active card left descendants in the tab order / a11y tree
RESOLVED — packages/kolibri-common/components/EmbeddedReadCard.vue — click/keyboard logic had no spec
RESOLVED — packages/kolibri-common/components/EmbeddedReadCard.vue:57 — Options API vs the setup() convention
RESOLVED — packages/kolibri-common/components/ViewerToolbar.vue — Options API vs the setup() convention
RESOLVED — packages/kolibri-common/components/ViewerToolbar.vue — source-order-dependent .base-toolbar override
RESOLVED — packages/kolibri-common/components/BaseToolbar.vue — restyle rippling into LessonMasteryBar
RESOLVED — kolibri/plugins/html5_viewer/frontend/views/Html5AppRendererIndex.vue — dead embedded branch / bloompub divergence
ACKNOWLEDGED — kolibri/plugins/html5_viewer/frontend/views/Html5AppRendererIndex.vue — no css_selectors / allow_object_tag on the ContentRendererHook (informational)
ACKNOWLEDGED — packages/kolibri-common/components/EmbeddedReadCard.vue:103 — praise on reworking the comment to the measured mechanism
ACKNOWLEDGED — packages/kolibri-common/components/EmbeddedReadCard.vue:18 — inert below the browserslist floor (informational)
ACKNOWLEDGED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:744 — scrollbar-gutter below the browserslist floor (informational)
ACKNOWLEDGED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:444 — openAs: 'epub' + dropped global.ePub
ACKNOWLEDGED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:837 — praise on the flex-column conversion
ACKNOWLEDGED — packages/kolibri-common/components/SafeHTML/tests/SafeHTML.spec.js:191 — praise on the components-own-their-class tests
ACKNOWLEDGED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue — praise on the display: contents comment
@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
| :class="{ | ||
| 'pdf-controls-open': showControls, | ||
| 'pdf-full-screen': isInFullscreen, | ||
| 'pdf-embedded': embedded, |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: The delta made the JS fullscreen-aware (embeddedInline = this.embedded && !this.isInFullscreen) but this class binding still keys off embedded, so the new .pdf-embedded block (lines 791–801 — develop applies the class but ships no rules for it) stays active in fullscreen-from-embedded.
Most of it is inert there: height: 25vh loses to CoreFullscreen's .normalize-fullscreen/.mimic-fullscreen height: 100% !important on the same element, and the nested .scroller-height repeats the base rule. The one live difference is /deep/ .pdf-page { margin: 2px auto } overriding PdfPage.vue:259's margin: 8px auto — so fullscreen reached from an embedded PDF separates pages by 2px where standalone fullscreen uses 8px. Cosmetic now that the backdrop matches, but if the intent is "fullscreen looks like standalone fullscreen", the class wants the same embeddedInline treatment the style got.
| slideshow_manifest: 'Slideshow ({fileSize})', | ||
| slideshow_image: 'Slideshow image ({fileSize})', | ||
| bloompub: 'Bloom Pub Document ({fileSize})', | ||
| kpub: 'Kolibri Article ({fileSize})', |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: The file header says these entries are the _READABLE strings from le_utils.constants.format_presets with ' ({fileSize})' appended. KPUB_ZIP_READABLE is Kolibri HTML5 Article; this is Kolibri Article.
The entry itself is correct and needed — format_presets.KPUB_ZIP is 'kpub', so getFilePresetString now resolves instead of hitting the logging.error fallthrough, which matters with safe_html5_viewer on by default. bloompub already deviates the same way, so there's precedent; worth a line only because the comment claims otherwise. Either match the readable name or note that the shorter label is deliberate.
There was a problem hiding this comment.
Not changing this — the file already deviates from le-utils elsewhere (bloompub vs Bloom Document), so the shorter label is in keeping with it.
There was a problem hiding this comment.
Understood — the shorter label is deliberate and consistent with the existing bloompub deviation. Deferring; the entry itself does the job of getting kpub out of the logging.error fallthrough.
| import { now } from 'kolibri/utils/serverClock'; | ||
| import CoreFullscreen from 'kolibri-common/components/CoreFullscreen'; | ||
|
|
||
| import ViewerToolbar from 'kolibri-common/components/ViewerToolbar'; |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: Stray blank lines around this import (and inside components) that BloomPubRendererIndex.vue — same edit, same file shape — doesn't have. BloomPubRendererIndex.vue:58-80 conversely reorders the useContentViewer destructuring and return with no functional effect. Both lint-clean, both add diff noise to an otherwise mechanical swap.
| */ | ||
| focusOnTocButton() { | ||
| this.$refs.tocButton.$el.focus(); | ||
| if (this.$refs.tocButton) { |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: These three ref guards can't be false — all three buttons render unconditionally inside ViewerToolbar's slots, with no v-if on any of them. The case that motivated the guard is one level up (TopBar itself is v-if="!mobileEmbedded"), and EpubRendererIndex.vue:413 already guards this.$refs.topBar before calling in. Dead branches read as if a state exists that doesn't.
4decdb2 to
625acce
Compare
|
I think this is ready for QA rereview now. |
|
@rtibbles I'm getting an error when trying to view the 
The other previously reported issues are fixed. |
ViewerToolbar is a reusable viewer chrome bar built on BaseToolbar, for adoption across the content viewers. EmbeddedReadCard frames embedded read previews in a bordered card. SafeHTML now wraps embedded ContentViewers in a layout container so they render consistently within prose.
Replace the bespoke toolbars in the bloompub, epub, html5, and pdf viewers with the shared ViewerToolbar, and adopt EmbeddedReadCard for the embedded read preview in the epub and pdf viewers. Four epub changes ride along: - `BottomBar` absorbs the `0 40px` horizontal padding deleted from `EpubRendererIndex`'s scoped block — same-specificity scoped rules whose winner depended on bundle order. Vertical padding stays the child's `8px 0`. - `TocButton`/`SettingsButton`/`SearchButton` drop `size="small"` to match the other toolbar buttons. - The side bars lose their in-`FocusLock` toggle copies, so the top bar's toggles take the click; `handleMouseDown` now ignores events inside the top bar to stop close-then-reopen. - `SearchSideBar`'s input swaps `@keyup.esc.stop` for `@keydown.esc.prevent`, so Esc closes the search side bar from the input as it already did for the others, without also firing the search field's native clear.
The component's scoped `margin: 1em 0` outranked the global `.table-container` full-bleed, zeroing its negative margin but keeping the +32px width, so every table overhung its container by 16px. SafeHtmlTable is the sole renderer of the element; the two margin intents are merged.
KPUB_ZIP content has no renderer otherwise. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WAmANyoPp3rQdCNRdQt4k6
|
Hrm, I'm not able to replicate this issue locally, either in the devserver, a local production build, or with the PEX asset downloaded from this PR. My best guess is that it's another manifestation of this #15103 (which is being fixed in a parallel PR). I'll force push a rebase on latest develop just in case there's a weird issue with the build. |
625acce to
f1be724
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15038 — delta re-review of the rebase since 625accee. 37 of 37 prior findings settled (36 resolved, 1 acknowledged); no prior finding remains open. git diff 625accee..HEAD over PR-scoped paths is empty, so the only new material is the rebase itself.
Two new suggestions inline (embedded card accessible name; persisted pdf_scale vs. the new fit watchers). Neither blocks.
CI is red on one leg only: Python unit tests for uv-supported Python versions (3.13) reports 349 failed, 3637 passed, 978 errors, all cascading from sqlite3.OperationalError: database table is locked at BEGIN IMMEDIATE in test setup. Every other leg on the same commit is green (3.6, 3.7, 3.10, Windows 3.8, macOS 3.10, postgres, all Morango legs, frontend, lint), and kolibri/plugins/utils/test/test_default_plugins.py — the test that would catch a bad DEFAULT_PLUGINS entry — passes inside the failing job. The only Python change on the branch is one line in default_plugins.py. Looks like a runner flake; worth a re-run to confirm before treating it as red.
The consolidation itself reads well: four bespoke fullscreen headers and four copies of the enterFullscreen/exitFullscreen $trs pair collapse into one component with one string namespace, and ViewerToolbar.spec.js pins each slot independently.
Not approving: manual QA was required for this PR but did not run (the dev server failed to start), so nothing here has been visually verified, and CI is not green.
Prior-finding status
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:3 — EmbeddedReadCard as template root changes this.$el
RESOLVED — packages/kolibri-common/components/BaseToolbar.vue — shared with LessonMasteryBar, untouched consumer
RESOLVED — packages/kolibri-common/components/EmbeddedReadCard.vue — not purely presentational (emitRead/onSpace guards)
RESOLVED — packages/kolibri-common/components/EmbeddedReadCard.vue:57 — new components must use setup()
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:445 — dropped global.ePub + openAs: 'epub'
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue — unused commonCoreStrings mixin
RESOLVED — packages/kolibri-common/components/SafeHTML/tests/SafeHTML.spec.js:191 — custom-components class-ownership tests
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:22 — $refs.topBar undefined when TopBar is v-if'd out
RESOLVED — packages/kolibri-common/components/SafeHTML/style.scss — dropping .table-container negative-margin sizing
RESOLVED — packages/kolibri-common/components/SafeHTML/index.js:166 — new embedded-content-viewer branch lacked coverage
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue — display: contents measurement pitfall documented
RESOLVED — packages/kolibri-common/components/EmbeddedReadCard.vue — inert content vs. tab order
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue — height subtracts unrendered toolbar/bottom bar
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:283 — synchronous width measure vs. deferred sibling watcher
RESOLVED — kolibri/plugins/html5_viewer/frontend/views/Html5AppRendererIndex.vue — no css_selectors/allow_object_tag declared
RESOLVED — packages/kolibri-common/components/ViewerToolbar.vue — .viewer-toolbar overrides .base-toolbar at equal specificity
RESOLVED — packages/kolibri-common/components/SafeHTML/SafeHtmlTable.vue:104 — relocation commit changes rendered output
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:322 — dropped $refs.pdfContainer guard
RESOLVED — packages/kolibri-common/components/SafeHTML/SafeHtmlTable.vue — raw rgb(51, 172, 245) literal
RESOLVED — packages/kolibri-common/components/EmbeddedReadCard.vue:18 — inert vs. browserslist floor
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:747 — scrollbar-gutter vs. browserslist floor
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:838 — flex-column conversion removes the coupling
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue — ViewerToolbar no longer declares an embedded prop
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue:72 — removed in-FocusLock SearchButton copy
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:738 — height: 100% + display: block on passthrough root
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/BottomBar.vue:103 — non-toolbar changes riding along in d797bb8a
RESOLVED — packages/kolibri-common/components/EmbeddedReadCard.vue:106 — block root for every non-mobile-embedded render
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/SearchSideBar.vue:15 — dropping .stop
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:737 — .pdf-viewer sizing with flex: 1 gone
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/SearchSideBar.vue:17 — commit body now enumerates ride-along epub changes
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue — backdrop keys off embedded alone
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/EpubRendererIndex.vue — selectors cancelling .epub-viewer padding
RESOLVED — packages/kolibri-common/components/EmbeddedReadCard.vue:103 — comment reworked to the measured mechanism
RESOLVED — kolibri/plugins/pdf_viewer/frontend/views/PdfRendererIndex.vue:13 — .pdf-embedded keys off embedded, not embeddedInline
RESOLVED — kolibri/plugins/html5_viewer/frontend/views/Html5AppRendererIndex.vue:44 — stray blank lines / gratuitous reorder
RESOLVED — kolibri/plugins/epub_viewer/frontend/views/TopBar.vue:68 — unreachable $refs guards in focusOn*
ACKNOWLEDGED — packages/kolibri/components/internal/filePresetStrings.js:27 — kpub label deviates from KPUB_ZIP_READABLE
@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
| :class="active ? 'embedded-read-card' : 'embedded-read-card-passthrough'" | ||
| :role="active ? 'button' : null" | ||
| :tabindex="active ? 0 : null" | ||
| :aria-label="active ? readLabel : null" |
There was a problem hiding this comment.
suggestion: Every card's accessible name is the bare word "Read".
aria-label is coreString('read') — the activity-type label — and the wrapped content is inert, so nothing else contributes to the name. A SafeHTML article with two embedded PDFs presents to a screen reader as two adjacent buttons both named "Read", with no way to tell which document each opens. The visible pill has the same problem, but sighted users have the clipped preview for context; AT users don't, precisely because the content is inert.
Cheapest fix: accept an optional label prop and have the viewer pass defaultFile.file_name. Alternatively compose Read {title} from the source element's title/aria-label, which SafeHTML/index.js:134-138 already forwards through attrs.
Suggestion rather than blocking because the single-embed case in the PR screenshots reads fine, and no acceptance criterion covers naming.
| // preview state. | ||
| this.$nextTick(() => { | ||
| if (this.firstPageWidth) { | ||
| this.scale = this.viewerWidth() / (this.firstPageWidth * this.screenSizeMultiplier); |
There was a problem hiding this comment.
suggestion: Does the persisted global pdf_scale clobber this fit?
handleUpdate (line ~484, unchanged) reasserts localStorage.pdf_scale on every update event where the recycle-list height is unchanged, and that key is global across all PDFs, not per-node:
if (localStorage.getItem('pdf_scale') != null) {
this.setScale(parseFloat(localStorage.getItem('pdf_scale')));
}So on a device where the user has ever zoomed a PDF, entering the card state looks like: this watcher sets the fit scale → heights change → one update takes the if branch → the next stable-height update takes the else branch and restores the persisted scale, undoing the shrink. Same reasoning applies to the new recycleListIsMounted re-fit below.
You already identified pdf_scale as a masking factor when reproducing the scale-0 bug, so you may have checked this — can you confirm the card preview still shrinks correctly with a non-default pdf_scale in localStorage? If not, scoping the restore to the non-embedded path is the smallest fix.
|
Apologies, the above was completely my bad: I uploaded incorrectly formatted KPUB file during the last update of the QA channel. That error is now corrected and a follow up issue for the validation and denesting of KPUB files issue filed on Studio repo. Revisiting the issues reported above by @pcenov using the updated KPUB with embeded files, all now appear fixed (re-tested in Windows app, all supported browsers and on the physical Android device):
I bumped on a single issue while navigating pages of the 
|
radinamatic
left a comment
radinamatic
left a comment
There was a problem hiding this comment.
Manual QA passes, one follow-up issue upcoming!
5 participants
Summary
References
Finalizes / fixes #13824.
Reviewer guidance
Manual testing:
EmbeddedReadCard.vue:112— active card setspointer-events: noneon its content to act as one click target. Confirm embedded content is still interactive outside card mode.PdfRendererIndex.vue:31— bespoke toolbars replaced by the shared ViewerToolbar across all four viewers. Confirm each viewer's actions (page nav, TOC, fullscreen) survived the swap.Screenshots
The standalone ViewerToolbar is reproducible on this branch, verified live on the html5 and epub viewers.
The EmbeddedReadCard shots below come from the combined implementation PR #14548. The embedded PDF/EPUB preview needs the media-player migration to render the host article. It is not fully reproducible from this branch until #15035 and #15037 merge.
PDF initial scale
Wrapping the template in
EmbeddedReadCardmovedthis.$elonto the card's passthrough root. That root isdisplay: contents, soclientWidthis 0. The initial fit computedscale = 0, andPdfPagefell back to:scale="scale || 1". Fixed by measuring the viewer element instead.Reproduced at a 700px viewport. There
showSideBarstaysfalse, so its watcher never fires to recompute the scale. Left:scale0, page collapsed. Right:scale1.017, page fits the container.Not covered by a test. jsdom always reports
clientWidth0, so broken and fixed code behave identically there.AI usage
Used Claude Code to consolidate the viewer toolbars into a shared ViewerToolbar, add EmbeddedReadCard, and split the change into reviewable commits. Verified with the ViewerToolbar, EmbeddedReadCard and TopBar Jest specs, prek, and the before/after capture above.