Claude finished @Yeraze's task —— View job

PR Review: fix(traceroute): scope traceroute history to the active source

• Gather context and read changed files
• Review code quality and best practices
• Check for bugs or issues
• Evaluate test coverage
• Post final review

Overall: This is a clean, minimal, correct fix that follows the codebase's established source-scoping patterns. No backend changes were necessary — the infrastructure was already there; this PR simply threads the sourceId through the frontend chain. Ready to merge with two minor notes below.

Code Review

src/services/api.ts — getTracerouteHistory (line 738)

The implementation is consistent with how other methods in this file handle the sourceId parameter. One subtle inconsistency: the URL is built by appending ?limit=... first and then optionally appending &sourceId=.... This works, but unlike most of the other methods in this file that use URLSearchParams for cleaner param construction (e.g., getMessages, getChannelMessages). Not a bug, but worth a note for future consistency.

// Current
const sourceQuery = sourceId ? `&sourceId=${encodeURIComponent(sourceId)}` : '';
const response = await fetch(`...?limit=${limit}${sourceQuery}`);

// Consistent with the rest of the file would be:
const params = new URLSearchParams({ limit: String(limit) });
if (sourceId) params.set('sourceId', sourceId);
const response = await fetch(`...?${params}`);

src/components/TracerouteHistoryModal.tsx — useEffect dependency array (line 64)

The [fromNodeNum, toNodeNum, sourceId] dep array is correct and the isMounted ref guard pattern is solid. The effect will correctly re-fetch when the user switches sources mid-session.

src/App.tsx — sourceId={sourceId} (line 4703)

Straightforward. sourceId comes from useSource() at the top of the component. The prop was already wired in the right place — this is exactly the one-liner that was needed.

Bug / Potential Issue

src/services/api.test.ts — stale test at line 808 (not in this PR's changed files, but the PR broke it)

The existing test calls getTracerouteHistory(111, 222, 20) — but with the new signature getTracerouteHistory(fromNodeNum, toNodeNum, sourceId?, limit), the value 20 is now interpreted as the sourceId (a string | null | undefined) rather than the limit. The test asserts:

expect(mockFetch).toHaveBeenCalledWith('/api/traceroutes/history/111/222?limit=20');

But with the new code, passing 20 (a number) as sourceId will hit sourceId ? ... as truthy, so it'll produce ?limit=50&sourceId=20 — mismatching the assertion. The PR description says "23/23 pass," but this test looks like it should be failing unless TypeScript/the mock silently coerces it. Worth a double-check and an update to the api.test.ts call to use the new signature: getTracerouteHistory(111, 222, undefined, 20) (or getTracerouteHistory(111, 222, null, 20)).

Test Coverage

The new test in TracerouteHistoryModal.test.tsx (line 157–173) is exactly the right regression test — it verifies sourceId is passed through to the API call. The existing "no sourceId" tests now correctly assert ...CalledWith(100, 200, undefined) after the update.

The one gap is there's no test that verifies the API service actually appends &sourceId=... to the URL when a sourceId is provided (the api.test.ts file appears unchanged for this PR).

Summary

Claude finished @Yeraze's task —— View job

PR Review: fix(traceroute): scope traceroute history to the active source

• Gather context and read changed files
• Review code quality and best practices
• Check for bugs or issues
• Evaluate test coverage
• Post final review

Overall: Clean, minimal, correct fix. The infrastructure was already in place on the backend; this PR simply threads sourceId through the frontend call chain. The previous review's concern about api.test.ts breaking was a false alarm — see details below.

Code Review

src/services/api.ts — signature change (line 738)

async getTracerouteHistory(fromNodeNum: number, toNodeNum: number, limit: number = 50, sourceId?: string | null)

The new sourceId is appended as a 4th parameter, preserving limit in its existing 3rd-argument position. This is the right call — it maintains backward compatibility with any existing callers that pass an explicit limit. The URL construction works correctly:

const sourceQuery = sourceId ? `&sourceId=${encodeURIComponent(sourceId)}` : '';
const response = await fetch(`...?limit=${limit}${sourceQuery}`);

Minor style note: other methods in this file use URLSearchParams for cleaner parameter construction. Not a bug, just slightly inconsistent with the surrounding code.

src/components/TracerouteHistoryModal.tsx — call site (line 47)

await ApiService.getTracerouteHistory(fromNodeNum, toNodeNum, undefined, sourceId);

Correct — passes undefined for limit (uses the = 50 default) and sourceId as the new 4th arg. The useEffect dep array [fromNodeNum, toNodeNum, sourceId] is correct and will re-fetch when the user switches sources mid-session.

src/App.tsx — prop threading (line ~4703)

<TracerouteHistoryModal
  ...
  sourceId={sourceId}
  ...
/>

Simple one-liner. sourceId comes from useSource() at the top of the component, consistent with how every other source-scoped component in this file is wired.

Previous Review's api.test.ts Concern — Resolved / False Alarm

The earlier review worried that api.test.ts:808:

await apiService.getTracerouteHistory(111, 222, 20);

…would break because 20 would be misinterpreted as sourceId. This is not the case. The new signature kept limit as the 3rd argument (limit: number = 50) and added sourceId as the 4th. Passing 20 as the third argument still sets limit = 20, sourceId = undefined, so the URL is still ?limit=20 — the test passes correctly as-is. No update to api.test.ts is needed.

Test Coverage

The one genuine gap is there's no test in api.test.ts verifying that a non-null sourceId appends &sourceId=... to the request URL. This is a minor omission — the behavior is covered at the component level — but an API-service-level test for it would be a small improvement for future maintainability.

Summary

The fix is correct and follows all established patterns. No blocking issues. The optional improvements are:

1. Add an api.test.ts test case for the sourceId URL parameter (minor)
2. Consider URLSearchParams for consistency with other methods in api.ts (cosmetic)