Skip to content

fix(notifications): polish modal styling, credential display, and trigger filters#3571

Merged
waleedlatif1 merged 2 commits intofeat/mothership-copilotfrom
fix/notification-modal-polish
Mar 14, 2026
Merged

fix(notifications): polish modal styling, credential display, and trigger filters#3571
waleedlatif1 merged 2 commits intofeat/mothership-copilotfrom
fix/notification-modal-polish

Conversation

@waleedlatif1
Copy link
Collaborator

@waleedlatif1 waleedlatif1 commented Mar 14, 2026

Summary

  • Show credential display name (instead of raw account ID) in Slack account selector by joining the credential table
  • Fix label styling to use default Label component (text-primary) matching all other modals in the app
  • Fix modal body spacing with top padding after tab bar
  • Replace list-card skeleton with form-field skeleton matching actual layout
  • Replace custom "Select a Slack account first" box with disabled Combobox (matching the dependsOn pattern used in subblocks)
  • Use Label component in WorkflowSelector with consistent gap-[8px] spacing
  • Add overflow badge pattern (slice + +N) to level and trigger filter badge overlays
  • Use dynamic trigger options from getTriggerOptions() instead of hardcoded CORE_TRIGGER_TYPES, matching the logs toolbar
  • Relax API Zod validation to accept integration trigger types (z.string() instead of z.enum)
  • Deduplicate account rows from credential leftJoin in accounts API
  • Extract getTriggerOptions() to module-level constants to avoid per-render calls

Test plan

  • Open notification modal → verify label text is primary color, proper spacing after tabs
  • Switch between Webhook/Email/Slack tabs → verify skeleton shows form-field shapes during loading
  • On Slack tab without account selected → channel selector shows disabled Combobox with "Select an account first..." placeholder
  • On Slack tab with account selected → credential display name shown (not raw ID)
  • Select many trigger types → verify overflow badge shows +N after 6 visible badges
  • Create a notification with integration trigger types → verify API accepts them
  • Verify notifications still fire correctly for matching trigger types

…gger filters

- Show credential display name instead of raw account ID in Slack account selector
- Fix label styling to use default Label component (text-primary) for consistency
- Fix modal body spacing with proper top padding after tab bar
- Replace list-card skeleton with form-field skeleton matching actual layout
- Replace custom "Select a Slack account first" box with disabled Combobox (dependsOn pattern)
- Use proper Label component in WorkflowSelector with consistent gap spacing
- Add overflow badge pattern (slice + +N) to level and trigger filter badges
- Use dynamic trigger options from getTriggerOptions() instead of hardcoded CORE_TRIGGER_TYPES
- Relax API validation to accept integration trigger types (z.string instead of z.enum)
- Deduplicate account rows from credential leftJoin in accounts API
- Extract getTriggerOptions() to module-level constants to avoid per-render calls

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@cursor
Copy link

cursor bot commented Mar 14, 2026

PR Summary

Medium Risk
Moderate risk because it changes notification filtering semantics (empty triggerFilter now means match all) and relaxes API validation for triggers; could affect which notifications fire if misconfigured.

Overview
Improves the notifications modal UX (consistent Label styling/spacing, updated loading skeletons, overflow badges for selected trigger chips, and a disabled Slack channel combobox until an account is selected).

Updates trigger filtering to use dynamic options from getTriggerOptions() and broadens backend validation to accept arbitrary non-empty trigger strings; on the delivery side, an empty triggerFilter is now treated as no trigger filtering (match all).

Enhances OAuth account display by joining credential in the accounts API to prefer credential.displayName, and deduplicates results from the join to avoid duplicate account rows.

Written by Cursor Bugbot for commit 2d4f25e. Configure here.

@vercel
Copy link

vercel bot commented Mar 14, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Mar 14, 2026 1:06am

Request Review

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 14, 2026

Greptile Summary

This PR polishes the notifications modal with a set of focused UI and API improvements: it standardises label colours by removing text-[var(--text-secondary)] overrides, fixes modal body spacing, replaces a custom "no account" placeholder with a disabled Combobox, surfaces the credential display name in the Slack account selector by joining the credential table, extends the trigger filter to include integration trigger types via getTriggerOptions(), adds an overflow +N badge for long trigger selections, and relaxes the API Zod schema to accept arbitrary trigger strings while handling an empty triggerFilter as "match all" in the event emitter.

Key changes:

  • accounts/route.ts: Adds a leftJoin to the credential table to surface displayName and deduplicates results — the join lacks a workspaceId filter, which can produce a non-deterministic display name when the same OAuth account is connected to multiple workspaces.
  • notifications/route.ts + [notificationId]/route.ts: triggerFilterSchema changed from z.enum(CORE_TRIGGER_TYPES) to z.string().min(1), and the default changed from [...CORE_TRIGGER_TYPES] to [] (which events.ts correctly treats as "match all").
  • events.ts: Guards triggerMatches with subscription.triggerFilter.length === 0 to preserve "match all" semantics for the new empty default.
  • notifications.tsx: Replaces hardcoded CORE_TRIGGER_TYPES with getTriggerOptions() extracted to a module-level constant, adds +N overflow badge for trigger filter, and switches skeleton to form-field shapes.

Confidence Score: 4/5

  • Safe to merge with one minor logic concern in the accounts API that may surface an incorrect credential display name for users connected to the same OAuth provider across multiple workspaces.
  • The changes are well-scoped UI polish backed by a thorough test plan. The API schema relaxation and empty-triggerFilter semantics are handled consistently across both routes and the event emitter. The one genuine issue is in accounts/route.ts where the credential leftJoin has no workspaceId filter, making the display name non-deterministic for multi-workspace OAuth accounts — a low-severity cosmetic bug. All other changes are clean.
  • apps/sim/app/api/auth/accounts/route.ts — leftJoin credential without workspaceId filter can yield wrong display name for multi-workspace accounts.

Important Files Changed

Filename Overview
apps/sim/app/api/auth/accounts/route.ts Adds credentialDisplayName via leftJoin to show friendly names in the Slack account selector and deduplicates via Map. The leftJoin lacks a workspaceId filter, so an account used across multiple workspaces can match multiple credential rows and the deduplication non-deterministically picks the first, potentially returning the wrong workspace's display name.
apps/sim/app/api/workspaces/[id]/notifications/route.ts Relaxes triggerFilter validation from a strict enum to z.string().min(1) to support integration trigger types, and changes the default from CORE_TRIGGER_TYPES to an empty array. The empty-array default is handled correctly in events.ts (empty = match all), making new subscriptions match all triggers by default.
apps/sim/lib/logs/events.ts Adds an empty-array guard to triggerMatches so that triggerFilter: [] means "match all triggers", correctly pairing with the new empty-array default on POST. Safe change.
apps/sim/app/workspace/[workspaceId]/logs/components/logs-toolbar/components/notifications/notifications.tsx Large polish PR: removes all text-[var(--text-secondary)] overrides from Label components, replaces CORE_TRIGGER_TYPES with TRIGGER_OPTIONS from getTriggerOptions(), adds +N overflow badge to trigger filter, replaces the list-card skeleton with form-field shapes, and updates the ModalBody to use pt-4 spacing.

Sequence Diagram

sequenceDiagram
    participant UI as NotificationSettings UI
    participant AccountsAPI as GET /api/auth/accounts
    participant CredDB as credential table
    participant NotifAPI as POST/PUT /notifications
    participant EventsLib as lib/logs/events.ts

    UI->>AccountsAPI: GET ?provider=slack
    AccountsAPI->>CredDB: SELECT account LEFT JOIN credential ON credential.accountId = account.id
    CredDB-->>AccountsAPI: rows (may include multiple credential rows per account across workspaces)
    AccountsAPI-->>AccountsAPI: Deduplicate via Map (keeps first credential seen)
    AccountsAPI-->>UI: { accounts: [{ displayName: credentialDisplayName || accountId || providerId }] }

    UI->>NotifAPI: POST { triggerFilter: string[] (any value, min(1)) }
    NotifAPI-->>NotifAPI: Validate with z.string().min(1) (default: [])
    NotifAPI-->>UI: 201 Created subscription

    Note over EventsLib: On workflow execution
    EventsLib->>EventsLib: triggerMatches = triggerFilter.length === 0 OR triggerFilter.includes(log.trigger)
    EventsLib-->>EventsLib: Empty triggerFilter → matches ALL trigger types
Loading

Last reviewed commit: 2d4f25e

- Restore accountId in displayName fallback chain (credentialDisplayName || accountId || providerId)
- Add .default([]) to triggerFilter in create schema to preserve backward compatibility
- Treat empty triggerFilter as "match all" in notification matching logic
- Remove unreachable overflow badge for levelFilter (only 2 possible values)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@waleedlatif1
Copy link
Collaborator Author

@greptile

@waleedlatif1
Copy link
Collaborator Author

@cursor review

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

@waleedlatif1 waleedlatif1 merged commit 54e14f4 into feat/mothership-copilot Mar 14, 2026
4 checks passed
@waleedlatif1 waleedlatif1 deleted the fix/notification-modal-polish branch March 14, 2026 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant