fix(api): harden paper creation cleanup

[is0692vs]

Summary

• keep batch construction typed while preserving the insert path
• protect the cleanup path so R2/D1 cleanup failures are logged without losing the original error
• tighten the targeted papers route tests around inserts and empty-tag normalization

Validation

• npm run --workspace apps/api test -- src/routes/tests/papers.test.ts

Split out of #138 to keep the review focused on the paper creation error-handling change set.

Greptile Summary

このPRは POST /api/papers のエラーハンドリングを強化するもので、主に2点の改善を行っています。①バッチ構築を BatchItem[] 型を使って型安全に統一し、org_only 分岐を if/else から条件的な push に整理、②エラー発生時のR2/D1クリーンアップを try/catch で保護し、クリーンアップ失敗を cleanupFailures 配列に集約してログ出力後、必ず元のエラーを再スローする設計に変更しています。

主な変更点:

• papers.ts: バッチ操作配列を BatchItem 型で統一し、R2/D1クリーンアップを独立した try/catch で保護。cleanupFailures でクリーンアップ失敗を収集し console.error でロギング
• papers.test.ts: insert モックをバッチ引数の内容検証が可能な形式に変更し、org_only テストで orgId を含む挿入が正しくバッチに含まれることをアサート
• 空タグ正規化テストのコメントを整理し、res2 の結果を明示的にアサートするよう改善
• 懸念点: 今回の主要な変更であるクリーンアップエラーパス（db.batch 失敗時のR2/D1クリーンアップ呼び出しと元エラーの再スロー）に対するテストが追加されていない

Confidence Score: 4/5

• 概ねマージ可能。クリーンアップロジックの設計は適切で、元エラーの再スローも保証されている。
• R2/D1クリーンアップの保護とバッチ構築の型安全化は正しく実装されており、既存テストも改善されている。ただし、このPRの主要変更であるエラーパスの動作を直接検証するテストが欠如しているため、満点には至らない。
• apps/api/src/routes/tests/papers.test.ts — クリーンアップエラーパスのテストカバレッジが不足

Important Files Changed

Filename	Overview
apps/api/src/routes/papers.ts	エラーパスのR2/D1クリーンアップをtry/catchで保護し、cleanupFailuresに集約してログ出力後に元のエラーを再スローする実装。BatchItemの型付けも改善。単一要素のPromise.allSettledラップが若干冗長。
apps/api/src/routes/tests/papers.test.ts	insertモックをバッチ構造検証用に更新し、org_only時のバッチ引数を具体的にアサートするよう改善。空タグ正規化テストのコメントも整理。ただし、今回の主要変更であるクリーンアップエラーパスのテストが欠如。

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[POST /api/papers] --> B[入力バリデーション]
    B --> C[R2 ファイルアップロード]
    C --> D{エラー?}
    D -- No --> E[BatchItem配列構築]
    E --> F{org_only + orgId?}
    F -- Yes --> G[insertOrg 追加]
    F -- No --> H[insertFiles 追加]
    G --> H
    H --> I[db.batch 実行]
    I --> J{エラー?}
    J -- No --> K[201 Created]
    J -- Yes --> L[catch error]
    L --> M[R2クリーンアップ\nPromise.allSettled]
    M --> N[D1子テーブル削除\npaperFiles / paperOrgs / paperAuthors]
    N --> O[papers 削除]
    O --> P{cleanupFailures\nあり?}
    P -- Yes --> Q[console.error ログ出力]
    P -- No --> R[throw error 再スロー]
    Q --> R

    style K fill:#22c55e,color:#fff
    style R fill:#ef4444,color:#fff
    style Q fill:#f97316,color:#fff

Loading

Comments Outside Diff (2)

1. apps/api/src/routes/__tests__/papers.test.ts, line 22-26 (link)

   クリーンアップパスのテストが不足

   このPRの主目的である「クリーンアップの堅牢化」（db.batch 失敗時にR2とD1のクリーンアップが実行され、元のエラーが再スローされる）を検証するテストが存在しません。

   例えば、次のようなシナリオのテストを追加することを検討してください：

   it("POST /api/papers cleans up R2 and D1 when batch fails, and re-throws original error", async () => {
       const token = await createTestJWT({ sub: "user-1", githubId: "123", name: "Uploader" });
       const app = await createTestApp();
       const env = createTestEnv();
   
       const batchError = new Error("D1 batch failed");
       mockDb.batch = vi.fn(async () => { throw batchError; });
   
       const form = new FormData();
       form.set("metadata", JSON.stringify({ title: "Fail Paper", visibility: "private" }));
       form.set("files_0", new File(["%PDF-1.4\n%dummy"], "paper.pdf", { type: "application/pdf" }));
       form.set("file_types_0", "paper");
   
       const res = await app.request(
           "http://localhost/api/papers",
           { method: "POST", headers: { Authorization: `Bearer ${token}`, Origin: "http://localhost:3000" }, body: form },
           env as any
       );
   
       expect(res.status).toBe(500);
       expect(mockDb.delete).toHaveBeenCalled();
       // R2のBUCKET.deleteが呼ばれたことも確認できると理想的
   });

   現状のテストスイートはハッピーパス（正常系）のみをカバーしており、今回強化した例外ハンドリングのロジックが将来のリファクタリングで意図せず壊れた場合に検出できません。

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: apps/api/src/routes/__tests__/papers.test.ts
   Line: 22-26
   
   Comment:
   **クリーンアップパスのテストが不足**
   
   このPRの主目的である「クリーンアップの堅牢化」（`db.batch` 失敗時にR2とD1のクリーンアップが実行され、元のエラーが再スローされる）を検証するテストが存在しません。
   
   例えば、次のようなシナリオのテストを追加することを検討してください：
   
   ```typescript
   it("POST /api/papers cleans up R2 and D1 when batch fails, and re-throws original error", async () => {
       const token = await createTestJWT({ sub: "user-1", githubId: "123", name: "Uploader" });
       const app = await createTestApp();
       const env = createTestEnv();
   
       const batchError = new Error("D1 batch failed");
       mockDb.batch = vi.fn(async () => { throw batchError; });
   
       const form = new FormData();
       form.set("metadata", JSON.stringify({ title: "Fail Paper", visibility: "private" }));
       form.set("files_0", new File(["%PDF-1.4\n%dummy"], "paper.pdf", { type: "application/pdf" }));
       form.set("file_types_0", "paper");
   
       const res = await app.request(
           "http://localhost/api/papers",
           { method: "POST", headers: { Authorization: `Bearer ${token}`, Origin: "http://localhost:3000" }, body: form },
           env as any
       );
   
       expect(res.status).toBe(500);
       expect(mockDb.delete).toHaveBeenCalled();
       // R2のBUCKET.deleteが呼ばれたことも確認できると理想的
   });
   ```
   
   現状のテストスイートはハッピーパス（正常系）のみをカバーしており、今回強化した例外ハンドリングのロジックが将来のリファクタリングで意図せず壊れた場合に検出できません。
   
   How can I resolve this? If you propose a fix, please make it concise.

