fix(ui): live-refresh timeAgo timestamps in activity feed (SO-24)#4
fix(ui): live-refresh timeAgo timestamps in activity feed (SO-24)#4
Conversation
- Add GetStalledIssues(threshold) query: returns in_progress issues where MAX(last_comment.created_at, updated_at) < now - threshold - Add StalledIssue model (Issue + IdleDuration + LastCommentAt) - Add StartStallDetectionLoop(interval, threshold) in scheduler: ticks every 30 min, posts Stall Detector comment to CEO inbox (AC2), deduplicates notifications in-memory - Wire loop in main.go with configurable threshold from settings (AC3) - Migration 019: seed stall_threshold_hours=4 in settings table (AC3 default) - Dashboard: Stalled Issues amber panel, HTMX-refreshed (AC1) - Dashboard handler: reads stall_threshold_hours, passes StalledIssues - 7 new tests: NoneInProgress, TooRecent, StaleInProgress, RecentCommentPreventsStall, OldCommentCountsAsStalled, NonInProgressIgnored, StallThresholdSetting - Architecture doc: artifact-docs/architecture/SO-87-stall-detector.md All tests green (go test ./... -count=1)
- Add `isoTime` helper to templates.go funcMap as "iso" alongside timeAgo
- Update activity_entry span: add class="time-ago" and data-ts="{{iso .CreatedAt}}"
- Add client-side <script> block in activity.html that:
* Recomputes all .time-ago[data-ts] spans every 30 seconds via setInterval
* Also refreshes on htmx:afterSwap so SSE-injected entries are handled
* JS timeAgo() mirrors server-side logic (sec/min/hr/day thresholds)
Before: timestamps were static server-rendered strings (e.g. "2m ago")
that never updated after page load
After: timestamps auto-refresh client-side every 30s from the ISO
date stored in data-ts attribute
📝 WalkthroughWalkthroughAdds stall-detection for Changes
Sequence DiagramsequenceDiagram
participant Scheduler
participant Database
participant CommentSystem
participant InMemoryMap
loop Every interval
Scheduler->>Database: GetStalledIssues(threshold)
Database-->>Scheduler: []StalledIssue
alt For each stalled issue
Scheduler->>InMemoryMap: Check notified(issueKey)
alt Not notified
Scheduler->>Database: GetCEOAgent()
Database-->>Scheduler: CEO agent
Scheduler->>CommentSystem: CreateComment(issue, "Stall Detector" message)
CommentSystem-->>Scheduler: Comment created
Scheduler->>InMemoryMap: Mark notified(issueKey)
end
end
Scheduler->>Scheduler: Log sweep summary
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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. 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new 'Stall Detector' feature (SO-87) to automatically identify and flag in_progress issues that have had no activity for a configurable duration. The changes include adding a new database query to find stalled issues, implementing a background scheduler loop to periodically check for these issues and notify the CEO via comments, and integrating a new 'Stalled Issues' panel into the dashboard UI. The review comments highlight several areas for improvement, primarily focusing on robust error handling (e.g., for database operations and comment creation) where errors are currently silently ignored. Additionally, there are opportunities to refactor duplicated code for parsing the stall_threshold_hours setting and time strings into shared helper functions to improve maintainability and consistency. Finally, the IdleHours() method in StalledIssue is noted as redundant and should be removed.
|
|
||
| // Query the actual last comment time separately (for display/notification). | ||
| var lastCommentStr sql.NullString | ||
| _ = d.QueryRow(`SELECT MAX(created_at) FROM comments WHERE issue_key=?`, i.Key).Scan(&lastCommentStr) |
There was a problem hiding this comment.
The error returned by QueryRow(...).Scan(...) is ignored. If an error other than sql.ErrNoRows occurs (e.g., a database connection issue), it will be silently swallowed. This could lead to incorrect behavior, such as LastCommentAt not being set when it should be. The error should be checked and handled.
| idleHours, thresholdHours, | ||
| si.Issue.Status, | ||
| ) | ||
| _ = s.db.CreateComment(&models.Comment{ |
There was a problem hiding this comment.
The error from s.db.CreateComment is ignored. If creating the comment fails, the stall notification will not be delivered to the CEO, and this failure will be silent. This error should be logged.
| if val, err := database.GetSetting("stall_threshold_hours"); err == nil { | ||
| var h float64 | ||
| if _, err2 := fmt.Sscanf(val, "%f", &h); err2 == nil && h > 0 { | ||
| thresholdHours = h | ||
| } | ||
| } |
There was a problem hiding this comment.
This logic for parsing the stall_threshold_hours setting is duplicated in internal/handlers/ui.go (lines 74-78). To improve maintainability and avoid inconsistencies, consider creating a shared helper function that encapsulates fetching, parsing, and applying the default for this setting.
Additionally, strconv.ParseFloat is generally preferred over fmt.Sscanf for parsing a single float value from a string as it's more explicit and type-safe.
| for _, layout := range []string{ | ||
| "2006-01-02 15:04:05.999999999-07:00", | ||
| "2006-01-02T15:04:05.999999999Z07:00", | ||
| "2006-01-02 15:04:05.999999999", | ||
| "2006-01-02 15:04:05", | ||
| "2006-01-02T15:04:05Z", | ||
| } { | ||
| if t, err2 := time.Parse(layout, lastActivityStr); err2 == nil { | ||
| lastActivity = t.UTC() | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
This block of code for parsing a time string is duplicated on lines 1702-1714. To improve maintainability, this logic should be extracted into a helper function.
Additionally, the internal/db/time.go file already defines timeFmts for this purpose. It would be best to consolidate the time parsing logic to use a single, consistent set of layouts.
| if val, err := u.db.GetSetting("stall_threshold_hours"); err == nil { | ||
| var h float64 | ||
| if _, err2 := fmt.Sscanf(val, "%f", &h); err2 == nil && h > 0 { | ||
| stallThreshold = time.Duration(h * float64(time.Hour)) | ||
| } |
There was a problem hiding this comment.
This logic for parsing the stall_threshold_hours setting is duplicated in cmd/secondorder/main.go (lines 319-324). To improve maintainability and avoid inconsistencies, consider creating a shared helper function that encapsulates fetching, parsing, and applying the default for this setting.
Additionally, strconv.ParseFloat is generally preferred over fmt.Sscanf for parsing a single float value from a string as it's more explicit and type-safe.
| stallThreshold = time.Duration(h * float64(time.Hour)) | ||
| } | ||
| } | ||
| stalledIssues, _ := u.db.GetStalledIssues(stallThreshold) |
There was a problem hiding this comment.
The error returned from u.db.GetStalledIssues is ignored. If the database query fails, the error will be silently swallowed, and the 'Stalled Issues' panel will simply not be displayed. This can hide underlying problems. The error should be logged to aid in debugging.
| // IdleHours returns the idle duration rounded to the nearest tenth of an hour. | ||
| func (s *StalledIssue) IdleHours() float64 { | ||
| return s.IdleDuration.Hours() | ||
| } |
There was a problem hiding this comment.
The comment for IdleHours states that it 'returns the idle duration rounded to the nearest tenth of an hour', but the implementation simply calls s.IdleDuration.Hours() which does not perform any rounding.
Furthermore, this method is redundant because Go templates can directly call the Hours() method on the IdleDuration field (e.g., {{ .IdleDuration.Hours }}), as is already done in internal/templates/dashboard.html. This method can be removed.
| } | ||
|
|
||
| // Find the CEO agent once per sweep. | ||
| ceo, _ := s.db.GetCEOAgent() |
There was a problem hiding this comment.
The error from s.db.GetCEOAgent() is ignored. If this call fails, ceo will be nil, and no stall notification comments will be posted. This failure will be silent. The error should be logged to make this condition visible.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
internal/models/models.go (1)
417-420: Either round here or fix the contract.
IdleHours()currently returns the raw fractional hour value, so the new docstring is false as written.♻️ Minimal fix
-// IdleHours returns the idle duration rounded to the nearest tenth of an hour. +// IdleHours returns the idle duration in hours. func (s *StalledIssue) IdleHours() float64 { return s.IdleDuration.Hours() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/models/models.go` around lines 417 - 420, The docstring for StalledIssue.IdleHours claims it rounds to the nearest tenth but the method currently returns the raw fractional hours; update the implementation of IdleHours to perform rounding (use math.Round(s.IdleDuration.Hours()*10)/10) so it returns hours rounded to the nearest 0.1, and add the math import if missing, or alternatively change the docstring to reflect that it returns the raw fractional hour value—refer to the StalledIssue.IdleHours method and the IdleDuration field to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/db/queries.go`:
- Around line 1647-1657: Replace the COALESCE-based last_activity logic with a
comparison that takes the later of the issue's updated_at and the newest real
comment timestamp, and exclude the self-generated "Stall Detector" notification
comment from that comment timestamp calculation: use GREATEST(i.updated_at,
filtered_last_comment_at) where filtered_last_comment_at is obtained by SELECT
MAX(c.created_at) FROM comments c WHERE c.issue_key = i.key AND <exclude Stall
Detector notification comments> (e.g. exclude by c.author = 'Stall Detector'
and/or c.type = 'notification' per your schema). Apply the same filtered
MAX(...) change to the LastCommentAt calculation used elsewhere (the block
referenced at lines ~1698-1715) so both last_activity and LastCommentAt use the
filtered latest comment and then take the greater of that and i.updated_at.
In `@internal/scheduler/scheduler.go`:
- Around line 1341-1359: The current AC2 path creates a plain comment via
s.db.CreateComment(&models.Comment{...}) which does not surface in the CEO agent
inbox because buildHeartbeatPrompt() / GetAgentInbox(agent.ID) selects items by
issues.assignee_agent_id rather than comment author; to fix, instead of only
creating a comment, create or assign a light-weight CEO-owned issue/queue entry
(set issues.assignee_agent_id = ceo.ID or create an inbox-assigned Issue record)
and/or link the comment to that CEO-assigned issue so GetAgentInbox(ceo.ID) will
return it; update the AC2 logic in scheduler.go (the block referencing ceo,
CreateComment, and body) to create/assign the CEO issue (or set assignee on an
existing stub issue) and then attach the notification comment to that
CEO-assigned issue so buildHeartbeatPrompt/GetAgentInbox will include it.
- Around line 1289-1290: The notified map currently never removes keys so once
an issue is alerted it is suppressed forever; update the logic that builds/uses
notified (the map[string]bool named "notified") to prune entries that are no
longer in the current "stalled" slice/set before applying dedupe—i.e., compute
the current stalled IDs into a set, delete any notified keys not present in that
set so subsequent new stall events can re-notify, and apply this same pruning at
the other affected sites referenced (the other notified/stalled usages around
the 1317–1319 and 1326–1362 regions) so dedupe is per-stall-event rather than
permanent.
- Around line 1321-1362: The code currently ignores errors from
s.db.GetCEOAgent() and s.db.CreateComment() but marks issues as notified
regardless; change the logic so you capture and handle the error returned by
GetCEOAgent() and by CreateComment(), logging failures and skipping setting
notified[key], and only set notified[key] = true after a successful notification
delivery (i.e., after CreateComment returns nil). Specifically, update the use
of GetCEOAgent() to check its error return (from ceo, _ to ceo, err) and if err
!= nil log and continue without marking notified; when ceo != nil call
CreateComment and check its error, logging and continuing on failure; only
assign notified[key] = true when the comment creation succeeded (and decide not
to mark notified if ceo is nil or any DB call failed).
In `@internal/templates/activity.html`:
- Around line 263-274: The client timeAgo(isoStr) deviates from the Go helper by
rounding and emitting seconds; update timeAgo in
internal/templates/activity.html to mirror the Go logic: treat any sec < 60 as
"just now" (no seconds output), use Math.floor instead of Math.round for
subsequent units, compute min = Math.floor(sec/60), hr = Math.floor(min/60), day
= Math.floor(hr/24), and return values like "Xm ago", "Xh ago", "1d ago" or "Xd
ago" using the floored units and the same singular/day handling as the Go
helper.
In `@internal/templates/dashboard.html`:
- Around line 101-124: The panel element with id "#stalled-issues-panel" is
currently inside "{{if .StalledIssues}}" which prevents HTMX swaps when the
server response omits the panel; change the template so the outer <div
id="stalled-issues-panel" ... hx-get=... hx-select=... hx-target=... hx-swap=...
hx-trigger=...> is always rendered, and only the inner list body (the {{range
.StalledIssues}} ... {{end}} block or a message/empty state) is conditional on
".StalledIssues"; keep the same HTMX attributes and the existing child markup
(e.g., the header, counts using "{{len .StalledIssues}}", links using
"{{.Issue.Key}}", "{{.Issue.Title}}", and "{{printf "%.1f"
.IdleDuration.Hours}}h idle") so swaps work when the server adds or removes
stalled issues.
---
Nitpick comments:
In `@internal/models/models.go`:
- Around line 417-420: The docstring for StalledIssue.IdleHours claims it rounds
to the nearest tenth but the method currently returns the raw fractional hours;
update the implementation of IdleHours to perform rounding (use
math.Round(s.IdleDuration.Hours()*10)/10) so it returns hours rounded to the
nearest 0.1, and add the math import if missing, or alternatively change the
docstring to reflect that it returns the raw fractional hour value—refer to the
StalledIssue.IdleHours method and the IdleDuration field to locate the change.
🪄 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
Run ID: 9b9a42eb-cecb-4ed1-8dd6-59faed5a0987
📒 Files selected for processing (11)
artifact-docs/architecture/SO-87-stall-detector.mdcmd/secondorder/main.gointernal/db/db_test.gointernal/db/migrations/019_stall_detection.sqlinternal/db/queries.gointernal/handlers/ui.gointernal/models/models.gointernal/scheduler/scheduler.gointernal/templates/activity.htmlinternal/templates/dashboard.htmlinternal/templates/templates.go
| COALESCE( | ||
| (SELECT MAX(c.created_at) FROM comments c WHERE c.issue_key = i.key), | ||
| i.updated_at | ||
| ) AS last_activity | ||
| FROM issues i | ||
| LEFT JOIN agents a ON i.assignee_agent_id = a.id | ||
| WHERE i.status = 'in_progress' | ||
| AND COALESCE( | ||
| (SELECT MAX(c.created_at) FROM comments c WHERE c.issue_key = i.key), | ||
| i.updated_at | ||
| ) < ? |
There was a problem hiding this comment.
Compute last_activity from the newest real activity.
This query has two correctness gaps: COALESCE(last_comment, i.updated_at) picks the comment timestamp whenever one exists, even if i.updated_at is newer, and it includes the "Stall Detector" notification comment in the activity stream. The first causes false positives after a later status/assignment/edit, and the second makes the detector clear its own stalled state for another threshold window. Use MAX(i.updated_at, filtered_last_comment_at) here, and apply the same filter to LastCommentAt.
Also applies to: 1698-1715
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/db/queries.go` around lines 1647 - 1657, Replace the COALESCE-based
last_activity logic with a comparison that takes the later of the issue's
updated_at and the newest real comment timestamp, and exclude the self-generated
"Stall Detector" notification comment from that comment timestamp calculation:
use GREATEST(i.updated_at, filtered_last_comment_at) where
filtered_last_comment_at is obtained by SELECT MAX(c.created_at) FROM comments c
WHERE c.issue_key = i.key AND <exclude Stall Detector notification comments>
(e.g. exclude by c.author = 'Stall Detector' and/or c.type = 'notification' per
your schema). Apply the same filtered MAX(...) change to the LastCommentAt
calculation used elsewhere (the block referenced at lines ~1698-1715) so both
last_activity and LastCommentAt use the filtered latest comment and then take
the greater of that and i.updated_at.
| // Track issues we've already notified this process lifetime to avoid spam. | ||
| notified := make(map[string]bool) |
There was a problem hiding this comment.
Expire dedupe entries when an issue stops being stalled.
notified only grows, and the early return on an empty stalled slice means it never gets cleaned up. Once an issue has been alerted once, any later stall of the same issue in the same process is permanently suppressed, which does not match the "one notification per stall event" behavior described in the docstring.
Suggested fix
func (s *Scheduler) runStallDetection(threshold time.Duration, notified map[string]bool) {
stalled, err := s.db.GetStalledIssues(threshold)
if err != nil {
slog.Error("stall-detector: failed to query stalled issues", "error", err)
return
}
+ current := make(map[string]struct{}, len(stalled))
+ for _, si := range stalled {
+ current[si.Issue.Key] = struct{}{}
+ }
+ for key := range notified {
+ if _, ok := current[key]; !ok {
+ delete(notified, key)
+ }
+ }
+
if len(stalled) == 0 {
return
}Also applies to: 1317-1319, 1326-1362
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/scheduler/scheduler.go` around lines 1289 - 1290, The notified map
currently never removes keys so once an issue is alerted it is suppressed
forever; update the logic that builds/uses notified (the map[string]bool named
"notified") to prune entries that are no longer in the current "stalled"
slice/set before applying dedupe—i.e., compute the current stalled IDs into a
set, delete any notified keys not present in that set so subsequent new stall
events can re-notify, and apply this same pruning at the other affected sites
referenced (the other notified/stalled usages around the 1317–1319 and 1326–1362
regions) so dedupe is per-stall-event rather than permanent.
| // Find the CEO agent once per sweep. | ||
| ceo, _ := s.db.GetCEOAgent() | ||
|
|
||
| for _, si := range stalled { | ||
| key := si.Issue.Key | ||
| if notified[key] { | ||
| continue | ||
| } | ||
|
|
||
| idleHours := si.IdleDuration.Hours() | ||
| thresholdHours := threshold.Hours() | ||
|
|
||
| slog.Warn("stall-detector: issue stalled", | ||
| "issue", key, | ||
| "title", si.Issue.Title, | ||
| "assignee", si.Issue.AssigneeName, | ||
| "idle_hours", fmt.Sprintf("%.1f", idleHours), | ||
| "threshold_hours", fmt.Sprintf("%.1f", thresholdHours), | ||
| ) | ||
|
|
||
| // AC2: post a CEO inbox notification comment. | ||
| if ceo != nil { | ||
| body := fmt.Sprintf( | ||
| "⚠️ **Stall detected** on [%s]: *%s*\n\n"+ | ||
| "- Assignee: %s\n"+ | ||
| "- Idle for: %.1f hours (threshold: %.1f h)\n"+ | ||
| "- Status: %s\n\n"+ | ||
| "Please investigate and escalate if needed.", | ||
| key, si.Issue.Title, | ||
| si.Issue.AssigneeName, | ||
| idleHours, thresholdHours, | ||
| si.Issue.Status, | ||
| ) | ||
| _ = s.db.CreateComment(&models.Comment{ | ||
| IssueKey: key, | ||
| AgentID: &ceo.ID, | ||
| Author: "Stall Detector", | ||
| Body: body, | ||
| }) | ||
| } | ||
|
|
||
| notified[key] = true |
There was a problem hiding this comment.
Only mark an issue as notified after the notification is delivered.
GetCEOAgent() and CreateComment() failures are ignored here, but notified[key] is still set. A transient DB error will silently drop the alert for the rest of the process lifetime instead of retrying on the next sweep.
Suggested fix
- ceo, _ := s.db.GetCEOAgent()
+ ceo, err := s.db.GetCEOAgent()
+ if err != nil {
+ slog.Error("stall-detector: failed to load CEO agent", "error", err)
+ return
+ }
...
- if ceo != nil {
- body := fmt.Sprintf(
+ if ceo != nil {
+ body := fmt.Sprintf(
"⚠️ **Stall detected** on [%s]: *%s*\n\n"+
"- Assignee: %s\n"+
"- Idle for: %.1f hours (threshold: %.1f h)\n"+
"- Status: %s\n\n"+
"Please investigate and escalate if needed.",
key, si.Issue.Title,
si.Issue.AssigneeName,
idleHours, thresholdHours,
si.Issue.Status,
)
- _ = s.db.CreateComment(&models.Comment{
+ if err := s.db.CreateComment(&models.Comment{
IssueKey: key,
AgentID: &ceo.ID,
Author: "Stall Detector",
Body: body,
- })
+ }); err != nil {
+ slog.Error("stall-detector: failed to create notification comment", "issue", key, "error", err)
+ continue
+ }
}
notified[key] = true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/scheduler/scheduler.go` around lines 1321 - 1362, The code currently
ignores errors from s.db.GetCEOAgent() and s.db.CreateComment() but marks issues
as notified regardless; change the logic so you capture and handle the error
returned by GetCEOAgent() and by CreateComment(), logging failures and skipping
setting notified[key], and only set notified[key] = true after a successful
notification delivery (i.e., after CreateComment returns nil). Specifically,
update the use of GetCEOAgent() to check its error return (from ceo, _ to ceo,
err) and if err != nil log and continue without marking notified; when ceo !=
nil call CreateComment and check its error, logging and continuing on failure;
only assign notified[key] = true when the comment creation succeeded (and decide
not to mark notified if ceo is nil or any DB call failed).
| // AC2: post a CEO inbox notification comment. | ||
| if ceo != nil { | ||
| body := fmt.Sprintf( | ||
| "⚠️ **Stall detected** on [%s]: *%s*\n\n"+ | ||
| "- Assignee: %s\n"+ | ||
| "- Idle for: %.1f hours (threshold: %.1f h)\n"+ | ||
| "- Status: %s\n\n"+ | ||
| "Please investigate and escalate if needed.", | ||
| key, si.Issue.Title, | ||
| si.Issue.AssigneeName, | ||
| idleHours, thresholdHours, | ||
| si.Issue.Status, | ||
| ) | ||
| _ = s.db.CreateComment(&models.Comment{ | ||
| IssueKey: key, | ||
| AgentID: &ceo.ID, | ||
| Author: "Stall Detector", | ||
| Body: body, | ||
| }) |
There was a problem hiding this comment.
This does not actually put the alert into the CEO inbox.
buildHeartbeatPrompt() pulls the CEO's work from GetAgentInbox(agent.ID), and that query is keyed off issues.assignee_agent_id, not comment authorship. Adding a comment to another agent's issue leaves it out of the CEO inbox, so the CEO agent will not see this on its next heartbeat. If AC2 requires an inbox notification, this needs a CEO-assigned issue/queue instead of a plain comment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/scheduler/scheduler.go` around lines 1341 - 1359, The current AC2
path creates a plain comment via s.db.CreateComment(&models.Comment{...}) which
does not surface in the CEO agent inbox because buildHeartbeatPrompt() /
GetAgentInbox(agent.ID) selects items by issues.assignee_agent_id rather than
comment author; to fix, instead of only creating a comment, create or assign a
light-weight CEO-owned issue/queue entry (set issues.assignee_agent_id = ceo.ID
or create an inbox-assigned Issue record) and/or link the comment to that
CEO-assigned issue so GetAgentInbox(ceo.ID) will return it; update the AC2 logic
in scheduler.go (the block referencing ceo, CreateComment, and body) to
create/assign the CEO issue (or set assignee on an existing stub issue) and then
attach the notification comment to that CEO-assigned issue so
buildHeartbeatPrompt/GetAgentInbox will include it.
| function timeAgo(isoStr) { | ||
| var d = new Date(isoStr); | ||
| var sec = Math.round((Date.now() - d.getTime()) / 1000); | ||
| if (sec < 5) return 'just now'; | ||
| if (sec < 60) return sec + 's ago'; | ||
| var min = Math.round(sec / 60); | ||
| if (min < 60) return min + 'm ago'; | ||
| var hr = Math.round(min / 60); | ||
| if (hr < 24) return hr + 'h ago'; | ||
| var day = Math.round(hr / 24); | ||
| if (day === 1) return '1d ago'; | ||
| return day + 'd ago'; |
There was a problem hiding this comment.
Keep the client timeAgo() logic aligned with the Go helper.
This version rounds and emits seconds, while internal/templates/templates.go keeps the whole first minute as just now and floors later units. That means the text can change meaning on the first refresh and can jump to 1h ago / 1d ago early.
🛠️ Proposed fix
function timeAgo(isoStr) {
var d = new Date(isoStr);
- var sec = Math.round((Date.now() - d.getTime()) / 1000);
- if (sec < 5) return 'just now';
- if (sec < 60) return sec + 's ago';
- var min = Math.round(sec / 60);
+ var sec = Math.floor((Date.now() - d.getTime()) / 1000);
+ if (sec < 60) return 'just now';
+ var min = Math.floor(sec / 60);
if (min < 60) return min + 'm ago';
- var hr = Math.round(min / 60);
+ var hr = Math.floor(min / 60);
if (hr < 24) return hr + 'h ago';
- var day = Math.round(hr / 24);
+ var day = Math.floor(hr / 24);
if (day === 1) return '1d ago';
return day + 'd 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.
| function timeAgo(isoStr) { | |
| var d = new Date(isoStr); | |
| var sec = Math.round((Date.now() - d.getTime()) / 1000); | |
| if (sec < 5) return 'just now'; | |
| if (sec < 60) return sec + 's ago'; | |
| var min = Math.round(sec / 60); | |
| if (min < 60) return min + 'm ago'; | |
| var hr = Math.round(min / 60); | |
| if (hr < 24) return hr + 'h ago'; | |
| var day = Math.round(hr / 24); | |
| if (day === 1) return '1d ago'; | |
| return day + 'd ago'; | |
| function timeAgo(isoStr) { | |
| var d = new Date(isoStr); | |
| var sec = Math.floor((Date.now() - d.getTime()) / 1000); | |
| if (sec < 60) return 'just now'; | |
| var min = Math.floor(sec / 60); | |
| if (min < 60) return min + 'm ago'; | |
| var hr = Math.floor(min / 60); | |
| if (hr < 24) return hr + 'h ago'; | |
| var day = Math.floor(hr / 24); | |
| if (day === 1) return '1d ago'; | |
| return day + 'd ago'; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/templates/activity.html` around lines 263 - 274, The client
timeAgo(isoStr) deviates from the Go helper by rounding and emitting seconds;
update timeAgo in internal/templates/activity.html to mirror the Go logic: treat
any sec < 60 as "just now" (no seconds output), use Math.floor instead of
Math.round for subsequent units, compute min = Math.floor(sec/60), hr =
Math.floor(min/60), day = Math.floor(hr/24), and return values like "Xm ago",
"Xh ago", "1d ago" or "Xd ago" using the floored units and the same singular/day
handling as the Go helper.
| {{if .StalledIssues}} | ||
| <div id="stalled-issues-panel" class="mb-6" | ||
| hx-get="/dashboard" hx-select="#stalled-issues-panel" hx-target="#stalled-issues-panel" hx-swap="outerHTML" | ||
| hx-trigger="run-complete from:body,issue-changed from:body"> | ||
| <div class="flex items-center justify-between mb-3"> | ||
| <div class="flex items-center gap-2"> | ||
| <h2 class="text-[13px] font-medium text-amber-400">Stalled Issues</h2> | ||
| <span class="inline-flex items-center px-1.5 py-0.5 rounded text-[10px] font-bold bg-amber-500/20 text-amber-400 border border-amber-500/30">{{len .StalledIssues}}</span> | ||
| </div> | ||
| <span class="text-xs text-ink3/50">No activity for >{{printf "%.0f" .StallThresholdH}}h</span> | ||
| </div> | ||
| <div class="bg-card border border-amber-500/30 rounded-lg divide-y divide-sf overflow-hidden"> | ||
| {{range .StalledIssues}} | ||
| <a href="/issues/{{.Issue.Key}}" class="flex items-center gap-3 px-4 py-2.5 hover:bg-sf transition-all group"> | ||
| <span class="inline-flex items-center gap-1 px-1.5 py-0.5 rounded text-[10px] font-medium shrink-0 bg-amber-500/20 text-amber-400">⏸ STALLED</span> | ||
| <span class="font-mono text-xs text-ink3/50 w-14 shrink-0">{{.Issue.Key}}</span> | ||
| <span class="text-[13px] text-ink flex-1 truncate group-hover:text-ink" title="{{.Issue.Title}}">{{.Issue.Title}}</span> | ||
| {{if .Issue.AssigneeName}}<span class="text-[11px] text-ink3/50 shrink-0">{{.Issue.AssigneeName}}</span>{{end}} | ||
| <span class="text-[11px] text-amber-400 shrink-0 tabular-nums font-mono">{{printf "%.1f" .IdleDuration.Hours}}h idle</span> | ||
| </a> | ||
| {{end}} | ||
| </div> | ||
| </div> | ||
| {{end}} |
There was a problem hiding this comment.
Always render a stable #stalled-issues-panel container.
Because the HTMX hooks live on the panel itself, wrapping the whole element in {{if .StalledIssues}} breaks both transitions: newly stalled issues will never appear when the initial list is empty, and resolved ones will never disappear once the response stops including #stalled-issues-panel.
🛠️ Proposed fix
- {{if .StalledIssues}}
- <div id="stalled-issues-panel" class="mb-6"
+ <div id="stalled-issues-panel" class="{{if .StalledIssues}}mb-6{{else}}hidden{{end}}"
hx-get="/dashboard" hx-select="#stalled-issues-panel" hx-target="#stalled-issues-panel" hx-swap="outerHTML"
hx-trigger="run-complete from:body,issue-changed from:body">
+ {{if .StalledIssues}}
<div class="flex items-center justify-between mb-3">
<div class="flex items-center gap-2">
<h2 class="text-[13px] font-medium text-amber-400">Stalled Issues</h2>
@@
<div class="bg-card border border-amber-500/30 rounded-lg divide-y divide-sf overflow-hidden">
{{range .StalledIssues}}
<a href="/issues/{{.Issue.Key}}" class="flex items-center gap-3 px-4 py-2.5 hover:bg-sf transition-all group">
@@
</a>
{{end}}
</div>
- </div>
- {{end}}
+ {{end}}
+ </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/templates/dashboard.html` around lines 101 - 124, The panel element
with id "#stalled-issues-panel" is currently inside "{{if .StalledIssues}}"
which prevents HTMX swaps when the server response omits the panel; change the
template so the outer <div id="stalled-issues-panel" ... hx-get=...
hx-select=... hx-target=... hx-swap=... hx-trigger=...> is always rendered, and
only the inner list body (the {{range .StalledIssues}} ... {{end}} block or a
message/empty state) is conditional on ".StalledIssues"; keep the same HTMX
attributes and the existing child markup (e.g., the header, counts using "{{len
.StalledIssues}}", links using "{{.Issue.Key}}", "{{.Issue.Title}}", and
"{{printf "%.1f" .IdleDuration.Hours}}h idle") so swaps work when the server
adds or removes stalled issues.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
artifact-docs/architecture/SO-24-timago-live-refresh.md (3)
5-5: Minor style: hyphenate compound adjective."full page reload" should be "full-page reload" when used as a compound adjective.
📝 Proposed fix
-The activity feed rendered `timeAgo` timestamps server-side as static HTML strings (e.g. "2m ago"). Once the page was loaded, these strings never updated — an entry that showed "2m ago" on load still showed "2m ago" an hour later without a full page reload. +The activity feed rendered `timeAgo` timestamps server-side as static HTML strings (e.g. "2m ago"). Once the page was loaded, these strings never updated — an entry that showed "2m ago" on load still showed "2m ago" an hour later without a full-page reload.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@artifact-docs/architecture/SO-24-timago-live-refresh.md` at line 5, Replace the un-hyphenated compound adjective "full page reload" with the hyphenated form "full-page reload" in the activity feed description (the sentence referencing timeAgo timestamps and page reload behavior) so the compound adjective is styled correctly.
40-52: Consider upper bounds for very old timestamps.The
timeAgo()function will display "365d ago" for year-old entries. For better UX, consider adding thresholds for months/years (e.g., "3mo ago", "1y ago") or falling back to absolute dates for entries older than 30-90 days.However, the document notes this mirrors the Go
timeAgo()function (line 71), so any change should be applied consistently to both implementations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@artifact-docs/architecture/SO-24-timago-live-refresh.md` around lines 40 - 52, The timeAgo() JS implementation only returns days for old timestamps and should be extended to handle months and years (or fall back to absolute dates) to avoid outputs like "365d ago"; update the timeAgo(isoStr) function to add thresholds and labels for months ("Xmo ago") and years ("Xy ago") or switch to an ISO/locale date string for entries older than a configurable cutoff (e.g., 30–90 days), and mirror the same logic change in the Go timeAgo() implementation referenced in the doc so both functions produce consistent output.
58-58: Consider storing the interval ID for cleanup, though the current implementation's risk is minimal.The
setIntervalat line 58 continues running after HTMX swaps content out of the DOM. However, line 59 already mitigates this by listening tohtmx:afterSwapand refreshing timestamps immediately on new content.The main risk is not a memory leak (the IIFE pattern prevents that) but rather an unnecessary query selector every 30 seconds when the activity feed is unmounted. For a 30-second interval, the performance impact is negligible, especially since the querySelector on removed elements simply returns an empty list.
For explicit cleanup if this pattern is reused elsewhere, consider storing the interval ID:
var timerId = setInterval(...)and clearing it via a custom event or page lifecycle hook. This is optional for the current code but would be cleaner if the activity feed becomes conditionally mounted/unmounted more frequently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@artifact-docs/architecture/SO-24-timago-live-refresh.md` at line 58, The setInterval call using setInterval(refreshTimestamps, 30000) keeps firing even after HTMX swaps the activity feed out of the DOM; store the interval ID (e.g., var timerId = setInterval(...)) and clear it when the feed is removed or on a lifecycle/HTMX event to avoid unnecessary work (use clearInterval(timerId) on a custom teardown or an htmx event listener tied to the feed); update the IIFE to capture timerId and add cleanup logic alongside the existing htmx:afterSwap handler so refreshTimestamps only runs while the feed is mounted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@artifact-docs/architecture/SO-24-timago-live-refresh.md`:
- Line 5: Replace the un-hyphenated compound adjective "full page reload" with
the hyphenated form "full-page reload" in the activity feed description (the
sentence referencing timeAgo timestamps and page reload behavior) so the
compound adjective is styled correctly.
- Around line 40-52: The timeAgo() JS implementation only returns days for old
timestamps and should be extended to handle months and years (or fall back to
absolute dates) to avoid outputs like "365d ago"; update the timeAgo(isoStr)
function to add thresholds and labels for months ("Xmo ago") and years ("Xy
ago") or switch to an ISO/locale date string for entries older than a
configurable cutoff (e.g., 30–90 days), and mirror the same logic change in the
Go timeAgo() implementation referenced in the doc so both functions produce
consistent output.
- Line 58: The setInterval call using setInterval(refreshTimestamps, 30000)
keeps firing even after HTMX swaps the activity feed out of the DOM; store the
interval ID (e.g., var timerId = setInterval(...)) and clear it when the feed is
removed or on a lifecycle/HTMX event to avoid unnecessary work (use
clearInterval(timerId) on a custom teardown or an htmx event listener tied to
the feed); update the IIFE to capture timerId and add cleanup logic alongside
the existing htmx:afterSwap handler so refreshTimestamps only runs while the
feed is mounted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a248aec0-6f20-4c70-ab06-faadc655b817
📒 Files selected for processing (1)
artifact-docs/architecture/SO-24-timago-live-refresh.md
1 participant
Summary
Fixes frozen relative timestamps in the activity feed. Timestamps were rendered server-side as static strings (e.g.
2m ago) that never updated after page load.Changes
internal/templates/templates.goisoTime(t time.Time) stringfunction (returns UTC RFC3339)"iso"infuncMapalongsidetimeAgointernal/templates/activity.htmlactivity_entrytemplate: Updated timestamp span to includeclass="time-ago"anddata-ts="{{iso .CreatedAt}}"attribute<span class="text-[11px]...">{{timeAgo .CreatedAt}}</span><span class="time-ago text-[11px]..." data-ts="{{iso .CreatedAt}}">{{timeAgo .CreatedAt}}</span><script>block with self-invoking JS that:.time-ago[data-ts]spans and recomputes text via JStimeAgo()every 30 seconds (setInterval)htmx:afterSwapso SSE-injected new entries are updated immediatelytimeAgo()mirrors server-side logic: just now / Xs ago / Xm ago / Xh ago / Xd agoTesting
Closes SO-24