-
Notifications
You must be signed in to change notification settings - Fork 1
feat: Migrate to one rust core crate #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
Conversation
|
@codex review |
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e651bc461
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| while !Task.isCancelled { | ||
| let current = try self.pollForStatus() | ||
| let current = self.pollStatus(pollIntervalMs: nil, timeoutMs: nil) | ||
|
|
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.
Use non-blocking status polling in async stream
In status(), the call to pollStatus(pollIntervalMs:timeoutMs:) blocks until the session completes (it loops internally until confirmed/failed in rust/core/src/bridge.rs), so the async stream will never emit intermediate waiting/awaiting updates and pollInterval/cancellation won’t be respected until the bridge finishes. This is a behavioral regression for callers who rely on incremental status updates or cancel the stream while waiting for a user to confirm; they’ll hang until the session completes or times out. Consider using a non-blocking single-poll API here, or exposing a pollForStatus wrapper that returns immediately and letting the Swift loop control intervals and cancellation.
Useful? React with 👍 / 👎.
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.
is this behavior (incremental status updates / cancellation) important? i don't have enough context to understand. @Takaros999
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.
yeah this is fine, it's how it worked before
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.
we can get an opinion from mobile devs later on but not important now
|
just one clarification question via codex. but otherwise no wired code smells on the rust stuff (still a rust noob so take that with some 🧂 ) |
0x1
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.
gucci
This PR continues the work in #36 and fixes CI and build issues.