Skip to content

Compute isRead and isSeen flags based on notification status#186

Open
gpunto wants to merge 1 commit intodevelopfrom
handle-read-seen-flags
Open

Compute isRead and isSeen flags based on notification status#186
gpunto wants to merge 1 commit intodevelopfrom
handle-read-seen-flags

Conversation

@gpunto
Copy link
Copy Markdown
Contributor

@gpunto gpunto commented Apr 20, 2026

Goal

We need to compute the two flags similarly to how the backend does it, i.e.:

  • Timestamp comparison (updated < lastSeen/lastRead)
  • Group ID is in the seen/read ID lists

Docs here.

Implementation

  • Implement the logic above whenever we receive the notification feed updated event with a non-null notification status. Reference JS implementation here.
  • Update sample to use the new flag

Testing

  1. Run sample app and generate some notifications, e.g. add a reaction or a comment to another user's activities
  2. Log in as the other user: new notifications should show red dots on the unread notifications
  3. Tap a notification to mark it as read: the red dot should disappear
  4. Tap "Mark all read": all red dots should disappear

Checklist

  • Issue linked (if any)
  • Tests/docs updated
  • I have signed the Stream CLA (required for external contributors)

Summary by CodeRabbit

  • New Features

    • Added read, seen, and watched status properties to aggregated notification activities, enabling more accurate status tracking and display.
  • Bug Fixes

    • Enhanced read-status determination in the notifications interface to rely on activity data properties instead of recalculated values.
  • Tests

    • Added comprehensive test coverage for notification feed status updates, including read and seen state handling.

@gpunto gpunto added the pr:improvement Improvement label Apr 20, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 20, 2026

PR checklist ✅

All required conditions are satisfied:

  • Title length is OK (or ignored by label).
  • At least one pr: label exists.
  • Sections ### Goal, ### Implementation, and ### Testing are filled.

🎉 Great job! This PR is ready for review.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 20, 2026

SDK Size Comparison 📏

SDK Before After Difference Status
stream-feeds-android-client 2.51 MB 2.51 MB 0.00 MB 🟢

@gpunto gpunto force-pushed the handle-read-seen-flags branch from b7dfcbb to fcec447 Compare April 20, 2026 14:20
@sonarqubecloud
Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
73.1% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@gpunto gpunto marked this pull request as ready for review April 20, 2026 14:31
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 20, 2026

Walkthrough

The changes extend AggregatedActivityData with three optional boolean fields (isRead, isSeen, isWatched) to track notification and story status. Network responses are mapped to populate these fields, state management logic derives and updates read/seen status based on notification status, and the sample app consumes the isRead property directly.

Changes

Cohort / File(s) Summary
Model Extensions
stream-feeds-android-client/src/main/kotlin/io/getstream/feeds/android/client/api/model/AggregatedActivityData.kt, stream-feeds-android-client/src/main/kotlin/io/getstream/feeds/android/client/internal/model/AggregatedActivityOperations.kt
Added isRead, isSeen, and isWatched nullable boolean properties to AggregatedActivityData. Updated mapping logic in AggregatedActivityOperations.toModel() to pass these fields from network responses.
State Management
stream-feeds-android-client/src/main/kotlin/io/getstream/feeds/android/client/internal/state/FeedStateImpl.kt
Added notificationStatus handling in onNotificationFeedUpdated() that derives and updates read/seen state for both activities and aggregatedActivities using timestamps and activity id lists. Introduced private updateReadSeenStatus() logic and helper extension String.isMarked().
Test Fixtures & Tests
stream-feeds-android-client/src/test/kotlin/io/getstream/feeds/android/client/internal/test/TestData.kt, stream-feeds-android-client/src/test/kotlin/io/getstream/feeds/android/client/internal/state/FeedStateImplTest.kt, stream-feeds-android-client/src/test/kotlin/io/getstream/feeds/android/client/internal/state/event/handler/FeedEventHandlerTest.kt
Extended test data builders activityData() and aggregatedActivityData() with isRead, isSeen, isWatched parameters. Added comprehensive tests verifying read/seen status updates in FeedStateImpl.onNotificationFeedUpdated() for both activity types.
Sample App
stream-feeds-android-sample/src/main/java/io/getstream/feeds/android/sample/notification/NotificationsScreen.kt, stream-feeds-android-sample/src/main/java/io/getstream/feeds/android/sample/notification/NotificationsViewModel.kt
Updated read-state derivation to use AggregatedActivityData.isRead directly instead of consulting notificationStatus.readActivities.

Sequence Diagram(s)

