-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Fix: Remove applied rules from chat history #7343
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
Fix: Remove applied rules from chat history #7343
Conversation
|
I have read the CLA Document and I hereby sign the CLA agrabauskas seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. |
| const handleOpen = async () => { | ||
| if (rule.slug) { | ||
| void ideMessenger.request("controlPlane/openUrl", { | ||
| path: `${rule.slug}/new-version`, | ||
| orgSlug: undefined, | ||
| }); | ||
| } else if (rule.ruleFile) { | ||
| ideMessenger.post("openFile", { | ||
| path: rule.ruleFile, | ||
| }); | ||
| } else if ( | ||
| rule.source === "default-chat" || | ||
| rule.source === "default-plan" || | ||
| rule.source === "default-agent" | ||
| ) { | ||
| ideMessenger.post("openUrl", DEFAULT_SYSTEM_MESSAGES_URL); | ||
| } else { | ||
| ideMessenger.post("config/openProfile", { | ||
| profileId: undefined, | ||
| element: { sourceFile: (rule as any).sourceFile }, | ||
| }); | ||
| } | ||
| }; |
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.
This is not ideal, but it's consistent with RuleCard: React.FC<RuleCardProps> on click will just open the rules files in the editor.
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.
@etherandrius could you add some explanation/context in the description?
|
@RomneyDa linked back to the issue |
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.
@etherandrius see nitpick
Also could we create a little util such as openRule that takes a RuleWithSource or AppliedRule so you could share all the rule opening logic between the components?
Overall agree with this, will be nice to reduce UI a bit here too
core/index.d.ts
Outdated
| conversationSummary?: string; | ||
| } | ||
|
|
||
| export interface AppliedRule { |
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.
Can use Omit<RuleWithSource, "rule">
| const RuleCard: React.FC<RuleCardProps> = ({ rule }) => { | ||
| const dispatch = useAppDispatch(); | ||
| const ideMessenger = useContext(IdeMessengerContext); | ||
| const mode = useAppSelector((store) => store.session.mode); |
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.
unused.
| } | ||
|
|
||
| export const openRules = async (rule: Omit<RuleWithSource, "rule">) => { | ||
| const ideMessenger = useContext(IdeMessengerContext); |
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.
ideMessenger this was define outside the function before. I assume the useContext(IdeMessengerContext) call is basically free and we can call it inside the function.
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.
Will need to pass ideMessenger for this one since can't use hooks outside of top level of component
| rules: appliedRules.map((rule) => ({ | ||
| id: getRuleId(rule), | ||
| rule: rule.rule, | ||
| rule: "", // TODO: remove rule key from type. The contents should be present in the prompt |
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.
a bit funky.
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.
Any specific reason to remove rules from dev data? Have you had issues with verbose dev data?
| appliedRules: rules.map(fullRule => { | ||
| const { rule, ...rest } = fullRule // destruct rule key | ||
| return rest; | ||
| }), |
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.
A bit funky as well.
Ideally this method would not take availableRules: Omit<RuleWithSource, "rule">[] and read rules from the file.
This would also fix the issue of rules sometimes being outdated <- maybe a FLUP.
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.
This function could still just return RuleWithSource, shouldn't effect storage if omitted at the point of redux
note that config should reload if a rule file is updated, so should be up to date in most cases
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.
- Main blocking feedback is to remove use of useContext in utility function
- create new type with Omit and pass the new type around to prevent duplicate
Omits - Leave rule content on dev data unless specific issues with dev data storage - let me know your thoughts on this?
| } | ||
|
|
||
| export const openRules = async (rule: Omit<RuleWithSource, "rule">) => { | ||
| const ideMessenger = useContext(IdeMessengerContext); |
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.
Will need to pass ideMessenger for this one since can't use hooks outside of top level of component
| isGatheringContext?: boolean; | ||
| reasoning?: Reasoning; | ||
| appliedRules?: RuleWithSource[]; | ||
| appliedRules?: Omit<RuleWithSource, "rule">[]; |
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.
Can keep the AppliedRule type and just say type AppliedRule = Omit<RuleWithSource, "rule"> to prevent having Omits all over
| appliedRules: rules.map(fullRule => { | ||
| const { rule, ...rest } = fullRule // destruct rule key | ||
| return rest; | ||
| }), |
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.
This function could still just return RuleWithSource, shouldn't effect storage if omitted at the point of redux
note that config should reload if a rule file is updated, so should be up to date in most cases
| rule: RuleWithSource; | ||
| } | ||
|
|
||
| export const openRules = async (rule: Omit<RuleWithSource, "rule">) => { |
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.
could take RuleWithSource | AppliedRule
| rules: appliedRules.map((rule) => ({ | ||
| id: getRuleId(rule), | ||
| rule: rule.rule, | ||
| rule: "", // TODO: remove rule key from type. The contents should be present in the prompt |
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.
Any specific reason to remove rules from dev data? Have you had issues with verbose dev data?
|
@etherandrius bump, note failing prettier/tests! |
|
@etherandrius bump! |
|
@etherandrius note that the root localStorage issue will also be solved by #7944 |
|
I think this is a good change, since this is a bit stale re-worked here! |
Description
#7337