Add support for HTML layouts with Snippet, Slots, and a default text processor#53
Conversation
… text processor, closes #52
📝 WalkthroughWalkthroughAdds package resource loading and a string-to-layout conversion path to support plain HTML layouts, propagates a text-to-layout converter through App/API construction, updates typing/typing organization, adds utilities and examples for HTML multi-slot and default-slot layouts, plus tests and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant App as App / Router Builder
participant FS as Package Resources
participant Transformer as str_to_layout / TextProcessor
participant Renderer as Renderer
Client->>App: Request route (GET /path)
App->>App: Locate layout (check for layout.py)
alt Python layout exists
App->>Renderer: use Python layout_definition
else HTML layout fallback
App->>FS: import_resource("layout.html")
FS-->>App: HTML text (or None)
App->>Transformer: str_to_layout(HTML text)
Transformer-->>App: LayoutDefinition (with slots/text processing)
App->>Renderer: use LayoutDefinition
end
Renderer->>Transformer: process(content, context)
Transformer-->>Renderer: formatted slots / HTML
Renderer-->>Client: HTML response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Failure to add the new IP will result in interrupted reviews. 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pyproject.toml`:
- Line 10: Update the pyproject dependency from "htmy[lxml]>=0.10.2" to
"htmy[lxml]>=0.10.1" and then audit type annotations that reference specific
html tag objects (e.g., usages like html.div, html.form) across the codebase:
replace those annotations with htmy-provided types such as ComponentType or
Component (imported from htmy) where needed, and run type checks to ensure
compatibility with htmy v0.10.0+ breaking change.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@examples/html-layout/my_app/layout.html`:
- Line 25: Update the hard-coded copyright year in the layout node that
currently reads "<p>© 2025 My App</p>" to the current year (change 2025 to
2026), or replace it with a dynamic year expression if your templating engine
supports it so it auto-updates in future releases.
In `@holm/_model.py`:
- Around line 178-184: The import_resource method should early-return None when
the application has no package root: add a check at the start of import_resource
that if self.package_name is in _no_app_package_roots then return None; this
mirrors import_module's handling (see import_module at line ~143), prevents
importlib.resources.read_text from raising ValueError for non-package apps, and
keeps behavior consistent with app.py's fallback flow.
🧹 Nitpick comments (1)
holm/_model.py (1)
185-195: Broad exception handling is acceptable but consider narrowing.The static analysis flags the broad
except Exception(BLE001). While defensive programming is reasonable for resource loading, consider whether you can enumerate more specific exceptions thatimportlib.resources.read_textmight raise (e.g.,TypeError,OSError).The
TRY003hint about the long error message is a minor style concern—the current message is helpful for users encountering this error.Optional: Narrow exception types
- except Exception: + except (TypeError, OSError, AttributeError) as e: import traceback logger.warning(f"Failed to load resource {filename} from package '{package_name}'.") logger.warning(traceback.format_exc()) return None
…tter resource import error message
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@holm/modules/_layout.py`:
- Around line 69-91: The html_to_layout function currently passes raw HTML to
default_text_processor which uses Python's str.format and will break on literal
braces; fix by ensuring html_to_layout uses a safe processor: implement a small
safe_text_processor that first escapes literal braces (replace "{" -> "{{" and
"}" -> "}}") then delegates to default_text_processor (or returns the text
unchanged), and call that processor when building the Snippet in html_to_layout
(referencing html_to_layout, default_text_processor, text_processor, Snippet,
and Text); alternatively update the default_text_processor itself to be
brace-safe so html_to_layout can keep its signature.
♻️ Duplicate comments (1)
holm/_model.py (1)
163-184: Non‑package apps will crash onlayout.htmlloading.
import_resourceraisesValueErrorfor_no_app_package_roots, butAppConfigexplicitly supports non‑packaged apps andapp.pynow calls this forlayout.html. That path will raise at startup whenlayout.pyis absent. Please mirrorimport_moduleand returnNone(and update the docstring) to keep graceful fallback.🔧 Suggested fix
- if package_name in _no_app_package_roots: - raise ValueError( - f"The resource ({filename}) you are trying to load is not in a Python package " - "and can not be safely loaded as a result." - ) + if package_name in _no_app_package_roots: + return None
🧹 Nitpick comments (1)
holm/modules/_layout.py (1)
83-85: Silence unusedctxparameter.Ruff flags the unused argument; renaming to
_ctxkeeps the signature without lint suppression.♻️ Suggested change
- def layout(props: Any, ctx: Context) -> Component: + def layout(props: Any, _ctx: Context) -> Component:
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@holm/modules/_layout.py`:
- Around line 107-109: The layout component currently declares an unused
parameter ctx in the function signature (def layout(props: Any, ctx: Context) ->
Component) which triggers Ruff ARG001; rename the parameter to _ctx (i.e., def
layout(props: Any, _ctx: Context) -> Component) to indicate it is intentionally
unused and silence the linter, making sure to update any internal references (if
any) from ctx to _ctx or remove them if none exist.
🧹 Nitpick comments (1)
holm/_model.py (1)
186-195: Don’t swallow unexpected resource-load errors.
ReturningNonefor all exceptions can mask decode/permission/package issues and silently skiplayout.html. Consider re-raising unexpected exceptions after logging, or narrowing the catch to only “not found” cases.♻️ Suggested tweak
- except Exception: + except Exception: import traceback logger.warning(f"Failed to load resource {filename} from package '{package_name}'.") logger.warning(traceback.format_exc()) - return None + raise
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@examples/html-layout-default-slots/README.md`:
- Line 1: Replace the fragment first line in the README ("The \"HTML layout\"
guide with a custom HTML layout converter that provides a dynamic navbar to HTML
layouts.") with a proper heading and a short descriptive sentence plus a quick
run instruction: e.g., add a top-level heading like "HTML layout with dynamic
navbar" followed by one sentence describing the example (what it demonstrates:
custom HTML layout converter with dynamic navbar) and another short sentence on
how to run or view the example (e.g., commands to start the example or open
index.html). Update the README in the examples/html-layout-default-slots example
to reflect this heading-and-run format.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@holm/typing.py`:
- Around line 56-60: The docstring for the LayoutFactory TypeAlias has a typo
"asynccallable"; update the text around LayoutFactory in typing.py to read
"async callable" (or "asynchronous callable") so the sentence becomes: "A layout
factory is a sync or async callable that expects a single argument (the layout's
properties) and returns the properties for its wrapper layout." Ensure the
corrected wording appears in the triple-quoted docstring immediately above the
LayoutFactory definition.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @.github/workflows/build-docs.yml:
- Around line 20-33: Update the GitHub Actions workflow step that uses
actions/checkout@v5 to the current major release (v6); locate the "Checkout
repository" step that references actions/checkout@v5 and change the version tag
to actions/checkout@v6 so the workflow uses the latest checkout action.
- Around line 36-38: Update the GitHub Actions step that runs the package sync
so CI uses the lockfile: modify the "Install dependencies" step's command that
currently runs "uv sync" to include the --frozen flag (i.e., run "uv sync
--frozen") so installs are reproducible; ensure this change is made in the
workflow step where the name is "Install dependencies".
In `@docs/api/typing.md`:
- Around line 1-4: Remove the leading '#' so the mkdocstrings directive ':::
holm.typing' is not treated as a heading; instead create a normal heading line
(e.g., "## Typing" or similar) above it, put the '::: holm.typing' directive on
its own line, and keep the indented options block (options: / show_root_heading:
true) directly under that directive so mkdocstrings can process it.
In `@holm/typing.py`:
- Around line 56-62: Typo in the LayoutFactory docstring: change "asynccallable"
to "async callable" in the string that documents LayoutFactory (the TypeAlias
and its surrounding docstring) so the phrase reads "A layout factory is a sync
or async callable..."; locate the docstring that references LayoutFactory and
update the wording accordingly.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@AGENTS.md`:
- Around line 9-11: Replace the bold-as-heading lines "**NEVER push code**" and
"**Never install anything**" in AGENTS.md with proper Markdown headings or list
items to satisfy MD036; for example change them to a top-level or subheading
(e.g., "## NEVER push code" and "## Never install anything") or convert them
into list bullets ("- NEVER push code", "- Never install anything") so they are
not bold-only lines acting as headings.
In `@ai/doc-guide.md`:
- Around line 68-72: The document contains unhyphenated compound adjectives
(e.g., the phrase shown around "Use **bold** for:" and the example "file-system
based routing") that should be hyphenated for consistency; update occurrences
like "file system based routing" to "file-system-based routing" and any similar
compound modifiers elsewhere (the other instances noted near the same sections)
so compound adjectives are consistently hyphenated throughout the doc.
In `@docs/application-components.md`:
- Around line 20-28: The documentation must note that HTML layouts are processed
with Python str.format() via default_text_processor, so literal braces in CSS/JS
must be escaped as double braces; update docs/application-components.md to
explicitly state that `{` and `}` must be written as `{{` and `}}` when using
the HTML layout pipeline (mention the html_to_layout() function in
holm/modules/_layout.py and default_text_processor so readers can find the
implementation), and add a short example or sentence showing a CSS rule or JS
object literal converted to doubled braces.
In `@docs/guides/html-layout-default-slots.md`:
- Line 3: The sentence is missing a space before the second link; update the
paragraph containing "[HTML layout guide](html-layout.md) and the[HTML
multi-slot layout guide](html-multi-slot-layout.md)" to insert a space so it
reads "...and the [HTML multi-slot layout guide](html-multi-slot-layout.md)"
(edit the line in docs/guides/html-layout-default-slots.md where that phrase
appears).
In `@docs/guides/html-layout.md`:
- Line 18: Update the fenced code block that currently uses only triple
backticks (` ``` `) to include a language identifier to satisfy markdownlint
MD040; specifically replace the bare opening fence with a language-specified
fence (e.g., change the opening "```" to "```text") so the code block is marked
as text.
- Around line 77-81: The docs fail to mention that layout HTML is processed as
Python format strings, so update the guide to call out that literal braces in
CSS/JS must be escaped; explicitly note that layouts created via html_to_layout
(in holm/modules/_layout.py) use default_text_processor and therefore any `{` or
`}` in the HTML/CSS/JS should be doubled as `{{` and `}}` to avoid format-string
interpolation errors.
In `@docs/guides/html-multi-slot-layout.md`:
- Line 10: Fix the typo in the example link: change both the link text and the
URL from "html-mutli-slot-layout" to "html-multi-slot-layout" in the
docs/guides/html-multi-slot-layout.md content where the sentence references the
examples/html-mutli-slot-layout directory so the displayed text and the target
URL match the actual repository directory name.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/guides/html-layout-default-slots.md`:
- Around line 85-103: Update the import to use the public typing export: replace
the private-module import of Layout from holm.modules._layout with the public
type from holm.typing; in the example where str_to_layout_with_navbar calls
snippet_to_layout (and the App is constructed with
str_to_layout=str_to_layout_with_navbar), change the import statement so it
reads from holm.typing import Layout.
🧹 Nitpick comments (1)
docs/guides/html-multi-slot-layout.md (1)
46-83: Consider adding a note about escaping literal braces in HTML layouts.Since HTML layouts are processed as Python format strings, users who include inline CSS rules (e.g.,
style { color: red; }) or JavaScript object literals will encounter errors. Literal braces must be escaped as{{and}}.Adding a brief note or tip in this section would help users avoid this common pitfall.
Based on learnings: "Literal braces in the HTML (e.g., CSS
{}or JavaScript object literals) must be escaped as{{and}}. This format-string behavior should be documented for users."
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docs/guides/html-layout-default-slots.md`:
- Around line 18-19: The fenced code block showing the directory tree in
docs/guides/html-layout-default-slots.md is missing a language identifier which
triggers markdownlint MD040; update that block by adding the language identifier
`text` after the opening triple backticks so the directory structure is fenced
as ```text ... ``` to satisfy the lint rule and render correctly.
- Around line 111-139: The HTML layout examples omit a warning that layouts are
processed with Python's format-string rules (via
default_text_processor/str.format in snippet_to_layout), so add a short note in
the "Create `my_app/layout.html`" section explaining that literal braces in
CSS/JS must be escaped as `{{` and `}}` (or the format-string escaping
mechanism) to avoid formatting errors; reference default_text_processor and
snippet_to_layout in the note so readers know why escaping is required and where
processing happens.
Closes #52
TODOs:
layout.htmltoLayoutconversionchildrenslotlayout.htmltoLayoutconversion (for example defining default slots that are available in every layout)childrenslotlayout.htmltoLayoutconversionSummary by CodeRabbit
New Features
Documentation
Examples
Tests