Repository navigation
fix(reminders): הבועה מתרעננת גם כשהיא מוצגת — השרת אומר מתי לחזור, next ירד, והסטאב מחיל היטלה - #3444
Merged
Conversation
… שלו, והסטאב מחיל היטלה שלב 3 בתוכנית התיקונים שאחרי הסקירה של #3430: WARN-004, WARN-011, YAGNI-001, ואיתם SUGG-001 (מדידה) ו-SUGG-008. WARN-004: כשיש תזכורת בשלה הלקוח נרדם לתקרה, חצי שעה, ובועה שנוקתה ממכשיר אחר או מונה שהשתנה חיכו עד אז. עכשיו next_in_seconds מחושב בשני המצבים, וכשיש בועה הוא נחתך ב-BADGE_REFRESH_SECONDS (חמש דקות, המרווח הקבוע הישן) או מוקדם יותר כשתזכורת נוספת מבשילה. הלקוח משתמש באותו חישוב בשני הענפים (requestedDelay), ו-null עדיין מתורגם לתקרה, כך ששרת ישן באמצע דיפלוי אינו משנה התנהגות. YAGNI-001: האובייקט next ירד מהתשובה יחד עם ה-find_one וההיטלה שבנו אותו, כי הצרכן היחיד (base.html) לא קרא אותו מעולם. WARN-011: אחרי ההסרה has_due ו-count_due נגזרים מהספירה בלבד, וה-find_one שנשאר שואל על העתיד, ולכן שתי התשובות אינן יכולות לשלוח בועה בלי תזכורת מאחוריה. בלי aggregate ובלי hint: אינדקס חסר היה הופך hint בשם ל-500 על נתיב שכל טאב מחובר דוגם. SUGG-008: הסטאב בטסטים מחיל היטלה כמו מונגו במקום לזרוק אותה. הראיה: היטלה שבורה בייצור עברה את הסוויטה עם הסטאב הישן ונופלת עם החדש. SUGG-001: explain לפני ואחרי מול הייצור. הספירה מכוסה על user_reminders_summary_idx; השאילתה העתידית עדיין בוחרת remind_at_idx עם FETCH שמסנן user_id, וצורתה אינה משתנה בקומיט הזה. המלצה להמשך בתיאור ה-PR. תיעוד: docs/dev/sticky_notes_extending.rst, docs/webapp/api-reference.rst, webapp/USER_GUIDE.md, וה-docstring של הנתיב (המשפט "נרדם עד הניווט הבא" הוחלף, חפיפה עם SUGG-012). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YNRfYgfyQFv3Gk9UuXDvFx
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
Contributor
Reviewer's GuideThe PR changes sticky-reminder polling to use a server-provided refresh deadline in both badge-visible and badge-hidden states, caps visible-badge polling at five minutes, removes the unused Sequence diagram for server-directed sticky reminder pollingsequenceDiagram
participant Browser
participant API as reminders_summary
participant DB as note_reminders
Browser->>API: GET /reminders/summary
API->>DB: count_documents(due_filter)
DB-->>API: count_due
API->>DB: find_one(upcoming_filter, projection, sort)
DB-->>API: upcoming remind_at
alt has_due
API-->>Browser: has_due, count_due, next_in_seconds = min(300, seconds_until(upcoming))
else no due reminder
API-->>Browser: has_due, count_due, next_in_seconds
end
Browser->>Browser: requestedDelay(j)
Browser->>Browser: schedule(delay)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Warning Review limit reachedNext included review available in 22 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: Advanced Run ID: 📒 Files selected for processing (7)
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) |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/test_sticky_note_reminders.py" line_range="60-63" />
<code_context>
return True
+def _project(doc, projection):
+ """מחיל היטלה כמו מונגו, במקום לזרוק אותה.
+
+ הכללה (``{f: 1}``) שומרת את השדות שנקבו ואת ``_id`` אלא אם ``_id: 0``;
+ החרגה (``{f: 0}``) מסירה את השדות שנקבו. בלי היטלה חוזר המסמך עצמו.
</code_context>
<issue_to_address>
**nitpick (testing):** `_project({'_id': 1})` returns the full document instead of only `_id`: `include` is empty, so the helper falls through to exclusion mode with no excluded fields. The test stub therefore does not implement MongoDB inclusion projection semantics for an `_id`-only projection and can let projection bugs pass.
**Triggers:** When a test exercises an inclusion projection containing only `_id`.
**Suggested fix:** Treat any projection containing inclusion fields, including `_id`, as inclusion mode; return only the requested fields and retain `_id` according to the explicit `_id` projection.
```suggestion
include = {k for k, v in projection.items() if v}
if include:
keep = include | ({'_id'} if projection.get('_id', 1) else set())
return {k: v for k, v in doc.items() if k in keep}
```
</issue_to_address>Sourcery assessment
Approved.
Contributor
⏱️ Performance report(No performance test durations collected. Mark tests with |
Contributor
📖 Documentation PreviewThe documentation has been built successfully!
To view locally:
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
ממצא בסקירה של #3444: היטלה של _id בלבד החזירה בסטאב את המסמך המלא, כי מצב ההכללה נקבע רק לפי שדות שאינם _id. נמדד מול MongoDB 8.0.32 בייצור: {_id: 1} מחזיר רק _id; {_id: 1, f: 0} הוא החרגה (_id הוא היוצא מן הכלל היחיד לאיסור על ערבוב); {_id: 0} מחזיר את כל השאר. התיקון המוצע בסקירה היה שולח את המקרה השני להכללה, ולכן הכלל ממומש במלואו ולא הקיצור. טסט יחידה לעוזר עם חמשת המקרים: על הקוד הישן נופל {_id: 1}, ועל הקיצור המוצע נופל {_id: 1, f: 0}. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YNRfYgfyQFv3Gk9UuXDvFx
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 free
to 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.
✨ תיאור קצר
שלב 3 בתוכנית התיקונים שאחרי הסקירה של #3430 (
code-review-3430-sticky-reminders.mdב-Ck): WARN-004, WARN-011, YAGNI-001, ואיתם SUGG-001 (מדידה בלבד) ו-SUGG-008. הבועה כבר לא נרדמת לחצי שעה: כשיש תזכורת בשלה השרת אומר ללקוח לחזור לכל היותר בעוד חמש דקות, ומוקדם יותר כשתזכורת נוספת מבשילה — ולכן בועה שנוקתה ממכשיר אחר ומונה שהשתנה נראים תוך דקות. האובייקטnext, שאין לו צרכן, ירד יחד עם השאילתה שבנתה אותו, ואיתו הסתירה האפשרית בינו לבין הספירה. והסטאב בטסטים מחיל היטלה כמו מונגו, כך שטסט על היטלה בודק משהו. קומיט אחד,da65a6d, על ענף שאותחל מ-mainאחרי מיזוג #3438 (שלב 2).📦 שינויים עיקריים
webapp/sticky_notes_api.py, הנתיבreminders_summaryבלבדinitStickyRemindersIndicatorב-webapp/templates/base.html, בלי CSStests/test_sticky_note_reminders.py,tests/test_sticky_reminders_polling_browser.py)פירוט:
next_in_secondsחושב רק כשאין תזכורת בשלה, והלקוח, כשיש בועה, החזיר את התקרה: חצי שעה שבה בועה שנוקתה בטלפון נשארה על המסך בדסקטופ, ומונה שהשתנה הראה את המספר הישן. עכשיו השאילתה על התזכורת העתידית הקרובה רצה בשני המצבים, וכשיש בועה התשובה נחתכת ב-BADGE_REFRESH_SECONDS— קבוע חדש במודול, חמש דקות, המרווח הקבוע שהיה כאן לפני הדגימה לפי השרת. הקבוע יושב בשרת ולא בלקוח כדי שללקוח יישאר כלל אחד: "ישן כמה שהשרת אמר, בין הרצפה לתקרה". בלקוח שני הענפים עוברים דרך פונקציה אחת,requestedDelay(j), במקום שני עותקים של אותו חישוב (R6), עם אותה בדיקת טיפוס שכבר הייתה;nullעדיין מתורגם לתקרה, ולכן שרת ישן באמצע דיפלוי אינו משנה את ההתנהגות.nextירד. בדקתי בעצמי שאין צרכן:grepעלreminders/summaryועלreminders_summaryבכל הריפו מוצא רק את ה-IIFE ב-base.html(שקוראok,has_due,count_due,next_in_secondsולאnext), טסטים ותיעוד;sw.jsאינו קורא לנתיב. ירדו: ההיטלהnext_projection, ה-find_oneהממוין על הבשלות, ההמרה ל-ISO והמפתח בתשובה. עודכנו:test_summary_has_due,_summaryו-_DUEבטסטי הדפדפן, הדוגמה ב-docs/webapp/api-reference.rst, השורה ב-webapp/USER_GUIDE.md, וה-docstring.next,has_dueו-count_dueנגזרים מהספירה בלבד, וה-find_oneשנשאר שואל שאלה אחרת — מתי העתידית הקרובה — ולכן שתי התשובות אינן יכולות לשלוח בועה בלי תזכורת מאחוריה. שקלתי אגרגציה יחידה ($matchואז$group) שנותנת צילום עקבי בסיבוב אחד, ודחיתי: היא דורשת מנועaggregateבסטאב, והרווח מול שני סיבובים מאונדקסים על קומץ מסמכים של משתמש אחד אינו פרופורציונלי (R8). ואיןhint: שני האינדקסים המורכבים נוצרים best-effort בתוךexcept: pass, ואינדקס שחסר היה הופךhintבשם ל-500 על נתיב שכל טאב מחובר דוגם; היום אינדקס חסר הוא רק שאילתה איטית יותר._StubColl.find_oneו-findקיבלוprojectionוזרקו אותו, ולכן כל טסט ראה מסמך מלא היכן שהייצור רואה שדה אחד. פונקציית עזר_projectמחילה הכללה (השדות שנקבו ועוד_id, אלא אם_id: 0) והחרגה, כמו במונגו; המיון וה-limitרצים על המסמכים המלאים וההיטלה חלה על היוצא. הראיה שזה משנה משהו: ב-worktree שיניתי את ההיטלה בייצור מ-{'remind_at': 1}ל-{'note_id': 1}— עם הסטאב הישןtest_summary_reports_seconds_until_next_reminderעבר (1 passed), ועם החדש הוא נופל (unexpectedly None). כל שאר הקובץ עובר עם הסטאב החדש, כלומר שום נתיב לא קרא שדה מחוץ להיטלה שלו.explainמול הייצור, לפיdocs/database/indexing.rstנמדד דרך ה-MCP של Atlas, קריאה בלבד,
executionStats, עלcode_keeper_bot.note_reminders(MongoDB 8.0.32), עם ה-user_idשלי; התאריך הועבר כ-EJSON ופורש נכון (הגבולות מראיםnew Date(1789963200000)).count_documents)COUNT←IXSCAN user_reminders_summary_idx(partial), שני טווחיstatus, בליFETCH— מכוסהfind_oneממוין על הבשלות (בנה אתnext)LIMIT←PROJECTION_SIMPLE←FETCHבלי פילטר ←SORT_MERGEעלuser_reminders_summary_idx— שונה מהסקירה, שראתהremind_at_idxעםFETCHעלuser_idfind_oneממוין על העתידית (remind_at > now)LIMIT←PROJECTION_SIMPLE←FETCHעם פילטר{ack_at, status, user_id}←IXSCAN remind_at_idx— צורת SUGG-001; התוכניות עםSORT_MERGEעל שני האינדקסים המורכבים נדחוnReturned,totalKeysExaminedו-totalDocsExaminedהיו 0 בשלוש — למשתמש הזה אין כרגע תזכורת בשלה או עתידית, ולכן הראיה היא צורת התוכנית ולא הזמנים. הבחירה השונה מהסקירה בשאילתה השנייה מאששת את הניתוח שבתוכנית: ל-$inעל שני ערכיstatusהאינדקס המורכב מספק את המיון דרךSORT_MERGE, למתכנן יש שני מועמדים, ועל 23 מסמכים ניסוי התוכניות אינו מבחין ביניהם. המלצה להמשך, לא ל-PR הזה: לשאילתה העתידיתhintבשםuser_reminders_summary_idx, בתנאי שיצירת האינדקס תעבור לאותו מודול ותפסיק להיות שקטה — כי בייצור קיימים שני אינדקסים עם אותו דפוס מפתחות (user_reminders_summary_idxהחלקי ו-user_status_time_idxהמלא), ולכןhintלפי דפוס היה דו-משמעי. הספירה, שמכוסה, אינה צריכה כלום.מקורות חיצוניים שקבעו את המימוש
_idאלא אם_id: 0; החרגה מסירה)find_one(filter, projection, sort=...)pymongo/synchronous/collection.py— החתימה לא השתנתה, ולכן הסטאב תואםexplain(stage,totalDocsExamined,sortPatternבליSORT)docs/database/indexing.rstexplainדרך ה-MCP{"$date": ...}; אומת מהפלט עצמו — גבולות האינדקס הודפסו כ-new Date(...)ולא כמחרוזת🧪 בדיקות
כל טסט חדש הורץ על הקוד של היום ונפל עם הערך הישן; כל תיקון עבר בדיקת מוטציה ב-worktree בסקרצ'פאד או על עותק של הסקריפט בזיכרון; הלקוח נמדד ב-Chromium 141 דרך Playwright 1.49.0. ל-CI אין Chromium (
docs/testing.rst), ולכן הראיה לטסטי הדפדפן היא הפלט המקומי שכאן.שרת (unittest, Flask test client, הסטאב):
next_in_secondsהואNone(None != 300)None(unexpectedly None)Nonenextבתשובה'next' unexpectedly found)count_due1מוטציות (ב-worktree, עם הקובץ המתוקן): הענף מחזיר
Noneכמו קודם ← שלושת טסטי הבשלה נופלים; בלי ה-min, הקבוע גובר תמיד ← "עתידית בעוד שתי דקות" נופל, שני האחרים עוברים. הקובץ: 25 עברו.דפדפן: בשלה עם
next_in_seconds=300← הטיימר[300000]והבועה מוצגת; על הלקוח של היום:[1800000](assert [1800000] == [300000]). בקרה: מוטציה שמחזירה את הענף ל-return MAX_POLL_MS←[1800000]. הטסטים הקיימים עם_DUE(בליnext_in_seconds) ממשיכים לצפות לתקרה — זו תאימות לשרת ישן, ולא שינוי בהם.base.htmlאו בסטיקי יחד עםtest_browser_suite_is_gated_once— 1325 עברו, 45 דולגו, אפס נפלו.webapp/sticky_notes_api.py15 ממצאים, אותם 15 בדיוק ב-main, אף אחד בשורות שנגעתי בהן. ל-JS אין לינטר ב-CI. לא הורץ Prettier על התבנית.explainעל נתוני הייצור של היום (23 מסמכים, ותוצאות ריקות למשתמש שלי) אינו מנבא את בחירת המתכנן בנפח גדול — וזו הסיבה שלא הוסףhintעל סמך המדידה; בניית RTD — פרוזה בעמודים קיימים, בלי כותרות ובלי עוגנים חדשים, ה-check על ה-PR בונה.🧪 בדיקות נדרשות ב‑PR
📝 סוג שינוי
✅ צ'קליסט
docs/database/indexing.rst| המשפט: "sortPattern: וודאו שהמיון נתמך ע"י האינדקס (ללא SORT נוסף)" — זה מה שנמדד בטבלת ה-explain. וגם:AI-MAP.mdבתחילת התכנון,docs/dev/sticky_notes_extending.rst(החוזה של הדגימה — עודכן במשפט אחד),docs/performance-sticky-notes.rst(מי יוצר אינדקסים ומתי — הבסיס להחלטה נגדhint),docs/webapp/api-reference.rst(עודכן),docs/testing.rst.docs/webapp/theming_and_css.rst— לא נקרא, במכוון: אין CSS בשינוי.תיעוד שעודכן:
docs/dev/sticky_notes_extending.rst— משפט אחד בפסקת הדגימה (כשיש בועה השרת עונה לכל היותר חמש דקות), בלי כותרת ובלי עוגן חדש;docs/webapp/api-reference.rst— תיאור הנתיב ודוגמת התשובה בליnextועםnext_in_seconds;webapp/USER_GUIDE.md— השורה על הנתיב; ה-docstring שלreminders_summary— החוזה החדש, והמשפט "הלקוח נרדם עד הניווט הבא" (שהיה שגוי, SUGG-012) הוחלף ב"עד התקרה, חצי שעה".🧩 השפעות/סיכונים
שינויי התנהגות מכוונים: בזמן שבועה מוצגת טאב דוגם לכל היותר כל חמש דקות במקום כל חצי שעה — עד 12 בקשות בשעה לטאב במצב הזה, שנגמר בפעולת משתמש; מגבלת הקצב של הנתיב (300 לדקה למשתמש) רחוקה מזה. השדה
nextנעלם מתשובת ה-API — אין לו צרכן, וה-api-reference מעודכן. סדר דיפלוי: לקוח ישן מול שרת חדש מתעלם מ-next_in_secondsכשיש בועה ומתנהג כמו היום; לקוח חדש מול שרת ישן מקבלnullוחוזר לתקרה — גם הוא כמו היום.סטיות מהתוכנית שאישרת: אין. שתי החלטות שאפשר לשנות במילה:
BADGE_REFRESH_SECONDS = 300, ו-minעם העתידית ולא הקבוע לבדו.מה שנשאר פתוח לפי התוכנית: שלב 4 (WARN-008, WARN-009, WARN-005); שלב 5 (WARN-001, WARN-010, WARN-012, ה-SUGG-ים, ובהם SUGG-002 —
math.ceilב-seconds_until, שמשפיע גם על הערכים שהשרת מחזיר כאן; SUGG-011 — המדריך למשתמש עדיין אינו מציין את זמני ההמתנה). ההמלצה עלhint(למעלה) — לשלב 5 או ל-PR נפרד, יחד עם יצירת האינדקס באותו מודול.🔗 קישורים
code-review-3430-sticky-reminders.mdב-Ck — WARN-004, WARN-011, YAGNI-001, SUGG-001, SUGG-008.🧯 סיכון / החזרה לאחור (Rollback)
Revert של הקומיט היחיד מחזיר את הכל, כולל השדה
nextבתשובה. אין מיגרציה, אין שינוי באינדקסים ואין שינוי בסכימה.🤖 Generated with Claude Code
https://claude.ai/code/session_01YNRfYgfyQFv3Gk9UuXDvFx
Generated by Claude Code
Summary by Sourcery
Keep sticky reminder indicators synchronized across devices by using server-directed polling even while a reminder bubble is visible.
Bug Fixes:
nextobject from the reminders summary API and eliminate the associated inconsistent query.Enhancements:
next_in_secondsvalue with backward-compatible fallback behavior.Documentation:
next.Tests: