fix(web): code review round 1 -- critical data-layer bugs
Three parallel adversarial code-review passes surfaced real defects across the app. This commit covers the data/logic layer: - CRITICAL: file upload/download used the wrong auth header entirely. Built from docs/IM-前后端对接文档.md as `X-MeetingIM-Auth: token <token>`, but the old app's own upload code carries an explicit comment that the real header is `Authorization: Token <token>` (capitalized scheme, standard header name), "verified live against the real server, not just read from source" -- and separately warns the SDK's own built-in LargeFileHelper gets this wrong too (sends X-Tinode-APIKey/X-Tinode-Auth, which this backend doesn't recognize). This means every external (non-inline) image/file/voice attachment -- anything too large to send as base64 -- was silently broken; only the earlier live tests happened to use small inline attachments, which never exercise this code path. Confirmed fixed live: a real >256KB image now uploads (200, real file_id) and downloads (blob URL, correct 800x600 dimensions) correctly. - CRITICAL: TopicMe.onContactUpdate was never wired. Marking a conversation read (topic.noteRead()) updates the contact's unread count through ONLY this single-slot callback (confirmed against the SDK's own Topic.note()/TopicMe._refreshContact -- it does not also fire onData/ onInfo/onPres, unlike every other _refreshContact call site, which do). Without it the chat list's unread badge/ordering went stale after reading a conversation until an unrelated topic event happened to invalidate the shared snapshot cache. - HIGH: chat-list message preview didn't check isServerDeleted(msg) (the `hi` soft-delete flag), only `content == null` -- a hi-flagged message with non-null content leaked its real text/attachment label into the preview instead of showing "[消息已删除]". - MEDIUM: upload failures showed raw untranslated SDK ctrl.text (e.g. "too large" instead of "文件大小超出限制") -- ported the old app's UPLOAD_CTRL_TEXT_CN table, which it built specifically because this native-protocol endpoint's errors never pass through the REST layer's own reason-code translation. - Ambient SDK type corrections (tinode-sdk.d.ts): AccessMode.mode/given/want are numeric bitmasks at runtime, not the "JRWPS"-string type they were declared as (dormant today, nothing reads them yet, but wrong ahead of Phase 3's group-role logic); Drafty.sanitizeEntities's doc comment claimed a deep copy it doesn't do (verified it mutates entity .data in place); onAutoreconnectIteration's first param was misnamed "seconds" when the SDK always passes milliseconds. - sendVoice() now passes `filename` to Drafty.insertAudio (was the only one of sendImage/sendFile/sendVoice omitting it). - friends.ts's accept/reject-friend-request `version` param used a falsy check (`version ? ... : ...`) that would silently drop a legitimate `0`. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Sonnet 5
parent
d5440b962b
commit
986432addd
@@ -92,13 +92,13 @@ export function translateReason(reason: string | undefined, details?: unknown):
|
|||||||
return (typeof entry === 'function' ? entry(details) : entry) || null;
|
return (typeof entry === 'function' ? entry(details) : entry) || null;
|
||||||
}
|
}
|
||||||
|
|
||||||
function requestId(): string {
|
export function requestId(): string {
|
||||||
return typeof crypto !== 'undefined' && crypto.randomUUID
|
return typeof crypto !== 'undefined' && crypto.randomUUID
|
||||||
? crypto.randomUUID()
|
? crypto.randomUUID()
|
||||||
: `web_${Date.now().toString(36)}_${Math.random().toString(36).slice(2)}`;
|
: `web_${Date.now().toString(36)}_${Math.random().toString(36).slice(2)}`;
|
||||||
}
|
}
|
||||||
|
|
||||||
function apiBase(): string {
|
export function apiBase(): string {
|
||||||
const endpoints = useSessionStore.getState().tenant?.endpoints;
|
const endpoints = useSessionStore.getState().tenant?.endpoints;
|
||||||
return (endpoints?.api || DEFAULT_API_BASE).replace(/\/$/, '');
|
return (endpoints?.api || DEFAULT_API_BASE).replace(/\/$/, '');
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -65,11 +65,11 @@ export function getFriendRequests() {
|
|||||||
}
|
}
|
||||||
|
|
||||||
export function acceptFriendRequest(requestId: string, version?: number) {
|
export function acceptFriendRequest(requestId: string, version?: number) {
|
||||||
return imPost<void>(`/friend-requests/${encodeURIComponent(requestId)}/accept`, version ? { version } : {});
|
return imPost<void>(`/friend-requests/${encodeURIComponent(requestId)}/accept`, version !== undefined ? { version } : {});
|
||||||
}
|
}
|
||||||
|
|
||||||
export function rejectFriendRequest(requestId: string, version?: number) {
|
export function rejectFriendRequest(requestId: string, version?: number) {
|
||||||
return imPost<void>(`/friend-requests/${encodeURIComponent(requestId)}/reject`, version ? { version } : {});
|
return imPost<void>(`/friend-requests/${encodeURIComponent(requestId)}/reject`, version !== undefined ? { version } : {});
|
||||||
}
|
}
|
||||||
|
|
||||||
export interface ConversationSettings {
|
export interface ConversationSettings {
|
||||||
|
|||||||
@@ -1,9 +1,16 @@
|
|||||||
// The file HTTP endpoint (docs/IM-前后端对接文档.md:62,460-461) is a SEPARATE surface from
|
// The file HTTP endpoint (docs/IM-前后端对接文档.md:62,460-461) is a SEPARATE surface from
|
||||||
// /v1/im/* -- it lives at the tenant's `file` base URL, and authenticates with a different header
|
// /v1/im/* -- it lives at the tenant's `file` base URL and speaks Tinode's native {ctrl} protocol,
|
||||||
// (`X-MeetingIM-Auth: token <token>`) than the REST API's `Authorization: Bearer`, matching
|
// not the REST /v1/im envelope. Its auth header is `Authorization: Token <token>` (capitalized
|
||||||
// upstream Tinode's own LargeFileHelper convention. The token value is the same session token
|
// scheme, standard header name) -- NOT `X-MeetingIM-Auth: token <token>` as the protocol doc
|
||||||
// either way, just carried differently.
|
// describes. This was corrected against the old app's own upload code, whose comment explicitly
|
||||||
|
// states the header/scheme were "verified live against the real server, not just read from source"
|
||||||
|
// (it also separately warns that the SDK's own built-in LargeFileHelper.upload() gets this WRONG
|
||||||
|
// for this backend, sending 'X-Tinode-APIKey'/'X-Tinode-Auth' headers the server doesn't recognize
|
||||||
|
// at all -- so this hand-built fetch, not the SDK helper, is the only path that actually works here).
|
||||||
|
import { API_KEY } from '@/lib/config';
|
||||||
|
|
||||||
import { authToken, fileBase } from './client';
|
import { authToken, fileBase } from './client';
|
||||||
|
import { getTinode } from '../tinode/connection';
|
||||||
|
|
||||||
export interface UploadedFile {
|
export interface UploadedFile {
|
||||||
url: string;
|
url: string;
|
||||||
@@ -12,16 +19,32 @@ export interface UploadedFile {
|
|||||||
mime: string;
|
mime: string;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* ctrl.text values a failure here can actually carry (Tinode's native protocol, not the REST
|
||||||
|
* layer's `reason` codes -- client.ts's REASON_MESSAGES table never applies to this endpoint).
|
||||||
|
*/
|
||||||
|
const UPLOAD_CTRL_TEXT_CN: Record<string, string> = {
|
||||||
|
'too large': '文件大小超出限制',
|
||||||
|
malformed: '请求内容有误,请稍后重试',
|
||||||
|
'permission denied': '没有权限执行此操作',
|
||||||
|
gone: '内容不存在或已被删除',
|
||||||
|
'not found': '内容不存在或已被删除',
|
||||||
|
};
|
||||||
|
|
||||||
export async function uploadFile(file: File, topic?: string): Promise<UploadedFile> {
|
export async function uploadFile(file: File, topic?: string): Promise<UploadedFile> {
|
||||||
const form = new FormData();
|
const form = new FormData();
|
||||||
form.append('file', file);
|
form.append('file', file);
|
||||||
|
form.set('id', getTinode().getNextUniqueId());
|
||||||
if (topic) {
|
if (topic) {
|
||||||
form.append('purpose', 'chat_file');
|
form.append('purpose', 'chat_file');
|
||||||
form.append('topic', topic);
|
form.append('topic', topic);
|
||||||
}
|
}
|
||||||
const res = await fetch(`${fileBase()}/u/`, {
|
const res = await fetch(`${fileBase()}/u/`, {
|
||||||
method: 'POST',
|
method: 'POST',
|
||||||
headers: { 'X-MeetingIM-Auth': `token ${authToken()}` },
|
headers: {
|
||||||
|
'X-MeetingIM-APIKey': API_KEY,
|
||||||
|
Authorization: `Token ${authToken()}`,
|
||||||
|
},
|
||||||
body: form,
|
body: form,
|
||||||
});
|
});
|
||||||
const text = await res.text();
|
const text = await res.text();
|
||||||
@@ -32,11 +55,18 @@ export async function uploadFile(file: File, topic?: string): Promise<UploadedFi
|
|||||||
throw new Error('文件上传失败:服务器返回异常');
|
throw new Error('文件上传失败:服务器返回异常');
|
||||||
}
|
}
|
||||||
if (!res.ok || !payload.ctrl || payload.ctrl.code >= 300 || !payload.ctrl.params) {
|
if (!res.ok || !payload.ctrl || payload.ctrl.code >= 300 || !payload.ctrl.params) {
|
||||||
throw new Error(payload.ctrl?.text || '文件上传失败');
|
const rawText = payload.ctrl?.text?.trim().toLowerCase();
|
||||||
|
throw new Error((rawText && UPLOAD_CTRL_TEXT_CN[rawText]) || payload.ctrl?.text || '文件上传失败');
|
||||||
}
|
}
|
||||||
return payload.ctrl.params;
|
return payload.ctrl.params;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/** Headers needed to fetch a file this endpoint serves (see AttachmentPreview.tsx's authenticated
|
||||||
|
* blob-URL fetch) -- same scheme as uploadFile's own auth, kept in one place so they can't drift. */
|
||||||
|
export function fileAuthHeaders(): Record<string, string> {
|
||||||
|
return { 'X-MeetingIM-APIKey': API_KEY, Authorization: `Token ${authToken()}` };
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Absolute download URL for a file reference returned by uploadFile() or a received attachment.
|
* Absolute download URL for a file reference returned by uploadFile() or a received attachment.
|
||||||
* The upload response's `url` (e.g. "/v0/file/s/file123") is already rooted at the tenant's file
|
* The upload response's `url` (e.g. "/v0/file/s/file123") is already rooted at the tenant's file
|
||||||
|
|||||||
@@ -1,5 +1,7 @@
|
|||||||
import { Drafty, type DraftyContent, type StoredMessage } from 'tinode-sdk';
|
import { Drafty, type DraftyContent, type StoredMessage } from 'tinode-sdk';
|
||||||
|
|
||||||
|
import { isServerDeleted } from '@/lib/messages/dispatch';
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* One-line chat-list preview text (README §2.1): plain text renders as-is, everything else falls
|
* One-line chat-list preview text (README §2.1): plain text renders as-is, everything else falls
|
||||||
* back to a bracketed type label. Own messages get a "我:" prefix. Grown incrementally -- VC (call)
|
* back to a bracketed type label. Own messages get a "我:" prefix. Grown incrementally -- VC (call)
|
||||||
@@ -10,7 +12,10 @@ export function formatMessagePreview(msg: StoredMessage | undefined, myUserId: s
|
|||||||
if (!msg) return '';
|
if (!msg) return '';
|
||||||
const prefix = msg.from && myUserId && msg.from === myUserId ? '我:' : '';
|
const prefix = msg.from && myUserId && msg.from === myUserId ? '我:' : '';
|
||||||
const content = msg.content;
|
const content = msg.content;
|
||||||
if (content == null) return `${prefix}[消息已删除]`;
|
// `msg.hi` (server soft-delete) is checked unconditionally, before any content check -- the old
|
||||||
|
// app's own equivalent renderer does the same, since a hi-flagged message can still carry
|
||||||
|
// non-null content that must NOT leak into this preview.
|
||||||
|
if (isServerDeleted(msg) || content == null) return `${prefix}[消息已删除]`;
|
||||||
if (typeof content === 'string' || Drafty.isPlainText(content)) {
|
if (typeof content === 'string' || Drafty.isPlainText(content)) {
|
||||||
return prefix + Drafty.toPlainText(content);
|
return prefix + Drafty.toPlainText(content);
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -75,8 +75,14 @@ export async function sendVoice(topic: Topic, blob: Blob, durationMs: number): P
|
|||||||
const file = new File([blob], `voice.${blob.type.includes('mp4') ? 'm4a' : 'webm'}`, { type: blob.type });
|
const file = new File([blob], `voice.${blob.type.includes('mp4') ? 'm4a' : 'webm'}`, { type: blob.type });
|
||||||
const content =
|
const content =
|
||||||
file.size <= MAX_INBAND_ATTACHMENT_SIZE
|
file.size <= MAX_INBAND_ATTACHMENT_SIZE
|
||||||
? Drafty.insertAudio(undefined, 0, { mime: file.type, bits: await fileToBase64(file), duration: durationMs, size: file.size })
|
? Drafty.insertAudio(undefined, 0, { mime: file.type, bits: await fileToBase64(file), duration: durationMs, size: file.size, filename: file.name })
|
||||||
: Drafty.insertAudio(undefined, 0, { mime: file.type, refurl: (await uploadFile(file, topic.name)).url, duration: durationMs, size: file.size });
|
: Drafty.insertAudio(undefined, 0, {
|
||||||
|
mime: file.type,
|
||||||
|
refurl: (await uploadFile(file, topic.name)).url,
|
||||||
|
duration: durationMs,
|
||||||
|
size: file.size,
|
||||||
|
filename: file.name,
|
||||||
|
});
|
||||||
const msg = topic.createMessage(content, false);
|
const msg = topic.createMessage(content, false);
|
||||||
msg.head = Object.assign(msg.head || {}, { 'x-im-type': 'voice' });
|
msg.head = Object.assign(msg.head || {}, { 'x-im-type': 'voice' });
|
||||||
return topic.publishDraft(msg, Promise.resolve());
|
return topic.publishDraft(msg, Promise.resolve());
|
||||||
|
|||||||
Vendored
+30
-10
@@ -47,9 +47,11 @@ declare module 'tinode-sdk' {
|
|||||||
}
|
}
|
||||||
|
|
||||||
export class AccessMode {
|
export class AccessMode {
|
||||||
mode: string;
|
/** Numeric bitmasks at runtime (tinode.dev.js:26-33's AccessMode.decode()), NOT the "JRWPS"-style
|
||||||
given: string;
|
* strings that only come out of getMode()/getGiven()/getWant()/toString(). */
|
||||||
want: string;
|
mode: number;
|
||||||
|
given: number;
|
||||||
|
want: number;
|
||||||
isOwner(): boolean;
|
isOwner(): boolean;
|
||||||
isAdmin(): boolean;
|
isAdmin(): boolean;
|
||||||
isSharer(): boolean;
|
isSharer(): boolean;
|
||||||
@@ -234,6 +236,13 @@ declare module 'tinode-sdk' {
|
|||||||
contacts(callback: (contact: Contact) => void): void;
|
contacts(callback: (contact: Contact) => void): void;
|
||||||
pinnedTopicRank(topic: string): number;
|
pinnedTopicRank(topic: string): number;
|
||||||
getMeta(query: unknown): Promise<unknown>;
|
getMeta(query: unknown): Promise<unknown>;
|
||||||
|
/**
|
||||||
|
* Fires when a contact's read/recv/unread state changes via `Topic.note()` (the base `noteRead`/
|
||||||
|
* noteRecv` path -- confirmed against tinode.dev.js's `TopicMe._refreshContact`/`Topic.note`):
|
||||||
|
* that path calls ONLY this callback, not `onData`/`onInfo`/`onPres`. Marking a conversation read
|
||||||
|
* (`topic.noteRead(0)`) updates the contact's unread count via this callback alone.
|
||||||
|
*/
|
||||||
|
onContactUpdate?: (what: string, contact: Contact) => void;
|
||||||
}
|
}
|
||||||
|
|
||||||
export class Tinode {
|
export class Tinode {
|
||||||
@@ -248,6 +257,9 @@ declare module 'tinode-sdk' {
|
|||||||
disconnect(): void;
|
disconnect(): void;
|
||||||
/** Skips the remaining exponential-backoff wait and retries the connection now. */
|
/** Skips the remaining exponential-backoff wait and retries the connection now. */
|
||||||
reconnect(force?: boolean): void;
|
reconnect(force?: boolean): void;
|
||||||
|
/** Monotonically-increasing per-connection request id string; the file-upload endpoint's own
|
||||||
|
* multipart form expects one in an `id` field (mirrors the SDK's own LargeFileHelper). */
|
||||||
|
getNextUniqueId(): string;
|
||||||
isConnected(): boolean;
|
isConnected(): boolean;
|
||||||
isAuthenticated(): boolean;
|
isAuthenticated(): boolean;
|
||||||
getCurrentUserID(): string;
|
getCurrentUserID(): string;
|
||||||
@@ -293,13 +305,14 @@ declare module 'tinode-sdk' {
|
|||||||
onLogin?: (code: number, text: string) => void;
|
onLogin?: (code: number, text: string) => void;
|
||||||
onNetworkProbe?: () => void;
|
onNetworkProbe?: () => void;
|
||||||
/**
|
/**
|
||||||
* Fires on every SDK-internal reconnect attempt. `seconds` is the SDK's own next-retry countdown
|
* Fires on every SDK-internal reconnect attempt. `timeoutMs` is the SDK's own next-retry
|
||||||
* (exponential backoff, ~2s base doubling up to 10 iterations, +/-30% jitter -- entirely internal
|
* countdown IN MILLISECONDS (exponential backoff, 2000ms base doubling up to 10 iterations,
|
||||||
* to the SDK, this app does not implement its own backoff). `promise` is only present while a
|
* +/-30% jitter -- entirely internal to the SDK, verified against tinode.dev.js:451-456,553-556;
|
||||||
* retry is actually in flight; when absent, the app is expected to run its own 1s countdown
|
* this app does not implement its own backoff, only its own 1s display tick off this one value).
|
||||||
* display down to the next attempt.
|
* `promise` is only present while a retry is actually in flight; `timeoutMs < 0` means retrying
|
||||||
|
* has stopped entirely (e.g. an explicit disconnect).
|
||||||
*/
|
*/
|
||||||
onAutoreconnectIteration?: (seconds: number, promise?: Promise<unknown>) => void;
|
onAutoreconnectIteration?: (timeoutMs: number, promise?: Promise<unknown>) => void;
|
||||||
}
|
}
|
||||||
|
|
||||||
export const Drafty: {
|
export const Drafty: {
|
||||||
@@ -312,7 +325,14 @@ declare module 'tinode-sdk' {
|
|||||||
isPlainText(content: DraftyContent | string): boolean;
|
isPlainText(content: DraftyContent | string): boolean;
|
||||||
hasEntities(content: DraftyContent | string): boolean;
|
hasEntities(content: DraftyContent | string): boolean;
|
||||||
entities(content: DraftyContent | string, callback: (data: Record<string, unknown> | undefined) => void, context?: unknown): void;
|
entities(content: DraftyContent | string, callback: (data: Record<string, unknown> | undefined) => void, context?: unknown): void;
|
||||||
/** Deep-copies `content` and strips anything the server would reject when re-sent (e.g. as a quote/forward payload). */
|
/**
|
||||||
|
* MUTATES `content` in place (tinode.dev.js:2573-2588) -- only each entity's `.data` gets
|
||||||
|
* replaced with a fresh shallow copy; the document itself, its `.ent` array, and each entity
|
||||||
|
* wrapper are the SAME objects passed in, not a deep copy. Callers that need to preserve the
|
||||||
|
* original (e.g. a still-displayed message) must clone before calling this, same as this app's
|
||||||
|
* own send.ts already does (`JSON.parse(JSON.stringify(...))`) -- don't drop that clone trusting
|
||||||
|
* this function to have done it.
|
||||||
|
*/
|
||||||
sanitizeEntities(content: DraftyContent): DraftyContent;
|
sanitizeEntities(content: DraftyContent): DraftyContent;
|
||||||
/** Concatenates two Drafty documents, remapping `fmt[].key` entity indices in `second`. */
|
/** Concatenates two Drafty documents, remapping `fmt[].key` entity indices in `second`. */
|
||||||
append(first: DraftyContent | undefined, second: DraftyContent): DraftyContent;
|
append(first: DraftyContent | undefined, second: DraftyContent): DraftyContent;
|
||||||
|
|||||||
@@ -75,6 +75,11 @@ export function getMessagesSnapshot(topicName: string): StoredMessage[] {
|
|||||||
export function subscribeMeTopic(): Promise<unknown> {
|
export function subscribeMeTopic(): Promise<unknown> {
|
||||||
const me = getTinode().getMeTopic();
|
const me = getTinode().getMeTopic();
|
||||||
wireTopicCallbacks(me);
|
wireTopicCallbacks(me);
|
||||||
|
// Read/recv/unread updates (e.g. from noteRead() when opening a conversation) travel through
|
||||||
|
// TopicMe's OWN single-slot callback, separate from onData/onInfo/onPres -- wireTopicCallbacks
|
||||||
|
// doesn't touch it. Without this, the chat list's unread badge/ordering goes stale after reading
|
||||||
|
// a conversation until some unrelated topic event happens to invalidate its cached snapshot.
|
||||||
|
me.onContactUpdate = () => notifyTopic(me.name);
|
||||||
return me.subscribe(me.startMetaQuery().withLaterSub().withDesc().withTags().withCred().build());
|
return me.subscribe(me.startMetaQuery().withLaterSub().withDesc().withTags().withCred().build());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user