fix(user): address code review issues for #161

Sora4431 · claude · Sora4431 · commit 438d481abd8b · 2026-04-06T23:04:09.000+09:00
- Fix N+1 queries in syncBookmarks using batch INSERT OR IGNORE
- Add 50-item limit validation for sync-bookmarks endpoint
- Wrap addBookmark in try/catch to prevent false 500 on itinerary creation
- Revert client.ts user-token fallback; use explicit postWithUserToken instead
- Filter syncLocalBookmarks to only owned itineraries (token !== null)
- Add integration tests for sync-bookmarks and auto-bookmark on create

Co-Authored-By: Claude Sonnet 4.6 &lt;noreply@anthropic.com&gt;
diff --git a/apps/api/src/routes/itineraries.ts b/apps/api/src/routes/itineraries.ts
@@ -36,8 +36,12 @@ itineraries.post('/', optionalUserAuthMiddleware, async (c) => {
 
   const userId = c.get('userId');
   if (userId) {
-    const userService = new UserService(c.env.DB);
-    await userService.addBookmark(userId, data.id);
+    try {
+      const userService = new UserService(c.env.DB);
+      await userService.addBookmark(userId, data.id);
+    } catch {
+      // 非致命的: しおりは作成済み、/me/sync-bookmarks で後から同期可能
+    }
   }
 
   return c.json({ success: true, data: { ...response, token } }, 201);
diff --git a/apps/api/src/routes/users.ts b/apps/api/src/routes/users.ts
@@ -80,6 +80,13 @@ users.post('/me/sync-bookmarks', userAuthMiddleware, async (c) => {
     }, 400);
   }
 
+  if (input.itinerary_ids.length > 50) {
+    return c.json({
+      success: false,
+      error: { code: 'INVALID_INPUT', message: 'Too many itinerary_ids (max 50)' }
+    }, 400);
+  }
+
   const service = new UserService(c.env.DB);
   const result = await service.syncBookmarks(userId, input.itinerary_ids);
   return c.json({ success: true, data: result });
diff --git a/apps/api/src/services/user.service.ts b/apps/api/src/services/user.service.ts
@@ -154,46 +154,34 @@ export class UserService {
   }
 
   // ログイン時の localStorage→server 同期
-  // 存在する itinerary_id のみ user_bookmarks に追加、既存はスキップ
+  // 存在する itinerary_id のみ user_bookmarks に追加、既存はスキップ（INSERT OR IGNORE で重複排除）
   async syncBookmarks(userId: string, itineraryIds: string[]): Promise<SyncBookmarksResponse> {
-    let synced = 0;
-    let skipped = 0;
-
-    for (const itineraryId of itineraryIds) {
-      // itinerary が存在するか確認
-      const itinerary = await this.db
-        .prepare('SELECT id FROM itineraries WHERE id = ?')
-        .bind(itineraryId)
-        .first();
+    if (itineraryIds.length === 0) return { synced: 0, skipped: 0 };
 
-      if (!itinerary) {
-        skipped++;
-        continue;
-      }
+    // 1. 存在する itinerary を一括確認
+    const placeholders = itineraryIds.map(() => '?').join(', ');
+    const existing = await this.db
+      .prepare(`SELECT id FROM itineraries WHERE id IN (${placeholders})`)
+      .bind(...itineraryIds)
+      .all<{ id: string }>();
 
-      // 既に紐付け済みか確認
-      const existing = await this.db
-        .prepare('SELECT user_id FROM user_bookmarks WHERE user_id = ? AND itinerary_id = ?')
-        .bind(userId, itineraryId)
-        .first();
+    const validIds = (existing.results ?? []).map(r => r.id);
+    const skipped = itineraryIds.length - validIds.length;
 
-      if (existing) {
-        skipped++;
-        continue;
-      }
+    if (validIds.length === 0) return { synced: 0, skipped };
 
-      const now = getCurrentTimestamp();
-      await this.db
-        .prepare(
-          'INSERT INTO user_bookmarks (user_id, itinerary_id, is_visible, created_at, updated_at) VALUES (?, ?, 1, ?, ?)'
-        )
-        .bind(userId, itineraryId, now, now)
-        .run();
+    // 2. INSERT OR IGNORE で一括追加（複合主キーの重複は自動スキップ）
+    const now = getCurrentTimestamp();
+    const stmts = validIds.map(id =>
+      this.db
+        .prepare('INSERT OR IGNORE INTO user_bookmarks (user_id, itinerary_id, is_visible, created_at, updated_at) VALUES (?, ?, 1, ?, ?)')
+        .bind(userId, id, now, now)
+    );
+    const results = await this.db.batch(stmts);
 
-      synced++;
-    }
+    const synced = results.filter(r => r.meta?.changes && r.meta.changes > 0).length;
 
-    return { synced, skipped };
+    return { synced, skipped: skipped + (validIds.length - synced) };
   }
 
   async addBookmark(userId: string, itineraryId: string): Promise<UserBookmark> {
diff --git a/apps/api/test/users-sync.test.ts b/apps/api/test/users-sync.test.ts
@@ -0,0 +1,221 @@
+import { env } from 'cloudflare:test';
+import { describe, it, expect, beforeEach } from 'vitest';
+import app from '../src/index';
+
+async function applyMigrations(db: D1Database) {
+  const migrations = [
+    `CREATE TABLE IF NOT EXISTS itineraries (
+      id TEXT PRIMARY KEY,
+      title TEXT NOT NULL,
+      theme_id TEXT NOT NULL DEFAULT 'standard-autumn',
+      memo TEXT,
+      password TEXT,
+      created_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP,
+      updated_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP
+    );`,
+    `CREATE TABLE IF NOT EXISTS steps (
+      id TEXT PRIMARY KEY,
+      itinerary_id TEXT NOT NULL,
+      title TEXT NOT NULL,
+      start_at INTEGER NOT NULL,
+      end_at INTEGER NOT NULL,
+      location TEXT,
+      notes TEXT,
+      type TEXT NOT NULL DEFAULT 'normal:general',
+      is_all_day INTEGER NOT NULL DEFAULT 0,
+      created_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP,
+      updated_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP,
+      FOREIGN KEY (itinerary_id) REFERENCES itineraries(id) ON DELETE CASCADE
+    );`,
+    `CREATE INDEX IF NOT EXISTS idx_steps_itinerary ON steps(itinerary_id);`,
+    `CREATE TABLE IF NOT EXISTS itinerary_secrets (
+      itinerary_id TEXT PRIMARY KEY,
+      enabled BOOLEAN DEFAULT FALSE,
+      offset_minutes INTEGER DEFAULT 60,
+      created_at TEXT NOT NULL,
+      updated_at TEXT NOT NULL,
+      FOREIGN KEY (itinerary_id) REFERENCES itineraries(id) ON DELETE CASCADE
+    );`,
+    `CREATE TABLE IF NOT EXISTS itinerary_walica_settings (
+      itinerary_id TEXT PRIMARY KEY,
+      walica_id TEXT NOT NULL,
+      created_at TEXT NOT NULL,
+      updated_at TEXT NOT NULL,
+      FOREIGN KEY (itinerary_id) REFERENCES itineraries(id) ON DELETE CASCADE
+    );`,
+    `CREATE TABLE IF NOT EXISTS users (
+      id TEXT PRIMARY KEY,
+      username TEXT UNIQUE NOT NULL,
+      email TEXT UNIQUE NOT NULL,
+      password_hash TEXT NOT NULL,
+      created_at TEXT NOT NULL,
+      updated_at TEXT NOT NULL
+    );`,
+    `CREATE TABLE IF NOT EXISTS user_bookmarks (
+      user_id TEXT NOT NULL,
+      itinerary_id TEXT NOT NULL,
+      is_visible BOOLEAN NOT NULL DEFAULT 1,
+      created_at TEXT NOT NULL,
+      updated_at TEXT NOT NULL,
+      PRIMARY KEY (user_id, itinerary_id),
+      FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE,
+      FOREIGN KEY (itinerary_id) REFERENCES itineraries(id) ON DELETE CASCADE
+    );`,
+  ];
+  for (const sql of migrations) {
+    await db.prepare(sql).run();
+  }
+}
+
+async function registerAndGetToken(username: string, email: string): Promise<string> {
+  const res = await app.request('/api/v1/users/register', {
+    method: 'POST',
+    headers: { 'Content-Type': 'application/json' },
+    body: JSON.stringify({ username, email, password: 'password123' }),
+  }, env);
+  const json = await res.json() as { success: boolean; data: { token: string } };
+  return json.data.token;
+}
+
+async function createItinerary(): Promise<string> {
+  const res = await app.request('/api/v1/itineraries', {
+    method: 'POST',
+    headers: { 'Content-Type': 'application/json' },
+    body: JSON.stringify({ title: 'テスト旅程' }),
+  }, env);
+  const json = await res.json() as { success: boolean; data: { id: string } };
+  return json.data.id;
+}
+
+describe('POST /api/v1/users/me/sync-bookmarks', () => {
+  beforeEach(async () => {
+    await applyMigrations(env.DB);
+    await env.DB.prepare('DELETE FROM user_bookmarks').run();
+    await env.DB.prepare('DELETE FROM users').run();
+    await env.DB.prepare('DELETE FROM itineraries').run();
+  });
+
+  it('returns 401 without auth token', async () => {
+    const res = await app.request('/api/v1/users/me/sync-bookmarks', {
+      method: 'POST',
+      headers: { 'Content-Type': 'application/json' },
+      body: JSON.stringify({ itinerary_ids: [] }),
+    }, env);
+    expect(res.status).toBe(401);
+  });
+
+  it('syncs valid itinerary IDs and returns correct counts', async () => {
+    const token = await registerAndGetToken('testuser', 'test@example.com');
+    const id1 = await createItinerary();
+    const id2 = await createItinerary();
+
+    const res = await app.request('/api/v1/users/me/sync-bookmarks', {
+      method: 'POST',
+      headers: {
+        'Content-Type': 'application/json',
+        Authorization: `Bearer ${token}`,
+      },
+      body: JSON.stringify({ itinerary_ids: [id1, id2] }),
+    }, env);
+
+    expect(res.status).toBe(200);
+    const json = await res.json() as { success: boolean; data: { synced: number; skipped: number } };
+    expect(json.success).toBe(true);
+    expect(json.data.synced).toBe(2);
+    expect(json.data.skipped).toBe(0);
+  });
+
+  it('skips non-existent itinerary IDs', async () => {
+    const token = await registerAndGetToken('testuser2', 'test2@example.com');
+
+    const res = await app.request('/api/v1/users/me/sync-bookmarks', {
+      method: 'POST',
+      headers: {
+        'Content-Type': 'application/json',
+        Authorization: `Bearer ${token}`,
+      },
+      body: JSON.stringify({ itinerary_ids: ['nonexistent-id'] }),
+    }, env);
+
+    expect(res.status).toBe(200);
+    const json = await res.json() as { success: boolean; data: { synced: number; skipped: number } };
+    expect(json.data.synced).toBe(0);
+    expect(json.data.skipped).toBe(1);
+  });
+
+  it('skips already-bookmarked IDs on second call', async () => {
+    const token = await registerAndGetToken('testuser3', 'test3@example.com');
+    const id = await createItinerary();
+
+    await app.request('/api/v1/users/me/sync-bookmarks', {
+      method: 'POST',
+      headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${token}` },
+      body: JSON.stringify({ itinerary_ids: [id] }),
+    }, env);
+
+    const res = await app.request('/api/v1/users/me/sync-bookmarks', {
+      method: 'POST',
+      headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${token}` },
+      body: JSON.stringify({ itinerary_ids: [id] }),
+    }, env);
+
+    expect(res.status).toBe(200);
+    const json = await res.json() as { success: boolean; data: { synced: number; skipped: number } };
+    expect(json.data.synced).toBe(0);
+    expect(json.data.skipped).toBe(1);
+  });
+
+  it('returns 400 for more than 50 IDs', async () => {
+    const token = await registerAndGetToken('testuser4', 'test4@example.com');
+    const ids = Array.from({ length: 51 }, (_, i) => `id-${i}`);
+
+    const res = await app.request('/api/v1/users/me/sync-bookmarks', {
+      method: 'POST',
+      headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${token}` },
+      body: JSON.stringify({ itinerary_ids: ids }),
+    }, env);
+
+    expect(res.status).toBe(400);
+  });
+});
+
+describe('POST /api/v1/itineraries with user token', () => {
+  beforeEach(async () => {
+    await applyMigrations(env.DB);
+    await env.DB.prepare('DELETE FROM user_bookmarks').run();
+    await env.DB.prepare('DELETE FROM users').run();
+    await env.DB.prepare('DELETE FROM itineraries').run();
+  });
+
+  it('creates user_bookmark record when creating itinerary while logged in', async () => {
+    const token = await registerAndGetToken('creator', 'creator@example.com');
+
+    const res = await app.request('/api/v1/itineraries', {
+      method: 'POST',
+      headers: {
+        'Content-Type': 'application/json',
+        Authorization: `Bearer ${token}`,
+      },
+      body: JSON.stringify({ title: 'マイ旅程' }),
+    }, env);
+
+    expect(res.status).toBe(201);
+    const json = await res.json() as { success: boolean; data: { id: string } };
+    const itineraryId = json.data.id;
+
+    const bookmarks = await app.request('/api/v1/users/me/bookmarks', {
+      headers: { Authorization: `Bearer ${token}` },
+    }, env);
+    const bJson = await bookmarks.json() as { success: boolean; data: { bookmarks: { itinerary_id: string }[] } };
+    expect(bJson.data.bookmarks.some(b => b.itinerary_id === itineraryId)).toBe(true);
+  });
+
+  it('creates itinerary normally without user token (no bookmark)', async () => {
+    const res = await app.request('/api/v1/itineraries', {
+      method: 'POST',
+      headers: { 'Content-Type': 'application/json' },
+      body: JSON.stringify({ title: '匿名旅程' }),
+    }, env);
+    expect(res.status).toBe(201);
+  });
+});
diff --git a/apps/web/src/lib/api/client.ts b/apps/web/src/lib/api/client.ts
@@ -1,6 +1,5 @@
 import type { ApiResult } from '@tabitabi/types';
 import { auth } from '../auth';
-import { userAuth } from '../user-auth';
 import { getIsDemoMode } from '../demo';
 
 // Prefer PUBLIC_ for Cloudflare Pages, fallback to VITE_
@@ -25,16 +24,9 @@ export class ApiClient {
       const token = auth.getToken(shioriId);
       if (token) {
         headers['Authorization'] = `Bearer ${token}`;
-        return headers;
       }
     }
 
-    // shiori トークンがない場合はユーザートークンを使用（ログイン済み時のしおり作成など）
-    const userToken = userAuth.getToken();
-    if (userToken) {
-      headers['Authorization'] = `Bearer ${userToken}`;
-    }
-
     return headers;
   }
 
@@ -78,6 +70,15 @@ export class ApiClient {
     }, shioriId);
   }
 
+  // ユーザートークンを明示的に使用する POST（しおり作成時のユーザー紐付けなど）
+  async postWithUserToken<T>(endpoint: string, data: unknown, userToken: string): Promise<T> {
+    return this.request<T>(endpoint, {
+      method: 'POST',
+      body: JSON.stringify(data),
+      headers: { Authorization: `Bearer ${userToken}` },
+    });
+  }
+
   async put<T>(endpoint: string, data: unknown, shioriId?: string): Promise<T> {
     return this.request<T>(endpoint, {
       method: 'PUT',
diff --git a/apps/web/src/lib/api/itinerary.ts b/apps/web/src/lib/api/itinerary.ts
@@ -1,13 +1,19 @@
 import type { Itinerary, CreateItineraryInput, UpdateItineraryInput, ItineraryResponse } from '@tabitabi/types';
 import { apiClient } from './client';
+import { userAuth } from '../user-auth';
 
 export const itineraryApi = {
   list: () => apiClient.get<ItineraryResponse[]>('/itineraries'),
 
   get: (id: string) => apiClient.get<ItineraryResponse>(`/itineraries/${id}`),
 
-  create: (data: CreateItineraryInput) =>
-    apiClient.post<ItineraryResponse & { token: string }>('/itineraries', data),
+  create: (data: CreateItineraryInput) => {
+    const userToken = userAuth.getToken();
+    if (userToken) {
+      return apiClient.postWithUserToken<ItineraryResponse & { token: string }>('/itineraries', data, userToken);
+    }
+    return apiClient.post<ItineraryResponse & { token: string }>('/itineraries', data);
+  },
 
   update: (id: string, data: UpdateItineraryInput) =>
     apiClient.put<ItineraryResponse>(`/itineraries/${id}`, data, id),
diff --git a/apps/web/src/routes/profile/+page.svelte b/apps/web/src/routes/profile/+page.svelte
@@ -23,7 +23,7 @@
   async function syncLocalBookmarks() {
     const history = auth.getHistory();
     const ids = history
-      .filter((h) => h.shioriId !== "demo")
+      .filter((h) => h.shioriId !== "demo" && h.token !== null)
       .map((h) => h.shioriId);
     if (ids.length === 0) return;
     try {
