Connection Charts Page & Testing > 95% Frontend Coverage [GSOC 2025] #341
Conversation
WalkthroughAdds frontend testing (Vitest) and CI enforcement, implements ConnectionCharts and wiring into device page, rewrites TopologyChart, extends frontend types and backend GraphQL resolvers, introduces many component/page tests, and updates build/test/config files and docs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant DevicePage as Device Page
participant ConnectionCharts as ConnectionCharts
participant API as GraphQL API
User->>DevicePage: Open "Connection Charts" tab
DevicePage->>ConnectionCharts: Render with device prop
ConnectionCharts->>API: POST GraphQL QUERY(hostname, range)
API-->>ConnectionCharts: Metrics + interfaces
alt Valid data
ConnectionCharts->>ConnectionCharts: Aggregate, paginate, render charts
User->>ConnectionCharts: Switch tab / range / expand panels
ConnectionCharts->>ConnectionCharts: Recompute series / validate dates
User->>ConnectionCharts: Download CSV
ConnectionCharts->>User: Trigger file download
else Error
ConnectionCharts->>User: Show inline error alert
end
sequenceDiagram
autonumber
actor User
participant Home as Home Page
participant API as GraphQL API
User->>Home: Load with zoneId
alt zoneId == "all"
Home->>API: GetAllZonesDevices
else Specific zone
Home->>API: GetZoneDevices(zoneId)
end
API-->>Home: Devices (may include duplicates)
Home->>Home: Deduplicate by hostname
Home-->>User: Render Topology + DevicesOverview
alt Fetch/error
Home->>Home: Build error message
Home-->>User: Show error UI
end
sequenceDiagram
autonumber
actor Dev
participant CI as PR Workflow
participant Node as Node 22
participant Codecov as Codecov
Dev->>CI: Open/Update PR
CI->>Node: Setup + cache deps
CI->>Node: Run vitest with coverage
Node-->>CI: lcov.info
CI->>Codecov: Upload lcov.info
CI->>CI: Enforce >=80% frontend coverage
alt <80%
CI-->>Dev: Fail job
else >=80%
CI-->>Dev: Pass job
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Our Pull Request Approval ProcessWe have these basic policies to make the approval process smoother for our volunteer team. Testing Your CodePlease make sure your code passes all tests. Our test code coverage system will fail if either of these two conditions occur:
The process helps maintain the overall reliability of the code base and is a prerequisite for getting your PR approved. Assigned reviewers regularly review the PR queue and tend to focus on PRs that are passing. ReviewersWhen your PR has been assigned reviewers contact them to get your code reviewed and approved via:
Reviewing Your CodeYour reviewer(s) will have the following roles:
🎯 Contacting the person who assigned the mentors is not advised unless they make a request. Do not @ the person who did the assignment otherwise. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (65)
docs/docs/frontend/utils/time.md (2)
15-15: Avoid line‑anchored links; they drift on edits.Linking to
#L2is brittle. Prefer a permalink (commit SHA) or drop the line anchor to the file path to prevent doc rot.-Defined in: [utils/time.ts:2](https://github.com/PalisadoesFoundation/switchmap-ng/blob/develop/frontend/src/app/utils/time.ts#L2) +Defined in: [utils/time.ts](https://github.com/PalisadoesFoundation/switchmap-ng/blob/develop/frontend/src/app/utils/time.ts)
11-26: Add a one‑line purpose, units, and behavior note.A brief description improves self‑containment: state input units (hundredths of a second) and flooring behavior. Consider an example.
### formatUptime() > **formatUptime**(`hundredths`): `string` +Converts device uptime from hundredths of a second to a human‑readable "Xd Xh Xm Xs" string. Values are floored to whole seconds. + +Example: `formatUptime(9006100) -> "1d 1h 1m 1s"` + #### Parameters @@ `string`frontend/src/app/utils/time.spec.tsx (2)
21-24: Add edge/boundary tests to lock flooring semantics and minute/hour rollovers.These help prevent regressions if rounding logic changes later.
it("formats days correctly", () => { expect(formatUptime(9006100)).toBe("1d 1h 1m 1s"); // 9006100 hundredths = 1d 1h 1m 1s }); + + it("floors fractional seconds (hundredths) as implemented", () => { + expect(formatUptime(150)).toBe("0d 0h 0m 1s"); // 1.5s -> floors to 1s + expect(formatUptime(199)).toBe("0d 0h 0m 1s"); // 1.99s -> floors to 1s + }); + + it("handles 59s -> 1m transition", () => { + expect(formatUptime(5900)).toBe("0d 0h 0m 59s"); + expect(formatUptime(6000)).toBe("0d 0h 1m 0s"); + }); + + it("handles 3599s -> 1h transition", () => { + expect(formatUptime(359900)).toBe("0d 0h 59m 59s"); + expect(formatUptime(360000)).toBe("0d 1h 0m 0s"); + }); });
1-1: Use .spec.ts for non‑JSX tests (consistency).This file doesn’t use JSX; renaming to
.spec.tsaligns with common conventions and can speed up TSX transforms slightly.You can rename
frontend/src/app/utils/time.spec.tsx→frontend/src/app/utils/time.spec.ts.frontend/.gitignore (8)
7-11: Add Yarn Berry SDKs exception.Include .yarn/sdks to avoid ignoring editor SDKs when using Yarn PnP.
.yarn/* !.yarn/patches !.yarn/plugins !.yarn/releases !.yarn/versions +!.yarn/sdks
16-22: Remove duplicated Next.js ignores (consolidate later block).This block duplicates the more complete Next.js section below. Keep one canonical block to reduce noise.
-# next.js -/.next/ -/out/ -/.next/ -/out/ -.next/cache
37-40: Avoid blanket.env*ignore; keep example files tracked.
.env*also ignores .env.example; prefer explicit patterns to keep example files versioned.-# env files (can opt-in for committing if needed) -.env* -.env.*.local +# env files (explicit; keep .env.example tracked) +.env +.env.local +.env.development.local +.env.test.local +.env.production.local
79-81: Remove Svelte-specific ignores if not applicable.If the frontend is Next.js-only, drop .svelte-kit/ to avoid confusion.
-# Svelte -.svelte-kit/
95-104: Deduplicate IDE entries..vscode/ and .idea/ already appear above. Keep one occurrence.
-# JetBrains IDEs -*.iml -*.ipr -*.iws -.idea/ - -# VS Code settings -.vscode/ -.history/ +# JetBrains IDEs +*.iml +*.ipr +*.iws
105-134: Collapse redundant Next.js ignores.Since .next/ and out/ are ignored, the nested files/dirs under .next are redundant. Keep the minimal set.
-# Next.js build output -.next/ -out/ -.next/cache/ - -# Next.js static export -out/ - -# Next.js serverless functions output -.next/server/ - -# Next.js telemetry -.next/telemetry/ - -# Next.js build manifests -.next/static/ - -# Next.js cache -.next/cache/ - -# Next.js build traces -.next/trace - -# Next.js build logs -.next/build-manifest.json -.next/prerender-manifest.json -.next/routes-manifest.json -.next/export-marker.json -.next/BUILD_ID +# Next.js build output +.next/ +out/
138-143: Env files duplicated below or above.After adopting the explicit env list (see Lines 37-40), keep only one env section to avoid drift.
144-149: Deduplicate npm/yarn/pnpm log ignores.Already covered in the “# debug” section (Lines 32-35). Keep one copy.
-# npm logs -npm-debug.log* -yarn-debug.log* -yarn-error.log* -pnpm-debug.log*frontend/src/app/history/page.tsx (3)
199-201: Sort hostnames for stable, deterministic suggestions (and tests).Add a localeCompare sort to prevent flicker and make snapshots stable.
- setAllDeviceHostnames( - Array.from(new Set(devicesWithZones.map((d) => d.hostname))) - ); // all devices, unfiltered + setAllDeviceHostnames( + Array.from(new Set(devicesWithZones.map((d) => d.hostname))).sort( + (a, b) => a.localeCompare(b) + ) + ); // all devices, unfiltered & sorted
319-319: Avoid nested scrollbars (double overflow).Both the outer container (this line) and the inner content (Line 321) have overflow-y-auto, which can cause double scrollbars. Drop it on the outer container.
- <div className="flex h-screen overflow-y-auto"> + <div className="flex h-screen">
333-335: Userole="search"and add an accessible name for the landmark.Form already implies role=form; search is a better landmark.
- role="form" - data-testid="search-form" + role="search" + aria-label="Device search" + data-testid="search-form"docs/docs/frontend/utils/stringUtils.md (1)
17-38: Document defaults and clarify ellipsis behavior.Please note in the docs:
- Defaults: lines=2, maxLength=50
- If str is falsy => "N/A"
- If str.length <= maxLength => returns original string (no ellipsis)
- Ellipsis "..." is appended when truncated and is not counted toward maxLength
frontend/src/app/utils/stringUtils.spec.tsx (2)
29-34: Add edge-case tests for invalid options (lines<=0, maxLength<=0).Current implementation can produce odd output (e.g., empty lines then "...") when lines > maxLength or lines<=0. Recommend guarding in the util and adding tests like below.
Apply this diff to extend tests (insert before the closing brace):
@@ it("respects the maxLength option", () => { const str = "abcdefghijklmnopqrstuvwxyz"; const result = truncateLines(str, { maxLength: 12, lines: 2 }); expect(result).toBe("abcdef\nghijkl..."); }); + + it("handles lines=0 by falling back to simple truncation", () => { + const str = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ"; + const result = truncateLines(str, { lines: 0 }); + expect(result.endsWith("...")).toBe(true); + expect(result.includes("\n")).toBe(false); + }); + + it("handles maxLength=0 by returning only an ellipsis", () => { + const result = truncateLines("abc", { maxLength: 0 }); + expect(result).toBe("..."); + });And update the util accordingly:
- const { lines = 2, maxLength = 50 } = options ?? {}; + const { lines = 2, maxLength = 50 } = options ?? {}; + const safeLines = Math.floor(lines); + const safeMaxLen = Math.floor(maxLength); if (!str) return "N/A"; - if (str.length <= maxLength) return str; + if (safeLines <= 0 || safeMaxLen <= 0) return str.slice(0, Math.max(0, safeMaxLen)) + "..."; + if (str.length <= safeMaxLen) return str; - const segmentLength = Math.floor(maxLength / lines); + const segmentLength = Math.floor(safeMaxLen / safeLines); + if (segmentLength <= 0) return str.slice(0, safeMaxLen) + "..."; let result = ""; - for (let i = 0; i < lines; i++) { - const start = i * segmentLength; + for (let i = 0; i < safeLines; i++) { + const start = i * segmentLength; const end = start + segmentLength; result += str.slice(start, end); - if (i < lines - 1) result += "\n"; + if (i < safeLines - 1) result += "\n"; }
1-1: Consider .spec.ts instead of .spec.tsx.The file doesn’t use JSX; renaming to .spec.ts can speed up tooling marginally and align with conventions.
frontend/src/app/components/DevicesOverview.tsx (3)
63-70: Guard against undefined edges to avoid runtime crashesIf
l1interfacesoredgesis absent, this will throw. Add nullish guards with a safe fallback.- const interfaces = device.l1interfaces.edges.map( - (e: InterfaceEdge) => e.node - ); + const interfaces = + device.l1interfaces?.edges?.map((e: InterfaceEdge) => e.node) ?? [];
42-48: Align row type with data shape and use stable row IDsYou return
idin data but it’s not declared onDeviceRow. Declare it and wiregetRowIdfor stable row keys.interface DeviceRow { + id: string; name: string; hostname: string; ports: string; uptime: string; link: string; }const table = useReactTable({ data, columns, state: { sorting, globalFilter, }, onSortingChange: setSorting, onGlobalFilterChange: setGlobalFilter, getCoreRowModel: getCoreRowModel(), getSortedRowModel: getSortedRowModel(), getFilteredRowModel: getFilteredRowModel(), + getRowId: (row) => row.id, });Also applies to: 110-123
134-139: Add accessible name to the search inputProvide an aria-label so screen readers can identify the control.
<input value={globalFilter} onChange={(e) => setGlobalFilter(e.target.value)} placeholder="Search..." + aria-label="Search devices" className="mb-4 p-2 border rounded w-full max-w-sm" />frontend/vitest.setup.ts (1)
6-26: Harden ResizeObserver polyfill typings and assignmentMatch DOM signatures to reduce TS friction and assign via globalThis for broader compatibility.
-class ResizeObserver { - callback: ResizeObserverCallback; - constructor(callback: ResizeObserverCallback) { - this.callback = callback; - } - observe() { - // no-op - } - unobserve() { - // no-op - } - disconnect() { - // no-op - } -} - -(global as any).ResizeObserver = ResizeObserver; +class ResizeObserver { + private callback: ResizeObserverCallback; + constructor(callback: ResizeObserverCallback) { + this.callback = callback; + } + observe(_target: Element, _options?: ResizeObserverOptions) { + // no-op + } + unobserve(_target: Element) { + // no-op + } + disconnect() { + // no-op + } +} + +// Assign in a cross-environment safe way +// @ts-ignore +globalThis.ResizeObserver = ResizeObserver; +// @ts-ignore +if (typeof window !== 'undefined') window.ResizeObserver = ResizeObserver as any;frontend/tsconfig.json (1)
19-22: Consider isolating Vitest globals to a test tsconfigAdding
vitest/globalsto the main tsconfig leaks test globals into app code. Prefer a separate tsconfig (e.g., tsconfig.vitest.json) that extends this file and adds the Vitest types, referenced by your Vitest config.Would you like a follow-up diff to introduce tsconfig.vitest.json and update vite/vitest config accordingly?
switchmap/server/db/schemas.py (1)
210-231: Consider addressing the unused kwargs parameter.The new
resolve_devicesresolver correctly implements hostname-based device filtering with proper encoding. The implementation follows GraphQL best practices and matches the existingresolve_deviceMetricspattern.However, the static analysis tool identified that the
kwargsparameter is unused. While this may be intentional for future extensibility, consider one of these approaches:Apply this fix to address the unused parameter warning:
- def resolve_devices(root, info, hostname=None, **kwargs): + def resolve_devices(root, info, hostname=None, **_kwargs):Alternatively, if the parameter serves no purpose, it could be removed entirely:
- def resolve_devices(root, info, hostname=None, **kwargs): + def resolve_devices(root, info, hostname=None):frontend/src/app/components/ConnectionCharts.tsx (1)
41-62: Consider parameterizing the GraphQL query and endpoint.The hardcoded GraphQL query and endpoint configuration could be improved for maintainability and testability:
- Extract the GraphQL query: Consider moving the query to a separate file or constant:
// queries/deviceQueries.ts export const GET_DEVICE_INTERFACES_QUERY = ` { devices(hostname: $hostname) { // ... query fields } } `;
Consider using a GraphQL client: For better error handling and caching, consider using a proper GraphQL client like Apollo Client or urql instead of raw fetch calls.
Extract endpoint configuration: The endpoint fallback could be centralized:
const GRAPHQL_ENDPOINT = process.env.NEXT_PUBLIC_GRAPHQL_ENDPOINT || "http://localhost:7000/switchmap/api/graphql";frontend/package.json (1)
11-13: Align coverage script with CI needs (emit lcov).Your CI expects lcov.info. Ensure Vitest outputs lcov via script to avoid Codecov failures.
Apply:
- "test": "vitest run", + "test": "vitest run", "test:watch": "vitest --ui", - "test:coverage": "vitest run --coverage" + "test:coverage": "vitest run --coverage --coverage.reporter=lcov --coverage.reportsDirectory=coverage".github/workflows/pull-request.yml (4)
262-269: Use the dedicated coverage script to guarantee lcov output.CI currently runs npm run test -- --coverage, but later steps assume lcov at frontend/coverage/lcov.info. Call the script that enforces lcov.
- - name: Run frontend tests with coverage - working-directory: frontend - run: npm run test -- --coverage + - name: Run frontend tests with coverage (lcov) + working-directory: frontend + run: npm run test:coverageIf you prefer flags over scripts, configure Vitest coverage reporter to include "lcov" in vitest.config.
272-278: Codecov token handling for forked PRs.Tokens aren’t available from forks; this may fail PRs. Prefer omitting token on public repos or conditionally gating the step.
Option A (remove token for public repo):
- token: ${{ secrets.CODECOV_TOKEN }} files: frontend/coverage/lcov.info fail_ci_if_error: true verbose: trueOption B (skip on forks):
- - name: Upload coverage to Codecov + - name: Upload coverage to Codecov + if: ${{ github.event.pull_request.head.repo.fork == false }} uses: codecov/codecov-action@v5
263-265: Prefer reproducible installs.Use npm ci for deterministic, faster CI installs.
- run: npm install + run: npm ci
317-323: Target branch gate will fail this PR.This job hard-fails unless base is develop. Current PR targets develop-abhi. Adjust list or add a bypass label.
Suggestion:
- Allowlist both branches:
- if: ${{ github.event.pull_request.base.ref != 'develop' }} + if: ${{ github.event.pull_request.base.ref != 'develop' && github.event.pull_request.base.ref != 'develop-abhi' }}Or gate behind a label (similar to your sensitive-files bypass).
frontend/src/app/history/page.spec.tsx (4)
143-164: Duplicate test for 180‑day custom range.Both cases assert the same rule; keep one to reduce noise.
Remove one of the two “custom range cannot exceed 180 days” tests or consolidate into a table-driven case.
Also applies to: 275-293
7-41: Tighten fetch mocking and teardown to avoid cross‑test leaks.Resetting mocks doesn’t restore globals. Capture and restore original fetch.
+const originalFetch = global.fetch; beforeEach(() => { global.fetch = vi.fn().mockResolvedValue({ @@ }); afterEach(() => { vi.resetAllMocks(); + global.fetch = originalFetch as any; });
236-240: Restore window.location after stubbing reload.Define/restore to prevent bleed into other tests.
+const originalLocation = window.location; @@ Object.defineProperty(window, "location", { configurable: true, value: { ...window.location, reload: reloadMock }, }); @@ expect(reloadMock).toHaveBeenCalled(); + Object.defineProperty(window, "location", { + configurable: true, + value: originalLocation, + });Alternatively, use vi.spyOn(window.location, "reload") if your test environment supports it.
Also applies to: 249-250
18-22: Consider awaiting UI that appears after effects.Theme/components that render after useEffect can be flaky with getBy*. Prefer findBy* in those cases.
No action required if stable locally/CI; otherwise switch to findBy* where appropriate.
Also applies to: 24-29
frontend/src/app/components/ZoneDropdown.tsx (2)
83-89: Type safety: avoid relying on GetZoneDevices types for a different query shape.events → zones edges may not align with GetZoneDevices.ZoneEdge. Define a local type for this query.
- const rawZones = - json?.data?.events?.edges?.[0]?.node?.zones?.edges?.map( - (edge: ZoneEdge) => edge.node - ) ?? []; + type ZonesEdge = { node: { id: string; idxZone: string; name: string; tsCreated: string } }; + const rawZones = + (json?.data?.events?.edges?.[0]?.node?.zones?.edges as ZonesEdge[] | undefined)?.map( + (edge) => edge.node + ) ?? [];
121-124: Minor UX: format tsCreated for display.Raw timestamps can be unreadable. Format or hide when empty.
- {selected?.tsCreated || ""} + {selected?.tsCreated ? new Date(selected.tsCreated).toLocaleString() : ""}Also applies to: 167-169
frontend/src/app/theme-toggle.spec.tsx (1)
18-22: Stabilize tests by waiting for post‑effect render.ThemeToggle renders after useEffect. Use findByRole and clear mocks between tests.
- const button = screen.getByRole("button", { name: /toggle theme/i }); + const button = await screen.findByRole("button", { name: /toggle theme/i }); @@ - const button = screen.getByRole("button", { name: /toggle theme/i }); + const button = await screen.findByRole("button", { name: /toggle theme/i });Also:
+import { afterEach } from "vitest"; +afterEach(() => setThemeMock.mockClear());Also applies to: 24-29
frontend/src/app/components/DeviceOverview.spec.tsx (2)
47-59: Strengthen sorting tests with an actual order assertion.Right now we only fire keys and assert the header exists. Add a second device and assert row order toggles on Enter/Space to actually verify the sorting handler.
Apply this diff to introduce a second device and assert order:
it("calls sorting handler on Enter and Space key press", () => { - render( - <DevicesOverview devices={[mockDevice]} loading={false} error={null} /> - ); + const another = { + ...mockDevice, + id: "2", + idxDevice: 2, + sysName: "A-Device", + hostname: "host2", + }; + render( + <DevicesOverview devices={[mockDevice, another]} loading={false} error={null} /> + ); const headerCell = screen.getByRole("button", { name: /sort by device name/i, }); fireEvent.keyDown(headerCell, { key: "Enter" }); fireEvent.keyDown(headerCell, { key: " " }); - expect(headerCell).toBeInTheDocument(); + const rows = screen.getAllByRole("row").slice(1); // skip header + expect(rows[0]).toHaveTextContent("A-Device"); // asc then desc + fireEvent.keyDown(headerCell, { key: "Enter" }); + const rowsDesc = screen.getAllByRole("row").slice(1); + expect(rowsDesc[0]).toHaveTextContent("Device 1"); });
61-72: Also verify click sorting changes order.Same as keyboard: verify the table order changes after click.
it("calls sorting handler on mouse click", () => { - render( - <DevicesOverview devices={[mockDevice]} loading={false} error={null} /> - ); + const another = { ...mockDevice, id: "2", idxDevice: 2, sysName: "A-Device", hostname: "host2" }; + render( + <DevicesOverview devices={[mockDevice, another]} loading={false} error={null} /> + ); const headerCell = screen.getByRole("button", { name: /sort by device name/i, }); fireEvent.click(headerCell); - expect(headerCell).toBeInTheDocument(); + const rows = screen.getAllByRole("row").slice(1); + expect(rows[0]).toHaveTextContent("A-Device"); });frontend/src/app/components/Sidebar.tsx (2)
108-114: Add ARIA state to the hamburger for accessibility.Expose expanded state and control relationship.
<button className="p-3 text-2xl lg:hidden fixed top-4 left-4 z-50 bg-bg border border-border rounded" onClick={() => setOpen(true)} - aria-label="Open sidebar" + aria-label="Open sidebar" + aria-expanded={open} + aria-controls="slide-in-sidebar" >
126-129: Make slide‑in focusable for better keyboard UX.Auto-focus the panel when opened; enables ESC/Tab handling later if desired.
- <aside - data-testid="slide-in-sidebar" - ref={sidebarRef} + <aside + id="slide-in-sidebar" + data-testid="slide-in-sidebar" + ref={sidebarRef} + tabIndex={-1} className="fixed top-0 left-0 w-60 h-full bg-bg border-r border-border z-50 p-4 shadow-md transition-transform transform lg:hidden" >Optionally focus on open:
useEffect(() => { const handleClickOutside = (e: MouseEvent): void => { @@ if (open) { document.addEventListener("mousedown", handleClickOutside); + // focus slide-in when opened + setTimeout(() => sidebarRef.current?.focus(), 0); }frontend/src/app/components/HistoricalChart.spec.tsx (1)
2-2: Remove unused import.fireEvent isn’t used in this file.
-import { fireEvent, render, screen } from "@testing-library/react"; +import { render, screen } from "@testing-library/react";frontend/src/app/components/TopologyChart.tsx (3)
340-345: Fix edge reset logic: updates target undefined ids.initialGraph edges don’t have ids; updates no‑op. Reset using DataSet’s actual edge ids.
- // Reset edges color - initialGraph.current.edges.forEach((originalEdge) => { - edgesData.current!.update({ - id: originalEdge.id, - color: originalEdge.color || (isDark ? "#444" : "#BBBBBB"), // fallback if color missing - }); - }); + // Reset edges color using current DataSet ids + edgesData.current.get().forEach((edge) => { + edgesData.current!.update({ + id: edge.id as string | number, + color: (edge as any).color || (isDark ? "#444" : "#BBBBBB"), + }); + }); @@ - initialGraph.current.edges.forEach((originalEdge) => { - edgesData.current!.update({ - id: originalEdge.id, - color: originalEdge.color || (isDark ? "#444" : "#BBBBBB"), // fallback if color missing - arrows: { to: false }, // reset arrow visibility - }); - }); + edgesData.current.get().forEach((edge) => { + edgesData.current!.update({ + id: edge.id as string | number, + color: (edge as any).color || (isDark ? "#444" : "#BBBBBB"), + arrows: { to: false }, + }); + });Also applies to: 382-389
77-115: Include clickToUse/zoomView in options dependencies.Props changes won’t reflect without updating the memo.
- const options: Options = useMemo( + const options: Options = useMemo( () => ({ clickToUse: clickToUse ?? true, @@ - }), - [isDark] + }), + [isDark, clickToUse, zoomView] );
229-230: Avoid 8‑digit hex; use RGB/RGBA for compatibility.vis-network may not parse #RRGGBBAA consistently.
- color: "#383e44ff", + color: "rgba(56, 62, 68, 1)",frontend/src/app/components/Sidebar.spec.tsx (3)
28-38: Scope assertions to the slide‑in sidebar to avoid false positivesgetAllByText may match the static (lg) sidebar; assert within data-testid="slide-in-sidebar" after opening.
fireEvent.click(button); - const networkLink = screen.getAllByText(/Network Topology/i)[0]; - const devicesLink = screen.getAllByText(/Devices Overview/i)[0]; + const slideIn = screen.getByTestId("slide-in-sidebar"); + const networkLink = within(slideIn).getByText(/Network Topology/i); + const devicesLink = within(slideIn).getByText(/Devices Overview/i); expect(networkLink).toBeInTheDocument(); expect(devicesLink).toBeInTheDocument();
40-48: Fix selector: the slide‑in has data‑test id, not an aria‑labelqueryByLabelText("slide-in sidebar") will always be null. Use queryByTestId to actually verify closure.
- expect(screen.queryByLabelText("slide-in sidebar")).toBeNull(); + expect(screen.queryByTestId("slide-in-sidebar")).toBeNull();
50-60: Deduplicate the “closes sidebar when clicking outside” testTwo tests assert the same behavior. Keep the version using queryByTestId and remove or rename the other to reduce noise.
docs/docs/frontend/devices/[id]/page.md (1)
13-20: Docs mismatch: default() can return null in codeDevicePage returns null when sidebarOpen === null. Update the return type to Element | null.
-> **default**(): `Element` +> **default**(): `Element` | `null` @@ -`Element` +`Element` | `null`frontend/src/app/components/TopologyChart.spec.tsx (2)
167-204: This test overrides Network.on and no longer exercises component logicBy replacing mockInstance.on with your own impl, you’re testing the test, not the component’s registered handler. Prefer invoking the actual callback captured from the component’s on calls.
Example refactor:
const selectNodeHandler = mockInstance.on.mock.calls.find(([e]: [string, Function]) => e === "selectNode")?.[1]; expect(selectNodeHandler).toBeDefined(); selectNodeHandler({ nodes: ["1"] }); expect(mockUpdate).toHaveBeenCalledWith(expect.objectContaining({ id: "1", opacity: 1 }));
24-57: Consider making DataSet.get(id) honor the id for closer parityYour DataSet mock’s get() ignores the id and returns the full array, which can mask issues in id-based lookups (e.g., doubleClick handler). Optional: make get(id) return the matching item.
docs/docs/frontend/components/ConnectionDetails.md (1)
13-28: Docs don’t match the component signatureDocs show ConnectionDetails({ deviceId?: string }) while code exports ConnectionDetails({ device }: { device: DeviceNode }). Update param docs to device: DeviceNode.
-**ConnectionDetails**(`__namedParameters`): `null` | `Element` +**ConnectionDetails**(`__namedParameters`): `null` | `Element` @@ -###### deviceId? - -`string` +###### device + +`DeviceNode`frontend/src/app/components/ZoneDropdown.spec.tsx (2)
15-23: Unnecessary vi.restoreAllMocks() hereYou’re manually assigning global.fetch and restoring it in afterEach. restoreAllMocks isn’t needed and can surprise by resetting spies elsewhere.
beforeEach(() => { originalFetch = global.fetch; vi.clearAllMocks(); - vi.restoreAllMocks(); });
79-94: Resolve the pending fetch using a promise-returning json for consistencyKeep json returning a Promise to mirror real fetch.json() and reduce timing flakiness.
-act(() => - resolveFetch({ ok: true, json: () => ({ data: { events: [] } }) }) -); +act(() => + resolveFetch({ + ok: true, + json: () => Promise.resolve({ data: { events: [] } }), + }) +);frontend/src/app/components/ConnectionDetails.spec.tsx (1)
84-102: Tighten CDP/LLDP “missing” test to actually clear both identifiersCurrently only lldpremsysname is cleared; CDP id may still render, and other “-” cells can cause false positives. Clear both IDs to validate the intended columns.
const deviceWithMissingCDP = { ...mockDevice, l1interfaces: { edges: [ { node: { ...mockDevice.l1interfaces.edges[0].node, - cdpcachedeviceport: "", - lldpremsysname: "", + cdpcachedeviceid: "", + cdpcachedeviceport: "", + lldpremsysname: "", + lldpremportdesc: "", }, }, ], }, }; render(<ConnectionDetails device={deviceWithMissingCDP} />); - expect(screen.getByText("-")).toBeInTheDocument(); + // Expect at least two “-” entries for CDP and LLDP columns + expect(screen.getAllByText("-", { exact: true }).length).toBeGreaterThanOrEqual(2);frontend/src/app/devices/[id]/page.spec.tsx (2)
88-100: Add an assertion for the “Connection Charts” tabSince the page now includes a Connection Charts tab, add a quick switch-and-render assertion to keep coverage aligned with the new feature.
Example:
fireEvent.click(screen.getByText("Connection Charts")); await waitFor(() => { // e.g., assert placeholder text rendered by ConnectionCharts mock or component expect(screen.getByText(/No device data\./i)).not.toBeInTheDocument(); });
12-16: Optionally assert router.replace on tab changehandleTabChange updates the URL’s tab param via router.replace. Spy and assert called to harden behavior.
frontend/src/app/components/DeviceDetails.tsx (2)
122-126: Potential uptime unit mismatch (×100)formatUptime expects SNMP hundredths. Backend stores sysUpTime in hundredths (see ingest/update/device.py). Multiplying deviceMetrics.uptime by 100 likely overstates uptime. Use the raw value.
- value={formatUptime( - device.sysUptime ?? (deviceMetrics?.uptime ?? 0) * 100 - )} + value={formatUptime(device.sysUptime ?? (deviceMetrics?.uptime ?? 0))}
201-233: Minor DRY: factor timestamp mapping into a helperlastPolled → ISO conversion is repeated thrice. Extract a small helper to reduce duplication. Optional.
frontend/src/app/components/ConnectionCharts.spec.tsx (2)
6-6: Restore stubbed globals after each test to prevent leakage.
vi.stubGlobal("fetch", ...)and theURLstub should be restored per test to avoid cross-test interference.Apply this diff:
-import { describe, it, expect, vi, beforeEach, Mock } from "vitest"; +import { describe, it, expect, vi, beforeEach, afterEach, Mock } from "vitest"; @@ vi.stubGlobal("fetch", vi.fn()); // ---------- Helpers ---------- @@ describe("ConnectionCharts", () => { beforeEach(() => { @@ }); + + afterEach(() => { + vi.restoreAllMocks(); + });
134-149: Make dropdown querying resilient to label changes.The test caches the dropdown button initially labeled “Past 1 day” and reuses the same node after label changes. Prefer re-querying by role each time to avoid brittle coupling to text content.
frontend/src/app/page.tsx (4)
98-98: Remove unused parameterretryCount.
retryCountis not used. Drop it to reduce noise.Apply this diff:
- const fetchDevices = async (retryCount = 0) => { + const fetchDevices = async () => {
146-154: Unify environment variable for GraphQL endpoint across the app.This file uses
NEXT_PUBLIC_API_URLwhile ConnectionCharts.tsx usesNEXT_PUBLIC_GRAPHQL_ENDPOINT. Divergent env names lead to misconfig in deployments.Apply this diff (or update the other component to match; pick one name project‑wide):
- const res = await fetch( - process.env.NEXT_PUBLIC_API_URL || + const res = await fetch( + process.env.NEXT_PUBLIC_GRAPHQL_ENDPOINT || "http://localhost:7000/switchmap/api/graphql",
104-143: Include LLDP fields in the “all zones” query for consistency.Zone-specific query requests
lldpremportdesc,lldpremsysname,lldpremsysdesc,lldpremsyscapenabled, but the “all” query omits them. TopologyChart and downstream features may rely on these fields.Apply this diff to the all‑zones selection set:
hostname l1interfaces { edges { node { ifoperstatus cdpcachedeviceid cdpcachedeviceport + lldpremportdesc + lldpremsysname + lldpremsysdesc + lldpremsyscapenabled } } }
170-186: Deduplication logic is sound; consider extracting to a helper.Same hostname-dedup appears in both branches. Extracting a small helper reduces duplication and risk of drift.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (52)
.github/workflows/pull-request.yml(1 hunks)docs/docs/backend/Server.md(1 hunks)docs/docs/frontend/components/ConnectionDetails.md(1 hunks)docs/docs/frontend/components/TopologyChart.md(1 hunks)docs/docs/frontend/components/ZoneDropdown.md(1 hunks)docs/docs/frontend/devices/[id]/page.md(1 hunks)docs/docs/frontend/modules.md(0 hunks)docs/docs/frontend/types/graphql/GetZoneDevices.md(5 hunks)docs/docs/frontend/utils/stringUtils.md(1 hunks)docs/docs/frontend/utils/time.md(1 hunks)frontend/.gitignore(1 hunks)frontend/README.md(1 hunks)frontend/package.json(2 hunks)frontend/postcss.config.mjs(1 hunks)frontend/src/app/components/ConnectionCharts.spec.tsx(1 hunks)frontend/src/app/components/ConnectionCharts.tsx(1 hunks)frontend/src/app/components/ConnectionDetails.spec.tsx(1 hunks)frontend/src/app/components/ConnectionDetails.tsx(1 hunks)frontend/src/app/components/DeviceDetails.module.css(1 hunks)frontend/src/app/components/DeviceDetails.spec.tsx(1 hunks)frontend/src/app/components/DeviceDetails.tsx(1 hunks)frontend/src/app/components/DeviceOverview.spec.tsx(1 hunks)frontend/src/app/components/DevicesOverview.tsx(1 hunks)frontend/src/app/components/HistoricalChart.spec.tsx(1 hunks)frontend/src/app/components/HistoricalChart.tsx(1 hunks)frontend/src/app/components/LineChartWrapper.spec.tsx(1 hunks)frontend/src/app/components/Sidebar.spec.tsx(1 hunks)frontend/src/app/components/Sidebar.tsx(1 hunks)frontend/src/app/components/TopologyChart.spec.tsx(1 hunks)frontend/src/app/components/TopologyChart.tsx(1 hunks)frontend/src/app/components/ZoneDropdown.spec.tsx(1 hunks)frontend/src/app/components/ZoneDropdown.tsx(1 hunks)frontend/src/app/components/__mocks__/chartMocks.ts(1 hunks)frontend/src/app/components/__mocks__/deviceMocks.ts(1 hunks)frontend/src/app/devices/[id]/page.spec.tsx(1 hunks)frontend/src/app/devices/[id]/page.tsx(2 hunks)frontend/src/app/globals.css(2 hunks)frontend/src/app/history/page.spec.tsx(1 hunks)frontend/src/app/history/page.tsx(8 hunks)frontend/src/app/layout.spec.tsx(1 hunks)frontend/src/app/page.spec.tsx(1 hunks)frontend/src/app/page.tsx(2 hunks)frontend/src/app/theme-toggle.spec.tsx(1 hunks)frontend/src/app/types/graphql/GetDeviceInterfaces.ts(1 hunks)frontend/src/app/utils/stringUtils.spec.tsx(1 hunks)frontend/src/app/utils/time.spec.tsx(1 hunks)frontend/src/app/utils/timeStamp.spec.tsx(1 hunks)frontend/tsconfig.json(2 hunks)frontend/vite.config.ts(1 hunks)frontend/vitest.setup.ts(1 hunks)switchmap/server/db/schemas.py(1 hunks)temp-frontend(1 hunks)
💤 Files with no reviewable changes (1)
- docs/docs/frontend/modules.md
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-09-06T17:08:44.440Z
Learnt from: Abhi-MS
PR: PalisadoesFoundation/switchmap-ng#337
File: frontend/src/app/components/HistoricalChart.tsx:62-67
Timestamp: 2025-09-06T17:08:44.440Z
Learning: In HistoricalChart.tsx, using XAxis with type="number" and custom tickFormatter can cause empty charts if the lastPolled data contains null values or mixed timestamp formats (seconds vs milliseconds). The default categorical behavior is more robust for handling inconsistent timestamp data.
Applied to files:
frontend/src/app/components/HistoricalChart.tsxfrontend/src/app/components/DeviceDetails.tsx
📚 Learning: 2025-09-04T20:04:37.212Z
Learnt from: Abhi-MS
PR: PalisadoesFoundation/switchmap-ng#337
File: docs/docs/frontend/types/graphql/GetZoneDevices.md:59-64
Timestamp: 2025-09-04T20:04:37.212Z
Learning: Documentation files in docs/docs/frontend/ are autogenerated by TypeDoc from TypeScript source code and should not be manually edited. Changes should be made to the source TypeScript files instead.
Applied to files:
docs/docs/frontend/components/ZoneDropdown.mddocs/docs/frontend/components/TopologyChart.mdfrontend/README.md
📚 Learning: 2025-07-30T15:34:06.789Z
Learnt from: Abhi-MS
PR: PalisadoesFoundation/switchmap-ng#333
File: frontend/src/app/components/TopologyChart.tsx:174-178
Timestamp: 2025-07-30T15:34:06.789Z
Learning: In the TopologyChart component, manual cleanup is implemented when devices change to address potential memory leaks from DOM element creation in the htmlTitle function.
Applied to files:
frontend/src/app/components/TopologyChart.tsxfrontend/src/app/components/TopologyChart.spec.tsxdocs/docs/frontend/components/TopologyChart.mdfrontend/src/app/components/DeviceDetails.module.css
📚 Learning: 2025-09-04T19:53:01.998Z
Learnt from: Abhi-MS
PR: PalisadoesFoundation/switchmap-ng#337
File: frontend/src/app/history/page.tsx:45-51
Timestamp: 2025-09-04T19:53:01.998Z
Learning: GraphQL lastPolled field returns an integer timestamp (seconds since epoch), not a string. The backend uses graphene.Int for this field, and frontend components like DeviceDetails multiply by 1000 to convert to milliseconds for Date constructor.
Applied to files:
docs/docs/frontend/types/graphql/GetZoneDevices.md
📚 Learning: 2025-09-05T23:17:28.772Z
Learnt from: Abhi-MS
PR: PalisadoesFoundation/switchmap-ng#337
File: frontend/src/app/history/page.tsx:20-41
Timestamp: 2025-09-05T23:17:28.772Z
Learning: In the switchmap-ng project, each poll creates a new device/event record, so the zones->devices GraphQL query effectively returns all historical points for devices, not just current snapshots. This enables time-series visualization without needing a separate historical data source.
Applied to files:
docs/docs/frontend/types/graphql/GetZoneDevices.mdfrontend/src/app/page.tsx
🧬 Code graph analysis (30)
switchmap/server/db/schemas.py (1)
switchmap/server/db/models.py (1)
Device(168-211)
frontend/src/app/components/ConnectionCharts.tsx (3)
frontend/src/app/types/graphql/GetZoneDevices.ts (1)
DeviceNode(38-48)frontend/src/app/types/graphql/GetDeviceInterfaces.ts (1)
InterfaceNode(44-73)frontend/src/app/components/HistoricalChart.tsx (1)
HistoricalChart(48-79)
frontend/src/app/components/__mocks__/deviceMocks.ts (2)
frontend/src/app/types/graphql/GetDeviceInterfaces.ts (1)
InterfaceNode(44-73)frontend/src/app/types/graphql/GetZoneDevices.ts (1)
DeviceNode(38-48)
frontend/src/app/components/ConnectionDetails.spec.tsx (2)
frontend/src/app/components/ConnectionDetails.tsx (1)
ConnectionDetails(29-162)frontend/src/app/components/__mocks__/deviceMocks.ts (1)
mockDevice(47-59)
frontend/src/app/page.spec.tsx (1)
frontend/src/app/page.tsx (1)
Home(34-232)
frontend/vite.config.ts (1)
frontend/eslint.config.mjs (1)
__dirname(6-6)
frontend/src/app/theme-toggle.spec.tsx (1)
frontend/src/app/theme-toggle.tsx (1)
ThemeToggle(23-43)
frontend/src/app/components/TopologyChart.tsx (2)
frontend/src/app/types/graphql/GetZoneDevices.ts (1)
DeviceNode(38-48)frontend/src/app/utils/time.ts (1)
formatUptime(6-13)
frontend/src/app/layout.spec.tsx (1)
frontend/src/app/layout.tsx (1)
RootLayout(43-57)
frontend/src/app/components/ZoneDropdown.spec.tsx (1)
frontend/src/app/components/ZoneDropdown.tsx (1)
ZoneDropdown(35-211)
frontend/src/app/components/TopologyChart.spec.tsx (2)
frontend/src/app/components/TopologyChart.tsx (1)
TopologyChart(34-579)frontend/src/app/components/__mocks__/deviceMocks.ts (2)
mockDeviceLLDP(98-103)mockDevice(47-59)
frontend/src/app/history/page.spec.tsx (1)
frontend/src/app/history/page.tsx (1)
DeviceHistoryChart(85-580)
frontend/src/app/components/DeviceOverview.spec.tsx (2)
frontend/src/app/components/DevicesOverview.tsx (1)
DevicesOverview(50-246)frontend/src/app/components/__mocks__/deviceMocks.ts (1)
mockDevice(47-59)
frontend/src/app/components/HistoricalChart.spec.tsx (2)
frontend/src/app/components/HistoricalChart.tsx (1)
HistoricalChart(48-79)frontend/src/app/components/__mocks__/chartMocks.ts (1)
mockData(1-4)
frontend/src/app/components/LineChartWrapper.spec.tsx (2)
frontend/src/app/components/LineChartWrapper.tsx (1)
LineChartWrapper(66-117)frontend/src/app/components/__mocks__/chartMocks.ts (1)
mockLineData(6-9)
frontend/src/app/components/DeviceDetails.spec.tsx (2)
frontend/src/app/components/__mocks__/deviceMocks.ts (2)
mockMetricsForHost(61-86)mockDevice(47-59)frontend/src/app/components/DeviceDetails.tsx (1)
DeviceDetails(65-444)
frontend/src/app/components/DevicesOverview.tsx (3)
frontend/src/app/types/graphql/GetZoneDevices.ts (1)
DeviceNode(38-48)frontend/src/app/types/graphql/GetDeviceInterfaces.ts (1)
InterfaceNode(44-73)frontend/src/app/utils/time.ts (1)
formatUptime(6-13)
frontend/src/app/devices/[id]/page.tsx (1)
frontend/src/app/components/ConnectionCharts.tsx (1)
ConnectionCharts(64-496)
frontend/src/app/utils/time.spec.tsx (1)
frontend/src/app/utils/time.ts (1)
formatUptime(6-13)
frontend/src/app/utils/stringUtils.spec.tsx (1)
frontend/src/app/utils/stringUtils.ts (1)
truncateLines(8-28)
frontend/src/app/components/ConnectionCharts.spec.tsx (2)
frontend/src/app/components/ConnectionCharts.tsx (1)
ConnectionCharts(64-496)frontend/src/app/components/__mocks__/deviceMocks.ts (1)
mockDevice(47-59)
frontend/src/app/components/ConnectionDetails.tsx (2)
frontend/src/app/types/graphql/GetZoneDevices.ts (2)
DeviceNode(38-48)InterfaceEdge(30-32)frontend/src/app/types/graphql/GetDeviceInterfaces.ts (3)
MacPort(40-42)InterfaceEdge(75-77)InterfaceNode(44-73)
frontend/src/app/devices/[id]/page.spec.tsx (1)
frontend/src/app/devices/[id]/page.tsx (1)
DevicePage(79-271)
frontend/src/app/components/Sidebar.tsx (1)
frontend/src/app/theme-toggle.tsx (1)
ThemeToggle(23-43)
frontend/src/app/page.tsx (1)
frontend/src/app/types/graphql/GetZoneDevices.ts (1)
DeviceNode(38-48)
frontend/src/app/components/DeviceDetails.tsx (7)
frontend/src/app/types/graphql/GetZoneDevices.ts (1)
DeviceNode(38-48)frontend/src/app/components/TopologyChart.tsx (1)
TopologyChart(34-579)switchmap/server/db/ingest/update/device.py (1)
device(53-140)frontend/src/app/utils/stringUtils.ts (1)
truncateLines(8-28)frontend/src/app/utils/time.ts (1)
formatUptime(6-13)frontend/src/app/utils/timeStamp.ts (1)
formatUnixTimestamp(7-16)frontend/src/app/components/HistoricalChart.tsx (1)
HistoricalChart(48-79)
frontend/src/app/utils/timeStamp.spec.tsx (1)
frontend/src/app/utils/timeStamp.ts (1)
formatUnixTimestamp(7-16)
frontend/src/app/components/Sidebar.spec.tsx (1)
frontend/src/app/components/Sidebar.tsx (1)
Sidebar(29-136)
frontend/src/app/components/ZoneDropdown.tsx (3)
switchmap/server/db/schemas.py (1)
Zone(143-150)frontend/src/app/types/graphql/GetZoneDevices.ts (2)
Zone(69-71)ZoneEdge(57-59)switchmap/server/db/models.py (1)
Zone(129-165)
frontend/src/app/history/page.tsx (1)
frontend/src/app/components/Sidebar.tsx (1)
Sidebar(29-136)
🪛 Ruff (0.13.1)
switchmap/server/db/schemas.py
214-214: Unused method argument: kwargs
(ARG002)
🪛 markdownlint-cli2 (0.18.1)
docs/docs/backend/Server.md
2035-2035: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test Application
There was a problem hiding this comment.
Actionable comments posted: 27
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
frontend/.gitignore (1)
1-149: Revert frontend/.gitignore — CI rejects modificationsPipeline error: "frontend/.gitignore is unauthorized to change/delete." Restore this file to the target branch state; use .git/info/exclude locally or move ignore rules to an approved path. Confirm repo policy on protected files.
docs/docs/frontend/devices/[id]/page.md (1)
13-20: Do not edit auto-generated docs directlyPer team guidance, files under docs/docs/frontend are generated by TypeDoc. Revert manual edits and update the corresponding TS source (frontend/src/app/devices/[id]/page.tsx), then regenerate docs.
.github/workflows/pull-request.yml (1)
117-124: Fix sensitive-files pattern matching (false positives on nested package.json)Current regex match flags frontend/package.json as unauthorized. Use glob matching so root-only names don’t match nested paths.
Apply:
- # Check if any file in the diff matches our sensitive patterns - for PATTERN in "${SENSITIVE_FILES[@]}"; do - for FILE in $CHANGED_FILES $DELETED_FILES; do - if [[ "$FILE" == "$PATTERN" || "$FILE" =~ $PATTERN ]]; then - ALL_CHANGED_FILES+="$FILE " - SENSITIVE_CHANGED=true - fi - done - done + # Check if any file in the diff matches our sensitive patterns + # Use shell globs so 'package.json' doesn't match 'frontend/package.json', + # and 'docs/sidebar*.js' works as intended. + for PATTERN in "${SENSITIVE_FILES[@]}"; do + for FILE in $CHANGED_FILES $DELETED_FILES; do + case "$FILE" in + $PATTERN) + ALL_CHANGED_FILES+="$FILE " + SENSITIVE_CHANGED=true + ;; + esac + done + donefrontend/package.json (2)
15-23: Restore 'recharts' to frontend/package.json or remove its importsfrontend/src/app/components/HistoricalChart.tsx:11 and frontend/src/app/components/LineChartWrapper.tsx:11 import "recharts" — either re-add the dependency or refactor those files to remove/replace recharts usage.
1-49: CI blocks nested package.json edits — workflow matcher is too broadThe Check-Sensitive-Files job in .github/workflows/pull-request.yml includes "package.json" in SENSITIVE_FILES and uses
if [[ "$FILE" == "$PATTERN" || "$FILE" =~ $PATTERN ]], so any path containing "package.json" (e.g., frontend/package.json) is flagged and the PR is blocked.
Fix options: anchor the pattern (e.g.^package\.json$) to match only the repo root, use explicit paths (e.g.docs/package.json), or change the matcher to compare basename; alternatively apply the "ignore-sensitive-files-pr" label to bypass.
Location: .github/workflows/pull-request.yml — SENSITIVE_FILES array + matcher loop (~lines 77–125).frontend/src/app/page.tsx (1)
63-87: Missing lastPolled in single-zone query; align shape with types and dependents.
DeviceNodeincludeslastPolled. Add it to the selection to prevent undefineds downstream.node { idxDevice sysObjectid sysUptime sysDescription sysName hostname + lastPolled l1interfaces {
🧹 Nitpick comments (47)
frontend/src/app/utils/time.spec.tsx (1)
4-24: Refactor to table-driven tests for brevity and scalability.Reduce repetition and make it easier to add new cases.
Apply this diff:
describe("formatUptime", () => { - it("formats zero hundredths correctly", () => { - expect(formatUptime(0)).toBe("0d 0h 0m 0s"); - }); - - it("formats seconds correctly", () => { - expect(formatUptime(100)).toBe("0d 0h 0m 1s"); // 100 hundredths = 1 second - }); - - it("formats minutes correctly", () => { - expect(formatUptime(6100)).toBe("0d 0h 1m 1s"); // 6100 hundredths = 61 seconds - }); - - it("formats hours correctly", () => { - expect(formatUptime(366000)).toBe("0d 1h 1m 0s"); // 366000 hundredths = 1h 1m - }); - - it("formats days correctly", () => { - expect(formatUptime(9006100)).toBe("1d 1h 1m 1s"); // 9006100 hundredths = 1d 1h 1m 1s - }); + it.each([ + [0, "0d 0h 0m 0s"], + [100, "0d 0h 0m 1s"], // 1s + [6100, "0d 0h 1m 1s"], // 61s + [366000, "0d 1h 1m 0s"], // 1h1m + [9006100, "1d 1h 1m 1s"], // 1d1h1m1s + ])("formats %d hundredths correctly", (input, expected) => { + expect(formatUptime(input)).toBe(expected); + }); });frontend/src/app/utils/stringUtils.spec.tsx (1)
28-33: Add a test for uneven maxLength/lines split.Validates floor-based segment sizing and truncation of leftovers.
Append this test:
it("respects the maxLength option", () => { const str = "abcdefghijklmnopqrstuvwxyz"; const result = truncateLines(str, { maxLength: 12, lines: 2 }); expect(result).toBe("abcdef\nghijkl..."); }); + + it("handles maxLength not divisible by lines (drops remainder)", () => { + const str = "abcdefghijklmnopqrstuvwxyz"; + const result = truncateLines(str, { maxLength: 10, lines: 3 }); + const lines = result.replace(/\.\.\.$/, "").split("\n"); + expect(lines).toHaveLength(3); + expect(lines[0]).toHaveLength(3); + expect(lines[1]).toHaveLength(3); + expect(lines[2]).toHaveLength(3); + expect(result.endsWith("...")).toBe(true); + });frontend/src/app/utils/timeStamp.spec.tsx (1)
17-27: Consider additional inputs and clarify formatting requirements.
- If a locale-agnostic format is desired in UX (e.g., ISO), reliance on toLocaleString may be insufficient; confirm product intent.
- Add a test for numeric strings with whitespace to mirror real-world inputs.
Proposed test addition:
it("formats valid string timestamps correctly", () => { const tsStr = "1693574400"; const formatted = formatUnixTimestamp(tsStr); expect(new Date(Number(tsStr) * 1000).toLocaleString()).toBe(formatted); }); + + it("trims and parses numeric string timestamps with spaces", () => { + const tsStr = " 1693574400 "; + const formatted = formatUnixTimestamp(tsStr); + expect(new Date(Number(tsStr) * 1000).toLocaleString()).toBe(formatted); + });If we need locale-agnostic formatting, I can provide a variant using date-fns/Intl.DateTimeFormat with explicit locale/timeZone.
docs/docs/frontend/utils/time.md (1)
15-15: Avoid hardcoded line anchors in source links.Line numbers drift; prefer linking to the file (or a permalinks/typedoc anchor) without #L2.
Apply this diff:
-Defined in: [utils/time.ts:2](https://github.com/PalisadoesFoundation/switchmap-ng/blob/develop/frontend/src/app/utils/time.ts#L2) +Defined in: [utils/time.ts](https://github.com/PalisadoesFoundation/switchmap-ng/blob/develop/frontend/src/app/utils/time.ts)docs/docs/frontend/utils/stringUtils.md (1)
15-15: Prefer stable source links over line-specific anchors.Prevents docs rot as code shifts.
Apply this diff:
-Defined in: [utils/stringUtils.ts:5](https://github.com/PalisadoesFoundation/switchmap-ng/blob/develop/frontend/src/app/utils/stringUtils.ts#L5) +Defined in: [utils/stringUtils.ts](https://github.com/PalisadoesFoundation/switchmap-ng/blob/develop/frontend/src/app/utils/stringUtils.ts)frontend/README.md (1)
32-36: “Deploy on Vercel” may be misleading for this project.If deployment is self-hosted/on-prem (Docker/K8s/Ansible), linking to Vercel could misdirect users. Align deployment docs with project’s supported ops paths, or gate this note behind a “for local previews only” qualifier.
Would you like a short “Deployment” section pointing to the project’s actual deployment methods and CI?
frontend/src/app/globals.css (1)
36-36: Consider theme-specific values for subtle borders.Currently light and dark both use
#343B4F, which may appear heavy on light backgrounds. Suggest a lighter alpha for light mode.Apply in light theme:
- --border-subtle: #343B4F; + --border-subtle: rgba(8, 16, 40, 0.10);And keep a stronger value in
.dark(already present) to ensure visibility on dark surfaces.frontend/.gitignore (6)
16-22: Deduplicate Next.js ignores; keep a single canonical block.You have repeated .next/, out/, .next/cache[/], manifests, telemetry, etc. Consolidate to one block and standardize trailing slashes.
If/when edits to this file are allowed, collapse duplicates to a single section to reduce maintenance risk.
Also applies to: 105-134, 107-112, 123-127
37-40: Unify env file patterns..env*, .env..local, and the later .env..local entries overlap. Keep a single, minimal set (e.g., .env*, .env.local, .env.development.local, .env.test.local, .env.production.local).
Also applies to: 138-143
29-29: Consolidate log patterns.With *.log already present, npm/yarn/pnpm debug log patterns are redundant. Prefer a single pattern like .log or keep specific debug logs and drop the wildcard — not both.
Also applies to: 144-149, 31-36
79-81: Remove framework-specific ignores not used (Svelte)..svelte-kit/ appears unrelated to this Next.js app. Drop it to reduce noise unless you actually use Svelte in this workspace.
Is Svelte used anywhere under frontend/? If not, we can remove this line.
76-78: Verify Storybook usage.If storybook-static/ isn’t produced in this project, remove the entry; otherwise keep it.
Do we have Storybook configured for frontend? If no, we can prune this.
48-53: Editor/IDE ignores duplicated..vscode/ and .idea/ are declared twice. Keep them once.
Also applies to: 95-104
docs/docs/frontend/types/graphql/GetZoneDevices.md (1)
172-179: Docs: add a note on zones→devices returning historical snapshots.Per project behavior, each poll yields a new record; consumers may assume “current state.” Add a short remark to clarify this.
Apply:
@@ #### Remarks @@ It is designed to provide a type-safe interface for working with GraphQL data in TypeScript. The types defined here are used throughout the application to ensure that the data structures are consistent and correctly typed, reducing the risk of runtime errors and improving developer experience. + +Note: In Switchmap‑NG, each poll creates a new device/event record. As a result, the +zones → devices query may return multiple historical snapshots for a device, not only a +single “current” snapshot. Callers should account for time‑series semantics when aggregating +or rendering device data..github/workflows/pull-request.yml (3)
249-269: Use npm cache via setup-node + npm ci; call test:coverage scriptFaster, reproducible installs and simpler command.
Apply:
- name: Set up Node.js uses: actions/setup-node@v4 with: node-version: 22 + cache: npm + cache-dependency-path: frontend/package-lock.json - - name: Cache node modules - uses: actions/cache@v4 - with: - path: frontend/node_modules - key: ${{ runner.os }}-node-${{ hashFiles('frontend/package-lock.json') }} - restore-keys: | - ${{ runner.os }}-node- - - name: Install frontend dependencies working-directory: frontend - run: npm install + run: npm ci - name: Run frontend tests with coverage working-directory: frontend - run: npm run test -- --coverage + run: npm run test:coverage
316-323: Workflow will fail if PR target branch ≠ developThis PR targets develop-abhi; the job enforces base ref 'develop'. Retarget the PR or relax this check.
324-343: Gate CodeRabbit validation on frontend tests as wellOptional: add Test-Frontend to needs to avoid validating before FE tests finish.
Apply:
- needs: [Code-Quality-Checks, Test-Application] + needs: [Code-Quality-Checks, Test-Application, Test-Frontend]frontend/vitest.setup.ts (1)
24-26: Clear/reset mocks after each testPrevents cross-test leakage; cheap safety.
Apply:
-import { vi } from 'vitest'; +import { vi, afterEach } from 'vitest'; @@ (global as any).ResizeObserver = ResizeObserver; @@ vi.mock('next/navigation', () => ({ useRouter: () => ({ push: vi.fn() }) })); vi.mock('next-themes', () => ({ useTheme: () => ({ theme: 'light' }) })); + +afterEach(() => { + vi.clearAllMocks(); + vi.resetAllMocks(); +});frontend/package.json (1)
2-14: Optional: declare Node engineHelps CI/local consistency.
Apply:
{ "name": "frontend", "version": "0.1.0", "type": "module", "private": true, + "engines": { + "node": ">=20 <=22" + }, "scripts": {frontend/tsconfig.json (1)
19-23: Vitest globals in base tsconfigWorks, but consider isolating test-only types in a tsconfig.vitest.json to avoid bleeding into app type space.
frontend/src/app/components/__mocks__/chartMocks.ts (1)
1-9: Add explicit types (optional).Annotating the mock arrays prevents drift with component expectations.
Apply this diff:
-export const mockData = [ +type MockDailyPoint = { lastPolled: string; value: number }; +export const mockData: MockDailyPoint[] = [ { lastPolled: "2025-08-01", value: 1 }, { lastPolled: "2025-08-02", value: 0 }, ]; -export const mockLineData = [ +type MockLinePoint = { time: string; value: number }; +export const mockLineData: MockLinePoint[] = [ { time: "2025-08-01T00:00:00Z", value: 10 }, { time: "2025-08-02T00:00:00Z", value: 20 }, ];frontend/src/app/components/DevicesOverview.tsx (1)
152-165: Expose sorting state to assistive tech.Add aria-sort (and scope) to sortable headers for better a11y.
Apply this diff:
<th key={header.id} onClick={header.column.getToggleSortingHandler()} onKeyDown={(e) => { if (e.key === "Enter" || e.key === " ") { e.preventDefault(); header.column.getToggleSortingHandler()?.(e); } }} className="cursor-pointer px-4 py-2" tabIndex={0} role="button" aria-label={`Sort by ${header.column.columnDef.header}`} + aria-sort={ + header.column.getIsSorted() === "asc" + ? "ascending" + : header.column.getIsSorted() === "desc" + ? "descending" + : "none" + } + scope="col" >frontend/src/app/components/ConnectionDetails.tsx (2)
2-2: Remove unused useParams import/variable.They are not used; keep surface aligned with prop-driven component.
Apply this diff:
-import { useParams } from "next/navigation"; @@ - const params = useParams();Also applies to: 30-30
115-119: Use strict equality for status checks.Prevents accidental coercion.
Apply this diff:
- {iface.ifoperstatus == 1 + {iface.ifoperstatus === 1 ? "Active" - : iface.ifoperstatus == 2 + : iface.ifoperstatus === 2 ? "Disabled" : "N/A"}frontend/src/app/components/DeviceDetails.module.css (1)
25-28: Remove redundant border reset
border-width: 0is redundant whenborder: noneis already set..tableCustom *{ - border: none ; - border-width: 0 ; + border: none; }docs/docs/frontend/components/TopologyChart.md (1)
15-15: Avoid manual edits to autogenerated frontend docsPer prior learnings, docs under docs/docs/frontend are autogenerated by TypeDoc; update the TypeScript source and regenerate instead of editing this file directly.
frontend/src/app/components/Sidebar.tsx (1)
108-114: Improve a11y: reflect expanded state and associate controller/regionExpose open/closed state and control relationship; add an id on the slide‑in aside.
<button className="p-3 text-2xl lg:hidden fixed top-4 left-4 z-50 bg-bg border border-border rounded" onClick={() => setOpen(true)} aria-label="Open sidebar" + aria-expanded={open} + aria-controls="mobile-sidebar" >{open && ( <> <div className="fixed inset-0 bg-black/50 z-40 lg:hidden" /> <aside + id="mobile-sidebar" data-testid="slide-in-sidebar" ref={sidebarRef} className="fixed top-0 left-0 w-60 h-full bg-bg border-r border-border z-50 p-4 shadow-md transition-transform transform lg:hidden" >Optional: add Escape key handling to close the sidebar when open.
Also applies to: 125-131
docs/docs/frontend/components/ZoneDropdown.md (1)
15-15: Frontend docs appear autogenerated — prefer regeneratingZoneDropdown docs under docs/docs/frontend should be generated via TypeDoc; make changes in the TS source and regenerate.
frontend/src/app/devices/[id]/page.spec.tsx (1)
4-4: Restore globals after tests to prevent cross-test pollution
matchMediaandfetchare globally stubbed; addafterEachto restore/un-stub.-import { describe, it, expect, vi, beforeEach } from "vitest"; +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; @@ }); }); +afterEach(() => { + vi.restoreAllMocks(); + vi.unstubAllGlobals(); +});Also applies to: 71-71
frontend/src/app/components/TopologyChart.tsx (2)
77-115: Options should react to prop changes (clickToUse/zoomView)
useMemodepends only on theme;clickToUseandzoomViewchanges won’t update options.- const options: Options = useMemo( + const options: Options = useMemo( () => ({ @@ - }), - [isDark] + }), + [isDark, clickToUse, zoomView] );
503-522: Remove duplicate suggestion filtering; keep logic in one placeYou filter suggestions both in
onChangeand in theuseEffectforinputTerm. Keep only the effect.- onChange={(e) => { - const value = e.target.value; - setInputTerm(value); - if (value.trim() === "") { - setSuggestions([]); - return; - } - const filtered = allNodeLabels - .filter((label) => - label.toLowerCase().includes(value.toLowerCase()) - ) - .slice(0, 5); - setSuggestions(filtered); - }} + onChange={(e) => setInputTerm(e.target.value)} />Also applies to: 399-410
frontend/src/app/components/Sidebar.spec.tsx (1)
40-60: Consolidate duplicate outside-click test and assert open/close state via test idThe first outside-click test uses a non-existent aria-label and duplicates the second. Keep one and verify the element exists before/after.
- it("closes sidebar when clicking outside", () => { - render(<Sidebar />); - const button = screen.getByLabelText(/open sidebar/i); - fireEvent.click(button); - - fireEvent.mouseDown(document.body); - - expect(screen.queryByLabelText("slide-in sidebar")).toBeNull(); - }); - - it("closes sidebar when clicking outside", () => { + it("closes sidebar when clicking outside", () => { render(<Sidebar />); const button = screen.getByLabelText(/open sidebar/i); fireEvent.click(button); + // Sidebar is open + expect(screen.getByTestId("slide-in-sidebar")).toBeInTheDocument(); // Click outside fireEvent.mouseDown(document.body); // Slide-in sidebar should be removed expect(screen.queryByTestId("slide-in-sidebar")).toBeNull(); });frontend/src/app/history/page.tsx (1)
87-87: Suggestion: minor cleanups and a11y/test hooks look good
- New
allDeviceHostnames-based suggestions: good.- Consider removing now-unused
uniqueHostnamesmemo to reduce dead code.- Added role/testids/aria-labels improve testability and a11y. LGTM.
Also applies to: 226-230, 319-335
frontend/src/app/components/TopologyChart.spec.tsx (3)
25-57: Improve vis-network DataSet mock to mimic real API (fix id-based get).Your
DataSetMock.getalways returns the full data array. The component likely callsget(id)to retrieve a single node. Aligning the mock avoids brittle handler logic and future regressions.Apply:
vi.mock("vis-network/standalone/esm/vis-network", () => { - const updateMock = vi.fn(); + const updateMock = vi.fn(); const removeMock = vi.fn(); const focusMock = vi.fn(); const addMock = vi.fn(); const forEachMock = vi.fn((callback: any) => [{ id: "1" }, { id: "2" }].forEach(callback) ); - const DataSetMock = vi.fn((data) => ({ - get: vi.fn(() => data), + const DataSetMock = vi.fn((data) => ({ + get: vi.fn((id?: string) => + id ? (Array.isArray(data) ? data.find((n: any) => n?.id === id) : undefined) : data + ), add: vi.fn(), clear: vi.fn(), update: updateMock, current: data || [], - forEach: vi.fn(), + forEach: vi.fn((cb?: (v: any) => void) => { + if (!cb || !Array.isArray(data)) return; + data.forEach(cb); + }), })); const NetworkMock = vi.fn(() => ({ fit: vi.fn(), moveTo: vi.fn(), on: vi.fn(), focus: focusMock, unselectAll: vi.fn(), nodes: [], edges: [], nodesData: { add: addMock, update: updateMock, forEach: forEachMock }, edgesData: { update: updateMock, forEach: forEachMock }, getSelectedEdges: vi.fn(() => []), })); - return { Network: NetworkMock, DataSet: DataSetMock, removeMock }; + return { Network: NetworkMock, DataSet: DataSetMock, removeMock }; });
167-204: Don’t overrideon; use the component’s registered handler in tests.This test replaces
mockInstance.onand executes your own callback, so it no longer verifies the component’s realselectNodelogic.Refactor to invoke the handler the component registered:
- const callbacks: Record<string, (params: any) => void> = {}; - mockInstance.on = vi.fn((event, cb) => { - callbacks[event] = cb; - }); - - mockInstance.on("selectNode", ({ nodes }: { nodes: string[] }) => { - const selected = nodes[0]; - mockInstance.nodesData.current.forEach((node: any) => { - const isSelected = node.id === selected; - mockInstance.nodesData.update({ - id: node.id, - opacity: isSelected ? 1 : 0.6, - color: { border: "#555" }, - font: { color: isSelected ? "black" : "#A9A9A9" }, - }); - }); - }); - - callbacks["selectNode"]({ nodes: ["1"] }); - expect(mockUpdate).toHaveBeenCalledWith( + const selectNodeHandler = mockInstance.on.mock.calls.find( + ([e]: [string, Function]) => e === "selectNode" + )?.[1]; + // Ensure nodesData.get(id) returns a node; DataSetMock.get was fixed above. + selectNodeHandler?.({ nodes: ["1"] }); + // Assert that updates were issued (from the component’s handler) + expect(mockInstance.nodesData.update).toHaveBeenCalledWith( expect.objectContaining({ id: "1", opacity: 1 }) ); - expect(mockUpdate).toHaveBeenCalledWith( + expect(mockInstance.nodesData.update).toHaveBeenCalledWith( expect.objectContaining({ id: "2", opacity: 0.6 }) );
206-213: Assert updates via the component’s handler, not test-owned objects.Asserting
mockInstance.nodesData.update/edgesData.updateis okay only if the component writes there. Ensure this matches the handler’s behavior; otherwise, observe theDataSetMock.updatecalls instead.If the above assertion is flaky, prefer asserting call count on the
DataSetMock.updateyou defined for vis mocks.frontend/src/app/components/ConnectionCharts.spec.tsx (1)
91-104: Hostname is asserted correctly; consider tighter assertion on GraphQL body.You can parse the body to assert
queryandvariablesshape to catch regressions.Example:
const [, init] = (fetch as Mock).mock.calls[0]; const body = JSON.parse(init.body as string); expect(body.query).toMatch(/query/i); expect(body.query).toMatch(/devices|interfaces/i); expect(body).toEqual(expect.objectContaining({ query: expect.any(String) }));frontend/src/app/components/ZoneDropdown.spec.tsx (1)
79-94: Resolve pending promise with act; consider asserting end-of-loading UI.You already resolve the pending fetch; optionally assert loading indicator disappears to strengthen the test.
await waitFor(() => expect(screen.queryByText(/\(Loading...\)/)).not.toBeInTheDocument());frontend/src/app/page.tsx (3)
98-104: Remove dead parameter or implement retry/backoff.
retryCountis unused. Remove it or implement exponential backoff.- const fetchDevices = async (retryCount = 0) => { + const fetchDevices = async () => {
90-97: Avoid setting error when no zone is selected (noise).When no zone is selected, you set an error string that isn’t shown. Consider leaving error as null.
- setError("Waiting for zone selection to load devices."); + setError(null);
50-56: Derived state:zoneSelectedcan be computed.
zoneSelectedduplicates!!zoneId. Compute inline to reduce state.- const [zoneSelected, setZoneSelected] = useState<boolean>(false); + const zoneSelected = !!zoneId; ... - useEffect(() => { - if (zoneId) { - localStorage.setItem("zoneId", zoneId); - setZoneSelected(!!zoneId); - } - }, [zoneId]); + useEffect(() => { + if (zoneId) localStorage.setItem("zoneId", zoneId); + }, [zoneId]);frontend/src/app/components/ZoneDropdown.tsx (2)
42-76: Add AbortController for fetch cancellation on unmount.The active flag prevents setState after unmount but still consumes network. Abort to free resources.
useEffect(() => { - let active = true; + let active = true; + const ac = new AbortController(); const fetchZones = async () => { setLoading(true); setError(null); try { const res = await fetch( process.env.NEXT_PUBLIC_GRAPHQL_ENDPOINT || "http://localhost:7000/switchmap/api/graphql", { method: "POST", headers: { "Content-Type": "application/json" }, body: JSON.stringify({ query: `{ events(last: 1) { edges { node { zones { edges { node { tsCreated id idxZone name } } } } } } }`, }), + signal: ac.signal, } ); if (!res.ok) throw new Error(`Network error: ${res.status}`);- return () => { - active = false; - }; + return () => { + active = false; + ac.abort(); + };
141-169: Minor a11y: add ARIA attributes.Expose expanded state and menu semantics.
- <button + <button type="button" onClick={() => setOpen(!open)} className="inline-flex justify-between items-center bg-bg w-48 px-4 py-2 border border-gray-300 rounded-md shadow-sm outline" + aria-haspopup="menu" + aria-expanded={open} > ... - <p className="text-sm text-gray-700 pt-2 text-right"> + <p className="text-sm text-gray-700 pt-2 text-right" aria-live="polite"> {selected?.tsCreated || ""} </p>And for the menu:
- <div + <div data-testid="zone-dropdown-menu" - className="absolute bg-bg mt-1 w-48 rounded-md shadow-lg border border-color z-10 max-h-60 overflow-auto" + className="absolute bg-bg mt-1 w-48 rounded-md shadow-lg border border-color z-10 max-h-60 overflow-auto" + role="menu" > ... - <button + <button data-testid={`zone-button-${zone.id}`} key={zone.id} onClick={() => { onChange(zone.id); setOpen(false); }} - className="block w-full text-left px-4 py-2 hover:bg-hover-bg focus:outline-none" + className="block w-full text-left px-4 py-2 hover:bg-hover-bg focus:outline-none" + role="menuitem" >frontend/src/app/components/DeviceDetails.tsx (2)
269-274: Add accessible alert semantics to error bannerAdd role and aria-live so screen readers announce errors.
- <div className="fixed inset-0 flex mt-2 items-start justify-center z-50 pointer-events-none"> - <div className="bg-gray-300 text-gray-900 px-6 py-3 rounded shadow-lg animate-fade-in pointer-events-auto"> + <div className="fixed inset-0 flex mt-2 items-start justify-center z-50 pointer-events-none"> + <div + role="alert" + aria-live="assertive" + className="bg-gray-300 text-gray-900 px-6 py-3 rounded shadow-lg animate-fade-in pointer-events-auto" + > {errorMsg} </div> </div>
395-440: Avoid recomputing filters on every renderfilterByRange is called 3 times per render. Memoize filtered datasets to reduce work.
Example:
const filteredUptime = useMemo(() => filterByRange(uptimeData), [uptimeData, selectedRange, customRange]); const filteredCpu = useMemo(() => filterByRange(cpuUsageData), [cpuUsageData, selectedRange, customRange]); const filteredMem = useMemo(() => filterByRange(memoryUsageData), [memoryUsageData, selectedRange, customRange]);frontend/src/app/components/ConnectionCharts.tsx (2)
194-195: Sort series by timestamp before renderingGraphQL edge order isn’t guaranteed. Sort each per-interface series by lastPolled for stable charts.
- setData(newData); + Object.values(newData).forEach(seriesByTab => { + Object.values(seriesByTab).forEach(series => { + series.sort((a, b) => new Date(a.lastPolled).getTime() - new Date(b.lastPolled).getTime()); + }); + }); + setData(newData);
285-357: Add accessible alert semantics to error banner and inputsMirror DeviceDetails: role="alert" with aria-live; also add aria-invalid on date inputs when showing an error.
Example:
- {error && ( + {error && ( <div className="fixed inset-0 flex mt-2 items-start justify-center z-50 pointer-events-none"> - <div className="bg-gray-300 text-gray-900 px-6 py-3 rounded shadow-lg animate-fade-in pointer-events-auto"> + <div role="alert" aria-live="assertive" className="bg-gray-300 text-gray-900 px-6 py-3 rounded shadow-lg animate-fade-in pointer-events-auto"> {error} </div> </div> )}And when error is set, add aria-invalid on the offending input.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (52)
.github/workflows/pull-request.yml(1 hunks)docs/docs/backend/Server.md(1 hunks)docs/docs/frontend/components/ConnectionDetails.md(1 hunks)docs/docs/frontend/components/TopologyChart.md(1 hunks)docs/docs/frontend/components/ZoneDropdown.md(1 hunks)docs/docs/frontend/devices/[id]/page.md(1 hunks)docs/docs/frontend/modules.md(0 hunks)docs/docs/frontend/types/graphql/GetZoneDevices.md(5 hunks)docs/docs/frontend/utils/stringUtils.md(1 hunks)docs/docs/frontend/utils/time.md(1 hunks)frontend/.gitignore(1 hunks)frontend/README.md(1 hunks)frontend/package.json(2 hunks)frontend/postcss.config.mjs(1 hunks)frontend/src/app/components/ConnectionCharts.spec.tsx(1 hunks)frontend/src/app/components/ConnectionCharts.tsx(1 hunks)frontend/src/app/components/ConnectionDetails.spec.tsx(1 hunks)frontend/src/app/components/ConnectionDetails.tsx(1 hunks)frontend/src/app/components/DeviceDetails.module.css(1 hunks)frontend/src/app/components/DeviceDetails.spec.tsx(1 hunks)frontend/src/app/components/DeviceDetails.tsx(1 hunks)frontend/src/app/components/DeviceOverview.spec.tsx(1 hunks)frontend/src/app/components/DevicesOverview.tsx(1 hunks)frontend/src/app/components/HistoricalChart.spec.tsx(1 hunks)frontend/src/app/components/HistoricalChart.tsx(1 hunks)frontend/src/app/components/LineChartWrapper.spec.tsx(1 hunks)frontend/src/app/components/Sidebar.spec.tsx(1 hunks)frontend/src/app/components/Sidebar.tsx(1 hunks)frontend/src/app/components/TopologyChart.spec.tsx(1 hunks)frontend/src/app/components/TopologyChart.tsx(1 hunks)frontend/src/app/components/ZoneDropdown.spec.tsx(1 hunks)frontend/src/app/components/ZoneDropdown.tsx(1 hunks)frontend/src/app/components/__mocks__/chartMocks.ts(1 hunks)frontend/src/app/components/__mocks__/deviceMocks.ts(1 hunks)frontend/src/app/devices/[id]/page.spec.tsx(1 hunks)frontend/src/app/devices/[id]/page.tsx(2 hunks)frontend/src/app/globals.css(2 hunks)frontend/src/app/history/page.spec.tsx(1 hunks)frontend/src/app/history/page.tsx(8 hunks)frontend/src/app/layout.spec.tsx(1 hunks)frontend/src/app/page.spec.tsx(1 hunks)frontend/src/app/page.tsx(2 hunks)frontend/src/app/theme-toggle.spec.tsx(1 hunks)frontend/src/app/types/graphql/GetDeviceInterfaces.ts(1 hunks)frontend/src/app/utils/stringUtils.spec.tsx(1 hunks)frontend/src/app/utils/time.spec.tsx(1 hunks)frontend/src/app/utils/timeStamp.spec.tsx(1 hunks)frontend/tsconfig.json(2 hunks)frontend/vite.config.ts(1 hunks)frontend/vitest.setup.ts(1 hunks)switchmap/server/db/schemas.py(1 hunks)temp-frontend(1 hunks)
💤 Files with no reviewable changes (1)
- docs/docs/frontend/modules.md
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-07-30T15:34:06.789Z
Learnt from: Abhi-MS
PR: PalisadoesFoundation/switchmap-ng#333
File: frontend/src/app/components/TopologyChart.tsx:174-178
Timestamp: 2025-07-30T15:34:06.789Z
Learning: In the TopologyChart component, manual cleanup is implemented when devices change to address potential memory leaks from DOM element creation in the htmlTitle function.
Applied to files:
docs/docs/frontend/components/TopologyChart.mdfrontend/src/app/components/TopologyChart.tsxfrontend/src/app/components/DeviceDetails.module.css
📚 Learning: 2025-09-04T20:04:37.212Z
Learnt from: Abhi-MS
PR: PalisadoesFoundation/switchmap-ng#337
File: docs/docs/frontend/types/graphql/GetZoneDevices.md:59-64
Timestamp: 2025-09-04T20:04:37.212Z
Learning: Documentation files in docs/docs/frontend/ are autogenerated by TypeDoc from TypeScript source code and should not be manually edited. Changes should be made to the source TypeScript files instead.
Applied to files:
docs/docs/frontend/components/TopologyChart.mdfrontend/README.mddocs/docs/frontend/components/ZoneDropdown.mddocs/docs/frontend/devices/[id]/page.md
📚 Learning: 2025-09-04T19:53:01.998Z
Learnt from: Abhi-MS
PR: PalisadoesFoundation/switchmap-ng#337
File: frontend/src/app/history/page.tsx:45-51
Timestamp: 2025-09-04T19:53:01.998Z
Learning: GraphQL lastPolled field returns an integer timestamp (seconds since epoch), not a string. The backend uses graphene.Int for this field, and frontend components like DeviceDetails multiply by 1000 to convert to milliseconds for Date constructor.
Applied to files:
docs/docs/frontend/types/graphql/GetZoneDevices.md
📚 Learning: 2025-09-05T23:17:28.772Z
Learnt from: Abhi-MS
PR: PalisadoesFoundation/switchmap-ng#337
File: frontend/src/app/history/page.tsx:20-41
Timestamp: 2025-09-05T23:17:28.772Z
Learning: In the switchmap-ng project, each poll creates a new device/event record, so the zones->devices GraphQL query effectively returns all historical points for devices, not just current snapshots. This enables time-series visualization without needing a separate historical data source.
Applied to files:
docs/docs/frontend/types/graphql/GetZoneDevices.mdfrontend/src/app/page.tsx
📚 Learning: 2025-09-06T17:08:44.440Z
Learnt from: Abhi-MS
PR: PalisadoesFoundation/switchmap-ng#337
File: frontend/src/app/components/HistoricalChart.tsx:62-67
Timestamp: 2025-09-06T17:08:44.440Z
Learning: In HistoricalChart.tsx, using XAxis with type="number" and custom tickFormatter can cause empty charts if the lastPolled data contains null values or mixed timestamp formats (seconds vs milliseconds). The default categorical behavior is more robust for handling inconsistent timestamp data.
Applied to files:
frontend/src/app/components/HistoricalChart.tsxfrontend/src/app/components/HistoricalChart.spec.tsx
🧬 Code graph analysis (30)
frontend/src/app/components/LineChartWrapper.spec.tsx (2)
frontend/src/app/components/LineChartWrapper.tsx (1)
LineChartWrapper(66-117)frontend/src/app/components/__mocks__/chartMocks.ts (1)
mockLineData(6-9)
frontend/src/app/components/DeviceOverview.spec.tsx (2)
frontend/src/app/components/DevicesOverview.tsx (1)
DevicesOverview(50-246)frontend/src/app/components/__mocks__/deviceMocks.ts (1)
mockDevice(47-59)
frontend/vite.config.ts (1)
frontend/eslint.config.mjs (1)
__dirname(6-6)
frontend/src/app/components/__mocks__/deviceMocks.ts (2)
frontend/src/app/types/graphql/GetDeviceInterfaces.ts (1)
InterfaceNode(44-73)frontend/src/app/types/graphql/GetZoneDevices.ts (1)
DeviceNode(38-48)
frontend/src/app/devices/[id]/page.tsx (1)
frontend/src/app/components/ConnectionCharts.tsx (1)
ConnectionCharts(64-496)
frontend/src/app/components/DeviceDetails.spec.tsx (2)
frontend/src/app/components/__mocks__/deviceMocks.ts (2)
mockMetricsForHost(61-86)mockDevice(47-59)frontend/src/app/components/DeviceDetails.tsx (1)
DeviceDetails(65-444)
frontend/src/app/components/ConnectionCharts.spec.tsx (2)
frontend/src/app/components/ConnectionCharts.tsx (1)
ConnectionCharts(64-496)frontend/src/app/components/__mocks__/deviceMocks.ts (1)
mockDevice(47-59)
frontend/src/app/components/ZoneDropdown.tsx (2)
frontend/src/app/types/graphql/GetZoneDevices.ts (2)
Zone(69-71)ZoneEdge(57-59)switchmap/server/db/models.py (1)
Zone(129-165)
frontend/src/app/layout.spec.tsx (1)
frontend/src/app/layout.tsx (1)
RootLayout(43-57)
frontend/src/app/utils/timeStamp.spec.tsx (1)
frontend/src/app/utils/timeStamp.ts (1)
formatUnixTimestamp(7-16)
frontend/src/app/theme-toggle.spec.tsx (1)
frontend/src/app/theme-toggle.tsx (1)
ThemeToggle(23-43)
frontend/src/app/components/TopologyChart.tsx (2)
frontend/src/app/types/graphql/GetZoneDevices.ts (1)
DeviceNode(38-48)frontend/src/app/utils/time.ts (1)
formatUptime(6-13)
frontend/src/app/components/Sidebar.spec.tsx (1)
frontend/src/app/components/Sidebar.tsx (1)
Sidebar(29-136)
frontend/src/app/devices/[id]/page.spec.tsx (2)
frontend/src/app/components/__mocks__/deviceMocks.ts (1)
mockDevice(47-59)frontend/src/app/devices/[id]/page.tsx (1)
DevicePage(79-271)
frontend/src/app/utils/time.spec.tsx (1)
frontend/src/app/utils/time.ts (1)
formatUptime(6-13)
frontend/src/app/components/ConnectionCharts.tsx (3)
frontend/src/app/types/graphql/GetZoneDevices.ts (1)
DeviceNode(38-48)frontend/src/app/types/graphql/GetDeviceInterfaces.ts (1)
InterfaceNode(44-73)frontend/src/app/components/HistoricalChart.tsx (1)
HistoricalChart(48-79)
frontend/src/app/components/ConnectionDetails.spec.tsx (2)
frontend/src/app/components/ConnectionDetails.tsx (1)
ConnectionDetails(29-162)frontend/src/app/components/__mocks__/deviceMocks.ts (1)
mockDevice(47-59)
frontend/src/app/components/ZoneDropdown.spec.tsx (1)
frontend/src/app/components/ZoneDropdown.tsx (1)
ZoneDropdown(35-211)
switchmap/server/db/schemas.py (1)
switchmap/server/db/models.py (1)
Device(168-211)
frontend/src/app/components/Sidebar.tsx (1)
frontend/src/app/theme-toggle.tsx (1)
ThemeToggle(23-43)
frontend/src/app/components/HistoricalChart.spec.tsx (2)
frontend/src/app/components/HistoricalChart.tsx (1)
HistoricalChart(48-79)frontend/src/app/components/__mocks__/chartMocks.ts (1)
mockData(1-4)
frontend/src/app/components/DeviceDetails.tsx (6)
frontend/src/app/types/graphql/GetZoneDevices.ts (1)
DeviceNode(38-48)frontend/src/app/components/TopologyChart.tsx (1)
TopologyChart(34-579)frontend/src/app/utils/stringUtils.ts (1)
truncateLines(8-28)frontend/src/app/utils/time.ts (1)
formatUptime(6-13)frontend/src/app/utils/timeStamp.ts (1)
formatUnixTimestamp(7-16)frontend/src/app/components/HistoricalChart.tsx (1)
HistoricalChart(48-79)
frontend/src/app/components/ConnectionDetails.tsx (2)
frontend/src/app/types/graphql/GetZoneDevices.ts (2)
DeviceNode(38-48)InterfaceEdge(30-32)frontend/src/app/types/graphql/GetDeviceInterfaces.ts (3)
MacPort(40-42)InterfaceEdge(75-77)InterfaceNode(44-73)
frontend/src/app/page.spec.tsx (1)
frontend/src/app/page.tsx (1)
Home(34-232)
frontend/src/app/components/TopologyChart.spec.tsx (2)
frontend/src/app/components/TopologyChart.tsx (1)
TopologyChart(34-579)frontend/src/app/components/__mocks__/deviceMocks.ts (2)
mockDeviceLLDP(98-103)mockDevice(47-59)
frontend/src/app/history/page.tsx (1)
frontend/src/app/components/Sidebar.tsx (1)
Sidebar(29-136)
frontend/src/app/utils/stringUtils.spec.tsx (1)
frontend/src/app/utils/stringUtils.ts (1)
truncateLines(8-28)
frontend/src/app/history/page.spec.tsx (1)
frontend/src/app/history/page.tsx (1)
DeviceHistoryChart(85-580)
frontend/src/app/components/DevicesOverview.tsx (3)
frontend/src/app/types/graphql/GetZoneDevices.ts (2)
DeviceNode(38-48)InterfaceEdge(30-32)frontend/src/app/types/graphql/GetDeviceInterfaces.ts (2)
InterfaceEdge(75-77)InterfaceNode(44-73)frontend/src/app/utils/time.ts (1)
formatUptime(6-13)
frontend/src/app/page.tsx (1)
frontend/src/app/types/graphql/GetZoneDevices.ts (1)
DeviceNode(38-48)
🪛 GitHub Actions: PR Workflow
frontend/README.md
[error] 1-1: frontend/README.md is unauthorized to change/delete.
frontend/src/app/components/ConnectionCharts.spec.tsx
[error] 1-1: Test suite failed to run due to module resolution error for recharts in LineChartWrapper/HistoricalChart components.
frontend/src/app/devices/[id]/page.spec.tsx
[error] 11-11: Command: npm run test -- --coverage. Failed to resolve import "recharts" from "src/app/components/HistoricalChart.tsx". Does the file exist?
frontend/src/app/components/HistoricalChart.tsx
[error] 11-11: Command: npm run test -- --coverage. Failed to resolve import "recharts" from "src/app/components/HistoricalChart.tsx". Does the file exist?
frontend/package.json
[error] 1-1: frontend/package.json is unauthorized to change/delete.
frontend/src/app/history/page.spec.tsx
[error] 1-1: Test suite could not be executed due to previous import-resolution failures.
frontend/.gitignore
[error] 1-1: frontend/.gitignore is unauthorized to change/delete.
🪛 Ruff (0.13.1)
switchmap/server/db/schemas.py
214-214: Unused method argument: kwargs
(ARG002)
🔇 Additional comments (33)
frontend/src/app/utils/time.spec.tsx (1)
4-24: Solid coverage of key uptime cases.Covers boundaries from 0s through multi-day; assertions align with implementation.
frontend/src/app/utils/stringUtils.spec.tsx (1)
4-34: Good coverage across empty, passthrough, and truncation scenarios.Assertions match current truncateLines behavior.
frontend/src/app/utils/timeStamp.spec.tsx (1)
4-28: Robust and environment-stable assertions.Comparing to Date(...).toLocaleString() within the same runtime avoids locale flakiness.
frontend/src/app/globals.css (1)
16-16: Confirm --color-border-subtle is consumed (or remove it)Mapping --color-border-subtle → var(--border-subtle) matches the token pattern; automated sandbox search returned "No files were searched" so please verify that components/utilities or Tailwind config actually reference --color-border-subtle (frontend/src/app/globals.css line 16) — remove it if unused.
frontend/.gitignore (1)
63-67: Don't ignore lockfiles; commit the one your package manager uses.frontend/.gitignore (lines 63–67) currently ignores package-lock.json, yarn.lock, pnpm-lock.yaml. frontend/package.json has no "packageManager" field and there are no lockfiles in the repo — pick a package manager, remove its lockfile from .gitignore, and commit exactly that lockfile (pnpm-lock.yaml for pnpm).
docs/docs/frontend/types/graphql/GetZoneDevices.md (3)
182-190: Anchor verified — InterfaceNode existsdocs/docs/frontend/types/graphql/GetDeviceInterfaces.md contains "### InterfaceNode" at lines 76–78; cross-reference resolves.
27-33: Removal oflastPolledis incorrect — it’s still used in frontend and backend.
- Frontend (types/usage): frontend/src/app/types/graphql/GetZoneDevices.ts, frontend/src/app/history/page.tsx, frontend/src/app/devices/[id]/page.tsx, frontend/src/app/components/DeviceDetails.tsx, frontend/src/app/components/ConnectionCharts.tsx, frontend/src/app/components/HistoricalChart.tsx, mocks/specs under frontend/src/app/components/mocks and tests.
- Backend/tests: switchmap/dashboard/net/routes/api/api.py, switchmap/dashboard/data/system.py, tests/switchmap_/server/api/routes/test_graphql.py, and testdata JSON files (e.g., tests/testdata_/device-01.json, device-03.json).
Revert the docs change or update the backend schema and all frontend callers/mocks/tests together.
Likely an incorrect or invalid review comment.
15-15: Verify 'Defined in' anchors and lastPolled in GetZoneDevicesExports exist in frontend/src/app/types/graphql/GetZoneDevices.ts (DeviceNode starts at line 38, DeviceEdge at 50, GetZoneDevicesVars at 81) but several docs "Defined in" anchors point to other line numbers (e.g., docs/docs/frontend/types/graphql/GetZoneDevices.md:15 links to types/graphql/GetZoneDevices.ts:49) — update anchors to match the TS file. Also, lastPolled is present in the TS file; confirm whether it should be documented or removed.
frontend/src/app/layout.spec.tsx (2)
17-26: LGTM: layout renders children as expectedTest is minimal and correct.
6-15: Vitest globals enabled — no import requiredvite.config.ts sets test.globals: true, so vi is available globally and using vi.mock is fine.
frontend/postcss.config.mjs (1)
1-5: LGTM: Tailwind v4 plugin map formatConfig is valid for Tailwind/PostCSS v4.
frontend/src/app/components/DeviceOverview.spec.tsx (1)
10-73: LGTM: solid coverage of states and interactionsMocks and assertions align with component behavior.
frontend/src/app/components/__mocks__/chartMocks.ts (1)
1-9: LGTM.Mock shapes look fine for chart tests.
frontend/src/app/components/ConnectionDetails.tsx (1)
121-123: Confirm units for Days Inactive and Speed.tsIdle type changed to number; ensure values are in expected units for "Days Inactive" label, and consider formatting ifspeed for readability (e.g., Mbps/Gbps).
frontend/src/app/page.spec.tsx (4)
72-77: LGTM.Good smoke test for critical layout elements.
79-122: LGTM.Fetch mocking and dedup assertions look sound; localStorage persistence verified.
160-183: LGTM.Error handling paths are well-covered.
240-255: LGTM.Scroll behavior test is precise and cleans up state.
frontend/src/app/types/graphql/GetDeviceInterfaces.ts (1)
50-62: Type extensions look correct.New counters as number|null and tsIdle as number align with charting needs.
Confirm backend resolvers/queries populate these fields (or that UI paths guard nulls) to avoid unexpected undefined at runtime.
frontend/src/app/devices/[id]/page.tsx (1)
169-178: Connection Charts tab wiring LGTMLoading/error states and rendering ConnectionCharts are correctly integrated.
Confirm NEXT_PUBLIC_GRAPHQL_ENDPOINT is set in CI, or that fetch is mocked in page tests to avoid external calls.
docs/docs/backend/Server.md (1)
2033-2054: Docs for resolve_devices match backend changesNew resolver docs align with schema updates and are clear.
frontend/src/app/history/page.spec.tsx (1)
185-190: LGTM: error handling path is coveredGood coverage for fetch rejection and error UI.
frontend/src/app/components/ConnectionDetails.spec.tsx (1)
18-25: LGTM: table and headers assertions are solidValidates key columns and section header correctly.
frontend/src/app/components/HistoricalChart.spec.tsx (1)
10-39: Tests are fine; will pass once "recharts" is installed or mockedNothing to change in this file; dependency fix required upstream.
Run the verification script in the HistoricalChart.tsx comment to confirm
rechartspresence and Vitest jsdom config.frontend/src/app/components/TopologyChart.spec.tsx (1)
216-241: Edge hover/blur interaction tests look good.frontend/src/app/components/ZoneDropdown.spec.tsx (2)
68-77: Auto-select test is valid and helpful.
159-170: Good coverage for network error path (non-ok response).frontend/src/app/components/ZoneDropdown.tsx (1)
119-135: Selection sync looks correct.Auto-fallback to first zone when an invalid id is provided is solid.
If zones can be empty, ensure parent handles
onChangenot being called (current code is safe).frontend/src/app/components/DeviceDetails.spec.tsx (3)
38-51: Solid happy-path coverageGood isolation via TopologyChart mock and assertions for all three charts plus device status.
79-159: Custom range validations are thoroughEdge cases are well covered (duration cap, ordering, in-range filtering). Nice.
161-225: Robust error handling testsNetwork failure, HTTP error, GraphQL error, and malformed responses are all asserted. Good use of spies.
frontend/src/app/components/ConnectionCharts.tsx (2)
4-4: HistoricalChart exported both named and default — both import styles are validfrontend/src/app/components/HistoricalChart.tsx contains both
export function HistoricalChart(...)andexport default HistoricalChart;, so the default import in ConnectionCharts and the named import in DeviceDetails both work; no change required. Consider standardizing imports for consistency.Likely an incorrect or invalid review comment.
186-190: Confirm ifspeed units; convert to Mbps if backend provides bpsSNMP ifSpeed is usually in bits/sec — this component labels speed as Mbps but uses iface.ifspeed raw; mocks set ifspeed = 1000 (suggesting Mbps). Confirm whether the GraphQL API returns Mbps or bps. If it returns bps, convert before use (chart + details). Example change:
- value: iface.ifspeed, + value: Math.round((iface.ifspeed ?? 0) / 1_000_000),Files to check/update: frontend/src/app/components/ConnectionCharts.tsx:186-190 and frontend/src/app/components/ConnectionDetails.tsx:122.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
frontend/src/app/components/TopologyChart.tsx (4)
365-375: Use consistent arrows API shape to disable arrowsAlign with the enabled/disabled object form to avoid type mismatches.
Apply:
networkRef.current.on("blurEdge", (params) => { @@ - if (!selectedEdges.includes(params.edge)) { - edgesData.current.update({ - id: params.edge, - arrows: { to: false }, - }); - } + if (!selectedEdges.includes(params.edge)) { + edgesData.current.update({ + id: params.edge, + arrows: { to: { enabled: false } }, + }); + } }); @@ networkRef.current.on("deselectEdge", (params) => { @@ - initialGraph.current.edges.forEach((originalEdge) => { - edgesData.current!.update({ - id: originalEdge.id, - color: originalEdge.color || (isDark ? "#444" : "#BBBBBB"), - arrows: { to: false }, - }); - }); + initialGraph.current.edges.forEach((originalEdge) => { + edgesData.current!.update({ + id: originalEdge.id, + color: originalEdge.color || (isDark ? "#444" : "#BBBBBB"), + arrows: { to: { enabled: false } }, + }); + }); });Also applies to: 386-396
468-490: Simplify reset to avoid redundant dataset churn and race with re-init
setGraph(initialGraph.current)already triggers network re-creation; clearing/adding datasets immediately after can race with cleanup.Apply:
const handleReset = () => { setInputTerm(""); setSearchResult(""); setGraph(initialGraph.current); - - if (!networkRef.current || !nodesData.current || !edgesData.current) return; - - // Clear selection - networkRef.current.unselectAll(); - - // Instead of clear + add, do update - const originalNodes = initialGraph.current.nodes; - const originalEdges = initialGraph.current.edges; - - nodesData.current.clear(); - edgesData.current.clear(); - - nodesData.current.add(originalNodes); - edgesData.current.add(originalEdges); - - // Reset view - networkRef.current.fit(); };
433-466: Update search highlight on theme changeTheme colors in this effect won’t update until searchTerm changes. Add
isDark.Apply:
- }, [searchTerm]); + }, [searchTerm, isDark]);
155-161: Type the interface node instead ofanyUse a typed shape for
ifaceto reduce runtime surprises and improve DX.For example:
type IfaceNode = { cdpcachedeviceid?: string | null; cdpcachedeviceport?: string | null; lldpremsysname?: string | null; lldpremportdesc?: string | null; }; (device.l1interfaces?.edges ?? []).forEach(({ node: iface }: { node: IfaceNode }) => { ... });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docs/docs/frontend/components/ConnectionDetails.md(1 hunks)frontend/src/app/components/DeviceDetails.tsx(1 hunks)frontend/src/app/components/LineChartWrapper.spec.tsx(1 hunks)frontend/src/app/components/TopologyChart.spec.tsx(1 hunks)frontend/src/app/components/TopologyChart.tsx(1 hunks)frontend/src/app/components/__mocks__/deviceMocks.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- frontend/src/app/components/TopologyChart.spec.tsx
- frontend/src/app/components/LineChartWrapper.spec.tsx
- frontend/src/app/components/DeviceDetails.tsx
- docs/docs/frontend/components/ConnectionDetails.md
- frontend/src/app/components/mocks/deviceMocks.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-30T15:34:06.789Z
Learnt from: Abhi-MS
PR: PalisadoesFoundation/switchmap-ng#333
File: frontend/src/app/components/TopologyChart.tsx:174-178
Timestamp: 2025-07-30T15:34:06.789Z
Learning: In the TopologyChart component, manual cleanup is implemented when devices change to address potential memory leaks from DOM element creation in the htmlTitle function.
Applied to files:
frontend/src/app/components/TopologyChart.tsx
🧬 Code graph analysis (1)
frontend/src/app/components/TopologyChart.tsx (2)
frontend/src/app/types/graphql/GetZoneDevices.ts (1)
DeviceNode(38-48)frontend/src/app/utils/time.ts (1)
formatUptime(6-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Check Docstring Compliance
- GitHub Check: Test Frontend and Coverage
- GitHub Check: Checks if number of files changed is acceptable
🔇 Additional comments (2)
frontend/src/app/components/TopologyChart.tsx (2)
162-191: Good fix: stable edge IDs addedEdge IDs are now assigned; reset/update handlers can safely reference them.
405-414: Good: Network cleanup prevents leaks/dupe handlersDestroying the instance and nulling refs on cleanup is correct.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
frontend/src/app/components/TopologyChart.tsx (5)
277-289: Handle empty graph: destroy instance and clear suggestions.Prevents stale canvas/listeners and leftover search suggestions.
Apply this diff:
- // If no graph data is available, do not render the network - if (!containerRef.current || graph.nodes.length === 0) return; + // If no graph data is available, do not render the network + if (!containerRef.current) return; + if (graph.nodes.length === 0) { + networkRef.current?.destroy(); + networkRef.current = null; + nodesData.current = null; + edgesData.current = null; + setAllNodeLabels([]); + return; + }
243-249: Let theme control extra-node color.Avoid hard-coded color so theme changes apply consistently.
Apply this diff:
nodesArray.push({ id: sysName, label: sysName, - color: "#383e44", });
450-454: Use theme-aware colors for search highlight.Improves contrast in dark mode.
Apply this diff:
- color: { - background: n.id === searchTerm ? "#FF6347" : "#D3D3D3", - border: "#555", - }, + color: { + background: + n.id === searchTerm ? "#FF6347" : (isDark ? "#444" : "#D3D3D3"), + border: isDark ? "#999" : "#555", + },
469-491: Simplify reset; avoid double work.Setting graph and also clearing/adding DataSets duplicates effort. Prefer one path.
Consider relying solely on setGraph(initialGraph.current) and let the [graph, options] effect recreate the instance, or skip setGraph and only restore via DataSet operations—don’t do both.
196-208: Optional: build titles via DOM, not innerHTML.You escape dynamic fields, which is good. For defense-in-depth, construct the tooltip via createElement/textContent.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/app/components/TopologyChart.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-30T15:34:06.789Z
Learnt from: Abhi-MS
PR: PalisadoesFoundation/switchmap-ng#333
File: frontend/src/app/components/TopologyChart.tsx:174-178
Timestamp: 2025-07-30T15:34:06.789Z
Learning: In the TopologyChart component, manual cleanup is implemented when devices change to address potential memory leaks from DOM element creation in the htmlTitle function.
Applied to files:
frontend/src/app/components/TopologyChart.tsx
🧬 Code graph analysis (1)
frontend/src/app/components/TopologyChart.tsx (2)
frontend/src/app/types/graphql/GetZoneDevices.ts (1)
DeviceNode(38-48)frontend/src/app/utils/time.ts (1)
formatUptime(6-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test Application
🔇 Additional comments (1)
frontend/src/app/components/TopologyChart.tsx (1)
77-115: Options reactivity: good fix.Including isDark, clickToUse, and zoomView in memo deps and using options in effect deps looks correct.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
frontend/src/app/components/TopologyChart.tsx (4)
349-355: Unify edge reset color with options to avoid theme inconsistencyResets use "#444"/"#BBBBBB" while options use "#888"/"#BBB". Align them.
Apply this diff:
- edgesData.current!.update({ - id: originalEdge.id!, - color: isDark ? "#444" : "#BBBBBB", - }); + edgesData.current!.update({ + id: originalEdge.id!, + color: isDark ? "#888" : "#BBB", + });- edgesData.current!.update({ - id: originalEdge.id!, - color: isDark ? "#444" : "#BBBBBB", - arrows: { to: false }, - }); + edgesData.current!.update({ + id: originalEdge.id!, + color: isDark ? "#888" : "#BBB", + arrows: { to: false }, + });Also applies to: 392-397
439-469: Make search robust to label vs id mismatchCurrently nodesData.get(searchTerm) assumes label===id. Add a label fallback.
Apply this diff:
- const node = nodesData.current.get(searchTerm); - if (!node) { - setSearchResult("No results found for: " + searchTerm); - return; - } else { - // Highlight the node and focus the network - setSearchResult("Showing results for: " + searchTerm); - - networkRef.current.focus(searchTerm, { scale: 1.5, animation: true }); + const byId = nodesData.current.get(searchTerm as any); + const node = Array.isArray(byId) ? byId[0] : byId; + let nodeId = searchTerm; + if (!node) { + const matches = nodesData.current.get({ + filter: (n: any) => + String(n.label ?? "").toLowerCase() === searchTerm.toLowerCase(), + }) as any[]; + if (!matches || matches.length === 0) { + setSearchResult("No results found for: " + searchTerm); + return; + } + nodeId = String(matches[0].id); + } + // Highlight the node and focus the network + setSearchResult("Showing results for: " + nodeId); + + networkRef.current.focus(nodeId, { scale: 1.5, animation: true }); @@ - nodesData.current.get().forEach((n) => { + nodesData.current.get().forEach((n) => { nodesData.current!.update({ id: n.id, color: { - background: n.id === searchTerm ? "#FF6347" : "#D3D3D3", + background: n.id === nodeId ? "#FF6347" : "#D3D3D3", border: "#555", }, font: { color: - n.id === searchTerm + n.id === nodeId ? isDark ? "#fff" : "black" : isDark ? "#aaa" : "#A9A9A9", }, }); }); - } +
246-250: Theme-aware color for extra nodes (optional)Consider adjusting extra node color per theme for visibility.
Apply this diff:
- nodesArray.push({ - id: sysName, - label: sysName, - color: "#383e44", - }); + nodesArray.push({ + id: sysName, + label: sysName, + color: isDark ? "#383e44" : "#E0E0E0", + });
155-193: Deduplicate edges to reduce visual noise (optional)If CDP/LLDP produce repeats, add a Set of keys like
${from}__${to}__${title}to skip duplicates when pushing edges.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/app/components/TopologyChart.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-30T15:34:06.789Z
Learnt from: Abhi-MS
PR: PalisadoesFoundation/switchmap-ng#333
File: frontend/src/app/components/TopologyChart.tsx:174-178
Timestamp: 2025-07-30T15:34:06.789Z
Learning: In the TopologyChart component, manual cleanup is implemented when devices change to address potential memory leaks from DOM element creation in the htmlTitle function.
Applied to files:
frontend/src/app/components/TopologyChart.tsx
🧬 Code graph analysis (1)
frontend/src/app/components/TopologyChart.tsx (2)
frontend/src/app/types/graphql/GetZoneDevices.ts (1)
DeviceNode(38-48)frontend/src/app/utils/time.ts (1)
formatUptime(6-13)
🔇 Additional comments (4)
frontend/src/app/components/TopologyChart.tsx (4)
77-115: Options react correctly to prop/theme changesIncluding clickToUse/zoomView/isDark in deps is correct; options now re-compute and re-init as expected.
163-176: Stable edge IDs implementedAssigning id on edges resolves prior reset/update issues that referenced originalEdge.id.
Also applies to: 177-189
194-206: XSS hardening looks goodEscaping dynamic fields and wrapping via htmlTitle prevents injection in tooltips.
Also applies to: 216-222, 174-175, 188-189
408-417: Network cleanup on dependency changes is correctDestroying the previous instance and nulling refs prevents leaks and duplicate handlers.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
frontend/src/app/components/TopologyChart.tsx (5)
284-291: Fit view after creating the Network.Ensures the initial layout is fully visible; also aligns with the comment about moving fit() to this effect.
networkRef.current = new Network( containerRef.current, { nodes: nodesData.current, edges: edgesData.current, }, options ); + networkRef.current.fit();
349-355: Align edge reset colors with options for consistency.Use the same theme colors as in options (dark: #888, light: #BBB) instead of #444/#BBBBBB.
- edgesData.current!.update({ - id: originalEdge.id!, - color: isDark ? "#444" : "#BBBBBB", - }); + edgesData.current!.update({ + id: originalEdge.id!, + color: isDark ? "#888" : "#BBB", + });- edgesData.current!.update({ - id: originalEdge.id!, - color: isDark ? "#444" : "#BBBBBB", - arrows: { to: false }, - }); + edgesData.current!.update({ + id: originalEdge.id!, + color: isDark ? "#888" : "#BBB", + arrows: { to: false }, + });Also applies to: 392-397
155-161: Avoidanyfor interface nodes; add a minimal shape.Improves type safety and readability.
- (device.l1interfaces?.edges ?? []).forEach( - ({ node: iface }: { node: any }) => { + (device.l1interfaces?.edges ?? []).forEach( + ({ + node: iface, + }: { + node: { + cdpcachedeviceid?: string; + cdpcachedeviceport?: string; + lldpremsysname?: string; + lldpremportdesc?: string; + }; + }) => {
63-63: Use resolvedTheme for accurate dark/light detection.Handles "system" theme correctly.
- const { theme } = useTheme(); + const { resolvedTheme } = useTheme(); @@ - const isDark = theme === "dark"; + const isDark = resolvedTheme === "dark";Also applies to: 76-76
437-469: Include theme in search highlight deps.Ensures highlighted colors update when theme toggles.
- }, [searchTerm]); + }, [searchTerm, isDark]);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/app/components/TopologyChart.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-30T15:34:06.789Z
Learnt from: Abhi-MS
PR: PalisadoesFoundation/switchmap-ng#333
File: frontend/src/app/components/TopologyChart.tsx:174-178
Timestamp: 2025-07-30T15:34:06.789Z
Learning: In the TopologyChart component, manual cleanup is implemented when devices change to address potential memory leaks from DOM element creation in the htmlTitle function.
Applied to files:
frontend/src/app/components/TopologyChart.tsx
🧬 Code graph analysis (1)
frontend/src/app/components/TopologyChart.tsx (2)
frontend/src/app/types/graphql/GetZoneDevices.ts (1)
DeviceNode(38-48)frontend/src/app/utils/time.ts (1)
formatUptime(6-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test Application
🔇 Additional comments (4)
frontend/src/app/components/TopologyChart.tsx (4)
279-283: Handle empty graphs by destroying any existing Network instance.Prevents stale canvases if graph becomes empty.
- // If no graph data is available, do not render the network - if (!containerRef.current || graph.nodes.length === 0) return; + // If no graph data is available, do not render the network + if (!containerRef.current) return; + if (graph.nodes.length === 0) { + networkRef.current?.destroy(); + networkRef.current = null; + nodesData.current = null; + edgesData.current = null; + setAllNodeLabels([]); + return; + }
163-176: Good: stable edge IDs and sanitized titles.Edge IDs are now assigned and titles are escaped/HTMLElement-wrapped. This unblocks reset logic and removes XSS risk.
Also applies to: 177-190
77-115: Good: options are memoized with reactive deps.Including isDark, clickToUse, and zoomView ensures runtime toggles take effect.
408-417: Good: Network cleanup on effect teardown.Destroying the instance and clearing DataSets prevents leaks and duplicate handlers.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
frontend/src/app/components/TopologyChart.tsx (5)
286-288: Avoid stale suggestions when graph becomes emptyWhen graph.nodes.length === 0 you early return without clearing allNodeLabels, leaving stale search suggestions.
Apply this diff:
- // If no graph data is available, do not render the network - if (!containerRef.current || graph.nodes.length === 0) return; + // If no graph data is available, do not render the network + if (!containerRef.current) return; + if (graph.nodes.length === 0) { + setAllNodeLabels([]); + return; + }
299-299: Reset edge colors consistently using Options; avoid hardcoded theme color driftUse the current options.edges.color for resets so colors match the active theme config.
Apply these diffs:
Introduce a base color right after creating the Network:
); + // Resolve default edge color from current options for consistent resets + const baseEdgeColor = + typeof options.edges?.color === "string" + ? (options.edges!.color as string) + : isDark + ? "#888" + : "#BBB";Use it in deselectNode:
- initialGraph.current.edges.forEach((originalEdge) => { - edgesData.current!.update({ - id: originalEdge.id!, - color: isDark ? "#444" : "#BBBBBB", - }); - }); + initialGraph.current.edges.forEach((originalEdge) => { + edgesData.current!.update({ + id: originalEdge.id!, + color: baseEdgeColor, + }); + });Use it in deselectEdge:
- initialGraph.current.edges.forEach((originalEdge) => { - edgesData.current!.update({ - id: originalEdge.id!, - color: isDark ? "#444" : "#BBBBBB", - arrows: { to: false }, - }); - }); + initialGraph.current.edges.forEach((originalEdge) => { + edgesData.current!.update({ + id: originalEdge.id!, + color: baseEdgeColor, + arrows: { to: false }, + }); + });Also applies to: 356-361, 398-405
303-311: Use VisNode typing in doubleClick handler instead of anyRemoves unnecessary array check and any-casts; improves type safety.
Apply this diff:
- const nodeData = nodesData.current?.get(nodeId); - const node = Array.isArray(nodeData) ? nodeData[0] : nodeData; - - // Only navigate if idxDevice exists - if ((node as any)?.idxDevice) { - const idxDevice = (node as any).idxDevice; - const sysName = (node as any)?.label ?? ""; + const node = nodesData.current?.get(nodeId) as VisNode | undefined; + if (node?.idxDevice) { + const idxDevice = node.idxDevice; + const sysName = String(node.label ?? "");
443-475: Re-apply highlight after network re-create/theme changeWhen options/graph changes (network re-created), the searchTerm highlight is lost; include graph/options in deps to re-apply.
Apply this diff:
- }, [searchTerm]); + }, [searchTerm, graph, options]);
487-499: Simplify reset: rely on setGraph + effect to re-create networkClearing/re-adding DataSets is redundant since setGraph triggers the effect to rebuild the Network; consider removing mid-flight mutations to avoid flicker.
Suggested simplification:
- Keep setGraph(initialGraph.current) and unselectAll().
- Drop nodesData.current.clear()/edgesData.current.clear()/add(...) and just call fit() after the effect runs (or omit fit() and let initial mount behavior handle it).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/app/components/TopologyChart.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-30T15:34:06.789Z
Learnt from: Abhi-MS
PR: PalisadoesFoundation/switchmap-ng#333
File: frontend/src/app/components/TopologyChart.tsx:174-178
Timestamp: 2025-07-30T15:34:06.789Z
Learning: In the TopologyChart component, manual cleanup is implemented when devices change to address potential memory leaks from DOM element creation in the htmlTitle function.
Applied to files:
frontend/src/app/components/TopologyChart.tsx
📚 Learning: 2025-09-23T19:27:36.395Z
Learnt from: Abhi-MS
PR: PalisadoesFoundation/switchmap-ng#341
File: frontend/src/app/components/TopologyChart.tsx:231-231
Timestamp: 2025-09-23T19:27:36.395Z
Learning: In TopologyChart component, vis-network tooltips use their own styling context separate from the application's theme system, so hardcoded white text color works correctly against the tooltip's background in both light and dark themes.
Applied to files:
frontend/src/app/components/TopologyChart.tsx
🧬 Code graph analysis (1)
frontend/src/app/components/TopologyChart.tsx (2)
frontend/src/app/types/graphql/GetZoneDevices.ts (1)
DeviceNode(38-48)frontend/src/app/utils/time.ts (1)
formatUptime(6-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test Application
🔇 Additional comments (3)
frontend/src/app/components/TopologyChart.tsx (3)
165-193: Good: Stable edge IDs and sanitized tooltips
- Stable edge IDs resolve update/reset correctness.
- Tooltips are properly escaped via escapeHtml and rendered via HTMLElement.
Also applies to: 197-209, 213-247
414-424: Good: Proper Network cleanup to prevent leaksCleanup destroys the Network and nulls DataSets, avoiding duplicate handlers and memory leaks.
288-289: Fix TS generic mismatch: use DataSetnodesData is typed as DataSet, but you’re instantiating DataSet. This will fail type-checking and weakens type safety for idxDevice.
Apply this diff:
- nodesData.current = new DataSet<Node>(graph.nodes); + nodesData.current = new DataSet<VisNode>(graph.nodes);
|
@palisadoes @aashimawadhwa @DMills27 Code rabbit has approved this PR, please review. |
aashimawadhwa
merged commit eee9db6
into
PalisadoesFoundation:develop-abhi
What kind of change does this PR introduce?
(Backend work on extending the poller to collect these metrics automatically is pending, once that is finished charts will update with live SNMP data. Currently only ifspeed is received by the frontend)
- Pagination → navigate through large connection sets.
- Expand all / Collapse all → quickly open or close the whole list.
- Download as CSV → export raw connection data for external analysis.
Issue Number:
Fixes #338
Snapshots/Videos:
Connection Charts UI
Connection.Charts.mp4
Connection Charts Responsiveness
SwitchMap.PR.5.2.mp4
Connection Charts Download Button
SwitchMap.PR.5.3.mp4
Frontend Test Coverage
If relevant, did you update the documentation?
Will update docs in the next PR to avoid lengthy PRs
Summary
Introduces connection charts tab, set up frontend testing and overall ui polish.
Does this PR introduce a breaking change?
No
Checklist
CodeRabbit AI Review
Test Coverage
Other information
This feature is part of the ongoing GSOC 2025 project
Have you read the contributing guide?
Yes
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests
Chores