fix(send): address extension review findings (#4271)

2026-05-22 23:31:04 +08:00
parent 9fa7cb266c
commit 81bd5ee6b8
14 changed files with 146 additions and 15 deletions
@@ -116,6 +116,13 @@ function localeFiles() {
  return codes.map((code) => ({ code, path: join(LOCALES_DIR, `${code}.json`) }));
 }

+function chromeLocaleCode(code) {
+  // Chrome's native i18n directories use underscore region separators
+  // (`pt_BR`, `zh_CN`, `zh_TW`) while the app locale list uses BCP-47
+  // (`pt-BR`, `zh-CN`, `zh-TW`).
+  return code.replace(/-/g, '_');
+}
+
 /**
 * Mirror the locale list into Chrome's native `_locales/<lang>/messages.json`
 * so each locale carries the manifest-side translations (extension name,
@@ -132,7 +139,7 @@ function seedChromeLocales() {
  const codes = loadLangs();
  for (const code of codes) {
    if (code === 'en') continue;
-    const path = join(CHROME_LOCALES_DIR, code, 'messages.json');
+    const path = join(CHROME_LOCALES_DIR, chromeLocaleCode(code), 'messages.json');
    if (existsSync(path)) continue; // never overwrite a translated bundle
    const stub = {};
    for (const [key, entry] of Object.entries(enBundle)) {
@@ -152,7 +159,7 @@ function seedChromeLocales() {
 function writeIndex(codes) {
  const sorted = [...codes].sort();
  const importLines = sorted.map((c) => `import ${jsIdent(c)}Messages from './${c}.json';`);
-  const mapEntries = sorted.map((c) => `  '${c}': ${jsIdent(c)}Messages as Messages,`);
+  const mapEntries = sorted.map((c) => `  ${jsObjectKey(c)}: ${jsIdent(c)}Messages as Messages,`);
  const out = `// AUTO-GENERATED by \`pnpm i18n:extract\`. Do not edit by hand —
 // re-run the extractor whenever a locale is added or removed in
 // \`apps/readest-app/i18n-langs.json\`. The runtime in
@@ -175,6 +182,10 @@ function jsIdent(code) {
  return code.replace(/[-_](\w)/g, (_m, c) => c.toUpperCase());
 }

+function jsObjectKey(code) {
+  return /^[A-Za-z_$][\w$]*$/.test(code) ? code : `'${code}'`;
+}
+
 function main() {
  const check = process.argv.includes('--check');
  const keys = extract();
@@ -41,6 +41,9 @@ export interface ChromeMock {
    onConnect: { addListener: Mock };
    lastError: chrome.runtime.LastError | undefined;
  };
+  i18n: {
+    getUILanguage: Mock;
+  };
 }

 /** Reset all spies and seed a fresh `storage.local` and `storage.session`. */
@@ -100,6 +103,9 @@ export function installChromeMock(): ChromeMock {
      onConnect: { addListener: vi.fn() },
      lastError: undefined,
    },
+    i18n: {
+      getUILanguage: vi.fn(() => 'en'),
+    },
  };

  (globalThis as unknown as { chrome: ChromeMock }).chrome = chromeMock;
