Refactor MessageDecoder with label indexing and plugin helper methods#366
Merged
kevinelliott merged 6 commits intomasterfrom Mar 19, 2026
Merged
Conversation
Analyzes the codebase and documents opportunities for improving code reuse, performance, type safety, and maintainability across the 68 decoder plugins. https://claude.ai/code/session_01WHPebkdmNV4aEh8wzpzoj5
…raw fields - Fix bug: MessageDecoder now passes options through to plugin.decode() - Add base class helpers: initResult(), setDecodeLevel(), debug(), failUnknown() - Add label index (Map<label, plugin[]>) for O(k) plugin matching instead of O(n) - Replace manual registerPlugin() calls with declarative pluginClasses array - Add RawFields typed interface for DecodeResult.raw with index signature escape hatch - Create Label_44_Base shared class for ON/OFF/IN event decoders - Update 22 plugins to use new base class helpers, reducing boilerplate - Add 30 new tests covering all new infrastructure Net result: -103 lines, improved type safety, faster decode, less duplication. https://claude.ai/code/session_01WHPebkdmNV4aEh8wzpzoj5
kevinelliott
requested review from
Copilot and
makrsmark
and removed request for
Copilot
WalkthroughAdds DecoderPlugin helper APIs and an options-aware decode signature; types DecodeResult.raw as RawFields; introduces Label_44_Base and refactors Label_44 subclasses; changes MessageDecoder to register/index plugins by label and wildcard and forward options; updates ~20 plugins to use the new helpers and adds related tests. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors the decoding architecture to improve plugin lookup performance and reduce per-plugin boilerplate by introducing label indexing in MessageDecoder and shared helper/base classes for decoders.
Changes:
- Added label-based plugin indexing (plus wildcard handling and preamble filtering) to speed up plugin selection in
MessageDecoder. - Introduced
DecoderPluginhelper methods (initResult,setDecodeLevel,debug,failUnknown) and migrated multiple plugins to use them. - Added a new
Label_44_Baseabstract class to share common Label 44 event parsing logic, and added/updated tests plus a typedRawFieldsinterface.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/plugins/Label_SQ.ts | Migrates to initResult/setDecodeLevel helpers. |
| lib/plugins/Label_H1_WRN.ts | Uses shared helpers; unknown handling now via failUnknown. |
| lib/plugins/Label_H1_M_POS.ts | Uses shared helpers; unknown handling now via failUnknown. |
| lib/plugins/Label_H1_FLR.ts | Uses shared helpers; unknown handling now via failUnknown. |
| lib/plugins/Label_80.ts | Uses initResult and infers decode level via setDecodeLevel. |
| lib/plugins/Label_4A_DOOR.ts | Replaces manual decode-level logic with setDecodeLevel. |
| lib/plugins/Label_4A_DIS.ts | Replaces manual decode-level logic with setDecodeLevel. |
| lib/plugins/Label_4A_01.ts | Replaces manual decode-level logic with setDecodeLevel. |
| lib/plugins/Label_4A.ts | Replaces manual decode-level logic with setDecodeLevel. |
| lib/plugins/Label_44_POS.ts | Uses initResult and debug; minor field naming/comment clarification. |
| lib/plugins/Label_44_ON.ts | Refactored to extend Label_44_Base and implement event-specific hook. |
| lib/plugins/Label_44_OFF.ts | Refactored to extend Label_44_Base and implement event-specific hook. |
| lib/plugins/Label_44_IN.ts | Refactored to extend Label_44_Base and implement event-specific hook. |
| lib/plugins/Label_44_ETA.ts | Uses shared helpers while remaining standalone (different layout). |
| lib/plugins/Label_44_Base.ts | New shared base class for Label 44 event-style decoders. |
| lib/plugins/Label_44_Base.test.ts | New tests validating shared Label 44 base parsing and helper behavior. |
| lib/plugins/Label_22_POS.ts | Uses initResult, debug, and setDecodeLevel. |
| lib/plugins/Label_22_OFF.ts | Uses initResult, debug, and setDecodeLevel. |
| lib/plugins/Label_21_POS.ts | Uses initResult, debug, and setDecodeLevel. |
| lib/plugins/Label_20_POS.ts | Uses initResult, debug, and setDecodeLevel. |
| lib/plugins/Label_16_POSA1.ts | Uses initResult, setDecodeLevel, and failUnknown. |
| lib/plugins/Label_15_FST.ts | Uses initResult and setDecodeLevel. |
| lib/plugins/Label_15.ts | Uses initResult, setDecodeLevel, and failUnknown. |
| lib/plugins/Label_12_POS.ts | Uses initResult, setDecodeLevel, and failUnknown. |
| lib/plugins/Label_10_POS.ts | Uses initResult, setDecodeLevel, and failUnknown. |
| lib/RawFields.test.ts | Adds tests demonstrating RawFields usage within DecodeResult. |
| lib/MessageDecoder.ts | Adds plugin class list + label index and passes options through to plugins. |
| lib/MessageDecoder.labelindex.test.ts | Adds tests for label indexing, wildcard ordering, and preamble filtering. |
| lib/DecoderPluginInterface.ts | Introduces RawFields and updates plugin decode signature to accept options. |
| lib/DecoderPlugin.ts | Adds protected helper methods to reduce plugin boilerplate. |
| lib/DecoderPlugin.test.ts | Adds unit tests for new DecoderPlugin helper methods. |
| lib/DateTimeUtils.ts | Removes an extra blank line (formatting-only change). |
Comments suppressed due to low confidence (1)
lib/plugins/Label_SQ.ts:18
DecodeResult.raw.versionis typed asnumberinRawFields, but this plugin assigns a single-character string (e.g.'2'). This will fail TypeScript type-checking and also conflicts with other decoders/formatters that treatraw.versionas a numeric version (e.g.ResultFormatter.version()usestoFixed). Consider storing the squitter version in a different field (e.g.squitter_version) or converting to a number and updating related formatting/tests accordingly.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lib/plugins/Label_44_IN.ts
Outdated
| export class Label_44_IN extends Label_44_Base { | ||
| name = 'label-44-in'; | ||
| get description() { | ||
| return 'In Air Report'; |
lib/plugins/Label_44_Base.ts
Outdated
Comment on lines
+7
to
+13
| * Base class for Label 44 event-style decoders (ON, OFF, IN, ETA). | ||
| * | ||
| * These all share the same CSV structure: | ||
| * preamble, coordinates, [field(s)...], departure, arrival, date, time(s), fuel | ||
| * | ||
| * Subclasses provide the description, qualifiers, minimum field count, | ||
| * and a hook to decode their specific fields from the CSV data. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b14c35a82
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
lib/MessageDecoder.ts
Outdated
Comment on lines
+126
to
+127
| const labelPlugins = this.labelIndex.get(message.label) ?? []; | ||
| const candidates = [...this.wildcardPlugins, ...labelPlugins]; |
There was a problem hiding this comment.
Combining wildcardPlugins and labelPlugins without deduplication means a plugin registered with mixed labels like ['*', '44'] will be invoked twice for label 44 (once from each bucket), which can duplicate side effects and distort precedence. This is a regression from the previous linear filter, where each plugin was evaluated at most once per message. Please ensure candidates are unique before iterating.
Useful? React with 👍 / 👎.
makrsmark
approved these changes
Mar 19, 2026
Collaborator
makrsmark
left a comment
makrsmark
left a comment
There was a problem hiding this comment.
I'll trust this. Check out the copilot comments. Approving to not block
- Fix Label_44_IN description from 'In Air Report' to 'In Gate Report' to match the OOOI convention (comment was correct, code was wrong) - Remove ETA from Label_44_Base doc comment since Label_44_ETA doesn't extend this base class - Change RawFields.version type to number | string since Label_SQ uses it as a string identifier - Deduplicate candidate plugins in MessageDecoder.decode() to prevent double execution if a plugin registers both '*' and a specific label https://claude.ai/code/session_01WHPebkdmNV4aEh8wzpzoj5
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
lib/plugins/Label_15_FST.ts (1)
46-47: PreferfailUnknownfor invalid coordinate format branch.This branch sets decode level to
nonebut drops the invalid payload from standardized unknown handling (remaining.text), which reduces debuggability consistency versus other plugins.♻️ Suggested change
- this.setDecodeLevel(decodeResult, false); - return decodeResult; + return this.failUnknown(decodeResult, message.text, options);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/plugins/Label_15_FST.ts` around lines 46 - 47, The invalid-coordinate branch currently calls this.setDecodeLevel(decodeResult, false) and returns decodeResult without invoking the standardized unknown handling, so replace that behavior by calling this.failUnknown(...) instead of only setDecodeLevel; specifically, call this.failUnknown(decodeResult, remaining.text) (or pass remaining.text into failUnknown's payload/message) to mark the decode as unknown and preserve remaining.text for debugging, then return the result from failUnknown rather than returning decodeResult after setDecodeLevel.
🤖 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/MessageDecoder.labelindex.test.ts`:
- Around line 133-151: The test's assertion is too indirect—update the test to
assert plugin-level options propagation explicitly by injecting or mocking a
plugin that emits a distinct debug-only log and then calling
MessageDecoder.decode with { debug: true }; specifically, create or stub a
deterministic plugin (or spy on an existing plugin method) that checks the
received options and logs a unique string only when options.debug is true, call
MessageDecoder.decode (the instance method) with that input and options, and
assert the spy was called with the plugin's unique debug message rather than
relying on decoder-level logs.
In `@lib/plugins/Label_44_IN.ts`:
- Around line 6-11: The file's class Label_44_IN has a comment "In Gate Report"
but its description getter returns "In Air Report", causing a mismatch; decide
which is correct and make them consistent by updating either the comment or the
description getter in the Label_44_IN class (and any related preamble handling
like IN01/IN02/IN03 if present) so that the comment, the name/description
getter, and any logic referencing Label_44_IN all reflect the chosen phrase ("In
Gate Report" or "In Air Report") consistently.
In `@lib/plugins/Label_44_POS.ts`:
- Around line 74-76: setDecodeLevel is being called unconditionally which marks
decodeResult as successful even when the regex failed; move the
setDecodeLevel(decodeResult, true, 'full') call inside the existing if
(results?.groups) block (where results and groups are processed) so it's only
invoked on a successful match, and ensure the else path leaves
decodeResult.decoded false (or explicitly calls setDecodeLevel(decodeResult,
false) if your codebase uses that) to avoid reporting false positives; update
references around setDecodeLevel, decodeResult, and results?.groups accordingly.
---
Nitpick comments:
In `@lib/plugins/Label_15_FST.ts`:
- Around line 46-47: The invalid-coordinate branch currently calls
this.setDecodeLevel(decodeResult, false) and returns decodeResult without
invoking the standardized unknown handling, so replace that behavior by calling
this.failUnknown(...) instead of only setDecodeLevel; specifically, call
this.failUnknown(decodeResult, remaining.text) (or pass remaining.text into
failUnknown's payload/message) to mark the decode as unknown and preserve
remaining.text for debugging, then return the result from failUnknown rather
than returning decodeResult after setDecodeLevel.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3eea3325-f2b4-48d5-8aea-1a30ce8e14b8
📒 Files selected for processing (32)
lib/DateTimeUtils.tslib/DecoderPlugin.test.tslib/DecoderPlugin.tslib/DecoderPluginInterface.tslib/MessageDecoder.labelindex.test.tslib/MessageDecoder.tslib/RawFields.test.tslib/plugins/Label_10_POS.tslib/plugins/Label_12_POS.tslib/plugins/Label_15.tslib/plugins/Label_15_FST.tslib/plugins/Label_16_POSA1.tslib/plugins/Label_20_POS.tslib/plugins/Label_21_POS.tslib/plugins/Label_22_OFF.tslib/plugins/Label_22_POS.tslib/plugins/Label_44_Base.test.tslib/plugins/Label_44_Base.tslib/plugins/Label_44_ETA.tslib/plugins/Label_44_IN.tslib/plugins/Label_44_OFF.tslib/plugins/Label_44_ON.tslib/plugins/Label_44_POS.tslib/plugins/Label_4A.tslib/plugins/Label_4A_01.tslib/plugins/Label_4A_DIS.tslib/plugins/Label_4A_DOOR.tslib/plugins/Label_80.tslib/plugins/Label_H1_FLR.tslib/plugins/Label_H1_M_POS.tslib/plugins/Label_H1_WRN.tslib/plugins/Label_SQ.ts
💤 Files with no reviewable changes (1)
- lib/DateTimeUtils.ts
makrsmark
requested changes
Mar 19, 2026
lib/DecoderPluginInterface.ts
Outdated
| label?: string; | ||
| sublabel?: string; | ||
| preamble?: string; | ||
| version?: number | string; |
Collaborator
There was a problem hiding this comment.
I would rather it always be a number unless it can't, then just make it a string
- Move setDecodeLevel() inside the regex match block in Label_44_POS so failed parses are not incorrectly marked as successful. Add failUnknown() fallback for non-matching messages. - Replace indirect console.log spy test with a probe plugin that verifies options are passed directly to the plugin's decode method. https://claude.ai/code/session_01WHPebkdmNV4aEh8wzpzoj5
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/MessageDecoder.ts`:
- Around line 85-91: The public mutable array field plugins is misleading
because decode() uses labelIndex and wildcardPlugins, so external mutations to
plugins won't affect dispatch; make plugins private (or at least a read-only
getter that returns a copy) and funnel all changes through
registerPlugin()/unregisterPlugin() (or add registerPlugin/removePlugin methods)
which must update labelIndex and wildcardPlugins consistently whenever a plugin
is added/removed; alternatively, if you must keep the public field, wrap it with
setters that synchronize updates into labelIndex and wildcardPlugins so decode()
stays correct (refer to symbols: plugins, labelIndex, wildcardPlugins,
registerPlugin, decode).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 900e86f3-b0f2-47d6-a793-f5ad66894766
📒 Files selected for processing (4)
lib/DecoderPluginInterface.tslib/MessageDecoder.tslib/plugins/Label_44_Base.tslib/plugins/Label_44_IN.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/plugins/Label_44_Base.ts
Comment on lines
85
to
+91
| plugins: Array<DecoderPluginInterface>; | ||
| debug: boolean; | ||
|
|
||
| /** Maps a label string to the plugins registered for it, preserving registration order. */ | ||
| private labelIndex: Map<string, DecoderPluginInterface[]> = new Map(); | ||
| /** Plugins that match all labels (qualifier label '*'). */ | ||
| private wildcardPlugins: DecoderPluginInterface[] = []; |
There was a problem hiding this comment.
Don't leave plugins publicly mutable here.
decode() is now driven by labelIndex/wildcardPlugins, so direct mutations to decoder.plugins no longer affect dispatch even though plugins still looks like the live source of truth. That's a silent behavioral break for existing consumers. Either make plugins read-only/private and funnel changes through registerPlugin(), or keep the indexes synchronized with external mutations.
Also applies to: 103-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/MessageDecoder.ts` around lines 85 - 91, The public mutable array field
plugins is misleading because decode() uses labelIndex and wildcardPlugins, so
external mutations to plugins won't affect dispatch; make plugins private (or at
least a read-only getter that returns a copy) and funnel all changes through
registerPlugin()/unregisterPlugin() (or add registerPlugin/removePlugin methods)
which must update labelIndex and wildcardPlugins consistently whenever a plugin
is added/removed; alternatively, if you must keep the public field, wrap it with
setters that synchronize updates into labelIndex and wildcardPlugins so decode()
stays correct (refer to symbols: plugins, labelIndex, wildcardPlugins,
registerPlugin, decode).
- Revert RawFields.version to number (not number | string). Fix Label_SQ to store Number() instead of raw substring, and compare against numeric 2 instead of string '2'. Update tests accordingly. - Replace setDecodeLevel(false) with failUnknown() in Label_15_FST for consistent error handling across all plugins. https://claude.ai/code/session_01WHPebkdmNV4aEh8wzpzoj5
|
@kevinelliott I've opened a new pull request, #367, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/MessageDecoder.labelindex.test.ts (1)
55-57: Misleading comment: decoder does include default plugins.The comment states "Create a fresh decoder with no default plugins," but the test expects
result.decoder.nameto contain'c-band'(line 67), which is a default plugin. This could confuse future maintainers.📝 Suggested comment fix
- // Create a fresh decoder with no default plugins + // Create a fresh decoder (includes default plugins like CBand) const decoder = new MessageDecoder();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/MessageDecoder.labelindex.test.ts` around lines 55 - 57, The comment is incorrect: MessageDecoder() does include default plugins; either update the comment to say "Create a fresh decoder with default plugins" or explicitly construct the decoder without defaults (e.g., using whatever option/constructor overload disables defaults) so the test intent matches reality; locate the instantiation of MessageDecoder and the expectation checking result.decoder.name (which contains 'c-band') and either change the comment text to reflect default plugins are present or modify the decoder construction to disable defaults so the existing comment is accurate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/MessageDecoder.labelindex.test.ts`:
- Around line 55-57: The comment is incorrect: MessageDecoder() does include
default plugins; either update the comment to say "Create a fresh decoder with
default plugins" or explicitly construct the decoder without defaults (e.g.,
using whatever option/constructor overload disables defaults) so the test intent
matches reality; locate the instantiation of MessageDecoder and the expectation
checking result.decoder.name (which contains 'c-band') and either change the
comment text to reflect default plugins are present or modify the decoder
construction to disable defaults so the existing comment is accurate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 316576c7-3e31-4366-bafc-853b33075ce2
📒 Files selected for processing (6)
lib/DecoderPluginInterface.tslib/MessageDecoder.labelindex.test.tslib/plugins/Label_15_FST.tslib/plugins/Label_44_POS.tslib/plugins/Label_SQ.test.tslib/plugins/Label_SQ.ts
✅ Files skipped from review due to trivial changes (2)
- lib/plugins/Label_SQ.test.ts
- lib/plugins/Label_15_FST.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/plugins/Label_SQ.ts
- lib/plugins/Label_44_POS.ts
- lib/DecoderPluginInterface.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR refactors the MessageDecoder and DecoderPlugin architecture to improve performance and reduce boilerplate code across plugins. The main changes include:
anyfor better IDE support and type checkingKey Changes
MessageDecoder (
lib/MessageDecoder.ts)pluginClassesarray at module level for claritylabelIndexMap to track plugins by label for fast lookupwildcardPluginsarray for plugins matching all labels (qualifier'*')decode()to use label index for efficient plugin selectionregisterPlugin()to populate the label index when plugins are registeredDecoderPlugin (
lib/DecoderPlugin.ts)initResult(message, description)helper to replace boilerplate result initializationsetDecodeLevel(result, decoded, level?)helper to manage decode status and level inferencedebug(options, ...args)helper for consistent debug logging with plugin name prefixfailUnknown(result, text, options?)helper to mark failed decodes and log debug messagesLabel_44_Base (
lib/plugins/Label_44_Base.ts)description,minFields, anddecodeEventFields()hookLabel 44 Plugins
Label_44_ON,Label_44_OFF,Label_44_INnow extend Label_44_BaseLabel_44_ETAremains standalone (different field layout)DecoderPluginInterface (
lib/DecoderPluginInterface.ts)RawFieldsinterface with well-known fields (position, altitude, airports, times, fuel, etc.)raw: anywithraw: RawFieldsfor type safetydecode()signature to accept optionalOptionsparameterTests
DecoderPlugin.test.ts: Tests for initResult, setDecodeLevel, debug, and failUnknown helpersMessageDecoder.labelindex.test.ts: Tests for label indexing, wildcard plugins, and preamble filteringLabel_44_Base.test.ts: Tests for shared Label 44 decoding logicRawFields.test.ts: Tests for type safety of RawFields interfaceImplementation Details
labels: ['*']are always attempted before label-specific pluginssetDecodeLevel()automatically infers 'full' vs 'partial' based on remaining text when no explicit level givenhttps://claude.ai/code/session_01WHPebkdmNV4aEh8wzpzoj5
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests