From a0e15650975ab3f1eeccd6f89a455fb4ffc921a3 Mon Sep 17 00:00:00 2001 From: Keith Smith Date: Mon, 17 Aug 2026 11:52:29 -0600 Subject: [PATCH] Fix chat/room ordering that could differ between devices (#45) Two independent gaps, both fixed since the report was ambiguous about which "chats" meant: - ChatPane.tsx concatenated history (REST-fetched) and live (WS-pushed) without sorting, so anything that could desync receipt order from send order -- a rejoin/resync racing a still-in-flight WS message, which opening the same room on another device triggers directly via a fresh socket connection -- could render messages out of chronological order. Now sorted by created_at (stable sort, so same-timestamp messages keep their relative order). - list_member_rooms/list_open_rooms/list_recent_messages ordered by created_at alone, with no secondary tiebreaker. Postgres doesn't guarantee a stable order for tied rows across separate query executions, so two rooms/messages sharing an identical timestamp (a real possibility -- rapid sends, bulk-created rooms) could come back in a different order on two separate fetches, i.e. two devices. Added id as a secondary sort key everywhere this showed up. Co-Authored-By: Claude Sonnet 5 --- backend/app/services/message_service.py | 7 +++- backend/app/services/room_service.py | 10 ++++-- backend/tests/test_rooms.py | 23 +++++++++++++ backend/tests/test_ws_chat.py | 45 +++++++++++++++++++++++++ frontend/src/components/ChatPane.tsx | 20 +++++++++-- 5 files changed, 100 insertions(+), 5 deletions(-) diff --git a/backend/app/services/message_service.py b/backend/app/services/message_service.py index 2334ab6..638a4bc 100644 --- a/backend/app/services/message_service.py +++ b/backend/app/services/message_service.py @@ -65,7 +65,12 @@ async def list_recent_messages( select(Message) .where(Message.room_id == room_id) .options(selectinload(Message.user), selectinload(Message.file)) - .order_by(Message.created_at.desc()) + # Secondary key on the primary key -- two messages can share the + # same created_at (rapid sends, e.g. from different clients or a + # webhook), and without a tiebreaker Postgres isn't obligated to + # return them in the same relative order on every call, which can + # look like messages swapping places between fetches/devices. + .order_by(Message.created_at.desc(), Message.id.desc()) .limit(limit) ) messages = list(result.scalars().all()) diff --git a/backend/app/services/room_service.py b/backend/app/services/room_service.py index f4a6392..84bc778 100644 --- a/backend/app/services/room_service.py +++ b/backend/app/services/room_service.py @@ -71,7 +71,7 @@ async def list_open_rooms(db: AsyncSession, user_id: uuid.UUID) -> list[tuple[Ro select(Room) .where(Room.is_private.is_(False), Room.is_archived.is_(False)) .options(selectinload(Room.memberships)) - .order_by(Room.created_at) + .order_by(Room.created_at, Room.id) ) rooms = result.scalars().all() return [ @@ -108,7 +108,13 @@ async def list_member_rooms( ) .join(RoomMembership, RoomMembership.room_id == Room.id) .where(RoomMembership.user_id == user_id) - .order_by(Room.created_at) + # A secondary key on the primary key -- without it, Postgres has no + # obligation to return two same-instant rooms (a plausible tie: + # bulk-created/migrated rooms, or just two created in quick + # succession) in the same order on every call, which without a + # stable order can visibly reshuffle the sidebar between one + # device's fetch and another's. + .order_by(Room.created_at, Room.id) ) return [ (room, role, last_message_at is not None and last_message_at > last_read_at, has_mention) diff --git a/backend/tests/test_rooms.py b/backend/tests/test_rooms.py index 817c7d0..76117c6 100644 --- a/backend/tests/test_rooms.py +++ b/backend/tests/test_rooms.py @@ -27,6 +27,29 @@ async def test_create_room_creates_owner_membership(client, db_session): assert membership.role == RoomRole.owner +async def test_my_rooms_tiebreaks_identical_created_at_by_id(client, db_session): + # Two rooms sharing the exact same created_at (bulk-created/migrated + # rooms, or just an unlucky timing collision) must still come back in a + # single, stable order every call -- see list_member_rooms's order_by + # comment. Without a secondary sort key, a second fetch (e.g. from a + # different device) isn't guaranteed to return ties in the same order. + await register_and_login(client, db_session, username="alice") + room_a = (await client.post("/api/rooms", json={"name": "room-a"})).json() + room_b = (await client.post("/api/rooms", json={"name": "room-b"})).json() + + result = await db_session.execute( + select(Room).where(Room.id.in_([uuid.UUID(room_a["id"]), uuid.UUID(room_b["id"])])) + ) + rooms_by_id = {str(r.id): r for r in result.scalars().all()} + rooms_by_id[room_a["id"]].created_at = rooms_by_id[room_b["id"]].created_at + await db_session.commit() + + resp = await client.get("/api/rooms/mine") + assert resp.status_code == 200 + ids = [r["id"] for r in resp.json() if r["id"] in (room_a["id"], room_b["id"])] + assert ids == sorted([room_a["id"], room_b["id"]]) + + async def test_list_rooms_excludes_private(client, db_session): user = await register_and_login(client, db_session, username="alice") await client.post("/api/rooms", json={"name": "open-room"}) diff --git a/backend/tests/test_ws_chat.py b/backend/tests/test_ws_chat.py index 197e2c0..f808afe 100644 --- a/backend/tests/test_ws_chat.py +++ b/backend/tests/test_ws_chat.py @@ -64,6 +64,51 @@ def test_ws_join_and_message_roundtrip(ws_client): assert any(m["content"] == "hello" and m["username"] == username for m in history) +def test_history_tiebreaks_identical_timestamps_by_id(ws_client): + # Two messages sharing the exact same created_at (a real possibility -- + # rapid sends, a webhook, microsecond-precision collisions under load) + # must still come back in a single, stable order every call, not + # whatever order Postgres feels like giving ties with no secondary sort + # key -- see list_recent_messages's order_by comment. + username = _unique("alice") + _register(ws_client, username=username) + room = ws_client.post("/api/rooms", json={"name": _unique("general")}).json() + + with ws_client.websocket_connect("/ws/chat") as ws: + ws.send_json({"type": "join", "room_id": room["id"]}) + assert ws.receive_json()["type"] == "joined" + ws.send_json({"type": "message", "room_id": room["id"], "content": "first"}) + first = ws.receive_json() + ws.send_json({"type": "message", "room_id": room["id"], "content": "second"}) + second = ws.receive_json() + + async def _force_same_timestamp(): + from sqlalchemy import select + + from app.models import Message + + async with ws_client.session_factory() as session: + result = await session.execute( + select(Message).where(Message.id.in_([uuid.UUID(first["id"]), uuid.UUID(second["id"])])) + ) + rows = {str(m.id): m for m in result.scalars().all()} + rows[first["id"]].created_at = rows[second["id"]].created_at + await session.commit() + + ws_client.portal.call(_force_same_timestamp) + + resp = ws_client.get(f"/api/rooms/{room['id']}/messages") + assert resp.status_code == 200 + history = [m for m in resp.json() if m["content"] in ("first", "second")] + assert len(history) == 2 + + # order_by(created_at.desc(), id.desc()) then reversed for display -- + # with a tied created_at, the higher id (whichever message that is) + # must consistently render second. + expected_order = sorted([first, second], key=lambda m: m["id"]) + assert [m["content"] for m in history] == [m["content"] for m in expected_order] + + def test_ws_message_without_join_errors(ws_client): _register(ws_client, username=_unique("alice")) room = ws_client.post("/api/rooms", json={"name": _unique("general")}).json() diff --git a/frontend/src/components/ChatPane.tsx b/frontend/src/components/ChatPane.tsx index a9015e3..241a5f9 100644 --- a/frontend/src/components/ChatPane.tsx +++ b/frontend/src/components/ChatPane.tsx @@ -1,4 +1,4 @@ -import { useCallback, useEffect, useState } from 'react' +import { useCallback, useEffect, useMemo, useState } from 'react' import { NetworkError } from '../api/client' import { getRoomMessages, markRoomRead } from '../api/rooms' import type { ChatSocketHandle } from '../ws/useChatSocket' @@ -127,6 +127,22 @@ export function ChatPane({ [socket, room.id, refreshHistory, markRead], ) + // history and live are just concatenated, not merge-sorted -- live is + // strictly receipt order, which isn't always send order. A rejoin (a + // reconnect, or opening the same room on another device) refetches + // history but doesn't guarantee anything about the timing of whatever + // WS messages land in live afterward relative to it, so without this + // sort a message can render above one that was actually sent earlier. + // Stable sort (guaranteed since ES2019) keeps same-timestamp messages in + // their original relative order rather than shuffling them. + const messages = useMemo( + () => + [...history, ...live].sort( + (a, b) => new Date(a.created_at).getTime() - new Date(b.created_at).getTime(), + ), + [history, live], + ) + const connected = socket.connected const send = useCallback( (content: string, imageId?: string, fileId?: string) => socket.send(room.id, content, imageId, fileId), @@ -179,7 +195,7 @@ export function ChatPane({