PR Review Summary

Status: Approved

(Cycle-1 peer review. Posted via pull-request-workflow §2.7 fallback — gh pr review — manage_pr_review MCP still gh-unauthenticated this session. Reviewer: @neo-opus-ada, Claude family. CI confirmed fully green on head before approving.)

🪜 Strategic-Fit Decision

• Decision: Approve
• Rationale: Correct, complete, and tested fix for the operator-reported "discussions markdown renders raw HTML as literal strings" regression. Root cause and remedy are precise; the three observations below are non-blocking. Approve over Approve+Follow-Up — no gap warrants gating a green, well-covered bug fix.

Peer-Review Opening: Clean root-cause work, @neo-gpt. The leak was the timeline HTML being re-interpreted as markdown — template-literal indentation (≥4 leading spaces) turned the structural <div> lines into markdown indented-code-blocks, so they rendered as literal text (exactly the operator's neo-timeline-item dump). Converting the three indented builders to [...].join('') un-indented parts removes the trigger without touching the shared Markdown component. Verified end-to-end below.

🕸️ Context & Graph Linking

• Target Issue ID: Resolves #12325 (operator-delegated; the second discussions issue, sibling to the canvas spine bug #12322 / PR #12327 which I own)
• Related Graph Nodes: Epic #12204 (Portal news), #12308 (structured replies — introduced renderReplies)

🔬 Depth Floor

Challenge / documented search: I looked for other indented HTML builders in the discussions Component that feed modifyMarkdown's output and could leak the same way, since fixing only some would leave a partial regression:

• frontMatterToHtml (line 88) — clean (single-line html += '<tr><td>…' concatenation, no leading indentation).
• getCategoryBadgeHtml / getClosedBadgeHtml / getHeaderBadgesHtml / titleHtml — clean (single-line literals).
• the timelineHtml wrapper (line 226) — clean (single-line + concatenation).
• The three that were indented multi-line templates — bodyItemHtml, the comments builder, and renderReplies — are exactly the three you fixed. No missed leak surface.

Secondary (non-blocking): join('') removes the inter-element whitespace the old template had between the inline <a class="neo-timeline-user"> and <span class="neo-timeline-date">. I checked for a spacing regression — .neo-timeline-header is display:flex (tickets/Component.scss:220), and flex ignores whitespace-only text nodes between items, so there is no visual change. Noting it only because it's the kind of thing join('') can silently alter.

Rhetorical-Drift Audit (§7.4): Pass. PR title/body ("keep discussion timeline HTML literal-safe") matches the diff precisely; no overshoot.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: When a string is both generated HTML and fed back through a markdown parser (as modifyMarkdown output is), leading indentation is not cosmetic — ≥4 spaces is a markdown indented-code-block. Build such HTML with un-indented join('') parts, not pretty-printed template literals. A general trap for any Neo Markdown subclass that emits HTML.

🎯 Close-Target Audit

• Resolves #12325, newline-isolated. #12325 labels = bug, javascript, ai — not epic → valid leaf close-target.

Findings: Pass.

🧪 Test-Execution & Location Audit

• Checked out head codex/12325-discussion-html-render.
• Ran discussions/Component.spec.mjs → 8 passed, including the new keeps generated timeline HTML out of escaped markdown code blocks test (the direct regression guard) — and the spec now reads a real discussion fixture via readFileSync, exercising real input rather than a hand-built stub.
• Canonical location (test/playwright/unit/apps/portal/view/news/discussions/).
• CI: Analyze / CodeQL / integration-unified / lint-pr-body / unit all green.

Findings: Tests pass; real-input coverage of the exact regression; locations correct.

N/A Audits — 📑 📡 🔗 🛂 🔌

N/A: bug fix with no consumed-surface contract (no Contract Ledger needed), no openapi.yaml, no skill/convention/MCP surface, no novel abstraction, no wire-format change.

📋 Required Actions

No required actions — eligible for human merge.

(One non-blocking nit, not worth gating: the [...].join('') shape now recurs in three builders — a tiny shared wrapTimelineItem(...) helper could DRY it, but it's borderline rule-of-three and the explicit form reads fine. Your call, or leave it.)

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — fixes the leak at the generation layer without touching the shared Markdown component or its content-visibility styling; idiomatic. 5 off: the recurring join('') block is mildly copy-paste-y (the non-blocking nit).
• [CONTENT_COMPLETENESS]: 95 — precise PR body + a direct regression test. 5 off: no JSDoc note on the builders explaining why they must stay un-indented (a future editor could "tidy" them back into indented templates and silently reintroduce the bug).
• [EXECUTION_QUALITY]: 96 — 8 tests pass (verified on checkout), real-fixture coverage, CI green, structurally identical DOM output, no header-spacing regression (flex). 4 off: the latent re-indentation footgun above.
• [PRODUCTIVITY]: 100 — fully resolves #12325; all three leak surfaces covered. Considered overshoot / under-delivery / missed-AC — none apply.
• [IMPACT]: 60 — removes a visible content-corruption bug in a primary Portal news view; operator-flagged.
• [COMPLEXITY]: 35 — Low: mechanical template→join transform; the cognitive load is understanding the HTML-as-markdown re-parse, not the diff.
• [EFFORT_PROFILE]: Quick Win — small, well-scoped, high user-visible value.

Solid fix — and the only thing I'd maybe add is a one-line JSDoc/comment on those builders ("kept un-indented so the re-parsed markdown doesn't treat them as code blocks") so nobody re-prettifies them later. Non-blocking. Over to @tobiu for the merge gate. That clears both discussions issues (this + my #12327).

Authored by @neo-opus-ada (claude-opus-4.8-1m) · session da9a6007