fix(api): honour limit query param on /sessions and /replay/sessions (#1022)#1034
fix(api): honour limit query param on /sessions and /replay/sessions (#1022)#1034ziishanahmad wants to merge 2 commits into
Conversation
…ohitg00#1022) Signed-off-by: Zeeshan Ahmad <ziishanahmad@gmail.com>
|
@ziishanahmad is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds support for a ChangesSessions limit query parameter
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/triggers/api.ts (3)
505-519: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a shared limit-parsing helper to eliminate duplication.
The limit parsing logic (
rawLimit→parseInt→Number.isFinite→ slice-or-full) is duplicated verbatim across both endpoints. The file already usesparseOptionalIntelsewhere (e.g., line 800 forapi::commits). A small helper likeparsePositiveLimit(raw: string | undefined): number | undefinedwould centralize the validation and keep the two endpoints consistent.♻️ Proposed helper extraction
+// e.g. in a shared utils section: +function parsePositiveLimit(raw: string | undefined): number | undefined { + if (typeof raw !== "string" || raw.length === 0) return undefined; + const n = parseInt(raw, 10); + return Number.isFinite(n) && n > 0 ? n : undefined; +} + // Then in each handler: -const rawLimit = req.query_params?.["limit"]; -const parsedLimit = - typeof rawLimit === "string" && rawLimit.length > 0 - ? parseInt(rawLimit, 10) - : NaN; -const limited = - Number.isFinite(parsedLimit) && parsedLimit > 0 - ? sessions.slice(0, parsedLimit) - : sessions; +const limit = parsePositiveLimit(req.query_params?.["limit"]); +const limited = limit !== undefined ? sessions.slice(0, limit) : sessions;Also applies to: 848-861
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/triggers/api.ts` around lines 505 - 519, The replay list endpoint duplicates the same limit parsing and slicing logic used elsewhere in this module, so extract a shared helper to keep both endpoints consistent. Add a small helper near the existing parseOptionalInt usage, such as a positive-limit parser, and update the replay handler and the api::commits path to use it instead of repeating rawLimit/parsing/slice logic in each endpoint. Keep the behavior identical for valid positive limits and fallback to the full sessions list when no usable limit is provided.
505-509: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrim WHAT-explaining comments per coding guidelines.
The coding guidelines for
src/**/*.tsstate: "avoid code comments explaining WHAT — use clear naming instead." The multi-line comments here describe what the code does ("Honour ?limit=N (>=1)...") alongside the rationale. Consider trimming to just the WHY (issue reference + motivation) and letting the code speak for itself.Also applies to: 848-851
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/triggers/api.ts` around lines 505 - 509, Trim the WHAT-explaining inline comments around the replay list limit handling in the API trigger code and keep only the WHY. In the area around the replay endpoint logic in `src/triggers/api.ts` (and the matching comment block elsewhere noted in the review), remove descriptions of what the `limit` behavior does and retain only the motivation/reference to the related issue or fix so the code itself explains the behavior.Source: Coding guidelines
840-861: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSlice before summary lookups to avoid unnecessary I/O when limit is set.
api::sessionsfetches summaries viaPromise.allfor every filtered session (lines 840–844) and only slices afterward (line 859). When?limit=Nis provided, this still performs N_totalkv.getcalls instead of N. Applying the slice tofilteredbefore the summary lookup would eliminate the wasted I/O. Note thatapi::sessionsdoes not sort bystartedAt(unlikeapi::replay::sessions), so the first N sessions are inkv.listorder — consider whether that ordering is intentional for paginated results.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/triggers/api.ts` around lines 840 - 861, The `api::sessions` handler is doing summary fetches for every item in `filtered` before applying `limit`, so `Promise.all` still triggers unnecessary `kv.get` I/O when `?limit=N` is set. Update the `api::sessions` flow to compute `parsedLimit`/`limited` earlier and slice `filtered` before the `Promise.all` summary lookup, using the `withSummary`/`limited` logic in `src/triggers/api.ts` to keep only the requested number of sessions. Also confirm the resulting pagination order is intentional since `api::sessions` relies on `kv.list` order rather than sorting like `api::replay::sessions`.test/sessions-limit.test.ts (1)
1-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffSource-level regex assertions are fragile — prefer behavioral tests.
These tests verify that the source code contains specific string patterns (
query_params?.["limit"],slice(0, parsedLimit)) rather than testing actual endpoint behavior. A variable rename or refactor would break the tests even if behavior is correct, and a buggy implementation that happens to contain the right patterns would pass. The comment acknowledges this tradeoff, but consider adding a behavioral test that mockssdk/kvand asserts the sliced response shape, following the patterns intest/crystallize.test.ts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/sessions-limit.test.ts` around lines 1 - 46, The current assertions in sessions-limit.test.ts are source-level regex checks against src/triggers/api.ts, which makes the test brittle and unable to verify real behavior. Add a behavioral test like the ones in test/crystallize.test.ts by mocking sdk/kv and exercising the api::sessions and api::replay::sessions handlers so you can assert the returned list is sliced when limit is valid and remains full when limit is missing or invalid. Keep the existing source checks only if needed as a supplement, but move the main coverage to runtime behavior around sdk.registerFunction and the sessions/replay session handlers.Source: Coding guidelines
🤖 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 `@test/sessions-limit.test.ts`:
- Around line 38-45: The api::replay::sessions test only checks limit parsing
and slicing, but it is missing the fallback coverage that api::sessions already
verifies. In test/sessions-limit.test.ts, extend the api::replay::sessions spec
to assert the same parsedLimit > 0 fallback behavior by matching the replay
endpoint’s handling in api and confirming it returns the full sessions list when
the limit is invalid. Keep the new assertion рядом with the existing
sdk.registerFunction checks so the companion fix stays aligned with the
api::replay::sessions symbol.
---
Nitpick comments:
In `@src/triggers/api.ts`:
- Around line 505-519: The replay list endpoint duplicates the same limit
parsing and slicing logic used elsewhere in this module, so extract a shared
helper to keep both endpoints consistent. Add a small helper near the existing
parseOptionalInt usage, such as a positive-limit parser, and update the replay
handler and the api::commits path to use it instead of repeating
rawLimit/parsing/slice logic in each endpoint. Keep the behavior identical for
valid positive limits and fallback to the full sessions list when no usable
limit is provided.
- Around line 505-509: Trim the WHAT-explaining inline comments around the
replay list limit handling in the API trigger code and keep only the WHY. In the
area around the replay endpoint logic in `src/triggers/api.ts` (and the matching
comment block elsewhere noted in the review), remove descriptions of what the
`limit` behavior does and retain only the motivation/reference to the related
issue or fix so the code itself explains the behavior.
- Around line 840-861: The `api::sessions` handler is doing summary fetches for
every item in `filtered` before applying `limit`, so `Promise.all` still
triggers unnecessary `kv.get` I/O when `?limit=N` is set. Update the
`api::sessions` flow to compute `parsedLimit`/`limited` earlier and slice
`filtered` before the `Promise.all` summary lookup, using the
`withSummary`/`limited` logic in `src/triggers/api.ts` to keep only the
requested number of sessions. Also confirm the resulting pagination order is
intentional since `api::sessions` relies on `kv.list` order rather than sorting
like `api::replay::sessions`.
In `@test/sessions-limit.test.ts`:
- Around line 1-46: The current assertions in sessions-limit.test.ts are
source-level regex checks against src/triggers/api.ts, which makes the test
brittle and unable to verify real behavior. Add a behavioral test like the ones
in test/crystallize.test.ts by mocking sdk/kv and exercising the api::sessions
and api::replay::sessions handlers so you can assert the returned list is sliced
when limit is valid and remains full when limit is missing or invalid. Keep the
existing source checks only if needed as a supplement, but move the main
coverage to runtime behavior around sdk.registerFunction and the sessions/replay
session handlers.
🪄 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: 4b522fdd-34ba-4bac-93fc-36dbb534f48a
📒 Files selected for processing (2)
src/triggers/api.tstest/sessions-limit.test.ts
…ddress review feedback (rohitg00#1022) Signed-off-by: Zeeshan Ahmad <ziishanahmad@gmail.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/sessions-limit.test.ts (1)
1-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMirror the behavioral test pattern used in
test/crystallize.test.ts.This file asserts against
src/triggers/api.tsdirectly; the existing suite uses runtime tests withmockSdk(),mockKV(), andsdk.trigger(...). Rewriting this to exercise the endpoint with mockediii-sdkwould make it less fragile and consistent with the rest of the tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/sessions-limit.test.ts` around lines 1 - 14, The current sessions limit test is a source-level assertion against src/triggers/api.ts, but it should follow the runtime pattern used in crystallize.test.ts. Rework the test to exercise the endpoint through mocked iii-sdk behavior using mockSdk(), mockKV(), and sdk.trigger(...), so the GET /agentmemory/sessions and GET /agentmemory/replay/sessions limit handling is validated behaviorally instead of by reading source text.Sources: Coding guidelines, Learnings
🤖 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.
Nitpick comments:
In `@test/sessions-limit.test.ts`:
- Around line 1-14: The current sessions limit test is a source-level assertion
against src/triggers/api.ts, but it should follow the runtime pattern used in
crystallize.test.ts. Rework the test to exercise the endpoint through mocked
iii-sdk behavior using mockSdk(), mockKV(), and sdk.trigger(...), so the GET
/agentmemory/sessions and GET /agentmemory/replay/sessions limit handling is
validated behaviorally instead of by reading source text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a583f1f4-23f5-4140-b337-d1ed8d0b4fec
📒 Files selected for processing (2)
src/triggers/api.tstest/sessions-limit.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/triggers/api.ts
1 participant
What
The HTTP endpoints
GET /agentmemory/sessionsandGET /agentmemory/replay/sessionswere returning the full session table on every call, ignoring any?limit=Nquery parameter. The MCP toolmemory_sessionsalready supportslimit(added in #139) but the HTTP twins did not.Why
The real-time viewer and CLI both want to page through sessions without forcing the engine to ship every record on every request. Returning the full table is wasteful for projects with thousands of sessions and the existing MCP tool already does the right thing — the HTTP path was the gap.
How
api::sessions(src/triggers/api.ts) now reads?limit=Nand slices the response. Negative or non-numeric values fall back to the full list so a bad query never produces an empty result.api::replay::sessions(same file) gets the same treatment. The replay endpoint had the same gap.Tests
Added
test/sessions-limit.test.tswith 4 source-level assertions (matches the style oftest/memories-pagination.test.ts). Full suite: 1374/1419 pass (45 pre-existing Windows HOME/USERPROPLACE failures in upstream on Win, unrelated to this change).Notes
Diff is rebased on clean upstream main — only 2 files changed (src/triggers/api.ts + test/sessions-limit.test.ts), 75 lines. No rebrand churn.
Signed-off-by: Zeeshan Ahmad ziishanahmad@gmail.com
Summary by CodeRabbit
New Features
limitsupport to session listing endpoints, so clients can request only the first N sessions when a valid value is provided.limitbehavior for replay session listings to improve result manageability.Bug Fixes
limitis missing, invalid, or non-positive, endpoints now safely return the full filtered session list instead of applying an incorrect or partial slice.