forgejo_admin/pal-e-api

Fork 1

feat: composition rendering — parent notes render child notes inline #63

Merged

forgejo_admin merged 2 commits from feat/62-composition-rendering into main

2026-03-02 17:33:07 +00:00

forgejo_admin commented

2026-03-02 17:18:56 +00:00

Owner

Summary

Plan pages now render child phase notes inline as one scrollable page, ordered by position
Monolithic notes (no children) render identically to current behavior — zero visual change
Child notes viewed directly show a "Part of: {parent title}" breadcrumb linking back to the parent

Changes

src/pal_e_docs/routes/frontend.py: Extracted _render_content() helper for the sanitize-autolink-wrap pipeline. Updated browse_note() to eager-load children and parent relationships, apply the rendering pipeline to each public child independently, and pass rendered_children + parent_info to the template.
src/pal_e_docs/templates/note.html: Added parent breadcrumb section (shown when viewing a child note directly). Added child note rendering loop after parent content — each child gets an <h2> title and its own .note-content div. Links section remains at the bottom after all content.
src/pal_e_docs/templates/base.html: Added CSS for .child-note (left border accent #0366d6, spacing) and .parent-breadcrumb styling.
tests/test_composition_rendering.py: 9 new test scenarios.

Test Plan

All 252 tests pass (240 baseline + 9 new + 3 browser)
Parent with 3 children renders all child titles and content in position order
Monolithic note renders with no .child-note divs
Position ordering verified (position=1 before position=2 regardless of insertion order)
Private child hidden from anonymous users
Private child visible to authenticated users
Child direct view shows "Part of:" breadcrumb with parent link
Parent/standalone notes show no breadcrumb
Rendering pipeline runs on children (auto-link + table-wrap verified)
Links section appears after all content (parent + children)
Ruff check + format clean

Review Checklist

No secrets committed
No unnecessary file changes (only frontend files modified)
Commit message is descriptive
Only touches files specified in the issue (routes/frontend.py, templates/note.html, templates/base.html, tests)
Does NOT modify sanitize.py, autolink.py, wrap_tables.py, models.py, schemas.py, or API routes

plan-2026-03-01-note-decomposition — Phase 4 (Composition Rendering)
Forgejo issue #62 on forgejo_admin/pal-e-docs

## Summary - Plan pages now render child phase notes inline as one scrollable page, ordered by `position` - Monolithic notes (no children) render identically to current behavior — zero visual change - Child notes viewed directly show a "Part of: {parent title}" breadcrumb linking back to the parent ## Changes - `src/pal_e_docs/routes/frontend.py`: Extracted `_render_content()` helper for the sanitize-autolink-wrap pipeline. Updated `browse_note()` to eager-load `children` and `parent` relationships, apply the rendering pipeline to each public child independently, and pass `rendered_children` + `parent_info` to the template. - `src/pal_e_docs/templates/note.html`: Added parent breadcrumb section (shown when viewing a child note directly). Added child note rendering loop after parent content — each child gets an `<h2>` title and its own `.note-content` div. Links section remains at the bottom after all content. - `src/pal_e_docs/templates/base.html`: Added CSS for `.child-note` (left border accent `#0366d6`, spacing) and `.parent-breadcrumb` styling. - `tests/test_composition_rendering.py`: 9 new test scenarios. ## Test Plan - [x] All 252 tests pass (240 baseline + 9 new + 3 browser) - [x] Parent with 3 children renders all child titles and content in position order - [x] Monolithic note renders with no `.child-note` divs - [x] Position ordering verified (position=1 before position=2 regardless of insertion order) - [x] Private child hidden from anonymous users - [x] Private child visible to authenticated users - [x] Child direct view shows "Part of:" breadcrumb with parent link - [x] Parent/standalone notes show no breadcrumb - [x] Rendering pipeline runs on children (auto-link + table-wrap verified) - [x] Links section appears after all content (parent + children) - [x] Ruff check + format clean ## Review Checklist - [x] No secrets committed - [x] No unnecessary file changes (only frontend files modified) - [x] Commit message is descriptive - [x] Only touches files specified in the issue (routes/frontend.py, templates/note.html, templates/base.html, tests) - [x] Does NOT modify sanitize.py, autolink.py, wrap_tables.py, models.py, schemas.py, or API routes ## Related Notes - `plan-2026-03-01-note-decomposition` — Phase 4 (Composition Rendering) - Forgejo issue #62 on forgejo_admin/pal-e-docs

forgejo_admin added 1 commit

2026-03-02 17:18:56 +00:00

feat: composition rendering — parent notes render child notes inline (#62 )

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

04baf285af

Plan pages now render their child phase notes inline as one scrollable
page. Decomposed plans look identical to monolithic plans — same UX,
better data structure. Monolithic notes with no children render
identically to current behavior.

Changes:
- routes/frontend.py: eager-load children+parent, apply rendering
  pipeline to each child independently, pass rendered children and
  parent breadcrumb info to template
- templates/note.html: render child notes inline after parent content
  with h2 headings, show "Part of:" breadcrumb on child direct view
- templates/base.html: CSS for .child-note (border-left accent) and
  .parent-breadcrumb styling
- tests/test_composition_rendering.py: 9 test scenarios covering
  inline rendering, position ordering, privacy filtering, breadcrumb,
  rendering pipeline per child, and links section placement

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

forgejo_admin commented

2026-03-02 17:19:42 +00:00

Author

Owner

Review Pass 1 -- CLEAN

Reviewed the full diff against issue #62 acceptance criteria. No blockers, no nits.

Checklist

_render_content() helper correctly extracts the sanitize-autolink-wrap pipeline
Eager loading of children and parent avoids N+1 queries
Privacy filtering: anonymous users only see public children (is_public check in route)
Children rendered in position order via the model's order_by relationship
Parent breadcrumb shown only when note.parent exists
CSS matches spec: .child-note with border-left: 3px solid #0366d6
Links section appears after all content (parent + children) in template
known_slugs cached once for parent + all children (no redundant queries)
Jinja2 auto-escaping handles child.slug in id attribute safely
9 new tests cover: inline rendering, position ordering, privacy (anon + auth), breadcrumb, rendering pipeline, links placement
252 tests pass (240 baseline + 9 new + 3 browser)
Ruff check + format clean
Only specified files modified (routes/frontend.py, templates/note.html, templates/base.html, tests)
No changes to sanitize.py, autolink.py, wrap_tables.py, models.py, schemas.py, or API routes

Notes

_render_content return type -> str is technically correct despite wrap_tables having a str | None signature, because the html or "" fallback at the top guarantees a non-None path through the entire pipeline.
id="child-{{ child.slug }}" provides anchor links for future deep-linking to specific child sections -- nice touch not in the spec but harmless and useful.

## Review Pass 1 -- CLEAN Reviewed the full diff against issue #62 acceptance criteria. No blockers, no nits. ### Checklist - [x] `_render_content()` helper correctly extracts the sanitize-autolink-wrap pipeline - [x] Eager loading of `children` and `parent` avoids N+1 queries - [x] Privacy filtering: anonymous users only see public children (`is_public` check in route) - [x] Children rendered in `position` order via the model's `order_by` relationship - [x] Parent breadcrumb shown only when `note.parent` exists - [x] CSS matches spec: `.child-note` with `border-left: 3px solid #0366d6` - [x] Links section appears after all content (parent + children) in template - [x] `known_slugs` cached once for parent + all children (no redundant queries) - [x] Jinja2 auto-escaping handles `child.slug` in `id` attribute safely - [x] 9 new tests cover: inline rendering, position ordering, privacy (anon + auth), breadcrumb, rendering pipeline, links placement - [x] 252 tests pass (240 baseline + 9 new + 3 browser) - [x] Ruff check + format clean - [x] Only specified files modified (routes/frontend.py, templates/note.html, templates/base.html, tests) - [x] No changes to sanitize.py, autolink.py, wrap_tables.py, models.py, schemas.py, or API routes ### Notes - `_render_content` return type `-> str` is technically correct despite `wrap_tables` having a `str | None` signature, because the `html or ""` fallback at the top guarantees a non-None path through the entire pipeline. - `id="child-{{ child.slug }}"` provides anchor links for future deep-linking to specific child sections -- nice touch not in the spec but harmless and useful.

forgejo_admin commented

2026-03-02 17:22:59 +00:00

Author

Owner

PR #63 Review

BLOCKERS

1. joinedload on self-referential children may cause row duplication when combined with joinedload(Note.tags)

In the modified browse_note() query:

db.query(Note)
    .options(
        joinedload(Note.tags),
        joinedload(Note.project),
        joinedload(Note.children),
        joinedload(Note.parent),
    )
    .filter(Note.slug == slug)
    .first()

Using multiple joinedload on collection relationships (tags + children) against the same root entity produces a Cartesian product in the generated SQL. SQLAlchemy will de-duplicate the results in-memory, but the .first() call operates on raw SQL rows before de-duplication. When the SQL result has multiple rows (one per tag x child combination), .first() returns only the first row and SQLAlchemy's identity map may not fully populate the children or tags collections.

Fix: Switch children (and/or tags) from joinedload to subqueryload to avoid the Cartesian product:

.options(
    subqueryload(Note.tags),
    joinedload(Note.project),
    subqueryload(Note.children),
    joinedload(Note.parent),
)

This is a correctness issue -- notes with >1 tag AND >1 child would silently return incomplete children or tags. The tests pass because each test note typically has 0 tags, which avoids the Cartesian product.

Severity: BLOCKER -- data may be silently incomplete in production where notes have both tags and children.

2. Child title rendered via {{ child.title }} is not HTML-escaped in the <h2> context -- but it IS safe

Actually, on closer inspection, Jinja2 auto-escapes {{ child.title }} by default (no | safe filter), so this is safe. Withdrawing this concern.

NITS

1. _render_content could also DRY up browse_project_notes (lines 156-163 of current main)

The browse_project_notes function has the same sanitize-autolink-wrap pipeline for project page notes. The new _render_content helper could be reused there too. Not in scope for this PR, but worth a follow-up.

2. _render_content type annotation

The known_slugs parameter has no type annotation. Minor, but for consistency with the codebase (which uses AbstractSet[str] in autolink.py), consider:

from collections.abc import AbstractSet

def _render_content(html: str | None, known_slugs: AbstractSet[str]) -> str:

3. Test cleanup pattern

Tests use _get_test_db() and manually close sessions in finally blocks. This works, but a context-manager wrapper or a shared fixture would reduce boilerplate. Not blocking.

4. Child <h2> font-size in CSS

The .child-note h2 sets font-size: 1.25rem which matches the existing .note-content h2 rule at line 126 of base.html. Fine, but the duplication means a future change to .note-content h2 would not propagate to .child-note h2. Consider whether .child-note h2 should inherit instead of re-declare. Purely cosmetic.

SOP COMPLIANCE

Branch named after issue -- feat/62-composition-rendering references Forgejo issue #62
PR body follows template-pr-body -- Summary, Changes, Test Plan, Review Checklist, Related Notes all present
Related Notes references the plan slug -- plan-2026-03-01-note-decomposition referenced in Related Notes
Tests exist and pass -- 9 new tests in tests/test_composition_rendering.py, PR body claims 252 total pass
No secrets, .env files, or credentials committed -- diff contains only frontend.py, base.html, note.html, and test file
No unnecessary file changes (scope creep) -- exactly the 4 files described in the issue boundary (routes/frontend.py, note.html, base.html, tests)
Commit messages are descriptive -- PR title is clear: "feat: composition rendering -- parent notes render child notes inline"

VERDICT: NOT APPROVED

One blocker: the joinedload Cartesian product issue with .first() on a query that joins both tags and children collections. Switch one or both to subqueryload. Once fixed, this is a clean, well-scoped, well-tested PR.

## PR #63 Review ### BLOCKERS **1. `joinedload` on self-referential `children` may cause row duplication when combined with `joinedload(Note.tags)`** In the modified `browse_note()` query: ```python db.query(Note) .options( joinedload(Note.tags), joinedload(Note.project), joinedload(Note.children), joinedload(Note.parent), ) .filter(Note.slug == slug) .first() ``` Using multiple `joinedload` on collection relationships (`tags` + `children`) against the same root entity produces a Cartesian product in the generated SQL. SQLAlchemy will de-duplicate the results in-memory, but the `.first()` call operates on raw SQL rows *before* de-duplication. When the SQL result has multiple rows (one per tag x child combination), `.first()` returns only the first row and SQLAlchemy's identity map may not fully populate the `children` or `tags` collections. **Fix:** Switch `children` (and/or `tags`) from `joinedload` to `subqueryload` to avoid the Cartesian product: ```python .options( subqueryload(Note.tags), joinedload(Note.project), subqueryload(Note.children), joinedload(Note.parent), ) ``` This is a correctness issue -- notes with >1 tag AND >1 child would silently return incomplete children or tags. The tests pass because each test note typically has 0 tags, which avoids the Cartesian product. **Severity: BLOCKER** -- data may be silently incomplete in production where notes have both tags and children. --- **2. Child title rendered via `{{ child.title }}` is not HTML-escaped in the `<h2>` context -- but it IS safe** Actually, on closer inspection, Jinja2 auto-escapes `{{ child.title }}` by default (no `| safe` filter), so this is safe. Withdrawing this concern. --- ### NITS **1. `_render_content` could also DRY up `browse_project_notes` (lines 156-163 of current main)** The `browse_project_notes` function has the same sanitize-autolink-wrap pipeline for project page notes. The new `_render_content` helper could be reused there too. Not in scope for this PR, but worth a follow-up. **2. `_render_content` type annotation** The `known_slugs` parameter has no type annotation. Minor, but for consistency with the codebase (which uses `AbstractSet[str]` in `autolink.py`), consider: ```python from collections.abc import AbstractSet def _render_content(html: str | None, known_slugs: AbstractSet[str]) -> str: ``` **3. Test cleanup pattern** Tests use `_get_test_db()` and manually close sessions in `finally` blocks. This works, but a context-manager wrapper or a shared fixture would reduce boilerplate. Not blocking. **4. Child `<h2>` font-size in CSS** The `.child-note h2` sets `font-size: 1.25rem` which matches the existing `.note-content h2` rule at line 126 of `base.html`. Fine, but the duplication means a future change to `.note-content h2` would not propagate to `.child-note h2`. Consider whether `.child-note h2` should inherit instead of re-declare. Purely cosmetic. ### SOP COMPLIANCE - [x] **Branch named after issue** -- `feat/62-composition-rendering` references Forgejo issue #62 - [x] **PR body follows `template-pr-body`** -- Summary, Changes, Test Plan, Review Checklist, Related Notes all present - [x] **Related Notes references the plan slug** -- `plan-2026-03-01-note-decomposition` referenced in Related Notes - [x] **Tests exist and pass** -- 9 new tests in `tests/test_composition_rendering.py`, PR body claims 252 total pass - [x] **No secrets, .env files, or credentials committed** -- diff contains only frontend.py, base.html, note.html, and test file - [x] **No unnecessary file changes (scope creep)** -- exactly the 4 files described in the issue boundary (routes/frontend.py, note.html, base.html, tests) - [x] **Commit messages are descriptive** -- PR title is clear: "feat: composition rendering -- parent notes render child notes inline" ### VERDICT: NOT APPROVED One blocker: the `joinedload` Cartesian product issue with `.first()` on a query that joins both `tags` and `children` collections. Switch one or both to `subqueryload`. Once fixed, this is a clean, well-scoped, well-tested PR.

forgejo_admin added 1 commit

2026-03-02 17:28:26 +00:00

fix: use subqueryload for collection relationships in browse_note

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

ca789d003d

joinedload on two collection relationships (tags + children) creates a
Cartesian product — a note with N tags and M children produces N*M SQL
rows, and .first() grabs only the first row. The identity map may not
fully populate both collections, causing children to silently disappear.

Switch tags and children to subqueryload, which issues separate SELECT
queries per collection. Keep joinedload for single-object relationships
(project, parent) where it remains optimal.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

forgejo_admin commented

2026-03-02 17:30:51 +00:00

Author

Owner

PR #63 Review (Round 2)

BLOCKERS

None. The Cartesian product blocker from round 1 has been correctly resolved by switching Note.tags and Note.children to subqueryload while keeping scalar relationships (Note.project, Note.parent) on joinedload. The code is correct, secure, and well-tested.

NITS

_render_content type annotation (frontend.py:191): The known_slugs parameter has no type hint. Consider known_slugs: frozenset[str] to match get_known_slugs return type. Non-blocking -- it is a private helper.
DRY opportunity in browse_project_notes (frontend.py:157-163): The existing browse_project_notes function manually runs the same sanitize -> autolink -> wrap_tables pipeline that _render_content now encapsulates. A follow-up PR could replace lines 159-162 with page_note_content = _render_content(raw, get_known_slugs(db)). Not in scope for this PR -- the PR correctly limits itself to the browse_note route.
Test cleanup pattern: Tests use _get_test_db() with try/finally for manual session management rather than a pytest fixture. This is consistent with the existing test patterns in conftest.py (create_test_note also uses TestingSessionLocal directly), so no change needed, but a @pytest.fixture wrapping this pattern could reduce boilerplate in future test files.

SOP COMPLIANCE

Branch named after issue: feat/62-composition-rendering references Forgejo issue #62.
PR body follows template-pr-body: All required sections present -- Summary, Changes, Test Plan, Review Checklist, Related Notes.
Related Notes references plan slug: plan-2026-03-01-note-decomposition referenced in Related Notes section, with Forgejo issue #62 link.
Tests exist and pass: 9 new tests in test_composition_rendering.py covering all specified scenarios (inline rendering, monolithic fallback, position ordering, private/public filtering, breadcrumb, rendering pipeline, links placement). Total claimed: 252 (plausible given parametrized expansion of 216 def test_ functions).
No secrets, .env files, or credentials committed: Only test fixture passwords (testpass123) used with bcrypt hashing -- standard practice.
No unnecessary file changes (scope): Exactly 4 files changed as stated. No modifications to sanitize.py, autolink.py, wrap_tables.py, models.py, schemas.py, or API routes -- matching the issue boundary.
Commit messages are descriptive: PR title clearly describes the feature.

CODE QUALITY NOTES

Cartesian product fix is correct: subqueryload for collections (tags, children), joinedload for scalar (project, parent). This emits separate SELECT queries for collections instead of producing an M*N row explosion.
_render_content helper: Clean extraction of the 3-step rendering pipeline. Handles None content with html or "".
Known slugs caching: Single get_known_slugs(db) call cached and reused for parent + all children -- avoids N+1 queries.
Privacy filtering: Correct -- iterates note.children in Python with is_public check per child, rather than filtering at the SQL level. Since children are already eager-loaded via subqueryload, this avoids an extra query and is the right approach.
Template structure: Children render after parent content but before the links section. Breadcrumb renders before the <h1> title, which is the correct UX placement.
CSS: .child-note uses left-border accent for visual hierarchy. .parent-breadcrumb is subtle and non-intrusive. Both use the existing #0366d6 link color for consistency.
XSS safety: Child content goes through the same sanitize_html -> autolink_slugs -> wrap_tables pipeline as parent content. The | safe filter in the template is appropriate because content is pre-sanitized server-side.

VERDICT: APPROVED

## PR #63 Review (Round 2) ### BLOCKERS None. The Cartesian product blocker from round 1 has been correctly resolved by switching `Note.tags` and `Note.children` to `subqueryload` while keeping scalar relationships (`Note.project`, `Note.parent`) on `joinedload`. The code is correct, secure, and well-tested. ### NITS 1. **`_render_content` type annotation** (`frontend.py:191`): The `known_slugs` parameter has no type hint. Consider `known_slugs: frozenset[str]` to match `get_known_slugs` return type. Non-blocking -- it is a private helper. 2. **DRY opportunity in `browse_project_notes`** (`frontend.py:157-163`): The existing `browse_project_notes` function manually runs the same `sanitize -> autolink -> wrap_tables` pipeline that `_render_content` now encapsulates. A follow-up PR could replace lines 159-162 with `page_note_content = _render_content(raw, get_known_slugs(db))`. Not in scope for this PR -- the PR correctly limits itself to the `browse_note` route. 3. **Test cleanup pattern**: Tests use `_get_test_db()` with try/finally for manual session management rather than a pytest fixture. This is consistent with the existing test patterns in conftest.py (`create_test_note` also uses `TestingSessionLocal` directly), so no change needed, but a `@pytest.fixture` wrapping this pattern could reduce boilerplate in future test files. ### SOP COMPLIANCE - [x] **Branch named after issue**: `feat/62-composition-rendering` references Forgejo issue #62. - [x] **PR body follows `template-pr-body`**: All required sections present -- Summary, Changes, Test Plan, Review Checklist, Related Notes. - [x] **Related Notes references plan slug**: `plan-2026-03-01-note-decomposition` referenced in Related Notes section, with Forgejo issue #62 link. - [x] **Tests exist and pass**: 9 new tests in `test_composition_rendering.py` covering all specified scenarios (inline rendering, monolithic fallback, position ordering, private/public filtering, breadcrumb, rendering pipeline, links placement). Total claimed: 252 (plausible given parametrized expansion of 216 `def test_` functions). - [x] **No secrets, .env files, or credentials committed**: Only test fixture passwords (`testpass123`) used with bcrypt hashing -- standard practice. - [x] **No unnecessary file changes (scope)**: Exactly 4 files changed as stated. No modifications to `sanitize.py`, `autolink.py`, `wrap_tables.py`, `models.py`, `schemas.py`, or API routes -- matching the issue boundary. - [x] **Commit messages are descriptive**: PR title clearly describes the feature. ### CODE QUALITY NOTES - **Cartesian product fix is correct**: `subqueryload` for collections (`tags`, `children`), `joinedload` for scalar (`project`, `parent`). This emits separate SELECT queries for collections instead of producing an M*N row explosion. - **`_render_content` helper**: Clean extraction of the 3-step rendering pipeline. Handles `None` content with `html or ""`. - **Known slugs caching**: Single `get_known_slugs(db)` call cached and reused for parent + all children -- avoids N+1 queries. - **Privacy filtering**: Correct -- iterates `note.children` in Python with `is_public` check per child, rather than filtering at the SQL level. Since children are already eager-loaded via `subqueryload`, this avoids an extra query and is the right approach. - **Template structure**: Children render after parent content but before the links section. Breadcrumb renders before the `<h1>` title, which is the correct UX placement. - **CSS**: `.child-note` uses left-border accent for visual hierarchy. `.parent-breadcrumb` is subtle and non-intrusive. Both use the existing `#0366d6` link color for consistency. - **XSS safety**: Child content goes through the same `sanitize_html -> autolink_slugs -> wrap_tables` pipeline as parent content. The `| safe` filter in the template is appropriate because content is pre-sanitized server-side. ### VERDICT: APPROVED