Allow frontmatter in included files.#228
Conversation
This is to later allow local frontmatter in included files Has the side effect of removing cli options, which is I think better: a file is more self-contained this way!
Even though we do nothing with it now
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (4)
📝 WalkthroughWalkthroughThis change enables frontmatter in included files and refactors frontmatter into a unified Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/compiler/frontmatter.ml (1)
382-387:⚠️ Potential issue | 🔴 Critical | 💤 Low valueFix frontmatter opening delimiter detection for CRLF line endings.
The logic at lines 384-386 has a critical bug for
"---\r\n"prefixes. Sinces.[4]is always'\n'in that case, the condition will always be true, causing the function to always returnSome 3instead of the correct offset. This breaks frontmatter extraction on Windows systems using CRLF line endings.The issue stems from checking
s.[4]unconditionally despite the prefix length being different:
- For
"---\n"(4 bytes):s.[4]is the first content character- For
"---\r\n"(5 bytes):s.[4]is always'\n'from the prefix, not contentConsider using prefix-aware logic or checking the appropriate position based on the matched prefix length.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/compiler/frontmatter.ml` around lines 382 - 387, The function find_opening incorrectly checks s.[4] regardless of which prefix matched; change the logic to test the CRLF prefix first and return the correct offset for each prefix (i.e., if String.starts_with ~prefix:"---\r\n" then return Some 4, else if String.starts_with ~prefix:"---\n" then return Some 3), ensuring the check is prefix-aware and avoids indexing the wrong byte; update the function find_opening to match the longer prefix first and return the appropriate offset for the matched prefix.
🧹 Nitpick comments (3)
docs/frontmatter.rst (1)
53-55: ⚡ Quick winClarify precedence for conflicting single-value options.
Line 53–55 explains combine vs warning, but not which value wins on conflict. Please state precedence explicitly (and mention list options are appended) to prevent ambiguity in multifile setups.
Proposed doc tweak
-When you split your presentation in multiple files, you can define a frontmatter -in the included files. All options can be duplicated, and will be combined when -applicable. When not applicable, a warning will be raised. -``toplevel-attributes`` refers to the included file. +When you split your presentation in multiple files, you can define frontmatter +in included files. + +Compatible options are combined (for example, ``css``, ``js``, and +``external-ids`` are appended). For single-value options (for example, +``theme`` or ``dimension``), conflicting values raise a warning and the value +already defined in the including file is kept. + +``toplevel-attributes`` always refers to the included file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/frontmatter.rst` around lines 53 - 55, Update the paragraph around the "toplevel-attributes" description to explicitly state precedence: for conflicting single-value options the value from the last-included file (i.e., the later include) overrides earlier values, list-valued options are appended/concatenated across included files, and a warning is still emitted when a single-value option is overwritten to make conflict visibility explicit.src/compiler/cmarkit_proxy.ml (1)
1-19: ⚡ Quick winConsider renaming the private
Cmarkit.Doc.of_stringwrapper to avoid name shadowing.Having two
let of_stringbindings in the same file — where the second shadows the first and the internal call on line 18 silently resolves to the first — is non-obvious. Re-ordering the bindings, wrapping either inlet rec, or renaming only one would silently break the cross-call without a compile error.A simple rename makes the intent explicit:
♻️ Proposed rename
-let of_string ?loc_offset ~file = +let parse_doc ?loc_offset ~file = let locs = Option.is_some file in Cmarkit.Doc.of_string ~heading_auto_ids:false ~strict:false ~locs ?loc_offset ?file let of_string ~read_file ~file s = ... - let doc = of_string ~loc_offset ~file s in + let doc = parse_doc ~loc_offset ~file s in (doc, frontmatter)All external call sites already use the new
~read_file ~filesignature, so this refactor is safe.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/compiler/cmarkit_proxy.ml` around lines 1 - 19, The private wrapper Cmarkit.Doc.of_string called via the local let of_string is being shadowed by the public of_string ~read_file ~file binding; rename the private wrapper (e.g., to of_string_raw or cmarkit_of_string) so the cross-call is explicit, update the internal call site that currently does of_string ~loc_offset ~file s to call the new name, and leave the public of_string ~read_file ~file signature unchanged so external call sites aren’t affected.src/compiler/compile.ml (1)
316-321: 💤 Low valueIncorrect parameter name in
inlinefunction signature.The first parameter should be named
m(for mapper) to be consistent with the Fold_mapper API and other functions in this module. Currently it's namedi, which shadows the intended purpose.♻️ Suggested fix for consistency
- let inline i fm = function + let inline m fm = function | Inline.Image img as inl -> ( - match handle_image_inlining i fm defs current_path img with - | `Default -> Ast.Fold_mapper.default.inline i fm inl + match handle_image_inlining m fm defs current_path img with + | `Default -> Ast.Fold_mapper.default.inline m fm inl | `Return (fm, i) -> (fm, Some i)) - | inl -> Ast.Fold_mapper.default.inline i fm inl + | inl -> Ast.Fold_mapper.default.inline m fm inl🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/compiler/compile.ml` around lines 316 - 321, The inline function's first parameter is misnamed `i` and should be `m` to match the Fold_mapper API and other functions; rename the parameter in the inline function signature (and all its occurrences inside the function) from `i` to `m`, keeping the match arms and calls to handle_image_inlining and Ast.Fold_mapper.default.inline unchanged so that the mapper argument is correctly passed through.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@vendor/github.com/panglesd/cmarkit/src/cmarkit.ml`:
- Around line 3860-3880: The fold_map_doc function double-processes definitions
because it folds the whole Doc.block and then iterates Doc.defs; fix by
processing Doc.defs first (use Label.Map.fold with fold_map_def to update acc
and build new defs), then remove any Ext_footnote_definition /
Ext_attribute_definition nodes from the block before calling m.block so they are
not folded a second time. Concretely, in fold_map_doc call Label.Map.fold on
(Doc.defs d) using fold_map_def to produce (acc, defs), build a cleaned_block by
filtering out Block.Footnote.Def and Block.Attribute_definition.Def nodes from
(Doc.block d), then call m.block m acc cleaned_block and finally return acc, { d
with block = Option.value ~default:Block.empty block; defs } (using the same
symbols: fold_map_doc, fold_map_def, Doc.block, Doc.defs, Block.Footnote.Def,
Block.Attribute_definition.Def, m.block).
- Around line 3793-3810: The record-update uses invalid qualified field
shorthand in the Block.Ext_table branch and similarly for list items; replace `{
t with Block.Table.rows }` with an explicit assignment `{ t with
Block.Table.rows = rows }` in the Block.Ext_table handling (where t and rows are
in scope) and likewise replace `{ i with Block.List_item.block }` with `{ i with
Block.List_item.block = block }` in the list-item branch (using the local i and
block variables) so the qualified field updates are valid OCaml syntax.
- Around line 3824-3838: The pattern matching branch handling Block.List uses an
invalid qualified record update `{ i with Block.List_item.block }`; in the
map_item function you should assign the field explicitly by setting the
qualified field to the local value returned by m.block (i.e. use
`Block.List_item.block = block` in the record update). Locate the Block.List
branch and the map_item function that calls `m.block m acc
(Block.List_item.block i)` and change the record update so the qualified field
name is followed by `= block`, ensuring the Option.map constructs the updated
list item correctly.
---
Outside diff comments:
In `@src/compiler/frontmatter.ml`:
- Around line 382-387: The function find_opening incorrectly checks s.[4]
regardless of which prefix matched; change the logic to test the CRLF prefix
first and return the correct offset for each prefix (i.e., if String.starts_with
~prefix:"---\r\n" then return Some 4, else if String.starts_with ~prefix:"---\n"
then return Some 3), ensuring the check is prefix-aware and avoids indexing the
wrong byte; update the function find_opening to match the longer prefix first
and return the appropriate offset for the matched prefix.
---
Nitpick comments:
In `@docs/frontmatter.rst`:
- Around line 53-55: Update the paragraph around the "toplevel-attributes"
description to explicitly state precedence: for conflicting single-value options
the value from the last-included file (i.e., the later include) overrides
earlier values, list-valued options are appended/concatenated across included
files, and a warning is still emitted when a single-value option is overwritten
to make conflict visibility explicit.
In `@src/compiler/cmarkit_proxy.ml`:
- Around line 1-19: The private wrapper Cmarkit.Doc.of_string called via the
local let of_string is being shadowed by the public of_string ~read_file ~file
binding; rename the private wrapper (e.g., to of_string_raw or
cmarkit_of_string) so the cross-call is explicit, update the internal call site
that currently does of_string ~loc_offset ~file s to call the new name, and
leave the public of_string ~read_file ~file signature unchanged so external call
sites aren’t affected.
In `@src/compiler/compile.ml`:
- Around line 316-321: The inline function's first parameter is misnamed `i` and
should be `m` to match the Fold_mapper API and other functions; rename the
parameter in the inline function signature (and all its occurrences inside the
function) from `i` to `m`, keeping the match arms and calls to
handle_image_inlining and Ast.Fold_mapper.default.inline unchanged so that the
mapper argument is correctly passed through.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4ffc0848-2968-43e3-9127-1da24cb12aec
⛔ Files ignored due to path filters (1)
test/compiler/multi-frontmatter.t/chapter2/image_of_chapter_2.pngis excluded by!**/*.png
📒 Files selected for processing (32)
CHANGELOG.mddocs/doc-repl/main.mldocs/frontmatter.rstsrc/cli/main.mlsrc/cli/run.mlsrc/cli/run.mlisrc/cm_plugin/slipshow_communication.mlsrc/cm_plugin/slipshow_communication.mlisrc/compiler/ast.mlsrc/compiler/cmarkit_proxy.mlsrc/compiler/compile.mlsrc/compiler/compile.mlisrc/compiler/frontmatter.mlsrc/compiler/frontmatter.mlisrc/compiler/slipshow.mlsrc/compiler/slipshow.mlisrc/diagnosis/diagnosis.mlsrc/diagnosis/diagnosis.mlisrc/engine/previewer/previewer.mlsrc/engine/previewer/previewer.mlitest/compiler/dimension.t/run.ttest/compiler/multi-frontmatter.t/chapter1.mdtest/compiler/multi-frontmatter.t/chapter2/chapter2.mdtest/compiler/multi-frontmatter.t/chapter2/parts/part1.mdtest/compiler/multi-frontmatter.t/chapter2/parts/part2.mdtest/compiler/multi-frontmatter.t/main.mdtest/compiler/multi-frontmatter.t/run.ttest/compiler/simple.t/run.ttest/compiler/theme.t/run.ttest/compiler/warnings.t/run.tvendor/github.com/panglesd/cmarkit/src/cmarkit.mlvendor/github.com/panglesd/cmarkit/src/cmarkit.mli
💤 Files with no reviewable changes (1)
- test/compiler/simple.t/run.t
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/compiler/frontmatter.ml (2)
204-204: 💤 Low valueSlightly fragile:
of_string ~to_asset:()only works whileto_assetstays unused inDimension.of_string.
let of_string' = of_string ~to_asset:()type-checks today only becauseDimension.of_stringmatches~to_asset:_and stays polymorphic in that argument; the moment someone actually usesto_asset(turning it intostring -> Asset.t), this line silently breaks compilation in a confusing way. A small comment explaining the intent, or a no-op default like~to_asset:(fun _ -> assert false)(orFun.idifAsset.t = string), makes the fragility explicit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/compiler/frontmatter.ml` at line 204, The binding let of_string' = of_string ~to_asset:() is fragile because it relies on Dimension.of_string remaining polymorphic in ~to_asset; change it to explicitly provide a no-op placeholder for the to_asset argument (e.g. ~to_asset:(fun _ -> assert false) or another sentinel) or add a clarifying comment, so the intent is explicit and the code fails clearly if to_asset becomes used; update the occurrence of of_string' and call site expectations around of_string and Dimension.of_string accordingly.
113-119: 💤 Low valueDesign inconsistency:
toplevel-attributesmerges silently while other duplicate fields warn.Unlike Global option fields (e.g.,
math-link,theme) that emitInconsistentOptiondiagnostics when encountered multiple times with conflicting values viacombine_opt,Toplevel_attributes.update_frontmattersilently merges viaCmarkit.Attributes.mergewith no diagnostic. This is visible in the commit "Raise warnings in case of duplicated frontmatter fields" which added warnings for other field conflicts, yet attributes bypass this.The merge behavior is technically not a "silent override"—conflicting keys coexist in the attribute list with the newer value prepended and found first—but no collision warning is issued. This may be intentional given that attributes are inherently mergeable (unlike scalar options), but it creates an inconsistency with the stated goal of warning on duplicated fields.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/compiler/frontmatter.ml` around lines 113 - 119, The update_frontmatter function currently merges attributes with Cmarkit.Attributes.merge without emitting a diagnostic for duplicate keys; change update_frontmatter (in Toplevel_attributes) to detect overlapping attribute keys between the existing toplevel_attributes (a) and the incoming v, and when conflicts exist emit the same InconsistentOption diagnostic used by combine_opt (or the module's diagnostic constructor) before performing the merge; keep using Cmarkit.Attributes.merge to produce the combined list but ensure you call the diagnostics API with a clear message and context (same style as combine_opt) referencing the conflicting key(s).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/compiler/multi-frontmatter.t/run.t`:
- Around line 20-21: The test currently asserts the exact concatenated
stylesheet string by running show_source main.html | grep "rel=\"stylesheet\""
which is fragile; change it to check for each stylesheet independently (e.g.,
assert presence of file.css and file2.css separately) by replacing the single
grep with two independent assertions that each look for the corresponding
href/rel pair (or grep for href="file.css" and href="file2.css" individually) so
the test no longer depends on the renderer's whitespace/ordering in main.html.
---
Nitpick comments:
In `@src/compiler/frontmatter.ml`:
- Line 204: The binding let of_string' = of_string ~to_asset:() is fragile
because it relies on Dimension.of_string remaining polymorphic in ~to_asset;
change it to explicitly provide a no-op placeholder for the to_asset argument
(e.g. ~to_asset:(fun _ -> assert false) or another sentinel) or add a clarifying
comment, so the intent is explicit and the code fails clearly if to_asset
becomes used; update the occurrence of of_string' and call site expectations
around of_string and Dimension.of_string accordingly.
- Around line 113-119: The update_frontmatter function currently merges
attributes with Cmarkit.Attributes.merge without emitting a diagnostic for
duplicate keys; change update_frontmatter (in Toplevel_attributes) to detect
overlapping attribute keys between the existing toplevel_attributes (a) and the
incoming v, and when conflicts exist emit the same InconsistentOption diagnostic
used by combine_opt (or the module's diagnostic constructor) before performing
the merge; keep using Cmarkit.Attributes.merge to produce the combined list but
ensure you call the diagnostics API with a clear message and context (same style
as combine_opt) referencing the conflicting key(s).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3416f9e3-838f-4289-be61-ea27e6e69f1b
📒 Files selected for processing (2)
src/compiler/frontmatter.mltest/compiler/multi-frontmatter.t/run.t
1 participant
Toplevel-attributes apply to the included file. Otherwise, fields are combined when applicable (and raise a warning when not).
Summary by CodeRabbit
New Features
Bug Fixes / Warnings
Documentation
Tests