|
| 1 | +# Recommended CLAUDE.md Improvements |
| 2 | + |
| 3 | +Based on the CI breakage analysis, here are specific additions to prevent similar issues: |
| 4 | + |
| 5 | +## 1. Add Section on Large Refactors and Directory Structure Changes |
| 6 | + |
| 7 | +**CRITICAL: When working on PRs that change directory structure or move files:** |
| 8 | + |
| 9 | +1. **Create a comprehensive checklist** of all places that reference the old paths: |
| 10 | + |
| 11 | + - Search for hardcoded paths in all config files (grep -r "old/path" .) |
| 12 | + - Check package.json, package-scripts.yml, webpack configs, CI workflows |
| 13 | + - Check documentation and example code |
| 14 | + - Check generator templates |
| 15 | + |
| 16 | +2. **Run ALL test suites** after directory changes: |
| 17 | + |
| 18 | + - `rake` (all tests) |
| 19 | + - `rake run_rspec:example_basic` (generator tests) |
| 20 | + - Manual test of `yalc publish` if changing package structure |
| 21 | + - Build and test in a fresh clone to catch path issues |
| 22 | + |
| 23 | +3. **Add temporary validation** in CI: |
| 24 | + |
| 25 | + - Add a CI job that explicitly tests the changed paths |
| 26 | + - Keep this validation for 2-3 releases after the refactor |
| 27 | + |
| 28 | +4. **Document the change** in CHANGELOG.md with migration instructions |
| 29 | + |
| 30 | +5. **Consider breaking large refactors into smaller PRs**: |
| 31 | + - PR 1: Prepare new structure (create new dirs, copy files) |
| 32 | + - PR 2: Update references |
| 33 | + - PR 3: Remove old structure |
| 34 | + - This makes it easier to catch issues and roll back if needed |
| 35 | + |
| 36 | +## 2. Add Section on Build Script Testing |
| 37 | + |
| 38 | +**CRITICAL: Build scripts in package.json and package-scripts.yml need validation:** |
| 39 | + |
| 40 | +1. **The prepack/prepare scripts are CRITICAL** - they run during: |
| 41 | + |
| 42 | + - `npm install` / `yarn install` (for git dependencies) |
| 43 | + - `yalc publish` |
| 44 | + - `npm publish` |
| 45 | + - Package manager prepare phase |
| 46 | + |
| 47 | +2. **Always test these scripts manually after changes:** |
| 48 | + |
| 49 | + ```bash |
| 50 | + # Test the prepack script |
| 51 | + yarn run prepack |
| 52 | + |
| 53 | + # Test yalc publish (critical for local development) |
| 54 | + yarn run yalc.publish |
| 55 | + |
| 56 | + # Verify build artifacts exist at expected paths |
| 57 | + ls -la lib/ReactOnRails.full.js |
| 58 | + ls -la packages/*/lib/ |
| 59 | + ``` |
| 60 | + |
| 61 | +3. **If you change directory structure:** |
| 62 | + |
| 63 | + - Update ALL path checks in package-scripts.yml |
| 64 | + - Test with a clean install: `rm -rf node_modules && yarn install` |
| 65 | + - Test yalc publish to ensure it works for users |
| 66 | + |
| 67 | +4. **Add tests for critical build paths:** |
| 68 | + - Consider adding a CI job that validates expected build artifacts exist |
| 69 | + - Add tests that verify package.json/package-scripts.yml paths are correct |
| 70 | + |
| 71 | +## 3. Add Section on Monitoring Master Health |
| 72 | + |
| 73 | +**CRITICAL: Don't let master stay broken:** |
| 74 | + |
| 75 | +1. **If CI fails on master after your PR merges:** |
| 76 | + |
| 77 | + - Check GitHub Actions within 30 minutes of merge |
| 78 | + - Run `gh pr view <pr-number> --json statusCheckRollup` after merge |
| 79 | + - Set up GitHub notifications for master branch failures |
| 80 | + |
| 81 | +2. **If you discover master is broken:** |
| 82 | + |
| 83 | + - Investigate IMMEDIATELY - don't assume "someone else will fix it" |
| 84 | + - Use `gh run list --branch master --limit 10` to see recent failures |
| 85 | + - Check if it's a recurring failure or a new issue |
| 86 | + - If the failure is from your PR, either: |
| 87 | + - Submit a fix PR immediately, OR |
| 88 | + - Consider reverting your PR and re-submitting with the fix |
| 89 | + |
| 90 | +3. **Silent failures are the most dangerous:** |
| 91 | + |
| 92 | + - yalc publish failures won't show up in most CI runs |
| 93 | + - Build artifact path issues may only surface during actual usage |
| 94 | + - Always test the actual use case (yalc publish, npm install from git, etc.) |
| 95 | + |
| 96 | +4. **When manually re-running workflows:** |
| 97 | + - Re-running a workflow DOES NOT change its conclusion in the GitHub API |
| 98 | + - Our CI safety checks now handle this, but be aware of the limitation |
| 99 | + - If a rerun succeeds, monitor that subsequent commits don't get blocked |
| 100 | + |
| 101 | +## 4. Update the Merge Conflict Resolution Section |
| 102 | + |
| 103 | +**Enhancement to existing section:** |
| 104 | + |
| 105 | +```markdown |
| 106 | +### Merge Conflict Resolution Workflow |
| 107 | + |
| 108 | +**CRITICAL**: When resolving merge conflicts, follow this exact sequence: |
| 109 | + |
| 110 | +1. **Resolve logical conflicts only** - don't worry about formatting |
| 111 | +2. **VERIFY FILE PATHS** - if the conflict involved directory structure: |
| 112 | + - Check if any hardcoded paths need updating |
| 113 | + - Grep for old paths: `grep -r "old/path" .` |
| 114 | + - Pay special attention to package-scripts.yml, webpack configs |
| 115 | +3. **Add resolved files**: `git add .` (or specific files) |
| 116 | +4. **Auto-fix everything**: `rake autofix` |
| 117 | +5. **Add any formatting changes**: `git add .` |
| 118 | +6. **Continue rebase/merge**: `git rebase --continue` or `git commit` |
| 119 | +7. **TEST CRITICAL SCRIPTS**: If package-scripts.yml or build configs changed: |
| 120 | + - Run `yarn run prepack` to verify build scripts work |
| 121 | + - Run `yarn run yalc.publish` if package structure changed |
| 122 | + - Run relevant test suites |
| 123 | + |
| 124 | +**❌ NEVER manually format during conflict resolution** - this causes formatting wars. |
| 125 | +**❌ NEVER blindly accept path changes** - verify they're correct for current structure. |
| 126 | +``` |
| 127 | + |
| 128 | +## 5. Add Section on Path Management |
| 129 | + |
| 130 | +**NEW SECTION:** |
| 131 | + |
| 132 | +````markdown |
| 133 | +## Managing File Paths in Configuration |
| 134 | + |
| 135 | +**CRITICAL: Hardcoded paths are a major source of bugs:** |
| 136 | + |
| 137 | +1. **Before committing changes to any config file with paths:** |
| 138 | + |
| 139 | + - Verify the path actually exists: `ls -la <path>` |
| 140 | + - Test that operations using the path work |
| 141 | + - If changing package structure, search for ALL references to old paths |
| 142 | + |
| 143 | +2. **Common files with path references:** |
| 144 | + |
| 145 | + - `package-scripts.yml` - build artifact paths |
| 146 | + - `package.json` - "files", "main", "types" fields |
| 147 | + - Webpack configs - output.path, resolve.modules |
| 148 | + - `.github/workflows/*.yml` - cache paths, artifact paths |
| 149 | + - Generator templates - paths in generated code |
| 150 | + - Documentation - example paths |
| 151 | + |
| 152 | +3. **After any directory structure change:** |
| 153 | + |
| 154 | + ```bash |
| 155 | + # Find potential hardcoded paths (adjust as needed) |
| 156 | + grep -r "node_package" . --exclude-dir=node_modules --exclude-dir=.git |
| 157 | + grep -r "packages/react-on-rails" . --exclude-dir=node_modules |
| 158 | + |
| 159 | + # Verify build artifacts are in expected locations |
| 160 | + yarn build |
| 161 | + find . -name "ReactOnRails.full.js" -type f |
| 162 | + ``` |
| 163 | +```` |
| 164 | + |
| 165 | +4. **Use variables/constants when possible:** |
| 166 | + - In JavaScript/TypeScript, use `__dirname` or import.meta.url |
| 167 | + - In Ruby, use `Rails.root` or `File.expand_path` |
| 168 | + - Avoid hardcoding relative paths in config files when possible |
| 169 | + |
| 170 | +``` |
| 171 | +
|
| 172 | +--- |
| 173 | +
|
| 174 | +## Root Cause of This CI Breakage |
| 175 | +
|
| 176 | +**What happened:** |
| 177 | +
|
| 178 | +1. PR #1830 (Sep 29) changed directory from `node_package/` to `packages/react-on-rails/` |
| 179 | +2. The path in `package-scripts.yml` was updated to `packages/react-on-rails/lib/ReactOnRails.full.js` |
| 180 | +3. Later, the directory structure was partially reverted back to `lib/` at the root |
| 181 | +4. But `package-scripts.yml` wasn't updated, leaving the wrong path |
| 182 | +5. This made yalc publish fail silently for weeks |
| 183 | +6. When the path was fixed in PR #2054, the CI safety check blocked it due to previous failures |
| 184 | +7. The safety check had a bug: it checked `run.conclusion` which never updates after reruns |
| 185 | +
|
| 186 | +**What we learned:** |
| 187 | +
|
| 188 | +- ✅ Large refactors need comprehensive path audits |
| 189 | +- ✅ Build scripts (prepack/prepare) need manual testing |
| 190 | +- ✅ Silent failures (yalc publish) are dangerous |
| 191 | +- ✅ CI safety mechanisms need to handle manual reruns correctly |
| 192 | +- ✅ Master health needs active monitoring |
| 193 | +
|
| 194 | +**How the recommendations prevent this:** |
| 195 | +
|
| 196 | +1. Section 1 (Large Refactors) - Would have caught incomplete path updates |
| 197 | +2. Section 2 (Build Scripts) - Would have caught yalc publish failure early |
| 198 | +3. Section 3 (Master Health) - Would have alerted team to broken master |
| 199 | +4. Section 4 (Merge Conflicts) - Would prevent path regressions during merges |
| 200 | +5. Section 5 (Path Management) - Would have caught the incorrect path |
| 201 | +
|
| 202 | +--- |
| 203 | +
|
| 204 | +## Implementation Priority |
| 205 | +
|
| 206 | +**High Priority** (add to CLAUDE.md this week): |
| 207 | +- Section 2: Testing Build and Package Scripts |
| 208 | +- Section 3: Master Branch Health Monitoring |
| 209 | +- Section 5: Managing File Paths in Configuration |
| 210 | +
|
| 211 | +**Medium Priority** (add within 2 weeks): |
| 212 | +- Section 1: Large Refactors and Directory Structure Changes |
| 213 | +- Section 4: Enhanced Merge Conflict Resolution |
| 214 | +
|
| 215 | +**Future Improvements** (consider later): |
| 216 | +- Automated tests for path validation |
| 217 | +- CI job to verify build artifacts exist at expected paths |
| 218 | +- Pre-commit hook to validate package-scripts.yml paths |
| 219 | +``` |
0 commit comments