fix: slim packet loss history payloads#222
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis PR implements packet-loss history pagination by decoupling MTR data from summary responses. The backend introduces paginated summary endpoints (without MTR data) and on-demand detail endpoints (with full MTR data). Database indexes improve query performance. Frontend migrates to infinite scroll with lazy-loaded detail fetching. Changes
Sequence DiagramsequenceDiagram
participant User
participant FrontendUI as Frontend<br/>(MonitorResultsTable)
participant APILayer as API Layer<br/>(getPacketLossHistory)
participant Backend as Backend<br/>(Handler)
participant Database as Database
User->>FrontendUI: Load monitor history
FrontendUI->>APILayer: getPacketLossHistory(page=1)
APILayer->>Backend: GET /packetloss/monitors/:id/history?page=1&limit=25
Backend->>Database: GetPacketLossResults(monitorID, page=1, limit=25)
Database-->>Backend: PaginatedPacketLossResults{data, total, page, limit}
Backend-->>APILayer: 200 JSON (summary, no MTR data)
APILayer-->>FrontendUI: PaginatedResponse with summary list
FrontendUI->>FrontendUI: Render rows, show "Load More"
User->>FrontendUI: Expand row (view MTR details)
FrontendUI->>APILayer: getPacketLossHistoryDetail(resultId)
APILayer->>Backend: GET /packetloss/monitors/:id/history/:resultId
Backend->>Database: GetPacketLossResultDetail(monitorID, resultID)
Database-->>Backend: PacketLossResult{data, MTRData}
Backend-->>APILayer: 200 JSON (with MTR data)
APILayer-->>FrontendUI: PacketLossResultDetail (cache)
FrontendUI->>FrontendUI: Parse MTRData, render detail view
User->>FrontendUI: Scroll & click "Load More"
FrontendUI->>APILayer: getPacketLossHistory(page=2)
APILayer->>Backend: GET /packetloss/monitors/:id/history?page=2&limit=25
Backend->>Database: GetPacketLossResults(monitorID, page=2, limit=25)
Database-->>Backend: Next page results
Backend-->>APILayer: 200 JSON
APILayer-->>FrontendUI: Append next page to list
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/superpowers/plans/2026-04-09-packetloss-history-cutover.md (1)
15-43: Replace absolute paths with repository-relative paths.The file map contains absolute paths like
/Users/soup/.codex/worktrees/4fd8/netronome/internal/types/types.gowhich are specific to a developer's local machine. These should be relative paths from the repository root for portability.Example fix for one entry
-- Modify: `/Users/soup/.codex/worktrees/4fd8/netronome/internal/types/types.go` +- Modify: `internal/types/types.go`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-04-09-packetloss-history-cutover.md` around lines 15 - 43, The plan file docs/superpowers/plans/2026-04-09-packetloss-history-cutover.md contains absolute developer-local paths (e.g. /Users/soup/.codex/worktrees/4fd8/netronome/internal/...) — replace all such absolute paths with repository-relative paths from the repo root (e.g. internal/types/types.go, internal/database/packetloss.go, web/src/components/... etc.), ensuring both modified and added entries (like the new migrations and web files) use the relative form; search for any lines beginning with “/Users/” in that file and update them consistently so the plan is portable.web/src/components/speedtest/packetloss/components/MonitorResultsTable.tsx (1)
280-293: Button shows hover styles while disabled.The "Load More" button has
disabled={isFetchingMore}but the className lacksdisabled:variants, so hover effects remain active on disabled state. This can mislead users into thinking the button is interactive.♻️ Suggested enhancement
className="px-4 py-2 bg-gray-200/30 dark:bg-gray-800/30 border border-gray-300/50 dark:border-gray-900/50 text-gray-600/50 dark:text-gray-300/50 hover:text-gray-700 dark:hover:text-gray-300 rounded-lg hover:bg-gray-300/50 dark:hover:bg-gray-800/50 transition-colors" + className="px-4 py-2 bg-gray-200/30 dark:bg-gray-800/30 border border-gray-300/50 dark:border-gray-900/50 text-gray-600/50 dark:text-gray-300/50 rounded-lg transition-colors disabled:opacity-50 disabled:cursor-not-allowed hover:enabled:text-gray-700 dark:hover:enabled:text-gray-300 hover:enabled:bg-gray-300/50 dark:hover:enabled:bg-gray-800/50"The same applies to the mobile "Load More" button at lines 419-432.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/speedtest/packetloss/components/MonitorResultsTable.tsx` around lines 280 - 293, The Load More button in MonitorResultsTable.tsx is still showing hover styles when disabled via disabled={isFetchingMore}; update the button rendering (both desktop and mobile instances around the onLoadMore handlers) to either add Tailwind disabled: utility classes (e.g., disabled:opacity-50 disabled:cursor-not-allowed disabled:hover:bg-transparent disabled:hover:text-current) to the className or conditionally remove hover classes when isFetchingMore is true so hover styles and pointer interactions are suppressed; ensure both the button using onLoadMore and its mobile counterpart use the same disabled styling logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/components/speedtest/packetloss/components/MonitorResultsTable.tsx`:
- Around line 252-265: The two expanded-state rows render with colSpan={8}
though the table only has 7 columns; update both occurrences in
MonitorResultsTable.tsx (the conditional blocks referencing result.usedMtr &&
isExpanded && isLoadingDetail and result.usedMtr && isExpanded &&
hasDetailError) to use colSpan={7} so the loading and error rows span the
correct number of columns.
---
Nitpick comments:
In `@docs/superpowers/plans/2026-04-09-packetloss-history-cutover.md`:
- Around line 15-43: The plan file
docs/superpowers/plans/2026-04-09-packetloss-history-cutover.md contains
absolute developer-local paths (e.g.
/Users/soup/.codex/worktrees/4fd8/netronome/internal/...) — replace all such
absolute paths with repository-relative paths from the repo root (e.g.
internal/types/types.go, internal/database/packetloss.go, web/src/components/...
etc.), ensuring both modified and added entries (like the new migrations and web
files) use the relative form; search for any lines beginning with “/Users/” in
that file and update them consistently so the plan is portable.
In `@web/src/components/speedtest/packetloss/components/MonitorResultsTable.tsx`:
- Around line 280-293: The Load More button in MonitorResultsTable.tsx is still
showing hover styles when disabled via disabled={isFetchingMore}; update the
button rendering (both desktop and mobile instances around the onLoadMore
handlers) to either add Tailwind disabled: utility classes (e.g.,
disabled:opacity-50 disabled:cursor-not-allowed disabled:hover:bg-transparent
disabled:hover:text-current) to the className or conditionally remove hover
classes when isFetchingMore is true so hover styles and pointer interactions are
suppressed; ensure both the button using onLoadMore and its mobile counterpart
use the same disabled styling logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8ad4fe33-0f46-429e-8865-5a6d25634f80
📒 Files selected for processing (17)
docs/superpowers/plans/2026-04-09-packetloss-history-cutover.mdinternal/database/cascade_delete_integration_test.gointernal/database/database.gointernal/database/migrations/postgres/021_packetloss_history_summary_index_postgres.sqlinternal/database/migrations/sqlite/021_packetloss_history_summary_index.sqlinternal/database/packetloss.gointernal/database/packetloss_integration_test.gointernal/handlers/packetloss.gointernal/server/server.gointernal/types/types.goweb/src/api/packetloss.tsweb/src/components/speedtest/TracerouteTab.tsxweb/src/components/speedtest/packetloss/PacketLossMonitorDetails.tsxweb/src/components/speedtest/packetloss/components/MonitorResultsTable.tsxweb/src/components/speedtest/packetloss/constants/packetLossConstants.tsweb/src/components/speedtest/packetloss/hooks/usePacketLossMonitorStatus.tsweb/src/types/types.ts
Comment on lines
+252
to
+265
| {result.usedMtr && isExpanded && isLoadingDetail && ( | ||
| <tr className="bg-gray-100/50 dark:bg-gray-900/50"> | ||
| <td colSpan={8} className="p-4 text-sm text-gray-600 dark:text-gray-400"> | ||
| Loading MTR details... | ||
| </td> | ||
| </tr> | ||
| )} | ||
| {result.usedMtr && isExpanded && hasDetailError && ( | ||
| <tr className="bg-gray-100/50 dark:bg-gray-900/50"> | ||
| <td colSpan={8} className="p-4 text-sm text-red-600 dark:text-red-400"> | ||
| Failed to load MTR details. | ||
| </td> | ||
| </tr> | ||
| )} |
There was a problem hiding this comment.
Incorrect colSpan value — table has 7 columns, not 8.
The loading and error rows use colSpan={8}, but the table header defines only 7 columns (Time, Loss, Avg RTT, Min RTT, Max RTT, Sent/Recv, Mode). This doesn't break rendering but is semantically incorrect.
🔧 Proposed fix
{result.usedMtr && isExpanded && isLoadingDetail && (
<tr className="bg-gray-100/50 dark:bg-gray-900/50">
- <td colSpan={8} className="p-4 text-sm text-gray-600 dark:text-gray-400">
+ <td colSpan={7} className="p-4 text-sm text-gray-600 dark:text-gray-400">
Loading MTR details...
</td>
</tr>
)}
{result.usedMtr && isExpanded && hasDetailError && (
<tr className="bg-gray-100/50 dark:bg-gray-900/50">
- <td colSpan={8} className="p-4 text-sm text-red-600 dark:text-red-400">
+ <td colSpan={7} className="p-4 text-sm text-red-600 dark:text-red-400">
Failed to load MTR details.
</td>
</tr>
)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {result.usedMtr && isExpanded && isLoadingDetail && ( | |
| <tr className="bg-gray-100/50 dark:bg-gray-900/50"> | |
| <td colSpan={8} className="p-4 text-sm text-gray-600 dark:text-gray-400"> | |
| Loading MTR details... | |
| </td> | |
| </tr> | |
| )} | |
| {result.usedMtr && isExpanded && hasDetailError && ( | |
| <tr className="bg-gray-100/50 dark:bg-gray-900/50"> | |
| <td colSpan={8} className="p-4 text-sm text-red-600 dark:text-red-400"> | |
| Failed to load MTR details. | |
| </td> | |
| </tr> | |
| )} | |
| {result.usedMtr && isExpanded && isLoadingDetail && ( | |
| <tr className="bg-gray-100/50 dark:bg-gray-900/50"> | |
| <td colSpan={7} className="p-4 text-sm text-gray-600 dark:text-gray-400"> | |
| Loading MTR details... | |
| </td> | |
| </tr> | |
| )} | |
| {result.usedMtr && isExpanded && hasDetailError && ( | |
| <tr className="bg-gray-100/50 dark:bg-gray-900/50"> | |
| <td colSpan={7} className="p-4 text-sm text-red-600 dark:text-red-400"> | |
| Failed to load MTR details. | |
| </td> | |
| </tr> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/src/components/speedtest/packetloss/components/MonitorResultsTable.tsx`
around lines 252 - 265, The two expanded-state rows render with colSpan={8}
though the table only has 7 columns; update both occurrences in
MonitorResultsTable.tsx (the conditional blocks referencing result.usedMtr &&
isExpanded && isLoadingDetail and result.usedMtr && isExpanded &&
hasDetailError) to use colSpan={7} so the loading and error rows span the
correct number of columns.
Collaborator
Author
|
@codex review |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
mtrDatafrom the normal history payload and keep the existing expand UI via a detail fetchWhy
Testing
pnpm -C web lintpnpm -C web buildgo test ./...Screenshots (if UI)
Checklist
Summary by CodeRabbit
Release Notes
New Features
Performance Improvements