Some minor QoL features around spellcheck

[plx]

No description provided.

[github-actions]

PR Validation ❌

Some checks failed. Please review the errors above.

Checks Performed:

Please check the workflow logs for details on which checks failed.

This is a complete dry-run of the deployment process, ensuring your changes will deploy successfully when merged.

[claude]

PR Review: Some minor QoL features around spellcheck

Thanks for these quality-of-life improvements! This PR adds useful tooling around spellcheck management and refactors the justfile. Overall, the changes are well-implemented. Here's my detailed review:

---

✅ Code Quality & Best Practices

Strengths:

• Good refactoring: Moving bash scripts from justfile into separate shell scripts improves maintainability and testability
• Consistent error handling: All shell scripts use set -euo pipefail which is excellent practice
• Clear script structure: Each shell script validates arguments and provides helpful error messages
• Useful utility: The learn-spelling.js script addresses a real pain point in managing spell check dictionaries
• Good UX: Clear console output with counts and duplicate detection

Minor Issues:

1. Module type mismatch in scripts/learn-spelling.js:

   • The script uses CommonJS (require, module.exports style) but package.json specifies "type": "module"
   • Other scripts in the repo (like validate-links.js) use ES modules
   • Recommendation: Convert to ES modules for consistency:

     import fs from 'fs';
     import path from 'path';
     import { fileURLToPath } from 'url';
     
     const __filename = fileURLToPath(import.meta.url);
     const __dirname = path.dirname(__filename);

2. Typo in commit message: "incosistency" should be "inconsistency" in commit d0d3b6f

3. Shell script portability: open command in scripts/open-browser.sh:14 is macOS-specific

   • Won't work on Linux (would need xdg-open) or Windows (would need start)
   • Recommendation: Add platform detection or document macOS requirement in comments

---

🐛 Potential Bugs

1. Error handling in learn-spelling.js:

   • No try-catch around JSON.parse() at line 25 - could crash with corrupted JSON
   • No try-catch around fs.writeFileSync() at line 50 - could fail due to permissions
   • Recommendation: Add error handling:

     try {
       const config = JSON.parse(fs.readFileSync(configPath, 'utf8'));
     } catch (error) {
       console.error('Error reading cspell.json:', error.message);
       process.exit(1);
     }

2. Missing executable permissions check: The scripts have shebang lines but no explicit verification of execute permissions in the repo

---

⚡ Performance Considerations

• Performance is appropriate for the use case
• learn-spelling.js reads/writes synchronously, which is fine for a CLI tool
• Shell scripts use sleep 3 which is reasonable for server startup verification
• No concerns here

---

🔒 Security Concerns

Minor Issue:

• Command injection potential in shell scripts when interpolating $PORT variable
  • Currently mitigated by justfile's parameter handling, but if scripts are called directly with untrusted input, could be problematic
  • Recommendation: Add input validation in shell scripts:

    # Validate port is a number
    if ! [[ "$PORT" =~ ^[0-9]+$ ]]; then
        echo "Error: Port must be a number"
        exit 1
    fi

Good practices observed:

• Using double quotes around variables ("$PORT", "$PID") prevents word splitting
• Using || true to prevent failures from breaking set -e

---

🧪 Test Coverage

Missing:

• No tests for the new learn-spelling.js script
• No tests for the shell scripts

Recommendations:

1. Add basic validation tests for learn-spelling.js:

   • Test duplicate detection
   • Test sorting behavior
   • Test error cases (missing file, invalid JSON)

2. Consider adding shellcheck to CI to catch common shell script issues

3. Could add integration test to verify:

   • Words are actually added to cspell.json
   • Alphabetical sorting is preserved
   • File formatting matches expected structure

---

📝 Additional Observations

Positive changes:

1. cspell.json formatting improvements (multi-line arrays) improve readability
2. Adding software-terms dictionary to HTML spell check is sensible
3. The alphabetical fix for "Playwright" and "plx" is good attention to detail

Questions/Suggestions:

