-
-
Notifications
You must be signed in to change notification settings - Fork 85
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
Added easy label creation to multiselect #425
Conversation
WalkthroughThe pull request introduces enhancements to the Multiselect component, focusing on improving label creation and management. The changes primarily involve modifying the dropdown rendering logic, simplifying prop management, and adding a new feature that allows users to create labels dynamically during item creation. Additionally, a WebSocket connection configuration update has been made to support different development environments. Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Security Recommendations
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🔇 Additional comments (7)
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 (
|
Currently creating a label closes the multiselect dropdown, in theory there is also a race condition if the user creates a label and then quickly submits it, its also not especially clear that a label is being created not sure how much we care about these issues though? @tankerkiller125 @katosdev |
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.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🧹 Nitpick comments (3)
frontend/composables/use-server-events.ts (1)
21-23
: Use environment-based configuration for clarity and security
Having the dynamic replacement for the port in development mode is good for local testing. However, consider reading the environment configuration (e.g., from environment variables or a separate config file) so as to avoid potential confusion and to ensure the port isn't inadvertently exposed or changed in production.frontend/components/Form/Multiselect.vue (2)
23-23
: Dropdown styling
Thez-[9999]
ensures the dropdown stays above other elements. Just confirm that this high z-index won't introduce weird stacking issues in complex UIs.
82-82
: Search filtering
Filtering byitem.name.toLowerCase()
is straightforward. Be mindful of items that may have additional relevant fields (e.g., synonyms) in the future.
🛑 Comments failed to post (1)
frontend/components/Form/Multiselect.vue (1)
106-119:
⚠️ Potential issueItem creation with user-specified name
The asynchronous call toapi.labels.create
followed by updating the local array is correct. Validate user input to avoid any potential injection or unwanted characters, particularly if these labels appear in any shared or public context.[security_recommendation]
+ // Example code snippet + function sanitizeName(name: string): string { + return name.replace(/[^\w\s-]/g, ""); + } + async function createAndAdd(name: string) { // ... }Committable suggestion skipped: line range outside the PR's diff.
I also removed the generalisation and made it specific to labels, this makes it a lot more readable and if we want to have a multi select for something else we can always copy it. |
I'm perfectly fine with a race condition as long as it's not something drastic that happens basically every time unless you wait 3 minutes or something stupid. Generally I like to avoid them, but if it's something that should happen basically instantly, I'm not too worried about it. |
What type of PR is this?
What this PR does / why we need it:
Adds easy label creation to the label mutliselect component.
Also fixes the websocket connection in dev mode as this is needed for the easy label creation to work fully.
Which issue(s) this PR fixes:
Fixes #187
Summary by CodeRabbit
New Features
Refactor