Add new InputModel class for managing input state
#171
Conversation
|
|
dlqqq
changed the title
InputModel class for managing input state
|
#148 should probably also be merged first to address any merge conflicts. I've approved that PR. |
| // When a command is selected, the input value is not updated to avoid conflict | ||
| // changes. It must be updated here, even if there is no replacement string. | ||
| inputModel.replaceCurrentWord(command.replaceWith ?? command.name); |
There was a problem hiding this comment.
This change is to avoid concurrency on the input update.
When a command is selected from the list, the onInputChange() callback of the ChatInput is called first with the value (name) of the command. Then the onChange() callback of the Autocomplete is called. This resulted to a replacement of the whole input.
To avoid this behavior, the onInputChange() does not update the input value when change come from a selected command (using the reason props, set to reset in that case), and the value is updated in the onchange(), whether it need to be replaced or not.
This seems to work smoothly, but maybe it deserves some more testing to ensure it did not break anything else.
cc. @dlqqq
There was a problem hiding this comment.
@brichet Wow, thanks for working on this so quickly! All of the high-level changes look good. I left some feedback for you below, but still need more time to complete a full review.
My day tomorrow will be full of meetings, so I will likely need until the end of Thursday to finish reviewing this. Feel free to work on another issue using the Kanban board: https://github.com/orgs/jupyterlab/projects/9
| handleChatCommand( | ||
| command: ChatCommand, | ||
| currentWord: string, | ||
| replaceCurrentWord: (newWord: string) => void | ||
| inputModel: IInputModel | ||
| ): Promise<void>; |
There was a problem hiding this comment.
Can you also pass the input model to getChatCommands() method above on line 60? The currentWord: string argument should be replaced with inputModel: IInputModel.
| import { useChatCommands } from './input/use-chat-commands'; | ||
| import { IChatCommandRegistry } from '../chat-commands'; | ||
|
|
||
| const INPUT_BOX_CLASS = 'jp-chat-input-container'; | ||
|
|
||
| export function ChatInput(props: ChatInput.IProps): JSX.Element { | ||
| const { documentManager, model } = props; | ||
| const [input, setInput] = useState<string>(props.value || ''); | ||
| const [input, setInput] = useState<string>(model.value); |
There was a problem hiding this comment.
I'm hesitant to leave this line here because it creates 2 sources of truth about what the current state of the input is (model.value v.s. input). It seems like we should remove the input and setInput variables entirely. Can you explore making these changes?
| * The initial value of the input (default to '') | ||
| */ | ||
| value?: string; | ||
| model: IInputModel; |
There was a problem hiding this comment.
This is a nitpick, but can we prefer naming input models just input instead of model or inputModel? The word model doesn't convey anything useful. Preferring input produces code that is easier to follow:
input.value // value of the input
input.attachments // the input attachments
input.cursorIndex // index of the cursor in the inputSame goes for the arguments to the IChatCommandProvider methods.
This PR adds an
InputModelclass to handle input state.This is proposed for:
By default, the chat model includes an input model, for the "main" input of the chat panel. When editing a message, a new input model is created for this edition, and deleted when edition is over (cancelled or validated).
Fixes #163