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

fix(web): Handle duplicate library settings gracefully #6950

Merged
merged 6 commits into from
Feb 9, 2024

Conversation

etnoy
Copy link
Contributor

@etnoy etnoy commented Feb 6, 2024

Don't allow adding duplicate import paths. Disable save button if input is empty or duplicate. Add error text if duplicate path.
image

Add text saying import paths are empty
image

Also, the same thing for exclusion patterns

Copy link

cloudflare-workers-and-pages bot commented Feb 6, 2024

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 80a6754
Status:⚡️  Build in progress...

View logs

@etnoy etnoy changed the title fix(web): Handle duplicate import paths fix(web): Handle duplicate library settings gracefully Feb 6, 2024
@etnoy etnoy linked an issue Feb 6, 2024 that may be closed by this pull request
3 tasks
Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@danieldietzler danieldietzler left a comment

Choose a reason for hiding this comment

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

Nice

Comment on lines +51 to +55
// Check so that exclusion pattern isn't duplicated
if (!library.exclusionPatterns.includes(exclusionPatternToAdd)) {
library.exclusionPatterns.push(exclusionPatternToAdd);
exclusionPatterns = library.exclusionPatterns;
}
Copy link
Member

Choose a reason for hiding this comment

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

We're doing that check in 3 different places or so. Not sure if I'm a huge fan of this...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We check both in the modal for add/edit and in the handle function. I think both are needed. I'll likely also add a check in the server to prevent duplicates submitted via API.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another option is to not do anything client side and auto dedupe server side instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We still need to dedupe client side because of the client-side bugs in #6694. I'll address the server side in another PR

@etnoy etnoy enabled auto-merge (squash) February 9, 2024 00:06
@etnoy etnoy merged commit b67fddf into main Feb 9, 2024
23 of 24 checks passed
@etnoy etnoy deleted the fix/handle-duplicate-import-paths branch February 9, 2024 00:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Web UI handling of duplicate library import paths
3 participants