fix(itinerary): address code review issues for SOR-11

- Use db.batch() for atomic fork operation (itinerary + steps + fork_count)
- Merge duplicate onMount into single callback, use userAuth.isLoggedIn()
- Add null guard before postWithUserToken in fork() API client
- Make fork_count required (non-optional) in Itinerary type
- Consolidate fork test migrations into shared applyMigrations helper
- Use explicit column list in steps SELECT for robustness
- Add fork_count=0 to create() Itinerary object literal

Author: Sora4431

apps/api/src/services/itinerary.service.ts

@@ -65,6 +65,7 @@ export class ItineraryService {
         enabled: input.secret_settings.enabled,
         offset_minutes: input.secret_settings.offset_minutes
       } : null,
+      fork_count: 0,
       created_at: now,
       updated_at: now,
     };
@@ -202,29 +203,31 @@ export class ItineraryService {
     const newId = generateId();
     const now = getCurrentTimestamp();
 
-    await this.db
-      .prepare('INSERT INTO itineraries (id, title, theme_id, memo, password, fork_count, created_at, updated_at) VALUES (?, ?, ?, ?, NULL, 0, ?, ?)')
-      .bind(newId, `${source.title}（コピー）`, source.theme_id, source.memo, now, now)
-      .run();
-
+    // Fetch source steps before batch to generate new IDs
+    // secret_settings and walica_id are intentionally excluded from forks (personal configuration)
     const sourceSteps = await this.db
-      .prepare('SELECT * FROM steps WHERE itinerary_id = ? ORDER BY start_at ASC')
+      .prepare('SELECT id, itinerary_id, title, start_at, end_at, location, notes, type, is_all_day FROM steps WHERE itinerary_id = ? ORDER BY start_at ASC')
       .bind(sourceId)
       .all();
 
     const rows = sourceSteps.results ?? [];
-    for (const row of rows) {
-      const stepId = generateId();
-      await this.db
-        .prepare('INSERT INTO steps (id, itinerary_id, title, start_at, end_at, location, notes, type, is_all_day, created_at, updated_at) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)')
-        .bind(stepId, newId, row.title, row.start_at, row.end_at, row.location, row.notes, row.type, row.is_all_day, now, now)
-        .run();
-    }
 
-    await this.db
-      .prepare('UPDATE itineraries SET fork_count = fork_count + 1, updated_at = ? WHERE id = ?')
-      .bind(now, sourceId)
-      .run();
+    // Use batch() for atomic execution: all inserts + fork_count increment succeed or fail together
+    const stepStatements = rows.map(row =>
+      this.db
+        .prepare('INSERT INTO steps (id, itinerary_id, title, start_at, end_at, location, notes, type, is_all_day, created_at, updated_at) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)')
+        .bind(generateId(), newId, row.title, row.start_at, row.end_at, row.location, row.notes, row.type, row.is_all_day, now, now)
+    );
+
+    await this.db.batch([
+      this.db
+        .prepare('INSERT INTO itineraries (id, title, theme_id, memo, password, fork_count, created_at, updated_at) VALUES (?, ?, ?, ?, NULL, 0, ?, ?)')
+        .bind(newId, `${source.title}（コピー）`, source.theme_id, source.memo, now, now),
+      ...stepStatements,
+      this.db
+        .prepare('UPDATE itineraries SET fork_count = fork_count + 1, updated_at = ? WHERE id = ?')
+        .bind(now, sourceId),
+    ]);
 
     const forked = await this.get(newId);
     return { itinerary: forked!, steps: rows.length };

apps/api/test/itineraries.test.ts

@@ -44,6 +44,24 @@ async function applyMigrations(db: D1Database) {
       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) {
@@ -241,72 +259,6 @@ describe('Itineraries API', () => {
 });
 
 describe('POST /api/v1/itineraries/:id/fork', () => {
-  async function applyForkMigrations(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,
-        fork_count INTEGER NOT NULL DEFAULT 0,
-        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',
@@ -318,7 +270,7 @@ describe('POST /api/v1/itineraries/:id/fork', () => {
   }
 
   beforeEach(async () => {
-    await applyForkMigrations(env.DB);
+    await applyMigrations(env.DB);
     await env.DB.prepare('DELETE FROM user_bookmarks').run();
     await env.DB.prepare('DELETE FROM steps').run();
     await env.DB.prepare('DELETE FROM itinerary_secrets').run();

apps/web/src/lib/api/itinerary.ts

@@ -22,6 +22,7 @@ export const itineraryApi = {
 
   fork: (id: string) => {
     const userToken = userAuth.getToken();
-    return apiClient.postWithUserToken<ForkItineraryResponse>(`/itineraries/${id}/fork`, {}, userToken!);
+    if (!userToken) throw new Error('Not logged in');
+    return apiClient.postWithUserToken<ForkItineraryResponse>(`/itineraries/${id}/fork`, {}, userToken);
   },
 };

apps/web/src/routes/[id]/+page.svelte

@@ -89,6 +89,7 @@
       }
     };
 
+    isLoggedIn = userAuth.isLoggedIn();
     init();
 
     return () => {
@@ -180,11 +181,9 @@
   let isLoggedIn = $state(false);
   let forking = $state(false);
 
-  onMount(() => {
-    isLoggedIn = !!userAuth.getToken();
-  });
-
   async function handleFork() {
+    const userToken = userAuth.getToken();
+    if (!userToken) return;
     forking = true;
     try {
       const result = await itineraryApi.fork(data.itinerary.id);

packages/types/src/itinerary.ts

@@ -11,7 +11,7 @@ export interface Itinerary {
   walica_id?: string | null;
   password?: string | null;
   secret_settings?: ItinerarySecretSettings | null;
-  fork_count?: number;
+  fork_count: number;
   created_at: string;
   updated_at: string;
 }