Adding Label H2 header 02E parser #294
Conversation
Bug Report
Comments? Email us. |
|
Warning Rate limit exceeded@makrsmark has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 12 minutes and 1 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdd a new H2-02E weather report decoder plugin (Label_H2_02E) that parses fixed-width weather formats containing wind, temperature, and flight-level data. Introduce a Wind type interface, refactor wind data collection in h1_helper.ts, and add windData() formatting method to ResultFormatter. Changes
Sequence DiagramsequenceDiagram
participant Msg as Message
participant Plugin as Label_H2_02E<br/>(Decoder Plugin)
participant Parser as parseWeatherReport()
participant Utils as DateTimeUtils<br/>CoordinateUtils
participant Formatter as ResultFormatter
participant Result as DecodeResult
Msg->>Plugin: decode(message)
Note over Plugin: Validate message<br/>termination with "Q"
Plugin->>Plugin: Parse header<br/>Extract Q-prefixed segments
loop For each weather segment
Plugin->>Parser: parseWeatherReport(text)
Parser->>Utils: Extract coordinates<br/>time, altitude
Utils-->>Parser: Parsed values
Parser->>Utils: Decode wind direction<br/>speed, temperature
Utils-->>Parser: Wind values
Parser-->>Plugin: Wind object
Plugin->>Plugin: Collect into windData[]
end
Plugin->>Formatter: windData(decodeResult, windData[])
Formatter->>Result: Assign raw.wind_data
Formatter->>Result: Build & append<br/>formatted items
Formatter-->>Plugin: Updated DecodeResult
Plugin-->>Msg: Return DecodeResult<br/>(decoded, full/partial level)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
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 |
| @@ -0,0 +1,12 @@ | |||
| import { Waypoint } from "./waypoint"; | |||
|
|
|||
| export interface Wind { | |||
There was a problem hiding this comment.
I'm tempted to make this Weather and have an altitude with units (flight level or feet)
There was a problem hiding this comment.
This makes me think further about us needing to find a standard unit (with conversion capability) in everything we do!
There was a problem hiding this comment.
Pull request overview
This PR adds support for parsing Label H1 header 02E20 messages, which are weather reports containing wind and temperature data at various waypoints along a flight route.
Key Changes:
- Introduces a new
Windtype interface to standardize wind data representation - Creates a new
Label_H1_02E20decoder plugin to parse 02E20 weather report messages - Refactors wind data formatting into a centralized
ResultFormatter.windData()method
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/types/wind.ts | Defines new Wind interface for structured wind data representation |
| lib/plugins/Label_H1_02E20.ts | Implements decoder for 02E20 weather report messages |
| lib/plugins/Label_H1_02E20.test.ts | Adds comprehensive test coverage for the new decoder |
| lib/utils/result_formatter.ts | Adds centralized windData() method for formatting wind information |
| lib/utils/h1_helper.ts | Refactored to use new Wind type and centralized formatting |
| lib/plugins/official.ts | Exports new Label_H1_02E20 plugin |
| lib/MessageDecoder.ts | Registers the new Label_H1_02E20 plugin |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let text = `${RouteUtils.waypointToString(wind.waypoint)} at FL${wind.flightLevel}: ${wind.windDirection}° at ${wind.windSpeed}kt`; | ||
| if (wind.temperature) { | ||
| text += `, ${wind.temperature.degreesC}°C at FL${wind.temperature.flightLevel}`; | ||
|
|
There was a problem hiding this comment.
Empty line inside the if block is unnecessary and inconsistent with the rest of the codebase's style.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Bug Report
Comments? Email us. |
Bug Report
Comments? Email us. |
![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
♻️ Duplicate comments (2)
lib/utils/result_formatter.ts (1)
396-411: Fix indentation inconsistencies in the method.The method has inconsistent indentation that differs from the rest of the class. The opening
statickeyword has extra leading spaces, and the closing braces are misaligned.🔎 Proposed fix for indentation
- static windData(decodeResult: DecodeResult, windData: Wind[]) { - decodeResult.raw.wind_data = windData; - for(const wind of windData) { - let text = `${RouteUtils.waypointToString(wind.waypoint)} at FL${wind.flightLevel}: ${wind.windDirection}° at ${wind.windSpeed}kt`; - if (wind.temperature) { - text += `, ${wind.temperature.degreesC}°C at FL${wind.temperature.flightLevel}`; - - } - decodeResult.formatted.items.push({ - type: 'wind_data', - code: 'WIND', - label: 'Wind Data', - value: text, - }); - } - } + static windData(decodeResult: DecodeResult, windData: Wind[]) { + decodeResult.raw.wind_data = windData; + for (const wind of windData) { + let text = `${RouteUtils.waypointToString(wind.waypoint)} at FL${wind.flightLevel}: ${wind.windDirection}° at ${wind.windSpeed}kt`; + if (wind.temperature) { + text += `, ${wind.temperature.degreesC}°C at FL${wind.temperature.flightLevel}`; + } + decodeResult.formatted.items.push({ + type: 'wind_data', + code: 'WIND', + label: 'Wind Data', + value: text, + }); + } + }Also removes the unnecessary empty line at line 402 inside the
ifblock.lib/plugins/Label_H1_02E20.ts (1)
52-52: Add semicolon for consistency.While JavaScript allows omitting semicolons, the project consistently uses them elsewhere.
🔎 Suggested fix
- continue + continue;
🧹 Nitpick comments (3)
lib/types/wind.ts (1)
3-12: Well-structured interface definition.The
Windinterface correctly models the weather data with required and optional fields. Consider adding JSDoc comments to document the units (e.g.,windSpeedin knots,windDirectionin degrees,flightLevelin hundreds of feet) for clarity, similar to the documentation style inWaypointinterface.lib/plugins/Label_H1_02E20.ts (2)
24-32: Consider filtering empty parts from split.The
split(' ')call will create empty strings when there are consecutive spaces in the message (which appears common in the test data). While the current code handles this, filtering empty parts would be more efficient and clearer.🔎 Suggested refactor
- const parts = message.text.split(' '); + const parts = message.text.split(' ').filter(p => p.length > 0);
82-82: Remove or document commented code.The commented line about altitude calculation should either be removed if it's not needed, or uncommented with a clear explanation of why both altitude and flight level are tracked.
🔎 Suggested fix
- // const altitude = parseInt(text.substring(17,21), 10) * 10; // use FL instead
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
lib/MessageDecoder.tslib/plugins/Label_H1_02E20.test.tslib/plugins/Label_H1_02E20.tslib/plugins/official.tslib/types/wind.tslib/utils/h1_helper.tslib/utils/result_formatter.ts
🧰 Additional context used
🧬 Code graph analysis (4)
lib/utils/result_formatter.ts (3)
lib/DecoderPluginInterface.ts (1)
DecodeResult(20-42)lib/types/wind.ts (1)
Wind(3-12)lib/utils/route_utils.ts (1)
RouteUtils(6-86)
lib/types/wind.ts (1)
lib/types/waypoint.ts (1)
Waypoint(12-47)
lib/utils/h1_helper.ts (2)
lib/types/wind.ts (1)
Wind(3-12)lib/utils/result_formatter.ts (1)
ResultFormatter(12-424)
lib/plugins/Label_H1_02E20.ts (5)
lib/DecoderPluginInterface.ts (3)
Message(4-8)Options(13-15)DecodeResult(20-42)lib/utils/result_formatter.ts (3)
windData(396-411)ResultFormatter(12-424)text(386-394)lib/types/wind.ts (1)
Wind(3-12)lib/utils/coordinate_utils.ts (1)
CoordinateUtils(1-76)lib/DateTimeUtils.ts (1)
DateTimeUtils(1-75)
🔇 Additional comments (8)
lib/plugins/official.ts (1)
50-50: LGTM!The new plugin export follows the existing pattern and maintains alphabetical ordering among the H1 plugins.
lib/MessageDecoder.ts (1)
59-59: LGTM!The plugin is correctly registered before the generic
Label_H1plugin, ensuring the more specific H1-02E20 decoder is matched first when applicable.lib/utils/h1_helper.ts (1)
303-341: Good refactor to centralized wind data handling.The refactor from incremental mutations to batch collection with a final
ResultFormatter.windData()call improves code clarity and maintainability. The waypoint object construction correctly satisfies theWaypointinterface requirements, and the temperature parsing with 'M' for negative values is handled correctly.Minor: Consider removing the unnecessary empty lines at lines 305, 330, and 340 for consistency with the surrounding code style.
lib/plugins/Label_H1_02E20.test.ts (1)
1-159: Excellent test coverage!The test suite is comprehensive and well-structured. It covers:
- Plugin metadata validation (name, qualifiers)
- Multiple valid input scenarios with varying data
- Invalid input handling
- Both raw data parsing and formatted output validation
lib/plugins/Label_H1_02E20.ts (4)
1-16: LGTM!The plugin structure and qualifiers are correctly defined.
34-46: LGTM!The header parsing logic correctly extracts the airport codes and first wind report, with proper error handling for invalid data.
64-67: LGTM!The result formatting and decode level logic correctly reflect whether the entire message was parsed.
70-108: Solid parsing logic with clear field extraction.The method correctly parses the fixed-width wind report format with appropriate validation (length check, 'G' marker). The coordinate, time, flight level, temperature, and wind data extraction all follow the expected format.
Bug Report| Name | Severity | Example test case | Description |\n|---|---|---|---|\n| DoS in Label_H1_02E20 | Medium | Craft an ACARS message with Label H1 and preamble 02E20. Include a weather report string like "02E20HEGNLKPRN40XXXE02208116253601M627259020G Q". | The Comments? Email us. |
|
An error occured. This error may be due to rate limits. If this error persists, please email us. |
|
An error occured. This error may be due to rate limits. If this error persists, please email us. |
makrsmark
changed the title
|
An error occured. This error may be due to rate limits. If this error persists, please email us. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
lib/plugins/Label_H1_02E.test.ts (2)
38-69: Consider adding raw Wind object structure validation.While the test thoroughly validates the formatted output strings, it only checks
weather.lengthbut doesn't validate the structure of the underlying Wind objects (e.g.,waypoint.latitude,windSpeed,temperature.degreesC). Consider adding assertions to verify the raw data structure matches expected values.Example: Add raw data validation
expect(weather.length).toBe(6); +// Validate first wind entry raw structure +expect(weather[0].waypoint.latitude).toBeCloseTo(40.598, 3); +expect(weather[0].waypoint.longitude).toBeCloseTo(22.135, 3); +expect(weather[0].flightLevel).toBe(360); +expect(weather[0].windDirection).toBe(259); +expect(weather[0].windSpeed).toBe(20); +expect(weather[0].temperature?.degreesC).toBeCloseTo(-62.7, 1);
155-164: Consider adding more edge case tests.The invalid message test is good, but consider adding tests for additional edge cases:
- A valid message with no wind data entries (just header + 'Q')
- A message with a malformed header but valid trailing 'Q'
- A message with some valid and some invalid wind entries (to test partial decoding)
- Boundary cases for coordinates, temperatures, flight levels
lib/plugins/Label_H1_02E.ts (5)
42-47: Consider extracting duplicate remaining text logic.Lines 46, 52, and 59 contain duplicate logic for appending to
decodeResult.remaining.text. This pattern could be extracted into a helper method to improve maintainability.Example helper method
+private appendToRemaining(decodeResult: DecodeResult, text: string): void { + decodeResult.remaining.text += (decodeResult.remaining.text ? ' ' : '') + text; +} const firstWind = this.parseWeatherReport(header.substring(13)); if(firstWind) { windData.push(firstWind); } else { - decodeResult.remaining.text += (decodeResult.remaining.text ? ' ' : '') + header.substring(13); + this.appendToRemaining(decodeResult, header.substring(13)); }Also applies to: 49-61
63-64: Remove extra blank lines.Lines 63-64 contain unnecessary blank lines that should be removed for consistency with the codebase style.
Proposed fix
} } - - - ResultFormatter.windData(decodeResult, windData);
71-71: Fix inconsistent spacing.Line 71 has inconsistent spacing before
private. Remove the leading space for consistency.Proposed fix
- private parseWeatherReport(text: string): Wind | null { +private parseWeatherReport(text: string): Wind | null {
79-79: Remove commented-out code.Line 79 contains commented-out altitude calculation code. If this code is no longer needed, remove it. If it's useful for reference, consider moving it to documentation or commit history.
Proposed fix
const flightLevel = parseInt(text.substring(17,20), 10); -// const altitude = parseInt(text.substring(17,21), 10) * 10; // use FL instead const tempSign = text[21] === 'M' ? -1 : 1;
85-87: Improve validation comment clarity.The comment
// G?on line 85 is not very informative. Consider clarifying what the 'G' character represents in the protocol.Proposed improvement
-// G? +// Verify trailing 'G' flag (protocol terminator for wind entry) if(text[31] !== 'G') { return null; }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/MessageDecoder.tslib/plugins/Label_H1_02E.test.tslib/plugins/Label_H1_02E.tslib/plugins/official.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/plugins/official.ts
- lib/MessageDecoder.ts
🧰 Additional context used
🧬 Code graph analysis (2)
lib/plugins/Label_H1_02E.test.ts (3)
lib/plugins/Label_H1_02E.ts (1)
Label_H1_02E(8-106)lib/MessageDecoder.ts (1)
MessageDecoder(6-176)lib/utils/result_formatter.ts (1)
text(386-394)
lib/plugins/Label_H1_02E.ts (5)
lib/DecoderPluginInterface.ts (3)
Message(4-8)Options(13-15)DecodeResult(20-42)lib/utils/result_formatter.ts (2)
windData(396-411)text(386-394)lib/types/wind.ts (1)
Wind(3-12)lib/utils/coordinate_utils.ts (1)
CoordinateUtils(1-76)lib/DateTimeUtils.ts (1)
DateTimeUtils(1-75)
🔇 Additional comments (5)
lib/plugins/Label_H1_02E.test.ts (3)
1-10: LGTM! Test setup is correct.The test setup properly instantiates MessageDecoder and the plugin before each test.
11-19: LGTM! Metadata validation is thorough.The test properly validates the plugin's name and qualifiers.
72-153: LGTM with same suggestion as example 1.These test cases follow the same thorough pattern as example 1. The same suggestion about adding raw Wind object validation applies here as well.
lib/plugins/Label_H1_02E.ts (2)
1-16: LGTM! Class structure and qualifiers are correct.The plugin properly extends DecoderPlugin and defines appropriate qualifiers for H1 label with 02E preamble.
107-107: No action needed—this export pattern is standard across the plugin codebase.The
export default {}pattern at line 107 is consistent with the established convention used across all plugin files in thelib/plugins/directory (30+ files follow this exact pattern). This appears to be an intentional part of the plugin architecture rather than an unnecessary export.Likely an incorrect or invalid review comment.
lib/plugins/Label_H2_02E.ts
Outdated
| const header = parts[0]; | ||
| // header.substring(0,3) is '02E' | ||
| ResultFormatter.day(decodeResult, parseInt(header.substring(3,5))); | ||
| ResultFormatter.departureAirport(decodeResult, header.substring(5,9)); | ||
| ResultFormatter.arrivalAirport(decodeResult, header.substring(9,13)); |
There was a problem hiding this comment.
Add header length validation before substring operations.
The code performs substring operations on header without first validating its length. If the header is shorter than 13 characters (the minimum required), these operations could silently produce incorrect results or unexpected behavior.
Suggested fix
const header = parts[0];
+if (header.length < 13) {
+ decodeResult.remaining.text = message.text;
+ decodeResult.decoded = false;
+ decodeResult.decoder.decodeLevel = 'none';
+ return decodeResult;
+}
// header.substring(0,3) is '02E'
ResultFormatter.day(decodeResult, parseInt(header.substring(3,5), 10));📝 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 header = parts[0]; | |
| // header.substring(0,3) is '02E' | |
| ResultFormatter.day(decodeResult, parseInt(header.substring(3,5))); | |
| ResultFormatter.departureAirport(decodeResult, header.substring(5,9)); | |
| ResultFormatter.arrivalAirport(decodeResult, header.substring(9,13)); | |
| const header = parts[0]; | |
| if (header.length < 13) { | |
| decodeResult.remaining.text = message.text; | |
| decodeResult.decoded = false; | |
| decodeResult.decoder.decodeLevel = 'none'; | |
| return decodeResult; | |
| } | |
| // header.substring(0,3) is '02E' | |
| ResultFormatter.day(decodeResult, parseInt(header.substring(3,5), 10)); | |
| ResultFormatter.departureAirport(decodeResult, header.substring(5,9)); | |
| ResultFormatter.arrivalAirport(decodeResult, header.substring(9,13)); |
🤖 Prompt for AI Agents
In lib/plugins/Label_H1_02E.ts around lines 37 to 41, the code calls
substring(3,5), substring(5,9) and substring(9,13) on header without checking
its length; add a guard that verifies header.length >= 13 before performing
these substrings and handle the failure path (throw a descriptive Error or log
and return/skip processing) so you never call substring on a too-short string;
then only call ResultFormatter.day/departureAirport/arrivalAirport when the
header is valid.
Bug Report
Comments? Email us. |
makrsmark
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
lib/plugins/Label_H2_02E.ts (2)
37-41: Add header length validation before substring operations.The header length is not validated before performing substring operations. If
headeris shorter than 13 characters, these operations will produce incorrect or empty results.This issue was raised in a previous review and remains unresolved.
Suggested fix
const header = parts[0]; +if (header.length < 13) { + decodeResult.remaining.text = message.text; + decodeResult.decoded = false; + decodeResult.decoder.decodeLevel = 'none'; + return decodeResult; +} // header.substring(0,3) is '02E'
39-39: Specify radix in parseInt to avoid potential octal interpretation.
parseInt(header.substring(3,5))lacks an explicit radix. For day values with leading zeros (e.g., "01", "09"), this could cause issues in older environments.This issue was raised in a previous review and remains unresolved.
Suggested fix
-ResultFormatter.day(decodeResult, parseInt(header.substring(3,5))); +ResultFormatter.day(decodeResult, parseInt(header.substring(3,5), 10));
🧹 Nitpick comments (2)
lib/plugins/Label_H2_02E.ts (2)
71-105: Fix inconsistent indentation in parseWeatherReport method.The method body uses 2-space indentation while the class uses 2-space indentation, but the method content appears to use 1 or 2 spaces inconsistently relative to the method declaration.
Suggested fix
private parseWeatherReport(text: string): Wind | null { - const posString = text.substring(0,13); - const pos = CoordinateUtils.decodeStringCoordinatesDecimalMinutes(posString); - if (text.length !== 32 || !pos) { - return null; - } + const posString = text.substring(0,13); + const pos = CoordinateUtils.decodeStringCoordinatesDecimalMinutes(posString); + if (text.length !== 32 || !pos) { + return null; + } // ... rest of method with consistent 4-space indentation
107-107: Remove or replace the empty default export.
export default {}exports an empty object, which provides no value. Consider either removing this line or exporting the plugin class as default if that's the intended pattern.Suggested fix
-export default {};Or if a default export is needed:
-export default {}; +export default Label_H2_02E;
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/MessageDecoder.tslib/plugins/Label_H2_02E.test.tslib/plugins/Label_H2_02E.tslib/plugins/official.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/plugins/official.ts
- lib/MessageDecoder.ts
🧰 Additional context used
🧬 Code graph analysis (1)
lib/plugins/Label_H2_02E.test.ts (3)
lib/plugins/Label_H2_02E.ts (1)
Label_H2_02E(8-106)lib/MessageDecoder.ts (1)
MessageDecoder(6-176)lib/utils/result_formatter.ts (1)
text(386-394)
🔇 Additional comments (2)
lib/plugins/Label_H2_02E.test.ts (1)
1-165: Test coverage looks comprehensive.The test suite provides good coverage with:
- Plugin metadata validation (name, qualifiers)
- Three positive decoding scenarios with different message formats
- One invalid message case to verify error handling
Minor note: The describe block name
"Label_H2 02E"uses a space while the class name uses an underscore (Label_H2_02E). This is cosmetic but worth keeping consistent.lib/plugins/Label_H2_02E.ts (1)
77-77: The functionconvertHHMMSSToTodis explicitly designed to handle both 4-character (HHMM) and 6-character (HHMMSS) input formats. The implementation at lines 33-35 checks the input length and automatically appends'00'for seconds when a 4-character string is provided. The JSDoc comment at line 29 correctly documents this:@param time HHMMSS or HHMM. The code at line 77 is correct and requires no changes.
Bug Report
Comments? Email us. |
Bug Report
Comments? Email us. |
kevinelliott
left a comment
kevinelliott
left a comment
There was a problem hiding this comment.
LGTM. Let's merge away!
| @@ -0,0 +1,12 @@ | |||
| import { Waypoint } from "./waypoint"; | |||
|
|
|||
| export interface Wind { | |||
There was a problem hiding this comment.
This makes me think further about us needing to find a standard unit (with conversion capability) in everything we do!
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.