-
Notifications
You must be signed in to change notification settings - Fork 2.4k
feat: support inserting current date into assistant prompt #6205
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
Conversation
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.
Important
Looks good to me! 👍
Reviewed everything up to 63d99f7 in 1 minute and 49 seconds. Click for details.
- Reviewed
396lines of code in13files - Skipped
0files when reviewing. - Skipped posting
11draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/containers/dialogs/AddEditAssistant.tsx:321
- Draft comment:
Good addition of the instructions hint. For accessibility, consider linking this hint to the textarea via an aria-describedby attribute. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. web-app/src/hooks/__tests__/useChat.instructions.test.ts:119
- Draft comment:
The test for replacing {{current_date}} using fake timers is comprehensive. Consider adding an extra case where no placeholder exists to ensure idempotency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The test file is new and focused specifically on the placeholder replacement feature. The suggested additional test seems like a reasonable edge case to test, but it's not critical since the main functionality (placeholder replacement) is already well tested. The comment is making a suggestion for improvement rather than pointing out a bug or issue. The comment might be overengineering - if there's no placeholder, there's nothing to replace, so it may not be worth testing. Also, we don't know if handling instructions without placeholders is even an important use case. While the suggested test might be nice to have, it's not critical enough to warrant a comment. The core functionality is already well tested. The comment should be removed as it suggests a nice-to-have but non-essential test case, and we want to keep PR comments focused on more important issues.
3. web-app/src/hooks/useChat.ts:249
- Draft comment:
Nice integration of renderInstructions to process the assistant's instructions. Since renderInstructions handles undefined inputs, this usage is safe. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, as it praises the integration and reassures that the usage is safe. It doesn't provide any actionable feedback or suggestions for improvement.
4. web-app/src/lib/instructionTemplate.ts:20
- Draft comment:
The renderInstructions function cleanly replaces all occurrences of {{current_date}}. Optionally, consider renaming the variable 'rendered' to something like 'processedInstructions' for clearer intent. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
5. web-app/src/locales/en/assistants.json:33
- Draft comment:
New key 'instructionsDateHint' added with appropriate guidance. Ensure that the placeholder syntax {{current_date}} is maintained consistently across all locale files. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to ensure consistency in placeholder syntax across locale files. This falls under the category of asking the author to double-check or ensure something, which is against the rules. Therefore, this comment should be removed.
6. web-app/src/locales/de-DE/assistants.json:33
- Draft comment:
The German translation for 'instructionsDateHint' looks consistent. Verify that users understand the placeholder usage. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify user understanding, which is not specific enough and violates the rule against asking for confirmation or verification of intention. It doesn't provide a specific suggestion or point out a clear issue with the code.
7. web-app/src/locales/id/assistants.json:33
- Draft comment:
The Indonesian translation for 'instructionsDateHint' is correctly added. Consistency across locales is maintained. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, stating that the Indonesian translation is correct and consistent. It doesn't provide any actionable feedback or suggestions for improvement.
8. web-app/src/locales/vn/assistants.json:33
- Draft comment:
The Vietnamese translation for 'instructionsDateHint' is added properly. The placeholder syntax remains intact. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, stating that the translation is added properly and the syntax remains intact. It does not provide any actionable feedback or suggestions for improvement.
9. web-app/src/locales/zh-CN/assistants.json:33
- Draft comment:
The Simplified Chinese translation for 'instructionsDateHint' is correctly added. Ensure consistency with other locale files. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative and asks the PR author to ensure consistency, which is against the rules. It doesn't provide a specific suggestion or point out a specific issue.
10. web-app/src/locales/zh-TW/assistants.json:33
- Draft comment:
The Traditional Chinese translation for 'instructionsDateHint' has been added and is consistent with other locales. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, as it only states that a translation has been added and is consistent with other locales. It does not provide any actionable feedback or suggestions for improvement.
11. web-app/src/utils/formatDate.ts:29
- Draft comment:
The new includeTime flag is implemented correctly. In date-only mode, consider if specifying a timeZone is necessary for consistency, though the current behavior seems intended. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment starts with a purely informative statement about the implementation being correct, which is not allowed. The second part suggests considering the necessity of specifying a timeZone, but it does not make a specific suggestion or point out a potential issue. It also assumes the current behavior is intended, which is against the rules.
Workflow ID: wflow_YlDAyjYE0fUqVIIk
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
63d99f7 to
b77c893
Compare
|
Thanks for the contribution. We've been waiting for this update for a while. 💯 |
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.
LGTM
Describe Your Changes
Adds the ability to insert the current date into the assistant prompt via a
{{current_date}}template variable.Without this ability,
Jan-v1-4B-Q4_K_Mthinks today is October 5, 2023.Fixes Issues
Self Checklist
Important
Adds support for
{{current_date}}template variable in assistant prompts, updatesuseChatandformatDate, and includes tests and localization updates.{{current_date}}template variable support inrenderInstructions()ininstructionTemplate.tsto insert the current date.useChatinuseChat.tsto userenderInstructions()for processing assistant instructions.formatDate()informatDate.tsto support date-only formatting withincludeTimeoption.renderInstructions()ininstructionTemplate.test.tsto verify date replacement.formatDate()informatDate.test.tsto verify date-only formatting.useChat.instructions.test.tsto test instruction rendering with date replacement.assistants.jsonin multiple locales to includeinstructionsDateHintfor using{{current_date}}.This description was created by
for 63d99f7. You can customize this summary. It will automatically update as commits are pushed.