Fix class dropdown + add cache-busting for all non-machine characters

[RobGruhl]

No description provided.

[claude]

Pull Request Review: Fix class dropdown + add cache-busting

Summary

This PR addresses two important issues:

1. Fixes class dropdown logic - Ensures non-machine characters get all class options
2. Adds cache-busting - Implements versioned asset loading to prevent browser caching issues

---

✅ Code Quality & Best Practices

Strengths:

• Clean, well-structured JavaScript with clear logic flow
• Good separation of concerns (machine-only vs. non-machine characters)
• Descriptive comments explaining the intent
• Consistent code style with existing codebase
• Proper use of Python's datetime for version generation
• Template substitution approach is clean and maintainable

Minor Suggestions:

1. Cache-busting version format inconsistency: The build version format changed from 2025-10-14.005 to 2025.11.05.001 (dashes to dots). While this works, consider:

   • Keeping the format consistent with previous versions OR
   • Documenting the format change in commit message/docs

2. Magic strings: The isMachineOnly check uses hardcoded strings 'machine' and 'machineTech':

   const isMachineOnly = boards.every(board =>
       board.id === 'machine' || board.id === 'machineTech'
   );

   Consider extracting these as constants at the top of the file:

   const MACHINE_CLASS_IDS = ['machine', 'machineTech'];
   const isMachineOnly = boards.every(board => MACHINE_CLASS_IDS.includes(board.id));

---

🐛 Potential Bugs & Issues

No critical bugs identified, but some edge cases to consider:

1. Partial machine classes: What if a character has machine + another non-machine class? Current logic would treat them as non-machine, which seems correct, but worth validating against game rules.

2. Empty boards array: The code checks !boards || !boards.length early, which is good. However, the boards.every() call would return true for an empty array, making an empty character "machine-only". This is caught by the earlier guard, but worth noting.

3. Build version collision: If the script runs multiple times in a day, it always generates .001. Consider using a timestamp or incrementing counter:

   BUILD_VERSION = datetime.now().strftime("%Y.%m.%d.%H%M")

---

⚡ Performance Considerations

All changes are performance-neutral or positive:

✅ Cache-busting is good: Forces browser to reload updated assets
✅ Query parameters are lightweight: ?v=2025.11.05.001 adds negligible overhead
✅ Logic changes are O(n): boards.every() and boards.map() are efficient for small arrays
✅ No new network requests: Only forces reload when version changes

One consideration:

• Aggressive cache invalidation: Every deployment changes ALL file versions. Consider per-file hashing for larger projects, but for this small site, the current approach is fine.

---

🔒 Security Concerns

No security issues identified. The changes are low-risk:

✅ No user input processing
✅ No external API calls
✅ No sensitive data handling
✅ Template substitution is safe (Python's Template.substitute())
✅ No XSS vectors introduced
✅ Client-side storage (localStorage) use remains unchanged

---

🧪 Test Coverage

Current state: No automated tests found in repository

Recommendations:

1. Manual testing checklist (should be verified before merge):

   • Machine-only characters (XL?) show only machine/machineTech classes
   • Non-machine characters show all available classes
   • Class dropdown populates correctly for each character type
   • Hard refresh loads new JS/CSS versions
   • Character-specific class customizations still merge correctly
   • localStorage skill selections persist after version change

2. Consider adding tests (future enhancement):

   # test_generate_character_pages.py
   def test_build_version_format():
       assert BUILD_VERSION matches r'\d{4}\.\d{2}\.\d{2}\.\d{3}'
   
   def test_all_characters_generated():
       # Ensure all characters in JSON get HTML pages

---

📋 Specific Code Review

docs/js/character-page.js (lines 108-153)

The core logic change - well implemented:

// Check if this is a machine-only character
const isMachineOnly = boards.every(board =>
    board.id === 'machine' || board.id === 'machineTech'
);

✅ Correct logic: Uses every() to ensure ALL boards are machine classes
✅ Clear branching: Separates machine vs. non-machine paths
✅ Good comments: Explains the intent

The fallback merge is clever:

const customBoards = new Map(boards.map(board => [board.id, board]));
return fallbackClasses.map((globalClass) => {
    const customBoard = customBoards.get(globalClass.id);
    // ... merge logic
});

✅ Efficient lookup: Uses Map for O(1) customization lookup
✅ Preserves global classes: All classes available, with character customizations merged
✅ Fallback handling: Gracefully handles missing customizations

generate_character_pages.py (lines 6-13)

Cache-busting implementation - simple and effective:

BUILD_VERSION = datetime.now().strftime("%Y.%m.%d.001")

✅ Auto-generated: No manual version bumping needed
✅ Date-based: Easy to identify when pages were built

⚠️ Minor issue: Hardcoded .001 suffix - see "Potential Bugs" section above

templates/character-page.html (lines 7-9)

Template changes - properly applied:

<link rel="stylesheet" href="style.css?v=$build_version">

✅ Consistent: All three assets (CSS, CSS, JS) get versioned
✅ Query string approach: Works with all web servers
✅ Template variable: $build_version substituted at build time

---

🎯 Overall Assessment

Recommendation: ✅ APPROVE with minor suggestions

This is a solid PR that addresses real issues:

• The class dropdown fix is well-reasoned and properly implemented
• Cache-busting will prevent user confusion from stale assets
• Code quality is good and consistent with the project
• No security or performance concerns

Before merging:

1. Manually test machine vs. non-machine characters
2. Verify cache-busting works (hard refresh shows new version)
3. Consider the minor suggestions above (non-blocking)

Great work! The logic for handling machine-only vs. regular characters is clear and maintainable. The cache-busting solution is simple and effective for this project's scale.

---

📚 Alignment with CLAUDE.md Conventions

✅ Follows project architecture: Hub & spoke pattern maintained
✅ Consistent with design system: No UI changes, only logic fixes
✅ Quality standards met: Accurate, functional, maintains consistency
✅ Deployment process: Ready for GitHub Pages automatic deployment

The changes align well with the project's philosophy of "accuracy over speed" and "test locally first" - just ensure manual testing before merge!