Configurable page numbers in PDFs #8003
Conversation
⛔ Feature branch deployment currently inactive.If the PR is still open, you can add the |
| }, | ||
| ], | ||
| documentName: 'test camp', | ||
| options: { pageNumbers: false }, |
There was a problem hiding this comment.
maybe this test could use a little drying in a future pr.
|
@coderabbitai review this pr |
🧩 Analysis chain@BacLuc I'll review the PR for configurable page numbers in PDFs. Let me gather the changes and analyze the implementation. 🏁 Script executed: Length of output: 100092 @BacLuc I've reviewed the PR for configurable page numbers in PDFs. This is a comprehensive and well-implemented feature! Here's my analysis: ✅ Strengths1. Comprehensive Implementation
2. Excellent Configuration Management
3. Smart Page Number Tracking
4. Thorough Test Coverage
🔍 Technical Implementation ReviewFrontend Components:
PDF Structure:
Backend:
💡 Minor Suggestions
🏁 Script executed: Length of output: 3807
🏁 Script executed: Length of output: 1573 Let me continue with the review and check one more technical aspect: 🏁 Script executed: Length of output: 2251 ✅ Final AssessmentThis is an excellent implementation of configurable page numbers! The code quality is high and the feature is comprehensive. Technical Excellence:
Missing Localization FixThe only improvement needed is adding the missing translations. Here are the exact additions needed: frontend/src/locales/en.json: "printConfigurator": {
"add": "Add content",
"config": {
// ... existing config entries
},
"options": "Options",
"pageNumbers": "Page numbers"
}frontend/src/locales/fr.json: "options": "Options",
"pageNumbers": "Numéros de page"frontend/src/locales/it.json: "options": "Opzioni",
"pageNumbers": "Numeri di pagina"frontend/src/locales/rm.json: "options": "Opziuns",
"pageNumbers": "Numers da pagina"🎉 RecommendationAPPROVE - This is a well-architected feature that:
The missing localizations are a minor issue that can be addressed in a follow-up commit or before merging. Great work on this comprehensive feature implementation! 🚀 ✅ Actions performedReview triggered.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughIntroduces a configurable page-number feature across print/PDF flows: adds options.pageNumbers to print configs and UI, normalizes it in repair, wires it into server PDF footer rendering, initializes a global $toc map, annotates pages with IDs and page numbers, updates TOC components to optionally display page numbers, and extends tests and locales accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Frontend UI (Configurator)
participant App as Frontend Components
participant Srv as Print Server (pdf.js)
participant Rend as PDF Renderer (Vue app)
User->>UI: Toggle "Page numbers"
UI->>App: onChange(config.options.pageNumbers)
App->>Srv: Request PDF with query ?config=JSON(config)
Srv->>Srv: Parse config JSON
alt options.pageNumbers = true
Srv->>Srv: Build footerTemplate with page numbers
else
Srv->>Srv: Build empty footerTemplate
end
Srv->>Rend: Render HTML via headless (header/footer/templates)
activate Rend
Rend->>Rend: Initialize $toc = {}
Rend->>Rend: For each Page/View: record $toc[id]=pageNumber
alt options.pageNumbers = true
Rend->>Rend: Render per-page footer numbers and TOC numbers
else
Rend->>Rend: Suppress page/TOC numbers
end
Rend-->>User: PDF
deactivate Rend
sequenceDiagram
autonumber
participant Page as Page Components
participant Hook as View(render)
participant Map as $toc
participant TOC as TOC Entry Components
Page->>Hook: Render with ({ pageNumber })
Hook->>Map: $toc[compositeId] = pageNumber
TOC->>Map: Lookup $toc[compositeId]
alt config.options.pageNumbers
TOC->>TOC: Render TocEntryPageNumber
else
TOC->>TOC: Omit page number
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.38.6)frontend/src/components/print/__tests__/repairPrintConfig.spec.jsTip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frontend/src/components/print/__tests__/repairPrintConfig.spec.js (1)
118-142: Add a positive test forpageNumbers: trueThe grep confirmed there are currently no tests covering the
truebranch ofpageNumbers. Please add at least one unit (or e2e) spec to ensure that supplying{ options: { pageNumbers: true } }is preserved byrepairConfig.• Location
frontend/src/components/print/__tests__/repairPrintConfig.spec.js• Example unit‐test to insert:
@@ describe('repairConfig', () => { test('fills empty config with default data', async () => { … }) + + test('preserves true pageNumbers option', () => { + // given + const config = { + camp: '/camps/1a2b3c4d', + contents: [{ type: 'Picasso', options: { periods: ['/periods/1a2b3c4d'] } }], + documentName: 'test camp', + options: { pageNumbers: true }, + language: 'en-GB', + } + + // when + const result = repairConfig(config, ...args) + + // then + expect(result.options.pageNumbers).toBe(true) + })Feel free to also add a matching e2e assertion against your PDF‐output stub to catch any regressions in the rendered output.
pdf/src/campPrint/picasso/PicassoPeriod.vue (2)
2-12: Add a key to the v-for’d component to stabilize instance identityWithout a
:key, Vue may reuse component instances across pages, causing subtle state bleed and incorrect render caching.Apply this diff:
- <PicassoPage - v-for="pageDays in pages" + <PicassoPage + v-for="(pageDays, pageIndex) in pages" + :key="`${id}-${period.id}-${pageIndex}`" :id="id" :config="config" :content="content" :period="period" :days="pageDays" :bedtime="bedtimes.bedtime" :get-up-time="bedtimes.getUpTime" :time-step="timeStep" :filter="filter"
2-12: Enforce first-write-wins TOC mapping in PicassoPage.vueThe unguarded assignment in pdf/src/campPrint/picasso/PicassoPage.vue will overwrite the TOC entry on every page of a multi-page period, causing later pages to clobber the first-page mapping. We need to only set the entry once (on the first page).
• File: pdf/src/campPrint/picasso/PicassoPage.vue
• Line: 4Recommended change:
- <View :render="({ pageNumber }) => ($toc[`${id}-${period.id}`] = pageNumber)" /> + <View + :render="({ pageNumber }) => { + const key = `${id}-${period.id}` + if ($toc[key] == null) { + $toc[key] = pageNumber + } + return null + }" + />This ensures the TOC entry for each period ID is only set on its first page.
♻️ Duplicate comments (2)
frontend/src/components/print/__tests__/repairPrintConfig.spec.js (1)
118-3931: DRY the repeated expectations by centralizing defaults.You’re repeating
{ options: { pageNumbers: false } }in virtually every test. This obscures the intent of each scenario.Consider:
- Introduce
const DEFAULT_DOC = { language: 'en-GB', options: { pageNumbers: false } }and useexpect.objectContaining(DEFAULT_DOC)for the common tail.- Or add a small helper
expectConfig(result, overrides)that merges defaults and compares.Example pattern you can adopt incrementally:
- expect(result).toEqual({ - camp: '/camps/1a2b3c4d', - contents: [...], - documentName: 'test camp', - options: { pageNumbers: false }, - language: 'en-GB', - }) + const BASE = { language: 'en-GB', options: { pageNumbers: false } } + expect(result).toEqual( + expect.objectContaining({ + camp: '/camps/1a2b3c4d', + contents: [...], + documentName: 'test camp', + ...BASE, + }) + )This keeps focus on the part under test and reduces maintenance churn if defaults evolve again.
print/server/api/pdf.js (1)
124-127: Harden config parsing and extract a small factory (also addresses prior feedback)Direct
JSON.parse(queryParams.config)will throw if the param is URL-encoded or malformed, causing a 500. Also, BacLuc requested a factory over the ternary. Let’s safely decode/parse and centralize footer construction.Apply this diff in-place:
- const footerTemplate = JSON.parse(queryParams.config).options?.pageNumbers - ? `<div id="footer-template" style="font-size:7pt; text-align: center; width: 100%; font-family: Helvetica, sans-serif; font-weight: 500"><span class="pageNumber"></span> / <span class="totalPages"></span></div>` - : '<div id="footer-template" style="font-size:7pt; text-align: center; width: 100%; font-family: Helvetica, sans-serif; font-weight: 500"></div>' + const footerTemplate = createFooterTemplateFromConfig(queryParams.config)Add these helpers (anywhere in module scope; below is fine), which safely handle both raw and percent-encoded payloads and default to no footer on parse failure:
+function createFooterTemplateFromConfig(configParam) { + const cfg = safeParseConfig(configParam) + const show = cfg?.options?.pageNumbers === true + const style = + 'font-size:7pt; text-align: center; width: 100%; font-family: Helvetica, sans-serif; font-weight: 500' + return show + ? `<div id="footer-template" style="${style}"><span class="pageNumber"></span> / <span class="totalPages"></span></div>` + : `<div id="footer-template" style="${style}"></div>` +} + +function safeParseConfig(configParam) { + if (!configParam || typeof configParam !== 'string') return {} + try { + const maybeDecoded = + configParam.includes('%') ? decodeURIComponent(configParam) : configParam + return JSON.parse(maybeDecoded) + } catch (e) { + // Keep PDF generation going without footer rather than failing the request + captureError?.(e) + return {} + } +}
🧹 Nitpick comments (27)
pdf/src/campPrint/activityList/ActivityList.vue (1)
3-4: Gate TOC page-number capture behind config and avoid returning a value from the render callback
- Avoid writing to $toc when page numbers are disabled.
- Prefer a block-bodied arrow function to make the side effect explicit and avoid returning a number.
Apply this diff:
- <slot></slot> - <View :render="({ pageNumber }) => ($toc[id] = pageNumber)" /> + <slot></slot> + <View + v-if="config.options.pageNumbers" + :render="({ pageNumber }) => { $toc[id] = pageNumber }" + />Optional: If this pattern repeats across pages, consider extracting a tiny helper like to DRY it up. I can sketch this if helpful.
frontend/src/components/activity/ScheduleEntry.vue (1)
413-414: Defaulting print options here is fine; consider centralizing defaults to avoid driftSetting options: { pageNumbers: false } is correct and consistent with repair logic. To prevent future divergence across places that construct printConfig, consider centralizing default print options.
Example (no immediate change required, but if you opt in, do the following):
- Create a small helper:
// frontend/src/components/print/printOptions.js export const DEFAULT_PRINT_OPTIONS = { pageNumbers: false }
- Use it here:
- options: { pageNumbers: false }, + options: DEFAULT_PRINT_OPTIONS,
- And import at the top of this file:
import { DEFAULT_PRINT_OPTIONS } from '@/components/print/printOptions.js'frontend/src/components/print/repairPrintConfig.js (2)
23-31: Solid normalization; optionally coerce common non-boolean inputsCurrent logic strictly enforces boolean, defaulting to false when not strictly true/false. If you want to be lenient with inputs coming from URLs, forms, or legacy configs, you can coerce 'true'/'false' strings.
Apply this diff:
if ( - configClone.options.pageNumbers !== true && - configClone.options.pageNumbers !== false + typeof configClone.options.pageNumbers !== 'boolean' ) { - configClone.options.pageNumbers = false + const pn = configClone.options.pageNumbers + // Coerce common representations; default to false otherwise + configClone.options.pageNumbers = pn === true || pn === 'true' }
47-49: Guard against missing options when repairPrintFilterConfig is called standalonerepairPrintFilterConfig assumes config.options exists; if it’s ever called without repairConfig first, this can throw. Add a minimal guard.
Apply this diff:
export function repairPrintFilterConfig(config, camp, knownPeriods) { + if (!config.options || typeof config.options !== 'object') { + config.options = {} + } if (!config.options.filter || typeof config.options.filter !== 'object') { config.options.filter = { category: [],pdf/src/campPrint/cover/Cover.vue (1)
3-4: Mirror ActivityList: gate capture on config and prefer explicit side-effect functionSame reasoning as elsewhere: don’t populate $toc when page numbers are disabled; make the side effect explicit.
Apply this diff:
- <slot></slot> - <View :render="({ pageNumber }) => ($toc[id] = pageNumber)" /> + <slot></slot> + <View + v-if="config.options.pageNumbers" + :render="({ pageNumber }) => { $toc[id] = pageNumber }" + />pdf/src/campPrint/summary/SafetyConsiderations.vue (1)
3-3: Consider capturing the page number for TOC consistencyOther sections (e.g., Cover, ActivityList) record their starting page in $toc. If SafetyConsiderations has a TOC entry, you’ll likely want the same capture here.
Apply this diff:
<Page :id="id" class="page"> - <slot></slot> + <slot></slot> + <View + v-if="config.options.pageNumbers" + :render="({ pageNumber }) => { $toc[id] = pageNumber }" + />If a child like SummaryPeriod already performs the capture (or if the TOC entry intentionally omits a page number), feel free to skip this. Please confirm which component provides the capture for this section.
pdf/src/campPrint/program/ProgramPeriod.vue (1)
2-2: Gate TOC page-number tracking behind config and declare the prop.You’re always mutating
$tocduring render, even when page numbers are disabled. Gate this to reduce side-effects and noise.Apply this diff to gate the assignment:
- <View :render="({ pageNumber }) => ($toc[`${id}-${period.id}`] = pageNumber)" /> + <View + v-if="config && config.options && config.options.pageNumbers" + :render="({ pageNumber }) => ($toc[`${id}-${period.id}`] = pageNumber)" + />Additionally, since
ProgramPeriodis being passed:config="config"upstream, explicitly declare the prop to avoid leaking it into$attrs:// Add to props (outside the changed lines) props: { period: { type: Object, required: true }, filter: { type: Object, default: () => ({}) }, config: { type: Object, required: true }, },If
configwas intentionally not a prop (e.g., inherited viaPdfComponent), confirm that$attrsaren’t polluting the PDF DOM. I can generate a script to scan similar components for consistent prop declaration if helpful.pdf/src/CampPrint.vue (1)
126-134: Align bottom offset with page padding to avoid overlap.
.pageusespadding: 30;while the page number sits atbottom: 15;. Withfixedpositioning this can overlap content. Consider matching the bottom offset to the page padding or slightly above it.-.page-number { - position: absolute; - bottom: 15; +.page-number { + position: absolute; + bottom: 30; /* align with .page padding */ left: 0; right: 0; width: 100%; text-align: center; font-size: 10; }Optionally, expose this offset via a style constant if you expect different paddings per layout.
pdf/src/campPrint/activity/Activity.vue (1)
3-6: Gate TOC page-number tracking to the feature flag.Mirror the gating you use for rendering the page-number footer and avoid populating
$tocwhen disabled.- <slot></slot> - <View - :render="({ pageNumber }) => ($toc[`${id}-${scheduleEntry.id}`] = pageNumber)" - /> + <slot></slot> + <View + v-if="config && config.options && config.options.pageNumbers" + :render="({ pageNumber }) => ($toc[`${id}-${scheduleEntry.id}`] = pageNumber)" + />Double-check other components that write to
$toc(e.g., Picasso pages, Program periods, Story) follow the same gating for consistency.e2e/specs/nuxtPrint.cy.js (1)
18-24: Add a positive-coverage test for pageNumbers=true and assert UI behaviorRight now we only exercise the “off” path. Add a second case with
pageNumbers: trueand assert that page numbers appear in the TOC/preview. Exposing a stable selector in the page-number component will make this robust.Apply this diff to add a second test in the same spec (assumes a data-cy hook on the page-number element):
@@ - const printConfig = { + const printConfig = { language: 'en', documentName: 'camp', options: { pageNumbers: false }, @@ cy.contains(camp.motto) cy.get('#content_0_cover').should('have.css', 'font-size', '50px') // this ensures Tailwind is properly built and integrated }) }) }) + it('shows TOC page numbers when enabled', () => { + cy.login('test@example.com') + cy.request('/api/camps.jsonhal').then((response) => { + const camp = response.body._embedded.items.filter((c) => c.motto)[0] + const campUri = camp._links.self.href + cy.request(camp._links.periods.href).then((periodsResponse) => { + const periodUri = periodsResponse.body._embedded.items[0]._links.self.href + const printConfig = { + language: 'en', + documentName: 'camp', + options: { pageNumbers: true }, + camp: campUri, + contents: [ + { type: 'Cover', options: {} }, + { type: 'Program', options: { periods: [periodUri], dayOverview: true } }, + { type: 'Toc', options: {} }, + ], + } + cy.visit( + Cypress.env('PRINT_URL') + '/?config=' + encodeURIComponent(JSON.stringify(printConfig)) + ) + // Assert that at least one TOC entry shows a page number + cy.get('[data-cy="toc-entry-page-number"]').should('exist') + }) + }) + })And expose a stable selector in the page-number component:
--- a/pdf/src/campPrint/tableOfContents/TocEntryPageNumber.vue +++ b/pdf/src/campPrint/tableOfContents/TocEntryPageNumber.vue @@ - <Text>{{ pageNumber }}</Text> + <Text data-cy="toc-entry-page-number">{{ pageNumber }}</Text>pdf/src/campPrint/tableOfContents/entry/Summary.vue (1)
9-10: Ensureconfigprop is always provided to Summary.vue
- Static searches did not reveal any direct
<Summary>usages without aconfigattribute; bothStory.vueandSafetyConsiderations.vueextendSummaryand referenceconfig.options.pageNumbers.- However, static regexes can miss dynamic registrations or programmatic renders—please manually verify every call site (including any
<component :is="…">or render functions) passes theconfigprop to these entry components.Optional fallback default for backward compatibility:
- config: { type: Object, required: true }, + config: { type: Object, required: false, default: () => ({ options: {} }) },pdf/src/campPrint/picasso/Picasso.vue (1)
2-10: Add a :key on the v-for to prevent vnode reuse issues across periodsWithout a key, Vue may reuse ProgramPeriod instances across iterations, which can subtly break per-page capture or TOC mapping when period counts change.
Apply:
<PicassoPeriod v-for="period in periods" + :key="period.id" :id="id" :config="config" :content="content" :period="period" >pdf/src/renderer/vueRenderer.js (1)
19-20: Use a null-prototype object for $toc to avoid prototype key collisionsSafer if any id ever equals "proto", "constructor", etc.
- app.config.globalProperties.$toc = {} + // Use a null-prototype object to avoid accidental collisions with Object.prototype keys. + app.config.globalProperties.$toc = Object.create(null)frontend/src/views/camp/Story.vue (1)
65-71: Centralizeoptions.pageNumbersdefault across print configurationsWe’ve identified multiple components explicitly setting
options: { pageNumbers: false }. To reduce duplication and keep defaults in sync withrepairPrintConfig, consider extracting this into a shared constant.
Locations found:
frontend/src/views/camp/Story.vue(line 70)frontend/src/views/camp/CampProgram.vue(line 210)frontend/src/components/print/PrintConfigurator.vue(line 183)frontend/src/components/activity/ScheduleEntry.vue(line 413)Suggested steps:
- Create a module, e.g.
frontend/src/utils/printDefaults.js:export const defaultPrintOptions = { pageNumbers: false };- In each component, replace the inline object with a spread of the shared default:
- options: { pageNumbers: false }, + import { defaultPrintOptions } from '@/utils/printDefaults'; + options: { ...defaultPrintOptions },- Update any related tests (e.g., in
repairPrintConfig.spec.js) to import and referencedefaultPrintOptionsinstead of hardcoding.This will keep your defaults DRY and ensure consistency whenever the default behavior needs to change.
pdf/src/campPrint/tableOfContents/entry/Program.vue (1)
2-8: Add a :key on the v-for to stabilize ProgramPeriod instancesThis avoids vnode reuse pitfalls that could desync captured page numbers for periods and schedule entries.
<ProgramPeriod v-for="period in periods" + :key="period.id" :id="id" :period="period" :filter="entry.options.filter" :config="config" />pdf/src/campPrint/summary/Story.vue (1)
3-3: Passidvia slot props to make projections self-containedProjectors might need the page
idwithout coupling to parent scope. Expose it as a slot prop; keeps the pattern consistent with other Page wrappers and helps future consumers.Apply this diff:
- <slot></slot> + <slot :id="id"></slot>pdf/src/campPrint/program/Program.vue (1)
3-3: Mirror Story/Cover slot API: passidthrough the slotSame rationale: let injected content consume the page anchor without reaching up the component tree. This also future‑proofs any page-number probes injected via the slot.
Apply this diff:
- <slot></slot> + <slot :id="id"></slot>pdf/src/renderer/__tests__/vueRenderer.spec.js (1)
58-58: Add at least one positive test withoptions.pageNumbers: trueAll updated fixtures set
pageNumbers: false. We should cover the enabled path to lock in the TOC number rendering and renderer plumbing.Proposed addition (append near the existing “table of contents” test):
+ it('renders the table of contents with page numbers enabled', async () => { + // given + const store = wrap(fullCampStoreContent) + + // when + const result = renderVueToPdfStructure(CampPrint, { + store, + $tc: tcMock, + locale: 'de', + config: { + language: 'de', + documentName: 'Pfila 2023.pdf', + options: { pageNumbers: true }, + camp: store.get('/camps/c4cca3a51342'), + contents: [ + { + type: 'Toc', + options: {}, + }, + ], + }, + }) + + // then + expect(result).toMatchFileSnapshot( + './__snapshots__/table_of_contents_with_page_numbers.spec.json.snap' + ) + })If you prefer parameterization, I can refactor these cases into
it.each([{pageNumbers:false},{pageNumbers:true}], ...).Also applies to: 95-95, 134-134, 163-163, 192-192, 222-222, 272-272
pdf/src/campPrint/tableOfContents/entry/Picasso.vue (1)
2-5: TOC entry robustness: add:keyand null-safe guard forconfig.options
- Add
:keyon thev-forLink for stable VDOM.- Guard the template against missing
options(defensive; repair should set it, but this costs nothing).Apply this diff:
- <Link v-for="period in periods" class="toc-entry" :href="`#${id}-${period.id}`"> + <Link + v-for="period in periods" + :key="period.id" + class="toc-entry" + :href="`#${id}-${period.id}`" + > <Text>{{ $tc('print.picasso.title', { period: period.description }) }}</Text> - <TocEntryPageNumber v-if="config.options.pageNumbers" :id="`${id}-${period.id}`" /> + <TocEntryPageNumber + v-if="config?.options?.pageNumbers" + :id="`${id}-${period.id}`" + />Also applies to: 9-9, 13-13, 17-17
frontend/src/views/camp/CampProgram.vue (1)
210-211: Defaulting options.pageNumbers here is fine; consider centralizing defaultsNo functional issues. For consistency and to reduce duplication across views, consider centralizing print-config defaults (e.g., a
buildBasePrintConfig({ documentName, contents, camp, language })helper that injectsoptions: { pageNumbers: false }) and reuse it here and in other views.pdf/src/campPrint/tableOfContents/entry/SafetyConsiderations.vue (1)
2-8: Add a key on v-for list items to prevent Vue warnings and improve patchingAdd a stable key on the
Linkrendered in thev-for.Apply:
- <Link v-for="period in periods" class="toc-entry" :href="`#${id}-${period.id}`"> + <Link v-for="period in periods" :key="period.id" class="toc-entry" :href="`#${id}-${period.id}`">pdf/src/campPrint/tableOfContents/entry/Story.vue (1)
2-5: Add a key on the v-forMirror the key addition to avoid potential Vue list-diffing warnings.
- <Link v-for="period in periods" class="toc-entry" :href="`#${id}-${period.id}`"> + <Link v-for="period in periods" :key="period.id" class="toc-entry" :href="`#${id}-${period.id}`">pdf/src/campPrint/tableOfContents/entry/Activity.vue (1)
8-11: DRY the anchor id to prevent divergence with the Link’s href.The id string is duplicated here and on Line 2. Prefer a single computed
anchorIdto avoid future drift.Suggested changes within this block:
- <TocEntryPageNumber - v-if="config.options.pageNumbers" - :id="`${id}-${scheduleEntry.id}`" - /> + <TocEntryPageNumber + v-if="config.options.pageNumbers" + :id="anchorId" + />And add (outside the selected lines) to
<script>:computed: { // ... anchorId() { return `${this.id}-${this.scheduleEntry?.id}` }, }Optionally, update the Link (Line 2) to
:href="\#${anchorId}`"`.pdf/src/campPrint/tableOfContents/entry/ProgramPeriod.vue (2)
5-5: Guard against missing options and centralize the flag.Use optional chaining to avoid runtime errors if
config.optionsis absent in legacy configs, and prefer a computedshowPageNumbersfor clarity and reuse.- <TocEntryPageNumber v-if="config.options.pageNumbers" :id="`${id}-${period.id}`" /> + <TocEntryPageNumber v-if="showPageNumbers" :id="`${id}-${period.id}`" /> ... - <TocEntryPageNumber - v-if="config.options.pageNumbers" - :id="`${id}-${period.id}-${scheduleEntry.id}`" - /> + <TocEntryPageNumber + v-if="showPageNumbers" + :id="`${id}-${period.id}-${scheduleEntry.id}`" + />Add this computed in the script block:
export default { name: 'Program', components: { TocEntryPageNumber }, extends: PdfComponent, props: { period: { type: Object, required: true }, filter: { type: Object, default: () => ({}) }, config: { type: Object, required: true }, }, computed: { + showPageNumbers() { + return this.config?.options?.pageNumbers ?? false + }, anyScheduleEntries() { return this.period.scheduleEntries().items.length },Also applies to: 17-20
27-37: Wiring looks good; tiny naming nit.Import/registration of
TocEntryPageNumberand the newconfigprop are correct. Minor nit: the component is named'Program'while the file isProgramPeriod.vue. Consider renaming to improve DX when debugging.-export default { - name: 'Program', +export default { + name: 'ProgramPeriod',pdf/src/campPrint/tableOfContents/TableOfContents.vue (2)
3-5: Capture TOC page number — prefer a block body for readability.The render-prop approach is correct and ensures the TOC entry can reference its own page number. Slight readability win by using a block body.
- <slot></slot> - <View :render="({ pageNumber }) => ($toc[id] = pageNumber)" /> + <slot></slot> + <View :render="({ pageNumber }) => { $toc[id] = pageNumber }" />
33-35: Component name likely incorrect.
name: 'Cover'in TableOfContents is misleading. Consider renaming to'TableOfContents'for clarity in devtools and error logs.export default { - name: 'Cover', + name: 'TableOfContents',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (7)
pdf/src/renderer/__tests__/__snapshots__/cover_page.spec.json.snapis excluded by!**/*.snappdf/src/renderer/__tests__/__snapshots__/picasso.spec.json.snapis excluded by!**/*.snappdf/src/renderer/__tests__/__snapshots__/program.spec.json.snapis excluded by!**/*.snappdf/src/renderer/__tests__/__snapshots__/single_activity.spec.json.snapis excluded by!**/*.snappdf/src/renderer/__tests__/__snapshots__/single_activity_with_special_text_with_empty_lists.spec.json.snapis excluded by!**/*.snappdf/src/renderer/__tests__/__snapshots__/story_overview.spec.json.snapis excluded by!**/*.snappdf/src/renderer/__tests__/__snapshots__/table_of_contents.spec.json.snapis excluded by!**/*.snap
📒 Files selected for processing (38)
e2e/specs/nuxtPrint.cy.js(1 hunks)frontend/src/components/activity/ScheduleEntry.vue(1 hunks)frontend/src/components/print/PrintConfigurator.vue(2 hunks)frontend/src/components/print/__tests__/repairPrintConfig.spec.js(179 hunks)frontend/src/components/print/repairPrintConfig.js(1 hunks)frontend/src/locales/de.json(1 hunks)frontend/src/views/camp/CampProgram.vue(1 hunks)frontend/src/views/camp/Story.vue(1 hunks)pdf/src/CampPrint.vue(2 hunks)pdf/src/campPrint/activity/Activity.vue(1 hunks)pdf/src/campPrint/activityList/ActivityList.vue(1 hunks)pdf/src/campPrint/activityList/ActivityListScheduleEntry.vue(1 hunks)pdf/src/campPrint/cover/Cover.vue(1 hunks)pdf/src/campPrint/picasso/Picasso.vue(1 hunks)pdf/src/campPrint/picasso/PicassoPage.vue(1 hunks)pdf/src/campPrint/picasso/PicassoPeriod.vue(1 hunks)pdf/src/campPrint/program/Program.vue(1 hunks)pdf/src/campPrint/program/ProgramPeriod.vue(1 hunks)pdf/src/campPrint/scheduleEntry/ScheduleEntry.vue(1 hunks)pdf/src/campPrint/scheduleEntry/ScheduleEntryTitle.vue(1 hunks)pdf/src/campPrint/summary/SafetyConsiderations.vue(1 hunks)pdf/src/campPrint/summary/Story.vue(1 hunks)pdf/src/campPrint/summary/SummaryPeriod.vue(1 hunks)pdf/src/campPrint/tableOfContents/TableOfContents.vue(4 hunks)pdf/src/campPrint/tableOfContents/TocEntryPageNumber.vue(1 hunks)pdf/src/campPrint/tableOfContents/entry/Activity.vue(1 hunks)pdf/src/campPrint/tableOfContents/entry/ActivityList.vue(1 hunks)pdf/src/campPrint/tableOfContents/entry/Cover.vue(1 hunks)pdf/src/campPrint/tableOfContents/entry/Picasso.vue(1 hunks)pdf/src/campPrint/tableOfContents/entry/Program.vue(2 hunks)pdf/src/campPrint/tableOfContents/entry/ProgramPeriod.vue(2 hunks)pdf/src/campPrint/tableOfContents/entry/SafetyConsiderations.vue(1 hunks)pdf/src/campPrint/tableOfContents/entry/Story.vue(1 hunks)pdf/src/campPrint/tableOfContents/entry/Summary.vue(1 hunks)pdf/src/campPrint/tableOfContents/entry/Toc.vue(1 hunks)pdf/src/renderer/__tests__/vueRenderer.spec.js(7 hunks)pdf/src/renderer/vueRenderer.js(1 hunks)print/server/api/pdf.js(1 hunks)
🔇 Additional comments (25)
pdf/src/CampPrint.vue (1)
10-17: No server-side PDF footer logic found in this repo
I ran a global search for bothpageNumbersandfooteracross the entire repository and didn’t find any server-side PDF generation code (e.g. noserver/pdf.jsor similar files) that conditionally renders a footer. As a result, there is no risk of duplicate page numbers within this codebase.If your application’s PDF footer is implemented in a separate service or repository, please verify the gating there—but within this PR, no additional changes are needed.
Likely an incorrect or invalid review comment.
frontend/src/components/print/__tests__/repairPrintConfig.spec.js (1)
118-142: LGTM: defaulting now includes options.pageNumbers consistently.The assertions clearly encode the new default shape, which will protect against regressions in
repairPrintConfig.e2e/specs/nuxtPrint.cy.js (1)
21-21: Good: config now explicitly sets pageNumbers=false in E2E previewThis aligns the preview with the new option and avoids accidental regressions from defaults.
pdf/src/campPrint/picasso/PicassoPeriod.vue (1)
13-15: Slot forwarding looks correct and matches the new PicassoPage default slotThe projection enables consumers to inject per-page content (e.g., page-number overlays) cleanly.
pdf/src/campPrint/activityList/ActivityListScheduleEntry.vue (1)
2-6: Id contract verified end-to-end – approved
PdfComponent declares anidprop, ScheduleEntryTitle inherits and forwards it into the rendered<Text :id="id" …>binding, and all TOCEntryPageNumber usages (including Activity.vue’s:id="${id}-${scheduleEntry.id}") consume the same identifier patterns for deterministic page-number mapping.pdf/src/campPrint/picasso/Picasso.vue (2)
8-10: Enabling a slot per period is the right hook for capturing page numbersSwitching from self-closing to a block element with a default slot lets downstream components inject the page-number capture without changing data flow. Looks good.
2-10: Sanity check: no slot projections found in scanned components—manual verification still recommendedThe recent ripgrep scan across all
.vuefiles inpdf/srcandfrontend/srcdid not locate any<Picasso>or<PicassoPeriod>usages wrapping projected slot content. This suggests no callers are currently injecting heavier markup into the slot, but to be safe, please manually verify any of the following:
- Dynamic or programmatic usages of the
Picasso/PicassoPeriodcomponents (e.g. in render functions, other directories, or via plugin registration) only project the minimal capture node.- Ensure no unexpected
<template v-slot>or scoped‐slot patterns aroundPicassoin other parts of the codebase.pdf/src/renderer/vueRenderer.js (1)
11-23: LGTM: per-app $toc initialization is scoped and reset per renderThe $toc map lives on the app instance, so each render gets a fresh map; avoids cross-render leakage.
pdf/src/campPrint/tableOfContents/entry/Program.vue (1)
7-8: No missing:configpropagation forProgram.vueAll instantiations of
Program.vueoccur via the dynamic entry‐component loop inTableOfContents.vue, which unconditionally passes the parentconfigprop to every entry component (includingProgram) here:<template v-for="(entry, index) in config.contents"> <component :is="entryComponents[entry.type]" v-if="entry.type in entryComponents" :id="`entry-${index}`" :entry="entry" :config="config" /> … </template>Since that binding covers all entry types—
Programincluded—there are no call sites that omit:config. The requirement forconfiginProgram.vueand downstream inProgramPeriod.vueis fully satisfied by this single, global template. You can safely ignore the original warning.Likely an incorrect or invalid review comment.
print/server/api/pdf.js (1)
135-135: Confirm product intent: header always shown even when footer is empty
displayHeaderFooter: truekeeps the header visible regardless ofpageNumbers. If the intent is “numbers off, header still on,” this is correct. If “no page adornments at all,” consider togglingdisplayHeaderFooteralongside the footer.If “no adornments” is desired, replace with:
- displayHeaderFooter: true, + displayHeaderFooter: footerTemplate.includes('pageNumber') || true, // keep true if header must stayOr better, gate with a config flag e.g.,
options.header.pdf/src/campPrint/scheduleEntry/ScheduleEntry.vue (1)
2-2: No changes needed:idprop is correctly defined and inheritedVerified that
pdf/src/PdfComponent.jsdeclares anidprop and that bothScheduleEntry.vueandScheduleEntryTitle.vueextendPdfComponent, inheriting this prop. Passing:id="id"intoScheduleEntryTitleis therefore safe and will not beundefinedat runtime.•
PdfComponent.jsexports a component withprops: { id: { type: String, default: '' }, },confirming
idis defined
•ScheduleEntry.vueextendsPdfComponent, soidis available in its template
•ScheduleEntryTitle.vuealso extendsPdfComponent(in addition to its own props), so it accepts theidprop as expectedNo further action required.
pdf/src/campPrint/tableOfContents/entry/SafetyConsiderations.vue (1)
7-7: No null guard needed forconfig
TheSummary.vuecomponent (pdf/src/campPrint/tableOfContents/entry/Summary.vue) declaresprops: { entry: { type: Object, required: true }, config: { type: Object, required: true }, },so
configis guaranteed to be present. The existing usage in SafetyConsiderations.vue:<TocEntryPageNumber v-if="config.options.pageNumbers" :id="`${id}-${period.id}`" />is safe and requires no additional guard.
pdf/src/campPrint/tableOfContents/entry/Story.vue (1)
4-4: Resolved –configis guaranteed by Summary.vue
I’ve confirmed in pdf/src/campPrint/tableOfContents/entry/Summary.vue thatconfigis declared as a required prop (type Object, required: true). Therefore, it can never be undefined at runtime, and usingv-if="config.options.pageNumbers"safely guards rendering.pdf/src/campPrint/tableOfContents/entry/Cover.vue (2)
4-4: LGTM: conditional page number rendering on CoverThe conditional
<TocEntryPageNumber>render underconfig.options.pageNumbersis consistent with the rest of the TOC entries.
15-17: Prop contract consistency:configis correctly forwardedVerified that in pdf/src/campPrint/tableOfContents/TableOfContents.vue, the dynamic entry renderer includes
:config="config"(line 15), ensuring that theCovercomponent receives theconfigprop. No changes required.pdf/src/campPrint/picasso/PicassoPage.vue (1)
3-3: Good addition: slot improves extensibility of page layout.Exposes a content hook without affecting existing structure. No issues spotted.
pdf/src/campPrint/tableOfContents/entry/Toc.vue (1)
4-4: TOC entry integration looks correct and consistent.Conditional page-number rendering, proper import/registration, and new config prop are all in line with the broader change.
Also applies to: 9-9, 13-13, 15-17
pdf/src/campPrint/tableOfContents/entry/Activity.vue (1)
8-11: Activity TOC: page-number wiring is correct.
- Uses the same anchor id format as the Link.
- Properly gated by
config.options.pageNumbers.- Import/registration are minimal and localized.
Also applies to: 16-16, 20-20, 24-24
pdf/src/campPrint/tableOfContents/entry/ActivityList.vue (1)
1-5: ActivityList TOC entry: straightforward and consistent with the pattern.
- Correct conditional rendering via
config.options.pageNumbers.- Proper import/registration and required
configprop.Also applies to: 8-17
frontend/src/components/print/PrintConfigurator.vue (3)
61-64: v-model on a nested computed field — confirm stability of object identity.Binding to
cnf.options.pageNumbersis fine, but sincecnfis a computed getter, ensure it returns a stable object identity (until store changes) so nested mutations aren’t lost on re-computation. Your@input="onChange"mitigates this by persisting to the store.
183-183: Good defaulting for backward compatibility.Adding
options: { pageNumbers: false }to the last-config fallback is the right call. EnsurerepairPrintConfig.jsalso normalizesconfig.options.pageNumberswhen absent to protect server-side renders that bypass the UI.
54-67: Verify that both translation keys exist undercomponents.print.printConfiguratorin every localeRipgrep confirms the
printConfiguratorobject is defined in all locale files (includingde.json,en.json,fr.json,it.json,rm.jsonand their-CH-scoutvariants). However, we weren’t able to programmatically confirm the presence of theoptionsandpageNumbersproperties in each block due to environment permission issues. Please manually verify that each of these locale JSON files includes both keys:
- frontend/src/locales/en.json
- frontend/src/locales/de.json
- frontend/src/locales/fr.json
- frontend/src/locales/it.json
- frontend/src/locales/rm.json
- And the corresponding
*-CH-scout.jsonfilesIf any locale is missing either key, add entries for
"components": { "print": { "printConfigurator": { "options": "...", "pageNumbers": "..." } } }in that file.
pdf/src/campPrint/tableOfContents/TableOfContents.vue (3)
15-16: Passingconfigdown is necessary and correct.This enables conditional page-number rendering in entry components without duplicating global wiring.
31-31: ActivityList inclusion in TOC components looks consistent.Importing and registering
ActivityListaligns the TOC with supported content types.Also applies to: 50-51
70-76: TOC layout tweaks — verify dotted leaders/spacing remain aligned.Switching
.toc-entrytowidth: 100%and using padding on.toc-sub-entryshould improve alignment, but double-check that leader dots and page numbers still line up across wrapped titles.
carlobeltrame
temporarily deployed
to
feature-branch
GitHub Actions
Inactive
Also, extract the logic as a separate component with a more speaking name and explaining comment.
carlobeltrame
force-pushed
the
page-numbers
branch
from
0840055 to
3d95fb8
Compare
carlobeltrame
temporarily deployed
to
feature-branch
GitHub Actions
Inactive
carlobeltrame
temporarily deployed
to
feature-branch
GitHub Actions
Inactive
3 participants
No description provided.