Repository navigation
fix(desktop): retry evidence lookups without breaking trace navigation - #11
Conversation
nishantmodak
left a comment
nishantmodak
left a comment
There was a problem hiding this comment.
The retry path now re-fetches the failed source, but the evidence-navigation refactor introduces two user-visible regressions that should be fixed before merging: a repeated evidence click can leave a different trace selected, and an unsuccessful lookup now closes the dialog without an error.
I reproduced both with component tests against the actual App/useLive code: both tests pass on the base commit and fail on this head. The existing 23 desktop tests pass locally, and the reported frontend, core, and macOS bundle CI checks are green. Please retain the retry behavior while restoring reliable navigation and explicit lookup failure handling. Native WebView smoke testing was not run in this review.
| function inspectEvidence(reference: string) { | ||
| const [trace, span] = reference.split("/"); | ||
| try { | ||
| const page = await api.read<RecentTracePage>( | ||
| `traces?limit=200&q=${trace}`, | ||
| ); | ||
| const target = page.traces.find( | ||
| (t) => t.run_id === runId && t.trace_id === trace, | ||
| ); | ||
| if (!target) throw new Error("Referenced trace was not found."); | ||
| setSelection(target); | ||
| setRequestedSpan(span); | ||
| setShowRun(false); | ||
| setEvidenceError(""); | ||
| } catch (error) { | ||
| setEvidenceError(String(error)); | ||
| } | ||
| setEvidenceRef(trace); | ||
| setRequestedSpan(span); | ||
| setShowRun(false); |
There was a problem hiding this comment.
[P2] Treat each evidence click as a new navigation request
Open an evidence link for trace A, select trace B from the list in the same run, then open the same evidence link for A again. evidenceRef is still A, so setEvidenceRef(trace) does not change the hook key; referenced.data, evidenceRef, and runId are also unchanged, so the selection effect does not run. The dialog closes while B remains selected, and A's requested span is applied to B's inspector. I reproduced this with an A → B → A component test that passes on the base and fails on this head. Give each click a fresh request identity, or explicitly navigate using a matching cached result, with the run included in the request identity. Add this repeated-navigation regression test.
nishantmodak
changed the title
nishantmodak
left a comment
nishantmodak
left a comment
There was a problem hiding this comment.
Approved after re-review. Both previous blockers are addressed: each evidence action can trigger a fresh lookup, completed/cancelled requests are cleared, and an absent trace produces a visible, retryable error. The selection and requested span are now committed only after a matching run/trace is found. I also checked cancellation when the user selects another trace while the lookup is pending.
All 26 desktop tests passed locally on this head, including repeated navigation, missing-evidence recovery, transient-error retry, and stale-response coverage. Frontend, Linux/macOS core, and macOS bundle CI checks pass. No remaining blocking findings. Native WebView smoke testing was not run in this review.
0935709 to
9d6e62e
Compare
1 participant
Evidence lookups can fail independently of the desktop’s polling requests. The error-banner Retry button now repeats the failed lookup and opens the referenced trace and span when it succeeds.
Each lookup captures its run, trace, and span. Completed requests are cleared so the same evidence link works again after navigating elsewhere. A missing matching trace produces a visible, retryable error, and selecting another trace cancels pending evidence navigation so late responses cannot overwrite that selection. Span selection is applied only after the target trace is found.
Validation: all 26 frontend tests pass with coverage, along with Prettier and the TypeScript/Vite production build. Regression tests cover transient failure and retry, reopening the same evidence after navigation, missing evidence and recovery, and ignoring a late lookup after selecting another trace.
Closes #5