fix(viewer): graph tab blank on large graphs (graph/query pagination)#789
Merged
Conversation
Closes #753. POST /agentmemory/graph/query with an unbounded body ({}) on a corpus with 11k+ nodes returned HTTP 500 'Invocation stopped' because the response payload exceeded the iii state response channel ceiling. Viewer's api() returns null on non-2xx so the failure surfaced as '0 nodes / 0 edges' on a blank canvas with no signal — same shape as #544, never covered for graph/query. Backend (src/functions/graph.ts) - mem::graph-query accepts limit + offset on every branch and applies a default cap (DEFAULT_GRAPH_QUERY_LIMIT = 500, MAX = 5000) so an empty {} body never materializes the whole graph in one invocation. - Empty-body / nodeType-only branch ranks nodes by incident-edge degree before paginating so the truncated page surfaces the densest subgraph rather than an arbitrary KV scan order. - Edges in the page response are restricted to edges with BOTH endpoints in the page; cross-page edges are dropped so the viewer doesn't render dangling lines. - Response shape extended: totalNodes, totalEdges, truncated, limit, offset. totalNodes / totalEdges reflect the unbounded result for the given filter so callers can render 'showing X of Y' without re-querying. API (src/triggers/api.ts) - api::graph-query whitelists payload fields (startNodeId, nodeType, maxDepth, query, limit, offset) per AGENTS.md security rule. No more raw req.body spread. Viewer (src/viewer/index.html) - Initial /graph/query sends an explicit limit (500) rather than {}. - Distinguishes apiPost === null (server error) from { nodes: [], edges: [] } (truly empty corpus). Failure now renders a visible 'Graph query failed' banner with a Retry button instead of the 'No graph data yet. Building...' empty state. - Truncation banner above the search box: 'Showing N of M nodes (most-connected first). The full graph is too large to render at once.' so operators know the page is bounded. - state.graph gains queryError, truncated, totalNodes, totalEdges fields. Tests (test/graph.test.ts) - Unbounded {} body caps to 500 with truncated=true on a 1,200-node seed; high-degree nodes land on page 1. - limit + offset paginate without overlap on a 50-node seed. - limit clamped above MAX returns at most MAX. - Cross-page edges excluded from page response; totalEdges still counts unbounded. 121 test files / 1330 tests pass.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThis PR adds pagination and safety limits to graph queries to prevent unbounded full-graph materialization on large corpuses. It introduces default and maximum query limits, updates the result type with truncation metadata, sanitizes API payloads, and integrates bounded loading with error feedback in the viewer. ChangesGraph Query Pagination and Safety
Sequence DiagramsequenceDiagram
participant Viewer
participant APIHandler as api::graph-query
participant GraphQuery as mem::graph-query
participant Paginate as paginate()
Viewer->>APIHandler: POST graph/query {limit, offset, nodeType?}
APIHandler->>APIHandler: Whitelist fields into payload
APIHandler->>GraphQuery: Sanitized payload with limit/offset
alt Query branch
GraphQuery->>GraphQuery: Apply query filter
GraphQuery->>Paginate: Collected nodes/edges
else StartNodeId branch
GraphQuery->>GraphQuery: Traverse from node
GraphQuery->>Paginate: Traversed nodes/edges
else NodeType or empty body
GraphQuery->>GraphQuery: rankByDegree(nodes)
GraphQuery->>Paginate: Ranked nodes/edges
end
Paginate->>Paginate: Slice to [offset, offset+limit)
Paginate->>Paginate: Filter edges to contained endpoints
Paginate->>Paginate: Compute unbounded totals
GraphQuery->>APIHandler: GraphQueryResult + pagination metadata
APIHandler->>Viewer: {nodes, edges, totalNodes, totalEdges, truncated, limit, offset}
alt Success (null check)
Viewer->>Viewer: Set state.graph fields, build/render
else Failure (null result)
Viewer->>Viewer: Set queryError, render error banner
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/triggers/api.ts (1)
1346-1355: 💤 Low valueWhitelist implementation is correct.
The explicit payload construction properly follows the coding guideline requiring REST endpoints to never pass raw
req.bodytosdk.trigger.For consistency with other endpoints (e.g.,
api::searchvalidateslimit), consider adding input validation forlimitandoffsetat the API boundary. Since the backend handles clamping viaresolvePagination, this is optional.♻️ Optional: Add boundary validation
const authErr = checkAuth(req, secret); if (authErr) return authErr; + const body = (req.body ?? {}) as Record<string, unknown>; + if ( + body.limit !== undefined && + (!Number.isInteger(body.limit) || (body.limit as number) < 1) + ) { + return { status_code: 400, body: { error: "limit must be a positive integer" } }; + } + if ( + body.offset !== undefined && + (!Number.isInteger(body.offset) || (body.offset as number) < 0) + ) { + return { status_code: 400, body: { error: "offset must be a non-negative integer" } }; + } // Whitelist payload fields explicitly; AGENTS.md security rule: // REST endpoints never pass raw req.body through to sdk.trigger. const payload = {As per coding guidelines:
{src/mcp/**,src/triggers/**}/*.ts: Perform input validation at system boundaries.🤖 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 1346 - 1355, The payload is properly whitelisted but lacks API-boundary validation for pagination; add simple input checks before constructing payload: validate and coerce req.body.limit and req.body.offset to safe integers (e.g., ensure non-negative integers and apply sensible defaults) so payload.limit and payload.offset are always numeric, and keep backend clamping via resolvePagination; update the block building payload (referencing payload, req.body, sdk.trigger, and resolvePagination) to perform these checks consistent with api::search.test/graph.test.ts (2)
339-366: 💤 Low valueConsider condensing the implementation-detail comments.
The comments on lines 339-366 explain HOW the test works (tight cluster, degree ranking, cross-page edge mechanics). While helpful, they narrate WHAT the code does rather than WHY. The setup code and assertions are self-documenting.
♻️ Suggested simplification
- // Make the first 10 nodes a tightly connected cluster so they - // rank highest by degree and land on the page deterministically. + // First 10 nodes form a cluster (high degree → ranked first). for (let i = 0; i < 10; i++) { const next = (i + 1) % 10; await kv.set("mem:graph:edges", `cluster_${i}`, { @@ -353,10 +339,7 @@ }); } - // Cross-page edge: source in the high-degree cluster (on page), - // target is an isolated node (degree 1; cluster nodes have - // degree 2 so the target ranks below the cap). + // Cross-page edge: source on-page, target off-page. await kv.set("mem:graph:edges", "cross", {As per coding guidelines: "No code comments explaining WHAT — use clear naming instead" for
src/**/*.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/graph.test.ts` around lines 339 - 366, Replace the multi-line HOW comments above the cluster creation loop and the "cross" edge with a single concise purpose comment: e.g., "Create a high-degree cluster and a cross-page edge to exercise degree-based pagination." Keep the setup code as-is (the for loop that calls kv.set("mem:graph:edges", `cluster_${i}`...) and the subsequent kv.set("mem:graph:edges", "cross", ...)), but remove the detailed step-by-step narrative and keep only the short WHY-focused comment so the self-documenting names (cluster_${i}, "cross", sourceNodeId x_005, targetNodeId x_055) describe WHAT is happening.
218-223: ⚡ Quick winRemove or condense the explanatory comment.
This comment explains WHAT the code does rather than WHY it exists. As per coding guidelines, use clear naming instead of comments explaining WHAT. The test name and assertions already document the expected behavior.
♻️ Suggested simplification
- // `#753`: an unbounded {} body used to materialize every node+edge in - // one payload, which exceeded the iii state response channel on - // large corpora (11k+ nodes) and returned HTTP 500 "Invocation - // stopped". The fix caps the page at DEFAULT_GRAPH_QUERY_LIMIT (500) - // and surfaces totalNodes / totalEdges so callers know it was - // truncated. + // `#753` it("caps an unbounded graph-query body to a default page and reports totals", async () => {As per coding guidelines: "No code comments explaining WHAT — use clear naming instead" for
src/**/*.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/graph.test.ts` around lines 218 - 223, Remove or greatly shorten the explanatory comment block that describes what the test does; instead rely on the test name and assertions to document behavior and (if a short note is still helpful) replace it with a one-line rationale like "Ensure pagination prevents unbounded payloads" referencing DEFAULT_GRAPH_QUERY_LIMIT so readers know the limit being asserted; update or trim the comment around the test asserting DEFAULT_GRAPH_QUERY_LIMIT, totalNodes, and totalEdges to a single-line WHY note or delete it entirely.
🤖 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/viewer/index.html`:
- Around line 1718-1726: When handling the post-rebuild freshResults, ensure we
set a visible query error if the subsequent graph/query failed: check
freshResults[0] (freshQuery) and if it is falsy, assign state.graph.queryError
to freshResults[1] (or a small default error object/message) so the UI shows an
error banner; otherwise proceed to clear/set state.graph.nodes, edges,
truncated, totalNodes, totalEdges and ensure state.graph.queryError is cleared
when the query succeeds. Reference symbols: freshResults, freshQuery,
state.graph, state.graph.queryError, graph/query.
---
Nitpick comments:
In `@src/triggers/api.ts`:
- Around line 1346-1355: The payload is properly whitelisted but lacks
API-boundary validation for pagination; add simple input checks before
constructing payload: validate and coerce req.body.limit and req.body.offset to
safe integers (e.g., ensure non-negative integers and apply sensible defaults)
so payload.limit and payload.offset are always numeric, and keep backend
clamping via resolvePagination; update the block building payload (referencing
payload, req.body, sdk.trigger, and resolvePagination) to perform these checks
consistent with api::search.
In `@test/graph.test.ts`:
- Around line 339-366: Replace the multi-line HOW comments above the cluster
creation loop and the "cross" edge with a single concise purpose comment: e.g.,
"Create a high-degree cluster and a cross-page edge to exercise degree-based
pagination." Keep the setup code as-is (the for loop that calls
kv.set("mem:graph:edges", `cluster_${i}`...) and the subsequent
kv.set("mem:graph:edges", "cross", ...)), but remove the detailed step-by-step
narrative and keep only the short WHY-focused comment so the self-documenting
names (cluster_${i}, "cross", sourceNodeId x_005, targetNodeId x_055) describe
WHAT is happening.
- Around line 218-223: Remove or greatly shorten the explanatory comment block
that describes what the test does; instead rely on the test name and assertions
to document behavior and (if a short note is still helpful) replace it with a
one-line rationale like "Ensure pagination prevents unbounded payloads"
referencing DEFAULT_GRAPH_QUERY_LIMIT so readers know the limit being asserted;
update or trim the comment around the test asserting DEFAULT_GRAPH_QUERY_LIMIT,
totalNodes, and totalEdges to a single-line WHY note or delete it entirely.
🪄 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: 35319d50-34ca-4745-8205-957eafb56348
📒 Files selected for processing (5)
src/functions/graph.tssrc/triggers/api.tssrc/types.tssrc/viewer/index.htmltest/graph.test.ts
Comment on lines
+1718
to
1726
| var freshQuery = freshResults[0]; | ||
| if (freshQuery) { | ||
| state.graph.nodes = freshQuery.nodes || []; | ||
| state.graph.edges = freshQuery.edges || []; | ||
| state.graph.truncated = freshQuery.truncated === true; | ||
| state.graph.totalNodes = typeof freshQuery.totalNodes === 'number' ? freshQuery.totalNodes : state.graph.nodes.length; | ||
| state.graph.totalEdges = typeof freshQuery.totalEdges === 'number' ? freshQuery.totalEdges : state.graph.edges.length; | ||
| } | ||
| state.graph.stats = freshResults[1] || {}; |
Contributor
There was a problem hiding this comment.
Handle post-rebuild query failure.
If the rebuild succeeds but the subsequent graph/query request fails (returns null), the viewer does not set state.graph.queryError, so no error banner is shown. The user sees an empty graph with no indication that the query failed.
While rare (requires first load + successful build + failed query), this violates the PR objective to surface backend errors as a visible error state instead of falling silent.
🛡️ Proposed fix to set queryError when fresh query fails
var freshResults = await Promise.all([
apiPost('graph/query', { limit: GRAPH_INITIAL_LIMIT }),
apiGet('graph/stats')
]);
var freshQuery = freshResults[0];
if (freshQuery) {
state.graph.nodes = freshQuery.nodes || [];
state.graph.edges = freshQuery.edges || [];
state.graph.truncated = freshQuery.truncated === true;
state.graph.totalNodes = typeof freshQuery.totalNodes === 'number' ? freshQuery.totalNodes : state.graph.nodes.length;
state.graph.totalEdges = typeof freshQuery.totalEdges === 'number' ? freshQuery.totalEdges : state.graph.edges.length;
+ } else {
+ state.graph.queryError = 'graph/query failed after rebuild (check server logs)';
}
state.graph.stats = freshResults[1] || {};
}📝 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
| var freshQuery = freshResults[0]; | |
| if (freshQuery) { | |
| state.graph.nodes = freshQuery.nodes || []; | |
| state.graph.edges = freshQuery.edges || []; | |
| state.graph.truncated = freshQuery.truncated === true; | |
| state.graph.totalNodes = typeof freshQuery.totalNodes === 'number' ? freshQuery.totalNodes : state.graph.nodes.length; | |
| state.graph.totalEdges = typeof freshQuery.totalEdges === 'number' ? freshQuery.totalEdges : state.graph.edges.length; | |
| } | |
| state.graph.stats = freshResults[1] || {}; | |
| var freshQuery = freshResults[0]; | |
| if (freshQuery) { | |
| state.graph.nodes = freshQuery.nodes || []; | |
| state.graph.edges = freshQuery.edges || []; | |
| state.graph.truncated = freshQuery.truncated === true; | |
| state.graph.totalNodes = typeof freshQuery.totalNodes === 'number' ? freshQuery.totalNodes : state.graph.nodes.length; | |
| state.graph.totalEdges = typeof freshQuery.totalEdges === 'number' ? freshQuery.totalEdges : state.graph.edges.length; | |
| } else { | |
| state.graph.queryError = 'graph/query failed after rebuild (check server logs)'; | |
| } | |
| state.graph.stats = freshResults[1] || {}; |
🤖 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/viewer/index.html` around lines 1718 - 1726, When handling the
post-rebuild freshResults, ensure we set a visible query error if the subsequent
graph/query failed: check freshResults[0] (freshQuery) and if it is falsy,
assign state.graph.queryError to freshResults[1] (or a small default error
object/message) so the UI shows an error banner; otherwise proceed to clear/set
state.graph.nodes, edges, truncated, totalNodes, totalEdges and ensure
state.graph.queryError is cleared when the query succeeds. Reference symbols:
freshResults, freshQuery, state.graph, state.graph.queryError, graph/query.
rohitg00
added a commit
that referenced
this pull request
Jun 2, 2026
Bump to 0.9.25 across 9 files + CHANGELOG. Closes #778 #775 #783 (PR #791), #758 #726 (PR #773), #759 (PR #772), #752 (PR #774), #729 (PR #780), #781 (PR #782), #753 (PR #789), #771 (PR #786), #762 (PR #764). Files bumped: - package.json - packages/mcp/package.json - plugin/.claude-plugin/plugin.json - plugin/.codex-plugin/plugin.json - plugin/plugin.json - src/version.ts - src/types.ts (ExportData.version union) - src/functions/export-import.ts (supportedVersions Set) - test/export-import.test.ts (assertion) 125 test files / 1379 tests pass. npm audit (root + website): 0 vulns.
4 tasks
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.
Closes #753.
POST /agentmemory/graph/querywith an unbounded body{}on a corpus with 11k+ nodes returned HTTP 500Invocation stoppedbecause the response payload exceeded the iii state response channel ceiling. The viewer'sapi()returnsnullon non-2xx so the failure surfaced as "0 nodes / 0 edges" on a blank canvas with no signal. Same shape as #544; was never covered forgraph/query.Backend (
src/functions/graph.ts)mem::graph-queryacceptslimit+offseton every branch and applies a default cap (DEFAULT_GRAPH_QUERY_LIMIT = 500,MAX = 5000). An empty{}body never materializes the whole graph in one invocation.nodeType-only branch ranks nodes by incident-edge degree before paginating, so the truncated page surfaces the densest subgraph rather than an arbitrary KV scan order.totalNodes,totalEdges,truncated,limit,offset. Totals reflect the unbounded result for the given filter so callers render "showing X of Y" without re-querying.API (
src/triggers/api.ts)api::graph-querywhitelists payload fields (startNodeId,nodeType,maxDepth,query,limit,offset) per AGENTS.md security rule. No more rawreq.bodyspread.Viewer (
src/viewer/index.html)/graph/querysends{ limit: 500 }instead of{}.apiPost === null(server error) from{ nodes: [], edges: [] }(truly empty corpus). Failure renders a visibleGraph query failedbanner with a Retry button instead of the "No graph data yet. Building..." empty state.state.graphgainsqueryError,truncated,totalNodes,totalEdges.Tests (
test/graph.test.ts)4 new cases:
{}body caps to 500 withtruncated: trueon a 1,200-node seed; the 50 high-degree nodes land on page 1.limit+offsetpaginate without overlap on a 50-node seed.limit: 999999clamps toMAX_GRAPH_QUERY_LIMIT.totalEdgesstill counts unbounded.Full suite: 121 files / 1330 tests pass.
Edge cases handled
limitabove MAX: clamped to 5000.limitnon-finite / negative: falls back to default 500 / clamped to 1.offsetpast end: returns emptynodespage;totalNodesstill accurate so the viewer can stop paging.state.graph.stats.totalNodes === 0path preserved (still triggersgraph/build).Acceptance map (issue #753)
graph/queryaccepts + enforceslimit/offset, returnstotalNodes/totalEdges, default cap when body is{}graph/queryat 1,200+ nodes (proxy for 10k+)Summary by CodeRabbit
New Features
limitandoffsetparameters.Bug Fixes
Tests