Skip to content

refactor(client): extract poll_command_completion helper#45

Merged
BlindMaster24 merged 1 commit intomainfrom
devin/1776935603-extract-poll-command-completion
Apr 23, 2026
Merged

refactor(client): extract poll_command_completion helper#45
BlindMaster24 merged 1 commit intomainfrom
devin/1776935603-extract-poll-command-completion

Conversation

@devin-ai-integration
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot commented Apr 23, 2026

Summary

Three Client::*_and_wait sites duplicated the same ~50-line timeout
loop pattern: check cmd_id.is_ok() for immediate rejection, then
branch between a timeout_ms < 0 infinite poll(50) loop and a
deadline loop driven by a local wait_slice helper, matching terminal
CmdSuccess/CmdError for cmd_id and (for list-style waits)
accumulating data events (BannedUser, UserAccount, ...) along the
way. Every copy agreed today but with small stylistic divergences and
no shared test surface, so any future adjustment (e.g. honouring a new
terminal event, widening the no-op-timeout semantics, reworking how
CommandId::ZERO is reported) would silently drift between call
sites.

Hoist the shape into Client::poll_command_completion next to the
existing poll_until primitive. It is:

  • built on top of poll_until (which already handles timeout_ms < 0
    and positive deadlines uniformly);
  • parameterised by an error_context string used to rebuild the exact
    same error messages the old copies produced; and
  • parameterised by an FnMut(Event, &Message) hook so list-style
    callers can push into a local accumulator without the helper needing
    to know anything about BannedUser, UserAccount, etc.

Applied to:

  • Client::wait_for_command (no accumulator; passes |_, _| {}).
  • Client::list_bans_and_wait (BannedUser accumulator).
  • Client::list_user_accounts_and_wait (UserAccount accumulator).

Deliberately not touched in this PR:

  • join_channel_and_wait / login_and_wait - short-circuit on
    ConnectionState changes every iteration, not just on events.
    Folding them in cleanly requires a separate helper shape, or pushing
    the state check into poll_until.
  • update_server_and_wait / create_user_account_and_wait /
    delete_user_account_and_wait / wait_for_file_transfer /
    wait_for_file_transfer_terminal - return early on a data event
    rather than on CmdSuccess. Fits a different "first-match-wins"
    helper. Saving that for a follow-up PR to keep this one narrowly
    scoped.

The local wait_slice helpers in channels.rs, server.rs,
directory.rs, files.rs, and users/auth.rs are intentionally kept
as-is. They still have one or more non-converted callers each, and
removing them opportunistically would mask the fact that those sites
are next in the queue.

Structural only, no semantic change:

  • Error messages rebuilt verbatim (" rejected in current
    state" / " failed" / Error::Timeout) via
    format!("{error_context} ...").
  • cmd_id validation runs before any polling, matching the previous
    early-return behaviour bit-for-bit.
  • The poll_until predicate matches exactly the terminal set
    {CmdSuccess, CmdError} tied to cmd_id; non-terminal events flow
    through on_event in the same order as before.
  • poll_command_completion is pub(crate) - invisible to downstream
    crates, same surface as the previous private helpers.

Net diff: -81 / +20 lines in existing files, +55 lines in
client/core/runtime.rs (mostly doc comments on the new helper).

Local verification:

  • cargo fmt --all --check clean
  • cargo clippy --workspace --all-targets --all-features -- -D warnings clean
  • cargo test --workspace --all-features -> 287 passed / 0 failed

Review & Testing Checklist for Human

  • Skim crates/teamtalk/src/client/core/runtime.rs and confirm the
    new poll_command_completion preserves the original error
    strings bit-for-bit. In particular: "command rejected in current state" / "command failed" for wait_for_command, "ban list command rejected in current state" / "ban list command failed"
    for list_bans_and_wait, "user account list command rejected in current state" / "user account list command failed" for
    list_user_accounts_and_wait.
  • Confirm you are happy with the scope: three pure cmd-terminal
    sites converted; the five first-match-wins sites and the two
    state-aborting sites left for follow-up PRs. The alternative is
    one large PR that also introduces a poll_command_or_event
    helper and folds the state-aborting sites in; happy to do that if
    preferred.

Notes

This is PR 2 of the P0 structural refactor queue (PR 1 was #44, which
deduped can_issue_logged_in_command). Next up: either convert the
remaining wait_slice-driven sites to poll_until, or start splitting
the oversized modules (client/bus.rs, bot/storage.rs, bot/fsm.rs,
client/hooks/builders.rs, types/entities/media_common.rs, and the
client/backend*.rs pair) into sub-modules. Each will ship as its own
reviewable PR.

The pre-existing semver CI gate is expected to remain red here too;
release-plz handles the eventual version bump. Every other check is
expected to pass.

Link to Devin session: https://app.devin.ai/sessions/71fdd6196cb74723a2e277bb81993a9c
Requested by: @BlindMaster24


Open in Devin Review

Three command-dispatch sites duplicated the same 50+ line timeout loop
pattern: issue a command, check cmd_id.is_ok() for rejection, then spin
either an infinite poll(50) loop (timeout_ms < 0) or a deadline loop
with a local wait_slice helper, matching terminal CmdSuccess/CmdError
for cmd_id and (for list-style waits) accumulating data events along
the way.

Hoist that shared shape into Client::poll_command_completion next to
the existing poll_until primitive, so it is:

* built on top of poll_until (which already handles both the negative
  and positive timeout_ms cases uniformly), and
* parameterised by an error_context string for messages and an
  FnMut(Event, &Message) hook that lets callers accumulate non-terminal
  events (BannedUser, UserAccount, ...) without branching on timing.

Apply to:

* Client::wait_for_command (no accumulator; passes |_, _| {}).
* Client::list_bans_and_wait (BannedUser accumulator).
* Client::list_user_accounts_and_wait (UserAccount accumulator).

Other wait_for_* helpers (join_channel_and_wait, login_and_wait,
update_server_and_wait, create/delete_user_account_and_wait,
wait_for_file_transfer*) either terminate early on a data event rather
than on CmdSuccess, or short-circuit on connection-state changes. Those
have genuinely different semantics and stay on their current manual
loop; the wait_slice helpers in channels.rs, server.rs, directory.rs,
files.rs, and users/auth.rs are preserved for now. Future PRs can fold
them in with their own dedicated helper.

Structural only, no semantic change:

* Error messages are reconstructed verbatim ("<context> rejected in
  current state", "<context> failed", Error::Timeout) via
* cmd_id validation runs before any polling, matching the previous
  early-return behaviour.
* The predicate fed to poll_until matches exactly the terminal set
  {CmdSuccess, CmdError} tied to cmd_id, preserving the original
  accumulator/terminal ordering.

Net diff: -81 / +20 lines in existing files, +55 lines in
client/core/runtime.rs (mostly doc comments).

Local verification:
* cargo fmt --all --check clean
* cargo clippy --workspace --all-targets --all-features -- -D warnings clean
* cargo test --workspace --all-features -> 287 passed / 0 failed
@devin-ai-integration
Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

Copy link
Copy Markdown
Contributor Author

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

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

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 3 additional findings.

Open in Devin Review

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.

1 participant