Feature/fetch#54
Conversation
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR updates Tempo’s request helpers and shared utilities, adds shorthand unit support across duration, mutation, and type definitions, renames the plugin factory API to ChangesLibrary Request Helper Rewrite
Tempo Shorthand Units and Epoch Handling
Plugin API Rename
Package Version Bumps
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
![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
🧹 Nitpick comments (4)
packages/library/src/common/request.library.ts (2)
68-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
fetchRequestandfetchHeadthrow inconsistent error types.
fetchRequestnow throws a structuredHttpError(status/statusText/body), butfetchHeadstill throws a plainErrorwith just the status/statusText message. Since both are part of the same public "request" surface and re-exported together, callers can't uniformlycatch/instanceof-check failures across the two helpers.Consider having
fetchHeadalso throwHttpErrorfor consistency.Also applies to: 82-93
🤖 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 `@packages/library/src/common/request.library.ts` around lines 68 - 76, `fetchRequest` and `fetchHead` are throwing different error shapes, so update `fetchHead` to use the same `HttpError` structure as `fetchRequest`. In the request helpers in `request.library.ts`, keep the existing status/statusText/body handling consistent by constructing `HttpError` from the `fetchHead` failure path instead of a plain `Error`, so callers can reliably `instanceof`-check both functions’ failures across the shared request API.
50-66: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winJSON detection misses
+jsonstructured-syntax suffixes.
contentType.includes('application/json')won't match common JSON media types likeapplication/problem+json,application/vnd.api+json,application/ld+json, etc. (the+jsonsuffix is a registered structured syntax suffix per RFC 6839). Responses using these content types will now be returned as raw strings viares.text()instead of parsed objects, silently changing behavior for callers expecting a parsed object.🔧 Proposed fix
- const isJson = contentType.includes('application/json'); + const isJson = contentType.includes('application/json') || contentType.includes('+json');🤖 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 `@packages/library/src/common/request.library.ts` around lines 50 - 66, Update the JSON handling in request.library.ts inside the response parsing logic so it recognizes structured-syntax JSON media types, not just application/json. In the res.ok branch where contentType and isJson are computed, adjust the detection used by the config.prefix-free path and the default res.json()/res.text() selection to treat any Content-Type ending in +json (for example application/problem+json or application/vnd.api+json) as JSON. Keep the existing raw-prefix unwrapping behavior in the same request parsing flow and preserve the current symbols res.ok, config.prefix, and res.headers.get('Content-Type') as the entry points for the fix.packages/tempo/src/module/module.duration.ts (1)
84-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate duplicated shorthand-unit normalization logic.
The
ww→weeks/enums.ELEMENT→ plural-name mapping is duplicated near-verbatim in three places:balance()(Lines 84-88),duration()(Lines 185-190), andtoDuration.toDurationobject remapping (Lines 260-272). Any future shorthand-key addition or bugfix must be applied in all three spots, which is easy to miss (e.g.module.mutate.tsalso has its own separateunitMapfor the same shorthand keys).♻️ Suggested refactor — extract a shared helper
function resolveElementUnit(key: string): string | undefined { if (key === 'ww') return 'weeks'; if (key in enums.ELEMENT) return `${enums.ELEMENT[key as t.Element]}s`; return undefined; }Then each call-site applies its own fallback:
- let temporalUnit = largestUnit; - if (temporalUnit === 'ww') temporalUnit = 'weeks'; - else if (temporalUnit in enums.ELEMENT) temporalUnit = `${enums.ELEMENT[temporalUnit as t.Element]}s`; + const temporalUnit = resolveElementUnit(largestUnit) ?? largestUnit;- let temporalUnit = unit; - if (isDefined(temporalUnit)) { - if (temporalUnit === 'ww') temporalUnit = 'weeks'; - else if (temporalUnit in enums.ELEMENT) temporalUnit = `${enums.ELEMENT[temporalUnit as t.Element]}s`; - else temporalUnit = `${singular(temporalUnit)}s`; - } + let temporalUnit = unit; + if (isDefined(temporalUnit)) + temporalUnit = resolveElementUnit(temporalUnit) ?? `${singular(temporalUnit)}s`;- if (k === 'ww') { mappedInput['weeks'] = v; delete mappedInput[k]; } - else if (k in enums.ELEMENT) { - mappedInput[`${enums.ELEMENT[k as t.Element]}s`] = v; - delete mappedInput[k]; - } + const mapped = resolveElementUnit(k); + if (mapped) { mappedInput[mapped] = v; delete mappedInput[k]; }Also applies to: 185-196, 260-272
🤖 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 `@packages/tempo/src/module/module.duration.ts` around lines 84 - 88, The shorthand unit normalization logic is duplicated across balance(), duration(), and toDuration.toDuration, so extract a shared helper (for example, a resolver like resolveElementUnit) that maps ww to weeks and enums.ELEMENT keys to plural names, then have each call site use it with its own fallback behavior. Update the existing unit normalization branches in those methods to call the shared helper so future shorthand additions or fixes only need one change.packages/tempo/src/tempo.type.ts (1)
134-153: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse existing numeric range aliases instead of generic
numberfor shorthand fields.
MutateShorthand.yy/mm/dd/hh/mi/ssare typed asstring | number, but the file already exports stricter bounded aliases (dd = IntRange<1, 31>at Line 159, and likely similarhh/mi/ssaliases referenced fromtempo.class.ts). Using plainnumberhere forfeits compile-time protection against out-of-range literals (e.g.{ dd: 99 }would type-check).♻️ Suggested refactor
export type MutateShorthand = { yy?: string | number; - mm?: string | number; + mm?: string | mm; ww?: string; - dd?: string | number; - hh?: string | number; - mi?: string | number; - ss?: string | number; + dd?: string | dd; + hh?: string | hh; + mi?: string | mi; + ss?: string | ss; ms?: number; us?: number; ns?: number; wkd?: string; };Note: the existing
mm = IntRange<0, 12>(Line 158) differs from thereadonly mm: IntRange<1, 12>field defined earlier (Line 62); worth reconciling before reusing it here.🤖 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 `@packages/tempo/src/tempo.type.ts` around lines 134 - 153, MutateShorthand currently uses broad string | number types for yy/mm/dd/hh/mi/ss, which bypasses the stricter bounded aliases already defined in this file and elsewhere. Update MutateShorthand to reuse the existing numeric range aliases (for example dd, and the corresponding hh/mi/ss aliases from tempo.class.ts) instead of plain number, and reconcile the mm alias mismatch before wiring it into MutateSet so shorthand mutation fields keep compile-time range checking.
🤖 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 `@packages/library/src/common/assertion.library.ts`:
- Around line 78-80: Normalize shorthand duration keys before the Temporal.add
path by updating Tempo’s `#swap` and/or `#isOptions` flow so shorthand-only objects
accepted by isDurationLike are remapped to Temporal-compatible duration fields
before add is called. Use the existing shorthand mapping behavior from
module.duration and module.mutate as the reference, and ensure new Tempo({ ww: 1
}) or new Tempo({ mi: 5 }) is converted consistently instead of passing
non-Temporal keys through unchanged.
In `@packages/tempo/src/library.index.ts`:
- Line 14: `library.index.ts` currently exposes `fetchRequest` and `fetchHead`
without the `HttpError` type/class that `fetchRequest` can throw, so update this
barrel to re-export `HttpError` as well. Add the missing export alongside the
existing `#library/request.library.js` re-exports so consumers of
`@magmacomputing/tempo` can import and catch `HttpError` from the curated entry
point instead of reaching into `@magmacomputing/library`.
In `@packages/tempo/src/plugin/plugin.util.ts`:
- Around line 175-181: Restore backward compatibility by adding a deprecated
defineExtension alias alongside definePlugin in plugin.util.ts, since
plugin.index.ts re-exports this module and consumers of
`@magmacomputing/tempo/plugin` still expect defineExtension to exist. Keep
definePlugin as the primary implementation, have defineExtension delegate to it
without changing behavior, and mark the alias as deprecated in its documentation
so existing imports continue working on 3.6.0.
---
Nitpick comments:
In `@packages/library/src/common/request.library.ts`:
- Around line 68-76: `fetchRequest` and `fetchHead` are throwing different error
shapes, so update `fetchHead` to use the same `HttpError` structure as
`fetchRequest`. In the request helpers in `request.library.ts`, keep the
existing status/statusText/body handling consistent by constructing `HttpError`
from the `fetchHead` failure path instead of a plain `Error`, so callers can
reliably `instanceof`-check both functions’ failures across the shared request
API.
- Around line 50-66: Update the JSON handling in request.library.ts inside the
response parsing logic so it recognizes structured-syntax JSON media types, not
just application/json. In the res.ok branch where contentType and isJson are
computed, adjust the detection used by the config.prefix-free path and the
default res.json()/res.text() selection to treat any Content-Type ending in
+json (for example application/problem+json or application/vnd.api+json) as
JSON. Keep the existing raw-prefix unwrapping behavior in the same request
parsing flow and preserve the current symbols res.ok, config.prefix, and
res.headers.get('Content-Type') as the entry points for the fix.
In `@packages/tempo/src/module/module.duration.ts`:
- Around line 84-88: The shorthand unit normalization logic is duplicated across
balance(), duration(), and toDuration.toDuration, so extract a shared helper
(for example, a resolver like resolveElementUnit) that maps ww to weeks and
enums.ELEMENT keys to plural names, then have each call site use it with its own
fallback behavior. Update the existing unit normalization branches in those
methods to call the shared helper so future shorthand additions or fixes only
need one change.
In `@packages/tempo/src/tempo.type.ts`:
- Around line 134-153: MutateShorthand currently uses broad string | number
types for yy/mm/dd/hh/mi/ss, which bypasses the stricter bounded aliases already
defined in this file and elsewhere. Update MutateShorthand to reuse the existing
numeric range aliases (for example dd, and the corresponding hh/mi/ss aliases
from tempo.class.ts) instead of plain number, and reconcile the mm alias
mismatch before wiring it into MutateSet so shorthand mutation fields keep
compile-time range checking.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 983064ea-f60e-4627-8299-8d68510a489d
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (29)
CHANGELOG.mdpackage.jsonpackages/library/package.jsonpackages/library/src/common.index.tspackages/library/src/common/assertion.library.tspackages/library/src/common/request.library.tspackages/library/src/server.index.tspackages/library/test/common/request.library.test.tspackages/library/test/common/temporal_library.test.tspackages/tempo/CHANGELOG.mdpackages/tempo/doc/ecosystem.mdpackages/tempo/doc/releases/v3.x.mdpackages/tempo/doc/tempo.extension.mdpackages/tempo/doc/tempo.plugin.mdpackages/tempo/doc/tempo.term.mdpackages/tempo/package.jsonpackages/tempo/src/library.index.tspackages/tempo/src/module/module.duration.tspackages/tempo/src/module/module.mutate.tspackages/tempo/src/plugin/plugin.type.tspackages/tempo/src/plugin/plugin.util.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.type.tspackages/tempo/src/tempo.version.tspackages/tempo/test/instance/instance.add.test.tspackages/tempo/test/instance/instance.set.test.tspackages/tempo/test/instance/instance.since.test.tspackages/tempo/test/instance/instance.until.test.tspackages/tempo/test/plugins/plugin_registration.test.ts
💤 Files with no reviewable changes (1)
- packages/library/src/server.index.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/tempo/src/tempo.type.ts`:
- Around line 134-151: `MutateShorthand` is missing the supported `wy` week key,
so update that type to expose `wy` alongside `ww` using the existing
`LooseUnion<wy>` pattern already used for the other shorthand fields. Keep the
change localized in `MutateShorthand` so `MutateSet` automatically picks up the
new key through its existing intersection.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 044aa76e-0570-44f0-a85a-7050c45d72a1
📒 Files selected for processing (12)
packages/library/src/common/request.library.tspackages/library/src/common/type.library.tspackages/tempo/CHANGELOG.mdpackages/tempo/src/engine/engine.lexer.tspackages/tempo/src/engine/engine.pattern.tspackages/tempo/src/library.index.tspackages/tempo/src/module/module.duration.tspackages/tempo/src/module/module.mutate.tspackages/tempo/src/plugin/plugin.util.tspackages/tempo/src/support/support.enum.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.type.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/tempo/src/library.index.ts
- packages/tempo/src/tempo.class.ts
- packages/library/src/common/request.library.ts
- packages/tempo/src/module/module.mutate.ts
1 participant
Summary by CodeRabbit
New Features
Tempo.now()to be unit-aware, and added theTempo.epochgetter.definePluginregistration entry point (withdefineExtensionmarked deprecated).Bug Fixes
HttpError, better prefix/plain-text vs JSON parsing, and consistentfetchHeadfailure behavior.Chores