Fix recently updated list to ignore never-updated tools

[simonw]

Summary

• ensure the recent updates list only includes tools with commits beyond their initial addition
• reuse the existing selection helper against the filtered set to keep recently added tools from appearing twice

Testing

• python3 -m compileall build_index.py

---

https://chatgpt.com/codex/tasks/task_e_68f4537195608326ab6e2b8ac75f816a

[claude]

Claude finished @simonw's task —— View job

---

Code Review Complete ✅

Todo List:

• Read and analyze the modified build_index.py file
• Review code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Assess security concerns
• Review test coverage
• Provide comprehensive feedback

Review Summary

This PR introduces a clean and logical improvement to the recent updates display logic. The changes are well-implemented with good separation of concerns.

Code Quality & Best Practices ✅

Positive aspects:

• Clean function design: The new _has_distinct_update() function follows the existing code style perfectly
• Clear naming: Function and variable names are descriptive and follow Python conventions
• Proper documentation: Includes a clear docstring explaining the function's purpose
• Consistent error handling: Handles None values gracefully, consistent with existing _parse_iso_datetime() usage
• Type hints: Missing but consistent with the rest of the file (which doesn't use comprehensive type hints)

Suggestions:

• Consider adding type hints to the new function: def _has_distinct_update(tool: dict) -> bool:
• The logic at build_index.py:52 could benefit from a comment explaining why None created date returns True

Potential Issues & Bugs ✅

No significant issues found. The logic is sound:

1. Edge case handling: Properly handles None values for both updated and created timestamps
2. Comparison logic: The updated > created comparison correctly identifies tools with meaningful updates
3. Integration: The filtering is applied before the existing _select_recent() helper, maintaining the original exclusion logic

Performance Considerations ✅

Minor performance impact, but acceptable:

• Additional list comprehension: tools_with_updates = [tool for tool in tools if _has_distinct_update(tool)] adds O(n) filtering
• DateTime parsing overhead: Each tool gets its timestamps parsed twice (once for filtering, once in _select_recent)

Optimization opportunity (optional):

# Could cache parsed dates to avoid double parsing
tools_with_parsed_dates = [
    (tool, _parse_iso_datetime(tool.get("created")), _parse_iso_datetime(tool.get("updated")))
    for tool in tools
]
tools_with_updates = [
    tool for tool, created, updated in tools_with_parsed_dates 
    if updated and (not created or updated > created)
]

Security Concerns ✅

No security issues identified. The changes:

• Don't introduce new user input handling
• Don't modify file I/O operations
• Don't change HTML generation logic
• Only add internal data filtering logic

Test Coverage 📝

Testing considerations:

• The provided test (python3 -m compileall build_index.py) only validates syntax
• Recommended additional tests:

  # Test cases for _has_distinct_update()
  assert _has_distinct_update({"updated": "2023-12-01T10:00:00Z", "created": "2023-11-01T10:00:00Z"}) == True
  assert _has_distinct_update({"updated": "2023-11-01T10:00:00Z", "created": "2023-11-01T10:00:00Z"}) == False
  assert _has_distinct_update({"updated": None, "created": "2023-11-01T10:00:00Z"}) == False
  assert _has_distinct_update({"updated": "2023-12-01T10:00:00Z", "created": None}) == True

Overall Assessment

This is a well-executed fix that addresses the core issue cleanly. The new _has_distinct_update() function is a good abstraction that could be useful elsewhere. The implementation correctly handles edge cases and integrates seamlessly with existing code.

Recommendation: Approve ✅