Skip to content

possible fix for nested layouts#832

Merged
sagzy merged 3 commits into
fix/recursive-templatefrom
suggestion/recursive-template
Jun 10, 2026
Merged

possible fix for nested layouts#832
sagzy merged 3 commits into
fix/recursive-templatefrom
suggestion/recursive-template

Conversation

@troyciesco

Copy link
Copy Markdown
Contributor

No description provided.

@coderabbitai

coderabbitai Bot commented Jun 8, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ae72115e-1be2-4f4d-ac4d-940ba12e24ed

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch suggestion/recursive-template

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@lib/checks/130-template-inheritance.js`:
- Around line 25-27: The code returns absolute layout keys (leading '/') which
don't match theme-relative lookup keys; change the branch in the layout
normalization to normalize then strip any leading slashes so returned keys are
theme-relative. Specifically, update the logic around layout.startsWith('/') to
use path.posix.normalize(layout) and then remove leading '/' characters (e.g.,
via a replace(/^\/+/, '')) before returning so lookups using the layout variable
match the graph keys like "default.hbs".

In `@test/130-template-inheritance.test.js`:
- Around line 6-14: The tests currently use a custom runCheck(files) helper that
calls thisCheck directly; replace those inline file-object tests with the shared
fixture-based harness by creating theme fixtures under test/fixtures/themes/ and
invoking utils.testCheck(...) (using themePath(...) when referencing fixture
dirs) instead of runCheck/thisCheck; update each occurrence (runCheck,
thisCheck) in the listed blocks (and the test file's calls at lines ~6 and the
other ranges) to call utils.testCheck with the appropriate fixture path and
expected results so the tests follow the standard contract and fixture flow.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b9ccae43-0dea-4c63-8ef5-a7d82abff2de

📥 Commits

Reviewing files that changed from the base of the PR and between 4ea22b9 and a32a2e9.

📒 Files selected for processing (2)
  • lib/checks/130-template-inheritance.js
  • test/130-template-inheritance.test.js

Comment thread lib/checks/130-template-inheritance.js
Comment thread test/130-template-inheritance.test.js Outdated
@troyciesco troyciesco force-pushed the suggestion/recursive-template branch from a32a2e9 to 5da00cb Compare June 8, 2026 22:21
sagzy and others added 2 commits June 10, 2026 15:17
Ghost resolves layout paths with path.resolve(templateDir, layout), so a
leading slash produces a filesystem-absolute path that express-hbs's
restrictLayoutsTo check blocks — it can never load a theme file. Treat
such references as unresolvable (same as missing targets) instead of
mapping them to the theme root and reporting a misleading recursion error.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Superseded by indirect-root-cycle when the test was re-pointed in the
nested-layouts fix. It also wasn't a true runtime cycle: in Ghost,
{{!< default}} inside layouts/base.hbs resolves to the nonexistent
layouts/default.hbs, which is a render error rather than recursion.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@sagzy sagzy merged commit 5e34852 into fix/recursive-template Jun 10, 2026
3 checks passed
@sagzy sagzy deleted the suggestion/recursive-template branch June 10, 2026 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants