refactor(react): chat new connect components polish#10725
Merged
djabarovgeorge merged 4 commits intonextfrom Apr 15, 2026
Merged
refactor(react): chat new connect components polish#10725djabarovgeorge merged 4 commits intonextfrom
djabarovgeorge merged 4 commits intonextfrom
Conversation
- Removed the playground directory from .cursorignore to streamline project structure. - Added a test case to ensure that `subscriberId` is not stored when `connectionMode` is set to "shared" in the channel connection creation process. - Refactored `CreateChannelConnection` and `GenerateSlackOauthUrl` use cases to handle `subscriberId` based on `connectionMode`, improving clarity and maintainability. - Updated UI components to ensure proper context resolution and connection mode handling, enhancing user experience in chat integration.
…context validation and tooltips - Added context validation for connection mode in both ConnectChat and SlackConnectButton components, ensuring proper error handling when using 'shared' mode. - Introduced tooltips to inform users about missing context requirements, improving user experience and clarity. - Refactored connection handling logic to streamline context resolution and connection mode management. - Updated appearance keys to include new tooltip identifiers for better styling management.
…onnection hooks - Enhanced the `useChannelConnection` and `useChannelConnections` hooks to include checks for `args.identifier`, ensuring that only relevant events trigger state updates. - Wrapped the channel connections list call in a try-catch block to handle potential errors more gracefully, improving overall error management and user experience.
Contributor
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
✅ Deploy Preview for dashboard-v2-novu-staging canceled.
|
Contributor
|
Hey there and thank you for opening this pull request! 👋 We require pull request titles to follow specific formatting rules and it looks like your proposed title needs to be adjusted. Your PR title is: Requirements:
Expected format: Details: PR title must end with 'fixes TICKET-ID' (e.g., 'fixes NOV-123') or include ticket ID in branch name |
Contributor
📝 WalkthroughWalkthroughAllows supplying Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client UI
participant API as Novu API
participant OAuth as OAuth Provider
participant Repo as ChannelConnection Repo
UI->>API: Request generate chat OAuth URL (connectionMode, subscriberId?, context?)
API->>API: derive subscriberId = undefined if connectionMode == "shared"
API->>OAuth: create secure state (includes subscriberId undefined for shared)
OAuth-->>UI: redirect URL
UI->>OAuth: OAuth flow -> callback to API with state
API->>API: decode state (subscriberId absent/undefined for shared)
API->>Repo: create channel connection (store subscriberId undefined for shared)
Repo-->>API: stored connection
API-->>UI: success response (contextKeys present)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
packages/js/src/ui/components/slack-connect-button/SlackConnectButton.tsx (1)
260-309: Consider extracting the disabled button rendering to reduce duplication.Both branches (misconfigured fallback and normal) render similar
Buttoncomponents with overlapping styling logic. The main differences are the tooltip wrapper,disabledstate, andonClickhandler.This is a minor observation—the current implementation is readable and functional.
♻️ Optional: Extract shared button props
const buttonClass = (connected: boolean, pointerEventsOverride?: boolean) => style({ key: 'channelConnectButton', className: `nt-transition-[width] nt-duration-800 nt-will-change-[width]${pointerEventsOverride ? ' !nt-pointer-events-auto' : ''}`, context: { connected } satisfies Parameters< ChannelConnectButtonAppearanceCallback['channelConnectButton'] >[0], });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/js/src/ui/components/slack-connect-button/SlackConnectButton.tsx` around lines 260 - 309, The duplicated Button rendering in SlackConnectButton should be consolidated: create a helper that builds shared props/class for the Button (referencing the style key 'channelConnectButton' and the appearance callback type ChannelConnectButtonAppearanceCallback), then use that helper in both branches of the Show conditional; keep the Tooltip.Root wrapper only in the misconfigured branch and ensure that in that branch the Button is rendered with disabled={true} and pointer-events override, while in the normal branch it uses onClick={handleClick}, disabled={isLoading()}, and context connected={isConnected()}; reuse buttonContent() for children.apps/api/src/app/channel-connections/e2e/create-channel-connection.e2e.ts (1)
73-98: Good regression test; consider adding a duplicate-shared create assertion too.This validates null persistence well. I’d also add a second create with same
integrationIdentifier + context + connectionMode: 'shared'and assert conflict, to lock uniqueness behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/src/app/channel-connections/e2e/create-channel-connection.e2e.ts` around lines 73 - 98, Add a second create attempt with the exact same integrationIdentifier + context + connectionMode: 'shared' after the existing successful create in the test (using the same CreateChannelConnectionRequestDto built with createSlackIntegration, createSubscribersService and novuClient.channelConnections.create) and assert the call fails as a duplicate/unique-conflict (e.g., catch the thrown error and assert HTTP status 409 or the service's conflict error code/message). Keep the initial assertions (result.subscriberId is null and contextKeys non-empty) and then perform the duplicate create and assert the conflict response to validate uniqueness enforcement.packages/react/src/hooks/useChannelConnection.ts (1)
62-71: Add blank lines before these early returns.Both guards miss the repo spacing rule for
return.As per coding guidelines "Include a blank line before every
returnstatement".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/src/hooks/useChannelConnection.ts` around lines 62 - 71, In the two event handler registrations (cleanupGetPending and cleanupGetResolved) for novu.on('channel-connection.get.pending') and novu.on('channel-connection.get.resolved'), add a blank line immediately before each early return guarded by the args check (the lines that compare args.identifier to propsRef.current.identifier) so they follow the repo rule "Include a blank line before every return"; keep the existing guard logic and subsequent calls like setIsFetching(true) and handling of data/error unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/api/src/app/channel-connections/usecases/create-channel-connection/create-channel-connection.usecase.ts`:
- Around line 114-123: The uniqueness check uses command.subscriberId while the
create call uses a normalized subscriberId (undefined for shared mode), causing
duplicates; normalize subscriberId once (e.g., const subscriberId =
command.connectionMode === 'shared' ? undefined : command.subscriberId) before
performing the uniqueness pre-check and use that same subscriberId variable in
the duplicate-query and in channelConnectionRepository.create so both checks
share the same semantics (refer to subscriberId, the uniqueness pre-check call,
and channelConnectionRepository.create in create-channel-connection.usecase.ts).
In
`@apps/api/src/app/integrations/usecases/generate-chat-oath-url/generate-slack-oath-url/generate-slack-oauth-url.usecase.ts`:
- Around line 50-53: The check currently uses the raw command.subscriberId
before you strip it for shared mode, causing assertResourceExists to fail for
shared OAuth flows; update the call site so assertResourceExists uses the
normalized subscriberId variable (the same one passed into createSecureState)
instead of command.subscriberId, ensuring you compute subscriberId =
command.connectionMode === 'shared' ? undefined : command.subscriberId first and
then call assertResourceExists(integration, subscriberId) and
createSecureState(integration, subscriberId).
In `@packages/react/src/hooks/useChannelConnection.ts`:
- Around line 62-72: The fetchConnection flow can still apply a stale awaited
response when propsRef.current.identifier changes mid-request; update
fetchConnection to capture the current identifier (or generate a local request
token) at start and, before any state updates or applying the awaited response,
verify that propsRef.current.identifier (or the token) still matches the
captured identifier; if it doesn’t match, drop the response. Make this check in
the async resolution path that calls setIsFetching/setConnection so stale A
responses cannot overwrite B, and keep the existing novu.on guards
(cleanupGetPending/cleanupGetResolved) as-is.
In `@packages/react/src/hooks/useChannelConnections.ts`:
- Around line 47-60: The try/catch should only wrap the SDK call to
novu.channelConnections.list so consumer exceptions from onSuccess/onError
aren't misclassified; change the code in useChannelConnections.ts to await
novu.channelConnections.list(...) inside a small try block, assign the response
to a variable, and then handle response.error and response.data outside the try:
call setError/setConnections and invoke onError/onSuccess after the SDK call
completes. Ensure you do not call onError twice for the same error — when
response.error is present call setError and onError once, and in the catch block
(SDK failure) call setError and onError once.
---
Nitpick comments:
In `@apps/api/src/app/channel-connections/e2e/create-channel-connection.e2e.ts`:
- Around line 73-98: Add a second create attempt with the exact same
integrationIdentifier + context + connectionMode: 'shared' after the existing
successful create in the test (using the same CreateChannelConnectionRequestDto
built with createSlackIntegration, createSubscribersService and
novuClient.channelConnections.create) and assert the call fails as a
duplicate/unique-conflict (e.g., catch the thrown error and assert HTTP status
409 or the service's conflict error code/message). Keep the initial assertions
(result.subscriberId is null and contextKeys non-empty) and then perform the
duplicate create and assert the conflict response to validate uniqueness
enforcement.
In `@packages/js/src/ui/components/slack-connect-button/SlackConnectButton.tsx`:
- Around line 260-309: The duplicated Button rendering in SlackConnectButton
should be consolidated: create a helper that builds shared props/class for the
Button (referencing the style key 'channelConnectButton' and the appearance
callback type ChannelConnectButtonAppearanceCallback), then use that helper in
both branches of the Show conditional; keep the Tooltip.Root wrapper only in the
misconfigured branch and ensure that in that branch the Button is rendered with
disabled={true} and pointer-events override, while in the normal branch it uses
onClick={handleClick}, disabled={isLoading()}, and context
connected={isConnected()}; reuse buttonContent() for children.
In `@packages/react/src/hooks/useChannelConnection.ts`:
- Around line 62-71: In the two event handler registrations (cleanupGetPending
and cleanupGetResolved) for novu.on('channel-connection.get.pending') and
novu.on('channel-connection.get.resolved'), add a blank line immediately before
each early return guarded by the args check (the lines that compare
args.identifier to propsRef.current.identifier) so they follow the repo rule
"Include a blank line before every return"; keep the existing guard logic and
subsequent calls like setIsFetching(true) and handling of data/error unchanged.
🪄 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: dca57f54-d9a5-45ea-96de-81cc0de0b566
📒 Files selected for processing (11)
apps/api/src/app/channel-connections/e2e/create-channel-connection.e2e.tsapps/api/src/app/channel-connections/usecases/channel-connection.utils.tsapps/api/src/app/channel-connections/usecases/create-channel-connection/create-channel-connection.usecase.tsapps/api/src/app/inbox/inbox.controller.tsapps/api/src/app/integrations/usecases/generate-chat-oath-url/generate-slack-oath-url/generate-slack-oauth-url.usecase.tspackages/js/src/ui/components/connect-chat/ConnectChat.tsxpackages/js/src/ui/components/slack-connect-button/SlackConnectButton.tsxpackages/js/src/ui/config/appearanceKeys.tspackages/react/src/hooks/useChannelConnection.tspackages/react/src/hooks/useChannelConnections.tsplayground/nextjs/src/pages/connect-chat/index.tsx
Comment on lines
+114
to
+123
| const subscriberId = command.connectionMode === 'shared' ? undefined : command.subscriberId; | ||
|
|
||
| const channelConnection = await this.channelConnectionRepository.create({ | ||
| identifier, | ||
| integrationIdentifier: integration.identifier, | ||
| providerId: integration.providerId, | ||
| channel: integration.channel, | ||
| _organizationId: command.organizationId, | ||
| _environmentId: command.environmentId, | ||
| subscriberId: command.subscriberId, | ||
| subscriberId, |
Contributor
There was a problem hiding this comment.
Uniqueness check and persistence use different subscriberId semantics in shared mode.
subscriberId is stripped only at create time here, but uniqueness pre-check still queries with command.subscriberId (Line 88). For shared mode, existing records are stored with null/undefined subscriberId, so the duplicate check can miss them and allow duplicate connections.
Suggested fix
async execute(command: CreateChannelConnectionCommand): Promise<ChannelConnectionEntity> {
this.validateResourceOrContext(command);
const integration = await this.findIntegration(command);
const contextKeys = await this.resolveContexts(command);
+ const effectiveSubscriberId = command.connectionMode === 'shared' ? undefined : command.subscriberId;
await this.assertSubscriberExists(command);
- await this.ensureUniqueConnectionForResourceAndContext(command, integration, contextKeys);
+ await this.ensureUniqueConnectionForResourceAndContext(command, integration, contextKeys, effectiveSubscriberId);
const identifier = command.identifier || this.generateIdentifier();
// ...
- const channelConnection = await this.createChannelConnection(command, identifier, integration, contextKeys);
+ const channelConnection = await this.createChannelConnection(
+ command,
+ identifier,
+ integration,
+ contextKeys,
+ effectiveSubscriberId
+ );
}
private async ensureUniqueConnectionForResourceAndContext(
command: CreateChannelConnectionCommand,
integration: IntegrationEntity,
- contextKeys: string[]
+ contextKeys: string[],
+ effectiveSubscriberId?: string
) {
const baseQuery = {
_organizationId: command.organizationId,
_environmentId: command.environmentId,
integrationIdentifier: integration.identifier,
- subscriberId: command.subscriberId,
+ subscriberId: effectiveSubscriberId,
};
// ...
}
private async createChannelConnection(
command: CreateChannelConnectionCommand,
identifier: string,
integration: IntegrationEntity,
- contextKeys: string[]
+ contextKeys: string[],
+ effectiveSubscriberId?: string
): Promise<ChannelConnectionEntity> {
- const subscriberId = command.connectionMode === 'shared' ? undefined : command.subscriberId;
-
const channelConnection = await this.channelConnectionRepository.create({
// ...
- subscriberId,
+ subscriberId: effectiveSubscriberId,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/api/src/app/channel-connections/usecases/create-channel-connection/create-channel-connection.usecase.ts`
around lines 114 - 123, The uniqueness check uses command.subscriberId while the
create call uses a normalized subscriberId (undefined for shared mode), causing
duplicates; normalize subscriberId once (e.g., const subscriberId =
command.connectionMode === 'shared' ? undefined : command.subscriberId) before
performing the uniqueness pre-check and use that same subscriberId variable in
the duplicate-query and in channelConnectionRepository.create so both checks
share the same semantics (refer to subscriberId, the uniqueness pre-check call,
and channelConnectionRepository.create in create-channel-connection.usecase.ts).
Comment on lines
+50
to
+53
| const subscriberId = command.connectionMode === 'shared' ? undefined : command.subscriberId; | ||
| const secureState = await this.createSecureState( | ||
| command.integration, | ||
| command.subscriberId, | ||
| subscriberId, |
Contributor
There was a problem hiding this comment.
Shared-mode stripping is incomplete because existence check still uses raw command.subscriberId.
You strip subscriberId for shared mode here, but assertResourceExists(command) runs earlier against the unstripped value. That can fail shared OAuth generation for a subscriberId that should be ignored in shared mode.
Suggested fix
async execute(command: GenerateSlackOauthUrlCommand): Promise<string> {
+ const effectiveSubscriberId = command.connectionMode === 'shared' ? undefined : command.subscriberId;
this.validateSubscriberIdOrContext(command);
- await this.assertResourceExists(command);
+ await this.assertResourceExists({
+ subscriberId: effectiveSubscriberId,
+ organizationId: command.organizationId,
+ environmentId: command.environmentId,
+ });
const { clientId } = await this.getIntegrationCredentials(command.integration);
- const subscriberId = command.connectionMode === 'shared' ? undefined : command.subscriberId;
const secureState = await this.createSecureState(
command.integration,
- subscriberId,
+ effectiveSubscriberId,
command.context,
command.connectionIdentifier,
command.mode,
command.connectionMode
);
}
-private async assertResourceExists(command: GenerateSlackOauthUrlCommand) {
- const { subscriberId, organizationId, environmentId } = command;
+private async assertResourceExists({
+ subscriberId,
+ organizationId,
+ environmentId,
+}: {
+ subscriberId?: string;
+ organizationId: string;
+ environmentId: string;
+}) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/api/src/app/integrations/usecases/generate-chat-oath-url/generate-slack-oath-url/generate-slack-oauth-url.usecase.ts`
around lines 50 - 53, The check currently uses the raw command.subscriberId
before you strip it for shared mode, causing assertResourceExists to fail for
shared OAuth flows; update the call site so assertResourceExists uses the
normalized subscriberId variable (the same one passed into createSecureState)
instead of command.subscriberId, ensuring you compute subscriberId =
command.connectionMode === 'shared' ? undefined : command.subscriberId first and
then call assertResourceExists(integration, subscriberId) and
createSecureState(integration, subscriberId).
- Updated the Postman collection to reflect changes in API endpoints and request structures, including new connection modes and OAuth flow options. - Incremented the OpenAPI document version to 3.15.0 and updated the SDK generation version to 2.879.13 to align with the latest changes. - Enhanced the CreateChannelConnection and GenerateChatOauthUrl request DTOs to support new connection modes and OAuth flow modes, improving flexibility in channel connection management.
@novu/agent-toolkit
@novu/js
@novu/nextjs
novu
@novu/react
@novu/react-native
commit: |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libs/internal-sdk/src/models/components/generatechatoauthurlrequestdto.ts (1)
23-178:⚠️ Potential issue | 🟠 MajorAvoid direct edits in this auto-generated SDK file.
This file is marked generated (
DO NOT EDIT), but it contains hand-reviewed changes in this PR. Please apply the change at the source definition level and regenerate SDK artifacts to keep regeneration reproducible and prevent drift.As per coding guidelines:
libs/internal-sdk/**: Never edit auto-generated files in libs/internal-sdk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/internal-sdk/src/models/components/generatechatoauthurlrequestdto.ts` around lines 23 - 178, The PR edited an auto-generated SDK file (symbols: GenerateChatOauthUrlRequestDto, Mode, GenerateChatOauthUrlRequestDtoConnectionMode, and related outbound schemas); revert those manual changes in libs/internal-sdk and instead update the upstream source model (the generator input/schema that defines Mode, GenerateChatOauthUrlRequestDto, and context types), then run the SDK generation script to produce updated artifacts so the changes are reproducible and the generated file remains untouched.libs/internal-sdk/src/models/components/createchannelconnectionrequestdto.ts (1)
1-3:⚠️ Potential issue | 🟠 MajorDo not manually edit auto-generated SDK files. This file is generated by Speakeasy and marked
DO NOT EDIT. Any updates must come from regenerating the SDK based on source API spec changes (i.e., modifications toapps/api/swagger-spec.jsonfollowed by running the Speakeasy workflow), not through direct edits to the generated code.Note: There is also a documentation inconsistency—the field docs claim
"subscriber" (default), but the backend validation allows eithersubscriberIdorcontextwhenconnectionModeis omitted. This should be resolved in the OpenAPI spec definition upstream, not by editing the generated SDK file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/internal-sdk/src/models/components/createchannelconnectionrequestdto.ts` around lines 1 - 3, This is an auto-generated file (createchannelconnectionrequestdto.ts) and must not be edited directly; instead, fix the OpenAPI spec upstream (update the relevant schema in apps/api/swagger-spec.json to resolve the documentation vs. validation inconsistency about the default "subscriber" and the rule that allows either subscriberId or context when connectionMode is omitted), then regenerate the SDK by running the Speakeasy workflow so the CreateChannelConnectionRequestDto model is updated consistently in the generated output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@libs/internal-sdk/src/models/components/createchannelconnectionrequestdto.ts`:
- Around line 33-35: The JSDoc for the connectionMode field in
createchannelconnectionrequestdto.ts incorrectly states `"subscriber"
(default)`; update the comment for the connectionMode property (and the other
similar blocks referenced) to remove the default claim and instead explain valid
usage: note that connectionMode can be "subscriber" or "shared", and that it may
be omitted when either subscriberId or context is provided (backend accepts
omitted connectionMode in those cases), and clarify that when omitted the
backend will accept either scope based on those accompanying fields.
---
Outside diff comments:
In
`@libs/internal-sdk/src/models/components/createchannelconnectionrequestdto.ts`:
- Around line 1-3: This is an auto-generated file
(createchannelconnectionrequestdto.ts) and must not be edited directly; instead,
fix the OpenAPI spec upstream (update the relevant schema in
apps/api/swagger-spec.json to resolve the documentation vs. validation
inconsistency about the default "subscriber" and the rule that allows either
subscriberId or context when connectionMode is omitted), then regenerate the SDK
by running the Speakeasy workflow so the CreateChannelConnectionRequestDto model
is updated consistently in the generated output.
In `@libs/internal-sdk/src/models/components/generatechatoauthurlrequestdto.ts`:
- Around line 23-178: The PR edited an auto-generated SDK file (symbols:
GenerateChatOauthUrlRequestDto, Mode,
GenerateChatOauthUrlRequestDtoConnectionMode, and related outbound schemas);
revert those manual changes in libs/internal-sdk and instead update the upstream
source model (the generator input/schema that defines Mode,
GenerateChatOauthUrlRequestDto, and context types), then run the SDK generation
script to produce updated artifacts so the changes are reproducible and the
generated file remains untouched.
🪄 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: 8882828b-98c4-4bd4-8fc5-856e126f2994
⛔ Files ignored due to path filters (1)
libs/internal-sdk/postman/.speakeasy/gen.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
libs/internal-sdk/postman/novu_api_postman_collection.jsonlibs/internal-sdk/src/lib/config.tslibs/internal-sdk/src/models/components/createchannelconnectionrequestdto.tslibs/internal-sdk/src/models/components/generatechatoauthurlrequestdto.ts
✅ Files skipped from review due to trivial changes (1)
- libs/internal-sdk/src/lib/config.ts
Comment on lines
+33
to
+35
| /** | ||
| * Connection mode that determines how the channel connection is scoped. Use "subscriber" (default) to associate the connection with a specific subscriber. Use "shared" to associate the connection with a context instead of a subscriber — subscriberId will not be stored on the connection. | ||
| */ |
Contributor
There was a problem hiding this comment.
connectionMode docs currently overstate a default that backend validation does not enforce.
The comment says "subscriber" (default), but backend validation allows omitted connectionMode when either subscriberId or context is provided. This can mislead SDK consumers.
Suggested doc wording update
- * Connection mode that determines how the channel connection is scoped. Use "subscriber" (default) to associate the connection with a specific subscriber. Use "shared" to associate the connection with a context instead of a subscriber — subscriberId will not be stored on the connection.
+ * Connection mode that determines how the channel connection is scoped.
+ * Use "subscriber" to scope to a specific subscriber.
+ * Use "shared" to scope to context (subscriberId is ignored for persistence).
+ * If omitted, server-side validation accepts either subscriberId or context.Also applies to: 41-42, 58-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/internal-sdk/src/models/components/createchannelconnectionrequestdto.ts`
around lines 33 - 35, The JSDoc for the connectionMode field in
createchannelconnectionrequestdto.ts incorrectly states `"subscriber"
(default)`; update the comment for the connectionMode property (and the other
similar blocks referenced) to remove the default claim and instead explain valid
usage: note that connectionMode can be "subscriber" or "shared", and that it may
be omitted when either subscriberId or context is provided (backend accepts
omitted connectionMode in those cases), and clarify that when omitted the
backend will accept either scope based on those accompanying fields.
This was referenced Apr 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
subscriberIdis not stored whenconnectionModeis set to "shared" in the channel connection creation process.CreateChannelConnectionandGenerateSlackOauthUrluse cases to handlesubscriberIdbased onconnectionMode, improving clarity and maintainability.What changed
This PR clarifies and centralizes chat connection handling: backend use cases and OAuth URL generation now conditionally omit persisting subscriberId when connectionMode is "shared", and a new E2E test verifies subscriberId is not stored for shared connections. UI components were refactored to detect misconfiguration (shared mode without context) and surface it with disabled buttons and tooltips instead of throwing errors. Hooks and SDK metadata were updated to make event handling and API/DTO contracts more robust.
Affected areas
api: CreateChannelConnection and GenerateSlackOauthUrl use cases now set subscriberId to undefined for shared connections; validation was relaxed so callers control subscriberId stripping; controller forwards connectionMode into OAuth flows; an E2E test added to assert subscriberId is not persisted for shared mode.
js: ConnectChat and SlackConnectButton components refactored to memoize connectionMode and resolved context, detect misconfiguration, log warnings, and render disabled buttons with tooltips; new appearance keys added for tooltip text.
react: useChannelConnection handlers now ignore events that don't match the current identifier to avoid stale updates; useChannelConnections list call is wrapped in try/catch/finally to surface thrown errors and always clear loading state.
libs/internal-sdk: SDK metadata bumped and generated DTOs extended to include connectionMode (and related OAuth/mode fields) so the API surface and SDK reflect new connection and OAuth flow options.
playground: Example updated to demonstrate shared mode with context passed to NovuProvider.
Key technical decisions
Testing
Added an E2E test validating that subscriberId is not persisted for shared connections; no additional unit tests were added—changes are covered by integration/E2E verification of the connection and OAuth flows.
Screenshots
Expand for optional sections
Related enterprise PR
Special notes for your reviewer