Claude finished @Yeraze's task —— View job

PR Review: {DATE} and {TIME} Tokens for Auto Announce

🎯 Summary

This PR successfully implements {DATE} and {TIME} template tokens for the Auto Announce feature. The implementation is solid and follows established patterns in the codebase.

✅ Strengths

1. Consistent Architecture

• Follows existing token replacement patterns in replaceAnnouncementTokens() (lines 10996-11008)
• Properly integrates with global date/time format settings
• Uses established formatDate/formatTime utilities (line 12)

2. Good Test Coverage

• Comprehensive test suite in meshtasticManager.announce-datetime.test.ts
• Tests multiple format combinations (DD/MM vs MM/DD, 12h vs 24h)
• Tests fallback behavior when settings are undefined
• Tests edge cases (no tokens, combined tokens)
• Uses timezone-safe test approach with fixed time

3. UI/UX Implementation

• Clean UI with insert buttons matching existing patterns (lines 665-694 in AutoAnnounceSection.tsx)
• Live preview automatically picks up new tokens via existing preview endpoint
• Documentation properly updated in automation.md

4. Proper Scoping

• Correctly scoped to Meshtastic Auto Announce (as noted in PR scope)
• Doesn't break MeshCore's intentionally-narrow token set

🔍 Code Quality Issues

1. Type Safety Concern

// Line 10840 - meshtasticManager.ts
let result: string = typeof message === 'string' ? message : String(message);

This defensive coercion suggests the type system isn't catching potential non-string inputs. Consider strengthening the type signature of replaceAnnouncementTokens() instead of runtime type checking.

2. Inconsistent Error Handling
The date/time token replacement has no try/catch around formatDate/formatTime calls, unlike other token replacements that have error handling. Consider adding:

try {
  if (result.includes('{DATE}')) {
    result = result.replace(/{DATE}/g, encode(formatDate(now, dateFormat as 'MM/DD/YYYY' | 'DD/MM/YYYY')));
  }
  if (result.includes('{TIME}')) {
    result = result.replace(/{TIME}/g, encode(formatTime(now, timeFormat as '12' | '24')));
  }
} catch (error) {
  logger.warn('Failed to format date/time tokens:', error);
  // Could leave tokens unreplaced or use defaults
}

3. Minor Performance Optimization
The date settings are fetched even when no date/time tokens are present. Consider checking for tokens first:

if (result.includes('{DATE}') || result.includes('{TIME}')) {
  const now = new Date();
  const dateFormat = await databaseService.settings.getSetting('dateFormat') || 'MM/DD/YYYY';
  const timeFormat = await databaseService.settings.getSetting('timeFormat') || '24';
  // ... rest of implementation
}

🧪 Test Issues

1. Skipped Frontend Tests
The AutoAnnounceSection.test.tsx has comprehensive tests but they're all skipped due to architectural changes. The skip comment (lines 58-71) mentions three specific issues:

• SaveBar pattern change breaking save assertions
• Label/input mismatch for broadcast channels
• Vitest 4 deadlock with userEvent + fake timers

While there are tests for the new token buttons (lines 246-262), they can't run. This reduces confidence in the UI changes.

2. Test Coverage Gap
No integration test verifies the end-to-end flow: UI button → message change → preview update with actual date/time.

🔒 Security Assessment

No security concerns identified. The implementation:

• Doesn't expose sensitive data
• Uses existing safe formatting utilities
• Follows established token replacement patterns
• Doesn't introduce new attack vectors

🚀 Performance Assessment

Minimal performance impact:

• Date/time evaluation happens once per announcement (not per token)
• Uses existing efficient formatDate/formatTime utilities
• Could benefit from conditional evaluation optimization noted above

📝 Documentation

Well documented:

• Clear documentation added to automation.md (lines 587-588)
• Follows existing token documentation format
• Examples show proper usage

🎯 Recommendations

1. Fix Type Safety: Strengthen parameter typing instead of runtime coercion
2. Add Error Handling: Protect date/time formatting with try/catch
3. Performance: Only fetch date settings when tokens are present
4. Tests: Address the skipped frontend test suite as follow-up work
5. Integration: Add one integration test for the full flow

📊 Overall Assessment

Approval Recommended ✅

This is a solid, well-implemented feature that follows established patterns. The functionality works correctly and the test coverage for the core logic is excellent. The identified issues are minor improvements rather than blocking problems.

The implementation correctly addresses issue #3382 by providing current date/time tokens that respect user formatting preferences, exactly as requested.