fix: address PR 414 review feedback#415
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Merging this PR will not alter performance
Comparing 
Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request parallelizes Cloudflare R2 object deletions using Promise.all and updates E2E tests to use stricter equality assertions with complete user object schemas. Review feedback indicates that the Cloudflare R2 delete method does not support bulk deletion via arrays as implemented; it is recommended to use pMap for concurrent single-key deletions while avoiding side effects within the mapping function to adhere to repository standards.
| const cleanupPromises: Promise<void>[] = []; | ||
| for (let i = 0; i < uploadedKeys.length; i += 1000) { | ||
| try { | ||
| await c.env.BUCKET.delete(uploadedKeys.slice(i, i + 1000)); | ||
| } /* v8 ignore start */ catch (e) { | ||
| // Ignore cleanup errors | ||
| console.error("Cleanup failed intentionally:", e instanceof Error ? e.message : String(e)); | ||
| } /* v8 ignore stop */ | ||
| cleanupPromises.push( | ||
| c.env.BUCKET.delete(uploadedKeys.slice(i, i + 1000)).catch((e) => { | ||
| // Ignore cleanup errors | ||
| console.error("Cleanup failed intentionally:", e instanceof Error ? e.message : String(e)); | ||
| }), | ||
| ); | ||
| } | ||
| await Promise.all(cleanupPromises); |
There was a problem hiding this comment.
The Cloudflare R2 delete method expects a single string key, not an array of keys. Passing an array (via slice) will likely fail to delete the intended objects. Use pMap for concurrent deletion. To adhere to repository rules, ensure the mapper function returns a result object instead of performing side effects like logging, which should be handled after the mapping is complete.
const cleanupResults = await pMap(
uploadedKeys,
(key) =>
c.env.BUCKET.delete(key)
.then(() => ({ success: true }))
.catch((e) => ({ success: false, error: e })),
{ concurrency: MAX_CONCURRENT_UPLOADS },
);
cleanupResults.forEach((res) => {
if (!res.success) {
console.error("Cleanup failed intentionally:", res.error instanceof Error ? res.error.message : String(res.error));
}
});References
- Avoid side effects in mapper functions (like in pMap). Instead of modifying external state, the mapper should return a result object, and the collection of results should be processed afterwards.
| const deletePromises: Promise<void>[] = []; | ||
| for (let i = 0; i < keys.length; i += 1000) { | ||
| await c.env.BUCKET.delete(keys.slice(i, i + 1000)); | ||
| deletePromises.push(c.env.BUCKET.delete(keys.slice(i, i + 1000))); | ||
| } | ||
| await Promise.all(deletePromises); |
There was a problem hiding this comment.
The R2 delete method does not support bulk deletion with an array of keys. Use pMap for concurrent deletion. Additionally, to maintain consistency with repository patterns, the mapper function should return a result object rather than performing side effects (like logging) directly inside the mapper.
| const deletePromises: Promise<void>[] = []; | |
| for (let i = 0; i < keys.length; i += 1000) { | |
| await c.env.BUCKET.delete(keys.slice(i, i + 1000)); | |
| deletePromises.push(c.env.BUCKET.delete(keys.slice(i, i + 1000))); | |
| } | |
| await Promise.all(deletePromises); | |
| const deletionResults = await pMap( | |
| keys, | |
| (key) => | |
| c.env.BUCKET.delete(key) | |
| .then(() => ({ success: true })) | |
| .catch((e) => ({ success: false, error: e })), | |
| { concurrency: MAX_CONCURRENT_UPLOADS }, | |
| ); | |
| deletionResults.forEach((res) => { | |
| if (!res.success) { | |
| console.error("File deletion failed:", res.error instanceof Error ? res.error.message : String(res.error)); | |
| } | |
| }); |
References
- Avoid side effects in mapper functions (like in pMap). Instead of modifying external state, the mapper should return a result object, and the collection of results should be processed afterwards.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| await db.delete(papers).where(eq(papers.id, paperId)); | ||
| throw error; |
There was a problem hiding this comment.
db.delete エラーが元のエラーを上書きする
db.delete(papers) が例外を投げると、throw error（元のアップロードエラー）は実行されず、DBエラーがそのまま伝播します。その結果、ファイルのない孤立した papers レコードがDBに残り（ステップ 3〜5 でパーパーが既にインサートされていた場合）、ユーザーへ返るエラーも元の原因とは無関係のDB例外になります。.catch() でエラーを飲み込んだ上で元のエラーを再スローしてください。
| await db.delete(papers).where(eq(papers.id, paperId)); | |
| throw error; | |
| await db.delete(papers).where(eq(papers.id, paperId)).catch((dbErr) => { | |
| console.error("Failed to clean up paper record during rollback:", dbErr instanceof Error ? dbErr.message : String(dbErr)); | |
| }); | |
| throw error; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/api/src/routes/papers.ts
Line: 707-708
Comment:
**クリーンアップの `db.delete` エラーが元のエラーを上書きする**
`db.delete(papers)` が例外を投げると、`throw error`（元のアップロードエラー）は実行されず、DBエラーがそのまま伝播します。その結果、ファイルのない孤立した `papers` レコードがDBに残り（ステップ 3〜5 でパーパーが既にインサートされていた場合）、ユーザーへ返るエラーも元の原因とは無関係のDB例外になります。`.catch()` でエラーを飲み込んだ上で元のエラーを再スローしてください。
```suggestion
await db.delete(papers).where(eq(papers.id, paperId)).catch((dbErr) => {
console.error("Failed to clean up paper record during rollback:", dbErr instanceof Error ? dbErr.message : String(dbErr));
});
throw error;
```
How can I resolve this? If you propose a fix, please make it concise.| const deletePromises: Promise<void>[] = []; | ||
| for (let i = 0; i < keys.length; i += 1000) { | ||
| await c.env.BUCKET.delete(keys.slice(i, i + 1000)); | ||
| deletePromises.push(c.env.BUCKET.delete(keys.slice(i, i + 1000))); | ||
| } | ||
| await Promise.all(deletePromises); | ||
| await db.delete(papers).where(eq(papers.id, paperId)); |
There was a problem hiding this comment.
Promise.all(deletePromises) がリジェクトすると db.delete(papers) はスキップされ、ペーパーレコードはDBに残ります（ユーザーに 500 が返る）。ただし Promise.all は既にスタート済みの他バッチをキャンセルしないため、複数バッチがある場合に一部の R2 ファイルだけが削除されてしまいます。R2 エラーをキャッチして DB 削除まで進めるか、DBから先に削除することを検討してください。
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/api/src/routes/papers.ts
Line: 1298-1303
Comment:
**R2 バッチ削除が失敗した際にファイルが部分的に消える可能性**
`Promise.all(deletePromises)` がリジェクトすると `db.delete(papers)` はスキップされ、ペーパーレコードはDBに残ります（ユーザーに 500 が返る）。ただし Promise.all は既にスタート済みの他バッチをキャンセルしないため、複数バッチがある場合に一部の R2 ファイルだけが削除されてしまいます。R2 エラーをキャッチして DB 削除まで進めるか、DBから先に削除することを検討してください。
How can I resolve this? If you propose a fix, please make it concise.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
1 participant
Resolve the open review feedback on PR #414 by restoring concurrent cleanup/deletion in apps/api/src/routes/papers.ts and updating the Playwright JSON assertions to use toEqual with full payloads.
Greptile Summary
PR #414 のレビュー指摘に対応し、
papers.tsのアップロードエラー時クリーンアップを並列R2バッチ削除方式に戻すとともに、PlaywrightのAPIアサーションをtoEqualによる完全ペイロード検証に更新しています。db.delete(papers)がエラーを投げるとthrow errorが実行されず、元のアップロードエラーがDBエラーに上書きされます。また、既にインサート済みのpapersレコードが孤立してDBに残ります。.catch()で吸収してから元のエラーを再スローしてください。Confidence Score: 4/5
P1バグ（アップロードキャッチブロックでの db.delete エラーハンドリング欠如）が残っているためマージ前に修正が必要。
E2Eテストの修正は適切で問題なし。papers.ts のR2並列削除の復元も意図通り。しかしアップロードエラーのcatchブロックで db.delete が失敗した場合に孤立レコードが残り元エラーが失われるP1バグが存在し、スコアは4。
apps/api/src/routes/papers.ts（特にL707のクリーンアップ処理）
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[POST /api/papers - アップロード開始] --> B[pMap: R2ファイル並列アップロード] B -->|成功| C[db.insert papers] B -->|失敗| E[catch block] C --> D[db.insert paperAuthors / paperOrgs / paperFiles] D -->|成功| F[201 OK] D -->|失敗| E E --> G[Promise.all: R2クリーンアップ - エラー無視] G --> H[db.delete papers ⚠️ エラーハンドリングなし] H -->|成功| I[throw 元エラー] H -->|失敗⚠️| J[DBエラー伝播 - 孤立レコード残存] K[DELETE /api/papers/:id] --> L[権限確認] L --> M[R2ファイルキー取得] M --> N[Promise.all: R2バッチ削除 並列実行] N -->|成功| O[db.delete papers] N -->|部分失敗⚠️| P[500 - 一部R2ファイルが消える可能性] O --> Q[200 OK]Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix: address PR 414 review feedback" | Re-trigger Greptile
Context used: