Skip to content

feat(soup): notification style#3577

Closed
whutchinson98 wants to merge 3 commits into
mainfrom
hutch/feat-soup-notification
Closed

feat(soup): notification style#3577
whutchinson98 wants to merge 3 commits into
mainfrom
hutch/feat-soup-notification

Conversation

@whutchinson98

Copy link
Copy Markdown
Member

No description provided.

@coderabbitai

coderabbitai Bot commented May 27, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 1dbe0187-fc78-434c-ad4b-92b37d3ff25a

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This pull request adds end-to-end support for notifications in the Soup unified feed system. It introduces a SoupNotification model with enrichable source payloads, extends SoupItem to include notification variants, and integrates notification fetching and enrichment into the service pipeline. The repository layer queries PostgreSQL for user notifications with cursor pagination, and the API layer exposes notifications through existing toolsets with proper serialization of enriched sources across documents, chats, projects, email threads, channels, and calls. Supporting model types are updated to derive Clone for compatibility with the notification infrastructure.

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Description check ❓ Inconclusive No description was provided by the author, making it impossible to assess whether the description relates to the changeset. Add a pull request description explaining the purpose, scope, and implementation details of the notification feature being added to the soup service.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title follows conventional commits format (feat:) and is under 72 characters (30 chars), clearly indicating a new feature for soup notifications.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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


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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 305ccf47-973b-4b80-bbce-eed0c2805d85

📥 Commits

Reviewing files that changed from the base of the PR and between 9e595cf and b181382.

⛔ Files ignored due to path filters (1)
  • rust/cloud-storage/Cargo.lock is excluded by !**/*.lock, !**/Cargo.lock
📒 Files selected for processing (20)
  • rust/cloud-storage/document_storage_service/src/api/swagger.rs
  • rust/cloud-storage/models_soup/Cargo.toml
  • rust/cloud-storage/models_soup/src/call_record.rs
  • rust/cloud-storage/models_soup/src/comms.rs
  • rust/cloud-storage/models_soup/src/email_thread.rs
  • rust/cloud-storage/models_soup/src/item.rs
  • rust/cloud-storage/models_soup/src/lib.rs
  • rust/cloud-storage/models_soup/src/notification.rs
  • rust/cloud-storage/soup/src/domain/models.rs
  • rust/cloud-storage/soup/src/domain/ports.rs
  • rust/cloud-storage/soup/src/domain/service.rs
  • rust/cloud-storage/soup/src/domain/service/tests.rs
  • rust/cloud-storage/soup/src/inbound/axum_router/tests.rs
  • rust/cloud-storage/soup/src/inbound/toolset/list_entities.rs
  • rust/cloud-storage/soup/src/inbound/toolset/mod.rs
  • rust/cloud-storage/soup/src/inbound/toolset/test.rs
  • rust/cloud-storage/soup/src/outbound/pg_soup_repo.rs
  • rust/cloud-storage/soup/src/outbound/pg_soup_repo/expanded/tests.rs
  • rust/cloud-storage/soup/src/outbound/pg_soup_repo/notification.rs
  • rust/cloud-storage/soup/src/outbound/pg_soup_repo/notification/tests.rs

Comment on lines +42 to +57
let rows = build_user_notifications_query(UserNotificationsQueryArgs {
user_id: req.user_id.as_ref(),
limit: req.limit as i64,
cursor_id: cursor_id.copied(),
cursor_timestamp: cursor_timestamp.copied(),
})
.build_query_as::<NotificationRow>()
.fetch_all(db)
.await?;

let mut notifications = Vec::with_capacity(rows.len());
for row in rows {
if let Some(notification) = row_to_notification(row) {
notifications.push(SoupItem::Notification(Box::new(notification)));
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Pagination can silently drop valid notifications due to post-LIMIT filtering.

Rows are limited in SQL first, then invalid rows are discarded in row_to_notification. If any row in that window is dropped, the page can return fewer than limit even when additional valid rows exist later, which can prematurely truncate pagination flows.

Suggested fix
 fn build_user_notifications_query<'a>(
     args: UserNotificationsQueryArgs<'a>,
 ) -> QueryBuilder<'a, Postgres> {
     let mut builder = QueryBuilder::new(
         r#"
@@
             FROM user_notification un
             JOIN notification n ON n.id = un.notification_id
             WHERE un.user_id = "#,
     );
     builder.push_bind(args.user_id);
-    builder.push(" AND un.deleted_at IS NULL AND un.done = false");
+    builder.push(" AND un.deleted_at IS NULL AND un.done = false");
+    builder.push(" AND n.event_item_type IN ('document', 'chat', 'project', 'emailThread', 'channel', 'call')");
@@
 }

Also applies to: 88-99, 102-156

@whutchinson98 whutchinson98 force-pushed the hutch/feat-soup-notification branch from b181382 to 2a3a61e Compare May 27, 2026 17:25
@github-actions

Copy link
Copy Markdown

@whutchinson98 whutchinson98 deleted the hutch/feat-soup-notification branch June 2, 2026 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant