Skip to content

fix: promote queued follow-ups through native RPC - #67

Draft
andrebrait wants to merge 2 commits into
kahme247:mainfrom
andrebrait:fix/queued-message-steer
Draft

andrebrait wants to merge 2 commits into
kahme247:mainfrom
andrebrait:fix/queued-message-steer

Conversation

@andrebrait

@andrebrait andrebrait commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Replace client-only queue relabeling with OMP’s atomic promote_queued_message RPC command.
  • Relabel only after native acknowledgement; preserve the queue on refusal/error and handle overlapping clicks, delivery-before-acknowledgement, and navigation.
  • Restore the hook’s alive flag during effect setup so StrictMode does not silently disable promotion.
  • Document runtime compatibility and localize the unavailable-message notice.

Native dependency

Draft: do not merge until an OMP release provides promote_queued_message. The native implementation is can1357/oh-my-pi#11618. Released OMP 18.1.16 and 18.1.17 do not support the command. The combined changes were tested successfully in an isolated deployment. Unsupported runtimes produce an error rather than sending a duplicate steering message.

Verification

  • Regression reproduced before the fix: Steer changed local labels without sending any command. The StrictMode regression also failed with zero outgoing promotion requests before the lifecycle correction.
  • Actual browser → ompweb → source OMP smoke, using an isolated HOME/workspace and deterministic local model server: Queue → Steer received promoted: true, visibly changed to QUEUED STEER, and delivered the original correction exactly once with steering: true before background Bash completion.
  • Native interruption semantics are unchanged: Bash may be backgrounded to handle steering, not canceled.
  • No live user session, installed OMP package, or global executable was changed.
  • npm run release:check: typecheck, lint, 634 tests passed (one Windows-only test skipped on Linux), and production build passed.
  • Coordinated native verification: coding-agent package lint/format/typecheck passed; 53 focused queue, attachment, RPC, and client lifecycle tests passed. Existing unrelated unused-variable warning remains unchanged.

Review follow-up

Queue transitions now execute synchronously in event/effect handlers; React receives immutable queue snapshots, not replayable functional updaters with promotion side effects. The batched removal/ACK and delivery-before-ACK paths are covered under StrictMode.

@kahme247

Copy link
Copy Markdown
Owner

Review: blocked on the native command — not mergeable as-is

Thanks for the split-out work here. I reviewed d2b7aeb against main and verified the runtime situation locally before triaging.

Native dependency is genuinely missing

  • omp 18.1.16 (omp --version): the strings queuedMessageCount, follow_up and steer are present in the installed binary, promote_queued_message is not.
  • So on every current runtime the Steer action lands on the unavailable-message path you documented and nothing is promoted. That matches your Draft note; keeping it a Draft is the right call.

The web-side change itself reviews clean

  • The queue is now ref-serialized through updateQueuedMessages; no direct setQueuedMessages calls remain, and every reader/writer (queuedMessageCount === 0 drain, persisted-queue restore, handleSteer/handleFollowUp append) goes through it consistently.
  • Adding promote_queued_message to PASSTHROUGH_COMMANDS in lib/rpc-manager.ts is the right hook, and "relabel only after native acknowledgement" matches the no-duplicate-steering goal.

Asks

  1. Please split the runtime-independent parts into a PR that can land now:
    • the StrictMode lifecycle fix — this is a real latent bug, not just a test artifact. On main, hookAliveRef is useRef(true) and is only ever assigned false in the unmount cleanup, so React StrictMode's mount → cleanup → mount cycle leaves hookAliveRef.current === false and permanently disables every guarded path. Restoring true in the effect setup is worth landing independently of the new RPC command.
    • the queue serialization (updateQueuedMessages + promotion bookkeeping), if it stands alone without the native call.
    • the README/i18n documentation that does not mention promote_queued_message.
  2. Fail closed loudly: keep the unsupported-runtime path a clearly localized notice (no silent no-op, no duplicate message), and state the minimum OMP version in the description once the native command ships.
  3. Rebase and re-request review when the coordinated OMP change is released, and link the native PR from here.

Context: #64, #68 and #69 landed today, so inline delete confirmation and mobile Enter are already in main; queued-message promotion is the last queue gap. Nothing that depends on the missing command has been merged, and this PR is still a Draft and still MERGEABLE for whenever the runtime catches up.

@andrebrait

Copy link
Copy Markdown
Contributor Author

Native implementation: can1357/oh-my-pi#11618. The combined changes were tested successfully in the isolated /test deployment. This PR still depends on an OMP release containing promote_queued_message; OMP 18.1.16 does not support it.

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