1. Should scripts/learn-spelling.js also be added to the spellcheck patterns in package.json line 14?
2. Consider documenting the new just learn-spelling command in the repository (if there's a README or docs)
3. The justfile comment on line 60 has a typo: "incosistency" → "inconsistency" (matches the commit message typo)

---

🎯 Summary

Overall Assessment: ✅ Good to merge with minor improvements

This PR delivers solid quality-of-life improvements. The main suggestions are:

1. Convert learn-spelling.js to ES modules for consistency
2. Add error handling to file operations
3. Add platform detection or documentation for open command
4. Consider adding basic tests

The changes are safe to merge as-is, but addressing the module type and error handling would improve robustness.

Priority fixes before merge:

• Convert to ES modules (prevents potential issues)
• Add error handling for JSON parsing

Nice-to-haves:

• Platform detection for browser opening
• Input validation in shell scripts
• Test coverage

[claude]

PR Review: QoL Features Around Spellcheck

Thanks for this PR! Overall, this is a clean refactoring that improves code organization and adds a helpful utility. Here's my detailed review:

✅ Strengths

1. Good refactoring: Extracting bash scripts from the justfile into separate files improves maintainability and readability
2. Clean script structure: All bash scripts follow best practices with proper shebang, set -euo pipefail, and argument validation
3. Useful feature: The learn-spelling.js script addresses a real workflow pain point
4. Consistent error handling: Good use of error messages with helpful guidance for users

🔍 Code Quality Observations

learn-spelling.js (scripts/learn-spelling.js)

Potential Issues:

1. Error handling: No try-catch around JSON.parse() or file operations. If cspell.json is malformed or unreadable, the script will crash with an unhelpful error.

   // Current (line 25):
   const config = JSON.parse(fs.readFileSync(configPath, 'utf8'));
   
   // Suggested:
   try {
       const config = JSON.parse(fs.readFileSync(configPath, 'utf8'));
   } catch (error) {
       console.error(`Error reading cspell.json: ${error.message}`);
       process.exit(1);
   }

2. Input validation: No sanitization of input words. Consider validating that words contain only valid characters and rejecting clearly invalid inputs.

3. Case sensitivity: The script doesn't handle case variations. If someone adds "GitHub" but "github" already exists, both will be kept. Consider case-insensitive duplicate detection or normalization.

Bash Scripts

Potential Issues:

1. open-browser.sh portability (scripts/open-browser.sh:15): Uses macOS-specific open command. This will fail on Linux. Consider detecting OS and using appropriate command:

   if command -v xdg-open > /dev/null; then
       xdg-open "http://localhost:$PORT"
   elif command -v open > /dev/null; then
       open "http://localhost:$PORT"
   else
       echo "Error: No browser opener found"
       exit 1
   fi

2. Race condition in preview-server.sh (scripts/preview-server.sh:24): The 3-second sleep is arbitrary. On slow machines, the server might not be ready; on fast machines, it's wasteful. Consider polling with timeout instead.

3. Signal handling: shutdown-server.sh:19 uses plain kill which sends SIGTERM. Consider graceful shutdown with timeout, then SIGKILL if needed.

cspell.json

Minor formatting changes: The dictionary arrays are reformatted to multi-line (which is fine), but this seems incidental rather than intentional. Also note that "software-terms" was added to the dist HTML override - is this intentional? It's not mentioned in the PR description.

🛡️ Security Considerations

1. Command injection risk: The bash scripts properly quote $PORT variable, which is good. However, if justfile ever passes unsanitized user input to these scripts, there could be issues.

2. Temp file location (scripts/preview-server.sh:21): Writing logs to /tmp/astro-$PORT.log could be a minor security issue in shared environments. Consider using a project-specific location or mktemp.

⚡ Performance Considerations

No significant performance concerns. The scripts are lightweight and appropriate for their use case.

🧪 Test Coverage

Missing tests: None of the new scripts have automated tests. Consider adding:

• Unit tests for learn-spelling.js (testing duplicate detection, sorting, file I/O errors)
• Integration tests for bash scripts (though these are harder to test portably)

At minimum, manual testing should verify:

• ✅ learn-spelling with valid words
• ✅ learn-spelling with duplicates
• ✅ learn-spelling with empty input
• ✅ learn-spelling with malformed JSON in cspell.json
• ✅ All bash scripts on both macOS and Linux (if applicable)

📋 Recommendations

High Priority:

1. Add error handling to learn-spelling.js for file operations and JSON parsing
2. Fix cross-platform compatibility in open-browser.sh

Medium Priority:
3. Add case-insensitive duplicate detection to learn-spelling.js
4. Improve server startup detection in preview-server.sh (polling instead of sleep)
5. Document the software-terms addition to cspell.json if intentional

Nice to Have:
6. Add input validation/sanitization to learn-spelling.js
7. Add basic tests for learn-spelling.js
8. Use project-specific temp directory for logs

Summary

This is good work that improves the codebase. The refactoring is clean, and the new learn-spelling feature is helpful. The main concerns are error handling robustness and cross-platform compatibility. With the high-priority fixes, this would be ready to merge.

Recommendation: Address error handling and cross-platform issues, then merge. ✅

[github-actions]

PR Validation ❌

Some checks failed. Please review the errors above.

Checks Performed:

Please check the workflow logs for details on which checks failed.

This is a complete dry-run of the deployment process, ensuring your changes will deploy successfully when merged.

[github-actions]

PR Validation ✅

All checks passed! Ready for review.

Checks Performed:

✓ Linting
✓ Spell check (source)
✓ Type checking & Build
✓ Spell check (HTML)
✓ Internal link validation
✓ Artifact upload

This is a complete dry-run of the deployment process, ensuring your changes will deploy successfully when merged.

[claude]

PR Review: Some minor QoL features around spellcheck

Overall, this is a solid PR that improves developer experience with good refactoring and useful new functionality. The changes are well-structured and follow good practices.

✅ Strengths

1. Good Separation of Concerns: Extracting bash scripts from the justfile into dedicated files improves maintainability and testability
2. Useful New Feature: The learn-spelling command provides a clean workflow for managing spelling dictionary
3. Consistent Code Style: JavaScript follows project conventions (double quotes, semicolons)
4. Proper Error Handling: Scripts validate inputs and check for edge cases

🔍 Code Quality Observations

scripts/learn-spelling.js

Positive:

• Clean, readable code with good comments
• Handles edge cases (duplicates, empty input)
• Case-insensitive sorting is a nice touch
• Proper exit codes

Minor Concerns:

1. Error Handling: No try-catch around fs.readFileSync or JSON.parse (lines 24-25). If cspell.json is malformed or missing, the script will crash ungracefully.

   // Consider adding:
   try {
       const config = JSON.parse(fs.readFileSync(configPath, "utf8"));
   } catch (err) {
       console.error(`Error reading cspell.json: ${err.message}`);
       process.exit(1);
   }

2. Assumption about words array: Line 28 assumes config.words exists or is falsy. While || [] handles this, it would be safer to validate the config structure.

Shell Scripts (preview-server.sh, shutdown-server.sh, open-browser.sh)

Positive:

• Proper use of set -euo pipefail for safety
• Consistent error messaging
• Good validation of arguments

Platform Compatibility Concern:

1. macOS-specific command in open-browser.sh: Line 14 uses open which only works on macOS. On Linux, this would need xdg-open. Consider:

   # Cross-platform browser opening
   if command -v open &> /dev/null; then
       open "http://localhost:$PORT"
   elif command -v xdg-open &> /dev/null; then
       xdg-open "http://localhost:$PORT"
   else
       echo "Browser URL: http://localhost:$PORT"
   fi

2. Race Condition: preview-server.sh:23 waits 3 seconds, but on slower systems the server might not be ready. Consider polling with a timeout instead.

cspell.json

Positive:

• Improved formatting (better readability)
• Added "software-terms" dictionary to HTML overrides (good catch!)

Minor:

• The alphabetization change (plx before Playwright) is technically correct but creates unnecessary noise in the diff

🛡️ Security Considerations

Low Risk: No security concerns identified. Scripts handle user input appropriately with proper quoting and validation.

⚡ Performance Considerations

No issues: The scripts are efficient for their purpose. The 3-second sleep in preview-server.sh is reasonable for startup time.

🧪 Test Coverage

Not Applicable: These are developer utility scripts. Manual testing is appropriate, though you could consider:

• Adding examples to documentation
• Testing on different platforms (especially the browser opening functionality)

📋 Suggestions

1. Add error handling to learn-spelling.js for file I/O operations
2. Make open-browser.sh cross-platform compatible
3. Consider making the sleep duration in preview-server.sh configurable or use polling
4. Optional: Add a --help flag to learn-spelling.js

📝 Documentation

The justfile comments are clear, but consider adding:

• A comment in justfile about learn-spelling usage (e.g., just learn-spelling "word1,word2,word3")
• Brief header comments in each shell script explaining their purpose

✅ Recommendation

Approve with minor suggestions. The core functionality is solid and ready to merge. The suggestions above are enhancements, not blockers. The platform compatibility issue with open is the only item I'd consider addressing before merge if you expect Linux contributors.

Nice work on the QoL improvements! 🎉

[github-actions]

PR Validation ✅

All checks passed! Ready for review.

Checks Performed:

✓ Linting
✓ Spell check (source)
✓ Type checking & Build
✓ Spell check (HTML)
✓ Internal link validation
✓ Artifact upload

This is a complete dry-run of the deployment process, ensuring your changes will deploy successfully when merged.

[claude]

Pull Request Review: QoL Features for Spellcheck

Overall, this is a well-structured PR that adds useful developer quality-of-life improvements. Here's my detailed review:

✅ Strengths

1. Good Code Organization: Extracting shell logic from the justfile into dedicated scripts in scripts/ is a solid architectural decision that improves maintainability and testability.

2. Helpful Automation: The Claude Code hook for auto-linting on save is a nice developer experience improvement.

3. Practical Utility: The learn-spelling.js script addresses a common pain point - adding words to the spell check dictionary.

4. Proper Error Handling: All shell scripts use set -euo pipefail for robust error handling.

🔍 Issues & Recommendations

1. Security/Robustness Issues

scripts/learn-spelling.js:24-25 - Missing error handling for file operations:

const configPath = path.join(process.cwd(), "cspell.json");
const config = JSON.parse(fs.readFileSync(configPath, "utf8"));

Issue: No error handling if cspell.json doesn't exist or contains invalid JSON.

Recommendation: Add try-catch:

try {
    const config = JSON.parse(fs.readFileSync(configPath, "utf8"));
} catch (err) {
    console.error(`Error reading cspell.json: ${err.message}`);
    process.exit(1);
}

2. Potential Bugs

scripts/open-browser.sh:15 - Platform-specific command:

open "http://localhost:$PORT"

Issue: The open command only works on macOS. Will fail on Linux (needs xdg-open) or Windows (needs start).

Recommendation: Add platform detection or document macOS-only requirement in CLAUDE.md.

3. Code Quality Issues

.claude/hooks/lint-on-save.sh:19 - Inefficient spellcheck:

*.md|*.mdx)
    cd "$CLAUDE_PROJECT_DIR"
    npm run spellcheck 2>/dev/null || true

