Skip to content

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

Merged
etnoy merged 6 commits into
mainfrom
fix/handle-duplicate-import-paths
Feb 9, 2024
Merged

etnoy merged 6 commits into
mainfrom
fix/handle-duplicate-import-paths

Conversation

@etnoy

@etnoy etnoy commented Feb 6, 2024 •

Copy link
Copy Markdown
Collaborator

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

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Feb 6, 2024 •

Copy link
Copy Markdown

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
2 of 3 tasks

@jrasm91 jrasm91 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Comment thread web/src/lib/components/forms/library-import-path-form.svelte

@danieldietzler danieldietzler left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
Collaborator 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
Copy Markdown
Member

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
Copy Markdown
Collaborator 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
@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