feat(search): ברירת המחדל בחיפוש הגלובלי היא 10 תוצאות, ונוספה אפשרות 5 - #3344
Merged
Conversation
הערך חי בשלושה מקומות בשלוש שפות — ה-``selected`` ב-``files.html``, הקבוע ב-``global_search.js``, וברירת המחדל של ``/api/search/global`` ב-``app.py``. קודם הוא היה משוכפל גם בתוך ה-JS עצמו בשני אתרי קריאה. עכשיו ה-JS מחזיק קבוע אחד (``DEFAULT_RESULTS_PER_PAGE``), שלושת הקבצים מסכימים, ו-``tests/test_search_results_per_page_default.py`` קושר ביניהם: אין דרך התנהגותית לאמת ש-HTML, JS ו-Python מסכימים על מספר בלי להרים דפדפן, ולכן הטסט קורא את שלושתם. הוכח שהוא נופל — שיניתי את ה-``selected`` ב-HTML בלבד והוא תפס. ברירת המחדל בשרת שונתה גם היא ל-10. הלקוח תמיד שולח ``limit``, אז זו נפילה-לאחור בלבד, אבל ברירת מחדל שאינה תואמת הייתה מחזירה 20 למי שקורא ל-API ישירות בזמן שהמסך מבטיח 10. השרת כולא ל-1..100, ולכן 5 עובר. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011URGXZrC6oiQR8PBET7QCM
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
Reviewer's GuideThe PR changes global search from a default of 20 results to 10, adds a 5-result option, and prevents drift between the HTML selector, JavaScript request/reset logic, and API fallback through a shared-client constant plus focused regression tests. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
Warning Review limit reachedNext included review available in 4 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
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 |
Contributor
🧯 Dangerous deletes guard reportPolicy: see .cursorrules — dangerous deletions are blocked unless wrapped safely. Summary:
Flagged findings (file:line:snippet): Excluded matches (by path pattern) |
Contributor
⏱️ Performance report(No performance test durations collected. Mark tests with |
Contributor
📖 Documentation PreviewThe documentation has been built successfully!
To view locally:
|
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments### Comment 1
<locationpath="tests/test_search_results_per_page_default.py"line_range="78-81" />
<code_context>
+++@pytest.mark.parametrize("value", EXPECTED_OPTIONS)
+def test_every_offered_option_survives_the_server_clamp(value):
+ """5 הוא חדש — צריך לוודא שהשרת לא כולא אותו כלפי מעלה."""
+ assert min(100, max(1, value)) == value
</code_context>
<issue_to_address>
**issue (testing):** This test only evaluates Python's local `min(100, max(1, value))` expression and never exercises or inspects the server's clamp, so a backend change that clamps the new `5` option to `10` still passes.
**Triggers:** When the server's limit-clamping implementation changes independently of the test.
**Suggested fix:** Exercise the endpoint with each offered value, or add a source assertion that the actual `api_search_global` clamp preserves every offered value.
```suggestion@pytest.mark.parametrize("value", EXPECTED_OPTIONS)def test_every_offered_option_survives_the_server_clamp(value): """5 הוא חדש — צריך לוודא שהשרת לא כולא אותו כלפי מעלה.""" py = _read("webapp/app.py") m = re.search( r"def api_search_global\(\):.*?limit = min\((\d+), max\((\d+), " r"int\(payload\.get\('limit'\) or \d+\)\)\)", py, re.S, ) assert m, "the /api/search/global limit clamp changed shape" upper, lower = map(int, m.groups()) assert lower <= value <= upper```
</issue_to_address>
### Comment 2
<locationpath="webapp/app.py"line_range="9604" />
<code_context>
+ # ``files.html``. הלקוח תמיד שולח ``limit``, ולכן זו נפילה-לאחור+ # בלבד — אבל ברירת מחדל שאינה תואמת הייתה מחזירה 20 למי שקורא+ # ל-API ישירות בזמן שהמסך מבטיח 10.+ limit = min(100, max(1, int(payload.get('limit') or 10))) except Exception:- limit = 20
</code_context>
<issue_to_address>
**nitpick:** The repository's global-search implementation guides still document the old `10 / 20 / 50` selector with `20` selected, and one guide still specifies `DEFAULT_RESULTS_PER_PAGE: 20`; those instructions now contradict the implemented default of `10` and the new `5` option.
**Triggers:** When a maintainer uses the feature guides as the reference for future changes or reimplementation.
**Suggested fix:** Update the affected global-search guides to document `5 / 10 / 20 / 50` with `10` as the default, or clearly mark the old snippets as historical.
</issue_to_address>Sourcery assessment
Approval pending. 1 finding to address first.
Blocking findings: tests/test_search_results_per_page_default.py:81
Comment on lines
+78
to
+81
| @pytest.mark.parametrize("value", EXPECTED_OPTIONS) | ||
| def test_every_offered_option_survives_the_server_clamp(value): | ||
| """5 הוא חדש — צריך לוודא שהשרת לא כולא אותו כלפי מעלה.""" | ||
| assert min(100, max(1, value)) == value |
Contributor
There was a problem hiding this comment.
issue (testing): This test only evaluates Python's local min(100, max(1, value)) expression and never exercises or inspects the server's clamp, so a backend change that clamps the new 5 option to 10 still passes.
Triggers: When the server's limit-clamping implementation changes independently of the test.
Suggested fix: Exercise the endpoint with each offered value, or add a source assertion that the actual api_search_global clamp preserves every offered value.
Suggested change
| @pytest.mark.parametrize("value", EXPECTED_OPTIONS) | |
| deftest_every_offered_option_survives_the_server_clamp(value): | |
| """5 הוא חדש — צריך לוודא שהשרת לא כולא אותו כלפי מעלה.""" | |
| assertmin(100, max(1, value)) ==value | |
| @pytest.mark.parametrize("value", EXPECTED_OPTIONS) | |
| deftest_every_offered_option_survives_the_server_clamp(value): | |
| """5 הוא חדש — צריך לוודא שהשרת לא כולא אותו כלפי מעלה.""" | |
| py=_read("webapp/app.py") | |
| m=re.search( | |
| r"def api_search_global\(\):.*?limit = min\((\d+), max\((\d+), " | |
| r"int\(payload\.get\('limit'\) or \d+\)\)\)", | |
| py, | |
| re.S, | |
| ) | |
| assertm, "the /api/search/global limit clamp changed shape" | |
| upper, lower=map(int, m.groups()) | |
| assertlower<=value<=upper |
| # ``files.html``. הלקוח תמיד שולח ``limit``, ולכן זו נפילה-לאחור | ||
| # בלבד — אבל ברירת מחדל שאינה תואמת הייתה מחזירה 20 למי שקורא | ||
| # ל-API ישירות בזמן שהמסך מבטיח 10. | ||
| limit = min(100, max(1, int(payload.get('limit') or 10))) |
Contributor
There was a problem hiding this comment.
nitpick: The repository's global-search implementation guides still document the old 10 / 20 / 50 selector with 20 selected, and one guide still specifies DEFAULT_RESULTS_PER_PAGE: 20; those instructions now contradict the implemented default of 10 and the new 5 option.
Triggers: When a maintainer uses the feature guides as the reference for future changes or reimplementation.
Suggested fix: Update the affected global-search guides to document 5 / 10 / 20 / 50 with 10 as the default, or clearly mark the old snippets as historical.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
3 issues found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/test_search_results_per_page_default.py">
<violation number="1" location="tests/test_search_results_per_page_default.py:81">
P3: test_every_offered_option_survives_the_server_clamp doesn't check the server at all — it recomputes the same formula `min(100, max(1, value))` with literals and asserts a tautology that [5,10,20,50] always satisfies. It never reads app.py, so if the server clamp changed (e.g. to `max(20, ...)`) this test still passes, contradicting its docstring. Remove it (test_the_server_default_agrees_with_the_client already locks the clamp shape) or make it read app.py and verify each EXPECTED_OPTIONS value survives the clamp.</violation>
</file>
<file name="webapp/app.py">
<violation number="1" location="webapp/app.py:9604">
P3: Low severity: Update the global-search implementation guides to document the `5 / 10 / 20 / 50` selector with `10` selected; the current guides now describe behavior this change no longer implements.</violation>
</file>
<file name="webapp/static/js/global_search.js">
<violation number="1" location="webapp/static/js/global_search.js:7">
P2: Custom agent: **Enforce Pragmatic Test Coverage**
This changes the user-visible results-per-page behavior, but the new tests only compare source text and a local clamp expression. Add a behavioral test that exercises the selector/default and verifies the corresponding search request (including the new `5` option) and result state.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // על הערך הזה: ה-``selected`` שב-``webapp/templates/files.html``, הקבוע | ||
| // הזה, וברירת המחדל של ``/api/search/global`` ב-``webapp/app.py``. | ||
| // הם בשלוש שפות ואי אפשר לחלוק ביניהם קבוע, ולכן ההערה הזו היא הקישור. | ||
| const DEFAULT_RESULTS_PER_PAGE = '10'; |
Contributor
There was a problem hiding this comment.
P2: Custom agent: Enforce Pragmatic Test Coverage
This changes the user-visible results-per-page behavior, but the new tests only compare source text and a local clamp expression. Add a behavioral test that exercises the selector/default and verifies the corresponding search request (including the new 5 option) and result state.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At webapp/static/js/global_search.js, line 7:
<comment>This changes the user-visible results-per-page behavior, but the new tests only compare source text and a local clamp expression. Add a behavioral test that exercises the selector/default and verifies the corresponding search request (including the new `5` option) and result state.</comment>
<file context>
@@ -1,5 +1,11 @@
+ // על הערך הזה: ה-``selected`` שב-``webapp/templates/files.html``, הקבוע
+ // הזה, וברירת המחדל של ``/api/search/global`` ב-``webapp/app.py``.
+ // הם בשלוש שפות ואי אפשר לחלוק ביניהם קבוע, ולכן ההערה הזו היא הקישור.
+ const DEFAULT_RESULTS_PER_PAGE = '10';
+
let currentSearchQuery = '';
</file context>
| @pytest.mark.parametrize("value", EXPECTED_OPTIONS) | ||
| def test_every_offered_option_survives_the_server_clamp(value): | ||
| """5 הוא חדש — צריך לוודא שהשרת לא כולא אותו כלפי מעלה.""" | ||
| assert min(100, max(1, value)) == value |
Contributor
There was a problem hiding this comment.
P3: test_every_offered_option_survives_the_server_clamp doesn't check the server at all — it recomputes the same formula min(100, max(1, value)) with literals and asserts a tautology that [5,10,20,50] always satisfies. It never reads app.py, so if the server clamp changed (e.g. to max(20, ...)) this test still passes, contradicting its docstring. Remove it (test_the_server_default_agrees_with_the_client already locks the clamp shape) or make it read app.py and verify each EXPECTED_OPTIONS value survives the clamp.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_search_results_per_page_default.py, line 81:
<comment>test_every_offered_option_survives_the_server_clamp doesn't check the server at all — it recomputes the same formula `min(100, max(1, value))` with literals and asserts a tautology that [5,10,20,50] always satisfies. It never reads app.py, so if the server clamp changed (e.g. to `max(20, ...)`) this test still passes, contradicting its docstring. Remove it (test_the_server_default_agrees_with_the_client already locks the clamp shape) or make it read app.py and verify each EXPECTED_OPTIONS value survives the clamp.</comment>
<file context>
@@ -0,0 +1,81 @@
+@pytest.mark.parametrize("value", EXPECTED_OPTIONS)
+def test_every_offered_option_survives_the_server_clamp(value):
+ """5 הוא חדש — צריך לוודא שהשרת לא כולא אותו כלפי מעלה."""
+ assert min(100, max(1, value)) == value
</file context>
| # ``files.html``. הלקוח תמיד שולח ``limit``, ולכן זו נפילה-לאחור | ||
| # בלבד — אבל ברירת מחדל שאינה תואמת הייתה מחזירה 20 למי שקורא | ||
| # ל-API ישירות בזמן שהמסך מבטיח 10. | ||
| limit = min(100, max(1, int(payload.get('limit') or 10))) |
Contributor
There was a problem hiding this comment.
P3: Low severity: Update the global-search implementation guides to document the 5 / 10 / 20 / 50 selector with 10 selected; the current guides now describe behavior this change no longer implements.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At webapp/app.py, line 9604:
<comment>Low severity: Update the global-search implementation guides to document the `5 / 10 / 20 / 50` selector with `10` selected; the current guides now describe behavior this change no longer implements.</comment>
<file context>
@@ -9596,9 +9596,14 @@ def _is_regex_safe(p: str) -> bool:
+ # ``files.html``. הלקוח תמיד שולח ``limit``, ולכן זו נפילה-לאחור
+ # בלבד — אבל ברירת מחדל שאינה תואמת הייתה מחזירה 20 למי שקורא
+ # ל-API ישירות בזמן שהמסך מבטיח 10.
+ limit = min(100, max(1, int(payload.get('limit') or 10)))
except Exception:
- limit = 20
</file context>
Uh oh!
There was an error while loading. Please reload this page.
17 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
תבנית Pull Request
✨ תיאור קצר
בורר מספר התוצאות בחיפוש הגלובלי הציע
10 / 20 / 50עם ברירת מחדל 20. עכשיו הוא מציע5 / 10 / 20 / 50עם ברירת מחדל 10.הערך הזה חי בשלוש שפות: ה-
selectedב-webapp/templates/files.html, קוד ה-JS ב-webapp/static/js/global_search.js, וברירת המחדל של/api/search/globalב-webapp/app.py. בתוך ה-JS הוא היה משוכפל גם בין שני אתרי קריאה — איפוס הסלקטים ובניית גוף הבקשה. ארבעה מקומות, בלי שום דבר שקושר ביניהם.📦 שינויים עיקריים
1. הבורר (
files.html) — נוספה<option value="5">, ו-selectedעבר ל-10.2. קבוע אחד ב-JS —
DEFAULT_RESULTS_PER_PAGEמוצהר פעם אחת ומשמש בשני אתרי הקריאה, במקום'20'קשיח בכל אחד מהם.3. ברירת המחדל בשרת יושרה ל-10. הלקוח תמיד שולח
limit, ולכן זו נפילה-לאחור בלבד — אבל ברירת מחדל שאינה תואמת הייתה מחזירה 20 למי שקורא ל-API ישירות בזמן שהמסך מבטיח 10. השרת כולא ל-[1, 100], ולכן 5 עובר בלי שינוי נוסף.🧪 בדיקות
tests/test_search_results_per_page_default.py— 9 טסטים שקושרים בין שלושת הקבצים: רשימת האפשרויות, ה-selectedב-HTML, הקבוע ב-JS, ברירת המחדל בשרת, והיעדר עותקים קשיחים שנשארו מאחור.הוכח שהטסט מסוגל להיכשל: החזרתי את ה-
selectedל-20 ב-HTML בלבד, והטסט תפס (1 failed, 8 passed). אחרי השחזור — 9 עוברים.זו בדיקת טקסט על קוד מקור, וזה מכוון. אין דרך התנהגותית לאמת שקובץ HTML, קובץ JS וקובץ Python מסכימים על מספר בלי להרים דפדפן אמיתי. המחיר ידוע — פירמוט מחדש יפיל אותה — והודעות הכשל מנוסחות כדי שיהיה ברור מיד שזה מה שקרה. זה מתועד ב-docstring של הקובץ.
רגרסיה: 27 טסטים בקבצים שנוגעים לחיפוש הגלובלי — עוברים.
🧪 בדיקות נדרשות ב‑PR
📝 סוג שינוי
✅ צ'קליסט
/api/search/global🧩 השפעות/סיכונים
משתמש שרגיל ל-20 תוצאות יקבל 10. הבחירה שלו אינה נשמרת בין טעינות ממילא —
global_search.jsמאפס את הסלקטים לברירת המחדל, וזו התנהגות קיימת שלא שיניתי.🧯 סיכון / החזרה לאחור (Rollback)
git revert. אין מצב מתמשך ואין נתונים שנוגעים בהם.🔗 קישורים
🤖 Generated with Claude Code
https://claude.ai/code/session_011URGXZrC6oiQR8PBET7QCM
Generated by Claude Code
Summary by Sourcery
Set the global search default to 10 results, add a 5-result option, and enforce consistency across the UI, client, and API.
New Features:
Bug Fixes:
Enhancements:
Tests: