Alerts & subscriptions#42
Conversation
|
Warning Review limit reached
Next review available in: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (36)
📝 WalkthroughWalkthroughThe CLI adds alert and dashboard subscription commands backed by new notification and pulse schemas, API merge logic, command registration, end-to-end coverage, documentation, specialized skills, and a package version bump. ChangesNotification management
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Command
participant MetabaseAPI
participant Renderer
CLI->>Command: Execute alert or subscription command
Command->>MetabaseAPI: Request resource operation
MetabaseAPI-->>Command: Return validated resource
Command->>Renderer: Render result or summary
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
alxnddr
force-pushed
the
alerts-subscriptions
branch
from
8e124bf to
f6f5541
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
src/commands/alert/list.ts (2)
1-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
NotificationApiListschema across two command files. Both files define the identicalz.array(Notification)schema locally to parse the same/api/notificationlist response; centralize it once.
src/commands/alert/list.ts#L1-L15: export a shared array-parse schema (e.g.NotificationList = z.array(Notification)) fromsrc/domain/notification.tsand import it here instead of the localNotificationApiList.src/commands/card/alerts.ts#L1-L11: import the same shared schema instead of redeclaringNotificationApiListlocally.🤖 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 `@src/commands/alert/list.ts` around lines 1 - 15, Centralize the shared notification-list schema by exporting a single z.array(Notification) schema from src/domain/notification.ts. In src/commands/alert/list.ts and src/commands/card/alerts.ts, import that shared schema and remove each local NotificationApiList declaration, updating references to use the shared symbol.
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
NotificationApiListduplicated withsrc/commands/card/alerts.ts.This exact schema (
z.array(Notification)) is redefined identically insrc/commands/card/alerts.ts. See consolidated comment.Also applies to: 15-15
🤖 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 `@src/commands/alert/list.ts` at line 1, Remove the duplicate NotificationApiList schema definition from the alert list command and reuse the existing shared definition from the card alerts module. Update imports and references as needed so both commands use the single z.array(Notification) schema.src/commands/card/alerts.ts (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
NotificationApiListduplicated withsrc/commands/alert/list.ts.Same schema redefined verbatim in both files. See consolidated comment.
Also applies to: 11-11
🤖 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 `@src/commands/card/alerts.ts` at line 1, Remove the duplicated NotificationApiList schema from the alerts command and reuse the existing shared definition from the alert list implementation. Update imports and references as needed so both commands use the single canonical schema.src/commands/subscription/list.ts (1)
10-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
PulseApiListschema across two command files. Both files declare an identical, unexportedz.array(Pulse)validator for the raw/api/pulselist response; centralize it once to avoid drift.
src/commands/subscription/list.ts#L10: movePulseApiListintosrc/domain/pulse.ts(export it alongsidePulse/PulseCompact) and import it here instead of re-declaring locally.src/commands/dashboard/subscriptions.ts#L11: drop the localPulseApiListdeclaration and import the same exported schema fromsrc/domain/pulse.ts.🤖 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 `@src/commands/subscription/list.ts` at line 10, Centralize the duplicated PulseApiList schema by exporting the z.array(Pulse) validator from src/domain/pulse.ts alongside Pulse and PulseCompact. In src/commands/subscription/list.ts:10 and src/commands/dashboard/subscriptions.ts:11, remove the local declarations and import the shared PulseApiList export.src/domain/pulse.ts (1)
187-199: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueNo conditional validation tying
schedule_hour/schedule_day/schedule_frametoschedule_type.
PulseChannelInputleaves all schedule fields optional regardless ofschedule_type, so e.g. a"monthly"channel can be submitted with onlyschedule_frameand noschedule_hour/schedule_day. That still fails fast in the sense of hitting the server, but the CLI loses the chance to give a precise, local error message the way it already does for invalidchannel_type. Consider a.superRefine()(or discriminated union keyed onschedule_type) enforcing the per-type required fields documented increate.ts'sdetailsstring.🤖 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 `@src/domain/pulse.ts` around lines 187 - 199, Add conditional validation to PulseChannelInput based on schedule_type, using superRefine or a discriminated union. Require the schedule_hour, schedule_day, and schedule_frame fields appropriate to each schedule type according to the documented rules in create.ts, and attach precise validation errors to the missing fields while preserving the existing field constraints.src/commands/alert/patch.ts (1)
43-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a more specific type instead of
object.Using
objectbypasses type checking for the incoming patch payload. Sincepatch.payloadoriginates fromNotificationUpdateInput, consider utilizing its derived type to ensure better type safety during the spread operation.♻️ Proposed refactor
-function mergePayload(current: Notification, patch: object): object { +function mergePayload( + current: Notification, + patch: NonNullable<NotificationUpdateInput["payload"]> +): object {🤖 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 `@src/commands/alert/patch.ts` at line 43, Update mergePayload to replace the generic object type for patch with the derived payload type from NotificationUpdateInput, preserving type safety when spreading patch.payload and merging it with current notification data.
🤖 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 `@README.md`:
- Around line 1099-1255: Update the flag tables for mb subscription list and mb
alert list to match the CLI examples: use --dashboard-id, --card-id,
--creator-id, and --recipient-id instead of the shortened names. Leave the
surrounding descriptions and examples unchanged.
In `@src/commands/alert/get.ts`:
- Around line 9-21: The get command advertises an unsupported --full option.
Update the command definition around defineMetabaseCommand and its run
implementation to either define and implement --full so it returns the hydrated
alert response, or remove all --full references from details and examples; keep
help text and runtime behavior consistent.
In `@src/commands/alert/send.ts`:
- Around line 11-45: Make the alert send command a CLI-only wrapper by moving
AlertSendResult and alertSendResultView into a dedicated result helper such as
send-result.ts, then reuse those exports from send.ts. Extract the
client.requestRaw call into an API helper such as sendAlert in patch.ts or a new
alert API module, and have run call that helper instead of performing HTTP
directly.
---
Nitpick comments:
In `@src/commands/alert/list.ts`:
- Around line 1-15: Centralize the shared notification-list schema by exporting
a single z.array(Notification) schema from src/domain/notification.ts. In
src/commands/alert/list.ts and src/commands/card/alerts.ts, import that shared
schema and remove each local NotificationApiList declaration, updating
references to use the shared symbol.
- Line 1: Remove the duplicate NotificationApiList schema definition from the
alert list command and reuse the existing shared definition from the card alerts
module. Update imports and references as needed so both commands use the single
z.array(Notification) schema.
In `@src/commands/alert/patch.ts`:
- Line 43: Update mergePayload to replace the generic object type for patch with
the derived payload type from NotificationUpdateInput, preserving type safety
when spreading patch.payload and merging it with current notification data.
In `@src/commands/card/alerts.ts`:
- Line 1: Remove the duplicated NotificationApiList schema from the alerts
command and reuse the existing shared definition from the alert list
implementation. Update imports and references as needed so both commands use the
single canonical schema.
In `@src/commands/subscription/list.ts`:
- Line 10: Centralize the duplicated PulseApiList schema by exporting the
z.array(Pulse) validator from src/domain/pulse.ts alongside Pulse and
PulseCompact. In src/commands/subscription/list.ts:10 and
src/commands/dashboard/subscriptions.ts:11, remove the local declarations and
import the shared PulseApiList export.
In `@src/domain/pulse.ts`:
- Around line 187-199: Add conditional validation to PulseChannelInput based on
schedule_type, using superRefine or a discriminated union. Require the
schedule_hour, schedule_day, and schedule_frame fields appropriate to each
schedule type according to the documented rules in create.ts, and attach precise
validation errors to the missing fields while preserving the existing field
constraints.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 22084c83-b398-4e4b-a898-3a4cf9067311
📒 Files selected for processing (35)
README.mdmissing-skills.mdpackage.jsonskill-data/core/SKILL.mdskill-data/notification/SKILL.mdsrc/commands/alert/archive.tssrc/commands/alert/create.tssrc/commands/alert/get.tssrc/commands/alert/index.tssrc/commands/alert/list.tssrc/commands/alert/patch.test.tssrc/commands/alert/patch.tssrc/commands/alert/send.tssrc/commands/alert/summary.tssrc/commands/alert/update.tssrc/commands/card/alerts.tssrc/commands/card/index.tssrc/commands/dashboard/index.tssrc/commands/dashboard/subscriptions.tssrc/commands/subscription/archive.tssrc/commands/subscription/create.tssrc/commands/subscription/get.tssrc/commands/subscription/index.tssrc/commands/subscription/list.tssrc/commands/subscription/patch.test.tssrc/commands/subscription/patch.tssrc/commands/subscription/update.tssrc/domain/cron.tssrc/domain/notification.tssrc/domain/pulse.tssrc/domain/transform-job.tssrc/main.tssrc/runtime/command-help.test.tstests/e2e/alert.e2e.test.tstests/e2e/subscription.e2e.test.ts
💤 Files with no reviewable changes (1)
- missing-skills.md
| export default defineMetabaseCommand({ | ||
| meta: { name: "get", description: "Get a question alert by id" }, | ||
| details: | ||
| "`--full` includes the hydrated card the alert watches, alongside its schedules and handlers.", | ||
| capabilities: { minVersion: 58 }, | ||
| args: { | ||
| ...outputFlags, | ||
| ...profileFlag, | ||
| ...connectionFlags, | ||
| id: { type: "positional", description: "Alert id", required: true }, | ||
| }, | ||
| outputSchema: Notification, | ||
| examples: ["mb alert get 9", "mb alert get 9 --full --json"], |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm no "full" flag/hydration exists anywhere in the alert command chain
rg -n '\bfull\b' src/commands/alert src/commands/card --type=tsRepository: metabase/metabase-cli
Length of output: 381
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== get.ts ==\n'
cat -n src/commands/alert/get.ts
printf '\n== alert directory refs to full ==\n'
rg -n '\bfull\b' src/commands/alert src/domain src/core --type=ts || true
printf '\n== defineMetabaseCommand ==\n'
fd -a 'defineMetabaseCommand' src || true
rg -n 'function defineMetabaseCommand|const defineMetabaseCommand|export .*defineMetabaseCommand' src --type=ts || true
printf '\n== notification schema ==\n'
cat -n src/domain/notification.ts
printf '\n== fetchAlert / related alert commands ==\n'
rg -n 'fetchAlert\(|NotificationCompact|NotificationCardPayload|hydrate|card_id' src/commands/alert src/domain --type=ts || trueRepository: metabase/metabase-cli
Length of output: 29172
--full is advertised but not implemented.
details and examples mention --full, but args never defines it and run always returns the compact alert payload. Either wire up a real full response or remove the flag from the help text/example.
🤖 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 `@src/commands/alert/get.ts` around lines 9 - 21, The get command advertises an
unsupported --full option. Update the command definition around
defineMetabaseCommand and its run implementation to either define and implement
--full so it returns the hydrated alert response, or remove all --full
references from details and examples; keep help text and runtime behavior
consistent.
| export const AlertSendResult = z.object({ | ||
| id: z.number().int(), | ||
| sent: z.boolean(), | ||
| }); | ||
| export type AlertSendResultJson = z.infer<typeof AlertSendResult>; | ||
|
|
||
| const alertSendResultView: ResourceView<AlertSendResultJson> = { | ||
| compactPick: AlertSendResult, | ||
| tableColumns: [ | ||
| { key: "id", label: "ID" }, | ||
| { key: "sent", label: "Sent" }, | ||
| ], | ||
| }; | ||
|
|
||
| export default defineMetabaseCommand({ | ||
| meta: { name: "send", description: "Send a question alert now, off-schedule" }, | ||
| details: | ||
| "Delivers to every handler on the alert, ignoring its send condition and schedule. Requires the channel to be configured on the server (email needs SMTP set up).", | ||
| capabilities: { minVersion: 58 }, | ||
| args: { | ||
| ...outputFlags, | ||
| ...profileFlag, | ||
| ...connectionFlags, | ||
| id: { type: "positional", description: "Alert id", required: true }, | ||
| }, | ||
| outputSchema: AlertSendResult, | ||
| examples: ["mb alert send 9", "mb alert send 9 --json"], | ||
| async run({ args, ctx, getClient }) { | ||
| const id = parseId(args.id); | ||
| const client = await getClient(); | ||
| await fetchAlert(client, id); | ||
| await client.requestRaw(`/api/notification/${id}/send`, { | ||
| method: "POST", | ||
| expectContentType: "binary", | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Extract HTTP, parsing, and formatting logic from the command file.
The send.ts command file contains a Zod schema (AlertSendResult), view formatting (alertSendResultView), and direct HTTP logic (client.requestRaw). As per coding guidelines, command files must be strictly CLI shell only: no HTTP, no parsing, and no formatting logic.
Please extract the schema and view to a helper file (e.g., send-result.ts) and move the HTTP call to an API helper function (e.g., sendAlert inside patch.ts or a new API file).
🤖 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 `@src/commands/alert/send.ts` around lines 11 - 45, Make the alert send command
a CLI-only wrapper by moving AlertSendResult and alertSendResultView into a
dedicated result helper such as send-result.ts, then reuse those exports from
send.ts. Extract the client.requestRaw call into an API helper such as sendAlert
in patch.ts or a new alert API module, and have run call that helper instead of
performing HTTP directly.
Source: Coding guidelines
alxnddr
force-pushed
the
alerts-subscriptions
branch
from
f6f5541 to
44f5a85
Compare
alxnddr
force-pushed
the
alerts-subscriptions
branch
from
44f5a85 to
5e96027
Compare
2 participants
Description
We're building alerts and subscriptions in MCPv2 but they were missing in the CLI too so this PR adds new commands and skills to use alerts & subscriptions.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Chores