Draft
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request updates documentation and code examples across multiple files to reflect a public API rename from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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: 6
🧹 Nitpick comments (1)
website/components/sections/AgentReadySection.tsx (1)
74-76: Add the imports these Python examples still rely on.Both snippets now start with
from iii import register_worker, but they still useos.environandLogger()later in the block. As copied, they'll fail before users even reach the renamed initializer.✏️ Suggested snippet update
- python: `from iii import register_worker + python: `import os +from iii import Logger, register_worker ... - python: `from iii import register_worker + python: `import os +from iii import Logger, register_workerAlso applies to: 278-280
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@website/components/sections/AgentReadySection.tsx` around lines 74 - 76, The Python examples import register_worker but still use os.environ and Logger(), so add the missing imports to the snippet: import os (for os.environ) and import logging or from iii import Logger if using the package Logger; update the example to call logging.getLogger() or the appropriate Logger from iii so functions like register_worker, os.environ and Logger() work together in the snippet.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/content/api-reference/iii-sdk.mdx`:
- Around line 30-32: The docs example now uses the Rust type InitOptions (shown
in the code snippet with register_worker and iii_sdk), but the reference section
only describes TypeScript options; add a Rust subsection describing InitOptions:
list each public field name and its type, the default values (or note
Default::default behavior), and any semantic differences vs the TypeScript
options; ensure the subsection is placed near the existing options reference and
clearly labeled (e.g., "Rust — InitOptions") so Rust users can see how
InitOptions::default() maps to the documented defaults.
In `@docs/content/api-reference/sdk-node.mdx`:
- Around line 14-24: The page inconsistently uses registerWorker but still lists
init in the Subpath Exports table; update the Subpath Exports (and any other
occurrences) to use registerWorker consistently. Search for the symbol "init" in
this file and replace or rename those instances to "registerWorker" (and update
any associated type/signature text such as the ResponseField or export
signatures) so the function name, type signature, and documentation all match
the exported API (e.g., ensure ResponseField name, table entries, and any
examples referencing init now reference registerWorker and the same signature
"(address: string, options?: InitOptions) => ISdk").
In `@docs/content/api-reference/sdk-python.mdx`:
- Line 20: The doc currently contradicts itself: the intro states
register_worker() auto-establishes the WebSocket but the Methods section treats
connect() as the required entry point; update the Methods section and examples
to reflect that register_worker() performs the automatic connection and mark
connect() as optional/advanced (or remove it from the primary flow).
Specifically, change prose around the connect() method to say it is only needed
for manual control/reconnect/testing, adjust examples to show register_worker()
as the primary entry point (no explicit connect call), and ensure method
list/usage order and any "required" language reference register_worker() as the
default connection mechanism.
In `@docs/content/api-reference/sdk-rust.mdx`:
- Around line 25-33: The Re-exports section is out of sync: replace the outdated
III re-export and add the new symbols promoted by the example by exporting
register_worker and InitOptions (and Logger if you intend to keep it) so the
"Re-exports from iii_sdk" block matches the initializer shown; update the
exported symbol list to include register_worker and InitOptions and remove or
rename III to the current API symbol(s) referenced in the example.
In `@docs/content/architecture/workers.mdx`:
- Around line 65-67: The defaults in the Register Worker Options table are out
of sync with the SDK reference; update the table so the defaults for
registerWorker options match the canonical values in the SDK reference
(specifically reconcile workerName, enableMetricsReporting, and otel to the
values shown in the iii-sdk reference) and scan the rest of the option rows to
ensure all defaults align; edit the table rows for the registerWorker option
keys to match the SDK's default values and add a brief note or comment if any
SDK default is intentionally different so future reviewers can detect drift.
In `@website/components/sections/DependencyVisualization.tsx`:
- Around line 841-842: The check against lowerLine mistakenly uses a mixed-case
TypeScript matcher includes("registerWorker(") which will never match because
lowerLine is already toLowerCase(); replace that matcher with the lowercased
form (e.g., "registerworker(") so the condition correctly detects TypeScript
samples—update the conditional that references
lowerLine.includes("registerWorker(") to use the lowercase string and keep the
existing snake_case check lowerLine.includes("register_worker(") intact.
---
Nitpick comments:
In `@website/components/sections/AgentReadySection.tsx`:
- Around line 74-76: The Python examples import register_worker but still use
os.environ and Logger(), so add the missing imports to the snippet: import os
(for os.environ) and import logging or from iii import Logger if using the
package Logger; update the example to call logging.getLogger() or the
appropriate Logger from iii so functions like register_worker, os.environ and
Logger() work together in the snippet.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f88356b4-6e5c-40b5-a5e5-3c4c9c12e28e
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlwebsite/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
docs/content/api-reference/iii-sdk.mdxdocs/content/api-reference/sdk-node.mdxdocs/content/api-reference/sdk-python.mdxdocs/content/api-reference/sdk-rust.mdxdocs/content/architecture/workers.mdxdocs/content/workers.mdxwebsite/components/MachineView.tsxwebsite/components/Terminal.tsxwebsite/components/sections/AgentReadySection.tsxwebsite/components/sections/CodeExamples.tsxwebsite/components/sections/DependencyVisualization.tsxwebsite/components/sections/EngineSection.tsxwebsite/components/sections/HelloWorldSection.tsx
Comment on lines
+30
to
+32
| use iii_sdk::{register_worker, InitOptions}; | ||
|
|
||
| let iii = III::new("ws://localhost:49134"); | ||
| iii.connect().await?; | ||
| let iii = register_worker("ws://localhost:49134", InitOptions::default())?; |
Contributor
There was a problem hiding this comment.
Document Rust InitOptions now that the example uses it.
The Rust tab introduces InitOptions::default(), but the only options reference below is still TypeScript-specific. That leaves Rust readers without any description of the fields/defaults for the new public type.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/content/api-reference/iii-sdk.mdx` around lines 30 - 32, The docs
example now uses the Rust type InitOptions (shown in the code snippet with
register_worker and iii_sdk), but the reference section only describes
TypeScript options; add a Rust subsection describing InitOptions: list each
public field name and its type, the default values (or note Default::default
behavior), and any semantic differences vs the TypeScript options; ensure the
subsection is placed near the existing options reference and clearly labeled
(e.g., "Rust — InitOptions") so Rust users can see how InitOptions::default()
maps to the documented defaults.
Comment on lines
+14
to
+24
| The Node.js SDK exports a `registerWorker` function that creates a connected SDK instance. The WebSocket connection is established automatically — there is no separate `connect()` call. | ||
|
|
||
| ```typescript | ||
| import { init } from 'iii-sdk' | ||
| import { registerWorker } from 'iii-sdk' | ||
|
|
||
| const iii = init(process.env.III_BRIDGE_URL ?? 'ws://localhost:49134', { | ||
| const iii = registerWorker(process.env.III_BRIDGE_URL ?? 'ws://localhost:49134', { | ||
| workerName: 'my-worker', | ||
| }) | ||
| ``` | ||
|
|
||
| <ResponseField name="init" type="(address: string, options?: InitOptions) => ISdk" required> | ||
| <ResponseField name="registerWorker" type="(address: string, options?: InitOptions) => ISdk" required> |
Contributor
There was a problem hiding this comment.
Finish the rename on this page.
This section documents registerWorker, but the Subpath Exports table later in the file still lists init at Line 324. That leaves the page internally inconsistent and easy to misread.
💡 Suggested fix
-| `iii-sdk` | `init`, `Logger`, `OTel context`, `Logger`, `ISdk`, `ApiRequest`, `ApiResponse` |
+| `iii-sdk` | `registerWorker`, `Logger`, `OTel context`, `Logger`, `ISdk`, `ApiRequest`, `ApiResponse` |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/content/api-reference/sdk-node.mdx` around lines 14 - 24, The page
inconsistently uses registerWorker but still lists init in the Subpath Exports
table; update the Subpath Exports (and any other occurrences) to use
registerWorker consistently. Search for the symbol "init" in this file and
replace or rename those instances to "registerWorker" (and update any associated
type/signature text such as the ResponseField or export signatures) so the
function name, type signature, and documentation all match the exported API
(e.g., ensure ResponseField name, table entries, and any examples referencing
init now reference registerWorker and the same signature "(address: string,
options?: InitOptions) => ISdk").
| ## Initialization | ||
|
|
||
| The Python SDK requires an explicit `connect()` call to establish the WebSocket connection. | ||
| `register_worker()` establishes the WebSocket connection automatically. |
Contributor
There was a problem hiding this comment.
Clarify the connect() story after auto-connection.
The new intro says register_worker() connects automatically, but the Methods section still documents connect as a required entry point. Please either de-emphasize connect() as manual/advanced usage or remove it from the primary flow so the page doesn't contradict itself.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/content/api-reference/sdk-python.mdx` at line 20, The doc currently
contradicts itself: the intro states register_worker() auto-establishes the
WebSocket but the Methods section treats connect() as the required entry point;
update the Methods section and examples to reflect that register_worker()
performs the automatic connection and mark connect() as optional/advanced (or
remove it from the primary flow). Specifically, change prose around the
connect() method to say it is only needed for manual control/reconnect/testing,
adjust examples to show register_worker() as the primary entry point (no
explicit connect call), and ensure method list/usage order and any "required"
language reference register_worker() as the default connection mechanism.
Comment on lines
+25
to
+33
| `register_worker()` establishes the WebSocket connection automatically. | ||
|
|
||
| ```rust | ||
| use iii_sdk::{III, Logger}; | ||
| use iii_sdk::{register_worker, InitOptions, Logger}; | ||
| use serde_json::json; | ||
|
|
||
| #[tokio::main] | ||
| async fn main() -> Result<(), Box<dyn std::error::Error>> { | ||
| let iii = III::new("ws://127.0.0.1:49134"); | ||
| iii.connect().await?; | ||
| let iii = register_worker("ws://127.0.0.1:49134", InitOptions::default())?; |
Contributor
There was a problem hiding this comment.
Update the later re-exports block to match this initializer.
This example now promotes register_worker and InitOptions, but the Re-exports from iii_sdk section later in the page still lists III and omits both new symbols. The Rust API reference ends up contradicting itself.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/content/api-reference/sdk-rust.mdx` around lines 25 - 33, The Re-exports
section is out of sync: replace the outdated III re-export and add the new
symbols promoted by the example by exporting register_worker and InitOptions
(and Logger if you intend to keep it) so the "Re-exports from iii_sdk" block
matches the initializer shown; update the exported symbol list to include
register_worker and InitOptions and remove or rename III to the current API
symbol(s) referenced in the example.
Comment on lines
+65
to
+67
| ## Register Worker Options | ||
|
|
||
| The second argument to `init()` configures the Worker's behavior: | ||
| The second argument to `registerWorker()` configures the Worker's behavior: |
Contributor
There was a problem hiding this comment.
The defaults in the table below are out of sync with the SDK reference.
Now that this section is framed around registerWorker(), the option table underneath should match docs/content/api-reference/iii-sdk.mdx. Right now at least workerName, enableMetricsReporting, and otel disagree between the two pages, so one of them is stale.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/content/architecture/workers.mdx` around lines 65 - 67, The defaults in
the Register Worker Options table are out of sync with the SDK reference; update
the table so the defaults for registerWorker options match the canonical values
in the SDK reference (specifically reconcile workerName, enableMetricsReporting,
and otel to the values shown in the iii-sdk reference) and scan the rest of the
option rows to ensure all defaults align; edit the table rows for the
registerWorker option keys to match the SDK's default values and add a brief
note or comment if any SDK default is intentionally different so future
reviewers can detect drift.
Comment on lines
+841
to
+842
| lowerLine.includes("registerWorker(") || | ||
| lowerLine.includes("register_worker(") || |
Contributor
There was a problem hiding this comment.
Lowercase the TypeScript matcher.
lowerLine is already normalized with toLowerCase(), so includes("registerWorker(") never matches. As written, TypeScript samples using registerWorker(...) won't be classified as architecture lines.
💡 Suggested fix
- lowerLine.includes("registerWorker(") ||
+ lowerLine.includes("registerworker(") ||
lowerLine.includes("register_worker(") ||📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| lowerLine.includes("registerWorker(") || | |
| lowerLine.includes("register_worker(") || | |
| lowerLine.includes("registerworker(") || | |
| lowerLine.includes("register_worker(") || |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@website/components/sections/DependencyVisualization.tsx` around lines 841 -
842, The check against lowerLine mistakenly uses a mixed-case TypeScript matcher
includes("registerWorker(") which will never match because lowerLine is already
toLowerCase(); replace that matcher with the lowercased form (e.g.,
"registerworker(") so the condition correctly detects TypeScript samples—update
the conditional that references lowerLine.includes("registerWorker(") to use the
lowercase string and keep the existing snake_case check
lowerLine.includes("register_worker(") intact.
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
init()→registerWorker(); PythonIII→register_worker(); RustIII::new()→register_worker()