Add QTE time conversion library with parsing and formatting#1
Merged
adelrodriguez merged 1 commit intomainfrom Feb 18, 2026
Conversation
|
Warning Rate limit exceeded
⌛ 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. WalkthroughAdds a new time-expression library (qte): parsing, formatting, validation helpers, unit conversion functions, constants, types, extensive tests, docs, build/CI updates, and release metadata. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 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 |
Owner
Author
|
@greptileai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
src/lib/parse.ts (1)
147-156:parse("1e999h")returnsInfinity— consider whether non-finite results should map toNaN.
Number.parseFloat("1e999")yieldsInfinity, so the multiplication on line 154 producesInfinity. The function has no finiteness guard before returningtotal. If very large exponents are supported for round-tripping (as documented in the changeset), it may be worth adding aNumber.isFinite(total)check before line 183, returningNaNfor non-finite results.This is a minor edge case since realistic inputs won't hit it, but it would make the contract ("returns a number in ms, or NaN") airtight.
Proposed guard before return
+ if (!Number.isFinite(total)) { + return Number.NaN + } + return total }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/parse.ts` around lines 147 - 156, In the parse function, after accumulating partValue into total and absoluteTotal (using variables total and absoluteTotal within parse and following the loop that computes parts), add a finiteness guard: check Number.isFinite(total) (and optionally Number.isFinite(absoluteTotal) if you rely on it) before returning the final value and return Number.NaN when the result is not finite; this ensures inputs like parse("1e999h") produce NaN instead of Infinity.src/lib/format.ts (1)
105-183: Multi-precision formatting: carry-over logic is correct but complex — consider adding a brief inline note.I traced through several edge cases (e.g.,
3_599_600with precision 2 → carry from 60s→1m→1h, collapsing to"1h"). The backward carry loop with splice + index adjustment works correctly, and the trailing-zero trim + all-zero guard properly avoids-0ms.One optional readability improvement: a short comment before the main loop (lines 114–131) explaining why the last-segment branch rounds while earlier branches floor would help future readers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/format.ts` around lines 105 - 183, The carry-over logic in formatMultiPrecision is correct but hard to follow; add a concise inline comment before the main for-loop (the loop that builds segments using Math.floor and the special-case rounding when segments.length === precision - 1) explaining that earlier units use floor to take whole units while the final allowed segment uses Math.round to implement proper rounding/carry semantics (e.g., rounding seconds can overflow into minutes/hours), to clarify why we mix floor and round and why the backward carry/splice step is needed.src/__tests__/round-trip.test.ts (2)
133-146: Verify large-value round-trip correctness beyond sign preservation.The test checks
isNaN,isFinite, andMath.sign— but doesn't verify that the parsed value is reasonably close to the original. For1e100ms, the formatted string goes through rounding and unit decomposition, so the parsed value could be wildly different from the input while still satisfying these three checks. Consider adding a relative-error assertion or at least a comment explaining why sign-only validation is sufficient here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/round-trip.test.ts` around lines 133 - 146, The test "should keep very large finite format output parseable" currently only asserts non-NaN, finite, and sign preservation for values formatted by format(...) and parsed by parse(...); update the test to also assert that the parsed value is within an acceptable relative error of the original (e.g., compute Math.abs(parsed - value) / Math.abs(value) and expect it to be below a small threshold appropriate for the formatting options), or if exact numeric closeness is infeasible for extreme values, add a clarifying comment in the test explaining why only sign/finite/NaN checks are sufficient; reference the existing usage of format and parse and the test case name when making this change.
21-48:getLastUnitMsonly handles short-form unit suffixes.If this helper is ever used with a
long: trueformatted expression (e.g."1 hour 30 minutes"), the switch would hit the default branch and throw, since"minutes"doesn't match any case. Currently it's only used in the short-format sampled-values test (line 116), so this works — but adding a comment or a couple of long-form cases ("years","months","minutes", etc.) would make it more robust if future tests reuse it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/round-trip.test.ts` around lines 21 - 48, The helper getLastUnitMs currently only recognizes short unit suffixes and will throw for long-form units like "minutes" or "hours"; update getLastUnitMs to accept long-form variants by extending the switch (or normalizing the unit) to handle "year"/"years", "month"/"months", "week"/"weeks", "day"/"days", "hour"/"hours", "minute"/"minutes", "second"/"seconds", and "millisecond"/"milliseconds" (returning the same MS_PER_YEAR, MS_PER_MONTH, MS_PER_WEEK, MS_PER_DAY, MS_PER_HOUR, MS_PER_MINUTE, MS_PER_SECOND, and 1 respectively), or alternatively normalize common plurals to their short forms before the switch so getLastUnitMs and the constants MS_PER_YEAR, MS_PER_MONTH, MS_PER_WEEK, MS_PER_DAY, MS_PER_HOUR, MS_PER_MINUTE, and MS_PER_SECOND handle both short and long forms..agents/SPEC.md (1)
324-324: Fenced code blocks missing language specifiers.The code blocks at lines 324 and 342 lack a language identifier, which triggers markdownlint MD040. Adding
textor an appropriate language tag will satisfy the linter and improve rendering.📝 Proposed fix
Line 324:
-``` +```text 1 second = 1_000 msLine 342:
-``` +```text src/Also applies to: 342-342
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/SPEC.md at line 324, The Markdown fenced code blocks that contain the snippets "1 second = 1_000 ms" and "src/" are missing language specifiers causing markdownlint MD040; update those fences to include a language tag (for example, replace ``` with ```text) for the blocks that contain "1 second = 1_000 ms" and "src/" so the linter and renderer recognize them properly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/SPEC.md:
- Line 193: The spec has inconsistent expected outputs for format(0, { long:
true }) — update the Step 4 test expectation that currently shows "0 ms" to the
long-form "0 milliseconds" so it matches the Edge Cases entry; search for
occurrences of format(0, { long: true }) and replace any "0 ms" test expectation
with "0 milliseconds" to keep the SPEC.md consistent.
- Line 40: Update the expected value for weeks("1y") in SPEC.md: replace 52.1775
with the correct calculation MS_PER_YEAR / MS_PER_WEEK = 365.25 / 7 ≈
52.178571... (or truncate/round to the same decimal precision used elsewhere) so
the documented output for weeks("1y") matches the actual computed value.
- Line 48: The expected value for minutes("1 year 2 weeks 5 days") in SPEC.md is
incorrect; update the example output from 532_890 to the correct total 553_320
(1 year = 525,960 min, 2 weeks = 20,160 min, 5 days = 7,200 min) so the
minutes(...) example matches the computed constants and documented behavior.
In @.changeset/fuzzy-places-wish.md:
- Line 5: Update the changeset text to reference the correct exported function
name: replace the mention of isValid with isTimeExpression (the actual export
from src/lib/utils.ts) so the changelog lists isTimeExpression alongside parse
and format; ensure the changeset line uses the exact symbol isTimeExpression to
match the public API.
In `@README.md`:
- Line 187: The README example for the compound expression using parse is wrong:
update the expected numeric result for parse("1 year 2 weeks 5 days") from
32_594_400_000 to 33_199_200_000 so the example matches the correct computation
(31_557_600_000 + 2×604_800_000 + 5×86_400_000); locate the example referencing
the parse function and replace the incorrect literal with 33_199_200_000.
- Around line 249-259: The README's Error Handling section incorrectly states
that parse and format throw TypeError; update the documentation to reflect
actual implementation by changing the error type wording from "TypeError" to
"Error" for parse() and for format() where non-finite numbers are mentioned,
leaving the existing "RangeError" description for format() precision unchanged;
search for the symbols parse and format in that section to locate and update the
two occurrences.
---
Nitpick comments:
In @.agents/SPEC.md:
- Line 324: The Markdown fenced code blocks that contain the snippets "1 second
= 1_000 ms" and "src/" are missing language specifiers causing markdownlint
MD040; update those fences to include a language tag (for example, replace ```
with ```text) for the blocks that contain "1 second = 1_000 ms" and "src/" so
the linter and renderer recognize them properly.
In `@src/__tests__/round-trip.test.ts`:
- Around line 133-146: The test "should keep very large finite format output
parseable" currently only asserts non-NaN, finite, and sign preservation for
values formatted by format(...) and parsed by parse(...); update the test to
also assert that the parsed value is within an acceptable relative error of the
original (e.g., compute Math.abs(parsed - value) / Math.abs(value) and expect it
to be below a small threshold appropriate for the formatting options), or if
exact numeric closeness is infeasible for extreme values, add a clarifying
comment in the test explaining why only sign/finite/NaN checks are sufficient;
reference the existing usage of format and parse and the test case name when
making this change.
- Around line 21-48: The helper getLastUnitMs currently only recognizes short
unit suffixes and will throw for long-form units like "minutes" or "hours";
update getLastUnitMs to accept long-form variants by extending the switch (or
normalizing the unit) to handle "year"/"years", "month"/"months",
"week"/"weeks", "day"/"days", "hour"/"hours", "minute"/"minutes",
"second"/"seconds", and "millisecond"/"milliseconds" (returning the same
MS_PER_YEAR, MS_PER_MONTH, MS_PER_WEEK, MS_PER_DAY, MS_PER_HOUR, MS_PER_MINUTE,
MS_PER_SECOND, and 1 respectively), or alternatively normalize common plurals to
their short forms before the switch so getLastUnitMs and the constants
MS_PER_YEAR, MS_PER_MONTH, MS_PER_WEEK, MS_PER_DAY, MS_PER_HOUR, MS_PER_MINUTE,
and MS_PER_SECOND handle both short and long forms.
In `@src/lib/format.ts`:
- Around line 105-183: The carry-over logic in formatMultiPrecision is correct
but hard to follow; add a concise inline comment before the main for-loop (the
loop that builds segments using Math.floor and the special-case rounding when
segments.length === precision - 1) explaining that earlier units use floor to
take whole units while the final allowed segment uses Math.round to implement
proper rounding/carry semantics (e.g., rounding seconds can overflow into
minutes/hours), to clarify why we mix floor and round and why the backward
carry/splice step is needed.
In `@src/lib/parse.ts`:
- Around line 147-156: In the parse function, after accumulating partValue into
total and absoluteTotal (using variables total and absoluteTotal within parse
and following the loop that computes parts), add a finiteness guard: check
Number.isFinite(total) (and optionally Number.isFinite(absoluteTotal) if you
rely on it) before returning the final value and return Number.NaN when the
result is not finite; this ensures inputs like parse("1e999h") produce NaN
instead of Infinity.
adelrodriguez
force-pushed
the
02-17-add_qte_time_conversion_library_with_parsing_and_formatting
branch
3 times, most recently
from
4666c7d to
e7e5def
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/__tests__/round-trip.test.ts (1)
67-83: Merge the three near-identical clean-value precision tests.The three
"should round-trip clean values at ..."tests share identical structure and can be collapsed into a single parameterized loop.♻️ Proposed refactor
- it("should round-trip clean values at default precision", () => { - for (const val of CLEAN_VALUES) { - expect(parse(format(val))).toBe(val) - } - }) - - it("should round-trip clean values at precision 2", () => { - for (const val of CLEAN_VALUES) { - expect(parse(format(val, { precision: 2 }))).toBe(val) - } - }) - - it("should round-trip clean values at precision 3", () => { - for (const val of CLEAN_VALUES) { - expect(parse(format(val, { precision: 3 }))).toBe(val) - } - }) + for (const precision of [1, 2, 3]) { + it(`should round-trip clean values at precision ${precision}`, () => { + for (const val of CLEAN_VALUES) { + const opts = precision === 1 ? undefined : { precision } + expect(parse(format(val, opts))).toBe(val) + } + }) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/round-trip.test.ts` around lines 67 - 83, The three nearly identical tests that call format/parse over CLEAN_VALUES at different precisions should be merged into one parameterized test to avoid duplication; replace the three it(...) blocks with a single test that iterates over precisions (e.g., [undefined, 2, 3] or use test.each) and for each precision runs for (const val of CLEAN_VALUES) expect(parse(format(val, precision ? { precision } : undefined))).toBe(val); reference CLEAN_VALUES, format, and parse to locate and update the test logic in round-trip.test.ts.src/lib/format.ts (2)
171-173: Dead code —segments.length === 0is unreachable here.By the time execution reaches Line 171, the fallback at Lines 133–137 guarantees at least one element in
segments, and neither the carry-over loop nor the trailing-zeros loop (while (segments.length > 1)) can empty the array. Thesegments.length === 0branch is never entered. The check at Lines 175–177 (every(seg => seg.value === 0)) is the meaningful guard for the zero-value fallback path and should be kept.♻️ Proposed removal
- if (segments.length === 0) { - return formatSegment(0, getUnit(UNITS.length - 1), long) as TimeExpression - } - if (segments.every((segment) => segment.value === 0)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/format.ts` around lines 171 - 173, Remove the unreachable dead branch that checks `if (segments.length === 0)` and returns `formatSegment(0, getUnit(UNITS.length - 1), long)`, since the earlier fallback guarantees `segments` is never empty; instead rely on the existing zero-value guard that uses `segments.every(seg => seg.value === 0)` to handle the all-zero case. Update the `format`/`formatSegments` flow by deleting the `segments.length === 0` block and ensuring the code proceeds directly to the `every(...)` check and subsequent logic; preserve use of `formatSegment`, `getUnit`, `UNITS`, and the `long` flag as before.
140-162: Carry-over uses a non-integer ratio for month→week; the result is implicitly safe but fragile.
ratio = largerUnit.ms / unit.msequalsMS_PER_MONTH / MS_PER_WEEK ≈ 4.348, soseg.value -= carry * ratiocan produce a fractional week value (e.g.,5 - 4.348 = 0.652). With the current constants this never triggers — exhaustive analysis shows that after greedy extraction of months and weeks, the remaining is always <37,800,000 ms, so the last rounding segment rounds to at most11h < 24, preventing the chain carry into week. However, this safety guarantee is an implicit invariant of the specificMS_PER_MONTH/MS_PER_WEEKvalues and will silently break if either constant changes.♻️ Suggestions
Option A — add an explanatory comment:
+ // NOTE: ratio may be non-integer for month/week (≈4.348). Fractional seg.value is impossible + // with the current constants because greedy extraction constrains the max carry-chain so that + // week values never reach the overflow threshold. Verify this invariant if MS_PER_* constants change. const ratio = largerUnit.ms / unit.msOption B — defensively round the result:
const carry = Math.floor(seg.value / ratio) - seg.value -= carry * ratio + seg.value = Math.round(seg.value - carry * ratio)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/format.ts` around lines 140 - 162, The carry logic can produce fractional unit counts because ratio is non-integer (e.g., month→week); update the loop around segments, getUnit, ratio, carry so that after computing carry and doing seg.value -= carry * ratio you force seg.value to an integer and non-negative (e.g., seg.value = Math.max(0, Math.round(seg.value))). Also add a short comment near ratio/carry explaining why we round/clamp to defend against non-integer MS_PER_* constants. Ensure references to segments, seg.value, largerUnitIdx, ratio and carry are used so the fix is applied to the exact block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/format.ts`:
- Around line 94-103: In formatPrecisionOne, you are rounding the signed ms
value which makes rounding asymmetric for negative inputs; change the logic to
round using abs (e.g., compute roundedCount = Math.round(abs / unit.ms)), then
reapply the original sign (Math.sign(ms)) to produce a signed count before
calling formatSegment; do the same for the fallback return (use abs when
computing the unit count and then apply sign) and keep using getUnit, UNITS, and
formatSegment as the existing helpers.
---
Duplicate comments:
In `@README.md`:
- Around line 256-259: The docs say format() should throw a TypeError for
non-finite inputs; confirm and update the non-finite input check inside the
format function (the code path that currently does the "non-finite" or
"isFinite" validation) to throw new TypeError(...) instead of throw new
Error(...), and add/adjust tests if present that assert TypeError is thrown for
Infinity, -Infinity, and NaN to match the README.
- Around line 185-188: Update the incorrect numeric example for parse("1 year 2
weeks 5 days") in the README: replace the hardcoded comment 32_594_400_000 with
the correct value 33_199_200_000 (calculated as 31_557_600_000 + 2 × 604_800_000
+ 5 × 86_400_000) so the example for the parse function matches the expression
used in parse.test.ts.
---
Nitpick comments:
In `@src/__tests__/round-trip.test.ts`:
- Around line 67-83: The three nearly identical tests that call format/parse
over CLEAN_VALUES at different precisions should be merged into one
parameterized test to avoid duplication; replace the three it(...) blocks with a
single test that iterates over precisions (e.g., [undefined, 2, 3] or use
test.each) and for each precision runs for (const val of CLEAN_VALUES)
expect(parse(format(val, precision ? { precision } : undefined))).toBe(val);
reference CLEAN_VALUES, format, and parse to locate and update the test logic in
round-trip.test.ts.
In `@src/lib/format.ts`:
- Around line 171-173: Remove the unreachable dead branch that checks `if
(segments.length === 0)` and returns `formatSegment(0, getUnit(UNITS.length -
1), long)`, since the earlier fallback guarantees `segments` is never empty;
instead rely on the existing zero-value guard that uses `segments.every(seg =>
seg.value === 0)` to handle the all-zero case. Update the
`format`/`formatSegments` flow by deleting the `segments.length === 0` block and
ensuring the code proceeds directly to the `every(...)` check and subsequent
logic; preserve use of `formatSegment`, `getUnit`, `UNITS`, and the `long` flag
as before.
- Around line 140-162: The carry logic can produce fractional unit counts
because ratio is non-integer (e.g., month→week); update the loop around
segments, getUnit, ratio, carry so that after computing carry and doing
seg.value -= carry * ratio you force seg.value to an integer and non-negative
(e.g., seg.value = Math.max(0, Math.round(seg.value))). Also add a short comment
near ratio/carry explaining why we round/clamp to defend against non-integer
MS_PER_* constants. Ensure references to segments, seg.value, largerUnitIdx,
ratio and carry are used so the fix is applied to the exact block.
Owner
Author
|
@greptileai review |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/lib/format.ts (1)
145-147: Dead code:segments.length === 0is unreachable after trailing-zero trimming.The
whileloop at Line 139 preserves at least one segment (segments.length > 1guard), and the fallback at Lines 107–111 ensures at least one segment is always pushed. This branch can never execute.The check at Lines 149–151 (
every(…value === 0)) already handles the "all zeroes" scenario, so lines 145–147 are redundant.♻️ Suggested removal
- if (segments.length === 0) { - return formatSegment(0, getUnit(UNITS.length - 1), long) as TimeExpression - } - if (segments.every((segment) => segment.value === 0)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/format.ts` around lines 145 - 147, The branch checking segments.length === 0 is unreachable and should be removed: delete the if-block that returns formatSegment(0, getUnit(UNITS.length - 1), long) and rely on the existing trailing-zero trimming and the subsequent every(... value === 0) handling; update any comments if present around the segments array logic (refer to the segments variable, formatSegment, getUnit, UNITS, and the long parameter) so the code no longer contains this dead branch.src/lib/parsers.ts (1)
157-272: Unit conversion helpers propagate NaN correctly, but consider documenting it.When
parse()returnsNaN, the divisionNaN / MS_PER_*correctly yieldsNaN— no silent data corruption. However, the JSDoc for each helper says it returns "The value in X" without noting thatNaNis a possible return value for unparseable input. Consumers may not expect this.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/parsers.ts` around lines 157 - 272, Update the JSDoc for the time conversion helpers (ms, seconds, minutes, hours, days, weeks, months, years) to explicitly state that when parse(value) cannot interpret the input it returns NaN and therefore these helpers will return NaN for unparseable inputs; mention this in the `@returns` description (e.g., "The value in X, or NaN if the input cannot be parsed") and keep the behavior unchanged (they should continue delegating to parse()).src/index.ts (1)
1-27: LGTM — clean, explicit barrel with full tree-shaking support.All symbols referenced in the README are present. No wildcard re-exports, so the public surface is fully intentional.
One minor doc gap:
SimpleTimeExpression(line 21) is part of the public API but not mentioned in the README's Types section. Worth a one-liner there so users know it exists.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 1 - 27, Add a one-line mention of the exported type SimpleTimeExpression (referenced in src/index.ts's type export list) to the README Types section so it appears in the public API docs; update the README Types list to include "SimpleTimeExpression — brief description of what it represents (e.g., a minimal/compact form of TimeExpression)" so users can discover this exported type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/review.md:
- Line 15: Remove the machine-specific absolute path entry in the .agents review
artifact: locate the literal reference to the test artifact (the
"round-trip.test.ts:133" string) inside .agents/review.md and replace it with a
relative or generic path, redact the username, or remove the file entirely if
it’s an AI-generated artifact; then add the .agents/ directory to .gitignore to
prevent committing similar local artifacts in the future.
In `@knip.json`:
- Around line 26-29: The two glob patterns for .context are inconsistent:
ignoreFiles contains "**/.context/**" (any depth) while ignore contains
".context/**" (root-anchored); update the ignore entry to use the same depth so
nested .context dirs aren't missed — change the ".context/**" pattern in the
"ignore" array to "**/.context/**" (ensure both "ignoreFiles" and "ignore" use
"**/.context/**").
---
Duplicate comments:
In `@README.md`:
- Line 187: The README example for parse("1 year 2 weeks 5 days") has an
incorrect expected value (32_594_400_000); update the example to the correct
computed value 33_199_200_000 so it matches the actual parse behavior and the
test's inline computation; locate the parse("1 year 2 weeks 5 days") example in
README.md and replace the numeric literal after the comment with 33_199_200_000.
- Around line 253-259: The README claims format() throws a TypeError for
non-finite inputs but the implementation may still throw a generic Error; update
the format function in src/lib/format.ts so that the non-finite-number check
throws a TypeError (not Error) and ensure the validation for precision still
throws a RangeError for non-positive/ non-integer/ non-finite precision values;
search for throw new in format.ts and replace the generic Error thrown for the
non-finite input check inside the format(number, precision) implementation with
throw new TypeError(...) while leaving the other RangeError throws intact.
In `@src/lib/format.ts`:
- Around line 68-77: In formatPrecisionOne, the current Math.round(ms / unit.ms)
performs asymmetric rounding for negative values; change it to round the
absolute value then reapply the original sign (e.g., const rounded =
Math.round(abs / unit.ms) * Math.sign(ms)) before passing to formatSegment so
formatPrecisionOne, UNITS, formatSegment and getUnit produce symmetric rounding
for positive and negative half-unit boundaries.
---
Nitpick comments:
In `@src/index.ts`:
- Around line 1-27: Add a one-line mention of the exported type
SimpleTimeExpression (referenced in src/index.ts's type export list) to the
README Types section so it appears in the public API docs; update the README
Types list to include "SimpleTimeExpression — brief description of what it
represents (e.g., a minimal/compact form of TimeExpression)" so users can
discover this exported type.
In `@src/lib/format.ts`:
- Around line 145-147: The branch checking segments.length === 0 is unreachable
and should be removed: delete the if-block that returns formatSegment(0,
getUnit(UNITS.length - 1), long) and rely on the existing trailing-zero trimming
and the subsequent every(... value === 0) handling; update any comments if
present around the segments array logic (refer to the segments variable,
formatSegment, getUnit, UNITS, and the long parameter) so the code no longer
contains this dead branch.
In `@src/lib/parsers.ts`:
- Around line 157-272: Update the JSDoc for the time conversion helpers (ms,
seconds, minutes, hours, days, weeks, months, years) to explicitly state that
when parse(value) cannot interpret the input it returns NaN and therefore these
helpers will return NaN for unparseable inputs; mention this in the `@returns`
description (e.g., "The value in X, or NaN if the input cannot be parsed") and
keep the behavior unchanged (they should continue delegating to parse()).
.agents/review.md
Outdated
|
|
||
| ### 1. Low: Large-value round-trip test checks parseability and sign, but not numeric error bounds | ||
|
|
||
| - File: `/Users/adelrodriguez/Developer/qte/src/__tests__/round-trip.test.ts:133` |
There was a problem hiding this comment.
Remove machine-specific absolute paths from version-controlled file.
The file references absolute local filesystem paths (e.g., /Users/adelrodriguez/Developer/qte/...) that are meaningless to other contributors and leak the developer's machine username. This file looks like an AI review artifact that shouldn't be committed; consider adding .agents/ to .gitignore.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/review.md at line 15, Remove the machine-specific absolute path
entry in the .agents review artifact: locate the literal reference to the test
artifact (the "round-trip.test.ts:133" string) inside .agents/review.md and
replace it with a relative or generic path, redact the username, or remove the
file entirely if it’s an AI-generated artifact; then add the .agents/ directory
to .gitignore to prevent committing similar local artifacts in the future.
Comment on lines
+26
to
+29
| "**/.turbo/**", | ||
| "**/.context/**" | ||
| ], | ||
| "ignore": ["**/*.d.ts"], | ||
| "ignore": ["**/*.d.ts", ".context/**"], |
There was a problem hiding this comment.
Inconsistent glob depth between ignoreFiles and ignore for .context.
ignoreFiles uses "**/.context/**" (matches at any directory depth), while ignore uses ".context/**" (anchored to the project root). If .context directories only ever live at the repo root this is harmless, but the two entries should be consistent to avoid silently missing nested occurrences.
🔧 Proposed fix
- "ignore": ["**/*.d.ts", ".context/**"],
+ "ignore": ["**/*.d.ts", "**/.context/**"],📝 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.
Suggested change
| "**/.turbo/**", | |
| "**/.context/**" | |
| ], | |
| "ignore": ["**/*.d.ts"], | |
| "ignore": ["**/*.d.ts", ".context/**"], | |
| "**/.turbo/**", | |
| "**/.context/**" | |
| ], | |
| "ignore": ["**/*.d.ts", "**/.context/**"], |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@knip.json` around lines 26 - 29, The two glob patterns for .context are
inconsistent: ignoreFiles contains "**/.context/**" (any depth) while ignore
contains ".context/**" (root-anchored); update the ignore entry to use the same
depth so nested .context dirs aren't missed — change the ".context/**" pattern
in the "ignore" array to "**/.context/**" (ensure both "ignoreFiles" and
"ignore" use "**/.context/**").
cb33f15 to
c8a8f8a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
59-59:⚠️ Potential issue | 🟡 MinorPinned nightly dev build of
@typescript/native-previewis a CI reliability risk.
"7.0.0-dev.20260108.1"is a date-stamped pre-release of the TypeScript Go rewrite (Project Corsa). Exact nightly builds can be removed from npm, silently breaking contributor installs and CI pipelines. Consider either replacing it withtypescript@next(the rolling nightly channel) or removing it until a stable beta is available.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 59, The package.json pins a date-stamped pre-release "@typescript/native-preview": "7.0.0-dev.20260108.1", which is fragile; update package.json to avoid the nightly-specific version by either replacing the dependency with the rolling channel (e.g., use the canonical "typescript@next" package) or remove the "@typescript/native-preview" entry until a stable beta exists; ensure any scripts or imports that reference "@typescript/native-preview" continue to work or are updated to reference "typescript" accordingly.
🧹 Nitpick comments (3)
src/lib/format.ts (1)
152-154:segments.length === 0check is unreachable dead code.After the trailing-zero while loop (
while (segments.length > 1)),segments.lengthis always>= 1:
- The fallback at lines 114–118 guarantees at least one element is pushed before the while loop.
- The while loop itself only pops while
length > 1, flooring at 1.The all-zero guard at line 156 (
segments.every(s => s.value === 0)) is the one that actually fires (e.g.format(0.1, { precision: 2 })) and should be retained.♻️ Proposed cleanup
- if (segments.length === 0) { - return formatSegment(0, getUnit(UNITS.length - 1), long) - } - if (segments.every((segment) => segment.value === 0)) { return formatSegment(0, getUnit(UNITS.length - 1), long) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/format.ts` around lines 152 - 154, The if-block that returns formatSegment(0, getUnit(UNITS.length - 1), long) when segments.length === 0 is unreachable and should be removed; instead keep the existing all-zero guard (segments.every(s => s.value === 0)) which handles cases like format(0.1, { precision: 2 }), and ensure the trailing-zero while loop that reduces segments (while (segments.length > 1)) and the fallback that initially pushes at least one segment remain unchanged; remove only the unreachable segments.length === 0 branch and run tests to confirm behavior.src/lib/types.ts (1)
47-52:precisionJSDoc omits the "positive integer" constraint validated at runtime.The
format()implementation throwsRangeErrorfor non-integer or< 1precision values. The JSDoc only says "Maximum number of unit segments to include" — adding the constraint avoids surprises for callers.♻️ Proposed fix
- /** Maximum number of unit segments to include. Defaults to `1`. */ + /** Maximum number of unit segments to include. Must be a finite positive integer. Defaults to `1`. */ precision?: number🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/types.ts` around lines 47 - 52, Update the JSDoc for the FormatOptions.precision property to state it must be a positive integer (>=1) to match runtime validation in format(), so callers know inputs that would cause format() to throw a RangeError; reference the FormatOptions interface and its precision field and mention format() as the function enforcing the constraint.src/lib/constants.ts (1)
1-2:MS_PER_SECONDis missing the explicit: numberannotation present on all other constants.Without it, TypeScript infers the narrower literal type
1000instead ofnumber, which is inconsistent with the rest of the file.♻️ Proposed fix
-export const MS_PER_SECOND = 1000 +export const MS_PER_SECOND: number = 1000🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/constants.ts` around lines 1 - 2, MS_PER_SECOND currently lacks an explicit type annotation and is inferred as the numeric literal 1000; update the declaration for the exported constant MS_PER_SECOND to include an explicit : number type (match the style used by other constants) so it reads as a number-typed export and avoids the narrowed literal type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/fuzzy-places-wish.md:
- Line 5: Update the changeset description to include the exported symbol
isCompoundTimeExpression alongside isTimeExpression so it appears in the
changelog; edit the sentence on line 5 to list both isTimeExpression and
isCompoundTimeExpression (matching the export from src/index.ts) in the summary
of public API additions.
---
Outside diff comments:
In `@package.json`:
- Line 59: The package.json pins a date-stamped pre-release
"@typescript/native-preview": "7.0.0-dev.20260108.1", which is fragile; update
package.json to avoid the nightly-specific version by either replacing the
dependency with the rolling channel (e.g., use the canonical "typescript@next"
package) or remove the "@typescript/native-preview" entry until a stable beta
exists; ensure any scripts or imports that reference
"@typescript/native-preview" continue to work or are updated to reference
"typescript" accordingly.
---
Duplicate comments:
In @.agents/review.md:
- Line 15: Replace the machine-specific absolute path string found in
round-trip.test.ts (the `/Users/adelrodriguez/...` occurrence) with a redacted
or environment-agnostic value and/or add the `.agents/` directory to .gitignore;
locate the literal absolute path in the test file (round-trip.test.ts) and
either change it to a relative path or a placeholder like "[REDACTED_PATH]" or
generate the path at runtime (e.g., using path.basename/process.cwd) and commit
the .gitignore change if the value lives under `.agents/`.
In `@README.md`:
- Line 198: The README example for the parse function has an incorrect inline
expected value; update the comment after parse("1 year 2 weeks 5 days") to the
correct computed total (33_199_200_000) or replace it with the expanded
expression (31_557_600_000 + 2 × 604_800_000 + 5 × 86_400_000 = 33_199_200_000)
so the example matches the actual output produced by the parse function and the
test in src/__tests__/parse.test.ts.
---
Nitpick comments:
In `@src/lib/constants.ts`:
- Around line 1-2: MS_PER_SECOND currently lacks an explicit type annotation and
is inferred as the numeric literal 1000; update the declaration for the exported
constant MS_PER_SECOND to include an explicit : number type (match the style
used by other constants) so it reads as a number-typed export and avoids the
narrowed literal type.
In `@src/lib/format.ts`:
- Around line 152-154: The if-block that returns formatSegment(0,
getUnit(UNITS.length - 1), long) when segments.length === 0 is unreachable and
should be removed; instead keep the existing all-zero guard (segments.every(s =>
s.value === 0)) which handles cases like format(0.1, { precision: 2 }), and
ensure the trailing-zero while loop that reduces segments (while
(segments.length > 1)) and the fallback that initially pushes at least one
segment remain unchanged; remove only the unreachable segments.length === 0
branch and run tests to confirm behavior.
In `@src/lib/types.ts`:
- Around line 47-52: Update the JSDoc for the FormatOptions.precision property
to state it must be a positive integer (>=1) to match runtime validation in
format(), so callers know inputs that would cause format() to throw a
RangeError; reference the FormatOptions interface and its precision field and
mention format() as the function enforcing the constraint.
adelrodriguez
force-pushed
the
02-17-add_qte_time_conversion_library_with_parsing_and_formatting
branch
from
c8a8f8a to
de51ec7
Compare
adelrodriguez
deleted the
02-17-add_qte_time_conversion_library_with_parsing_and_formatting
branch
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.
TL;DR
Implement the QTE (Quick Time Expressions) library for parsing and formatting human-readable time expressions.
What changed?
parse()function to convert expressions to millisecondsformat()function to convert milliseconds to human-readable stringsms,seconds,minutes,hours,days,weeks,months,years)isTimeExpression()type guard for validationTimeExpressionbranded type"1h 30m"or"1 day, 6 hours"How to test?
Why make this change?
This library provides a more powerful alternative to existing time parsing libraries like Vercel's
ms. Key improvements include:The implementation follows a clean, modular design with thorough test coverage and documentation.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores