-
Notifications
You must be signed in to change notification settings - Fork 3
Danny/kernel 297 docs mention timeouts for async invocation #43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Danny/kernel 297 docs mention timeouts for async invocation #43
Conversation
Add info about timeout of 15 minutes for async invocations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Performed full review of e765bee...6713198
Tip
⚡ Quick Actions
This review was generated by Mesa.
Actions:
Slash Commands:
/review- Request a full code review/review latest- Review only changes since the last review/describe- Generate PR description. This will update the PR body or issue comment depending on your configuration/help- Get help with Mesa commands and configuration options
3 files reviewed | 3 comments | Review on Mesa | Edit Reviewer Settings
|
|
||
| console.log(deployments); | ||
| // Automatically fetches more pages as needed. | ||
| for await (const deploymentListResponse of client.deployments.list()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code change appears inconsistent with other list API examples in this project. The for await...of loop assumes the client returns an async iterator, but other examples (like get-browsers.mdx, get-apps.mdx, get-profiles.mdx) use a simple await client.resource.list() pattern. This change breaks the consistency unless the deployments API specifically behaves differently from other list endpoints. Please verify this API behavior and ensure consistency across all list examples.
| console.log(deployments); | ||
| // Automatically fetches more pages as needed. | ||
| for await (const deploymentListResponse of client.deployments.list()) { | ||
| console.log(deploymentListResponse.id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accessing deploymentListResponse.id assumes each item in the iterator has an id property, but this should be deploymentListResponse represents individual deployment objects. The variable name suggests this might be a response object rather than a deployment. Consider renaming to deployment for clarity and verify the correct property access pattern.
| deployments = client.deployments.list() | ||
| print(deployments) | ||
| page = client.deployments.list() | ||
| page = page.items[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Python example has a logic error. Line 22 assigns the result of client.deployments.list() to page, but line 23 reassigns page to page.items[0] (assuming it's a paginated response with an items array). This overwrites the page object with a single item. If the goal is to access the first deployment's ID, this should be two separate variables: page = client.deployments.list() and deployment = page.items[0], then print(deployment.id).
|
|
||
| console.log(deployments); | ||
| // Automatically fetches more pages as needed. | ||
| for await (const deploymentListResponse of client.deployments.list()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the issue with making changes to this and is if we regenerate this, it'll be overwritten.
I think this auto-generated snippet list may be overkill and we may just revert back to manually written code snippets. It seems difficult to use and we have more custom code we write on our docs than the snippets we use...
masnwilliams
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Description
Added notes to the docs for 15 minute timeouts on async invocations.
Implementation Checklist
Testing
mintlify devworks (see installation here)Visual Proof
TL;DR
Docs updated to clarify the 15-minute timeout for asynchronous invocations.
Why we made these changes
To inform users about the execution limit for async invocations. This helps prevent unexpected failures for long-running tasks and allows users to design their applications accordingly.
What changed?
apps/invoke.mdx: Added an informational note about the 15-minute timeout.reference/cli/apps.mdx: Updated the CLI documentation to also mention the async timeout.snippets/openapi/get-deployments.mdx: Updated JavaScript/TypeScript and Python code snippets for listing deployments to improve clarity and better demonstrate handling paginated results.Description generated by Mesa. Update settings