mirror of
https://github.com/J3vb/OwnCord.git
synced 2026-09-03 03:50:00 +03:00
fix(client): review follow-ups for the connection-status batch
Fixes from an adversarial review of the previous commit:
- MainPage banner: sync the banner with the current store status at mount.
The selector subscription baselines on the current value and only fires
on change, so a MainPage mounted mid-outage (status already
"reconnecting") would never show the banner — the whole retry cycle maps
to the same 3-state value. The status→banner dispatch is extracted to
ServerBanner.applyConnectionStatus and unit-tested.
- History-fetch failure is no longer silent when the channel already has
rows (live broadcasts / optimistic sends): the inline error region only
renders in an empty channel, so loadMessages now also raises a toast in
that case.
- Composer disable reason distinguishes "Reconnecting…" from
"Not connected" per the spec §3 table (it previously showed
"Reconnecting…" while disconnected, contradicting the banner).
- The single-writer wiring is extracted to
dispatcher.wireConnectionStatus(ws) and pinned by a test (it was
previously an untestable main.ts module-scope line — deleting it would
have failed zero tests).
- Docs honesty: messaging.md's transport-drop diagram arm now shows both
codes (channel full → NETWORK, closed/not-open → OFFLINE) instead of
claiming NETWORK for both; README §3's callout now explicitly lists the
voice column ("frozen" during reconnect) as a remaining gap instead of
implying the section is fully closed; the composer table documents both
offline reasons.
- New pinning tests: SidebarArea passes ws to UserBar (the production-bug
fix was previously unasserted), ServerBanner.showDisconnected,
applyConnectionStatus mapping, ChannelController onRetryLoad /
onRetry-resend / onDeleteDraft, composer reason per status, and the
history-failure toast fallback.
Verified: tsc + full client unit suite (3234 tests) + oxlint/eslint +
prettier all green.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -67,3 +67,21 @@ export function createServerBanner(): ServerBannerControl {
|
||||
|
||||
return { element: root, showRestart, showReconnecting, showDisconnected, hide, destroy };
|
||||
}
|
||||
|
||||
/**
|
||||
* Apply a store connection status to the banner (UX spec §3 table):
|
||||
* reconnecting → "Reconnecting...", disconnected → "Disconnected",
|
||||
* connected → hidden.
|
||||
*/
|
||||
export function applyConnectionStatus(
|
||||
banner: ServerBannerControl,
|
||||
status: "connected" | "reconnecting" | "disconnected",
|
||||
): void {
|
||||
if (status === "reconnecting") {
|
||||
banner.showReconnecting();
|
||||
} else if (status === "disconnected") {
|
||||
banner.showDisconnected();
|
||||
} else {
|
||||
banner.hide();
|
||||
}
|
||||
}
|
||||
|
||||
@@ -3,8 +3,9 @@
|
||||
// Each server message type maps to one or more store actions.
|
||||
|
||||
import type { WsClient } from "./ws";
|
||||
import { toConnectionStatus } from "./ws";
|
||||
import { authStore, setAuth, clearAuth } from "@stores/auth.store";
|
||||
import { setTransientError } from "@stores/ui.store";
|
||||
import { setTransientError, setConnectionStatus } from "@stores/ui.store";
|
||||
import {
|
||||
setChannels,
|
||||
setRoles,
|
||||
@@ -85,6 +86,16 @@ function mapDmPayload(p: DmChannelPayload): DmChannel {
|
||||
/** Unsubscribe all listeners. */
|
||||
export type DispatcherCleanup = () => void;
|
||||
|
||||
/**
|
||||
* The single writer for ui.store.connectionStatus (UX spec §3): collapses the
|
||||
* ws client's internal state machine onto the 3-state status. Wired once at
|
||||
* startup and kept for the app's lifetime — deliberately separate from
|
||||
* wireDispatcher, whose listeners are torn down per connection.
|
||||
*/
|
||||
export function wireConnectionStatus(ws: Pick<WsClient, "onStateChange">): () => void {
|
||||
return ws.onStateChange((s) => setConnectionStatus(toConnectionStatus(s)));
|
||||
}
|
||||
|
||||
/**
|
||||
* Wire a WsClient to all domain stores.
|
||||
* Returns a cleanup function that removes all listeners.
|
||||
|
||||
@@ -9,10 +9,10 @@ import "@styles/theme-neon-glow.css";
|
||||
import { installGlobalErrorHandlers, safeMount } from "@lib/safe-render";
|
||||
import { createRouter } from "@lib/router";
|
||||
import { createApiClient } from "@lib/api";
|
||||
import { createWsClient, toConnectionStatus } from "@lib/ws";
|
||||
import { wireDispatcher } from "@lib/dispatcher";
|
||||
import { createWsClient } from "@lib/ws";
|
||||
import { wireDispatcher, wireConnectionStatus } from "@lib/dispatcher";
|
||||
import { authStore, clearAuth } from "@stores/auth.store";
|
||||
import { setTransientError, setConnectionStatus } from "@stores/ui.store";
|
||||
import { setTransientError } from "@stores/ui.store";
|
||||
import { voiceStore, leaveVoiceChannel } from "@stores/voice.store";
|
||||
import { leaveVoice as voiceSessionLeave } from "@lib/livekitSession";
|
||||
import { createConnectPage } from "@pages/ConnectPage";
|
||||
@@ -99,7 +99,7 @@ const ws = createWsClient();
|
||||
// live controls read ui.store.connectionStatus reactively instead of wiring
|
||||
// their own ws.onStateChange. Lifecycle plumbing that needs the exact internal
|
||||
// transition (the connected overlay below) stays on ws.onStateChange.
|
||||
ws.onStateChange((s) => setConnectionStatus(toConnectionStatus(s)));
|
||||
wireConnectionStatus(ws);
|
||||
const profileManager = createProfileManager(createTauriBackend());
|
||||
let dispatcherCleanup: (() => void) | null = null;
|
||||
let connectedOverlay: ConnectedOverlayControl | null = null;
|
||||
|
||||
@@ -9,7 +9,7 @@ import type { ApiClient } from "@lib/api";
|
||||
import { createLogger } from "@lib/logger";
|
||||
import { createRateLimiterSet } from "@lib/rate-limiter";
|
||||
import type { VideoGridComponent } from "@components/VideoGrid";
|
||||
import { createServerBanner } from "@components/ServerBanner";
|
||||
import { createServerBanner, applyConnectionStatus } from "@components/ServerBanner";
|
||||
import type { ServerBannerControl } from "@components/ServerBanner";
|
||||
import { createSettingsOverlay } from "@components/SettingsOverlay";
|
||||
import { createToastContainer } from "@components/Toast";
|
||||
@@ -205,19 +205,18 @@ export function createMainPage(options: MainPageOptions): MountableComponent {
|
||||
(status) => {
|
||||
try {
|
||||
if (banner === null) return;
|
||||
if (status === "reconnecting") {
|
||||
banner.showReconnecting();
|
||||
} else if (status === "disconnected") {
|
||||
banner.showDisconnected();
|
||||
} else {
|
||||
banner.hide();
|
||||
}
|
||||
applyConnectionStatus(banner, status);
|
||||
} catch (err) {
|
||||
log.error("Connection status handler error", err);
|
||||
}
|
||||
},
|
||||
),
|
||||
);
|
||||
// Synchronous initial sync: the selector subscription baselines on the
|
||||
// current value and only fires on change, so a MainPage mounted mid-outage
|
||||
// (status already "reconnecting") would otherwise never show the banner —
|
||||
// the whole retry cycle maps to the same 3-state value.
|
||||
applyConnectionStatus(banner, uiStore.getState().connectionStatus);
|
||||
|
||||
unsubscribers.push(
|
||||
ws.on("server_restart", (payload) => {
|
||||
|
||||
@@ -319,7 +319,9 @@ export function createChannelController(opts: ChannelControllerOptions): Channel
|
||||
// server still enforces block/permission and a refused send shows as a
|
||||
// failed row.
|
||||
const computeComposerReason = (): string | null => {
|
||||
if (uiStore.getState().connectionStatus !== "connected") return "Reconnecting…";
|
||||
const status = uiStore.getState().connectionStatus;
|
||||
if (status === "reconnecting") return "Reconnecting…";
|
||||
if (status === "disconnected") return "Not connected";
|
||||
const ch = channelsStore.getState().channels.get(channelId);
|
||||
if (ch === undefined) return null;
|
||||
if (!ch.canSend) {
|
||||
|
||||
@@ -99,6 +99,12 @@ export function createMessageController(opts: MessageControllerOptions): Message
|
||||
// Inline section error + Retry in the message region (UX spec §2) —
|
||||
// a toast would vanish and leave the region silently empty.
|
||||
setChannelLoadError(channelId);
|
||||
// The inline region only renders when the channel has no rows; live
|
||||
// broadcasts or an optimistic send may already have populated it, in
|
||||
// which case the failure must still be surfaced (no silent drop).
|
||||
if (getChannelMessages(channelId).length > 0) {
|
||||
showError("Failed to load message history");
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -15,6 +15,7 @@ const {
|
||||
mockSetReplyTo,
|
||||
mockStartEdit,
|
||||
mockScrollToMessage,
|
||||
mockSetDisabled,
|
||||
} = vi.hoisted(() => ({
|
||||
mockMessageListMount: vi.fn(),
|
||||
mockMessageListDestroy: vi.fn(),
|
||||
@@ -33,6 +34,7 @@ const {
|
||||
mockSetReplyTo: vi.fn(),
|
||||
mockStartEdit: vi.fn(),
|
||||
mockScrollToMessage: vi.fn(() => true),
|
||||
mockSetDisabled: vi.fn(),
|
||||
}));
|
||||
|
||||
vi.mock("@lib/logger", () => ({
|
||||
@@ -80,7 +82,7 @@ vi.mock("@components/MessageInput", () => ({
|
||||
startEdit: mockStartEdit,
|
||||
clearReply: vi.fn(),
|
||||
cancelEdit: vi.fn(),
|
||||
setDisabled: vi.fn(),
|
||||
setDisabled: mockSetDisabled,
|
||||
};
|
||||
}),
|
||||
}));
|
||||
@@ -366,6 +368,68 @@ describe("createChannelController", () => {
|
||||
expect(mockMarkSendFailed).toHaveBeenCalledWith(expect.any(String), "OFFLINE");
|
||||
});
|
||||
|
||||
it("composer disable reason distinguishes reconnecting from disconnected", () => {
|
||||
const opts = makeOpts();
|
||||
setConnectionStatus("reconnecting");
|
||||
const ctrl = createChannelController(opts);
|
||||
ctrl.mountChannel(42, "general");
|
||||
expect(mockSetDisabled).toHaveBeenLastCalledWith("Reconnecting…");
|
||||
|
||||
ctrl.destroyChannel();
|
||||
setConnectionStatus("disconnected");
|
||||
ctrl.mountChannel(43, "general-2");
|
||||
expect(mockSetDisabled).toHaveBeenLastCalledWith("Not connected");
|
||||
});
|
||||
|
||||
it("onRetryLoad re-invokes loadMessages for the mounted channel", () => {
|
||||
const opts = makeOpts();
|
||||
const ctrl = createChannelController(opts);
|
||||
ctrl.mountChannel(42, "general");
|
||||
(opts.msgCtrl.loadMessages as ReturnType<typeof vi.fn>).mockClear();
|
||||
|
||||
capturedMessageListOpts.onRetryLoad();
|
||||
|
||||
expect(opts.msgCtrl.loadMessages).toHaveBeenCalledWith(42, expect.any(AbortSignal));
|
||||
});
|
||||
|
||||
it("onRetry re-sends the failed draft with a fresh correlation id", () => {
|
||||
const opts = makeOpts();
|
||||
let n = 0;
|
||||
(opts.ws.send as ReturnType<typeof vi.fn>).mockImplementation(() => `cid-${++n}`);
|
||||
const ctrl = createChannelController(opts);
|
||||
ctrl.mountChannel(42, "general");
|
||||
|
||||
// cid-1 is channel_focus; the chat_send gets cid-2.
|
||||
capturedMessageInputOpts.onSend("hello", null, []);
|
||||
expect(mockAddOptimistic).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ correlationId: "cid-2", content: "hello" }),
|
||||
);
|
||||
|
||||
capturedMessageListOpts.onRetry("cid-2");
|
||||
|
||||
// The old row is discarded and the draft re-sent under a new id.
|
||||
expect(mockRemoveOptimistic).toHaveBeenCalledWith("cid-2");
|
||||
expect(mockAddOptimistic).toHaveBeenLastCalledWith(
|
||||
expect.objectContaining({ correlationId: "cid-3", content: "hello" }),
|
||||
);
|
||||
});
|
||||
|
||||
it("onDeleteDraft discards the failed row without re-sending", () => {
|
||||
const opts = makeOpts();
|
||||
let n = 0;
|
||||
(opts.ws.send as ReturnType<typeof vi.fn>).mockImplementation(() => `cid-${++n}`);
|
||||
const ctrl = createChannelController(opts);
|
||||
ctrl.mountChannel(42, "general");
|
||||
|
||||
capturedMessageInputOpts.onSend("hello", null, []);
|
||||
const sendCalls = (opts.ws.send as ReturnType<typeof vi.fn>).mock.calls.length;
|
||||
|
||||
capturedMessageListOpts.onDeleteDraft("cid-2");
|
||||
|
||||
expect(mockRemoveOptimistic).toHaveBeenCalledWith("cid-2");
|
||||
expect((opts.ws.send as ReturnType<typeof vi.fn>).mock.calls.length).toBe(sendCalls);
|
||||
});
|
||||
|
||||
it("onTyping sends typing_start via ws", () => {
|
||||
const opts = makeOpts();
|
||||
const ctrl = createChannelController(opts);
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
|
||||
import { wireDispatcher } from "../../src/lib/dispatcher";
|
||||
import { wireDispatcher, wireConnectionStatus } from "../../src/lib/dispatcher";
|
||||
import { createMockWsClient } from "../helpers/mock-ws";
|
||||
import { authStore, clearAuth } from "../../src/stores/auth.store";
|
||||
import { channelsStore } from "../../src/stores/channels.store";
|
||||
import {
|
||||
@@ -1254,3 +1255,30 @@ describe("WS Dispatcher", () => {
|
||||
expect(messagesStore.getState().messagesByChannel.get(1)).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
describe("wireConnectionStatus", () => {
|
||||
beforeEach(() => {
|
||||
uiStore.setState((prev) => ({ ...prev, connectionStatus: "disconnected" }));
|
||||
});
|
||||
|
||||
it("writes ws state changes into ui.store.connectionStatus via the 5→3 mapping", () => {
|
||||
const mockWs = createMockWsClient();
|
||||
const unsub = wireConnectionStatus(mockWs);
|
||||
|
||||
mockWs.simulateStateChange("connecting");
|
||||
expect(uiStore.getState().connectionStatus).toBe("reconnecting");
|
||||
|
||||
mockWs.simulateStateChange("authenticating");
|
||||
expect(uiStore.getState().connectionStatus).toBe("reconnecting");
|
||||
|
||||
mockWs.simulateStateChange("connected");
|
||||
expect(uiStore.getState().connectionStatus).toBe("connected");
|
||||
|
||||
mockWs.simulateStateChange("disconnected");
|
||||
expect(uiStore.getState().connectionStatus).toBe("disconnected");
|
||||
|
||||
unsub();
|
||||
mockWs.simulateStateChange("connected");
|
||||
expect(uiStore.getState().connectionStatus).toBe("disconnected");
|
||||
});
|
||||
});
|
||||
|
||||
@@ -145,6 +145,22 @@ describe("createMessageController", () => {
|
||||
expect(showError).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("falls back to a toast on failure when the channel already has rows", async () => {
|
||||
// Live broadcasts or an optimistic send can populate a channel before
|
||||
// history loads; the inline region won't render then, so the failure
|
||||
// must surface as a toast instead of silently.
|
||||
mockGetChannelMessages.mockReturnValue([{ id: 5, content: "live row" }]);
|
||||
const api = makeApi({
|
||||
getMessages: vi.fn().mockRejectedValue(new Error("network error")),
|
||||
});
|
||||
const ctrl = createMessageController({ api, showError });
|
||||
|
||||
await ctrl.loadMessages(42, makeAbort().signal);
|
||||
|
||||
expect(mockSetChannelLoadError).toHaveBeenCalledWith(42);
|
||||
expect(showError).toHaveBeenCalledWith("Failed to load message history");
|
||||
});
|
||||
|
||||
it("does not mark an error when aborted before failure", async () => {
|
||||
const { signal, abort } = makeAbort();
|
||||
abort();
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
|
||||
import { createServerBanner } from "@components/ServerBanner";
|
||||
import { createServerBanner, applyConnectionStatus } from "@components/ServerBanner";
|
||||
|
||||
describe("ServerBanner", () => {
|
||||
beforeEach(() => {
|
||||
@@ -47,6 +47,33 @@ describe("ServerBanner", () => {
|
||||
banner.destroy();
|
||||
});
|
||||
|
||||
it('showDisconnected adds visible class with "Disconnected" text', () => {
|
||||
const banner = createServerBanner();
|
||||
banner.showDisconnected();
|
||||
|
||||
expect(banner.element.classList.contains("visible")).toBe(true);
|
||||
expect(banner.element.textContent).toBe("Disconnected");
|
||||
|
||||
banner.destroy();
|
||||
});
|
||||
|
||||
it("applyConnectionStatus maps each store status to the right banner state", () => {
|
||||
const banner = createServerBanner();
|
||||
|
||||
applyConnectionStatus(banner, "reconnecting");
|
||||
expect(banner.element.classList.contains("visible")).toBe(true);
|
||||
expect(banner.element.textContent).toBe("Reconnecting...");
|
||||
|
||||
applyConnectionStatus(banner, "disconnected");
|
||||
expect(banner.element.classList.contains("visible")).toBe(true);
|
||||
expect(banner.element.textContent).toBe("Disconnected");
|
||||
|
||||
applyConnectionStatus(banner, "connected");
|
||||
expect(banner.element.classList.contains("visible")).toBe(false);
|
||||
|
||||
banner.destroy();
|
||||
});
|
||||
|
||||
it("countdown decrements every second", () => {
|
||||
const banner = createServerBanner();
|
||||
banner.showRestart(3);
|
||||
|
||||
@@ -1104,6 +1104,18 @@ describe("SidebarArea", () => {
|
||||
cleanup(result);
|
||||
});
|
||||
|
||||
it("passes the ws client to the user bar (presence picker send path)", () => {
|
||||
const opts = defaultOpts();
|
||||
const result = createSidebarArea(opts);
|
||||
container.appendChild(result.sidebarWrapper);
|
||||
|
||||
// Without ws, the status picker is permanently disabled and
|
||||
// presence_update can never be sent — this pins the fix.
|
||||
expect(createUserBar).toHaveBeenCalledWith(expect.objectContaining({ ws: opts.ws }));
|
||||
|
||||
cleanup(result);
|
||||
});
|
||||
|
||||
it("voice widget and user bar are included in children", () => {
|
||||
const result = createSidebarArea(defaultOpts());
|
||||
expect(result.children.length).toBe(2);
|
||||
|
||||
@@ -85,14 +85,16 @@ source of truth in `ui.store.connectionStatus`
|
||||
> the internal 5-state machine onto the 3-state status (`connecting` /
|
||||
> `authenticating` read as `reconnecting`, since a reconnect cycle passes
|
||||
> through them). Consumers subscribe to the store instead of wiring ad-hoc
|
||||
> callbacks: the reconnect banner (`MainPage`, which now also shows
|
||||
> "Disconnected" instead of going stale), the composer gating
|
||||
> (`ChannelController`), and the presence picker (`UserBar` — previously dead in
|
||||
> production because `SidebarArea` never passed it a `ws`; it now gates on the
|
||||
> store and receives the `ws` send path). The one-shot connected-overlay wiring
|
||||
> in `main.ts` stays on `ws.onStateChange` deliberately — it needs the exact
|
||||
> internal transition. Voice controls remain independent: LiveKit reconnection
|
||||
> "retries underneath" per the table below.
|
||||
> callbacks: the reconnect banner (`MainPage`, synced at mount and now also
|
||||
> showing "Disconnected" instead of going stale), the composer gating
|
||||
> (`ChannelController`, "Reconnecting…" / "Not connected" per the table), and
|
||||
> the presence picker (`UserBar` — previously dead in production because
|
||||
> `SidebarArea` never passed it a `ws`; it now gates on the store and receives
|
||||
> the `ws` send path). The one-shot connected-overlay wiring in `main.ts` stays
|
||||
> on `ws.onStateChange` deliberately — it needs the exact internal transition.
|
||||
> **Remaining gap:** the table's voice column. Voice controls are not yet
|
||||
> frozen during a WS reconnect — LiveKit reconnection retries underneath, but
|
||||
> join/leave controls stay enabled and would send over the down socket.
|
||||
|
||||
| Status | Composer / send | Voice controls | Presence picker | Reconnect banner |
|
||||
|--------|-----------------|----------------|-----------------|------------------|
|
||||
|
||||
@@ -59,7 +59,7 @@ stateDiagram-v2
|
||||
| `enabled` | Editable textarea, attach + pickers active | — |
|
||||
| `read-only` (announcement, no MANAGE_MESSAGES) | Textarea replaced by a disabled bar | "Only moderators can post in announcement channels." |
|
||||
| `no-permission` | Disabled bar | "You don't have permission to send messages here." |
|
||||
| `offline` | Disabled, "Reconnecting…" | connection status (README §3) |
|
||||
| `offline` | Disabled — "Reconnecting…" while retrying, "Not connected" when disconnected | connection status (README §3) |
|
||||
| `slow-mode` | Disabled with a live countdown | "Slow mode: wait Ns." |
|
||||
| `uploading` | Send disabled until uploads settle (already `MessageInput.ts:138-141`) | per-attachment spinner |
|
||||
|
||||
@@ -100,7 +100,7 @@ sequenceDiagram
|
||||
SRV-->>WS: error{code} %% SLOW_MODE / RATE_LIMITED / FORBIDDEN / INVALID_INPUT
|
||||
WS->>S: markSendFailed(correlationId, code) %% row → "failed", Retry
|
||||
else transport drop
|
||||
WS-->>S: markSendFailed(correlationId, "NETWORK") %% ws_send channel-full/closed
|
||||
WS-->>S: markSendFailed(correlationId, code) %% channel full → "NETWORK"; closed/not-open → "OFFLINE"
|
||||
end
|
||||
```
|
||||
|
||||
|
||||
Reference in New Issue
Block a user