fix(api): sort memory_sessions by recency, expose limit param#961
fix(api): sort memory_sessions by recency, expose limit param#961shgew wants to merge 2 commits into
Conversation
api::sessions returns sessions sorted by most-recent activity and accepts a numeric ?limit. When ?limit is absent, all sessions are returned (preserves the viewer dashboard, which consumes the same endpoint). When provided, the value is honored verbatim with a non-negative floor; no upper cap. The MCP memory_sessions path self-defaults to 20 and exposes the same limit field in its tool schema. Pre-fix: REST returned every session in KV order with an N+1 summary fetch on the full set, bloating MCP client context on large stores. The fix lets the caller choose how many sessions they want without imposing an arbitrary ceiling. Both MCP paths (in-process handler and standalone proxy shim) self-default to 20, so neither relies on the REST default. Tests: test/api-sessions-limit.test.ts (4 cases). Signed-off-by: Hleb Shauchenka <me@marleb.org>
|
@shgew is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughSession listing is updated in two surfaces: the ChangesSession Listing: Recency Sort and Limit
Sequence DiagramsequenceDiagram
participant Client
participant api_sessions as api::sessions handler
participant KV
Client->>api_sessions: GET /sessions?limit=N
api_sessions->>KV: list sessions (all)
KV-->>api_sessions: raw session list
api_sessions->>api_sessions: compute recencyKey per session
api_sessions->>api_sessions: sort by recencyKey desc
api_sessions->>api_sessions: slice to limit (or all if undefined)
api_sessions->>KV: lookup summaries for recent subset only
KV-->>api_sessions: summaries
api_sessions-->>Client: sorted, limited session list
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/triggers/api.ts`:
- Around line 827-830: The recencyKey function in src/triggers/api.ts references
properties updatedAt and lastCheckpointAt on the Session object, but the Session
interface in src/types.ts does not declare these properties, causing TypeScript
type errors. Add updatedAt and lastCheckpointAt as string properties (or
optional string properties if they're conditionally present) to the Session
interface definition in src/types.ts to match what the recencyKey function
expects when comparing session timestamps.
In `@test/api-sessions-limit.test.ts`:
- Around line 126-132: The test "treats non-positive limit as zero" currently
only validates the behavior when limit is "0", but the test name implies it
should cover all non-positive values including negative numbers. Add an
additional assertion or test case within this test that also checks the behavior
when passing a negative limit value like "-1" to the reqWithLimit function to
ensure both zero and negative limits are handled consistently and treated as
zero in the sessions response.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 80bc192e-fd51-45dc-a0aa-61c53782b18f
📒 Files selected for processing (4)
src/mcp/server.tssrc/mcp/tools-registry.tssrc/triggers/api.tstest/api-sessions-limit.test.ts
…negative limit recencyKey() in src/triggers/api.ts and src/mcp/server.ts reads these fields, so the Session interface needs to declare them or strict mode emits TS2339. Both are optional in practice (persisted sessions backfill them via checkpoint runs), so they are typed as optional strings. Also extend the 'treats non-positive limit as zero' case to assert -1 in addition to 0, matching the contract the endpoint promises.
What
api::sessionsnow sorts sessions by most-recent activity and accepts a numeric?limit. When?limitis absent, all sessions are returned (preserves the viewer dashboard, which consumes the same endpoint). When provided, the value is honored verbatim with a non-negative floor; no upper cap. The MCPmemory_sessionspath self-defaults to 20 and exposeslimitin its tool schema.Why
Pre-fix: REST returned every session in KV order with an N+1 summary fetch on the full set, which bloated MCP client context on large stores. The fix lets the caller choose how many sessions they want without imposing an arbitrary ceiling.
How
Both MCP paths (in-process handler and standalone proxy shim) self-default to 20, so neither relies on the REST default.
Tests
test/api-sessions-limit.test.ts(4 cases). Targeted suite: 4/4 pass.Compatibility
REST default-all matches pre-existing behavior. MCP self-default is opaque to REST consumers. No upper bound clamp; callers passing very large limits get exactly what they ask for.
Summary by CodeRabbit
New Features
limitparameter to control the number of results returned (defaults to 20).Tests