@@ -16,6 +16,7 @@ let chromeMock: ChromeMock;
 let addEventListenerSpy: ReturnType<typeof vi.spyOn>;

 beforeEach(() => {
+  vi.useFakeTimers();
  chromeMock = installChromeMock();
  localStorage.clear();
  addEventListenerSpy = vi.spyOn(window, 'addEventListener');
@@ -23,6 +24,8 @@ beforeEach(() => {
 });

 afterEach(() => {
+  vi.clearAllTimers();
+  vi.useRealTimers();
  addEventListenerSpy.mockRestore();
  uninstallChromeMock();
  localStorage.clear();
@@ -101,6 +104,25 @@ describe('auth-bridge — token extraction', () => {
    expect(calls.length).toBe(1);
  });

+  test('polls for same-page token writes that do not fire a storage event', async () => {
+    await import('./auth-bridge');
+    chromeMock.storage.local.set.mockClear();
+    chromeMock.storage.local.remove.mockClear();
+
+    localStorage.setItem(
+      'sb-projectref-auth-token',
+      JSON.stringify({ access_token: 'same-page-token' }),
+    );
+    await vi.advanceTimersByTimeAsync(5_000);
+
+    expect(chromeMock.storage.local.set).toHaveBeenCalledTimes(1);
+    expect(
+      (chromeMock.storage.local.set.mock.calls[0]![0] as Record<string, unknown>)[
+        'readestAccessToken'
+      ],
+    ).toBe('same-page-token');
+  });
+
  test('storage event with a non-sb key is ignored', async () => {
    await import('./auth-bridge');
    chromeMock.storage.local.set.mockClear();
@@ -12,6 +12,9 @@ interface SupabaseAuthValue {
  access_token?: string;
 }

+const TOKEN_SYNC_INTERVAL_MS = 5_000;
+let lastSyncedToken: string | null | undefined;
+
 function findAccessToken(): string | null {
  for (let i = 0; i < localStorage.length; i++) {
    const key = localStorage.key(i);
@@ -32,6 +35,9 @@ function findAccessToken(): string | null {

 function syncToken(): void {
  const token = findAccessToken();
+  if (token === lastSyncedToken) return;
+  lastSyncedToken = token;
+
  if (token) {
    chrome.storage.local.set({
      readestAccessToken: token,
@@ -46,11 +52,19 @@ function syncToken(): void {

 syncToken();

-// Refresh on storage changes too — Supabase rotates the access token a few
-// times an hour. Polling localStorage isn't free, but a `storage` event
-// listener fires only when the value actually changes.
+// Refresh periodically because the browser `storage` event only fires in
+// other same-origin documents, not in the SPA document that performed the
+// Supabase localStorage write. Deduping above keeps this cheap and avoids
+// rewriting extension storage when the token has not changed.
+setInterval(syncToken, TOKEN_SYNC_INTERVAL_MS);
+
+// Still listen for cross-tab updates so a second Readest tab can refresh this
+// content script without waiting for the next poll.
 window.addEventListener('storage', (event) => {
  if (event.key && /^sb-.*-auth-token$/.test(event.key)) {
    syncToken();
  }
 });
+
+window.addEventListener('focus', syncToken);
+window.addEventListener('pageshow', syncToken);
@@ -0,0 +1,39 @@
+import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest';
+import {
+  installChromeMock,
+  uninstallChromeMock,
+  type ChromeMock,
+} from '../__test-utils__/chromeMock';
+
+let chromeMock: ChromeMock;
+
+beforeEach(() => {
+  chromeMock = installChromeMock();
+  vi.resetModules();
+});
+
+afterEach(() => {
+  uninstallChromeMock();
+});
+
+describe('extension i18n locale selection', () => {
+  test('uses exact regional bundles for Chrome hyphen locale codes', async () => {
+    chromeMock.i18n.getUILanguage.mockReturnValue('zh-CN');
+    const { translate } = await import('./i18n');
+    expect(translate('Send to Readest')).toBe('发送到 Readest');
+  });
+
+  test('uses exact regional bundles for Chrome underscore locale codes', async () => {
+    chromeMock.i18n.getUILanguage.mockReturnValue('pt_BR');
+    const { translate } = await import('./i18n');
+    expect(translate('Article is too large to send')).toBe(
+      'O artigo é grande demais para ser enviado',
+    );
+  });
+
+  test('falls back to the base language when no regional bundle exists', async () => {
+    chromeMock.i18n.getUILanguage.mockReturnValue('de-CH');
+    const { translate } = await import('./i18n');
+    expect(translate('Send to Readest')).toBe('An Readest senden');
+  });
+});
@@ -8,8 +8,7 @@
 *     _('Send to Readest')
 *     _('Sent — {count} images could not be fetched.', { count: 3 })
 *
- * Translation tables live at `src/i18n/<locale>.json` (locale = short
- * code from `chrome.i18n.getUILanguage()`). The English bundle is
+ * Translation tables live at `src/locales/<locale>.json`. The English bundle is
 * literally `{}` — fall-through to the key. Non-English bundles are
 * `{ "<english source>": "<translation>" }`.
 *
@@ -45,6 +44,20 @@ function loadBundle(raw: unknown): Messages {
 const bundles: Record<string, Messages> = Object.fromEntries(
  Object.entries(rawBundles).map(([code, raw]) => [code, loadBundle(raw)]),
 );
+const bundleCodeByLower = new Map(Object.keys(bundles).map((code) => [code.toLowerCase(), code]));
+
+function resolveBundleCode(uiLocale: string): string {
+  const normalized = uiLocale.replace(/_/g, '-');
+  const exact = bundleCodeByLower.get(normalized.toLowerCase());
+  if (exact) return exact;
+
+  const language = normalized.split('-')[0]?.toLowerCase();
+  if (language) {
+    const base = bundleCodeByLower.get(language);
+    if (base) return base;
+  }
+  return 'en';
+}

 function pickLocale(): string {
  // `chrome` is undeclared (not just undefined) outside extension
@@ -52,8 +65,7 @@ function pickLocale(): string {
  // (vitest, ad-hoc imports, etc).
  const c = (globalThis as { chrome?: typeof chrome }).chrome;
  const ui = c?.i18n?.getUILanguage?.() ?? 'en';
-  const short = ui.toLowerCase().split('-')[0] ?? 'en';
-  return bundles[short] ? short : 'en';
+  return resolveBundleCode(ui);
 }

 const active: Messages = bundles[pickLocale()] ?? bundles['en'] ?? {};
@@ -23,11 +23,11 @@
    "lint:lua": "node ../readest.koplugin/scripts/lint-koplugin.mjs",
    "test:lua": "node ../readest.koplugin/scripts/test-koplugin.mjs",
    "test": "dotenv -e .env -e .env.test.local -- vitest",
-    "test:extension": "pnpm test extensions/send-to-readest/ --run",
+    "test:extension": "dotenv -e .env -e .env.test.local -- vitest extensions/send-to-readest/ --run",
    "test:coverage": "dotenv -e .env -e .env.test.local -- vitest run --coverage",
    "test:browser": "dotenv -e .env -e .env.test.local -- vitest run --config vitest.browser.config.mts",
    "test:tauri": "bash scripts/test-tauri.sh",
-    "test:pr:web": "pnpm test -- --watch=false && pnpm test:browser && pnpm test:extension",
+    "test:pr:web": "pnpm test -- --watch=false && pnpm test:browser && pnpm test:extension && pnpm build-browser-ext",
    "test:pr:tauri": "bash scripts/test-tauri.sh",
    "test:all": "pnpm test -- --watch=false && pnpm test:browser && bash scripts/test-tauri.sh",
    "test:e2e": "wdio run wdio.conf.ts",
@@ -17,8 +17,10 @@ vi.mock('@/utils/cors', () => ({
 }));

 const putObjectMock = vi.fn();
+const deleteObjectMock = vi.fn();
 vi.mock('@/utils/object', () => ({
  putObject: (...args: unknown[]) => putObjectMock(...args),
+  deleteObject: (...args: unknown[]) => deleteObjectMock(...args),
 }));

 const insertMock = vi.fn();
@@ -115,6 +117,7 @@ const VALID_HEADERS = {
 beforeEach(() => {
  validateUserMock.mockReset().mockResolvedValue({ user: validUser });
  putObjectMock.mockReset().mockResolvedValue(undefined);
+  deleteObjectMock.mockReset().mockResolvedValue(undefined);
  countMock.mockReset().mockResolvedValue({ count: 0, error: null });
  insertMock.mockReset().mockResolvedValue({ data: { id: 'inbox-1' }, error: null });
  updateMock.mockReset().mockResolvedValue({ error: null });
@@ -222,5 +225,20 @@ describe('POST /api/send/inbox/file', () => {

    expect(res._status).toBe(500);
    expect(deleteMock).toHaveBeenCalledTimes(1);
+    expect(deleteObjectMock).not.toHaveBeenCalled();
+  });
+
+  test('rolls back the inbox row and stored payload when payload_key update fails', async () => {
+    updateMock.mockResolvedValueOnce({ error: { message: 'update failed' } });
+    const req = makeReq({ ...VALID_HEADERS, body: Buffer.from('PK\x03\x04') });
+    const res = makeRes();
+    await handler(req, res as unknown as NextApiResponse);
+
+    expect(res._status).toBe(500);
+    expect(deleteMock).toHaveBeenCalledTimes(1);
+    expect(deleteObjectMock).toHaveBeenCalledWith(
+      'inbox/user-123/inbox-1/clip.epub',
+      expect.any(String),
+    );
  });
 });
@@ -2,7 +2,7 @@ import type { NextApiRequest, NextApiResponse } from 'next';
 import { createSupabaseAdminClient } from '@/utils/supabase';
 import { corsAllMethods, runMiddleware } from '@/utils/cors';
 import { validateUserAndToken } from '@/utils/access';
-import { putObject } from '@/utils/object';
+import { deleteObject, putObject } from '@/utils/object';
 import { parseSubjectTag } from '@/services/send/sendAddress';
 import {
  SEND_INBOX_BUCKET,
@@ -164,7 +164,15 @@ export default async function handler(req: NextApiRequest, res: NextApiResponse)
    .from('send_inbox')
    .update({ payload_key: payloadKey })
    .eq('id', row.id);
-  if (updateError) return res.status(500).json({ error: updateError.message });
+  if (updateError) {
+    await supabase.from('send_inbox').delete().eq('id', row.id);
+    try {
+      await deleteObject(payloadKey, SEND_INBOX_BUCKET);
+    } catch (err) {
+      console.warn('Inbox file payload cleanup failed:', err);
+    }
+    return res.status(500).json({ error: updateError.message });
+  }

  return res.status(200).json({ id: row.id });
 }
@@ -154,7 +154,7 @@ function hex(bytes: Uint8Array, max = 16): string {
 }

 async function sha256(bytes: ArrayBuffer): Promise<Uint8Array> {
-  const digest = await crypto.subtle.digest('SHA-256', bytes);
+  const digest = await crypto.subtle.digest('SHA-256', new Uint8Array(bytes));
  return new Uint8Array(digest);
 }

@@ -149,7 +149,8 @@ async function htmlToBook(
    images,
    coverImage,
  );
-  const file = new File([blob], `${safeFileName(title)}.epub`, {
+  const fileBytes = new Uint8Array(await blob.arrayBuffer());
+  const file = new File([fileBytes], `${safeFileName(title)}.epub`, {
    type: 'application/epub+zip',
  });
  return { file, title, author };