Add CI workflow for Tempo project#22
Merged
magmacomputing merged 15 commits intomainfrom Apr 27, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces CI infrastructure for running tests and benchmarks, adds timezone-aware date parsing constants and fallback mappings, refactors timezone locale extraction with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/ci.yml (3)
14-26: Add npm caching and job timeout for better CI performance and reliability.The workflow could benefit from:
- npm caching: Significantly speeds up dependency installation across runs
- timeout-minutes: Prevents runaway jobs from consuming CI minutes
⚡ Proposed improvements
test: runs-on: ubuntu-latest + timeout-minutes: 10 steps: - uses: actions/checkout@v4 - uses: actions/setup-node@v4 with: node-version: '22' + cache: 'npm' + cache-dependency-path: packages/tempo/package-lock.json - name: Install dependencies run: npm ci working-directory: packages/tempo - name: Run standard tests run: npm test working-directory: packages/tempo🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 14 - 26, Update the "test" GitHub Actions job to add npm caching and a job timeout: add a timeout-minutes value (e.g., 30) to the "test" job and insert an actions/cache step before "Install dependencies" that caches the npm cache (use path ~/.npm and a cache key based on runner OS and packages/tempo/package-lock.json or package.json). Ensure the cache step runs before the "Install dependencies" step and that the "Install dependencies" step still runs npm ci in packages/tempo so the cache can be restored/updated.
28-48: Add npm caching and timeout for consistency.Similar to the
testjob, this job would benefit from npm caching and a timeout.⚡ Proposed improvements
test-parse-prefilter: name: Test with parsePrefilter enabled runs-on: ubuntu-latest + timeout-minutes: 15 if: github.ref == 'refs/heads/release-c-layout-order-planner' || github.event.pull_request.base.ref == 'main' steps: - uses: actions/checkout@v4 - uses: actions/setup-node@v4 with: node-version: '22' + cache: 'npm' + cache-dependency-path: packages/tempo/package-lock.json - name: Install dependencies run: npm ci working-directory: packages/tempo🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 28 - 48, The test-parse-prefilter job is missing npm caching and a timeout like the test job; update the "test-parse-prefilter" job to add an actions/cache step that caches ~/.npm (or the same cache key/restore logic used by the existing test job) before "Install dependencies" and set timeout-minutes on the job (matching the test job value) to avoid hanging runs; ensure the cache step and timeout reference the same npm cache key strategy used elsewhere so they remain consistent with the existing workflow.
46-48: Consider adding validation for the benchmark results.Running a benchmark without capturing or validating its output provides limited CI value. The benchmark could silently fail or complete with degraded performance without raising any alerts.
💡 Suggestions for improvement
Consider one of the following approaches:
- Add performance assertions if the benchmark script supports them
- Capture and archive results for historical comparison:
- name: Run end-to-end benchmark - run: npx tsx --conditions=development scratch/bench.parse.prefilter.e2e.ts + run: | + npx tsx --conditions=development scratch/bench.parse.prefilter.e2e.ts | tee benchmark-results.txt working-directory: packages/tempo + - name: Upload benchmark results + uses: actions/upload-artifact@v4 + with: + name: benchmark-results + path: packages/tempo/benchmark-results.txt
- Document the intent if the benchmark is only meant to verify it completes without error
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 46 - 48, Update the "Run end-to-end benchmark" CI step that runs scratch/bench.parse.prefilter.e2e.ts via npx tsx so the workflow captures and validates results: modify the benchmark script (scratch/bench.parse.prefilter.e2e.ts) to emit machine-readable output (JSON) and exit non-zero on failed assertions or thresholds, have the CI step redirect/capture that output to a file, add a subsequent GitHub Actions step to upload the output as an artifact (upload-artifact) and/or run a short validation step (using actions/github-script or a small node script) that parses the JSON and fails the job when performance regressions or assertion failures are detected; alternatively, if the benchmark is only for completion, add a comment step documenting that intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 40-42: Update the CI step that writes the prefilter setup so it
creates the file under packages/tempo (e.g. write to
packages/tempo/test/ci.prefilter.setup.js instead of test/ci.prefilter.setup.js)
and include an import for the Tempo symbol at top of the generated file (for
example add either `const { Tempo } = require('tempo')` or `import { Tempo }
from 'tempo'` depending on your module system) so the echoed content becomes a
valid module that calls Tempo.init({ parsePrefilter: true }); ensure the import
path matches how Tempo is resolved inside packages/tempo.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 14-26: Update the "test" GitHub Actions job to add npm caching and
a job timeout: add a timeout-minutes value (e.g., 30) to the "test" job and
insert an actions/cache step before "Install dependencies" that caches the npm
cache (use path ~/.npm and a cache key based on runner OS and
packages/tempo/package-lock.json or package.json). Ensure the cache step runs
before the "Install dependencies" step and that the "Install dependencies" step
still runs npm ci in packages/tempo so the cache can be restored/updated.
- Around line 28-48: The test-parse-prefilter job is missing npm caching and a
timeout like the test job; update the "test-parse-prefilter" job to add an
actions/cache step that caches ~/.npm (or the same cache key/restore logic used
by the existing test job) before "Install dependencies" and set timeout-minutes
on the job (matching the test job value) to avoid hanging runs; ensure the cache
step and timeout reference the same npm cache key strategy used elsewhere so
they remain consistent with the existing workflow.
- Around line 46-48: Update the "Run end-to-end benchmark" CI step that runs
scratch/bench.parse.prefilter.e2e.ts via npx tsx so the workflow captures and
validates results: modify the benchmark script
(scratch/bench.parse.prefilter.e2e.ts) to emit machine-readable output (JSON)
and exit non-zero on failed assertions or thresholds, have the CI step
redirect/capture that output to a file, add a subsequent GitHub Actions step to
upload the output as an artifact (upload-artifact) and/or run a short validation
step (using actions/github-script or a small node script) that parses the JSON
and fails the job when performance regressions or assertion failures are
detected; alternatively, if the benchmark is only for completion, add a comment
step documenting that intent.
🪄 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: 904dcb1c-07ed-42e9-84e1-494b7e88e4d5
📒 Files selected for processing (1)
.github/workflows/ci.yml
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
14-78: Heavy duplication between the two jobs.The
testandtest-parse-prefilterjobs share ~20 lines of identical setup (checkout, Node, cache, install). Consider extracting the common steps into a reusable composite action under.github/actions/setup-tempo/, or using a matrix withinclude:to vary just the prefilter-specific steps. This is optional but will pay off as the workflow grows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 14 - 78, The workflow duplicates setup steps between the jobs test and test-parse-prefilter; extract the shared steps (checkout, Set up Node.js, Cache npm, Install monorepo dependencies) into a reusable unit and call it from both jobs. Create a composite action at .github/actions/setup-tempo/ that exposes inputs for working-directory and node-version (or create a matrix on the test job and use include to vary only parsePrefilter-specific steps), then replace the repeated steps in both jobs with a single uses: ./.github/actions/setup-tempo (or a single matrix job) and keep only the parsePrefilter-specific steps (Write parsePrefilter setup file, Run tests with env TEMPO_PREFILTER_CI, E2E benchmark, upload/validate) in test-parse-prefilter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 67-74: The upload step "Upload benchmark output" should run
regardless of the benchmark step result and the benchmark command should capture
stderr into the output file; add if: always() to the "Upload benchmark output"
job and change the "Run end-to-end benchmark" command (the npx tsx
--conditions=development scratch/bench.parse.prefilter.e2e.ts >
bench-output.json) to redirect stderr as well (for example append 2>&1 or
otherwise write stderr to packages/tempo/bench-output.json) so that malformed
output or runtime errors are preserved and the actions/upload-artifact step
(named bench-parse-prefilter-e2e) always uploads
packages/tempo/bench-output.json for post-mortem.
- Around line 24-33: The cache key in the "Cache npm" step currently hashes
packages/tempo/package-lock.json while the "Install monorepo dependencies" step
runs npm ci at the repo root; update the cache key to hash the root
package-lock.json (use hashFiles('package-lock.json')) for both occurrences (the
duplicated cache block further down) so root dependency changes correctly bust
the cache, or replace the manual actions/cache@v4 usage with actions/setup-node
and its built-in cache: 'npm' option for simpler, correct npm caching.
- Around line 59-61: The generated ci.prefilter.setup.js is broken and unused:
fix by replacing the echo step with creating a proper JS setup module that sets
an env var or calls Tempo.init correctly (referencing Tempo.init and
parsePrefilter) from a compiled/runtime-loadable module (do not import
../src/tempo.index.ts directly); update vitest.config.ts to include this new
setup file in setupFiles so Vitest loads it, and modify the CI job to run tests
with TEMPO_PREFILTER_CI=true (or have the setup file read that env var) so the
configuration actually takes effect.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 14-78: The workflow duplicates setup steps between the jobs test
and test-parse-prefilter; extract the shared steps (checkout, Set up Node.js,
Cache npm, Install monorepo dependencies) into a reusable unit and call it from
both jobs. Create a composite action at .github/actions/setup-tempo/ that
exposes inputs for working-directory and node-version (or create a matrix on the
test job and use include to vary only parsePrefilter-specific steps), then
replace the repeated steps in both jobs with a single uses:
./.github/actions/setup-tempo (or a single matrix job) and keep only the
parsePrefilter-specific steps (Write parsePrefilter setup file, Run tests with
env TEMPO_PREFILTER_CI, E2E benchmark, upload/validate) in test-parse-prefilter.
🪄 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: e9319c2f-2e57-4cff-82cc-dfd6687eaece
📒 Files selected for processing (1)
.github/workflows/ci.yml
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/tempo/test/compact.time.test.ts (1)
47-55: Remove debugconsole.logstatements from the test.These look like leftover debugging artifacts. They add noise to CI output (especially under the new Tempo CI workflow), don't contribute to assertions, and previous tests in this file don't rely on similar logging. If they were added to diagnose a flaky/timezone-dependent failure, consider folding the relevant signals into assertion messages or
expectcalls instead so failures are self-describing.♻️ Proposed cleanup
test('does not regress compact date parsing for 8-digit input', () => { - console.log('process: ', process.env.TZ, process.env.LANG, process.env.LC_ALL); - console.log('Tempo: ', Tempo.config.sphere, Tempo.config.timeZone, Tempo.config.locale); const us = new Tempo('04012026', { timeZone: 'America/New_York' }); const uk = new Tempo('04012026', { timeZone: 'Europe/London' }); - console.log('US:', us.toString(), 'UK:', uk.toString()); - console.log('sphere: ', us.config.sphere, uk.config.sphere); - console.log('timeZone: ', us.config.timeZone, uk.config.timeZone); - console.log('locale: ', us.config.locale, uk.config.locale); - expect(us.mm).toBe(4); expect(us.dd).toBe(1); expect(uk.mm).toBe(1); expect(uk.dd).toBe(4); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/test/compact.time.test.ts` around lines 47 - 55, Remove the debug console.log statements in the test around the Tempo instantiations (the console.logs that print process.env, Tempo.config, and the 'US:'/'UK:' outputs); leave the Tempo creations (new Tempo('04012026', ...)) and any assertions intact, and if you need the printed state for failure diagnostics, convert those checks into explicit expect assertions (e.g., assert Tempo.config.sphere/timeZone/locale or us.toString()/uk.toString()) so test output is deterministic and CI logs are not noisy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/tempo/test/compact.time.test.ts`:
- Around line 47-55: Remove the debug console.log statements in the test around
the Tempo instantiations (the console.logs that print process.env, Tempo.config,
and the 'US:'/'UK:' outputs); leave the Tempo creations (new Tempo('04012026',
...)) and any assertions intact, and if you need the printed state for failure
diagnostics, convert those checks into explicit expect assertions (e.g., assert
Tempo.config.sphere/timeZone/locale or us.toString()/uk.toString()) so test
output is deterministic and CI logs are not noisy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 16ab2270-11a9-4b96-acd9-cbe0290274f7
📒 Files selected for processing (2)
.github/workflows/ci.ymlpackages/tempo/test/compact.time.test.ts
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/ci.yml
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/tempo/src/tempo.class.ts (2)
1014-1016: Drop the trailing commented-out fallback.
instance instanceof Tempoalready routes through[Symbol.hasInstance]on line 1010, which performs the exactBoolean(instance?.[$Identity])check that's commented out here — so behavior across module boundaries is preserved (the brand usesSymbol.for('$LibraryIdentity')). The trailing//Boolean(...)is dead code.♻️ Proposed cleanup
static isTempo(instance?: any): instance is Tempo { - return instance instanceof Tempo;//Boolean(instance?.[$Identity]) + return instance instanceof Tempo; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/tempo.class.ts` around lines 1014 - 1016, Remove the dead commented fallback in the static type guard: in Tempo.isTempo, delete the trailing "//Boolean(instance?.[$Identity])" comment so the method only returns "instance instanceof Tempo"; this keeps behavior routed through Tempo[Symbol.hasInstance] (declared near line 1010) and avoids leftover dead code.
460-460: Fallback won't trigger ifgetTimeZones?.()returns an empty array.The
??chain only falls through onnull/undefined. If a runtime exposesgetTimeZonesbut returns[]for a locale (some polyfills/stubs return empty arrays rather than throwing or returningundefined), the user getstimeZones: []instead of the curatedmdyFallbacklist, and#isMonthDaywill fail to detect MDY. Cheap defensive guard:♻️ Proposed refactor
- .map(mdy => ({ locale: mdy.baseName, timeZones: (mdy as Record<string, any>).getTimeZones?.() ?? mdyFallback[mdy.baseName] ?? [] })); + .map(mdy => { + const tzs = (mdy as Record<string, any>).getTimeZones?.(); + return { + locale: mdy.baseName, + timeZones: (Array.isArray(tzs) && tzs.length > 0) ? tzs : (mdyFallback[mdy.baseName] ?? []), + }; + });Worth confirming against the polyfill/shim that motivated this fallback (per the comment on lines 454-456 about google-apps-script's
Intl.Localeinterface) — does it return[],undefined, or omit the method entirely?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/tempo.class.ts` at line 460, The current mapping uses the optional chaining result of getTimeZones to decide fallback, but it only falls back on null/undefined so an empty array prevents using mdyFallback; update the map in the Tempo class (the expression that produces { locale: mdy.baseName, timeZones: ... }) to treat empty arrays as falsy by checking the returned value's length before choosing mdyFallback (e.g. capture the result of (mdy as Record<string, any>).getTimeZones?.() into a temp, and set timeZones to that temp if it has length, otherwise use mdyFallback[mdy.baseName] ?? []), ensuring `#isMonthDay` logic receives the curated fallback when getTimeZones returns [] or is missing.packages/tempo/src/support/tempo.default.ts (1)
195-244: Fallback only covers the defaultmdyLocales; non-default locales will silently get[].
mdyFallbackonly contains entries for'en-US'and'en-AS'(matchingDefault.mdyLocaleson line 190). In environments withoutIntl.Locale#getTimeZones(), any user-added MDY locale (e.g.,en-PH,en-GU,en-MP,en-FM,en-PR) will resolve to[]in#mdyLocales, so#isMonthDaywill quietly returnfalsefor those — exactly the regression this fallback is meant to prevent. Consider either:
- documenting (in the JSDoc here) that custom
mdyLocalesneed a correspondingmdyFallbackentry when targeting limited-Intl runtimes (e.g., older Apps Script), or- emitting a one-time
dbg.warnfrom#mdyLocaleswhengetTimeZones?.()is missing andmdyFallback[baseName]is also missing.Mixing canonical (
America/Indiana/Indianapolis) and legacy alias (US/Eastern,America/Indianapolis,America/Louisville) entries is fine — it defensively matches whatever stringshape.config.timeZoneis normalized to. No change needed there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/support/tempo.default.ts` around lines 195 - 244, mdyFallback currently only includes 'en-US' and 'en-AS', so when Intl.Locale.getTimeZones() is unavailable custom MDY locales (used by Default.mdyLocales and resolved in `#mdyLocales`) will silently return an empty array and cause `#isMonthDay` to return false; add a guard in the logic that builds `#mdyLocales` to detect when getTimeZones is missing and a given baseName has no mdyFallback entry, and emit a one-time dbg.warn naming the missing base (e.g., baseName or locale) and advising to add a mdyFallback entry (or alternatively document this in the JSDoc for mdyFallback); reference the mdyFallback constant, Default.mdyLocales, the `#mdyLocales` resolution path and `#isMonthDay` where the behavior manifests to locate where to add the warning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/tempo/src/support/tempo.default.ts`:
- Around line 195-244: mdyFallback currently only includes 'en-US' and 'en-AS',
so when Intl.Locale.getTimeZones() is unavailable custom MDY locales (used by
Default.mdyLocales and resolved in `#mdyLocales`) will silently return an empty
array and cause `#isMonthDay` to return false; add a guard in the logic that
builds `#mdyLocales` to detect when getTimeZones is missing and a given baseName
has no mdyFallback entry, and emit a one-time dbg.warn naming the missing base
(e.g., baseName or locale) and advising to add a mdyFallback entry (or
alternatively document this in the JSDoc for mdyFallback); reference the
mdyFallback constant, Default.mdyLocales, the `#mdyLocales` resolution path and
`#isMonthDay` where the behavior manifests to locate where to add the warning.
In `@packages/tempo/src/tempo.class.ts`:
- Around line 1014-1016: Remove the dead commented fallback in the static type
guard: in Tempo.isTempo, delete the trailing "//Boolean(instance?.[$Identity])"
comment so the method only returns "instance instanceof Tempo"; this keeps
behavior routed through Tempo[Symbol.hasInstance] (declared near line 1010) and
avoids leftover dead code.
- Line 460: The current mapping uses the optional chaining result of
getTimeZones to decide fallback, but it only falls back on null/undefined so an
empty array prevents using mdyFallback; update the map in the Tempo class (the
expression that produces { locale: mdy.baseName, timeZones: ... }) to treat
empty arrays as falsy by checking the returned value's length before choosing
mdyFallback (e.g. capture the result of (mdy as Record<string,
any>).getTimeZones?.() into a temp, and set timeZones to that temp if it has
length, otherwise use mdyFallback[mdy.baseName] ?? []), ensuring `#isMonthDay`
logic receives the curated fallback when getTimeZones returns [] or is missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cfc7d6b7-6a41-4254-9aa1-6efc75ce0e35
📒 Files selected for processing (4)
packages/tempo/doc/tempo.config.mdpackages/tempo/doc/tempo.parse.mdpackages/tempo/src/support/tempo.default.tspackages/tempo/src/tempo.class.ts
✅ Files skipped from review due to trivial changes (2)
- packages/tempo/doc/tempo.config.md
- packages/tempo/doc/tempo.parse.md
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
Release Notes
Documentation
Tests
Bug Fixes