Conversation
|
Warning Rate limit exceeded
⌛ 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 (4)
WalkthroughThe PR adds a new Label_H1_Paren plugin extending DecoderPlugin to decode aviation messages starting with '('. The plugin parses flight numbers, geographic coordinates, timestamps, altitude, fuel, and Mach information, accompanied by comprehensive unit tests validating metadata, successful decoding, and error scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
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 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: 2
🧹 Nitpick comments (2)
lib/plugins/Label_H1_Paren.ts (2)
5-5: Remove stray comment.The comment
// ...existing code...appears to be a template artifact and should be removed.🧹 Proposed fix
import { ResultFormatter } from '../utils/result_formatter'; -// ...existing code... export class Label_H1_Paren extends DecoderPlugin {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/plugins/Label_H1_Paren.ts` at line 5, Remove the stray template comment line "// ...existing code..." from the top-level of lib/plugins/Label_H1_Paren.ts; open the file, locate that exact comment (it may be at module scope or above the exported symbol such as the Label_H1_Paren plugin/class/function) and delete it so no leftover template artifacts remain, then run a quick lint/format to ensure no trailing blank lines or eslint warnings remain.
29-30: Regex hardcodes 'KLM' airline code.The regex only matches messages with
KLM\d+flight numbers. If this plugin is intended to handle messages from other airlines using the same format, consider generalizing the flight number pattern.If this is intentional (KLM-specific format), consider adding a comment to clarify the scope.
♻️ Suggested generalization (if applicable)
const regex = - /^\(POS-(?<flight>KLM\d+)\s+(?<lat>-?\d{4,5}[NS])(?<lon>\d{5}[EW])\/(?<timestamp>\d{6})\s+F(?<alt>\d{3})\r?\nRMK\/FUEL\s+(?<fuel>\d{2,3}\.\d)\s+M(?<mach>\d\.\d{2})\)/; + /^\(POS-(?<flight>[A-Z]{2,3}\d+)\s+(?<lat>-?\d{4,5}[NS])(?<lon>\d{5}[EW])\/(?<timestamp>\d{6})\s+F(?<alt>\d{3})\r?\nRMK\/FUEL\s+(?<fuel>\d{2,3}\.\d)\s+M(?<mach>\d\.\d{2})\)/;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/plugins/Label_H1_Paren.ts` around lines 29 - 30, The regex in Label_H1_Paren.ts (const regex) hardcodes the airline prefix as KLM in the named capture "flight", so update the pattern to accept other airline codes (for example replace "KLM\d+" with a more general airline prefix like "[A-Z]{2,3}\\d+" or another appropriate pattern) or make the flight-prefix configurable; if KLM-only behavior is intentional, add a clarifying comment above const regex stating the plugin is KLM-specific. Ensure the named capture remains "flight" so downstream code that uses that group (flight) keeps working.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/plugins/Label_H1_Paren.ts`:
- Around line 58-66: The regex in parseLat currently captures an optional '-' in
group 1 but ignores it; update parseLat so the captured prefix is applied to the
final sign: keep the existing regex (/(-?)(\d{2})(\d{2})([NS])/) and compute
sign by checking match[1] (if '-' then sign = -1) otherwise fall back to the N/S
suffix (match[4] === 'S' ? -1 : 1), then return sign * (deg + min / 60); this
preserves any explicit negative prefix while still honoring the N/S hemisphere
indicator.
- Around line 7-14: The Label_H1_Paren plugin class (Label_H1_Paren) is
implemented but not exported or registered, so add an export entry for it in the
plugins barrel (export * from './Label_H1_Paren'; in lib/plugins/official.ts)
and then register an instance in the message decoder initialization (call
this.registerPlugin(new Plugins.Label_H1_Paren(this)); alongside other H1 plugin
registrations in lib/MessageDecoder.ts) so the plugin is available at runtime.
---
Nitpick comments:
In `@lib/plugins/Label_H1_Paren.ts`:
- Line 5: Remove the stray template comment line "// ...existing code..." from
the top-level of lib/plugins/Label_H1_Paren.ts; open the file, locate that exact
comment (it may be at module scope or above the exported symbol such as the
Label_H1_Paren plugin/class/function) and delete it so no leftover template
artifacts remain, then run a quick lint/format to ensure no trailing blank lines
or eslint warnings remain.
- Around line 29-30: The regex in Label_H1_Paren.ts (const regex) hardcodes the
airline prefix as KLM in the named capture "flight", so update the pattern to
accept other airline codes (for example replace "KLM\d+" with a more general
airline prefix like "[A-Z]{2,3}\\d+" or another appropriate pattern) or make the
flight-prefix configurable; if KLM-only behavior is intentional, add a
clarifying comment above const regex stating the plugin is KLM-specific. Ensure
the named capture remains "flight" so downstream code that uses that group
(flight) keeps working.
| function parseLat(latStr: string): number { | ||
| // Example: -3911N | ||
| const match = latStr.match(/(-?)(\d{2})(\d{2})([NS])/); | ||
| if (!match) return NaN; | ||
| let deg = parseInt(match[2]); | ||
| let min = parseInt(match[3]); | ||
| let sign = match[4] === 'S' ? -1 : 1; | ||
| return sign * (deg + min / 60); | ||
| } |
There was a problem hiding this comment.
Captured - prefix is unused in latitude parsing.
The regex captures an optional - in group 1, but this value is never used in the calculation. The sign is determined solely by the N/S suffix. Either:
- Remove the
-?from the regex if it's not meaningful, or - Use the captured value if it carries semantic meaning
This could cause silent data loss if the prefix is significant.
🔧 Option 1: Remove unused capture if prefix is not meaningful
function parseLat(latStr: string): number {
// Example: -3911N
- const match = latStr.match(/(-?)(\d{2})(\d{2})([NS])/);
+ const match = latStr.match(/(\d{2})(\d{2})([NS])/);
if (!match) return NaN;
- let deg = parseInt(match[2]);
- let min = parseInt(match[3]);
- let sign = match[4] === 'S' ? -1 : 1;
+ let deg = parseInt(match[1]);
+ let min = parseInt(match[2]);
+ let sign = match[3] === 'S' ? -1 : 1;
return sign * (deg + min / 60);
}📝 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.
| function parseLat(latStr: string): number { | |
| // Example: -3911N | |
| const match = latStr.match(/(-?)(\d{2})(\d{2})([NS])/); | |
| if (!match) return NaN; | |
| let deg = parseInt(match[2]); | |
| let min = parseInt(match[3]); | |
| let sign = match[4] === 'S' ? -1 : 1; | |
| return sign * (deg + min / 60); | |
| } | |
| function parseLat(latStr: string): number { | |
| // Example: -3911N | |
| const match = latStr.match(/(\d{2})(\d{2})([NS])/); | |
| if (!match) return NaN; | |
| let deg = parseInt(match[1]); | |
| let min = parseInt(match[2]); | |
| let sign = match[3] === 'S' ? -1 : 1; | |
| return sign * (deg + min / 60); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/plugins/Label_H1_Paren.ts` around lines 58 - 66, The regex in parseLat
currently captures an optional '-' in group 1 but ignores it; update parseLat so
the captured prefix is applied to the final sign: keep the existing regex
(/(-?)(\d{2})(\d{2})([NS])/) and compute sign by checking match[1] (if '-' then
sign = -1) otherwise fall back to the N/S suffix (match[4] === 'S' ? -1 : 1),
then return sign * (deg + min / 60); this preserves any explicit negative prefix
while still honoring the N/S hemisphere indicator.
There was a problem hiding this comment.
Pull request overview
This PR introduces a new decoder plugin intended to parse a specific parenthesized H1 position/fuel report format and adds Jest coverage for the new plugin.
Changes:
- Added
Label_H1_Parendecoder plugin to parse(POS-...)style H1 messages into structured fields (flight, position, TOD, altitude, fuel, mach). - Added unit tests validating qualifiers and decoding behavior for a representative sample message.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
lib/plugins/Label_H1_Paren.ts |
New H1 plugin implementation for parenthesized (POS-...) message parsing and result formatting. |
lib/plugins/Label_H1_Paren.test.ts |
New Jest tests covering qualifiers and decode behavior for the new plugin. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| expect(decodeResult.raw.mach).toBe(0.69); | ||
| expect(decodeResult.formatted.description).toBe('Position Report'); | ||
| expect(decodeResult.formatted.items.length).toBe(6); | ||
| expect(decodeResult.remaining.text).toBe('RMK'); |
There was a problem hiding this comment.
The tests only call plugin.decode() directly, so they won't catch whether the plugin is wired into MessageDecoder registration. Add an integration assertion using new MessageDecoder().decode(message) that verifies this message is decoded by label-h1-paren once the plugin is exported/registered; this prevents regressions where the plugin exists but is unused.
| expect(decodeResult.remaining.text).toBe('RMK'); | |
| expect(decodeResult.remaining.text).toBe('RMK'); | |
| // Integration check: ensure MessageDecoder uses the registered label-h1-paren plugin. | |
| const integrationDecodeResult = new MessageDecoder().decode(message); | |
| expect(integrationDecodeResult.decoded).toBe(true); | |
| expect(integrationDecodeResult.raw.flight_number).toBe('KLM296'); |
lib/plugins/Label_H1_Paren.ts
Outdated
| qualifiers() { | ||
| return { | ||
| labels: ['H1'], | ||
| prefixes: ['('], |
There was a problem hiding this comment.
qualifiers() returns { labels, prefixes }, but MessageDecoder only filters on qualifiers.preambles (see lib/MessageDecoder.ts:99-106). As written, the prefixes field is ignored, so this plugin will be considered for all H1 messages once registered. Rename prefixes to preambles (and keep the value '(' or the more specific '(POS-').
| prefixes: ['('], | |
| preambles: ['(POS-'], |
There was a problem hiding this comment.
leaving open for now
Summary by CodeRabbit
New Features
Tests