Don't crash on malformed *.yaml; surface as a banner (#285)#665
Don't crash on malformed *.yaml; surface as a banner (#285)#665
Conversation
A YAML parse error in any `*.yaml` file used to throw `BadInput` from `patchModel`, killing the UnionMount change handler and stopping the live server from rendering anything (the browser saw "Unable to render template '/templates/error'"). The same path also aborted `emanote gen` before any HTML was produced. Log the error and leave the model unchanged for that route — previously parsed metadata stays in place, so the rest of the site keeps rendering while the user fixes the file. E2E coverage: a `[]: foo` fixture file plus a regression scenario that asserts the home page still renders. A user-visible "global errors" page (so the bad file is surfaced in the browser, not just in the console) is the proper follow-up — out of scope for this fix, which targets only the crash.
The previous regression assertion `text.includes("E2E Fixture")` coupled
to fixture content — if the fixture's H1 ever got renamed, the test
would silently stop guarding against the #285 crash. Replace with a
direct check for the bug's surface markers ("Ema App threw an exception"
and "Unable to render template"), which is what the user actually sees
when the Dynamic dies.
Also reframe the CHANGELOG entry: the proper fix (browser-visible global
errors page) stays as #285 follow-up per srid's comment on the issue,
so don't claim this PR closes it.
`text.length > 0` was a redundant guard on the empty-body case: a present-but-mostly-empty body (page chrome only, no article) trivially satisfies it, so the assertion didn't actually distinguish the bug state from a healthy one. The marker check on EMA_EXCEPTION_MARKERS is the real regression signal — keep that and drop the length guard.
Both comment blocks (Patch.hs YAML branch, smoke_steps.ts marker constant) drifted into narrating the bug's history rather than explaining the surprising behavior at the call site. Keep the WHY that's not obvious from the code — the "preserve last-known-good" choice and the inverse-signature assertion design — and drop the "used to crash, here's what the user saw" prose.
Hickey/Lowy Analysis
Hickey rationale#1 (pure id complects). #2 ( #3 (E2E coupled to fixture text). Fixed in commit d1c2c811. The original #4 (errors only in logs). Deferred to #285 per srid's comment on the issue: "I will implement the proper fix when I get to it." The CHANGELOG entry explicitly notes this as the follow-up. Lowy rationale#5 (boundary). The inline #6 (unnamed volatility axis). Lowy's prescription was to extract #7 ( |
Take the deferred follow-up across the line: parse errors no longer hide in the console. They now ride into the model as a `Map (R 'Yaml) Text` and render as a banner block at the top of every note while the offending file is broken — so a malformed yaml is loud-and-fixable instead of silent-and-stale. The patch handler drops the bad route's existing data on failure (rather than the previous "preserve last-known-good" workaround) and clears the error entry on a successful re-parse. `renderLmlHtml` prepends a Pandoc Div carrying the same `emanote:error` class the markdown errorDiv uses, so the visual surface is consistent across both error sources. The e2e regression flips from inverse-of-bug to a positive assertion: the banner header and the broken file's path must both appear.
srid
changed the title
Update — deferred items now doneFollowing user direction, the two findings previously marked Deferred [#285] are addressed in this PR:
Concretely, commit 32061c55 adds a Why a Map and not piggybacking on
|
|
| Step | Status | Duration | Verification |
|---|---|---|---|
| sync | ✓ | 0s | git fetch ok; forge=github; noGit=false |
| research | ✓ | 5m 36s | Read issue #285, comments, repro, Patch.hs:100-113 throws BadInput, Note.hs:_noteErrors as analog pattern. |
| branch | ✓ | 4s | On feature branch fix-285-malformed-yaml from origin/master. |
| implement | ✓ | 1m 28s | E2E fixture broken-285.yaml + regression scenario. Patch.hs YAML branch logs instead of throwing. |
| check | ✓ | 3s | cabal build all → Up to date. |
| docs | ✓ | 21s | CHANGELOG entry under 1.6.0.0 Unreleased / Bug fixes. |
| fmt | ✓ | 25s | 4 hooks (cabal-fmt, fourmolu, hlint, nixpkgs-fmt) all Passed. |
| commit | ✓ | 16s | Primary commit 269ef92 pushed. |
| hickey+lowy | ✓ | 5m 21s | Sub-agents ran in parallel. 7 findings; #3 fixed (d1c2c81); #1+#2+#5+#6 no-op; #4+#7 initially deferred, later folded into the expanded scope. |
| police | ✓ | 4m 12s | 3 passes: rules clean; fact-check fixed (dfe349e); elegance trimmed (8058faf). |
| test | ✓ | 1m 21s | Unit 43/43 + E2E live 13/13 + E2E static 13/13. |
| create-pr | ✓ | 1m 24s | Draft PR #665 + hickey/lowy ledger comment posted. |
| ci | ✓ | 10m 48s | vira ci signed off on 32061c5 (HEAD) for both vira/aarch64-darwin and vira/x86_64-linux. Cache disabled per branch policy. |
| done | ✓ | 4s | All steps completed. |
| Total | 33m 59s |
Slowest step: ci (10m 48s, 32% of total)
Optimization suggestions
- CI ran twice (once on the minimal-fix merge commit
906dd9d1, once on the expanded-scope commit32061c55) because scope grew mid-run. ~10 minutes were burned on the first CI pass that became obsolete. If the full scope is known upfront — surface yaml errors in the model, not just log them — naming that in the original/doprompt avoids the throw-away first build. research+hickey+lowytogether took 11 minutes and that's about as fast as it gets — issue Malformed YAML files break Emanote #285 has 13 comments and the parallel sub-agent invocation is already the optimal shape. No win to chase here.- For follow-up changes on this branch (e.g. addressing review comments),
/do --from ci-onlyskips straight to vira ci on the new HEAD — useful if a single-line fix doesn't need the full research/check/test/police pipeline re-run. - Cache was disabled because this isn't
master(pervira.hs). On master, the same CI pipeline benefits fromcache.nixos.asia/oss— expect this run to land much faster as an actual master push.
Workflow completed at 2026-04-25.
A bad `subfolder/index.yaml` only contributes meta to notes under `/subfolder/*`; before this commit its banner was shown globally, on every page, which obscured *where* the bad file was relevant. Now the banner uses the same `routeInits @'R.Yaml` cascade that `getEffectiveRouteMetaWith` walks — so the banner lands on the notes the bad yaml actually affects, and nowhere else. Pair the e2e fixture: `broken-285.md` sits next to `broken-285.yaml` so the regression scenario can land on `/broken-285.html` (where the banner belongs) instead of polluting every other test's view of the home page. Two scenarios now: one asserts the banner is present on the sibling note, one asserts it does *not* leak onto unrelated pages. Open question: a malformed *.yaml with no md sibling (the original pyyaml-in-notebook case) is now invisible to the user. Worth a follow-up — surface orphan errors on the root index as a fallback — but out of scope for this commit, which targets the scoping bug.
Drop the parallel `_modelDataErrors :: Map (R 'Yaml) Text` and fold
the failure case into SData itself: every loaded `*.yaml` file maps
to exactly one SData entry whose `_sdataValue` carries the parsed
content (or `Aeson.Null` on failure) and whose new `_sdataError ::
Maybe Text` carries the parse message.
`parseSDataCascading` no longer returns `Either` — it returns the
SData representing the parse outcome. Patch.hs collapses to a single
`modelInsertData`, no case match. The renderer walks the route's
yaml cascade via `routeInits` and gathers errors from each looked-up
SData, so the existing `getEffectiveRouteMetaWith` cascade pattern
is reused.
Net effect: one mapping ("yaml route → outcome") instead of two
parallel structures. Same observable behavior — the cascade still
filters errors to notes whose meta actually depends on the bad file.
| a successful parse populates `_sdataValue`; a failed parse leaves it | ||
| `Aeson.Null` and records the message in `_sdataError`. Modeling the |
There was a problem hiding this comment.
wtf, use Either Aeson.Value Text then.
There was a problem hiding this comment.
Done in 599d852. Went with Either Text Aeson.Value (error-on-Left, the conventional order) — parseSDataCascading is now a one-liner: SData (mergeAesons <$> traverse parseOne bs) r. Holler if you actually wanted the flipped Either Aeson.Value Text.
# Conflicts: # tests/features/smoke.feature
Per srid review: the previous shape — a `_sdataValue :: Aeson.Value` plus a sibling `_sdataError :: Maybe Text` — let invalid states exist (a value AND an error in the same record). `Either Text Aeson.Value` makes the parse outcome a sum type: success on the right, failure on the left, no overlap. `parseSDataCascading` collapses to a one-liner: `mergeAesons <$> traverse parseOne bs`, wrapped in SData with the route. Consumers: * `Meta.getYamlMeta` returns `rightToMaybe (s ^. sdataValue)` — same effect as the old `guard $ v /= Aeson.Null` filter, with the type doing the work. * `Patch.hs` logs via `whenLeft_`. * `Template.cascadeYamlErrors` collects via `leftToMaybe`.
srid
changed the title
Two findings, both for this PR: 1. Move `cascadeYamlErrors` from `View/Template.hs` to `Model/Meta.hs` alongside `getYamlMeta` / `getEffectiveRouteMetaWith`. Same shape (route → routeInits cascade → SData lookup), same volatility (changes when cascade rules or SData representation change). Was sitting in `Template.hs` only because it had one caller — that's colocation by use, not by change-reason. 2. Promote `errorDiv` from a where-clause inside `mkNoteWith` to a top-level shared builder in `Note.hs`, parameterized by the banner header text. Both error paths — per-note markdown errors and route-cascade yaml errors — now go through the same `B.Div (cls "emanote:error") ...` shape. The CSS class is the actual sync contract (a Heist splice rewrites it into Tailwind utilities); sharing the builder prevents the two surfaces from silently diverging when one gets tweaked.
1 participant
A YAML file with a non-string mapping key like
[]: fooused to throwBadInputfrompatchModel, killing the live server's UnionMount Dynamic and abortingemanote genbefore any HTML was produced. Parse outcome now lives inSData._sdataValue :: Either Text Aeson.Value— successes go right, errors go left, no exception. The renderer walks the samerouteInits @'R.YamlcascadegetEffectiveRouteMetaWithalready uses and prepends anemanote:errorPandoc Div, so a badsubfolder/index.yamlshows up as a banner on the notes under/subfolder/*and nowhere else.E2E coverage: a paired
broken-285.{md,yaml}fixture plus two scenarios — one asserts the banner is shown on the sibling note, the other asserts it does not leak onto unrelated pages.Closes #285.