fix(ui): format decimal values in memory diagnostics panel - #2201
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughMemory settings panels now share locale-aware decimal and day formatting. Diagnostics latency preserves em-dash fallbacks and displays non-finite values unchanged. Tests verify ChangesMemory formatting
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized formatting change leaves no actionable merge-blocking risk; it is merge-ready after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
zerob13
left a comment
There was a problem hiding this comment.
Review — format decimal values in memory diagnostics panel
Thanks for the fix. Using Intl.NumberFormat for locale-aware formatting is the right direction and consistent with the sibling MemoryLifecyclePanel.vue.
🔴 Must fix
1. Duplicated formatters with MemoryLifecyclePanel.vue
The newly added decimalFormatter, dayFormatter computeds and formatScore / formatDays functions are byte-for-byte identical to the ones already in MemoryLifecyclePanel.vue (L163–176, L257–269):
| definition | MemoryLifecyclePanel.vue | MemoryDiagnosticsPanel.vue (this PR) |
|---|---|---|
decimalFormatter |
L163–168 | L345–351 |
dayFormatter |
L170–175 | L352–358 |
formatScore |
L257–259 | L489–491 |
formatDays |
L267–269 | L493–495 |
Both components live in the same directory and operate on the same memory lifecycle/diagnostics domain. Please extract a shared composable (e.g. useMemoryNumberFormatters) into settings/lib/ — useMemoryInlineFeedback.ts there is a good precedent — and have both panels consume it. Otherwise the two copies will silently drift as formatting requirements evolve.
🟡 Suggestions (nits)
2. formatOptionalMilliseconds is misnamed
The function delegates to decimalFormatter (a generic decimal formatter), so "Milliseconds" in the name is misleading — nothing about it is millisecond-specific. The sibling component calls its nullable variant formatOptionalScore. Consider a domain-agnostic name like formatOptionalDecimal, or align with the sibling's naming once the shared composable lands.
3. Dropped Number.isFinite guard
The previous formatNumber guarded against NaN / Infinity and fell back to String(value). The new helpers pass straight to Intl.NumberFormat.format, so Infinity now renders as ∞ instead of Infinity. Realistic data is unlikely to hit this, but when extracting the shared composable, consider restoring a finite-check guard for robustness.
✅ Looks good
Intl.NumberFormatovertoFixed— correct, locale-aware, consistent withMemoryLifecyclePanel/OcrSettings/DashboardSettings.computed-cached formatter instances that rebuild onlocalechange — correct.- Test coverage for float truncation (
685.716,0.123,12.3) and the—fallback — solid, no over-testing. - Scope is tight and focused; no breaking changes.
Requested changes center on the duplication — once the shared composable is extracted and both panels consume it, this is good to go.
Avoid directly display numbers in memory diagnostics panel
Summary by CodeRabbit