fix: apply comprehensive API fixes and frontend loading regression#138
fix: apply comprehensive API fixes and frontend loading regression#138
Conversation
- \`papers.test.ts\`: Assert paperOrgs insertion for org_only papers, verify empty string mapping behavior. - \`users.test.ts\`: Use custom MAX_CACHE_SIZE via environment variable for LRU limits and remove duplicate 404 test block. - \`papers.ts\`: Log partial cleanup failures and re-throw origin errors in POST /api/papers. - \`users.ts\`: Delete and re-insert cached objects on hits to apply true MRU ordering to LRU cache. - \`validation.test.ts\`: Validate rejection of leading and trailing hyphens. Co-authored-by: is0692vs <135803462+is0692vs@users.noreply.github.com>
- \`papers.test.ts\`: Assert paperOrgs insertion for org_only papers, verify empty string mapping behavior. - \`users.test.ts\`: Use custom MAX_CACHE_SIZE via environment variable for LRU limits and remove duplicate 404 test block. - \`papers.ts\`: Log partial cleanup failures and re-throw origin errors in POST /api/papers. - \`users.ts\`: Delete and re-insert cached objects on hits to apply true MRU ordering to LRU cache. - \`validation.test.ts\`: Validate rejection of leading and trailing hyphens. Co-authored-by: is0692vs <135803462+is0692vs@users.noreply.github.com>
Docstrings generation was requested by @is0692vs. The following files were modified: * `apps/api/src/routes/users.ts` These files were ignored: * `apps/api/src/routes/__tests__/papers.test.ts` * `apps/api/src/routes/__tests__/users.test.ts` * `apps/api/src/utils/__tests__/validation.test.ts`
Co-authored-by: is0692vs <135803462+is0692vs@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
* Restore BatchItem typings in papers.ts db.batch calls. * Use Promise.allSettled and properly sequence child table deletion in catch blocks. * Fix private paper visibility logic by removing innerJoin in orgs.ts. * Fix memory cache LRU ordering and eviction in users.ts. * Clean up AI comments and strengthen tests in papers.test.ts and users.test.ts. * Revert validation en-dash for backward test compatibility. * Fix statsLoading async race condition in paper-detail-client.tsx. Co-authored-by: is0692vs <135803462+is0692vs@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Deployment failed with the following error: Learn More: https://vercel.com/hirokis-projects-afd618c7?upgradeToPro=build-rate-limit |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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 configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (9)
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Summary of ChangesHello, I'm Gemini Code Assist1! 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 focuses on restoring critical fixes and addressing regressions across the API and frontend, aiming to enhance the system's stability, data integrity, and user experience. It tackles issues ranging from database transaction robustness and cache management to UI loading states and test suite clarity, ensuring that previously identified problems are correctly resolved and preventing their reintroduction. Highlights
Changelog
Activity
Using Gemini Code AssistThe 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
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 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
|
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive set of fixes across the API and frontend, addressing regressions and improving robustness. The changes to the paper creation error handling and the LRU cache implementation are significant improvements. The frontend race condition fix in paper-detail-client.tsx is also well-implemented. I've identified a potential issue with unhandled errors during cleanup in papers.ts and an issue with committing runtime artifacts. Please see my detailed comments.
Replaced parallel `Promise.all` with a sequential `for...await` loop to populate the cache boundary, ensuring a deterministic insertion order to stabilize the eviction expectations for the `limit0` query. Co-authored-by: is0692vs <135803462+is0692vs@users.noreply.github.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
* Add `*.pid`, `*_dev.log`, and `start_dev.sh` to `.gitignore`. * Remove accidentally included `update_layout.js`. Co-authored-by: is0692vs <135803462+is0692vs@users.noreply.github.com>
|
このPRは分割し、以下の小さいPR群に置き換えます。\n\n- #139 fix(api): harden paper creation cleanup\n- #140 fix(api): tighten user search cache behavior\n- #141 fix(web): surface paper stats loading errors\n- #142 test(api): align slug validation messaging\n- #143 chore(repo): remove obsolete layout helper\n\n以降のレビューは各分割PRでお願いします。 |
了解しました。以降の対応は分割された各PRにて進めさせていただきます。本PRでの作業はここで終了とします。 |
This PR consolidates and supersedes #137.
Changes
POST /api/paperscleanup so R2/D1 cleanup failures are logged without losing the original errorMAX_CACHE_SIZE, centralizing the default, and keeping eviction tests deterministic*.pid,start_dev.sh) and ignores pid filesConsolidation
This PR absorbs:
Notes:
Greptile Summary
このPRは
POST /api/papersのクリーンアップ処理の堅牢化、ユーザー検索キャッシュのLRU実装改善、stats取得時のローディング状態リグレッション修正、および開発用一時ファイルの除去を目的としています。変更内容は全体的に正しい方向性ですが、前回レビューで指摘された2点が依然として未修正の状態です。papers.tsクリーンアップ改善: 子テーブルの削除がPromise.allSettledで保護されるようになり、FK制約違反も回避できるよう削除順序が整理されたusers.tsLRUキャッシュ修正: キャッシュヒット時にエントリを再挿入してLRU順序を維持するようになり、有効なエントリが誤って削除されていたバグも修正されたpaper-detail-client.tsxローディング修正:fetchStats()がawaitされずにsetStatsLoading(false)が即座に呼ばれていたリグレッションを.finally()チェーンで修正validation.ts表記統一: エラーメッセージのハイフンをエンダッシュに変更し、テストも合わせて更新update_layout.js)の削除と.gitignoreへの*.pid等の追加await db.delete(papers)がcatchブロック内でまだ try-catch に囲まれておらず、この行が失敗すると元のエラーが失われる問題が残っているfetchStats内のcatch { // Ignore }がsetStatsErrorを呼ばないため、APIエラー発生時もユーザーへのフィードバックがない状態が継続しているConfidence Score: 3/5
db.delete(papers)の try-catch 欠如とfetchStatsのsetStatsError非呼び出しの2点が今回のPRでも解消されていない。これらは実際の障害調査やUX品質に影響するため、スコアを 3 とした。apps/api/src/routes/papers.ts(クリーンアップのエラー伝播)とapps/web/src/app/papers/[id]/paper-detail-client.tsx(stats エラーのサイレント無視)に注意が必要Important Files Changed
Promise.allSettledで保護した。ただし最後のawait db.delete(papers)は依然として try-catch で囲まれておらず、失敗すると元のエラーが失われる問題(前回レビューで指摘済み)が残っている。.finally()で修正。ただしfetchStats内のcatch { // Ignore }はそのままで、エラー時にsetStatsErrorが呼ばれない問題(前回指摘済み)は依然未修正。Sequence Diagram
sequenceDiagram participant Client participant PapersRoute as POST /api/papers participant R2 as R2 Bucket participant D1 as D1 Database Client->>PapersRoute: POST /api/papers (FormData) PapersRoute->>R2: upload files (uploadedKeys[]) R2-->>PapersRoute: ok PapersRoute->>D1: db.batch([insertPaper, insertAuthor, (insertOrg?), insertFiles]) Note over PapersRoute,D1: org_only: 4 ops / other: 3 ops alt batch 成功 D1-->>PapersRoute: ok PapersRoute-->>Client: 201 { paper: { id } } else batch 失敗 (catch error) PapersRoute->>R2: Promise.allSettled(BUCKET.delete × N) Note over PapersRoute,R2: R2 削除失敗はログのみ (allSettled) PapersRoute->>D1: Promise.allSettled([delete paperFiles, paperOrgs, paperAuthors]) Note over PapersRoute,D1: 子テーブル削除失敗は無視 (allSettled) PapersRoute->>D1: await db.delete(papers) ⚠️ try-catch なし Note over PapersRoute,D1: 失敗すると元の error が失われる PapersRoute-->>Client: throw error (元のエラー) endPrompt To Fix All With AI
Last reviewed commit: f11a727