Conversation
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary: Migration from Jekyll to React
This PR migrates the blog from Jekyll to React with Vite. While the migration approach is sound, there are critical blocking issues that must be addressed before merge:
Critical Issues (Must Fix)
- ESLint Configuration Errors: Invalid imports and syntax that will prevent linting from working
- GitHub Actions Deployment: Missing authentication token will cause deployment failures
- Analytics Implementation: Logic error in gtag function implementation will break tracking
- Runtime Safety: Missing error handling in main.jsx could cause application crashes
- Security: Hardcoded tracking ID should be moved to environment variables
All critical issues have been commented inline with specific fixes. Please address these before merging to ensure the site deploys and functions correctly.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| "siteName": "Gapry's Technical Writing", | ||
| "analytics": { | ||
| "provider": "google-gtag", | ||
| "trackingId": "G-4DG3HSBT1T", |
There was a problem hiding this comment.
🛑 Security Vulnerability: Hardcoded Google Analytics tracking ID is committed to the repository. This tracking ID should be treated as sensitive configuration and moved to environment variables to prevent unauthorized usage and potential data collection hijacking.
"trackingId": import.meta.env.VITE_GA_TRACKING_ID,
.github/workflows/gh-pages.yml
Outdated
| - name: Deploy to GitHub Pages | ||
| uses: JamesIves/github-pages-deploy-action@v4 | ||
| with: | ||
| folder: dist | ||
| branch: gh-pages No newline at end of file |
There was a problem hiding this comment.
🛑 Deployment Failure: Missing authentication token for GitHub Pages deployment. The JamesIves/github-pages-deploy-action@v4 requires a token parameter for authentication, or the workflow will fail during deployment.
| - name: Deploy to GitHub Pages | |
| uses: JamesIves/github-pages-deploy-action@v4 | |
| with: | |
| folder: dist | |
| branch: gh-pages | |
| - name: Deploy to GitHub Pages | |
| uses: JamesIves/github-pages-deploy-action@v4 | |
| with: | |
| token: ${{ secrets.GITHUB_TOKEN }} | |
| folder: dist | |
| branch: gh-pages |
| createRoot(document.getElementById('root')).render( | ||
| <StrictMode> | ||
| <App /> | ||
| </StrictMode>, | ||
| ) |
There was a problem hiding this comment.
🛑 Crash Risk: Missing null check for DOM element. If the 'root' element doesn't exist in the HTML, createRoot will throw an error and crash the application. Add error handling to prevent runtime failure.
| createRoot(document.getElementById('root')).render( | |
| <StrictMode> | |
| <App /> | |
| </StrictMode>, | |
| ) | |
| const rootElement = document.getElementById('root'); | |
| if (!rootElement) { | |
| throw new Error('Root element not found. Please ensure index.html contains a div with id="root".'); | |
| } | |
| createRoot(rootElement).render( | |
| <StrictMode> | |
| <App /> | |
| </StrictMode>, | |
| ) |
src/Analytics.jsx
Outdated
| window.gtag = function() { | ||
| window.dataLayer.push(arguments); | ||
| }; |
There was a problem hiding this comment.
🛑 Logic Error: Unsafe use of arguments in arrow function. Arrow functions don't have their own arguments object, causing this gtag implementation to fail. Use rest parameters instead to properly forward arguments to dataLayer.push.
| window.gtag = function() { | |
| window.dataLayer.push(arguments); | |
| }; | |
| window.gtag = function(...args) { | |
| window.dataLayer.push(args); | |
| }; |
| import globals from 'globals' | ||
| import reactHooks from 'eslint-plugin-react-hooks' | ||
| import reactRefresh from 'eslint-plugin-react-refresh' | ||
| import { defineConfig, globalIgnores } from 'eslint/config' |
There was a problem hiding this comment.
🛑 Syntax Error: Invalid import from 'eslint/config' - this module doesn't exist in the eslint package. Remove this import and the defineConfig usage, or use the correct ESLint flat config export format. This will cause configuration loading to fail.
import { defineConfig, globalIgnores } from 'eslint/config'| { | ||
| files: ['**/*.{js,jsx}'], | ||
| extends: [ | ||
| js.configs.recommended, | ||
| reactHooks.configs.flat.recommended, | ||
| reactRefresh.configs.vite, | ||
| ], | ||
| languageOptions: { | ||
| ecmaVersion: 2020, | ||
| globals: globals.browser, | ||
| parserOptions: { | ||
| ecmaVersion: 'latest', | ||
| ecmaFeatures: { jsx: true }, | ||
| sourceType: 'module', | ||
| }, | ||
| }, | ||
| rules: { | ||
| 'no-unused-vars': ['error', { varsIgnorePattern: '^[A-Z_]' }], | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🛑 Syntax Error: Invalid usage of extends property in ESLint flat config format. The flat config format doesn't support the extends property - configs should be spread directly into the array. This will cause ESLint to fail with a configuration error.
| { | |
| files: ['**/*.{js,jsx}'], | |
| extends: [ | |
| js.configs.recommended, | |
| reactHooks.configs.flat.recommended, | |
| reactRefresh.configs.vite, | |
| ], | |
| languageOptions: { | |
| ecmaVersion: 2020, | |
| globals: globals.browser, | |
| parserOptions: { | |
| ecmaVersion: 'latest', | |
| ecmaFeatures: { jsx: true }, | |
| sourceType: 'module', | |
| }, | |
| }, | |
| rules: { | |
| 'no-unused-vars': ['error', { varsIgnorePattern: '^[A-Z_]' }], | |
| }, | |
| }, | |
| { | |
| files: ['**/*.{js,jsx}'], | |
| ...js.configs.recommended, | |
| ...reactHooks.configs.flat.recommended, | |
| ...reactRefresh.configs.vite, | |
| languageOptions: { | |
| ecmaVersion: 2020, | |
| globals: globals.browser, | |
| parserOptions: { | |
| ecmaVersion: 'latest', | |
| ecmaFeatures: { jsx: true }, | |
| sourceType: 'module', | |
| }, | |
| }, | |
| rules: { | |
| 'no-unused-vars': ['error', { varsIgnorePattern: '^[A-Z_]' }], | |
| }, | |
| }, |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request undertakes a significant architectural change by migrating the blog from a Jekyll-based static site generator to a dynamic React application using Vite. This transition modernizes the underlying technology stack, moving from Ruby-based templating to a JavaScript component-driven approach. The primary goal is to enhance maintainability, improve development experience, and provide a more flexible foundation for future features and content delivery. Highlights
Changelog
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request marks a significant migration of the blog's technical stack from Jekyll to React with Vite. The changes involve removing the old Jekyll structure and introducing a new, modern frontend setup. This includes configurations for development tools like Vite and ESLint, and a basic application structure.
While the new foundation is well-established, I've identified a critical issue in the public/robots.txt file. It contains what appears to be a shell command instead of the actual directives, which will make it invalid for search engine crawlers.
Additionally, this PR removes all existing blog content. It would be helpful to include a brief note in the PR description outlining the strategy for content migration to the new platform. Overall, this is a good foundational step for the new blog architecture.
public/robots.txt
Outdated
| echo "User-agent: * | ||
| Allow: /" > public/robots.txt No newline at end of file |
There was a problem hiding this comment.
The content of robots.txt appears to be a shell command instead of the actual robots.txt rules. The file should contain the rules directly, not the command used to generate them. This will result in an invalid robots.txt file on the deployed site, which can negatively impact SEO by preventing search engines from crawling your site.
User-agent: *
Allow: /
|
/gemini review |
|
/q review |
There was a problem hiding this comment.
Summary
This PR successfully migrates from Jekyll to React, but several critical issues must be addressed before merge:
Critical Issues Found (6)
- Security: Hardcoded Google Analytics tracking ID in config.json should be externalized to environment variables
- Security: XSS vulnerability in Analytics.jsx - trackingId needs sanitization with encodeURIComponent
- Memory Leak: Missing cleanup function in Analytics useEffect causes script tag accumulation on remount
- Crash Risk: Missing null check for root element in main.jsx will crash app if DOM element not found
- Configuration Error: ESLint flat config using invalid
extendsproperty will cause linting failures - Deployment Logic: GitHub Actions workflow deploys on every PR, risking production overwrites with unreviewed code
Migration Assessment
The core migration approach is sound - switching from Jekyll to Vite + React is appropriate for a modern blog platform. The build pipeline and dependencies are properly configured. However, the security vulnerabilities and logic errors identified above must be fixed to ensure reliable operation.
All comments include specific code suggestions that can be applied directly from the GitHub UI.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| "siteName": "Gapry's Technical Writing", | ||
| "analytics": { | ||
| "provider": "google-gtag", | ||
| "trackingId": "G-4DG3HSBT1T", |
There was a problem hiding this comment.
🛑 Security Vulnerability: Hardcoded Google Analytics tracking ID should be externalized to environment variables to prevent unauthorized usage and enable different configurations per environment.1
Footnotes
-
CWE-798: Use of Hard-coded Credentials - https://cwe.mitre.org/data/definitions/798.html ↩
src/Analytics.jsx
Outdated
| const script = document.createElement('script'); | ||
|
|
||
| script.id = 'google-analytics'; | ||
| script.src = `https://www.googletagmanager.com/gtag/js?id=${trackingId}`; |
There was a problem hiding this comment.
🛑 Security Vulnerability: Template literal injection risk - the trackingId from config.json is directly interpolated into the script URL without validation or sanitization, creating an XSS vulnerability if the config file is compromised.1
| script.src = `https://www.googletagmanager.com/gtag/js?id=${trackingId}`; | |
| script.src = ` |
Footnotes
-
CWE-79: Cross-site Scripting (XSS) - https://cwe.mitre.org/data/definitions/79.html ↩
src/Analytics.jsx
Outdated
| useEffect(() => { | ||
| if (!trackingId || document.getElementById('google-analytics')) { | ||
| return; | ||
| } | ||
|
|
||
| const script = document.createElement('script'); | ||
|
|
||
| script.id = 'google-analytics'; | ||
| script.src = `https://www.googletagmanager.com/gtag/js?id=${trackingId}`; | ||
| script.async = true; | ||
|
|
||
| document.head.appendChild(script); | ||
|
|
||
| window.dataLayer = window.dataLayer || []; | ||
|
|
||
| window.gtag = function() { | ||
| window.dataLayer.push(arguments); | ||
| }; | ||
|
|
||
| window.gtag('js', new Date()); | ||
| window.gtag('config', trackingId, { | ||
| anonymize_ip: siteConfig.analytics.anonymizeIp | ||
| }); | ||
| }, [trackingId]); |
There was a problem hiding this comment.
🛑 Logic Error: Missing cleanup function in useEffect causes memory leak - the dynamically created script element is never removed when the component unmounts, leading to duplicate script tags and potential tracking issues on component remount.
| useEffect(() => { | |
| if (!trackingId || document.getElementById('google-analytics')) { | |
| return; | |
| } | |
| const script = document.createElement('script'); | |
| script.id = 'google-analytics'; | |
| script.src = `https://www.googletagmanager.com/gtag/js?id=${trackingId}`; | |
| script.async = true; | |
| document.head.appendChild(script); | |
| window.dataLayer = window.dataLayer || []; | |
| window.gtag = function() { | |
| window.dataLayer.push(arguments); | |
| }; | |
| window.gtag('js', new Date()); | |
| window.gtag('config', trackingId, { | |
| anonymize_ip: siteConfig.analytics.anonymizeIp | |
| }); | |
| }, [trackingId]); | |
| useEffect(() => { | |
| if (!trackingId || document.getElementById('google-analytics')) { | |
| return; | |
| } | |
| const script = document.createElement('script'); | |
| script.id = 'google-analytics'; | |
| script.src = ` | |
| script.async = true; | |
| document.head.appendChild(script); | |
| window.dataLayer = window.dataLayer || []; | |
| window.gtag = function() { | |
| window.dataLayer.push(arguments); | |
| }; | |
| window.gtag('js', new Date()); | |
| window.gtag('config', trackingId, { | |
| anonymize_ip: siteConfig.analytics.anonymizeIp | |
| }); | |
| return () => { | |
| const scriptElement = document.getElementById('google-analytics'); | |
| if (scriptElement) { | |
| scriptElement.remove(); | |
| } | |
| }; | |
| }, [trackingId]); |
| createRoot(document.getElementById('root')).render( | ||
| <StrictMode> | ||
| <App /> | ||
| </StrictMode>, | ||
| ) |
There was a problem hiding this comment.
🛑 Crash Risk: Missing null check - if the 'root' element doesn't exist in the DOM, createRoot will throw an error causing the application to crash on page load.
| createRoot(document.getElementById('root')).render( | |
| <StrictMode> | |
| <App /> | |
| </StrictMode>, | |
| ) | |
| const rootElement = document.getElementById('root'); | |
| if (!rootElement) { | |
| throw new Error('Root element not found'); | |
| } | |
| createRoot(rootElement).render( | |
| <StrictMode> | |
| <App /> | |
| </StrictMode>, | |
| ) |
| { | ||
| files: ['**/*.{js,jsx}'], | ||
| extends: [ | ||
| js.configs.recommended, | ||
| reactHooks.configs.flat.recommended, | ||
| reactRefresh.configs.vite, | ||
| ], | ||
| languageOptions: { | ||
| ecmaVersion: 2020, | ||
| globals: globals.browser, | ||
| parserOptions: { | ||
| ecmaVersion: 'latest', | ||
| ecmaFeatures: { jsx: true }, | ||
| sourceType: 'module', | ||
| }, | ||
| }, | ||
| rules: { | ||
| 'no-unused-vars': ['error', { varsIgnorePattern: '^[A-Z_]' }], | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🛑 Logic Error: The extends property is invalid in ESLint flat config format (v9+) and will cause linting to fail. Flat config requires spreading the configs directly into the configuration object instead of using the extends property.
| { | |
| files: ['**/*.{js,jsx}'], | |
| extends: [ | |
| js.configs.recommended, | |
| reactHooks.configs.flat.recommended, | |
| reactRefresh.configs.vite, | |
| ], | |
| languageOptions: { | |
| ecmaVersion: 2020, | |
| globals: globals.browser, | |
| parserOptions: { | |
| ecmaVersion: 'latest', | |
| ecmaFeatures: { jsx: true }, | |
| sourceType: 'module', | |
| }, | |
| }, | |
| rules: { | |
| 'no-unused-vars': ['error', { varsIgnorePattern: '^[A-Z_]' }], | |
| }, | |
| }, | |
| { | |
| files: ['**/*.{js,jsx}'], | |
| ...js.configs.recommended, | |
| plugins: { | |
| 'react-hooks': reactHooks, | |
| 'react-refresh': reactRefresh, | |
| }, | |
| languageOptions: { | |
| ecmaVersion: 2020, | |
| globals: globals.browser, | |
| parserOptions: { | |
| ecmaVersion: 'latest', | |
| ecmaFeatures: { jsx: true }, | |
| sourceType: 'module', | |
| }, | |
| }, | |
| rules: { | |
| ...reactHooks.configs.recommended.rules, | |
| 'react-refresh/only-export-components': ['warn', { allowConstantExport: true }], | |
| 'no-unused-vars': ['error', { varsIgnorePattern: '^[A-Z_]' }], | |
| }, | |
| }, |
| build-and-deploy: | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
🛑 Logic Error: The workflow deploys to gh-pages branch on every pull request, which will overwrite production with unreviewed code. Deployment should only occur on push to main branch after PR approval.
| build-and-deploy: | |
| runs-on: ubuntu-latest | |
| build-and-deploy: | |
| runs-on: ubuntu-latest | |
| if: github.event_name == 'push' && github.ref == 'refs/heads/main' |
There was a problem hiding this comment.
Code Review
This pull request migrates the blog from Jekyll to a modern React setup using Vite. The changes involve removing all Jekyll-related files and introducing a standard Vite + React project structure. Overall, the migration is a significant step forward. I've identified a critical issue in the ESLint configuration that will prevent it from working, a high-severity bug in the Google Analytics component logic, and a medium-severity style issue.
| export default defineConfig([ | ||
| globalIgnores(['dist']), | ||
| { | ||
| files: ['**/*.{js,jsx}'], | ||
| extends: [ | ||
| js.configs.recommended, | ||
| reactHooks.configs.flat.recommended, | ||
| reactRefresh.configs.vite, | ||
| ], | ||
| languageOptions: { | ||
| ecmaVersion: 2020, | ||
| globals: globals.browser, | ||
| parserOptions: { | ||
| ecmaVersion: 'latest', | ||
| ecmaFeatures: { jsx: true }, | ||
| sourceType: 'module', | ||
| }, | ||
| }, | ||
| rules: { | ||
| 'no-unused-vars': ['error', { varsIgnorePattern: '^[A-Z_]' }], | ||
| }, | ||
| }, | ||
| ]) |
There was a problem hiding this comment.
Your ESLint configuration appears to be using the extends property within a flat config object, which is not supported in ESLint's new flat configuration format. In flat config, configurations are composed as an array of objects. Additionally, globalIgnores returns an array and should be spread into the main configuration array.
This will cause ESLint to fail. Here is a corrected version that follows the flat config structure:
export default defineConfig([
...globalIgnores(['dist']),
js.configs.recommended,
reactHooks.configs.flat.recommended,
reactRefresh.configs.vite,
{
files: ['**/*.{js,jsx}'],
languageOptions: {
ecmaVersion: 'latest',
globals: globals.browser,
parserOptions: {
ecmaFeatures: { jsx: true },
sourceType: 'module',
},
},
rules: {
'no-unused-vars': ['error', { varsIgnorePattern: '^[A-Z_]' }],
},
},
])
src/Analytics.jsx
Outdated
| useEffect(() => { | ||
| if (!trackingId || document.getElementById('google-analytics')) { | ||
| return; | ||
| } | ||
|
|
||
| const script = document.createElement('script'); | ||
|
|
||
| script.id = 'google-analytics'; | ||
| script.src = `https://www.googletagmanager.com/gtag/js?id=${trackingId}`; | ||
| script.async = true; | ||
|
|
||
| document.head.appendChild(script); | ||
|
|
||
| window.dataLayer = window.dataLayer || []; | ||
|
|
||
| window.gtag = function() { | ||
| window.dataLayer.push(arguments); | ||
| }; | ||
|
|
||
| window.gtag('js', new Date()); | ||
| window.gtag('config', trackingId, { | ||
| anonymize_ip: siteConfig.analytics.anonymizeIp | ||
| }); | ||
| }, [trackingId]); |
There was a problem hiding this comment.
This useEffect has a potential bug. If the trackingId changes, the effect will re-run, but because the script element with id='google-analytics' already exists, the function will return early without configuring the new trackingId. Also, window.gtag is only defined when the script is first injected, which can lead to issues on subsequent renders.
A better approach is to always ensure gtag is configured and only inject the script once. The anonymizeIp property should also be included in the dependency array.
useEffect(() => {
if (!trackingId) {
return;
}
// Ensure dataLayer and gtag stub are available.
window.dataLayer = window.dataLayer || [];
if (typeof window.gtag !== 'function') {
window.gtag = function() {
window.dataLayer.push(arguments);
};
window.gtag('js', new Date());
}
// Always configure on trackingId change.
window.gtag('config', trackingId, {
anonymize_ip: siteConfig.analytics.anonymizeIp,
});
// Inject the script only if it doesn't exist.
if (document.getElementById('google-analytics')) {
return;
}
const script = document.createElement('script');
script.id = 'google-analytics';
script.src = `https://www.googletagmanager.com/gtag/js?id=${trackingId}`;
script.async = true;
document.head.appendChild(script);
}, [trackingId, siteConfig.analytics.anonymizeIp]);| @@ -0,0 +1 @@ | |||
| My Technical Writing Licensed under **CC BY-NC-SA 4.0**, see [LICENSE](LICENSE) file. No newline at end of file | |||
There was a problem hiding this comment.
It's a good practice to end files with a newline character. Many tools and conventions expect this, and it can prevent issues with file concatenation or processing.
| My Technical Writing Licensed under **CC BY-NC-SA 4.0**, see [LICENSE](LICENSE) file. | |
| My Technical Writing Licensed under **CC BY-NC-SA 4.0**, see [LICENSE](LICENSE) file. | |
|
/gemini review |
|
/q review |
There was a problem hiding this comment.
Review Summary
This PR migrates from Jekyll to React with Vite and Bun, which is a significant architectural change. However, there are 4 critical issues that must be resolved before merging:
Critical Issues Found:
- Security: Hardcoded Google Analytics tracking ID in source code (should use environment variables)
- Build Breaking: Invalid ESLint import path (
eslint/configdoesn't exist in ESLint v9) - Syntax Error: Missing closing bracket in ESLint configuration
- Runtime Error: Missing error handling for Google Analytics script loading
Required Actions:
- Move the tracking ID to environment variables (
.envfile withVITE_GA_TRACKING_ID) - Fix ESLint configuration to use proper flat config syntax without the invalid import
- Add error handling for external script loading to prevent silent failures
Once these blocking issues are resolved, the migration should be ready for deployment.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| 'no-unused-vars': ['error', { varsIgnorePattern: '^[A-Z_]' }], | ||
| }, | ||
| }, | ||
| ]) |
There was a problem hiding this comment.
🛑 Logic Error: Missing closing bracket for the configuration array. Since the opening was changed, this closing bracket needs to be updated to properly close the array.
| ]) | |
| ] |
| "siteName": "Gapry's Technical Writing", | ||
| "analytics": { | ||
| "provider": "google-gtag", | ||
| "trackingId": "G-4DG3HSBT1T", |
There was a problem hiding this comment.
🛑 Security Vulnerability: Hardcoded Google Analytics tracking ID should be moved to environment variables. This exposes analytics configuration in version control and makes it difficult to use different tracking IDs across environments (development, staging, production).
| "trackingId": "G-4DG3HSBT1T", | |
| "trackingId": import.meta.env.VITE_GA_TRACKING_ID || "", |
| import { defineConfig, globalIgnores } from 'eslint/config' | ||
|
|
||
| export default defineConfig([ | ||
| globalIgnores(['dist']), |
There was a problem hiding this comment.
🛑 Logic Error: Import path eslint/config does not exist in ESLint v9. This will cause a runtime error when ESLint tries to load the configuration file. The correct approach for ESLint v9 flat config is to not use defineConfig and globalIgnores from this import.
| import { defineConfig, globalIgnores } from 'eslint/config' | |
| export default defineConfig([ | |
| globalIgnores(['dist']), | |
| export default [ | |
| { ignores: ['dist'] }, |
| const script = document.createElement('script'); | ||
| script.id = 'google-analytics'; | ||
| script.src = `https://www.googletagmanager.com/gtag/js?id=${trackingId}`; | ||
| script.async = true; | ||
|
|
||
| document.head.appendChild(script); |
There was a problem hiding this comment.
🛑 Crash Risk: Script loading failure is not handled. If the Google Analytics script fails to load (network error, blocked by ad blocker, CSP violation), the error will be silently swallowed. Add error handling to prevent silent failures.
| const script = document.createElement('script'); | |
| script.id = 'google-analytics'; | |
| script.src = `https://www.googletagmanager.com/gtag/js?id=${trackingId}`; | |
| script.async = true; | |
| document.head.appendChild(script); | |
| const script = document.createElement('script'); | |
| script.id = 'google-analytics'; | |
| script.src = ` | |
| script.async = true; | |
| script.onerror = () => console.warn('Failed to load Google Analytics'); | |
| document.head.appendChild(script); |
There was a problem hiding this comment.
Code Review
This pull request successfully migrates the blog from Jekyll to a modern React stack using Vite and Bun. The overall structure is clean and follows common practices for React projects. I've found a critical issue with the package dependencies that will prevent the project from being installed, and I've also suggested an improvement for handling configuration to enhance maintainability. Please see the detailed comments.
| "react": "^19.2.0", | ||
| "react-dom": "^19.2.0" |
There was a problem hiding this comment.
The specified React versions ^19.2.0 for react and react-dom do not appear to be valid, publicly available versions on npm. This will likely cause bun install (or npm install/yarn) to fail for anyone else trying to set up the project.
You should use a valid version string. If you intend to use the latest stable version, you can use ^18.3.1. If you want to use the upcoming React 19, you should use the correct version tag, such as a specific release candidate or an experimental build tag.
| "react": "^19.2.0", | |
| "react-dom": "^19.2.0" | |
| "react": "^18.3.1", | |
| "react-dom": "^18.3.1" |
| "siteName": "Gapry's Technical Writing", | ||
| "analytics": { | ||
| "provider": "google-gtag", | ||
| "trackingId": "G-4DG3HSBT1T", |
There was a problem hiding this comment.
Hardcoding configuration values like the Google Analytics trackingId directly in the configuration file is not recommended. While this ID is not a secret, using environment variables is a best practice for managing configuration that might differ between environments (e.g., development vs. production). This makes your application more flexible and easier to configure for deployment.
With Vite, you can achieve this by:
- Creating a
.env.localfile in your project root with the content:VITE_GA_TRACKING_ID=G-4DG3HSBT1T - Adding
.env.localto your.gitignorefile. - Accessing the variable in your code using
import.meta.env.VITE_GA_TRACKING_ID.
This approach keeps environment-specific configuration out of your version-controlled code.
1 participant
No description provided.