Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions backend/routes/social.py
Original file line number Diff line number Diff line change
Expand Up @@ -286,16 +286,20 @@ def get_room_messages(room_id: str, request: Request, before: str | None = None,
raise HTTPException(status_code=400, detail="`before` must be an ISO 8601 timestamp")
filters["created_at"] = f"lt.{before}"
# Fetch newest-first so the slice covers the page we need, then reverse to ascending.
# Over-fetch one row so `has_more` is exact: if the DB returns more than
# `limit`, an extra page exists. (Using `len(rows) == limit` reports a
# phantom "load more" at exact page boundaries — issue #131.)
rows = table("room_messages").select(
"id,room_id,user_id,user_name,text,image_url,reply_to_id,is_deleted,edited_at,created_at",
filters=filters,
order="created_at.desc",
limit=limit,
limit=limit + 1,
)
if not rows:
return {"messages": [], "has_more": False}
has_more = len(rows) > limit
rows = rows[:limit] # drop the probe row before reversing
rows = list(reversed(rows))
has_more = len(rows) == limit

msg_ids = [r["id"] for r in rows]

Expand Down
36 changes: 32 additions & 4 deletions backend/tests/test_social_messages.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,10 @@ def table_side_effect(name):
assert ids == ["m1", "m2", "m3"] # ascending
assert body["has_more"] is False # fewer than default limit (50)

def test_has_more_true_when_page_is_full(self):
def test_exact_page_boundary_reports_no_more(self):
# Off-by-one guard (issue #131): exactly `limit` rows must NOT report a
# phantom extra page. The route over-fetches limit+1; the DB returns only
# `limit` rows here, so has_more is False.
rows = [_mk_msg(i, f"2026-04-{i:02d}T00:00:00Z") for i in range(1, 6)] # 5 rows
def table_side_effect(name):
m = MagicMock()
Expand All @@ -65,9 +68,34 @@ def table_side_effect(name):
with patch("routes.social.table", side_effect=table_side_effect):
r = client.get(f"/api/social/rooms/{ROOM_ID}/messages?limit=5")

body = r.json()
assert body["has_more"] is False
assert len(body["messages"]) == 5

def test_has_more_true_when_probe_row_present(self):
# The route requests limit+1; when the DB returns limit+1 rows, an extra
# page exists. The probe row is dropped, so the page holds exactly `limit`.
rows = [_mk_msg(i, f"2026-04-{i:02d}T00:00:00Z") for i in range(1, 7)] # 6 rows for limit=5
def table_side_effect(name):
m = MagicMock()
if name == "room_messages":
m.select.return_value = list(reversed(rows)) # DB returns desc
elif name == "room_reactions":
m.select.return_value = []
elif name == "room_members":
m.select.return_value = [{"user_id": "user_andres"}]
else:
m.select.return_value = []
return m

with patch("routes.social.table", side_effect=table_side_effect):
r = client.get(f"/api/social/rooms/{ROOM_ID}/messages?limit=5")

body = r.json()
assert body["has_more"] is True
assert len(body["messages"]) == 5
# Probe row (oldest, m1) is dropped; page is the 5 newest, ascending.
assert [m["id"] for m in body["messages"]] == ["m2", "m3", "m4", "m5", "m6"]

def test_before_filter_is_passed_through(self):
captured = {}
Expand All @@ -94,7 +122,7 @@ def _select(cols, filters=None, order=None, limit=None):
assert r.status_code == 200
assert captured["filters"]["created_at"] == "lt.2026-04-02T00:00:00Z"
assert captured["order"] == "created_at.desc"
assert captured["limit"] == 20
assert captured["limit"] == 21 # requested 20 + 1 probe row for has_more

def test_invalid_before_rejected(self):
# Guards against PostgREST operator injection (e.g. ?before=null or
Expand Down Expand Up @@ -140,7 +168,7 @@ def _select(cols, filters=None, order=None, limit=None):
r = client.get(f"/api/social/rooms/{ROOM_ID}/messages?limit=9999")

assert r.status_code == 200
assert captured["limit"] == 200 # clamped
assert captured["limit"] == 201 # clamped to 200, +1 probe row

def test_limit_floor_is_one(self):
captured = {}
Expand All @@ -163,4 +191,4 @@ def _select(cols, filters=None, order=None, limit=None):
r = client.get(f"/api/social/rooms/{ROOM_ID}/messages?limit=0")

assert r.status_code == 200
assert captured["limit"] == 1
assert captured["limit"] == 2 # floored to 1, +1 probe row
75 changes: 66 additions & 9 deletions frontend/src/components/screens/Social.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -146,27 +146,55 @@ function RoomChat({ roomId, members }: { roomId: string; members: { user_id: str
(payload: any) => {
const row = payload.new as RoomMessageRow;
setMessages(prev => {
const withoutTmp = prev.filter(m => !m.id.startsWith("tmp_") || m.text !== row.text || m.user_id !== row.user_id);
if (withoutTmp.some(m => m.id === row.id)) return withoutTmp;
return [...withoutTmp, { ...row, reactions: [], reply_to: null }];
// Dedup by real id: the sender already has this row (optimistic tmp
// reconciled by the POST response, or a prior echo). Never append a
// realtime row that's already present, and don't clobber the loaded
// reply_to/reactions on a row we already have (issue #131).
if (prev.some(m => m.id === row.id)) return prev;
// Own messages are handled by send()'s POST reconciliation; skip the
// echo so we don't briefly render a ciphertext duplicate.
if (row.user_id === userId) return prev;
return [...prev, { ...row, reactions: [], reply_to: null }];
});
})
.on("postgres_changes",
{ event: "UPDATE", schema: "public", table: "room_messages", filter: `room_id=eq.${roomId}` },
(payload: any) => {
const row = payload.new as RoomMessageRow;
setMessages(prev => prev.map(m => m.id === row.id ? { ...m, ...row } : m));
setMessages(prev => prev.map(m => {
if (m.id !== row.id) return m;
// Own edits/deletes are applied optimistically with plaintext; the
// echo carries ciphertext `text`, so don't overwrite it. Keep our
// text/edited_at, take server-side flags like is_deleted (issue #131).
if (row.user_id === userId) {
return { ...m, is_deleted: row.is_deleted, edited_at: row.edited_at ?? m.edited_at };
}
return { ...m, ...row, reply_to: m.reply_to, reactions: m.reactions };
}));
})
.on("postgres_changes",
{ event: "INSERT", schema: "public", table: "room_reactions" },
() => { void load(); })
(payload: any) => {
// Scope the reload to this room: only reload if the reaction targets a
// message we have loaded, otherwise a reaction in any other room would
// refetch this open room (issue #131).
const mid = payload.new?.message_id;
if (mid) setMessages(prev => { if (prev.some(m => m.id === mid)) void load(); return prev; });
})
.on("postgres_changes",
{ event: "DELETE", schema: "public", table: "room_reactions" },
() => { void load(); })
(payload: any) => {
// DELETE payloads only carry the primary key (id), not message_id,
// unless the table has REPLICA IDENTITY FULL. Reload only when the
// affected message is loaded; if we can't tell, fall back to a reload.
const mid = payload.old?.message_id;
if (mid === undefined) { void load(); return; }
setMessages(prev => { if (prev.some(m => m.id === mid)) void load(); return prev; });
})
.subscribe();

return () => { supa.removeChannel(channel); };
}, [roomId, load]);
}, [roomId, load, userId]);

