Prevent duplicte click events #1522
Conversation
WalkthroughThe changes involve replacing Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HomeView
participant ProjectService
User->>HomeView: Click delete button for a project
HomeView->>HomeView: Evaluate if project is loading
alt Not loading
HomeView->>ProjectService: deleteProject(project.name)
else Loading active
HomeView-->>User: Ignore delete action
end
User->>HomeView: Click create example project button
HomeView->>HomeView: Check createProjectLoading state
alt Creation allowed
HomeView->>ProjectService: createExampleProject()
else Creation blocked
HomeView-->>User: Ignore create action
end
sequenceDiagram
participant User
participant ServerView
participant ProjectService
User->>ServerView: Click project list item
ServerView->>ServerView: Determine loading state & check if downloading
alt Not loading
ServerView->>ServerView: Check if project exists (matchesProject)
alt Project already exists
ServerView-->>User: No download initiated
else Project does not exist
ServerView->>ProjectService: downloadCrdtProject(project)
ProjectService-->>ServerView: Download successful
ServerView->>ServerView: Add project to localProjects
ServerView->>ServerView: Delay resetting downloading flag (setTimeout)
end
else Loading active
ServerView-->>User: Ignore download action
end
sequenceDiagram
participant User
participant SandboxView
User->>SandboxView: Click Increment Async button
SandboxView->>SandboxView: Check loading state
alt Not loading
SandboxView->>SandboxView: Set loading to true
SandboxView->>SandboxView: Call incrementAsync()
SandboxView-->>User: Show loading feedback
else Loading active
SandboxView-->>User: Ignore increment action
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (6)
🔇 Additional comments (6)
🪧 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: 0
🧹 Nitpick comments (1)
frontend/viewer/src/home/Server.svelte (1)
38-41: Consider a more robust approach than setTimeout.While the timeout effectively prevents immediate re-triggering of downloads, it's a bit of a workaround. Consider using Svelte's built-in reactivity or a more explicit state management approach.
- setTimeout(() => { - downloading = ''; - }, 100); + // Allow UI to update before clearing download state + await new Promise(resolve => requestAnimationFrame(resolve)); + downloading = '';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/viewer/src/home/HomeView.svelte(2 hunks)frontend/viewer/src/home/Server.svelte(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: GHA integration tests / playwright
- GitHub Check: GHA integration tests / dotnet
- GitHub Check: Build FW Lite and run tests
🔇 Additional comments (8)
frontend/viewer/src/home/HomeView.svelte (4)
150-150: Improved loading state tracking for better UX.Using a scoped constant to track the loading state for each project is a good practice as it prevents duplication of the condition check throughout the template.
155-155: Correctly propagating loading state to the UI component.Passing the loading state to the ListItem component ensures visual feedback to users during deletion operations, which improves UX by indicating that an action is in progress.
160-161: Good guard against duplicate delete operations.This check successfully prevents multiple deletion operations if a user clicks rapidly or repeatedly on the delete button while a deletion is already in progress, addressing the core issue mentioned in the PR.
178-182: Effective prevention of duplicate project creation operations.The condition in the click handler prevents users from triggering multiple project creation operations while one is already in progress. Additionally, the loading state is correctly applied to the UI to provide visual feedback.
frontend/viewer/src/home/Server.svelte (4)
26-27: Good optimization to prevent redundant downloads.The early return if the project already exists in local projects prevents unnecessary network requests and potential data inconsistencies. This is an important check for preventing duplicate operations.
32-35: Proper local state update after successful download.Adding the downloaded project to localProjects ensures the UI is immediately updated after a successful download, improving the user experience by providing immediate feedback without requiring a full refresh.
106-107: Effective loading state tracking for project downloads.Similar to the HomeView component, using a local constant for tracking the loading state makes the code more readable and prevents repetition of the condition.
109-111: Complete solution for preventing duplicate downloads.This implementation achieves three important things:
- Disables pointer events during loading, preventing accidental clicks
- Guards the download function with a loading check to prevent duplicate operations
- Provides visual feedback to users through the loading prop
This comprehensively addresses the core issue described in the PR objectives.
… for async triggers
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/viewer/src/lib/sandbox/Sandbox.svelte(3 hunks)frontend/viewer/src/lib/services/service-provider.ts(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Build API / publish-api
- GitHub Check: Build UI / publish-ui
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: check-and-lint
- GitHub Check: Build FW Lite and run tests
- GitHub Check: Analyze (csharp)
🔇 Additional comments (6)
frontend/viewer/src/lib/sandbox/Sandbox.svelte (4)
16-16: Good addition of delay utility.The import of the delay utility function from the time utility module is an appropriate dependency for implementing the asynchronous behavior.
52-53: Well-structured state management.Clear state variables for tracking count and loading status. This pattern aligns with the PR objective of preventing duplicate clicks by maintaining loading state.
54-59: Good pattern for preventing duplicate actions.This async function correctly implements a loading state pattern that:
- Sets loading state to true before operation
- Performs the async operation
- Updates the count
- Resets loading state afterward
This pattern effectively prevents duplicate actions during async operations, which aligns perfectly with the PR objective to "Prevent duplicate click events".
100-108: Effective UI implementation with loading state.The button implementation properly:
- Disables the button when loading is true
- Displays loading state visually (via the loading prop)
- Triggers the async action on click
- Shows the current count
This demonstrates a reusable pattern that could be applied to solve the issue with Download button events mentioned in the PR objectives.
frontend/viewer/src/lib/services/service-provider.ts (2)
20-20: Good addition of IFwEvent importAdding this type import enables proper type checking for the newly implemented Promise return type in nextEventAsync.
41-48: Default implementation helps prevent duplicate eventsGood approach to provide a default no-op implementation for JsEventListener. This aligns with the PR objective to prevent duplicate click events.
There was a problem hiding this comment.
alright so I played with this a bit, we can disable buttons and that will prevent them from firing the click event, the style change also indicates that to the user which I think is helpful.
Unfortunately that doesn't work for ListItem since it's an <li> which does not support the disabled attribute. My first thought was that we should change that... maybe we can make it a button instead? I'm not sure, but I'm wishing we had ShadCN right now and we could just change it.
| dispatch('refreshAll'); | ||
| } finally { | ||
| downloading = ''; | ||
| setTimeout(() => { | ||
| downloading = ''; |
There was a problem hiding this comment.
why the timeout? also while thinking about issues it could cause I realized that if you were to click download on 2 different projects, only one would show as downloading... We should just pull these into their own component then each project has it's own downloading state associated with the component.
Resolves #1494
Wow, this is messy. I'm not sure what to think.
On the Download button, click events while a project is downloading get queued up and triggered after the download is finished. So...that's why there's so much weird code in there.
This is not the way, but, I'm not quite sure what is. Probably different async context management in .NET?
Summary by CodeRabbit
New Features
Bug Fixes