fix(extension): enforce tsc — 9 type errors + gate build (H7 / P1-4) - #14
Conversation
…7 / P1-4)
plasmo build was silently ignoring tsc and shipping a production bundle compiled from code
the project's own tsc rejected. 9 errors, all on the CDP/injection path (browser-bridge.ts)
and config normalization (index.ts):
- browser-bridge.ts: route 5 destructured chrome.debugger.sendCommand calls (root/nodeId/
outerHTML — @types/chrome types sendCommand's return as Object, breaking destructuring of
real CDP fields) through the existing sendCdp helper (returns Promise<any>; ensureAttached is
an idempotent guard).
- browser-bridge.ts: add ScriptingResult = InjectionResult<any> & { error?: string } — @types
omits the runtime `error` field Chrome sets on injection failure; the fallback-on-error logic
now type-checks.
- index.ts: replace `as string` casts on unknown llm.api_key with a `typeof === "string"` runtime
guard (was masking unknown→string; a non-string key would have silently misbehaved).
Door closed at the build level (Kimi review): chrome-extension `build` is now
`tsc --noEmit && plasmo build`, so local/release builds fail on type errors too — not just CI.
CI runs `npm run build` for the extension (typecheck + bundle).
Verified: tsc --noEmit 0 errors; npm run build succeeds (manifest produced).
Co-Authored-By: Claude <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe pull request adds Chrome extension CI build validation, performs TypeScript checking before Plasmo builds, routes selected debugger commands through ChangesChrome extension updates
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 (4)
.github/workflows/ci.yml (1)
46-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider splitting
npm ciandnpm run buildinto separate steps.Combining install and build in one step works, but if
npm cifails, the step label "Build + typecheck chrome-extension (audit H7)" is misleading in CI logs. The companion job uses separate install and build steps (lines 23-33) for this reason. Splitting would improve diagnostic clarity and consistency.♻️ Suggested split
- name: Install chrome-extension dependencies run: | cd chrome-extension && npm ci - - name: Build + typecheck chrome-extension (audit H7) - run: | - cd chrome-extension && npm ci && npm run build + - name: Build + typecheck chrome-extension (audit H7) + run: | + cd chrome-extension && npm run build🤖 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 around lines 46 - 48, Split the combined chrome-extension command under “Build + typecheck chrome-extension (audit H7)” into separate workflow steps: one clearly named step running `npm ci` and another clearly named build/typecheck step running `npm run build`, matching the companion job’s structure and improving failure diagnostics.chrome-extension/src/background/index.ts (1)
74-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueVision API key handling still uses
as stringcasts.Lines 93–99 apply the same
as stringcast pattern forvision_api_keythat this change fixed forapi_key. Consider applying the sametypeofguard there for consistency and type safety.♻️ Proposed consistency fix for vision API keys
// Skip masked vision API keys - if (cfg.vision_api_key !== undefined && !isMaskedApiKey(cfg.vision_api_key as string)) { - next.vision_api_key = cfg.vision_api_key as string - } else if (vision?.api_key !== undefined && !isMaskedApiKey(vision.api_key as string)) { - next.vision_api_key = vision.api_key as string + if (typeof cfg.vision_api_key === "string" && cfg.vision_api_key && !isMaskedApiKey(cfg.vision_api_key)) { + next.vision_api_key = cfg.vision_api_key + } else if (typeof vision?.api_key === "string" && vision.api_key && !isMaskedApiKey(vision.api_key)) { + next.vision_api_key = vision.api_key } else if (extensionConfig?.vision_api_key !== undefined) { next.vision_api_key = extensionConfig.vision_api_key }🤖 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 `@chrome-extension/src/background/index.ts` around lines 74 - 75, Update the vision API key handling alongside the api_key logic to avoid as string casts. In the vision_api_key assignment, use a typeof guard, require a non-empty value, and exclude masked keys via isMaskedApiKey before returning the key.chrome-extension/src/background/browser-bridge.ts (2)
204-220: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
ensureAttachedcall.
getOuterHTMLViaDomcallsthis.ensureAttached(tabId)at line 199, andthis.sendCdpinternally callsensureAttachedagain for each of the three CDP commands (lines 204, 212, 220). The explicit call at line 199 can be removed sincesendCdpguarantees attachment.♻️ Proposed cleanup
private async getOuterHTMLViaDom(tabId: number, selector?: string): Promise<string> { - await this.ensureAttached(tabId) try { await chrome.debugger.sendCommand({ tabId }, "DOM.enable") } catch { /* ignore */ }🤖 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 `@chrome-extension/src/background/browser-bridge.ts` around lines 204 - 220, Remove the explicit ensureAttached call from getOuterHTMLViaDom, since each sendCdp invocation already guarantees attachment before executing the DOM.getDocument, DOM.querySelector, and DOM.getOuterHTML commands.
900-903: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
DOM.setFileInputFilesstill uses directchrome.debugger.sendCommand.Line 910 routes
DOM.setFileInputFilesthroughchrome.debugger.sendCommanddirectly while the precedingDOM.getDocumentandDOM.querySelectorcalls in this method were migrated tothis.sendCdp. For consistency with the PR's goal of routing CDP calls through the helper, consider migrating this call too. The explicitensureAttachedat line 898 is also redundant sincesendCdphandles attachment.♻️ Proposed consistency fix
private async uploadFile(params: Record<string, any>): Promise<ToolResult> { const tabId = this.getTabId(params) if (!params.selector || !params.filePath) throw new Error("selector and filePath are required") - await this.ensureAttached(tabId) try { const { root } = await this.sendCdp(tabId, "DOM.getDocument", {}) if (!root?.nodeId) throw new Error("Could not retrieve DOM Document root") const { nodeId } = await this.sendCdp(tabId, "DOM.querySelector", { nodeId: root.nodeId, selector: params.selector, }) if (!nodeId) throw new Error(`Element not found for selector: ${params.selector}`) - await chrome.debugger.sendCommand({ tabId }, "DOM.setFileInputFiles", { + await this.sendCdp(tabId, "DOM.setFileInputFiles", { files: [params.filePath], nodeId: nodeId, })🤖 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 `@chrome-extension/src/background/browser-bridge.ts` around lines 900 - 903, In the file-input handling method, replace the direct chrome.debugger.sendCommand call for DOM.setFileInputFiles with this.sendCdp, matching the existing DOM.getDocument and DOM.querySelector calls; remove the now-redundant ensureAttached invocation because sendCdp performs attachment internally.
🤖 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 `@chrome-extension/package.json`:
- Line 12: Update the package.json build script to run Plasmo’s type generation
before tsc --noEmit, ensuring .plasmo/index.d.ts exists during type-checking;
use the existing build command as the target and preserve the subsequent plasmo
build step.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 46-48: Split the combined chrome-extension command under “Build +
typecheck chrome-extension (audit H7)” into separate workflow steps: one clearly
named step running `npm ci` and another clearly named build/typecheck step
running `npm run build`, matching the companion job’s structure and improving
failure diagnostics.
In `@chrome-extension/src/background/browser-bridge.ts`:
- Around line 204-220: Remove the explicit ensureAttached call from
getOuterHTMLViaDom, since each sendCdp invocation already guarantees attachment
before executing the DOM.getDocument, DOM.querySelector, and DOM.getOuterHTML
commands.
- Around line 900-903: In the file-input handling method, replace the direct
chrome.debugger.sendCommand call for DOM.setFileInputFiles with this.sendCdp,
matching the existing DOM.getDocument and DOM.querySelector calls; remove the
now-redundant ensureAttached invocation because sendCdp performs attachment
internally.
In `@chrome-extension/src/background/index.ts`:
- Around line 74-75: Update the vision API key handling alongside the api_key
logic to avoid as string casts. In the vision_api_key assignment, use a typeof
guard, require a non-empty value, and exclude masked keys via isMaskedApiKey
before returning the key.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2c15c6da-c9a8-42fd-96bd-32dc372acfd6
📒 Files selected for processing (4)
.github/workflows/ci.ymlchrome-extension/package.jsonchrome-extension/src/background/browser-bridge.tschrome-extension/src/background/index.ts
| "test": "node -e \"require('fs').rmSync('.test-dist',{recursive:true,force:true})\" && tsc -p tsconfig.test.json && node --test .test-dist/tests/**/*.test.js", | ||
| "prebuild": "node scripts/generate-icons.mjs", | ||
| "build": "plasmo build", | ||
| "build": "tsc --noEmit && plasmo build", |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether Plasmo generates .plasmo/index.d.ts during npm ci or only during build/dev.
# Look for postinstall/prepare scripts in package.json and plasmo's package.json
cat chrome-extension/package.json | jq '.scripts'
# Check if .plasmo exists after install
ls -la chrome-extension/.plasmo/ 2>/dev/null || echo ".plasmo directory not found"Repository: nehcuh/cmspark
Length of output: 1246
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant files =="
git ls-files chrome-extension/package.json chrome-extension/tsconfig*.json chrome-extension/.plasmo/index.d.ts || true
echo
echo "== package.json =="
cat -n chrome-extension/package.json | sed -n '1,120p'
echo
echo "== tsconfig files =="
for f in chrome-extension/tsconfig.json chrome-extension/tsconfig.*.json; do
[ -f "$f" ] || continue
echo "--- $f ---"
cat -n "$f" | sed -n '1,220p'
done
echo
echo "== search for plasmo-generated type usage =="
rg -n "Plasmo|\.plasmo/index\.d\.ts|declare global|CSConfig|chrome-extension/.plasmo" chrome-extension -g '!chrome-extension/.plasmo/**' || trueRepository: nehcuh/cmspark
Length of output: 3995
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== lockfiles / workspace files =="
git ls-files | rg '(^|/)(package-lock\.json|pnpm-lock\.yaml|yarn.lock|npm-shrinkwrap\.json|package\.json)$' || true
echo
echo "== root package.json if present =="
[ -f package.json ] && cat -n package.json | sed -n '1,220p' || echo "no root package.json"
echo
echo "== plasmo-generated file contents =="
[ -f chrome-extension/.plasmo/index.d.ts ] && cat -n chrome-extension/.plasmo/index.d.ts | sed -n '1,160p' || echo "no chrome-extension/.plasmo/index.d.ts"
echo
echo "== plasmo package metadata if installed =="
if [ -f chrome-extension/node_modules/plasmo/package.json ]; then
cat -n chrome-extension/node_modules/plasmo/package.json | sed -n '1,220p'
else
echo "chrome-extension/node_modules/plasmo/package.json not found"
fiRepository: nehcuh/cmspark
Length of output: 3165
🌐 Web query:
Plasmo .plasmo/index.d.ts generated during npm install or only plasmo build dev
💡 Result:
The.plasmo/index.d.ts file and other generated type definition files are not created during the npm install process [1][2][3]. Instead, these files are generated by the Plasmo framework during the development (plasmo dev) or production build (plasmo build) processes [2][4][5]. Plasmo uses these generated files to provide type-safety, manage import resolution, and handle auto-generated metadata for features like messaging [2][3]. Since they are dynamically created as part of the build tool's execution, they do not exist until the framework runs its internal build/dev scripts [2][4]. If you encounter issues related to these files (such as TypeScript errors or missing definitions), ensure that you have run the development server at least once, as this triggers the generation of the.plasmo directory and its contents [2][4]. You should generally not manually edit files inside the.plasmo directory, as they will be overwritten by the framework [2].
Citations:
- 1: Update
tsconfig.jsonincustomization/src.mdxPlasmoHQ/docs#66 - 2: fix(messaging): generate req/res types from handler exports in messaging.d.ts PlasmoHQ/plasmo#1348
- 3: [BUG] installing @type/chrome leads to typing errors under .plasmo PlasmoHQ/plasmo#274
- 4: https://docs.plasmo.com/framework
- 5: https://docs.plasmo.com/framework/workflows/build
Generate Plasmo types before tsc --noEmit. npm ci doesn’t create .plasmo/index.d.ts; Plasmo generates it during plasmo dev/plasmo build, so this script can type-check a clean checkout without those ambient declarations and miss errors they would catch.
🤖 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 `@chrome-extension/package.json` at line 12, Update the package.json build
script to run Plasmo’s type generation before tsc --noEmit, ensuring
.plasmo/index.d.ts exists during type-checking; use the existing build command
as the target and preserve the subsequent plasmo build step.
…/P1-4) Adds docs/remediation-plan-2026-07-09.md (5-phase P0–P4 from the audit) and folds Kimi's independent-review corrections into the audit report. 4 independent PRs opened from the plan (zero file overlap, each kimi-gated + verified): #11 P0 stopgaps, #12 CI unblock (test isolation + teardown), #13 persistence (atomic writes + corrupt-preserve; H5 verified non-bug), #14 extension tsc + build gate. Memory: project-knowledge +3 pitfalls (test-isolation static-import, node:test+ws teardown async error, verify-race-before-locking); instincts +2 (verify-race-before-locking, kimi-gate-catches- real-issues). Handoff trimmed to S7 + S6. Co-Authored-By: Claude <noreply@anthropic.com>
1 participant
What
`plasmo build` was silently ignoring `tsc` and shipping a production bundle compiled from code the project's own `tsc` rejected (audit H7). This fixes the 9 type errors and closes the door at the build level.
Changes
Verification
Relationship to other PRs
Zero overlap with #11 (P0), #12 (P1-1), #13 (P1-3) — all companion-side; this is chrome-extension + a new CI step. Merge all four → green CI + type-safe extension build.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests