Replace Node.js built-ins with Web APIs for browser compatibility#380
Replace Node.js built-ins with Web APIs for browser compatibility#380kevinelliott wants to merge 3 commits intomasterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughReplaces Node Buffer/minizlib/base85 usage with browser-friendly implementations: adds Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant LabelDecoder
participant Inflator
participant Parser
Client->>LabelDecoder: provide payload (base64 / ascii85)
LabelDecoder->>LabelDecoder: ascii85Decode() or base64ToUint8Array()
LabelDecoder->>Inflator: push Uint8Array (compressed)
Inflator->>Inflator: pako.Inflate (raw or zlib, Z_SYNC_FLUSH)
Inflator-->>LabelDecoder: Uint8Array (decompressed) or undefined
LabelDecoder->>Parser: TextDecoder.decode(decompressed) -> parse JSON / MIAM
Parser->>Parser: dispatch to VersionPduHandlerTable (hdr: Uint8Array, body?: Uint8Array)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR migrates decoding utilities away from Node.js-only built-ins to browser-compatible Web APIs/pure-JS libraries, aiming to eliminate the Node polyfill chain (and its transitive vulnerabilities) for downstream browser builds.
Changes:
- Replace
base85+Bufferusage with a pure JS ASCII85 decoder andUint8Array/TextEncoder/TextDecoder. - Replace
minizlib-based inflation withpakofor decompression in MIAM and OHMA paths. - Update Jest/tooling and add new plugin test coverage.
Reviewed changes
Copilot reviewed 14 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test-simple.js | Adds a root-level local smoke-test script. |
| package.json | Swaps dependencies to pako, adjusts Jest versions/types, modifies test script. |
| lib/utils/miam.ts | Migrates MIAM decoding from Buffer/minizlib to Uint8Array + pako + ASCII85 utility. |
| lib/utils/ascii85.ts | Introduces a pure-JS ASCII85 (Adobe variant) decoder. |
| lib/plugins/Label_H1_OHMA.ts | Migrates OHMA decompression to pako and replaces base64 decode with an atob() utility. |
| lib/plugins/Label_QS.test.ts | Adds Jest tests for Label_QS. |
| lib/plugins/Label_QR.test.ts | Adds Jest tests for Label_QR. |
| lib/plugins/Label_QP.test.ts | Adds Jest tests for Label_QP. |
| lib/plugins/Label_ColonComma.test.ts | Adds Jest tests for Label_ColonComma. |
| lib/plugins/Label_B6.test.ts | Adds Jest tests for Label_B6. |
| lib/plugins/Label_20_CFB.01.test.ts | Adds Jest tests for Label_20_CFB.01. |
| jest.config.ts | Updates Jest configuration (ts-jest preset, globals, node environment settings). |
| jest.config.simple.js | Adds a secondary Jest config file. |
| firebase-debug.log | Adds a Firebase CLI debug log file. |
| .claude/agents/senior-typescript-dev.md | Adds Claude agent configuration/documentation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lib/utils/miam.ts
Outdated
| decompress.write(body); | ||
| decompress.flush(zlib.constants.Z_SYNC_FLUSH); | ||
| pduData = decompress.read(); | ||
| pduData = inflateData(body, true) || undefined; |
There was a problem hiding this comment.
inflateData() can return undefined (no chunks produced) when decompression fails or produces no output, but the Deflate branch doesn’t record an error when that happens—pduData just stays undefined and CRC validation is skipped silently. Consider explicitly treating an undefined return as an inflation failure (push an error into pduErrors and/or mark the decode as failed) so corrupted/invalid compressed PDUs don’t look like successful decodes with missing text.
| pduData = inflateData(body, true) || undefined; | |
| const inflated = inflateData(body, true); | |
| if (inflated === undefined) { | |
| pduErrors.push('Inflation produced no output for body'); | |
| } else { | |
| pduData = inflated; | |
| } |
There was a problem hiding this comment.
Fixed in d065ba4. inflateData() returning undefined now explicitly pushes an error to pduErrors instead of silently skipping CRC validation.
jest.config.simple.js
Outdated
| module.exports = { | ||
| preset: 'ts-jest', | ||
| testEnvironment: 'node', | ||
| clearMocks: true, | ||
| collectCoverage: true, | ||
| coverageDirectory: 'coverage', | ||
| coverageProvider: 'v8', | ||
| }; |
There was a problem hiding this comment.
jest.config.simple.js duplicates settings already present in jest.config.ts, but nothing in package.json references it. If it’s not used, consider removing it to avoid configuration drift; if it is intended to be used, wire it up via an npm script or document when to use each config.
| module.exports = { | |
| preset: 'ts-jest', | |
| testEnvironment: 'node', | |
| clearMocks: true, | |
| collectCoverage: true, | |
| coverageDirectory: 'coverage', | |
| coverageProvider: 'v8', | |
| }; | |
| // Deprecated: use `jest.config.ts` as the single source of truth for Jest configuration. | |
| // This file is retained only to avoid confusion with older tooling or documentation. | |
| // If you need a Jest config, reference `jest.config.ts` instead of this file. | |
| module.exports = {}; |
There was a problem hiding this comment.
Pre-existing file, not introduced by this PR. Agree it could be cleaned up in a separate PR.
package.json
Outdated
| "scripts": { | ||
| "build": "tsup", | ||
| "test": "jest" | ||
| "test": "NODE_OPTIONS='--localstorage-file=/tmp/jest-localstorage' jest" |
There was a problem hiding this comment.
npm test now sets NODE_OPTIONS='--localstorage-file=/tmp/jest-localstorage'. This hard-codes a Unix-only env-var assignment//tmp path and also relies on a Node flag that isn’t referenced anywhere else in the repo; CI runs Node 18.x (per README/workflow), so if this flag isn’t supported there it will break the test command. Consider removing this, or moving any required localStorage setup into Jest (e.g., setupFiles/testEnvironment) so tests run consistently across supported Node versions.
| "test": "NODE_OPTIONS='--localstorage-file=/tmp/jest-localstorage' jest" | |
| "test": "jest" |
There was a problem hiding this comment.
Pre-existing configuration, not changed by this PR.
test-simple.js
Outdated
| @@ -0,0 +1,2 @@ | |||
| const decoder = require('./lib/plugins/Label_ColonComma'); | |||
There was a problem hiding this comment.
This looks like a local smoke-test script committed to the repo root, and it require()s source files under lib/ (TypeScript) rather than the built dist/ output. If it’s only for local debugging, consider removing it or moving it under an examples/ or scripts/ folder and ensure it uses the published entrypoints so it doesn’t confuse consumers.
| const decoder = require('./lib/plugins/Label_ColonComma'); | |
| const decoder = require('./dist/plugins/Label_ColonComma'); |
There was a problem hiding this comment.
Pre-existing file, not introduced by this PR.
![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 (4)
test-simple.js (1)
1-2: Remove or wire this smoke script to avoid drift.This file appears to be a one-off loader and isn’t tied to the test scripts. Either add an explicit npm script for it or remove it to keep the root clean.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-simple.js` around lines 1 - 2, The file test-simple.js is a stray smoke loader that requires './lib/plugins/Label_ColonComma' but isn’t wired into npm scripts; either delete test-simple.js to avoid repository drift or wire it up by adding an explicit npm script (e.g., "test-simple": "node test-simple.js") in package.json and update any README/CI docs accordingly; if keeping it, ensure the require target (Label_ColonComma) is exported properly and the script name matches any CI or dev tooling references so it doesn't become an orphan..claude/agents/senior-typescript-dev.md (1)
8-24: Minor readability polish in the intro standards section.A few adjacent bullets/sentences start with the same phrasing; tightening that wording would improve scanability without changing intent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/agents/senior-typescript-dev.md around lines 8 - 24, The intro bullets under the "Type System Mastery" section repeat the same lead phrasing ("You ..."), hurting scanability; edit those bullets (e.g., "You design precise, expressive types that catch errors...", "You leverage TypeScript's structural typing system...", "You understand when to use `type` vs `interface`...", "You write utility types...", "You avoid `any`...") to vary sentence starts and shorten where possible—swap some to noun- or gerund-led phrases (e.g., "Design precise, expressive types...", "Leverage TypeScript's structural typing...", "Prefer `unknown` with proper narrowing..."), keep original meaning and strictness recommendations, and ensure consistent tone and parallel structure across the bullets.jest.config.simple.js (1)
1-8: Avoid maintaining a second Jest config without an explicit entrypoint.Since this file isn’t explicitly referenced by scripts, it can diverge from
jest.config.tsover time. Consider removing it or adding a dedicated script (for example,test:simple) that points to it directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@jest.config.simple.js` around lines 1 - 8, This file (jest.config.simple.js) is an unmanaged second Jest config that can drift; either remove the file or add an explicit package.json script that references it to keep it intentional. If you choose to keep it, add a script like "test:simple" that runs the Jest CLI with --config=jest.config.simple.js (so the file is exercised and stays in sync), and update any README or CI job to reference that script; if you choose to remove it, delete jest.config.simple.js and ensure existing tests use jest.config.ts only. Make the change by editing package.json scripts (add "test:simple") or by deleting the file and committing the removal.lib/plugins/Label_H1_OHMA.ts (1)
21-41: Consider extracting sharedinflateDatato a common utility.This function is nearly identical to the one in
lib/utils/miam.ts(lines 11-28), which additionally supports arawparameter forwindowBits. Consolidating into a single shared utility would reduce duplication and ensure consistent behavior.♻️ Suggested approach
Extract to a shared utility (e.g.,
lib/utils/inflate.ts) with arawparameter defaulting tofalse:// lib/utils/inflate.ts export function inflateData(data: Uint8Array, raw: boolean = false): Uint8Array | undefined { const chunks: Uint8Array[] = []; const inflator = new pako.Inflate({ windowBits: raw ? -15 : 15 }); inflator.onData = (chunk: Uint8Array) => { chunks.push(chunk); }; inflator.push(data, 2); // ... rest of implementation }Then import and use in both files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/plugins/Label_H1_OHMA.ts` around lines 21 - 41, The inflateData function is duplicated; extract it into a shared utility (e.g., export function inflateData(data: Uint8Array, raw: boolean = false)) and replace the local implementations with imports; implement the utility to collect chunks, create the inflator with new pako.Inflate({ windowBits: raw ? -15 : 15 }), set inflator.onData to push chunks, call inflator.push(data, 2), handle empty/single/multi-chunk assembly exactly as current code, and update callers (the local inflateData in this file and the one in miam.ts) to call the shared function, passing raw=true where the original used negative windowBits behavior.
🤖 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_B6.test.ts`:
- Around line 39-46: The test creates a console spy (consoleSpy =
jest.spyOn(console, 'log').mockImplementation()) but calls
consoleSpy.mockRestore() only after the expectation, so a failing assertion
skips restore and leaks the spy; fix by ensuring restoration always runs—either
move the restore into a shared afterEach that calls consoleSpy.mockRestore(), or
wrap the invocation of plugin.decode(...) and expect(...) in a try/finally and
call consoleSpy.mockRestore() in finally; update the test in
lib/plugins/Label_B6.test.ts referencing consoleSpy and plugin.decode to use one
of these patterns so the spy is always restored even on assertion failures.
In `@package.json`:
- Line 17: The test script in package.json uses the Node CLI flag
--localstorage-file (referenced in the "test" npm script) which requires Node
>=25 but engines.node currently allows >=18; either update engines.node to
">=25" to match the flag, or change the "test" script to avoid the flag by
adding a guard: check process.versions.node (or use a small shell snippet) and
only append NODE_OPTIONS='--localstorage-file=...' when node >=25, or remove the
flag and configure Jest/localStorage persistence via Jest setup files; modify
either the "test" script or the engines.node field accordingly to keep them
consistent.
---
Nitpick comments:
In @.claude/agents/senior-typescript-dev.md:
- Around line 8-24: The intro bullets under the "Type System Mastery" section
repeat the same lead phrasing ("You ..."), hurting scanability; edit those
bullets (e.g., "You design precise, expressive types that catch errors...", "You
leverage TypeScript's structural typing system...", "You understand when to use
`type` vs `interface`...", "You write utility types...", "You avoid `any`...")
to vary sentence starts and shorten where possible—swap some to noun- or
gerund-led phrases (e.g., "Design precise, expressive types...", "Leverage
TypeScript's structural typing...", "Prefer `unknown` with proper
narrowing..."), keep original meaning and strictness recommendations, and ensure
consistent tone and parallel structure across the bullets.
In `@jest.config.simple.js`:
- Around line 1-8: This file (jest.config.simple.js) is an unmanaged second Jest
config that can drift; either remove the file or add an explicit package.json
script that references it to keep it intentional. If you choose to keep it, add
a script like "test:simple" that runs the Jest CLI with
--config=jest.config.simple.js (so the file is exercised and stays in sync), and
update any README or CI job to reference that script; if you choose to remove
it, delete jest.config.simple.js and ensure existing tests use jest.config.ts
only. Make the change by editing package.json scripts (add "test:simple") or by
deleting the file and committing the removal.
In `@lib/plugins/Label_H1_OHMA.ts`:
- Around line 21-41: The inflateData function is duplicated; extract it into a
shared utility (e.g., export function inflateData(data: Uint8Array, raw: boolean
= false)) and replace the local implementations with imports; implement the
utility to collect chunks, create the inflator with new pako.Inflate({
windowBits: raw ? -15 : 15 }), set inflator.onData to push chunks, call
inflator.push(data, 2), handle empty/single/multi-chunk assembly exactly as
current code, and update callers (the local inflateData in this file and the one
in miam.ts) to call the shared function, passing raw=true where the original
used negative windowBits behavior.
In `@test-simple.js`:
- Around line 1-2: The file test-simple.js is a stray smoke loader that requires
'./lib/plugins/Label_ColonComma' but isn’t wired into npm scripts; either delete
test-simple.js to avoid repository drift or wire it up by adding an explicit npm
script (e.g., "test-simple": "node test-simple.js") in package.json and update
any README/CI docs accordingly; if keeping it, ensure the require target
(Label_ColonComma) is exported properly and the script name matches any CI or
dev tooling references so it doesn't become an orphan.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: da7b0670-992d-46c9-921c-f4d8dd53706c
⛔ Files ignored due to path filters (2)
firebase-debug.logis excluded by!**/*.logpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
.claude/agents/senior-typescript-dev.mdjest.config.simple.jsjest.config.tslib/plugins/Label_20_CFB.01.test.tslib/plugins/Label_B6.test.tslib/plugins/Label_ColonComma.test.tslib/plugins/Label_H1_OHMA.tslib/plugins/Label_QP.test.tslib/plugins/Label_QR.test.tslib/plugins/Label_QS.test.tslib/utils/ascii85.tslib/utils/miam.tspackage.jsontest-simple.js
lib/plugins/Label_B6.test.ts
Outdated
| const consoleSpy = jest.spyOn(console, 'log').mockImplementation(); | ||
| const text = '/DEBUG TEST'; | ||
|
|
||
| plugin.decode({ text: text }, { debug: true }); | ||
|
|
||
| expect(consoleSpy).toHaveBeenCalledWith(expect.stringContaining('CPDLC')); | ||
| consoleSpy.mockRestore(); | ||
| }); |
There was a problem hiding this comment.
Always restore console spies even on assertion failure.
In this test, mockRestore() is skipped if an expectation fails. Wrap with try/finally (or move to afterEach) to prevent cross-test leakage.
Suggested fix
test('debug option logs message', () => {
- const consoleSpy = jest.spyOn(console, 'log').mockImplementation();
- const text = '/DEBUG TEST';
-
- plugin.decode({ text: text }, { debug: true });
-
- expect(consoleSpy).toHaveBeenCalledWith(expect.stringContaining('CPDLC'));
- consoleSpy.mockRestore();
+ const consoleSpy = jest.spyOn(console, 'log').mockImplementation(() => {});
+ try {
+ const text = '/DEBUG TEST';
+ plugin.decode({ text }, { debug: true });
+ expect(consoleSpy).toHaveBeenCalledWith(expect.stringContaining('CPDLC'));
+ } finally {
+ consoleSpy.mockRestore();
+ }
});📝 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 consoleSpy = jest.spyOn(console, 'log').mockImplementation(); | |
| const text = '/DEBUG TEST'; | |
| plugin.decode({ text: text }, { debug: true }); | |
| expect(consoleSpy).toHaveBeenCalledWith(expect.stringContaining('CPDLC')); | |
| consoleSpy.mockRestore(); | |
| }); | |
| test('debug option logs message', () => { | |
| const consoleSpy = jest.spyOn(console, 'log').mockImplementation(() => {}); | |
| try { | |
| const text = '/DEBUG TEST'; | |
| plugin.decode({ text }, { debug: true }); | |
| expect(consoleSpy).toHaveBeenCalledWith(expect.stringContaining('CPDLC')); | |
| } finally { | |
| consoleSpy.mockRestore(); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/plugins/Label_B6.test.ts` around lines 39 - 46, The test creates a
console spy (consoleSpy = jest.spyOn(console, 'log').mockImplementation()) but
calls consoleSpy.mockRestore() only after the expectation, so a failing
assertion skips restore and leaks the spy; fix by ensuring restoration always
runs—either move the restore into a shared afterEach that calls
consoleSpy.mockRestore(), or wrap the invocation of plugin.decode(...) and
expect(...) in a try/finally and call consoleSpy.mockRestore() in finally;
update the test in lib/plugins/Label_B6.test.ts referencing consoleSpy and
plugin.decode to use one of these patterns so the spy is always restored even on
assertion failures.
There was a problem hiding this comment.
Pre-existing test pattern, not changed by this PR.
There was a problem hiding this comment.
@kevinelliott, understood — the Label_B6.test.ts console spy pattern pre-dates this PR and is out of scope here. I'll let it go. Thanks for the clarification!
🧠 Learnings used
Learnt from: makrsmark
Repo: airframesio/acars-decoder-typescript PR: 332
File: lib/plugins/Label_H1.ts:39-40
Timestamp: 2026-02-21T01:32:05.328Z
Learning: In lib/plugins/Label_H1.ts, when parsing two-part H1 messages (split by '#'), the offset logic `parts[0] === '- ' || isNaN(parseInt(parts[1][1])) ? 3 : 4` is correct. The `parts[0] === '- '` condition ensures that messages like '- `#M1FPN/`...' use offset=3 to skip 3 characters after '#' ('M1' + next char), whereas messages like 'F37AKL0767#M1BFPN/...' use offset=4 to skip 4 characters ('M1B' + '/'). Removing the `parts[0] === '- '` check would break '- `#M1`...' messages where parts[1][1] is numeric.
![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: d5882362e4
ℹ️ 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/plugins/Label_H1_OHMA.ts
Outdated
|
|
||
| function base64ToUint8Array(base64: string): Uint8Array { | ||
| // Match Buffer.from(str, 'base64') behavior: strip non-base64 chars, handle missing padding | ||
| const cleaned = base64.replace(/[^A-Za-z0-9+/]/g, ''); |
There was a problem hiding this comment.
The sanitizer removes - and _ via base64.replace(/[^A-Za-z0-9+/]/g, ''), but Buffer.from(str, 'base64') (the previous behavior) accepts URL-safe base64 characters. If an OHMA payload contains base64url data, this code silently drops bytes before inflation, which can corrupt or fail decoding and incorrectly mark valid messages as unknown.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Is this actually needed?
There was a problem hiding this comment.
Fixed in d065ba4. base64ToUint8Array now converts base64url chars (- → +, _ → /) before stripping invalid chars. The function was also moved to the shared lib/utils/compression.ts module.
There was a problem hiding this comment.
Yes — it replaces Buffer.from(data, 'base64') which is a Node.js-only API. The helper uses atob() which is available in all modern runtimes (browsers, Node 18+, Deno, Bun). It now lives in the shared lib/utils/compression.ts module so both miam.ts and Label_H1_OHMA.ts import it from one place.
firebase-debug.log
Outdated
| @@ -0,0 +1,28 @@ | |||
| [debug] [2026-01-06T07:22:31.539Z] > command requires scopes: ["email","openid","https://www.googleapis.com/auth/cloudplatformprojects.readonly","https://www.googleapis.com/auth/firebase","https://www.googleapis.com/auth/cloud-platform"] | |||
| [debug] [2026-01-06T07:22:31.545Z] > authorizing via signed-in user (kevin@phunc.com) | |||
There was a problem hiding this comment.
This commit adds a local Firebase debug log to source control, including a signed-in email address and OAuth scope details. Keeping this file in the repo exposes operator/account metadata in git history and package artifacts without providing runtime value, so it should be removed and ignored.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pre-existing file, not introduced by this PR. Agree it should be gitignored.
Migrate from Node.js-specific `Buffer`, `minizlib`, and `base85` packages to cross-platform alternatives (`Uint8Array`, `pako`, pure JS ASCII85 decoder). This eliminates the need for `vite-plugin-node-polyfills` or equivalent in browser environments and removes all transitive production vulnerabilities from `base85` → `crypto-browserify` → `elliptic`. Changes: - Replace `Buffer` with `Uint8Array` + `TextEncoder`/`TextDecoder` - Replace `minizlib` with `pako` (pure JS zlib, works everywhere) - Replace `base85` npm package with pure JS ASCII85 decoder - Replace `Buffer.from(data, 'base64')` with `atob()`-based utility Closes #379 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
kevinelliott
force-pushed
the
379-migrate-to-web-apis
branch
from
d588236 to
c78b720
Compare
lib/plugins/Label_H1_OHMA.ts
Outdated
|
|
||
| function base64ToUint8Array(base64: string): Uint8Array { | ||
| // Match Buffer.from(str, 'base64') behavior: strip non-base64 chars, handle missing padding | ||
| const cleaned = base64.replace(/[^A-Za-z0-9+/]/g, ''); |
There was a problem hiding this comment.
Is this actually needed?
| * Replaces the `base85` npm package to eliminate Node.js Buffer dependency. | ||
| * Works in browsers, Node.js 18+, Deno, Bun, and edge runtimes. | ||
| */ | ||
| export function ascii85Decode(input: string): Uint8Array | null { |
There was a problem hiding this comment.
this can probably be it's own lib, but i guess works for now. I won't even pretend to understand it
There was a problem hiding this comment.
Agreed it could be its own lib. For now it's a single function (~40 lines) with no dependencies, so keeping it in-tree seemed simplest. Happy to extract it if it ends up being useful elsewhere.
lib/utils/miam.ts
Outdated
| function inflateData(data: Uint8Array, raw: boolean): Uint8Array | undefined { | ||
| const chunks: Uint8Array[] = []; | ||
| const inflator = new pako.Inflate({ windowBits: raw ? -15 : 15 }); | ||
| inflator.onData = (chunk: Uint8Array) => { | ||
| chunks.push(chunk); | ||
| }; | ||
| inflator.push(data, 2); // Z_SYNC_FLUSH | ||
|
|
||
| if (chunks.length === 0) return undefined; | ||
| if (chunks.length === 1) return chunks[0]; | ||
|
|
||
| const totalLen = chunks.reduce((sum, c) => sum + c.length, 0); | ||
| const result = new Uint8Array(totalLen); | ||
| let offset = 0; | ||
| for (const chunk of chunks) { | ||
| result.set(chunk, offset); | ||
| offset += chunk.length; | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
isn't this the same as what's in OHMA?
There was a problem hiding this comment.
It was — good catch. Fixed in d065ba4: extracted both inflateData and base64ToUint8Array into a shared lib/utils/compression.ts module. Both miam.ts and Label_H1_OHMA.ts now import from there.
- Extract shared `inflateData` and `base64ToUint8Array` into `lib/utils/compression.ts` to eliminate duplication between miam.ts and Label_H1_OHMA.ts (makrsmark) - Add explicit error when inflate produces no output instead of silently skipping CRC validation (Copilot) - Fix base64url support: convert `-`/`_` to `+`/`/` before decoding instead of stripping them (chatgpt-codex-connector) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Addressed all actionable review feedback in d065ba4: Copilot (miam.ts:428): Fixed. chatgpt-codex-connector (OHMA base64url): Fixed. @makrsmark (miam.ts:30 duplication): Fixed. Extracted shared @makrsmark (OHMA base64 helper): Yes, this is needed to replace @makrsmark (ascii85): Agreed it could be its own lib. For now it's a single function (~40 lines) with no dependencies, so keeping it in-tree avoids adding another npm package. The remaining comments (jest.config.simple.js, test-simple.js, firebase-debug.log, Label_B6.test.ts spy, NODE_OPTIONS) are all pre-existing issues unrelated to this PR. |
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/utils/miam.ts`:
- Around line 171-177: The ascii85Decode() result must be treated as a hard
decode failure: inside the rawBody handling branch (where bpad is parsed and
ascii85Decode('<~' + rawBody + '~>') is called) reject the PDU when
ascii85Decode returns null or when the decoded buffer length is smaller than
bpadValue instead of collapsing to undefined or keeping an impossible payload;
specifically, if decode result is null or decoded.length < bpadValue, abort
processing (return or otherwise mark the PDU as malformed) so
MIAMCoreUtils.corePduDataHandler does not see body === undefined and incorrectly
accept it, otherwise trim the decoded buffer by bpadValue as currently done.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ca9693f9-e414-4866-961b-7dcd35d09014
📒 Files selected for processing (3)
lib/plugins/Label_H1_OHMA.tslib/utils/compression.tslib/utils/miam.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/plugins/Label_H1_OHMA.ts
When ascii85Decode() returns null for a malformed body, return an explicit decode error instead of collapsing to undefined (which silently sets pduCrcIsOk=true in corePduDataHandler). (coderabbitai) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Closes #379
BufferwithUint8Array+TextEncoder/TextDecoderthroughoutminizlib(Node.js zlib bindings) withpako(pure JS, works in all environments)base85npm package with a pure JS ASCII85 decoder (lib/utils/ascii85.ts)Buffer.from(data, 'base64')with anatob()-based utilityResults
base85→crypto-browserify→elliptic)vite-plugin-node-polyfillsrequirement)Label_20_POS.test.tsis a pre-existing TS strictness issue)Test plan
npm run buildsucceeds (CJS + ESM + DTS)npm audit --omit=devreports 0 vulnerabilities🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Bug Fixes