feat(soup): impl notification filters for foreign entities, github pr's in signal#4059
Conversation
synoet
commented
Jun 15, 2026
synoet
commented
- feat(soup): add notification filters to foreign entity (GitHub PR) queries
- feat(soup): surface GitHub PR notifications in the inbox Signal tab
…eries Foreign entities (GitHub PRs) fetched from soup can now be filtered by the requesting user's notification state, matching the done/seen filters already available for documents, chats, emails, projects, and channels. - Add `notification_filters` to `ForeignEntityFilters` and expand it into `nd`/`ns` AST literals. - In the foreign entity repo, hoist the notification literals (alongside `includes_me`) out of the metadata jsonpath and apply them as `EXISTS` predicates against `notification`/`user_notification`, scoped to the requesting user and the `foreign_entity` event type. A notification filter with no requesting user matches nothing. - Regenerate the storage/search OpenAPI + TypeScript clients and the sqlx cache. https://claude.ai/code/session_01V3PQwKGcmnjAdhQDhkG1De
Foreign entities (GitHub PRs) now appear in the inbox Signal tab, gated on the user's not-done notifications like the other notification-backed types. - Add `foreignEntityDone`/`foreignEntitySeen` filter-store fields mapping to the `fef` target (`nd`/`ns`), backed by the notification filters added to `ForeignEntityFilters`. - Reference `foreignEntityDone: false` in the inbox signal query so foreign entities are fetched (and opted past defineQueryFilters' exclusion of unreferenced entity types) and gated on not-done notifications. - Classify foreign entities as signal client-side (`signalFilter`). Rendering stays gated on the existing supported-foreign-entities flag. https://claude.ai/code/session_01V3PQwKGcmnjAdhQDhkG1De
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds notification-state filtering (done/seen) for foreign entities across the full stack. On the Rust side, 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
js/app/packages/app/component/next-soup/filters/inbox-filters.ts (1)
105-135: Usematchfromts-patterninstead of a native exhaustiveswitchinsignalFilter.The switch statement covers all cases of
entity.type(channel, chat, document, email, project, channel_message, call, crm_company, crm_contact, automation, foreign). Per project convention, this exhaustive branching should be expressed withmatch(...).exhaustive()for consistency and compile-time exhaustiveness guarantees.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@js/app/packages/app/component/next-soup/filters/inbox-filters.ts` around lines 105 - 135, Replace the exhaustive switch statement in the signalFilter function with a match expression from the ts-pattern library. Convert each case into a pattern entry using the match function, maintaining the same return values for each entity type (channel, chat, document, email, project, channel_message, call, crm_company, crm_contact, automation, and foreign), and append .exhaustive() to the match chain to ensure compile-time exhaustiveness checking per project convention.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/cloud-storage/foreign_entity/src/outbound/pg_foreign_entity_repo.rs`:
- Around line 61-70: The and method incorrectly merges conflicting
notification_done and notification_seen predicates by using Option::or, which
drops one side when both are Some. Instead of using or, check if both
notification_done values are Some and have conflicting boolean values (one true,
one false), and similarly for notification_seen; when conflicts are detected,
the filter should be marked as unsatisfiable. After fixing the and method to
properly detect unsatisfiable conditions, update the
get_foreign_entities_for_user method to return Ok(Vec::new()) when the resulting
filter is unsatisfiable.
---
Nitpick comments:
In `@js/app/packages/app/component/next-soup/filters/inbox-filters.ts`:
- Around line 105-135: Replace the exhaustive switch statement in the
signalFilter function with a match expression from the ts-pattern library.
Convert each case into a pattern entry using the match function, maintaining the
same return values for each entity type (channel, chat, document, email,
project, channel_message, call, crm_company, crm_contact, automation, and
foreign), and append .exhaustive() to the match chain to ensure compile-time
exhaustiveness checking per project convention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f4271965-c238-4f89-b803-81202c42f2d5
⛔ Files ignored due to path filters (4)
js/app/packages/service-clients/service-search/generated/models/foreignEntityFilters.tsis excluded by!**/generated/**js/app/packages/service-clients/service-storage/generated/schemas/foreignEntityFilters.tsis excluded by!**/generated/**js/app/packages/service-clients/service-storage/generated/zod.tsis excluded by!**/generated/**rust/cloud-storage/.sqlx/query-d45423d79a30cd22b475c082ccb2f2d433780b1d32441dcddc6350501de54260.jsonis excluded by!**/.sqlx/**
📒 Files selected for processing (13)
js/app/packages/app/component/app-sidebar/soup-filter-presets.tsjs/app/packages/app/component/next-soup/filters/configs/general.tsjs/app/packages/app/component/next-soup/filters/filter-store/compile.tsjs/app/packages/app/component/next-soup/filters/filter-store/types.tsjs/app/packages/app/component/next-soup/filters/inbox-filters.tsjs/app/packages/service-clients/service-search/openapi.jsonjs/app/packages/service-clients/service-storage/openapi.jsonrust/cloud-storage/foreign_entity/src/outbound/pg_foreign_entity_repo.rsrust/cloud-storage/foreign_entity/src/outbound/pg_foreign_entity_repo/tests.rsrust/cloud-storage/item_filters/src/ast/foreign_entity.rsrust/cloud-storage/item_filters/src/ast/tests.rsrust/cloud-storage/item_filters/src/lib.rsrust/cloud-storage/soup/src/domain/service/tests.rs
… unsatisfiable Combining hoisted notification predicates with `Option::or` silently dropped one side, so a contradictory AST like `NotificationDone(true) AND NotificationDone(false)` collapsed to a single-sided `done = true` predicate instead of matching nothing. Detect conflicting done/seen predicates while folding the AND spine and short-circuit to an empty result. Only reachable via a hand-crafted AST (the typed ForeignEntityFilters emits at most one done/seen literal each), but the silent collapse was misleading. https://claude.ai/code/session_01V3PQwKGcmnjAdhQDhkG1De
3 participants
