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

Include context in user message #1006

Merged
merged 3 commits into from
Jan 14, 2025

Conversation

zeroliu
Copy link
Collaborator

@zeroliu zeroliu commented Jan 5, 2025

Render context with the user message.

Screenshot 2025-01-05 at 12 50 54 AM Screenshot 2025-01-05 at 12 51 45 AM

@@ -86,52 +90,19 @@ const ChatInput = forwardRef<{ focus: () => void }, ChatInputProps>(
},
}));

const debounce = <T extends (...args: any[]) => any>(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the debounce function and the effect that listens to input message change do the same thing. It caused duplicated context values. Tested that everything still works after removing all the debounce calls

@zeroliu zeroliu force-pushed the zero/include-text-context branch 2 times, most recently from adbdfb8 to 77209ba Compare January 5, 2025 08:52
@zeroliu
Copy link
Collaborator Author

zeroliu commented Jan 5, 2025

Note that editing doesn't work as intended yet. It will make people think the edited message is sent with context but in reality it doesn't. Not sure if we want to push this change before we fix the issue.

@zeroliu zeroliu marked this pull request as ready for review January 5, 2025 08:54
@zeroliu zeroliu requested a review from logancyang January 5, 2025 08:54
@logancyang
Copy link
Owner

Note that editing doesn't work as intended yet. It will make people think the edited message is sent with context but in reality it doesn't. Not sure if we want to push this change before we fix the issue.

Right, let me look into fixing the edit and this one can rebase on that.

@zeroliu
Copy link
Collaborator Author

zeroliu commented Jan 6, 2025

Note that editing doesn't work as intended yet. It will make people think the edited message is sent with context but in reality it doesn't. Not sure if we want to push this change before we fix the issue.

Right, let me look into fixing the edit and this one can rebase on that.

I can look into this, too. It will be related to the edit UI work that will follow up on this one.

@logancyang
Copy link
Owner

Note that editing doesn't work as intended yet. It will make people think the edited message is sent with context but in reality it doesn't. Not sure if we want to push this change before we fix the issue.

Right, let me look into fixing the edit and this one can rebase on that.

I can look into this, too. It will be related to the edit UI work that will follow up on this one.

Sounds good. Right now we have "hidden user message" that has added context in "handleSendMessage" in Chat.tsx. We also have some ad-hoc processing and appending to the user message using contextProcessor. We also have tool calls results appending. So it's a bit of a mess 😅 sorry about that.

I think we need a clear design of a new ChatMessage object with context in it (currently in sharedState.ts), and a new consolidated way for making changes to its context, be it from an edit, or a send time processing. It may touch a bunch of places like Chat.tsx, ChainRunners, and wherever that has logic "enhancing" the chat message.

@logancyang
Copy link
Owner

One thing to be careful about is that any context should be fresh in case the context notes have changed. It is working correctly at the moment for chat input, but we should avoid saving a previous context in an old ChatMessage object.

@zeroliu zeroliu force-pushed the zero/include-text-context branch from 77209ba to edac059 Compare January 12, 2025 19:50
@zeroliu
Copy link
Collaborator Author

zeroliu commented Jan 12, 2025

@logancyang I added the temporary fix for editing message as what we discussed on Friday. Not perfect but at least make it clear whether context is used in different scenario. The editing message has other issues, and I'd like to fix them in a separate PR.

2025-01-12 11 53 36

@logancyang
Copy link
Owner

Nice! I tested non-activenote context and they are working, but seems the active note badge doesn't get included?
SCR-20250112-nxxx

@zeroliu
Copy link
Collaborator Author

zeroliu commented Jan 13, 2025

Nice! I tested non-activenote context and they are working, but seems the active note badge doesn't get included? SCR-20250112-nxxx

what is this active note feature? I thought you have to add {activeNote} keyword but I don't see it in your message

@logancyang
Copy link
Owner

@zeroliu It's just the active note added to context menu manually
notebadge

@zeroliu zeroliu force-pushed the zero/include-text-context branch 2 times, most recently from 7f0ae7c to 352fcd8 Compare January 14, 2025 05:35
@zeroliu
Copy link
Collaborator Author

zeroliu commented Jan 14, 2025

@logancyang the current note issue is fixed. Although I'm not sure about the value of having a separate active note. It would simplify the logic a lot if we only have context notes. Food for thoughts.

2025-01-13 21 35 52

@logancyang
Copy link
Owner

Pinged you in discord. There's a new issue where additional context in chat input don't get preserved after hitting send, only active note is preserved.

@zeroliu zeroliu force-pushed the zero/include-text-context branch from 352fcd8 to 8cdbee4 Compare January 14, 2025 07:04
Copy link
Owner

@logancyang logancyang left a comment

Choose a reason for hiding this comment

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

LGTM! Just as a note, in the follow up PRs we need to address

  1. Edit with context (better design of the chat message object)
  2. "Save as Note" and "Load" with context references

@logancyang logancyang merged commit 62aa100 into logancyang:master Jan 14, 2025
2 checks passed
@logancyang logancyang mentioned this pull request Jan 16, 2025
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.

2 participants