Skip to content

Conversation

@ainsleyclark
Copy link
Contributor

  • Creates scripts/check-deps.js in Payload apps to validate dependencies are in sync
  • Updates migrate:create script to run dependency check first
  • Prevents CI failures caused by outdated node_modules when running migrations
  • Adds troubleshooting documentation for common migration check issues

claude added 2 commits January 2, 2026 12:52
- Creates scripts/check-deps.js in Payload apps to validate dependencies are in sync
- Updates migrate:create script to run dependency check first
- Prevents CI failures caused by outdated node_modules when running migrations
- Adds troubleshooting documentation for common migration check issues
@claude

This comment has been minimized.

- Use content comparison instead of timestamps to avoid false positives
- Compare pnpm-lock.yaml against node_modules/.pnpm/lock.yaml
- Simplify troubleshooting documentation to focus on essential info
@claude

This comment has been minimized.

- Update package_json_test.go to expect new migrate:check script
- Simplify error wrapping per AGENTS.md guidelines
- Add comment explaining why strict file comparison is correct
@claude

This comment has been minimized.

@claude

This comment has been minimized.

- Move check-deps.js to internal/templates/scripts/
- Update migration_check.go to use CopyFromEmbed
- Add brief commentary to script header
- Follows webkit template pattern for better maintainability
@claude

This comment has been minimized.

@claude
Copy link

claude bot commented Jan 2, 2026

Review summary

  • Overall score: 7/10
  • Critical issues: 0
  • Warnings: 2
  • Suggestions: 2
  • Recommendation: ⚠️ Approve with comments

This PR adds a dependency check script to prevent CI migration failures in Payload apps. The implementation is well-tested and follows project conventions, but has a potential issue with strict lockfile comparison and missing documentation comments in the JavaScript file.

Critical issues 🔴

None

Warnings 🟡

1. Strict lockfile equality may cause false positives

Location: internal/templates/scripts/check-deps.js:27

The script uses strict string equality to compare pnpm-lock.yaml with node_modules/.pnpm/lock.yaml. While the comment claims "pnpm creates identical lockfiles," this may not always be true:

  • Different pnpm versions can produce slightly different lockfile formats
  • Whitespace or comment variations could exist
  • pnpm may add metadata that differs between the root and node_modules copy
Recommended approach

Consider parsing and comparing the YAML content semantically, or check the lockfile version field first. Alternatively, use pnpm's built-in pnpm install --frozen-lockfile --offline to verify sync without duplicating pnpm's logic.

// Example alternative using pnpm itself
const { execSync } = require('child_process');
try {
	execSync('pnpm install --frozen-lockfile --offline', { 
		stdio: 'pipe',
		cwd: path.join(__dirname, '..') 
	});
	console.log('✅ Dependencies are in sync');
} catch (err) {
	console.error('❌ Dependencies out of sync!');
	console.error('   Run: pnpm install');
	process.exit(1);
}

2. Missing JSDoc comment for script purpose

Location: internal/templates/scripts/check-deps.js:1-6

According to AGENTS.md section "JS > Documentation", exported functions should have JSDoc comments. While this is a standalone script, adding a proper JSDoc block would improve clarity and follow project conventions.

/**
 * Dependency synchronisation check for Payload CMS migrations.
 * Compares pnpm-lock.yaml against the cached lockfile in node_modules
 * to prevent CI failures from outdated dependencies.
 */

Suggestions 🟢

1. Consider adding emoji consistency note

Location: docs/getting-started/troubleshooting.md

The troubleshooting doc includes emojis (❌, ✅) which appear in script output. According to AGENTS.md content guidelines, emojis should generally be avoided unless explicitly requested. However, since these emojis are part of the script's user-facing output (not documentation prose), this may be acceptable. Consider documenting this decision or keeping script output plain.

2. Test coverage for actual file content validation

Location: internal/cmd/files/migration_check_test.go:73-78

The test verifies that the generated script contains expected strings, but doesn't validate that the script would actually work correctly. Consider adding an integration test that:

  • Creates actual lock files
  • Executes the script with Node
  • Verifies exit codes

This would catch issues like syntax errors or incorrect file path resolution.


Scoring rationale: The implementation is solid with comprehensive unit tests, proper integration into the update flow, and clear documentation. The main concern is the lockfile comparison approach which could cause false positives in production. The missing JSDoc and lack of integration testing are minor issues. Once the lockfile comparison is validated or improved, this would be an 8-9/10.

@ainsleyclark ainsleyclark merged commit 5d1ba13 into main Jan 2, 2026
4 checks passed
@ainsleyclark ainsleyclark deleted the claude/fix-ci-migration-check-ZMyoa branch January 2, 2026 19:10
@codecov
Copy link

codecov bot commented Jan 2, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.53%. Comparing base (7f6b060) to head (ebded2b).
⚠️ Report is 437 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #359      +/-   ##
==========================================
+ Coverage   64.59%   69.53%   +4.94%     
==========================================
  Files         154      185      +31     
  Lines        6064     7294    +1230     
==========================================
+ Hits         3917     5072    +1155     
+ Misses       2064     2025      -39     
- Partials       83      197     +114     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants