diff --git a/.claude/commands/branch.md b/.claude/commands/branch.md new file mode 100644 index 00000000..0b599734 --- /dev/null +++ b/.claude/commands/branch.md @@ -0,0 +1,203 @@ +--- +description: Analyze current git changes, infer intent, create a branch, commit, and open a PR if possible +allowed-tools: Bash(git status:*), Bash(git diff:*), Bash(git branch:*), Bash(git checkout:*), Bash(git switch:*), Bash(git add:*), Bash(git commit:*), Bash(git rev-parse:*), Bash(git log:*), Bash(gh auth status:*), Bash(gh pr create:*), Bash(which gh:*), Bash(pwd:*), Bash(sed:*), Bash(tr:*) +argument-hint: [optional-component-scope] +--- + +## Context + +- Working directory: !`pwd` +- Current branch: !`git branch --show-current` +- Git status: !`git status --short` +- Diff summary: !`git diff --stat && echo "\n--- STAGED ---\n" && git diff --cached --stat` +- Full unstaged diff: !`git diff` +- Full staged diff: !`git diff --cached` +- Recent commits: !`git log --oneline -10` + +## Task + +You are running the `/branch` workflow. + +Your job is to inspect the current repository changes and then do the following carefully and in order. + +### 1) Bail out if there is nothing to do + +First determine whether there are meaningful tracked changes to commit. + +Check both unstaged and staged changes. If there are no changes, stop immediately and reply with something like: + +`Nothing to be done.` + +Do not create a branch. +Do not stage anything. +Do not commit anything. +Do not attempt to create a PR. + +### 2) Infer the intent of the change + +Analyze the current diff from the perspective of a **consumer** of the codebase — not the author. + +Ask yourself: "If someone who uses this component, module, or package saw this change in a changelog, what would they need to know?" The answer to that question is the intent. Do not describe the implementation mechanism; describe the outcome or impact. + +For example, if the diff replaces a synchronous loader with an async one and adds a cache layer, the intent is NOT "refactor loader to async with caching" — it IS "improve loading times by caching results." + +Choose exactly one conventional prefix: + +- `feat` +- `fix` +- `docs` +- `chore` + +Use these meanings: + +- `feat` for a new user-facing capability or API enhancement +- `fix` for a bug fix, regression fix, accessibility fix, or behavior correction +- `docs` for documentation-only changes +- `chore` for maintenance, tooling, config, refactors without user-facing behavior change, or housekeeping + +### 3) Determine the optional component or scope + +If `$ARGUMENTS` is provided, use it as the scope. + +Otherwise, if the changed files clearly point to one component, package, module, or area, use that as the scope. + +Otherwise, ask the user exactly one concise question: + +`What scope should I use in the PR title? For example client, server, auth, docs. Say "none" to omit it.` + +Do not ask this question if the scope is already obvious from the diff or was provided in `$ARGUMENTS`. + +### 4) Draft the PR title + +Create a PR title in one of these forms: + +- `feat(scope): intent` +- `fix(scope): intent` +- `docs(scope): intent` +- `chore(scope): intent` +- `feat: intent` +- `fix: intent` +- `docs: intent` +- `chore: intent` + +Rules: + +- Keep it concise and specific +- Use sentence case after the colon +- Do not end with a period +- The title MUST describe the change from the consumer's perspective. Describe the impact or outcome, not the implementation. If your draft title mentions internal mechanisms (e.g., "refactor X to use Y", "replace A with B", "migrate from C to D"), rewrite it to describe what the consumer gains or what behavior changed. +- After drafting, apply this self-check: "Would a user of this code understand why they should care about this change from the title alone?" If not, rewrite it. + +Examples of rewriting implementation-focused titles into consumer-focused ones: + +| ❌ Implementation-focused | ✅ Consumer-focused | +| ---------------------------------------------------------------------- | -------------------------------------------------------------------- | +| `refactor(client): replace sync loader with async and add cache layer` | `feat(client): improve loading times by caching results` | +| `fix(auth): update token refresh logic to retry on 401` | `fix(auth): prevent unexpected logouts during session renewal` | +| `chore(deps): bump react-query from v4 to v5` | `chore(deps): upgrade data-fetching library to latest major version` | +| `fix(ui): change Button onClick handler to use useCallback` | `fix(ui): resolve unnecessary re-renders on button interactions` | + +Good title examples: + +- `feat(client): improve loading times by caching results` +- `fix(server): handle edge case where user has no email` +- `docs: clarify local setup steps` +- `chore(build): update test runner configuration` + +Bad title examples: + +- `fix stuff` +- `update code` +- `changes` +- `refactor things` + +### 5) Draft a commit body + +If the change is simple, a subject-only commit is acceptable. + +If the diff is non-trivial, add a short body that explains: + +- what changed +- why it changed +- any important implementation detail +- any consumer impact or migration consideration + +Keep the body concise, useful, and professional. + +### 6) Create a branch name from the PR title + +Generate a safe branch name derived from the PR title. + +Rules for the branch name: + +- lowercase +- replace spaces with underscores +- replace `:` with `_` +- remove characters that are awkward or unsafe in branch names +- remove parentheses but keep the scope text +- collapse duplicate separators +- trim leading and trailing separators + +Examples: + +- `feat(client): improve loading times by caching results` -> `feat_client_improve_loading_times_by_caching_results` +- `fix(server): handle edge case where user has no email` -> `fix_server_handle_edge_case_where_user_has_no_email` + +Before creating the branch: + +- inspect the current branch name +- if the current branch already appears to match this work reasonably well, ask the user whether to reuse it or create a new one +- otherwise create a new branch from the current `HEAD` + +Prefer `git switch -c ` if available, otherwise use `git checkout -b `. + +### 7) Show a concise plan before write actions + +Before creating the branch or committing, present a short summary with: + +- inferred type +- scope if any +- PR title +- branch name +- whether a commit body will be used + +Then ask for confirmation unless the user has explicitly indicated they want full automation. + +### 8) Commit the changes + +Once confirmed: + +- stage the relevant changes +- create the branch if needed +- commit using the PR title as the subject +- include the commit body if you drafted one + +### 9) Attempt to create a PR with GitHub CLI + +Check whether `gh` is installed and authenticated. + +Use: + +- `which gh` +- `gh auth status` + +If `gh` is available and authenticated, create the PR using the generated title and body. + +If PR creation succeeds, report success and show the branch name and PR title. + +If `gh` is unavailable, unauthenticated, or PR creation fails, do not keep retrying. Instead provide the user with: + +- branch name +- commit subject +- PR title +- PR body +- the exact `gh pr create` command they can run manually + +## Important behavior rules + +- Be conservative. If intent is ambiguous, ask one concise clarification question rather than guessing. +- When drafting the PR title, NEVER describe the implementation mechanism. Always describe the consumer-visible outcome. This is the single most important quality of a good PR title. If you find yourself writing words like "refactor", "replace", "migrate", "update X to use Y", or "change implementation of", stop and reframe from the consumer's point of view. +- Do not create a branch, commit, or PR when there are no changes. +- Do not invent scope names that are not supported by the diff or user input. +- If both staged and unstaged changes exist, treat the full working tree as the candidate change set unless the user indicates otherwise. +- If there are untracked files, include them only if they are clearly part of the intended change. diff --git a/.claude/settings.json b/.claude/settings.json new file mode 100644 index 00000000..4f491370 --- /dev/null +++ b/.claude/settings.json @@ -0,0 +1,60 @@ +{ + "$schema": "https://json.schemastore.org/claude-code-settings.json", + "env": { + "ENABLE_TOOL_SEARCH": "true" + }, + "permissions": { + "allow": [ + "Bash(cd:*)", + "Bash(done)", + "Bash(find:*)", + "Bash(gh auth status:*)", + "Bash(gh pr checks:*)", + "Bash(gh pr list:*)", + "Bash(gh pr view:*)", + "Bash(git log:*)", + "Bash(grep:*)", + "Bash(ls:*)", + "Bash(npx vitest:*)", + "Bash(pnpm --filter * lint:*)", + "Bash(pnpm --filter * test:*)", + "Bash(pnpm add:*)", + "Bash(pnpm build:*)", + "Bash(pnpm info:*)", + "Bash(pnpm lint:*)", + "Bash(pnpm list:*)", + "Bash(pnpm remove:*)", + "Bash(pnpm test:*)", + "Bash(pnpm why:*)", + "Bash(xxd:*)", + "mcp__Control_Chrome__get_page_content", + "mcp__Control_Chrome__list_tabs", + "mcp__Control_Chrome__switch_to_tab", + "WebFetch(domain:ai-sdk.dev)", + "WebFetch(domain:github.com)", + "WebFetch(domain:google.com)", + "WebFetch(domain:gizmette.com)", + "WebFetch(domain:npmjs.com)" + ], + "deny": [ + "Bash(rm -rf:*)", + "Read(./.env)", + "Read(./.env.*)", + "Read(./secrets/**)" + ], + "defaultMode": "acceptEdits" + }, + + "enabledPlugins": { + "claude-md-management@claude-plugins-official": true, + "code-review@claude-plugins-official": true, + "code-simplifier@claude-plugins-official": true, + "pr-review-toolkit@claude-plugins-official": true, + "typescript-lsp@claude-plugins-official": true + }, + "spinnerTipsEnabled": false, + "spinnerVerbs": { + "mode": "replace", + "verbs": ["Working"] + } +} diff --git a/packages/bundlecheck/src/__tests__/bundler.test.ts b/packages/bundlecheck/src/__tests__/bundler.test.ts index 1128f0cc..35278abf 100644 --- a/packages/bundlecheck/src/__tests__/bundler.test.ts +++ b/packages/bundlecheck/src/__tests__/bundler.test.ts @@ -1218,6 +1218,89 @@ describe("checkBundleSize", () => { expect(result.gzipSize).not.toBeNull(); }); + it("should auto-retry with node platform when browser build fails on Node.js built-ins", async () => { + // Package has no engines field, so auto-detection defaults to browser. + vi.mocked(fs.readFileSync).mockReturnValue( + JSON.stringify({ + name: "test-package", + version: "1.0.0", + main: "./dist/index.js", + dependencies: {}, + peerDependencies: {}, + }), + ); + + // First call (browser) fails with unresolved Node.js built-ins. + // Second call (node) succeeds. + let callCount = 0; + vi.mocked(esbuild.build).mockImplementation(async (options) => { + callCount++; + if (callCount === 1) { + // Simulate esbuild BuildFailure with unresolved "path" and "fs". + const error = new Error("Build failed") as Error & { + errors: Array<{ text: string }>; + }; + error.errors = [ + { text: 'Could not resolve "path"' }, + { text: 'Could not resolve "fs/promises"' }, + ]; + throw error; + } + // Second call should use node platform. + expect(options?.platform).toBe("node"); + return { + outputFiles: [ + { + contents: new Uint8Array(Buffer.from("minified code")), + path: "entry.js", + text: "minified code", + hash: "abc123", + }, + ], + errors: [], + warnings: [], + metafile: { inputs: {}, outputs: {} }, + mangleCache: {}, + } as esbuild.BuildResult<{ write: false; metafile: true }>; + }); + + const result = await checkBundleSize({ + packageName: "test-package", + }); + + expect(result.platform).toBe("node"); + expect(result.gzipSize).toBeNull(); + expect(esbuild.build).toHaveBeenCalledTimes(2); + }); + + it("should not auto-retry with node platform when explicit browser platform is set", async () => { + vi.mocked(fs.readFileSync).mockReturnValue( + JSON.stringify({ + name: "test-package", + version: "1.0.0", + main: "./dist/index.js", + dependencies: {}, + peerDependencies: {}, + }), + ); + + const error = new Error("Build failed") as Error & { + errors: Array<{ text: string }>; + }; + error.errors = [ + { text: 'Could not resolve "path"' }, + { text: 'Could not resolve "fs"' }, + ]; + vi.mocked(esbuild.build).mockRejectedValue(error); + + await expect( + checkBundleSize({ + packageName: "test-package", + platform: "browser", + }), + ).rejects.toThrow("Build failed"); + }); + it("should still auto-detect node when engines.node is present and no browser framework deps", async () => { vi.mocked(fs.readFileSync).mockReturnValue( JSON.stringify({ diff --git a/packages/bundlecheck/src/bundler.ts b/packages/bundlecheck/src/bundler.ts index 15c18eb7..55c474e7 100644 --- a/packages/bundlecheck/src/bundler.ts +++ b/packages/bundlecheck/src/bundler.ts @@ -312,11 +312,54 @@ function isEsbuildBuildFailure( * Tested against esbuild 0.27.x error format. * */ +/** + * Node.js built-in modules that indicate a package is meant for Node.js. + * When these fail to resolve during browser bundling, we auto-retry with + * platform: "node". + */ +const NODE_BUILTIN_MODULES = new Set([ + "assert", + "buffer", + "child_process", + "cluster", + "crypto", + "dgram", + "dns", + "events", + "fs", + "http", + "http2", + "https", + "module", + "net", + "os", + "path", + "perf_hooks", + "process", + "querystring", + "readline", + "stream", + "string_decoder", + "tls", + "tty", + "url", + "util", + "v8", + "vm", + "worker_threads", + "zlib", +]); + function parseUnresolvedModules(error: unknown): { hasUnresolvedReact: boolean; hasUnresolvedReactDom: boolean; + hasUnresolvedNodeBuiltins: boolean; } { - const result = { hasUnresolvedReact: false, hasUnresolvedReactDom: false }; + const result = { + hasUnresolvedReact: false, + hasUnresolvedReactDom: false, + hasUnresolvedNodeBuiltins: false, + }; /** * Pattern to extract module path from "Could not resolve X" errors. Matches: @@ -324,6 +367,17 @@ function parseUnresolvedModules(error: unknown): { */ const resolveErrorPattern = /Could not resolve ["']([^"']+)["']/; + /** + * Check if a module path is a Node.js built-in (with or without "node:" prefix). + */ + const isNodeBuiltin = (modulePath: string): boolean => { + const withoutPrefix = modulePath.startsWith("node:") + ? modulePath.slice(5) + : modulePath; + const [root] = withoutPrefix.split("/"); + return NODE_BUILTIN_MODULES.has(root); + }; + if (isEsbuildBuildFailure(error)) { // Use structured error objects from esbuild BuildFailure. for (const err of error.errors) { @@ -337,6 +391,9 @@ function parseUnresolvedModules(error: unknown): { if (modulePath === "react-dom" || modulePath.startsWith("react-dom/")) { result.hasUnresolvedReactDom = true; } + if (isNodeBuiltin(modulePath)) { + result.hasUnresolvedNodeBuiltins = true; + } } } } else { @@ -353,6 +410,9 @@ function parseUnresolvedModules(error: unknown): { if (modulePath === "react-dom" || modulePath.startsWith("react-dom/")) { result.hasUnresolvedReactDom = true; } + if (isNodeBuiltin(modulePath)) { + result.hasUnresolvedNodeBuiltins = true; + } } } @@ -792,31 +852,60 @@ export async function checkBundleSize( * This handles packages that don't properly declare react as a peer * dependency. */ + /** + * Common esbuild options shared across all build attempts. + * Native .node addons cannot be bundled by esbuild, so we treat them + * as empty files to avoid loader errors. + */ + const commonBuildOptions = { + entryPoints: [entryFile], + bundle: true, + write: false as const, + format: "esm" as const, + target, + minify: true, + treeShaking: true, + metafile: true as const, + loader: { ".node": "empty" as const }, + }; + let result: esbuild.BuildResult<{ write: false; metafile: true }>; try { result = await esbuild.build({ - entryPoints: [entryFile], - bundle: true, - write: false, - format: "esm", + ...commonBuildOptions, platform, - target, - minify: true, - treeShaking: true, external: esbuildExternals, - metafile: true, logLevel: "silent", // Suppress errors on first attempt (will retry if needed) }); } catch (error) { /** * Parse unresolved module errors using esbuild's structured error format. * This handles packages that don't properly declare react as a peer - * dependency. + * dependency, and Node.js packages incorrectly bundled for the browser. */ - const { hasUnresolvedReact, hasUnresolvedReactDom } = - parseUnresolvedModules(error); - - if (!noExternal && (hasUnresolvedReact || hasUnresolvedReactDom)) { + const { + hasUnresolvedReact, + hasUnresolvedReactDom, + hasUnresolvedNodeBuiltins, + } = parseUnresolvedModules(error); + + if ( + hasUnresolvedNodeBuiltins && + platform === "browser" && + !explicitPlatform + ) { + /** + * Node.js built-in modules failed to resolve during browser bundling. + * This means the package is a Node.js package that wasn't detected by + * the engines-based auto-detection. Retry with platform: "node". + */ + platform = "node"; + result = await esbuild.build({ + ...commonBuildOptions, + platform, + external: esbuildExternals, + }); + } else if (!noExternal && (hasUnresolvedReact || hasUnresolvedReactDom)) { // Auto-add react and/or react-dom to externals and retry. const autoExternals: string[] = []; if (hasUnresolvedReact && !externals.includes("react")) { @@ -831,16 +920,9 @@ export async function checkBundleSize( esbuildExternals = expandExternalsForEsbuild(externals); result = await esbuild.build({ - entryPoints: [entryFile], - bundle: true, - write: false, - format: "esm", + ...commonBuildOptions, platform, - target, - minify: true, - treeShaking: true, external: esbuildExternals, - metafile: true, }); } else { throw error; diff --git a/packages/bundlecheck/src/cache.ts b/packages/bundlecheck/src/cache.ts index 63a9b333..5a9d33f5 100644 --- a/packages/bundlecheck/src/cache.ts +++ b/packages/bundlecheck/src/cache.ts @@ -51,6 +51,7 @@ type CacheRow = { created_at: number; named_export_count: number | null; actual_externals: string; + actual_platform: string | null; }; let db: Database.Database | null = null; @@ -123,6 +124,17 @@ export function initCache(): Database.Database { // Column already exists, ignore error. } + /** + * Migration: Add actual_platform column if it doesn't exist (for existing + * databases). Stores the real platform from BundleResult, which may differ + * from the cache key platform (e.g., "auto" key but "node" actual). + */ + try { + db.exec(`ALTER TABLE bundle_cache ADD COLUMN actual_platform TEXT`); + } catch { + // Column already exists, ignore error. + } + return db; } @@ -210,8 +222,20 @@ export function getCachedResult(key: CacheKey): CachedBundleResult | null { * Convert row to BundleResult. Use actual_externals (computed externals * including auto-detected react/react-dom) if available, otherwise fall back * to externals (for old cache entries). + * + * Use actual_platform if available (stores the real detected platform), + * otherwise fall back to the cache key platform. For old entries where + * platform is "auto" and no actual_platform exists, default to "browser". */ const externalsStr = row.actual_externals || row.externals; + let platform: "browser" | "node"; + if (row.actual_platform === "browser" || row.actual_platform === "node") { + platform = row.actual_platform; + } else if (row.platform === "browser" || row.platform === "node") { + platform = row.platform; + } else { + platform = "browser"; + } return { packageName: row.display_name, packageVersion: row.version, @@ -221,7 +245,7 @@ export function getCachedResult(key: CacheKey): CachedBundleResult | null { gzipLevel: row.gzip_level, externals: externalsStr ? externalsStr.split(",").filter(Boolean) : [], dependencies, - platform: row.platform as "browser" | "node", + platform, namedExportCount: row.named_export_count ?? 0, }; } catch { @@ -248,8 +272,9 @@ export function setCachedResult(key: CacheKey, result: BundleResult): void { const stmt = database.prepare(` INSERT OR REPLACE INTO bundle_cache ( package_name, version, exports, platform, gzip_level, externals, no_external, - raw_size, gzip_size, dependencies, display_name, created_at, named_export_count, actual_externals - ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + raw_size, gzip_size, dependencies, display_name, created_at, named_export_count, actual_externals, + actual_platform + ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) `); stmt.run( @@ -267,6 +292,7 @@ export function setCachedResult(key: CacheKey, result: BundleResult): void { Date.now(), result.namedExportCount, result.externals.slice().sort().join(","), + result.platform, ); // Enforce max entries (LRU-style eviction based on created_at).