Feature/tempo pattern compiler#27
Merged
magmacomputing merged 8 commits intomainfrom May 7, 2026
Merged
Conversation
…r improved state management and caching
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors Tempo's regex/pattern compilation into a per-state PatternCompiler, wires it into Tempo.regexp, delegates layout-order logic to a pure resolver, migrates many internal imports from ChangesPattern compiler, layout wiring, and Tempo integration
Support layer renames and utility hardening
Layout order extraction and resolver changes
CI, docs, release notes, and minor UI
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
packages/tempo/src/tempo.class.ts (1)
26-29: 💤 Low valueConsolidate the duplicate
./support/tempo.util.jsimports.Lines 26 and 29 both import from
./support/tempo.util.js. Merge into a single statement for readability.♻️ Proposed change
-import { resolveMonthDay } from './support/tempo.util.js'; import { DEFAULT_LAYOUT_CLASS, resolveLayoutOrder, getLayoutOrder } from './parse/parse.layout.js'; import { datePattern } from './support/tempo.default.js'; -import { setProperty, proto, hasOwn, normalizeLayoutOrder } from './support/tempo.util.js'; +import { resolveMonthDay, setProperty, proto, hasOwn, normalizeLayoutOrder } from './support/tempo.util.js';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/src/tempo.class.ts` around lines 26 - 29, There are two imports from the same module './support/tempo.util.js' (resolveMonthDay on one line and setProperty, proto, hasOwn, normalizeLayoutOrder on the other); consolidate them into a single import statement that imports resolveMonthDay, setProperty, proto, hasOwn, and normalizeLayoutOrder together so only one import from './support/tempo.util.js' remains (keep other imports like DEFAULT_LAYOUT_CLASS, resolveLayoutOrder, getLayoutOrder, and datePattern unchanged).packages/tempo/src/tempo.type.ts (1)
19-19: ⚡ Quick winUse
import typeto make the cycle erased at runtime.
engine.pattern.tsalready imports types fromtempo.type.ts(import type * as t from '../tempo.type.js'), so importing the runtimePatternCompilervalue here closes the cycle at the value level. SincePatternCompileris only used as a type annotation on line 229, switching toimport typekeeps the cycle purely at the type layer and avoids any module-evaluation order surprises.♻️ Proposed change
-import { PatternCompiler } from './engine/engine.pattern.js'; +import type { PatternCompiler } from './engine/engine.pattern.js';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/src/tempo.type.ts` at line 19, The import of PatternCompiler in tempo.type.ts creates a runtime cycle with engine.pattern.ts; change the statement that currently imports PatternCompiler to a type-only import (use "import type { PatternCompiler } from './engine/engine.pattern.js'") so the symbol is erased at runtime—ensure PatternCompiler is only used as a type (e.g., in the type annotation where it is referenced) and not relied on as a runtime value anywhere in tempo.type.ts.doc/main_branch_protection.md (1)
38-40: 💤 Low valueNote the side-effect of
core.hooksPath.Setting
core.hooksPathglobally disables every per-repo.git/hooks/*for all repositories on the workstation (Husky, lefthook, lint-staged, etc.). Worth calling out so users who already rely on per-project hooks don't lose them silently. Consider mentioning that they should chain those tools from the global hook scripts, or scope this protection to specific repos viagit config core.hooksPath(without--global).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@doc/main_branch_protection.md` around lines 38 - 40, The doc currently suggests running `git config --global core.hooksPath ...` without warning that setting core.hooksPath globally disables all per-repo .git/hooks/* (breaking Husky, lefthook, lint-staged, etc.); update the guidance in main_branch_protection.md to call out this side-effect explicitly, and add recommended alternatives: either chain existing per-project hooks from the global hook scripts or scope the setting to individual repos using `git config core.hooksPath` (no --global) so you don’t silently disable per-repo hooks; reference the config key `core.hooksPath` and the global flag `--global` in the text so readers know exactly what to change.packages/tempo/src/engine/engine.pattern.ts (1)
38-44: 💤 Low valueSilent recursion bail-out hides misconfigured placeholders.
When
d > 10the matcher just returnssrcunchanged, so a circular snippet definition (e.g.,{a}→{b}→{a}) yields a regex with literal{...}braces in it and no diagnostic. Worth alogError(or at least a debug log) so cyclic snippet authors notice.📝 Suggested change
- const matcher = (src: string, d = 0): string => { - if (d > 10) return src; // prevent infinite recursion + const matcher = (src: string, d = 0): string => { + if (d > 10) { + logError(this.#state.config, `PatternCompiler: recursion limit hit while expanding "${src}"`); + return src; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/src/engine/engine.pattern.ts` around lines 38 - 44, The matcher function silently returns the original src on depth overflow (d > 10), hiding circular placeholder definitions; update matcher to detect this recursion limit and emit a diagnostic (e.g., call an existing logger like logError/log.warn or the local logger instance) when d > 10 before returning, including the problematic src and depth info to aid debugging (keep the existing return behavior but add the log call inside the matcher).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@doc/main_branch_protection.md`:
- Around line 28-33: The doc snippet hard-codes a personal absolute path
(/home/michael/Project/magma/.git/hooks/pre-commit) which will break for others;
update the example to use a generic placeholder or a repo-root lookup (e.g.
$(git rev-parse --show-toplevel)/.git/hooks/pre-commit) and show copying the
pre-commit and pre-push hooks into ~/.git-hooks, keeping the mkdir -p
~/.git-hooks and chmod +x ~/.git-hooks/* lines but replacing the absolute paths
with a placeholder like /path/to/repo/.git/hooks or the git command so
contributors can run the snippet on any machine.
In `@packages/tempo/plan/RELEASE-D.md`:
- Around line 17-21: The RELEASE-D.md checklist is inaccurate: the PR actually
includes the new PatternCompiler and cache extraction (see engine.pattern.ts and
tempo.class.ts) but the checklist only marks "Alias Resolution Engine
Extraction" items; update the checklist lines 9-14 to mark the "Pattern Compiler
+ Cache Extraction" items as completed (or split them into a separate PR entry
if you intend to keep them separate) and ensure the item text references the
PatternCompiler/Cache extraction so the release tracking matches the code
change.
In `@packages/tempo/src/engine/engine.pattern.ts`:
- Around line 29-36: The cache key in compileRegExp currently collapses all
non-null snippets into the same key (`cacheKey = \`${source}:${snippet ?
'custom' : 'global'}\``), causing different Snippet objects to collide and
return incorrect RegExp; fix by scoping the cache per Snippet identity: replace
or augment the current `#cache` with a WeakMap<Snippet, Map<string, RegExp>> (or
compute a stable digest of snippet entries) so compileRegExp uses
snippet-specific storage when snippet is provided (look for compileRegExp,
cacheKey, and this.#cache) and ensure lookups/sets use the inner Map keyed by
source to preserve caching for repeated calls with the same snippet object.
- Around line 76-86: The catch block should use the previously computed source
(not the original layout) when building the fallback regex, surface the original
error via logError, and avoid poisoning the cache with a fallback; specifically,
in the try/catch around matcher/expanded/compiled replace Match.escape(layout as
string) with Match.escape(source), call logError(e, { context: 'pattern compile
failed', pattern: source }) (or equivalent) before returning, and do not call
this.#cache.set(cacheKey, fallback) for the fallback (either skip caching the
fallback or mark it so setPatterns can invalidate it later) so transient compile
failures don’t persist; refer to matcher, expanded, compiled, fallback,
this.#cache and setPatterns when making the changes.
- Around line 102-109: The code builds snippet[Token.afx] using
snippet[Token.sep].source without checking existence; update the construction in
the block where enums?.NUMBER is handled so it guards snippet[Token.sep] (e.g.,
use optional chaining or a safe fallback like (snippet[Token.sep]?.source || '')
when composing the RegExp) to avoid accessing .source on undefined—locate the
snippet[Token.afx] assignment and replace the direct snippet[Token.sep].source
reference with a guarded expression.
In `@packages/tempo/src/tempo.class.ts`:
- Around line 889-896: The cache key in engine.pattern.ts conflates different
custom Snippet objects because it uses `${source}:${snippet ? 'custom' :
'global'}`; update caching in PatternCompiler.compileRegExp (or the cache
declaration) to differentiate by the actual snippet identity instead of just
presence—e.g., use a secondary map/WeakMap keyed by the snippet object (or
include a stable snippet identifier like snippet.name/snippet.id/snippet.source
in the key) so calls to PatternCompiler.compileRegExp(layout, snippet) return
distinct compiled patterns per snippet; alternatively, disable caching when a
custom snippet is provided. Ensure changes reference PatternCompiler and
compileRegExp to locate the code.
---
Nitpick comments:
In `@doc/main_branch_protection.md`:
- Around line 38-40: The doc currently suggests running `git config --global
core.hooksPath ...` without warning that setting core.hooksPath globally
disables all per-repo .git/hooks/* (breaking Husky, lefthook, lint-staged,
etc.); update the guidance in main_branch_protection.md to call out this
side-effect explicitly, and add recommended alternatives: either chain existing
per-project hooks from the global hook scripts or scope the setting to
individual repos using `git config core.hooksPath` (no --global) so you don’t
silently disable per-repo hooks; reference the config key `core.hooksPath` and
the global flag `--global` in the text so readers know exactly what to change.
In `@packages/tempo/src/engine/engine.pattern.ts`:
- Around line 38-44: The matcher function silently returns the original src on
depth overflow (d > 10), hiding circular placeholder definitions; update matcher
to detect this recursion limit and emit a diagnostic (e.g., call an existing
logger like logError/log.warn or the local logger instance) when d > 10 before
returning, including the problematic src and depth info to aid debugging (keep
the existing return behavior but add the log call inside the matcher).
In `@packages/tempo/src/tempo.class.ts`:
- Around line 26-29: There are two imports from the same module
'./support/tempo.util.js' (resolveMonthDay on one line and setProperty, proto,
hasOwn, normalizeLayoutOrder on the other); consolidate them into a single
import statement that imports resolveMonthDay, setProperty, proto, hasOwn, and
normalizeLayoutOrder together so only one import from './support/tempo.util.js'
remains (keep other imports like DEFAULT_LAYOUT_CLASS, resolveLayoutOrder,
getLayoutOrder, and datePattern unchanged).
In `@packages/tempo/src/tempo.type.ts`:
- Line 19: The import of PatternCompiler in tempo.type.ts creates a runtime
cycle with engine.pattern.ts; change the statement that currently imports
PatternCompiler to a type-only import (use "import type { PatternCompiler } from
'./engine/engine.pattern.js'") so the symbol is erased at runtime—ensure
PatternCompiler is only used as a type (e.g., in the type annotation where it is
referenced) and not relied on as a runtime value anywhere in tempo.type.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: 47da4174-1297-4b40-8459-5158bbde04b6
📒 Files selected for processing (7)
doc/main_branch_protection.mdpackages/tempo/plan/RELEASE-D.mdpackages/tempo/src/engine/engine.pattern.tspackages/tempo/src/support/support.index.tspackages/tempo/src/support/tempo.util.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.type.ts
💤 Files with no reviewable changes (1)
- packages/tempo/src/support/tempo.util.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@doc/main_branch_protection.md`:
- Around line 137-138: The doc currently instructs users to run "git reset
--hard origin/main" but omits a prior fetch; update the guidance around the git
reset step to first run a fetch so origin/main is up-to-date (i.e., add an
instruction to run git fetch origin before the git reset --hard origin/main
command) and mention that this prevents resetting to a stale origin/main
reference.
In `@packages/tempo/src/engine/engine.pattern.ts`:
- Around line 92-96: In the catch block in engine.pattern.ts (the RegExp compile
fallback in the function surrounding Match.escape), the call to logError uses
reversed arguments and optional chaining; change it to call logError with the
config object first and the error as a message (e.g. logError({ context:
'pattern compile failed', pattern: source }, e)) and remove the unnecessary ?.
so the compile error is logged correctly rather than being treated as the
config.
🪄 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: cbc288fd-36a3-417d-8bff-2e69c16da553
📒 Files selected for processing (11)
.agent/workflows/interactive-testing.md.github/workflows/ci.ymldoc/main_branch_protection.mdpackages/tempo/index.mdpackages/tempo/plan/RELEASE-D.mdpackages/tempo/src/engine/engine.alias.tspackages/tempo/src/engine/engine.pattern.tspackages/tempo/src/support/tempo.init.tspackages/tempo/src/support/tempo.util.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.type.ts
💤 Files with no reviewable changes (1)
- .github/workflows/ci.yml
✅ Files skipped from review due to trivial changes (1)
- packages/tempo/index.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/tempo/plan/RELEASE-D.md
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/tempo/src/engine/engine.pattern.ts (1)
92-96:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
logErroris still called with the wrong first argument.The past review flagged that
logError(config, ...msg)expectsthis.#state.configas its first argument. The?.was removed and args were swapped, but the first argument is still a plain object literal{ context: ..., pattern: source }rather than the config instance — so the error is still silently swallowed.🐛 Proposed fix
- logError({ context: 'pattern compile failed', pattern: source }, e); + logError(this.#state.config, `PatternCompiler: failed to compile layout "${source}"`, e);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/src/engine/engine.pattern.ts` around lines 92 - 96, The logError call in the catch block is passing an object literal as the first argument instead of the config instance, so the error is still swallowed; change the call to pass this.#state.config as the first parameter and then supply the contextual info and the caught error as subsequent args (i.e. call logError(this.#state.config, { context: 'pattern compile failed', pattern: source }, e)) in the catch inside the pattern compilation (references: logError, this.#state.config, Match.escape, RegExp).
🧹 Nitpick comments (2)
doc/main_branch_protection.md (1)
6-8: ⚡ Quick winUse neutral wording for shared documentation.
“I have already set up local hooks…” reads like a personal note and may mislead contributors into thinking hooks are preinstalled for them. Prefer repo-neutral wording.
Suggested patch
-I have already set up local hooks for the `magma` repository: +This repository uses local hooks for the `magma` repository:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@doc/main_branch_protection.md` around lines 6 - 8, The sentence "I have already set up local hooks..." reads as a personal note and should be made neutral for shared documentation; edit the document text that lists the hooks (the lines mentioning "pre-commit" and "pre-push") to use repository-neutral wording such as "Local hooks can be configured to:" or "Set up local hooks to:" and rephrase the two bullets to describe the behavior ("pre-commit: prevents direct commits to main", "pre-push: prevents pushing to remote main") without implying they are already installed for the reader.packages/tempo/src/engine/engine.pattern.ts (1)
57-57: ⚡ Quick win
new RegExp(Match.braces, 'g')is reconstructed on every recursivematchercall.
Match.bracesis a static constant, so thisRegExpcan be hoisted to a module-level (or class-level) constant and reused across all invocations, avoiding repeated object allocation on hot parse paths.♻️ Proposed refactor
+const BRACES_RE = new RegExp(Match.braces, 'g'); + export class PatternCompiler { ... compileRegExp(...) { ... const matcher = (source: string, d = 0): string => { ... - return source.replace(new RegExp(Match.braces, 'g'), (match, name) => { + return source.replace(BRACES_RE, (match, name) => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/src/engine/engine.pattern.ts` at line 57, The code recreates new RegExp(Match.braces, 'g') on every recursive matcher invocation; hoist this RegExp into a module- or class-level constant (e.g., BRACES_REGEX) and replace occurrences of new RegExp(Match.braces, 'g') inside the matcher function (the source.replace call that uses Match.braces) with that constant so the same RegExp instance is reused across all invocations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@doc/main_branch_protection.md`:
- Around line 143-144: The numbered step heading "# 3. Switch to your new branch
to continue working" is off-by-one in the recovery block; update that heading to
the correct sequence number to match the surrounding steps (e.g., change "# 3.
Switch to your new branch to continue working" to "# 4. Switch to your new
branch to continue working") so the instructions and the command "git checkout
feature/my-feature" align with the step ordering.
---
Duplicate comments:
In `@packages/tempo/src/engine/engine.pattern.ts`:
- Around line 92-96: The logError call in the catch block is passing an object
literal as the first argument instead of the config instance, so the error is
still swallowed; change the call to pass this.#state.config as the first
parameter and then supply the contextual info and the caught error as subsequent
args (i.e. call logError(this.#state.config, { context: 'pattern compile
failed', pattern: source }, e)) in the catch inside the pattern compilation
(references: logError, this.#state.config, Match.escape, RegExp).
---
Nitpick comments:
In `@doc/main_branch_protection.md`:
- Around line 6-8: The sentence "I have already set up local hooks..." reads as
a personal note and should be made neutral for shared documentation; edit the
document text that lists the hooks (the lines mentioning "pre-commit" and
"pre-push") to use repository-neutral wording such as "Local hooks can be
configured to:" or "Set up local hooks to:" and rephrase the two bullets to
describe the behavior ("pre-commit: prevents direct commits to main", "pre-push:
prevents pushing to remote main") without implying they are already installed
for the reader.
In `@packages/tempo/src/engine/engine.pattern.ts`:
- Line 57: The code recreates new RegExp(Match.braces, 'g') on every recursive
matcher invocation; hoist this RegExp into a module- or class-level constant
(e.g., BRACES_REGEX) and replace occurrences of new RegExp(Match.braces, 'g')
inside the matcher function (the source.replace call that uses Match.braces)
with that constant so the same RegExp instance is reused across all invocations.
🪄 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: fdd35921-ffc8-4f54-9a35-c085993bda7a
📒 Files selected for processing (2)
doc/main_branch_protection.mdpackages/tempo/src/engine/engine.pattern.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/tempo/src/parse/parse.layout.ts (1)
81-86:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHoist
ownEntries(ordered)and guard against length mismatch.Same root cause as in
engine.layout.ts:ownEntries(ordered)is recomputed inside.some, and(ownEntries(ordered)[idx] as LayoutEntry)[0]will throw iflayouts.lengthexceedsordered's entry count.resolveLayoutClassificationOrder(Line 70) already gates this with a length comparison.♻️ Proposed fix
- const layouts = resolveLayoutOrderPure(ordered, monthDayLayouts, isMonthDay); - const changed = layouts.some((entry, idx) => entry[0] !== (ownEntries(ordered)[idx] as LayoutEntry)[0]); - - if (changed) return Object.fromEntries(layouts) as Record<symbol, string>; - return ordered; + const layouts = resolveLayoutOrderPure(ordered, monthDayLayouts, isMonthDay); + const orderedEntries = ownEntries(ordered) as LayoutEntry[]; + const changed = layouts.length === orderedEntries.length + && layouts.some((entry, idx) => entry[0] !== orderedEntries[idx][0]); + + return changed ? Object.fromEntries(layouts) as Record<symbol, string> : ordered;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/src/parse/parse.layout.ts` around lines 81 - 86, Hoist the result of ownEntries(ordered) into a local variable before computing changed and guard against length mismatch: compute const own = ownEntries(ordered) once, then set changed based on either layouts.length !== own.length or layouts.some((entry, idx) => entry[0] !== (own[idx] as LayoutEntry)[0]); update the code around resolveLayoutOrderPure/ownEntries/layouts to use this guard so you don't recompute ownEntries(ordered) inside .some or access out-of-range indexes.packages/tempo/src/engine/engine.layout.ts (1)
98-103:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHoist
ownEntries(ordered)and guard against length mismatch.
ownEntries(ordered)is re-evaluated on every iteration of.some, making the change-detection O(n²) and allocating a fresh entries array per element. Also, ifresolveLayoutOrderPureever returns more elements thanorderedhas own entries,(ownEntries(ordered)[idx] as LayoutEntry)[0]will throw onundefined[0]. The companion helperresolveLayoutClassificationOrder(Line 78) already guards with a length check first; please mirror that here.♻️ Proposed fix
- const layouts = resolveLayoutOrderPure(ordered, monthDayLayouts, isMonthDay); - const changed = layouts.some((entry, idx) => entry[0] !== (ownEntries(ordered)[idx] as LayoutEntry)[0]); - - if (changed) return Object.fromEntries(layouts) as Record<symbol, string>; - return ordered; + const layouts = resolveLayoutOrderPure(ordered, monthDayLayouts, isMonthDay); + const orderedEntries = ownEntries(ordered) as LayoutEntry[]; + const changed = layouts.length === orderedEntries.length + && layouts.some((entry, idx) => entry[0] !== orderedEntries[idx][0]); + + return changed ? Object.fromEntries(layouts) as Record<symbol, string> : ordered;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/src/engine/engine.layout.ts` around lines 98 - 103, Hoist ownEntries(ordered) into a single const (e.g., const entries = ownEntries(ordered)) before calling resolveLayoutOrderPure, guard that layouts.length === entries.length and if they differ return ordered immediately, then compute changed using entries (e.g., layouts.some((entry, idx) => entry[0] !== (entries[idx] as LayoutEntry)[0])); update the return path to still use Object.fromEntries(layouts) when changed; reference resolveLayoutOrderPure, ownEntries, layouts, changed, and mirror the length-first check used in resolveLayoutClassificationOrder.
🧹 Nitpick comments (3)
packages/tempo/test/engine/pattern_compiler_optimization.test.ts (1)
6-44: ⚡ Quick winTests share mutable global Tempo state; ordering matters.
Each test obtains the same internal
statevia(Tempo as any)[$Internal]()and writes tostate.parse.snippet[getSymbol(...)], so mutations from earlier tests leak into later ones. Today this passes only because the symbol keys don't conflict in unintended ways (e.g.cset in test 1 is never overwritten,xis unique). Adding/reordering tests can produce confusing failures.Consider snapshotting and restoring the affected snippet entries in
beforeEach/afterEach, or usingTempoRuntime.createScoped()so each test gets a clean compiler input.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/test/engine/pattern_compiler_optimization.test.ts` around lines 6 - 44, Tests mutate shared Tempo internal state via (Tempo as any)[$Internal]() and writing to state.parse.snippet[getSymbol(...)], causing ordering-dependent leaks; fix by isolating state per test—either create a fresh scoped runtime with TempoRuntime.createScoped() and use its internal state when constructing PatternCompiler, or add beforeEach/afterEach that snapshots and restores (or clears) state.parse.snippet entries touched by the tests (refer to Tempo, $Internal, PatternCompiler, state.parse.snippet, getSymbol, and TempoRuntime.createScoped to locate the code to change).packages/tempo/src/engine/engine.pattern.ts (1)
50-50: 💤 Low valueDrop spurious optional chaining.
logWarnis a static import and#stateis set in the constructor — neither can be nullish at this call site. The?.on both makes the intent ambiguous (suggests they might be undefined). Same pattern is used correctly without?.at line 96 (logError(this.#state.config, …)).♻️ Proposed change
- logWarn?.(this.#state?.config, `[PatternCompiler] Recursion limit exceeded in matcher (d > 10) for src:`, source, `depth:`, d); + logWarn(this.#state.config, `[PatternCompiler] Recursion limit exceeded in matcher (d > 10) for src:`, source, `depth:`, d);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/src/engine/engine.pattern.ts` at line 50, The log call uses spurious optional chaining on both logWarn and this.#state which are guaranteed to be defined; remove the `?.` so the call becomes logWarn(this.#state.config, ...) to match the pattern used by logError(this.#state.config, ...) and avoid implying these values may be undefined; update the call in the PatternCompiler/matcher recursion branch accordingly.packages/tempo/src/parse/parse.layout.ts (1)
7-72: ⚖️ Poor tradeoffLayout-ordering logic is duplicated across
parse.layout.tsandengine.layout.tsand is starting to diverge.
createLayoutController,resolveLayoutClassificationOrder,resolveLayoutOrder,getLayoutOrder, theTOKEN_ALIAS/TOKEN_DESCRIPTION_BY_NAMEmaps, andDEFAULT_LAYOUT_CLASSare near-identical between the two files. They have already drifted in two ways:
LayoutControlleris typed asRecord<PropertyKey, (string | symbol)[]>here vsRecord<PropertyKey, string[]>inengine.layout.ts.resolveLayoutClassificationOrderhere additionally branches ontypeof name === 'symbol'(Lines 51-58); the engine copy does not.Future fixes will need to be applied in two places and are easy to forget. Consider keeping a single canonical implementation (e.g., in
engine.layout.tswith the broader(string | symbol)[]signature) and re-exporting fromparse.layout.ts, or extracting both into a sharedsupportmodule.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/src/parse/parse.layout.ts` around lines 7 - 72, Duplicate layout-ordering code between parse.layout.ts and engine.layout.ts should be consolidated; extract createLayoutController, resolveLayoutClassificationOrder, resolveLayoutOrder, getLayoutOrder, TOKEN_ALIAS, TOKEN_DESCRIPTION_BY_NAME, and DEFAULT_LAYOUT_CLASS into a single shared module (or keep the canonical implementation in engine.layout.ts) and re-export from parse.layout.ts. Ensure the canonical signature uses LayoutController = Record<PropertyKey, (string | symbol)[]> and retain the symbol-handling branch (the typeof name === 'symbol' logic) inside resolveLayoutClassificationOrder so behavior stays consistent; update imports in both files to use the shared exports and remove the duplicated implementations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/tempo/doc/tempo.layout.md`:
- Around line 88-94: The example output for Tempo.regexp is inaccurate:
compileRegExp (in engine.pattern.ts) wraps the whole expansion in an outer
capturing group and the default {sep} snippet is a named capture, so the doc
should either show the real produced pattern or be marked illustrative; update
the example to match actual output from Tempo.regexp (showing the leading ^( ...
)$ with the outer group and that {sep} expands to a named group like
(?<sep>...)), or add a clear “illustrative only” note. Reference: Tempo.regexp
and compileRegExp in engine.pattern.ts.
In `@packages/tempo/src/support/support.util.ts`:
- Around line 64-72: The create<T> function currently assumes proto(obj) is an
object and directly indexes into it, which will throw if proto(obj) is null
(e.g., Object.create(null)); first assign proto(obj) to a variable (e.g., const
prototype = proto(obj)), check that prototype is an object with
isObject(prototype) before accessing prototype[name], and if that check fails
call logError(...) and return {} as T; then proceed to validate entry with
isObject(entry) as before. This ensures create, proto, isObject and logError are
used to guard the prototype lookup and avoid TypeError.
---
Outside diff comments:
In `@packages/tempo/src/engine/engine.layout.ts`:
- Around line 98-103: Hoist ownEntries(ordered) into a single const (e.g., const
entries = ownEntries(ordered)) before calling resolveLayoutOrderPure, guard that
layouts.length === entries.length and if they differ return ordered immediately,
then compute changed using entries (e.g., layouts.some((entry, idx) => entry[0]
!== (entries[idx] as LayoutEntry)[0])); update the return path to still use
Object.fromEntries(layouts) when changed; reference resolveLayoutOrderPure,
ownEntries, layouts, changed, and mirror the length-first check used in
resolveLayoutClassificationOrder.
In `@packages/tempo/src/parse/parse.layout.ts`:
- Around line 81-86: Hoist the result of ownEntries(ordered) into a local
variable before computing changed and guard against length mismatch: compute
const own = ownEntries(ordered) once, then set changed based on either
layouts.length !== own.length or layouts.some((entry, idx) => entry[0] !==
(own[idx] as LayoutEntry)[0]); update the code around
resolveLayoutOrderPure/ownEntries/layouts to use this guard so you don't
recompute ownEntries(ordered) inside .some or access out-of-range indexes.
---
Nitpick comments:
In `@packages/tempo/src/engine/engine.pattern.ts`:
- Line 50: The log call uses spurious optional chaining on both logWarn and
this.#state which are guaranteed to be defined; remove the `?.` so the call
becomes logWarn(this.#state.config, ...) to match the pattern used by
logError(this.#state.config, ...) and avoid implying these values may be
undefined; update the call in the PatternCompiler/matcher recursion branch
accordingly.
In `@packages/tempo/src/parse/parse.layout.ts`:
- Around line 7-72: Duplicate layout-ordering code between parse.layout.ts and
engine.layout.ts should be consolidated; extract createLayoutController,
resolveLayoutClassificationOrder, resolveLayoutOrder, getLayoutOrder,
TOKEN_ALIAS, TOKEN_DESCRIPTION_BY_NAME, and DEFAULT_LAYOUT_CLASS into a single
shared module (or keep the canonical implementation in engine.layout.ts) and
re-export from parse.layout.ts. Ensure the canonical signature uses
LayoutController = Record<PropertyKey, (string | symbol)[]> and retain the
symbol-handling branch (the typeof name === 'symbol' logic) inside
resolveLayoutClassificationOrder so behavior stays consistent; update imports in
both files to use the shared exports and remove the duplicated implementations.
In `@packages/tempo/test/engine/pattern_compiler_optimization.test.ts`:
- Around line 6-44: Tests mutate shared Tempo internal state via (Tempo as
any)[$Internal]() and writing to state.parse.snippet[getSymbol(...)], causing
ordering-dependent leaks; fix by isolating state per test—either create a fresh
scoped runtime with TempoRuntime.createScoped() and use its internal state when
constructing PatternCompiler, or add beforeEach/afterEach that snapshots and
restores (or clears) state.parse.snippet entries touched by the tests (refer to
Tempo, $Internal, PatternCompiler, state.parse.snippet, getSymbol, and
TempoRuntime.createScoped to locate the code to change).
🪄 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: aa42f25e-5951-4506-bd6a-8d1fd90786f3
📒 Files selected for processing (26)
doc/main_branch_protection.mdpackages/tempo/doc/tempo.layout.mdpackages/tempo/package.jsonpackages/tempo/src/discrete/discrete.parse.tspackages/tempo/src/engine/engine.layout.tspackages/tempo/src/engine/engine.pattern.tspackages/tempo/src/engine/engine.resolver.tspackages/tempo/src/parse/parse.layout.tspackages/tempo/src/parse/parse.resolver.tspackages/tempo/src/plugin/plugin.util.tspackages/tempo/src/plugin/term/term.quarter.tspackages/tempo/src/plugin/term/term.season.tspackages/tempo/src/support/support.default.tspackages/tempo/src/support/support.enum.tspackages/tempo/src/support/support.index.tspackages/tempo/src/support/support.init.tspackages/tempo/src/support/support.intl.tspackages/tempo/src/support/support.register.tspackages/tempo/src/support/support.runtime.tspackages/tempo/src/support/support.symbol.tspackages/tempo/src/support/support.util.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.type.tspackages/tempo/test/engine/parse.layout.test.tspackages/tempo/test/engine/pattern_compiler_optimization.test.tspackages/tempo/test/plugins/plugin_registration.test.ts
💤 Files with no reviewable changes (1)
- packages/tempo/src/parse/parse.resolver.ts
✅ Files skipped from review due to trivial changes (8)
- packages/tempo/test/engine/parse.layout.test.ts
- packages/tempo/test/plugins/plugin_registration.test.ts
- packages/tempo/src/discrete/discrete.parse.ts
- packages/tempo/src/plugin/term/term.quarter.ts
- packages/tempo/src/plugin/plugin.util.ts
- packages/tempo/src/support/support.enum.ts
- packages/tempo/src/plugin/term/term.season.ts
- packages/tempo/src/support/support.default.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/tempo/src/tempo.type.ts
- packages/tempo/src/tempo.class.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/tempo/src/engine/engine.layout.ts (1)
54-60:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSymbol preferences should match exact symbol identity first
On Line 71-73, symbol resolution only checks description/alias. A preferred symbol that exists in
layoutbut lacks both won’t match, so classification is silently ignored for that token.Proposed fix
const entries = ownEntries(layout) as LayoutEntry[]; const byName = new Map<string, LayoutEntry>(); + const bySymbol = new Map<symbol, LayoutEntry>(); entries.forEach(([key, value]) => { + bySymbol.set(key, [key, value]); const description = key.description ?? ''; if (description) byName.set(description, [key, value]); const alias = TOKEN_ALIAS.get(key); if (alias) byName.set(alias, [key, value]); }); @@ const resolvedName = !isSym ? (TOKEN_DESCRIPTION_BY_NAME.get(name) ?? name) : undefined; const entry = isSym - ? (byName.get(description) ?? (alias ? byName.get(alias) : undefined)) + ? (bySymbol.get(name) ?? byName.get(description) ?? (alias ? byName.get(alias) : undefined)) : (byName.get(resolvedName!) ?? byName.get(name));Also applies to: 71-73
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/src/engine/engine.layout.ts` around lines 54 - 60, The classification misses tokens whose preferred symbol matches the actual symbol identity but lacks description/alias; when building the lookup in the block that uses ownEntries(layout) and populates byName (and reads TOKEN_ALIAS), also add an identity-based mapping keyed by the symbol itself (i.e., store the LayoutEntry under the symbol key) and/or check symbol identity first when resolving preferred symbols (compare preferredSymbol === key or consult a symbol->LayoutEntry map) before falling back to description or alias lookups.
🧹 Nitpick comments (1)
packages/tempo/src/engine/engine.pattern.ts (1)
150-167: 💤 Low value
hasOwncheck covers cross-state pollution; consider value-presence check too.
hasOwn(state, 'patternCompiler')only checks property existence, not value. If anything ever assignsstate.patternCompiler = undefined(e.g. during a registry reset), the non-null assertion on line 166 (state.patternCompiler!) will throw. A defensive value check is cheap and avoids the surprise:🛡️ Suggested guard
- if (!hasOwn(state, 'patternCompiler')) - state.patternCompiler = new PatternCompiler({ state }); - - state.patternCompiler!.setPatterns(); + if (!state.patternCompiler) + state.patternCompiler = new PatternCompiler({ state }); + + state.patternCompiler.setPatterns();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/src/engine/engine.pattern.ts` around lines 150 - 167, The current guard uses hasOwn(state, 'patternCompiler') which only checks property existence and can still leave state.patternCompiler undefined; update setPatterns to verify the value (e.g., ensure state.patternCompiler is truthy or an instance of PatternCompiler) before calling setPatterns(), so replace the hasOwn-only check with a defensive value check (e.g., if (!state.patternCompiler) state.patternCompiler = new PatternCompiler({ state });) and then call state.patternCompiler.setPatterns(); this ensures PatternCompiler and its setPatterns method are safe to invoke.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/tempo/src/engine/engine.layout.ts`:
- Around line 54-60: The classification misses tokens whose preferred symbol
matches the actual symbol identity but lacks description/alias; when building
the lookup in the block that uses ownEntries(layout) and populates byName (and
reads TOKEN_ALIAS), also add an identity-based mapping keyed by the symbol
itself (i.e., store the LayoutEntry under the symbol key) and/or check symbol
identity first when resolving preferred symbols (compare preferredSymbol === key
or consult a symbol->LayoutEntry map) before falling back to description or
alias lookups.
---
Nitpick comments:
In `@packages/tempo/src/engine/engine.pattern.ts`:
- Around line 150-167: The current guard uses hasOwn(state, 'patternCompiler')
which only checks property existence and can still leave state.patternCompiler
undefined; update setPatterns to verify the value (e.g., ensure
state.patternCompiler is truthy or an instance of PatternCompiler) before
calling setPatterns(), so replace the hasOwn-only check with a defensive value
check (e.g., if (!state.patternCompiler) state.patternCompiler = new
PatternCompiler({ state });) and then call state.patternCompiler.setPatterns();
this ensures PatternCompiler and its setPatterns method are safe to invoke.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c7a5496a-9a9a-47e2-88e2-01255138606c
📒 Files selected for processing (11)
packages/tempo/doc/tempo.layout.mdpackages/tempo/package.jsonpackages/tempo/src/engine/engine.layout.tspackages/tempo/src/engine/engine.pattern.tspackages/tempo/src/parse/parse.layout.tspackages/tempo/src/support/support.util.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tsconfig.jsonpackages/tempo/test/engine/parse.layout.test.tspackages/tempo/test/engine/pattern_compiler_optimization.test.tspackages/tempo/vitest.config.ts
💤 Files with no reviewable changes (3)
- packages/tempo/src/tsconfig.json
- packages/tempo/vitest.config.ts
- packages/tempo/src/parse/parse.layout.ts
✅ Files skipped from review due to trivial changes (2)
- packages/tempo/test/engine/parse.layout.test.ts
- packages/tempo/test/engine/pattern_compiler_optimization.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/tempo/src/tempo.class.ts
- packages/tempo/package.json
- packages/tempo/src/support/support.util.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
new PatternCompiler
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Tests