Fix/explorer backend failure states - #980
Merged
Merged
Conversation
The dependency-pre-bundle overlay had no failure path: on a fetch error it kept rendering the last progress frame forever with no retry. Route isError/error out of the load query, surface a real error card with the underlying message, and let retry re-fetch without a full page reload.
The status dot and 'System Online' text were static, so a dead backend still looked healthy. Track checking/online/offline explicitly and drive both off the same state so they can't disagree.
The results strip had no close affordance and stayed pinned until the next search. Add a header row with a dismiss button, and round scores to whole numbers instead of showing three decimals of a raw relevance value nobody can act on.
Typing in the search box now debounces a query against the existing search endpoint and shows a combobox dropdown, with arrow-key navigation, Enter/click to jump straight to a node, and Escape to dismiss. Previously nothing happened until the full form was submitted.
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
PR Summary by QodoFix Explorer backend error states and improve graph search UX
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
🔒 Security Scan ResultsSafety — dependency vulnerabilities✅ No findings. Bandit — HIGH-severity code issues✅ No findings. Semgrep — static analysis patternsFound 55. Show all findings
This security scan runs automatically on source-code PRs and bi-weekly (skipped for doc/markdown-only changes). 📊 Security Policy: CI fails on Safety vulnerabilities and Bandit HIGH-severity findings. Semgrep findings above are informational and do not block merge. |
Code Review by Qodo
1.
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
…n error Clearing the search box while a suggestion fetch was in flight never aborted it, so a late response could reopen the dropdown with results for a query that was no longer typed. A non-OK response also left whatever suggestions were already on screen untouched instead of clearing them. Abort on every effect cleanup (not just unmount) and clear suggestions on any non-abort failure.
Collaborator
Author
|
Fixed Qodo's finding |
🔒 Security Scan ResultsSafety — dependency vulnerabilities✅ No findings. Bandit — HIGH-severity code issues✅ No findings. Semgrep — static analysis patternsFound 55. Show all findings
This security scan runs automatically on source-code PRs and bi-weekly (skipped for doc/markdown-only changes). 📊 Security Policy: CI fails on Safety vulnerabilities and Bandit HIGH-severity findings. Semgrep findings above are informational and do not block merge. |
🔒 Security Scan ResultsSafety — dependency vulnerabilities✅ No findings. Bandit — HIGH-severity code issues✅ No findings. Semgrep — static analysis patternsFound 55. Show all findings
This security scan runs automatically on source-code PRs and bi-weekly (skipped for doc/markdown-only changes). 📊 Security Policy: CI fails on Safety vulnerabilities and Bandit HIGH-severity findings. Semgrep findings above are informational and do not block merge. |
Sameer6305
previously requested changes
Aug 14, 2026
Collaborator
There was a problem hiding this comment.
Hi @ZohaibHassan16, verified the fix mechanism directly against the code, it's solid.
useLoadGraph() genuinely returns real isError/error/refetch from React Query, showLoadingOverlay correctly resolves once isError is true even with stale loadingProgress sitting around, Retry calls a real refetch() (not just a UI reset), and the landing page's checking/online/offline state is driven by an actual fetch outcome with no stuck-state path and no drift between the status dot and the 4th metric card. Good work on all of that.
One thing needs addressing before merge, though it's the root cause the issue itself called out, not just a symptom:
GraphWorkspaceShell.tsx already had a working isError/error implementation, and it's still dead code after this PR. It's not imported anywhere (confirmed — no file imports it, App.tsx loads GraphWorkspace directly). This PR adds the fix as a third, separate implementation in GraphWorkspace.tsx instead. So after this merges, we'd have three places with overlapping graph-loading/error-handling logic — GraphWorkspaceShell.tsx (dead), the old GraphWorkspace.tsx logic being replaced, and the new one — rather than fewer. The issue named "two copies that drifted apart" as the actual reason this bug slipped through in the first place, so leaving a third copy around undermines the fix.
Could you either:
- Delete GraphWorkspaceShell.tsx if it's genuinely unused/superseded, or
- If it's meant to be used somewhere (a route or feature flag I'm missing?), wire it up and reuse the same error-handling logic instead of duplicating it.
Either is fine, just want to avoid us being back here again in a few months with a fourth copy.
Small, non-blocking note: graphLoadError is typed Error | null in GraphWorkspace.tsx, but React Query actually returns it as unknown, the instanceof check handles it safely at runtime, so this isn't a bug, just worth tightening the type annotation while you're in there.
Requesting changes on the dead-code question, everything else here looks ready to go once that's resolved.
Collaborator
Author
|
Deliberately leaving the GraphWorkspaceShell.tsx deletion out of this PR, see #981 . Makes it easier to track and test. |
Sameer6305
dismissed
their stale review
August 14, 2026 11:27
Zohaib cleared it out.
Withdrawing my earlier request-changes approving.
Collaborator
Author
|
Also graphLoadError type is already correct as-is.
So I also checked it with TypeScript instead of only relying on the docs: const asError: Error | null = error; // works
const asString: string = error; // TS2322The compiler error names the type directly, so that confirms it too. I’d still keep the But there’s no |
Collaborator
|
@KaifAhmad1 approved from my side. |
KaifAhmad1
previously approved these changes
Aug 14, 2026
KaifAhmad1
left a comment
KaifAhmad1
left a comment
Contributor
There was a problem hiding this comment.
Verified independently (worktree + `npm ci`), not just reading the description:
- Confirmed in `useLoadGraph.ts` that `useQuery` returns the full result (isError/error/refetch all real) with `retry: 0`, and the uncaught `fetch()` inside `queryFn` genuinely propagates a backend-down failure instead of being swallowed — matches the root cause in #977.
- `npx tsc -b`: clean.
- `test:graph-store` / `test:graph-workspace` / `test:plugin-registry`: 42/42 passed.
- `npm run build`: succeeds.
- Traced the stale-suggestion abort fix (`AbortController` in the typeahead effect) — correctly cancels in-flight requests on re-debounce and on selection-clear.
Thanks @ZohaibHassan16 for the thorough writeup and testing notes, and @Sameer6305 for catching the dead-code question early and getting it properly scoped out to #981 instead of blocking here. Approving for merge.
KaifAhmad1
previously approved these changes
Aug 14, 2026
KaifAhmad1
left a comment
KaifAhmad1
left a comment
Contributor
There was a problem hiding this comment.
Re-approving after the changelog-only follow-up commit — no code changed, review above still stands.
…failure-states # Conflicts: # CHANGELOG.md
KaifAhmad1
approved these changes
Aug 14, 2026
KaifAhmad1
left a comment
KaifAhmad1
left a comment
Contributor
There was a problem hiding this comment.
Re-approving after merging main to resolve the CHANGELOG.md conflict with #932 — no functional code touched, review above still stands.
🔒 Security Scan ResultsSafety — dependency vulnerabilities✅ No findings. Bandit — HIGH-severity code issues✅ No findings. Semgrep — static analysis patternsFound 55. Show all findings
This security scan runs automatically on source-code PRs and bi-weekly (skipped for doc/markdown-only changes). 📊 Security Policy: CI fails on Safety vulnerabilities and Bandit HIGH-severity findings. Semgrep findings above are informational and do not block merge. |
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
3 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
Fixes #977.
This fixes a few Explorer UI issues around backend errors and search.
The main issue was that if the backend was down or returned an error, the graph page would just keep showing the loading/progress UI forever. The landing page also always showed "System Online" even when the backend could not be reached.
I also fixed a few smaller search UX issues I found while testing this.
Changes
Graph load error state + retry
useLoadGraph()already returnedisErroranderror, but they were not being used.Because retries are disabled (
retry: 0), a failed graph request would leave the loading overlay stuck on the last progress state. There was no error message or retry option, so the only way to try again was refreshing the whole page.GraphLoadingOverlaynow takeserrorandonRetry.When loading fails, it shows:
Landing page connection status
The landing page status was hardcoded to always show the green dot and "System Online".
I replaced the old
readyboolean with three states:checkingonlineofflineThe status dot, text and fourth metric card now all use the same state, so they should stay in sync.
When the backend cannot be reached, the page now shows "Backend Unreachable" instead of "System Online".
Search result dismiss + score display
The search results strip did not have a close button, so it stayed open and kept taking space from the graph until another search happened.
I added a small header with:
I also changed relevance scores to display as whole numbers instead of values like
96.900or138.000.Search typeahead
The graph search input only did something after submitting the form.
I added a debounced typeahead using the existing search endpoint.
Current behavior:
aria-activedescendanttracks the active resultSelecting a result also clears the current query.
Testing
I tested these against the running app:
aria-activedescendantupdated with the selected suggestionnpx tsc -bpassesnpm run test:graph-store,test:graph-workspaceandtest:plugin-registry: 42/42 tests passnpm run buildsucceeds