Skip to content

fix(deck): move the closing footer to the finale, the deck's last slide (deck-footer-on-finale) - #28

Merged
OriNachum merged 2 commits into
mainfrom
deck-footer-on-finale
Jul 22, 2026
Merged

fix(deck): move the closing footer to the finale, the deck's last slide (deck-footer-on-finale)#28
OriNachum merged 2 commits into
mainfrom
deck-footer-on-finale

Conversation

@OriNachum

Copy link
Copy Markdown
Contributor

The deck's closing footer — "The full argument, in text →", "All
presentations"
, and the subordinate Sources disclosure — sat on slide 6
(close), which is where the deck used to end. Now that the whats-next finale
is slide 7, that chrome belongs on the actual last slide, so a viewer reaches
it at the end of the talk rather than one slide early.

What moved

  • The close-foot block (CTA pair + Sources <details>) moves from the
    close branch to the next branch, rendering after the finale's spoken
    close.
  • The padding-bottom: 7rem that reserved room for the fixed pager chrome
    beneath the sources moves with it: .slide--close.slide--next.
  • Dataset prose updated — deckSources renders once in the finale's footer,
    not on the close slide.

Verification

The new location is pinned, not just moved: check-presentations.mjs's finale
check now asserts the article CTA and all-presentations link live inside the
finale, and that the Sources disclosure renders exactly once both deck-wide
and within that section. The CTA assertion was observed failing against the
pre-move build before the markup moved.

20/20 node tests, astro build clean, 23/23 presentation checks, 56 pytest,
markdownlint clean. Both affected slides screenshot-reviewed: the close slide
now ends on the verbatim thesis, and the finale ends on the CTA pair and
Sources. Version 0.18.0 → 0.18.1.

🤖 Generated with Claude Code

https://claude.ai/code/session_01FqTq59W6wi1YZqxGf228Tt

  • org (Claude)

The "full argument, in text" CTA, the all-presentations link, and the
subordinate Sources disclosure sat on slide 6 (close) — where the deck
used to end. With the whats-next finale as slide 7, the deck's closing
chrome now lives there, so a viewer reaches it at the actual end.

The .slide--close padding-bottom that reserved room for the fixed pager
chrome beneath the sources moves to .slide--next with the footer.
check-presentations pins the new location (article CTA and
all-presentations link inside the finale, Sources rendered exactly once
deck-wide and inside the finale) — the CTA assertion was observed
failing against the pre-move build. Dataset prose updated: deckSources
renders once in the finale's footer, not on the close slide.

Verified: 20/20 node tests, astro build clean, 23/23 presentation
checks, 56 pytest, markdownlint clean, both slides screenshot-reviewed.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FqTq59W6wi1YZqxGf228Tt
@OriNachum

Copy link
Copy Markdown
Contributor Author

/agentic_review

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Fix deck closing footer to render on finale (last) slide

🐞 Bug fix 🧪 Tests 📝 Documentation ⚙️ Configuration changes 🕐 20-40 Minutes

Grey Divider

AI Description

• Move the deck’s closing footer (CTAs + Sources disclosure) from slide 6 to slide 7 (finale).
• Shift the footer’s viewport padding rule to the finale slide styling to preserve chrome spacing.
• Pin the new footer location with presentation checks and update related documentation/versioning.
Diagram

graph TD
  A["mind-nervous-system-body.astro"] --> B["Finale slide (whats-next)"] --> C["Closing footer (CTAs + Sources)"]
  A --> D["slide--next padding"]
  A --> E["deckSources data"]
  F["check-presentations.mjs"] --> B
  F --> C
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Render footer via a shared “last-slide footer” component
  • ➕ Prevents future drift if slide ordering changes again
  • ➕ Centralizes CTA/Sources markup and spacing rules
  • ➖ Slightly more abstraction/indirection for a single-deck need
  • ➖ Requires deciding where “last slide” is computed (data vs template)
2. Compute “last slide” dynamically and conditionally render footer
  • ➕ Footer always follows the actual last slide without manual moves
  • ➕ Works if additional slides are inserted/removed later
  • ➖ Adds conditional logic to the slide rendering path
  • ➖ Harder to reason about than explicit placement in the finale markup

Recommendation: The PR’s explicit move is the simplest and lowest-risk fix, and the new check-presentations assertions appropriately pin the behavior. If this deck pattern repeats across multiple decks or slide order is expected to evolve, consider extracting the closing footer into a shared component (or last-slide conditional) to avoid future manual relocation.

Files changed (6) +69 / -36

Bug fix (1) +36 / -32
mind-nervous-system-body.astroMove closing footer markup and spacing from close slide to finale +36/-32

Move closing footer markup and spacing from close slide to finale

• Relocates the closing footer block (CTAs + Sources disclosure) from the close slide section to the whats-next finale section. Moves the padding-bottom rule from .slide--close to .slide--next to preserve space for fixed chrome under short viewports.

site-astro/src/pages/presentations/mind-nervous-system-body.astro

Tests (1) +22 / -0
check-presentations.mjsAssert closing footer and Sources render only on the finale +22/-0

Assert closing footer and Sources render only on the finale

• Extends the finale validation to require the article CTA and all-presentations link within the finale section. Adds checks ensuring the Sources disclosure renders exactly once in the finale and exactly once deck-wide.

site-astro/scripts/check-presentations.mjs

Documentation (2) +9 / -2
CHANGELOG.mdDocument footer move in 0.18.1 changelog entry +6/-0

Document footer move in 0.18.1 changelog entry

• Adds a 0.18.1 entry describing the closing footer relocation to the finale slide and noting the spacing/check updates.

CHANGELOG.md

mind-nervous-system-body-slides.tsUpdate deckSources docs to reflect finale footer placement +3/-2

Update deckSources docs to reflect finale footer placement

• Updates comments and docstrings to say the subordinate sources note is rendered on the finale slide (closing footer) rather than the close slide.

site-astro/src/data/mind-nervous-system-body-slides.ts

Other (2) +2 / -2
pyproject.tomlBump project version to 0.18.1 +1/-1

Bump project version to 0.18.1

• Updates the project version from 0.18.0 to 0.18.1.

pyproject.toml

uv.lockUpdate lockfile for 0.18.1 version bump +1/-1

Update lockfile for 0.18.1 version bump

• Aligns the lockfile’s editable package metadata to version 0.18.1.

uv.lock

@qodo-code-review

qodo-code-review Bot commented Jul 22, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 5 rules
✅ Skills: doc-test-alignment

Grey Divider


Remediation recommended

1. Finale link check too lax ✓ Resolved 🐞 Bug ☼ Reliability
Description
The finale validation asserts the all-presentations link exists but does not pin it to exactly one
occurrence, so duplicate footer rendering or accidental extra /presentations/ links inside the
finale would still pass. This weakens the “pinned, not just moved” guarantee compared to the article
CTA check which requires exactly one match.
Code

site-astro/scripts/check-presentations.mjs[R684-687]

+  assert(
+    linksTo(finale.inner, presentationsRoute).length >= 1,
+    `the finale must carry the all-presentations link (${presentationsRoute})`,
+  );
Relevance

⭐⭐⭐ High

