Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions docs/adr/0014-demote-markdown-headings-in-lesson-rendering.md
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,12 @@ audits exist to catch.
- Trade-off: the rendered HTML no longer matches what a generic CommonMark
renderer would produce for the same source (a surprise when comparing with
an external preview)
- Amendment (2026-07-28, issue #117): demotion turns the common habit of
starting a document with its own title into an `h2` that duplicates the
page `h1`, so the lesson route now strips a leading level-1 heading whose
text equals the lesson title before rendering
(`MarkdownRenderer.stripLeadingTitleHeading`); the stored Markdown is
untouched

## Pros and Cons of the Options

Expand Down
107 changes: 107 additions & 0 deletions docs/adr/0015-render-lesson-alerts-with-commonmark-alerts.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
# Render lesson alerts with the commonmark-java alerts extension

- Status: accepted
- Date: 2026-07-28
- Deciders: Eric Bouchut

## Context and Problem Statement

Instructors write lesson content in Markdown (see
[ADR-0013](0013-render-lesson-markdown-with-commonmark-java.md)) and expect
the callout syntax they know from GitHub and Obsidian to work:
`> [!note]`, `> [!tip] Custom title`, nested callouts. Plain CommonMark
renders these as ordinary blockquotes with the marker as literal text. How
should lessons render alert/callout blocks while keeping the sanitization
pipeline allowlist-based and the rendered HTML free of author-controlled
markup tricks?

## Decision Drivers

- Author familiarity: the GitHub/Obsidian syntax should just work, including
content pasted from either tool
- ADR-0013's pipeline stays: parse, render, sanitize against an allowlist;
no raw SVG or style attributes may survive into the page
- Maintenance cost: prefer an official, tested module over in-repo parser code
- GitHub fidelity: content authored for GitHub must render with the same
type semantics as on GitHub

## Considered Options

- The official `commonmark-ext-gfm-alerts` extension (requires upgrading
commonmark 0.24.0 => 0.29.0)
- A custom commonmark-java block parser and renderer kept in this repository
- A jsoup post-processing pass that rewrites `[!note]` blockquotes after
rendering
- Do nothing: callout markers render as literal blockquote text

## Decision Outcome

Chosen: "the official `commonmark-ext-gfm-alerts` extension", because it is
maintained by the commonmark-java project itself, implements the exact GFM
syntax plus the options needed to cover Obsidian (custom types, custom
titles, nested alerts), and emits plain `div`/`p` markup with predictable
class names that the sanitizer can allowlist precisely. The extension is
version-locked to its same-version core and its configuration API only
exists in 0.29.0, so the whole commonmark stack moves 0.24.0 => 0.29.0
(changelog reviewed: no API removals; the notable parser change is that
tables no longer require a preceding blank line).

Configuration decisions that follow:

- The five GFM types (`NOTE`, `TIP`, `IMPORTANT`, `WARNING`, `CAUTION`) keep
their GitHub identity: `IMPORTANT` and `CAUTION` are registered as
standalone types, not as Obsidian aliases, so GitHub-authored content
renders with GitHub's semantics. The remaining Obsidian callout set joins
them with all aliases (27 registered types in total); markers are
case-insensitive; custom titles and nesting are enabled.
- Icons are Octicons (the set GitHub uses for alerts), matched per type
against Obsidian's icon semantics, and shipped as CSS `mask-image` data
URIs colored by `currentColor`. The sanitizer therefore never has to let
SVG through.
- The jsoup allowlist must now admit `class` on `div` and `p` plus
`data-alert-type`. To keep raw HTML in lessons from borrowing site classes
(`alert`, `site-header`, ...), a second sanitization pass strips every
class value the alert renderer does not emit: only
`markdown-alert markdown-alert-<type>` on `div` and `markdown-alert-title`
on `p` survive.

### Consequences

- Good: GitHub- and Obsidian-authored content renders as its authors expect,
with no house syntax to teach
- Good: the sanitization posture of ADR-0013 is preserved; the allowlist
widens by two attributes and is immediately narrowed by an exact-value pass
- Good: the commonmark stack is current again (0.24.0 dated from the initial
integration)
- Trade-off: a major-feature upgrade of the Markdown stack rides along; the
full test suite is the regression net for tables, heading demotion, and
caching
- Trade-off: 27 registered types means 27 CSS selectors and a set of icon
data URIs in `base.css`, a one-time styling cost paid in this change

## Pros and Cons of the Options

### Official commonmark-ext-gfm-alerts extension

- 👍 Maintained upstream by the commonmark-java project, GFM-exact syntax
- 👍 Options cover the full Obsidian feature set (types, titles, nesting)
- 👍 Emits sanitizer-friendly `div`/`p` markup, no inline styles or SVG
- 👎 Forces the 0.24.0 => 0.29.0 core upgrade in the same change

### Custom block parser in this repository

- 👍 No version constraint on the core
- 👎 Reimplements and then maintains non-trivial parsing (nesting, titles,
lazy continuation) that upstream already tests

### jsoup post-processing of rendered blockquotes

- 👍 No parser changes at all
- 👎 Fragile text matching inside rendered HTML; nesting and custom titles
get hard quickly; the marker has already been mangled by inline rendering

### Do nothing

- 👍 No code
- 👎 Pasted GitHub/Obsidian content degrades into blockquotes with visible
`[!note]` markers, exactly what instructors would report as a bug
3 changes: 2 additions & 1 deletion docs/adr/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,4 +44,5 @@ NNNN-short-title-in-kebab-case.md
| [0011](0011-start-ci-quality-checks-as-advisory-reports.md) | Start CI quality checks as advisory reports, gates come later | superseded by ADR-0012 |
| [0012](0012-publish-test-coverage-to-codecov.md) | Publish test coverage to Codecov | accepted |
| [0013](0013-render-lesson-markdown-with-commonmark-java.md) | Render lesson Markdown with commonmark-java, sanitized by jsoup | accepted |
| [0014](0014-demote-markdown-headings-in-lesson-rendering.md) | Demote Markdown headings one level in lesson rendering | accepted |
| [0014](0014-demote-markdown-headings-in-lesson-rendering.md) | Demote Markdown headings one level in lesson rendering | accepted |
| [0015](0015-render-lesson-alerts-with-commonmark-alerts.md) | Render lesson alerts with the commonmark-java alerts extension | accepted |
32 changes: 32 additions & 0 deletions docs/plans/2026-07-28-lesson-alerts-and-title-dedup.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
# Lesson Alerts and Title Dedup Implementation Plan

**Goal:** Two improvements to lesson Markdown rendering, on the `fix/lesson-content-styles` branch: (1) alert/callout blocks (`> [!note]`) in the syntax GitHub and Obsidian share, styled with octicons and theme accents (issue #116); (2) stop showing a duplicated heading when a lesson's Markdown starts with a level-1 heading equal to the lesson title (issue #117).

**Architecture:** Both changes stay inside the rendering pipeline of [ADR-0013](../adr/0013-render-lesson-markdown-with-commonmark-java.md) and [ADR-0014](../adr/0014-demote-markdown-headings-in-lesson-rendering.md). Alerts are parsed by the official `commonmark-ext-gfm-alerts` extension (decision recorded in [ADR-0015](../adr/0015-render-lesson-alerts-with-commonmark-alerts.md)), which forces a commonmark 0.24.0 => 0.29.0 upgrade; sanitization stays allowlist-based with a second pass that rejects any class value the alert renderer does not emit. The title dedup is a pure static helper in `MarkdownRenderer`, applied by the lesson route before `render()` so the render cache stays content-addressed.

**Out of scope:** alert support outside lessons, a Markdown preview in the lesson form, and any change to stored lesson content.

---

## Version Control (GitButler)

- Commit with `but commit fix/lesson-content-styles -m "<msg>"` from the main repository; never `git add`/`git commit`.
- **NEVER push.** The user reviews in GitButler and pushes manually.
- One atomic commit per task below; tests land with the code they prove.

---

## Tasks

- [ ] `docs(plan): Add the lesson alerts and title dedup plan` (this document)
- [ ] `docs(adr): Record the lesson alert rendering decision` ([ADR-0015](../adr/0015-render-lesson-alerts-with-commonmark-alerts.md) plus the ADR index row)
- [ ] `build(deps): Upgrade commonmark to 0.29.0` (version property only; required by the alerts extension, which is version-locked to its core; changelog reviewed, no API removals; full suite green proves tables, heading demotion, and caching survive)
- [ ] `feat(markdown): Render GFM and Obsidian-style alerts in lessons` (alerts dependency; `MarkdownRenderer` wiring: 27 registered types where the five GFM types keep their GitHub identity and the Obsidian set joins them with aliases, custom titles, nesting; sanitizer allowlist for `div[class, data-alert-type]` and `p[class]` plus the `stripUnknownAlertClasses` second pass; renderer tests incl. a class-spoofing case)
- [ ] `feat(frontend): Style lesson alerts with octicon icons` (alert styles in the `.lesson-content` section of `base.css`: accent border and title color per type family via theme tokens, octicons as `mask` data URIs colored by `currentColor`, nested alerts inset; closes #116)
- [ ] `fix(markdown): Skip a leading heading duplicating the lesson title` (static `stripLeadingTitleHeading`; lesson route applies it before `render()`; tests for ATX/setext, case and whitespace tolerance, non-matching heading kept; lesson form hint; consequence note in [ADR-0014](../adr/0014-demote-markdown-headings-in-lesson-rendering.md); closes #117)

## Verification

- `make test` and `./mvnw checkstyle:check` green after the upgrade commit and again at the end.
- Browser pass on a lesson exercising several alert types, nesting, and a custom title: correct icon, accent, and title in both themes; axe reports zero violations; no horizontal page scroll (RGAA 10.11).
- A lesson whose Markdown starts with `# <lesson title>` renders exactly one heading with that text.
16 changes: 15 additions & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@
<maven-checkstyle-plugin.version>3.6.0</maven-checkstyle-plugin.version>
<maven-javadoc-plugin.version>3.11.2</maven-javadoc-plugin.version>
<jacoco-maven-plugin.version>0.8.15</jacoco-maven-plugin.version>
<commonmark.version>0.24.0</commonmark.version>
<commonmark.version>0.29.0</commonmark.version>
<jsoup.version>1.21.1</jsoup.version>
</properties>

Expand Down Expand Up @@ -135,6 +135,20 @@
<artifactId>commonmark</artifactId>
<version>${commonmark.version}</version>
</dependency>
<!-- GFM pipe tables in lesson Markdown (extension added on demand,
as ADR-0013 planned) -->
<dependency>
<groupId>org.commonmark</groupId>
<artifactId>commonmark-ext-gfm-tables</artifactId>
<version>${commonmark.version}</version>
</dependency>
<!-- GFM/Obsidian-style alerts ("> [!NOTE]") in lesson Markdown;
needs commonmark >= 0.29.0 for custom types, titles, nesting -->
<dependency>
<groupId>org.commonmark</groupId>
<artifactId>commonmark-ext-gfm-alerts</artifactId>
<version>${commonmark.version}</version>
</dependency>
<dependency>
<groupId>org.jsoup</groupId>
<artifactId>jsoup</artifactId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,9 +122,11 @@ public String drop(@PathVariable Long courseId, Principal principal) {
/**
* Display one published lesson to an enrolled student, with the lesson
* Markdown rendered to sanitized HTML and previous/next links in
* reading order. A student who is not actively enrolled is sent back to
* the course page with an {@code enroll-required} hint instead of a 403:
* the Register button is right there.
* reading order. A leading heading that repeats the lesson title is
* skipped: the page already renders the title as its {@code h1}. A
* student who is not actively enrolled is sent back to the course page
* with an {@code enroll-required} hint instead of a 403: the Register
* button is right there.
*
* @param courseId the course the lesson belongs to
* @param lessonId the lesson to read
Expand Down Expand Up @@ -153,7 +155,9 @@ public String lesson(
int index = indexOf(lessons, lesson);
model.addAttribute("course", course);
model.addAttribute("lesson", lesson);
model.addAttribute("contentHtml", markdownRenderer.render(lesson.getContentMarkdown()));
model.addAttribute("contentHtml", markdownRenderer.render(
MarkdownRenderer.stripLeadingTitleHeading(
lesson.getContentMarkdown(), lesson.getTitle())));
model.addAttribute("previousLesson", index > 0 ? lessons.get(index - 1) : null);
model.addAttribute("nextLesson",
index < lessons.size() - 1 ? lessons.get(index + 1) : null);
Expand Down
Loading