tempo.config.ts pattern#48
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughTempo v3.3.0 extends the v3.2.3 bootstrap infrastructure with localized relative-date modifier keywords via ChangesTempo v3.3.0: Localized Modifiers and Unified Bootstrap
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant CLI as tempo CLI
participant FS as Filesystem
participant App as Application
participant Tempo as Tempo
participant Resolver as resolveConfig
rect rgba(200, 150, 255, 0.5)
note over Dev,CLI: Scaffolding Phase
Dev->>CLI: npx tempo scaffold:all
CLI->>FS: check tempo.config.ts & index.html exist
alt Neither exists
CLI->>FS: copy templates
CLI-->>Dev: Success messages
else Either exists
CLI-->>Dev: Error (already scaffolded)
end
end
rect rgba(150, 200, 255, 0.5)
note over App,Resolver: Bootstrap Phase
Dev->>App: edit config, import code
App->>Tempo: await Tempo.bootstrap({ cwd? })
Tempo->>Resolver: dynamically import and call
Resolver->>FS: traverse upward for tempo.config.*
FS-->>Resolver: config object or undefined
Resolver-->>Tempo: Options
Tempo->>Tempo: init(config || defaults)
Tempo->>Tempo: await ready()
Tempo-->>App: initialized Tempo
end
rect rgba(200, 255, 150, 0.5)
note over App,Tempo: Localized Parsing (v3.3.0)
App->>Tempo: new Tempo('demain prochain')
Tempo->>Tempo: parseModifier(mod, config)
Tempo->>Tempo: normalizeModifier via registry
Tempo->>Tempo: resolve to symbol (+/-)
Tempo-->>App: formatted future date
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
packages/tempo/template/index.sample.html (1)
49-55: ⚡ Quick winReplace
innerHTMLinterpolation with safe DOM/text updates.Line 49-Line 55 introduces XSS sink patterns in a scaffold template. Even if current values are expected-safe, this teaches unsafe defaults and is easy to harden.
Suggested patch
- output.innerHTML = ` - <strong>Success!</strong> Tempo is initialized.<br><br> - Current Local Time: <span style="color: blue;">${formatted}</span> - `; + output.replaceChildren(); + const strong = document.createElement('strong'); + strong.textContent = 'Success!'; + const msg = document.createTextNode(' Tempo is initialized.'); + const br1 = document.createElement('br'); + const br2 = document.createElement('br'); + const label = document.createTextNode('Current Local Time: '); + const time = document.createElement('span'); + time.style.color = 'blue'; + time.textContent = formatted; + output.append(strong, msg, br1, br2, label, time); } catch (err) { - output.innerHTML = `<strong style="color: red;">Error initializing Tempo:</strong> ${err.message}`; + output.replaceChildren(); + const strong = document.createElement('strong'); + strong.style.color = 'red'; + strong.textContent = 'Error initializing Tempo:'; + const message = document.createTextNode(` ${err instanceof Error ? err.message : String(err)}`); + output.append(strong, message); console.error(err); }🤖 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/template/index.sample.html` around lines 49 - 55, Replace the unsafe innerHTML assignments with safe DOM manipulation methods. In both the success message block (lines 49-52) and the error message block (line 55), avoid using innerHTML with string interpolation. Instead, use document.createElement to build the HTML structure, use textContent to set plain text content (such as the formatted time and err.message), and use appendChild to add elements to the output element. This eliminates XSS vulnerabilities and establishes secure patterns in the template.Source: Linters/SAST tools
packages/tempo/src/config/resolveConfig.ts (1)
29-39: ⚡ Quick winConsider using async file operations for consistency.
The
loadFilehelper is async but usesreadFileSync(line 31), which blocks the event loop. Since this is already an async function, preferfs.promises.readFilefor consistency and to avoid blocking I/O.♻️ Proposed refactor to use async I/O
const loadFile = async (configPath: string, ext: string) => { if (ext === '.json') { - const content = fs.readFileSync(configPath, 'utf8'); + const content = await fs.promises.readFile(configPath, 'utf8'); return JSON.parse(content) as Options; } else {🤖 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/config/resolveConfig.ts` around lines 29 - 39, The loadFile function is declared as async but uses synchronous file I/O with fs.readFileSync in the JSON handling branch, which blocks the event loop. Replace the fs.readFileSync call with fs.promises.readFile and add the await keyword to make the operation truly asynchronous. This ensures the function properly awaits the file read operation instead of blocking, making it consistent with the async nature of the function and the import operation already present in the else branch.
🤖 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/bin/tempo.js`:
- Around line 47-49: The scaffold:all command performs two sequential safelyCopy
calls without pre-validation, which can result in a half-scaffolded directory if
the first copy succeeds but the second fails. Before executing any safelyCopy
operations in the scaffold:all block, pre-check both destination files
(tempo.config.ts and index.html) to ensure neither already exists. Only proceed
with both copy operations if both validation checks pass, implementing an
all-or-nothing approach to prevent partial scaffolding.
In `@packages/tempo/doc/tempo.config.md`:
- Around line 48-57: The code example violates ES module syntax rules by placing
a static import statement after an await statement at module scope. Fix this in
the bootstrap code example by either replacing the static import statement for
App with a dynamic import using await import() syntax that executes after the
Tempo.bootstrap() call, or by restructuring the code to move the
Tempo.bootstrap() call to occur after all static imports but before any app
instance creation. Ensure all static imports appear before any other
module-level statements.
- Line 24: The link fragment anchor in the "Target Environment" paragraph is
broken because the generated GitHub Flavored Markdown anchor does not match the
reference. Update the link fragment from `#3-explicit-initialization-tempo-init`
to `#3-explicit-initialization-tempoinit` (remove the hyphen between "tempo" and
"init") to correctly match the anchor generated from the heading "3. Explicit
Initialization (`Tempo.init`)" on line 163.
In `@packages/tempo/src/config/resolveConfig.ts`:
- Around line 58-72: The catch block in the config file discovery loop (within
the while loop checking extensions) currently returns undefined immediately when
loadFile fails, preventing fallback to other file extensions. Replace the return
undefined statement with continue to allow the loop to proceed to the next
extension in the exts array. Additionally, update the documentation to
explicitly state that TypeScript config files require Bun, Deno, or Node.js with
a TypeScript loader such as tsx, since vanilla Node.js cannot dynamically import
TypeScript files without a loader.
---
Nitpick comments:
In `@packages/tempo/src/config/resolveConfig.ts`:
- Around line 29-39: The loadFile function is declared as async but uses
synchronous file I/O with fs.readFileSync in the JSON handling branch, which
blocks the event loop. Replace the fs.readFileSync call with
fs.promises.readFile and add the await keyword to make the operation truly
asynchronous. This ensures the function properly awaits the file read operation
instead of blocking, making it consistent with the async nature of the function
and the import operation already present in the else branch.
In `@packages/tempo/template/index.sample.html`:
- Around line 49-55: Replace the unsafe innerHTML assignments with safe DOM
manipulation methods. In both the success message block (lines 49-52) and the
error message block (line 55), avoid using innerHTML with string interpolation.
Instead, use document.createElement to build the HTML structure, use textContent
to set plain text content (such as the formatted time and err.message), and use
appendChild to add elements to the output element. This eliminates XSS
vulnerabilities and establishes secure patterns in the template.
🪄 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: 7a9b61c7-531e-4aa7-b12d-83b8d1226080
📒 Files selected for processing (14)
packages/tempo/CHANGELOG.mdpackages/tempo/bin/tempo.jspackages/tempo/doc/tempo.config.mdpackages/tempo/importmap.jsonpackages/tempo/package.jsonpackages/tempo/src/config/defineConfig.tspackages/tempo/src/config/resolveConfig.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.index.tspackages/tempo/src/tempo.version.tspackages/tempo/template/index.sample.htmlpackages/tempo/template/tempo.config.sample.tspackages/tempo/test/core/__fixtures__/config/tempo.config.jspackages/tempo/test/core/bootstrap.test.ts
💤 Files with no reviewable changes (1)
- packages/tempo/importmap.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
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/tempo.class.ts (1)
470-472:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid global pattern rebuilds during local guard updates.
At Line 470, this branch still recompiles global patterns even when
[$buildGuard]was called with a localtargetState. That means local config updates can trigger unnecessary globalsetPatterns(...)work.Proposed fix
- if (this[$Internal]() === _global) { + if (!targetState && this[$Internal]() === _global) { setPatterns(this[$Internal]()); }🤖 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 470 - 472, The condition at the setPatterns call site checks if this[$Internal]() equals _global, but it doesn't verify whether the current [$buildGuard] call was for a local targetState or a global update. Modify the condition to also check if the update was a global one (not a local targetState update) before calling setPatterns, so that local config updates do not trigger unnecessary global pattern rebuilds even when the internal reference happens to be the global object.
🧹 Nitpick comments (1)
packages/tempo/src/tempo.class.ts (1)
327-331: ⚡ Quick winRemove duplicate pattern compilation in
[$setConfig].
setPatterns(shape)runs in thepatternsDirtyblock (Line 328) and again unconditionally at Line 346. This double rebuild is redundant on dirty updates.Refactor sketch
const patternsDirty = extendState(shape, mergedOptions); if (patternsDirty) { - setPatterns(shape); if (shape.config.scope === 'local') this[$buildGuard](shape); }Also applies to: 346-346
🤖 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 327 - 331, The setPatterns(shape) method is being called twice in the [$setConfig] method - once conditionally when patternsDirty is true (line 328) and again unconditionally later (line 346). This causes redundant pattern compilation on dirty updates. Remove the unconditional setPatterns(shape) call at line 346 or wrap it in a condition to only execute when patternsDirty is false, so patterns are compiled only once per configuration update.
🤖 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/bench/bench.v3.3.0.ts`:
- Around line 4-7: The block comment at the top of the file describing the three
benchmark configurations (A, B, C) does not match the actual benchmark cases
being run in the code. Update the comment block to accurately describe what
configurations B and C actually test, ensuring B correctly describes the module
comparison case and C correctly describes the localized fr-FR case, so the
benchmark summary accurately reflects what is being measured.
In `@packages/tempo/bench/modifiers.ts`:
- Around line 4-13: The runBenchmark function currently times Tempo expression
creation without accounting for cold-start JIT behavior and ambient runtime
defaults, making results non-deterministic across different machines and runs.
Before the timing loop (between performance.now() calls), first call
Tempo.init(...) with fixed initialization parameters, then add a warm-up pass
that runs the same loop with ITERATIONS a smaller number of times (or a
representative subset) to allow the JIT compiler to stabilize, before measuring
the actual performance in the existing timing loop.
In `@packages/tempo/src/engine/engine.lexer.ts`:
- Around line 81-85: The code at line 84 calls `.map()` directly on `words`
without checking its type, but the config contract allows `words` to be either a
`string` or `string[]`. When `words` is a single string, `.map()` will fail with
a TypeError. Fix this by ensuring `words` is converted to an array before
calling `.map()`. Check if `words` is a string using `typeof words === 'string'`
and wrap it in an array, then apply the normalization logic (toLowerCase, trim,
normalize, and replace operations) to handle both single-string and
array-of-strings cases.
In `@packages/tempo/src/engine/engine.term.ts`:
- Around line 82-86: The issue is that when comparing the normalized modifier
`norm` against configured words in the modifiers registry loop, the words from
`state.config.registry.modifiers` are not being normalized in the same way. The
`norm` variable is lowercased and trimmed, but the words from the registry are
being compared as-is without the same normalization. Fix this by normalizing
each word in the words array (applying toLowerCase and trim) before comparing it
with `norm` in the includes check on line 85, so that the matching behavior is
consistent with how the lexer parses modifiers.
---
Outside diff comments:
In `@packages/tempo/src/tempo.class.ts`:
- Around line 470-472: The condition at the setPatterns call site checks if
this[$Internal]() equals _global, but it doesn't verify whether the current
[$buildGuard] call was for a local targetState or a global update. Modify the
condition to also check if the update was a global one (not a local targetState
update) before calling setPatterns, so that local config updates do not trigger
unnecessary global pattern rebuilds even when the internal reference happens to
be the global object.
---
Nitpick comments:
In `@packages/tempo/src/tempo.class.ts`:
- Around line 327-331: The setPatterns(shape) method is being called twice in
the [$setConfig] method - once conditionally when patternsDirty is true (line
328) and again unconditionally later (line 346). This causes redundant pattern
compilation on dirty updates. Remove the unconditional setPatterns(shape) call
at line 346 or wrap it in a condition to only execute when patternsDirty is
false, so patterns are compiled only once per configuration update.
🪄 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: 278d984f-bb09-4f30-a661-b711759afa21
📒 Files selected for processing (27)
packages/tempo/CHANGELOG.mdpackages/tempo/bench/bench.v3.3.0.tspackages/tempo/bench/benchmark-results-v3.3.0.jsonpackages/tempo/bench/modifiers.tspackages/tempo/bench/results/modifiers_post_refactor.txtpackages/tempo/bench/results/modifiers_pre_refactor_20260621_075533.txtpackages/tempo/bin/tempo.jspackages/tempo/doc/tempo.config.mdpackages/tempo/doc/tempo.cookbook.mdpackages/tempo/doc/tempo.parse.mdpackages/tempo/package.jsonpackages/tempo/plan/licensing_architecture.mdpackages/tempo/plan/ticker-reactive.mdpackages/tempo/src/config/resolveConfig.tspackages/tempo/src/engine/engine.lexer.tspackages/tempo/src/engine/engine.pattern.tspackages/tempo/src/engine/engine.term.tspackages/tempo/src/module/module.parse.tspackages/tempo/src/support/support.default.tspackages/tempo/src/support/support.enum.tspackages/tempo/src/support/support.init.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.type.tspackages/tempo/src/tempo.version.tspackages/tempo/template/index.sample.htmlpackages/tempo/template/locales.sample.tspackages/tempo/test/engine/modifiers.test.ts
💤 Files with no reviewable changes (2)
- packages/tempo/plan/licensing_architecture.md
- packages/tempo/plan/ticker-reactive.md
✅ Files skipped from review due to trivial changes (6)
- packages/tempo/bench/benchmark-results-v3.3.0.json
- packages/tempo/bench/results/modifiers_pre_refactor_20260621_075533.txt
- packages/tempo/doc/tempo.cookbook.md
- packages/tempo/src/tempo.version.ts
- packages/tempo/template/locales.sample.ts
- packages/tempo/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/tempo/package.json
- packages/tempo/bin/tempo.js
- packages/tempo/doc/tempo.config.md
- packages/tempo/template/index.sample.html
- packages/tempo/src/config/resolveConfig.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.
Summary by CodeRabbit
Tempo.bootstrap()for automatic config discovery and initializationtempoCLI with scaffolding commands for config and HTML templates