fix(result_formatter): preserve full hex for checksums wider than 16 bits#488
fix(result_formatter): preserve full hex for checksums wider than 16 bits#488fuleinist wants to merge 1 commit into
Conversation
…bits
ResultFormatter.checksum used ('0000' + hex).slice(-4) which truncates
any checksum > 0xFFFF to its last 4 hex digits. For example, 0xAFF5C
displayed as 0xFF5C and 0xDEADBEEF as 0xBEEF — silently wrong values
in formatted.items that downstream consumers render to users.
Replace with (value >>> 0).toString(16).padStart(4, '0') which:
- Pads values < 0x1000 to 4 hex digits (preserving prior behaviour)
- Preserves the full hex representation for wider checksums
- Coerces to unsigned via >>> 0, avoiding negative-hex display for
32-bit CRCs whose top bit is set (companion to airframesio#458)
Add result_formatter.test.ts with 6 unit tests covering 12-bit, 16-bit,
20-bit, and 32-bit checksum round-trips.
Fixes airframesio#484
Walkthrough
ChangesChecksum formatting
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Pull request overview
This PR fixes checksum hex formatting in ResultFormatter.checksum so checksums wider than 16 bits are no longer silently truncated in the formatted output consumed by downstream renderers.
Changes:
- Replace the prior 4-hex-digit truncation logic with
padStart(4, '0')while keeping the full hex for wider checksums. - Coerce checksum values to unsigned form for display to avoid negative hex strings for signed-Int32 CRC sources.
- Add unit tests for
ResultFormatter.checksumcovering padding and non-truncation scenarios.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/utils/result_formatter.ts | Updates checksum formatting logic to avoid truncation and improve unsigned display handling. |
| lib/utils/result_formatter.test.ts | Adds unit tests validating checksum formatting across multiple widths and padding cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| code: 'CHECKSUM', | ||
| label: 'Message Checksum', | ||
| value: '0x' + ('0000' + decodeResult.raw.checksum.toString(16)).slice(-4), | ||
| value: '0x' + (decodeResult.raw.checksum >>> 0).toString(16).padStart(4, '0'), |
| test('displays large 32-bit CRC without sign artifacts', () => { | ||
| // 0xDEADBEEF has the top bit set; toString(16) on a signed int would | ||
| // produce a negative string. >>> 0 coerces to unsigned first. | ||
| const dr = makeDecodeResult(); | ||
| ResultFormatter.checksum(dr, 0xdeadbeef); | ||
| expect(dr.raw.checksum).toBe(0xdeadbeef); | ||
| const item = dr.formatted.items[dr.formatted.items.length - 1]; | ||
| expect(item.value).toBe('0xdeadbeef'); | ||
| }); |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/utils/result_formatter.test.ts`:
- Around line 40-45: Rename the test case around ResultFormatter.checksum from
“32-bit checksum” to “24-bit checksum,” keeping its assertions and test behavior
unchanged.
- Around line 48-55: Update the test around ResultFormatter.checksum to pass the
signed int32 representation of 0xDEADBEEF rather than the positive unsigned
Number, while continuing to assert the stored checksum and formatted value are
0xdeadbeef.
In `@lib/utils/result_formatter.ts`:
- Line 297: Format the value expression in the result formatter object according
to Prettier, wrapping the concatenation and conversion expression as needed so
the prettier/prettier lint check passes; preserve the existing checksum
formatting behavior.
🪄 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 Plus
Run ID: a469c2fa-a52c-418d-87f9-a8270254dc98
📒 Files selected for processing (2)
lib/utils/result_formatter.test.tslib/utils/result_formatter.ts
| test('preserves full hex for 32-bit checksum (no truncation)', () => { | ||
| const dr = makeDecodeResult(); | ||
| ResultFormatter.checksum(dr, 0x123456); | ||
| expect(dr.raw.checksum).toBe(0x123456); | ||
| const item = dr.formatted.items[dr.formatted.items.length - 1]; | ||
| expect(item.value).toBe('0x123456'); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Rename this as a 24-bit checksum test.
0x123456 is a 24-bit value, not a 32-bit value. The suite’s 0xdeadbeef case covers 32-bit formatting, but this test name should accurately describe its input.
Proposed fix
- test('preserves full hex for 32-bit checksum (no truncation)', () => {
+ test('preserves full hex for 24-bit checksum (no truncation)', () => {📝 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.
| test('preserves full hex for 32-bit checksum (no truncation)', () => { | |
| const dr = makeDecodeResult(); | |
| ResultFormatter.checksum(dr, 0x123456); | |
| expect(dr.raw.checksum).toBe(0x123456); | |
| const item = dr.formatted.items[dr.formatted.items.length - 1]; | |
| expect(item.value).toBe('0x123456'); | |
| test('preserves full hex for 24-bit checksum (no truncation)', () => { | |
| const dr = makeDecodeResult(); | |
| ResultFormatter.checksum(dr, 0x123456); | |
| expect(dr.raw.checksum).toBe(0x123456); | |
| const item = dr.formatted.items[dr.formatted.items.length - 1]; | |
| expect(item.value).toBe('0x123456'); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/utils/result_formatter.test.ts` around lines 40 - 45, Rename the test
case around ResultFormatter.checksum from “32-bit checksum” to “24-bit
checksum,” keeping its assertions and test behavior unchanged.
| test('displays large 32-bit CRC without sign artifacts', () => { | ||
| // 0xDEADBEEF has the top bit set; toString(16) on a signed int would | ||
| // produce a negative string. >>> 0 coerces to unsigned first. | ||
| const dr = makeDecodeResult(); | ||
| ResultFormatter.checksum(dr, 0xdeadbeef); | ||
| expect(dr.raw.checksum).toBe(0xdeadbeef); | ||
| const item = dr.formatted.items[dr.formatted.items.length - 1]; | ||
| expect(item.value).toBe('0xdeadbeef'); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exercise the signed int32 input path.
This test passes the positive Number 0xdeadbeef, for which toString(16) already returns deadbeef; it would pass even without >>> 0. Pass the signed representation instead to verify the regression fix.
Proposed fix
const dr = makeDecodeResult();
- ResultFormatter.checksum(dr, 0xdeadbeef);
- expect(dr.raw.checksum).toBe(0xdeadbeef);
+ const signedChecksum = -559038737; // int32 representation of 0xdeadbeef
+ ResultFormatter.checksum(dr, signedChecksum);
+ expect(dr.raw.checksum).toBe(signedChecksum);
const item = dr.formatted.items[dr.formatted.items.length - 1];
expect(item.value).toBe('0xdeadbeef');📝 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.
| test('displays large 32-bit CRC without sign artifacts', () => { | |
| // 0xDEADBEEF has the top bit set; toString(16) on a signed int would | |
| // produce a negative string. >>> 0 coerces to unsigned first. | |
| const dr = makeDecodeResult(); | |
| ResultFormatter.checksum(dr, 0xdeadbeef); | |
| expect(dr.raw.checksum).toBe(0xdeadbeef); | |
| const item = dr.formatted.items[dr.formatted.items.length - 1]; | |
| expect(item.value).toBe('0xdeadbeef'); | |
| test('displays large 32-bit CRC without sign artifacts', () => { | |
| // 0xDEADBEEF has the top bit set; toString(16) on a signed int would | |
| // produce a negative string. >>> 0 coerces to unsigned first. | |
| const dr = makeDecodeResult(); | |
| const signedChecksum = -559038737; // int32 representation of 0xdeadbeef | |
| ResultFormatter.checksum(dr, signedChecksum); | |
| expect(dr.raw.checksum).toBe(signedChecksum); | |
| const item = dr.formatted.items[dr.formatted.items.length - 1]; | |
| expect(item.value).toBe('0xdeadbeef'); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/utils/result_formatter.test.ts` around lines 48 - 55, Update the test
around ResultFormatter.checksum to pass the signed int32 representation of
0xDEADBEEF rather than the positive unsigned Number, while continuing to assert
the stored checksum and formatted value are 0xdeadbeef.
| code: 'CHECKSUM', | ||
| label: 'Message Checksum', | ||
| value: '0x' + ('0000' + decodeResult.raw.checksum.toString(16)).slice(-4), | ||
| value: '0x' + (decodeResult.raw.checksum >>> 0).toString(16).padStart(4, '0'), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Apply Prettier formatting to this expression.
ESLint reports a prettier/prettier error on this line; wrap the value expression so the lint check passes.
Proposed fix
- value: '0x' + (decodeResult.raw.checksum >>> 0).toString(16).padStart(4, '0'),
+ value:
+ '0x' + (decodeResult.raw.checksum >>> 0).toString(16).padStart(4, '0'),📝 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.
| value: '0x' + (decodeResult.raw.checksum >>> 0).toString(16).padStart(4, '0'), | |
| value: | |
| '0x' + (decodeResult.raw.checksum >>> 0).toString(16).padStart(4, '0'), |
🧰 Tools
🪛 ESLint
[error] 297-297: Insert ⏎·······
(prettier/prettier)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/utils/result_formatter.ts` at line 297, Format the value expression in
the result formatter object according to Prettier, wrapping the concatenation
and conversion expression as needed so the prettier/prettier lint check passes;
preserve the existing checksum formatting behavior.
Source: Linters/SAST tools
| expect(item.value).toBe('0x003b'); | ||
| }); | ||
|
|
||
| test('preserves full hex for 20-bit checksum (no truncation)', () => { |
There was a problem hiding this comment.
Are there any examples of real messages that have this length checksum?
| expect(item.value).toBe('0x123456'); | ||
| }); | ||
|
|
||
| test('displays large 32-bit CRC without sign artifacts', () => { |
Summary
Fixes #484
ResultFormatter.checksumused('0000' + hex).slice(-4)which silently truncates any checksum wider than 16 bits to its last 4 hex digits. For example:0xAFF5C→ displayed as0xFF5C(wrong)0x123456→ displayed as0x3456(wrong)0xDEADBEEF→ displayed as0xBEEF(wrong)The
raw.checksumvalue is stored correctly; only theformatted.itemsentry (what downstream consumers render) is corrupted.Fix
Replaced the truncating pad-and-slice with:
padStart(4, '0')pads values < 0x1000 to 4 hex digits (preserving prior behaviour)>>> 0coerces to unsigned, avoiding negative-hex display for 32-bit CRCs whose top bit is set (companion to Bug: MIAM v1 PDU CRC stored as signed Int32 — top-bit-set CRCs come out negative #458)Test coverage
Added
lib/utils/result_formatter.test.tswith 6 unit tests:0x12340x12340x003b0x003b0xaff5c0xaff5c0x1234560x1234560xdeadbeef0xdeadbeef0xabc0x0abcVerification
Full suite green. No regressions.
Summary by CodeRabbit
Bug Fixes
Tests