2. apps/api/src/routes/__tests__/papers.test.ts, line 56-89 (link)

   エラーパス（クリーンアップ）のテストが欠如

   このPRの主な変更点はエラー発生時のR2/D1クリーンアップの堅牢化ですが、そのパスに対するテストが追加されていません。

   具体的に検証できていない挙動:

   • db.batch が失敗した場合、R2クリーンアップ (BUCKET.delete) が呼ばれること
   • db.batch が失敗した場合、D1クリーンアップ (delete paperFiles/paperOrgs/paperAuthors/papers) が呼ばれること
   • クリーンアップが失敗しても、元のエラーが必ず throw error で再スローされること
   • クリーンアップ失敗時に console.error が呼ばれること

   例えば以下のようなテストケースを追加することで、新しいエラーハンドリングロジックが意図通りに動作することを保証できます:

   it("POST /api/papers cleans up R2 and D1 on batch failure and rethrows original error", async () => {
       const token = await createTestJWT({ sub: "user-1", githubId: "123", name: "Uploader" });
       const app = await createTestApp();
       const env = createTestEnv();
   
       const originalError = new Error("batch failed");
       mockDb.batch = vi.fn(async () => { throw originalError; });
   
       const form = new FormData();
       form.set("metadata", JSON.stringify({ title: "Test Paper", visibility: "private" }));
       form.set("files_0", new File(["%PDF-1.4\n%dummy-pdf"], "paper.pdf", { type: "application/pdf" }));
       form.set("file_types_0", "paper");
   
       const res = await app.request(
           "http://localhost/api/papers",
           { method: "POST", headers: { Authorization: `Bearer ${token}` }, body: form },
           env as any
       );
   
       // R2クリーンアップが呼ばれたことを確認
       // D1クリーンアップが呼ばれたことを確認
       // レスポンスはオリジナルエラーに基づくものであること
       expect(res.status).toBe(500);
       expect(mockDb.delete).toHaveBeenCalled();
   });

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: apps/api/src/routes/__tests__/papers.test.ts
   Line: 56-89
   
   Comment:
   **エラーパス（クリーンアップ）のテストが欠如**
   
   このPRの主な変更点はエラー発生時のR2/D1クリーンアップの堅牢化ですが、そのパスに対するテストが追加されていません。
   
   具体的に検証できていない挙動:
   - `db.batch` が失敗した場合、R2クリーンアップ (`BUCKET.delete`) が呼ばれること
   - `db.batch` が失敗した場合、D1クリーンアップ (`delete paperFiles/paperOrgs/paperAuthors/papers`) が呼ばれること
   - クリーンアップが失敗しても、**元のエラーが必ず `throw error` で再スローされること**
   - クリーンアップ失敗時に `console.error` が呼ばれること
   
   例えば以下のようなテストケースを追加することで、新しいエラーハンドリングロジックが意図通りに動作することを保証できます:
   
   ```ts
   it("POST /api/papers cleans up R2 and D1 on batch failure and rethrows original error", async () => {
       const token = await createTestJWT({ sub: "user-1", githubId: "123", name: "Uploader" });
       const app = await createTestApp();
       const env = createTestEnv();
   
       const originalError = new Error("batch failed");
       mockDb.batch = vi.fn(async () => { throw originalError; });
   
       const form = new FormData();
       form.set("metadata", JSON.stringify({ title: "Test Paper", visibility: "private" }));
       form.set("files_0", new File(["%PDF-1.4\n%dummy-pdf"], "paper.pdf", { type: "application/pdf" }));
       form.set("file_types_0", "paper");
   
       const res = await app.request(
           "http://localhost/api/papers",
           { method: "POST", headers: { Authorization: `Bearer ${token}` }, body: form },
           env as any
       );
   
       // R2クリーンアップが呼ばれたことを確認
       // D1クリーンアップが呼ばれたことを確認
       // レスポンスはオリジナルエラーに基づくものであること
       expect(res.status).toBe(500);
       expect(mockDb.delete).toHaveBeenCalled();
   });
   ```
   
   How can I resolve this? If you propose a fix, please make it concise.

Prompt To Fix All With AI

This is a comment left during a code review.
Path: apps/api/src/routes/__tests__/papers.test.ts
Line: 56-89

Comment:
**エラーパス（クリーンアップ）のテストが欠如**

このPRの主な変更点はエラー発生時のR2/D1クリーンアップの堅牢化ですが、そのパスに対するテストが追加されていません。

具体的に検証できていない挙動:
- `db.batch` が失敗した場合、R2クリーンアップ (`BUCKET.delete`) が呼ばれること
- `db.batch` が失敗した場合、D1クリーンアップ (`delete paperFiles/paperOrgs/paperAuthors/papers`) が呼ばれること
- クリーンアップが失敗しても、**元のエラーが必ず `throw error` で再スローされること**
- クリーンアップ失敗時に `console.error` が呼ばれること

例えば以下のようなテストケースを追加することで、新しいエラーハンドリングロジックが意図通りに動作することを保証できます:

```ts
it("POST /api/papers cleans up R2 and D1 on batch failure and rethrows original error", async () => {
    const token = await createTestJWT({ sub: "user-1", githubId: "123", name: "Uploader" });
    const app = await createTestApp();
    const env = createTestEnv();

    const originalError = new Error("batch failed");
    mockDb.batch = vi.fn(async () => { throw originalError; });

    const form = new FormData();
    form.set("metadata", JSON.stringify({ title: "Test Paper", visibility: "private" }));
    form.set("files_0", new File(["%PDF-1.4\n%dummy-pdf"], "paper.pdf", { type: "application/pdf" }));
    form.set("file_types_0", "paper");

    const res = await app.request(
        "http://localhost/api/papers",
        { method: "POST", headers: { Authorization: `Bearer ${token}` }, body: form },
        env as any
    );

    // R2クリーンアップが呼ばれたことを確認
    // D1クリーンアップが呼ばれたことを確認
    // レスポンスはオリジナルエラーに基づくものであること
    expect(res.status).toBe(500);
    expect(mockDb.delete).toHaveBeenCalled();
});
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: apps/api/src/routes/papers.ts
Line: 485-492

Comment:
**単一要素の `Promise.allSettled` ラップが冗長**

`db.delete(papers)` の1件だけを `Promise.allSettled([...])` に包んでいます。`Promise.allSettled` は複数の並行処理のために設計されており、単一要素の場合は通常の `await` + `.catch()` と機能的には同等ですが、不必要なコードノイズになります。

同じ `try/catch` ブロック内にあるため、シンプルに書き換えられます:

```suggestion
            try {
                await db.delete(papers).where(eq(papers.id, paperId));
            } catch (e) {
                cleanupFailures.push(e);
            }
```

なお、外部キー制約の観点から子テーブルを先に削除してから親 (`papers`) を削除する現在の順序は正しいです。

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: 2d9bc3c}

[vercel]

Deployment failed with the following error:

Resource is limited - try again in 24 hours (more than 100, code: "api-deployments-free-per-day").

Learn More: https://vercel.com/hirokis-projects-afd618c7?upgradeToPro=build-rate-limit

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the reliability of the paper creation process by hardening its cleanup mechanisms and improving error handling. It refines the construction of database batch commands for better type safety and introduces more rigorous testing to validate data insertion and tag normalization behaviors.

Highlights

• Typed Batch Construction: Ensured that database batch operations maintain type safety, particularly for insert paths, by refining how batch items are constructed.
• Robust Cleanup Error Handling: Implemented comprehensive error handling for cleanup operations involving R2 bucket deletions and D1 database record removals, ensuring that any failures are logged without masking the original error.
• Enhanced Test Coverage: Strengthened tests for the papers route, specifically focusing on insert operations within batches and the normalization of empty tags in patch requests.

Changelog

• apps/api/src/routes/tests/papers.test.ts
  • Refactored db.insert mock to return structured data for detailed batch query inspection.
  • Added specific assertions to verify the number and content of queries within db.batch calls.
  • Updated tag normalization test to explicitly check the HTTP status code after processing an empty string array for tags.
• apps/api/src/routes/papers.ts
  • Modified the paper creation endpoint to use a more flexible and typed approach for constructing database batch operations.
  • Introduced a robust error handling mechanism in the catch block to manage failures during R2 object deletion and D1 record cleanup, logging all cleanup failures while re-throwing the original error.

Activity

• No human activity has been recorded on this pull request yet.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request successfully hardens the cleanup logic for paper creation by ensuring that cleanup failures are logged without losing the original error. The changes also refactor the batch insert logic to be more streamlined and typed, and tighten the tests for more specific assertions.

[coderabbitai]

Warning

Rate limit exceeded

@is0692vs has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 16 minutes and 23 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 4988bd31-9e1c-4987-8aba-fce4d62d309b

📥 Commits

Reviewing files that changed from the base of the PR and between f852003 and 2d9bc3c.

📒 Files selected for processing (2)

• apps/api/src/routes/__tests__/papers.test.ts
• apps/api/src/routes/papers.ts

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch split/pr138-papers-cleanup

📝 Coding Plan

• Generate coding plan for human review comments

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment

Project	Deployment	Actions	Updated (UTC)
open-shelf	Ignored		Mar 17, 2026 11:42am