Issue: Runs spellcheck on all source files when only one file changed. This could be slow for large projects.

Recommendation: Either:

• Accept this limitation (simple approach)
• Pass the specific file to cspell: npx cspell "$file_path" 2>/dev/null || true
• Document this behavior as a known limitation

scripts/preview-server.sh:24 - Fixed sleep duration:

sleep 3

Issue: Hard-coded 3-second wait may be too short on slower systems or too long on fast ones.

Recommendation: Consider a polling approach with timeout:

for i in {1..10}; do
    if lsof -i :"$PORT" | grep -q LISTEN; then
        echo "Server started successfully"
        exit 0
    fi
    sleep 1
done
echo "Error: Server failed to start in time"
exit 1

4. Missing Input Validation

scripts/learn-spelling.js:13-16 - No word validation:

const newWords = process.argv[2]
    .split(",")
    .map(w => w.trim())
    .filter(w => w.length > 0);

Issue: Accepts any input without validation. Could add problematic entries to the dictionary.

Recommendation: Add basic validation:

const newWords = process.argv[2]
    .split(",")
    .map(w => w.trim())
    .filter(w => w.length > 0 && /^[a-zA-Z0-9-]+$/.test(w));

5. Code Style

cspell.json - The formatting changes (adding line breaks in arrays) are fine but seem unrelated to the core PR purpose. Consider mentioning this was auto-formatting in the PR description.

