Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds heartbeat emission/recording and pending-prompt state to agent sessions, implements per-user session limits with automatic cleanup, updates frontend UI for paginated session lists and a session sidebar, and replaces generated HTTP handlers with a middleware-based StrictServerInterface and strictHandler pattern. Changes
Sequence Diagram(s)mermaid Note: components use rgba(100,149,237,0.5) for visualization boxes where rendered. Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 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 |
![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 (7)
internal/cmn/schema/config.schema.json (1)
755-766: Consider adding the"default"keyword to formally express the default value.The default of 100 is documented only in prose. Adding
"default": 100to themax_per_userproperty would allow schema-aware editors and YAML completion tools to surface it automatically to operators writing config files.♻️ Proposed improvement
"max_per_user": { "type": "integer", "description": "Maximum sessions to keep per user. Oldest sessions are deleted when exceeded. Default: 100. Set to 0 for unlimited.", - "minimum": 0 + "minimum": 0, + "default": 100 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cmn/schema/config.schema.json` around lines 755 - 766, The JSON schema for SessionDef documents a default of 100 for the max_per_user property only in prose; update the SessionDef -> properties -> max_per_user definition to include a "default": 100 so schema-aware editors can surface the default value (target the "SessionDef" object and its "max_per_user" property).api/v1/api.gen.go (1)
522-530:HasPendingPrompt,Model,Working, andTotalCostare duplicated acrossAgentSessionStateandAgentSessionWithStatein the OpenAPI spec.The spec defines both schemas independently with overlapping fields (
working,hasPendingPrompt,model,totalCost). If a new state field is added toAgentSessionState, the generator will not automatically updateAgentSessionWithState, risking drift between the two.Since this is generated code, fix the spec in
api.yaml: defineAgentSessionWithStateviaallOfcomposingAgentSessionandAgentSessionStaterather than redeclaring the state fields. This keeps the two in sync automatically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v1/api.gen.go` around lines 522 - 530, The generated type AgentSessionWithState duplicates fields from AgentSessionState (HasPendingPrompt, Model, Working, TotalCost); update the OpenAPI spec (api.yaml) to define AgentSessionWithState using allOf composition so it composes AgentSession and AgentSessionState instead of redeclaring state fields—i.e., replace the independent schema for AgentSessionWithState with an allOf that references both AgentSession and AgentSessionState so the generator will produce a single combined struct and avoid drift.internal/cmn/config/definition.go (1)
299-302: Consider documenting any “unlimited” sentinel behavior.If
max_per_user: 0is treated as unlimited elsewhere, noting that here would reduce ambiguity for config readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cmn/config/definition.go` around lines 299 - 302, Document the sentinel behavior for unlimited session storage on SessionDef.MaxPerUser: update the comment on the SessionDef struct (and specifically the MaxPerUser field) to state whether a value of 0 (or nil) is treated as "unlimited" and what the default semantics are (e.g., "Default: 100; 0 = unlimited"). Reference the SessionDef type and MaxPerUser field so future readers know the exact sentinel meaning.internal/agent/session.go (1)
176-188:HasPendingPrompt()read is not atomic with the working-state snapshot.
SetWorkingcallsupdateWorkingState(which readsid/model/totalCostundersm.mu), then callsHasPendingPrompt()outside that lock scope. The broadcast therefore capturesworkingandHasPendingPromptat slightly different instants. This is benign for an advisory UI flag—just noting it as a design trade-off.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/agent/session.go` around lines 176 - 188, The broadcast currently captures working-state fields (via updateWorkingState in SetWorking) and then calls HasPendingPrompt() outside the same lock, causing a non-atomic snapshot; to fix, ensure HasPendingPrompt is read under the same mutex or returned alongside other state: either move the HasPendingPrompt() call inside the sm.mu-protected section (or modify updateWorkingState to compute and return hasPendingPrompt with id/model/totalCost) and then use that single atomic value when constructing StreamResponse/SessionState before sm.subpub.Broadcast and callback.internal/persis/filesession/store.go (2)
377-415:deleteSessionLocked: file I/O under write lock is acceptable but worth noting.
loadSessionFromFileandos.Removeperform disk I/O while the write lock is held. For the cleanup path (which deletes a handful of sessions infrequently), this is fine. If the store grows to high-throughput use, consider deferring file removal outside the critical section.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/persis/filesession/store.go` around lines 377 - 415, deleteSessionLocked currently does disk I/O (calling loadSessionFromFile and os.Remove) while holding the store write lock, which can block concurrent operations; to fix this, change the flow in deleteSessionLocked (and helpers like removeSessionFromUserIndex) to minimize time under the lock by: (1) acquire the lock only to check and remove in-memory indexes (s.byID, s.updatedAt, s.byParent) and capture the filePath plus any metadata needed (userID, parentID), (2) release the lock, perform loadSessionFromFile and os.Remove outside the lock (or perform os.Remove in a background goroutine if you prefer async cleanup), and (3) if loadSessionFromFile is required to compute index removals, load the file before taking the lock and then revalidate existence under lock to avoid races; reference deleteSessionLocked, loadSessionFromFile, os.Remove and s.byID/s.byParent when implementing this change.
443-498:enforceMaxSessionsLockedbuilds a global sub-session set on every invocation.Lines 453–458 iterate over all entries in
s.byParent(across all users) to build the exclusion set, even though only sessions belonging touserIDneed to be checked. For a store with many users and sub-sessions this could become a hot spot on everyCreateSession.A lighter approach: only check whether each candidate session ID appears as a value in
byParent, or maintain a dedicatedisChild map[string]boolalongsidebyParent.This is fine for the current scale (config default 100), but worth keeping in mind.
internal/agent/api.go (1)
811-821: Stuck-session detection looks correct; minor TOCTOU is benign.After
mgr.Cancel()on line 815 setsworking = false, the follow-up check on line 819 (!mgr.IsWorking()) becomes true. However,LastActivity()will still reflect the recent heartbeat timestamp, so the session won't be prematurely added totoDeleteunless it's also past the 30-minute idle cutoff—which is the desired behavior.Two small observations:
- Cancel error is silently discarded — consider logging it at debug level for diagnostics.
- The cancelled session stays in
sessionsmap until the next cleanup cycle. This is fine since the user can still view/reactivate it, but it's worth documenting.🔧 Minor: log cancel error
- _ = mgr.Cancel(context.Background()) - a.logger.Warn("Cancelled stuck session", "session_id", id) + if err := mgr.Cancel(context.Background()); err != nil { + a.logger.Warn("Failed to cancel stuck session", "session_id", id, "error", err) + } else { + a.logger.Warn("Cancelled stuck session", "session_id", id) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/agent/api.go` around lines 811 - 821, When cancelling stuck sessions call out and log any error returned by mgr.Cancel instead of discarding it: capture the error from mgr.Cancel(context.Background()) and emit a.debug-level log via a.logger.Debug (or similar) including the session id and the error; keep the existing a.logger.Warn("Cancelled stuck session", "session_id", id) but add the debug log for the error path. Also add a short code comment near the cleanup logic (around IsWorking/LastActivity checks and the sessions map) noting that cancelled sessions remain in the sessions map until the next cleanup cycle so they can be viewed/reactivated, to make the behavior explicit for future readers.
🤖 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/agent/api_test.go`:
- Around line 1208-1277: Update the three tests
TestAPI_CleanupIdleSessions_CancelsStuckSession,
TestAPI_CleanupIdleSessions_DoesNotCancelHealthyWorkingSession, and
TestAPI_CleanupIdleSessions_DoesNotCancelZeroHeartbeat to use require.* (from
stretchr/testify/require) instead of assert.* so failures fail fast; replace
assert.False/True and assert calls with the corresponding
require.False/True/NoError as appropriate, and stop duplicating mocks by using
the shared fixture from internal/test for the config store instead of
newMockConfigStore (i.e., replace newMockConfigStore(true) with the
project-provided mock from internal/test or its helper function), keeping
references to NewAPI, SessionManager, api.cleanupIdleSessions, and the session
IDs unchanged.
In `@internal/agent/loop_test.go`:
- Around line 246-291: Replace the final assertion in the "calls OnHeartbeat
during tool calls" test to use require for fail-fast behavior: change
assert.GreaterOrEqual(t, count, 1, ...) to require.GreaterOrEqual(t, count, 1,
"OnHeartbeat should fire during tool call handling"), and add the require import
if missing; keep the rest of the test (mockLLMProvider, NewLoop,
LoopConfig.OnHeartbeat, runLoopForDuration, heartbeatCount) unchanged.
In `@internal/agent/session_test.go`:
- Around line 807-846: Replace the loose assert calls in
TestSessionManager_HasPendingPrompt with require assertions so failures fail
fast: in the TestSessionManager_HasPendingPrompt function, change
assert.False/assert.True to require.False/require.True when asserting
sm.HasPendingPrompt(), and add the testify require import
("github.com/stretchr/testify/require") to the test file; keep the same
setup/manipulation of sm.pendingPrompts (the locks and channel
insertion/deletion) but use require to validate expectations to follow project
test conventions.
In `@internal/cmn/config/config.go`:
- Around line 132-133: Add validation to reject negative values for
Session.MaxPerUser (where Session is of type SessionConfig) by adding a check in
validateServer or a new validateSession function: ensure MaxPerUser >= 0, treat
0 as "unlimited", and return an error if MaxPerUser is negative; update any
server startup/config-load paths that call validateServer to surface this
validation so invalid configs are rejected early.
In `@ui/src/api/v1/schema.ts`:
- Around line 27-46: The /auth/setup route in the generated schema (the type
block containing "post: operations[\"setup\"]") is missing the required
remoteNode query parameter; update the OpenAPI spec for the /auth/setup
operation to include a remoteNode query parameter (matching the shape and name
used across other routes), re-run the schema/codegen to regenerate
ui/src/api/v1/schema.ts so the generated type contains remoteNode under
parameters.query for the "/auth/setup" entry, and ensure the operations["setup"]
typing aligns with the new parameter.
In `@ui/src/features/agent/components/AgentChatModalHeader.tsx`:
- Around line 89-96: The status indicator spans in AgentChatModalHeader.tsx (the
conditional <span> elements showing bg-orange-400 and bg-green-500 with title
attributes) rely on color and title which aren’t reliably announced by screen
readers; update those spans to include an accessible name by adding role="img"
and a meaningful aria-label (e.g., "Waiting for input" and "Running") or
alternatively render a visually-hidden text sibling with those labels, ensuring
the accessible text reflects sess.has_pending_prompt and sess.working states so
assistive tech can announce the status.
---
Duplicate comments:
In `@ui/src/api/v1/schema.ts`:
- Around line 3635-3685: The setup operation is missing the remoteNode field
referenced in the `/auth/setup` comment; update the API spec so the requestBody
for the setup operation includes remoteNode (i.e., ensure
components.schemas.SetupRequest defines the remoteNode property with the correct
type and required status, or adjust the setup operation to reference a
SetupRequest schema that contains remoteNode). Locate the "setup" operation in
schema.ts and either modify the inline requestBody/content schema or update
components["schemas"]["SetupRequest"] to include remoteNode so the operation's
request accepts that field.
---
Nitpick comments:
In `@api/v1/api.gen.go`:
- Around line 522-530: The generated type AgentSessionWithState duplicates
fields from AgentSessionState (HasPendingPrompt, Model, Working, TotalCost);
update the OpenAPI spec (api.yaml) to define AgentSessionWithState using allOf
composition so it composes AgentSession and AgentSessionState instead of
redeclaring state fields—i.e., replace the independent schema for
AgentSessionWithState with an allOf that references both AgentSession and
AgentSessionState so the generator will produce a single combined struct and
avoid drift.
In `@internal/agent/api.go`:
- Around line 811-821: When cancelling stuck sessions call out and log any error
returned by mgr.Cancel instead of discarding it: capture the error from
mgr.Cancel(context.Background()) and emit a.debug-level log via a.logger.Debug
(or similar) including the session id and the error; keep the existing
a.logger.Warn("Cancelled stuck session", "session_id", id) but add the debug log
for the error path. Also add a short code comment near the cleanup logic (around
IsWorking/LastActivity checks and the sessions map) noting that cancelled
sessions remain in the sessions map until the next cleanup cycle so they can be
viewed/reactivated, to make the behavior explicit for future readers.
In `@internal/agent/session.go`:
- Around line 176-188: The broadcast currently captures working-state fields
(via updateWorkingState in SetWorking) and then calls HasPendingPrompt() outside
the same lock, causing a non-atomic snapshot; to fix, ensure HasPendingPrompt is
read under the same mutex or returned alongside other state: either move the
HasPendingPrompt() call inside the sm.mu-protected section (or modify
updateWorkingState to compute and return hasPendingPrompt with
id/model/totalCost) and then use that single atomic value when constructing
StreamResponse/SessionState before sm.subpub.Broadcast and callback.
In `@internal/cmn/config/definition.go`:
- Around line 299-302: Document the sentinel behavior for unlimited session
storage on SessionDef.MaxPerUser: update the comment on the SessionDef struct
(and specifically the MaxPerUser field) to state whether a value of 0 (or nil)
is treated as "unlimited" and what the default semantics are (e.g., "Default:
100; 0 = unlimited"). Reference the SessionDef type and MaxPerUser field so
future readers know the exact sentinel meaning.
In `@internal/cmn/schema/config.schema.json`:
- Around line 755-766: The JSON schema for SessionDef documents a default of 100
for the max_per_user property only in prose; update the SessionDef -> properties
-> max_per_user definition to include a "default": 100 so schema-aware editors
can surface the default value (target the "SessionDef" object and its
"max_per_user" property).
In `@internal/persis/filesession/store.go`:
- Around line 377-415: deleteSessionLocked currently does disk I/O (calling
loadSessionFromFile and os.Remove) while holding the store write lock, which can
block concurrent operations; to fix this, change the flow in deleteSessionLocked
(and helpers like removeSessionFromUserIndex) to minimize time under the lock
by: (1) acquire the lock only to check and remove in-memory indexes (s.byID,
s.updatedAt, s.byParent) and capture the filePath plus any metadata needed
(userID, parentID), (2) release the lock, perform loadSessionFromFile and
os.Remove outside the lock (or perform os.Remove in a background goroutine if
you prefer async cleanup), and (3) if loadSessionFromFile is required to compute
index removals, load the file before taking the lock and then revalidate
existence under lock to avoid races; reference deleteSessionLocked,
loadSessionFromFile, os.Remove and s.byID/s.byParent when implementing this
change.
Comment on lines
1208
to
1277
| func TestAPI_CleanupIdleSessions_CancelsStuckSession(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| api := NewAPI(APIConfig{ | ||
| ConfigStore: newMockConfigStore(true), | ||
| WorkingDir: t.TempDir(), | ||
| }) | ||
|
|
||
| mgr := NewSessionManager(SessionManagerConfig{ID: "stuck-sess"}) | ||
| mgr.mu.Lock() | ||
| mgr.working = true | ||
| mgr.lastHeartbeat = time.Now().Add(-1 * time.Minute) // stale heartbeat | ||
| mgr.lastActivity = time.Now() // recent activity | ||
| mgr.mu.Unlock() | ||
|
|
||
| api.sessions.Store("stuck-sess", mgr) | ||
|
|
||
| api.cleanupIdleSessions() | ||
|
|
||
| // Session should have been cancelled (working set to false) | ||
| assert.False(t, mgr.IsWorking(), "stuck session should be cancelled") | ||
| } | ||
|
|
||
| func TestAPI_CleanupIdleSessions_DoesNotCancelHealthyWorkingSession(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| api := NewAPI(APIConfig{ | ||
| ConfigStore: newMockConfigStore(true), | ||
| WorkingDir: t.TempDir(), | ||
| }) | ||
|
|
||
| mgr := NewSessionManager(SessionManagerConfig{ID: "healthy-sess"}) | ||
| mgr.mu.Lock() | ||
| mgr.working = true | ||
| mgr.lastHeartbeat = time.Now() // fresh heartbeat | ||
| mgr.lastActivity = time.Now() | ||
| mgr.mu.Unlock() | ||
|
|
||
| api.sessions.Store("healthy-sess", mgr) | ||
|
|
||
| api.cleanupIdleSessions() | ||
|
|
||
| // Session should still be working | ||
| _, exists := api.sessions.Load("healthy-sess") | ||
| assert.True(t, exists, "healthy working session should not be removed") | ||
| } | ||
|
|
||
| func TestAPI_CleanupIdleSessions_DoesNotCancelZeroHeartbeat(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| api := NewAPI(APIConfig{ | ||
| ConfigStore: newMockConfigStore(true), | ||
| WorkingDir: t.TempDir(), | ||
| }) | ||
|
|
||
| // Working session with zero heartbeat (loop hasn't started heartbeating yet) | ||
| mgr := NewSessionManager(SessionManagerConfig{ID: "no-hb-sess"}) | ||
| mgr.mu.Lock() | ||
| mgr.working = true | ||
| mgr.lastActivity = time.Now() | ||
| mgr.mu.Unlock() | ||
|
|
||
| api.sessions.Store("no-hb-sess", mgr) | ||
|
|
||
| api.cleanupIdleSessions() | ||
|
|
||
| // Should not be cancelled because lastHeartbeat is zero | ||
| _, exists := api.sessions.Load("no-hb-sess") | ||
| assert.True(t, exists, "session with zero heartbeat should not be cancelled") | ||
| } |
There was a problem hiding this comment.
Use require for assertions in the new cleanup tests.
These subtests rely on assertions that should fail fast; switch to require.* for consistency and to match project conventions.
As per coding guidelines, "Use stretchr/testify/require for assertions and shared fixtures from internal/test instead of duplicating mocks".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/agent/api_test.go` around lines 1208 - 1277, Update the three tests
TestAPI_CleanupIdleSessions_CancelsStuckSession,
TestAPI_CleanupIdleSessions_DoesNotCancelHealthyWorkingSession, and
TestAPI_CleanupIdleSessions_DoesNotCancelZeroHeartbeat to use require.* (from
stretchr/testify/require) instead of assert.* so failures fail fast; replace
assert.False/True and assert calls with the corresponding
require.False/True/NoError as appropriate, and stop duplicating mocks by using
the shared fixture from internal/test for the config store instead of
newMockConfigStore (i.e., replace newMockConfigStore(true) with the
project-provided mock from internal/test or its helper function), keeping
references to NewAPI, SessionManager, api.cleanupIdleSessions, and the session
IDs unchanged.
Comment on lines
89
to
96
| <div className="flex items-center gap-1.5"> | ||
| {sess.has_pending_prompt ? ( | ||
| <span className="h-2 w-2 rounded-full bg-orange-400 flex-shrink-0" title="Waiting for input" /> | ||
| ) : sess.working ? ( | ||
| <span className="h-2 w-2 rounded-full bg-green-500 flex-shrink-0" title="Running" /> | ||
| ) : null} | ||
| {formatDate(sess.session.created_at)} | ||
| </div> |
There was a problem hiding this comment.
Status dots rely on color alone — title on <span> isn't reliably announced by screen readers.
title on a non-interactive, non-focusable element is inconsistently announced across VoiceOver / NVDA / JAWS. AT users get no indication of the "Waiting for input" / "Running" state. Adding role="img" + aria-label (or a visually-hidden sibling) on each dot resolves this without layout impact.
♿ Proposed fix
- {sess.has_pending_prompt ? (
- <span className="h-2 w-2 rounded-full bg-orange-400 flex-shrink-0" title="Waiting for input" />
- ) : sess.working ? (
- <span className="h-2 w-2 rounded-full bg-green-500 flex-shrink-0" title="Running" />
- ) : null}
+ {sess.has_pending_prompt ? (
+ <span className="h-2 w-2 rounded-full bg-orange-400 flex-shrink-0" role="img" aria-label="Waiting for input" />
+ ) : sess.working ? (
+ <span className="h-2 w-2 rounded-full bg-green-500 flex-shrink-0" role="img" aria-label="Running" />
+ ) : null}📝 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.
Suggested change
| <div className="flex items-center gap-1.5"> | |
| {sess.has_pending_prompt ? ( | |
| <span className="h-2 w-2 rounded-full bg-orange-400 flex-shrink-0" title="Waiting for input" /> | |
| ) : sess.working ? ( | |
| <span className="h-2 w-2 rounded-full bg-green-500 flex-shrink-0" title="Running" /> | |
| ) : null} | |
| {formatDate(sess.session.created_at)} | |
| </div> | |
| <div className="flex items-center gap-1.5"> | |
| {sess.has_pending_prompt ? ( | |
| <span className="h-2 w-2 rounded-full bg-orange-400 flex-shrink-0" role="img" aria-label="Waiting for input" /> | |
| ) : sess.working ? ( | |
| <span className="h-2 w-2 rounded-full bg-green-500 flex-shrink-0" role="img" aria-label="Running" /> | |
| ) : null} | |
| {formatDate(sess.session.created_at)} | |
| </div> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/features/agent/components/AgentChatModalHeader.tsx` around lines 89 -
96, The status indicator spans in AgentChatModalHeader.tsx (the conditional
<span> elements showing bg-orange-400 and bg-green-500 with title attributes)
rely on color and title which aren’t reliably announced by screen readers;
update those spans to include an accessible name by adding role="img" and a
meaningful aria-label (e.g., "Waiting for input" and "Running") or alternatively
render a visually-hidden text sibling with those labels, ensuring the accessible
text reflects sess.has_pending_prompt and sess.working states so assistive tech
can announce the status.
Collaborator
Author
|
@coderabbitai full review please |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/src/features/agent/components/AgentChatModalHeader.tsx (1)
18-38:⚠️ Potential issue | 🟡 Minor
sessionIdis defined inPropsbut never destructured or used in the component.The
sessionIdprop (Line 19) is passed fromAgentChatModal(Line 175 ofAgentChatModal.tsx) but not destructured in the function signature and not referenced anywhere in the component body. Remove it from the type and the call site to avoid confusion.Suggested fix
type Props = { - sessionId: string | null; totalCost?: number; isSidebarOpen: boolean;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/features/agent/components/AgentChatModalHeader.tsx` around lines 18 - 38, Remove the unused sessionId prop: update the Props type by deleting the sessionId field and remove sessionId from the call site that passes it into AgentChatModalHeader (search for AgentChatModalHeader in AgentChatModal to find the invocation). Ensure no other references to sessionId exist in the AgentChatModalHeader component (function name AgentChatModalHeader and its props destructuring) and adjust any related typings or imports if necessary.
🧹 Nitpick comments (14)
internal/upgrade/upgrade_test.go (1)
1644-1651: Consider usingSetPreRequestHookfor more explicit URL redirection.The current
OnBeforeRequestapproach mutatesreq.URLto redirect requests to the test server. While this works in practice with resty v2, the approach relies on behavior that isn't explicitly guaranteed by resty's public API contract. Per resty v2 documentation, user-defined middlewares run before Resty's internal request preparation, and Resty's default middleware may perform additional URL normalization after your hook completes.A more explicit and maintainable approach is to use
SetPreRequestHook, which runs immediately before the HTTP request is sent and gives direct access to the final*http.Request:func newTestGitHubClient(serverURL string) *GitHubClient { client := NewGitHubClient() client.client.SetPreRequestHook(func(_ *http.Client, req *http.Request) error { req.URL.Scheme = "http" req.URL.Host = strings.TrimPrefix(serverURL, "http://") req.URL.Path = strings.Replace(req.URL.Path, "/repos/dagu-org/dagu/releases", "/releases", 1) return nil }) return client }This makes the redirection intent clear and eliminates reliance on internal Resty behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/upgrade/upgrade_test.go` around lines 1644 - 1651, newTestGitHubClient currently uses client.client.OnBeforeRequest to mutate resty.Request.URL which relies on Resty's internal request prep; change this to use client.client.SetPreRequestHook so you operate on the final *http.Request before send. Update newTestGitHubClient to replace the OnBeforeRequest hook with a SetPreRequestHook callback that adjusts req.URL.Scheme, req.URL.Host (trim serverURL prefix), and rewrites req.URL.Path (replace "/repos/dagu-org/dagu/releases" with "/releases") so test requests are explicitly redirected to the test server; keep the function name newTestGitHubClient and the GitHubClient instance usage the same.ui/src/features/agent/components/messages/SubAgentChips.tsx (1)
37-45: Disambiguate fallback delegate matching when tasks repeat.
Matching by task text can pick the wrong delegate if multiple tasks share the same description. Consider only enabling the fallback when there’s a unique match (or prefer a deterministic tie‑breaker like most recent).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/features/agent/components/messages/SubAgentChips.tsx` around lines 37 - 45, The current fallback assigns a delegate by matching task text which can return the wrong delegate when multiple delegates share the same task; update the fallback in SubAgentChips.tsx (the delegateId / delegateStatuses / delegate variables) to only accept a match when Object.values(delegateStatuses).filter(d => d.task === task) yields a single result, and if multiple matches exist apply a deterministic tie-breaker (e.g., pick the delegate with the latest activity timestamp property such as d.lastSeen, d.updatedAt or d.last_active if available); if no timestamp field exists, do not fall back to a match to avoid ambiguity.api/v1/api.yaml (1)
7876-7911: Consider a default forhasPendingPromptto aid generated clients.
If the field is always present, adefault: false(or making it required) keeps client models predictable.📝 Optional schema tweak
hasPendingPrompt: type: boolean + default: false description: "Whether the agent is waiting for user input"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v1/api.yaml` around lines 7876 - 7911, Add a default boolean for hasPendingPrompt (or make it required) in the AgentSessionState schema so generated clients get a predictable value; specifically update the hasPendingPrompt property in the AgentSessionState definition to include "default: false" (or add "hasPendingPrompt" to the required list) to ensure AgentSessionState / AgentSessionWithState consumers receive a consistent value.api/v1/api.gen.go (1)
510-534:AgentSessionWithState.SessionIdis redundant and shared field duplication exists acrossAgentSessionStateandAgentSessionWithState
AgentSessionWithStatealready carriesSession(of typeAgentSession), which has anIdfield. The separateSessionIdfield at line 531 duplicates this data (confirmed in the mapping atagent_sessions.goline 262 whereSessionId: s.Session.ID).Additionally, both
AgentSessionState(lines 512–521) andAgentSessionWithState(lines 524–536) declare duplicate fields:HasPendingPrompt,Model,SessionId,TotalCost, andWorking. This schema-level duplication should be resolved inapi.yaml. Since this file is generated, any fix must be applied to the source spec.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v1/api.gen.go` around lines 510 - 534, The generated structs duplicate session and state fields: AgentSessionWithState declares a redundant SessionId (AgentSessionWithState.SessionId) even though Session (type AgentSession) already contains ID, and AgentSessionState and AgentSessionWithState repeat fields like HasPendingPrompt, Model, TotalCost, and Working. Fix the source OpenAPI spec (api.yaml) to remove duplicated properties — remove SessionId from AgentSessionWithState and consolidate the shared state fields into a single reusable schema (e.g., reference AgentSessionState from AgentSessionWithState or embed AgentSession) so the generator produces one canonical definition; then regenerate api.gen.go and ensure mapping code that assigns SessionId (mapping that uses s.Session.ID) is updated or removed to match the new generated shape.internal/agent/session_test.go (1)
852-863: Userequirefor the initial-state precondition assertion.Line 856 (
assert.True(t, sm.LastHeartbeat().IsZero())) establishes a precondition: ifLastHeartbeatis unexpectedly non-zero beforeRecordHeartbeatis called, the subsequent update assertion is meaningless. Arequirewould fail fast and surface the root cause immediately. As per coding guidelines,requireshould be preferred for assertions.♻️ Proposed fix
- assert.True(t, sm.LastHeartbeat().IsZero()) + require.True(t, sm.LastHeartbeat().IsZero()) sm.RecordHeartbeat() hb := sm.LastHeartbeat() - assert.False(t, hb.IsZero()) + require.False(t, hb.IsZero()) assert.WithinDuration(t, time.Now(), hb, time.Second)Based on learnings: "Use
stretchr/testify/requirefor assertions and shared fixtures frominternal/testinstead of duplicating mocks."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/agent/session_test.go` around lines 852 - 863, Replace the non-fatal initial-state assertion with a fatal one so the test fails fast: in the "updates lastHeartbeat" subtest, change the precondition check that uses assert.True(t, sm.LastHeartbeat().IsZero()) to use require.True from stretchr/testify/require (the test constructs sm via NewSessionManager(SessionManagerConfig{ID: "hb-test"}) and then calls sm.RecordHeartbeat(); keep subsequent checks unchanged for LastHeartbeat() and WithinDuration assertions).internal/cmn/config/loader_test.go (1)
167-167: LGTM! — optional: add a test for non-defaultsession.max_per_user.Both
TestLoad_EnvandTestLoad_YAMLcorrectly assert the default of 100. Consider adding a sub-case (similar toTestLoad_Audit) that verifies overridingsession.max_per_uservia YAML and its corresponding env var, and that zero (unlimited) passes validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cmn/config/loader_test.go` at line 167, Add a sub-case to the existing tests (modeled after TestLoad_Audit) that verifies overriding session.max_per_user via YAML and via the corresponding environment variable and that zero (meaning unlimited) is accepted by validation; specifically, update TestLoad_YAML and/or TestLoad_Env to include an entry where SessionConfig{MaxPerUser: <non-default>} is set in the YAML input and a separate env-driven case that sets the session.max_per_user env var, then assert the loaded config's SessionConfig.MaxPerUser equals the overridden value and that when set to 0 validation passes.ui/src/features/agent/types.ts (1)
91-105:has_pending_promptshould be requiredboolean, not optional.The Go counterpart (
HasPendingPrompt boolwithoutomitempty) always serializes this field, just likeworking. Making it optional in TypeScript is inconsistent withworking: booleanand misleads consumers into guarding againstundefinedwhen the value is always present.♻️ Proposed fix
export interface SessionState { session_id: string; working: boolean; - has_pending_prompt?: boolean; + has_pending_prompt: boolean; model?: string; total_cost?: number; } export interface SessionWithState { session: Session; working: boolean; - has_pending_prompt?: boolean; + has_pending_prompt: boolean; model?: string; total_cost?: number; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/features/agent/types.ts` around lines 91 - 105, The has_pending_prompt property is optional in both SessionState and SessionWithState but should be a required boolean (like working) to match the Go counterpart; update both interfaces (SessionState and SessionWithState) to change has_pending_prompt?: boolean to has_pending_prompt: boolean so consumers no longer need to guard for undefined and the TypeScript types align with the serialized Go shape.ui/src/features/agent/components/SessionSidebar.tsx (2)
32-41: IntersectionObserver may fireonLoadMorerepeatedly before new data arrives.When the sentinel is visible and
onLoadMoretriggers a fetch, the observer can fire again on the next animation frame if the sentinel remains in view (e.g., the new items haven't rendered yet). Consider adding a loading guard or disconnecting the observer after firing until the next page loads.A simple approach is to accept an
isLoadingprop:Suggested guard
useEffect(() => { const el = sentinelRef.current; - if (!el || !hasMore) return; + if (!el || !hasMore || isLoading) return; const observer = new IntersectionObserver( ([entry]) => { if (entry.isIntersecting) onLoadMore(); }, { threshold: 0.1 } );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/features/agent/components/SessionSidebar.tsx` around lines 32 - 41, The IntersectionObserver in the useEffect for sentinelRef can call onLoadMore repeatedly while new items are still loading; update the logic in the component (the useEffect that references sentinelRef, hasMore, onLoadMore) to accept an isLoading prop and use it as a guard (do not call onLoadMore if isLoading is true) and/or disconnect the observer immediately after it fires until loading completes; ensure isLoading is added to the useEffect dependency array so the observer is re-established when loading finishes and new pages can be fetched.
75-93: Mobile overlay lacks keyboard dismissal.The overlay
<div>handlesonClickfor closing, but there's noonKeyDownhandler for keyboard accessibility (e.g., pressing Escape to close). Consider adding an Escape key listener or wrapping in a dialog pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/features/agent/components/SessionSidebar.tsx` around lines 75 - 93, The mobile overlay in SessionSidebar (when isMobile is true) only closes via the backdrop's onClick and lacks keyboard accessibility; update the backdrop <div> (the element rendering the overlay in SessionSidebar.tsx) to also handle Escape key presses by adding an onKeyDown handler that calls onClose when event.key === 'Escape' (and ensure the div is focusable, e.g., tabIndex={0}, or alternatively add a global keydown listener in useEffect that watches for Escape and invokes onClose), so the overlay can be dismissed with the keyboard as well as mouse.internal/agent/session.go (1)
192-208: Consider a result struct for the 6-tuple return ofupdateWorkingState.The function returns
(string, string, float64, bool, func(string, bool), bool)which is hard to read at call sites. A named struct would make this self-documenting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/agent/session.go` around lines 192 - 208, The updateWorkingState method returns a cryptic 6-tuple (string, string, float64, bool, func(string, bool), bool); introduce a small result struct (e.g., type WorkingStateResult struct { ID string; Model string; TotalCost float64; HasPending bool; OnWorkingChange func(string,bool); Changed bool }) and change updateWorkingState to return that struct instead of the tuple; update all call sites that destructure the tuple to use the struct fields and adjust any named returns or tests accordingly so callers read like result.ID/result.HasPending rather than positional values, keeping the locking and logic in SessionManager.updateWorkingState unchanged.ui/src/features/agent/components/AgentChatModal.tsx (1)
131-142:value === 'new'branch inhandleSelectSessionappears unreachable.
SessionSidebaronly callsonSelectSessionwith actual session IDs. The'new'case (Line 133) looks like a remnant from the removed in-header session dropdown. Consider removing it to reduce confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/features/agent/components/AgentChatModal.tsx` around lines 131 - 142, handleSelectSession contains an unreachable branch checking for value === 'new' (legacy from removed in-header dropdown); remove that branch and simplify handleSelectSession to always call selectSession(value).catch(...) so it no longer references 'new' or clearSession; update the dependency array if any unused references remain (remove clearSession if it is no longer used) and ensure SessionSidebar's onSelectSession usage still maps to handleSelectSession and error handling via setError remains intact.internal/agent/api_test.go (1)
1336-1405: Cleanup tests properly userequire.*and cover heartbeat edge cases.The three new tests cover the essential stuck-session scenarios (stale heartbeat → cancel, fresh heartbeat → preserve, zero heartbeat → skip). The assertions use
require.*as per project conventions.One minor note: the stuck heartbeat test (Line 1347) hardcodes
time.Now().Add(-1 * time.Minute). This implicitly depends onstuckHeartbeatTimeoutbeing less than 1 minute. Consider using the constant directly to make the test self-documenting:Suggestion
- mgr.lastHeartbeat = time.Now().Add(-1 * time.Minute) // stale heartbeat + mgr.lastHeartbeat = time.Now().Add(-(stuckHeartbeatTimeout + time.Second)) // stale heartbeat🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/agent/api_test.go` around lines 1336 - 1405, The test TestAPI_CleanupIdleSessions_CancelsStuckSession currently hardcodes time.Now().Add(-1 * time.Minute) for mgr.lastHeartbeat which assumes stuckHeartbeatTimeout < 1 minute; update the test to compute the stale heartbeat relative to the actual stuckHeartbeatTimeout constant (e.g., time.Now().Add(-stuckHeartbeatTimeout - time.Second)) so the test is self-documenting and robust to changes — modify the assignment to mgr.lastHeartbeat in that test to use stuckHeartbeatTimeout instead of the fixed 1 minute value.internal/agent/api.go (1)
646-661: All persisted sessions are loaded into memory for pagination.
a.store.ListSessionsfetches every session for the user, only to slice a single page. This works at small scale but degrades with many sessions. Consider adding aCountSessionsmethod to the store and aListSessionsPaged(ctx, userID, offset, limit)method for efficient query-level pagination.This can be deferred but is worth tracking as the feature matures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/agent/api.go` around lines 646 - 661, The current code calls a.store.ListSessions and loads all sessions into memory for pagination; replace this with paginated store calls: add CountSessions(ctx, userID) to get total session count and ListSessionsPaged(ctx, userID, offset, limit) to fetch only the page needed, then iterate the returned page results (filtering out sess.ParentSessionID != "" and skipping IDs present in activeIDs) and append SessionWithState entries to combined; update the call site that uses a.store.ListSessions and any callers to compute offset/limit for the requested page and fallback to ListSessionsPaged, and implement the new methods on the concrete store(s).ui/src/features/agent/hooks/useAgentChat.ts (1)
176-176: Consider extracting the magic number30to a named constant.
perPage: 30is an inline magic number with no documented rationale. Naming it (e.g.SESSIONS_PER_PAGE) would make it trivially adjustable and self-documenting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/src/features/agent/hooks/useAgentChat.ts` at line 176, Extract the magic number 30 used in the params object inside the useAgentChat hook into a named constant (e.g., SESSIONS_PER_PAGE) and replace perPage: 30 with perPage: SESSIONS_PER_PAGE; declare the constant near the top of the file or just above the useAgentChat function so it’s easy to find and adjust, and add a short comment if needed to document its purpose.
🤖 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/agent/api.go`:
- Around line 635-668: ListSessionsPaginated currently appends unordered active
sessions (from collectActiveSessions) and persisted sessions then slices for
pagination, causing non-deterministic pages; after building combined (slice of
SessionWithState) sort it deterministically (use sort.SliceStable) by
Session.UpdatedAt descending (most recent first) so both active and persisted
sessions are ordered consistently before computing start/end and returning
exec.NewPaginatedResult; update ListSessionsPaginated to perform this stable
sort on combined prior to slicing.
In `@internal/persis/filesession/store_test.go`:
- Around line 1484-1653: Tests in TestCreateSession_EnforcesMaxPerUser,
TestCreateSession_MaxPerUser_SubSessionsNotCounted,
TestCreateSession_MaxPerUser_DeletesSubSessionsWithParent,
TestCreateSession_MaxPerUser_ZeroMeansUnlimited, and
TestCreateSession_MaxPerUser_MultipleUsers use assert.* but must use require.*;
update all assert.ErrorIs, assert.NoError, assert.Len, and assert.Equal calls to
their require equivalents (e.g., require.ErrorIs, require.NoError, require.Len,
require.Equal) around the GetSession and ListSessions checks and any other
assertions in those test functions so failures abort the test immediately.
In `@internal/service/frontend/api/v1/agent_sessions_test.go`:
- Around line 53-127: Replace the local mockSessionStore with the shared fixture
in internal/test (or import the existing mock) to avoid duplication: remove the
type mockSessionStore and its methods (CreateSession, GetSession, ListSessions,
UpdateSession, DeleteSession, AddMessage, GetMessages, GetLatestSequenceID,
ListSubSessions) from this test file and import/instantiate the shared
SessionStore mock from internal/test; ensure the test uses the same exported
constructor or variable name provided by internal/test and update any references
to m or its methods accordingly so tests compile and use the centralized
fixture.
- Around line 156-162: Replace all usages of assert.* with require.* in this
test file so assertions fail fast and follow guidelines; for example change
assert.Empty(t, listResp.Sessions) to require.Empty(t, listResp.Sessions) and
change assert.Equal(t, 0, listResp.Pagination.TotalRecords), assert.Equal(t, 1,
listResp.Pagination.CurrentPage), assert.Equal(t, 1,
listResp.Pagination.TotalPages) to require.Equal(...) respectively; update any
other assert.* calls (there are ~39) to require.* and ensure the test
imports/uses github.com/stretchr/testify/require (and remove the unused assert
import if present).
- Around line 137-152: The test contains invalid pointer construction and an
unsupported directive: remove the `//go:fix inline` directive and replace the
helper `func ptrInt(v int) *int { return new(v) }` with a proper
address-returning form (e.g. return &v) and update all call sites in
`TestListAgentSessions` (and other tests) that use `new(1)`/`new(10)` to use
`ptrInt(1)`/`ptrInt(10)` or `&v` patterns; also change all `assert.*` assertions
to `require.*` in this test file and relocate locally-defined mocks
(`mockSessionStore`, `mockAgentConfigStore`) into `internal/test` for reuse,
adjusting imports accordingly.
In `@ui/src/features/agent/hooks/useAgentChat.ts`:
- Around line 195-198: The loadMoreSessions function has a race where concurrent
calls read stale hasMoreSessions/sessionPage and fetch the same page twice; fix
it by adding a ref-based in-flight guard (e.g., inFlightLoadMoreRef) and a
nextPage ref (or update nextPage from sessionPage once, e.g., const nextPage =
sessionPage + 1) so only one caller proceeds: check hasMoreSessions AND
!inFlightLoadMoreRef.current, set inFlightLoadMoreRef.current = true before
calling fetchSessionsPage(nextPage), await the fetch, then in finally set
inFlightLoadMoreRef.current = false and update sessionPage/state via the
existing appendSessions flow to avoid duplicate appends; reference
loadMoreSessions, fetchSessionsPage, hasMoreSessions, sessionPage,
appendSessions.
In `@ui/src/features/agent/utils/formatDate.ts`:
- Around line 1-7: The formatDate function currently calls new
Date(dateStr).toLocaleString(...) and will render "Invalid Date" for empty or
malformed inputs; update formatDate to construct the Date object, check its
validity with isNaN(date.getTime()) (or Number.isNaN(date.valueOf())), and if
invalid return an empty string (or a clear fallback) instead of calling
toLocaleString; keep the existing options and behavior for valid dates so only
the Date validity guard around new Date(dateStr) and the toLocaleString call
needs to be added.
---
Outside diff comments:
In `@ui/src/features/agent/components/AgentChatModalHeader.tsx`:
- Around line 18-38: Remove the unused sessionId prop: update the Props type by
deleting the sessionId field and remove sessionId from the call site that passes
it into AgentChatModalHeader (search for AgentChatModalHeader in AgentChatModal
to find the invocation). Ensure no other references to sessionId exist in the
AgentChatModalHeader component (function name AgentChatModalHeader and its props
destructuring) and adjust any related typings or imports if necessary.
---
Nitpick comments:
In `@api/v1/api.gen.go`:
- Around line 510-534: The generated structs duplicate session and state fields:
AgentSessionWithState declares a redundant SessionId
(AgentSessionWithState.SessionId) even though Session (type AgentSession)
already contains ID, and AgentSessionState and AgentSessionWithState repeat
fields like HasPendingPrompt, Model, TotalCost, and Working. Fix the source
OpenAPI spec (api.yaml) to remove duplicated properties — remove SessionId from
AgentSessionWithState and consolidate the shared state fields into a single
reusable schema (e.g., reference AgentSessionState from AgentSessionWithState or
embed AgentSession) so the generator produces one canonical definition; then
regenerate api.gen.go and ensure mapping code that assigns SessionId (mapping
that uses s.Session.ID) is updated or removed to match the new generated shape.
In `@api/v1/api.yaml`:
- Around line 7876-7911: Add a default boolean for hasPendingPrompt (or make it
required) in the AgentSessionState schema so generated clients get a predictable
value; specifically update the hasPendingPrompt property in the
AgentSessionState definition to include "default: false" (or add
"hasPendingPrompt" to the required list) to ensure AgentSessionState /
AgentSessionWithState consumers receive a consistent value.
In `@internal/agent/api_test.go`:
- Around line 1336-1405: The test
TestAPI_CleanupIdleSessions_CancelsStuckSession currently hardcodes
time.Now().Add(-1 * time.Minute) for mgr.lastHeartbeat which assumes
stuckHeartbeatTimeout < 1 minute; update the test to compute the stale heartbeat
relative to the actual stuckHeartbeatTimeout constant (e.g.,
time.Now().Add(-stuckHeartbeatTimeout - time.Second)) so the test is
self-documenting and robust to changes — modify the assignment to
mgr.lastHeartbeat in that test to use stuckHeartbeatTimeout instead of the fixed
1 minute value.
In `@internal/agent/api.go`:
- Around line 646-661: The current code calls a.store.ListSessions and loads all
sessions into memory for pagination; replace this with paginated store calls:
add CountSessions(ctx, userID) to get total session count and
ListSessionsPaged(ctx, userID, offset, limit) to fetch only the page needed,
then iterate the returned page results (filtering out sess.ParentSessionID != ""
and skipping IDs present in activeIDs) and append SessionWithState entries to
combined; update the call site that uses a.store.ListSessions and any callers to
compute offset/limit for the requested page and fallback to ListSessionsPaged,
and implement the new methods on the concrete store(s).
In `@internal/agent/session_test.go`:
- Around line 852-863: Replace the non-fatal initial-state assertion with a
fatal one so the test fails fast: in the "updates lastHeartbeat" subtest, change
the precondition check that uses assert.True(t, sm.LastHeartbeat().IsZero()) to
use require.True from stretchr/testify/require (the test constructs sm via
NewSessionManager(SessionManagerConfig{ID: "hb-test"}) and then calls
sm.RecordHeartbeat(); keep subsequent checks unchanged for LastHeartbeat() and
WithinDuration assertions).
In `@internal/agent/session.go`:
- Around line 192-208: The updateWorkingState method returns a cryptic 6-tuple
(string, string, float64, bool, func(string, bool), bool); introduce a small
result struct (e.g., type WorkingStateResult struct { ID string; Model string;
TotalCost float64; HasPending bool; OnWorkingChange func(string,bool); Changed
bool }) and change updateWorkingState to return that struct instead of the
tuple; update all call sites that destructure the tuple to use the struct fields
and adjust any named returns or tests accordingly so callers read like
result.ID/result.HasPending rather than positional values, keeping the locking
and logic in SessionManager.updateWorkingState unchanged.
In `@internal/cmn/config/loader_test.go`:
- Line 167: Add a sub-case to the existing tests (modeled after TestLoad_Audit)
that verifies overriding session.max_per_user via YAML and via the corresponding
environment variable and that zero (meaning unlimited) is accepted by
validation; specifically, update TestLoad_YAML and/or TestLoad_Env to include an
entry where SessionConfig{MaxPerUser: <non-default>} is set in the YAML input
and a separate env-driven case that sets the session.max_per_user env var, then
assert the loaded config's SessionConfig.MaxPerUser equals the overridden value
and that when set to 0 validation passes.
In `@internal/upgrade/upgrade_test.go`:
- Around line 1644-1651: newTestGitHubClient currently uses
client.client.OnBeforeRequest to mutate resty.Request.URL which relies on
Resty's internal request prep; change this to use
client.client.SetPreRequestHook so you operate on the final *http.Request before
send. Update newTestGitHubClient to replace the OnBeforeRequest hook with a
SetPreRequestHook callback that adjusts req.URL.Scheme, req.URL.Host (trim
serverURL prefix), and rewrites req.URL.Path (replace
"/repos/dagu-org/dagu/releases" with "/releases") so test requests are
explicitly redirected to the test server; keep the function name
newTestGitHubClient and the GitHubClient instance usage the same.
In `@ui/src/features/agent/components/AgentChatModal.tsx`:
- Around line 131-142: handleSelectSession contains an unreachable branch
checking for value === 'new' (legacy from removed in-header dropdown); remove
that branch and simplify handleSelectSession to always call
selectSession(value).catch(...) so it no longer references 'new' or
clearSession; update the dependency array if any unused references remain
(remove clearSession if it is no longer used) and ensure SessionSidebar's
onSelectSession usage still maps to handleSelectSession and error handling via
setError remains intact.
In `@ui/src/features/agent/components/messages/SubAgentChips.tsx`:
- Around line 37-45: The current fallback assigns a delegate by matching task
text which can return the wrong delegate when multiple delegates share the same
task; update the fallback in SubAgentChips.tsx (the delegateId /
delegateStatuses / delegate variables) to only accept a match when
Object.values(delegateStatuses).filter(d => d.task === task) yields a single
result, and if multiple matches exist apply a deterministic tie-breaker (e.g.,
pick the delegate with the latest activity timestamp property such as
d.lastSeen, d.updatedAt or d.last_active if available); if no timestamp field
exists, do not fall back to a match to avoid ambiguity.
In `@ui/src/features/agent/components/SessionSidebar.tsx`:
- Around line 32-41: The IntersectionObserver in the useEffect for sentinelRef
can call onLoadMore repeatedly while new items are still loading; update the
logic in the component (the useEffect that references sentinelRef, hasMore,
onLoadMore) to accept an isLoading prop and use it as a guard (do not call
onLoadMore if isLoading is true) and/or disconnect the observer immediately
after it fires until loading completes; ensure isLoading is added to the
useEffect dependency array so the observer is re-established when loading
finishes and new pages can be fetched.
- Around line 75-93: The mobile overlay in SessionSidebar (when isMobile is
true) only closes via the backdrop's onClick and lacks keyboard accessibility;
update the backdrop <div> (the element rendering the overlay in
SessionSidebar.tsx) to also handle Escape key presses by adding an onKeyDown
handler that calls onClose when event.key === 'Escape' (and ensure the div is
focusable, e.g., tabIndex={0}, or alternatively add a global keydown listener in
useEffect that watches for Escape and invokes onClose), so the overlay can be
dismissed with the keyboard as well as mouse.
In `@ui/src/features/agent/hooks/useAgentChat.ts`:
- Line 176: Extract the magic number 30 used in the params object inside the
useAgentChat hook into a named constant (e.g., SESSIONS_PER_PAGE) and replace
perPage: 30 with perPage: SESSIONS_PER_PAGE; declare the constant near the top
of the file or just above the useAgentChat function so it’s easy to find and
adjust, and add a short comment if needed to document its purpose.
In `@ui/src/features/agent/types.ts`:
- Around line 91-105: The has_pending_prompt property is optional in both
SessionState and SessionWithState but should be a required boolean (like
working) to match the Go counterpart; update both interfaces (SessionState and
SessionWithState) to change has_pending_prompt?: boolean to has_pending_prompt:
boolean so consumers no longer need to guard for undefined and the TypeScript
types align with the serialized Go shape.
Comment on lines
+635
to
+668
| // ListSessionsPaginated returns a paginated list of sessions for the given user. | ||
| // Active sessions appear first, followed by persisted inactive sessions. | ||
| func (a *API) ListSessionsPaginated(ctx context.Context, userID string, page, perPage int) exec.PaginatedResult[SessionWithState] { | ||
| pg := exec.NewPaginator(page, perPage) | ||
|
|
||
| activeIDs := make(map[string]struct{}) | ||
| activeSessions := a.collectActiveSessions(userID, activeIDs) | ||
|
|
||
| combined := make([]SessionWithState, 0, len(activeSessions)) | ||
| combined = append(combined, activeSessions...) | ||
|
|
||
| if a.store != nil { | ||
| persisted, err := a.store.ListSessions(ctx, userID) | ||
| if err != nil { | ||
| a.logger.Warn("Failed to list persisted sessions", "error", err) | ||
| } else { | ||
| for _, sess := range persisted { | ||
| if _, exists := activeIDs[sess.ID]; exists { | ||
| continue | ||
| } | ||
| if sess.ParentSessionID != "" { | ||
| continue | ||
| } | ||
| combined = append(combined, SessionWithState{Session: *sess}) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| total := len(combined) | ||
| start := min(pg.Offset(), total) | ||
| end := min(pg.Offset()+pg.Limit(), total) | ||
|
|
||
| return exec.NewPaginatedResult(combined[start:end], total, pg) | ||
| } |
There was a problem hiding this comment.
Paginated results have non-deterministic ordering.
collectActiveSessions iterates sync.Map which has no guaranteed order, and persisted sessions (sorted by UpdatedAt descending per the store contract) are appended after the unordered active sessions. Across calls, the same page may return different items, and sessions can be missed or duplicated as users paginate.
Sort combined by a stable key (e.g., UpdatedAt descending) before slicing:
Suggested fix
+ slices.SortFunc(combined, func(a, b SessionWithState) int {
+ return b.Session.UpdatedAt.Compare(a.Session.UpdatedAt)
+ })
+
total := len(combined)
start := min(pg.Offset(), total)📝 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.
Suggested change
| // ListSessionsPaginated returns a paginated list of sessions for the given user. | |
| // Active sessions appear first, followed by persisted inactive sessions. | |
| func (a *API) ListSessionsPaginated(ctx context.Context, userID string, page, perPage int) exec.PaginatedResult[SessionWithState] { | |
| pg := exec.NewPaginator(page, perPage) | |
| activeIDs := make(map[string]struct{}) | |
| activeSessions := a.collectActiveSessions(userID, activeIDs) | |
| combined := make([]SessionWithState, 0, len(activeSessions)) | |
| combined = append(combined, activeSessions...) | |
| if a.store != nil { | |
| persisted, err := a.store.ListSessions(ctx, userID) | |
| if err != nil { | |
| a.logger.Warn("Failed to list persisted sessions", "error", err) | |
| } else { | |
| for _, sess := range persisted { | |
| if _, exists := activeIDs[sess.ID]; exists { | |
| continue | |
| } | |
| if sess.ParentSessionID != "" { | |
| continue | |
| } | |
| combined = append(combined, SessionWithState{Session: *sess}) | |
| } | |
| } | |
| } | |
| total := len(combined) | |
| start := min(pg.Offset(), total) | |
| end := min(pg.Offset()+pg.Limit(), total) | |
| return exec.NewPaginatedResult(combined[start:end], total, pg) | |
| } | |
| // ListSessionsPaginated returns a paginated list of sessions for the given user. | |
| // Active sessions appear first, followed by persisted inactive sessions. | |
| func (a *API) ListSessionsPaginated(ctx context.Context, userID string, page, perPage int) exec.PaginatedResult[SessionWithState] { | |
| pg := exec.NewPaginator(page, perPage) | |
| activeIDs := make(map[string]struct{}) | |
| activeSessions := a.collectActiveSessions(userID, activeIDs) | |
| combined := make([]SessionWithState, 0, len(activeSessions)) | |
| combined = append(combined, activeSessions...) | |
| if a.store != nil { | |
| persisted, err := a.store.ListSessions(ctx, userID) | |
| if err != nil { | |
| a.logger.Warn("Failed to list persisted sessions", "error", err) | |
| } else { | |
| for _, sess := range persisted { | |
| if _, exists := activeIDs[sess.ID]; exists { | |
| continue | |
| } | |
| if sess.ParentSessionID != "" { | |
| continue | |
| } | |
| combined = append(combined, SessionWithState{Session: *sess}) | |
| } | |
| } | |
| } | |
| slices.SortFunc(combined, func(a, b SessionWithState) int { | |
| return b.Session.UpdatedAt.Compare(a.Session.UpdatedAt) | |
| }) | |
| total := len(combined) | |
| start := min(pg.Offset(), total) | |
| end := min(pg.Offset()+pg.Limit(), total) | |
| return exec.NewPaginatedResult(combined[start:end], total, pg) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/agent/api.go` around lines 635 - 668, ListSessionsPaginated
currently appends unordered active sessions (from collectActiveSessions) and
persisted sessions then slices for pagination, causing non-deterministic pages;
after building combined (slice of SessionWithState) sort it deterministically
(use sort.SliceStable) by Session.UpdatedAt descending (most recent first) so
both active and persisted sessions are ordered consistently before computing
start/end and returning exec.NewPaginatedResult; update ListSessionsPaginated to
perform this stable sort on combined prior to slicing.
Comment on lines
+1484
to
+1653
| func setupTestStoreWithMaxPerUser(t *testing.T, maxPerUser int) (*Store, context.Context) { | ||
| t.Helper() | ||
| store, err := New(t.TempDir(), WithMaxPerUser(maxPerUser)) | ||
| require.NoError(t, err) | ||
| return store, context.Background() | ||
| } | ||
|
|
||
| func createTestSessionWithTime(id, userID string, updatedAt time.Time) *agent.Session { | ||
| return &agent.Session{ | ||
| ID: id, | ||
| UserID: userID, | ||
| CreatedAt: updatedAt, | ||
| UpdatedAt: updatedAt, | ||
| } | ||
| } | ||
|
|
||
| func TestCreateSession_EnforcesMaxPerUser(t *testing.T) { | ||
| store, ctx := setupTestStoreWithMaxPerUser(t, 3) | ||
|
|
||
| base := time.Now() | ||
| // Create 5 sessions with increasing timestamps so ordering is deterministic. | ||
| for i := range 5 { | ||
| sess := createTestSessionWithTime( | ||
| fmt.Sprintf("sess-%d", i), | ||
| "user1", | ||
| base.Add(time.Duration(i)*time.Second), | ||
| ) | ||
| require.NoError(t, store.CreateSession(ctx, sess)) | ||
| } | ||
|
|
||
| // Newest 3 should survive, oldest 2 should be deleted. | ||
| for i := range 5 { | ||
| _, err := store.GetSession(ctx, fmt.Sprintf("sess-%d", i)) | ||
| if i < 2 { | ||
| assert.ErrorIs(t, err, agent.ErrSessionNotFound, "session sess-%d should have been deleted", i) | ||
| } else { | ||
| assert.NoError(t, err, "session sess-%d should still exist", i) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func TestCreateSession_MaxPerUser_SubSessionsNotCounted(t *testing.T) { | ||
| store, ctx := setupTestStoreWithMaxPerUser(t, 3) | ||
|
|
||
| base := time.Now() | ||
| // Create 3 top-level sessions. | ||
| for i := range 3 { | ||
| sess := createTestSessionWithTime( | ||
| fmt.Sprintf("parent-%d", i), | ||
| "user1", | ||
| base.Add(time.Duration(i)*time.Second), | ||
| ) | ||
| require.NoError(t, store.CreateSession(ctx, sess)) | ||
| } | ||
|
|
||
| // Create 5 sub-sessions under parent-2 — these should NOT count toward limit. | ||
| for i := range 5 { | ||
| sub := createTestSessionWithTime( | ||
| fmt.Sprintf("sub-%d", i), | ||
| "user1", | ||
| base.Add(time.Duration(10+i)*time.Second), | ||
| ) | ||
| sub.ParentSessionID = "parent-2" | ||
| require.NoError(t, store.CreateSession(ctx, sub)) | ||
| } | ||
|
|
||
| // All 3 parents should still exist (sub-sessions don't count). | ||
| for i := range 3 { | ||
| _, err := store.GetSession(ctx, fmt.Sprintf("parent-%d", i)) | ||
| assert.NoError(t, err, "parent-%d should still exist", i) | ||
| } | ||
| } | ||
|
|
||
| func TestCreateSession_MaxPerUser_DeletesSubSessionsWithParent(t *testing.T) { | ||
| store, ctx := setupTestStoreWithMaxPerUser(t, 2) | ||
|
|
||
| base := time.Now() | ||
| // Create parent-0 (oldest). | ||
| sess0 := createTestSessionWithTime("parent-0", "user1", base) | ||
| require.NoError(t, store.CreateSession(ctx, sess0)) | ||
|
|
||
| // Create 4 sub-sessions under parent-0 (3+ to catch range-mutation bugs). | ||
| for i := range 4 { | ||
| sub := createTestSessionWithTime( | ||
| fmt.Sprintf("sub-%d", i), | ||
| "user1", | ||
| base.Add(time.Duration(1+i)*time.Second), | ||
| ) | ||
| sub.ParentSessionID = "parent-0" | ||
| require.NoError(t, store.CreateSession(ctx, sub)) | ||
| } | ||
|
|
||
| // Create parent-1 and parent-2, pushing parent-0 beyond the limit of 2. | ||
| sess1 := createTestSessionWithTime("parent-1", "user1", base.Add(10*time.Second)) | ||
| require.NoError(t, store.CreateSession(ctx, sess1)) | ||
|
|
||
| sess2 := createTestSessionWithTime("parent-2", "user1", base.Add(20*time.Second)) | ||
| require.NoError(t, store.CreateSession(ctx, sess2)) | ||
|
|
||
| // parent-0 should be deleted along with ALL its sub-sessions. | ||
| _, err := store.GetSession(ctx, "parent-0") | ||
| assert.ErrorIs(t, err, agent.ErrSessionNotFound) | ||
| for i := range 4 { | ||
| _, err = store.GetSession(ctx, fmt.Sprintf("sub-%d", i)) | ||
| assert.ErrorIs(t, err, agent.ErrSessionNotFound, "sub-%d should have been deleted", i) | ||
| } | ||
|
|
||
| // parent-1 and parent-2 should survive. | ||
| _, err = store.GetSession(ctx, "parent-1") | ||
| assert.NoError(t, err) | ||
| _, err = store.GetSession(ctx, "parent-2") | ||
| assert.NoError(t, err) | ||
| } | ||
|
|
||
| func TestCreateSession_MaxPerUser_ZeroMeansUnlimited(t *testing.T) { | ||
| store, ctx := setupTestStoreWithMaxPerUser(t, 0) | ||
|
|
||
| base := time.Now() | ||
| for i := range 200 { | ||
| sess := createTestSessionWithTime( | ||
| fmt.Sprintf("sess-%d", i), | ||
| "user1", | ||
| base.Add(time.Duration(i)*time.Millisecond), | ||
| ) | ||
| require.NoError(t, store.CreateSession(ctx, sess)) | ||
| } | ||
|
|
||
| // All 200 should exist. | ||
| sessions, err := store.ListSessions(ctx, "user1") | ||
| require.NoError(t, err) | ||
| assert.Len(t, sessions, 200) | ||
| } | ||
|
|
||
| func TestCreateSession_MaxPerUser_MultipleUsers(t *testing.T) { | ||
| store, ctx := setupTestStoreWithMaxPerUser(t, 2) | ||
|
|
||
| base := time.Now() | ||
| // Create 4 sessions for user1. | ||
| for i := range 4 { | ||
| sess := createTestSessionWithTime( | ||
| fmt.Sprintf("u1-sess-%d", i), | ||
| "user1", | ||
| base.Add(time.Duration(i)*time.Second), | ||
| ) | ||
| require.NoError(t, store.CreateSession(ctx, sess)) | ||
| } | ||
|
|
||
| // Create 4 sessions for user2. | ||
| for i := range 4 { | ||
| sess := createTestSessionWithTime( | ||
| fmt.Sprintf("u2-sess-%d", i), | ||
| "user2", | ||
| base.Add(time.Duration(i)*time.Second), | ||
| ) | ||
| require.NoError(t, store.CreateSession(ctx, sess)) | ||
| } | ||
|
|
||
| // Each user should have exactly 2 sessions (the newest). | ||
| sessions1, err := store.ListSessions(ctx, "user1") | ||
| require.NoError(t, err) | ||
| assert.Len(t, sessions1, 2) | ||
| assert.Equal(t, "u1-sess-3", sessions1[0].ID) | ||
| assert.Equal(t, "u1-sess-2", sessions1[1].ID) | ||
|
|
||
| sessions2, err := store.ListSessions(ctx, "user2") | ||
| require.NoError(t, err) | ||
| assert.Len(t, sessions2, 2) | ||
| assert.Equal(t, "u2-sess-3", sessions2[0].ID) | ||
| assert.Equal(t, "u2-sess-2", sessions2[1].ID) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n '\bassert\.' internal/persis/filesession/store_test.go | head -50Repository: dagu-org/dagu
Length of output: 2206
🏁 Script executed:
rg -n 'func TestCreateSession_MaxPerUser|func TestCreateSession_EnforcesMaxPerUser' internal/persis/filesession/store_test.goRepository: dagu-org/dagu
Length of output: 423
🏁 Script executed:
ls -la internal/test/Repository: dagu-org/dagu
Length of output: 486
Switch max-per-user tests to require.* assertions.
The test functions at lines 1500–1617 use assert.ErrorIs(), assert.NoError(), assert.Len(), and assert.Equal() for assertions. Per coding guidelines, tests should use stretchr/testify/require for all assertions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/persis/filesession/store_test.go` around lines 1484 - 1653, Tests in
TestCreateSession_EnforcesMaxPerUser,
TestCreateSession_MaxPerUser_SubSessionsNotCounted,
TestCreateSession_MaxPerUser_DeletesSubSessionsWithParent,
TestCreateSession_MaxPerUser_ZeroMeansUnlimited, and
TestCreateSession_MaxPerUser_MultipleUsers use assert.* but must use require.*;
update all assert.ErrorIs, assert.NoError, assert.Len, and assert.Equal calls to
their require equivalents (e.g., require.ErrorIs, require.NoError, require.Len,
require.Equal) around the GetSession and ListSessions checks and any other
assertions in those test functions so failures abort the test immediately.
Comment on lines
+53
to
+127
| // mockSessionStore implements agent.SessionStore for integration tests. | ||
| type mockSessionStore struct { | ||
| sessions []*agent.Session | ||
| } | ||
|
|
||
| func (m *mockSessionStore) CreateSession(_ context.Context, sess *agent.Session) error { | ||
| if sess.ID == "" { | ||
| return agent.ErrInvalidSessionID | ||
| } | ||
| if sess.UserID == "" { | ||
| return agent.ErrInvalidUserID | ||
| } | ||
| m.sessions = append(m.sessions, sess) | ||
| return nil | ||
| } | ||
|
|
||
| func (m *mockSessionStore) GetSession(_ context.Context, id string) (*agent.Session, error) { | ||
| for _, s := range m.sessions { | ||
| if s.ID == id { | ||
| return s, nil | ||
| } | ||
| } | ||
| return nil, agent.ErrSessionNotFound | ||
| } | ||
|
|
||
| func (m *mockSessionStore) ListSessions(_ context.Context, userID string) ([]*agent.Session, error) { | ||
| if userID == "" { | ||
| return nil, agent.ErrInvalidUserID | ||
| } | ||
| var result []*agent.Session | ||
| for _, s := range m.sessions { | ||
| if s.UserID == userID { | ||
| result = append(result, s) | ||
| } | ||
| } | ||
| return result, nil | ||
| } | ||
|
|
||
| func (m *mockSessionStore) UpdateSession(_ context.Context, sess *agent.Session) error { | ||
| for i, s := range m.sessions { | ||
| if s.ID == sess.ID { | ||
| m.sessions[i] = sess | ||
| return nil | ||
| } | ||
| } | ||
| return agent.ErrSessionNotFound | ||
| } | ||
|
|
||
| func (m *mockSessionStore) DeleteSession(_ context.Context, id string) error { | ||
| for i, s := range m.sessions { | ||
| if s.ID == id { | ||
| m.sessions = append(m.sessions[:i], m.sessions[i+1:]...) | ||
| return nil | ||
| } | ||
| } | ||
| return agent.ErrSessionNotFound | ||
| } | ||
|
|
||
| func (m *mockSessionStore) AddMessage(_ context.Context, _ string, _ *agent.Message) error { | ||
| return nil | ||
| } | ||
|
|
||
| func (m *mockSessionStore) GetMessages(_ context.Context, _ string) ([]agent.Message, error) { | ||
| return nil, nil | ||
| } | ||
|
|
||
| func (m *mockSessionStore) GetLatestSequenceID(_ context.Context, _ string) (int64, error) { | ||
| return 0, nil | ||
| } | ||
|
|
||
| func (m *mockSessionStore) ListSubSessions(_ context.Context, _ string) ([]*agent.Session, error) { | ||
| return nil, nil | ||
| } | ||
|
|
||
| var _ agent.SessionStore = (*mockSessionStore)(nil) |
There was a problem hiding this comment.
Consider moving the local SessionStore mock into internal/test (or reuse an existing fixture).
This file defines its own mockSessionStore; to avoid duplication, prefer shared fixtures in internal/test where possible.
As per coding guidelines, "Use stretchr/testify/require for assertions and shared fixtures from internal/test instead of duplicating mocks".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/service/frontend/api/v1/agent_sessions_test.go` around lines 53 -
127, Replace the local mockSessionStore with the shared fixture in internal/test
(or import the existing mock) to avoid duplication: remove the type
mockSessionStore and its methods (CreateSession, GetSession, ListSessions,
UpdateSession, DeleteSession, AddMessage, GetMessages, GetLatestSequenceID,
ListSubSessions) from this test file and import/instantiate the shared
SessionStore mock from internal/test; ensure the test uses the same exported
constructor or variable name provided by internal/test and update any references
to m or its methods accordingly so tests compile and use the centralized
fixture.
Comment on lines
+137
to
+152
| //go:fix inline | ||
| func ptrInt(v int) *int { return new(v) } | ||
|
|
||
| func TestListAgentSessions(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| t.Run("returns empty list when no sessions", func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| setup := newSessionTestSetup(t) | ||
|
|
||
| resp, err := setup.api.ListAgentSessions(sessionAdminCtx(), apigen.ListAgentSessionsRequestObject{ | ||
| Params: apigen.ListAgentSessionsParams{ | ||
| Page: new(1), | ||
| PerPage: new(10), | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the actual file to see the code sections mentioned
cat -n internal/service/frontend/api/v1/agent_sessions_test.go | sed -n '1,160p'Repository: dagu-org/dagu
Length of output: 5141
🏁 Script executed:
# Search for new() usage patterns with numeric literals
rg -n 'new\(' internal/service/frontend/api/v1/agent_sessions_test.go | head -20Repository: dagu-org/dagu
Length of output: 407
🏁 Script executed:
# Search for //go:fix directive
rg -n '//go:fix' internal/service/frontend/api/v1/agent_sessions_test.goRepository: dagu-org/dagu
Length of output: 76
🏁 Script executed:
# Check for assert vs require usage
rg -n 'assert\.|require\.' internal/service/frontend/api/v1/agent_sessions_test.go | head -20Repository: dagu-org/dagu
Length of output: 981
Fix invalid int pointer construction and remove unsupported //go:fix directive; these will prevent compilation.
new(1) and new(v) are invalid Go syntax—new() expects a type, not a value or variable. //go:fix is not a recognized compiler directive. Replace with &v in the helper and update all call sites.
Additionally, replace all assert.* calls with require.* per guidelines, and move locally-defined mocks (mockSessionStore, mockAgentConfigStore) to internal/test for reuse.
Fix pointer construction and directive
-//go:fix inline
-func ptrInt(v int) *int { return new(v) }
+// ptrInt returns a pointer to the provided int.
+func ptrInt(v int) *int { return &v }
@@
- Page: new(1),
- PerPage: new(10),
+ Page: ptrInt(1),
+ PerPage: ptrInt(10),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/service/frontend/api/v1/agent_sessions_test.go` around lines 137 -
152, The test contains invalid pointer construction and an unsupported
directive: remove the `//go:fix inline` directive and replace the helper `func
ptrInt(v int) *int { return new(v) }` with a proper address-returning form (e.g.
return &v) and update all call sites in `TestListAgentSessions` (and other
tests) that use `new(1)`/`new(10)` to use `ptrInt(1)`/`ptrInt(10)` or `&v`
patterns; also change all `assert.*` assertions to `require.*` in this test file
and relocate locally-defined mocks (`mockSessionStore`, `mockAgentConfigStore`)
into `internal/test` for reuse, adjusting imports accordingly.
Comment on lines
+156
to
+162
| listResp, ok := resp.(apigen.ListAgentSessions200JSONResponse) | ||
| require.True(t, ok) | ||
| assert.Empty(t, listResp.Sessions) | ||
| assert.Equal(t, 0, listResp.Pagination.TotalRecords) | ||
| assert.Equal(t, 1, listResp.Pagination.CurrentPage) | ||
| assert.Equal(t, 1, listResp.Pagination.TotalPages) | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n '\bassert\.' internal/service/frontend/api/v1/agent_sessions_test.goRepository: dagu-org/dagu
Length of output: 1782
Convert all assert.* calls to require.* for consistency with coding guidelines.
The file contains widespread use of assert.* assertions (at least 39 occurrences across multiple test cases, including lines 158–161 in the shown snippet). Per coding guidelines, tests should use stretchr/testify/require for assertions instead. This provides fail-fast behavior and aligns with the repository's testing standards.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/service/frontend/api/v1/agent_sessions_test.go` around lines 156 -
162, Replace all usages of assert.* with require.* in this test file so
assertions fail fast and follow guidelines; for example change assert.Empty(t,
listResp.Sessions) to require.Empty(t, listResp.Sessions) and change
assert.Equal(t, 0, listResp.Pagination.TotalRecords), assert.Equal(t, 1,
listResp.Pagination.CurrentPage), assert.Equal(t, 1,
listResp.Pagination.TotalPages) to require.Equal(...) respectively; update any
other assert.* calls (there are ~39) to require.* and ensure the test
imports/uses github.com/stretchr/testify/require (and remove the unused assert
import if present).
Comment on lines
+195
to
+198
| const loadMoreSessions = useCallback(async (): Promise<void> => { | ||
| if (!hasMoreSessions) return; | ||
| await fetchSessionsPage(sessionPage + 1); | ||
| }, [fetchSessionsPage, sessionPage, hasMoreSessions]); |
There was a problem hiding this comment.
Race condition: concurrent loadMoreSessions calls will append duplicate sessions.
hasMoreSessions and sessionPage are React state — they don't update synchronously during an in-flight async call. Two rapid invocations both pass the !hasMoreSessions guard, both read the same stale sessionPage, compute the same sessionPage + 1, and both resolve appendSessions with identical page data, producing duplicate entries in the session list.
A ref-based in-flight guard prevents this:
🐛 Proposed fix
+ const isLoadingMoreRef = useRef(false);
+
const loadMoreSessions = useCallback(async (): Promise<void> => {
- if (!hasMoreSessions) return;
- await fetchSessionsPage(sessionPage + 1);
+ if (!hasMoreSessions || isLoadingMoreRef.current) return;
+ isLoadingMoreRef.current = true;
+ try {
+ await fetchSessionsPage(sessionPage + 1);
+ } finally {
+ isLoadingMoreRef.current = false;
+ }
}, [fetchSessionsPage, sessionPage, hasMoreSessions]);📝 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.
Suggested change
| const loadMoreSessions = useCallback(async (): Promise<void> => { | |
| if (!hasMoreSessions) return; | |
| await fetchSessionsPage(sessionPage + 1); | |
| }, [fetchSessionsPage, sessionPage, hasMoreSessions]); | |
| const isLoadingMoreRef = useRef(false); | |
| const loadMoreSessions = useCallback(async (): Promise<void> => { | |
| if (!hasMoreSessions || isLoadingMoreRef.current) return; | |
| isLoadingMoreRef.current = true; | |
| try { | |
| await fetchSessionsPage(sessionPage + 1); | |
| } finally { | |
| isLoadingMoreRef.current = false; | |
| } | |
| }, [fetchSessionsPage, sessionPage, hasMoreSessions]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/features/agent/hooks/useAgentChat.ts` around lines 195 - 198, The
loadMoreSessions function has a race where concurrent calls read stale
hasMoreSessions/sessionPage and fetch the same page twice; fix it by adding a
ref-based in-flight guard (e.g., inFlightLoadMoreRef) and a nextPage ref (or
update nextPage from sessionPage once, e.g., const nextPage = sessionPage + 1)
so only one caller proceeds: check hasMoreSessions AND
!inFlightLoadMoreRef.current, set inFlightLoadMoreRef.current = true before
calling fetchSessionsPage(nextPage), await the fetch, then in finally set
inFlightLoadMoreRef.current = false and update sessionPage/state via the
existing appendSessions flow to avoid duplicate appends; reference
loadMoreSessions, fetchSessionsPage, hasMoreSessions, sessionPage,
appendSessions.
Comment on lines
+1
to
+7
| export function formatDate(dateStr: string): string { | ||
| return new Date(dateStr).toLocaleString(undefined, { | ||
| month: 'short', | ||
| day: 'numeric', | ||
| hour: '2-digit', | ||
| minute: '2-digit', | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
JavaScript new Date invalid string behavior toLocaleString
💡 Result:
new Date(badString)(andDate.parse(badString)) tries to parse the string. If it can’t, you get an “Invalid Date” object whose internal time value isNaN(sodate.getTime()isNaN,date.toString()is"Invalid Date"). Converting an invalid date to ISO (e.g.toISOString()/toJSON()) throws a RangeError: Invalid time value. [1][2]- Don’t round-trip via
toLocaleString():toLocaleString()is meant for display, and its output is language/region/implementation dependent (it can include different ordering, separators, non‑breaking spaces, etc.). The spec only requires parsers to reliably accept ISO-like formats and (in practice)toString()/toUTCString()formats—nottoLocaleString()—sonew Date(date.toLocaleString())may become Invalid Date or a different date across environments. [3][4][5] - If you need a reliable string round-trip, use
toISOString()(and parse that), or round-trip the timestamp (date.getTime()). [3][6]
Sources
[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Invalid_date
[2] https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Date/parse
[3] https://stackoverflow.com/questions/29988868/why-does-parsing-a-locale-date-string-result-in-an-invalid-date
[4] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toLocaleString
[5] https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Date
[6] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toISOString
Guard against invalid dates to avoid "Invalid Date" UI.
If the backend returns an empty or malformed string, new Date() creates an Invalid Date object that renders literally as "Invalid Date" when passed to toLocaleString(). A validity check keeps the UI clean.
🔧 Suggested fix
export function formatDate(dateStr: string): string {
- return new Date(dateStr).toLocaleString(undefined, {
+ const date = new Date(dateStr);
+ if (Number.isNaN(date.getTime())) return dateStr || '-';
+ return date.toLocaleString(undefined, {
month: 'short',
day: 'numeric',
hour: '2-digit',
minute: '2-digit',
});
}📝 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.
Suggested change
| export function formatDate(dateStr: string): string { | |
| return new Date(dateStr).toLocaleString(undefined, { | |
| month: 'short', | |
| day: 'numeric', | |
| hour: '2-digit', | |
| minute: '2-digit', | |
| }); | |
| export function formatDate(dateStr: string): string { | |
| const date = new Date(dateStr); | |
| if (Number.isNaN(date.getTime())) return dateStr || '-'; | |
| return date.toLocaleString(undefined, { | |
| month: 'short', | |
| day: 'numeric', | |
| hour: '2-digit', | |
| minute: '2-digit', | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/src/features/agent/utils/formatDate.ts` around lines 1 - 7, The formatDate
function currently calls new Date(dateStr).toLocaleString(...) and will render
"Invalid Date" for empty or malformed inputs; update formatDate to construct the
Date object, check its validity with isNaN(date.getTime()) (or
Number.isNaN(date.valueOf())), and if invalid return an empty string (or a clear
fallback) instead of calling toLocaleString; keep the existing options and
behavior for valid dates so only the Date validity guard around new
Date(dateStr) and the toLocaleString call needs to be added.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1692 +/- ##
==========================================
+ Coverage 70.10% 70.21% +0.11%
==========================================
Files 369 369
Lines 40771 40878 +107
==========================================
+ Hits 28582 28704 +122
+ Misses 9920 9903 -17
- Partials 2269 2271 +2
... and 11 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
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
1 participant
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 by CodeRabbit
New Features
UI
Behavior / Reliability
Configuration