Conversation
✅ Deploy Preview for openms ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughAdds a new Tally embed partial that initializes Changes
Sequence Diagram(s)sequenceDiagram
participant Page as Site Template
participant Browser
participant Tally as Tally Embed (external)
participant User
Page->>Browser: serve HTML (includes tally partial)
Browser->>Browser: set window.TallyConfig { formId:"pbV6Qq", popup:{...} }
User->>Browser: scroll / trigger event
Browser->>Browser: popup open logic (per TallyConfig.open.trigger)
Browser->>Tally: load embed script request (https://tally.so/widgets/embed.js)
Tally-->>Browser: script loads and initializes with window.TallyConfig
Browser->>User: show popup widget (emoji, animation)
User->>Browser: submit form
Browser->>Browser: honor doNotShowAfterSubmit / showOnce behavior
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
layouts/index.html (1)
42-42: Add a site param gate for quick rollback/tuning.Line 42 always enables the popup. Given UX sensitivity, add a config flag so maintainers can disable or A/B this without template edits.
🔧 Suggested change
- {{ partial "tally.html" . }} + {{ if .Site.Params.tallySurvey.enabled }} + {{ partial "tally.html" . }} + {{ end }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@layouts/index.html` at line 42, Wrap the unconditional partial call to "tally.html" in a site-parameter gate so maintainers can toggle it from config; specifically, change the layout that currently calls partial "tally.html" with context (the line containing partial "tally.html" .) to be executed only when a site param (e.g., .Site.Params.enableTally or .Site.Params.showTally) is true — update the template to check that param before invoking partial "tally.html" and document the new boolean param in site params for quick rollback/A-B toggling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@layouts/partials/tally.html`:
- Line 19: The third-party script tag <script async
src="https://tally.so/widgets/embed.js"></script> is loaded unconditionally;
change it to only load after user consent by replacing the static tag with a
consent-gated loader: render a non-functional placeholder (or load button)
instead, check your consent state (e.g., consent manager cookie or JS API) and
only dynamically create/append the tally.so script when consent is granted, and
also listen for consent changes to inject the script when the user later gives
consent; update any initialization that expects the widget to exist to run after
the dynamic script insertion (i.e., ensure the loader logic triggers the
existing widget init code).
---
Nitpick comments:
In `@layouts/index.html`:
- Line 42: Wrap the unconditional partial call to "tally.html" in a
site-parameter gate so maintainers can toggle it from config; specifically,
change the layout that currently calls partial "tally.html" with context (the
line containing partial "tally.html" .) to be executed only when a site param
(e.g., .Site.Params.enableTally or .Site.Params.showTally) is true — update the
template to check that param before invoking partial "tally.html" and document
the new boolean param in site params for quick rollback/A-B toggling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 28213438-5a99-406d-809b-3d8d280c0b7a
📒 Files selected for processing (2)
layouts/index.htmllayouts/partials/tally.html
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
layouts/partials/tally.html (1)
19-27: Complete the consent integration or remove the placeholder.The code contains a placeholder for a consent check, but the variable
window.userHasConsentedToExternalEmbedsis never initialized anywhere in the codebase. As written, this condition will never be true, so the Tally script won't load.Consider either:
- Integrating with your site's actual CMP/consent API (if one exists).
- If no consent mechanism is required for this site, removing the conditional and loading directly.
- Adding a listener for consent state changes to handle delayed consent, along with properly initializing the consent variable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@layouts/partials/tally.html` around lines 19 - 27, The placeholder consent check uses window.userHasConsentedToExternalEmbeds (which is never initialized) so the Tally embed will never load; either integrate this check with your real CMP API by reading the actual consent state and/or subscribing to consent change events to append the Tally script (the logic around creating the script element, setting s.async and s.src and appending to document.head is in this block), or if no CMP is required remove the conditional and always create/append the Tally script; ensure any added listener updates/reads the same window.userHasConsentedToExternalEmbeds flag or directly calls the script-injection logic when consent becomes true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@layouts/partials/tally.html`:
- Around line 19-27: The placeholder consent check uses
window.userHasConsentedToExternalEmbeds (which is never initialized) so the
Tally embed will never load; either integrate this check with your real CMP API
by reading the actual consent state and/or subscribing to consent change events
to append the Tally script (the logic around creating the script element,
setting s.async and s.src and appending to document.head is in this block), or
if no CMP is required remove the conditional and always create/append the Tally
script; ensure any added listener updates/reads the same
window.userHasConsentedToExternalEmbeds flag or directly calls the
script-injection logic when consent becomes true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e37bab03-9704-44b1-bd9a-8e2554834cf2
📒 Files selected for processing (1)
layouts/partials/tally.html
|
Thanks @matteopilz was having difficulty getting it to build. |
Replaced consent-based script loading with a direct async script tag for Tally widget.
|
The coderabbit recommendation did not work for me, so I removed it. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
layouts/partials/tally.html (1)
19-19:⚠️ Potential issue | 🟠 MajorGate third-party script loading behind consent.
Line 19 still loads
tally.sounconditionally. If your site requires prior consent for non-essential embeds, this remains a compliance/privacy risk.🔧 Suggested change
-<script async src="https://tally.so/widgets/embed.js"></script> +<script> + (function loadTallyWhenConsented() { + // Wire this to your actual CMP signal/API + const hasConsent = window.userHasConsentedToExternalEmbeds === true; + if (!hasConsent || document.getElementById("tally-embed-script")) return; + + const s = document.createElement("script"); + s.id = "tally-embed-script"; + s.async = true; + s.src = "https://tally.so/widgets/embed.js"; + document.head.appendChild(s); + })(); +</script>Use this read-only check to confirm whether a consent signal already exists and how other third-party scripts are handled:
#!/bin/bash set -euo pipefail echo "== Consent/CMP signals in repo ==" rg -n -C2 -i 'consent|cmp|gdpr|ccpa|cookiebot|onetrust|userHasConsented|analytics_storage|ad_storage' echo echo "== Third-party script loading patterns ==" rg -n -C2 '<script[^>]+src=' layouts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@layouts/partials/tally.html` at line 19, layouts/partials/tally.html currently unconditionally injects the Tally embed script (https://tally.so/widgets/embed.js); change this to only load when a consent signal is present by: remove the inline script tag and instead conditionally create/append the external script when your consent API returns true (e.g., check userHasConsented(), consentManager.hasConsent('third_party'), or analyticsConsent flag used elsewhere); alternatively mark the script with a non-executing type (e.g., type="text/plain" or data-src) and have the same consent-success handler replace it with a proper script element that sets src to https://tally.so/widgets/embed.js; update any initialization that expects Tally to ensure it runs after the script load.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@layouts/partials/tally.html`:
- Line 19: layouts/partials/tally.html currently unconditionally injects the
Tally embed script (https://tally.so/widgets/embed.js); change this to only load
when a consent signal is present by: remove the inline script tag and instead
conditionally create/append the external script when your consent API returns
true (e.g., check userHasConsented(), consentManager.hasConsent('third_party'),
or analyticsConsent flag used elsewhere); alternatively mark the script with a
non-executing type (e.g., type="text/plain" or data-src) and have the same
consent-success handler replace it with a proper script element that sets src to
https://tally.so/widgets/embed.js; update any initialization that expects Tally
to ensure it runs after the script load.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6f55764f-c32b-4edc-877e-24b51d9cc5ab
📒 Files selected for processing (1)
layouts/partials/tally.html
|
Yes seems to be working now. It did not work for me either because I think codderabbit created a dummy variable that did not actually exist. I think we have an actual variable somewhere for checking consent with analytics but I don't know where that is. Having said that I am not sure if we need to consent with this since consent would just be implied if they fill out the form? |
Yes, I think using it without consent is fine. It would have been nice, but its fine like this, too. |
2 participants
This adds the tally.so user survey to the home page as a pop up. I am not sure if this is the best way to add it as I am not too familiar with website design so input is appreciated.
Also I don't know if it is too intrusive, but I kind of want it to be for people to fill it out.
Summary by CodeRabbit