-
Notifications
You must be signed in to change notification settings - Fork 80
feat(MessageComposer): control command sendability #1746
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
ace3930
a72e8d5
5a0e0ad
2284b22
a9c05c9
0f693a7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| import type { | ||
| CommandsConfig, | ||
| CommandSendValidator, | ||
| MessageComposerConfig, | ||
| } from './types'; | ||
| import type { DeepPartial } from '../../types.utility'; | ||
| import { stripMentionTokens } from '../middleware'; | ||
|
|
||
| export const MENTION_ONLY_COMMANDS = new Set(['mute', 'unmute', 'unban']); | ||
| export const defaultCommandSendabilityValidator: CommandSendValidator = ({ | ||
| command, | ||
| commandArgsText, | ||
| mentionedUsersInText, | ||
| }) => { | ||
| if (command.name !== 'ban' && !MENTION_ONLY_COMMANDS.has(command.name ?? '')) return; | ||
|
|
||
| if (mentionedUsersInText.length === 0) { | ||
| return { command, ready: false, reason: 'missing_mention' }; | ||
| } | ||
|
|
||
| if (command.name !== 'ban') { | ||
| return { command, ready: true }; | ||
| } | ||
|
|
||
| const reason = stripMentionTokens(commandArgsText, mentionedUsersInText); | ||
|
|
||
| if (!reason.length) { | ||
| return { command, ready: false, reason: 'missing_reason' }; | ||
| } | ||
|
|
||
| return { command, ready: true }; | ||
| }; | ||
| export const DEFAULT_COMMANDS_CONFIG: CommandsConfig = { | ||
| sendValidators: [defaultCommandSendabilityValidator], | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we keep these as an array ? Since the validation step is a middleware anyway, integrators can either add their own additional validation steps (or replace our current one) |
||
| }; | ||
| export const applyCommandValidatorOverride = ( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the command validation anyway goes through a middleware, this might not be needed I think. While convenient for integrators, I think convergence towards a middleware approach would be more appropriate (it would separate the need for overrides as well), unless I'm missing something important here |
||
| targetConfig: MessageComposerConfig, | ||
| sourceConfig?: DeepPartial<MessageComposerConfig>, | ||
| ) => { | ||
| const overrideValidators = sourceConfig?.commands?.sendValidators as | ||
| | CommandSendValidator[] | ||
| | undefined; | ||
|
|
||
| if (typeof overrideValidators === 'undefined') { | ||
| return targetConfig; | ||
| } | ||
|
|
||
| return { | ||
| ...targetConfig, | ||
| commands: { | ||
| ...targetConfig.commands, | ||
| sendValidators: overrideValidators, | ||
| }, | ||
| }; | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,2 +1,6 @@ | ||
| export * from './configuration'; | ||
| export * from './types'; | ||
| export { applyCommandValidatorOverride } from './commands.configuration'; | ||
| export { DEFAULT_COMMANDS_CONFIG } from './commands.configuration'; | ||
| export { defaultCommandSendabilityValidator } from './commands.configuration'; | ||
| export { MENTION_ONLY_COMMANDS } from './commands.configuration'; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,7 @@ import { LocationComposer } from './LocationComposer'; | |
| import { MessageComposerEffectHandlers } from './MessageComposerEffectHandlers'; | ||
| import { PollComposer } from './pollComposer'; | ||
| import { TextComposer } from './textComposer'; | ||
| import { DEFAULT_COMPOSER_CONFIG } from './configuration'; | ||
| import { applyCommandValidatorOverride, DEFAULT_COMPOSER_CONFIG } from './configuration'; | ||
| import type { MessageComposerMiddlewareValue } from './middleware'; | ||
| import { | ||
| MessageComposerMiddlewareExecutor, | ||
|
|
@@ -30,7 +30,7 @@ import type { | |
| } from '../types'; | ||
| import { WithSubscriptions } from '../utils/WithSubscriptions'; | ||
| import type { StreamChat } from '../client'; | ||
| import type { MessageComposerConfig } from './configuration/types'; | ||
| import type { CommandSendability, MessageComposerConfig } from './configuration/types'; | ||
| import type { | ||
| CommandSuggestionDisabledReason, | ||
| TextComposerCommandActivationEffect, | ||
|
|
@@ -44,6 +44,10 @@ import type { PollComposerSnapshot } from './pollComposer'; | |
| import type { TextComposerSnapshot } from './textComposer'; | ||
| import type { DeepPartial } from '../types.utility'; | ||
| import type { MergeWithCustomizer } from '../utils/mergeWith/mergeWithCore'; | ||
| import { | ||
| getMentionedUsersInText, | ||
| stripCommandFromText, | ||
| } from './middleware/textComposer/commandUtils'; | ||
|
|
||
| type UnregisterSubscriptions = Unsubscribe; | ||
|
|
||
|
|
@@ -208,7 +212,16 @@ export class MessageComposer extends WithSubscriptions { | |
| ); | ||
| } | ||
|
|
||
| const mergeChannelConfigCustomizer: MergeWithCustomizer< | ||
| /** | ||
| * Customizes config merges for the composer constructor. | ||
| * | ||
| * It catches two scalar override cases that should not use the default deep merge: | ||
| * - client-disabled `enabled` flags stay disabled even if the channel config tries to re-enable them | ||
| * - scalar channel-config values replace client defaults for matching config keys | ||
| * | ||
| * All other values fall back to the normal `mergeWith` behavior. | ||
| */ | ||
| const mergeMessageComposerConfigCustomizer: MergeWithCustomizer< | ||
| DeepPartial<MessageComposerConfig> | ||
| > = (originalVal, channelConfigVal, key) => | ||
| typeof originalVal === 'object' | ||
|
|
@@ -223,14 +236,17 @@ export class MessageComposer extends WithSubscriptions { | |
| : originalVal; | ||
|
|
||
| this.configState = new StateStore<MessageComposerConfig>( | ||
| mergeWith( | ||
| mergeWith(DEFAULT_COMPOSER_CONFIG, config ?? {}), | ||
| { | ||
| location: { | ||
| enabled: this.channel.getConfig()?.shared_locations, | ||
| applyCommandValidatorOverride( | ||
| mergeWith( | ||
| mergeWith(DEFAULT_COMPOSER_CONFIG, config ?? {}), | ||
| { | ||
| location: { | ||
| enabled: this.channel.getConfig()?.shared_locations, | ||
| }, | ||
| }, | ||
| }, | ||
| mergeChannelConfigCustomizer, | ||
| mergeMessageComposerConfigCustomizer, | ||
| ), | ||
| config, | ||
| ), | ||
| ); | ||
|
|
||
|
|
@@ -360,6 +376,14 @@ export class MessageComposer extends WithSubscriptions { | |
| return this.state.getLatestValue().quotedMessage; | ||
| } | ||
|
|
||
| get pollId() { | ||
| return this.state.getLatestValue().pollId; | ||
| } | ||
|
|
||
| get showReplyInChannel() { | ||
| return this.state.getLatestValue().showReplyInChannel; | ||
| } | ||
|
|
||
| getCommandDisabledReason = ( | ||
| command: CommandResponse, | ||
| ): CommandSuggestionDisabledReason | undefined => { | ||
|
|
@@ -378,21 +402,49 @@ export class MessageComposer extends WithSubscriptions { | |
| isCommandDisabled = (command: CommandResponse) => | ||
| !!this.getCommandDisabledReason(command); | ||
|
|
||
| get pollId() { | ||
| return this.state.getLatestValue().pollId; | ||
| } | ||
| validateCommandSendability = ( | ||
| command: CommandResponse, | ||
| text = this.textComposer.text, | ||
| ): CommandSendability => { | ||
| const currentMentionedUsers = this.textComposer.mentionedUsers; | ||
| const mentionedUsersInText = getMentionedUsersInText(text, currentMentionedUsers); | ||
|
|
||
| const validationContext = { | ||
| command, | ||
| commandArgsText: command.name | ||
| ? stripCommandFromText(text, command.name).trim() | ||
| : text.trim(), | ||
| composer: this, | ||
| mentionedUsersInText, | ||
| rawText: text, | ||
| }; | ||
|
|
||
| get showReplyInChannel() { | ||
| return this.state.getLatestValue().showReplyInChannel; | ||
| } | ||
| for (const validator of this.config.commands.sendValidators) { | ||
| const result = validator(validationContext); | ||
| if (result && !result.ready) { | ||
| return result; | ||
| } | ||
| } | ||
|
|
||
| return { command, ready: true }; | ||
| }; | ||
|
|
||
| isCommandSendable = (command: CommandResponse, text = this.textComposer.text) => | ||
| this.validateCommandSendability(command, text).ready; | ||
|
|
||
| get hasSendableData() { | ||
| return !!( | ||
| (!this.attachmentManager.uploadsInProgressCount && | ||
| (!this.textComposer.textIsEmpty || | ||
| this.attachmentManager.successfulUploadsCount > 0)) || | ||
| this.pollId || | ||
| !!this.locationComposer.validLocation | ||
| const currentCommand = this.textComposer.command; | ||
| const commandIsSendable = !currentCommand || this.isCommandSendable(currentCommand); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be possible to extract this in a separate getter please ? So something like: |
||
|
|
||
| return ( | ||
| commandIsSendable && | ||
| !!( | ||
| (!this.attachmentManager.uploadsInProgressCount && | ||
| (!this.textComposer.textIsEmpty || | ||
| this.attachmentManager.successfulUploadsCount > 0)) || | ||
| this.pollId || | ||
| !!this.locationComposer.validLocation | ||
| ) | ||
| ); | ||
| } | ||
|
|
||
|
|
@@ -426,7 +478,9 @@ export class MessageComposer extends WithSubscriptions { | |
| } | ||
|
|
||
| updateConfig(config: DeepPartial<MessageComposerConfig>) { | ||
| this.configState.partialNext(mergeWith(this.config, config)); | ||
| this.configState.partialNext( | ||
| applyCommandValidatorOverride(mergeWith(this.config, config), config), | ||
| ); | ||
| } | ||
|
|
||
| refreshId = () => { | ||
|
|
@@ -624,6 +678,7 @@ export class MessageComposer extends WithSubscriptions { | |
| draft.channel_cid !== this.channel.cid | ||
| ) | ||
| return; | ||
| if (this.editedMessage) return; | ||
| this.initState({ composition: draft }); | ||
| }).unsubscribe; | ||
|
|
||
|
|
@@ -637,6 +692,7 @@ export class MessageComposer extends WithSubscriptions { | |
| ) { | ||
| return; | ||
| } | ||
| if (this.editedMessage) return; | ||
|
|
||
| this.logDraftUpdateTimestamp(); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,7 +1,12 @@ | ||||||||||||
| import { textIsEmpty } from '../../textComposer'; | ||||||||||||
| import type { CommandResponse } from '../../../types'; | ||||||||||||
| import { CommandSearchSource } from '../textComposer/commands'; | ||||||||||||
| import { getRawCommandName, notifyCommandDisabled } from '../textComposer/commandUtils'; | ||||||||||||
| import { | ||||||||||||
| getCommandByName, | ||||||||||||
| getRawCommandName, | ||||||||||||
| notifyCommandDisabled, | ||||||||||||
| notifyCommandNotReady, | ||||||||||||
| } from '../textComposer/commandUtils'; | ||||||||||||
| import type { | ||||||||||||
| MessageComposerMiddlewareState, | ||||||||||||
| MessageCompositionMiddleware, | ||||||||||||
|
|
@@ -11,21 +16,9 @@ import type { | |||||||||||
| import type { MessageComposer } from '../../messageComposer'; | ||||||||||||
| import type { MiddlewareHandlerParams } from '../../../middleware'; | ||||||||||||
|
|
||||||||||||
| const getCommandByName = ( | ||||||||||||
| searchSource: CommandSearchSource, | ||||||||||||
| commandName?: string, | ||||||||||||
| ): CommandResponse | undefined => { | ||||||||||||
| if (!commandName) return; | ||||||||||||
|
|
||||||||||||
| const normalizedCommandName = commandName.toLowerCase(); | ||||||||||||
| return searchSource | ||||||||||||
| .query(normalizedCommandName) | ||||||||||||
| .items.find((command) => command.name?.toLowerCase() === normalizedCommandName); | ||||||||||||
| }; | ||||||||||||
|
|
||||||||||||
| const getDisabledRawCommand = ( | ||||||||||||
| composer: MessageComposer, | ||||||||||||
| searchSource: CommandSearchSource, | ||||||||||||
| searchSource: Pick<CommandSearchSource, 'query'>, | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not just use |
||||||||||||
| text?: string, | ||||||||||||
| ): CommandResponse | undefined => { | ||||||||||||
| const rawCommand = getCommandByName(searchSource, getRawCommandName(text)); | ||||||||||||
|
|
@@ -36,8 +29,10 @@ const getDisabledRawCommand = ( | |||||||||||
|
|
||||||||||||
| export const createCompositionValidationMiddleware = ( | ||||||||||||
| composer: MessageComposer, | ||||||||||||
| commandSearchSource?: Pick<CommandSearchSource, 'query'>, | ||||||||||||
| ): MessageCompositionMiddleware => { | ||||||||||||
| const commandSearchSource = new CommandSearchSource(composer.channel); | ||||||||||||
| const effectiveCommandSearchSource = | ||||||||||||
| commandSearchSource ?? new CommandSearchSource(composer.channel); | ||||||||||||
|
|
||||||||||||
| return { | ||||||||||||
| id: 'stream-io/message-composer-middleware/data-validation', | ||||||||||||
|
|
@@ -52,14 +47,28 @@ export const createCompositionValidationMiddleware = ( | |||||||||||
|
|
||||||||||||
| const disabledRawCommand = getDisabledRawCommand( | ||||||||||||
| composer, | ||||||||||||
| commandSearchSource, | ||||||||||||
| effectiveCommandSearchSource, | ||||||||||||
| inputText, | ||||||||||||
| ); | ||||||||||||
| if (disabledRawCommand) { | ||||||||||||
| notifyCommandDisabled(composer, disabledRawCommand); | ||||||||||||
| return await discard(); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| const currentCommand = | ||||||||||||
| composer.textComposer.command ?? | ||||||||||||
| getCommandByName(effectiveCommandSearchSource, getRawCommandName(inputText)); | ||||||||||||
| if (currentCommand) { | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||
| if ( | ||||||||||||
| notifyCommandNotReady({ | ||||||||||||
| composer, | ||||||||||||
| sendability: composer.validateCommandSendability(currentCommand, inputText), | ||||||||||||
| }) | ||||||||||||
| ) { | ||||||||||||
| return await discard(); | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| const hasExceededMaxLength = | ||||||||||||
| typeof maxLengthOnSend === 'number' && inputText.length > maxLengthOnSend; | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be missing something here, but how would this generate a good
reasonfor why thecommandis currently not sendable ?