Merged
Conversation
make convertDateTimeToEpoch be in a reasonable format fix code/tests accordingly
|
Caution Review failedPull request was closed or merged during review 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)
WalkthroughThis PR switches date interpretation from MMDDYY/MMDDYYYY to DDMMYY/DDMMYYYY across the core epoch converter, updates label plugins to pass DDMMYY inputs, and adjusts/adds tests and H1 message-type handling accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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
🤖 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/DateTimeUtils.ts`:
- Around line 56-63: convertDateTimeToEpoch currently assumes the incoming date
is DDMMYY or DDMMYYYY and mangles other formats (causing RangeError/NaN
downstream); update the function to be migration-safe by detecting common
incoming layouts (e.g., DDMMYY/DDMMYYYY, YYYYMMDD, and already-ISO or dashed
forms) and attempt parsing in order of likelihood: normalize 2-digit year to
4-digit, if date matches /^\d{8}$/ decide whether it's DDMMYYYY or YYYYMMDD by
checking a plausible year range (e.g., first four chars between 1900–2100) or
try both and pick the one where Date.parse(timestamp) yields a valid number, and
if none succeed return a clear error (or throw) rather than producing NaN;
reference the convertDateTimeToEpoch function and the timestamp/millis variables
to locate the change.
In `@lib/plugins/Label_H1_INR.test.ts`:
- Around line 13-29: The INR test is failing because plugin.decode(message)
returns decoded=false for the provided INR fixture; either update the fixture or
fix the INR decode path so this input is accepted. Locate the INR handler used
by plugin.decode and adjust its detection/parsing logic to accept strings like
'INR/ID91511S,,/DC04032026,143534/...' and populate decodeResult.decoded = true,
decoder.decodeLevel = 'partial' and the raw fields (tail, message_timestamp,
day, eta_time, fuel_on_board, checksum) plus remaining.text as asserted in the
test; alternatively, if the handler is correct, modify the test fixture to match
the exact format the INR parser expects so the assertions (decodeResult.decoded,
decoder.decodeLevel, raw.tail, raw.message_timestamp, raw.day, raw.eta_time,
raw.fuel_on_board, raw.checksum, formatted.items.length, remaining.text) become
valid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 29942a1a-cc72-4d54-9324-24b6299b9706
📒 Files selected for processing (9)
lib/DateTimeUtils.tslib/plugins/Label_15.tslib/plugins/Label_1J_2J_FTX.test.tslib/plugins/Label_1M_Slash.tslib/plugins/Label_24_Slash.tslib/plugins/Label_4J_POS.test.tslib/plugins/Label_H1_FLR.test.tslib/plugins/Label_H1_INR.test.tslib/plugins/Label_H1_WRN.test.ts
💤 Files with no reviewable changes (1)
- lib/plugins/Label_4J_POS.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/utils/h1_helper.ts (1)
413-415:⚠️ Potential issue | 🟠 MajorSingle-field
/DCdate is now silently dropped.On Line 413-Line 415, valid date data (
data.length === 1) is ignored. This causes raw decode data loss compared to prior behavior and can break consumers expecting date presence when timestamp cannot yet be derived.💡 Proposed fix
function processDateCode(decodeResult: DecodeResult, data: string[]) { if (data.length === 1) { - // noop? + // Preserve raw date when time component is not present yet + decodeResult.raw.message_date = data[0]; } else if (data.length === 2) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/utils/h1_helper.ts` around lines 413 - 415, The branch handling date components drops valid single-field `/DC` values: in lib/utils/h1_helper.ts where the code branches on data.length (the `if (data.length === 1) { // noop? } else if (data.length === 2) { ... }` block), change the noop to preserve and normalize the single-field date instead of dropping it—reuse the same parsing/assignment logic used for the two-field case (the `data` array processing) to convert `data[0]` into the output date/DC field (or keep it as the decoded date string if that’s the module’s convention) so consumers receive the date when only one component is present; ensure you update the same output variable/field that the two-field branch sets and add any minimal validation used there.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lib/utils/h1_helper.ts`:
- Around line 413-415: The branch handling date components drops valid
single-field `/DC` values: in lib/utils/h1_helper.ts where the code branches on
data.length (the `if (data.length === 1) { // noop? } else if (data.length ===
2) { ... }` block), change the noop to preserve and normalize the single-field
date instead of dropping it—reuse the same parsing/assignment logic used for the
two-field case (the `data` array processing) to convert `data[0]` into the
output date/DC field (or keep it as the decoded date string if that’s the
module’s convention) so consumers receive the date when only one component is
present; ensure you update the same output variable/field that the two-field
branch sets and add any minimal validation used there.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5de19aed-6852-4440-9117-2bb3cda8b043
📒 Files selected for processing (1)
lib/utils/h1_helper.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
1 participant
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.
make convertDateTimeToEpoch be in a reasonable format
fix code/tests accordingly
Summary by CodeRabbit
New Features
Bug Fixes
Tests