// Presence / typing channel.
React.useEffect(() => {
Expand Down Expand Up @@ -253,7 +281,28 @@ function RoomChat({ roomId, members }: { roomId: string; members: { user_id: str
const r = replyTo;
setReplyTo(null);
try {
await sendRoomMessage(roomId, userId, userName || "Me", text, undefined, r?.id);
// Replace the optimistic temp row with the server's decrypted message
// (matched by real id). This is the authoritative dedup: the realtime
// INSERT echo can't reconcile because it carries ciphertext, never the
// plaintext we appended (issue #131).
const res = await sendRoomMessage(roomId, userId, userName || "Me", text, undefined, r?.id);
const saved = res?.message as RoomMessageRow | undefined;
if (saved?.id) {
// Drop the tmp row, then upsert the saved row by id (replace if the
// realtime echo already landed it, else append). reply_to/reactions
// come from the optimistic tmp since the POST response omits them.
const reconciled: RoomMessageRow = {
...saved,
reply_to: saved.reply_to ?? tmp.reply_to,
reactions: saved.reactions ?? [],
};
setMessages(prev => {
const next = prev.filter(m => m.id !== tmpId);
return next.some(m => m.id === saved.id)
? next.map(m => m.id === saved.id ? { ...m, ...reconciled, reactions: m.reactions ?? reconciled.reactions } : m)
: [...next, reconciled];
});
}
} catch (err) {
setMessages(prev => prev.filter(m => m.id !== tmpId));
toast.error(`Send failed: ${String(err)}`);
Expand Down Expand Up @@ -307,7 +356,15 @@ function RoomChat({ roomId, members }: { roomId: string; members: { user_id: str
if (up.error) throw up.error;
const pub = supa.storage.from("chat-images").getPublicUrl(path);
const url: string = pub.data.publicUrl;
await sendRoomMessage(roomId, userId, userName || "Me", "", url);
const res = await sendRoomMessage(roomId, userId, userName || "Me", "", url);
// Append from the POST response (own realtime echoes are skipped to avoid
// duplicates), deduping by id in case the echo already landed.
const saved = res?.message as RoomMessageRow | undefined;
if (saved?.id) {
setMessages(prev => prev.some(m => m.id === saved.id)
? prev
: [...prev, { ...saved, reply_to: saved.reply_to ?? null, reactions: saved.reactions ?? [] }]);
}
toast.success("Image sent");
} catch (err) {
toast.error(`Upload failed: ${String(err)}`);
Expand Down
Loading