sequenceDiagram
    participant Network as Network Response
    participant Ops as AggregatedActivityOperations
    participant Model as AggregatedActivityData
    participant State as FeedStateImpl
    participant UI as UI Layer

    Network->>Ops: Response with isRead,<br/>isSeen, isWatched
    Ops->>Model: Map fields to<br/>AggregatedActivityData
    Model->>State: toModel() returns<br/>populated model
    State->>State: onNotificationFeedUpdated()<br/>with notificationStatus
    State->>State: updateReadSeenStatus()<br/>derives read/seen<br/>from timestamps & ids
    State->>Model: Update isRead/isSeen<br/>in aggregatedActivities
    State->>UI: Emit updated<br/>aggregatedActivities
    UI->>UI: Render using<br/>AggregatedActivityData.isRead
Loading

Estimated Code Review Effort

🎯 3 (Moderate) | ⏱️ ~35 minutes

Poem

🐰 Three flags flutter in the feed stream—
isRead, isSeen, isWatched—a notification dream!
State blooms with logic to mark what's been found,
While our UI happily skips the old ground. ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: computing isRead and isSeen flags based on notification status, which aligns with the changeset's primary objective.
Description check ✅ Passed The description covers all required sections (Goal, Implementation, Testing) with clear explanations and references. It is comprehensive and well-structured against the template.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch handle-read-seen-flags

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
stream-feeds-android-client/src/main/kotlin/io/getstream/feeds/android/client/internal/state/FeedStateImpl.kt (1)

368-375: ⚠️ Potential issue | 🟠 Major

Compute read/seen after merging incoming groups.

Line 372 updates the current aggregated groups, but Line 373 can immediately replace those groups with the incoming aggregatedActivities payload and add new groups that never pass through updateReadSeenStatus. This can leave updated/new notification groups with stale or null isRead/isSeen despite a non-null notificationStatus.

🐛 Proposed fix
 override fun onNotificationFeedUpdated(
     aggregatedActivities: List<AggregatedActivityData>,
     notificationStatus: NotificationStatusResponse?,
 ) {
-    notificationStatus?.let(::updateReadSeenStatus)
     updateAggregatedActivities(aggregatedActivities, prependNew = true)
+    notificationStatus?.let(::updateReadSeenStatus)
     _notificationStatus.update { notificationStatus }
 }

The existing FeedStateImplTest expectation around incoming updated/new groups should also be adjusted to expect computed flags when notificationStatus is present.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@stream-feeds-android-client/src/main/kotlin/io/getstream/feeds/android/client/internal/state/FeedStateImpl.kt`
around lines 368 - 375, onNotificationFeedUpdated currently calls
updateReadSeenStatus before updateAggregatedActivities which means newly merged
or incoming groups never get their isRead/isSeen flags computed; swap the
operations so you first call updateAggregatedActivities(aggregatedActivities,
prependNew = true), then, if notificationStatus != null, call
updateReadSeenStatus(notificationStatus) to compute flags on the merged groups,
and finally update _notificationStatus via _notificationStatus.update {
notificationStatus }; also update the FeedStateImplTest expectations to assert
computed read/seen flags for updated/new groups when notificationStatus is
present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In
`@stream-feeds-android-client/src/main/kotlin/io/getstream/feeds/android/client/internal/state/FeedStateImpl.kt`:
- Around line 368-375: onNotificationFeedUpdated currently calls
updateReadSeenStatus before updateAggregatedActivities which means newly merged
or incoming groups never get their isRead/isSeen flags computed; swap the
operations so you first call updateAggregatedActivities(aggregatedActivities,
prependNew = true), then, if notificationStatus != null, call
updateReadSeenStatus(notificationStatus) to compute flags on the merged groups,
and finally update _notificationStatus via _notificationStatus.update {
notificationStatus }; also update the FeedStateImplTest expectations to assert
computed read/seen flags for updated/new groups when notificationStatus is
present.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 70700193-0528-492f-aeca-bac2a4eed037

📥 Commits

Reviewing files that changed from the base of the PR and between 30f114d and fcec447.

📒 Files selected for processing (8)
  • stream-feeds-android-client/src/main/kotlin/io/getstream/feeds/android/client/api/model/AggregatedActivityData.kt
  • stream-feeds-android-client/src/main/kotlin/io/getstream/feeds/android/client/internal/model/AggregatedActivityOperations.kt
  • stream-feeds-android-client/src/main/kotlin/io/getstream/feeds/android/client/internal/state/FeedStateImpl.kt
  • stream-feeds-android-client/src/test/kotlin/io/getstream/feeds/android/client/internal/state/FeedStateImplTest.kt
  • stream-feeds-android-client/src/test/kotlin/io/getstream/feeds/android/client/internal/state/event/handler/FeedEventHandlerTest.kt
  • stream-feeds-android-client/src/test/kotlin/io/getstream/feeds/android/client/internal/test/TestData.kt
  • stream-feeds-android-sample/src/main/java/io/getstream/feeds/android/sample/notification/NotificationsScreen.kt
  • stream-feeds-android-sample/src/main/java/io/getstream/feeds/android/sample/notification/NotificationsViewModel.kt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr:improvement Improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant