PR publish plugins#61
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis release adds the ChangesFunctions package
Plugin tooling
Format typing
Documentation pipeline
CI and release metadata
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant package_json
participant repl_mts
participant plugin_packages
participant repl_context
package_json->>repl_mts: start REPL with Temporal preload
repl_mts->>plugin_packages: scan compiled plugin entrypoints
plugin_packages-->>repl_mts: return matching plugin exports
repl_mts->>repl_context: inject Tempo, enums, and plugin globals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/plugins/tsup.shared.ts (1)
111-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the comment to reflect the property rename.
The property checked is now
plan, but the comment still refers topricing tier.📝 Proposed fix
- // If the package explicitly declares the 'community' pricing tier, skip the license wrapper + // If the package explicitly declares the 'community' plan, skip the license wrapper🤖 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/plugins/tsup.shared.ts` around lines 111 - 114, Update the comment above the plan === 'community' check to refer to the package’s explicitly declared plan property rather than the pricing tier, keeping the existing behavior unchanged.
🤖 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 `@package.json`:
- Line 26: Update the package.json repl:plugins script to invoke the actual
packages/plugins/.bin/repl.mts entrypoint with the tsx runner, matching the new
TypeScript REPL file and noEmit configuration.
In `@vitest.config.ts`:
- Around line 64-65: Update both alias entries in vitest.config.ts and
packages/plugins/vitest.shared.ts at the specified ranges: change the
`#tempo/plugins/`... patterns to the singular `#tempo/plugin/`... form and update
their replacement targets from packages/tempo/src/plugins/... to
packages/tempo/src/plugin/..., preserving the existing util/type and wildcard
mappings.
---
Nitpick comments:
In `@packages/plugins/tsup.shared.ts`:
- Around line 111-114: Update the comment above the plan === 'community' check
to refer to the package’s explicitly declared plan property rather than the
pricing tier, keeping the existing behavior unchanged.
🪄 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: 06b08e52-fddd-460a-843c-2e0d98e2410f
📒 Files selected for processing (15)
package.jsonpackages/plugins/.bin/README.mdpackages/plugins/.bin/repl.mtspackages/plugins/.bin/temporal-polyfill.mtspackages/plugins/.bin/tsconfig.jsonpackages/plugins/astro/package.jsonpackages/plugins/batch/package.jsonpackages/plugins/finance/package.jsonpackages/plugins/snap/package.jsonpackages/plugins/sync/package.jsonpackages/plugins/tsup.shared.tspackages/plugins/vitest.shared.tspackages/tempo/.vitepress/theme/components/CatalogList.vuepackages/tempo/vitest.config.tsvitest.config.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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 @.github/workflows/ci.yml:
- Line 21: Disable checkout credential persistence for all four
actions/checkout@v4 steps by adding a with configuration with
persist-credentials set to false at .github/workflows/ci.yml lines 21, 37, 54,
and 71.
- Line 15: Add a top-level permissions block alongside the workflow’s top-level
configuration in ci.yml, explicitly restricting the default token to
least-privilege access, preferably disabling all permissions unless individual
jobs require specific scopes. Keep any required job-level permissions explicit
and unchanged.
- Around line 62-63: Update the CI test step in the packages/plugins
working-directory to target only the plugin workspaces explicitly, rather than
relying on npm workspace discovery from the repository root. Preserve the
existing optional-test behavior while excluding library, tempo, and functions
workspaces.
In `@packages/plugins/.app/doc/index.md`:
- Around line 46-77: Update all plugin references in the documentation examples
to use the exported TickerModule symbol consistently, replacing the truncated
TickerPlugi identifier in the import, plugins array, and Tempo.extend call.
Preserve the existing initialization examples and side-effect import behavior.
- Around line 121-126: The duplicated licensing guides incorrectly describe
revocation-check failures as console warnings. Update the documentation at
packages/plugins/.app/doc/index.md:121-126 and
packages/tempo/doc/9-plugins/.app.md:121-126 to state the implementation’s
actual debug-level logging, preserving the existing fail-open behavior.
- Around line 97-106: The frontend bundler documentation must ensure
TEMPO_LICENSE_KEY is set before Tempo initialization. Update the examples in
packages/plugins/.app/doc/index.md (lines 97-106) and
packages/tempo/doc/9-plugins/.app.md (lines 97-106) to use dynamic imports after
the globalThis.TEMPO_LICENSE_KEY assignment, or demonstrate Tempo.init({ license
}) instead of relying on static imports.
In `@packages/plugins/ticker/doc/index.md`:
- Around line 11-12: Update the License Key Guide link in the documentation text
to point to the shared, existing guide path rather than the nonexistent
ticker-relative path. Preserve the link label and surrounding content while
using the correct relative Tempo documentation destination.
In `@packages/plugins/tsup.shared.ts`:
- Around line 40-52: The resolver plugin ordering currently lets esm-external
short-circuit plugin imports before wrapper processing. Move the esm-external
plugin definition after the wrapper resolver plugin so the version-injection and
license-alias hooks run first, while preserving its existing IIFE exemption and
ESM externalization behavior.
In `@packages/tempo/src/module/module.format.ts`:
- Around line 26-34: Remove the catch-all `format(fmt?: any, options?: any):
any` overload from the `format` declarations so invalid format strings cannot
bypass `ValidateFormat<S>` and are rejected by TypeScript. If zero-argument
calls must remain supported, replace it with an explicit parameterless
`format(): string | number | bigint` overload while preserving the validated
generic overload.
In `@packages/tempo/src/tempo.class.ts`:
- Around line 1628-1636: Revert the implementation signature of the format
method to permissive parameter types that encompass the numeric and
Intl.DateTimeFormatOptions overloads declared by the module.format.ts interface.
Preserve the existing runtime delegation through `#resolve` and interpret, while
leaving strict format validation to the interface overloads.
🪄 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: 8ca0ad03-b08f-41ab-8c3d-aae45e74c01b
📒 Files selected for processing (22)
.github/workflows/ci.ymlpackage.jsonpackages/plugins/.app/doc/index.mdpackages/plugins/.bin/repl.mtspackages/plugins/.bin/temporal-polyfill.mtspackages/plugins/finance/package.jsonpackages/plugins/ticker/doc/index.mdpackages/plugins/tsconfig.shared.jsonpackages/plugins/tsup.shared.tspackages/plugins/vitest.shared.tspackages/tempo/bin/harvest-plugins.mjspackages/tempo/doc/3-extending-tempo/tempo.extension.mdpackages/tempo/doc/8-project-and-support/commercial.mdpackages/tempo/doc/9-plugins/.app.mdpackages/tempo/doc/9-plugins/ticker.mdpackages/tempo/package.jsonpackages/tempo/plan/refactor-path-terms.mdpackages/tempo/plan/typescript-format-template.mdpackages/tempo/src/module/module.format.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.type.tsvitest.config.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/plugins/.bin/temporal-polyfill.mts
- packages/plugins/finance/package.json
- packages/plugins/vitest.shared.ts
- vitest.config.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (8)
packages/tempo/test/instance/instance.format.test.ts (1)
63-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid bypassing the new
Tempo.formatcontract in tests.The
any/stringcasts prevent these tests from checking whether supportedIntloptions and plugin tokens are accepted by the new overloads. Prefer properly typed fixtures, augmentTempoFormatTokensformyDay, or add dedicated type-level assertions while keeping the runtime calls uncast.Also applies to: 80-80, 96-97
🤖 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/test/instance/instance.format.test.ts` at line 63, Update the tests around the uncast runtime calls to use properly typed Arabic and option fixtures that exercise the new Tempo.format overloads. Remove the any/string casts, and augment TempoFormatTokens with the myDay token where needed or add dedicated type-level assertions so supported Intl options and plugin tokens are validated by the compiler.packages/library/src/common/storage.library.ts (1)
15-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRestore trailing semicolons for variable declarations.
While automatic semicolon insertion (ASI) usually handles this, it is considered best practice (and often enforced by linters/formatters) to include trailing semicolons at the end of variable declarations assigning objects or arrow functions.
♻️ Proposed fixes
For
mockStorage:- } + };For
getSafeStorage:- } + };Also applies to: 24-24
🤖 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/storage.library.ts` at line 15, Restore trailing semicolons on the variable declarations for mockStorage and getSafeStorage, including declarations assigning objects or arrow functions. Apply the same formatting at both referenced locations without changing their behavior.packages/functions/src/duration/normaliseFractionalDurations.ts (1)
15-24: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winIntermediate
Math.rounddrops precision that could cascade.Rounding
frac * factorat every level discards sub-unit remainder instead of letting it flow into the next smaller unit on the following iteration. For example{ minutes: 1.51 }yields{ minutes: 1, seconds: 31 }(round(30.6)), losing ~600ms that should have cascaded tomilliseconds. Since eachsmallunit is re-examined as the nextbig, adding the un-rounded contribution for non-terminal units (and rounding only the terminalnanoseconds) preserves accuracy.🤖 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/functions/src/duration/normaliseFractionalDurations.ts` around lines 15 - 24, Update the fractional conversion loop in the SCALE iteration to avoid rounding payload[small] at intermediate units, allowing fractional remainders to cascade into progressively smaller units. Preserve whole-unit handling, but round only the contribution when converting into the terminal nanoseconds unit; keep intermediate conversions unrounded so values such as fractional minutes retain their precision.packages/functions/src/scheduling/cron.ts (1)
89-89: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winIteration bound
5 * 366doesn't guarantee a 5-year search window.The loop advances by month, day, hour, or minute depending on which check fails, so 1830 iterations cover far less than 5 wall-clock years in the worst case (a match ~5 years out reached mainly via day-stepping already consumes ~1826 iterations, before any hour/minute steps). This can throw a spurious "could not find match" for a valid schedule. Bound the search by elapsed time (e.g. compare
currentagainst a+5 yearslimit) rather than a fixed iteration count.
packages/functions/src/scheduling/cron.ts#L89-L89: replace the fixed5 * 366iteration cap innextCronwith a time-based limit (stop whencurrentexceedsstart + 5 years).packages/functions/src/scheduling/cron.ts#L124-L124: apply the mirrored fix inprevCron(stop whencurrentfalls belowstart - 5 years).🤖 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/functions/src/scheduling/cron.ts` at line 89, Replace the fixed iteration caps in nextCron (packages/functions/src/scheduling/cron.ts, lines 89-89) and prevCron (packages/functions/src/scheduling/cron.ts, lines 124-124) with elapsed-time bounds: stop nextCron when current exceeds start plus 5 years, and stop prevCron when current falls below start minus 5 years. Preserve the existing matching and failure behavior within those windows.packages/functions/bin/sync-docs.mjs (1)
9-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExclude internal directories from being synced to public docs.
The
supportdirectory is documented as internal, but this loop will inadvertently syncsrc/support/README.mdtodoc/functions/support/index.md. Since a separatedoc/support.mdfile exists in the codebase, this may result in duplicate or unintended public documentation.Consider filtering out
supportand any hidden directories to prevent internal docs from leaking into the generated function documentation site.♻️ Proposed fix
const categories = fs.readdirSync(srcDir, { withFileTypes: true }) - .filter(dirent => dirent.isDirectory()) + .filter(dirent => dirent.isDirectory() && dirent.name !== 'support' && !dirent.name.startsWith('.')) .map(dirent => dirent.name);🤖 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/functions/bin/sync-docs.mjs` around lines 9 - 11, Update the categories construction in the sync-docs flow to exclude the internal support directory and hidden directories before mapping their names. Preserve syncing for visible public category directories, while preventing support and any directory whose name begins with “.” from reaching the documentation generation loop.packages/functions/doc/README.md (1)
5-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the logo alt text.
The
alttext on the logo image still references the oldTempo-fnsname. Consider updating it to match the new package name.🔨 Proposed fix
- <img src="../img/functions-logo.svg" width="90" height="90" alt="Tempo-fns Logo"> + <img src="../img/functions-logo.svg" width="90" height="90" alt="Functions Logo"> </td> <td valign="middle"> <h1 style="border-bottom: none; margin-bottom: 0;"><code>`@magmacomputing/functions`</code></h1>🤖 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/functions/doc/README.md` around lines 5 - 8, Update the logo image’s alt attribute in the README header to use the new `@magmacomputing/functions` package name instead of the outdated Tempo-fns text; leave the image source and surrounding markup unchanged.packages/functions/README.md (1)
5-5: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the logo image's alt text.
The
altattribute still references the old name "Tempo-fns Logo".🖼️ Proposed fix
- <img src="../img/functions-logo.svg" width="90" height="90" alt="Tempo-fns Logo"> + <img src="../img/functions-logo.svg" width="90" height="90" alt="Functions Logo">🤖 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/functions/README.md` at line 5, Update the alt attribute on the logo img element in the README to use the current project name instead of the outdated “Tempo-fns Logo” text, preserving the existing image source and dimensions.packages/functions/src/business/workingHoursUntil.ts (1)
42-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate error message prefix to match the new package name.
The error message still uses the old
[Tempo-Fns]prefix, whereas the warning log on line 104 was updated to[functions].💡 Proposed fix
- throw new Error('[Tempo-Fns] startHour must be strictly less than endHour'); + throw new Error('[functions] startHour must be strictly less than endHour');🤖 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/functions/src/business/workingHoursUntil.ts` at line 42, Update the error message in workingHoursUntil to use the new “[functions]” prefix instead of “[Tempo-Fns]”, matching the existing warning log prefix while preserving the rest of the validation message.
🤖 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 @.github/workflows/ci.yml:
- Around line 15-17: Update the workflow permissions used by the job running
actions/checkout@v4 so repository contents have read access instead of none,
ensuring checkout can fetch the code while preserving the minimal permission
scope.
In `@packages/functions/doc/preview.html`:
- Around line 15-16: Regenerate the embedded base64 payload assigned to markdown
in the preview HTML from the current rebranded functions README, replacing the
deprecated tempo-fns README content, logo paths, package references, and CDN
links while leaving the marked.parse rendering flow unchanged.
In `@packages/functions/rollup.config.js`:
- Around line 5-11: Update the Rollup output configuration to use the functions
branding: change the IIFE bundle filename from tempo-fns.global.js to
functions.global.js and expose it through the Functions global name instead of
TempoFns. Preserve the existing format and globals mapping.
In `@packages/functions/src/timezone/isDST.ts`:
- Around line 9-14: Update the string-handling fallback in the timezone
conversion flow to try TemporalAPI.PlainDateTime.from(date) and convert it with
the requested timeZone before falling back to TemporalAPI.PlainDate.from(date).
Preserve the existing Instant.from(date) path and ensure date-only strings still
use the PlainDate fallback.
In `@packages/functions/src/timezone/normalizeUtcOffset.ts`:
- Around line 15-16: Update the range validation condition in normalizeUtcOffset
so negative offsets at 12 hours are rejected when minutes are greater than zero,
enforcing the lower bound of -12:00 while preserving valid offsets through
-12:00 and the existing +14:00 checks.
In `@packages/tempo/bin/harvest-plugins.mjs`:
- Around line 45-49: Update the link-rewrite regexes in
packages/tempo/bin/harvest-plugins.mjs (lines 45-49) to optionally capture and
preserve trailing query strings or hash fragments for both local and
cross-plugin links. Also update the markdown-link rewrite logic in
packages/tempo/.vitepress/config.ts (lines 142-150) to capture the URL remainder
after .md and append it to the rewritten href, preserving hashes and queries.
---
Nitpick comments:
In `@packages/functions/bin/sync-docs.mjs`:
- Around line 9-11: Update the categories construction in the sync-docs flow to
exclude the internal support directory and hidden directories before mapping
their names. Preserve syncing for visible public category directories, while
preventing support and any directory whose name begins with “.” from reaching
the documentation generation loop.
In `@packages/functions/doc/README.md`:
- Around line 5-8: Update the logo image’s alt attribute in the README header to
use the new `@magmacomputing/functions` package name instead of the outdated
Tempo-fns text; leave the image source and surrounding markup unchanged.
In `@packages/functions/README.md`:
- Line 5: Update the alt attribute on the logo img element in the README to use
the current project name instead of the outdated “Tempo-fns Logo” text,
preserving the existing image source and dimensions.
In `@packages/functions/src/business/workingHoursUntil.ts`:
- Line 42: Update the error message in workingHoursUntil to use the new
“[functions]” prefix instead of “[Tempo-Fns]”, matching the existing warning log
prefix while preserving the rest of the validation message.
In `@packages/functions/src/duration/normaliseFractionalDurations.ts`:
- Around line 15-24: Update the fractional conversion loop in the SCALE
iteration to avoid rounding payload[small] at intermediate units, allowing
fractional remainders to cascade into progressively smaller units. Preserve
whole-unit handling, but round only the contribution when converting into the
terminal nanoseconds unit; keep intermediate conversions unrounded so values
such as fractional minutes retain their precision.
In `@packages/functions/src/scheduling/cron.ts`:
- Line 89: Replace the fixed iteration caps in nextCron
(packages/functions/src/scheduling/cron.ts, lines 89-89) and prevCron
(packages/functions/src/scheduling/cron.ts, lines 124-124) with elapsed-time
bounds: stop nextCron when current exceeds start plus 5 years, and stop prevCron
when current falls below start minus 5 years. Preserve the existing matching and
failure behavior within those windows.
In `@packages/library/src/common/storage.library.ts`:
- Line 15: Restore trailing semicolons on the variable declarations for
mockStorage and getSafeStorage, including declarations assigning objects or
arrow functions. Apply the same formatting at both referenced locations without
changing their behavior.
In `@packages/tempo/test/instance/instance.format.test.ts`:
- Line 63: Update the tests around the uncast runtime calls to use properly
typed Arabic and option fixtures that exercise the new Tempo.format overloads.
Remove the any/string casts, and augment TempoFormatTokens with the myDay token
where needed or add dedicated type-level assertions so supported Intl options
and plugin tokens are validated by the compiler.
🪄 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: 9861a964-967b-495d-8860-92af29c03634
⛔ Files ignored due to path filters (3)
package-lock.jsonis excluded by!**/package-lock.jsonpackages/functions/doc/public/img/functions-logo.svgis excluded by!**/*.svgpackages/functions/img/functions-logo.svgis excluded by!**/*.svg
📒 Files selected for processing (81)
.github/workflows/ci.yml.github/workflows/deploy-docs.yml.gitignorepackage.jsonpackages/functions/CHANGELOG.mdpackages/functions/LICENSEpackages/functions/README.mdpackages/functions/bin/sync-docs.mjspackages/functions/doc/.vitepress/config.mtspackages/functions/doc/CONTRIBUTING.mdpackages/functions/doc/README.mdpackages/functions/doc/functions/business/index.mdpackages/functions/doc/functions/calendar/index.mdpackages/functions/doc/functions/duration/index.mdpackages/functions/doc/functions/index.mdpackages/functions/doc/functions/scheduling/Interval.mdpackages/functions/doc/functions/scheduling/cron.mdpackages/functions/doc/functions/scheduling/index.mdpackages/functions/doc/functions/timezone/index.mdpackages/functions/doc/index.mdpackages/functions/doc/preview.htmlpackages/functions/doc/support.mdpackages/functions/package.jsonpackages/functions/rollup.config.jspackages/functions/src/business/README.mdpackages/functions/src/business/isSameFiscalQuarter.tspackages/functions/src/business/workingHoursUntil.tspackages/functions/src/calendar/README.mdpackages/functions/src/calendar/getISOWeekOfYear.tspackages/functions/src/calendar/getPublicHolidays.tspackages/functions/src/calendar/isFirstDayOfMonth.tspackages/functions/src/duration/README.mdpackages/functions/src/duration/normaliseFractionalDurations.tspackages/functions/src/index.tspackages/functions/src/scheduling/README.mdpackages/functions/src/scheduling/cron.tspackages/functions/src/support/README.mdpackages/functions/src/support/assert.tspackages/functions/src/support/fetch.tspackages/functions/src/support/index.tspackages/functions/src/support/intl.tspackages/functions/src/support/temporal.tspackages/functions/src/timezone/README.mdpackages/functions/src/timezone/getHemisphere.tspackages/functions/src/timezone/getOffsets.tspackages/functions/src/timezone/isDST.tspackages/functions/src/timezone/normalizeUtcOffset.tspackages/functions/test/business/isSameFiscalQuarter.test.tspackages/functions/test/business/workingHoursUntil.test.tspackages/functions/test/calendar/isFirstDayOfMonth.test.tspackages/functions/test/scheduling/cron.test.tspackages/functions/test/setup.tspackages/functions/test/tsconfig.jsonpackages/functions/tsconfig.jsonpackages/functions/vitest.config.tspackages/library/package.jsonpackages/library/src/common/storage.library.tspackages/plugins/.setup/README.mdpackages/plugins/.setup/doc/index.mdpackages/plugins/ticker/doc/index.mdpackages/plugins/tsup.shared.tspackages/tempo/.vitepress/config.tspackages/tempo/CHANGELOG.mdpackages/tempo/README.mdpackages/tempo/bin/harvest-plugins.mjspackages/tempo/doc/1-getting-started/tempo.cookbook.mdpackages/tempo/doc/3-extending-tempo/tempo.plugin.mdpackages/tempo/doc/8-project-and-support/commercial.mdpackages/tempo/doc/9-plugins/astro.mdpackages/tempo/doc/9-plugins/batch.mdpackages/tempo/doc/9-plugins/finance.mdpackages/tempo/doc/9-plugins/snap.mdpackages/tempo/doc/9-plugins/sync.mdpackages/tempo/doc/9-plugins/ticker.mdpackages/tempo/package.jsonpackages/tempo/plan/refactor-path-terms.mdpackages/tempo/src/module/module.format.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.type.tspackages/tempo/src/tempo.version.tspackages/tempo/test/instance/instance.format.test.ts
💤 Files with no reviewable changes (6)
- packages/tempo/doc/9-plugins/astro.md
- packages/tempo/doc/9-plugins/snap.md
- packages/tempo/doc/9-plugins/sync.md
- packages/tempo/doc/9-plugins/batch.md
- packages/tempo/doc/9-plugins/finance.md
- packages/tempo/doc/9-plugins/ticker.md
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/plugins/ticker/doc/index.md
- packages/tempo/src/module/module.format.ts
- packages/plugins/tsup.shared.ts
- packages/tempo/src/tempo.type.ts
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 6
🧹 Nitpick comments (8)
packages/tempo/test/instance/instance.format.test.ts (1)
63-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid bypassing the new
Tempo.formatcontract in tests.The
any/stringcasts prevent these tests from checking whether supportedIntloptions and plugin tokens are accepted by the new overloads. Prefer properly typed fixtures, augmentTempoFormatTokensformyDay, or add dedicated type-level assertions while keeping the runtime calls uncast.Also applies to: 80-80, 96-97
🤖 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/test/instance/instance.format.test.ts` at line 63, Update the tests around the uncast runtime calls to use properly typed Arabic and option fixtures that exercise the new Tempo.format overloads. Remove the any/string casts, and augment TempoFormatTokens with the myDay token where needed or add dedicated type-level assertions so supported Intl options and plugin tokens are validated by the compiler.packages/library/src/common/storage.library.ts (1)
15-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRestore trailing semicolons for variable declarations.
While automatic semicolon insertion (ASI) usually handles this, it is considered best practice (and often enforced by linters/formatters) to include trailing semicolons at the end of variable declarations assigning objects or arrow functions.
♻️ Proposed fixes
For
mockStorage:- } + };For
getSafeStorage:- } + };Also applies to: 24-24
🤖 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/storage.library.ts` at line 15, Restore trailing semicolons on the variable declarations for mockStorage and getSafeStorage, including declarations assigning objects or arrow functions. Apply the same formatting at both referenced locations without changing their behavior.packages/functions/src/duration/normaliseFractionalDurations.ts (1)
15-24: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winIntermediate
Math.rounddrops precision that could cascade.Rounding
frac * factorat every level discards sub-unit remainder instead of letting it flow into the next smaller unit on the following iteration. For example{ minutes: 1.51 }yields{ minutes: 1, seconds: 31 }(round(30.6)), losing ~600ms that should have cascaded tomilliseconds. Since eachsmallunit is re-examined as the nextbig, adding the un-rounded contribution for non-terminal units (and rounding only the terminalnanoseconds) preserves accuracy.🤖 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/functions/src/duration/normaliseFractionalDurations.ts` around lines 15 - 24, Update the fractional conversion loop in the SCALE iteration to avoid rounding payload[small] at intermediate units, allowing fractional remainders to cascade into progressively smaller units. Preserve whole-unit handling, but round only the contribution when converting into the terminal nanoseconds unit; keep intermediate conversions unrounded so values such as fractional minutes retain their precision.packages/functions/src/scheduling/cron.ts (1)
89-89: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winIteration bound
5 * 366doesn't guarantee a 5-year search window.The loop advances by month, day, hour, or minute depending on which check fails, so 1830 iterations cover far less than 5 wall-clock years in the worst case (a match ~5 years out reached mainly via day-stepping already consumes ~1826 iterations, before any hour/minute steps). This can throw a spurious "could not find match" for a valid schedule. Bound the search by elapsed time (e.g. compare
currentagainst a+5 yearslimit) rather than a fixed iteration count.
packages/functions/src/scheduling/cron.ts#L89-L89: replace the fixed5 * 366iteration cap innextCronwith a time-based limit (stop whencurrentexceedsstart + 5 years).packages/functions/src/scheduling/cron.ts#L124-L124: apply the mirrored fix inprevCron(stop whencurrentfalls belowstart - 5 years).🤖 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/functions/src/scheduling/cron.ts` at line 89, Replace the fixed iteration caps in nextCron (packages/functions/src/scheduling/cron.ts, lines 89-89) and prevCron (packages/functions/src/scheduling/cron.ts, lines 124-124) with elapsed-time bounds: stop nextCron when current exceeds start plus 5 years, and stop prevCron when current falls below start minus 5 years. Preserve the existing matching and failure behavior within those windows.packages/functions/bin/sync-docs.mjs (1)
9-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExclude internal directories from being synced to public docs.
The
supportdirectory is documented as internal, but this loop will inadvertently syncsrc/support/README.mdtodoc/functions/support/index.md. Since a separatedoc/support.mdfile exists in the codebase, this may result in duplicate or unintended public documentation.Consider filtering out
supportand any hidden directories to prevent internal docs from leaking into the generated function documentation site.♻️ Proposed fix
const categories = fs.readdirSync(srcDir, { withFileTypes: true }) - .filter(dirent => dirent.isDirectory()) + .filter(dirent => dirent.isDirectory() && dirent.name !== 'support' && !dirent.name.startsWith('.')) .map(dirent => dirent.name);🤖 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/functions/bin/sync-docs.mjs` around lines 9 - 11, Update the categories construction in the sync-docs flow to exclude the internal support directory and hidden directories before mapping their names. Preserve syncing for visible public category directories, while preventing support and any directory whose name begins with “.” from reaching the documentation generation loop.packages/functions/doc/README.md (1)
5-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the logo alt text.
The
alttext on the logo image still references the oldTempo-fnsname. Consider updating it to match the new package name.🔨 Proposed fix
- <img src="../img/functions-logo.svg" width="90" height="90" alt="Tempo-fns Logo"> + <img src="../img/functions-logo.svg" width="90" height="90" alt="Functions Logo"> </td> <td valign="middle"> <h1 style="border-bottom: none; margin-bottom: 0;"><code>`@magmacomputing/functions`</code></h1>🤖 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/functions/doc/README.md` around lines 5 - 8, Update the logo image’s alt attribute in the README header to use the new `@magmacomputing/functions` package name instead of the outdated Tempo-fns text; leave the image source and surrounding markup unchanged.packages/functions/README.md (1)
5-5: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the logo image's alt text.
The
altattribute still references the old name "Tempo-fns Logo".🖼️ Proposed fix
- <img src="../img/functions-logo.svg" width="90" height="90" alt="Tempo-fns Logo"> + <img src="../img/functions-logo.svg" width="90" height="90" alt="Functions Logo">🤖 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/functions/README.md` at line 5, Update the alt attribute on the logo img element in the README to use the current project name instead of the outdated “Tempo-fns Logo” text, preserving the existing image source and dimensions.packages/functions/src/business/workingHoursUntil.ts (1)
42-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate error message prefix to match the new package name.
The error message still uses the old
[Tempo-Fns]prefix, whereas the warning log on line 104 was updated to[functions].💡 Proposed fix
- throw new Error('[Tempo-Fns] startHour must be strictly less than endHour'); + throw new Error('[functions] startHour must be strictly less than endHour');🤖 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/functions/src/business/workingHoursUntil.ts` at line 42, Update the error message in workingHoursUntil to use the new “[functions]” prefix instead of “[Tempo-Fns]”, matching the existing warning log prefix while preserving the rest of the validation message.
🤖 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 @.github/workflows/ci.yml:
- Around line 15-17: Update the workflow permissions used by the job running
actions/checkout@v4 so repository contents have read access instead of none,
ensuring checkout can fetch the code while preserving the minimal permission
scope.
In `@packages/functions/doc/preview.html`:
- Around line 15-16: Regenerate the embedded base64 payload assigned to markdown
in the preview HTML from the current rebranded functions README, replacing the
deprecated tempo-fns README content, logo paths, package references, and CDN
links while leaving the marked.parse rendering flow unchanged.
In `@packages/functions/rollup.config.js`:
- Around line 5-11: Update the Rollup output configuration to use the functions
branding: change the IIFE bundle filename from tempo-fns.global.js to
functions.global.js and expose it through the Functions global name instead of
TempoFns. Preserve the existing format and globals mapping.
In `@packages/functions/src/timezone/isDST.ts`:
- Around line 9-14: Update the string-handling fallback in the timezone
conversion flow to try TemporalAPI.PlainDateTime.from(date) and convert it with
the requested timeZone before falling back to TemporalAPI.PlainDate.from(date).
Preserve the existing Instant.from(date) path and ensure date-only strings still
use the PlainDate fallback.
In `@packages/functions/src/timezone/normalizeUtcOffset.ts`:
- Around line 15-16: Update the range validation condition in normalizeUtcOffset
so negative offsets at 12 hours are rejected when minutes are greater than zero,
enforcing the lower bound of -12:00 while preserving valid offsets through
-12:00 and the existing +14:00 checks.
In `@packages/tempo/bin/harvest-plugins.mjs`:
- Around line 45-49: Update the link-rewrite regexes in
packages/tempo/bin/harvest-plugins.mjs (lines 45-49) to optionally capture and
preserve trailing query strings or hash fragments for both local and
cross-plugin links. Also update the markdown-link rewrite logic in
packages/tempo/.vitepress/config.ts (lines 142-150) to capture the URL remainder
after .md and append it to the rewritten href, preserving hashes and queries.
---
Nitpick comments:
In `@packages/functions/bin/sync-docs.mjs`:
- Around line 9-11: Update the categories construction in the sync-docs flow to
exclude the internal support directory and hidden directories before mapping
their names. Preserve syncing for visible public category directories, while
preventing support and any directory whose name begins with “.” from reaching
the documentation generation loop.
In `@packages/functions/doc/README.md`:
- Around line 5-8: Update the logo image’s alt attribute in the README header to
use the new `@magmacomputing/functions` package name instead of the outdated
Tempo-fns text; leave the image source and surrounding markup unchanged.
In `@packages/functions/README.md`:
- Line 5: Update the alt attribute on the logo img element in the README to use
the current project name instead of the outdated “Tempo-fns Logo” text,
preserving the existing image source and dimensions.
In `@packages/functions/src/business/workingHoursUntil.ts`:
- Line 42: Update the error message in workingHoursUntil to use the new
“[functions]” prefix instead of “[Tempo-Fns]”, matching the existing warning log
prefix while preserving the rest of the validation message.
In `@packages/functions/src/duration/normaliseFractionalDurations.ts`:
- Around line 15-24: Update the fractional conversion loop in the SCALE
iteration to avoid rounding payload[small] at intermediate units, allowing
fractional remainders to cascade into progressively smaller units. Preserve
whole-unit handling, but round only the contribution when converting into the
terminal nanoseconds unit; keep intermediate conversions unrounded so values
such as fractional minutes retain their precision.
In `@packages/functions/src/scheduling/cron.ts`:
- Line 89: Replace the fixed iteration caps in nextCron
(packages/functions/src/scheduling/cron.ts, lines 89-89) and prevCron
(packages/functions/src/scheduling/cron.ts, lines 124-124) with elapsed-time
bounds: stop nextCron when current exceeds start plus 5 years, and stop prevCron
when current falls below start minus 5 years. Preserve the existing matching and
failure behavior within those windows.
In `@packages/library/src/common/storage.library.ts`:
- Line 15: Restore trailing semicolons on the variable declarations for
mockStorage and getSafeStorage, including declarations assigning objects or
arrow functions. Apply the same formatting at both referenced locations without
changing their behavior.
In `@packages/tempo/test/instance/instance.format.test.ts`:
- Line 63: Update the tests around the uncast runtime calls to use properly
typed Arabic and option fixtures that exercise the new Tempo.format overloads.
Remove the any/string casts, and augment TempoFormatTokens with the myDay token
where needed or add dedicated type-level assertions so supported Intl options
and plugin tokens are validated by the compiler.
🪄 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: 9861a964-967b-495d-8860-92af29c03634
⛔ Files ignored due to path filters (3)
package-lock.jsonis excluded by!**/package-lock.jsonpackages/functions/doc/public/img/functions-logo.svgis excluded by!**/*.svgpackages/functions/img/functions-logo.svgis excluded by!**/*.svg
📒 Files selected for processing (81)
.github/workflows/ci.yml.github/workflows/deploy-docs.yml.gitignorepackage.jsonpackages/functions/CHANGELOG.mdpackages/functions/LICENSEpackages/functions/README.mdpackages/functions/bin/sync-docs.mjspackages/functions/doc/.vitepress/config.mtspackages/functions/doc/CONTRIBUTING.mdpackages/functions/doc/README.mdpackages/functions/doc/functions/business/index.mdpackages/functions/doc/functions/calendar/index.mdpackages/functions/doc/functions/duration/index.mdpackages/functions/doc/functions/index.mdpackages/functions/doc/functions/scheduling/Interval.mdpackages/functions/doc/functions/scheduling/cron.mdpackages/functions/doc/functions/scheduling/index.mdpackages/functions/doc/functions/timezone/index.mdpackages/functions/doc/index.mdpackages/functions/doc/preview.htmlpackages/functions/doc/support.mdpackages/functions/package.jsonpackages/functions/rollup.config.jspackages/functions/src/business/README.mdpackages/functions/src/business/isSameFiscalQuarter.tspackages/functions/src/business/workingHoursUntil.tspackages/functions/src/calendar/README.mdpackages/functions/src/calendar/getISOWeekOfYear.tspackages/functions/src/calendar/getPublicHolidays.tspackages/functions/src/calendar/isFirstDayOfMonth.tspackages/functions/src/duration/README.mdpackages/functions/src/duration/normaliseFractionalDurations.tspackages/functions/src/index.tspackages/functions/src/scheduling/README.mdpackages/functions/src/scheduling/cron.tspackages/functions/src/support/README.mdpackages/functions/src/support/assert.tspackages/functions/src/support/fetch.tspackages/functions/src/support/index.tspackages/functions/src/support/intl.tspackages/functions/src/support/temporal.tspackages/functions/src/timezone/README.mdpackages/functions/src/timezone/getHemisphere.tspackages/functions/src/timezone/getOffsets.tspackages/functions/src/timezone/isDST.tspackages/functions/src/timezone/normalizeUtcOffset.tspackages/functions/test/business/isSameFiscalQuarter.test.tspackages/functions/test/business/workingHoursUntil.test.tspackages/functions/test/calendar/isFirstDayOfMonth.test.tspackages/functions/test/scheduling/cron.test.tspackages/functions/test/setup.tspackages/functions/test/tsconfig.jsonpackages/functions/tsconfig.jsonpackages/functions/vitest.config.tspackages/library/package.jsonpackages/library/src/common/storage.library.tspackages/plugins/.setup/README.mdpackages/plugins/.setup/doc/index.mdpackages/plugins/ticker/doc/index.mdpackages/plugins/tsup.shared.tspackages/tempo/.vitepress/config.tspackages/tempo/CHANGELOG.mdpackages/tempo/README.mdpackages/tempo/bin/harvest-plugins.mjspackages/tempo/doc/1-getting-started/tempo.cookbook.mdpackages/tempo/doc/3-extending-tempo/tempo.plugin.mdpackages/tempo/doc/8-project-and-support/commercial.mdpackages/tempo/doc/9-plugins/astro.mdpackages/tempo/doc/9-plugins/batch.mdpackages/tempo/doc/9-plugins/finance.mdpackages/tempo/doc/9-plugins/snap.mdpackages/tempo/doc/9-plugins/sync.mdpackages/tempo/doc/9-plugins/ticker.mdpackages/tempo/package.jsonpackages/tempo/plan/refactor-path-terms.mdpackages/tempo/src/module/module.format.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.type.tspackages/tempo/src/tempo.version.tspackages/tempo/test/instance/instance.format.test.ts
💤 Files with no reviewable changes (6)
- packages/tempo/doc/9-plugins/astro.md
- packages/tempo/doc/9-plugins/snap.md
- packages/tempo/doc/9-plugins/sync.md
- packages/tempo/doc/9-plugins/batch.md
- packages/tempo/doc/9-plugins/finance.md
- packages/tempo/doc/9-plugins/ticker.md
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/plugins/ticker/doc/index.md
- packages/tempo/src/module/module.format.ts
- packages/plugins/tsup.shared.ts
- packages/tempo/src/tempo.type.ts
🛑 Comments failed to post (6)
.github/workflows/ci.yml (1)
15-17: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Update permissions to allow repository checkout.
Setting
contents: noneat the top level without overriding it at the job level will causeactions/checkout@v4to fail with a 403 Forbidden error, as it requirescontents: readpermission to fetch the repository code.🛠️ Proposed fix
permissions: - contents: none + contents: read🤖 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 @.github/workflows/ci.yml around lines 15 - 17, Update the workflow permissions used by the job running actions/checkout@v4 so repository contents have read access instead of none, ensuring checkout can fetch the code while preserving the minimal permission scope.packages/functions/doc/preview.html (1)
15-16: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the base64 payload to reflect the rebranded
functionscontent.The embedded base64 markdown string decodes to the older, deprecated
tempo-fnsREADME content (including the old logo paths, package name, and CDN links).To keep the local preview accurate, please regenerate and replace the base64 string with the contents of the newly rebranded
functionsREADME.🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 15-15: Avoid assigning untrusted data to innerHTML/outerHTML or document.write
Context: document.getElementById('content').innerHTML = marked.parse(markdown)
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting').(inner-outer-html)
🤖 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/functions/doc/preview.html` around lines 15 - 16, Regenerate the embedded base64 payload assigned to markdown in the preview HTML from the current rebranded functions README, replacing the deprecated tempo-fns README content, logo paths, package references, and CDN links while leaving the marked.parse rendering flow unchanged.packages/functions/rollup.config.js (1)
5-11: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Align IIFE bundle name and filename with the new
functionsbranding.The Rollup configuration still outputs the bundle as
dist/tempo-fns.global.jswith the global variable nameTempoFns. However, the documentation andREADME.mdexpect the file to bedist/functions.global.jsand expose aFunctionsglobal object.🔧 Proposed fix
output: { - file: 'dist/tempo-fns.global.js', + file: 'dist/functions.global.js', format: 'iife', - name: 'TempoFns', + name: 'Functions', globals: {📝 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.output: { file: 'dist/functions.global.js', format: 'iife', name: 'Functions', globals: { '`@magmacomputing/tempo`': 'Tempo' }🤖 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/functions/rollup.config.js` around lines 5 - 11, Update the Rollup output configuration to use the functions branding: change the IIFE bundle filename from tempo-fns.global.js to functions.global.js and expose it through the Functions global name instead of TempoFns. Preserve the existing format and globals mapping.packages/functions/src/timezone/isDST.ts (1)
9-14: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash node -e ' try { const { Temporal } = require("`@js-temporal/polyfill`"); const str = "2024-03-10T12:00:00"; const pd = Temporal.PlainDate.from(str).toZonedDateTime("America/New_York"); console.log("PlainDate evaluated at:", pd.toString()); // Incorrectly evaluates at midnight const pdt = Temporal.PlainDateTime.from(str).toZonedDateTime("America/New_York"); console.log("PlainDateTime evaluated at:", pdt.toString()); // Preserves 12:00 } catch (e) { console.log("Polyfill not in sandbox, but the spec behaves as described."); } 'Repository: magmacomputing/magma
Length of output: 298
Preserve time components for string inputs without an offset.
Temporal.PlainDate.from(date)drops the time portion, so2024-03-10T12:00:00is evaluated at midnight. On DST transition days that can return the wrong result; tryTemporal.PlainDateTime.from(date)first, then fall back toPlainDate.🤖 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/functions/src/timezone/isDST.ts` around lines 9 - 14, Update the string-handling fallback in the timezone conversion flow to try TemporalAPI.PlainDateTime.from(date) and convert it with the requested timeZone before falling back to TemporalAPI.PlainDate.from(date). Preserve the existing Instant.from(date) path and ensure date-only strings still use the PlainDate fallback.packages/functions/src/timezone/normalizeUtcOffset.ts (1)
15-16: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Enforce the stated lower bound of
-12:00.The comment specifies a valid range of
-12:00 .. +14:00. However, the current check(sign === '-' && h > 12)erroneously permits offsets like-12:30(sinceh = 12is not strictly greater than12), which bypasses the-12:00bound.Update the condition to reject minutes greater than
0whensignis'-'andhoursis12.🐛 Proposed fix
// Temporal-valid range: -12:00 .. +14:00, minutes 0..59 - if (h > 14 || m > 59 || (sign === '+' && h === 14 && m !== 0) || (sign === '-' && h > 12)) return zone; + if (h > 14 || m > 59 || (sign === '+' && h === 14 && m !== 0) || (sign === '-' && (h > 12 || (h === 12 && m !== 0)))) return zone;📝 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.// Temporal-valid range: -12:00 .. +14:00, minutes 0..59 if (h > 14 || m > 59 || (sign === '+' && h === 14 && m !== 0) || (sign === '-' && (h > 12 || (h === 12 && m !== 0)))) return zone;🤖 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/functions/src/timezone/normalizeUtcOffset.ts` around lines 15 - 16, Update the range validation condition in normalizeUtcOffset so negative offsets at 12 hours are rejected when minutes are greater than zero, enforcing the lower bound of -12:00 while preserving valid offsets through -12:00 and the existing +14:00 checks.packages/tempo/bin/harvest-plugins.mjs (1)
45-49: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve URL hashes and query parameters in markdown links.
Both the documentation harvest script and the VitePress markdown config rewrite plugin documentation links, but their regular expressions fail to account for
#hashor?querysuffixes.
In
harvest-plugins.mjs, links like[text](./other.md#section)will not match the regex (which strictly requires\.md\)), leaving them un-rewritten and causing broken links in VitePress.In
config.ts, the regex matches up to.mdbut ignores the suffix, meaning section anchors are stripped from the final generated link.
packages/tempo/bin/harvest-plugins.mjs#L45-L49: Update the regexes to optionally match and preserve trailing hashes/queries.
packages/tempo/.vitepress/config.ts#L142-L150: Update the regex to capture the remainder of the URL and append it to the rewritten href.🛠️ Proposed fixes for both files
For
packages/tempo/bin/harvest-plugins.mjs:- content = content.replace(/\]\(\.\/([^/]+)\.md\)/g, `](./${pluginId}.$1.md)`); + content = content.replace(/\]\(\.\/([^/#?)]+)\.md([#?][^)]*)?\)/g, (_m, f, suffix) => `](./${pluginId}.${f}.md${suffix || ''})`); // Rewrite cross-plugin relative links so they work in VitePress // Maps: ../../[plugin-dir]/doc/[filename].md -> ./[normalised-pluginId].[filename].md - content = content.replace(/\]\(\.\.\/\.\.\/([^/]+)\/doc\/([^/]+)\.md\)/g, (_m, p, f) => `](./${p.replace(/^\./, '_')}.${f}.md)`); + content = content.replace(/\]\(\.\.\/\.\.\/([^/]+)\/doc\/([^/#?)]+)\.md([#?][^)]*)?\)/g, (_m, p, f, suffix) => `](./${p.replace(/^\./, '_')}.${f}.md${suffix || ''})`);For
packages/tempo/.vitepress/config.ts:- const match = href.match(/(?:\.\.\/)+plugins\/([^\/]+)\/doc\/([^/]+)\.md/); + const match = href.match(/(?:\.\.\/)+plugins\/([^\/]+)\/doc\/([^/#?]+)\.md(.*)/); if (match) { // Normalise: replace leading dot with underscore (mirrors harvest-plugins.mjs) // e.g. '.setup' -> '_setup', 'ticker' -> 'ticker' // Using '_' (not stripping) avoids silent collision between '.setup/' and 'setup/'. const pluginId = match[1].replace(/^\./, '_'); // Rewrite the href to point to the absolute harvested VitePress path - hrefAttr[1] = `/doc/9-plugins/${pluginId}.${match[2]}.md`; + hrefAttr[1] = `/doc/9-plugins/${pluginId}.${match[2]}.md${match[3] || ''}`; }📝 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.content = content.replace(/\]\(\.\/([^/#?)]+)\.md([#?][^)]*)?\)/g, (_m, f, suffix) => `](./${pluginId}.${f}.md${suffix || ''})`); // Rewrite cross-plugin relative links so they work in VitePress // Maps: ../../[plugin-dir]/doc/[filename].md -> ./[normalised-pluginId].[filename].md content = content.replace(/\]\(\.\.\/\.\.\/([^/]+)\/doc\/([^/#?)]+)\.md([#?][^)]*)?\)/g, (_m, p, f, suffix) => `](./${p.replace(/^\./, '_')}.${f}.md${suffix || ''})`);📍 Affects 2 files
packages/tempo/bin/harvest-plugins.mjs#L45-L49(this comment)packages/tempo/.vitepress/config.ts#L142-L150🤖 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/bin/harvest-plugins.mjs` around lines 45 - 49, Update the link-rewrite regexes in packages/tempo/bin/harvest-plugins.mjs (lines 45-49) to optionally capture and preserve trailing query strings or hash fragments for both local and cross-plugin links. Also update the markdown-link rewrite logic in packages/tempo/.vitepress/config.ts (lines 142-150) to capture the URL remainder after .md and append it to the rewritten href, preserving hashes and queries.
Contributor
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 (3)
.github/workflows/deploy-docs.yml (1)
54-60: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBuild the functions workspace before copying its docs.
The workflow still runs
docs:buildfor@magmacomputing/tempo-fns, while the merge step reads frompackages/functions/doc/.vitepress/dist. This can fail because the old workspace no longer exists, or produce a missing/empty functions output. Rename the step and target the functions workspace instead.Proposed fix
- - name: Build Tempo-Fns Docs - run: npm run docs:build --workspace=`@magmacomputing/tempo-fns` + - name: Build Functions Docs + run: npm run docs:build --workspace=`@magmacomputing/functions`🤖 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 @.github/workflows/deploy-docs.yml around lines 54 - 60, Update the documentation build step currently named “Build Tempo-Fns Docs” to build the functions workspace that produces packages/functions/doc/.vitepress/dist, and rename the step to reflect that workspace. Keep the subsequent “Merge Documentation Sites” copy source unchanged.packages/functions/doc/CONTRIBUTING.md (1)
57-57: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the obsolete CI disclaimer.
This says automated CI is unavailable, but this PR adds CI jobs that run the functions tests. Update the contributor instructions to describe the current CI workflow.
Proposed wording
-Once your files are structured correctly, submit a PR to the `magma` monorepo! Please ensure you run the tests and linter locally before submitting, as our automated CI pipeline is not yet available for this initial release. +Once your files are structured correctly, submit a PR to the `magma` monorepo. Please run the tests and linter locally; CI will also validate the relevant workspace jobs.🤖 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/functions/doc/CONTRIBUTING.md` at line 57, Update the contributor instructions in CONTRIBUTING.md to remove the obsolete statement that automated CI is unavailable. Revise the submission guidance to reflect that CI now runs the functions tests, while retaining the instruction to run tests and lint locally if still applicable.packages/functions/doc/functions/scheduling/cron.md (1)
42-42: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExport
parseCronfrom the package root or fix this import.
packages/functions/src/index.tsexportsnextCronandprevCron, but notparseCron; this example will fail when copied. Use a supported subpath or addparseCronto the root barrel.🤖 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/functions/doc/functions/scheduling/cron.md` at line 42, Fix the cron documentation example’s parseCron import by either switching it to a supported package subpath or exporting parseCron from the package root barrel alongside nextCron and prevCron in packages/functions/src/index.ts; ensure the copied example resolves successfully.
🤖 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.
Outside diff comments:
In @.github/workflows/deploy-docs.yml:
- Around line 54-60: Update the documentation build step currently named “Build
Tempo-Fns Docs” to build the functions workspace that produces
packages/functions/doc/.vitepress/dist, and rename the step to reflect that
workspace. Keep the subsequent “Merge Documentation Sites” copy source
unchanged.
In `@packages/functions/doc/CONTRIBUTING.md`:
- Line 57: Update the contributor instructions in CONTRIBUTING.md to remove the
obsolete statement that automated CI is unavailable. Revise the submission
guidance to reflect that CI now runs the functions tests, while retaining the
instruction to run tests and lint locally if still applicable.
In `@packages/functions/doc/functions/scheduling/cron.md`:
- Line 42: Fix the cron documentation example’s parseCron import by either
switching it to a supported package subpath or exporting parseCron from the
package root barrel alongside nextCron and prevCron in
packages/functions/src/index.ts; ensure the copied example resolves
successfully.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d6cdcbe3-1878-451f-a1fc-84b63a14d893
📒 Files selected for processing (37)
.github/workflows/ci.yml.github/workflows/deploy-docs.ymlpackages/functions/CHANGELOG.mdpackages/functions/README.mdpackages/functions/bin/sync-docs.mjspackages/functions/doc/.vitepress/config.mtspackages/functions/doc/CONTRIBUTING.mdpackages/functions/doc/README.mdpackages/functions/doc/functions/business/index.mdpackages/functions/doc/functions/calendar/index.mdpackages/functions/doc/functions/duration/index.mdpackages/functions/doc/functions/index.mdpackages/functions/doc/functions/scheduling/Interval.mdpackages/functions/doc/functions/scheduling/cron.mdpackages/functions/doc/functions/scheduling/index.mdpackages/functions/doc/functions/timezone/index.mdpackages/functions/doc/index.mdpackages/functions/doc/support.mdpackages/functions/rollup.config.jspackages/functions/src/business/README.mdpackages/functions/src/business/workingHoursUntil.tspackages/functions/src/calendar/README.mdpackages/functions/src/duration/README.mdpackages/functions/src/duration/normaliseFractionalDurations.tspackages/functions/src/scheduling/README.mdpackages/functions/src/scheduling/cron.tspackages/functions/src/timezone/README.mdpackages/functions/src/timezone/isDST.tspackages/functions/src/timezone/normalizeUtcOffset.tspackages/plugins/finance/src/index.tspackages/plugins/snap/src/index.tspackages/tempo/.vitepress/config.tspackages/tempo/bin/harvest-plugins.mjspackages/tempo/src/module/module.format.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.type.tspackages/tempo/test/instance/instance.format.test.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- packages/functions/rollup.config.js
- packages/functions/doc/.vitepress/config.mts
- packages/functions/src/timezone/README.md
- packages/functions/src/timezone/isDST.ts
- packages/tempo/.vitepress/config.ts
- packages/functions/bin/sync-docs.mjs
- packages/functions/src/duration/normaliseFractionalDurations.ts
- packages/functions/src/timezone/normalizeUtcOffset.ts
- packages/tempo/src/tempo.type.ts
- packages/tempo/src/module/module.format.ts
- packages/functions/doc/functions/scheduling/Interval.md
- .github/workflows/ci.yml
- packages/functions/src/scheduling/cron.ts
- packages/tempo/bin/harvest-plugins.mjs
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.
Summary by CodeRabbit
New Features
Tempo.format()template tokens, including extensible plugin tokens.Documentation
Bug Fixes
Release