Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughBumps packages to v2.5.0; adds a comprehensive assertion library; consolidates proxy/freezing/unwrap behavior; refactors symbol branding to a shared Identity symbol; introduces deterministic layout-order engine and Tempo.create sandbox factory; reorganizes module/plugin paths with widespread import rewiring, tests, and docs updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant TempoBase as Tempo
participant Sandbox as SandboxSubclass
participant State as LocalState
participant Registry as LocalRegistry
Client->>TempoBase: Tempo.create(options)
TempoBase->>State: clone/init parse & registries
TempoBase->>Registry: clone or bind local registry view
TempoBase->>Sandbox: create derived subclass bound to State/Registry
TempoBase->>Sandbox: freeze/harden subclass
TempoBase-->>Client: return Sandbox subclass
Client->>Sandbox: new Sandbox(input)
Sandbox->>Registry: resolve aliases (local-first)
Registry-->>Sandbox: match + source:'local'
Sandbox->>State: parse using local config
State-->>Client: parsing result (with source metadata)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
…controller framework
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
packages/library/src/common/logify.class.ts (1)
35-70:⚠️ Potential issue | 🔴 CriticalCritical: default
debug:falsenow silently drops.log/.info/.warn/.debug, and a low numericdebugcan also suppress.errorrethrows.Two behavioral problems in the new severity gate:
Default regression. The constructor defaults
debugtofalse(Line 95). With the new mapping,debug:falseproducescurrentLevel = Level[Error] = 1, so.log()(3),.info()(3),.warn()(2), and.debug()(4) all early-return at Line 40. Previously only.debug()was gated on a falsydebug; all other methods emitted. Existing callers that rely on plain.log()/.warn()output will now see nothing by default.Rethrow suppression. The early
returnon Line 40 short-circuits the entire#trapbody, including themethod === Method.Error && !config.catchrethrow at Lines 62–70. If a caller setsdebug: 0(a validnumberper the new type) they silently swallow every error — losing both console output and the exception, which violates the comment at Lines 32–34.🛠️ Suggested fix
Keep the severity filter for console output only, and preserve the old default (info-and-above visible):
`#trap`(method: Logify.Method, ...msg: any[]) { const config = (isObject(msg[0]) && (msg[0] as any)[sym.$Logify] === true) ? msg.shift() : this.#opts; - const currentLevel = (typeof config.debug === 'number') ? config.debug : (config.debug ? Level[Method.Debug] : Level[Method.Error]); + // default (debug===false) keeps legacy behaviour: show everything except Debug. + const currentLevel = (typeof config.debug === 'number') + ? config.debug + : (config.debug ? Level[Method.Debug] : Level[Method.Info]); const methodLevel = Level[method] ?? 0; - if (methodLevel > currentLevel) return; - - - const output = msg.map(m => { + const shouldEmit = methodLevel <= currentLevel; + const output = msg.map(m => { if (m instanceof Error) return m.message; ... }).filter(s => !isEmpty(s)).join(' '); - if (!config.silent && !isEmpty(output)) + if (shouldEmit && !config.silent && !isEmpty(output)) (console as any)[method](`${this.#name}: ${output}`); if (method === Method.Error && !config.catch) {This keeps the rethrow path unconditional for
.error()(matching the documented contract) and restores the previous default UX.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/library/src/common/logify.class.ts` around lines 35 - 70, The severity gate currently maps config.debug=false to Level.Error and early-returns from `#trap`, which silently drops non-debug logs and suppresses error rethrows; fix by (1) changing the currentLevel mapping so that typeof config.debug === 'number' uses that number, config.debug === true maps to Level[Method.Debug], and falsy/non-number (including false/undefined) maps to Level[Method.Info] to restore prior defaults, and (2) remove the early return that exits the whole `#trap`; instead only skip the console emission by checking if methodLevel > currentLevel immediately before the console call (keep the error path: the Method.Error && !config.catch rethrow must still execute unconditionally).packages/tempo/src/plugin/plugin.util.ts (1)
99-105:⚠️ Potential issue | 🟡 MinorSilent collision when
$logErrorisn't registered; drop the commentedconsole.error.With the
console.error(msg)fallback removed, ifTempoClass[sym.$logError]is not yet a function (e.g.,attachStaticsruns before the logger/extension is wired, or in a stripped-down host), a static name collision is silently skipped viacontinue. That removes the only diagnostic signal for an otherwise easy-to-miss conflict. Either restore theconsole.erroras a fallback or at least delete the dead line 103 to make intent explicit.♻️ Suggested fallback + dead-code cleanup
if (Object.hasOwn(TempoClass, key)) { const msg = `Static name collision on "${key}". Property is already defined on the host class.`; if (isFunction(TempoClass[sym.$logError])) { // use catch:true to report the collision without a fatal throw (supports re-extension in shared environments) TempoClass[sym.$logError]({ ...TempoClass.config, catch: true }, msg); + } else { + console.error(msg); } - // console.error(msg); continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/plugin/plugin.util.ts` around lines 99 - 105, The collision-detection branch silently skips reporting when TempoClass[sym.$logError] isn't registered; update the attach logic around TempoClass and sym.$logError so that if isFunction(TempoClass[sym.$logError]) is false you still emit a diagnostic (restore a console.error fallback that logs a clear message including msg and TempoClass.config or remove the stray commented console.error to make intent explicit); ensure the fallback uses the same context info (TempoClass.config and msg) so collisions are visible even when the extension logger isn't wired.packages/tempo/src/discrete/discrete.parse.ts (1)
326-336:⚠️ Potential issue | 🟡 Minor
prevZdtis captured but never restored — either use it or drop it.Line 329 saves
const prevZdt = state.zdt;, but thefinallyblock (lines 477–482) unconditionally overwritesstate.zdtwith the mutateddateTimeand never referencesprevZdt. If the intent is to scopestate.zdtto the current parse frame (matching theprevAnchorhandling right next to it), you should restore it:} finally { state.anchor = prevAnchor; - if (isZonedDateTime(dateTime)) state.zdt = dateTime; + state.zdt = isZonedDateTime(dateTime) ? dateTime : prevZdt; state.parseDepth--; if (state.parseDepth === 0) state.matches = undefined; }If the current behavior (latest dateTime persists out of the frame) is intentional, drop
prevZdtto remove the misleading capture.Also applies to: 477-482
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/discrete/discrete.parse.ts` around lines 326 - 336, In parseGroups, the local const prevZdt = state.zdt is captured but never used; either restore state.zdt from prevZdt in the finally block (so state.zdt is scoped to the parse frame like state.anchor) or remove the unused prevZdt declaration to avoid confusion — update the finally that currently overwrites state.zdt with the mutated dateTime to instead set state.zdt = prevZdt (or delete prevZdt if you choose to persist the mutated zdt); refer to parseGroups, prevZdt, prevAnchor and the finally block that resets state.anchor/state.zdt for locating the change.packages/tempo/index.md (1)
252-300:⚠️ Potential issue | 🟡 Minor
resumeTimerisn't cleared on unmount.
handleVisibilityclearsresumeTimeron each invocation, butonUnmounteddoesn't. If the user navigates away within the 1200 ms grace window, the pending timeout will still fire on an unmounted component, callingstartTicker()and setting reactive refs on a torn-down view.onUnmounted(() => { isMounted = false ticker?.stop() if (fallbackIntervalId) clearInterval(fallbackIntervalId) clearInterval(carouselTimer) + if (resumeTimer) clearTimeout(resumeTimer) document.removeEventListener('visibilitychange', handleVisibility) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/index.md` around lines 252 - 300, The component doesn't clear the pending resumeTimer on unmount which can fire after teardown; update the onUnmounted block to clearTimeout(resumeTimer) (guarded by a truthy check) and set resumeTimer = null, and ensure any state-setting in the timeout (startTicker(), isResuming, tickerActive) won't run after unmount by relying on isMounted or the cleared timer; reference symbols: resumeTimer, handleVisibility, onUnmounted, startTicker, isResuming, tickerActive, clearTimeout.packages/tempo/src/support/tempo.default.ts (1)
144-156:⚠️ Potential issue | 🟡 Minor
half hourdoesn't fit the Period contract.Every other Period entry resolves to a clock string (e.g.
'24:00','8:00','18:00') representing a time-of-day.half[ -]?hourinstead returnsthis.add({ minutes: 30 }), which is a relative duration, not a time-of-day — effectively a relative-offset behavior that belongs in Event (matchingtomorrow/yesterday's pattern) or in the{rel}layout, not in{per}. Downstream{tm}matching treats Period as a clock-reading alternative, so a phrase like "at half hour" will route through the wrong resolution path and interact strangely with subsequent date parsing.Please either (a) move the entry to
Event, or (b) have it return a clock string like other Period entries.🤖 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 144 - 156, The Period map contains an entry 'half[ -]?hour' that returns a relative duration (function using this.add({ minutes: 30 })) which violates Period's time-of-day contract; either remove that key from Period and add it to the Event mapping (so the function stays as Event's handler and uses Tempo.add to produce a relative offset), or change the Period entry to return a concrete clock string like '00:30'/'0:30' (matching other Period values) so it resolves as a time-of-day; update the mapping for the symbol Period and the key 'half[ -]?hour' accordingly and ensure downstream {per}/{tm} behavior is consistent.packages/tempo/src/support/tempo.enum.ts (1)
246-265:⚠️ Potential issue | 🟡 Minor
LOGis declared but not included in the defaultenumsexport.The new numeric
LOGenum is exported as a named export on Line 18 and its type on Lines 26-27, but it's absent from the default-export registry below. Iftempo.type.tsreally does typeInternal.BaseOptions.debugasboolean | enums.LOG(per the AI summary), consumers accessing it via the public-reachable registry — orTempo.LOGvia a static getter — will receiveundefined. Add it to keep symmetry with the other enum entries:OPTION, MODE, PARSE, + LOG, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/support/tempo.enum.ts` around lines 246 - 265, The default export object in tempo.enum.ts is missing the LOG entry, so add LOG to the exported registry (alongside SEASON, COMPASS, etc.) to ensure Tempo.LOG and any consumers using the public enums registry receive the numeric LOG enum; update the default export list to include LOG exactly as the other symbols are listed and verify the named export/type for LOG remains intact so tempo.type.ts consumers still accept enums.LOG.
🧹 Nitpick comments (21)
packages/tempo/src/tempo.index.ts (1)
5-6: Consider matching import order to usage order.The imports are now ordered as Parse, Format, Mutate, Duration, Terms, but the
corearray on Line 13 uses them in the order Parse, Mutate, Format, Duration, Terms. Aligning the import order with usage order can improve readability.📝 Optional refactor for consistency
import { ParseModule } from '#tempo/parse'; -import { FormatModule } from '#tempo/format'; - import { MutateModule } from '#tempo/mutate'; +import { FormatModule } from '#tempo/format'; import { DurationModule } from '#tempo/duration';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/tempo.index.ts` around lines 5 - 6, Reorder the import statements so they follow the same sequence used in the core array to improve readability: import ParseModule first, then MutateModule, then FormatModule, then DurationModule, then TermsModule (matching the usage order in the core array). Update the import lines that reference FormatModule and the other tempo modules to match this order so the file imports align with the core array ordering.packages/library/src/common/coercion.library.ts (1)
73-75: LGTM — tiny naming caveat.
??coalesces bothnullandundefined, so these helpers apply to nullish values generally rather than onlynull. That matches the usual convention, butnullishToZero/nullishToEmptywould be technically more accurate. Feel free to ignore.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/library/src/common/coercion.library.ts` around lines 73 - 75, Rename the helpers to reflect that they operate on nullish (null or undefined) values: change nullToZero -> nullishToZero, nullToEmpty -> nullishToEmpty, and optionally nullToValue -> nullishToValue; update all exports and any call sites that reference the functions (search for nullToZero, nullToEmpty, nullToValue) and adjust imports/usage accordingly to avoid breaking references, keeping the original behavior (obj ?? ...).packages/tempo/test/term_unified.test.ts (1)
91-96: Good coverage — consider one additional case to strengthen the contract.The test confirms
'SouthSouthWest'maps onto the'south'range, which validates substring matching. Since the AI summary states matching is case-insensitive substring, you could strengthen the contract by also asserting an all-caps / all-lowercase variant (e.g.'SOUTH'or'southsouthwest') — otherwise a regression that dropstoLowerCase()but keeps.includes()would still pass this case (the literal substringsouthalready appears verbatim).Suggested additional assertion
it('should support granular sphere matching (e.g. SouthSouthWest)', () => { // Even though ranges are defined for 'south', 'SouthSouthWest' should match it via .includes() const t = new Tempo(testDate, { catch: true, sphere: 'SouthSouthWest' }); expect(t.format('{`#qtr`}')).toBe('Q4'); expect(t.set({ start: '#quarter' }).format('{yyyy}-{mm}-{dd}')).toBe('2024-04-01'); + + // Also verify case-insensitivity with an all-caps variant that does NOT contain the + // literal lowercase substring 'south' — this guards against a regression that drops + // the .toLowerCase() normalisation but keeps .includes(). + const u = new Tempo(testDate, { catch: true, sphere: 'SOUTH' }); + expect(u.format('{`#qtr`}')).toBe('Q4'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/test/term_unified.test.ts` around lines 91 - 96, Add an extra assertion to ensure sphere matching is case-insensitive: when creating a Tempo instance (new Tempo(testDate, { catch: true, sphere: ... })) add a second case using an all-caps or all-lowercase sphere value (e.g., 'SOUTH' or 'southsouthwest') and assert the same outputs via t.format('{`#qtr`}') and t.set({ start: '#quarter' }).format('{yyyy}-{mm}-{dd}'), so that the matching logic in Tempo (constructor handling of sphere and its includes() check) is verified for case-insensitivity as well as substring matching.packages/tempo/doc/tempo.cookbook.md (1)
56-59: Consider noting that the hhmiss-vs-compact-date precedence is timezone-independent.The bullet says 6-digit is checked as compact time first; the adjacent examples mix
timeZone: 'UTC'(for the time) andtimeZone: 'Europe/London'(for the date fallback), which may read as if timezone participates in the first decision. A short clarification that timezone only influences the compact-date fallback (not the hhmiss validity check) would prevent misreading.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/doc/tempo.cookbook.md` around lines 56 - 59, Clarify that the 6-digit hhmiss-vs-compact-date precedence is timezone-independent: update the paragraph describing 6-digit parsing to state explicitly that Tempo always first validates the 6-digit string as compact time (`hhmiss`) regardless of `timeZone`, and that `timeZone` only affects interpretation when falling back to compact date layouts (e.g., `ddmmyy` vs `mmddyy`); reference `hhmiss` and the `timeZone` option so readers see which decision is timezone-agnostic and which is not.packages/tempo/CHANGELOG.md (1)
8-15: Consider cutting a version header before release.PR objectives describe a 2.4.0 → 2.5.0 bump, but the changelog currently parks this as
[Unreleased]and the history jumps from2.2.x→2.3.0with no2.4.0entry. Before tagging, replace## [Unreleased]with the concrete target version and date so consumers can tie the alias-precedence behavioral change (which is a breaking parse-output change in overlap cases) to a specific release.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/CHANGELOG.md` around lines 8 - 15, Replace the placeholder header "## [Unreleased]" with a concrete release header for the target bump (e.g. "## [2.5.0] - YYYY-MM-DD") so the "Alias Precedence" change is tied to a specific release; also verify the changelog sequence around "2.2.x", "2.3.0" and the missing "2.4.0" entry and add a short 2.4.0 history line if that version was released, ensuring the final changelog reflects the 2.4.0 → 2.5.0 bump and includes the release date.packages/tempo/src/engine/engine.term.ts (1)
2-4: Consolidate duplicate imports from#library/assertion.library.js.
isDefined,isString,isZonedDateTimeandisNumericare imported from the same module across two separate statements. Combine them into one import for cleanliness and to avoid future churn/merge noise.♻️ Proposed fix
import { toZonedDateTime, toInstant } from '#library/temporal.library.js'; -import { isDefined, isString, isZonedDateTime } from '#library/assertion.library.js'; +import { isDefined, isNumeric, isString, isZonedDateTime } from '#library/assertion.library.js'; import { asArray } from '#library/coercion.library.js'; -import { isNumeric } from '#library/assertion.library.js';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/engine/engine.term.ts` around lines 2 - 4, Combine the duplicate imports from the same module by replacing the two separate import statements that bring in isDefined, isString, isZonedDateTime and isNumeric from '#library/assertion.library.js' with a single import declaration that imports all four symbols (isDefined, isString, isZonedDateTime, isNumeric) together, and remove the redundant import; update the import group that also contains asArray from '#library/coercion.library.js' only if necessary (leave as-is otherwise).packages/tempo/src/plugin/extend/extend.ticker.ts (1)
1-4: Duplicate import source — merge into one statement.Lines 1 and 4 both import from
#library/assertion.library.js. Consolidating avoids churn and keeps the module list tidy.♻️ Proposed dedupe
-import { isObject, isFunction, isDefined, isUndefined, isEmpty, isNumber } from '#library/assertion.library.js' +import { isObject, isFunction, isDefined, isUndefined, isEmpty, isNumber, isNumeric } from '#library/assertion.library.js' import { Pledge } from '#library/pledge.class.js' import { asArray } from '#library/coercion.library.js' -import { isNumeric } from '#library/assertion.library.js' import { instant, normaliseFractionalDurations } from '#library/temporal.library.js'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/plugin/extend/extend.ticker.ts` around lines 1 - 4, The file imports from '#library/assertion.library.js' twice; consolidate the two import statements into a single import that includes all needed symbols (isObject, isFunction, isDefined, isUndefined, isEmpty, isNumber, isNumeric) so there is only one import from '#library/assertion.library.js' and remove the duplicate import line; update the top-of-file imports to reference Pledge and asArray as before and keep the single combined assertion.library import.packages/library/src/common/utility.library.ts (1)
68-80:skipis read-only insidedeepFreeze— document or rename.
skipis never added to; only checked on entry. That's intentional (caller pre-populates with objects it wants preserved mutable), but the signature alone suggests it behaves likeseen. Two small improvements:
- Add a JSDoc line clarifying
skipis an opt-out set the caller owns, distinct from the internal cycle trackerseen.- Consider making
skipa required first-class named option (e.g.,deepFreeze(obj, { skip })) so it isn't confused positionally withseenby callers.Also: the default
skip = new WeakSet()is allocated on every top-level call even when unused — negligible, but a shared sentinelEMPTY_SKIPwould make intent obvious.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/library/src/common/utility.library.ts` around lines 68 - 80, The deepFreeze function's skip parameter is currently read-only and confusingly positioned; update deepFreeze to document and/or convert skip into an explicit caller-owned opt-out (either via a JSDoc comment and a clearer name or by changing the signature to deepFreeze(obj, { skip }?: { skip?: WeakSet<object> }), replace the per-call default new WeakSet() with a shared EMPTY_SKIP sentinel to avoid allocations, and ensure seen remains the internal cycle-tracker; add a JSDoc above deepFreeze explaining that skip is an externally owned opt-out set (not mutated by deepFreeze) and reference the symbols deepFreeze, skip, seen, and any new EMPTY_SKIP constant to locate the change.packages/tempo/test/engine.layout.test.ts (1)
14-96: LGTM — thorough resolver coverage.The
toBe(layout)checks (lines 23, 94) implicitly lock in thatresolveLayoutOrder/resolveLayoutClassificationOrderreturn the original reference on no-op/fallback paths. Worth a short comment in the resolver noting this contract so it isn't accidentally broken by a future “always return a new object” refactor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/test/engine.layout.test.ts` around lines 14 - 96, Tests rely on referential equality (expect(...).toBe(layout)) so ensure resolveLayoutOrder and resolveLayoutClassificationOrder preserve and return the original layout object when no changes are needed; update those functions to explicitly return the input layout when no swaps/classification occur and add a short in-code comment above resolveLayoutOrder and resolveLayoutClassificationOrder documenting this contract (i.e., "must return the original reference for no-op paths to satisfy tests/consumers expecting referential equality").packages/library/src/common/primitive.library.ts (1)
16-23: Add a cycle/depth guard tounwrap— currently hangable on self-referential proxies.The loop terminates only when
curr[sym.$Target](or the string-key fallback) is falsy. A proxy whose$Targetreturns the proxy itself — or any cycle of length > 1 — produces an infinite loop, because each iteration re-observes a truthy$Target.ownEntriesalready caps prototype climbing at 50 for the same reason;unwrapshould do the same.🛡️ Proposed guard
export function unwrap<T extends object>(obj: T): T { let curr = (obj as any)?.[sym.$Target] ?? obj; - // Hardened against different Symbol instances across module boundaries - while (curr && (Object.prototype.hasOwnProperty.call(curr, sym.$Target) || (curr as any).$Target)) { - curr = curr[sym.$Target] ?? (curr as any).$Target; - } + // Hardened against different Symbol instances across module boundaries + // and against self-referential / cyclic $Target chains. + const seen = new Set<any>([curr]); + let depth = 0; + while (curr && depth++ < 50 + && (Object.prototype.hasOwnProperty.call(curr, sym.$Target) || (curr as any).$Target)) { + const next = curr[sym.$Target] ?? (curr as any).$Target; + if (!next || seen.has(next)) break; + seen.add(next); + curr = next; + } return curr; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/library/src/common/primitive.library.ts` around lines 16 - 23, The unwrap function can loop forever on self-referential or cyclic $Target chains; add a depth/cycle guard (e.g., maxDepth = 50) inside unwrap to break and return the current value when the guard is reached. Modify the while loop in function unwrap to increment a counter each iteration and stop when the counter exceeds the guard, ensuring you still check both symbol-based (sym.$Target) and string-key ('$Target') fallbacks; reference the existing symbols/fields used in unwrap (curr, sym.$Target, (curr as any).$Target) so the guard integrates without changing the lookup logic.packages/tempo/src/discrete/discrete.parse.ts (3)
21-23: Trailing blank-line block left over from refactor.Three consecutive blank lines right after the imports look like a merge artifact. Collapse to a single blank line for consistency with the rest of the file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/discrete/discrete.parse.ts` around lines 21 - 23, There are three consecutive blank lines after the import block at the top of discrete.parse.ts (a leftover merge artifact); collapse them to a single blank line so the import section is followed by exactly one blank line for consistency with the rest of the file.
449-452: No-op try/catch: remove the bare rethrow.catch (e: any) { throw e; }adds nothing (not even a log or a wrapped message) and only costs readers a second look. Either drop the wrapper or wrap the error with context (alias name, source, etc.):
♻️ Proposed fix
- catch (e: any) { - throw e; - } + // (let caller handle; add context here if future diagnostics need it)Or, if diagnostics are wanted:
- catch (e: any) { - throw e; - } + catch (e: any) { + const msg = `[Tempo#parseGroups] ${aliasKey} handler threw: ${e?.message ?? e}`; + if (TempoClass) (TempoClass as any)[sym.$logError](state.config, new Error(msg)); + throw e; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/discrete/discrete.parse.ts` around lines 449 - 452, The catch block that simply rethrows the error ("catch (e: any) { throw e; }") in packages/tempo/src/discrete/discrete.parse.ts is a no-op and should be removed or replaced with contextual error wrapping; either delete the try/catch so the original error and stack propagate, or replace the catch with a wrapped error that adds context (e.g., include alias/source info) while preserving the original as the cause (throw new Error(`...context...`, { cause: e })). Locate the trivial catch in the parse logic (the catch block shown) and apply one of these changes to eliminate the useless rethrow.
133-133: Redundant disjuncts inisAnchoredVal.
valat line 63 is assigned asdateTime ?? state.anchor ?? …, soisDefined(val)(stored inisAnchored) already impliesisDefined(dateTime) || isDefined(state.anchor). The extra checks on line 133 are no-ops:- const isAnchoredVal = isAnchored || isDefined(dateTime) || isDefined(state.anchor); + const isAnchoredVal = isAnchored;Or just pass
isAnchoredstraight through.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/discrete/discrete.parse.ts` at line 133, The expression computing isAnchoredVal is redundant because val is assigned as dateTime ?? state.anchor ?? … so isAnchored (or isDefined(val)) already covers dateTime and state.anchor; replace the compound disjunction with simply using isAnchored (or isDefined(val)) directly. Locate the isAnchoredVal assignment in discrete.parse (symbol: isAnchoredVal) and change it to pass through isAnchored (or compute isDefined(val)) instead of recomputing isDefined(dateTime) || isDefined(state.anchor).packages/tempo/src/tempo.type.ts (1)
190-192: Optional: narrowMatch.sourceto a string-literal union.Downstream code uses distinct values (
'global','local','default','plugin:X'). Consider:source?: 'global' | 'local' | 'default' | `plugin:${string}`;to give consumers discriminated-union autocompletion while still allowing
plugin:*growth. Purely optional; the currentstringis fine.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/tempo.type.ts` around lines 190 - 192, Narrow the Match.source type from a plain string to a discriminated string-literal union to improve autocompletion and type-safety: change the Match interface's source property (referenced as Match.source) to optional source?: 'global' | 'local' | 'default' | `plugin:${string}` so existing uses still accept arbitrary plugin values while giving consumers the specific union variants for discrimination and tooling support.packages/tempo/test/sandbox-factory.test.ts (2)
81-90: Immutability assertion is weak — consider asserting the throw.
(Sandbox as any).someNewProp = 'test'will silently succeed in non-strict mode and silently fail underObject.freeze, making the subsequenttoBeUndefined()pass for the wrong reason. If@Immutableis intended to throw in strict mode, assert that directly:- try { - (Sandbox as any).someNewProp = 'test'; - } catch (e) { } - - expect((Sandbox as any).someNewProp).toBeUndefined(); + 'use strict'; + expect(() => { (Sandbox as any).someNewProp = 'test'; }).toThrow(); + expect((Sandbox as any).someNewProp).toBeUndefined();Or, if frozen-silent is the contract, document it in the test name (
silently drops new static props).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/test/sandbox-factory.test.ts` around lines 81 - 90, The current immutability test for Sandbox is ambiguous because assigning (Sandbox as any).someNewProp = 'test' can silently fail; update the test to either assert a thrown error (if `@Immutable` should throw) or explicitly assert the frozen-silent contract: replace the try/catch with expect(() => { (Sandbox as any).someNewProp = 'test'; }).toThrow() when you want a thrown error from Tempo.create / Sandbox, or change the test name to indicate "silently drops new static props" and add a direct frozen check such as expect(Object.isFrozen(Sandbox)).toBe(true) plus expect((Sandbox as any).someNewProp).toBeUndefined(); reference the Sandbox variable created by Tempo.create({ locale: 'fr-FR' }) and update the it(...) title accordingly.
1-12: Vitest globals are enabled; consider adding explicit imports for clarity.Lines 1–12 use
describe,it,expect, and on line 12viwithout importing them. This works becausepackages/tempo/vitest.config.tssetsglobals: trueandpackages/tempo/test/tsconfig.jsonincludesvitest/globalsin types. Adding an explicit import is optional but improves code clarity and self-documentation:+import { describe, it, expect, vi } from 'vitest'; import { Tempo } from '#tempo';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/test/sandbox-factory.test.ts` around lines 1 - 12, The test uses Vitest globals (describe, it, expect, vi) without explicit imports which reduces clarity; add a top-level import from 'vitest' (e.g., import { describe, it, expect, vi } from 'vitest') so the functions used in the tests (describe, it, expect) and the spy utility (vi) are explicitly imported; update the import statement near the top of sandbox-factory.test.ts so the test remains functionally unchanged but is self-documenting.packages/tempo/src/support/tempo.init.ts (1)
23-60:getDateTimeFormat()is called three times; outer destructure is dead.Line 23 destructures
{ timeZone, calendar }but those values aren't used in either branch — the global branch re-calls on line 50 (shadowing), and line 54 calls a third time just to read.locale. Consolidate to a single call:♻️ Proposed fix
- const { timeZone, calendar } = getDateTimeFormat(); - const state = { config: {}, parse: {} } as t.Internal.State @@ if (isGlobal) { markConfig(Object.assign(state.config, Default)); - const { timeZone, calendar } = getDateTimeFormat(); + const fmt = getDateTimeFormat() as any; + const { timeZone, calendar } = fmt; Object.defineProperties(state.config, { calendar: { value: calendar, enumerable: true, writable: true, configurable: true }, timeZone: { value: timeZone, enumerable: true, writable: true, configurable: true }, - locale: { value: (getDateTimeFormat() as any).locale ?? 'en-US', enumerable: true, writable: true, configurable: true }, + locale: { value: fmt.locale ?? 'en-US', enumerable: true, writable: true, configurable: true },🤖 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 23 - 60, The initial call to getDateTimeFormat() is duplicated and its outer destructure (timeZone, calendar) is unused; remove the outer destructure and call getDateTimeFormat() once, capture its result into a single local (e.g., dtf = getDateTimeFormat()), then reuse dtf.timeZone, dtf.calendar and dtf.locale when initializing state.config inside the isGlobal branch and anywhere else; update references in the markConfig/isGlobal block (the Object.defineProperties entries for calendar, timeZone, locale and sphere) to use the single dtf variable and remove the extra calls to getDateTimeFormat().packages/library/src/common/type.library.ts (1)
40-44: Defensive check for non-arraysym.$Registry.
resetRegistry(Line 62) already guards withArray.isArray(...)before mutating the global registry, butgetTypeassumes the value is iterable. If third-party code writes anything non-array toglobalThis[sym.$Registry], the spread at Line 41 will throw at runtime. Consider mirroring theArray.isArrayguard here for symmetry.🛡️ Proposed defensive guard
- const globalRegistry = (globalThis as any)[sym.$Registry] ?? []; + const globalRegistry = Array.isArray((globalThis as any)[sym.$Registry]) + ? (globalThis as any)[sym.$Registry] + : [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/library/src/common/type.library.ts` around lines 40 - 44, The getType function assumes globalThis[sym.$Registry] is iterable and spreads it into [...registry, ...globalRegistry], which will throw if a non-array was written there; mirror the defensive check used in resetRegistry by ensuring globalRegistry is an array (e.g., use Array.isArray((globalThis as any)[sym.$Registry]) ? (globalThis as any)[sym.$Registry] : []) before spreading, then iterate over [...registry, ...safeGlobalRegistry] and continue using inst.class / (inst.class as any)[sym.$Target] to resolve types.packages/tempo/src/engine/engine.layout.ts (1)
35-52: Duplicate / undefineddescriptioncollapses layout entries.
byNameis keyed onkey.description ?? ''. If the Layout map ever contains two symbols with the same description (or two symbols withundefineddescription — which both collapse to''), theMapconstruction silently drops the earlier one, andresolveLayoutClassificationOrderwill reorder based on the second of the two. The Tempo-shipped tokens are unique today, but any plugin authoring layout extensions via rawSymbol(...)could accidentally introduce a dup.Either (a) assert uniqueness and warn, or (b) switch to a multi-map that preserves insertion order when descriptions collide.
🛡️ Minimal guard
const entries = ownEntries(layout) as LayoutEntry[]; - const byName = new Map(entries.map(([key, value]) => [key.description ?? '', [key, value] as LayoutEntry])); + const byName = new Map<string, LayoutEntry>(); + entries.forEach(([key, value]) => { + const name = key.description ?? ''; + if (!name || byName.has(name)) return; // skip anonymous/duplicates rather than silently overwrite + byName.set(name, [key, value]); + });Additionally, the
changedcheck on Line 51 combinesnext.length === entries.length(always true by construction — unseen entries are always appended) with.some(...). The length guard is dead — you can drop it and just compare order:- const changed = next.length === entries.length && next.some((entry, idx) => entry[0] !== entries[idx][0]); + const changed = next.some((entry, idx) => entry[0] !== entries[idx][0]);🤖 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 35 - 52, The layout reordering silently drops earlier entries when two keys share the same description because byName is a Map keyed on key.description ?? '', so change byName into a multi-map that preserves insertion order (e.g., Map<string, LayoutEntry[]>) and push every ([key, value]) into the array for that description; in resolveLayoutClassificationOrder (or the function using ownEntries/preferred), when iterating preferred use the first matching entry from the array and mark each consumed symbol in seen (and optionally log a warning when a description array length > 1 to signal duplicates), and remove the dead length check in the changed calculation — simply compare the symbol order (next.some((entry, idx) => entry[0] !== entries[idx][0])) to decide if layout changed.packages/tempo/src/support/tempo.default.ts (1)
30-30: Redundant\-insideguardcharacter class.
\-appears twice in the character class: once after\d\sand again after\+. Harmless but looks unintentional — likely a merge artifact.💄 Cleanup
- /** base guard characters (digits and common symbols) */ guard: /[\d\s\-\.\:T\/Z\+\-\(\)\,\=\#\<\>]/i, + /** base guard characters (digits and common symbols) */ guard: /[\d\s\-\.\:T\/Z\+\(\)\,\=\#\<\>]/i,🤖 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` at line 30, The guard regex in tempo.default.ts defines the property guard: /[\d\s\-\.\:T\/Z\+\-\(\)\,\=\#\<\>]/i and contains a duplicated escaped hyphen (\-); remove the redundant \- so the character class only includes a single hyphen (ensure the hyphen remains escaped or placed at the start/end to avoid creating a range) while preserving all other characters and the case-insensitive flag on the guard property.packages/tempo/src/tempo.class.ts (1)
683-692: Cosmetic: mix ofsym.$Internaland bare$Internal.
$Internalis imported on Line 24 and is the same symbol thatsym.$Internalresolves to via the prototype chain intempo.symbol.ts. Within this method alone, lines 683/691/692 usesym.$Internalbut 703/714/723 (further down) use$Internal. Unifying to the bare import would be easier to read and slightly cheaper (avoids the prototype lookup).🤖 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 683 - 692, The method mixes sym.$Internal and the imported bare $Internal; replace uses of sym.$Internal with the imported $Internal throughout this method so the same symbol is referenced consistently (e.g., when accessing (this as any)[sym.$Internal]().parse.period and (this as any)[sym.$Internal]() used before calling $setPeriods/$setEvents). Update all occurrences so you read (this as any)[$Internal]() instead of (this as any)[sym.$Internal](), leaving other symbols ($setPeriods, $setEvents, parse.period, parse.event, target, config.ranges) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4d488875-2c90-4c58-a492-251634afc4d7
📒 Files selected for processing (85)
package.jsonpackages/library/package.jsonpackages/library/src/browser/mapper.library.tspackages/library/src/browser/tapper.class.tspackages/library/src/browser/webstore.class.tspackages/library/src/common.index.tspackages/library/src/common/array.library.tspackages/library/src/common/assertion.library.tspackages/library/src/common/coercion.library.tspackages/library/src/common/enumerate.library.tspackages/library/src/common/function.library.tspackages/library/src/common/logify.class.tspackages/library/src/common/object.library.tspackages/library/src/common/pledge.class.tspackages/library/src/common/primitive.library.tspackages/library/src/common/proxy.library.tspackages/library/src/common/reflection.library.tspackages/library/src/common/serialize.library.tspackages/library/src/common/storage.library.tspackages/library/src/common/string.library.tspackages/library/src/common/symbol.library.tspackages/library/src/common/temporal.library.tspackages/library/src/common/type.library.tspackages/library/src/common/utility.library.tspackages/library/test/common/temporal_guards.test.tspackages/tempo/CHANGELOG.mdpackages/tempo/bin/tsconfig.jsonpackages/tempo/demo/3-modular-granular.htmlpackages/tempo/doc/Tempo.mdpackages/tempo/doc/installation.mdpackages/tempo/doc/migration-guide.mdpackages/tempo/doc/releases/v2.x.mdpackages/tempo/doc/sandbox-factory.mdpackages/tempo/doc/tempo.cookbook.mdpackages/tempo/doc/tempo.modularity.mdpackages/tempo/importmap.jsonpackages/tempo/index.mdpackages/tempo/package.jsonpackages/tempo/plan/.WISHLIST.mdpackages/tempo/plan/alias-precedence-strategy.mdpackages/tempo/plan/astrological_seasons.mdpackages/tempo/plan/configuration.mdpackages/tempo/plan/release-process.mdpackages/tempo/plan/shorthand.mdpackages/tempo/plan/standalone_parse.mdpackages/tempo/public/esm_core.index.htmlpackages/tempo/src/discrete/discrete.format.tspackages/tempo/src/discrete/discrete.parse.tspackages/tempo/src/engine/engine.composer.tspackages/tempo/src/engine/engine.layout.tspackages/tempo/src/engine/engine.lexer.tspackages/tempo/src/engine/engine.term.tspackages/tempo/src/module/module.duration.tspackages/tempo/src/module/module.mutate.tspackages/tempo/src/plugin/extend/extend.ticker.tspackages/tempo/src/plugin/plugin.type.tspackages/tempo/src/plugin/plugin.util.tspackages/tempo/src/plugin/term.util.tspackages/tempo/src/plugin/term/term.quarter.tspackages/tempo/src/plugin/term/term.zodiac.tspackages/tempo/src/support/support.index.tspackages/tempo/src/support/tempo.default.tspackages/tempo/src/support/tempo.enum.tspackages/tempo/src/support/tempo.init.tspackages/tempo/src/support/tempo.register.tspackages/tempo/src/support/tempo.symbol.tspackages/tempo/src/support/tempo.util.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.index.tspackages/tempo/src/tempo.type.tspackages/tempo/src/tsconfig.jsonpackages/tempo/test/compact.time.test.tspackages/tempo/test/constructor.shorthand.test.tspackages/tempo/test/discovery_security.test.tspackages/tempo/test/engine.layout.test.tspackages/tempo/test/instance.since.rtf.test.tspackages/tempo/test/layout.order.test.tspackages/tempo/test/pattern.weekday.test.tspackages/tempo/test/runtime_brand.test.tspackages/tempo/test/sandbox-factory.test.tspackages/tempo/test/symbol-import.test.tspackages/tempo/test/term_unified.test.tspackages/tempo/test/tsconfig.jsonpackages/tempo/vitest.config.tsvitest.config.ts
💤 Files with no reviewable changes (4)
- packages/tempo/test/runtime_brand.test.ts
- packages/tempo/plan/astrological_seasons.md
- packages/tempo/plan/standalone_parse.md
- packages/tempo/plan/shorthand.md
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/tempo/index.md (1)
305-313:⚠️ Potential issue | 🟠 MajorClear snap/restore timers when the page becomes hidden.
Line 305 onward pauses autoplay, but pending
carouselSnapTimer/carouselRestoreTimercan still fire while hidden and mutate carousel state unexpectedly. Clear them in the hidden branch too.♻️ Proposed fix
} else { isResuming.value = false tickerActive.value = false console.info('%c[Tempo]%c 💤 Pausing Ticker (Standby)', 'color: `#71717a`; font-weight: bold', 'color: inherit') ticker?.stop() if (fallbackIntervalId) clearInterval(fallbackIntervalId) + clearCarouselSnapTimers() clearInterval(carouselTimer) carouselTimer = null } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/index.md` around lines 305 - 313, When handling the hidden-page branch that pauses autoplay (the else block managing isResuming.value, tickerActive.value, ticker?.stop(), fallbackIntervalId, and carouselTimer), also clear any pending snap/restore timers by clearing carouselSnapTimer and carouselRestoreTimer (and setting them to null) to prevent them firing while hidden; update the same hidden-branch logic to call clearTimeout/clearInterval on carouselSnapTimer and carouselRestoreTimer and nullify those variables alongside the existing carouselTimer cleanup.
🧹 Nitpick comments (15)
packages/library/src/common/logify.class.ts (2)
103-113: Orphan severityTrace: no corresponding log method.
LOG.Trace = 5is exported but there is notracemethod onLogify, and noLevelentry for it, so settingdebug: LOG.Tracebehaves identically toLOG.Debugfor emission. IfTraceis intended as “verbose Debug”, consider either adding atrace = (...msg) => this.#trap(Method.Trace, ...)(and a correspondingMethod.Trace/Levelentry) or droppingTracefrom the enum until it's wired up, to avoid misleading public API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/library/src/common/logify.class.ts` around lines 103 - 113, LOG includes an orphaned Trace level (LOG.Trace) that has no corresponding Logify.trace method or Method/Level entry, so enable it by adding a trace handler or remove it; specifically either add a Logify.trace(...) that forwards to this.#trap(Method.Trace, ...) and create matching Method.Trace and Level.Trace entries so emission differs from Debug, or remove LOG.Trace from the enum to avoid exposing an unsupported level; update any references to LogLevel/Level to reflect the chosen change.
40-43: Severity gating semantics look correct; consider binding public LOG to internal Level.The runtime Level map (lines 17-24) and the exported
LOGenum (lines 103-111) independently assign numeric severities; they currently agree (Error=1, Warn=2, Info=3, Debug=4), but they can drift in future edits. Consider deriving one from the other so changes to public LOG levels stay in sync with themethodLevel <= currentLevelgate, e.g.:♻️ Proposed refactor
-const Level = { - [Method.Error]: 1, - [Method.Warn]: 2, - [Method.Info]: 3, - [Method.Log]: 3, - [Method.Debug]: 4, -} as const; +const Level = { + [Method.Error]: LOG.Error, + [Method.Warn]: LOG.Warn, + [Method.Info]: LOG.Info, + [Method.Log]: LOG.Info, + [Method.Debug]: LOG.Debug, +} as const;(which requires declaring
LOGaboveLevel, or inlining the numbers consistently).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/library/src/common/logify.class.ts` around lines 40 - 43, The public LOG enum and the internal Level map can drift; change the code so LOG is derived from (or directly references) the internal Level values instead of duplicating numeric assignments: ensure the exported LOG symbol reflects Level's severities and update any checks using currentLevel and methodLevel (e.g., the calculation of currentLevel from config.debug and the lookup Level[method] in methodLevel) to use the unified source so methodLevel <= currentLevel gating remains correct. Locate the Level object and exported LOG enum definitions and refactor so LOG is bound to Level (or vice versa) to keep severities in sync across the class.packages/library/src/common/assertion.library.ts (2)
57-84: Consider extracting the repeated TemporalisType / instanceof / toStringTagpattern.Each Temporal guard duplicates the same four-branch shape (
isType→instanceof Temporal.X→Symbol.toStringTag→ duck-typed heuristic). A small helper likehasTemporalTag(obj, 'Temporal.PlainDate')would collapse the boilerplate and make the duck-type heuristics easier to audit:♻️ Sketch
const hasTemporalTag = (obj: any, tag: keyof typeof globalThis['Temporal'] | string) => !!(globalThis as any).Temporal?.[tag] && obj instanceof (globalThis as any).Temporal[tag] || (!!obj && obj[Symbol.toStringTag] === `Temporal.${tag}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/library/src/common/assertion.library.ts` around lines 57 - 84, Many Temporal type-guards (isTemporal, isInstant, isZonedDateTime, isPlainDate, isPlainTime, isPlainDateTime, isDuration, isPlainYearMonth, isPlainMonthDay, and related *Like helpers) duplicate the same four-branch pattern (isType -> instanceof globalThis.Temporal.X -> Symbol.toStringTag -> duck-typing), so extract a small helper (e.g. hasTemporalTag) and refactor each guard to call it; update isTemporal to use protoType + helper for specific Temporal classes and replace repeated instanceof/toStringTag checks inside isInstant, isZonedDateTime, isPlainDate, isPlainTime, isPlainDateTime, isDuration, isPlainYearMonth, and isPlainMonthDay with calls to that helper and keep only the duck-typed heuristics in each guard.
33-33:isDigitis a misleading name for aNumber | BigIntcheck.The guard accepts any finite or non-finite number and any BigInt, not a single digit. Consider renaming to something like
isNumeric-adjacent (isNumberOrBigInt,isNumericType, or similar) to prevent misuse at call sites that actually want single-digit semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/library/src/common/assertion.library.ts` at line 33, The exported type-guard isDigit misnames a check for Number|BigInt; rename the function to a clearer name (e.g., isNumberOrBigInt or isNumericType) and update its export and all internal imports/usages to that new identifier; keep the implementation (isType(obj, 'Number', 'BigInt')) but change the function name and associated JSDoc/comments, and optionally add a backward-compatible deprecated alias export named isDigit that delegates to the new function to avoid breaking external callers while you update call sites.packages/library/test/common/logify.class.test.ts (2)
3-52: Consider extending coverage todebug: trueand theLogmethod.The new severity-gating table treats
debug: true → LOG.DebugandMethod.Log → 3. Neither path is currently exercised. A couple of extra cases would cement the contract and guard against future drift between the internalLevelmap and the publicLOGenum:♻️ Suggested additions
test('debug: true maps to Debug level (emits debug)', () => { const debugSpy = vi.spyOn(console, 'debug').mockImplementation(() => {}); try { new Logify('LogifyTestDebugTrue', { debug: true }).debug('debug-visible'); expect(debugSpy).toHaveBeenCalledWith('LogifyTestDebugTrue: debug-visible'); } finally { debugSpy.mockRestore(); } }); test('log() emits at Info level by default', () => { const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); try { new Logify('LogifyTestLog').log('log-visible'); expect(logSpy).toHaveBeenCalledWith('LogifyTestLog: log-visible'); } finally { logSpy.mockRestore(); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/library/test/common/logify.class.test.ts` around lines 3 - 52, Add two tests to exercise the debug:true mapping and the Log method: create a test that constructs new Logify('LogifyTestDebugTrue', { debug: true }) and calls .debug('debug-visible'), asserting console.debug was called with 'LogifyTestDebugTrue: debug-visible' (restore spy afterward), and add a test that constructs new Logify('LogifyTestLog') and calls .log('log-visible'), asserting console.log was called with 'LogifyTestLog: log-visible' (restore spy afterward); reference the Logify constructor and its .debug and .log methods and ensure spies use mockImplementation to avoid real output.
40-51: Add a comment or test name capturing thatdebug: 0intentionally suppressesconsole.error.The assertion
expect(errorSpy).not.toHaveBeenCalled()is the key behavioral contract from the severity-gating change (Error level is1, so1 <= 0is false). Since this will look surprising to readers who expect errors to always emit, consider renaming the test or adding a short comment referencingLOG.Off:♻️ Proposed tweak
- test('rethrows errors even when log emission is gated off', () => { + test('rethrows errors even when log emission is gated off (debug: 0 / LOG.Off)', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/library/test/common/logify.class.test.ts` around lines 40 - 51, Rename the test or add a short inline comment to clarify that setting debug: 0 intentionally suppresses console.error (i.e., LOG.Off behavior) so the assertion expect(errorSpy).not.toHaveBeenCalled() is deliberate; update the test name 'rethrows errors even when log emission is gated off' or add a comment next to the Logify instantiation (new Logify('LogifyTestRethrow', { debug: 0, catch: false })) referencing that debug: 0 corresponds to LOG.Off and therefore error-level emissions are suppressed even though log.error rethrows the error.packages/tempo/src/tempo.type.ts (1)
10-10: MakeTempoBranda type-only import.
TempoBrandis declared asexport type TempoBrand = { ... }inpackages/tempo/src/support/tempo.symbol.tsand is only used in a type position on line 32. UnderverbatimModuleSyntax/isolatedModules, mixing it with the value import ofsymwithout thetypequalifier can trigger build errors. Use an inline type modifier:♻️ Proposed fix
-import { sym, TempoBrand } from '#tempo/support/tempo.symbol.js'; +import { sym, type TempoBrand } from '#tempo/support/tempo.symbol.js';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/tempo.type.ts` at line 10, The import mixes a value import (sym) with a type-only export (TempoBrand); change the import to import sym as a value and TempoBrand as a type-only import (use the inline type modifier for TempoBrand) so TempoBrand is imported with the type qualifier and avoids isolatedModules/verbatimModuleSyntax build errors when used in the type position (refer to the TempoBrand symbol and the sym value import in this file).packages/tempo/test/term_unified.test.ts (1)
91-108: LGTM — tests cover granular and case-insensitive sphere matching paths.Assertions on both
{#qtr}formatting and{ start: '#quarter' }mutation give good coverage of the newgetRange/resolveCycleWindowsphere fallback. One optional follow-up: add a negative test (e.g.sphere: 'unknownzone') to verify the fallback does not resolve to an arbitrary hemisphere — this would lock in the “no match → empty/error” path for theTermErrordispatch thatterm.util.tswas refactored to use.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/test/term_unified.test.ts` around lines 91 - 108, Add a negative test that constructs a Tempo instance with an unknown sphere (e.g. new Tempo(testDate, { catch: true, sphere: 'unknownzone' })) and assert that queries relying on getRange/resolveCycleWindow do not resolve to a hemisphere value but instead trigger the intended failure path (e.g. produce an empty result or cause the TermError dispatch from term.util.ts); use the same pattern as existing tests (format('{`#qtr`}') and set({ start: '#quarter' })) to confirm it does not return Q4 or a valid date and that TermError handling is invoked.packages/tempo/src/support/tempo.default.ts (1)
30-30: Minor:\-duplicated inside theguardcharacter class.
\-appears twice (…\-\.\:T\/Z\+\-\(\)…). Harmless but redundant; one occurrence is enough.🤖 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` at line 30, The guard regular expression on the exported property named "guard" contains a duplicated escaped hyphen (\-) inside the character class; edit the guard value in tempo.default.ts (the guard property) to remove the redundant \- so the character class only includes a single hyphen escape, leaving the rest of the tokens unchanged.packages/tempo/src/engine/engine.layout.ts (2)
69-95: Reference‑preservation contract reads correctly; consider memoizing description lookups.The delegation to
resolveLayoutClassificationOrderfirst, plus returningorderedwhen no pair swap occurs, preserves theresolvedLayout === originalLayoutinvariant thatTempo.#swapLayoutrelies on attempo.class.ts:238. For larger layout registries, the repeatedlayouts.findIndex([key]) => key.description === …)inside themdyLayouts.forEachis O(N·M); a one‑shotMap<description, index>built afterownEntries(ordered)would keep the intent but drop it to O(N+M). Purely optional for current sizes.🤖 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 69 - 95, The current resolveLayoutOrder uses repeated layouts.findIndex calls inside mdyLayouts.forEach which is O(N·M); to speed this up, after creating layouts = ownEntries(ordered) build a Map from key.description to index (e.g., descIndexMap) and use that map to look up idx1/idx2 instead of findIndex; when you swap entries in layouts update both the map entries for the two descriptions and set changed = true as before, and keep the existing return behavior (return ordered when !changed to preserve the reference-preservation contract used by Tempo.#swapLayout).
45-58: Duplicate names in a controller entry can silently suppress reordering.The first
forEachinresolveLayoutClassificationOrderpushes tonextwithout checkingseen, so if a classification's preferred array contains the same name twice (e.g. through a misconfigured controller),next.length > entries.length, causing thenext.length === entries.lengthguard on line 56 to fail and the function to return the originallayout— masking the intended reorder entirely rather than surfacing the misconfiguration.Defensive dedup in the preferred loop would prevent that silent no‑op:
♻️ Proposed hardening
preferred.forEach(name => { const entry = byName.get(name); - if (!entry) return; + if (!entry || 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 45 - 58, The preferred-names loop in resolveLayoutClassificationOrder can push the same entry multiple times (if preferred contains duplicates) which makes next.length > entries.length and prevents detection of reordering; fix by deduplicating when iterating preferred (use seen to skip if entry[0] already in seen) so you only push unique entries from preferred into next (references: resolveLayoutClassificationOrder, preferred, byName, seen, next, entries), ensuring the changed check correctly detects reorderings instead of silently returning the original layout.packages/tempo/src/support/tempo.init.ts (1)
48-80: Substantial duplication between global and fallback branches.Lines 51‑60 and 68‑77 define the same 7 descriptors (
calendar,timeZone,locale,discovery,formats,sphere,get) with identical values. Onlyscopeandcatchdiffer. A small helper would remove the drift risk:♻️ Suggested helper extraction
- if (isGlobal) { - markConfig(Object.assign(state.config, Default)); - const { timeZone, calendar } = getDateTimeFormat(); - Object.defineProperties(state.config, { - calendar: { value: calendar, enumerable: true, writable: true, configurable: true }, - timeZone: { value: timeZone, enumerable: true, writable: true, configurable: true }, - locale: { value: (getDateTimeFormat() as any).locale ?? 'en-US', enumerable: true, writable: true, configurable: true }, - discovery: { value: Symbol.keyFor(sym.$Tempo) as string, enumerable: true, writable: true, configurable: true }, - formats: { value: enumify(STATE.FORMAT, false), enumerable: true, writable: true, configurable: true }, - sphere: { value: getHemisphere(timeZone), enumerable: true, writable: true, configurable: true }, - get: { value: function (key: string) { return this[key] }, enumerable: false, writable: true, configurable: true }, - scope: { value: 'global', enumerable: true, writable: true, configurable: true }, - catch: { value: options.catch ?? false, enumerable: true, writable: true, configurable: true } - }); - } else if (baseState) { + const defineBaseConfig = (scope: 'global' | 'local') => { + markConfig(Object.assign(state.config, Default)); + Object.defineProperties(state.config, { + calendar: { value: calendar, enumerable: true, writable: true, configurable: true }, + timeZone: { value: timeZone, enumerable: true, writable: true, configurable: true }, + locale: { value: (getDateTimeFormat() as any).locale ?? 'en-US', enumerable: true, writable: true, configurable: true }, + discovery: { value: Symbol.keyFor(sym.$Tempo) as string, enumerable: true, writable: true, configurable: true }, + formats: { value: enumify(STATE.FORMAT, false), enumerable: true, writable: true, configurable: true }, + sphere: { value: getHemisphere(timeZone), enumerable: true, writable: true, configurable: true }, + get: { value: function (k: string) { return this[k] }, enumerable: false, writable: true, configurable: true }, + scope: { value: scope, enumerable: true, writable: true, configurable: true }, + catch: { value: options.catch ?? false, enumerable: true, writable: true, configurable: true }, + }); + }; + + if (isGlobal) { + defineBaseConfig('global'); + } else if (baseState) { state.config = markConfig(Object.create(baseState.config)); setProperty(state.config, 'scope', 'local'); if (isDefined(options.catch)) setProperty(state.config, 'catch', options.catch); } else { - markConfig(Object.assign(state.config, Default)); - Object.defineProperties(state.config, { /* …same 7 descriptors… */ }); - if (isDefined(options.catch)) - setProperty(state.config, 'catch', options.catch); + defineBaseConfig('local'); }(Line 50's re‑destructuring of
timeZone/calendarcan be dropped since line 23 already provides them.)🤖 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 48 - 80, There is duplicated Object.defineProperties descriptor setup for state.config in the isGlobal and fallback branches; extract a small helper (e.g., buildConfigDescriptors or applyCommonConfigDescriptors) that returns/applies the shared descriptors for calendar, timeZone, locale, discovery (using Symbol.keyFor(sym.$Tempo)), formats (enumify(STATE.FORMAT, false)), sphere (getHemisphere(timeZone)), and get (function), then call that helper from both branches and only set scope/catch separately (use setProperty for catch when needed); also remove the redundant re-destructuring of timeZone/calendar since getDateTimeFormat() was already called earlier.packages/library/src/common/proxy.library.ts (1)
97-109: Discovery side‑effect:getmutates the target to cache misses.When
onGetreturnsundefinedand the target is extensible, the trap writes{ value: undefined, enumerable: false, configurable: true }onto the real target. This is a useful "don't look it up again" hint, but has a few consequences worth documenting:
- The raw target accretes hidden properties over the lifetime of the proxy (no eviction).
- If
onGet's answer for a key ever becomes defined after a prior miss, the cached descriptor will shadow further discovery attempts (sinceReflect.has(t, k)is now true).Reflect.ownKeys(t)will include these marker keys, which leaks intoownKeyson line 56 (they're non‑enumerable sofor…in/Object.keysskip them, butReflect.ownKeys/Object.getOwnPropertyNamesexpose them).Not a bug for the current callers, but a deliberate contract worth a comment, or a
WeakMap<target, Set<key>>side‑cache to avoid mutating user data.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/library/src/common/proxy.library.ts` around lines 97 - 109, The get trap currently writes a hidden undefined property onto the real target (see onGet, pending, t, k and the Object.defineProperty call guarded by Reflect.isExtensible) to mark misses, which mutates user data and can shadow later valid discoveries and leak into Reflect.ownKeys; fix it by replacing that mutation with a side-cache: introduce a WeakMap (e.g., missCache: WeakMap<object, Set<PropertyKey>>) keyed by target and store k in the Set instead of defining a property, check missCache.has(target) && missCache.get(target).has(k) before calling onGet, and remove the Object.defineProperty call and related Reflect.has checks that rely on the synthetic descriptor; alternatively if mutation is acceptable, add a clear explanatory comment at the Object.defineProperty site documenting the tradeoffs (shadowing, no eviction, visibility via Reflect.ownKeys) so reviewers know it is intentional.packages/library/test/common/pledge.class.test.ts (1)
68-111: Good coverage of the numericdebuggating contract.Both tests cover the key behaviors of the new
Logify.Constructor["debug"]typing: the.then(runSafely)chain is given time to flush (await p.promise+await Promise.resolve()) before assertions, and thedebug: 0case correctly exercises themethodLevel (2) <= currentLevel (0) === falsegate inLogify#trap.Minor note: line 84's assertion on the exact string
'Pledge: Pledge callback failed'is tightly coupled to Logify's${name}: ${output}format. If that prefix contract is deliberate, consider promoting it to a shared constant; otherwise be prepared for cross‑module churn if Logify's framing ever changes. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/library/test/common/pledge.class.test.ts` around lines 68 - 111, The assertion that the warn message equals the exact prefixed string is too tightly coupled to Logify's framing; update the test in the 'callback failures warn at default debug level (indirect Logify integration)' case to either (a) assert only on the variable part of the message (e.g., that the warn output contains 'Pledge callback failed' rather than the full 'Pledge: Pledge callback failed' prefix) or (b) import/use a shared constant from Logify (eg. a published MESSAGE_PREFIX or format helper) and build the expected string from that constant so the test uses Pledge and Logify's canonical prefix rather than a hardcoded literal; keep the existing warnSpy/debugSpy setup and the await flush sequence unchanged.packages/library/src/common/primitive.library.ts (1)
44-73:ownEntries(all=true)edge case with prototype shadowing.Walking the prototype chain into
levelsthen iterating withlevels.reverse()goes base → derived, and values are resolved fromtgt(line 68). If a derived prototype shadows a base key with a non‑enumerable descriptor, the loop still emits the base level's (enumerable) entry even thoughtgt[k]would pick up the derived value through prototype resolution. Theenumerablecheck is applied per‑level rather than at the effective (topmost) descriptor.Unlikely to hit in practice for the way this is used today, but worth a comment noting the invariant, or checking enumerability of the effective property (via
Object.getOwnPropertyDescriptorwalking fromtgtdown).
🤖 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/library/src/common/assertion.library.ts`:
- Around line 70-72: isPlainDate/isPlainDateTime duck-typing branches
incorrectly conflate PlainDate and PlainDateTime and make PlainDateTime
impossible to match; update the fallbacks so isPlainDate explicitly ensures the
absence of time fields and isPlainDateTime requires the presence of time fields.
Concretely, in isPlainDate add a check that hour/minute/second/nanosecond are
undefined (so objects with time parts don't match), and in isPlainDateTime
remove the isUndefined(daysInMonth) requirement and instead require at least one
of the time-part properties (hour, minute, second, nanosecond) to be defined;
keep the existing isType/instanceof/@@toStringTag branches and the
toZonedDateTime/timeZoneId checks but use these time-field tests to disambiguate
the fallback paths in isPlainDate and isPlainDateTime.
In `@packages/library/src/common/logify.class.ts`:
- Around line 40-43: Add a JSDoc note to the Logify constructor's debug option
(Logify.Constructor.debug) documenting that debug accepts boolean | number
mapped to LOG levels and that numeric 0 (LOG.Off) will suppress all logging
including error output (so console.error will not be called even if errors are
rethrown when catch:false); update the comment near the existing JSDoc at the
constructor declaration to explicitly call out this behavior and recommend using
a higher numeric level or true to ensure errors are logged.
In `@packages/library/src/common/primitive.library.ts`:
- Around line 16-27: The loop in unwrap uses
Object.prototype.hasOwnProperty.call(curr, sym.$Target) and (curr as
any).$Target which trigger getOwnPropertyDescriptor traps and thus cannot detect
proxy-fabricated $Target; fix by driving traversal via direct property access
instead: in unwrap(), repeatedly read next = curr[sym.$Target] ?? (curr as
any).$Target and if next is falsy or depth >= maxDepth break, otherwise set curr
= next and increment depth (keep sym.$Target and string "$Target" checks and the
maxDepth guard). This preserves proxy get-trap behavior and correctly unwraps
chained proxies.
In `@packages/library/src/common/proxy.library.ts`:
- Around line 146-149: The delegator function currently uses Object.keys(keys)
which drops symbol keys when a Record with symbol keys is passed; change the key
extraction to use Reflect.ownKeys(keys) so both string and symbol keys are
preserved (e.g., use const keyList = Array.isArray(keys) ? keys :
Reflect.ownKeys(keys) as K[]), and ensure the resulting keyList is passed into
factory({} as any, { keys: keyList, onGet: fn as any }) so
has/getOwnPropertyDescriptor/ownKeys behave correctly for symbol keys.
- Around line 44-48: The getOwnPropertyDescriptor implementation currently
returns a generic data descriptor when keys is fixed, which can disagree with
the get trap (get -> onGet) by showing value: undefined; update
getOwnPropertyDescriptor to, when keys is present and includes k, attempt to
obtain the actual value via onGet(k, t) and return a descriptor that
materializes that value (enumerable: true, configurable: true, value:
<materialized>), but guard this call with the same recursion/pending check used
elsewhere (the pending set/flag referenced in the get trap) so you skip calling
onGet if recursion is already in flight and fall back to the existing generic
descriptor in that case. Ensure you reference and reuse the existing onGet
function and the pending recursion guard used around the get trap.
In `@packages/tempo/doc/migration-guide.md`:
- Around line 70-78: The fenced JSON example in the migration guide is indented
as an indented code block and triggers MD046; replace the indented/fenced mix
with a plain fenced block by removing the leading indentation and surrounding
the JSON with a proper triple-backtick fence labeled json (i.e., change the
current indented lines to: ```json on its own line, the JSON object unindented,
then closing ``` on its own line) so the Example fenced code block is valid and
lint-free.
In `@packages/tempo/doc/releases/v2.x.md`:
- Line 9: Update the release note bullet that reads "Added fortnight event and
half hour period aliases." to hyphenate the compound modifier by changing "half
hour" to "half-hour" so the line becomes "Added fortnight event and half-hour
period aliases."
In `@packages/tempo/index.md`:
- Around line 234-243: onSwipeStart can be interrupted by a second pointer which
overwrites swipePointerId; update onSwipeStart to early-return if a drag is
already active (check isDragging.value) and to only start a new drag for the
primary pointer (check e.isPrimary or prefer primary pointer for touch). In
practice, at the top of onSwipeStart guard with something like: return if
isDragging.value is true, and also return if the incoming event is not the
primary pointer (e.isPrimary === false) so multi-touch won't override
swipePointerId; keep the rest of the logic (setting swipePointerId, swipeStartX,
swipeLastX, dragOffsetX.value, transitionEnabled.value, isDragging.value, and
calling setPointerCapture) unchanged.
In `@packages/tempo/src/plugin/term.util.ts`:
- Around line 273-276: The code currently invokes the TermError symbol on the
constructor directly ((t.constructor as any)[TermError](...)) which can throw a
TypeError if the symbol/method isn't installed; change these calls to use
optional chaining so the symbol is only invoked if present (e.g., (t.constructor
as any)[TermError]?.(t.config, 'template')), preserving the fallback return []
behavior; apply the same optional-chaining fix to the other occurrence around
the block referenced (the second dispatch at the later lines) so
unresolved-template cases degrade gracefully.
- Around line 164-171: The sphere-matching logic is inconsistent: in getRange's
list filter (vars valA/valB) it checks config includes range
(valB.includes(valA)), but resolveCycleWindow uses mutual containment
(targetSphere.includes(sphereLower) || sphereLower.includes(targetSphere)); make
both use the same asymmetric rule where the config may be more specific than the
range key (i.e., check that config.includes(range)). Update resolveCycleWindow
to replace the bidirectional check with the single-direction check consistent
with getRange, and ensure the variable names (targetSphere, sphereLower) are
used so the logic reads targetSphere.includes(sphereLower).
- Around line 164-171: The current filter inside list.filter / keys.every treats
missing sphere values as '' so ''.includes('') or any.includes('') returns true
and unconditionally matches; change the sphere branch so it only matches when
the range's sphere is present and non-empty and the term's config sphere
contains it — i.e., compute valA = String(r[key] ?? '').toLowerCase() and valB =
String((t.config as any)[key] ?? '').toLowerCase() as before but require valA
!== '' (or both valA and valB non-empty) before using valB.includes(valA),
otherwise return false; update the sphere conditional in the list.filter block
accordingly.
In `@packages/tempo/src/support/tempo.init.ts`:
- Around line 62-80: The baseState branch currently creates state.config via
Object.create(baseState.config) which leaves core keys (timeZone, calendar,
locale, formats, sphere, discovery, get) on the prototype and causes
inconsistent own-key shape vs the global/fallback branches; change the baseState
branch to materialize the same own, enumerable properties as the other branches
(i.e. copy or define own properties for calendar, timeZone, locale, discovery,
formats, sphere, get, scope) after calling markConfig(state.config) so
Object.keys/Reflect.ownKeys/spread see the same surface, and explicitly set
'catch' with setProperty(state.config, 'catch', options.catch) (or
defineProperty) so 'catch' is always an own property regardless of Default.
---
Outside diff comments:
In `@packages/tempo/index.md`:
- Around line 305-313: When handling the hidden-page branch that pauses autoplay
(the else block managing isResuming.value, tickerActive.value, ticker?.stop(),
fallbackIntervalId, and carouselTimer), also clear any pending snap/restore
timers by clearing carouselSnapTimer and carouselRestoreTimer (and setting them
to null) to prevent them firing while hidden; update the same hidden-branch
logic to call clearTimeout/clearInterval on carouselSnapTimer and
carouselRestoreTimer and nullify those variables alongside the existing
carouselTimer cleanup.
---
Nitpick comments:
In `@packages/library/src/common/assertion.library.ts`:
- Around line 57-84: Many Temporal type-guards (isTemporal, isInstant,
isZonedDateTime, isPlainDate, isPlainTime, isPlainDateTime, isDuration,
isPlainYearMonth, isPlainMonthDay, and related *Like helpers) duplicate the same
four-branch pattern (isType -> instanceof globalThis.Temporal.X ->
Symbol.toStringTag -> duck-typing), so extract a small helper (e.g.
hasTemporalTag) and refactor each guard to call it; update isTemporal to use
protoType + helper for specific Temporal classes and replace repeated
instanceof/toStringTag checks inside isInstant, isZonedDateTime, isPlainDate,
isPlainTime, isPlainDateTime, isDuration, isPlainYearMonth, and isPlainMonthDay
with calls to that helper and keep only the duck-typed heuristics in each guard.
- Line 33: The exported type-guard isDigit misnames a check for Number|BigInt;
rename the function to a clearer name (e.g., isNumberOrBigInt or isNumericType)
and update its export and all internal imports/usages to that new identifier;
keep the implementation (isType(obj, 'Number', 'BigInt')) but change the
function name and associated JSDoc/comments, and optionally add a
backward-compatible deprecated alias export named isDigit that delegates to the
new function to avoid breaking external callers while you update call sites.
In `@packages/library/src/common/logify.class.ts`:
- Around line 103-113: LOG includes an orphaned Trace level (LOG.Trace) that has
no corresponding Logify.trace method or Method/Level entry, so enable it by
adding a trace handler or remove it; specifically either add a Logify.trace(...)
that forwards to this.#trap(Method.Trace, ...) and create matching Method.Trace
and Level.Trace entries so emission differs from Debug, or remove LOG.Trace from
the enum to avoid exposing an unsupported level; update any references to
LogLevel/Level to reflect the chosen change.
- Around line 40-43: The public LOG enum and the internal Level map can drift;
change the code so LOG is derived from (or directly references) the internal
Level values instead of duplicating numeric assignments: ensure the exported LOG
symbol reflects Level's severities and update any checks using currentLevel and
methodLevel (e.g., the calculation of currentLevel from config.debug and the
lookup Level[method] in methodLevel) to use the unified source so methodLevel <=
currentLevel gating remains correct. Locate the Level object and exported LOG
enum definitions and refactor so LOG is bound to Level (or vice versa) to keep
severities in sync across the class.
In `@packages/library/src/common/proxy.library.ts`:
- Around line 97-109: The get trap currently writes a hidden undefined property
onto the real target (see onGet, pending, t, k and the Object.defineProperty
call guarded by Reflect.isExtensible) to mark misses, which mutates user data
and can shadow later valid discoveries and leak into Reflect.ownKeys; fix it by
replacing that mutation with a side-cache: introduce a WeakMap (e.g., missCache:
WeakMap<object, Set<PropertyKey>>) keyed by target and store k in the Set
instead of defining a property, check missCache.has(target) &&
missCache.get(target).has(k) before calling onGet, and remove the
Object.defineProperty call and related Reflect.has checks that rely on the
synthetic descriptor; alternatively if mutation is acceptable, add a clear
explanatory comment at the Object.defineProperty site documenting the tradeoffs
(shadowing, no eviction, visibility via Reflect.ownKeys) so reviewers know it is
intentional.
In `@packages/library/test/common/logify.class.test.ts`:
- Around line 3-52: Add two tests to exercise the debug:true mapping and the Log
method: create a test that constructs new Logify('LogifyTestDebugTrue', { debug:
true }) and calls .debug('debug-visible'), asserting console.debug was called
with 'LogifyTestDebugTrue: debug-visible' (restore spy afterward), and add a
test that constructs new Logify('LogifyTestLog') and calls .log('log-visible'),
asserting console.log was called with 'LogifyTestLog: log-visible' (restore spy
afterward); reference the Logify constructor and its .debug and .log methods and
ensure spies use mockImplementation to avoid real output.
- Around line 40-51: Rename the test or add a short inline comment to clarify
that setting debug: 0 intentionally suppresses console.error (i.e., LOG.Off
behavior) so the assertion expect(errorSpy).not.toHaveBeenCalled() is
deliberate; update the test name 'rethrows errors even when log emission is
gated off' or add a comment next to the Logify instantiation (new
Logify('LogifyTestRethrow', { debug: 0, catch: false })) referencing that debug:
0 corresponds to LOG.Off and therefore error-level emissions are suppressed even
though log.error rethrows the error.
In `@packages/library/test/common/pledge.class.test.ts`:
- Around line 68-111: The assertion that the warn message equals the exact
prefixed string is too tightly coupled to Logify's framing; update the test in
the 'callback failures warn at default debug level (indirect Logify
integration)' case to either (a) assert only on the variable part of the message
(e.g., that the warn output contains 'Pledge callback failed' rather than the
full 'Pledge: Pledge callback failed' prefix) or (b) import/use a shared
constant from Logify (eg. a published MESSAGE_PREFIX or format helper) and build
the expected string from that constant so the test uses Pledge and Logify's
canonical prefix rather than a hardcoded literal; keep the existing
warnSpy/debugSpy setup and the await flush sequence unchanged.
In `@packages/tempo/src/engine/engine.layout.ts`:
- Around line 69-95: The current resolveLayoutOrder uses repeated
layouts.findIndex calls inside mdyLayouts.forEach which is O(N·M); to speed this
up, after creating layouts = ownEntries(ordered) build a Map from
key.description to index (e.g., descIndexMap) and use that map to look up
idx1/idx2 instead of findIndex; when you swap entries in layouts update both the
map entries for the two descriptions and set changed = true as before, and keep
the existing return behavior (return ordered when !changed to preserve the
reference-preservation contract used by Tempo.#swapLayout).
- Around line 45-58: The preferred-names loop in
resolveLayoutClassificationOrder can push the same entry multiple times (if
preferred contains duplicates) which makes next.length > entries.length and
prevents detection of reordering; fix by deduplicating when iterating preferred
(use seen to skip if entry[0] already in seen) so you only push unique entries
from preferred into next (references: resolveLayoutClassificationOrder,
preferred, byName, seen, next, entries), ensuring the changed check correctly
detects reorderings instead of silently returning the original layout.
In `@packages/tempo/src/support/tempo.default.ts`:
- Line 30: The guard regular expression on the exported property named "guard"
contains a duplicated escaped hyphen (\-) inside the character class; edit the
guard value in tempo.default.ts (the guard property) to remove the redundant \-
so the character class only includes a single hyphen escape, leaving the rest of
the tokens unchanged.
In `@packages/tempo/src/support/tempo.init.ts`:
- Around line 48-80: There is duplicated Object.defineProperties descriptor
setup for state.config in the isGlobal and fallback branches; extract a small
helper (e.g., buildConfigDescriptors or applyCommonConfigDescriptors) that
returns/applies the shared descriptors for calendar, timeZone, locale, discovery
(using Symbol.keyFor(sym.$Tempo)), formats (enumify(STATE.FORMAT, false)),
sphere (getHemisphere(timeZone)), and get (function), then call that helper from
both branches and only set scope/catch separately (use setProperty for catch
when needed); also remove the redundant re-destructuring of timeZone/calendar
since getDateTimeFormat() was already called earlier.
In `@packages/tempo/src/tempo.type.ts`:
- Line 10: The import mixes a value import (sym) with a type-only export
(TempoBrand); change the import to import sym as a value and TempoBrand as a
type-only import (use the inline type modifier for TempoBrand) so TempoBrand is
imported with the type qualifier and avoids isolatedModules/verbatimModuleSyntax
build errors when used in the type position (refer to the TempoBrand symbol and
the sym value import in this file).
In `@packages/tempo/test/term_unified.test.ts`:
- Around line 91-108: Add a negative test that constructs a Tempo instance with
an unknown sphere (e.g. new Tempo(testDate, { catch: true, sphere: 'unknownzone'
})) and assert that queries relying on getRange/resolveCycleWindow do not
resolve to a hemisphere value but instead trigger the intended failure path
(e.g. produce an empty result or cause the TermError dispatch from
term.util.ts); use the same pattern as existing tests (format('{`#qtr`}') and
set({ start: '#quarter' })) to confirm it does not return Q4 or a valid date and
that TermError handling is invoked.
🪄 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: 8e12ea4a-29d9-4120-8ff7-835529fdb43f
📒 Files selected for processing (37)
CHANGELOG.mdpackages/library/src/common/array.library.tspackages/library/src/common/assertion.library.tspackages/library/src/common/coercion.library.tspackages/library/src/common/enumerate.library.tspackages/library/src/common/logify.class.tspackages/library/src/common/pledge.class.tspackages/library/src/common/primitive.library.tspackages/library/src/common/proxy.library.tspackages/library/src/common/string.library.tspackages/library/src/common/temporal.library.tspackages/library/src/common/utility.library.tspackages/library/test/common/logify.class.test.tspackages/library/test/common/pledge.class.test.tspackages/tempo/CHANGELOG.mdpackages/tempo/bin/tsconfig.jsonpackages/tempo/doc/installation.mdpackages/tempo/doc/migration-guide.mdpackages/tempo/doc/releases/v2.x.mdpackages/tempo/doc/sandbox-factory.mdpackages/tempo/doc/tempo.cookbook.mdpackages/tempo/index.mdpackages/tempo/plan/release-process.mdpackages/tempo/src/discrete/discrete.parse.tspackages/tempo/src/engine/engine.layout.tspackages/tempo/src/engine/engine.term.tspackages/tempo/src/plugin/extend/extend.ticker.tspackages/tempo/src/plugin/term.util.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.index.tspackages/tempo/src/tempo.type.tspackages/tempo/test/sandbox-factory.test.tspackages/tempo/test/term_unified.test.tspackages/tempo/test/tsconfig.json
✅ Files skipped from review due to trivial changes (6)
- packages/tempo/src/tempo.index.ts
- packages/tempo/doc/tempo.cookbook.md
- packages/tempo/plan/release-process.md
- packages/tempo/doc/sandbox-factory.md
- CHANGELOG.md
- packages/tempo/src/engine/engine.term.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/tempo/CHANGELOG.md
- packages/tempo/test/tsconfig.json
- packages/library/src/common/enumerate.library.ts
- packages/tempo/test/sandbox-factory.test.ts
- packages/library/src/common/array.library.ts
- packages/tempo/bin/tsconfig.json
- packages/library/src/common/temporal.library.ts
- packages/tempo/src/plugin/extend/extend.ticker.ts
- packages/library/src/common/utility.library.ts
- packages/tempo/src/support/tempo.enum.ts
- packages/tempo/src/discrete/discrete.parse.ts
- packages/tempo/src/tempo.class.ts
|
@coderabbitai please review |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
packages/library/src/common/logify.class.ts (1)
19-26: Dedupe severity numbers betweenLevelandLOG.
Level(19–26) andLOG(107–114) independently encode the sameerror=1…trace=5ladder. If someone later renumbersLOG(e.g., inserts a new level) without updatingLevel, gating silently goes wrong. Derive one from the other so there’s a single source of truth.♻️ Proposed refactor — move `LOG` above and derive `Level` from it
-/** `@internal` severity levels mapped to Method names for gating logic (not exported; see LOG enum for public API) */ -const Level = { - [Method.Error]: 1, - [Method.Warn]: 2, - [Method.Info]: 3, - [Method.Log]: 3, - [Method.Debug]: 4, - [Method.Trace]: 5, -} as const; +/** logging severity levels for Logify output control */ +export const LOG = enumify({ + Off: 0, + Error: 1, + Warn: 2, + Info: 3, + Debug: 4, + Trace: 5, +}, false); +export type LOG = ValueOf<typeof LOG> +export type LogLevel = KeyOf<typeof LOG> + +/** `@internal` severity levels mapped to Method names for gating logic */ +const Level = { + [Method.Error]: LOG.Error, + [Method.Warn]: LOG.Warn, + [Method.Info]: LOG.Info, + [Method.Log]: LOG.Info, + [Method.Debug]: LOG.Debug, + [Method.Trace]: LOG.Trace, +} as const;And remove the duplicate
LOG/LogLevelblock at lines 106–116.Also applies to: 107-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/library/src/common/logify.class.ts` around lines 19 - 26, Level and LOG currently duplicate the same severity mappings (error=1…trace=5), which risks divergence; move the LOG/LogLevel definition above the Level block and derive Level from LOG (e.g., map Method.* keys to LOG values) so there's a single source of truth, then remove the duplicate LOG/LogLevel block later in the file; update any references to use the canonical LOG/LogLevel symbol and keep Method, Level, and LogLevel names intact.packages/tempo/src/plugin/term.util.ts (1)
285-316: Sphere-aware fallback: specificity + deterministic tie-break are aligned withgetRange; minor index-safety nit.The scan-all/longest-sphere-wins logic with
localeComparetie-break is a clear improvement over the previousfind(...)behaviour, and the containment direction (targetSphere.includes(sphereLower)) now matches thegetRangefilter (config-as-more-specific).Small nit — Line 288:
targetParts[sphereIdx].toLowerCase()assumesgroupKey.split('.')yields at leastsphereIdx + 1entries. It will, unless any group value (e.g.options[key]/anchor?.[key]/t.config[key]) contains a literal.(e.g. a calendar-like id). In that pathological casetargetParts[sphereIdx]can beundefinedand.toLowerCase()throws. Defensive fallback:🛡️ Suggested guard
- const sphereIdx = groupBy.indexOf('sphere'); - const targetParts = groupKey.split('.'); - const targetSphere = targetParts[sphereIdx].toLowerCase(); + const sphereIdx = groupBy.indexOf('sphere'); + const targetParts = groupKey.split('.'); + const targetSphere = (targetParts[sphereIdx] ?? '').toLowerCase();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/plugin/term.util.ts` around lines 285 - 316, The code in the sphere-aware fallback uses targetParts[sphereIdx].toLowerCase() which can throw if groupKey.split('.') produced fewer segments; update the guard in the block inside the if (list.length === 0 && groupBy.includes('sphere')) so targetSphere is computed safely (e.g. derive targetParts then use a defensive fallback like (targetParts[sphereIdx] ?? '') or check targetParts.length > sphereIdx before calling toLowerCase), and ensure subsequent logic that depends on targetSphere handles the empty-string case (skip matching or continue) so bestKey selection remains safe; change only the targetSphere extraction in the block around targetParts/targetSphere in term.util.ts.
🤖 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/library/src/common/proxy.library.ts`:
- Around line 44-59: The getOwnPropertyDescriptor trap currently invokes onGet
for symbol keys while the get trap skips symbols (uses !isSymbol(k)), causing
inconsistent materialization; update getOwnPropertyDescriptor in the proxy
implementation to mirror the get trap's symbol handling by skipping onGet for
symbols (i.e., check isSymbol(k) before calling onGet) so both
getOwnPropertyDescriptor and get behave consistently when keys and onGet are
used together; apply the same alignment to the corresponding logic in the other
proxy block (the one around the get trap at ~lines 95–129).
- Around line 108-120: The sentinel property added during property discovery is
defined non-writable, which prevents future assignments when using
delegate(target, onGet, false); update the Object.defineProperty call inside the
Property Discovery block (where onGet, pending, isSymbol, isDefined are used) to
mark the cached property as writable (set writable: true) so non-frozen
delegates can later accept writes, or alternatively avoid mutating the target by
tracking missing keys in the existing pending/another Set; ensure the descriptor
includes writable: true along with enumerable: false and configurable: true.
In `@packages/tempo/src/tempo.type.ts`:
- Line 15: The import statement in tempo.type.ts has inconsistent double spaces
inside the destructured import; update the import for TermPlugin and Plugin (the
line importing TermPlugin and Plugin from '#tempo/plugin/plugin.type.js') to use
normalized single spacing between the braces and identifiers so it reads with
consistent spacing (e.g., single spaces after '{' and between items).
- Around line 191-193: The type declares non-optional fields matches and anchor
but several call sites create t.Internal.State with empty objects using
assertions; update the model to reflect actual usage by making matches and
anchor optional (matches?: Match[]; anchor?: Temporal.ZonedDateTime) OR ensure
they are always initialized in the init path (e.g., in init() and where new
state objects are created such as the constructors/creators used by tempo.init,
discrete.parse, and tempo.class) and remove the unsafe "as t.Internal.State"
assertions; reference the symbols matches, anchor and the t.Internal.State type
and fix either the type definition or the state creation sites so TypeScript
safety matches runtime guarantees.
---
Nitpick comments:
In `@packages/library/src/common/logify.class.ts`:
- Around line 19-26: Level and LOG currently duplicate the same severity
mappings (error=1…trace=5), which risks divergence; move the LOG/LogLevel
definition above the Level block and derive Level from LOG (e.g., map Method.*
keys to LOG values) so there's a single source of truth, then remove the
duplicate LOG/LogLevel block later in the file; update any references to use the
canonical LOG/LogLevel symbol and keep Method, Level, and LogLevel names intact.
In `@packages/tempo/src/plugin/term.util.ts`:
- Around line 285-316: The code in the sphere-aware fallback uses
targetParts[sphereIdx].toLowerCase() which can throw if groupKey.split('.')
produced fewer segments; update the guard in the block inside the if
(list.length === 0 && groupBy.includes('sphere')) so targetSphere is computed
safely (e.g. derive targetParts then use a defensive fallback like
(targetParts[sphereIdx] ?? '') or check targetParts.length > sphereIdx before
calling toLowerCase), and ensure subsequent logic that depends on targetSphere
handles the empty-string case (skip matching or continue) so bestKey selection
remains safe; change only the targetSphere extraction in the block around
targetParts/targetSphere in term.util.ts.
🪄 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: e8d1fa2e-af8b-461f-84eb-4449d6f82344
📒 Files selected for processing (11)
packages/library/src/common/assertion.library.tspackages/library/src/common/logify.class.tspackages/library/src/common/primitive.library.tspackages/library/src/common/proxy.library.tspackages/library/test/common/pledge.class.test.tspackages/tempo/doc/migration-guide.mdpackages/tempo/doc/releases/v2.x.mdpackages/tempo/index.mdpackages/tempo/src/plugin/term.util.tspackages/tempo/src/support/tempo.init.tspackages/tempo/src/tempo.type.ts
✅ Files skipped from review due to trivial changes (2)
- packages/tempo/doc/releases/v2.x.md
- packages/tempo/doc/migration-guide.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/library/src/common/primitive.library.ts
- packages/tempo/src/support/tempo.init.ts
- packages/tempo/index.md
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/library/src/common/logify.class.ts (1)
45-80:⚠️ Potential issue | 🟡 MinorTrace output is hidden even with
debug: true.With
debug: true→currentLevel = LOG.Debug (4)andMethod.Trace → LOG.Trace (5), sotrace()calls never emit unless the caller passes a numericdebug >= 5(orLOG.Trace). That's consistent with syslog semantics, but since the JSDoc onLogify.Constructor.debug(Lines 115-125) only documentstrue → LOG.Debug, consumers who enable debug will silently lose theirtrace()output.Consider adding a brief note to the JSDoc that enabling
trace()requires a numericdebugofLOG.Trace, or maptruetoLOG.Traceif that was the intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/library/src/common/logify.class.ts` around lines 45 - 80, The trap currently maps config.debug === true to LOG.Debug, which hides Method.Trace output; change the mapping in Logify.#trap so that (config.debug === true) maps to LOG.Trace instead of LOG.Debug (i.e., set currentLevel = typeof config.debug === 'number' ? config.debug : (config.debug === true ? LOG.Trace : LOG.Info)), and update the Logify.Constructor.debug JSDoc to state that debug: true enables trace-level logging (or alternately document that numeric values control exact levels) so callers understand trace behavior; reference symbols: `#trap`, config.debug, Method.Trace, LOG.Debug, LOG.Trace, and Logify.Constructor.debug.
♻️ Duplicate comments (1)
packages/tempo/doc/tempo.modularity.md (1)
112-112:⚠️ Potential issue | 🟠 MajorDocumentation incorrectly suggests re-calling
Tempo.init()to pick up late imports.Line 112 states: "You would need to call
Tempo.init()again or useTempo.extend()to pick up the latecomers." However, as noted in the earlier comment on Line 11, the implementation returns early when state already exists, preventing re-initialization from picking up late imports.The correct guidance should be to exclusively use
Tempo.extend()for late-loaded modules, asinit()will not re-process them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/doc/tempo.modularity.md` at line 112, Update the documentation text that currently advises calling Tempo.init() again to pick up late-imported side-effect plugins: clarify that because Tempo.init() short-circuits when state already exists (see the init early-return behavior), re-calling init will not load late modules; instead instruct users to use Tempo.extend() exclusively for late-loaded plugins and remove the suggestion to re-run Tempo.init().
🧹 Nitpick comments (7)
packages/tempo/doc/tempo.config.md (1)
115-115: Clarify thatpluginsoption is processed differently from core init options.Line 115 documents
pluginsas a standardTempo.init()option. However, the implementation shows thatpluginsis handled inpackages/tempo/src/tempo.class.ts:411-413viathis.extend(p)rather than in the coreextendStatefunction (packages/tempo/src/support/tempo.init.ts:100-173) where other options likesnippet,layout,event,period, etc. are processed.While this is still a valid init option from a user perspective, it follows a different code path. Consider adding a note clarifying that plugins are extended onto the class rather than merged into the internal state, which distinguishes them from other configuration options.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/doc/tempo.config.md` at line 115, Update the plugins entry to note that the `plugins` option is processed differently from core init options: instead of being merged into the internal state via `extendState`, each plugin is applied to the Tempo instance via `this.extend(p)` (see `Tempo.init()` flow and the `extend` call), so plugins are extended onto the class instance rather than merged into the internal state like `snippet`, `layout`, `event`, `period`, etc.; add a short sentence describing this behavior and its implication for plugin authors (they augment the instance, not the internal state).packages/tempo/doc/architecture.md (1)
22-22: Consider varying sentence structure for better readability.The explanation contains three consecutive sentences beginning with "Scoped runtimes," which makes the text somewhat repetitive. Consider rephrasing to improve flow.
✍️ Suggested rewrite
-- **Scoped runtimes (Experimental)** — `TempoRuntime.createScoped()` returns a fresh, isolated runtime that is *not* stored on `globalThis`, enabling clean test isolation without globalThis manipulation. Scoped runtimes are currently an experimental internal feature and are not yet fully threaded through all core utilities. Scoped runtimes are not pinned to `globalThis`, lack the `defineProperty` descriptor protections of the primary instance, and instead rely solely on the lexical reference returned (contrasting with the hardened `getRuntime()` and `globalThis[BRIDGE]` behavior). Implementation examples of this test-scoping pattern can be found in [plugin_registration.test.ts](../test/plugin_registration.test.ts) and [duration.core.test.ts](../test/duration.core.test.ts). +- **Scoped runtimes (Experimental)** — `TempoRuntime.createScoped()` returns a fresh, isolated runtime that is *not* stored on `globalThis`, enabling clean test isolation without globalThis manipulation. Currently an experimental internal feature, they are not yet fully threaded through all core utilities. Unlike the primary instance, they are not pinned to `globalThis`, lack the `defineProperty` descriptor protections, and instead rely solely on the lexical reference returned (contrasting with the hardened `getRuntime()` and `globalThis[BRIDGE]` behavior). Implementation examples of this test-scoping pattern can be found in [plugin_registration.test.ts](../test/plugin_registration.test.ts) and [duration.core.test.ts](../test/duration.core.test.ts).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/doc/architecture.md` at line 22, The paragraph describing scoped runtimes is repetitive because three sentences begin with "Scoped runtimes"; reword it to vary sentence structure and improve flow by starting with the API name and effects (e.g., "TempoRuntime.createScoped() returns a fresh, isolated runtime..."), follow with a separate sentence noting experimental/internal status, then explain differences from the primary runtime (not pinned to globalThis, lacking defineProperty protections and relying on the lexical reference) and finish with the pointer to the example tests (plugin_registration.test.ts and duration.core.test.ts); update the block that mentions TempoRuntime.createScoped(), globalThis, defineProperty, getRuntime(), and [BRIDGE] accordingly to reflect the new sentence order and reduce repetition.packages/library/src/common/proxy.library.ts (1)
122-127: Consider caching bound functions to avoid per-access allocation and identity breakage.
val.bind(t)runs on every property read whenbindis enabled (allproxify(frozen=true)andsecure(...)consumers), which allocates a new function each time and breaks reference equality (e.g.,proxy.method === proxy.methodyieldsfalse, which can subtly invalidate memoization, event-listener add/remove pairing, and React dependency arrays).A small
WeakMap<Function, Function>keyed byvalgives a stable wrapper with negligible overhead.♻️ Sketch
function factory<T extends object>(target: T, options: ProxyOptions = {}): T { const { frozen, lock, appendOnly, onGet, keys, bind, skip } = options; const pending = new Set<PropertyKey>(); + const boundCache = bind ? new WeakMap<Function, Function>() : undefined; let cachedJSON: any; ... const val = Reflect.get(t, k, r); if (bind && isFunction(val)) { const desc = Object.getOwnPropertyDescriptor(t, k); if (desc && !desc.configurable && !desc.writable) return val; - return val.bind(t); + let bound = boundCache!.get(val); + if (!bound) { bound = val.bind(t); boundCache!.set(val, bound); } + return bound; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/library/src/common/proxy.library.ts` around lines 122 - 127, The property getter currently calls val.bind(t) on every access (see Reflect.get usage and the bind branch) which allocates a new function and breaks identity; fix by adding a WeakMap<Function, Function> cache keyed by the original val to return a stable bound wrapper instead of calling val.bind(t) each time (create the cache in the module scope or in proxify/secure initialization, use it when bind && isFunction(val) to return cachedBound || (cachedBound = val.bind(t)) and store it). Ensure you still return val for non-configurable/non-writable descriptors as currently handled.packages/tempo/src/tempo.type.ts (1)
199-204:Match.sourceis free-form — consider a typed literal set.The JSDoc hints at
'default' | 'local' | 'plugin:X', anddiscrete.parse.tsLine 455 currently emits only'global'or'local'. A string-literal union (e.g.,'default' | 'global' | 'local' | \plugin:${string}``) would prevent drift between producer and consumer, and make sandbox-traceability assertions in tests self-documenting.♻️ Suggested typing
- /** where this match came from (e.g. 'default', 'local', 'plugin:X') */ source?: string; + /** where this match came from */ + source?: 'default' | 'global' | 'local' | `plugin:${string}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/tempo.type.ts` around lines 199 - 204, The Match.type currently declares source?: string which is free-form; change it to a string-literal union to lock producers/consumers together (e.g., 'default' | 'global' | 'local' | `plugin:${string}`) and update the Match JSDoc accordingly so callers and tests (and the producer in discrete.parse.ts) use the same allowed set; locate the Match type declaration and replace the source property type with the literal union and adjust any imports/types if needed.packages/tempo/src/discrete/discrete.parse.ts (2)
133-135:isAnchoredValis equivalent toisAnchored.At Line 67
isAnchored = isDefined(val)whereval = dateTime ?? state.anchor ?? …. By the time we reach Line 133, nothing between has mutatedstate.anchorordateTime, soisAnchored || isDefined(dateTime) || isDefined(state.anchor)collapses toisAnchored. Safe to drop the alias.♻️ Suggested simplification
- const isAnchoredVal = isAnchored || isDefined(dateTime) || isDefined(state.anchor); const resolvingKeys = new Set<string>(); - const res = _ParseEngine.conform(state, tempo, today, isAnchoredVal, resolvingKeys); + const res = _ParseEngine.conform(state, tempo, today, isAnchored, resolvingKeys);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/discrete/discrete.parse.ts` around lines 133 - 135, The variable isAnchoredVal is redundant because isAnchored already equals isDefined(dateTime ?? state.anchor ?? …); remove isAnchoredVal and pass isAnchored directly to _ParseEngine.conform (update the call site where isAnchoredVal is used and delete its declaration), keeping the rest of the logic and resolvingKeys unchanged; specifically edit the block that defines isAnchoredVal and calls _ParseEngine.conform to use isAnchored instead.
451-469: ReuseisFninstead of recomputingisFunction(definition).
isFnis already computed at Line 397 for the samedefinition. Minor consistency nit.♻️ Suggested
- const resolveVal = isFunction(definition) ? res : definition; + const resolveVal = isFn ? res : definition;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/src/discrete/discrete.parse.ts` around lines 451 - 469, The code recomputes isFunction(definition) when isFn was already determined earlier; update the assignment of resolveVal to use the existing isFn variable (resolveVal = isFn ? res : definition) inside the try block where type, pat, resolveVal, source are set so you reuse the precomputed predicate and maintain consistency with the earlier computation of isFn for definition.packages/tempo/package.json (1)
25-39:sideEffectsglobs are broad — double-check they don't over-include.
**/engine.*.js,**/module.*.js, and**/discrete.*.jsmatch any file namedengine.foo.js,module.foo.js, ordiscrete.foo.jsanywhere in the package (e.g., intest/or nested bundles). Given thatfileswhitelists onlydist/andimg/, that's fine in practice, but future-you may add otherengine.*/module.*utility files that are pure and should be tree-shaken. Consider scoping these to the directories that actually host side-effect modules, e.g.**/engine/engine.*.js,**/module/module.*.js,**/discrete/discrete.*.js.♻️ Suggested tightening
- "**/plugin/extend/extend.*.js", - "**/plugin/extend/extend.*.ts", - "**/engine.*.js", - "**/engine.*.ts", - "**/module.*.js", - "**/module.*.ts", - "**/discrete.*.js", - "**/discrete.*.ts" + "**/plugin/extend/extend.*.js", + "**/plugin/extend/extend.*.ts", + "**/engine/engine.*.js", + "**/engine/engine.*.ts", + "**/module/module.*.js", + "**/module/module.*.ts", + "**/discrete/discrete.*.js", + "**/discrete/discrete.*.ts"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tempo/package.json` around lines 25 - 39, The sideEffects array contains broad globs ("**/engine.*.js", "**/module.*.js", "**/discrete.*.js" and their .ts variants) that may over-include unrelated files; narrow them to the actual directories that contain side-effectful modules by updating those entries in package.json's sideEffects to target the specific folders that host those runtime modules (e.g., scope to engine/, module/, discrete/ subfolders or the dist/ layout you publish), and update the matching .ts entries as well; keep the existing files whitelist behavior (dist/ and img/) in mind and run a quick packaging/build smoke test to confirm nothing intended for side-effects is accidentally excluded.
🤖 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/library/src/common/proxy.library.ts`:
- Around line 157-160: delegator currently returns a writable proxy so
assignments like proxy[key] = value mutate the empty target and break future
delegation; fix this by making the proxy read-only when calling factory inside
delegator: pass the appropriate immutability flag (e.g., frozen: true) in the
second argument to factory (alongside keys: keyList and onGet: fn) so writes
cannot overwrite the virtual forwarding behavior of delegator.
In `@packages/tempo/doc/tempo.modularity.md`:
- Line 11: The docs claim Tempo.init() will pick up side-effect modules if
called again but the implementation in tempo.init.ts short-circuits when
runtime.state exists (the if (isGlobal && runtime.state && !baseState) return
runtime.state;), so reconcile by choosing one approach: either remove/adjust
that early-return in Tempo.init (and ensure re-init re-processes imports and
merges state) or update documentation to state re-calling Tempo.init() will NOT
discover new side-effect modules and instruct users to call Tempo.extend() to
add modules; update tempo.modularity.md to remove the misleading line about
re-calling init and add clear guidance to use Tempo.extend(), and add a comment
near the runtime.state check in tempo.init.ts explaining the chosen behavior.
In `@packages/tempo/doc/tempo.plugin.md`:
- Around line 77-79: Remove or populate the empty admonition block in the
Markdown: locate the standalone "::: warning" / closing "::: " callout in
tempo.plugin.md and either delete both the opening and closing markers or add
the intended warning text between them so the warning box is not rendered empty.
In `@packages/tempo/doc/tempo.shorthand.md`:
- Around line 89-97: Update the Docusaurus admonitions to use the correct syntax
by replacing both occurrences of "::: warning" with ":::warning" so the blocks
render as intended; then reconcile the reserved-characters list for Range-Keys
by either adding the hyphen character "-" to the listed reserved characters (so
the Range-Keys warning matches the documented Navigation Modifier "Relative
Past" / "-" alias for "<") or add a short sentence next to the Range-Keys
section explaining why "-" is not reserved when used in range-keys, referencing
the "Range-Keys" heading and the "Relative Past" (alias "<") / "-" modifier.
In `@packages/tempo/src/discrete/discrete.parse.ts`:
- Around line 427-446: The HH:MM check using /^\d{1,2}:\d{2}$/ should be
tightened to only accept valid hours and minutes (e.g. hours 00–23 and minutes
00–59) before calling dateTime.with({ hour, minute, ... })—update the regex used
in the isString(result) branch to enforce that range so invalid times like
"25:70" are treated as non-matches and fall through to the stringify branch;
also remove the redundant try/catch that simply rethrows (the catch block around
the handler-return logic that does "catch (e: any) { throw e; }"), letting
exceptions propagate naturally while keeping assignment to state.zdt and the
other branches (isTempo, isZonedDateTime, object with toDateTime) intact.
---
Outside diff comments:
In `@packages/library/src/common/logify.class.ts`:
- Around line 45-80: The trap currently maps config.debug === true to LOG.Debug,
which hides Method.Trace output; change the mapping in Logify.#trap so that
(config.debug === true) maps to LOG.Trace instead of LOG.Debug (i.e., set
currentLevel = typeof config.debug === 'number' ? config.debug : (config.debug
=== true ? LOG.Trace : LOG.Info)), and update the Logify.Constructor.debug JSDoc
to state that debug: true enables trace-level logging (or alternately document
that numeric values control exact levels) so callers understand trace behavior;
reference symbols: `#trap`, config.debug, Method.Trace, LOG.Debug, LOG.Trace, and
Logify.Constructor.debug.
---
Duplicate comments:
In `@packages/tempo/doc/tempo.modularity.md`:
- Line 112: Update the documentation text that currently advises calling
Tempo.init() again to pick up late-imported side-effect plugins: clarify that
because Tempo.init() short-circuits when state already exists (see the init
early-return behavior), re-calling init will not load late modules; instead
instruct users to use Tempo.extend() exclusively for late-loaded plugins and
remove the suggestion to re-run Tempo.init().
---
Nitpick comments:
In `@packages/library/src/common/proxy.library.ts`:
- Around line 122-127: The property getter currently calls val.bind(t) on every
access (see Reflect.get usage and the bind branch) which allocates a new
function and breaks identity; fix by adding a WeakMap<Function, Function> cache
keyed by the original val to return a stable bound wrapper instead of calling
val.bind(t) each time (create the cache in the module scope or in proxify/secure
initialization, use it when bind && isFunction(val) to return cachedBound ||
(cachedBound = val.bind(t)) and store it). Ensure you still return val for
non-configurable/non-writable descriptors as currently handled.
In `@packages/tempo/doc/architecture.md`:
- Line 22: The paragraph describing scoped runtimes is repetitive because three
sentences begin with "Scoped runtimes"; reword it to vary sentence structure and
improve flow by starting with the API name and effects (e.g.,
"TempoRuntime.createScoped() returns a fresh, isolated runtime..."), follow with
a separate sentence noting experimental/internal status, then explain
differences from the primary runtime (not pinned to globalThis, lacking
defineProperty protections and relying on the lexical reference) and finish with
the pointer to the example tests (plugin_registration.test.ts and
duration.core.test.ts); update the block that mentions
TempoRuntime.createScoped(), globalThis, defineProperty, getRuntime(), and
[BRIDGE] accordingly to reflect the new sentence order and reduce repetition.
In `@packages/tempo/doc/tempo.config.md`:
- Line 115: Update the plugins entry to note that the `plugins` option is
processed differently from core init options: instead of being merged into the
internal state via `extendState`, each plugin is applied to the Tempo instance
via `this.extend(p)` (see `Tempo.init()` flow and the `extend` call), so plugins
are extended onto the class instance rather than merged into the internal state
like `snippet`, `layout`, `event`, `period`, etc.; add a short sentence
describing this behavior and its implication for plugin authors (they augment
the instance, not the internal state).
In `@packages/tempo/package.json`:
- Around line 25-39: The sideEffects array contains broad globs
("**/engine.*.js", "**/module.*.js", "**/discrete.*.js" and their .ts variants)
that may over-include unrelated files; narrow them to the actual directories
that contain side-effectful modules by updating those entries in package.json's
sideEffects to target the specific folders that host those runtime modules
(e.g., scope to engine/, module/, discrete/ subfolders or the dist/ layout you
publish), and update the matching .ts entries as well; keep the existing files
whitelist behavior (dist/ and img/) in mind and run a quick packaging/build
smoke test to confirm nothing intended for side-effects is accidentally
excluded.
In `@packages/tempo/src/discrete/discrete.parse.ts`:
- Around line 133-135: The variable isAnchoredVal is redundant because
isAnchored already equals isDefined(dateTime ?? state.anchor ?? …); remove
isAnchoredVal and pass isAnchored directly to _ParseEngine.conform (update the
call site where isAnchoredVal is used and delete its declaration), keeping the
rest of the logic and resolvingKeys unchanged; specifically edit the block that
defines isAnchoredVal and calls _ParseEngine.conform to use isAnchored instead.
- Around line 451-469: The code recomputes isFunction(definition) when isFn was
already determined earlier; update the assignment of resolveVal to use the
existing isFn variable (resolveVal = isFn ? res : definition) inside the try
block where type, pat, resolveVal, source are set so you reuse the precomputed
predicate and maintain consistency with the earlier computation of isFn for
definition.
In `@packages/tempo/src/tempo.type.ts`:
- Around line 199-204: The Match.type currently declares source?: string which
is free-form; change it to a string-literal union to lock producers/consumers
together (e.g., 'default' | 'global' | 'local' | `plugin:${string}`) and update
the Match JSDoc accordingly so callers and tests (and the producer in
discrete.parse.ts) use the same allowed set; locate the Match type declaration
and replace the source property type with the literal union and adjust any
imports/types if needed.
🪄 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: a11838de-47d4-4278-b8b1-d91efbe710c4
📒 Files selected for processing (23)
packages/library/src/common/logify.class.tspackages/library/src/common/proxy.library.tspackages/tempo/doc/Tempo.mdpackages/tempo/doc/architecture.mdpackages/tempo/doc/commercial.mdpackages/tempo/doc/installation.mdpackages/tempo/doc/soft_freeze_strategy.mdpackages/tempo/doc/tempo.api.mdpackages/tempo/doc/tempo.benchmarks.mdpackages/tempo/doc/tempo.config.mdpackages/tempo/doc/tempo.cookbook.mdpackages/tempo/doc/tempo.enumerators.mdpackages/tempo/doc/tempo.format.mdpackages/tempo/doc/tempo.modularity.mdpackages/tempo/doc/tempo.parse.mdpackages/tempo/doc/tempo.plugin.mdpackages/tempo/doc/tempo.shorthand.mdpackages/tempo/doc/tempo.term.mdpackages/tempo/doc/tempo.ticker.mdpackages/tempo/package.jsonpackages/tempo/src/discrete/discrete.parse.tspackages/tempo/src/plugin/term.util.tspackages/tempo/src/tempo.type.ts
💤 Files with no reviewable changes (1)
- packages/tempo/doc/Tempo.md
✅ Files skipped from review due to trivial changes (8)
- packages/tempo/doc/commercial.md
- packages/tempo/doc/tempo.benchmarks.md
- packages/tempo/doc/tempo.api.md
- packages/tempo/doc/soft_freeze_strategy.md
- packages/tempo/doc/tempo.format.md
- packages/tempo/doc/tempo.parse.md
- packages/tempo/doc/tempo.enumerators.md
- packages/tempo/doc/installation.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/tempo/doc/tempo.cookbook.md
magmacomputing
left a comment
magmacomputing
left a comment
There was a problem hiding this comment.
Code meets Project standards, and test have 100% pass rate.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
packages/library/test/common/proxy.library.test.ts (1)
1-1: Consider adding adelegatetest for parity with the AI summary / module surface.The PR summary advertises coverage of
delegate, but onlyproxifyanddelegatorare imported here. Sincedelegateshares the same factory plumbing but with a different surface (callback-driven discovery on a real target, optional readonly), a small smoke test for it would round out the suite and protect against future regressions in theonGetpathway when keys are not fixed.-import { delegator, proxify } from '#library/proxy.library.js'; +import { delegate, delegator, proxify } from '#library/proxy.library.js';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/library/test/common/proxy.library.test.ts` at line 1, Add a small smoke test that imports delegate from '#library/proxy.library.js' alongside proxify/delegator and exercises the callback-driven discovery path: create a plain target object (e.g. {foo: 'bar'}), call delegate(target, discoveryCallback, { readonly: true/false } as needed) where discoveryCallback is invoked on property access and returns the underlying value, then assert that reading proxy.foo yields 'bar' and that readonly mode blocks writes (or allows writes when not readonly). Place the new test next to existing proxify/delegator tests and reference the delegate symbol so future changes to the onGet pathway are covered.packages/tempo/doc/tempo.config.md (1)
126-128: Note numeric0(LOG.Off) also silences error logs.While this info block correctly clarifies that string labels aren't supported, consider also mentioning the subtle trap where
debug: 0silencesconsole.erroroutput (the error is still thrown whencatch: false, but nothing is logged). This matches the warning already present inLogify.Constructor.debug's JSDoc and will save users a debugging detour.🤖 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 126 - 128, Add a short sentence to the info block explaining that setting debug to numeric 0 (LOG.Off) will also silence console.error messages (errors are still thrown when catch: false but won’t be logged), and reference the existing JSDoc identifier Logify.Constructor.debug to make the behavior consistent with its warning; keep the note concise and mention LOG.Off so users recognize the numeric value.packages/tempo/src/support/tempo.init.ts (2)
49-62: RedundantgetDateTimeFormat()call.Line 51 re-invokes
getDateTimeFormat()and re-destructures{ timeZone, calendar }which are already computed and in scope from Line 24. Remove the duplicate call;localecan still be fetched on Line 55 if needed, or pulled from the same initial call.♻️ Proposed fix
if (isGlobal) { markConfig(Object.assign(state.config, Default)); - const { timeZone, calendar } = getDateTimeFormat(); Object.defineProperties(state.config, {🤖 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 49 - 62, The code makes a redundant call to getDateTimeFormat() inside the Object.defineProperties block; reuse the previously computed { timeZone, calendar, locale } (from the earlier getDateTimeFormat() call) instead of calling getDateTimeFormat() again—remove the duplicate invocation, reference the existing timeZone and calendar variables for their properties, and read locale from the same earlier result (or from the already-in-scope variable) when defining the locale property on state.config; keep the other properties (discovery, formats, sphere, get, scope, catch) unchanged.
49-90: Consider consolidating the three config branches.Global (49–62) and local-without-baseState (76–90) are nearly identical except for
scopeand the conditionalcatch. The baseState branch (63–75) duplicates the list of defined property names. Extracting a shared descriptor builder would reduce drift risk (e.g., adding a new config field today requires editing three places and failing to keep them in sync is exactly what the past review flagged about shape differences across branches).No behavior change intended; purely a maintainability suggestion.
🤖 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 49 - 90, The three branches in tempo.init.ts duplicate Object.defineProperties for state.config (seen around markConfig, Object.defineProperties, and setProperty) — extract a shared descriptor builder function (e.g., buildConfigDescriptors or getConfigDescriptors) that accepts overrides (scope value and whether to include/set 'catch') and uses getDateTimeFormat(), enumify(STATE.FORMAT), getHemisphere(timeZone), Symbol.keyFor(sym.$Tempo), etc., then call markConfig and Object.defineProperties(state.config, buildConfigDescriptors(...)) in each branch, and only apply setProperty(state.config, 'catch', options.catch) or a different scope ('global' vs 'local') via the overrides so behavior is unchanged but the descriptor list is defined once.
🤖 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/library/src/common/logify.class.ts`:
- Around line 47-50: The tempo.config.md doc incorrectly states that debug: true
maps to LOG.Debug; update that doc to match the implementation in Logify where
config.debug === true maps to LOG.Trace (refer to the Logify class and the
config.debug runtime mapping that uses LOG.Trace), ensuring the doc text and any
examples mention LOG.Trace instead of LOG.Debug so docs and runtime are
consistent.
In `@packages/library/test/common/proxy.library.test.ts`:
- Line 52: The test passes a readonly tuple literal to delegator (proxy test
uses delegator(['alpha'] as const, key => `${key}-value`) cast to
Record<'alpha', string>) which fails under strict types because delegator's
first parameter is typed as K[] | Record<K, any>; update the delegator signature
in proxy.library.ts (the delegator function) to accept readonly K[] (e.g.,
change K[] to readonly K[]) or alternatively remove the as const in the test so
a mutable array is passed; pick one fix and update the corresponding declaration
or test accordingly so the readonly array is assignable.
In `@packages/tempo/doc/tempo.config.md`:
- Line 118: The docs table entry for the `debug` option is wrong: update the
description in tempo.config.md so it reflects Logify's actual behavior by
stating that `debug: true` maps to `LOG.Trace` (per the Logify implementation
and JSDoc in Logify class) and that numeric values map directly to `LOG` levels;
reference the `Logify` symbol and `LOG.Trace` when making the change so readers
aren't misled.
In `@packages/tempo/src/support/tempo.init.ts`:
- Around line 63-75: In the baseState sandbox branch, avoid unconditionally
calling setProperty(state.config, 'catch', options.catch) because assigning
undefined will shadow baseState.config.catch; update the else if (baseState)
branch to only call setProperty for 'catch' when options.catch is defined (e.g.,
wrap with isDefined(options.catch) or options.catch !== undefined) so the
sandbox inherits the parent's catch handler unless explicitly overridden.
---
Nitpick comments:
In `@packages/library/test/common/proxy.library.test.ts`:
- Line 1: Add a small smoke test that imports delegate from
'#library/proxy.library.js' alongside proxify/delegator and exercises the
callback-driven discovery path: create a plain target object (e.g. {foo:
'bar'}), call delegate(target, discoveryCallback, { readonly: true/false } as
needed) where discoveryCallback is invoked on property access and returns the
underlying value, then assert that reading proxy.foo yields 'bar' and that
readonly mode blocks writes (or allows writes when not readonly). Place the new
test next to existing proxify/delegator tests and reference the delegate symbol
so future changes to the onGet pathway are covered.
In `@packages/tempo/doc/tempo.config.md`:
- Around line 126-128: Add a short sentence to the info block explaining that
setting debug to numeric 0 (LOG.Off) will also silence console.error messages
(errors are still thrown when catch: false but won’t be logged), and reference
the existing JSDoc identifier Logify.Constructor.debug to make the behavior
consistent with its warning; keep the note concise and mention LOG.Off so users
recognize the numeric value.
In `@packages/tempo/src/support/tempo.init.ts`:
- Around line 49-62: The code makes a redundant call to getDateTimeFormat()
inside the Object.defineProperties block; reuse the previously computed {
timeZone, calendar, locale } (from the earlier getDateTimeFormat() call) instead
of calling getDateTimeFormat() again—remove the duplicate invocation, reference
the existing timeZone and calendar variables for their properties, and read
locale from the same earlier result (or from the already-in-scope variable) when
defining the locale property on state.config; keep the other properties
(discovery, formats, sphere, get, scope, catch) unchanged.
- Around line 49-90: The three branches in tempo.init.ts duplicate
Object.defineProperties for state.config (seen around markConfig,
Object.defineProperties, and setProperty) — extract a shared descriptor builder
function (e.g., buildConfigDescriptors or getConfigDescriptors) that accepts
overrides (scope value and whether to include/set 'catch') and uses
getDateTimeFormat(), enumify(STATE.FORMAT), getHemisphere(timeZone),
Symbol.keyFor(sym.$Tempo), etc., then call markConfig and
Object.defineProperties(state.config, buildConfigDescriptors(...)) in each
branch, and only apply setProperty(state.config, 'catch', options.catch) or a
different scope ('global' vs 'local') via the overrides so behavior is unchanged
but the descriptor list is defined once.
🪄 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: c7e7ca34-1526-4517-ba5f-b2a30d9f8bb4
📒 Files selected for processing (13)
packages/library/src/common/logify.class.tspackages/library/src/common/proxy.library.tspackages/library/test/common/logify.class.test.tspackages/library/test/common/proxy.library.test.tspackages/tempo/doc/architecture.mdpackages/tempo/doc/tempo.config.mdpackages/tempo/doc/tempo.modularity.mdpackages/tempo/doc/tempo.plugin.mdpackages/tempo/doc/tempo.shorthand.mdpackages/tempo/package.jsonpackages/tempo/src/discrete/discrete.parse.tspackages/tempo/src/support/tempo.init.tspackages/tempo/src/tempo.type.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/tempo/doc/tempo.plugin.md
- packages/library/test/common/logify.class.test.ts
- packages/tempo/doc/architecture.md
- packages/tempo/doc/tempo.shorthand.md
- packages/library/src/common/proxy.library.ts
| const currentLevel = (typeof config.debug === 'number') | ||
| ? config.debug | ||
| : (config.debug === true ? LOG.Trace : LOG.Info); | ||
| const methodLevel = Level[method] ?? 0; |
There was a problem hiding this comment.
Doc mismatch between Logify and tempo.config.md regarding debug: true mapping.
The JSDoc here (Line 117) and the runtime mapping (Line 49) both say debug === true maps to LOG.Trace. However, packages/tempo/doc/tempo.config.md Line 118 states true maps to LOG.Debug. One of the docs is wrong — the implementation is the source of truth, so the Tempo config doc needs updating (flagged in that file).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/library/src/common/logify.class.ts` around lines 47 - 50, The
tempo.config.md doc incorrectly states that debug: true maps to LOG.Debug;
update that doc to match the implementation in Logify where config.debug ===
true maps to LOG.Trace (refer to the Logify class and the config.debug runtime
mapping that uses LOG.Trace), ensuring the doc text and any examples mention
LOG.Trace instead of LOG.Debug so docs and runtime are consistent.
|
|
||
| describe('delegator', () => { | ||
| test('does not allow writes to overwrite delegated values', () => { | ||
| const proxy = delegator(['alpha'] as const, key => `${key}-value`) as Record<'alpha', string>; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm delegator's parameter typing in the source file.
fd -t f 'proxy.library.ts' packages/library/src
rg -nP -C2 '\bexport function delegator\b' --type=ts
# Check tsconfig strictness applied to tests.
fd -t f 'tsconfig*.json' | xargs -I{} sh -c 'echo "=== {} ==="; cat "{}"'Repository: magmacomputing/magma
Length of output: 8326
🏁 Script executed:
cat packages/library/test/common/proxy.library.test.ts | head -60 | tail -20Repository: magmacomputing/magma
Length of output: 688
🏁 Script executed:
cat tsconfig.base.jsonRepository: magmacomputing/magma
Length of output: 10657
🏁 Script executed:
rg -n 'delegator\(\[' packages/library/test/common/proxy.library.test.ts -A 2 -B 2Repository: magmacomputing/magma
Length of output: 319
🏁 Script executed:
# Try to understand the full context around the delegator call
cat packages/library/test/common/proxy.library.test.ts | sed -n '45,65p'Repository: magmacomputing/magma
Length of output: 550
Fix TypeScript type incompatibility: readonly array to mutable parameter
Line 52 passes ['alpha'] as const (a readonly ['alpha']) to the delegator function's first parameter, which is typed as K[] | Record<K, any> (mutable array). Under strict TypeScript settings ("strict": true in tsconfig.base.json), readonly arrays are not assignable to mutable array parameters.
Either widen the parameter to readonly K[] in packages/library/src/common/proxy.library.ts (line 169), or remove as const from the test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/library/test/common/proxy.library.test.ts` at line 52, The test
passes a readonly tuple literal to delegator (proxy test uses
delegator(['alpha'] as const, key => `${key}-value`) cast to Record<'alpha',
string>) which fails under strict types because delegator's first parameter is
typed as K[] | Record<K, any>; update the delegator signature in
proxy.library.ts (the delegator function) to accept readonly K[] (e.g., change
K[] to readonly K[]) or alternatively remove the as const in the test so a
mutable array is passed; pick one fix and update the corresponding declaration
or test accordingly so the readonly array is assignable.
| } else if (baseState) { | ||
| state.config = markConfig(Object.create(baseState.config)); | ||
| Object.defineProperties(state.config, { | ||
| calendar: { value: (state.config as any).calendar, enumerable: true, writable: true, configurable: true }, | ||
| timeZone: { value: (state.config as any).timeZone, enumerable: true, writable: true, configurable: true }, | ||
| locale: { value: (state.config as any).locale, enumerable: true, writable: true, configurable: true }, | ||
| discovery: { value: (state.config as any).discovery, enumerable: true, writable: true, configurable: true }, | ||
| formats: { value: (state.config as any).formats, enumerable: true, writable: true, configurable: true }, | ||
| sphere: { value: (state.config as any).sphere, enumerable: true, writable: true, configurable: true }, | ||
| get: { value: (state.config as any).get, enumerable: false, writable: true, configurable: true }, | ||
| scope: { value: 'local', enumerable: true, writable: true, configurable: true }, | ||
| }); | ||
| setProperty(state.config, 'catch', options.catch); |
There was a problem hiding this comment.
Unconditional catch write in the sandbox branch breaks inheritance from baseState.
In the baseState branch, Line 75 calls setProperty(state.config, 'catch', options.catch) unconditionally. When a sandbox is created without an explicit options.catch, this assigns undefined as an own property and shadows the prototype-inherited value from baseState.config.catch. Contrast with the fallback branch (Lines 88–89), which gates the same call with isDefined(options.catch).
This means Tempo.create() sandboxes silently lose the parent's catch setting even when the caller didn't ask to change it.
🛠️ Proposed fix
- setProperty(state.config, 'catch', options.catch);
+ if (isDefined(options.catch))
+ setProperty(state.config, 'catch', options.catch);🤖 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 63 - 75, In the
baseState sandbox branch, avoid unconditionally calling
setProperty(state.config, 'catch', options.catch) because assigning undefined
will shadow baseState.config.catch; update the else if (baseState) branch to
only call setProperty for 'catch' when options.catch is defined (e.g., wrap with
isDefined(options.catch) or options.catch !== undefined) so the sandbox inherits
the parent's catch handler unless explicitly overridden.
|
prepare Tempo for future features (multiple layout paths) |
1 participant
Sandboxing the Tempo Class to allow for multiple concurrent configurations.
First phase of the 'Layout Ordering' pattern matching.
Summary by CodeRabbit
New Features
Behavior Changes
Improvements