fix(intelligence): Graph Reach — BFS gate, tab wiring, i18n flat (#2985)#3190
Closed
staimoorulhassan wants to merge 2 commits into
Closed
fix(intelligence): Graph Reach — BFS gate, tab wiring, i18n flat (#2985)#3190staimoorulhassan wants to merge 2 commits into
staimoorulhassan wants to merge 2 commits into
Conversation
added 2 commits
May 31, 2026 06:38
…n flat (tinyhumansai#2985) - Add MAX_REACH_NODES (5000) safety cap with isGraphReachFeasible() check to prevent BFS-from-every-node O(V*(V+E)) from freezing large graphs - Wire GraphReachTab into Intelligence.tsx (import, type union, tab entry, render) - Add memory.tab.reach + 21 graphReach.* i18n keys to en.ts and all 13 locales with real translations (flat single-file format) - Fix French/Italian apostrophe quoting (single→double quotes for l'excentricité etc.)
# Conflicts: # app/src/lib/i18n/ar.ts # app/src/lib/i18n/bn.ts # app/src/lib/i18n/de.ts # app/src/lib/i18n/es.ts # app/src/lib/i18n/fr.ts # app/src/lib/i18n/hi.ts # app/src/lib/i18n/id.ts # app/src/lib/i18n/it.ts # app/src/lib/i18n/ko.ts # app/src/lib/i18n/pl.ts # app/src/lib/i18n/pt.ts # app/src/lib/i18n/ru.ts # app/src/lib/i18n/zh-CN.ts # app/src/pages/Intelligence.tsx
Contributor
📝 WalkthroughWalkthroughAdds a complete Graph Reach feature: a pure graph algorithm computing per-node eccentricity and component metrics; an API facade fetching data and applying the algorithm; React presentational and container components for visualization with namespace filtering; and i18n translations for all UI text. ChangesGraph Reach Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
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. 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
added
feature
![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 (4)
app/src/components/intelligence/GraphReachTab.tsx (1)
40-47: ⚡ Quick winUse
async/awaitconsistently in the mount effect.The
loadNamespaces()chain here is the only promise flow in this component that still uses.then/.catch, which conflicts with the repo’s TypeScript convention and makes the effect harder to read alongsideload().♻️ Suggested refactor
useEffect(() => { // Namespaces are optional UI sugar; a failure to list them must not block // the reach view, so swallow that error specifically. - loadNamespaces() - .then(setNamespaces) - .catch(() => setNamespaces([])); + const initNamespaces = async (): Promise<void> => { + try { + setNamespaces(await loadNamespaces()); + } catch { + setNamespaces([]); + } + }; + + void initNamespaces(); void load(''); }, [load]);As per coding guidelines, "Always use async/await for promises in TypeScript".
🤖 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 `@app/src/components/intelligence/GraphReachTab.tsx` around lines 40 - 47, Refactor the mount useEffect to use an async function with await instead of .then/.catch: create an async inner function invoked immediately that awaits loadNamespaces() and calls setNamespaces(result) but catches and sets setNamespaces([]) on error, and also awaits or calls load('') in the same async flow; reference the existing loadNamespaces, setNamespaces, load, and useEffect symbols so the behavior (namespaces are optional and errors are swallowed) remains identical while using async/await.app/src/components/intelligence/GraphReachTab.test.tsx (1)
41-60: ⚡ Quick winAdd a stale-response regression test for the request-id guard.
The core correctness safeguard in
GraphReachTabis that an olderloadReach()result must not overwrite a newer namespace selection, but this suite never exercises that path. A deferred-promise test that resolves the first request after the second one would lock in the main race-condition protection of the container.🤖 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 `@app/src/components/intelligence/GraphReachTab.test.tsx` around lines 41 - 60, Add a regression test in GraphReachTab.test.tsx that exercises the request-id guard by using deferred promises for mockLoadReach: when rendering GraphReachTab trigger two sequential loadReach calls (initial mount and a namespace change via the combobox) but resolve the second promise before the first, then assert that the UI reflects only the second (latest) response and that the first (stale) resolution does not overwrite it; use mockLoadReach, mockLoadNamespaces, render(<GraphReachTab />), fireEvent.change on the combobox, and waitFor assertions to verify the stale-response protection.app/src/services/api/graphReachApi.ts (1)
25-26: ⚡ Quick winAwait the namespace RPC in this async service method.
This works, but it violates the repo’s TypeScript rule to use async/await for promises and makes local logging/error handling harder to extend later.
export async function loadNamespaces(): Promise<string[]> { - return memoryListNamespaces(); + return await memoryListNamespaces(); }As per coding guidelines, "Always use async/await for promises in TypeScript."
🤖 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 `@app/src/services/api/graphReachApi.ts` around lines 25 - 26, The function loadNamespaces currently returns the Promise from memoryListNamespaces directly; change it to await the RPC call inside the async function so the promise is resolved before returning and future logging/error handling can be added. Edit loadNamespaces to call and await memoryListNamespaces() (referencing the loadNamespaces function and memoryListNamespaces symbol), assign or return the awaited result, and preserve the Promise<string[]> signature.app/src/lib/memory/graphReach.test.ts (1)
103-171: ⚡ Quick winAdd boundary coverage for the new feasibility guard.
This suite locks down
computeGraphReach()well, but it never exercisesisGraphReachFeasible()at theMAX_REACH_NODESboundary. That leaves the new freeze-prevention contract unprotected. Please add assertions for exactly 5,000 distinct nodes and 5,001 distinct nodes.🤖 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 `@app/src/lib/memory/graphReach.test.ts` around lines 103 - 171, The tests for computeGraphReach are missing coverage for the new MAX_REACH_NODES feasibility guard (isGraphReachFeasible); add two cases that construct inputs with exactly MAX_REACH_NODES distinct node ids and MAX_REACH_NODES+1 distinct node ids, call computeGraphReach on each, and assert that the first call returns a normal reach result (nodeCount === MAX_REACH_NODES and no feasibility error) while the second triggers the feasibility cutoff (e.g., nodeCount === MAX_REACH_NODES or an explicit indicator your implementation uses when exceeding MAX_REACH_NODES); locate the new tests near the existing 'is order-independent' or normalization block and reference computeGraphReach, isGraphReachFeasible, and MAX_REACH_NODES to guide placement.
🤖 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 `@app/src/services/api/graphReachApi.ts`:
- Around line 18-21: The loadReach function must enforce the 5,000-node cap
before calling computeGraphReach: after obtaining relations from
memoryGraphQuery(namespace) check the node count (relations.length or derived
node count as appropriate) and if it exceeds the cap, return/throw an error or a
ReachResult indicating the oversized-graph condition instead of calling
computeGraphReach; update loadReach to log the overflow case and short-circuit
(e.g., return a failure/empty ReachResult or reject) so computeGraphReach is
never executed on oversized graphs.
---
Nitpick comments:
In `@app/src/components/intelligence/GraphReachTab.test.tsx`:
- Around line 41-60: Add a regression test in GraphReachTab.test.tsx that
exercises the request-id guard by using deferred promises for mockLoadReach:
when rendering GraphReachTab trigger two sequential loadReach calls (initial
mount and a namespace change via the combobox) but resolve the second promise
before the first, then assert that the UI reflects only the second (latest)
response and that the first (stale) resolution does not overwrite it; use
mockLoadReach, mockLoadNamespaces, render(<GraphReachTab />), fireEvent.change
on the combobox, and waitFor assertions to verify the stale-response protection.
In `@app/src/components/intelligence/GraphReachTab.tsx`:
- Around line 40-47: Refactor the mount useEffect to use an async function with
await instead of .then/.catch: create an async inner function invoked
immediately that awaits loadNamespaces() and calls setNamespaces(result) but
catches and sets setNamespaces([]) on error, and also awaits or calls load('')
in the same async flow; reference the existing loadNamespaces, setNamespaces,
load, and useEffect symbols so the behavior (namespaces are optional and errors
are swallowed) remains identical while using async/await.
In `@app/src/lib/memory/graphReach.test.ts`:
- Around line 103-171: The tests for computeGraphReach are missing coverage for
the new MAX_REACH_NODES feasibility guard (isGraphReachFeasible); add two cases
that construct inputs with exactly MAX_REACH_NODES distinct node ids and
MAX_REACH_NODES+1 distinct node ids, call computeGraphReach on each, and assert
that the first call returns a normal reach result (nodeCount === MAX_REACH_NODES
and no feasibility error) while the second triggers the feasibility cutoff
(e.g., nodeCount === MAX_REACH_NODES or an explicit indicator your
implementation uses when exceeding MAX_REACH_NODES); locate the new tests near
the existing 'is order-independent' or normalization block and reference
computeGraphReach, isGraphReachFeasible, and MAX_REACH_NODES to guide placement.
In `@app/src/services/api/graphReachApi.ts`:
- Around line 25-26: The function loadNamespaces currently returns the Promise
from memoryListNamespaces directly; change it to await the RPC call inside the
async function so the promise is resolved before returning and future
logging/error handling can be added. Edit loadNamespaces to call and await
memoryListNamespaces() (referencing the loadNamespaces function and
memoryListNamespaces symbol), assign or return the awaited result, and preserve
the Promise<string[]> signature.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3fa2ca39-d1cd-48b6-a193-5a2f3103642d
📒 Files selected for processing (9)
app/src/components/intelligence/GraphReachPanel.test.tsxapp/src/components/intelligence/GraphReachPanel.tsxapp/src/components/intelligence/GraphReachTab.test.tsxapp/src/components/intelligence/GraphReachTab.tsxapp/src/lib/i18n/en.tsapp/src/lib/memory/graphReach.test.tsapp/src/lib/memory/graphReach.tsapp/src/services/api/graphReachApi.test.tsapp/src/services/api/graphReachApi.ts
Comment on lines
+18
to
+21
| export async function loadReach(namespace?: string): Promise<ReachResult> { | ||
| const relations = await memoryGraphQuery(namespace); | ||
| log('loadReach namespace=%s relations=%d', namespace ?? '(all)', relations.length); | ||
| return computeGraphReach(relations); |
Contributor
There was a problem hiding this comment.
Enforce the node cap inside loadReach().
Line 21 still runs computeGraphReach() unconditionally. Any caller that forgets to pre-check can hit the O(V·(V+E)) path and freeze the renderer, which is exactly what the new 5,000-node safeguard is meant to prevent. Reject oversized graphs here before computing so the UI can surface a warning/error state instead.
Suggested fix
-import { computeGraphReach, type ReachResult } from '../../lib/memory/graphReach';
+import {
+ MAX_REACH_NODES,
+ computeGraphReach,
+ isGraphReachFeasible,
+ type ReachResult,
+} from '../../lib/memory/graphReach';
import { memoryGraphQuery, memoryListNamespaces } from '../../utils/tauriCommands/memory';
@@
export async function loadReach(namespace?: string): Promise<ReachResult> {
const relations = await memoryGraphQuery(namespace);
+ if (!isGraphReachFeasible(relations)) {
+ throw new Error(`Graph Reach is limited to ${MAX_REACH_NODES} nodes`);
+ }
log('loadReach namespace=%s relations=%d', namespace ?? '(all)', relations.length);
return computeGraphReach(relations);
}🤖 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 `@app/src/services/api/graphReachApi.ts` around lines 18 - 21, The loadReach
function must enforce the 5,000-node cap before calling computeGraphReach: after
obtaining relations from memoryGraphQuery(namespace) check the node count
(relations.length or derived node count as appropriate) and if it exceeds the
cap, return/throw an error or a ReachResult indicating the oversized-graph
condition instead of calling computeGraphReach; update loadReach to log the
overflow case and short-circuit (e.g., return a failure/empty ReachResult or
reject) so computeGraphReach is never executed on oversized graphs.
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
2 participants
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
Addresses review findings from PR #2985 (Graph Reach — eccentricity / diameter / radius):
MAX_REACH_NODES = 5_000constant andisGraphReachFeasible()check ingraphReach.ts. BFS-from-every-node is O(V·(V+E)); beyond 5k nodes the computation could freeze the browser.memory.tab.reach+ 21graphReach.*keys added toen.tsand all 13 non-English locale files with real translations. French/Italian apostrophe quoting fixed.Test plan
pnpm typecheckpasses (verified locally)pnpm format:checkpasses (verified locally)graphReach.test.ts,GraphReachPanel.test.tsx,GraphReachTab.test.tsxpassisGraphReachFeasible()returns false for graphs with >5000 nodesSummary by CodeRabbit
New Features