Skip to content
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

Refactor InputBar with Tip Tap Library #2992

Merged
merged 31 commits into from
Dec 22, 2023
Merged

Refactor InputBar with Tip Tap Library #2992

merged 31 commits into from
Dec 22, 2023

Conversation

flvndvd
Copy link
Contributor

@flvndvd flvndvd commented Dec 21, 2023

Description

This PR overhauls the fundamental workings of the input bar (distinct from the code definition, which includes the generation animation and additional logic). Specifically, this update replaces the complex, legacy code in the InputBar file, which previously managed a content-editable interface, with a new implementation powered by TipTap.

Enhancements introduced in this update include:

  • Improved handling of mentions, including fixes for typos and issues with copy/pasting.
  • Maintained styling of the Input bar which is now more restrictive with larger content.
  • Deactivation of the submit button when no content is present.
  • Significantly reduces the complexity of this part of the code.

🔈 Additional changes were made during the overhaul:

  • The input bar now clears text when switching conversations, aligning with the context-specific nature of conversation-assistant interactions. Although this design decision is up for debate, it could lead to future implementations preserving drafts across conversation browsing.

Pending issues to address include:

  • Implementing file type filtering during uploads to ensure only supported files can be selected (was already broken).
  • Adding scroll functionality to the @mention dropdown within the input bar, matching the usability provided by the right-hand call-to-action.
  • Placeholder should be removed on focus
  • Always display the mention dropdown around

🔮 Long-term enhancements on the horizon:

  • The interim solution of employing tippy.js (which by the way has poor TS types coverage -- hence the any) for positioning the dropdown when typing @ will need revisiting for potential dependency removal.

Undertaking such a refactoring carries the inherent risk of regressions. The input bar is a pivotal component, serving as the initial interface for users interacting with Dust. However, this modernization significantly reduces complexity, boosting our development velocity and simplifying maintenance. It also opens the door to effortlessly integrating text formatting features like bold, italics, and code blocks (which could be leverage to influence the models).

Tests

  • Direct mentions
  • Sticky mentions
  • Selected assistant
  • Copy / paste mentions
  • Expanding (focus + close on submit)
  • Assistant picker
  • File picker
  • Mobile screen size
  • Big payload
  • Copy / Paste external payload (code, text)

Design

The design does not change from the former implementation.

mentions-tiptap

On mobile:

tiptap-mobile

Action items before merging:

  • Move to an input_bar folder
  • Fix placeholder cursor (88acac5)
  • Typing the handle in full doesn't turn the text into a mention (eg typing @dust doesn't make a mention) (c61fe56)
  • Using TAB doesn't allow to turn the text into a mention (existing input bar does that) (f52269b)
  • Only accept supported file extensions (8038b9c)
  • Editor should get autofocus when page loads (b2f597c)
  • Attachment is not always considered (47aba84)

@flvndvd flvndvd self-assigned this Dec 21, 2023

const SUGGESTION_DISPLAY_LIMIT = 7;

export function makeGetAssistantSuggestions(suggestions: EditorSuggestion[]) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typpy has limited coverage for TypeScript types, so I've used any as a temporary solution (see the PR description for details).

@flvndvd flvndvd marked this pull request as ready for review December 21, 2023 15:46
Copy link
Contributor

@spolu spolu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks great! Incredible improvement.

From testing:

  • Blocking for merge IMHO:

    • Placeholder should disapear on focus
  • Non blocking for merge (which you mention in the description)

    • The suggestion moving below the cursor when there's only one element left

As you mention this component is quite critical. I would suggest getting a +1 from everyone before deploy based on testing on front-edge.

Assume you have my +1 as soon as the placeholder thing is resolved 👍

@spolu
Copy link
Contributor

spolu commented Dec 21, 2023

Oh and as per IRL let's move everything in an input_bar folder 🙏

@@ -1,994 +0,0 @@
import {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file has been moved to input_bar. No changes.

@fontanierh
Copy link
Contributor

Awesome work 🔥

As per slack, 2 small issues identified:

  • typing the handle in full doesn't turn the text into a mention (eg typing @dust doesn't make a mention)
  • using TAB doesn't allow to turn the text into a mention (existing input bar does that)

@fontanierh
Copy link
Contributor

(but other than that you have my +1 as well)

Copy link
Contributor

@lasryaric lasryaric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just caught a little setTimeout() not being cleaned-up while debugging the missing file bug.

useEffect(() => {
if (animate && !isAnimating) {
setIsAnimating(true);
setTimeout(() => setIsAnimating(false), 1500);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we not missing a timeout clean up here in the return of useEffect()?

@philipperolet
Copy link
Contributor

Keen to see it shipped ! 👍 for me too

@flvndvd
Copy link
Contributor Author

flvndvd commented Dec 22, 2023

Just caught a little setTimeout() not being cleaned-up while debugging the missing file bug.

@lasryaric done in 5e1f3bd :)

@flvndvd flvndvd requested a review from lasryaric December 22, 2023 11:57
// for operations that cannot rely on the normal asynchronous nature of setState.
// This is not an ideal solution and should be addressed with a more robust state management strategy
// as a follow-up action to minimize unnecessary re-renders and improve component performance.
function useSyncedState<T>(initialValue: T | undefined) {
Copy link
Contributor Author

@flvndvd flvndvd Dec 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's far from ideal, but this requires some more refactoring that falls out of scope of this already pretty big PR. The issue lies in dependencies that are not stable which leads to re-rendering.

Also from the react documentation:

The set function only updates the state variable for the next render. If you read the state variable after calling the set function, you will still get the old value that was on the screen before your call.

Copy link
Contributor

@lasryaric lasryaric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Truly amazing job! Thanks.

@flvndvd flvndvd merged commit 2457d98 into main Dec 22, 2023
3 checks passed
@flvndvd flvndvd deleted the flav/tip-tap branch December 22, 2023 15:22
flvndvd added a commit that referenced this pull request Dec 22, 2023
flvndvd added a commit that referenced this pull request Dec 22, 2023
@flvndvd flvndvd restored the flav/tip-tap branch December 22, 2023 18:22
flvndvd added a commit that referenced this pull request Dec 22, 2023
flvndvd added a commit that referenced this pull request Dec 23, 2023
* Revert "Revert "Refactor InputBar with Tip Tap Library (#2992)" (#3010)"

This reverts commit 94e1d54.

* Does not handle enter if mention dropdown is opened.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants