Refactor Network Analyser for Modularity and Performance#1
Refactor Network Analyser for Modularity and Performance#1
Conversation
- Restructured project into `model`, `service`, `ui` packages. - Introduced `Packet` model with composition-based protocol headers (`EthernetHeader`, `IPHeader`, etc.) replacing inheritance. - Implemented `HexDumpParser` in `service` for robust and efficient file parsing. - Refactored UI to use `PacketViewModel` and `PacketTreeBuilder` for better separation of concerns and performance. - Added background file loading using `Task` to prevent UI freezing. - Enhanced UI with SplitPane layout, Hex View, and Status Bar. - Re-implemented Export functionality. - Added unit tests for parser and protocol headers. Co-authored-by: zhenyuefu <56567636+zhenyuefu@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Changed `on: create` to `on: push` with `tags: ['*']`. The previous configuration triggered the release workflow on branch creation, which caused failures because `action-gh-release` expects a tag ref. This ensures the workflow only runs when a tag is pushed. Co-authored-by: zhenyuefu <56567636+zhenyuefu@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors the Network Analyser into a more modular Model/Service/UI structure, replacing the prior inheritance-heavy packet model and parser with composition-based headers and a streaming hexdump parser, alongside a refreshed JavaFX UI (table + protocol tree + hex view + status bar).
Changes:
- Introduces composition-based packet/header model (
Packet,EthernetHeader,IPHeader,UDPHeader,DNSHeader,DHCPHeader). - Replaces parsing with
HexDumpParserand adds unit tests for parsing and header decoding. - Updates JavaFX UI layout (
net.fxml) and controller logic for background load/export plus new hex/status views.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/projectreseau/networkanalyser/MainController.java | Refactored controller to load/export in background, bind TableView to PacketViewModel, and populate tree + hex views. |
| src/main/resources/projectreseau/networkanalyser/net.fxml | Moves to BorderPane/SplitPane layout; adds hex view and status bar. |
| src/main/java/projectreseau/networkanalyser/service/HexDumpParser.java | New line-by-line hexdump parser producing model.Packet instances. |
| src/main/java/projectreseau/networkanalyser/ui/PacketViewModel.java | New view model deriving table fields from parsed headers. |
| src/main/java/projectreseau/networkanalyser/ui/PacketTreeBuilder.java | Builds protocol detail tree from headers for UI/export. |
| src/main/java/projectreseau/networkanalyser/model/Packet.java | New raw-packet container with id/length/data. |
| src/main/java/projectreseau/networkanalyser/model/EthernetHeader.java | Ethernet header decoding utilities. |
| src/main/java/projectreseau/networkanalyser/model/IPHeader.java | IPv4 header decoding utilities. |
| src/main/java/projectreseau/networkanalyser/model/UDPHeader.java | UDP header decoding utilities. |
| src/main/java/projectreseau/networkanalyser/model/DNSHeader.java | DNS header decoding utilities. |
| src/main/java/projectreseau/networkanalyser/model/DHCPHeader.java | DHCP/BOOTP header field accessors used for tree display. |
| src/test/java/projectreseau/networkanalyser/service/HexDumpParserTest.java | Tests parsing multiple packets from string input. |
| src/test/java/projectreseau/networkanalyser/model/ProtocolHeaderTest.java | Tests Ethernet and IPv4 header field decoding. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| EthernetHeader eth = new EthernetHeader(packet.getData(), 0); | ||
| String p = eth.getTypeDescription(); | ||
| String s = eth.getSourceMac(); | ||
| String d = eth.getDestinationMac(); | ||
| String i = ""; | ||
|
|
||
| try { |
There was a problem hiding this comment.
The initial Ethernet parsing (getTypeDescription(), getSourceMac(), getDestinationMac()) happens before the try block. If the packet is truncated (< 14 bytes) this can throw and prevent the table from loading. Consider moving the Ethernet parsing inside the try (or validating length first) and falling back to safe defaults on failure.
| EthernetHeader eth = new EthernetHeader(packet.getData(), 0); | |
| String p = eth.getTypeDescription(); | |
| String s = eth.getSourceMac(); | |
| String d = eth.getDestinationMac(); | |
| String i = ""; | |
| try { | |
| String p = "Unknown"; | |
| String s = ""; | |
| String d = ""; | |
| String i = ""; | |
| try { | |
| EthernetHeader eth = new EthernetHeader(packet.getData(), 0); | |
| p = eth.getTypeDescription(); | |
| s = eth.getSourceMac(); | |
| d = eth.getDestinationMac(); |
| task.setOnSucceeded(e -> { | ||
| List<Packet> packets = task.getValue(); | ||
| packetList.clear(); | ||
| packetList.addAll(packets.stream().map(PacketViewModel::new).collect(Collectors.toList())); | ||
| if (statusLabel != null) statusLabel.setText("Loaded " + packets.size() + " packets from " + file.getName()); | ||
| }); |
There was a problem hiding this comment.
PacketViewModel instances are created in setOnSucceeded, which runs on the JavaFX Application Thread. For large captures, this will freeze the UI even though file parsing was offloaded. Consider doing the packets -> PacketViewModel transformation inside the Task.call() and only applying the final list to packetList on success.
| Task<Void> task = new Task<>() { | ||
| @Override | ||
| protected Void call() throws Exception { | ||
| try (java.io.PrintWriter writer = new java.io.PrintWriter(file)) { | ||
| for (PacketViewModel pvm : packetList) { | ||
| Packet packet = pvm.getPacket(); | ||
| TreeItem<String> root = PacketTreeBuilder.build(packet); | ||
| dumpTree(writer, root, ""); | ||
| writer.println(); | ||
| writer.println("--------------------------------------------------"); | ||
| writer.println(); | ||
| } | ||
| } |
There was a problem hiding this comment.
exportPackets iterates over packetList from the background Task thread. ObservableList is not thread-safe, and this can race with UI updates (or throw during iteration). Snapshot the current items on the FX thread before starting the task (e.g., copy to a plain List) and iterate over that snapshot in the background.
| new Thread(task).start(); | ||
| } |
There was a problem hiding this comment.
Both loadPackets and exportPackets start raw Threads without setting them as daemon or using a managed executor. Non-daemon threads can keep the JVM alive after the window closes, and unmanaged threads make cancellation/lifecycle harder. Prefer an ExecutorService (or set thread name + daemon) and consider wiring Task cancellation to UI actions.
| List<Packet> packets = new ArrayList<>(); | ||
| int packetId = 1; | ||
| ByteArrayOutputStream buffer = new ByteArrayOutputStream(); | ||
| int expectedOffset = 0; // The next expected byte offset within the current packet | ||
|
|
There was a problem hiding this comment.
HexDumpParser uses an unbounded ByteArrayOutputStream for packet accumulation. If the input is malformed (missing a new packet offset) or extremely large, this can grow until OOM. Consider enforcing a maximum packet size (similar to the previous 64KiB snaplength) and terminating/starting a new packet or failing fast when exceeded.
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| public class HexDumpParser { | ||
|
|
||
| public List<Packet> parse(File file) throws IOException { | ||
| try (BufferedReader reader = new BufferedReader(new FileReader(file))) { |
There was a problem hiding this comment.
parse(File) uses FileReader, which relies on the platform default charset. Since the project encoding is UTF-8, consider using an explicit charset (e.g., Files.newBufferedReader(..., UTF_8)) to avoid inconsistent behavior across machines.
| import java.util.ArrayList; | |
| import java.util.List; | |
| public class HexDumpParser { | |
| public List<Packet> parse(File file) throws IOException { | |
| try (BufferedReader reader = new BufferedReader(new FileReader(file))) { | |
| import java.nio.charset.StandardCharsets; | |
| import java.nio.file.Files; | |
| import java.util.ArrayList; | |
| import java.util.List; | |
| public class HexDumpParser { | |
| public List<Packet> parse(File file) throws IOException { | |
| try (BufferedReader reader = Files.newBufferedReader(file.toPath(), StandardCharsets.UTF_8)) { |
| StringBuilder sb = new StringBuilder(); | ||
| for (int i = 0; i < 6; i++) { | ||
| if (i > 0) sb.append(":"); | ||
| sb.append(String.format("%02x", bytes[pos + i])); |
There was a problem hiding this comment.
extractMac formats signed byte values with %02x, which can produce strings like ffffff8c for bytes >= 0x80. Mask each byte with & 0xFF (or otherwise convert to an unsigned int) before formatting so MAC addresses are always two hex digits per octet; this also makes ProtocolHeaderTest expectations pass.
| sb.append(String.format("%02x", bytes[pos + i])); | |
| sb.append(String.format("%02x", bytes[pos + i] & 0xFF)); |
| for (int j = 0; j < 16; j++) { | ||
| if (i + j < data.length) { | ||
| sb.append(String.format("%02x ", data[i + j])); | ||
| } else { | ||
| sb.append(" "); | ||
| } | ||
| if (j == 7) sb.append(" "); | ||
| } |
There was a problem hiding this comment.
String.format("%02x", data[i + j]) formats a signed byte and can output ffffff8c-style values for bytes >= 0x80, which breaks the hex view. Convert the byte to an unsigned int (e.g., mask with & 0xFF) before formatting.
|
|
||
| for (int j = 0; j < 16; j++) { | ||
| if (i + j < data.length) { | ||
| sb.append(String.format("%02x ", data[i + j])); |
There was a problem hiding this comment.
This array access might be out of bounds, as the index might be equal to the array length + 14.
|
|
||
| for (int j = 0; j < 16; j++) { | ||
| if (i + j < data.length) { | ||
| char c = (char) (data[i + j] & 0xFF); |
There was a problem hiding this comment.
This array access might be out of bounds, as the index might be equal to the array length + 14.
This PR completely refactors the Network Analyser application to address performance issues, improve code modularity, and enhance the user interface.
Key Changes:
IPPacket extends EthernetPacket) with a composition-based approach where aPacketholds raw data and specificHeaderclasses parse protocols on demand. This improves flexibility and correctness.PacketAnalyserwith a newHexDumpParserthat reads files line-by-line and handles large files more gracefully.PacketViewModelfor theTableViewto efficiently display packet summaries.Task.net.fxmlto useBorderPaneandSplitPanefor a resizable and modern layout.PR created automatically by Jules for task 4288025737854752343 started by @zhenyuefu