docs: document async loading sub-parameters - #398
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThe change centralizes async-loading option resolution, adds snapshot notification controls, marks legacy readiness settings as compatibility options, updates serve forwarding, and documents atomic capability snapshots and health endpoint semantics in English and Chinese. ChangesAsync loading behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ServeCLI
participant serveCommand
participant resolveAsyncLoadingOptions
participant AgentConfigManager
ServeCLI->>serveCommand: Provide async-loading flags
serveCommand->>resolveAsyncLoadingOptions: Resolve CLI and application settings
resolveAsyncLoadingOptions-->>serveCommand: Return resolved asyncLoading options
serveCommand->>AgentConfigManager: Initialize with resolved asyncLoading options
AgentConfigManager-->>serveCommand: Publish capability snapshot and notifications
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Thank you for your contribution. I noticed this is still marked as a draft — would you like any further revisions, or is it ready to be merged? |
Thanks for drafting the bilingual documentation. Source tracing found that the current table describes behavior that is not implemented: Please keep this PR in draft and revise it against the accepted contract in #393: #393 (comment) Required changes before this can be ready:
The English and Chinese documentation should remain aligned. |
|
Updated this draft to match the accepted runtime contract and merged current main. Deprecated async server-count/timeout CLI, ONE_MCP env, and TOML inputs now warn as compatibility no-ops; --async-notify-on-snapshot is canonical, the old ready name warns as an alias, and the canonical value wins conflicts. English/Chinese docs now describe immediate listener availability, atomic snapshots, notification coalescing, and /health/ready vs /health/mcp. Validation: 51 focused tests passed; pnpm lint passed; pnpm typecheck passed; pnpm docs:build passed. Head: 0249dd1. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/core/notifications/notificationManager.test.ts (1)
106-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the shared mock factory for
AgentConfigManager.These lines create a local untyped manager test double. Use
test/unit-utils/MockFactories.tsfor this mock. If the factory does not support this case, add the factory there and reuse it.As per coding guidelines, “Use
test/unit-utils/MockFactories.tsfor consistent mock data and mock factories.”🤖 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 `@src/core/notifications/notificationManager.test.ts` around lines 106 - 108, Replace the inline AgentConfigManager double in the relevant notification manager test with the shared mock factory from MockFactories.ts, configuring it to return clientNotifications: false. If the factory lacks the needed configuration, extend it there first, then reuse the typed factory output instead of the local cast and spy setup.Source: Coding guidelines
🤖 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 `@docs/en/reference/health-check.md`:
- Line 173: Update the `/health/ready` descriptions in
docs/en/reference/health-check.md:173-173,
docs/en/guide/advanced/fast-startup.md:178-178,
docs/zh/reference/health-check.md:158-158, and
docs/zh/guide/advanced/fast-startup.md:172-172 to state that readiness returns
503 when any supervised stdio backend is restarting or in a crash loop, while
retaining the distinction that it does not wait for backend loading or a
server-count threshold.
In `@src/commands/serve/asyncLoadingOptions.ts`:
- Around line 38-64: Update resolveAsyncLoadingOptions to validate and normalize
resolved values before returning them: cast boolean options at their consumption
points, reject or normalize invalid batchDelayMs values before they reach
setTimeout, and require parsed async-batch-delay CLI input to be an integer
before applying it to configuration. Preserve existing defaults and
canonical-option precedence while preventing malformed values from reaching
server config.
In `@src/commands/serve/index.test.ts`:
- Around line 65-78: Replace the direct process.env reads and writes in the
“passes deprecated async environment inputs through for warning handling” test
with the repository-approved environment stub or yargs environment helper. Set
ONE_MCP_ASYNC_MIN_SERVERS through that helper, preserve the existing assertion,
and restore the isolated environment value using the helper’s cleanup mechanism.
In `@src/commands/serve/serve.ts`:
- Around line 445-447: Align the async-loading batch delay default used by
resolveAsyncLoadingOptions with the value persisted by AgentConfigManager and
documented elsewhere. Update the relevant default configuration and tests so
serve.ts, the manager, notifications, documentation, and fast-startup examples
consistently use the same runtime default.
---
Nitpick comments:
In `@src/core/notifications/notificationManager.test.ts`:
- Around line 106-108: Replace the inline AgentConfigManager double in the
relevant notification manager test with the shared mock factory from
MockFactories.ts, configuring it to return clientNotifications: false. If the
factory lacks the needed configuration, extend it there first, then reuse the
typed factory output instead of the local cast and spy setup.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 363f5c5d-8db9-466e-9f9b-28727889a5b4
📒 Files selected for processing (13)
docs/en/guide/advanced/fast-startup.mddocs/en/reference/health-check.mddocs/zh/guide/advanced/fast-startup.mddocs/zh/reference/health-check.mdsrc/commands/serve/asyncLoadingOptions.test.tssrc/commands/serve/asyncLoadingOptions.tssrc/commands/serve/index.test.tssrc/commands/serve/index.tssrc/commands/serve/serve.transport-precedence.test.tssrc/commands/serve/serve.tssrc/commands/serve/serveBackground.tssrc/core/notifications/notificationManager.test.tssrc/core/types/transport.ts
|
Addressed all four new CodeRabbit findings in 6f91db7: documented the supervised-backend readiness condition, restored the shared 1000 ms batch default, rejected malformed batch-delay values, and isolated the environment test with Vitest stubbing. Validation: 13 focused tests passed, typecheck passed, ESLint and Prettier passed, and the commit hook passed. All four threads are resolved. |
2 participants
Summary
Validation
pnpm docs:buildCloses #393
Summary by CodeRabbit