Skip to content
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

feat(hub): add separate protocol for actor inspect #1946

Conversation

jog1t
Copy link
Contributor

@jog1t jog1t commented Jan 28, 2025

Closes ACTR-32

Discussion points

  • Current implementation vs. an additional layer of abstraction Actor > { ActorUser, ActorInspect )

Copy link

cloudflare-workers-and-pages bot commented Jan 28, 2025

Deploying rivet-hub with  Cloudflare Pages  Cloudflare Pages

Latest commit: 1e1fc74
Status: ✅  Deploy successful!
Preview URL: https://6fa5a81e.rivet-hub-7jb.pages.dev
Branch Preview URL: https://01-28-feat-add-separate-prot.rivet-hub-7jb.pages.dev

View logs

Copy link
Contributor Author

jog1t commented Jan 28, 2025

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

Copy link

linear bot commented Jan 28, 2025

@jog1t jog1t marked this pull request as ready for review January 28, 2025 01:02
Copy link

cloudflare-workers-and-pages bot commented Jan 28, 2025

Deploying rivet with  Cloudflare Pages  Cloudflare Pages

Latest commit: 1e1fc74
Status: ✅  Deploy successful!
Preview URL: https://1269109f.rivet.pages.dev
Branch Preview URL: https://01-28-feat-add-separate-prot.rivet.pages.dev

View logs

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

This PR introduces a separate protocol for actor inspection, significantly refactoring the actor communication and inspection mechanisms across the codebase.

  • Critical logical error in frontend/apps/hub/src/domains/project/components/actors/actors-actor-details.tsx where enabled={!data.destroyedAt || !data.endpoint} should likely be enabled={!data.destroyedAt && data.endpoint} to properly handle actor state
  • New ActorInspection class in sdks/actor/runtime/src/inspect.ts needs improved error handling for WebSocket connection failures and resource cleanup
  • Potential type safety issue in frontend/apps/hub/src/domains/project/queries/actors/query-options.ts with undocumented URL property using ts-ignore flags
  • Added [INSPECT_SYMBOL]() in sdks/actor/runtime/src/connection.ts improves encapsulation but should include documentation for symbol usage
  • New validateMessageEvent and handleMessageEvent functions in sdks/actor/runtime/src/event.ts need additional error cases for malformed messages

13 file(s) reviewed, 11 comment(s)
Edit PR Review Bot Settings | Greptile

sdks/actor/runtime/src/errors.ts Show resolved Hide resolved
sdks/actor/runtime/src/event.ts Outdated Show resolved Hide resolved
sdks/actor/runtime/src/event.ts Outdated Show resolved Hide resolved
sdks/actor/runtime/src/inspect.ts Outdated Show resolved Hide resolved
sdks/actor/runtime/src/inspect.ts Show resolved Hide resolved
sdks/actor/runtime/src/inspect.ts Show resolved Hide resolved
sdks/actor/runtime/src/log.ts Show resolved Hide resolved
sdks/actor/runtime/src/log.ts Show resolved Hide resolved
@jog1t jog1t force-pushed the 01-28-feat_add_separate_protocol_for_actor_inspect branch from 35845f3 to ef5887d Compare January 28, 2025 01:15
@jog1t jog1t changed the title feat: add separate protocol for actor inspect feat(hub): add separate protocol for actor inspect Jan 28, 2025
@NathanFlurry NathanFlurry force-pushed the 01-27-feat_add_more_options_to_create_actor_form branch from cc63118 to ac2e9de Compare January 28, 2025 05:20
@NathanFlurry NathanFlurry force-pushed the 01-28-feat_add_separate_protocol_for_actor_inspect branch from ef5887d to 1e1fc74 Compare January 28, 2025 05:39
@NathanFlurry
Copy link
Member

@greptileai

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

This PR continues the implementation of a separate protocol for actor inspection, focusing on the actor runtime and connection handling components.

  • Added throttling mechanism in sdks/actor/runtime/src/inspect.ts for state and connection change notifications to prevent excessive updates
  • Improved error handling in sdks/actor/runtime/src/actor.ts with new Unsupported error class for feature-specific failures
  • Refactored ActorWorkerContainer in frontend/apps/hub/src/domains/project/components/actors/worker/actor-worker-container.ts to use direct endpoints instead of manager URLs
  • Added dedicated inspectLogger in sdks/actor/runtime/src/log.ts for better debugging of inspection-related operations
  • Implemented proper cleanup in sdks/actor/runtime/src/actor.ts for WebSocket connections with a 1.5 second timeout

13 file(s) reviewed, 7 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines +44 to +48
<ActorWorkerContextProvider
enabled={!data.destroyedAt}
endpoint={data.endpoint}
{...props}
>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: endpoint prop is passed before spreading ...props which could lead to endpoint being overridden if it exists in props

{!size?.includes("icon") && isLoading ? null : (
<Slottable>{children}</Slottable>
)}
{isIcon && isLoading ? null : <Slottable>{children}</Slottable>}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: This change may have introduced a regression. Previously, children were hidden for non-icon buttons during loading (!size?.includes('icon') && isLoading). Now children are only hidden for icon buttons during loading (isIcon && isLoading). This means non-icon buttons will show both the loading spinner and children simultaneously.

@@ -150,6 +150,12 @@ export class StateTooLarge extends ActorError {
}
}

export class Unsupported extends ActorError {
constructor(feature: string) {
super("unsupported", `Unsupported feature: ${feature}`);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: consider using backticks around feature name for consistency with other error messages like InvalidProtocolVersion

throw new errors.Unsupported("RPC");
}

const { i: id, n: name, a: args = [] } = message.body.rr;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: No validation on RPC name or args array length. Could lead to security issues if malicious input is provided.

throw new errors.Unsupported("Subscriptions");
}

const { e: eventName, s: subscribe } = message.body.sr;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: No validation on eventName - should check for empty string or invalid characters that could cause issues.

rpcRequestId = id;

const ctx = new Rpc<A>(conn);
const output = await handlers.onExecuteRpc(ctx, name, args);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: No timeout handling for RPC execution. Long-running RPCs could block the event loop.

Comment on lines +131 to +139
if (error instanceof errors.ActorError && error.public) {
code = error.code;
message = String(error);
metadata = error.metadata;
} else {
code = errors.INTERNAL_ERROR_CODE;
message = errors.INTERNAL_ERROR_DESCRIPTION;
internal = true;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: Error handling could leak sensitive information if error.public is true but error.metadata contains sensitive data.

Copy link
Contributor

graphite-app bot commented Jan 28, 2025

Merge activity

  • Jan 28, 12:47 AM EST: A user added this pull request to the Graphite merge queue.
  • Jan 28, 12:48 AM EST: CI is running for this PR on a draft PR: #1949
  • Jan 28, 12:49 AM EST: A user merged this pull request with the Graphite merge queue via draft PR: #1949.

NathanFlurry pushed a commit that referenced this pull request Jan 28, 2025
<!-- Please make sure there is an issue that this PR is correlated to. -->

Closes ACTR-32

## Discussion points
- [ ] Current implementation vs. an additional layer of abstraction  `Actor > { ActorUser, ActorInspect )`

<!-- If there are frontend changes, please include screenshots. -->
@graphite-app graphite-app bot closed this Jan 28, 2025
@graphite-app graphite-app bot deleted the 01-28-feat_add_separate_protocol_for_actor_inspect branch January 28, 2025 05:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants