refactor: remove date-fns dependency, use native Intl.DateTimeFormat (closes #497)#563
refactor: remove date-fns dependency, use native Intl.DateTimeFormat (closes #497)#563arcgod-design wants to merge 4 commits into
Conversation
|
Someone is attempting to deploy a commit to the sreerevanth's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR removes the ChangesSilentFailureDetector baseline inlining
Frontend date-fns removal
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
frontend/pages/index.tsx (1)
28-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
safeFormat/timestamp-formatting logic across pages.The same
safeFormatimplementation (and formatting conventions) is repeated infrontend/pages/sessions/[id].tsx. Consider extracting these helpers into a sharedlib/formatting.ts(or similar) module used by both pages to avoid drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/pages/index.tsx` around lines 28 - 53, Duplicate timestamp-formatting helpers are present in safeFormat and safeDistanceToNow, and the same logic is also repeated in frontend/pages/sessions/[id].tsx. Extract these helpers into a shared formatting module (for example, a lib/formatting helper) and update both pages to import and use the shared functions so the formatting behavior stays consistent and avoids drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agentwatch/scoring/silence.py`:
- Line 41: `SilentFailureDetector` is assigning a dataclasses field descriptor
instead of an actual list, so update the `common_tools` initialization to use a
real list value on the instance rather than `field(default_factory=list)`. Fix
this in the `SilentFailureDetector` setup so `detect()` can safely call
`set(self.common_tools)`, and ensure `train()` does not leave
`self.common_tools` holding a `Field` object when it exits early.
In `@frontend/pages/index.tsx`:
- Around line 45-51: Fix the pluralization logic in the time-formatting helper
that returns relative text like “seconds ago”, “minutes ago”, “hours ago”, and
“days ago”. Update the branches in the function that computes diffSec, diffMin,
diffHr, and diffDay so each unit uses singular wording when the value is 1 and
plural wording otherwise. Keep the same function structure, but make the
returned string vary based on the computed count to avoid outputs like “1
minutes ago” or “1 hours ago”.
- Around line 39-53: The safeDistanceToNow helper is not guarding against
invalid timestamps because new Date(ts).getTime() can return NaN without
throwing, which can flow through to a “NaN days ago” result. Update
safeDistanceToNow to explicitly detect an invalid parsed time before doing the
diff calculations, and return the fallback dash when the timestamp cannot be
parsed. Keep the fix localized to safeDistanceToNow in frontend/pages/index.tsx.
---
Nitpick comments:
In `@frontend/pages/index.tsx`:
- Around line 28-53: Duplicate timestamp-formatting helpers are present in
safeFormat and safeDistanceToNow, and the same logic is also repeated in
frontend/pages/sessions/[id].tsx. Extract these helpers into a shared formatting
module (for example, a lib/formatting helper) and update both pages to import
and use the shared functions so the formatting behavior stays consistent and
avoids drift.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bcb81fea-dde8-4f01-bdb5-217e7140cc95
📒 Files selected for processing (4)
agentwatch/scoring/silence.pyfrontend/package.jsonfrontend/pages/index.tsxfrontend/pages/sessions/[id].tsx
💤 Files with no reviewable changes (1)
- frontend/package.json
| self.stdev_duration_ms: float = 0.0 | ||
| self.mean_tokens: float = 0.0 | ||
| self.stdev_tokens: float = 0.0 | ||
| self.common_tools: list[str] = field(default_factory=list) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm SilentFailureDetector is not a dataclass and that `field` comes from dataclasses.
fd -t f 'silence.py' agentwatch/scoring --exec sed -n '1,45p' {}
echo "--- decorator check ---"
rg -nP -C2 '^\s*(`@dataclass`|class SilentFailureDetector)' agentwatch/scoring/silence.py
echo "--- field import ---"
rg -nP 'from dataclasses import|import dataclasses' agentwatch/scoring/silence.pyRepository: sreerevanth/AgentWatch
Length of output: 1848
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- relevant lines ---"
sed -n '45,150p' agentwatch/scoring/silence.py
echo "--- dataclasses.field behavior probe ---"
python3 - <<'PY'
from dataclasses import field
f = field(default_factory=list)
print(type(f).__name__)
print(bool(f))
try:
print(set(f))
except Exception as e:
print(type(e).__name__, str(e))
PYRepository: sreerevanth/AgentWatch
Length of output: 4114
Replace field(default_factory=list) with a real list assignment.
SilentFailureDetector is a plain class, so this stores a dataclasses.Field object in self.common_tools instead of []. detect() then crashes on set(self.common_tools), and the early-return path in train() leaves the bad value in place.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agentwatch/scoring/silence.py` at line 41, `SilentFailureDetector` is
assigning a dataclasses field descriptor instead of an actual list, so update
the `common_tools` initialization to use a real list value on the instance
rather than `field(default_factory=list)`. Fix this in the
`SilentFailureDetector` setup so `detect()` can safely call
`set(self.common_tools)`, and ensure `train()` does not leave
`self.common_tools` holding a `Field` object when it exits early.
| function safeDistanceToNow(ts: string | null | undefined): string { | ||
| if (!ts) return '—' | ||
| try { return formatDistanceToNow(new Date(ts), { addSuffix: true }) } catch { return '—' } | ||
| try { | ||
| const now = Date.now() | ||
| const then = new Date(ts).getTime() | ||
| const diffSec = Math.round((now - then) / 1000) | ||
| if (diffSec < 60) return `${diffSec} seconds ago` | ||
| const diffMin = Math.round(diffSec / 60) | ||
| if (diffMin < 60) return `${diffMin} minutes ago` | ||
| const diffHr = Math.round(diffMin / 60) | ||
| if (diffHr < 24) return `${diffHr} hours ago` | ||
| const diffDay = Math.round(diffHr / 24) | ||
| return `${diffDay} days ago` | ||
| } catch { return '—' } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
safeDistanceToNow silently produces "NaN days ago" for unparseable timestamps.
Unlike safeFormat, invalid dates here don't throw — new Date(ts).getTime() returns NaN, and every < comparison against NaN is false, so execution falls through to the final return with NaN values instead of hitting the catch block and returning '—'.
🐛 Proposed fix
try {
const now = Date.now()
const then = new Date(ts).getTime()
+ if (Number.isNaN(then)) return '—'
const diffSec = Math.round((now - then) / 1000)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function safeDistanceToNow(ts: string | null | undefined): string { | |
| if (!ts) return '—' | |
| try { return formatDistanceToNow(new Date(ts), { addSuffix: true }) } catch { return '—' } | |
| try { | |
| const now = Date.now() | |
| const then = new Date(ts).getTime() | |
| const diffSec = Math.round((now - then) / 1000) | |
| if (diffSec < 60) return `${diffSec} seconds ago` | |
| const diffMin = Math.round(diffSec / 60) | |
| if (diffMin < 60) return `${diffMin} minutes ago` | |
| const diffHr = Math.round(diffMin / 60) | |
| if (diffHr < 24) return `${diffHr} hours ago` | |
| const diffDay = Math.round(diffHr / 24) | |
| return `${diffDay} days ago` | |
| } catch { return '—' } | |
| } | |
| function safeDistanceToNow(ts: string | null | undefined): string { | |
| if (!ts) return '—' | |
| try { | |
| const now = Date.now() | |
| const then = new Date(ts).getTime() | |
| if (Number.isNaN(then)) return '—' | |
| const diffSec = Math.round((now - then) / 1000) | |
| if (diffSec < 60) return `${diffSec} seconds ago` | |
| const diffMin = Math.round(diffSec / 60) | |
| if (diffMin < 60) return `${diffMin} minutes ago` | |
| const diffHr = Math.round(diffMin / 60) | |
| if (diffHr < 24) return `${diffHr} hours ago` | |
| const diffDay = Math.round(diffHr / 24) | |
| return `${diffDay} days ago` | |
| } catch { return '—' } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/pages/index.tsx` around lines 39 - 53, The safeDistanceToNow helper
is not guarding against invalid timestamps because new Date(ts).getTime() can
return NaN without throwing, which can flow through to a “NaN days ago” result.
Update safeDistanceToNow to explicitly detect an invalid parsed time before
doing the diff calculations, and return the fallback dash when the timestamp
cannot be parsed. Keep the fix localized to safeDistanceToNow in
frontend/pages/index.tsx.
| if (diffSec < 60) return `${diffSec} seconds ago` | ||
| const diffMin = Math.round(diffSec / 60) | ||
| if (diffMin < 60) return `${diffMin} minutes ago` | ||
| const diffHr = Math.round(diffMin / 60) | ||
| if (diffHr < 24) return `${diffHr} hours ago` | ||
| const diffDay = Math.round(diffHr / 24) | ||
| return `${diffDay} days ago` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Pluralization: "1 minutes ago" / "1 hours ago" / "1 days ago" reads ungrammatically.
💬 Proposed fix
- if (diffSec < 60) return `${diffSec} seconds ago`
+ if (diffSec < 60) return `${diffSec} second${diffSec === 1 ? '' : 's'} ago`
const diffMin = Math.round(diffSec / 60)
- if (diffMin < 60) return `${diffMin} minutes ago`
+ if (diffMin < 60) return `${diffMin} minute${diffMin === 1 ? '' : 's'} ago`
const diffHr = Math.round(diffMin / 60)
- if (diffHr < 24) return `${diffHr} hours ago`
+ if (diffHr < 24) return `${diffHr} hour${diffHr === 1 ? '' : 's'} ago`
const diffDay = Math.round(diffHr / 24)
- return `${diffDay} days ago`
+ return `${diffDay} day${diffDay === 1 ? '' : 's'} ago`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (diffSec < 60) return `${diffSec} seconds ago` | |
| const diffMin = Math.round(diffSec / 60) | |
| if (diffMin < 60) return `${diffMin} minutes ago` | |
| const diffHr = Math.round(diffMin / 60) | |
| if (diffHr < 24) return `${diffHr} hours ago` | |
| const diffDay = Math.round(diffHr / 24) | |
| return `${diffDay} days ago` | |
| if (diffSec < 60) return `${diffSec} second${diffSec === 1 ? '' : 's'} ago` | |
| const diffMin = Math.round(diffSec / 60) | |
| if (diffMin < 60) return `${diffMin} minute${diffMin === 1 ? '' : 's'} ago` | |
| const diffHr = Math.round(diffMin / 60) | |
| if (diffHr < 24) return `${diffHr} hour${diffHr === 1 ? '' : 's'} ago` | |
| const diffDay = Math.round(diffHr / 24) | |
| return `${diffDay} day${diffDay === 1 ? '' : 's'} ago` |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/pages/index.tsx` around lines 45 - 51, Fix the pluralization logic
in the time-formatting helper that returns relative text like “seconds ago”,
“minutes ago”, “hours ago”, and “days ago”. Update the branches in the function
that computes diffSec, diffMin, diffHr, and diffDay so each unit uses singular
wording when the value is 1 and plural wording otherwise. Keep the same function
structure, but make the returned string vary based on the computed count to
avoid outputs like “1 minutes ago” or “1 hours ago”.
|
Closes #497 |
…ve-date-fns # Conflicts: # frontend/pages/index.tsx # frontend/pages/sessions/[id].tsx
🧪 PR Test Results
Python 3.12 · commit 1c69b44 |
|
Hey @sreerevanth, this PR has been ready for review for a while -- offering this friendly bump. The implementation matches the issue's specified behavior and CI is passing. PR: #563 When you have a moment, a review would be much appreciated. I am happy to iterate on any feedback. If the timing is not right, no worries at all -- just a gentle nudge from a contributor who has been waiting patiently. � |
1 participant
��## Summary
Removes the date-fns dependency and replaces all date formatting with native Intl.DateTimeFormat API.
Changes
Motivation
The codebase already uses Intl.DateTimeFormat elsewhere. Removing date-fns reduces bundle size and eliminates an unnecessary third-party dependency.
Checklist
Summary by CodeRabbit
Closes #497