chore: enable umami on production only#131
Conversation
WalkthroughThe changes remove the hardcoded Umami Analytics script from the HTML and instead introduce a React component, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant UmamiScriptLoader
participant Document
User->>App: Loads application
App->>UmamiScriptLoader: Renders component
UmamiScriptLoader->>Document: On mount, injects Umami script (if production)
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (2)
src/renderer/src/components/analytics/umami.tsx (2)
10-15: Consider error handling and configuration externalization.The script creation and configuration logic is solid, but consider these improvements:
- Add error handling for script loading failures
- Consider externalizing the hardcoded configuration values to environment variables or a config file
const script = document.createElement('script'); script.src = '/umami.js'; script.defer = true; script.setAttribute('data-host-url', 'https://umami.iamevan.dev'); script.setAttribute('data-website-id', '846df382-cb68-4e59-a97e-76df33a73e90'); + +script.onerror = () => { + console.warn('Failed to load Umami analytics script'); +};
7-21: ```bash
#!/bin/bashLocate all usages of the UmamiScriptLoader component to check if it can mount multiple times
rg -n "UmamiScriptLoader" -g '*.tsx' -A3 -B3
</blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** **Plan: Pro** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between c5cd6d8f9059b4d7c61bf08765b18340c387cbd2 and 20b4a75956357df0218245863f1319d4c18d73e2. </details> <details> <summary>📒 Files selected for processing (3)</summary> * `src/renderer/index.html` (0 hunks) * `src/renderer/src/App.tsx` (2 hunks) * `src/renderer/src/components/analytics/umami.tsx` (1 hunks) </details> <details> <summary>💤 Files with no reviewable changes (1)</summary> * src/renderer/index.html </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>🧬 Code Graph Analysis (1)</summary> <details> <summary>src/renderer/src/App.tsx (2)</summary><blockquote> <details> <summary>src/renderer/src/components/analytics/umami.tsx (1)</summary> * `UmamiScriptLoader` (3-24) </details> <details> <summary>src/renderer/src/components/main/platform.tsx (1)</summary> * `PlatformProvider` (91-121) </details> </blockquote></details> </details> </details> <details> <summary>⏰ Context from checks skipped due to timeout of 90000ms (5)</summary> * GitHub Check: build (macos-13) * GitHub Check: build (windows-latest) * GitHub Check: build (macos-latest) * GitHub Check: build (ubuntu-24.04-arm) * GitHub Check: build (ubuntu-latest) </details> <details> <summary>🔇 Additional comments (4)</summary><blockquote> <details> <summary>src/renderer/src/components/analytics/umami.tsx (1)</summary> `8-8`: **LGTM: Production environment check is correct.** The condition properly restricts analytics loading to production environments only, which aligns with the PR objectives. </details> <details> <summary>src/renderer/src/App.tsx (3)</summary> `3-3`: **LGTM: Import statement is correct.** The import path and naming convention follow the project structure properly. --- `73-82`: **LGTM: Component integration is well-structured.** The UmamiScriptLoader is correctly placed at the top level of the App component, ensuring early analytics initialization while maintaining the existing component hierarchy. The React fragment wrapper is appropriate for grouping multiple root elements. --- `4-5`: **Import reordering looks good.** The RouterProvider and Route imports have been reordered, which maintains consistency in the import organization. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| return () => { | ||
| document.body.removeChild(script); | ||
| }; |
There was a problem hiding this comment.
Fix potential error in script cleanup.
The cleanup function could throw an error if the script element doesn't exist in the DOM or was already removed.
Apply this diff to add safe cleanup:
return () => {
- document.body.removeChild(script);
+ if (script.parentNode) {
+ script.parentNode.removeChild(script);
+ }
};📝 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.
| return () => { | |
| document.body.removeChild(script); | |
| }; | |
| return () => { | |
| if (script.parentNode) { | |
| script.parentNode.removeChild(script); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In src/renderer/src/components/analytics/umami.tsx around lines 18 to 20, the
cleanup function removes the script element without checking if it exists in the
DOM, which can cause an error. Modify the cleanup function to first check if the
script element is still a child of document.body before attempting to remove it,
ensuring safe cleanup without errors.
| script.setAttribute('data-host-url', 'https://umami.iamevan.dev'); | ||
| script.setAttribute('data-website-id', '846df382-cb68-4e59-a97e-76df33a73e90'); | ||
|
|
||
| document.body.appendChild(script); |
There was a problem hiding this comment.
Why is this needed? Can't you just do that below?
<script
defer
src="/umami.js"
data-host-url="https://umami.iamevan.dev"
data-website-id="846df382-cb68-4e59-a97e-76df33a73e90"
></script>There was a problem hiding this comment.
If you enable it like this, it will also run during development, which isn't necessary and may capture random data, since developers often perform non-typical actions. This data won't help analyze how a 'normal' user behaves.
MHase
force-pushed
the
chore/analytics-prod-only
branch
from
20b4a75 to
1d2b4a3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/renderer/src/components/analytics/umami.tsx (2)
2-3: Add TypeScript return type annotation for better type safety.Consider adding an explicit return type annotation to improve type safety and code documentation.
-export const UmamiScriptLoader = () => { +export const UmamiScriptLoader = (): JSX.Element | null => {
8-13: Clarify script source and consider environment-based configuration.The implementation is clean and uses the JSX approach effectively. However, consider these improvements:
Script source clarity: The script loads from
/umami.js(local) but connects to an external host. Ensure the Umami script file is available in your public folder.Environment-based configuration: The hardcoded values could be moved to environment variables for better flexibility across different environments.
Consider this approach for environment-based configuration:
return <script defer src="/umami.js" - data-host-url="https://umami.iamevan.dev" - data-website-id="846df382-cb68-4e59-a97e-76df33a73e90" + data-host-url={process.env.VITE_UMAMI_HOST_URL || "https://umami.iamevan.dev"} + data-website-id={process.env.VITE_UMAMI_WEBSITE_ID || "846df382-cb68-4e59-a97e-76df33a73e90"} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/renderer/index.html(0 hunks)src/renderer/src/App.tsx(2 hunks)src/renderer/src/components/analytics/umami.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- src/renderer/index.html
🚧 Files skipped from review as they are similar to previous changes (1)
- src/renderer/src/App.tsx
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (ubuntu-24.04-arm)
- GitHub Check: build (macos-latest)
- GitHub Check: build (windows-latest)
- GitHub Check: build (macos-13)
- GitHub Check: build (ubuntu-latest)
🔇 Additional comments (1)
src/renderer/src/components/analytics/umami.tsx (1)
5-7: Well-documented analytics choice.The comments clearly explain the privacy-focused nature of Umami Analytics, which helps other developers understand the rationale behind this analytics solution.
2 participants
Refactor of the Umami analytics integration by:
(so analytics won't be affected with fake data during development)
Summary by CodeRabbit
New Features
Refactor