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>
|
👋 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 |
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 significantly improves the stability and reliability of the API by addressing several critical areas. It enhances testing coverage for key functionalities, refines caching mechanisms for better performance, and strengthens error handling during resource cleanup. These changes collectively lead to a more robust and predictable API behavior, reducing potential issues and improving maintainability. 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
|
![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 introduces a good set of fixes and improvements across API routes and their tests. The changes enhance test coverage for paper creation, make user cache tests more efficient, and improve error logging in the paper upload process. The implementation of an MRU cache for user search is a definite improvement. I have one suggestion regarding the handling of the cache size configuration to make it more robust.
apps/api/src/routes/users.ts
Outdated
| .all(); | ||
|
|
||
| setCachedResults(cacheKey, results); | ||
| setCachedResults(cacheKey, results, c.env.MAX_CACHE_SIZE as any as number ?? 1000); |
There was a problem hiding this comment.
The type assertion as any as number is not safe as it bypasses TypeScript's type checking and doesn't correctly parse a string value from environment variables. This can lead to unexpected behavior if the environment variable is not a number. Using Number() provides explicit and safer type conversion, with a reliable fallback for invalid or missing values. Additionally, consider defining the fallback value 1000 as a pre-defined constant to improve maintainability, as per repository guidelines on avoiding hardcoded values.
| setCachedResults(cacheKey, results, c.env.MAX_CACHE_SIZE as any as number ?? 1000); | |
| setCachedResults(cacheKey, results, Number(c.env.MAX_CACHE_SIZE) || 1000); |
References
- Use pre-defined constants for validation logic instead of hardcoding values to improve maintainability.
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
- \`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>
|
Note Docstrings generation - SUCCESS |
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`
|
Plan generation is already in progress for this pull request. Please wait for it to complete. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/api/src/routes/users.ts`:
- Around line 88-91: The function setCachedResults currently duplicates the
default cache size (maxSize = 1000) with a caller-side fallback (|| 1000);
remove the redundant fallback and centralize the default by using the existing
MAX_CACHE_SIZE constant as the single source of truth. Update setCachedResults
signature to accept maxSize = MAX_CACHE_SIZE (or rely on the existing signature
if already set) and remove any "|| 1000" fallbacks where setCachedResults is
called so callers can pass a test-specific size when needed; reference function
setCachedResults and constant MAX_CACHE_SIZE to locate the changes.
- Line 136: The code is failing because Env lacks MAX_CACHE_SIZE and the
cast/`??` order is wrong; add MAX_CACHE_SIZE?: string to the Env type
declaration and change the call that uses c.env.MAX_CACHE_SIZE (the
setCachedResults(cacheKey, results, ... ) call) to coerce the env string to a
number with the nullish fallback applied first, e.g. Number(c.env.MAX_CACHE_SIZE
?? '1000') or parseInt(c.env.MAX_CACHE_SIZE ?? '1000', 10), so the fallback is
used when the env is undefined and the value passed to setCachedResults is a
proper number.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ca1b7eae-070d-4150-a718-f3407243d4dc
📒 Files selected for processing (5)
apps/api/src/routes/__tests__/papers.test.tsapps/api/src/routes/__tests__/users.test.tsapps/api/src/routes/papers.tsapps/api/src/routes/users.tsapps/api/src/utils/__tests__/validation.test.ts
| function setCachedResults(key: string, data: any[], maxSize = 1000) { | ||
| if (searchCache.has(key)) { | ||
| searchCache.delete(key); | ||
| } else if (searchCache.size >= MAX_CACHE_SIZE) { | ||
| } else if (searchCache.size >= maxSize) { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
maxSizeパラメータのデフォルト値が重複しています。
setCachedResults関数のデフォルト値maxSize = 1000と、呼び出し側のフォールバック|| 1000（修正後）が重複しています。どちらか一方で十分です。
♻️ リファクタリング案
デフォルト値を関数シグネチャに統一する場合:
-function setCachedResults(key: string, data: any[], maxSize = 1000) {
+function setCachedResults(key: string, data: any[], maxSize?: number) {
+ const effectiveMaxSize = maxSize ?? MAX_CACHE_SIZE;
if (searchCache.has(key)) {
searchCache.delete(key);
- } else if (searchCache.size >= maxSize) {
+ } else if (searchCache.size >= effectiveMaxSize) {これにより、MAX_CACHE_SIZE定数（line 75）をデフォルト値として再利用し、テスト時のみ異なる値を注入できます。
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function setCachedResults(key: string, data: any[], maxSize = 1000) { | |
| if (searchCache.has(key)) { | |
| searchCache.delete(key); | |
| } else if (searchCache.size >= MAX_CACHE_SIZE) { | |
| } else if (searchCache.size >= maxSize) { | |
| function setCachedResults(key: string, data: any[], maxSize?: number) { | |
| const effectiveMaxSize = maxSize ?? MAX_CACHE_SIZE; | |
| if (searchCache.has(key)) { | |
| searchCache.delete(key); | |
| } else if (searchCache.size >= effectiveMaxSize) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/api/src/routes/users.ts` around lines 88 - 91, The function
setCachedResults currently duplicates the default cache size (maxSize = 1000)
with a caller-side fallback (|| 1000); remove the redundant fallback and
centralize the default by using the existing MAX_CACHE_SIZE constant as the
single source of truth. Update setCachedResults signature to accept maxSize =
MAX_CACHE_SIZE (or rely on the existing signature if already set) and remove any
"|| 1000" fallbacks where setCachedResults is called so callers can pass a
test-specific size when needed; reference function setCachedResults and constant
MAX_CACHE_SIZE to locate the changes.
|
Plan generation is already in progress for this pull request. Please wait for it to complete. |
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
|
|
Summary
🤖 Prompt for AI Agents |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
|
✅ Unit tests committed locally. Commit: |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
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>
|
Summary
🤖 Prompt for AI Agents |
|
このPRは #138 に統合しました。以降のレビューは統合先PRでお願いします。 |
了解しました。このPRが別のPRに統合されたことを確認しました。こちらの作業は停止し、以降の対応は不要と認識しております。 |
1 participant
This PR addresses multiple test gaps, caching flaws, and error handling bugs across `apps/api` features.
Changes:
c.env.MAX_CACHE_SIZEinjection.validateSlug. Added assertions to correctly reject inputs with leading or trailing hyphens.PR created automatically by Jules for task 8376435226217890193 started by @is0692vs
Greptile Summary
このPRは
apps/apiの複数のテストギャップ・キャッシュ不具合・エラーハンドリングのバグを修正します。主な変更はユーザー検索キャッシュへのLRUタッチ機能追加、ペーパーアップロード失敗時のクリーンアップフロー改善、テストカバレッジの強化、およびスラグバリデーションテストの拡充です。users.ts:getCachedResultsでキャッシュヒット時に delete + re-insert によるLRUタッチを実装。setCachedResultsにmaxSizeパラメータを追加し、c.env.MAX_CACHE_SIZEをNumber()で適切に変換して注入する形に変更。papers.ts: アップロード失敗時の catch ブロックでPromise.allSettledの結果を収集し、R2/D1 クリーンアップ失敗をpaperId付きでログ出力。元エラーを正しく re-throw するよう改善。papers.test.ts: insert モックを拡張して batch 引数の詳細検証を可能にし、クリーンアップ失敗ロギングと元エラー re-throw の新テストを追加。users.test.ts: LRUタッチ動作・既存キー更新時の非退避を検証する新テストを追加。MAX_CACHE_SIZE テストを小さな env 変数で高速化。validation.test.ts:validateSlugに対して先頭/末尾ハイフン、全ハイフン、最小長スラグ等のエッジケーステストを追加。Confidence Score: 3/5
papers.tsとusers.tsの本番コード変更は正確で意図した動作を実現している。一方、users.test.tsの新テストでvi.useFakeTimers()がtry/finallyなしで使用されており、アサション失敗時にフェイクタイマーが後続テストに漏れ込むリスクがある。またusers.tsのMAX_CACHE_SIZE定数がデッドコードとして残存している。Important Files Changed
Number()変換も正しく実装されているが、未使用のMAX_CACHE_SIZE定数が残っている。setCachedResults does not evict another entryテスト内でvi.useFakeTimers()がtry/finallyなしで使用されており、テスト失敗時にフェイクタイマーが残り後続テストに影響する可能性がある。thrown変数が残るが、全体的にカバレッジ向上に貢献している。Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A["GET /api/users/search?q=xxx"] --> B{クエリ長 >= 2?} B -- No --> C["空リストを返す"] B -- Yes --> D["getCachedResults(cacheKey)"] D --> E{キャッシュヒット\nかつTTL有効?} E -- Yes --> F["delete + re-insert\nLRUタッチ"] F --> G["キャッシュデータを返す"] E -- No --> H["期限切れエントリ削除"] H --> I["DBクエリ実行"] I --> J["setCachedResults(key, results, maxSize)"] J --> K{既存キーあり?} K -- Yes --> L["delete して LRU 更新"] K -- No --> M{size >= maxSize?} M -- Yes --> N["先頭キーを削除\n最古エントリを退避"] M -- No --> O["新エントリを set"] L --> O N --> O O --> P["結果を返す"]Comments Outside Diff (2)
apps/api/src/routes/users.ts, line 75 (link)MAX_CACHE_SIZEがデッドコードになっているsetCachedResultsにはデフォルト引数maxSize = 1000が追加され、呼び出し元ではc.env.MAX_CACHE_SIZEを利用するようになりました。そのため、モジュールスコープで宣言されたconst MAX_CACHE_SIZE = 1000はどこからも参照されなくなっています。（行を削除）
Prompt To Fix With AI
apps/api/src/routes/__tests__/users.test.ts, line 322-326 (link)vi.useFakeTimers()がtry/finallyなしで使用されているvi.useFakeTimers()を呼んだ後、app.request()が例外をスローした場合や、その後のアサション（line 329）が失敗した場合、vi.useRealTimers()が呼ばれずフェイクタイマーが残り続けます。これにより後続のテストがフェイクタイマーの影響を受け、断続的な失敗の原因となります。同じファイル内の
GET /api/users/search handles cache eviction when key exists but expired（line 246）はtry/finallyパターンを正しく使用しているので、同様の実装を推奨します：Prompt To Fix With AI
Prompt To Fix All With AI
Last reviewed commit: 422ade4