Jinja layouts - #56
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe application now supports Jinja-based layouts with metadata, default and named slots, nested layout resolution, context propagation, and autoescaping. HTML layout conversion utilities and related examples were removed or replaced, dependencies were updated, and Jinja examples, tests, and documentation were added. ChangesJinja layout integration
Jinja examples and validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant HolmApp
participant AppConfig
participant JinjaTemplate
Client->>HolmApp: request page or action route
HolmApp->>AppConfig: merge default context and metadata
AppConfig->>JinjaTemplate: render layout with slots
JinjaTemplate->>Client: return HTML response
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)mkdocs.ymlTraceback (most recent call last): 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: 10
🤖 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 `@examples/jinja-layout-default-slots/my_app/navbar.py`:
- Around line 4-7: Update the module-level highlight_style declaration to
include a str type annotation, and remove the standalone documentation string
immediately following it. Do not replace it with a comment or add other
documentation.
In `@examples/jinja-layout-default-slots/my_app/page.py`:
- Line 10: Remove the newly added explanatory comments from
examples/jinja-layout-default-slots/my_app/page.py lines 10-10 and 15-15,
examples/jinja-layout-default-slots/my_app/main.py lines 5-7, and
examples/jinja-multi-slot-layout/my_app/page.py lines 3-3; leave the surrounding
example code unchanged and do not add replacement comments.
- Line 3: Annotate the exported module-level values: in
examples/jinja-layout-default-slots/my_app/page.py lines 3-3 and
examples/jinja-multi-slot-layout/my_app/page.py lines 4-4, type metadata as
dict[str, str]; in examples/jinja-layout-default-slots/my_app/main.py lines 8-8
and examples/jinja-multi-slot-layout/my_app/main.py lines 3-3, type app as
FastAPI.
In `@examples/jinja-layout/my_app/layout.jinja`:
- Around line 1-2: Add the English language declaration to the <html> element in
both affected layouts: examples/jinja-layout/my_app/layout.jinja lines 1-2 and
examples/jinja-layout-default-slots/my_app/layout.jinja lines 1-2. Update each
opening tag to include lang="en".
In `@examples/jinja-layout/my_app/main.py`:
- Around line 1-3: Annotate the exported app instance in main.py by adding the
appropriate App type to the app declaration. Keep the existing App()
initialization unchanged.
In `@examples/jinja-layout/my_app/page.py`:
- Line 3: Remove the “Static metadata for this page” comment from the page
module, leaving the surrounding implementation unchanged.
- Line 4: Add a type annotation to the exported metadata mapping in page.py,
ensuring metadata is explicitly typed as a string-to-string mapping while
preserving its existing title value.
In `@test_app/jinja_slots/about/page.py`:
- Line 9: Remove the unrequested explanatory comments while preserving the
existing slot wiring and assertions: delete the inline comment in
test_app/jinja_slots/about/page.py lines 9-9, and the explanatory comments in
tests/jinja_slots/about/test_page.py lines 7-14 and
tests/jinja_slots/test_page.py lines 8-17.
In `@test_app/navbar.py`:
- Around line 5-7: Update nav_item’s active-link comparison to normalize both
current_path and href consistently, including trailing-slash handling, before
determining the bold style. Preserve the existing link rendering while ensuring
equivalent routes such as “/jinja-slots” and “/jinja-slots/” are marked active.
- Around line 5-7: Update nav_item to normalize the href before comparing it
with current_path, ensuring trailing-slash variants such as the Slots link match
the stripped current path while preserving the existing link href and styling
behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8242867e-6d7e-48da-b1b6-443f90e98499
📒 Files selected for processing (77)
.ignoreAGENTS.mdexamples/html-layout-default-slots/README.mdexamples/html-layout-default-slots/my_app/layout.htmlexamples/html-layout-default-slots/my_app/main.pyexamples/html-layout/README.mdexamples/html-layout/my_app/__init__.pyexamples/html-layout/my_app/layout.htmlexamples/html-multi-slot-layout/README.mdexamples/html-multi-slot-layout/my_app/__init__.pyexamples/html-multi-slot-layout/my_app/layout.htmlexamples/jinja-layout-default-slots/README.mdexamples/jinja-layout-default-slots/my_app/__init__.pyexamples/jinja-layout-default-slots/my_app/about/__init__.pyexamples/jinja-layout-default-slots/my_app/about/page.pyexamples/jinja-layout-default-slots/my_app/layout.jinjaexamples/jinja-layout-default-slots/my_app/main.pyexamples/jinja-layout-default-slots/my_app/navbar.pyexamples/jinja-layout-default-slots/my_app/page.pyexamples/jinja-layout/README.mdexamples/jinja-layout/my_app/__init__.pyexamples/jinja-layout/my_app/about/__init__.pyexamples/jinja-layout/my_app/about/page.pyexamples/jinja-layout/my_app/layout.jinjaexamples/jinja-layout/my_app/main.pyexamples/jinja-layout/my_app/page.pyexamples/jinja-multi-slot-layout/README.mdexamples/jinja-multi-slot-layout/my_app/__init__.pyexamples/jinja-multi-slot-layout/my_app/about/__init__.pyexamples/jinja-multi-slot-layout/my_app/about/page.pyexamples/jinja-multi-slot-layout/my_app/layout.jinjaexamples/jinja-multi-slot-layout/my_app/main.pyexamples/jinja-multi-slot-layout/my_app/page.pyholm/__init__.pyholm/_jinja.pyholm/_model.pyholm/app.pyholm/module_options/_metadata.pyholm/modules/_layout.pyholm/typing.pyholm/utils.pypyproject.tomltest_app/html_and_python_layout/layout.htmltest_app/html_layout/dict_slots/layout.htmltest_app/html_layout/layout.htmltest_app/html_layout/page.pytest_app/jinja_and_python_layout/__init__.pytest_app/jinja_and_python_layout/layout.jinjatest_app/jinja_and_python_layout/layout.pytest_app/jinja_and_python_layout/page.pytest_app/jinja_layout/__init__.pytest_app/jinja_layout/dict_slots/__init__.pytest_app/jinja_layout/dict_slots/layout.jinjatest_app/jinja_layout/dict_slots/page.pytest_app/jinja_layout/layout.jinjatest_app/jinja_layout/page.pytest_app/jinja_slots/__init__.pytest_app/jinja_slots/about/__init__.pytest_app/jinja_slots/about/layout.jinjatest_app/jinja_slots/about/page.pytest_app/jinja_slots/layout.jinjatest_app/jinja_slots/leaf.jinjatest_app/jinja_slots/page.pytest_app/main.pytest_app/navbar.pytests/html_layout/dict_slots/test_page.pytests/html_layout/test_page.pytests/jinja_and_python_layout/__init__.pytests/jinja_and_python_layout/test_page.pytests/jinja_layout/__init__.pytests/jinja_layout/dict_slots/__init__.pytests/jinja_layout/dict_slots/test_page.pytests/jinja_layout/test_page.pytests/jinja_slots/__init__.pytests/jinja_slots/about/__init__.pytests/jinja_slots/about/test_page.pytests/jinja_slots/test_page.py
💤 Files with no reviewable changes (17)
- test_app/html_layout/layout.html
- examples/html-layout/my_app/init.py
- examples/html-layout-default-slots/README.md
- examples/html-layout-default-slots/my_app/layout.html
- examples/html-multi-slot-layout/README.md
- examples/html-layout/README.md
- test_app/html_layout/dict_slots/layout.html
- tests/html_layout/test_page.py
- examples/html-layout-default-slots/my_app/main.py
- examples/html-multi-slot-layout/my_app/init.py
- tests/html_layout/dict_slots/test_page.py
- examples/html-multi-slot-layout/my_app/layout.html
- holm/utils.py
- holm/typing.py
- examples/html-layout/my_app/layout.html
- test_app/html_layout/page.py
- test_app/html_and_python_layout/layout.html
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 10
🤖 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 `@examples/jinja-layout-default-slots/my_app/navbar.py`:
- Around line 4-7: Update the module-level highlight_style declaration to
include a str type annotation, and remove the standalone documentation string
immediately following it. Do not replace it with a comment or add other
documentation.
In `@examples/jinja-layout-default-slots/my_app/page.py`:
- Line 10: Remove the newly added explanatory comments from
examples/jinja-layout-default-slots/my_app/page.py lines 10-10 and 15-15,
examples/jinja-layout-default-slots/my_app/main.py lines 5-7, and
examples/jinja-multi-slot-layout/my_app/page.py lines 3-3; leave the surrounding
example code unchanged and do not add replacement comments.
- Line 3: Annotate the exported module-level values: in
examples/jinja-layout-default-slots/my_app/page.py lines 3-3 and
examples/jinja-multi-slot-layout/my_app/page.py lines 4-4, type metadata as
dict[str, str]; in examples/jinja-layout-default-slots/my_app/main.py lines 8-8
and examples/jinja-multi-slot-layout/my_app/main.py lines 3-3, type app as
FastAPI.
In `@examples/jinja-layout/my_app/layout.jinja`:
- Around line 1-2: Add the English language declaration to the <html> element in
both affected layouts: examples/jinja-layout/my_app/layout.jinja lines 1-2 and
examples/jinja-layout-default-slots/my_app/layout.jinja lines 1-2. Update each
opening tag to include lang="en".
In `@examples/jinja-layout/my_app/main.py`:
- Around line 1-3: Annotate the exported app instance in main.py by adding the
appropriate App type to the app declaration. Keep the existing App()
initialization unchanged.
In `@examples/jinja-layout/my_app/page.py`:
- Line 3: Remove the “Static metadata for this page” comment from the page
module, leaving the surrounding implementation unchanged.
- Line 4: Add a type annotation to the exported metadata mapping in page.py,
ensuring metadata is explicitly typed as a string-to-string mapping while
preserving its existing title value.
In `@test_app/jinja_slots/about/page.py`:
- Line 9: Remove the unrequested explanatory comments while preserving the
existing slot wiring and assertions: delete the inline comment in
test_app/jinja_slots/about/page.py lines 9-9, and the explanatory comments in
tests/jinja_slots/about/test_page.py lines 7-14 and
tests/jinja_slots/test_page.py lines 8-17.
In `@test_app/navbar.py`:
- Around line 5-7: Update nav_item’s active-link comparison to normalize both
current_path and href consistently, including trailing-slash handling, before
determining the bold style. Preserve the existing link rendering while ensuring
equivalent routes such as “/jinja-slots” and “/jinja-slots/” are marked active.
- Around line 5-7: Update nav_item to normalize the href before comparing it
with current_path, ensuring trailing-slash variants such as the Slots link match
the stripped current path while preserving the existing link href and styling
behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8242867e-6d7e-48da-b1b6-443f90e98499
📒 Files selected for processing (77)
.ignoreAGENTS.mdexamples/html-layout-default-slots/README.mdexamples/html-layout-default-slots/my_app/layout.htmlexamples/html-layout-default-slots/my_app/main.pyexamples/html-layout/README.mdexamples/html-layout/my_app/__init__.pyexamples/html-layout/my_app/layout.htmlexamples/html-multi-slot-layout/README.mdexamples/html-multi-slot-layout/my_app/__init__.pyexamples/html-multi-slot-layout/my_app/layout.htmlexamples/jinja-layout-default-slots/README.mdexamples/jinja-layout-default-slots/my_app/__init__.pyexamples/jinja-layout-default-slots/my_app/about/__init__.pyexamples/jinja-layout-default-slots/my_app/about/page.pyexamples/jinja-layout-default-slots/my_app/layout.jinjaexamples/jinja-layout-default-slots/my_app/main.pyexamples/jinja-layout-default-slots/my_app/navbar.pyexamples/jinja-layout-default-slots/my_app/page.pyexamples/jinja-layout/README.mdexamples/jinja-layout/my_app/__init__.pyexamples/jinja-layout/my_app/about/__init__.pyexamples/jinja-layout/my_app/about/page.pyexamples/jinja-layout/my_app/layout.jinjaexamples/jinja-layout/my_app/main.pyexamples/jinja-layout/my_app/page.pyexamples/jinja-multi-slot-layout/README.mdexamples/jinja-multi-slot-layout/my_app/__init__.pyexamples/jinja-multi-slot-layout/my_app/about/__init__.pyexamples/jinja-multi-slot-layout/my_app/about/page.pyexamples/jinja-multi-slot-layout/my_app/layout.jinjaexamples/jinja-multi-slot-layout/my_app/main.pyexamples/jinja-multi-slot-layout/my_app/page.pyholm/__init__.pyholm/_jinja.pyholm/_model.pyholm/app.pyholm/module_options/_metadata.pyholm/modules/_layout.pyholm/typing.pyholm/utils.pypyproject.tomltest_app/html_and_python_layout/layout.htmltest_app/html_layout/dict_slots/layout.htmltest_app/html_layout/layout.htmltest_app/html_layout/page.pytest_app/jinja_and_python_layout/__init__.pytest_app/jinja_and_python_layout/layout.jinjatest_app/jinja_and_python_layout/layout.pytest_app/jinja_and_python_layout/page.pytest_app/jinja_layout/__init__.pytest_app/jinja_layout/dict_slots/__init__.pytest_app/jinja_layout/dict_slots/layout.jinjatest_app/jinja_layout/dict_slots/page.pytest_app/jinja_layout/layout.jinjatest_app/jinja_layout/page.pytest_app/jinja_slots/__init__.pytest_app/jinja_slots/about/__init__.pytest_app/jinja_slots/about/layout.jinjatest_app/jinja_slots/about/page.pytest_app/jinja_slots/layout.jinjatest_app/jinja_slots/leaf.jinjatest_app/jinja_slots/page.pytest_app/main.pytest_app/navbar.pytests/html_layout/dict_slots/test_page.pytests/html_layout/test_page.pytests/jinja_and_python_layout/__init__.pytests/jinja_and_python_layout/test_page.pytests/jinja_layout/__init__.pytests/jinja_layout/dict_slots/__init__.pytests/jinja_layout/dict_slots/test_page.pytests/jinja_layout/test_page.pytests/jinja_slots/__init__.pytests/jinja_slots/about/__init__.pytests/jinja_slots/about/test_page.pytests/jinja_slots/test_page.py
💤 Files with no reviewable changes (17)
- test_app/html_layout/layout.html
- examples/html-layout/my_app/init.py
- examples/html-layout-default-slots/README.md
- examples/html-layout-default-slots/my_app/layout.html
- examples/html-multi-slot-layout/README.md
- examples/html-layout/README.md
- test_app/html_layout/dict_slots/layout.html
- tests/html_layout/test_page.py
- examples/html-layout-default-slots/my_app/main.py
- examples/html-multi-slot-layout/my_app/init.py
- tests/html_layout/dict_slots/test_page.py
- examples/html-multi-slot-layout/my_app/layout.html
- holm/utils.py
- holm/typing.py
- examples/html-layout/my_app/layout.html
- test_app/html_layout/page.py
- test_app/html_and_python_layout/layout.html
🛑 Comments failed to post (9)
examples/jinja-layout-default-slots/my_app/navbar.py (1)
4-7: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Annotate
highlight_styleand remove the detached documentation string.
highlight_styleis untyped, and lines 5-7 are a discarded string expression rather than a module docstring. Usehighlight_style: str = ...and remove the detached string.As per coding guidelines, type hints are required and comments should not be added unless explicitly requested.
🤖 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 `@examples/jinja-layout-default-slots/my_app/navbar.py` around lines 4 - 7, Update the module-level highlight_style declaration to include a str type annotation, and remove the standalone documentation string immediately following it. Do not replace it with a comment or add other documentation.Source: Coding guidelines
examples/jinja-layout-default-slots/my_app/page.py (2)
3-3: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add explicit annotations to the new exported/module-level values.
The affected Python modules introduce unannotated
metadataorappvalues despite the repository requirement for type hints.
examples/jinja-layout-default-slots/my_app/page.py#L3-L3: annotatemetadataasdict[str, str].examples/jinja-layout-default-slots/my_app/main.py#L8-L8: annotateappasFastAPI.examples/jinja-multi-slot-layout/my_app/page.py#L4-L4: annotatemetadataasdict[str, str].examples/jinja-multi-slot-layout/my_app/main.py#L3-L3: annotateappasFastAPI.As per coding guidelines, type hints are required.
📍 Affects 4 files
examples/jinja-layout-default-slots/my_app/page.py#L3-L3(this comment)examples/jinja-layout-default-slots/my_app/main.py#L8-L8examples/jinja-multi-slot-layout/my_app/page.py#L4-L4examples/jinja-multi-slot-layout/my_app/main.py#L3-L3🤖 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 `@examples/jinja-layout-default-slots/my_app/page.py` at line 3, Annotate the exported module-level values: in examples/jinja-layout-default-slots/my_app/page.py lines 3-3 and examples/jinja-multi-slot-layout/my_app/page.py lines 4-4, type metadata as dict[str, str]; in examples/jinja-layout-default-slots/my_app/main.py lines 8-8 and examples/jinja-multi-slot-layout/my_app/main.py lines 3-3, type app as FastAPI.Source: Coding guidelines
10-10: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the newly added Python comments.
These explanatory comments are unnecessary for the small, self-describing example code and violate the repository’s comment policy.
examples/jinja-layout-default-slots/my_app/page.py#L10-L10: remove the intro-slot comment.examples/jinja-layout-default-slots/my_app/page.py#L15-L15: remove the details-slot comment.examples/jinja-layout-default-slots/my_app/main.py#L5-L7: remove thelayout_slotsexplanation.examples/jinja-multi-slot-layout/my_app/page.py#L3-L3: remove the static-metadata comment.As per coding guidelines, do not add comments unless explicitly requested.
📍 Affects 3 files
examples/jinja-layout-default-slots/my_app/page.py#L10-L10(this comment)examples/jinja-layout-default-slots/my_app/page.py#L15-L15examples/jinja-layout-default-slots/my_app/main.py#L5-L7examples/jinja-multi-slot-layout/my_app/page.py#L3-L3🤖 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 `@examples/jinja-layout-default-slots/my_app/page.py` at line 10, Remove the newly added explanatory comments from examples/jinja-layout-default-slots/my_app/page.py lines 10-10 and 15-15, examples/jinja-layout-default-slots/my_app/main.py lines 5-7, and examples/jinja-multi-slot-layout/my_app/page.py lines 3-3; leave the surrounding example code unchanged and do not add replacement comments.Source: Coding guidelines
examples/jinja-layout/my_app/layout.jinja (1)
1-2: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add a language declaration to both HTML layouts.
examples/jinja-layout/my_app/layout.jinja#L1-L2: change<html>to<html lang="en">.examples/jinja-layout-default-slots/my_app/layout.jinja#L1-L2: change<html>to<html lang="en">.📍 Affects 2 files
examples/jinja-layout/my_app/layout.jinja#L1-L2(this comment)examples/jinja-layout-default-slots/my_app/layout.jinja#L1-L2🤖 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 `@examples/jinja-layout/my_app/layout.jinja` around lines 1 - 2, Add the English language declaration to the <html> element in both affected layouts: examples/jinja-layout/my_app/layout.jinja lines 1-2 and examples/jinja-layout-default-slots/my_app/layout.jinja lines 1-2. Update each opening tag to include lang="en".examples/jinja-layout/my_app/main.py (1)
1-3: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Annotate the exported application instance.
As per coding guidelines, type hints are required.
Proposed fix
+from fastapi import FastAPI from holm import App -app = App() +app: FastAPI = App()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.from fastapi import FastAPI from holm import App app: FastAPI = App()🤖 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 `@examples/jinja-layout/my_app/main.py` around lines 1 - 3, Annotate the exported app instance in main.py by adding the appropriate App type to the app declaration. Keep the existing App() initialization unchanged.Source: Coding guidelines
examples/jinja-layout/my_app/page.py (2)
3-3: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the unrequested comment.
As per coding guidelines, do not add comments unless explicitly requested.
🤖 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 `@examples/jinja-layout/my_app/page.py` at line 3, Remove the “Static metadata for this page” comment from the page module, leaving the surrounding implementation unchanged.Source: Coding guidelines
4-4: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a type annotation to the exported metadata mapping.
As per coding guidelines, type hints are required.
Proposed fix
-metadata = {"title": "Home | My App"} +metadata: dict[str, str] = {"title": "Home | My App"}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.metadata: dict[str, str] = {"title": "Home | My App"}🤖 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 `@examples/jinja-layout/my_app/page.py` at line 4, Add a type annotation to the exported metadata mapping in page.py, ensuring metadata is explicitly typed as a string-to-string mapping while preserving its existing title value.Source: Coding guidelines
test_app/jinja_slots/about/page.py (1)
9-9: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove unrequested comments from the Python changes.
As per coding guidelines, comments must not be added unless explicitly requested. Remove the explanatory comments while keeping the underlying slot wiring and assertions unchanged.
test_app/jinja_slots/about/page.py#L9-L9: remove the inline slot-behavior comment.tests/jinja_slots/about/test_page.py#L7-L14: remove the explanatory comments at Lines 7, 10, 12, and 14.tests/jinja_slots/test_page.py#L8-L17: remove the explanatory comments at Lines 8, 10, 12, 14, and 17.📍 Affects 3 files
test_app/jinja_slots/about/page.py#L9-L9(this comment)tests/jinja_slots/about/test_page.py#L7-L14tests/jinja_slots/test_page.py#L8-L17🤖 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 `@test_app/jinja_slots/about/page.py` at line 9, Remove the unrequested explanatory comments while preserving the existing slot wiring and assertions: delete the inline comment in test_app/jinja_slots/about/page.py lines 9-9, and the explanatory comments in tests/jinja_slots/about/test_page.py lines 7-14 and tests/jinja_slots/test_page.py lines 8-17.Source: Coding guidelines
test_app/navbar.py (1)
5-7: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Normalize both sides of the active-link comparison.
Line 13 turns
/jinja-slots/into/jinja-slots, while Line 17 compares it with/jinja-slots/; consequently, the Slots link is never marked active on its own route.Proposed fix
def nav_item(text: str, *, href: str, current_path: str) -> Component: - style = "font-weight: bold;" if current_path == href else None + current = current_path.rstrip("/") or "/" + target = href.rstrip("/") or "/" + style = "font-weight: bold;" if current == target else NoneAlso applies to: 12-17
🤖 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 `@test_app/navbar.py` around lines 5 - 7, Update nav_item’s active-link comparison to normalize both current_path and href consistently, including trailing-slash handling, before determining the bold style. Preserve the existing link rendering while ensuring equivalent routes such as “/jinja-slots” and “/jinja-slots/” are marked active.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the target file and nearby context git ls-files test_app/navbar.py wc -l test_app/navbar.py cat -n test_app/navbar.py # Find the HTMY version and local type usage rg -n "htmy|ComponentType|Component\b|navbar|nav_item|custom_navbar|current_path" -S .Repository: volfpeter/holm
Length of output: 44568
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the Jinja slot route/layout wiring that uses test_app/navbar.py wc -l test_app/jinja_slots/page.py test_app/jinja_slots/layout.jinja test_app/jinja_slots/about/page.py test_app/jinja_slots/about/layout.jinja test_app/main.py test_app/jinja_slots/about/layout.jinja 2>/dev/null || true cat -n test_app/jinja_slots/page.py printf '\n---\n' cat -n test_app/jinja_slots/about/page.py printf '\n---\n' cat -n test_app/main.py printf '\n---\n' cat -n test_app/jinja_slots/layout.jinja printf '\n---\n' cat -n test_app/jinja_slots/about/layout.jinja 2>/dev/null || true # Inspect the documentation example that appears to be the source of this navbar pattern printf '\n=== docs example ===\n' cat -n docs/guides/html-layout-default-slots.md | sed -n '33,80p'Repository: volfpeter/holm
Length of output: 4530
🏁 Script executed:
python3 - <<'PY' import inspect import sys try: import htmy from htmy import html except Exception as e: print(f"IMPORT_ERROR: {e}") sys.exit(0) print("htmy_file:", getattr(htmy, "__file__", None)) print("htmy_attrs_has_Component:", hasattr(htmy, "Component")) print("htmy_attrs_has_ComponentType:", hasattr(htmy, "ComponentType")) print("htmy_attrs_has_is_component_type:", hasattr(htmy, "is_component_type")) print("html_file:", getattr(html, "__file__", None)) for name in ["Component", "ComponentType", "Context", "component"]: obj = getattr(htmy, name, None) print(f"{name}:", obj) # Best-effort introspection of the html builder return types if accessible. for attr in ["nav", "ul", "li", "a"]: obj = getattr(html, attr, None) print(f"html.{attr}:", obj) try: print(f" signature: {inspect.signature(obj)}") except Exception as e: print(f" signature_error: {e}") PYRepository: volfpeter/holm
Length of output: 189
Normalize the
Slotshref before comparing it tocurrent_path.
current_pathis stripped to/jinja-slots, but theSlotslink still uses/jinja-slots/, so it never gets the bold style. Use the same normalized path on both sides.🤖 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 `@test_app/navbar.py` around lines 5 - 7, Update nav_item to normalize the href before comparing it with current_path, ensuring trailing-slash variants such as the Slots link match the stripped current path while preserving the existing link href and styling behavior.Source: Learnings
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs/guides/jinja-layout-default-slots.md`:
- Line 18: Update the directory-tree code-fence opening in
docs/guides/jinja-layout-default-slots.md:18-18,
docs/guides/jinja-layout.md:16-16, and
docs/guides/jinja-multi-slot-layout.md:16-16 to specify the text language
identifier.
In `@docs/in-a-hurry.md`:
- Line 27: Revise the FastAPI comparison in the documentation sentence to remove
the claim that existing code transfers without adaptation. State that FastAPI
knowledge and components remain reusable, but the project must be adapted to
holm’s package, special-file, and route-discovery conventions.
In `@README.md`:
- Line 20: Qualify the built-in Jinja “zero configuration” claims with both
prerequisites: an App()-owned default renderer and discovery of at least one
layout.jinja; otherwise users must configure JinjaTemplates manually. Apply this
wording to README.md lines 20 and 119 and docs/index.md lines 11 and 110,
preserving the existing renderer qualification where present.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 551f7986-7c2b-42c5-b8d8-376a3669c203
📒 Files selected for processing (16)
.editorconfigREADME.mddocs/api/holm.mddocs/api/utils.mddocs/application-components.mddocs/file-system-based-routing.mddocs/guides/jinja-layout-default-slots.mddocs/guides/jinja-layout.mddocs/guides/jinja-multi-slot-layout.mddocs/in-a-hurry.mddocs/index.mdexamples/jinja-layout-default-slots/my_app/main.pyholm/_jinja.pyholm/app.pymkdocs.ymlpyproject.toml
💤 Files with no reviewable changes (1)
- docs/api/utils.md
🚧 Files skipped from review as they are similar to previous changes (4)
- examples/jinja-layout-default-slots/my_app/main.py
- pyproject.toml
- holm/_jinja.py
- holm/app.py
…upport is truly zero config now
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
holm/app.py (2)
186-187: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the newly added comments.
These explanatory comments violate the repository rule: “Do not add comments unless explicitly requested.” Keep the surrounding logic unchanged.
As per coding guidelines, “Do not add comments unless explicitly requested.”
Proposed cleanup
- # Always route through `_make_page_path_operation()` so every action render - # receives the app-scope default context via the component tree. path_operation = _make_page_path_operation( - # Only `layout.jinja` is recognized as a Jinja layout module. Other `.jinja` - # files are not valid holm modules and must not produce package markers. config.app_dir.rglob("layout.jinja"), - context.update(Metadata(metadata).to_context()) # always; empty Metadata when None + context.update(Metadata(metadata).to_context())Also applies to: 248-249, 319-319
🤖 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 `@holm/app.py` around lines 186 - 187, Remove the newly added explanatory comments near the `_make_page_path_operation()` call and the corresponding locations identified in the diff, while leaving all surrounding logic and behavior unchanged.Source: Coding guidelines
79-88: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPropagate the app default context into error rendering.
wrap_error_handler()renders exception components throughhtmy.render_component(), but the ownedHTMY()instance here is created withoutconfig.default_context. Jinja-based error pages andDefaultSlotswill be missing on the error path; pass the app default context into the owned renderer, or inject it via a request processor, before registering handlers.🤖 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 `@holm/app.py` around lines 79 - 88, Update the owned HTMY instance created in the app initialization flow around config and layout_slots so it receives config.default_context before error handlers are registered. Ensure wrap_error_handler() renders through an HTMY renderer with both Jinja template context and DefaultSlots available, while preserving the existing behavior for externally supplied htmy instances.
🤖 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.
Outside diff comments:
In `@holm/app.py`:
- Around line 186-187: Remove the newly added explanatory comments near the
`_make_page_path_operation()` call and the corresponding locations identified in
the diff, while leaving all surrounding logic and behavior unchanged.
- Around line 79-88: Update the owned HTMY instance created in the app
initialization flow around config and layout_slots so it receives
config.default_context before error handlers are registered. Ensure
wrap_error_handler() renders through an HTMY renderer with both Jinja template
context and DefaultSlots available, while preserving the existing behavior for
externally supplied htmy instances.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a0d094b7-c02c-4064-933c-67ecb21b46b7
📒 Files selected for processing (4)
docs/guides/jinja-layout.mdholm/_model.pyholm/app.pypyproject.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- pyproject.toml
- docs/guides/jinja-layout.md
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
examples/jinja-quick-start/layout.py (1)
13-13: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a language attribute to the root document element.
Set
lang="en"onhtml.html(...)so assistive technologies and translation tools can identify the document language.🤖 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 `@examples/jinja-quick-start/layout.py` at line 13, Update the root document element created by html.html(...) to include the language attribute lang="en", preserving the existing element content and structure.
🤖 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 `@examples/jinja-quick-start/layout.py`:
- Line 18: Remove the added inline comments at
examples/jinja-quick-start/layout.py lines 18-18 and
examples/jinja-quick-start/page.py lines 3-3, including the PicoCSS comment near
html.link and the static metadata comment; leave the surrounding code unchanged.
---
Nitpick comments:
In `@examples/jinja-quick-start/layout.py`:
- Line 13: Update the root document element created by html.html(...) to include
the language attribute lang="en", preserving the existing element content and
structure.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d5682af1-9396-40dd-9668-fe637b098ab0
📒 Files selected for processing (8)
examples/jinja-quick-start/README.mdexamples/jinja-quick-start/about/__init__.pyexamples/jinja-quick-start/about/page.jinjaexamples/jinja-quick-start/about/page.pyexamples/jinja-quick-start/layout.pyexamples/jinja-quick-start/main.pyexamples/jinja-quick-start/page.pyholm/app.py
🚧 Files skipped from review as they are similar to previous changes (1)
- holm/app.py
Owner
Author
|
@coderabbitai review fresh changes |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/guides/jinja-quick-start.md`:
- Around line 15-24: Update the directory-tree code fence in the Jinja
quick-start documentation to include an appropriate language identifier, such as
text, on its opening fence while preserving the displayed tree content.
In `@README.md`:
- Line 159: Change “file-system based routing” to “file-system-based routing” in
README.md lines 159-159 and docs/index.md lines 150-150, preserving the
surrounding documentation.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b5d56bc4-245c-4e3f-ac8a-aa4bc0a43f04
📒 Files selected for processing (24)
README.mddocs/guides/jinja-layout-default-slots.mddocs/guides/jinja-layout.mddocs/guides/jinja-multi-slot-layout.mddocs/guides/jinja-quick-start.mddocs/guides/quick-start-guide.mddocs/in-a-hurry.mddocs/index.mdexamples/actions-with-htmx/about/page.pyexamples/jinja-layout-default-slots/my_app/about/page.pyexamples/jinja-layout/my_app/about/page.pyexamples/jinja-multi-slot-layout/my_app/about/page.pyexamples/jinja-quick-start/README.mdexamples/jinja-quick-start/about/__init__.pyexamples/jinja-quick-start/about/page.jinjaexamples/jinja-quick-start/about/page.pyexamples/jinja-quick-start/layout.pyexamples/jinja-quick-start/main.pyexamples/jinja-quick-start/page.pyexamples/quick-start-guide/about/page.pyexamples/rendering-apis-with-htmx/about/page.pyholm/app.pymkdocs.ymlpyproject.toml
🚧 Files skipped from review as they are similar to previous changes (11)
- examples/jinja-quick-start/main.py
- examples/jinja-quick-start/about/page.jinja
- examples/jinja-quick-start/page.py
- examples/jinja-quick-start/about/page.py
- examples/jinja-layout-default-slots/my_app/about/page.py
- examples/jinja-quick-start/layout.py
- pyproject.toml
- docs/guides/jinja-layout-default-slots.md
- docs/guides/jinja-layout.md
- docs/guides/jinja-multi-slot-layout.md
- holm/app.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TODOs:
JinjaTemplatefor the about page (template intemplates/folder.Summary by CodeRabbit
Summary by CodeRabbit
layout_slots, with per-page overrides.0.9.0→0.10.0.layout.htmllayout support.