Polish npx shotfix for first-run experience#3
Conversation
- Watch mode ON by default (use --no-watch to disable) - First-run API key prompt: interactive setup, saves to .shotfix/.env - Clean startup banner with widget script tag instructions - Serve widget JS from /widget.js endpoint (for easy dev setup) - Include widget build in npm package (unpkg/jsdelivr compatible) - Add --help flag with full usage docs - Remove noxkey fallback (developer-only) - .shotfix/.gitignore now includes .env Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis pull request adds widget asset building and serving capabilities to the CLI package, introduces interactive Gemini API key management with environment file persistence, and inverts the watch mode flag to be enabled by default. It also updates package metadata and adds several CLI enhancements. Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI
participant Env as Environment
participant FileSystem as File System
participant GeminiAPI as Gemini API
User->>CLI: Start CLI (watch mode enabled by default)
CLI->>Env: Check GEMINI_API_KEY env var
alt API key in environment
Env-->>CLI: Return API key
CLI->>GeminiAPI: Use API key
else API key not in environment
CLI->>FileSystem: Check .shotfix/.env file
alt API key in file
FileSystem-->>CLI: Return API key from file
CLI->>GeminiAPI: Use API key
else API key not in file and TTY mode
CLI->>User: Prompt for Gemini API key
User-->>CLI: Enter API key
CLI->>FileSystem: Write API key to .shotfix/.env
FileSystem-->>CLI: File saved
CLI->>User: Confirm API key saved
CLI->>GeminiAPI: Use API key
end
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
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 significantly enhances the first-run experience and overall usability of the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
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 significantly improves the first-run experience for shotfix by adding a --help command, interactive API key setup, and serving the widget directly from the CLI. The changes are well-implemented. My review includes suggestions to enhance robustness and maintainability, focusing on cross-platform build script compatibility, improved error handling for file operations, and more resilient parsing of configuration files.
| try { | ||
| const existing = await readFile(join(shotfixDir, '.gitignore'), 'utf-8'); | ||
| if (!existing.includes('.env')) { | ||
| await writeFile(join(shotfixDir, '.gitignore'), existing.trimEnd() + '\n.env\n'); | ||
| } | ||
| } catch {} |
There was a problem hiding this comment.
The nested catch block is empty, which silences any errors that might occur when trying to read or update the .gitignore file. If an error other than the file already existing occurs (e.g., a permissions issue), it will fail silently, and the .env file might not be ignored. This increases the risk of accidentally committing secrets. It's crucial to log potential errors here to alert the user.
try {
const existing = await readFile(join(shotfixDir, '.gitignore'), 'utf-8');
if (!existing.includes('.env')) {
await writeFile(join(shotfixDir, '.gitignore'), existing.trimEnd() + '\n.env\n');
}
} catch (e) {
console.warn(`[shotfix] Failed to update .shotfix/.gitignore: ${(e as Error).message}`);
}| "build": "tsc", | ||
| "start": "node dist/index.js", | ||
| "prepublishOnly": "npm run build" | ||
| "build:widget": "cd ../widget && npm run build && mkdir -p ../cli/widget/dist && cp dist/shotfix.min.js ../cli/widget/dist/", |
There was a problem hiding this comment.
The build:widget script relies on shell commands (cd, mkdir -p, cp) that may not be available or behave consistently across all operating systems, particularly on standard Windows command prompts. This can hinder cross-platform development and usage. For better portability, consider using Node.js-based utilities for file system operations, either through a library like fs-extra or by creating a small Node script that uses the built-in fs/promises module.
| const match = envContent.match(/^GEMINI_API_KEY=(.+)$/m); | ||
| if (match) apiKey = match[1].trim(); |
There was a problem hiding this comment.
The regular expression for parsing the .env file is not robust enough to handle common formats, such as values enclosed in quotes or lines with trailing comments. This could lead to parsing an incorrect API key. To improve reliability, the regex should be updated to accommodate these variations.
const match = envContent.match(/^\s*GEMINI_API_KEY=(.*?)(\s*#.*)?$/m);
if (match) apiKey = match[1].trim().replace(/^['"]|['"]$/g, '');| } catch { | ||
| res.writeHead(404, { 'Content-Type': 'text/plain' }); | ||
| res.end('Widget not found'); | ||
| } |
There was a problem hiding this comment.
The catch block for serving /widget.js sends a 404 response but doesn't log the error on the server. If reading the file fails for any reason (e.g., permissions, build errors), the issue is not visible on the server, making debugging difficult. Logging the error here would provide valuable diagnostic information.
} catch (err) {
console.error(`[shotfix] Error serving widget.js: ${(err as Error).message}`);
res.writeHead(404, { 'Content-Type': 'text/plain' });
res.end('Widget not found');
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
cli/package.json (1)
16-17: Makebuild:widgetpublish flow cross-platform.Line 16 relies on POSIX shell commands (
mkdir -p,cp), which will fail in non-POSIX publish environments like Windows PowerShell or cmd.exe.♻️ Proposed refactor
- "build:widget": "cd ../widget && npm run build && mkdir -p ../cli/widget/dist && cp dist/shotfix.min.js ../cli/widget/dist/", + "build:widget": "node ./scripts/build-widget.mjs",// scripts/build-widget.mjs import { mkdir, copyFile } from 'node:fs/promises'; import { resolve } from 'node:path'; import { execFileSync } from 'node:child_process'; const widgetDir = resolve(process.cwd(), '../widget'); const targetDir = resolve(process.cwd(), 'widget/dist'); execFileSync('npm', ['run', 'build'], { cwd: widgetDir, stdio: 'inherit' }); await mkdir(targetDir, { recursive: true }); await copyFile(resolve(widgetDir, 'dist/shotfix.min.js'), resolve(targetDir, 'shotfix.min.js'));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/package.json` around lines 16 - 17, Replace the POSIX-only "build:widget" npm script with a cross-platform Node script: add a new scripts/build-widget.mjs that runs npm run build in the ../widget folder (use execFileSync or spawnSync), creates the target folder (use fs/promises.mkdir with { recursive: true }) and copies dist/shotfix.min.js into cli/widget/dist (use fs/promises.copyFile or copyFile), then update package.json "build:widget" to invoke node ./scripts/build-widget.mjs and keep "prepublishOnly" referencing "npm run build:widget && npm run build"; ensure the identifiers mentioned (build:widget, prepublishOnly, scripts/build-widget.mjs and functions execFileSync, mkdir, copyFile) are used so the change is cross-platform.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/src/cli.ts`:
- Around line 97-101: Several console.log calls in cli.ts (notably the branch
that checks apiKey and watchMode using variables apiKey and watchMode, plus the
other occurrences around the blocks referenced) are missing the required
"[Shotfix]" prefix; update each plain console.log invocation (including the
messages at the apiKey/watchMode check and the other occurrences flagged) to
prepend "[Shotfix] " to the message string so all CLI output follows the project
logging prefix rule. Locate the console.log calls in the top-level CLI flow (the
apiKey/watchMode branch) and the other unprefixed console.log blocks and change
their messages to start with "[Shotfix]" while preserving the original message
text and spacing.
- Around line 24-25: Remove the user-configurable port option from the CLI:
delete the `--port` entry from the help/usage text and remove any
parsing/handling of a `port` flag/option in cli.ts (e.g., where CLI args are
parsed or a `port` variable is set). Ensure the server always uses a fixed
constant PORT = 2847 (replace any reference to parsed port with the constant) so
the health/capture endpoints remain on 2847; update any variables or function
calls that used `port` to reference the fixed `PORT` instead and remove unused
imports/variables related to the removed option.
- Around line 50-55: The existing .gitignore backfill only appends ".env";
update the logic in the try block that reads/writes join(shotfixDir,
'.gitignore') to check for and append any missing entries from the list [".env",
"captures/", "screenshots.json"] (or whichever specific screenshot JSON filename
you use) instead of only ".env"; use the existing variable names (readFile,
writeFile, shotfixDir, existing) to build a newContents string that adds only
the missing lines and then writeFile(join(shotfixDir, '.gitignore'),
newContents).
- Around line 111-114: The .shotfix/.env is being written with default
permissions; update the mkdir and writeFile calls so the directory is created
with restrictive mode and the env file is written with mode 0o600: when calling
mkdir(shotfixDir, { recursive: true }) include a mode of 0o700 and when calling
writeFile(join(shotfixDir, '.env'), ...) pass options with mode: 0o600 (or
alternatively call fs.chmod after writing) so the API key file is only
readable/writable by the owner; locate the mkdir and writeFile usages in cli.ts
to apply this change.
---
Nitpick comments:
In `@cli/package.json`:
- Around line 16-17: Replace the POSIX-only "build:widget" npm script with a
cross-platform Node script: add a new scripts/build-widget.mjs that runs npm run
build in the ../widget folder (use execFileSync or spawnSync), creates the
target folder (use fs/promises.mkdir with { recursive: true }) and copies
dist/shotfix.min.js into cli/widget/dist (use fs/promises.copyFile or copyFile),
then update package.json "build:widget" to invoke node
./scripts/build-widget.mjs and keep "prepublishOnly" referencing "npm run
build:widget && npm run build"; ensure the identifiers mentioned (build:widget,
prepublishOnly, scripts/build-widget.mjs and functions execFileSync, mkdir,
copyFile) are used so the change is cross-platform.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 872934d2-b118-4e98-b686-e7657a0243f4
📒 Files selected for processing (2)
cli/package.jsoncli/src/cli.ts
| --port <n> Server port (default: 2847) | ||
| --dir <path> Captures directory (default: .shotfix/captures) |
There was a problem hiding this comment.
Remove --port from user-facing CLI behavior to keep endpoint contract fixed.
Line 24 advertises a configurable port, but this service contract is required to stay on 2847 for health/capture integration.
🔧 Proposed fix
- --port <n> Server port (default: 2847)
--dir <path> Captures directory (default: .shotfix/captures)And remove --port parsing so the server always listens on PORT = 2847.
As per coding guidelines: "cli/src/**/*.{js,ts}: Fixed port 2847 must be used for the local dev server health check and capture endpoint".
📝 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.
| --port <n> Server port (default: 2847) | |
| --dir <path> Captures directory (default: .shotfix/captures) | |
| --dir <path> Captures directory (default: .shotfix/captures) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/cli.ts` around lines 24 - 25, Remove the user-configurable port
option from the CLI: delete the `--port` entry from the help/usage text and
remove any parsing/handling of a `port` flag/option in cli.ts (e.g., where CLI
args are parsed or a `port` variable is set). Ensure the server always uses a
fixed constant PORT = 2847 (replace any reference to parsed port with the
constant) so the health/capture endpoints remain on 2847; update any variables
or function calls that used `port` to reference the fixed `PORT` instead and
remove unused imports/variables related to the removed option.
| // Already exists — make sure .env is in it | ||
| try { | ||
| const existing = await readFile(join(shotfixDir, '.gitignore'), 'utf-8'); | ||
| if (!existing.includes('.env')) { | ||
| await writeFile(join(shotfixDir, '.gitignore'), existing.trimEnd() + '\n.env\n'); | ||
| } |
There was a problem hiding this comment.
Backfill both .gitignore entries when the file already exists.
Lines 53-55 only append .env. If an existing .shotfix/.gitignore is missing captures/, screenshots/JSON can still be committed accidentally.
🛠️ Proposed fix
- const existing = await readFile(join(shotfixDir, '.gitignore'), 'utf-8');
- if (!existing.includes('.env')) {
- await writeFile(join(shotfixDir, '.gitignore'), existing.trimEnd() + '\n.env\n');
- }
+ const gitignorePath = join(shotfixDir, '.gitignore');
+ const existing = await readFile(gitignorePath, 'utf-8');
+ const lines = new Set(existing.split(/\r?\n/).filter(Boolean));
+ lines.add('captures/');
+ lines.add('.env');
+ await writeFile(gitignorePath, `${Array.from(lines).join('\n')}\n`);📝 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.
| // Already exists — make sure .env is in it | |
| try { | |
| const existing = await readFile(join(shotfixDir, '.gitignore'), 'utf-8'); | |
| if (!existing.includes('.env')) { | |
| await writeFile(join(shotfixDir, '.gitignore'), existing.trimEnd() + '\n.env\n'); | |
| } | |
| // Already exists — make sure .env is in it | |
| try { | |
| const gitignorePath = join(shotfixDir, '.gitignore'); | |
| const existing = await readFile(gitignorePath, 'utf-8'); | |
| const lines = new Set(existing.split(/\r?\n/).filter(Boolean)); | |
| lines.add('captures/'); | |
| lines.add('.env'); | |
| await writeFile(gitignorePath, `${Array.from(lines).join('\n')}\n`); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/cli.ts` around lines 50 - 55, The existing .gitignore backfill only
appends ".env"; update the logic in the try block that reads/writes
join(shotfixDir, '.gitignore') to check for and append any missing entries from
the list [".env", "captures/", "screenshots.json"] (or whichever specific
screenshot JSON filename you use) instead of only ".env"; use the existing
variable names (readFile, writeFile, shotfixDir, existing) to build a
newContents string that adds only the missing lines and then
writeFile(join(shotfixDir, '.gitignore'), newContents).
| if (!apiKey && watchMode && process.stdin.isTTY) { | ||
| console.log(''); | ||
| console.log(' 🔑 Shotfix needs a Gemini API key for auto-fix.'); | ||
| console.log(' Get one free at: https://aistudio.google.com/apikey'); | ||
| console.log(''); |
There was a problem hiding this comment.
Prefix new console logs with [Shotfix].
Lines 99-101, 114, 483, and 507-520 use unprefixed console output; changed logs should follow the project logging prefix rule.
As per coding guidelines: "Applies to /{widget,cli}//*.{js,ts} : Use [Shotfix] prefix for all console logging".
Also applies to: 114-114, 483-483, 507-520
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/cli.ts` around lines 97 - 101, Several console.log calls in cli.ts
(notably the branch that checks apiKey and watchMode using variables apiKey and
watchMode, plus the other occurrences around the blocks referenced) are missing
the required "[Shotfix]" prefix; update each plain console.log invocation
(including the messages at the apiKey/watchMode check and the other occurrences
flagged) to prepend "[Shotfix] " to the message string so all CLI output follows
the project logging prefix rule. Locate the console.log calls in the top-level
CLI flow (the apiKey/watchMode branch) and the other unprefixed console.log
blocks and change their messages to start with "[Shotfix]" while preserving the
original message text and spacing.
| if (apiKey) { | ||
| await mkdir(shotfixDir, { recursive: true }); | ||
| await writeFile(join(shotfixDir, '.env'), `GEMINI_API_KEY=${apiKey}\n`); | ||
| console.log(' ✅ Saved to .shotfix/.env\n'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify .env write call and whether explicit restrictive mode is set.
rg -n "writeFile\\(\\s*join\\(shotfixDir, '\\.env'\\)" cli/src/cli.ts -C4Repository: No-Box-Dev/shotfix
Length of output: 317
🏁 Script executed:
# Check if there are other instances of writeFile in this file that might have the same issue
rg -n "writeFile" cli/src/cli.tsRepository: No-Box-Dev/shotfix
Length of output: 772
Write .shotfix/.env with restrictive permissions.
Line 113 saves the API key without explicit file mode; default permissions (typically 0o644 with common umask settings) make the file readable by other users on the system.
Fix
- await writeFile(join(shotfixDir, '.env'), `GEMINI_API_KEY=${apiKey}\n`);
+ await writeFile(
+ join(shotfixDir, '.env'),
+ `GEMINI_API_KEY=${apiKey}\n`,
+ { mode: 0o600 }
+ );📝 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.
| if (apiKey) { | |
| await mkdir(shotfixDir, { recursive: true }); | |
| await writeFile(join(shotfixDir, '.env'), `GEMINI_API_KEY=${apiKey}\n`); | |
| console.log(' ✅ Saved to .shotfix/.env\n'); | |
| if (apiKey) { | |
| await mkdir(shotfixDir, { recursive: true }); | |
| await writeFile( | |
| join(shotfixDir, '.env'), | |
| `GEMINI_API_KEY=${apiKey}\n`, | |
| { mode: 0o600 } | |
| ); | |
| console.log(' ✅ Saved to .shotfix/.env\n'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/src/cli.ts` around lines 111 - 114, The .shotfix/.env is being written
with default permissions; update the mkdir and writeFile calls so the directory
is created with restrictive mode and the env file is written with mode 0o600:
when calling mkdir(shotfixDir, { recursive: true }) include a mode of 0o700 and
when calling writeFile(join(shotfixDir, '.env'), ...) pass options with mode:
0o600 (or alternatively call fs.chmod after writing) so the API key file is only
readable/writable by the owner; locate the mkdir and writeFile usages in cli.ts
to apply this change.
1 participant
Summary
shotfixstarts with auto-fix enabled; use--no-watchto disable.shotfix/.env, loads automatically on subsequent runs/widget.jsendpoint serves the built widget JS directly from the CLI server--helpflag — full usage documentationTest plan
node cli/dist/bin.js --helpshows usagenode cli/dist/bin.jswithout API key prompts interactively (saves to .shotfix/.env)GEMINI_API_KEY=xxx node cli/dist/bin.jsstarts with watch mode ONnode cli/dist/bin.js --no-watchstarts with watch mode OFFcurl localhost:2847/widget.jsreturns the widget JScurl localhost:2847/healthreturns{"status":"ok","watch":true}captures/and.env🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
--helpflag to display usage information and version details.--no-watchto disable.Chores