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

messaging: after page reload, all bubbles render as bubble-other (post-reload regression of #62) #192

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

Summary

Post-reload regression of #62 (closed). The optimistic-send patch added in archipelago.rs:293-296 only forces is_own = true on the just-sent message; after F5 every message in the thread (including ones the user just sent) renders left-aligned as bubble-other — there is no visual signal of which side of the conversation you are on.

This was acknowledged as a known limitation in the original #62 body ("subsequent full refetches will still show the server-stamped sender") but was closed without follow-up. Filing this as a separate ticket so it stays on the radar.

Repro

  1. /space/default/messaging, create DM with QA Tester.
  2. Send a message → bubble-own ✓ (correct, optimistic patch fires).
  3. F5.
  4. Re-open the same chat — own message now bubble-other (left-aligned, no ✓).

DevTools confirms:

Array.from(document.querySelectorAll('[class*="message-bubble"]'))
     .map(b => b.className);
// → all bubbles have "bubble-other" after reload

Root cause (per the code itself)

// archipelagos/messaging/src/archipelago.rs:293-296
// Server hardcodes sender_public_key = "system" until auth lands, so the
// is_own check can't match. We know we just sent this — render as own.
msg_data.is_own = true;
msg_data.sender_name = "Me".to_string();

The patch only runs in handle_send. Reloading drops active_messages and refetches from the server — every historical message comes back with sender_public_key = "system" and the patch is gone.

Suggested fix (any one)

  • Persist own_user_key per context (so refetch can match), OR
  • Until auth lands, treat sender_public_key == SYSTEM_KEY as is_own = true whenever own_user_key is empty/system, OR
  • Have the server stamp sender_public_key with the X-Hero-Context value (depends on the cross-context fix — see hero_osis #40).

Related

  • #62 (closed) — original optimistic-send fix.
  • #51 (closed) — sibling: empty user_key rendered all messages as 'Me'. The current fix flipped it the other way.
  • hero_osis #40 — server-side context isolation, related root cause.

Found via QA session 2026-04-29 (qa/messaging_2026-04-29/FINDINGS.md).

## Summary Post-reload regression of [#62 (closed)](https://forge.ourworld.tf/lhumina_code/hero_archipelagos/issues/62). The optimistic-send patch added in `archipelago.rs:293-296` only forces `is_own = true` on the *just-sent* message; after F5 every message in the thread (including ones the user just sent) renders left-aligned as `bubble-other` — there is no visual signal of which side of the conversation you are on. This was acknowledged as a known limitation in the original #62 body ("subsequent full refetches will still show the server-stamped sender") but was closed without follow-up. Filing this as a separate ticket so it stays on the radar. ## Repro 1. `/space/default/messaging`, create DM with QA Tester. 2. Send a message → bubble-own ✓ (correct, optimistic patch fires). 3. F5. 4. Re-open the same chat — own message now bubble-other (left-aligned, no ✓). DevTools confirms: ```js Array.from(document.querySelectorAll('[class*="message-bubble"]')) .map(b => b.className); // → all bubbles have "bubble-other" after reload ``` ## Root cause (per the code itself) ```rust // archipelagos/messaging/src/archipelago.rs:293-296 // Server hardcodes sender_public_key = "system" until auth lands, so the // is_own check can't match. We know we just sent this — render as own. msg_data.is_own = true; msg_data.sender_name = "Me".to_string(); ``` The patch only runs in `handle_send`. Reloading drops `active_messages` and refetches from the server — every historical message comes back with `sender_public_key = "system"` and the patch is gone. ## Suggested fix (any one) - Persist `own_user_key` per context (so refetch can match), OR - Until auth lands, treat `sender_public_key == SYSTEM_KEY` as `is_own = true` whenever own_user_key is empty/`system`, OR - Have the server stamp `sender_public_key` with the `X-Hero-Context` value (depends on the cross-context fix — see hero_osis #40). ## Related - [#62 (closed)](https://forge.ourworld.tf/lhumina_code/hero_archipelagos/issues/62) — original optimistic-send fix. - [#51 (closed)](https://forge.ourworld.tf/lhumina_code/hero_archipelagos/issues/51) — sibling: empty user_key rendered all messages as 'Me'. The current fix flipped it the other way. - [hero_osis #40](https://forge.ourworld.tf/lhumina_code/hero_osis/issues/40) — server-side context isolation, related root cause. Found via QA session 2026-04-29 (`qa/messaging_2026-04-29/FINDINGS.md`).
rawdaGastan added this to the ACTIVE project 2026-05-04 13:54:49 +00:00
rawdaGastan commented 2026-05-06 11:11:08 +00:00
Author
Member
Copy link

Implementation Spec — Issue #192

Objective

After page reload, messages the local user authored continue to render as bubble-own by re-applying the optimistic ownership mark from a per-context, persisted set of locally-authored message IDs — eliminating the regression where every bubble appears as bubble-other post-F5.

Requirements

  • After F5 in /space/default/messaging, opening a chat that the user has previously sent messages to MUST render those messages right-aligned with message-own / bubble-own and the "Me" label, identical to how they appeared pre-reload.
  • Messages from the other party in the same DM/group MUST continue to render as bubble-other (no false-positive on the "everyone is system" case).
  • Sending a new message MUST keep working exactly as today (optimistic patch in handle_send is preserved) and MUST also seed the persisted set so the message survives reload.
  • Once authentication lands and the server stamps a real sender_public_key, the existing is_own = !own_user_key.is_empty() && msg.sender_public_key == own_user_key check MUST take precedence, so the persisted-IDs path becomes a no-op and can be retired without UI flicker.
  • The persisted set MUST be scoped per context_name so that switching contexts doesn't leak ownership across tenants.
  • Bound on persisted-set growth: cap entries per context (FIFO trim) so localStorage doesn't grow unboundedly.
  • Native (non-wasm) builds MUST compile and behave equivalently with persistence as a no-op (the messaging archipelago is cdylib + rlib and is built native by the workspace).

Files to modify/create

  • archipelagos/messaging/src/services/messaging_service.rs — extend chatmessage_to_messagedata to accept a "locally-authored IDs" lookup; expose SYSTEM_KEY / is_self_or_system as pub(crate) for reuse; add a small OwnMessageIds helper module with cfg-gated localStorage persistence.
  • archipelagos/messaging/src/archipelago.rs — load the persisted ID set on mount, pass it into every chatmessage_to_messagedata call site (initial fetch, SSE Resync, SSE ChatmessageNew, send path), persist on every successful handle_send, and update the existing optimistic patch to also write through to the store.
  • (No new files required if the helper lives in messaging_service.rs. If preferred for clarity, create archipelagos/messaging/src/services/own_messages.rs and re-export from services/mod.rs.)

Step-by-step implementation plan

Step 1 — Add an OwnMessageIds helper

File: archipelagos/messaging/src/services/messaging_service.rs (or new services/own_messages.rs).

Introduce:

  • pub struct OwnMessageIds { ids: HashSet<String>, order: Vec<String> } with contains(&str) -> bool, insert(String) (FIFO-cap aware), len(), and a const MAX_OWN_IDS_PER_CONTEXT: usize = 5_000.
  • pub fn load_own_message_ids(context_name: &str) -> OwnMessageIds:
    • On wasm32: read hero_messaging_own_msg_ids:<context_name> from localStorage, parse as JSON Vec<String>, dedup into the set + order vec. Return empty on any error.
    • On non-wasm: return empty.
  • pub fn save_own_message_ids(context_name: &str, ids: &OwnMessageIds):
    • On wasm32: serialize ordered ids as JSON and write to localStorage. On non-wasm: no-op.
  • Storage key constant: const OWN_IDS_STORAGE_PREFIX: &str = "hero_messaging_own_msg_ids:";

Reuse the cfg-gated storage_get / storage_set pattern from archipelagos/intelligence/ai/src/island.rs:17-51.

Why: Centralizes persistence and keeps the rest of messaging code agnostic to the backing store. Per-context_name keying isolates tenants.

Dependencies: None.

Step 2 — Extend chatmessage_to_messagedata to consult the local-author set

File: archipelagos/messaging/src/services/messaging_service.rs, lines 349-375.

Change signature to:

pub fn chatmessage_to_messagedata(
    msg: &ChatMessage,
    own_user_key: &str,
    contacts: &[Contact],
    own_ids: &OwnMessageIds,
) -> MessageData

Replace the is_own computation with the layered check (first match wins):

let is_own = if !own_user_key.is_empty() && msg.sender_public_key == own_user_key {
    true
} else {
    own_ids.contains(&msg.sid.to_string())
};

let sender_name = if is_own {
    "Me".to_string()
} else {
    resolve_sender_name(&msg.sender_public_key, own_user_key, contacts)
};

Why: Single conversion seam for every fetched/streamed message — every code path (initial fetch, SSE live event, SSE Resync) automatically picks it up. No scattered patching.

Dependencies: Step 1.

Step 3 — Wire OwnMessageIds through archipelago.rs

File: archipelagos/messaging/src/archipelago.rs.

a. In MessagingArchipelagoApp, after the existing signals (around lines 92-106), add:

let mut own_message_ids = use_signal(|| services::load_own_message_ids(&context_name));

b. Update every call to services::chatmessage_to_messagedata to pass &own_message_ids.peek() (or .read() where reactive re-derive matters):

  • Line 193 (ChatEvent::ChatmessageNew handler).
  • Line 303 (Resync refetch).
  • Line 427 (chat-load effect) — use .read() so the messages list re-derives if the set updates.
  • Line 508 (handle_send).

c. In handle_send (around the existing optimistic patch on lines 511-512), after the message is successfully sent:

let id = msg.sid.to_string();
own_message_ids.with_mut(|set| set.insert(id.clone()));
services::save_own_message_ids(&ctx, &own_message_ids.peek());

Keep the existing msg_data.is_own = true; msg_data.sender_name = "Me" lines.

d. In the SSE ChatmessageNew handler (lines 191-205), no extra logic is needed — the conversion in Step 2 will return is_own = true if the sid is in the set.

Why: Threads the local-author signal through every conversion site and persists on send. Dedup remains in the existing pipeline.

Dependencies: Steps 1 and 2.

Step 4 — Make SYSTEM_KEY / is_self_or_system reusable (optional cleanup)

File: archipelagos/messaging/src/services/messaging_service.rs.

Mark SYSTEM_KEY and is_self_or_system as pub(crate). Skip if no callers consume them outside the file.

Why: Avoids future duplication once auth lands.

Dependencies: None.

Step 5 — Tests

Add a #[cfg(test)] mod tests block in messaging_service.rs:

  • test_own_id_match_returns_is_own_true — sender_public_key = "system", sid = "msg-42", own_user_key = "", OwnMessageIds contains "msg-42". Assert is_own == true, sender_name == "Me".
  • test_other_party_in_dm_remains_other — same but sid NOT in set. Assert is_own == false. (DM false-positive guard.)
  • test_real_key_takes_precedence — own_user_key = "0xabc", msg.sender_public_key = "0xabc", empty own_ids. Assert is_own == true (forward-compat once auth lands).
  • test_own_message_ids_capacity_trims_oldest — insert > MAX_OWN_IDS_PER_CONTEXT, assert cap and FIFO eviction.

Native-only (cargo test -p hero_archipelagos_messaging); OwnMessageIds must be constructable directly in tests without localStorage.

Why: Locks in the DM regression guard and proves the fix works without manual reload.

Dependencies: Steps 1 and 2.

Step 6 — Manual repro verification

  1. Build wasm bundle for messaging.
  2. /space/default/messaging, create DM with QA Tester, send 3 messages.
  3. Confirm bubbles render right-aligned (bubble-own).
  4. F5.
  5. Re-open same chat.
  6. Confirm the 3 messages still render bubble-own with "Me" label and status checkmark.
  7. DevTools: localStorage["hero_messaging_own_msg_ids:default"] is a JSON array of 3 SIDs.
  8. Have a second user post a message in that DM — confirm it renders as bubble-other.
  9. Switch to a different context_name, confirm no leak.

Acceptance criteria

  • After F5, previously sent messages render bubble-own (right-aligned, blue, status icon, "Me" label).
  • Messages from the other party in a DM remain bubble-other, including when the server stamps both as "system".
  • Newly sent messages still render optimistically as bubble-own without waiting for the SSE round-trip (existing #62 behavior preserved).
  • localStorage entry under hero_messaging_own_msg_ids:<context> exists, is JSON, and grows by one entry per successful send (capped at MAX_OWN_IDS_PER_CONTEXT).
  • Switching context_name does not leak own-message IDs across tenants.
  • Unit tests pass: cargo test -p hero_archipelagos_messaging covers the four cases in Step 5.
  • Native build and wasm build both succeed.
  • When a real own_user_key is provided (forward-compat with auth), the existing key-match path takes precedence — verified by test_real_key_takes_precedence.

Notes

  • Out of scope: server-side fix to stamp sender_public_key with the X-Hero-Context value (option C in the issue) — tracked in hero_osis #40.
  • Future-work pointer: Once auth lands, the persisted-IDs path becomes dead code. Retire in a follow-up: delete OwnMessageIds, remove the parameter from chatmessage_to_messagedata, and clear hero_messaging_own_msg_ids:* keys on next load.
  • Caveats:
    • localStorage is per-origin per-browser — different machine / private window means historical messages render bubble-other until real auth lands.
    • Clearing site data loses the optimistic ownership mark.
    • Cap of 5_000 means very heavy long-running users could see the oldest messages flip back to bubble-other after the cap is hit.
  • Why not Option A (persist own_user_key per context): the server stamps "system" regardless of who sent the message, so the predicate msg.sender_public_key == own_user_key is false in both directions. Option A buys nothing until the server stops stamping "system".
  • Why not Option B (sender_public_key == "system" => own when own_user_key empty): confirmed in hero_osis/.../rpc.rs:911,960 that both send_message and reply_to_message unconditionally write "system". In a DM where both parties send messages, every message comes back as "system" — Option B would mark the other party's messages as own. Strictly worse UX than the current bug.
  • Why this fix is correct: msg.sid is server-assigned and unique per message. The local user is the only one who knows which sids they authored (because they just got back Ok(msg) from send_message). Storing those sids locally is the only data we have that uniquely identifies "messages I sent" until the server stamps a real key.
## Implementation Spec — Issue #192 ### Objective After page reload, messages the local user authored continue to render as `bubble-own` by re-applying the optimistic ownership mark from a per-context, persisted set of locally-authored message IDs — eliminating the regression where every bubble appears as `bubble-other` post-F5. ### Requirements - After F5 in `/space/default/messaging`, opening a chat that the user has previously sent messages to MUST render those messages right-aligned with `message-own` / `bubble-own` and the "Me" label, identical to how they appeared pre-reload. - Messages from the *other* party in the same DM/group MUST continue to render as `bubble-other` (no false-positive on the "everyone is system" case). - Sending a new message MUST keep working exactly as today (optimistic patch in `handle_send` is preserved) and MUST also seed the persisted set so the message survives reload. - Once authentication lands and the server stamps a real `sender_public_key`, the existing `is_own = !own_user_key.is_empty() && msg.sender_public_key == own_user_key` check MUST take precedence, so the persisted-IDs path becomes a no-op and can be retired without UI flicker. - The persisted set MUST be scoped per `context_name` so that switching contexts doesn't leak ownership across tenants. - Bound on persisted-set growth: cap entries per context (FIFO trim) so localStorage doesn't grow unboundedly. - Native (non-wasm) builds MUST compile and behave equivalently with persistence as a no-op (the messaging archipelago is `cdylib + rlib` and is built native by the workspace). ### Files to modify/create - `archipelagos/messaging/src/services/messaging_service.rs` — extend `chatmessage_to_messagedata` to accept a "locally-authored IDs" lookup; expose `SYSTEM_KEY` / `is_self_or_system` as `pub(crate)` for reuse; add a small `OwnMessageIds` helper module with cfg-gated localStorage persistence. - `archipelagos/messaging/src/archipelago.rs` — load the persisted ID set on mount, pass it into every `chatmessage_to_messagedata` call site (initial fetch, SSE `Resync`, SSE `ChatmessageNew`, send path), persist on every successful `handle_send`, and update the existing optimistic patch to also write through to the store. - (No new files required if the helper lives in `messaging_service.rs`. If preferred for clarity, create `archipelagos/messaging/src/services/own_messages.rs` and re-export from `services/mod.rs`.) ### Step-by-step implementation plan #### Step 1 — Add an `OwnMessageIds` helper **File:** `archipelagos/messaging/src/services/messaging_service.rs` (or new `services/own_messages.rs`). Introduce: - `pub struct OwnMessageIds { ids: HashSet<String>, order: Vec<String> }` with `contains(&str) -> bool`, `insert(String)` (FIFO-cap aware), `len()`, and a const `MAX_OWN_IDS_PER_CONTEXT: usize = 5_000`. - `pub fn load_own_message_ids(context_name: &str) -> OwnMessageIds`: - On `wasm32`: read `hero_messaging_own_msg_ids:<context_name>` from `localStorage`, parse as JSON `Vec<String>`, dedup into the set + order vec. Return empty on any error. - On non-wasm: return empty. - `pub fn save_own_message_ids(context_name: &str, ids: &OwnMessageIds)`: - On `wasm32`: serialize ordered ids as JSON and write to localStorage. On non-wasm: no-op. - Storage key constant: `const OWN_IDS_STORAGE_PREFIX: &str = "hero_messaging_own_msg_ids:";` Reuse the cfg-gated `storage_get` / `storage_set` pattern from `archipelagos/intelligence/ai/src/island.rs:17-51`. **Why:** Centralizes persistence and keeps the rest of messaging code agnostic to the backing store. Per-`context_name` keying isolates tenants. **Dependencies:** None. #### Step 2 — Extend `chatmessage_to_messagedata` to consult the local-author set **File:** `archipelagos/messaging/src/services/messaging_service.rs`, lines 349-375. Change signature to: ```rust pub fn chatmessage_to_messagedata( msg: &ChatMessage, own_user_key: &str, contacts: &[Contact], own_ids: &OwnMessageIds, ) -> MessageData ``` Replace the `is_own` computation with the layered check (first match wins): ```rust let is_own = if !own_user_key.is_empty() && msg.sender_public_key == own_user_key { true } else { own_ids.contains(&msg.sid.to_string()) }; let sender_name = if is_own { "Me".to_string() } else { resolve_sender_name(&msg.sender_public_key, own_user_key, contacts) }; ``` **Why:** Single conversion seam for every fetched/streamed message — every code path (initial fetch, SSE live event, SSE `Resync`) automatically picks it up. No scattered patching. **Dependencies:** Step 1. #### Step 3 — Wire `OwnMessageIds` through `archipelago.rs` **File:** `archipelagos/messaging/src/archipelago.rs`. a. In `MessagingArchipelagoApp`, after the existing signals (around lines 92-106), add: ```rust let mut own_message_ids = use_signal(|| services::load_own_message_ids(&context_name)); ``` b. Update **every** call to `services::chatmessage_to_messagedata` to pass `&own_message_ids.peek()` (or `.read()` where reactive re-derive matters): - Line 193 (`ChatEvent::ChatmessageNew` handler). - Line 303 (`Resync` refetch). - Line 427 (chat-load effect) — use `.read()` so the messages list re-derives if the set updates. - Line 508 (`handle_send`). c. In `handle_send` (around the existing optimistic patch on lines 511-512), after the message is successfully sent: ```rust let id = msg.sid.to_string(); own_message_ids.with_mut(|set| set.insert(id.clone())); services::save_own_message_ids(&ctx, &own_message_ids.peek()); ``` Keep the existing `msg_data.is_own = true; msg_data.sender_name = "Me"` lines. d. In the SSE `ChatmessageNew` handler (lines 191-205), no extra logic is needed — the conversion in Step 2 will return `is_own = true` if the sid is in the set. **Why:** Threads the local-author signal through every conversion site and persists on send. Dedup remains in the existing pipeline. **Dependencies:** Steps 1 and 2. #### Step 4 — Make `SYSTEM_KEY` / `is_self_or_system` reusable (optional cleanup) **File:** `archipelagos/messaging/src/services/messaging_service.rs`. Mark `SYSTEM_KEY` and `is_self_or_system` as `pub(crate)`. Skip if no callers consume them outside the file. **Why:** Avoids future duplication once auth lands. **Dependencies:** None. #### Step 5 — Tests Add a `#[cfg(test)] mod tests` block in `messaging_service.rs`: - `test_own_id_match_returns_is_own_true` — `sender_public_key = "system"`, `sid = "msg-42"`, `own_user_key = ""`, `OwnMessageIds` contains `"msg-42"`. Assert `is_own == true`, `sender_name == "Me"`. - `test_other_party_in_dm_remains_other` — same but sid NOT in set. Assert `is_own == false`. (DM false-positive guard.) - `test_real_key_takes_precedence` — `own_user_key = "0xabc"`, `msg.sender_public_key = "0xabc"`, empty `own_ids`. Assert `is_own == true` (forward-compat once auth lands). - `test_own_message_ids_capacity_trims_oldest` — insert > `MAX_OWN_IDS_PER_CONTEXT`, assert cap and FIFO eviction. Native-only (`cargo test -p hero_archipelagos_messaging`); `OwnMessageIds` must be constructable directly in tests without `localStorage`. **Why:** Locks in the DM regression guard and proves the fix works without manual reload. **Dependencies:** Steps 1 and 2. #### Step 6 — Manual repro verification 1. Build wasm bundle for messaging. 2. `/space/default/messaging`, create DM with QA Tester, send 3 messages. 3. Confirm bubbles render right-aligned (`bubble-own`). 4. F5. 5. Re-open same chat. 6. Confirm the 3 messages still render `bubble-own` with "Me" label and status checkmark. 7. DevTools: `localStorage["hero_messaging_own_msg_ids:default"]` is a JSON array of 3 SIDs. 8. Have a second user post a message in that DM — confirm it renders as `bubble-other`. 9. Switch to a different `context_name`, confirm no leak. ### Acceptance criteria - [ ] After F5, previously sent messages render `bubble-own` (right-aligned, blue, status icon, "Me" label). - [ ] Messages from the other party in a DM remain `bubble-other`, including when the server stamps both as `"system"`. - [ ] Newly sent messages still render optimistically as `bubble-own` without waiting for the SSE round-trip (existing #62 behavior preserved). - [ ] localStorage entry under `hero_messaging_own_msg_ids:<context>` exists, is JSON, and grows by one entry per successful send (capped at `MAX_OWN_IDS_PER_CONTEXT`). - [ ] Switching `context_name` does not leak own-message IDs across tenants. - [ ] Unit tests pass: `cargo test -p hero_archipelagos_messaging` covers the four cases in Step 5. - [ ] Native build and wasm build both succeed. - [ ] When a real `own_user_key` is provided (forward-compat with auth), the existing key-match path takes precedence — verified by `test_real_key_takes_precedence`. ### Notes - **Out of scope:** server-side fix to stamp `sender_public_key` with the `X-Hero-Context` value (option C in the issue) — tracked in hero_osis #40. - **Future-work pointer:** Once auth lands, the persisted-IDs path becomes dead code. Retire in a follow-up: delete `OwnMessageIds`, remove the parameter from `chatmessage_to_messagedata`, and clear `hero_messaging_own_msg_ids:*` keys on next load. - **Caveats:** - localStorage is per-origin per-browser — different machine / private window means historical messages render `bubble-other` until real auth lands. - Clearing site data loses the optimistic ownership mark. - Cap of 5_000 means very heavy long-running users could see the oldest messages flip back to `bubble-other` after the cap is hit. - **Why not Option A (persist `own_user_key` per context):** the server stamps `"system"` regardless of who sent the message, so the predicate `msg.sender_public_key == own_user_key` is false in both directions. Option A buys nothing until the server stops stamping `"system"`. - **Why not Option B (`sender_public_key == "system"` => own when `own_user_key` empty):** confirmed in `hero_osis/.../rpc.rs:911,960` that both `send_message` and `reply_to_message` unconditionally write `"system"`. In a DM where both parties send messages, every message comes back as `"system"` — Option B would mark the *other party's* messages as own. Strictly worse UX than the current bug. - **Why this fix is correct:** `msg.sid` is server-assigned and unique per message. The local user is the only one who knows which sids they authored (because they just got back `Ok(msg)` from `send_message`). Storing those sids locally is the only data we have that uniquely identifies "messages I sent" until the server stamps a real key.
rawdaGastan commented 2026-05-06 11:49:08 +00:00
Author
Member
Copy link

Test Results

Crate: hero_archipelagos_messaging (cargo test --lib)

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

New tests covering #192

  • services::messaging_service::own_message_ids_tests::test_insert_dedupes — own-id set dedupes on repeat insert
  • services::messaging_service::own_message_ids_tests::test_insert_fifo_cap — FIFO eviction at MAX_OWN_IDS_PER_CONTEXT (5_000)
  • services::messaging_service::chatmessage_conversion_tests::test_own_id_match_returns_is_own_true — sid match makes is_own true post-refetch even when sender is "system"
  • services::messaging_service::chatmessage_conversion_tests::test_other_party_in_dm_remains_other — DM regression guard: "system" sender NOT in own-ids => is_own false (the option-B false-positive the issue calls out)
  • services::messaging_service::chatmessage_conversion_tests::test_real_key_takes_precedence — once auth lands, key match wins regardless of own-ids

Workspace check

cargo check --workspace --exclude hero_archipelagos_messaging: clean (no errors, finished in 2.83s).

cargo test --workspace --no-run: clean (all workspace test binaries compile).

## Test Results **Crate:** `hero_archipelagos_messaging` (`cargo test --lib`) - Total: 5 - Passed: 5 - Failed: 0 ### New tests covering #192 - `services::messaging_service::own_message_ids_tests::test_insert_dedupes` — own-id set dedupes on repeat insert - `services::messaging_service::own_message_ids_tests::test_insert_fifo_cap` — FIFO eviction at MAX_OWN_IDS_PER_CONTEXT (5_000) - `services::messaging_service::chatmessage_conversion_tests::test_own_id_match_returns_is_own_true` — sid match makes is_own true post-refetch even when sender is "system" - `services::messaging_service::chatmessage_conversion_tests::test_other_party_in_dm_remains_other` — DM regression guard: "system" sender NOT in own-ids => is_own false (the option-B false-positive the issue calls out) - `services::messaging_service::chatmessage_conversion_tests::test_real_key_takes_precedence` — once auth lands, key match wins regardless of own-ids ### Workspace check `cargo check --workspace --exclude hero_archipelagos_messaging`: clean (no errors, finished in 2.83s). `cargo test --workspace --no-run`: clean (all workspace test binaries compile).
rawdaGastan commented 2026-05-06 12:17:01 +00:00
Author
Member
Copy link

Implementation Summary

Branch: development_fix_bubble_own_after_reload

Approach

Persist the SIDs of locally-authored messages in a per-context localStorage set. The is-own check in chatmessage_to_messagedata now layers (key match) || own_ids.contains(&msg.sid.to_string()) so that refetched messages survive F5 with the right alignment. Falls back cleanly to the existing own_user_key match once auth lands — verified by test_real_key_takes_precedence.

Picked this over the issue's option B (treat sender_public_key == "system" as own) because the server stamps both DM parties as "system" (hero_osis/.../rpc.rs:911,960), so option B would mark the other party's messages as own — strictly worse than the current bug. Picked it over option A (persist own_user_key) because the server's "system" stamp makes the key-match predicate false in both directions, so persisting the key buys nothing pre-auth.

Files changed

  • archipelagos/messaging/Cargo.toml — added "Storage" to web-sys features.
  • archipelagos/messaging/src/services/messaging_service.rs:
    • pub struct OwnMessageIds (HashSet<String> + ordered Vec<String>, FIFO-capped at MAX_OWN_IDS_PER_CONTEXT = 5_000).
    • pub fn load_own_message_ids(context_name) / pub fn save_own_message_ids(context_name, &ids) — cfg-gated localStorage on wasm32 (key hero_messaging_own_msg_ids:<context>), no-op on native. Mirrors the storage pattern in archipelagos/intelligence/ai/src/island.rs:17-51.
    • chatmessage_to_messagedata now takes a 4th &OwnMessageIds argument; is_own = (key match) || own_ids.contains(&msg.sid.to_string()); sender_name = "Me" whenever is_own.
    • 5 unit tests (see test results).
  • archipelagos/messaging/src/archipelago.rs:
    • let mut own_message_ids = use_signal(|| services::load_own_message_ids(&context_name)); on mount.
    • All 4 chatmessage_to_messagedata call sites pass &own_message_ids.peek() (initial fetch, SSE Resync, SSE ChatmessageNew, send path).
    • In handle_send, on successful send: own_message_ids.with_mut(|s| s.insert(msg.sid.to_string())); services::save_own_message_ids(&ctx, &own_message_ids.peek()); — written before the existing optimistic patch, so the just-sent SID is part of the set when the data is built.
    • Existing optimistic patch (msg_data.is_own = true; msg_data.sender_name = "Me") preserved unchanged.
  • server/src/main.rs — added mut to 9 let X = use_signal(...) bindings (arch_transitioning, island_transitioning, paradigm_visible, wasm_visible, ai_visible, backend_visible, cta_visible, cards_visible, active_section). Pre-existing 11 E0596 errors blocked the showcase wasm build entirely on development. Bundled here so the fix is verifiable in make run. (Could be split into a separate PR if preferred.)

Diff stats vs development

 archipelagos/messaging/Cargo.toml                  |   1 +
 archipelagos/messaging/src/archipelago.rs          |  19 +-
 .../messaging/src/services/messaging_service.rs    | 223 ++++++++++++++++++++-
 server/src/main.rs                                 |  18 +-
 4 files changed, 248 insertions(+), 13 deletions(-)

Verification

  • cargo test -p hero_archipelagos_messaging --lib: 5/5 passed.
  • cargo check --workspace --exclude hero_archipelagos_messaging: clean.
  • cargo check -p archipelagos_server --target wasm32-unknown-unknown: clean (after the mut patch).
  • Live verified in browser: make run -> http://localhost:8886 -> messaging island -> sent messages render bubble-own, F5, re-open chat, messages still render bubble-own. localStorage entry hero_messaging_own_msg_ids:<context> populated. No re-fetch storm on send.

Notes

  • An early version of the wiring made the chat-load use_effect reactively subscribe to own_message_ids so the message list would re-derive when the set grew. That caused a re-fetch + wholesale active_messages.set(...) on every send, visible as a screen jump. Replaced the reactive subscription with .peek() everywhere — the set only matters on the initial chat load after F5; mid-session, the optimistic patch in handle_send already does the right thing.
  • localStorage is per-origin per-browser. Different machine / private window will see historical messages render bubble-other until real auth lands — acceptable degradation given that auth is the proper fix.
  • Cap of 5_000 SIDs per context is conservative; MAX_OWN_IDS_PER_CONTEXT is the constant if it ever needs tuning.
  • Once the server stamps a real sender_public_key, the existing key-match path takes precedence and the persisted-IDs path becomes dead code — retire in a follow-up: delete OwnMessageIds, drop the parameter from chatmessage_to_messagedata, one-time-clear hero_messaging_own_msg_ids:* keys on next load.

Out of scope

  • Server-side fix to stamp sender_public_key with X-Hero-Context (option C) — tracked in hero_osis #40.
## Implementation Summary Branch: `development_fix_bubble_own_after_reload` ### Approach Persist the SIDs of locally-authored messages in a per-context `localStorage` set. The is-own check in `chatmessage_to_messagedata` now layers `(key match) || own_ids.contains(&msg.sid.to_string())` so that refetched messages survive F5 with the right alignment. Falls back cleanly to the existing `own_user_key` match once auth lands — verified by `test_real_key_takes_precedence`. Picked this over the issue's option B (treat `sender_public_key == "system"` as own) because the server stamps both DM parties as `"system"` (`hero_osis/.../rpc.rs:911,960`), so option B would mark the *other* party's messages as own — strictly worse than the current bug. Picked it over option A (persist `own_user_key`) because the server's `"system"` stamp makes the key-match predicate false in both directions, so persisting the key buys nothing pre-auth. ### Files changed - `archipelagos/messaging/Cargo.toml` — added `"Storage"` to `web-sys` features. - `archipelagos/messaging/src/services/messaging_service.rs`: - `pub struct OwnMessageIds` (`HashSet<String>` + ordered `Vec<String>`, FIFO-capped at `MAX_OWN_IDS_PER_CONTEXT = 5_000`). - `pub fn load_own_message_ids(context_name)` / `pub fn save_own_message_ids(context_name, &ids)` — cfg-gated `localStorage` on wasm32 (key `hero_messaging_own_msg_ids:<context>`), no-op on native. Mirrors the storage pattern in `archipelagos/intelligence/ai/src/island.rs:17-51`. - `chatmessage_to_messagedata` now takes a 4th `&OwnMessageIds` argument; `is_own = (key match) || own_ids.contains(&msg.sid.to_string())`; `sender_name = "Me"` whenever `is_own`. - 5 unit tests (see [test results](#issuecomment-29848)). - `archipelagos/messaging/src/archipelago.rs`: - `let mut own_message_ids = use_signal(|| services::load_own_message_ids(&context_name));` on mount. - All 4 `chatmessage_to_messagedata` call sites pass `&own_message_ids.peek()` (initial fetch, SSE `Resync`, SSE `ChatmessageNew`, send path). - In `handle_send`, on successful send: `own_message_ids.with_mut(|s| s.insert(msg.sid.to_string())); services::save_own_message_ids(&ctx, &own_message_ids.peek());` — written before the existing optimistic patch, so the just-sent SID is part of the set when the data is built. - Existing optimistic patch (`msg_data.is_own = true; msg_data.sender_name = "Me"`) preserved unchanged. - `server/src/main.rs` — added `mut` to 9 `let X = use_signal(...)` bindings (`arch_transitioning`, `island_transitioning`, `paradigm_visible`, `wasm_visible`, `ai_visible`, `backend_visible`, `cta_visible`, `cards_visible`, `active_section`). Pre-existing 11 E0596 errors blocked the showcase wasm build entirely on `development`. Bundled here so the fix is verifiable in `make run`. (Could be split into a separate PR if preferred.) ### Diff stats vs `development` ``` archipelagos/messaging/Cargo.toml | 1 + archipelagos/messaging/src/archipelago.rs | 19 +- .../messaging/src/services/messaging_service.rs | 223 ++++++++++++++++++++- server/src/main.rs | 18 +- 4 files changed, 248 insertions(+), 13 deletions(-) ``` ### Verification - `cargo test -p hero_archipelagos_messaging --lib`: 5/5 passed. - `cargo check --workspace --exclude hero_archipelagos_messaging`: clean. - `cargo check -p archipelagos_server --target wasm32-unknown-unknown`: clean (after the `mut` patch). - **Live verified in browser**: `make run` -> `http://localhost:8886` -> messaging island -> sent messages render `bubble-own`, F5, re-open chat, messages still render `bubble-own`. localStorage entry `hero_messaging_own_msg_ids:<context>` populated. No re-fetch storm on send. ### Notes - An early version of the wiring made the chat-load `use_effect` reactively subscribe to `own_message_ids` so the message list would re-derive when the set grew. That caused a re-fetch + wholesale `active_messages.set(...)` on every send, visible as a screen jump. Replaced the reactive subscription with `.peek()` everywhere — the set only matters on the initial chat load after F5; mid-session, the optimistic patch in `handle_send` already does the right thing. - localStorage is per-origin per-browser. Different machine / private window will see historical messages render `bubble-other` until real auth lands — acceptable degradation given that auth is the proper fix. - Cap of 5_000 SIDs per context is conservative; `MAX_OWN_IDS_PER_CONTEXT` is the constant if it ever needs tuning. - Once the server stamps a real `sender_public_key`, the existing key-match path takes precedence and the persisted-IDs path becomes dead code — retire in a follow-up: delete `OwnMessageIds`, drop the parameter from `chatmessage_to_messagedata`, one-time-clear `hero_messaging_own_msg_ids:*` keys on next load. ### Out of scope - Server-side fix to stamp `sender_public_key` with `X-Hero-Context` (option C) — tracked in hero_osis #40.
rawdaGastan commented 2026-05-06 12:19:38 +00:00
Author
Member
Copy link

Pull request opened: #210

This PR implements the changes discussed in this issue.

Pull request opened: https://forge.ourworld.tf/lhumina_code/hero_archipelagos/pulls/210 This PR implements the changes discussed in this issue.
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
development
development_refactor_list_full
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#192
No description provided.