Simplified version that doesn't break the deploy. - #215
Conversation
|
Warning Rate limit exceeded@killev has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 34 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughRefactors the local @apostrophecms/seo module to externalize GTM ID logic into a new lib/gtm-utils.js with sanitizeGtmId and resolveGtmId. Removes the improve export and in-module init injections, replacing them with explicit template component calls. tagManagerHead and tagManagerBody now use gtmUtils.resolveGtmId(req, self.options). metaHead no longer handles GTM. layout.html adds component invocations for tagManagerHead and tagManagerBody. app.js clears previous GTM options for @apostrophecms/seo. package.json removes the external @apostrophecms/seo dependency. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
🔍 Vulnerabilities of
|
| digest | sha256:c700ca033c317d15777775bda5c72556ffbe9f2fc89a037123847b95fd25af96 |
| vulnerabilities |     |
| platform | linux/amd64 |
| size | 291 MB |
| packages | 984 |
📦 Base Image node:23-alpine
| also known as |
|
| digest | sha256:b9d38d589853406ff0d4364f21969840c3e0397087643aef8eede40edbb6c7cd |
| vulnerabilities |     |
Description
| ||||||||||||
Description
| ||||||||||||
Description
| ||||||||||||
Description
| ||||||||||||
Description
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
website/modules/@apostrophecms/seo/views/gtmHead.html (1)
2-6: Harden GTM snippet for CSP and optional custom dataLayer nameTwo small but impactful tweaks:
- Add a conditional nonce attribute to support strict Content Security Policy.
- Allow overriding the data layer name via data.layerName while preserving the default.
Apply this diff:
-<script>(function(w,d,s,l,i){w[l]=w[l]||[];w[l].push({'gtm.start': +<script{% if data.cspNonce %} nonce="{{ data.cspNonce }}"{% endif %}>(function(w,d,s,l,i){w[l]=w[l]||[];w[l].push({'gtm.start': new Date().getTime(),event:'gtm.js'});var f=d.getElementsByTagName(s)[0], j=d.createElement(s),dl=l!='dataLayer'?'&l='+l:'';j.async=true;j.src= 'https://www.googletagmanager.com/gtm.js?id='+i+dl;f.parentNode.insertBefore(j,f); -})(window,document,'script','dataLayer','{{ data.gtmId }}');</script> +})(window,document,'script','{{ data.layerName | default("dataLayer") }}','{{ data.gtmId }}');</script>website/modules/@apostrophecms/seo/views/gtmBody.html (1)
2-3: A11y: hide the noscript iframe from assistive techAdd
aria-hidden="true"andtabindex="-1"(optional) so screen readers don't announce the hidden iframe.Apply this diff:
-<noscript><iframe src="https://www.googletagmanager.com/ns.html?id={{ data.gtmId }}" -height="0" width="0" style="display:none;visibility:hidden"></iframe></noscript> +<noscript><iframe src="https://www.googletagmanager.com/ns.html?id={{ data.gtmId }}" +height="0" width="0" style="display:none;visibility:hidden" aria-hidden="true" tabindex="-1"></iframe></noscript>website/modules/@apostrophecms/seo/index.js (1)
24-28: Minor: dedupe the GTM ID resolution pattern
tagManagerBodyandtagManagerHeadshare identical resolution and conditional return logic. Consider extracting a tiny helper for maintainability.Example refactor:
components(self) { - return { + const resolveForPage = (req) => { + if (!req?.data?.page) return ''; + return gtmUtils.resolveGtmId(req, self.options); + }; + return { tagManagerBody(req, data) { - if (!req?.data?.page) { - return {}; - } - const gtmId = gtmUtils.resolveGtmId(req, self.options); + const gtmId = resolveForPage(req); if (gtmId) { return { gtmId }; } return {}; },Repeat similarly for
tagManagerHead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
website/modules/@apostrophecms/seo/index.js(3 hunks)website/modules/@apostrophecms/seo/lib/gtm-utils.js(1 hunks)website/modules/@apostrophecms/seo/views/gtmBody.html(1 hunks)website/modules/@apostrophecms/seo/views/gtmHead.html(1 hunks)website/modules/@apostrophecms/seo/views/tagManagerBody.html(1 hunks)website/modules/@apostrophecms/seo/views/tagManagerHead.html(1 hunks)website/package.json(0 hunks)website/views/layout.html(1 hunks)
💤 Files with no reviewable changes (1)
- website/package.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: e2e-tests
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: security-scan
🔇 Additional comments (6)
website/modules/@apostrophecms/seo/views/tagManagerBody.html (1)
2-2: LGTM: delegating to a single gtmBody templateGood consolidation; the gating by data.gtmId remains intact and prevents unnecessary markup.
website/modules/@apostrophecms/seo/views/tagManagerHead.html (1)
2-2: LGTM: render the dedicated GTM head templateClean delegation; keeps the gate and centralizes the snippet in one place.
website/modules/@apostrophecms/seo/views/gtmBody.html (1)
2-3: LGTM on GTM noscript blockThe markup is standard, uses the sanitized
data.gtmId, and is safely escaped by Nunjucks.website/modules/@apostrophecms/seo/index.js (3)
1-1: Sanitized GTM ID resolution via utility looks goodDepending on
gtmUtils.resolveGtmId(req, self.options)centralizes validation and keeps templates clean.
7-10: Verify template injection ordering for GTMGoogle recommends placing the GTM head snippet as early as possible in
<head>. You're currently appending it to<head>and prependingmetaHead. If early execution is important to you, consider alsoprepend-ingtagManagerHead(and ordering the two prepends so GTM comes first).Would you like me to propose an ordering that guarantees GTM precedes other head inserts?
3-10: No missing files — SEO template components presentYour script output confirmed "All expected files are present." Verified files:
- website/modules/@apostrophecms/seo/lib/gtm-utils.js
- website/modules/@apostrophecms/seo/views/tagManagerHead.html
- website/modules/@apostrophecms/seo/views/tagManagerBody.html
- website/modules/@apostrophecms/seo/views/metaHead.html
- website/modules/@apostrophecms/seo/views/gtmHead.html
- website/modules/@apostrophecms/seo/views/gtmBody.html
No action required.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
website/views/layout.html (1)
17-17: Good fix: replaced undefined globals with explicit SEO component renderSwitching to {% render '@apostrophecms/seo:tagManagerHead', data %} removes the dependency on missing Nunjucks globals and aligns with Apostrophe’s component pattern.
🧹 Nitpick comments (1)
website/modules/@apostrophecms/seo/index.js (1)
23-26: GTM sanitization OK; templates & resolver present — DRY optionalShort summary: sanitizeGtmId enforces a strict GTM pattern and resolveGtmId uses it; tag-manager templates exist. Small DRY improvement is optional. package.json was not found so I could not confirm npm package removal; app.js registers '@apostrophecms/seo' locally.
Files/locations:
- website/modules/@apostrophecms/seo/lib/gtm-utils.js
- sanitizeGtmId: String(id||'').trim().toUpperCase(); validates with /^GTM-[\dA-Z]+$/u and returns the value only if it matches.
- resolveGtmId: reads req?.data?.global?.seoGoogleTagManager or options?.googleTagManager?.id, trims and returns sanitizeGtmId(candidate).
- website/modules/@apostrophecms/seo/index.js
- Calls gtmUtils.resolveGtmId(req, self.options) at the spots you flagged (around lines 23–26 and 33–36).
- Templates present: website/modules/@apostrophecms/seo/views/tagManagerHead.html, tagManagerBody.html, gtmHead.html, gtmBody.html
- app.js: contains '@apostrophecms/seo' registration (line ~38) indicating the module is configured locally.
- package.json: not present in repo root — cannot verify npm package removal from package.json.
Optional small DRY suggestion (unchanged from original):
const resolve = (req) => gtmUtils.resolveGtmId(req, self.options);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
website/modules/@apostrophecms/seo/index.js(3 hunks)website/modules/@apostrophecms/seo/lib/gtm-utils.js(1 hunks)website/views/layout.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- website/modules/@apostrophecms/seo/lib/gtm-utils.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-02T16:04:13.069Z
Learnt from: IhorMasechko
PR: speedandfunction/website#143
File: website/modules/default-page/index.js:24-24
Timestamp: 2025-06-02T16:04:13.069Z
Learning: In ApostropheCMS projects, even when a global page-type module removes the 'orphan' field, individual page modules (like default-page, home-page, case-studies-page) may still need explicit `remove: ['orphan']` for the removal to work correctly. The global improvement may not be sufficient in all cases.
Applied to files:
website/modules/@apostrophecms/seo/index.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: e2e-tests
- GitHub Check: lint
- GitHub Check: unit-tests
- GitHub Check: security-scan
🔇 Additional comments (2)
website/views/layout.html (1)
21-21: Correct placement of GTM noscript blockRendering tagManagerBody in beforeMain puts the noscript iframe near the top of the body as recommended by GTM. Looks good.
website/modules/@apostrophecms/seo/index.js (1)
1-1: Centralizing GTM ID logic in a utility is the right moveImporting gtm-utils improves cohesion and keeps the component lean.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
website/modules/@apostrophecms/seo/index.js (1)
10-14: Remove auto-injection to prevent duplicate GTM tags (layout already renders them)Layout renders tagManagerHead/Body; keeping these insertions duplicates the tags and can cause GTM issues. Remove these injections.
Apply this diff:
init(self) { - // Ensure SEO components are injected into the template - self.apos.template.prepend('body', '@apostrophecms/seo:tagManagerBody'); - self.apos.template.append('head', '@apostrophecms/seo:tagManagerHead'); - self.apos.template.prepend('head', '@apostrophecms/seo:metaHead'); + // Layout handles rendering of SEO components explicitly. },
🧹 Nitpick comments (1)
website/modules/@apostrophecms/seo/index.js (1)
4-8: Sane default for GTM ID via env; consider consistency with app-level config (optional)Using process.env for a default is fine. If you want uniform config handling across the codebase, you could optionally source this via the same getEnv mechanism used in app.js and pass it in the modules config. Not required for functionality, just consistency.
Apply in app.js (optional):
- // GTM setup handled by local SEO module + // GTM setup handled by local SEO module + '@apostrophecms/seo': { + options: { + googleTagManager: { + id: getEnv('GOOGLE_TAG_MANAGER_ID'), + }, + }, + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
website/app.js(1 hunks)website/modules/@apostrophecms/seo/index.js(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-02T16:04:13.069Z
Learnt from: IhorMasechko
PR: speedandfunction/website#143
File: website/modules/default-page/index.js:24-24
Timestamp: 2025-06-02T16:04:13.069Z
Learning: In ApostropheCMS projects, even when a global page-type module removes the 'orphan' field, individual page modules (like default-page, home-page, case-studies-page) may still need explicit `remove: ['orphan']` for the removal to work correctly. The global improvement may not be sufficient in all cases.
Applied to files:
website/modules/@apostrophecms/seo/index.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: e2e-tests
- GitHub Check: unit-tests
- GitHub Check: lint
- GitHub Check: security-scan
🔇 Additional comments (3)
website/modules/@apostrophecms/seo/index.js (3)
1-1: Centralizing GTM resolution via a small utility is the right callImporting gtm-utils keeps the template components lean and the validation logic reusable. LGTM.
28-31: GTM ID resolution is clean and guarded by req.data.pageOnly resolves and passes gtmId on actual page requests; templates can rely on sanitized, uppercase IDs. LGTM.
Also applies to: 38-41
17-23: Don't remove metaHead — it's still registered by the template systemShort check shows metaHead is still referenced, so removing it would be incorrect:
- website/modules/@apostrophecms/seo/index.js:13 — self.apos.template.prepend('head', '@apostrophecms/seo:metaHead');
- website/modules/@apostrophecms/seo/index.js:17 — metaHead definition
If your intent is to stop auto-injection, remove the template.prepend call instead and verify there are no other callers; otherwise ignore the original diff.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
website/modules/@apostrophecms/seo/index.js (1)
15-23: If metaHead is unused, remove it to reduce surface area.Given layout only calls tagManagerHead/Body, keeping metaHead is unnecessary. Removing it avoids confusion and dead code.
Apply this diff:
return { - metaHead(req, data) { - // Only on front-end page requests - if (!req?.data?.page) { - return {}; - } - return {}; - },
🧹 Nitpick comments (2)
website/modules/@apostrophecms/seo/index.js (2)
9-14: Remove no-op init hook; it adds noise without behavior.The init hook only contains a comment. Drop it to keep the module lean.
Apply this diff:
- init(self) { - /* - * SEO components are called directly from layout.html template - * No automatic injection needed to avoid duplication - */ - }, + // No init hook required (layout renders components explicitly).
28-28: Minor DRY: factor out GTM ID resolution within components.Both components resolve the GTM ID identically. A tiny in-scope helper keeps this consistent and easier to tweak.
Apply this diff:
components(self) { - return { + const getGtmId = (req) => gtmUtils.resolveGtmId(req, self.options); + return { metaHead(req, data) { // Only on front-end page requests if (!req?.data?.page) { return {}; } return {}; }, tagManagerBody(req, data) { if (!req?.data?.page) { return {}; } - const gtmId = gtmUtils.resolveGtmId(req, self.options); + const gtmId = getGtmId(req); if (gtmId) { return { gtmId }; } return {}; }, tagManagerHead(req, data) { if (!req?.data?.page) { return {}; } - const gtmId = gtmUtils.resolveGtmId(req, self.options); + const gtmId = getGtmId(req); if (gtmId) { return { gtmId }; } return {}; }, }; },Also applies to: 38-38
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (3)
website/app.js(1 hunks)website/modules/@apostrophecms/seo/index.js(3 hunks)website/views/layout.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- website/views/layout.html
- website/app.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-02T16:04:13.069Z
Learnt from: IhorMasechko
PR: speedandfunction/website#143
File: website/modules/default-page/index.js:24-24
Timestamp: 2025-06-02T16:04:13.069Z
Learning: In ApostropheCMS projects, even when a global page-type module removes the 'orphan' field, individual page modules (like default-page, home-page, case-studies-page) may still need explicit `remove: ['orphan']` for the removal to work correctly. The global improvement may not be sufficient in all cases.
Applied to files:
website/modules/@apostrophecms/seo/index.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: lint
- GitHub Check: e2e-tests
- GitHub Check: security-scan
- GitHub Check: unit-tests
🔇 Additional comments (2)
website/modules/@apostrophecms/seo/index.js (2)
4-7: Sensible fallback for GTM ID via options (env).Defaulting GTM ID from process.env is a pragmatic fallback and aligns with the simplified approach. Assuming gtm-utils sanitizes invalid values to empty, this is safe and non-breaking.
1-1: GTM helper + component templates verified — no action required.Verified:
- website/modules/@apostrophecms/seo/lib/gtm-utils.js — defines sanitizeGtmId and resolveGtmId and exports them (module.exports).
- website/modules/@apostrophecms/seo/views/tagManagerHead.html — present.
- website/modules/@apostrophecms/seo/views/tagManagerBody.html — present.
- website/views/layout.html — invokes both components ({% component '@apostrophecms/seo:tagManagerHead' with data %} at line 17 and {% component '@apostrophecms/seo:tagManagerBody' with data %} at line 21).
No missing files or export issues found.
|
2 participants
Fix the error during the Dev deploy
TypeError: self.prependNodes is not a function
at Object.init (/app/node_modules/@apostrophecms/seo/index.js:20:10)
at self.create (/app/node_modules/apostrophe/lib/moog.js:310:20)
at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
at async instantiateModules (/app/node_modules/apostrophe/index.js:669:32)
at async apostrophe (/app/node_modules/apostrophe/index.js:319:5)
at async /app/node_modules/apostrophe/index.js:160:17
at async module.exports (/app/node_modules/apostrophe/index.js:159:16)
Get rid of the dependency on the contributed SEO package, simplify the approach to insert the scripts in the template.