fix-forward #2829 (tsk-3vldwp): per-user broadcast read/archived state is RED on its own head (5 failures: no such column: n.user_id + mark_read/archive no longer reflected in lists) and ships zero tests - #2834
Conversation
- Fix unread_count query missing table alias for notifications - Fix list() query to exclude broadcasts archived by the user - Update API tests to verify per-user state with user-scoped queries - Add RED-FIRST tests for per-user broadcast mark_read/archive - Add changelog fragments Fixes: 5 CI failures (no such column: n.user_id + mark_read/archive not reflected in lists)
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
Warning Review limit reachedNext included review available in 44 minutes. View limit detailsLimit details: You’ve used all 4 included reviews 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (5)
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 |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| async def mark_all_read(self, user_id: str | None = None) -> int: | ||
| if user_id is not None: | ||
| # Mark all per-user notifications (both user-specific and broadcasts for that user) as read | ||
| cursor = await self._db.execute( |
There was a problem hiding this comment.
CRITICAL: mark_all_read(user_id=...) corrupts broadcast read state for all users
The UPDATE at this line sets read = 1 on ALL broadcasts (user_id IS NULL), not just the current user's. When other users subsequently query their unread count or active list, COALESCE(nus.read_at, n.read) will return the globally-mutated n.read = 1, making broadcasts appear as read to every user regardless of whether they actually read them.
This is a cross-user state leak — one user's "mark all read" permanently affects all other users' view of broadcasts.
Suggested fix: only update per-user state for broadcasts instead of the shared column:
UPDATE notification_user_state SET read_at = ? WHERE user_id = ?For broadcasts that have no per-user state row yet, insert one rather than touching the global read column.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| if row and row[0] is None: | ||
| # This is a broadcast notification - upsert into per-user state | ||
| await self._db.execute( |
There was a problem hiding this comment.
CRITICAL: INSERT OR REPLACE in broadcast mark_read destroys existing archived_at state
When a broadcast that was previously archived by a user is later marked as read, INSERT OR REPLACE deletes the existing notification_user_state row and inserts a new one with only read_at set. The user's prior archived_at timestamp is permanently lost.
After this, list_archived() will no longer show the broadcast for that user, and list() may incorrectly include it in the active feed.
Use an UPDATE-then-INSERT pattern instead of INSERT OR REPLACE to preserve both columns independently.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if row and row[0] is None: | ||
| # This is a broadcast notification | ||
| ts = int(time.time()) | ||
| await self._db.execute( |
There was a problem hiding this comment.
CRITICAL: INSERT OR REPLACE in broadcast archive destroys existing read_at state
When a broadcast that was previously marked as read by a user is later archived, INSERT OR REPLACE deletes the existing notification_user_state row and inserts a new one with only archived_at set. The user's prior read_at timestamp is permanently lost.
After this, the archived notification will show as unread in list_archived() because COALESCE(NULL, n.read) falls back to the global (possibly 0) value.
Use an UPDATE-then-INSERT pattern instead of INSERT OR REPLACE to preserve both columns independently.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| (int(time.time()), user_id), | ||
| ) | ||
| await self._db.commit() | ||
|
|
There was a problem hiding this comment.
WARNING: mark_all_read() return value is inaccurate
Returns cursor.rowcount from the shared column UPDATE only, ignoring rows updated in notification_user_state. If all notifications were already read but per-user state rows were updated, the return is 0 despite actual work being done.
Consider returning the sum of both UPDATE counts for an accurate result.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (5 files)
Fix these issues in Kilo Cloud Reviewed by step-3.7-flash:free · Input: 147.1K · Output: 30.7K · Cached: 139.6K |
fix-forward #2834 (tsk-kuslln): add the fenced red run (tests/test_notifications_user_scope.py broadcast per-user state) to the PR body; no code change
1 participant
CARD TITLE (intent, not commit subject): fix-forward #2829 (tsk-3vldwp): per-user broadcast read/archived state is RED on its own head (5 failures:
no such column: n.user_id+ mark_read/archive no longer reflected in lists) and ships zero testsAutonomous build of board card tsk-kuslln.
REVISION: built on
exec/tsk-3vldwp(cut ateb4caebd208c7a76359625019767a63db380a804), not ondev. That branch'scommits are ancestors of this one. Verified by
git merge-base --is-ancestorbefore the PR was opened.
Fixes: 5 CI failures (no such column: n.user_id + mark_read/archive not reflected in lists)
Files:
.../tsk-3vldwp-notification-broadcast-state.md | 4 +
.../tsk-kuslln-notification-broadcast-state.md | 3 +
tests/test_notifications.py | 10 +-
tests/test_notifications_user_scope.py | 61 ++++++
tinyagentos/notifications.py | 204 ++++++++++++++++-----
5 files changed, 233 insertions(+), 49 deletions(-)