Skip to content

fix(core): sum repeated units in parseNaturalLanguageDurationInMs - #4373

Closed
eeshsaxena wants to merge 1 commit into
triggerdotdev:mainfrom
eeshsaxena:fix/duration-repeated-units
Closed

fix(core): sum repeated units in parseNaturalLanguageDurationInMs#4373
eeshsaxena wants to merge 1 commit into
triggerdotdev:mainfrom
eeshsaxena:fix/duration-repeated-units

Conversation

@eeshsaxena

Copy link
Copy Markdown

parseNaturalLanguageDurationInMs silently drops repeated units.

Problem

The valid-input check is /^(\d+(?:w|d|hr|h|m|s))+$/, whose + accepts a unit appearing more than once. But each unit was then read with a single .match(), so only the first occurrence counted:

parseNaturalLanguageDurationInMs("1h2h") // 3600000  (1h) - the 2h is dropped
parseNaturalLanguageDurationInMs("2m3m") // 120000   (2m) - the 3m is dropped

So the validation and the parsing disagree: a string the function accepts as valid is parsed to the wrong number.

Fix

Tokenize the whole string and sum every match. hr is matched before h so "1hr" stays one hour. All existing single-occurrence and combined-unit behaviour is unchanged; repeated units now add up. As a side effect this replaces five near-identical unit blocks with one loop.

If you would rather reject duplicates than sum them, that is a one-line change to the pattern instead - happy to switch. Summing seemed the least surprising (it is how "1h30m" already combines units).

Tests

The full duration.test.ts suite passes (56 tests). Added cases for the repeated-unit sum; I confirmed the new "sums a unit that appears more than once" case fails against the current code and passes with the fix. Included a changeset (@trigger.dev/core patch).

The valid-input pattern is `/^(\d+(?:w|d|hr|h|m|s))+$/`, which accepts a
unit appearing more than once (e.g. "1h2h"). But each unit was read with a
single `.match()`, so only the first occurrence of each was counted and the
rest were silently dropped - "1h2h" returned 1h.

Tokenize the whole string and sum every match instead. All existing
single-occurrence and multi-unit behaviour is unchanged (verified against the
existing suite); repeated units now add up. This also collapses five
near-identical unit blocks into one loop.

Adds tests for the repeated-unit case.
@changeset-bot

changeset-bot Bot commented Jul 25, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 29cfa42

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 26 packages
Name Type
@trigger.dev/core Patch
@trigger.dev/build Patch
trigger.dev Patch
@trigger.dev/python Patch
@trigger.dev/redis-worker Patch
@trigger.dev/schema-to-json Patch
@trigger.dev/sdk Patch
@internal/cache Patch
@internal/clickhouse Patch
@internal/llm-model-catalog Patch
@trigger.dev/rbac Patch
@internal/redis Patch
@internal/replication Patch
@internal/run-engine Patch
@internal/run-store Patch
@internal/schedule-engine Patch
@trigger.dev/sso Patch
@internal/testcontainers Patch
@internal/tracing Patch
@internal/tsql Patch
@internal/dashboard-agent Patch
@internal/sdk-compat-tests Patch
@trigger.dev/react-hooks Patch
@trigger.dev/rsc Patch
@trigger.dev/database Patch
@trigger.dev/otlp-importer Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@coderabbitai

coderabbitai Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 1badc47d-7283-4c26-ab89-2ea2170a1fc7

📥 Commits

Reviewing files that changed from the base of the PR and between be45cf9 and 29cfa42.

📒 Files selected for processing (3)
  • .changeset/duration-repeated-units.md
  • packages/core/src/v3/isomorphic/duration.ts
  • packages/core/test/duration.test.ts

Walkthrough

Updated parseNaturalLanguageDurationInMs to tokenize all duration unit occurrences and sum their millisecond values, including repeated units. Added tests for single, combined, repeated, aliased, empty, and invalid inputs. Added a patch changeset documenting the parser fix.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

Hi @eeshsaxena, thanks for your interest in contributing!

This project requires that pull request authors are vouched, and you are not in the list of vouched users.

This PR will be closed automatically. See https://github.com/triggerdotdev/trigger.dev/blob/main/CONTRIBUTING.md for more details.

@github-actions github-actions Bot closed this Jul 25, 2026

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

Open in Devin Review

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Identical repeated-unit bug remains in parseNaturalLanguageDurationAgo

This PR fixes the repeated-unit summing bug only in parseNaturalLanguageDurationInMs. The sibling function parseNaturalLanguageDurationAgo (packages/core/src/v3/isomorphic/duration.ts:91-128) still uses the old single-.match() approach for each unit, so it retains the exact same bug: a validated input like "1h2h" will drop all but the first occurrence of each repeated unit. Since this function was not touched by the diff (context-only lines, no +/-), it cannot be reported as a diff bug, but the reviewer may want to apply the same fix there for consistency.

(Refers to lines 91-128)

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant