aggregated traffic events#693
Conversation
📝 WalkthroughWalkthroughThis PR introduces optional aggregation fields on ChangesAggregated Traffic Events
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant TrafficEventsTable
participant TrafficEvent as "getTrafficEventCounts"
participant TrafficEventsTextCell
participant TrafficEventDescription
TrafficEventsTable->>TrafficEvent: getTrafficEventCounts(event)
TrafficEvent-->>TrafficEventsTable: isAggregated, drops, starts, ends
TrafficEventsTable->>TrafficEventsTable: decide renderExpandedRow (policy.id check)
TrafficEventsTextCell->>TrafficEvent: getTrafficEventCounts(event)
TrafficEvent-->>TrafficEventsTextCell: isAggregated, drops
TrafficEventsTextCell->>TrafficEventDescription: render(showCaret=isExpandable)
TrafficEventDescription->>TrafficEvent: getTrafficEventCounts(event)
TrafficEvent-->>TrafficEventDescription: starts, ends, drops
TrafficEventDescription->>TrafficEventDescription: aggregatedMessage() / eventWindow render
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/cloud/traffic-events/TrafficEventsFilter.tsxOops! Something went wrong! :( ESLint: 9.39.3 TypeError: Converting circular structure to JSON src/cloud/traffic-events/TrafficEventsTable.tsxOops! Something went wrong! :( ESLint: 9.39.3 TypeError: Converting circular structure to JSON src/cloud/traffic-events/interfaces/TrafficEvent.tsOops! Something went wrong! :( ESLint: 9.39.3 TypeError: Converting circular structure to JSON
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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cloud/traffic-events/table/TrafficEventDescription.tsx`:
- Around line 104-158: The blocked-connections text in aggregatedMessage() does
not respect info.isInbound, so the actor/phrasing is wrong for inbound traffic.
Update the drops sentence in TrafficEventDescription.tsx to mirror the
direction-aware logic used by the initiated clause and the other blocked-message
branches in this component, using info.sourceName/info.destinationName and the
correct “trying to connect” wording based on traffic direction.
🪄 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: 2d8ce9a0-9eef-4835-aaae-f9864085ef74
📒 Files selected for processing (7)
src/cloud/traffic-events/TrafficEventsFilter.tsxsrc/cloud/traffic-events/TrafficEventsTable.tsxsrc/cloud/traffic-events/interfaces/TrafficEvent.tssrc/cloud/traffic-events/table/TrafficEventDescription.tsxsrc/cloud/traffic-events/table/TrafficEventsDetailRow.tsxsrc/cloud/traffic-events/table/TrafficEventsTextCell.tsxsrc/contexts/PeersProvider.tsx
| const aggregatedMessage = () => { | ||
| const { starts, ends, drops } = getTrafficEventCounts(event); | ||
| const connections = (n: number) => | ||
| `${n.toLocaleString()} ${n === 1 ? "connection" : "connections"}`; | ||
|
|
||
| const joinClauses = (clauses: string[]) => | ||
| clauses.map((clause, index) => ( | ||
| <React.Fragment key={clause}> | ||
| {index > 0 && (index === clauses.length - 1 ? " and " : ", ")} | ||
| {clause} | ||
| </React.Fragment> | ||
| )); | ||
|
|
||
| const startVerb = info.isInbound ? "accepted" : "started"; | ||
| const initiated = [ | ||
| starts > 0 && `${startVerb} ${connections(starts)}`, | ||
| ends > 0 && `ended ${connections(ends)}`, | ||
| ].filter(Boolean) as string[]; | ||
|
|
||
| const sentences: React.ReactNode[] = []; | ||
| if (initiated.length > 0) { | ||
| sentences.push( | ||
| info.isInbound ? ( | ||
| <> | ||
| {info.destinationName} {joinClauses(initiated)} from{" "} | ||
| {info.sourceName} | ||
| </> | ||
| ) : ( | ||
| <> | ||
| {info.sourceName} {joinClauses(initiated)} to {info.destinationName} | ||
| </> | ||
| ), | ||
| ); | ||
| } | ||
| if (drops > 0) { | ||
| sentences.push( | ||
| <> | ||
| {info.sourceName} got blocked {drops.toLocaleString()}{" "} | ||
| {drops === 1 ? "time" : "times"} trying to connect to{" "} | ||
| {info.destinationName} | ||
| </>, | ||
| ); | ||
| } | ||
|
|
||
| return ( | ||
| <> | ||
| {sentences.map((sentence, index) => ( | ||
| <React.Fragment key={index}> | ||
| {index > 0 && ". "} | ||
| {sentence} | ||
| </React.Fragment> | ||
| ))} | ||
| </> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Blocked-connections sentence in aggregatedMessage() ignores traffic direction.
The initiated clause correctly swaps phrasing/actor based on info.isInbound (lines 117, 126-136), but the drops sentence always uses {sourceName} got blocked ... trying to connect to {destinationName} regardless of direction (lines 138-146). This is inconsistent with both the initiated-connections handling above it and the non-aggregated blocked messages elsewhere in this file (lines 251-271, 308-322), which do vary phrasing/actor by direction. For inbound aggregated blocked traffic this reads misleadingly, attributing the "connecting" action to the wrong party.
🩹 Proposed fix
if (drops > 0) {
sentences.push(
- <>
- {info.sourceName} got blocked {drops.toLocaleString()}{" "}
- {drops === 1 ? "time" : "times"} trying to connect to{" "}
- {info.destinationName}
- </>,
+ info.isInbound ? (
+ <>
+ {info.destinationName} blocked {drops.toLocaleString()}{" "}
+ {drops === 1 ? "connection" : "connections"} from{" "}
+ {info.sourceName}
+ </>
+ ) : (
+ <>
+ {info.sourceName} got blocked {drops.toLocaleString()}{" "}
+ {drops === 1 ? "time" : "times"} trying to connect to{" "}
+ {info.destinationName}
+ </>
+ ),
);
}📝 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.
| const aggregatedMessage = () => { | |
| const { starts, ends, drops } = getTrafficEventCounts(event); | |
| const connections = (n: number) => | |
| `${n.toLocaleString()} ${n === 1 ? "connection" : "connections"}`; | |
| const joinClauses = (clauses: string[]) => | |
| clauses.map((clause, index) => ( | |
| <React.Fragment key={clause}> | |
| {index > 0 && (index === clauses.length - 1 ? " and " : ", ")} | |
| {clause} | |
| </React.Fragment> | |
| )); | |
| const startVerb = info.isInbound ? "accepted" : "started"; | |
| const initiated = [ | |
| starts > 0 && `${startVerb} ${connections(starts)}`, | |
| ends > 0 && `ended ${connections(ends)}`, | |
| ].filter(Boolean) as string[]; | |
| const sentences: React.ReactNode[] = []; | |
| if (initiated.length > 0) { | |
| sentences.push( | |
| info.isInbound ? ( | |
| <> | |
| {info.destinationName} {joinClauses(initiated)} from{" "} | |
| {info.sourceName} | |
| </> | |
| ) : ( | |
| <> | |
| {info.sourceName} {joinClauses(initiated)} to {info.destinationName} | |
| </> | |
| ), | |
| ); | |
| } | |
| if (drops > 0) { | |
| sentences.push( | |
| <> | |
| {info.sourceName} got blocked {drops.toLocaleString()}{" "} | |
| {drops === 1 ? "time" : "times"} trying to connect to{" "} | |
| {info.destinationName} | |
| </>, | |
| ); | |
| } | |
| return ( | |
| <> | |
| {sentences.map((sentence, index) => ( | |
| <React.Fragment key={index}> | |
| {index > 0 && ". "} | |
| {sentence} | |
| </React.Fragment> | |
| ))} | |
| </> | |
| ); | |
| }; | |
| const aggregatedMessage = () => { | |
| const { starts, ends, drops } = getTrafficEventCounts(event); | |
| const connections = (n: number) => | |
| `${n.toLocaleString()} ${n === 1 ? "connection" : "connections"}`; | |
| const joinClauses = (clauses: string[]) => | |
| clauses.map((clause, index) => ( | |
| <React.Fragment key={clause}> | |
| {index > 0 && (index === clauses.length - 1 ? " and " : ", ")} | |
| {clause} | |
| </React.Fragment> | |
| )); | |
| const startVerb = info.isInbound ? "accepted" : "started"; | |
| const initiated = [ | |
| starts > 0 && `${startVerb} ${connections(starts)}`, | |
| ends > 0 && `ended ${connections(ends)}`, | |
| ].filter(Boolean) as string[]; | |
| const sentences: React.ReactNode[] = []; | |
| if (initiated.length > 0) { | |
| sentences.push( | |
| info.isInbound ? ( | |
| <> | |
| {info.destinationName} {joinClauses(initiated)} from{" "} | |
| {info.sourceName} | |
| </> | |
| ) : ( | |
| <> | |
| {info.sourceName} {joinClauses(initiated)} to {info.destinationName} | |
| </> | |
| ), | |
| ); | |
| } | |
| if (drops > 0) { | |
| sentences.push( | |
| info.isInbound ? ( | |
| <> | |
| {info.destinationName} blocked {drops.toLocaleString()}{" "} | |
| {drops === 1 ? "connection" : "connections"} from{" "} | |
| {info.sourceName} | |
| </> | |
| ) : ( | |
| <> | |
| {info.sourceName} got blocked {drops.toLocaleString()}{" "} | |
| {drops === 1 ? "time" : "times"} trying to connect to{" "} | |
| {info.destinationName} | |
| </> | |
| ), | |
| ); | |
| } | |
| return ( | |
| <> | |
| {sentences.map((sentence, index) => ( | |
| <React.Fragment key={index}> | |
| {index > 0 && ". "} | |
| {sentence} | |
| </React.Fragment> | |
| ))} | |
| </> | |
| ); | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cloud/traffic-events/table/TrafficEventDescription.tsx` around lines 104
- 158, The blocked-connections text in aggregatedMessage() does not respect
info.isInbound, so the actor/phrasing is wrong for inbound traffic. Update the
drops sentence in TrafficEventDescription.tsx to mirror the direction-aware
logic used by the initiated clause and the other blocked-message branches in
this component, using info.sourceName/info.destinationName and the correct
“trying to connect” wording based on traffic direction.
2 participants
Issue ticket number and link
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
E2E tests
Optional: override the image tags used by the Playwright e2e workflow.
Defaults to
mainwhen omitted.management-cloud-tag: main
reverse-proxy-tag: main
Summary by CodeRabbit
New Features
Bug Fixes