feat: add Microsoft Teams notification provider#261
feat: add Microsoft Teams notification provider#261mvanhorn wants to merge 3 commits intoPortabase:mainfrom
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (3)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds Microsoft Teams as a notification provider: new Teams provider module, registration in provider handlers, type and DB enum updates, UI form/schema for Teams channel, and corresponding DB migration and snapshot files. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Notification System
participant Handler as sendTeams
participant API as Teams Webhook API
App->>Handler: sendTeams(config{teamsWebhook}, payload)
Handler->>Handler: Build Adaptive Card JSON\n(include level, title, message, optional data via safeStringify)
Handler->>API: POST webhook (application/json) with 10s timeout
alt 2xx response
API-->>Handler: 2xx + body
Handler-->>App: Return DispatchResult(success)
else non-2xx response
API-->>Handler: status + body
Handler->>Handler: Read body text
Handler-->>App: Throw Error(status + body)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/features/notifications/providers/teams.ts`:
- Around line 30-35: The current Teams notification payload builds a TextBlock
using JSON.stringify(payload.data, null, 2) which can throw for circular or
non-serializable objects; in the payload construction (the spread that begins
with ...(payload.data ? [{ type: 'TextBlock', text: `Data:
${JSON.stringify(payload.data, null, 2)}`, fontType: 'monospace' }]) ), wrap the
serialization in a safe routine: try to JSON.stringify(payload.data, null, 2)
inside a try/catch and on error fall back to a non-throwing representation (e.g.
a simple string like "[unserializable data]" or use util.inspect/safeStringify),
then use that string in the TextBlock so the notification is still sent even
when payload.data cannot be serialized.
- Around line 46-50: The fetch call that posts to Teams (the const res = await
fetch(webhookUrl, {...}) in teams.ts) needs an AbortController timeout so slow
webhooks don't hang the process; create an AbortController, start a timer (e.g.,
setTimeout) to call controller.abort() after a configured timeout (e.g., 5–10s
or existing notification timeout), pass controller.signal into the fetch
options, and clear the timer after fetch completes or throws; apply the same
AbortController+timeout pattern to the other providers (webhook.ts, discord.ts,
slack.ts, telegram.ts, ntfy.ts, gotify.ts) where fetch is used.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d361f553-ebbd-41a8-a71c-43e7550c741d
📒 Files selected for processing (3)
src/features/notifications/providers/index.tssrc/features/notifications/providers/teams.tssrc/features/notifications/types.ts
📜 Review details
🔇 Additional comments (3)
src/features/notifications/providers/index.ts (1)
4-17: Teams provider registration is correctly wired.Import and handler-map registration are consistent with
ProviderKindand the dispatcher flow.src/features/notifications/types.ts (1)
1-1:ProviderKindextension is correct.Adding
'teams'keeps provider typing aligned across dispatcher and provider implementations.src/features/notifications/providers/teams.ts (1)
57-62: SuccessDispatchResultshape looks consistent.Return fields align with provider conventions and
DispatchResultcontract.
|
Hi @mvanhorn, Thanks for your contribution. However, this PR is missing important features, mainly on the UI side. It also appears to have been opened just 12 minutes after the “First Good Issue” was created, which suggests it may have been fully generated without human review. If you still want this PR to be considered for review and validation, please improve the Microsoft Teams integration and ensure a proper manual review of the implementation before resubmitting. Please also add the required screenshots as specified in the issue; the PR cannot be validated without them. |
- Add Teams webhook URL validation schema - Add Teams configuration form component (matching existing Discord pattern) - Register Teams in channel form schema, provider registry, and form renderer - Add DB migration to extend provider_kind enum with 'microsoft-teams' - Address CodeRabbit review: safe JSON.stringify for circular refs, AbortController timeout on fetch - Remove preview flag to make Teams a fully available provider
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/features/notifications/providers/teams.ts`:
- Around line 41-50: The conditional that decides whether to include the Data
TextBlock incorrectly uses truthiness on payload.data (so values like 0, false,
and "" are dropped); update the check in the teams notification builder to test
for null/undefined instead (e.g., payload.data != null or payload.data !==
undefined) so the block is included when payload.data is any falsy-but-present
value before calling safeStringify(payload.data).
- Around line 3-11: The safeStringify function currently throws on bigint
values; update the JSON.stringify replacer inside safeStringify (the replacer
used in safeStringify) to detect typeof value === 'bigint' and return
value.toString(), keep the existing circular-detection logic with seen
(WeakSet), and add a catch-all fallback in the replacer (e.g., return
String(value) for any value that would otherwise cause serialization errors) so
JSON.stringify never throws on unsupported primitives; preserve the 2-space
formatting.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: aef5bb84-a449-4d89-8fa7-878d018f0c3a
📒 Files selected for processing (12)
src/components/wrappers/dashboard/admin/channels/channel/channel-form/channel-form.schema.tssrc/components/wrappers/dashboard/admin/channels/channel/channel-form/providers/notifications/forms/teams.form.tsxsrc/components/wrappers/dashboard/admin/channels/channel/channel-form/providers/notifications/forms/teams.schema.tssrc/components/wrappers/dashboard/admin/channels/helpers/common.tsxsrc/components/wrappers/dashboard/admin/channels/helpers/notification.tsxsrc/db/migrations/0049_mixed_blur.sqlsrc/db/migrations/meta/0049_snapshot.jsonsrc/db/migrations/meta/_journal.jsonsrc/db/schema/09_notification-channel.tssrc/features/notifications/providers/index.tssrc/features/notifications/providers/teams.tssrc/features/notifications/types.ts
📜 Review details
🔇 Additional comments (12)
src/db/migrations/meta/_journal.json (1)
347-354: Migration journal append looks correct.The new entry is sequential (
idx: 49) and consistent with existing journal metadata shape.src/db/migrations/0049_mixed_blur.sql (1)
1-1: Enum migration change is aligned with provider rollout.Adding
'microsoft-teams'toprovider_kindis the expected DB-side update for this feature.src/features/notifications/providers/index.ts (1)
4-4: Teams provider registration is correctly wired.
sendTeamsis imported and mapped to"microsoft-teams"inhandlers, enabling dispatch routing as intended.Also applies to: 17-17
src/components/wrappers/dashboard/admin/channels/helpers/common.tsx (1)
23-25: Teams form rendering path is implemented correctly.The Teams form component is imported and returned in
renderChannelFormfor"microsoft-teams".Also applies to: 93-94
src/components/wrappers/dashboard/admin/channels/channel/channel-form/providers/notifications/forms/teams.schema.ts (1)
3-5: Teams config schema is clean and sufficient for this scope.
teamsWebhookURL validation is present and matches expected form/provider integration.src/features/notifications/types.ts (1)
1-1: ProviderKind extension is consistent with the new provider integration.Including
'microsoft-teams'keeps dispatch typing aligned across feature layers.src/components/wrappers/dashboard/admin/channels/helpers/notification.tsx (1)
16-16: Provider registry update correctly enables Teams.Removing preview status and keeping the Teams entry in
notificationProvidersmatches the rollout intent.src/db/schema/09_notification-channel.ts (1)
10-10: DB schema enum update is correctly aligned.Adding
'microsoft-teams'toproviderKindEnumkeeps persistence types in sync with the new provider.src/components/wrappers/dashboard/admin/channels/channel/channel-form/channel-form.schema.ts (1)
9-9: LGTM.The new discriminated-union arm keeps the form schema aligned with the Teams config schema and the provider key used elsewhere in the PR.
Also applies to: 52-55
src/components/wrappers/dashboard/admin/channels/channel/channel-form/providers/notifications/forms/teams.form.tsx (1)
20-35: LGTM.The field binding is correct, and using
PasswordInputhere avoids exposing the Teams webhook by default.src/features/notifications/providers/teams.ts (1)
57-80: Timeout handling looks good.The
AbortControllerplusfinallycleanup closes the hanging-request gap without leaking the timer.src/db/migrations/meta/0049_snapshot.json (1)
2436-2448: LGTM.The generated snapshot includes the new
microsoft-teamsenum value, which keeps the migration metadata aligned with the runtime/provider changes.
| function safeStringify(data: Record<string, any>): string { | ||
| const seen = new WeakSet(); | ||
| return JSON.stringify(data, (_key, value) => { | ||
| if (typeof value === 'object' && value !== null) { | ||
| if (seen.has(value)) return '[Circular]'; | ||
| seen.add(value); | ||
| } | ||
| return value; | ||
| }, 2); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
node <<'NODE'
try {
console.log(JSON.stringify({ id: 1n }, (_key, value) => value, 2));
} catch (error) {
console.error(String(error));
}
NODERepository: Portabase/portabase
Length of output: 111
🏁 Script executed:
cat -n src/features/notifications/providers/teams.ts | head -30Repository: Portabase/portabase
Length of output: 1233
🏁 Script executed:
#!/bin/bash
node <<'NODE'
// Test the proposed fix
const seen = new WeakSet();
function safeStringify(data) {
try {
return JSON.stringify(
data,
(_key, value) => {
if (typeof value === 'bigint') return value.toString();
if (typeof value === 'object' && value !== null) {
if (seen.has(value)) return '[Circular]';
seen.add(value);
}
return value;
},
2
) ?? '[unserializable payload data]';
} catch {
return '[unserializable payload data]';
}
}
// Test with bigint
console.log("Test 1 - bigint value:");
console.log(safeStringify({ id: 1n }));
// Test with circular reference
console.log("\nTest 2 - circular reference:");
const obj = { a: 1 };
obj.self = obj;
console.log(safeStringify(obj));
// Test with normal data
console.log("\nTest 3 - normal data:");
console.log(safeStringify({ id: 123, name: "test" }));
// Test with mixed types
console.log("\nTest 4 - mixed types:");
console.log(safeStringify({ id: 456n, count: 10, data: { nested: 789n } }));
NODERepository: Portabase/portabase
Length of output: 318
Handle bigint serialization in safeStringify.
The function throws TypeError: Do not know how to serialize a BigInt when the payload contains bigint values. Convert bigint to string in the replacer and add a catch-all fallback.
Proposed fix
-function safeStringify(data: Record<string, any>): string {
- const seen = new WeakSet();
- return JSON.stringify(data, (_key, value) => {
- if (typeof value === 'object' && value !== null) {
- if (seen.has(value)) return '[Circular]';
- seen.add(value);
- }
- return value;
- }, 2);
+function safeStringify(data: unknown): string {
+ const seen = new WeakSet<object>();
+
+ try {
+ return JSON.stringify(
+ data,
+ (_key, value) => {
+ if (typeof value === 'bigint') return value.toString();
+ if (typeof value === 'object' && value !== null) {
+ if (seen.has(value)) return '[Circular]';
+ seen.add(value);
+ }
+ return value;
+ },
+ 2
+ ) ?? '[unserializable payload data]';
+ } catch {
+ return '[unserializable payload data]';
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/notifications/providers/teams.ts` around lines 3 - 11, The
safeStringify function currently throws on bigint values; update the
JSON.stringify replacer inside safeStringify (the replacer used in
safeStringify) to detect typeof value === 'bigint' and return value.toString(),
keep the existing circular-detection logic with seen (WeakSet), and add a
catch-all fallback in the replacer (e.g., return String(value) for any value
that would otherwise cause serialization errors) so JSON.stringify never throws
on unsupported primitives; preserve the 2-space formatting.
| ...(payload.data | ||
| ? [ | ||
| { | ||
| type: 'TextBlock', | ||
| text: `Data: ${safeStringify(payload.data)}`, | ||
| fontType: 'monospace', | ||
| wrap: true, | ||
| }, | ||
| ] | ||
| : []), |
There was a problem hiding this comment.
Don't drop falsy payload.data values.
Line 41 gates the Data: block on truthiness, so 0, false, and "" disappear even when they are the payload. Check for null/undefined instead.
💡 Proposed fix
- ...(payload.data
+ ...(payload.data !== undefined && payload.data !== null
? [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/notifications/providers/teams.ts` around lines 41 - 50, The
conditional that decides whether to include the Data TextBlock incorrectly uses
truthiness on payload.data (so values like 0, false, and "" are dropped); update
the check in the teams notification builder to test for null/undefined instead
(e.g., payload.data != null or payload.data !== undefined) so the block is
included when payload.data is any falsy-but-present value before calling
safeStringify(payload.data).
|
Thanks for the detailed feedback, @KillianLarcher. You were right that the initial submission was incomplete. I've now added the full implementation: What's new in this push:
Screenshots: Provider picker showing Microsoft Teams as available: Create form: Edit form: |
2 participants
Summary
Adds Microsoft Teams as a notification provider. Uses the Teams Incoming Webhook with Adaptive Cards format, following the same pattern as the existing Slack provider.
Changes
src/features/notifications/providers/teams.tswithsendTeams()function'teams'toProviderKindunion intypes.tssendTeamshandler inproviders/index.tsThe Adaptive Card payload includes the alert level (bold heading), message body (with word wrap), and optional data block (monospace).
Fixes #260
This contribution was developed with AI assistance (Codex).
Summary by CodeRabbit