Skip to content

fix(ui): handle indexing errors in frontend and ignore local npm cache#549

Closed
sahil-mangla wants to merge 1 commit into
DeusData:mainfrom
sahil-mangla:main
Closed

fix(ui): handle indexing errors in frontend and ignore local npm cache#549
sahil-mangla wants to merge 1 commit into
DeusData:mainfrom
sahil-mangla:main

Conversation

@sahil-mangla

Copy link
Copy Markdown
Contributor

Surface indexing failures in UI and enable backend API startup in development mode

What does this PR do?

This PR addresses a poor user experience when indexing fails, particularly for extremely large repositories that may exhaust available memory.

Changes

  • Fixed the frontend to render a visible error banner when an indexing job fails (status === "error") instead of silently dismissing the progress indicator.
  • Preserved the existing success flow for completed indexing jobs.
  • Allowed the backend HTTP server to start API endpoints in development mode even when no frontend assets are embedded, improving the local development workflow.

Why is this needed?

During investigation of issue #524, I reproduced indexing failures using a synthetic repository containing approximately 200,000 Python files.

The indexing subprocess was terminated by the OS with exit code 137 (SIGKILL / out-of-memory condition). The backend correctly transitioned the job status to "error".

However, the frontend previously treated any non-"indexing" state as completion:

if (data.length > 0 &&
data.every((j) => j.status !== "indexing")) {
onDone();
}

As a result:

  1. The indexing spinner disappeared.
  2. No database was generated.
  3. The project did not appear in the UI.
  4. No error message was shown to the user.

This made indexing failures appear as if indexing had silently completed or hung.

This PR surfaces those failures directly in the UI so users receive immediate feedback when indexing fails.

Reproduction

  1. Start the UI and backend.
  2. Index a very large repository (or a synthetic repository with ~200k files).
  3. Force the indexer to fail (for example via OOM conditions).
  4. Observe that the UI now displays an error banner instead of silently dismissing the indexing job.

Testing

Manual testing performed

  • Indexed the following repositories successfully:
    • psf/requests
    • pallets/flask
    • tiangolo/fastapi
    • django/django
    • home-assistant/core
    • pytorch/pytorch

Verified that:

  • Successful indexing still dismisses the spinner as expected.
  • Failed indexing jobs now surface an error message in the UI.
  • Development mode API endpoints continue to function when frontend assets are not embedded.

Checklist

  • Every commit is signed off (git commit -s)
  • Tests pass locally (make -f Makefile.cbm test)
  • Lint passes (make -f Makefile.cbm lint-ci)
  • New behavior is covered by a test

@sahil-mangla sahil-mangla force-pushed the main branch 2 times, most recently from d9550be to bee202e Compare June 21, 2026 19:00
@sahil-mangla

Copy link
Copy Markdown
Contributor Author

Linter Fix (clang-format): The second force-push was made to wrap a long string literal in src/main.c that exceeded the maximum line length check, which was causing the CI formatting job (lint-format) to fail. No logic changes were introduced.

@sahil-mangla

Copy link
Copy Markdown
Contributor Author

Hey @DeusData shall i close the PR or should i wait for you to merge it first?

@DeusData

Copy link
Copy Markdown
Owner

Hey @sahil-mangla, a bit of patience please. At the moment there is a lot of "background work" happening. Coming back to you ASAP :)

@DeusData

Copy link
Copy Markdown
Owner

Thanks @sahil-mangla — the UI error-banner half is good and we'd like it.

The blocker is the src/main.c change: it removes the CBM_EMBEDDED_FILE_COUNT > 0 guard, so the HTTP server now starts whenever --ui is set, even in the standard binary with no embedded assets. The server binds loopback only (so it's local-only, not a network exposure), but it still turns a documented no-op into a live listener — a behavior-contract change.

Could you split this PR: keep the UI error-handling, and gate the always-start-server behavior behind an explicit opt-in (e.g. a --dev-api flag) rather than removing the asset guard? The UI half can merge as soon as it's separated. 🙏

@sahil-mangla

Copy link
Copy Markdown
Contributor Author

@DeusData
Thanks for the review and for clarifying the concern.

I agree that removing the CBM_EMBEDDED_FILE_COUNT > 0 guard changes the existing behavior contract for --ui, even if the listener is loopback-only.

I’ll split the changes and update this PR to keep only the UI error-handling improvements. I’ll revert the src/main.c change from this PR and keep the development-server behavior as a separate follow-up, potentially behind an explicit opt-in flag such as --dev-api.

@DeusData

Copy link
Copy Markdown
Owner

Thanks @sahil-mangla — the StatsTab.tsx error-banner fix itself is good and we'd like it. But the PR currently contains 709 changed files / +3022 lines: ~706 are graph-ui/.npm-cache-local/ npm-cache blobs that got committed, plus scratch/generate_stress_repo.py (which is gitignored and shouldn't be pushed) and some unrelated package-lock.json churn.

Could you reduce this to just the StatsTab.tsx change — remove the .npm-cache-local/ tree and the scratch script, add graph-ui/.npm-cache-local/ to .gitignore so it can't recur, and drop the lock-file churn? Once it's down to the single file, it's good to go. 🙏

@sahil-mangla sahil-mangla force-pushed the main branch 2 times, most recently from c67bf0c to 4a6ad9b Compare June 24, 2026 05:21
@sahil-mangla

Copy link
Copy Markdown
Contributor Author

@DeusData Can you check and verify if any other changes or something looks off to you i will fix them right away or is it all good.

@DeusData

Copy link
Copy Markdown
Owner

Thanks @sahil-mangla — the frontend fix is a real bug: StatsTab.tsx treated an error status as "done" (data.every(j => j.status !== "indexing")), silently dismissing failures, and splitting "still indexing" from "has errors" is the right fix.

Two things before it lands:

  1. Title/diff mismatch: the title says "…and enable dev mode API hosting", but the diff only touches graph-ui/src/components/StatsTab.tsx and .gitignore — there's no backend/dev-mode/API-hosting change here. Please update the title to match what the PR actually does (the error-handling fix + the npm-cache .gitignore entry) so the merge commit doesn't record a feature that isn't present. If the hosting change was meant to be included, it's missing from the diff.
  2. Test: a small vitest/RTL test for the error path (status error renders the banner and doesn't call onDone) would lock this in.

The .gitignore npm-cache line is unrelated to the fix — fine to keep, just noting. Thanks!

@sahil-mangla

Copy link
Copy Markdown
Contributor Author

@DeusData Will you please check it out now!

@sahil-mangla sahil-mangla requested a review from DeusData as a code owner June 29, 2026 07:46
@DeusData DeusData added bug Something isn't working ux/behavior Display bugs, docs, adoption UX priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. labels Jun 29, 2026
@sahil-mangla

Copy link
Copy Markdown
Contributor Author

@DeusData anything else needed please let me know!

@DeusData

DeusData commented Jul 1, 2026

Copy link
Copy Markdown
Owner

Thanks for the UI/dev-mode fix. The non-merge commit appears signed; if CI flags only the branch merge commits for DCO, please refresh the branch. Please also include the frontend test command/output and confirm the dev API hosting remains local-only.

@sahil-mangla

Copy link
Copy Markdown
Contributor Author

Hi! I have rebased the branch on upstream/main to remove the unsigned merge commits and force-pushed the update. The PR now contains only the single signed commit. Also verified that the dev API hosting is strictly loopback (local-only)

@DeusData

DeusData commented Jul 3, 2026

Copy link
Copy Markdown
Owner

Thank you — the core fix is right and wanted: the backend's /api/index-status already emits status:error with detail, and the UI was silently swallowing it; this is also fully complementary to the per-file skipped[] reporting that just landed server-side (different channel: whole-job failure vs per-file skips). Three asks before merge: (1) retitle/re-describe — the 'dev mode API hosting' half no longer exists in the diff after your force-push, and the title still claims it; (2) please change 'fixes #524' to 'refs #524' — #524's OOM root cause isn't fixed by surfacing the error, and we don't want the merge to auto-close it; (3) optional: route 'Indexing Failed'/'Dismiss' through useUiMessages() so the zh locale stays complete. Thanks!


Update: to keep momentum on the bug backlog, we're going to carry the changes above over the line ourselves shortly — a distilled follow-up on current main implementing the notes in this thread, with you credited as Co-authored-by on the commit, and this PR closed referencing it. If you'd prefer to push the update yourself, just reply within the next couple of days and we'll gladly take yours instead. Thanks again for the contribution!

…e (refs DeusData#524)

Signed-off-by: sahil-mangla <manglasahil2017@gmail.com>
@sahil-mangla sahil-mangla changed the title fix(ui): handle indexing errors in frontend and enable dev mode API hosting fix(ui): handle indexing errors in frontend and ignore local npm cache Jul 3, 2026
@sahil-mangla

Copy link
Copy Markdown
Contributor Author

Thanks for the guidance! I have made the requested updates:

  1. Title & Description: Updated the PR title and description on GitHub to remove references to the dev-mode API hosting, focusing strictly on the indexing error handling and npm cache gitignore.
  2. Commit Message: Amended the commit message to change fixes #524 to refs #524 and force-pushed.
  3. i18n Localization: Localized the "Indexing Failed" (t.projects.indexingFailed) and "Dismiss" (t.common.dismiss) buttons through useUiMessages(), including translations for both the en and zh locales.

All tests run and pass successfully:

# inside graph-ui
npm test

JadeCong pushed a commit to CloudEngineHub/codebase-memory-mcp that referenced this pull request Jul 4, 2026
…leting

The backend /api/index-status reports status:"error" plus an error
message for failed indexing jobs, but IndexProgress treated any
non-"indexing" state as successful completion: the spinner vanished
with no feedback (e.g. after an OOM-killed indexer subprocess), the
project never appeared, and no error was shown.

Render a visible error banner (path + error text) with a Dismiss
button instead, and keep the success flow unchanged. Beyond the
original PR:

- Restore the empty-jobs guard the PR dropped: the backend keeps
  finished jobs listed as "done"/"error" (handle_index_status only
  skips idle slots), so an empty list mid-index only occurs on
  transient state loss and must not be treated as completion.
- Route the new user-facing strings through the i18n system
  (projects.indexingFailed, common.dismiss; EN + zh entries).
- Cover error banner + dismiss, success flow and the empty-jobs
  guard with vitest cases on the now-exported IndexProgress.

Distilled from DeusData#549.

Refs DeusData#524

Co-authored-by: sahil-mangla <manglasahil2017@gmail.com>
Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
@DeusData

DeusData commented Jul 4, 2026

Copy link
Copy Markdown
Owner

Thank you — the core fix was right and needed: the backend already emitted status:error with detail and the UI was silently swallowing it. We carried it over the line as bfd9b89 (PR #816) with you credited as co-author, with the review deltas folded in: the empty-jobs guard restored (verified against the backend's status transitions — an empty list can only mean transient state loss, so treating it as success would be wrong), the new strings routed through i18n (EN+zh), and the issue reference kept as 'refs #524' since the OOM root cause there is still open. Your test shapes carried over almost verbatim. Closing in favor of the distill — thanks again!

@DeusData DeusData closed this Jul 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. ux/behavior Display bugs, docs, adoption UX

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants