Feat: support managing predefined prompts with editing and replacement" #454
Conversation
… after editing, it will replace the original prompt. Otherwise, it will create a new prompt.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@tanchekwei Could you give a review? I believe you must have some reason to prevent editing the predefined prompts. |
Added comment. If a user wants to edit a predefined prompt, currently they can pin a prompt (which will add it to local storage) and then edit it. |
|
@sunner, do you think we need to add another button, Duplicate button, for users to duplicate a predefined prompt and let them click the Edit button to modify it? Or, should we go with @qcgm1978’s approach and simply use the current Edit button for both duplicate and edit actions? If we choose to add a Duplicate button, users will need to click two buttons to edit a predefined prompt: first, they click the Duplicate button, and then they click the Edit button. This approach is clearer. if we use the Edit button for both actions, users will only need to click it once to edit the prompt. However, the existing edit button already use to edit the current user's prompt, same button with different behaviors may lead to confusion. What do you think? |
|
Frankly speaking, I prefer an empty customized prompt library. The owner can choose to insert/edit/delete prompts, and import bulk prompts from other sources. |
| <v-btn | ||
| flat | ||
| size="x-small" | ||
| icon="mdi-delete-outline" | ||
| @click="deletePrompt(item.raw)" | ||
| v-if="item.raw.index >= 0" | ||
| ></v-btn> | ||
| ></v-btn> |
There was a problem hiding this comment.
This change is not related to the topic.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe changes involve updates to the rendering logic and state management within the application. In Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
src/components/PromptModal.vue (2)
187-189: LGTM: Effective duplicate prevention logic.The introduction of
userPromptsTitlesand the modifieddefaultPromptscomputation effectively prevent duplicate prompts from being displayed. This aligns well with the PR objective of allowing users to edit predefined prompts while maintaining a clean prompt list.A minor optimization suggestion:
Consider using a Set for
userPromptsTitlesto improve lookup performance:const userPromptsTitles = new Set(userPrompts.value.map(d => d.title)); const valid = prompts[language.value].filter(d => !userPromptsTitles.has(d.act));This change would slightly improve the performance of the filtering operation, especially for larger lists of prompts.
80-80: Consider UX implications of edit/duplicate functionality.The current implementation allows editing of predefined prompts without an explicit "Duplicate" button. This approach simplifies the UI but may lead to some user confusion, as discussed in the PR comments.
Consider the following suggestions to improve user experience:
- Add a tooltip to the edit button explaining that editing a predefined prompt will create a new user prompt if the title is changed.
- Implement a confirmation dialog when editing a predefined prompt, asking the user if they want to create a new prompt or edit the existing one.
- Add visual differentiation between user prompts and predefined prompts in the list to make it clear which prompts are editable and which will create new prompts when edited.
These changes could help clarify the behavior for users without adding complexity to the UI.
src/store/index.js (2)
320-323: EnhanceeditPromptmutation with a more precise default index condition.The addition of a default index enhances the flexibility of the
editPromptfunction, allowing new prompts to be added at the end of the array when no specific index is provided. However, there are a few points to consider:
The condition
index || state.prompts.lengthmight have unintended consequences ifindexis explicitly set to 0. Consider using a more precise condition likeindex === undefined ? state.prompts.length : index.The indentation of the new line is inconsistent with the rest of the code. Please adjust it to match the surrounding code style.
It would be helpful to add a comment explaining this default index behavior for future maintainers.
Here's a suggested improvement:
editPrompt(state, values) { let { index } = values; - index=index || state.prompts.length + // If index is undefined, add the new prompt at the end of the array + index = index === undefined ? state.prompts.length : index; state.prompts[index] = { ...state.prompts[index], ...values }; },
320-323: Clarify the roles ofeditPromptandaddPromptmutations.The enhancement to
editPromptallows it to add new prompts, which may overlap with the functionality of the existingaddPromptmutation. To maintain clear separation of concerns and prevent potential confusion:
Consider clarifying the distinction between
editPromptandaddPromptin the codebase. You might want to renameeditPromptto something likeupsertPromptto better reflect its dual functionality.Update any components or actions that use
editPromptto ensure they handle the new behavior correctly, especially when dealing with indices.Add appropriate error handling for invalid indices to prevent unintended modifications to the prompts array.
Here's a suggestion for error handling:
editPrompt(state, values) { let { index } = values; // If index is undefined, add the new prompt at the end of the array index = index === undefined ? state.prompts.length : index; // Ensure index is within bounds if (index < 0 || index > state.prompts.length) { console.error(`Invalid prompt index: ${index}`); return; } state.prompts[index] = { ...state.prompts[index], ...values }; },Also, consider updating the related components to handle this new behavior appropriately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/components/PromptModal.vue (2 hunks)
- src/store/index.js (1 hunks)
🔇 Additional comments (1)
src/components/PromptModal.vue (1)
80-80: LGTM: Edit button visibility change aligns with PR objectives.The removal of the
v-ifdirective from the edit button (line 80) allows it to be always visible, which aligns with the PR objective of enabling users to edit predefined prompts. The delete button (line 87) retains its conditional visibility, which is appropriate to prevent users from deleting predefined prompts.Also applies to: 87-87
| let { index } = values; | ||
| index=index || state.prompts.length | ||
| state.prompts[index] = { ...state.prompts[index], ...values }; | ||
| }, |
There was a problem hiding this comment.
Implement title comparison logic to align with PR objectives
There's a discrepancy between the PR objectives and the current implementation of the editPrompt mutation. The PR states that editing a prompt with the same title should replace the original, while a different title should create a new prompt. However, the current implementation doesn't reflect this behavior. To address this:
-
Implement the title comparison logic as described in the PR objectives. This might involve searching for an existing prompt with the same title before deciding whether to edit or create a new one.
-
Add safeguards to prevent unintended data loss or duplication. For example, you could add a confirmation step when overwriting an existing prompt.
-
Consider updating the mutation to handle both editing and creating new prompts based on the title comparison, or split this functionality into separate mutations for clarity.
Here's a suggested implementation that aligns with the PR objectives:
editPrompt(state, values) {
const { title, ...otherValues } = values;
const existingIndex = state.prompts.findIndex(prompt => prompt.title === title);
if (existingIndex !== -1) {
// If a prompt with the same title exists, update it
state.prompts[existingIndex] = { ...state.prompts[existingIndex], ...otherValues };
} else {
// If no prompt with the same title exists, create a new one
state.prompts.push({ title, ...otherValues });
}
},This implementation ensures that the behavior matches the PR objectives and prevents unintended overwrites or duplications.
PeterDaveHello
changed the title
|
@qcgm1978 Hi! Just checking if you still want to continue with this project. Let me know if you have other thoughts! 😊 |
I haven't been following the project for a while now, so I don't recall well what the changes here are about, and it's possible the approach back then was inadequate. You should feel more confident to refine it. |
There was a problem hiding this comment.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no suggestions.
Comments skipped due to low confidence (2)
src/store/index.js:321
- The assignment
index = index || state.prompts.lengthcan lead to unintended behavior ifindexis0, as0is a falsy value in JavaScript. Consider using a more explicit check forundefinedornullvalues.
index=index || state.prompts.length
src/components/PromptModal.vue:188
- [nitpick] The variable name 'valid' is ambiguous. It should be renamed to 'filteredPrompts' for better clarity.
const valid = prompts[language.value].filter(d => !userPromptsTitles.includes(d.act));
… after editing, it will replace the original prompt. Otherwise, it will create a new prompt.
Summary by CodeRabbit
New Features
Bug Fixes