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

Feature: Allow users to edit chat thread title #6

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

giuliohome
Copy link

@giuliohome giuliohome commented Sep 22, 2023

image

useEffect(() => {
const fetchData = async () => {
console.log("fetching items")

Choose a reason for hiding this comment

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

Remove

useEffect(() => {
const fetchData = async () => {
console.log("fetching items")
const data = await FindAllChatThreadForCurrentUser();

Choose a reason for hiding this comment

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

Does this function handle fetching errors?


const doRefresh = () => {
console.log("should refresh")

Choose a reason for hiding this comment

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

remove


const doRefresh = () => {
console.log("should refresh")
setRefresh(!refresh);

Choose a reason for hiding this comment

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

Suggested change
setRefresh(!refresh);
setRefresh(prev => !prev)

Comment on lines +33 to +35

# secrets
start.sh

Choose a reason for hiding this comment

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

Remove

@@ -21,6 +22,11 @@ export const MenuItems: FC<Prop> = (props) => {
router.replace("/chat");
};

const renameTitle = async (threadID: string, title: string) => {
await userUpdateThreadTitle(threadID, title);

Choose a reason for hiding this comment

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

What happens if this fails?

@@ -45,6 +51,20 @@ export const MenuItems: FC<Prop> = (props) => {
<span className="flex gap-2 items-center overflow-hidden flex-1">
<span className="overflow-ellipsis truncate"> {thread.name}</span>

Choose a reason for hiding this comment

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

You're truncating thread names. If thread names are longer than 30 characters you will want to render ellipses here perhaps (...)

const chatThread = threads[0]
await UpsertChatThread({
...chatThread,
name: userTitle.substring(0, 30),

Choose a reason for hiding this comment

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

Instead of truncating here, prevent user from putting down more than 30 characters. That way we don't store truncated names in database. i.e. Review legal documents around tax fil

@giuliohome
Copy link
Author

giuliohome commented Oct 19, 2023

@Ali-Parandeh
I have been using this feature in my private repo for months already. Instead of worrying about unlikely scenarios and asking questions like "what happens if this fails", I can assure you that:

  • As a user, I would immediately stop using an app that doesn't allow editing.
  • As a developer, I don't need to prioritize unrealistic edge cases when there are more important common features missing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants