Feature/typedoc - #66
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 Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughChangesAI parsing plugin
Tempo integration and release
Library hardening and maintenance
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant parseAI
participant Tempo
participant AIProvider
Caller->>parseAI: Submit natural-language input
parseAI->>Tempo: Attempt native parsing
parseAI->>AIProvider: Send contextual JSON request
AIProvider-->>parseAI: Return ISO result or provider error
parseAI->>Tempo: Construct Tempo instance
parseAI-->>Caller: Return parsed result
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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: 11
🧹 Nitpick comments (3)
packages/library/src/common/enumerate.library.ts (1)
73-91: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the hardened caller-context check.
Please test the exact invalid
Modulecontext that previously caused prototype corruption, and separately verify that normal enum.extend()calls still inherit the enum methods. This protects both sides of the newgetType(this)/isFunction(this?.has)branch.🤖 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/library/src/common/enumerate.library.ts` around lines 73 - 91, Add regression tests for enumify’s caller-context branching: invoke enumify with the invalid Module context that previously corrupted the prototype and assert it uses the safe enum prototype, then verify a normal enum’s extend flow still inherits and exposes the expected enum methods. Anchor the coverage to enumify and the existing .extend behavior without changing unrelated functionality.packages/plugins/parseAI/src/index.ts (2)
11-19: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUnbounded in-memory cache growth.
_state.cacheis a module-levelMapwith no size cap or TTL eviction; every distinct(string, date)pair parsed accumulates indefinitely unless a caller manually invokesclearAiCache. For a long-running server process this is an unbounded memory growth path.🤖 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/plugins/parseAI/src/index.ts` around lines 11 - 19, Bound the module-level cache managed through _state.cache so entries cannot accumulate indefinitely: add and enforce a maximum size and/or TTL eviction when parsing caches distinct results, while preserving cache reuse and clearAiCache behavior. Ensure eviction occurs automatically during normal cache access or insertion.
193-194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winModel-name string sniffing for token-limit param is brittle.
isNewOpenAihardcodesgpt-5/o1/o3prefixes to decidemax_completion_tokensvsmax_tokens. Future OpenAI-compatible models (e.g. a hypotheticalo4,gpt-6) won't match and will silently use the wrong param name, likely causing a 400 from the provider.Consider making this configurable per-provider (e.g. an
AiProvider.tokenParamfield orprovider.optionsoverride) instead of name-sniffing.🤖 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/plugins/parseAI/src/index.ts` around lines 193 - 194, The token-limit selection in the model request should not depend on hardcoded model prefixes. Replace the isNewOpenAi name-sniffing in the surrounding provider/request flow with a configurable provider-level token parameter, such as AiProvider.tokenParam or an existing provider.options override, and use that configured value when constructing tokenLimit so future OpenAI-compatible models select the correct parameter.
🤖 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 @.github/workflows/ci.yml:
- Around line 60-61: Align the CI secret and parseAI integration tests so real
network tests execute: update .github/workflows/ci.yml lines 60-61 and
packages/plugins/parseAI/test/index.spec.ts lines 1-133 consistently to use
either OPENAI_API_KEY with the openai provider or GROQ_API_KEY with the groq
provider. Ensure hasKey, initAI, and provider configuration reference the same
key and provider supplied by CI, while preserving the no-key error test’s
behavior.
In `@packages/plugins/parseAI/doc/context.md`:
- Around line 31-35: The “Decoupled Output Bridge” documentation uses an
incorrect timezone-qualified ISO example. Update the example to a local ISO
string without an offset or Z suffix, and explicitly state that provider
responses must omit timezone suffixes to match the contract enforced by the
parseAI implementation.
In `@packages/plugins/parseAI/doc/index.md`:
- Around line 17-19: Fix the blockquote formatting in the parseAI documentation
by removing the blank line between the two quoted paragraphs or prefixing that
line with `>` so the entire note and security warning remains a single valid
blockquote.
- Around line 51-56: Update the “Global Debugging” documentation around initAI
to clearly mark debug: true as development-only. Warn that prompts, localized
context, and raw LLM responses may contain user-supplied or sensitive data, and
recommend disabling debug logging or redacting sensitive values in production.
In `@packages/plugins/parseAI/doc/rate-limits.md`:
- Around line 83-92: Update the “Extensible Caching (Enterprise)” documentation
to match the cache’s synchronous Map<string, string> contract: either document
only synchronous adapters or first define and export a synchronous CacheAdapter
interface for custom stores, and remove or revise the Redis/Edge KV example
unless it implements that contract. Do not describe async adapter methods
without updating the cache implementation to await them.
In `@packages/plugins/parseAI/src/index.ts`:
- Around line 137-165: Update the cache salt and cacheKey construction in the
parseAI flow to include the resolved tz, cal, loc, and sph values alongside the
anchor date. Ensure calls with different timeZone, calendar, locale, or sphere
contexts produce distinct keys while preserving cache hits for identical
resolved contexts; apply the same context-aware keying to the related cache
write around the parsed result.
- Around line 243-262: Validate the LLM response shape before accessing it in
the response-processing flow: ensure data.choices[0].message.content is a
string, and ensure parsedData.iso is a string before using replace. Convert any
missing or invalid fields into TempoAiError failures with the existing 422
contract, while preserving the current JSON parsing and INVALID handling.
- Around line 196-213: Update the provider fetch flow in parseAI to use an
AbortController-based timeout for each fetch(url, ...) request. Ensure the
timeout aborts slow or hanging providers and is handled as a provider failure so
the existing sequential fallback loop can continue to the next provider.
In `@packages/tempo/doc/8-project-and-support/releases/v3.x.md`:
- Around line 5-7: Update the “Observability & Control” entry for the AI plugin
to explicitly describe debug: true as verbose logging that may expose user
input, prompts/context, and raw provider responses; warn users to enable it only
with non-sensitive inputs and avoid implying it is safe for production use.
In `@packages/tempo/src/tempo.class.ts`:
- Line 1206: Update the private `#fmt` declaration and the public fmt getter in
packages/tempo/src/tempo.class.ts at lines 1206 and 1626 to use a partial string
map, reflecting that `#setDelegator`('fmt') may return undefined or omit
registered formats; preserve the existing delegation behavior while allowing
missing format values in both type declarations.
In `@packages/tempo/typedoc.json`:
- Around line 2-4: Update the `API Overview` navigation entry in the VitePress
configuration to target `/api/index.html`, matching the TypeDoc output
configured by `typedoc.json`; alternatively, add an explicit compatibility
redirect from `/doc/api/` to that generated path.
---
Nitpick comments:
In `@packages/library/src/common/enumerate.library.ts`:
- Around line 73-91: Add regression tests for enumify’s caller-context
branching: invoke enumify with the invalid Module context that previously
corrupted the prototype and assert it uses the safe enum prototype, then verify
a normal enum’s extend flow still inherits and exposes the expected enum
methods. Anchor the coverage to enumify and the existing .extend behavior
without changing unrelated functionality.
In `@packages/plugins/parseAI/src/index.ts`:
- Around line 11-19: Bound the module-level cache managed through _state.cache
so entries cannot accumulate indefinitely: add and enforce a maximum size and/or
TTL eviction when parsing caches distinct results, while preserving cache reuse
and clearAiCache behavior. Ensure eviction occurs automatically during normal
cache access or insertion.
- Around line 193-194: The token-limit selection in the model request should not
depend on hardcoded model prefixes. Replace the isNewOpenAi name-sniffing in the
surrounding provider/request flow with a configurable provider-level token
parameter, such as AiProvider.tokenParam or an existing provider.options
override, and use that configured value when constructing tokenLimit so future
OpenAI-compatible models select the correct parameter.
🪄 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 Plus
Run ID: 39c40d10-c104-4749-95e4-38480a5ac81b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (39)
.github/workflows/ci.yml.gitignorepackage.jsonpackages/library/CHANGELOG.mdpackages/library/package.jsonpackages/library/src/common/enumerate.library.tspackages/plugins/.setup/catalog.jsonpackages/plugins/.setup/community-plugin-template.mdpackages/plugins/parseAI/CHANGELOG.mdpackages/plugins/parseAI/LICENSEpackages/plugins/parseAI/README.mdpackages/plugins/parseAI/doc/architecture.mdpackages/plugins/parseAI/doc/context.mdpackages/plugins/parseAI/doc/index.mdpackages/plugins/parseAI/doc/rate-limits.mdpackages/plugins/parseAI/package.jsonpackages/plugins/parseAI/src/error.tspackages/plugins/parseAI/src/index.tspackages/plugins/parseAI/src/parseAI.type.tspackages/plugins/parseAI/test/index.spec.tspackages/plugins/parseAI/test/tsconfig.jsonpackages/plugins/parseAI/tsconfig.jsonpackages/plugins/parseAI/tsup.config.tspackages/tempo/.vitepress/config.tspackages/tempo/.vitepress/theme/components/CatalogList.vuepackages/tempo/.vitepress/theme/data/catalog.jsonpackages/tempo/CHANGELOG.mdpackages/tempo/doc/8-project-and-support/releases/v3.x.mdpackages/tempo/package.jsonpackages/tempo/plan/ordinal-offset-parsing.mdpackages/tempo/src/interval.class.tspackages/tempo/src/support/support.default.tspackages/tempo/src/support/support.enum.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.version.tspackages/tempo/typedoc.jsonpackages/tempo/typedoc.library.jsonpackages/tempo/vitest.config.tsvitest.config.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/plugins/parseAI/test/index.spec.ts (1)
89-107: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the live-mode rate-limit test issue its own request.
When
LIVE_AI_TESTis enabled, this test skipsparseAI()and asserts rate-limit state produced by earlier tests. Execute a forced request in both modes so the test is isolated.🤖 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/plugins/parseAI/test/index.spec.ts` around lines 89 - 107, Update the “should expose rate limits after a request” test to call parseAI with force enabled in both live and mocked modes, removing the live-mode skip while preserving the existing mocked fetch setup for non-live tests. Ensure the rate-limit assertions use state produced by this test’s own request.
🤖 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/plugins/parseAI/doc/context.md`:
- Line 35: Soften the final sentence in the parseAI documentation to claim only
that the bridge eliminates AST-construction ambiguity, not general LLM
hallucinations. Preserve the existing description of the local ISO contract and
native Tempo conversion.
In `@packages/plugins/parseAI/src/cache.ts`:
- Around line 35-39: Update the LRU access logic around super.get(key) so
entries whose stored value is undefined are still recognized as present and
refreshed. Use an existence check such as super.has(key) before deleting and
resetting the key, while preserving the existing behavior for missing keys.
In `@packages/plugins/parseAI/test/index.spec.ts`:
- Around line 211-215: Update the “should update BoundedCache options via
initAI” test to assert the cache configuration after each initAI call. Use the
created BoundedCache instance or its observable eviction behavior to verify both
maxCacheSize and cacheTtl values, and remove the unused local setup if it is not
the cache instance configured by initAI.
---
Outside diff comments:
In `@packages/plugins/parseAI/test/index.spec.ts`:
- Around line 89-107: Update the “should expose rate limits after a request”
test to call parseAI with force enabled in both live and mocked modes, removing
the live-mode skip while preserving the existing mocked fetch setup for non-live
tests. Ensure the rate-limit assertions use state produced by this test’s own
request.
🪄 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 Plus
Run ID: 6135a950-2674-4ec3-b671-6273371c4b97
📒 Files selected for processing (12)
.github/workflows/ci.ymlpackages/library/test/common/enumerate.test.tspackages/plugins/parseAI/doc/context.mdpackages/plugins/parseAI/doc/index.mdpackages/plugins/parseAI/doc/rate-limits.mdpackages/plugins/parseAI/src/cache.tspackages/plugins/parseAI/src/index.tspackages/plugins/parseAI/src/parseAI.type.tspackages/plugins/parseAI/test/index.spec.tspackages/tempo/doc/8-project-and-support/releases/v3.x.mdpackages/tempo/src/tempo.class.tspackages/tempo/typedoc.json
💤 Files with no reviewable changes (1)
- .github/workflows/ci.yml
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/tempo/typedoc.json
- packages/plugins/parseAI/src/parseAI.type.ts
- packages/plugins/parseAI/doc/index.md
- packages/tempo/doc/8-project-and-support/releases/v3.x.md
- packages/plugins/parseAI/doc/rate-limits.md
- packages/tempo/src/tempo.class.ts
- packages/plugins/parseAI/src/index.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
silentconfiguration option to suppress verbose native parse error output.enumifyto prevent prototype/registry corruption from invalid execution contexts.