Fix gitnexus-web server-mode repo switching and query readiness#400
Fix gitnexus-web server-mode repo switching and query readiness#400JayceeB1 wants to merge 2 commits intoabhigyanpatwari:mainfrom
Conversation
|
@JayceeB1 is attempting to deploy a commit to the NexusCore Team on Vercel. A member of the Team first needs to authorize it. |
|
thanks for the PR. can u resolve the conflicts |
xkonjin
left a comment
There was a problem hiding this comment.
Code Review
Overall: Fixes two real bugs: (1) LadybugDB query API compatibility across versions, and (2) server-mode repo switching not hydrating the graph database. Good refactoring of duplicated ingestion logic.
Positives
getQueryRowsadapter function correctly handles three different QueryResult shapes (getAllObjects→getAllRows→getAll), making the code resilient to LadybugDB WASM API changes.loadResultIntoWorkerStatededuplicates the graph loading + BM25 indexing that was previously copy-pasted acrossrunPipelineandrunPipelineFromFiles. The newhydrateServerGraphreuses it cleanly.- The
handleServerConnect→await hydrateServerGraph(result)call ensures server-mode connections get a queryable graph, fixing the core bug. - Type declaration update (
QueryResultmaking methods optional) matches the runtime adapter pattern.
Issues
-
getQueryRowspreference order may matter:getAllObjectsis preferred overgetAllRowsandgetAll. The existing code usedgetAll()which returns tuple arrays. IfgetAllObjectsreturns named-property objects, downstream code that accessesrow[0](tuple style) will break. The fallback chain in callers does handle both (row.cnt ?? row[0]), butexecuteQueryreconstructs named objects from tuples assuming array rows. IfgetAllObjectsalready returns objects, that reconstruction could double-wrap. Worth verifying which shape each method returns and whether the callers handle both consistently. -
Error swallowing in
hydrateServerGraph: TheloadResultIntoWorkerStatefunction catches LadybugDB errors silently. For server mode specifically, if LadybugDB fails to load, the user gets an "exploring" view with no query capability and no indication why. Consider surfacing a non-blocking warning. -
Missing
setProgress(null)timing: InApp.tsxline 159,setProgress(null)is called right afterhydrateServerGraphand before view transition. IfhydrateServerGraphthrows (e.g., corrupt server data),setProgress(null)is never reached and the UI stays stuck on the progress indicator. Wrap in try/finally. -
switchRepodependency array (useAppState.tsx ~1051):hydrateServerGraphwas added to the deps, which is correct. But theswitchRepofunction also callssetProgress(null)in its success path (line 1051) and in its error path (viasetTimeout). The success path sets progress to null but never sets it to something first, so the user sees no loading indicator during repo switching. Minor UX gap.
Minor
- The
try { ... } catch { }pattern inloadResultIntoWorkerState(empty catch) should at minimum log to console in dev mode, since the outer callers also have their own try/catch. Silent double-swallow makes debugging harder.
Solid fix. The query adapter pattern is the right long-term approach.
71957f1 to
4f2e4ac
Compare
|
Conflicts resolved. I rebased the branch on top of the current upstream main and force-pushed the updated head. Validation run locally on the updated branch:
The PR is now out of the DIRTY state on my side. |
|
@claude this is a bug fix, verify the code fixes the three bugs and give the initial workflow for the original issue this solves, suggest a unit tests to verify it works |
|
Claude finished @zander-raycraft's task in 1m 42s —— View job Bug Fix Verification & Analysis
Bug VerificationBug 1 — Status bar stuck on
|
|
@JayceeB1 good work on the bug fixes! Can you integrate these few tests above, test them, and then add that commit to this branch |
|
Integrated the requested regression tests and pushed them to this branch in a765be ( est: add server-mode hydration regressions). Added coverage for:
Validation run locally:
If you want, I can also add a follow-up React-level test for the progress-clearing path, but I kept this commit focused on the two pure regression surfaces with the least test harness overhead. |
|
@claude review this and make sure it is ready to merge |
|
Claude finished @zander-raycraft's task in 2m 48s —— View job PR Merge Readiness Review
Bug Fix VerificationBug 1 — Status bar stuck on
|
| # | Issue | Status |
|---|---|---|
| 1 | getAllObjects shape vs tuple reconstruction |
✅ Non-issue — executeQuery at lbug-adapter.ts:302 checks Array.isArray(row) before tuple-to-object conversion; named objects fall through unchanged |
| 2 | Silent error swallowing in loadResultIntoWorkerState |
ingestion.worker.ts:73–76, empty catch in prod is intentional ("LadybugDB is optional") |
| 3 | Missing setProgress(null) try/finally |
hydrateServerGraph throws on corrupt server data, setProgress(null) is unreachable in handleServerConnect. In the auto-connect path the outer .catch() handles it (with a 3-second delay), but the DropZone path calls handleServerConnect(result) without await at App.tsx:263, meaning a thrown error would be a silent unhandled rejection. Edge case (valid server data won't fail here), but a try/finally would be more robust. |
| 4 | No loading indicator during switchRepo |
✅ Actually fine — setProgress({ ... 'Switching repository...' }) is set at useAppState.tsx:1001 before the async work begins |
Additional Findings
Double hydration (intentional but worth noting): handleServerConnect calls await hydrateServerGraph(result) (synchronous, populates worker state immediately) and then immediately fires hydrateWorkerFromServer(...) as a background task, which re-runs the same BM25 + LadybugDB load. The second pass is needed to kick off agent init + embeddings after the worker is warm. Correct as designed, but LadybugDB gets loaded twice on each server connect/switch.
handleServerConnect not awaited from DropZone (pre-existing + worsened): At App.tsx:263, handleServerConnect(result) is fire-and-forget. Before this PR, the function had no await inside so this was harmless. Now that await hydrateServerGraph(result) is in the body, a failure there becomes an unhandled rejection in the DropZone path. The viewMode would remain 'onboarding' (not stuck on loading), so the user experience degrades gracefully, but it's cleaner to await it.
Tests
The two added test files have solid coverage of the regression surfaces:
query-result.test.ts— all four cases: prefersgetAllObjects, falls back togetAllRows, falls back togetAll, throws on no method. ✅ Completeserver-graph-hydration.test.ts— graph reconstruction (node count, relationship count, file map) andloadResultdelegation delegation. ✅ Complete
The author acknowledged the missing React-level setProgress(null) test and noted it was intentionally deferred.
CI Status
The CI run shows action_required — this is the Vercel deployment approval gate (as seen in earlier comments), not a test failure. The actual test suite result is not visible in CI output. Author confirmed npm run build and npm run test pass locally.
The Vercel deployment still needs manual authorization from a team member before it can be previewed.
Merge Readiness Verdict
This PR is ready to merge with one recommended fix before merging:
Recommended (non-blocking for merge): Wrap await hydrateServerGraph(result) + the subsequent state calls in a try/finally in handleServerConnect so setProgress(null) is guaranteed to be called even on unexpected errors. This prevents potential UI stuck states on corrupt server payloads. Fix this →
The three described bugs are correctly fixed, the refactoring is clean, tests cover the key regression paths, and the getQueryRows adapter is a solid long-term solution to the LadybugDB API versioning problem.
|
@JayceeB1 Can you fix that small thing above claude referenced? I will work on merging this after, thank you so much for the fast fix! |
Summary
This PR fixes three related problems in
gitnexus-webwhen loading repositories from a localgitnexus servebackend:Processing...QueryResult.getAll(), but newer@ladybugdb/wasm-corebuilds exposegetAllObjects()/getAllRows()insteadChanges
progressafter successful server-mode load/switchhydrateServerGraph(...)to load backend graph/file contents into local BM25 + LadybugDB stategetAllObjects()/getAllRows()withgetAll()as fallbackValidation
Validated locally in server mode on a non-default indexed repository:
ReadyCloses #398