🎯 Performance Considerations

• The lint-on-save.sh hook runs after every Write/Edit operation. For large codebases, consider adding a debounce mechanism or running linting asynchronously.
• The learn-spelling.js script rewrites the entire cspell.json file on every invocation. This is fine for occasional use but could be optimized if used frequently.

🔒 Security

No major security concerns. All user inputs are properly quoted in shell scripts, preventing injection attacks.

🧪 Test Coverage

Missing: No tests for the new scripts. Consider adding:

• Unit tests for learn-spelling.js
• Integration tests for shell scripts
• Test that the Claude hook doesn't break the workflow

📝 Documentation

Minor issue: The justfile comment says scripts are "gathered in a single place" but now much logic is in separate files. Update CLAUDE.md to mention the new scripts/ directory organization.

🎁 Nice-to-Haves (Optional)

1. Add a --dry-run flag to learn-spelling.js to preview changes
2. Make learn-spelling.js support reading words from stdin for better composability
3. Add color output to shell scripts for better UX (using tput or similar)
4. Consider adding a .nvmrc or documenting Node version requirements

📊 Summary

Verdict: ✅ Approve with minor suggestions

This PR adds genuine value and the code quality is good. The issues identified are mostly minor edge cases that could be addressed in follow-up work. The refactoring from inline justfile logic to separate scripts is a positive change.

Priority fixes (if you choose to address them):

1. Add error handling to learn-spelling.js
2. Document or fix the macOS-only open command
3. Consider the spellcheck performance issue

Great work on improving the developer experience! 🚀

[claude]

Code Review - PR #21: QoL Features Around Spellcheck

Thank you for this PR! I've reviewed the changes and have several observations organized by category.

---

Positive Highlights

Well-Structured Additions

• Script extraction: Moving inline bash from justfile to dedicated scripts is excellent for maintainability
• learn-spelling.js: Clean implementation with proper duplicate detection and alphabetical sorting
• Claude Code integration: The hook system setup is well-designed

Code Quality

• Proper error handling in bash scripts with set -euo pipefail
• Good input validation across all scripts
• Clear, informative error messages

---

Issues and Concerns

Critical: Security and Reliability

1. Shell Injection Vulnerability in lint-on-save.sh

Location: .claude/hooks/lint-on-save.sh:6, :15, :19

The hook script uses jq to parse JSON but doesn't validate/sanitize the file path before using it in shell commands. If a malicious file path contains shell metacharacters, it could lead to command injection.

Recommendation: Add validation to ensure the file path is safe and within the project directory.

2. Spellcheck Runs on ALL .md Files

Location: .claude/hooks/lint-on-save.sh:17-19

When any markdown file is edited, npm run spellcheck runs against the entire project, not just the edited file. This is inefficient and could be slow for large projects.

Recommendation: If cspell supports single-file checking, consider running it only on the modified file.

Medium Priority

3. macOS-Only Command in Scripts

Location: scripts/open-browser.sh:15

The open command is macOS-specific. This will fail on Linux/Windows.

Recommendation: Add cross-platform support using xdg-open for Linux or provide fallback messaging.

4. Missing write-pull-requests Permission

Location: .github/workflows/claude-code-review.yml:26

The workflow can read PRs but cannot write comments back to them. The gh pr comment command in the prompt will fail.

Required fix: Change pull-requests: read to pull-requests: write in the permissions section.

5. Hardcoded Sleep May Be Insufficient

Location: scripts/preview-server.sh:24

sleep 3 assumes the server starts in 3 seconds, but this may vary by machine load.

Better approach: Use a retry loop with timeout instead of a fixed sleep duration.

6. Race Condition in Server Scripts

Location: scripts/preview-server.sh

Between checking if port is in use (line 13) and starting the server (line 21), another process could bind to that port.

Note: This is unlikely in practice but worth documenting as a known limitation.

Low Priority

7. Inconsistent Error Output

Some scripts echo to stdout when nothing is found, others treat it as an error. Consider standardizing exit codes and output levels.

8. Missing Executable Check

The script has a shebang but the justfile calls it via node. Either remove the shebang or make it executable and call it directly.

---

Other Observations

CLAUDE.md Cleanup

The reduction from 206 lines to 118 lines is great—much more focused and readable. The removal of verbose explanations in favor of succinct documentation is a positive change.

Justfile Refactoring

Extracting bash to separate scripts is the right call. The justfile is now much cleaner and maintainable.

Content Updates

The generic-testing blog post received substantial updates (426 additions). I didn't review content quality in detail, but the structure looks sound.

---

Recommendations Summary

Must Fix Before Merge:

1. Add pull-requests: write permission to claude-code-review.yml
2. Sanitize file paths in lint-on-save.sh

Should Fix:
3. Make open-browser.sh cross-platform
4. Optimize spellcheck to run on single files
5. Improve server startup waiting logic

Nice to Have:
6. Document race condition limitation
7. Standardize error handling
8. Clean up learn-spelling.js invocation

---

Overall Assessment

This is a solid QoL improvement PR that adds useful automation and refactors scripts nicely. The main concerns are:

• Security: file path validation in hooks
• Functionality: missing GitHub permissions will prevent the workflow from working

With those two items addressed, this will be good to merge. Great work on the refactoring and Claude Code integration!

---

Review generated by Claude Code