Update Label 16 POSA1 — fix altitude field, surface track/ETA/OAT#419
Update Label 16 POSA1 — fix altitude field, surface track/ETA/OAT#419thepacket wants to merge 1 commit intoairframesio:masterfrom
Conversation
Extended to handle additional sub-formats and/or bug fixes.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 55 minutes and 12 seconds. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ 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 |
kevinelliott
left a comment
There was a problem hiding this comment.
Summary
Reinterprets Label_16_POSA1 field 3 as ground track (degrees) instead of altitude × 100, and surfaces the report timestamp, ETA at next waypoint, and OAT as their own labelled rows instead of dumping them into unknownArr. Decode level is bumped from partial to full. Excellent ASCII field-map docblock — very helpful for future maintainers.
Verdict
Comment-only review. Direction is good, the field reinterpretation is plausible, and the new structured output is a net improvement. However, this PR cannot merge as-is — it breaks two of the existing four unit tests because the test file is unchanged.
Must Fix
-
Existing tests fail.
Label_16_POSA1.test.tsis not updated. Bothdecodes variant 1anddecodes redactedfail:- assert
decodeLevel === 'partial'— now'full' - assert
formatted.items.length === 3— now ~6 - assert
items[1].label === 'Altitude'andvalue === '37000 feet'— Altitude row is removed - assert
remaining.text === ',-61,139,1174,829'and',*****,*****, 744, 0'—unknownArris no longer called for fields 6+
Please update both fixtures to assert the new structure (Track, Timestamp, ETA, OAT rows) and the new
decodeLevel: 'full'. Without this, CI will go red. - assert
-
Verify field 3 really is ground track, not altitude. The PR body says "Per analyst review of documented examples (
POSA1N52969E 16393,RANOK ,080055,330,KORUP ,083244,...), this field is ground track in degrees; altitude is not present." The existing fixture is370and the test labelled it37000 feet, which would be plausible for an airliner but the value349(the example you cite fromRANOK) is an extremely on-the-nose track value. I am inclined to trust your analyst review, but please add a brief reference to the document/source in a comment so future maintainers can verify. If altitude is genuinely absent from POSA1, that is a meaningful claim worth a citation.
Should Fix
- The redacted fixture has field 7 =
*****(5 stars), which fails your/^-?\d{1,3}$/test for OAT — fine, it just gets skipped. But field 3 in the redacted fixture is390, which still parses as track. That is probably wrong (390°is invalid for a track 0–359). Consider tightening the track regex to/^(0|[1-9]\d?|[12]\d{2}|3[0-5]\d)$/or justNumber(x) >= 0 && Number(x) <= 359. As-is the plugin will happily emit390°from370(the original variant 1 fixture).
Nits
- The "wild guess" trailing-fields comment is great, but consider a
// TODO(label-16-posa1): trailing fields 8–10 are undocumentedto make it greppable.
Backward Compat / Regression Risk
- Breaking output change: consumers reading
decodeResult.raw.altitudefrom this plugin will see it disappear. If altitude was actually correct (rare), this is a real regression. If altitude was always wrong (your claim), this is the right fix. Either way, the change is breaking and should be noted in the changelog. - The
unknownArrbody is now empty, sodecodeResult.remaining.textwill be''instead of the trailing fields. Consumers parsingremaining.textfor fields 6–10 will see them disappear. Reasonable, given the analyst guidance to ignore wild guesses. - Decode level
partial → fullis an upgrade — fine if all knowable fields are now surfaced.
Test Coverage
Currently strictly narrower because tests aren't updated. Once tests are rewritten:
- New rows (Timestamp, Track, ETA, OAT) need explicit assertions.
- A negative-OAT case is already covered by
-61in variant 1 and-53from your example — please assert the formatted OAT value in the test. - A track-out-of-range fixture (e.g.
390,400) would be a useful guard rail given the regex is permissive.
Outstanding Existing Comments
None. Only an auto-generated CodeRabbit rate-limit notice on the PR.
Thanks @thepacket! The structured output is a clear improvement and the field-map docblock is genuinely useful — this just needs the test file updated and ideally a citation for the altitude claim before merge.
Corrects the field-3 interpretation in
Label_16_POSA1:349) as altitude × 100 = 34,900 ftPOSA1N52969E 16393,RANOK ,080055,330,KORUP ,083244,,-53, 54, 93,828), this field is ground track in degrees; altitude is not presentAlso surfaces OAT, report timestamp, and ETA as their own rows instead of burying them inside
unknownArr. Wild-guess fields at positions 8–10 remain ignored.Decode level bumped from
partialtofull.npm run buildpasses.