Add pull request template#1
Merged
Merged
Conversation
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 20459644 | Triggered | Google API Key | 627a7d9 | src/entrypoints/content/components/Map/App.tsx | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR switches the extension to WXT and Oxc-based tooling, adds CI and project configuration, and replaces the old pages-based popup/content setup with WXT entrypoints backed by a storage item for the work address. ChangesBuild tooling and CI migration
WXT entrypoints, storage, and legacy removal
Estimated code review effort: 4 (Complex) | ~55 minutes Sequence Diagram(s)sequenceDiagram
participant ContentScript as Content script
participant Document as document.body
participant MapApp as App
participant Storage as workAddress
ContentScript->>Document: find "About the building" section and insert container
ContentScript->>MapApp: render App into injected container
MapApp->>Storage: read workAddress and watch for updates
MapApp->>Document: wait for meta[name='ICBM'] to resolve building coordinates
Estimated code review effort: 4 (Complex) | ~50 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
balloman
force-pushed
the
update-deps-05-2026
branch
from
July 2, 2026 05:00
943e830 to
21225b9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
src/entrypoints/content/index.tsx (1)
40-52: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueObserver callback runs
querySelectorAllon every DOM mutation until mount succeeds.Before the container is inserted, every mutation on
document.body(subtree, childList) triggersfindSectionByTextwhich scans all<section>elements. On a heavy SPA page this could fire very frequently prior to the first successful mount. Consider debouncing the callback or using a more targeted selector/root instead ofdocument.body.🤖 Prompt for 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. In `@src/entrypoints/content/index.tsx` around lines 40 - 52, The MutationObserver in the content entrypoint is doing an expensive full-page scan on every DOM mutation before the container mounts, which can be very frequent on SPA pages. Update the observer callback around mount(), cleanup(), and findSectionByText() so it avoids repeated querySelectorAll work on document.body, either by debouncing/throttling the callback or by watching a narrower root/selector that is more directly related to the container insertion. Keep the mounting behavior the same, but reduce how often the scan runs until the first successful mount.src/entrypoints/content/components/Map/App.tsx (1)
57-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale comment and unreachable/malformed fallback branch.
The comment on line 60 ("Gets the address by getting the string starting with the first numeric character") doesn't describe the code below it. Also, the
elsebranch (line 64) is effectively dead code —source()is only invoked inside<Show when={work()}>, so it never runs whenwork()is falsy — and if it were reached, the query string is malformed (missing&betweenorigin=...Tradeanddestination=...).🤖 Prompt for 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. In `@src/entrypoints/content/components/Map/App.tsx` around lines 57 - 65, The source() helper in App.tsx has a stale comment that no longer matches the logic and a fallback branch that is unreachable because source() is only called under <Show when={work()}>. Update source() to remove the misleading comment and either eliminate the dead fallback or make it a valid, reachable default path; if you keep the fallback, fix the Google Maps URL so the query string is well-formed. Use the source, work, and building symbols to locate the logic.
🤖 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 @.github/workflows/ci.yml:
- Line 14: The two actions/checkout@v6 steps in the CI workflow should disable
git credential persistence to harden the job. Update each checkout invocation to
set persist-credentials to false so the token is not left in the local git
config for later steps. Use the existing checkout steps in the workflow as the
target and keep the rest of the job unchanged.
In `@src/entrypoints/content/components/Map/App.tsx`:
- Line 62: The Google Maps embed URL in App.tsx is hardcoding an API key, which
should not be committed to source. Update the URL निर्माण in the App component
to read the key from a build-time environment variable or secret instead of a
literal string, and ensure the key is injected through the app’s config/setup
path rather than stored in the code. Also replace any other occurrences in the
same URL-building logic so the key is not duplicated.
- Line 1: The Map/App component is using solid-js APIs and workAddress without
importing them, which will cause runtime ReferenceError on mount. Update the
imports at the top of App.tsx to include createResource and onCleanup from
solid-js, and bring in workAddress from its source used in this component.
Double-check the App component’s setup code where these symbols are referenced
so all used identifiers are explicitly imported.
- Line 45: The waitForElm call in App.tsx is using a malformed selector string,
which will make the underlying querySelector throw a SyntaxError. Fix the
selector passed to waitForElm so the ICBM meta tag query is syntactically
complete, and verify the surrounding App component code that handles the
returned element still works with the corrected selector.
- Around line 11-29: The MutationObserver in waitForElm is never cleaned up, so
it can leak when the selector never matches or when App remounts. Update
waitForElm in App.tsx to always disconnect the observer once it resolves and
also add component cleanup using onCleanup so the observer is torn down on
unmount. Make sure the logic around document.querySelector, MutationObserver,
and observer.disconnect covers both success and teardown paths.
In `@src/entrypoints/content/index.tsx`:
- Line 1: The content entrypoint calls render(App, container) but never imports
render, so add the missing solid-js/web render import alongside the existing App
import. Update the entrypoint module that mounts the app so the render symbol is
available at runtime, matching the pattern used in
src/entrypoints/popup/main.tsx.
In `@src/entrypoints/popup/index.html`:
- Around line 7-13: The manifest.default_icon meta content in the popup
entrypoint is not valid JSON because it uses single quotes and a trailing comma,
which can break WXT parsing. Update the content string to valid JSON in the same
meta tag by using double quotes and removing the trailing comma, keeping the
icon mapping for the manifest override intact.
In `@src/entrypoints/popup/Popup.tsx`:
- Around line 1-6: The Popup component uses createResource without importing it,
so add the missing solid-js import in Popup so the resource setup can resolve
correctly. Update the imports at the top of Popup.tsx to bring in createResource
alongside the existing workAddress and style imports, and ensure the Popup
function keeps using createResource(workAddress.getValue) unchanged.
- Around line 8-14: The save confirmation in saveAddress uses alert(), which is
not reliable in the popup context. Update Popup.tsx so saveAddress reports
success through inline popup UI instead of a blocking browser alert, using an
existing or new status/toast element in the Popup component to show “Saved
successfully!” after workAddress.setValue resolves.
---
Nitpick comments:
In `@src/entrypoints/content/components/Map/App.tsx`:
- Around line 57-65: The source() helper in App.tsx has a stale comment that no
longer matches the logic and a fallback branch that is unreachable because
source() is only called under <Show when={work()}>. Update source() to remove
the misleading comment and either eliminate the dead fallback or make it a
valid, reachable default path; if you keep the fallback, fix the Google Maps URL
so the query string is well-formed. Use the source, work, and building symbols
to locate the logic.
In `@src/entrypoints/content/index.tsx`:
- Around line 40-52: The MutationObserver in the content entrypoint is doing an
expensive full-page scan on every DOM mutation before the container mounts,
which can be very frequent on SPA pages. Update the observer callback around
mount(), cleanup(), and findSectionByText() so it avoids repeated
querySelectorAll work on document.body, either by debouncing/throttling the
callback or by watching a narrower root/selector that is more directly related
to the container insertion. Keep the mounting behavior the same, but reduce how
often the scan runs until the first successful mount.
🪄 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: be258af8-d494-4243-bcdc-98a3fd0f02e4
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlsrc/assets/img/logo.svgis excluded by!**/*.svg
📒 Files selected for processing (42)
.eslintrc.github/PULL_REQUEST_TEMPLATE.md.github/workflows/ci.yml.gitignore.oxfmtrc.json.oxlintrc.json.prettierrc.vscode/settings.jsonREADME.mdmise.tomlnodemon.jsonpackage.jsonpnpm-workspace.yamlpostcss.config.cjssrc/entrypoints/content/components/Map/App.tsxsrc/entrypoints/content/index.tsxsrc/entrypoints/popup/Popup.tsxsrc/entrypoints/popup/index.htmlsrc/entrypoints/popup/main.tsxsrc/entrypoints/popup/style.csssrc/global.d.tssrc/manifest-type.tssrc/manifest.tssrc/pages/content/components/Map/App.module.csssrc/pages/content/components/Map/app.tsxsrc/pages/content/components/Map/index.tsxsrc/pages/content/index.tsxsrc/pages/popup/Popup.module.csssrc/pages/popup/Popup.tsxsrc/pages/popup/index.csssrc/pages/popup/index.htmlsrc/pages/popup/index.tsxsrc/styles/index.csssrc/utils/storage.tssrc/vite-env.d.tstailwind.config.cjstsconfig.jsonutils/log.tsutils/plugins/make-manifest.tsvite.config.tsweb-ext.config.tswxt.config.ts
💤 Files with no reviewable changes (21)
- postcss.config.cjs
- .eslintrc
- nodemon.json
- src/pages/content/components/Map/app.tsx
- tailwind.config.cjs
- src/vite-env.d.ts
- src/pages/popup/index.tsx
- src/pages/popup/Popup.tsx
- src/pages/content/components/Map/index.tsx
- src/manifest.ts
- utils/log.ts
- src/pages/content/components/Map/App.module.css
- src/pages/popup/index.html
- .prettierrc
- vite.config.ts
- src/pages/popup/index.css
- src/manifest-type.ts
- src/global.d.ts
- src/pages/content/index.tsx
- utils/plugins/make-manifest.ts
- src/pages/popup/Popup.module.css
| format: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v6 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Set persist-credentials: false on checkout.
Static analysis (zizmor) flags both actions/checkout@v6 steps for not disabling credential persistence. Since permissions: {} already limits token scope, risk is low, but this is a cheap hardening step to prevent the token from lingering in the git config for subsequent steps (e.g., pnpm install scripts).
🔒 Proposed fix
- uses: actions/checkout@v6
+ with:
+ persist-credentials: falseAlso applies to: 29-29
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 14-14: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 Prompt for 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.
In @.github/workflows/ci.yml at line 14, The two actions/checkout@v6 steps in
the CI workflow should disable git credential persistence to harden the job.
Update each checkout invocation to set persist-credentials to false so the token
is not left in the local git config for later steps. Use the existing checkout
steps in the workflow as the target and keep the rest of the job unchanged.
Source: Linters/SAST tools
| @@ -0,0 +1,60 @@ | |||
| import App from "./components/Map/App"; | |||
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Missing render import.
render(App, container) is called on line 37, but render from solid-js/web is never imported. src/entrypoints/popup/main.tsx explicitly imports it for the same purpose, confirming it isn't auto-imported here — this will throw a ReferenceError and the content script will never mount.
🐛 Proposed fix
import App from "./components/Map/App";
+import { render } from "solid-js/web";Also applies to: 37-37
🤖 Prompt for 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.
In `@src/entrypoints/content/index.tsx` at line 1, The content entrypoint calls
render(App, container) but never imports render, so add the missing solid-js/web
render import alongside the existing App import. Update the entrypoint module
that mounts the app so the render symbol is available at runtime, matching the
pattern used in src/entrypoints/popup/main.tsx.
|
Warning Docstrings generation - IN PROGRESS Generating docstrings for this pull request |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
main.Notes
update-deps-05-2026bookmark, so it includes the earlier WXT/OXC/dependency/readme/icon work plus the PR template and CI changes.wxt.config.ts; the old hand-written manifest types, CRXJS Vite config, PostCSS config, Tailwind v3 config, nodemon config, and old page-based entrypoints are removed..outputdirectory during development instead ofdist.src/utils/storage.tsasEmpire State Building.About the buildingsection when available, and cleans up on WXT invalidation.jdx/mise-action, so Node and pnpm versions come frommise.tomlinstead of a separatepackageManagerpin.pushruns onmainwill also fire for merged PRs; skipping only those reliably would require extra workflow logic or a repo convention, so this is kept simple.Testing
mise exec -- pnpm install --frozen-lockfilemise exec -- pnpm run format:checkmise exec -- pnpm lintmise exec -- pnpm buildSummary by CodeRabbit
New Features
Documentation
Bug Fixes