Team often pins structure with exact-count asserts in checks; similar “harden checks” suggestions
accepted (PR #5, #27).

PR-#5
PR-#27

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The new finale check uses >= 1 for the presentations link, while linksTo() simply filters
anchors by exact href, so multiple matching anchors would still satisfy the assertion. The finale
footer markup currently includes a single /presentations/ link, so tightening this would better
pin the intended structure.

site-astro/scripts/check-presentations.mjs[637-697]
site-astro/scripts/check-presentations.mjs[189-203]
site-astro/src/pages/presentations/mind-nervous-system-body.astro[456-464]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The finale check currently allows multiple `/presentations/` links because it asserts `>= 1`.

### Issue Context
`linksTo()` matches exact `href` values, so `>= 1` won’t catch duplicates.

### Fix Focus Areas
- site-astro/scripts/check-presentations.mjs[684-687]

### Suggested fix
Change the assertion to require exactly one match and include the found count in the failure message, e.g.:
```js
const finalePresentations = linksTo(finale.inner, presentationsRoute).length;
assert(
 finalePresentations === 1,
 `the finale must carry the all-presentations link (${presentationsRoute}) exactly once; found ${finalePresentations}`,
);
```

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


2. Brittle Sources count check ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
The new Sources uniqueness assertions count the literal substring >Sources</summary> in raw HTML,
which is fragile to harmless formatting/whitespace changes in the rendered <summary> content. This
can cause false failures in check-presentations even when a semantic Sources disclosure still exists
exactly once.
Code

site-astro/scripts/check-presentations.mjs[R688-697]

+  const finaleSources = countOccurrences(finale.inner, ">Sources</summary>"); // scope-attr safe
+  assert(
+    finaleSources === 1,
+    `the Sources disclosure must render exactly once, on the finale; found ${finaleSources} there`,
+  );
+  const deckSourcesTotal = countOccurrences(deckHtml, ">Sources</summary>");
+  assert(
+    deckSourcesTotal === 1,
+    `the deck must render exactly one Sources disclosure, found ${deckSourcesTotal}`,
+  );
Relevance

⭐⭐ Medium

No clear history on avoiding literal HTML substring checks; team sometimes relaxes over-strict
assertions (PR #22).

PR-#22

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The new check depends on finding an exact serialized HTML substring. Since countOccurrences()
performs a literal substring match, the assertion is sensitive to formatting differences rather than
the semantic presence of a <summary> whose text is "Sources".

site-astro/scripts/check-presentations.mjs[637-697]
site-astro/scripts/check-presentations.mjs[200-203]
site-astro/src/pages/presentations/mind-nervous-system-body.astro[465-467]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The check counts a raw HTML substring (`>Sources</summary>`) to detect Sources disclosures, which is fragile to formatting changes.

### Issue Context
`countOccurrences()` does a literal substring regex match; it does not ignore whitespace/newlines or validate that the match is actually a `<summary>` element with the text "Sources".

### Fix Focus Areas
- site-astro/scripts/check-presentations.mjs[688-697]
- site-astro/scripts/check-presentations.mjs[166-176]
- site-astro/scripts/check-presentations.mjs[189-203]

### Suggested fix
Replace substring counts with a semantic count using the existing HTML helpers:
```js
function countSourcesSummaries(html) {
 return elements(html, "summary").filter(
   (s) => textContent(s.inner).trim() === "Sources",
 ).length;
}

const finaleSources = countSourcesSummaries(finale.inner);
assert(finaleSources === 1, `...found ${finaleSources} there`);

const deckSourcesTotal = countSourcesSummaries(deckHtml);
assert(deckSourcesTotal === 1, `...found ${deckSourcesTotal}`);
```
This remains scope-attr safe while being resilient to whitespace.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread site-astro/scripts/check-presentations.mjs
Comment thread site-astro/scripts/check-presentations.mjs Outdated
…iew)

Two fixes to the finale check this PR added, both from Qodo on #28:

- the all-presentations link asserted >= 1, so duplicate footer
  rendering would still pass while the article CTA next to it required
  exactly one. Now === 1, with the found count in both messages.
- the Sources uniqueness assertions counted the raw substring
  ">Sources</summary>", which a harmless whitespace or formatting
  change in the rendered markup would break. Replaced with a semantic
  countSourcesSummaries() built on the existing elements() +
  textContent() helpers.

Both verified against the built HTML: injecting a second
/presentations/ link into the finale fails the tightened assertion, and
reformatting the summary across newlines still passes 23/23 where the
old substring count would have read 0.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FqTq59W6wi1YZqxGf228Tt
@sonarqubecloud

Copy link
Copy Markdown

@OriNachum
OriNachum merged commit 2cd0500 into main Jul 22, 2026
7 checks passed
@OriNachum
OriNachum deleted the deck-footer-on-finale branch July 22, 2026 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant