lhumina_code/hero_archipelagos
1
0
Fork 0
Code Issues 50 Pull requests Projects Releases Packages Wiki Activity Actions

messaging: rapid send-button clicks silently drop messages #183

New issue
Closed
opened 2026-04-29 13:23:16 +00:00 by rawdaGastan · 4 comments
rawdaGastan commented 2026-04-29 13:23:16 +00:00
Member
Copy link

Problem

Firing 10 sequential clicks on .send-btn from the same chat persists 0 messages — the user sees no error and no failed-status indicator on any bubble. Effectively a silent-drop race.

Repro

In an open DM, evaluate in DevTools:

(() => {
  const el = document.querySelector('input.chat-input');
  const setter = Object.getOwnPropertyDescriptor(window.HTMLInputElement.prototype, 'value').set;
  const btn = document.querySelector('.send-btn');
  for (let i = 0; i < 10; i++) {
    setter.call(el, `rapid#${i}`);
    el.dispatchEvent(new Event('input', {bubbles:true}));
    btn.click();
  }
})();

Reload the page → 0 of 10 rapid#N messages persisted.

Where

archipelagos/messaging/src/islands/chat_input.rs — handle_send reads the local input_value signal and calls props.on_send.call(value).

archipelagos/messaging/src/archipelago.rs:281-322 — handle_send spawns an async send per click; sending: Signal<bool> is set/unset but never gates new sends. Concurrent in-flight send_message futures race on the same client.

Related (not duplicate)

Closed #48 fixed rapid chat-switch races (different scenario). The send-while-sending case is different and unguarded.

Suggested fix

At the start of handle_send in archipelago.rs: if *sending.peek() { return; } (and re-enable on completion). Also pass disabled: sending() into ChatInput so the button reflects state. The button's existing disabled-class wiring needs the DOM-level fix in the sibling Button-component issue.

Found via QA session 2026-04-29.

## Problem Firing 10 sequential clicks on `.send-btn` from the same chat persists 0 messages — the user sees no error and no failed-status indicator on any bubble. Effectively a silent-drop race. ## Repro In an open DM, evaluate in DevTools: ```js (() => { const el = document.querySelector('input.chat-input'); const setter = Object.getOwnPropertyDescriptor(window.HTMLInputElement.prototype, 'value').set; const btn = document.querySelector('.send-btn'); for (let i = 0; i < 10; i++) { setter.call(el, `rapid#${i}`); el.dispatchEvent(new Event('input', {bubbles:true})); btn.click(); } })(); ``` Reload the page → 0 of 10 `rapid#N` messages persisted. ## Where `archipelagos/messaging/src/islands/chat_input.rs` — `handle_send` reads the local `input_value` signal and calls `props.on_send.call(value)`. `archipelagos/messaging/src/archipelago.rs:281-322` — `handle_send` `spawn`s an async send per click; `sending: Signal<bool>` is set/unset but **never gates** new sends. Concurrent in-flight `send_message` futures race on the same client. ## Related (not duplicate) Closed #48 fixed *rapid chat-switch* races (different scenario). The send-while-sending case is different and unguarded. ## Suggested fix At the start of `handle_send` in `archipelago.rs`: `if *sending.peek() { return; }` (and re-enable on completion). Also pass `disabled: sending()` into `ChatInput` so the button reflects state. The button's existing disabled-class wiring needs the DOM-level fix in the sibling Button-component issue. Found via QA session 2026-04-29.
rawdaGastan added this to the ACTIVE project 2026-05-06 12:29:01 +00:00
rawdaGastan commented 2026-05-06 12:36:32 +00:00
Author
Member
Copy link

Implementation Spec — Issue #183: Rapid send-button clicks silently drop messages

Objective

Prevent concurrent send_message RPCs from racing when the user fires repeated clicks on the send button (or rapid Enter presses) by re-entrancy-guarding handle_send synchronously, before any task is spawned.

Background (current state, post PR #210)

  • archipelagos/messaging/src/archipelago.rs:504-564 — handle_send currently spawns an async task on every call. sending.set(true) and sending.set(false) already exist (lines 511 and 561) but are inside the spawned future, so all rapid clicks complete their synchronous portion (clone + spawn) before any future runs to flip sending. Result: N concurrent send_message futures share one CommunicationClient and most resolutions are silently lost.
  • archipelagos/messaging/src/islands/chat_input.rs:7-15 — ChatInputProps already has pub disabled: bool (line 11). Wired to the inner input (disabled: props.disabled on the <input>, line 76) and to the send Button (disabled: props.disabled || input_value().trim().is_empty(), line 85).
  • archipelagos/messaging/src/islands/chat_view.rs:39-40,109-116 — ChatViewProps.sending: bool already exists and is forwarded as ChatInput { disabled: props.sending, ... }.
  • archipelagos/messaging/src/archipelago.rs:728-731 and :786-789 — both layout branches (Desktop/Full and Mobile) already pass sending: *sending.read() into ChatView, so reactivity from the sending signal to the disabled-state already works.
  • core/src/components/button.rs:117-166 — Button uses a CSS-class disabled style (hero-btn-disabled) and an in-handler click-guard (if !is_disabled { props.onclick.call(e) }). Crucially, it does NOT set the HTML disabled attribute. The click-guard only fires after the parent has actually re-rendered with disabled = true. Confirms the bug: the parent's sending signal is not flipped synchronously inside the click handler, so consecutive clicks all see is_disabled == false.
  • Empty-input guard already exists in chat_input.rs:28 and :38. Enter-to-send (line 35) routes through the same props.on_send, so any parent-level gate on on_send automatically covers Enter.

Requirements (post-fix behaviors)

  • 10 sequential .send-btn clicks (or 10 Enter presses) from one chat invoke services::send_message exactly once for the first call; subsequent calls during in-flight are dropped at the parent before spawn.
  • After completion (Ok or Err), the next click works again — sending resets in both branches of the existing match.
  • The send button visibly reflects in-flight state via the existing hero-btn-disabled CSS class while sending == true.
  • The text input is disabled while sending == true (already wired via props.disabled on <input>).
  • Enter-to-send is also gated (no extra change required — it routes through the same on_send).
  • Persistence of own-message ids (PR #210) keeps working: the single accepted send still writes to own_message_ids and services::save_own_message_ids.

Files to modify

  • archipelagos/messaging/src/archipelago.rs — add a synchronous re-entrancy guard at the top of handle_send and flip sending to true synchronously before spawn.

No new files. No prop signature changes. No call-site changes — ChatView/ChatInput/Button all already accept and propagate the disabled bool.

Step-by-step implementation plan

Step 1 — Add synchronous re-entrancy guard in handle_send

File: archipelagos/messaging/src/archipelago.rs (current handle_send at lines 504-564).

Change the body of the move |content: String| closure so that:

  1. The very first action — before reading route, before any clones, before spawn — is if *sending.peek() { return; }.
  2. Immediately after the if let Route::Chat { sid, .. } = current_route match succeeds (so we don't latch sending when the route isn't a chat), call sending.set(true) synchronously, outside the spawn block.
  3. Remove the now-redundant sending.set(true) from inside the spawned future (currently line 511). Keep the existing sending.set(false) at the end of the future (line 561) — it correctly covers both Ok and Err paths because it's after the match.

Why: peek() reads the signal without subscribing the closure to it (no spurious re-renders), and the synchronous set(true) ensures the second click within the same event-loop tick observes true and bails. The async end-of-task set(false) already handles re-enabling on both success and failure.

Resulting shape (illustrative):

  • if *sending.peek() { return; } at the top of the closure.
  • Inside if let Route::Chat { sid, .. } = current_route { sending.set(true); spawn(async move { ... sending.set(false); }); }.

Dependencies: none.

Step 2 — Verify reactivity path is intact

No code change. Confirm by reading:

  • archipelago.rs:728-731 and :786-789 — both pass sending: *sending.read() to ChatView. The *sending.read() subscribes the parent's render to the signal, so the prop value flows on each flip.
  • chat_view.rs:109-116 — ChatInput { disabled: props.sending, ... }.
  • chat_input.rs:76,85 — both the input and the send Button are gated on props.disabled.
  • core/src/components/button.rs:120,138-142 — is_disabled = props.disabled || props.loading, click-guard inside onclick.

Result: once sending.set(true) runs synchronously in the parent, the next click within the same event tick is blocked by the parent guard; the subsequent re-render disables the button and input visually. Both rails are covered.

Acceptance criteria

  • In an open DM, run the issue's DevTools snippet: 
    (() => {
  const el = document.querySelector('input.chat-input');
  const setter = Object.getOwnPropertyDescriptor(window.HTMLInputElement.prototype, 'value').set;
  const btn = document.querySelector('.send-btn');
  for (let i = 0; i < 10; i++) {
    setter.call(el, `rapid#${i}`);
    el.dispatchEvent(new Event('input', {bubbles:true}));
    btn.click();
  }
})();
    Reload the page. Exactly 1 rapid#N message persists (typically rapid#0). No silent drops.
  • During the burst, the send button briefly shows the hero-btn-disabled style and the input is disabled until the in-flight send_message resolves.
  • After the resolved send, sending another single message works normally end-to-end.
  • Holding Enter (key-repeat) in the input behaves identically — no concurrent sends.
  • On a forced send failure, sending resets to false, the error banner appears, and a subsequent send is again possible.
  • The own_message_ids localStorage entry written by the successful send (PR #210 path) contains the one accepted message id and no orphans for the dropped clicks.

Notes / caveats

  • Compatibility with PR #210: the success path that calls own_message_ids.with_mut(...) and services::save_own_message_ids runs once per accepted send — unaffected by the gate, since the gate only blocks before spawn.
  • Why a click-guard inside Button is not enough: Button's click-guard only sees the prop value at render time. Since the parent does not flip sending until inside the spawned future, the rendered Button still has is_disabled=false for the entire synchronous burst. The fix must be at the parent because that is where the signal lives and where the synchronous transition has to occur.
  • Why we do not switch disabled to Signal<bool>: the codebase convention is plain bool (see ButtonProps.disabled: bool, ChatInputProps.disabled: bool, ChatViewProps.sending: bool). Reactivity is achieved by reading the signal at the parent's render (*sending.read()), which subscribes the parent's component to re-render. No prop-shape change is required.
  • Out of scope: the sibling Button-component DOM-level disabled issue alluded to in the issue (re-introducing a real HTML disabled attribute or otherwise hardening Button against synthetic .click() calls during the brief tick before re-render) is intentionally not addressed here. The parent-level guard is the correct and sufficient fix for #183.
  • Empty-input guard: not changed. The DevTools repro sets a non-empty value before each click, so the empty-guard would not have caught it; the new re-entrancy guard does.
  • Keyboard send: the Enter handler in chat_input.rs:34-43 calls props.on_send.call(value) — the same on_send that the parent gates. No separate change needed.
## Implementation Spec — Issue #183: Rapid send-button clicks silently drop messages ### Objective Prevent concurrent `send_message` RPCs from racing when the user fires repeated clicks on the send button (or rapid Enter presses) by re-entrancy-guarding `handle_send` synchronously, before any task is spawned. ### Background (current state, post PR #210) - `archipelagos/messaging/src/archipelago.rs:504-564` — `handle_send` currently `spawn`s an async task on every call. `sending.set(true)` and `sending.set(false)` already exist (lines 511 and 561) but are inside the spawned future, so all rapid clicks complete their synchronous portion (clone + `spawn`) before any future runs to flip `sending`. Result: N concurrent `send_message` futures share one `CommunicationClient` and most resolutions are silently lost. - `archipelagos/messaging/src/islands/chat_input.rs:7-15` — `ChatInputProps` already has `pub disabled: bool` (line 11). Wired to the inner input (`disabled: props.disabled` on the `<input>`, line 76) and to the send Button (`disabled: props.disabled || input_value().trim().is_empty()`, line 85). - `archipelagos/messaging/src/islands/chat_view.rs:39-40,109-116` — `ChatViewProps.sending: bool` already exists and is forwarded as `ChatInput { disabled: props.sending, ... }`. - `archipelagos/messaging/src/archipelago.rs:728-731` and `:786-789` — both layout branches (Desktop/Full and Mobile) already pass `sending: *sending.read()` into `ChatView`, so reactivity from the `sending` signal to the disabled-state already works. - `core/src/components/button.rs:117-166` — `Button` uses a CSS-class disabled style (`hero-btn-disabled`) and an in-handler click-guard (`if !is_disabled { props.onclick.call(e) }`). Crucially, it does NOT set the HTML `disabled` attribute. The click-guard only fires after the parent has actually re-rendered with `disabled = true`. Confirms the bug: the parent's `sending` signal is not flipped synchronously inside the click handler, so consecutive clicks all see `is_disabled == false`. - Empty-input guard already exists in `chat_input.rs:28` and `:38`. Enter-to-send (line 35) routes through the same `props.on_send`, so any parent-level gate on `on_send` automatically covers Enter. ### Requirements (post-fix behaviors) - 10 sequential `.send-btn` clicks (or 10 Enter presses) from one chat invoke `services::send_message` exactly once for the first call; subsequent calls during in-flight are dropped at the parent before `spawn`. - After completion (Ok or Err), the next click works again — `sending` resets in both branches of the existing match. - The send button visibly reflects in-flight state via the existing `hero-btn-disabled` CSS class while `sending == true`. - The text input is disabled while `sending == true` (already wired via `props.disabled` on `<input>`). - Enter-to-send is also gated (no extra change required — it routes through the same `on_send`). - Persistence of own-message ids (PR #210) keeps working: the single accepted send still writes to `own_message_ids` and `services::save_own_message_ids`. ### Files to modify - `archipelagos/messaging/src/archipelago.rs` — add a synchronous re-entrancy guard at the top of `handle_send` and flip `sending` to `true` synchronously before `spawn`. No new files. No prop signature changes. No call-site changes — `ChatView`/`ChatInput`/`Button` all already accept and propagate the disabled bool. ### Step-by-step implementation plan #### Step 1 — Add synchronous re-entrancy guard in `handle_send` File: `archipelagos/messaging/src/archipelago.rs` (current `handle_send` at lines 504-564). Change the body of the `move |content: String|` closure so that: 1. The very first action — before reading `route`, before any `clone`s, before `spawn` — is `if *sending.peek() { return; }`. 2. Immediately after the `if let Route::Chat { sid, .. } = current_route` match succeeds (so we don't latch `sending` when the route isn't a chat), call `sending.set(true)` synchronously, *outside* the `spawn` block. 3. Remove the now-redundant `sending.set(true)` from inside the spawned future (currently line 511). Keep the existing `sending.set(false)` at the end of the future (line 561) — it correctly covers both Ok and Err paths because it's after the `match`. Why: `peek()` reads the signal without subscribing the closure to it (no spurious re-renders), and the synchronous `set(true)` ensures the second click within the same event-loop tick observes `true` and bails. The async end-of-task `set(false)` already handles re-enabling on both success and failure. Resulting shape (illustrative): - `if *sending.peek() { return; }` at the top of the closure. - Inside `if let Route::Chat { sid, .. } = current_route { sending.set(true); spawn(async move { ... sending.set(false); }); }`. Dependencies: none. #### Step 2 — Verify reactivity path is intact No code change. Confirm by reading: - `archipelago.rs:728-731` and `:786-789` — both pass `sending: *sending.read()` to `ChatView`. The `*sending.read()` subscribes the parent's render to the signal, so the prop value flows on each flip. - `chat_view.rs:109-116` — `ChatInput { disabled: props.sending, ... }`. - `chat_input.rs:76,85` — both the input and the send Button are gated on `props.disabled`. - `core/src/components/button.rs:120,138-142` — `is_disabled = props.disabled || props.loading`, click-guard inside `onclick`. Result: once `sending.set(true)` runs synchronously in the parent, the next click within the same event tick is blocked by the parent guard; the subsequent re-render disables the button and input visually. Both rails are covered. ### Acceptance criteria - [ ] In an open DM, run the issue's DevTools snippet: ```js (() => { const el = document.querySelector('input.chat-input'); const setter = Object.getOwnPropertyDescriptor(window.HTMLInputElement.prototype, 'value').set; const btn = document.querySelector('.send-btn'); for (let i = 0; i < 10; i++) { setter.call(el, `rapid#${i}`); el.dispatchEvent(new Event('input', {bubbles:true})); btn.click(); } })(); ``` Reload the page. Exactly 1 `rapid#N` message persists (typically `rapid#0`). No silent drops. - [ ] During the burst, the send button briefly shows the `hero-btn-disabled` style and the input is disabled until the in-flight `send_message` resolves. - [ ] After the resolved send, sending another single message works normally end-to-end. - [ ] Holding Enter (key-repeat) in the input behaves identically — no concurrent sends. - [ ] On a forced send failure, `sending` resets to `false`, the error banner appears, and a subsequent send is again possible. - [ ] The `own_message_ids` localStorage entry written by the successful send (PR #210 path) contains the one accepted message id and no orphans for the dropped clicks. ### Notes / caveats - **Compatibility with PR #210**: the success path that calls `own_message_ids.with_mut(...)` and `services::save_own_message_ids` runs once per accepted send — unaffected by the gate, since the gate only blocks before `spawn`. - **Why a click-guard inside Button is not enough**: `Button`'s click-guard only sees the prop value at render time. Since the parent does not flip `sending` until *inside* the spawned future, the rendered Button still has `is_disabled=false` for the entire synchronous burst. The fix must be at the parent because that is where the signal lives and where the synchronous transition has to occur. - **Why we do not switch `disabled` to `Signal<bool>`**: the codebase convention is plain `bool` (see `ButtonProps.disabled: bool`, `ChatInputProps.disabled: bool`, `ChatViewProps.sending: bool`). Reactivity is achieved by reading the signal at the parent's render (`*sending.read()`), which subscribes the parent's component to re-render. No prop-shape change is required. - **Out of scope**: the sibling Button-component DOM-level `disabled` issue alluded to in the issue (re-introducing a real HTML `disabled` attribute or otherwise hardening Button against synthetic `.click()` calls during the brief tick before re-render) is intentionally not addressed here. The parent-level guard is the correct and sufficient fix for #183. - **Empty-input guard**: not changed. The DevTools repro sets a non-empty value before each click, so the empty-guard would not have caught it; the new re-entrancy guard does. - **Keyboard send**: the Enter handler in `chat_input.rs:34-43` calls `props.on_send.call(value)` — the same `on_send` that the parent gates. No separate change needed.
rawdaGastan commented 2026-05-06 12:40:09 +00:00
Author
Member
Copy link

Test Results

Crate: hero_archipelagos_messaging (cargo test --lib)

  • Total: 5
  • Passed: 5
  • Failed: 0

Tests run

test services::messaging_service::chatmessage_conversion_tests::test_other_party_in_dm_remains_other ... ok
test services::messaging_service::chatmessage_conversion_tests::test_real_key_takes_precedence ... ok
test services::messaging_service::chatmessage_conversion_tests::test_own_id_match_returns_is_own_true ... ok
test services::messaging_service::own_message_ids_tests::test_insert_dedupes ... ok
test services::messaging_service::own_message_ids_tests::test_insert_fifo_cap ... ok

test result: ok. 5 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

These are the tests landed by PR #210; this fix is a pure structural change to handle_send (lift sending.set(true) out of the spawned future and add a synchronous if *sending.peek() { return; } at the top) and does not change any pure function under test, so no new unit tests were added.

Builds

  • cargo check -p hero_archipelagos_messaging: clean.
  • cargo check -p hero_archipelagos_messaging --target wasm32-unknown-unknown: clean.
  • cargo check --workspace --exclude hero_archipelagos_messaging: clean (9 unused_mut warnings in server/src/main.rs are pre-existing on development and unrelated to this fix).

Why no new unit tests

The race is across event-loop ticks: the first click's synchronous closure body runs, then control returns to the runtime, then the spawned future is polled and starts the RPC. Subsequent clicks fire their synchronous body before the future has had a chance to flip sending. Reproducing this in a cargo test would require a Dioxus runtime harness with a controllable executor — a unit test against Signal::set/peek adds no signal beyond what the type's existing tests already cover.

A live browser repro using the issue's DevTools snippet is the right verification rail. Live-test results to follow in the implementation summary.

## Test Results **Crate:** `hero_archipelagos_messaging` (`cargo test --lib`) - Total: 5 - Passed: 5 - Failed: 0 ### Tests run ``` test services::messaging_service::chatmessage_conversion_tests::test_other_party_in_dm_remains_other ... ok test services::messaging_service::chatmessage_conversion_tests::test_real_key_takes_precedence ... ok test services::messaging_service::chatmessage_conversion_tests::test_own_id_match_returns_is_own_true ... ok test services::messaging_service::own_message_ids_tests::test_insert_dedupes ... ok test services::messaging_service::own_message_ids_tests::test_insert_fifo_cap ... ok test result: ok. 5 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out ``` These are the tests landed by PR #210; this fix is a pure structural change to `handle_send` (lift `sending.set(true)` out of the spawned future and add a synchronous `if *sending.peek() { return; }` at the top) and does not change any pure function under test, so no new unit tests were added. ### Builds - `cargo check -p hero_archipelagos_messaging`: clean. - `cargo check -p hero_archipelagos_messaging --target wasm32-unknown-unknown`: clean. - `cargo check --workspace --exclude hero_archipelagos_messaging`: clean (9 `unused_mut` warnings in `server/src/main.rs` are pre-existing on `development` and unrelated to this fix). ### Why no new unit tests The race is across event-loop ticks: the first click's synchronous closure body runs, then control returns to the runtime, then the spawned future is polled and starts the RPC. Subsequent clicks fire their synchronous body before the future has had a chance to flip `sending`. Reproducing this in a `cargo test` would require a Dioxus runtime harness with a controllable executor — a unit test against `Signal::set`/`peek` adds no signal beyond what the type's existing tests already cover. A live browser repro using the issue's DevTools snippet is the right verification rail. Live-test results to follow in the implementation summary.
rawdaGastan commented 2026-05-06 12:47:47 +00:00
Author
Member
Copy link

Implementation Summary

Branch: development_fix_send_reentrancy_race

Approach

Added a single synchronous re-entrancy guard at the top of handle_send in archipelagos/messaging/src/archipelago.rs, and lifted the existing sending.set(true) out of the spawned future so it runs synchronously before spawn. The bug was that sending.set(true) ran inside the spawned async block, so 10 rapid clicks all completed their synchronous closure body (clone + spawn) before any future was polled — N concurrent send_message futures then raced on the same CommunicationClient and most resolutions were silently dropped.

The fix runs entirely at the parent. Button's in-handler click-guard cannot rescue this case because it only sees the prop value at render time, and the re-render does not happen until after the parent flips sending — which the buggy code only did inside the future.

Files changed

  • archipelagos/messaging/src/archipelago.rs — handle_send closure: if *sending.peek() { return; } at the top; sending.set(true) moved synchronously before spawn (inside the if let Route::Chat { sid, .. } branch); the original sending.set(false) at the end of the future is unchanged and continues to cover both Ok and Err.
 archipelagos/messaging/src/archipelago.rs | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Why no new unit tests

The race is across event-loop ticks (synchronous closure body vs. spawned future polling). Reproducing it in a cargo test would require a Dioxus runtime harness with a controllable executor; a unit test against Signal::set/Signal::peek would just retest the type's existing semantics. Live browser repro using the issue's DevTools snippet is the right verification rail — see below.

Verification

  • cargo test -p hero_archipelagos_messaging --lib: 5/5 passed (PR #210 tests still green).
  • cargo check -p hero_archipelagos_messaging: clean.
  • cargo check -p hero_archipelagos_messaging --target wasm32-unknown-unknown: clean.
  • cargo check --workspace --exclude hero_archipelagos_messaging: clean (9 unused_mut warnings in server/src/main.rs are pre-existing on development and unrelated).
  • Live verified in browser (make run -> http://localhost:8886 -> messaging DM): pasted the issue's 10-click DevTools snippet, observed exactly 1 message bubble appear, F5, re-opened the chat, exactly 1 of 10 rapid#N messages persisted (vs 0 before the fix). Send button reflected the disabled state during the in-flight RPC. Subsequent single send worked normally.

Notes

  • Compatibility with PR #210: the own_message_ids insert + services::save_own_message_ids calls (PR #210 path inside the spawned future) are unchanged and run exactly once for the accepted send. No orphan SIDs from dropped clicks because the guard returns before any state mutation.
  • Empty-input guard is unchanged. The DevTools repro sets a non-empty value before each click, so the existing empty-guard would not have caught this — the new re-entrancy guard does.
  • Enter-to-send is also covered: chat_input.rs routes Enter through the same props.on_send, which is the gated handle_send.

Out of scope

  • The sibling Button-component DOM-level disabled issue mentioned in the report (re-introducing the HTML disabled attribute or otherwise hardening Button against synthetic .click() calls during the brief tick before re-render) is not addressed here. The parent-level guard is sufficient and correct for #183. The Button hardening, if desired, should be tracked under that sibling issue.
## Implementation Summary Branch: `development_fix_send_reentrancy_race` ### Approach Added a single synchronous re-entrancy guard at the top of `handle_send` in `archipelagos/messaging/src/archipelago.rs`, and lifted the existing `sending.set(true)` out of the spawned future so it runs synchronously before `spawn`. The bug was that `sending.set(true)` ran *inside* the spawned async block, so 10 rapid clicks all completed their synchronous closure body (clone + `spawn`) before any future was polled — N concurrent `send_message` futures then raced on the same `CommunicationClient` and most resolutions were silently dropped. The fix runs entirely at the parent. `Button`'s in-handler click-guard cannot rescue this case because it only sees the prop value at render time, and the re-render does not happen until *after* the parent flips `sending` — which the buggy code only did inside the future. ### Files changed - `archipelagos/messaging/src/archipelago.rs` — `handle_send` closure: `if *sending.peek() { return; }` at the top; `sending.set(true)` moved synchronously before `spawn` (inside the `if let Route::Chat { sid, .. }` branch); the original `sending.set(false)` at the end of the future is unchanged and continues to cover both Ok and Err. ``` archipelagos/messaging/src/archipelago.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) ``` ### Why no new unit tests The race is across event-loop ticks (synchronous closure body vs. spawned future polling). Reproducing it in a `cargo test` would require a Dioxus runtime harness with a controllable executor; a unit test against `Signal::set`/`Signal::peek` would just retest the type's existing semantics. Live browser repro using the issue's DevTools snippet is the right verification rail — see below. ### Verification - `cargo test -p hero_archipelagos_messaging --lib`: 5/5 passed (PR #210 tests still green). - `cargo check -p hero_archipelagos_messaging`: clean. - `cargo check -p hero_archipelagos_messaging --target wasm32-unknown-unknown`: clean. - `cargo check --workspace --exclude hero_archipelagos_messaging`: clean (9 `unused_mut` warnings in `server/src/main.rs` are pre-existing on `development` and unrelated). - **Live verified in browser** (`make run` -> `http://localhost:8886` -> messaging DM): pasted the issue's 10-click DevTools snippet, observed exactly 1 message bubble appear, F5, re-opened the chat, exactly 1 of 10 `rapid#N` messages persisted (vs 0 before the fix). Send button reflected the disabled state during the in-flight RPC. Subsequent single send worked normally. ### Notes - **Compatibility with PR #210**: the `own_message_ids` insert + `services::save_own_message_ids` calls (PR #210 path inside the spawned future) are unchanged and run exactly once for the accepted send. No orphan SIDs from dropped clicks because the guard returns before any state mutation. - **Empty-input guard** is unchanged. The DevTools repro sets a non-empty value before each click, so the existing empty-guard would not have caught this — the new re-entrancy guard does. - **Enter-to-send** is also covered: `chat_input.rs` routes Enter through the same `props.on_send`, which is the gated `handle_send`. ### Out of scope - The sibling Button-component DOM-level disabled issue mentioned in the report (re-introducing the HTML `disabled` attribute or otherwise hardening Button against synthetic `.click()` calls during the brief tick before re-render) is not addressed here. The parent-level guard is sufficient and correct for #183. The Button hardening, if desired, should be tracked under that sibling issue.
rawdaGastan commented 2026-05-06 12:50:21 +00:00
Author
Member
Copy link

Pull request opened: #211

This PR implements the changes discussed in this issue.

Pull request opened: https://forge.ourworld.tf/lhumina_code/hero_archipelagos/pulls/211 This PR implements the changes discussed in this issue.
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
development
main
No results found.
Labels
Clear labels   
prio_critical

   
prio_low

   
type_bug

   
type_contact

   
type_issue

   
type_lead

   
type_question

   
type_story

   
type_task
No labels
prio_critical
prio_low
type_bug
type_contact
type_issue
type_lead
type_question
type_story
type_task
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
ACTIVE
Assignees
Clear assignees
No assignees
rawdaGastan
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
lhumina_code/hero_archipelagos#183
No description provided.