sandbox document#21
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 16 minutes and 52 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThis PR increments package versions to 2.6.0, introduces a UTC offset normalization utility, removes Chinese-specific season metadata, adds a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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 |
|
@codepilot please review |
|
@copilot resolve the merge conflicts in this pull request |
…cts in sandbox-factory.md Agent-Logs-Url: https://github.com/magmacomputing/magma/sessions/a94eb8fa-5557-4b7e-b6bc-d0c4f6b82900 Co-authored-by: magmacomputing <6935496+magmacomputing@users.noreply.github.com>
Merge conflicts resolved in commit f90d471. The conflict in |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/tempo/src/plugin/term/term.season.ts (1)
49-49:⚠️ Potential issue | 🟡 MinorTypo: "Meteorogical" → "Meteorological".
Minor user-facing string typo in the term description (consistent with the correctly spelled comment on line 5).
✏️ Proposed fix
- description: 'Meteorogical season', + description: 'Meteorological season',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/plugin/term/term.season.ts` at line 49, Fix the typo in the term description by changing the description property value from "Meteorogical season" to "Meteorological season" in the term definition (the description field in packages/tempo/src/plugin/term/term.season.ts); update the description string so it matches the correctly spelled comment and user-facing text.packages/tempo/src/engine/engine.layout.ts (1)
64-70:⚠️ Potential issue | 🟡 MinorDuplicate entries when
preferredmixes aliases and descriptions silently disable reordering.If
preferredcontains both a token alias and its symbol description (e.g.['dt', 'date']or['tm', 'time']),byName.get(resolvedName)returns the same entry for both names and the loop pushes that entry intonexttwice. Because no de-duplication is done here,next.length > entries.length, and the guard at line 76 (next.length === entries.length && …) collapses tofalse, returning the originallayout. The caller's preferred order is silently ignored.🔧 Suggested fix — skip already-seen entries
preferred.forEach(name => { const resolvedName = TOKEN_DESCRIPTION_BY_NAME.get(name) ?? name; const entry = byName.get(resolvedName) ?? byName.get(name); if (!entry) return; + if (seen.has(entry[0])) return; seen.add(entry[0]); next.push(entry); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/engine/engine.layout.ts` around lines 64 - 70, preferred loop can push the same entry twice when preferred mixes aliases and descriptions (TOKEN_DESCRIPTION_BY_NAME, byName -> entry), which causes next.length to exceed entries.length and the reordering guard to fail; change the loop in the preferred.forEach to skip entries whose identifier is already in seen (i.e., check if seen.has(entry[0]) and continue if true) before adding to seen and pushing to next so duplicate aliases/descriptions are ignored and the final guard can succeed to return the reordered layout.packages/tempo/src/support/tempo.init.ts (1)
147-153:⚠️ Potential issue | 🟡 Minor
timeZoneconfiguration diverges betweenextendStateand[$setConfig]code paths.
tempo.class.ts:401usesenums.TIMEZONE[zone] ?? arg.value, while this path usesenums.TIMEZONE[zone] ?? normalizeUtcOffset(String(arg.value)). ThenormalizeUtcOffsetfunction normalizes UTC offset strings in the form"UTC+8"to"+08:00"format, but returns the input unchanged if it doesn't match that pattern (including IANA names like"America/New_York"). This means the two entry points will normalize UTC offset formats inconsistently:tempo.class.tswill pass"UTC+8"through as-is, whiletempo.init.tsnormalizes it to"+08:00". Align the two code paths to ensure consistent behavior across all configuration entry points.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/support/tempo.init.ts` around lines 147 - 153, Both configuration entry points must normalize UTC offset strings the same way: change the logic in the other path (tempo.class.ts where it currently does enums.TIMEZONE[zone] ?? arg.value) to mirror this file’s behavior by using enums.TIMEZONE[zone] ?? normalizeUtcOffset(String(arg.value)) before calling setProperty(state.config, 'timeZone', ...) and getHemisphere(...); ensure you import or reference normalizeUtcOffset and use the resolvedZone variable the same way as in tempo.init.ts so timeZone normalization is consistent across extendState/[$setConfig] and tempo.init.ts.
🧹 Nitpick comments (15)
packages/tempo/test/season_metadata.test.ts (1)
4-9: Consider locking in the removed contract with a negative assertion.Since the v2.6.0 migration explicitly removes the
CNfield fromterm.seasonoutput, an assertion guarding against its reintroduction would make the regression coverage stronger:♻️ Proposed addition
expect(q.key).toBe('Spring'); + expect(q.CN).toBeUndefined();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/test/season_metadata.test.ts` around lines 4 - 9, The test for Tempo.term.season currently asserts the positive case for q.key but doesn't guard against reintroducing the removed CN field; update the test in season_metadata.test.ts (where Tempo('2024-05-15', { sphere: 'north' }) and q = t.term.season are used) to include a negative assertion that q.CN is undefined or otherwise not present (e.g., expect(q.CN).toBeUndefined() or expect('CN' in q).toBe(false)) so the regression test fails if the CN property is ever reintroduced.packages/tempo/doc/tempo.config.md (1)
46-76: Wrap code identifiers in backticks for consistency.The rewritten Global Discovery prose drops backticks around several identifiers (
globalThis,Tempo.init(),Symbol.for('$Tempo'),Tempo.extend(...)) that are formatted as code elsewhere in this same file (e.g., line 10 in the Precedence Hierarchy uses`Symbol.for('$Tempo')`). Re-adding backticks keeps the doc style uniform and renders the symbols correctly.✏️ Suggested edits
-To facilitate configuration in micro-frontend architectures or script-first bootstraps, Tempo can discover a Discovery object from globalThis during Tempo.init(). +To facilitate configuration in micro-frontend architectures or script-first bootstraps, Tempo can discover a Discovery object from `globalThis` during `Tempo.init()`. The intended flow is: -1. Write a Discovery object into globalThis under the configured discovery symbol key. +1. Write a Discovery object into `globalThis` under the configured discovery symbol key. 2. Import a module containing Tempo. -3. Tempo class static initialization runs Tempo.init(). -4. Tempo.init() reads the global discovery slot and merges it. +3. Tempo class static initialization runs `Tempo.init()`. +4. `Tempo.init()` reads the global discovery slot and merges it. -By default, the key is Symbol.for('$Tempo'). +By default, the key is `Symbol.for('$Tempo')`.### Explicit Runtime Registration (Not Global Discovery) -Using Tempo.extend(...) is explicit registration after Tempo is loaded. It is ergonomic and strongly recommended for normal application code, but it is a different mechanism from pre-bootstrap global discovery. +Using `Tempo.extend(...)` is explicit registration after Tempo is loaded. It is ergonomic and strongly recommended for normal application code, but it is a different mechanism from pre-bootstrap global discovery.### Security and Ergonomics Notes - Global Discovery is convenient for host-controlled bootstraps and cross-bundle handoff. -- Tempo.extend(...) is usually safer in app code because configuration is explicit, local, and easier to trace. +- `Tempo.extend(...)` is usually safer in app code because configuration is explicit, local, and easier to trace. - Use Global Discovery when you must configure Tempo before the first Tempo import executes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/doc/tempo.config.md` around lines 46 - 76, The documentation dropped inline code formatting for identifiers; wrap the identifiers `globalThis`, `Tempo.init()`, `Symbol.for('$Tempo')`, and `Tempo.extend(...)` (and any other code-like tokens in that section) in backticks so they render as code and match the rest of the doc's style; update the Pre-Bootstrap Discovery prose and the note about static ESM imports to use backticks for those symbols and ensure consistency with other sections.packages/tempo/doc/migration-guide.md (1)
89-96: Heading style inconsistent with neighboring section.The previous section is titled "Migrating from version 2.4" (line 80) while the new one uses "Migrating to version 2.6.0". Pick one preposition for consistency across the guide.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/doc/migration-guide.md` around lines 89 - 96, The section heading "## 🔁 Migrating to version 2.6.0" is inconsistent with the previous "Migrating from version 2.4"; pick a single preposition style for the guide and update the heading accordingly (e.g., change "Migrating to version 2.6.0" to "Migrating from version 2.6.0" or change the earlier "Migrating from version 2.4" to "Migrating to version 2.4") so headings match; edit the heading line text ("## 🔁 Migrating to version 2.6.0") to use the chosen preposition and ensure any other migration section headings follow the same convention.packages/tempo/src/plugin/term/term.season.ts (1)
19-37: Remove dead Chinese range entries and commented-out logic.With the CN derivation removed, the four
chineseentries on lines 19-23 are no longer consumed anywhere —resolveonly requestsgroup: 'meteorological', anddefinecallsgetTermRangeagainst that resolved meteorological list. Thechineseentries also lack asphere, sodefineRange(ranges, 'group', 'sphere')will produce achinese.undefinedgroup key that's never queryable through normalsphereconfig. Combined with the multi-line explanatory comment that retains commented-out code (lines 34-35), this leaves misleading artifacts in the source.Recommend deleting both the
chineseentries and the commented-out block; the migration guide already documents the supported path (separateTempoinstance with a ChinesetimeZone).♻️ Proposed cleanup
// Meteorological (South) { key: 'Spring', day: 1, month: 9, symbol: 'Flower', group: 'meteorological', sphere: COMPASS.South }, { key: 'Summer', day: 1, month: 12, symbol: 'Sun', group: 'meteorological', sphere: COMPASS.South }, { key: 'Autumn', day: 1, month: 3, symbol: 'Leaf', group: 'meteorological', sphere: COMPASS.South }, { key: 'Winter', day: 1, month: 6, symbol: 'Snowflake', group: 'meteorological', sphere: COMPASS.South }, - - // Chinese - { key: 'Spring', day: 1, month: 3, symbol: 'Flower', group: 'chinese', trait: 'A time of renewal and growth' }, - { key: 'Summer', day: 1, month: 6, symbol: 'Sun', group: 'chinese', trait: 'A period of heat and fruition' }, - { key: 'Autumn', day: 1, month: 9, symbol: 'Leaf', group: 'chinese', trait: 'A time for harvest and contraction' }, - { key: 'Winter', day: 1, month: 12, symbol: 'Snowflake', group: 'chinese', trait: 'A period of stillness and consolidation' }, ]; /** definition of meteorological season ranges */ const groups = defineRange(ranges, 'group', 'sphere'); /** resolve the full candidate list for the current context */ function resolve(t: Tempo, anchor?: any) { const list = resolveCycleWindow(t, groups, { anchor, groupBy: ['group', 'sphere'], group: 'meteorological' }); - - // append Chinese trait information as an additional metadata field (CN) - // const chinese = ranges.filter((g: any) => g.group === 'chinese'); - // list.forEach((itm: any) => itm['CN'] = getTermRange(t, chinese, false, anchor)); - // This was removed as it added complexity without clear value; the Chinese traits are already included in the base range definitions and - // should more-properly be surfaced by a new Tempo() with a Chinese-specific timeZone. return list; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/plugin/term/term.season.ts` around lines 19 - 37, The file contains dead Chinese season entries in the ranges array (entries with group: 'chinese') and a commented-out CN derivation block inside the resolve function; because resolve() calls resolveCycleWindow(..., group: 'meteorological') and defineRange(ranges, 'group', 'sphere') yields an unusable chinese.undefined key, those four chinese entries are unused and misleading—remove the four { group: 'chinese', ... } entries from the ranges list and delete the commented-out logic that attempted to append CN metadata (the commented getTermRange/filter block in resolve), leaving the meteorological ranges and resolve/defineRange usage intact; reference symbols: ranges, defineRange, resolve, resolveCycleWindow, getTermRange.packages/tempo/doc/architecture.md (1)
73-79: Define "proxy discovery" and explain when it occurs.Line 76 introduces the concept of "proxy discovery" that "pre-registers available keys as enumerable lazy getters," but the documentation doesn't explain:
- What triggers proxy discovery?
- When does it occur in the lifecycle?
- How does this relate to the lazy evaluation described in Section 1?
Adding a brief explanation or cross-reference would help readers understand this implementation detail.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/doc/architecture.md` around lines 73 - 79, Add a short definition of "proxy discovery" that states it is the proxy handler phase that enumerates available target keys and installs enumerable lazy getter properties on the proxy target without reading their values, and clarify that it is triggered by enumeration operations such as Object.keys(instance.fmt), for...in, and Reflect.ownKeys on the proxy; explain it happens at the moment those enumeration APIs run (i.e., before any property get), and cross-reference the lazy evaluation behavior in Section 1 by noting that discovery only registers getters while actual value computation and memoization happen later when the property getter is first invoked (e.g., when accessing instance.fmt.someKey).packages/tempo/plan/slick-syntax-duration-keys.md (8)
76-80: Expand testing strategy with additional edge cases.The testing strategy covers the main scenarios but could explicitly mention additional edge cases that are common sources of date/time bugs:
- DST (Daylight Saving Time) transitions
- Leap years (especially Feb 29 → Feb 28 transitions)
- End-of-month boundary conditions (e.g., Jan 31 → Feb 28/29)
- Cross-year boundaries (Dec → Jan)
Including these in the strategy ensures they won't be overlooked during implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/plan/slick-syntax-duration-keys.md` around lines 76 - 80, Update the "Testing Strategy (Future)" section to explicitly add edge-case test categories: DST transitions, leap-year cases (Feb 29 ↔ Feb 28), end-of-month rollovers (e.g., Jan 31 → Feb 28/29), and cross-year transitions (Dec → Jan); for each supported key (`wkd`, `dow`, and `week`) add table-driven tests covering these cases plus timezone-sensitive variants and regression checks that existing Slick term behavior remains unchanged.
19-20: Clarify the example semantics or use a simpler example.The example
t1.set({ week: ">2Mon" })is used before its semantics are defined. The meaning is ambiguous:
- "The 2nd Monday after today"?
- "The week containing the 2nd next Monday"?
- "Two weeks forward to Monday"?
Consider either:
- Using a Phase 1 example instead (
wkd: ">2"ordow: "next"), OR- Adding a brief semantic clarification here: e.g.,
t1.set({ week: ">2Mon" }) // advances to the 2nd following MondayThis would make the goal immediately clear without requiring readers to jump to line 52.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/plan/slick-syntax-duration-keys.md` around lines 19 - 20, The example t1.set({ week: ">2Mon" }) is ambiguous; either replace it with a simpler Phase 1 example like t1.set({ wkd: ">2" }) or t1.set({ dow: "next" }), or keep the current example but append a one‑sentence semantic clarification (e.g., "t1.set({ week: \">2Mon\" }) — advances to the 2nd following Monday") so readers immediately understand the intended meaning; update the markdown near the existing example string to reflect your chosen option and ensure the clarification references the exact example text ("t1.set({ week: \">2Mon\" })").
82-86: Consider providing recommended answers for critical questions.The open questions are well-identified, but Question 2 is particularly critical:
"Should symbolic modifiers (
>,>=,<,<=) map exactly to current term Slick semantics?"This question directly impacts user experience and consistency. Consider providing a recommended answer in the proposal:
"Recommendation: Yes, symbolic modifiers should map exactly to term Slick semantics to maintain consistency and minimize cognitive load. Any deviations would create confusion and make the mental model harder to learn."
Leaving this open risks inconsistent implementation that could be hard to fix later.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/plan/slick-syntax-duration-keys.md` around lines 82 - 86, Add a recommended answer for the Open Questions section specifically addressing Question 2: state that symbolic modifiers (`>`, `>=`, `<`, `<=`) should map exactly to current term Slick semantics to preserve consistency and reduce cognitive load, and include a brief justification and note about backward-compatibility and testing; update the document's Open Questions list (refer to "Question 2" and the symbolic modifiers `>`, `>=`, `<`, `<=`) to include this recommendation and a short implementation/testing action item.
63-67: Add example error messages to guardrails.Line 65 mentions "descriptive errors" but doesn't specify what those should look like. Adding examples would guide implementation:
Example error messages: - `Error: Slick syntax ">2Mon" not supported for key 'month'. Only numeric or object values allowed.` - `Error: Week anchor modifier '>=3Tue' requires explicit week anchoring policy. Feature not yet available.`This helps ensure consistent, helpful error communication when users try unsupported patterns.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/plan/slick-syntax-duration-keys.md` around lines 63 - 67, Update the "Suggested Semantics Guardrails" section to include concrete example error messages under the bullet "Unsupported key+pattern combinations should throw descriptive errors" so implementers have a template for consistent feedback; add the two suggested examples (one for invalid pattern for the 'month' key and one for an unsupported week-anchor modifier) and state they are illustrative templates to be used when throwing errors for key+pattern validation in the Slick syntax parsing logic.
52-53: Clarify phrasing.The phrase "advance to the second next Monday" is somewhat awkward. Consider clearer alternatives:
- "advance to the second following Monday"
- "advance to the Monday two weeks ahead"
- "navigate forward to the 2nd subsequent Monday"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/plan/slick-syntax-duration-keys.md` around lines 52 - 53, Rewrite the description for the `">2Mon"` contract to use clearer phrasing such as "advance to the second following Monday" (or "advance to the Monday two weeks ahead") instead of "advance to the second next Monday"; update the sentence that reads "`\">2Mon\" means advance to the second next Monday using a defined week anchor policy.`" to one of these clearer alternatives and keep the reference to the week anchor policy intact.
88-92: Solid recommendation with room for planning guidance.The conservative, phased recommendation aligns well with the risk mitigation strategy.
Optional enhancement: Consider adding rough timeline or priority guidance:
## Timeline & Priority (Optional) - Priority: Medium (nice-to-have, not blocking) - Estimated effort: - Phase 1 (`wkd`/`dow`): ~2-3 days (spec + impl + tests + docs) - Phase 2 (`week`): ~3-5 days (additional semantic complexity)This would help maintainers plan when to tackle this work relative to other priorities.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/plan/slick-syntax-duration-keys.md` around lines 88 - 92, Add an optional "Timeline & Priority" subsection to the document immediately after the existing "Recommendation" heading: include a Priority (e.g., Medium) and estimated effort bullets for Phase 1 (wkd/dow: ~2–3 days for spec + impl + tests + docs) and Phase 2 (week: ~3–5 days for added semantic complexity), so maintainers can prioritize and schedule work against other items.
69-74: Consider additional risk factors.The main risk (user confusion) is well-identified with good mitigations. Consider also noting:
Backward compatibility: Though unlikely, are there any edge cases where current code might already use patterns that would conflict with the new syntax?
Performance: Will parsing additional Slick patterns for unit keys introduce measurable overhead in hot paths?
These are likely low-risk but acknowledging them would make the risk assessment more comprehensive.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/plan/slick-syntax-duration-keys.md` around lines 69 - 74, Update the "Compatibility & Risk Notes" section to explicitly call out backward compatibility and performance risks: add a short bullet on backward compatibility describing to scan for existing Slick patterns that could collide with the new unit-key syntax, add compatibility tests and a deprecation/rollout strategy (e.g., opt-in flag, warnings), and add a short bullet on performance advising measurement of parser impact in hot paths, introducing benchmarks/profile results, and mitigating tactics (caching parse results, fast-path guards) to the existing Mitigation list so reviewers know how to validate and reduce those risks.
39-47: Consider adding concrete Phase 1 examples.Phase 1 identifies
wkdanddowas candidates but doesn't provide concrete example syntax. Adding examples would make the proposal more tangible:Examples for Phase 1: - `t1.set({ wkd: "Mon" })` or `t1.set({ wkd: "Monday" })` - `t1.set({ wkd: "next" })` // next occurrence of current weekday - `t1.set({ dow: ">2" })` // advance 2 weekdays forwardThis would help readers visualize the feature and enable early validation of the syntax design.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/plan/slick-syntax-duration-keys.md` around lines 39 - 47, Add concrete examples to the Phase 1 section showing how the new weekday keys work: include sample usages for the keys `wkd` and `dow` such as using `t1.set` to assign a short or full weekday name (e.g., Mon/Monday), a relative token like "next" to pick the next occurrence of the current weekday, and a numeric-relative form for `dow` (e.g., a greater-than offset like >2 to advance N weekdays); place these illustrative examples directly under the "Phase 1 (Recommended First Step)" heading so reviewers can see expected syntax and semantics for `wkd` and `dow`.packages/library/src/common/temporal.library.ts (1)
128-134: Optional: tighten offset bounds validation.The regex accepts any 1–2 digit hour (
\d{1,2}→ 0–99) and any 2 digit minute (\d{2}→ 0–99), so inputs such asUTC+25orUTC+8:75are reformatted to syntactically valid±HH:MMstrings that Temporal will then reject. Failing fast (or returning the original string) for clearly out-of-range values gives callers a more localized error site.♻️ Suggested guard
export function normalizeUtcOffset(zone: string): string { const match = /^UTC([+-])(\d{1,2})(?::(\d{2}))?$/i.exec(zone); if (!match) return zone; const [, sign, hours, minutes] = match; - return `${sign}${hours.padStart(2,'0')}:${minutes ?? '00'}`; + const h = Number(hours); + const m = Number(minutes ?? '0'); + // 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; + return `${sign}${hours.padStart(2,'0')}:${minutes ?? '00'}`; }packages/tempo/src/support/tempo.init.ts (1)
141-145:layoutOrdercoercion duplicated intempo.class.ts.The exact same
asArray(...).map(String/trim).filter(Boolean)pipeline appears intempo.class.tsat lines 386-388. Consider extracting a small helper (e.g.normalizeLayoutOrderintempo.util.ts) so bothextendStateand[$setConfig]stay in lockstep when the rules evolve (e.g. dedup, lower-casing, alias resolution).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/support/tempo.init.ts` around lines 141 - 145, The pipeline that coerces layoutOrder (asArray(...).map(String/trim).filter(Boolean)) is duplicated; create a small helper function normalizeLayoutOrder in tempo.util.ts that accepts the raw value and returns the cleaned string array, then replace the inline pipeline in tempo.init.ts (state.parse.layoutOrder assignment) and the duplicate code in tempo.class.ts (the [$setConfig]/extendState logic) to call normalizeLayoutOrder so both places share the same normalization logic going forward.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/tempo/doc/architecture.md`:
- Around line 69-72: Update the architecture doc to explicitly define "delegator
Proxy" (how it forwards traps to target/handler vs a standard Proxy), and
reconcile its relationship to the "shadowing" lazy evaluation described for
Tempo.#term and Tempo.#fmt by stating whether shadowing (prototype/private-field
replacement) and the "Generic Lazy Delegator Proxy" are distinct mechanisms, how
they interact (e.g., delegator Proxies handle public accessor delegation while
lazy shadowing initializes private fields once), or that one replaces the other;
reference the terms "delegator Proxy", "Generic Lazy Delegator Proxy", and
Tempo.#term/Tempo.#fmt in the same paragraph and add a concise sentence stating
which mechanism is used where (public API vs private fields) so readers can see
if both coexist or one supersedes the other.
In `@packages/tempo/doc/releases/v2.x.md`:
- Line 5: There's a stray closing brace in the markdown code span for the second
Tempo example; change the snippet `{timeZone: '+08:00'}}` to `{timeZone:
'+08:00'}` so it matches the first example `{timeZone: 'Asia/Shanghai'}` and
renders correctly in packages/tempo/doc/releases/v2.x.md (look for the text
mentioning "create a dedicated `Tempo` instance").
In `@packages/tempo/doc/tempo.plugin.md`:
- Around line 29-33: The JSDoc block has a misaligned asterisk on the `options`
line causing the comment to render crooked; edit the comment so the `*` on the
`options` JSDoc line matches the other lines (use the same leading spacing as
the `TempoClass` and `factory` lines) in the comment that documents
TempoClass/factory/options.
---
Outside diff comments:
In `@packages/tempo/src/engine/engine.layout.ts`:
- Around line 64-70: preferred loop can push the same entry twice when preferred
mixes aliases and descriptions (TOKEN_DESCRIPTION_BY_NAME, byName -> entry),
which causes next.length to exceed entries.length and the reordering guard to
fail; change the loop in the preferred.forEach to skip entries whose identifier
is already in seen (i.e., check if seen.has(entry[0]) and continue if true)
before adding to seen and pushing to next so duplicate aliases/descriptions are
ignored and the final guard can succeed to return the reordered layout.
In `@packages/tempo/src/plugin/term/term.season.ts`:
- Line 49: Fix the typo in the term description by changing the description
property value from "Meteorogical season" to "Meteorological season" in the term
definition (the description field in
packages/tempo/src/plugin/term/term.season.ts); update the description string so
it matches the correctly spelled comment and user-facing text.
In `@packages/tempo/src/support/tempo.init.ts`:
- Around line 147-153: Both configuration entry points must normalize UTC offset
strings the same way: change the logic in the other path (tempo.class.ts where
it currently does enums.TIMEZONE[zone] ?? arg.value) to mirror this file’s
behavior by using enums.TIMEZONE[zone] ?? normalizeUtcOffset(String(arg.value))
before calling setProperty(state.config, 'timeZone', ...) and
getHemisphere(...); ensure you import or reference normalizeUtcOffset and use
the resolvedZone variable the same way as in tempo.init.ts so timeZone
normalization is consistent across extendState/[$setConfig] and tempo.init.ts.
---
Nitpick comments:
In `@packages/tempo/doc/architecture.md`:
- Around line 73-79: Add a short definition of "proxy discovery" that states it
is the proxy handler phase that enumerates available target keys and installs
enumerable lazy getter properties on the proxy target without reading their
values, and clarify that it is triggered by enumeration operations such as
Object.keys(instance.fmt), for...in, and Reflect.ownKeys on the proxy; explain
it happens at the moment those enumeration APIs run (i.e., before any property
get), and cross-reference the lazy evaluation behavior in Section 1 by noting
that discovery only registers getters while actual value computation and
memoization happen later when the property getter is first invoked (e.g., when
accessing instance.fmt.someKey).
In `@packages/tempo/doc/migration-guide.md`:
- Around line 89-96: The section heading "## 🔁 Migrating to version 2.6.0" is
inconsistent with the previous "Migrating from version 2.4"; pick a single
preposition style for the guide and update the heading accordingly (e.g., change
"Migrating to version 2.6.0" to "Migrating from version 2.6.0" or change the
earlier "Migrating from version 2.4" to "Migrating to version 2.4") so headings
match; edit the heading line text ("## 🔁 Migrating to version 2.6.0") to use
the chosen preposition and ensure any other migration section headings follow
the same convention.
In `@packages/tempo/doc/tempo.config.md`:
- Around line 46-76: The documentation dropped inline code formatting for
identifiers; wrap the identifiers `globalThis`, `Tempo.init()`,
`Symbol.for('$Tempo')`, and `Tempo.extend(...)` (and any other code-like tokens
in that section) in backticks so they render as code and match the rest of the
doc's style; update the Pre-Bootstrap Discovery prose and the note about static
ESM imports to use backticks for those symbols and ensure consistency with other
sections.
In `@packages/tempo/plan/slick-syntax-duration-keys.md`:
- Around line 76-80: Update the "Testing Strategy (Future)" section to
explicitly add edge-case test categories: DST transitions, leap-year cases (Feb
29 ↔ Feb 28), end-of-month rollovers (e.g., Jan 31 → Feb 28/29), and cross-year
transitions (Dec → Jan); for each supported key (`wkd`, `dow`, and `week`) add
table-driven tests covering these cases plus timezone-sensitive variants and
regression checks that existing Slick term behavior remains unchanged.
- Around line 19-20: The example t1.set({ week: ">2Mon" }) is ambiguous; either
replace it with a simpler Phase 1 example like t1.set({ wkd: ">2" }) or t1.set({
dow: "next" }), or keep the current example but append a one‑sentence semantic
clarification (e.g., "t1.set({ week: \">2Mon\" }) — advances to the 2nd
following Monday") so readers immediately understand the intended meaning;
update the markdown near the existing example string to reflect your chosen
option and ensure the clarification references the exact example text ("t1.set({
week: \">2Mon\" })").
- Around line 82-86: Add a recommended answer for the Open Questions section
specifically addressing Question 2: state that symbolic modifiers (`>`, `>=`,
`<`, `<=`) should map exactly to current term Slick semantics to preserve
consistency and reduce cognitive load, and include a brief justification and
note about backward-compatibility and testing; update the document's Open
Questions list (refer to "Question 2" and the symbolic modifiers `>`, `>=`, `<`,
`<=`) to include this recommendation and a short implementation/testing action
item.
- Around line 63-67: Update the "Suggested Semantics Guardrails" section to
include concrete example error messages under the bullet "Unsupported
key+pattern combinations should throw descriptive errors" so implementers have a
template for consistent feedback; add the two suggested examples (one for
invalid pattern for the 'month' key and one for an unsupported week-anchor
modifier) and state they are illustrative templates to be used when throwing
errors for key+pattern validation in the Slick syntax parsing logic.
- Around line 52-53: Rewrite the description for the `">2Mon"` contract to use
clearer phrasing such as "advance to the second following Monday" (or "advance
to the Monday two weeks ahead") instead of "advance to the second next Monday";
update the sentence that reads "`\">2Mon\" means advance to the second next
Monday using a defined week anchor policy.`" to one of these clearer
alternatives and keep the reference to the week anchor policy intact.
- Around line 88-92: Add an optional "Timeline & Priority" subsection to the
document immediately after the existing "Recommendation" heading: include a
Priority (e.g., Medium) and estimated effort bullets for Phase 1 (wkd/dow: ~2–3
days for spec + impl + tests + docs) and Phase 2 (week: ~3–5 days for added
semantic complexity), so maintainers can prioritize and schedule work against
other items.
- Around line 69-74: Update the "Compatibility & Risk Notes" section to
explicitly call out backward compatibility and performance risks: add a short
bullet on backward compatibility describing to scan for existing Slick patterns
that could collide with the new unit-key syntax, add compatibility tests and a
deprecation/rollout strategy (e.g., opt-in flag, warnings), and add a short
bullet on performance advising measurement of parser impact in hot paths,
introducing benchmarks/profile results, and mitigating tactics (caching parse
results, fast-path guards) to the existing Mitigation list so reviewers know how
to validate and reduce those risks.
- Around line 39-47: Add concrete examples to the Phase 1 section showing how
the new weekday keys work: include sample usages for the keys `wkd` and `dow`
such as using `t1.set` to assign a short or full weekday name (e.g.,
Mon/Monday), a relative token like "next" to pick the next occurrence of the
current weekday, and a numeric-relative form for `dow` (e.g., a greater-than
offset like >2 to advance N weekdays); place these illustrative examples
directly under the "Phase 1 (Recommended First Step)" heading so reviewers can
see expected syntax and semantics for `wkd` and `dow`.
In `@packages/tempo/src/plugin/term/term.season.ts`:
- Around line 19-37: The file contains dead Chinese season entries in the ranges
array (entries with group: 'chinese') and a commented-out CN derivation block
inside the resolve function; because resolve() calls resolveCycleWindow(...,
group: 'meteorological') and defineRange(ranges, 'group', 'sphere') yields an
unusable chinese.undefined key, those four chinese entries are unused and
misleading—remove the four { group: 'chinese', ... } entries from the ranges
list and delete the commented-out logic that attempted to append CN metadata
(the commented getTermRange/filter block in resolve), leaving the meteorological
ranges and resolve/defineRange usage intact; reference symbols: ranges,
defineRange, resolve, resolveCycleWindow, getTermRange.
In `@packages/tempo/src/support/tempo.init.ts`:
- Around line 141-145: The pipeline that coerces layoutOrder
(asArray(...).map(String/trim).filter(Boolean)) is duplicated; create a small
helper function normalizeLayoutOrder in tempo.util.ts that accepts the raw value
and returns the cleaned string array, then replace the inline pipeline in
tempo.init.ts (state.parse.layoutOrder assignment) and the duplicate code in
tempo.class.ts (the [$setConfig]/extendState logic) to call normalizeLayoutOrder
so both places share the same normalization logic going forward.
In `@packages/tempo/test/season_metadata.test.ts`:
- Around line 4-9: The test for Tempo.term.season currently asserts the positive
case for q.key but doesn't guard against reintroducing the removed CN field;
update the test in season_metadata.test.ts (where Tempo('2024-05-15', { sphere:
'north' }) and q = t.term.season are used) to include a negative assertion that
q.CN is undefined or otherwise not present (e.g., expect(q.CN).toBeUndefined()
or expect('CN' in q).toBe(false)) so the regression test fails if the CN
property is ever reintroduced.
🪄 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: 671fb3c4-e837-4337-ba23-67984116061e
📒 Files selected for processing (24)
package.jsonpackages/library/package.jsonpackages/library/src/common/temporal.library.tspackages/tempo/doc/architecture.mdpackages/tempo/doc/migration-guide.mdpackages/tempo/doc/releases/v2.x.mdpackages/tempo/doc/sandbox-factory.mdpackages/tempo/doc/tempo.config.mdpackages/tempo/doc/tempo.modularity.mdpackages/tempo/doc/tempo.plugin.mdpackages/tempo/package.jsonpackages/tempo/plan/slick-syntax-duration-keys.mdpackages/tempo/src/discrete/discrete.parse.tspackages/tempo/src/engine/engine.layout.tspackages/tempo/src/plugin/term/term.season.tspackages/tempo/src/support/tempo.default.tspackages/tempo/src/support/tempo.enum.tspackages/tempo/src/support/tempo.init.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.type.tspackages/tempo/test/engine.layout.test.tspackages/tempo/test/layout.order.test.tspackages/tempo/test/sandbox-factory.test.tspackages/tempo/test/season_metadata.test.ts
magmacomputing
left a comment
magmacomputing
left a comment
There was a problem hiding this comment.
written to Tempo standard, test-cases 100% pass rate
Correct the wording on Tempo.create functionality
Summary by CodeRabbit
Release Notes v2.6.0
New Features
normalizeUtcOffsetutility for transforming informal UTC-offset strings to standard format.layoutOrderoption to customize parsing element precedence.Breaking Changes
Bug Fixes
Documentation