refactor: consolidate formatter definitions (Phase 04)#819
Merged
reachrazamair merged 6 commits intoRunMaestro:rcfrom Apr 16, 2026
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR consolidates duplicated formatting functions scattered across components into a centralized Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
jSydorowicz21
force-pushed
the
dedup/phase-04-formatter-consolidation
branch
from
fa6c031 to
41ddab7
Compare
Greptile SummaryConsolidates 43 duplicate formatter definitions across 51 files into canonical functions in
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[src/shared/formatters.ts\nCanonical source] --> B[formatDurationHuman\nformatDurationCompact\nformatDurationVerbose\nformatDurationParts\nformatDurationDecimal]
A --> C[formatTimestamp\nstyle: time / datetime / smart / full]
A --> D[estimateTokensFromLogs]
A --> E[formatNumber\nnum.toString for <1000\nuppercase K suffix]
A --> F[formatElapsedTime\nalready existed]
B --> G[UsageDashboard components\n10 files]
B --> H[CueModal / SymphonyModal\nToast / FirstRunCelebration]
B --> I[cli/output/formatter.ts\nformatDurationDecimal alias]
B --> J[web/mobile CuePanel\nuseContributorStats]
C --> K[LongestAutoRunsTable\nHistoryEntryItem\nGroupChatHistoryPanel\n+ 8 more]
D --> L[tokenCounter.ts\ndocumentStats.ts delegate]
E --> M[AgentSessionsBrowser\nUsageDashboard charts]
F --> N[MergeProgressModal\nTransferProgressModal\nSummarizeProgressModal\n+ overlays]
O[main/utils/stripAnsi.ts] -->|re-exports| P[shared/stringUtils\nstripAnsiCodes]
Q[main/stats/utils.ts] -->|keeps domain-specific\ngenerateId format| R[Stats DB\nprimary/foreign keys]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| minute: '2-digit', | ||
| }); | ||
| } | ||
| const formatTime = (timestamp: number) => formatTimestamp(timestamp, 'time'); |
There was a problem hiding this comment.
hour: '2-digit' vs hour: 'numeric' changes time display
The removed formatTime used hour: 'numeric' (no leading zero, e.g., "2:30 PM"), while formatTimestamp(timestamp, 'time') calls toLocaleTimeString([], { hour: '2-digit', minute: '2-digit' }) — which emits "02:30 PM" in many en-US environments. The visual change is minor, but it differs from the old output and from how formatDate (still using hour: 'numeric') renders the same column. Worth double-checking the table renders as intended.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/shared/formatters.ts (1)
43-47:⚠️ Potential issue | 🟡 MinorUpdate
formatNumberJSDoc examples to match output.The docs still show lowercase
k, while the function now returns uppercaseK.Also applies to: 49-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/formatters.ts` around lines 43 - 47, The JSDoc for formatNumber still shows lowercase "k" while the function returns uppercase "K"; update the examples in the JSDoc for the formatNumber function (and any adjacent doc comments that reference the output) to use "K" instead of "k" (e.g., "1.5K", "2.3M", "1.0B") so the documentation matches the actual output of formatNumber.
🧹 Nitpick comments (3)
src/__tests__/renderer/components/FilePreview/filePreviewUtils.test.ts (1)
123-144: Add a regression test for negative byte handling.Since
formatFileSizenow delegates to sharedformatSize, it’s worth pinning the localbytes <= 0 => '0 B'behavior with an explicit negative-value test.✅ Suggested test addition
describe('formatFileSize', () => { it('formats 0 bytes', () => { expect(formatFileSize(0)).toBe('0 B'); }); + + it('clamps negative bytes to 0 B', () => { + expect(formatFileSize(-1)).toBe('0 B'); + }); it('formats bytes', () => { expect(formatFileSize(512)).toBe('512 B'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/renderer/components/FilePreview/filePreviewUtils.test.ts` around lines 123 - 144, Add a regression test that verifies formatFileSize returns '0 B' for negative inputs; inside the describe('formatFileSize') block add a new it('formats negative bytes', ...) that calls formatFileSize(-1) (and optionally another negative value) and expects '0 B' to ensure the local bytes <= 0 behavior is preserved now that formatFileSize delegates to formatSize.src/renderer/components/SendToAgentModal.tsx (1)
24-25: Optional cleanup: remove the extra alias indirection.You can alias directly in the import and drop the standalone constant.
♻️ Proposed simplification
-import { estimateTokensFromLogs } from '../../shared/formatters'; +import { estimateTokensFromLogs as estimateTokens } from '../../shared/formatters'; @@ -const estimateTokens = estimateTokensFromLogs;Also applies to: 111-111
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/components/SendToAgentModal.tsx` around lines 24 - 25, The import currently creates an extra alias indirection by importing getAgentIcon then assigning it to a standalone constant; remove that intermediate constant and alias directly in the import (e.g., import { getAgentIcon as agentIcon } from '...') and update usages (e.g., any references to the standalone constant) to use the direct import name (reference symbols: getAgentIcon, estimateTokensFromLogs, SendToAgentModal component) so the extra indirection is eliminated.src/renderer/utils/tabExport.ts (1)
46-52: Use shared zero-duration output for the short-log fallback.Line 47 still returns a hardcoded
'0m', which can diverge from the shared compact formatter output style.♻️ Suggested consistency tweak
function formatDuration(logs: LogEntry[]): string { - if (logs.length < 2) return '0m'; + if (logs.length < 2) return formatDurationCompact(0); const firstTimestamp = logs[0].timestamp; const lastTimestamp = logs[logs.length - 1].timestamp; return formatDurationCompact(lastTimestamp - firstTimestamp); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/utils/tabExport.ts` around lines 46 - 52, The formatDuration function currently returns a hardcoded '0m' for short logs which can diverge from the shared compact formatter; change it to return the shared zero-duration output by calling formatDurationCompact(0) instead of '0m' so formatDuration uses the same formatting logic as formatDurationCompact (update within the formatDuration function referencing logs, firstTimestamp, lastTimestamp and formatDurationCompact).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/shared/formatters.ts`:
- Around line 384-387: The token estimate in estimateTokensFromLogs currently
uses Math.round which can undercount; change the calculation to use Math.ceil
for the division (i.e., replace Math.round(totalChars / 4) with
Math.ceil(totalChars / 4)) so non-empty logs never yield 0 and to align behavior
with estimateTokenCount; update the return expression in estimateTokensFromLogs
to use Math.ceil and keep the same totalChars computation.
---
Outside diff comments:
In `@src/shared/formatters.ts`:
- Around line 43-47: The JSDoc for formatNumber still shows lowercase "k" while
the function returns uppercase "K"; update the examples in the JSDoc for the
formatNumber function (and any adjacent doc comments that reference the output)
to use "K" instead of "k" (e.g., "1.5K", "2.3M", "1.0B") so the documentation
matches the actual output of formatNumber.
---
Nitpick comments:
In `@src/__tests__/renderer/components/FilePreview/filePreviewUtils.test.ts`:
- Around line 123-144: Add a regression test that verifies formatFileSize
returns '0 B' for negative inputs; inside the describe('formatFileSize') block
add a new it('formats negative bytes', ...) that calls formatFileSize(-1) (and
optionally another negative value) and expects '0 B' to ensure the local bytes
<= 0 behavior is preserved now that formatFileSize delegates to formatSize.
In `@src/renderer/components/SendToAgentModal.tsx`:
- Around line 24-25: The import currently creates an extra alias indirection by
importing getAgentIcon then assigning it to a standalone constant; remove that
intermediate constant and alias directly in the import (e.g., import {
getAgentIcon as agentIcon } from '...') and update usages (e.g., any references
to the standalone constant) to use the direct import name (reference symbols:
getAgentIcon, estimateTokensFromLogs, SendToAgentModal component) so the extra
indirection is eliminated.
In `@src/renderer/utils/tabExport.ts`:
- Around line 46-52: The formatDuration function currently returns a hardcoded
'0m' for short logs which can diverge from the shared compact formatter; change
it to return the shared zero-duration output by calling formatDurationCompact(0)
instead of '0m' so formatDuration uses the same formatting logic as
formatDurationCompact (update within the formatDuration function referencing
logs, firstTimestamp, lastTimestamp and formatDurationCompact).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 143e3a97-409f-420e-a8a8-777c680d78b9
📒 Files selected for processing (52)
src/__tests__/renderer/components/AgentSessionsBrowser.test.tsxsrc/__tests__/renderer/components/FilePreview/filePreviewUtils.test.tssrc/__tests__/renderer/components/TransferProgressModal.test.tsxsrc/__tests__/shared/formatters.test.tssrc/cli/output/formatter.tssrc/main/stats/utils.tssrc/main/utils/stripAnsi.tssrc/renderer/components/AboutModal.tsxsrc/renderer/components/CueModal/cueModalUtils.tssrc/renderer/components/FilePreview/filePreviewUtils.tssrc/renderer/components/FirstRunCelebration.tsxsrc/renderer/components/GroupChatHistoryPanel.tsxsrc/renderer/components/GroupChatMessages.tsxsrc/renderer/components/History/HistoryEntryItem.tsxsrc/renderer/components/HistoryDetailModal.tsxsrc/renderer/components/InlineWizard/WizardMessageBubble.tsxsrc/renderer/components/MergeProgressModal.tsxsrc/renderer/components/MergeProgressOverlay.tsxsrc/renderer/components/MergeSessionModal.tsxsrc/renderer/components/ParticipantCard.tsxsrc/renderer/components/SendToAgentModal.tsxsrc/renderer/components/SummarizeProgressModal.tsxsrc/renderer/components/SummarizeProgressOverlay.tsxsrc/renderer/components/SymphonyModal.tsxsrc/renderer/components/Toast.tsxsrc/renderer/components/TransferProgressModal.tsxsrc/renderer/components/UsageDashboard/ActivityHeatmap.tsxsrc/renderer/components/UsageDashboard/AgentComparisonChart.tsxsrc/renderer/components/UsageDashboard/AgentEfficiencyChart.tsxsrc/renderer/components/UsageDashboard/AgentUsageChart.tsxsrc/renderer/components/UsageDashboard/AutoRunStats.tsxsrc/renderer/components/UsageDashboard/DurationTrendsChart.tsxsrc/renderer/components/UsageDashboard/LocationDistributionChart.tsxsrc/renderer/components/UsageDashboard/LongestAutoRunsTable.tsxsrc/renderer/components/UsageDashboard/PeakHoursChart.tsxsrc/renderer/components/UsageDashboard/SourceDistributionChart.tsxsrc/renderer/components/UsageDashboard/SummaryCards.tsxsrc/renderer/components/UsageDashboard/WeekdayComparisonChart.tsxsrc/renderer/components/Wizard/screens/ConversationScreen.tsxsrc/renderer/hooks/agent/useMergeSession.tssrc/renderer/hooks/agent/useSendToAgent.tssrc/renderer/hooks/symphony/useContributorStats.tssrc/renderer/utils/documentStats.tssrc/renderer/utils/groupChatExport.tssrc/renderer/utils/tabExport.tssrc/renderer/utils/tokenCounter.tssrc/shared/formatters.tssrc/web/mobile/CuePanel.tsxsrc/web/mobile/GroupChatPanel.tsxsrc/web/mobile/MessageHistory.tsxsrc/web/mobile/MobileHistoryPanel.tsxsrc/web/mobile/ResponseViewer.tsx
Contributor
AI Code Trust — ship readinessScore: 83 · Verdict: RISKY · Model: deterministic-v1 Notable issues found; review before shipping. Top issues:
|
Contributor
Author
Hey Neha, a good bit of this feedback is out of scope for this pr and a bit off topic :p |
jSydorowicz21
added
refactor
Contributor
|
Thanks for the feedback that makes sense. I understand that some of my comments were broader than the scope of this PR. My intention was to point out a few improvements I noticed, but I agree they should be tracked separately if they are not directly related to the change here. I’ll keep future review comments more PR-scoped and focused only on the current change. @jSydorowicz21 thanks again your feedback actually helped me identify a useful improvement feature to implement in my product. :) |
Phase 04A - formatDuration: - Added 6 new canonical variants to shared/formatters.ts: formatDurationHuman, formatDurationCompact, formatDurationVerbose, formatDurationParts, formatDurationDecimal, estimateTokensFromLogs, formatTimestamp - Replaced 20 local formatDuration definitions across UsageDashboard (10), AboutModal, FirstRunCelebration, Toast, CueModal, CuePanel, SymphonyModal, useContributorStats, cli/formatter, tabExport, groupChatExport - Updated canonical formatNumber to use uppercase K and toString() for <1000 Phase 04B - formatElapsedTime: - Replaced 5 local formatElapsedTime definitions in MergeProgressModal, MergeProgressOverlay, SummarizeProgressModal, SummarizeProgressOverlay, TransferProgressModal with import from shared/formatters Phase 04C - formatTimestamp: - Added canonical formatTimestamp(timestamp, style) with 4 styles: time, datetime, smart (default), full - Replaced 13 local formatTime/formatTimestamp definitions across HistoryEntryItem, GroupChatHistoryPanel, MobileHistoryPanel, MessageHistory, GroupChatPanel (smart); WizardMessageBubble, ConversationScreen, LongestAutoRunsTable (time); HistoryDetailModal, ResponseViewer (datetime); tabExport, groupChatExport (full) - GroupChatMessages and ParticipantCard refactored to delegate string formatting to shared formatTimestamp - ThinkingStatusPill skipped (duration formatter, not timestamp) Phase 04D - formatNumber, formatFileSize, estimateTokens, stripAnsi, generateId: - Replaced 5 local formatNumber in UsageDashboard components - Replaced 2 formatFileSize (documentStats, filePreviewUtils) with thin wrappers around shared formatSize - Replaced 4 estimateTokens/estimateTokensFromLogs (MergeSessionModal, SendToAgentModal, useSendToAgent, useMergeSession); tokenCounter.ts now delegates to shared estimateTokenCount - Replaced main/utils/stripAnsi.ts with re-export from shared/stringUtils - Replaced main/stats/utils.ts generateId with re-export from shared/uuid - Skipped symphony.ts formatNumber (different function: toLocaleString) - Skipped useLayerStack/useBatchedSessionUpdates/useCommandHistory/ useOfflineQueue generateId (domain-specific ID formats with prefixes) - Skipped contextExtractor.ts estimateTokenCount (different signature)
main/stats/utils.ts generateId was incorrectly replaced with generateUUID in Phase 04. The stats DB depends on the timestamp-random format (<timestamp>-<random>) as a load-bearing invariant - primary keys, foreign keys, and backward compatibility with existing persisted stats rely on it. Restore the inline timestamp-random generator with a comment warning against future replacement.
…tNumber The shared formatNumber in src/shared/formatters.ts was updated in this phase to use uppercase K and omit the decimal for small numbers (matching dashboard convention). AgentSessionsBrowser.test.tsx still used the old '8.0k' / '700.0' expectations. Update three assertions to match the new canonical output: - '700.0' -> '700' (no decimal for sub-thousand) - '8.0k' -> '8.0K' (uppercase suffix) This is the intended UI consistency improvement - users will now see 8.0K everywhere instead of mixed k/K across components.
The migration from .substring(2, 11) to .slice(2, 10) silently shortened the random segment from 9 to 8 characters. This is a load-bearing invariant for stats DB primary/foreign keys.
- estimateTokensFromLogs now uses Math.ceil consistent with estimateTokenCount - formatTimestamp uses hour: 'numeric' to match previous formatTime behavior
jSydorowicz21
force-pushed
the
dedup/phase-04-formatter-consolidation
branch
from
a46d031 to
b24fe64
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Consolidates 43 duplicate formatter definitions across the codebase into canonical versions in
src/shared/formatters.ts(uses existing file, does NOT create a new one).Net: -422 lines across 51 files
04A - formatDuration (20 replaced)
Added 6 new canonical functions:
formatDurationHuman,formatDurationCompact,formatDurationVerbose,formatDurationParts,formatDurationDecimal,formatTimestamp. Replaced local definitions in 10 UsageDashboard files, AboutModal, FirstRunCelebration, Toast, CueModal, mobile CuePanel, SymphonyModal, useContributorStats, cli/output, tabExport, groupChatExport.04B - formatElapsedTime (5 replaced)
Replaced in: MergeProgressModal, MergeProgressOverlay, SummarizeProgressModal, SummarizeProgressOverlay, TransferProgressModal.
04C - formatTimestamp (13 replaced)
Canonical
formatTimestamp(timestamp, style)with 4 styles (time, datetime, smart, full). Replaced in HistoryEntryItem, GroupChatHistoryPanel, MobileHistoryPanel, MessageHistory, GroupChatPanel, WizardMessageBubble, ConversationScreen, LongestAutoRunsTable, HistoryDetailModal, ResponseViewer, tabExport, groupChatExport.04D - formatNumber / formatFileSize / estimateTokens / stripAnsi / generateId
formatNumber: 5 replaced in UsageDashboardformatFileSize: 2 replaced with thin wrappers aroundformatSizeestimateTokens: 4 array-based defs replaced;tokenCounter.tsdelegatesstripAnsi:main/utils/stripAnsi.tsre-exports fromshared/stringUtilsgenerateId:main/stats/utils.tsre-exports fromshared/uuid(skipped 4 domain-specific uses)Test plan
npm run lintpasses cleanRisk
Low to medium - formatters touch production code across main + renderer + cli + web. All canonical functions verified as strict supersets of the replaced versions. Note: canonical
formatNumberuses uppercase K andtoString()for small numbers (matching dashboard convention).Summary by CodeRabbit
Release Notes