[PM-28835] Added validations to prevent duplicate press on buttons #6209

aj-rosado · 2025-11-28T10:58:44Z

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-28835

📔 Objective

On some scenarios if the user tried to multi press the button he can create multiple entries by mistake. This address that issue on item, folder and send creation and also fixes multiple calls to remove password.

⏰ Reminders before review

Contributor guidelines followed
All formatters and local linters executed and passed
Written new unit and / or integration tests where applicable
Protected functional changes with optionality (feature flags)
Used internationalization (i18n) for all UI strings
CI builds passed
Communicated to DevOps any deployment requirements
Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

👍 (:+1:) or similar for great changes
📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
❓ (:question:) for questions
🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
🎨 (:art:) for suggestions / improvements
❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
⛏ (:pick:) for minor or nitpick changes

claude · 2025-11-28T11:00:04Z

Claude finished @aj-rosado's task —— View job

Code Review Complete

Tasks

Examine current PR changes
Launch code review agent
Analyze implementation approach
Provide comprehensive review feedback

Review Summary

Status: ❌ Changes Requested

All previous review comments from David have been successfully addressed. However, I've identified a critical race condition in the throttling implementation that needs to be fixed before merge, along with some API design improvements.

Critical Issues

1. Race Condition in Throttle Implementation ❌ CRITICAL

Location: ui/src/main/kotlin/com/bitwarden/ui/platform/components/util/ThrottledClick.kt:27-36

The current implementation has a race condition where multiple rapid clicks can bypass throttling:

var isEnabled by remember { mutableStateOf(value = true) }
return {
    coroutineScope.launch {
        if (isEnabled) {           // ← Multiple coroutines can pass this check
            isEnabled = false      // ← before any set it to false
            onClick()
            delay(timeMillis = delayMs)
            isEnabled = true
        }
    }
}

Problem: The check-then-set pattern is not atomic. If a user clicks twice within microseconds, both clicks might see isEnabled = true and both will execute, defeating the purpose of throttling.

Impact: This could still allow duplicate save operations, which is what this PR aims to prevent.

Recommended Fix: Use Mutex for proper synchronization:

@Composable
fun throttledClick(
    onClick: () -> Unit,
    delayMs: Long = 300,
    coroutineScope: CoroutineScope = rememberCoroutineScope(),
): () -> Unit {
    val mutex = remember { Mutex() }
    return {
        coroutineScope.launch {
            if (mutex.tryLock()) {
                try {
                    onClick()
                    delay(timeMillis = delayMs)
                } finally {
                    mutex.unlock()
                }
            }
        }
    }
}

Don't forget to add: import kotlinx.coroutines.sync.Mutex

2. Parameter Ordering Violates Kotlin Conventions ⚠️ MODERATE

Location: ui/src/main/kotlin/com/bitwarden/ui/platform/components/util/ThrottledClick.kt:22-26

The function signature places onClick as the last parameter, but Kotlin conventions dictate that required parameters should come before optional ones.

Current:

fun throttledClick(
    coroutineScope: CoroutineScope = rememberCoroutineScope(),
    delayMs: Long = 300,
    onClick: () -> Unit,  // ← Should be first
): () -> Unit

Expected:

fun throttledClick(
    onClick: () -> Unit,  // ← Required parameter first
    delayMs: Long = 300,
    coroutineScope: CoroutineScope = rememberCoroutineScope(),
): () -> Unit

Benefits: Cleaner call sites without redundant naming:

// Current (verbose)
onClick = throttledClick(onClick = onClick)

// After fix (cleaner)
onClick = throttledClick(onClick)

3. Missing Test Coverage ⚠️ MODERATE

No tests exist for the throttledClick function despite it being critical functionality.

Required test scenarios:

Verify first click executes immediately
Verify rapid clicks within delay window are ignored
Verify clicks after delay window execute successfully
Verify correct delay duration (300ms)
Verify behavior with custom delay values

Recommendation: Add ThrottledClickTest.kt in ui/src/test/kotlin/com/bitwarden/ui/platform/components/util/

Positive Observations ✅

All previous review comments addressed: File renamed, unnecessary changes removed, formatting fixed
Clean module placement: Correctly placed in :ui module under platform/components/util
Good documentation: Includes proper KDoc with parameter descriptions
Consistent integration: Applied uniformly across all three button types
Sensible defaults: 300ms aligns with common UX practices for double-click prevention
Non-invasive approach: Minimal changes to existing button components

Additional Recommendations

Documentation Enhancement

Add usage example and clarify behavior:

/**
 * Creates a throttled click handler that prevents rapid successive clicks.
 *
 * This implements "first-click-then-delay" throttling: the first click executes immediately,
 * then subsequent clicks are ignored for [delayMs] milliseconds.
 *
 * Example:
 * ```
 * Button(onClick = throttledClick(onClick = { saveItem() })) {
 *     Text("Save")
 * }
 * ```
 *
 * @param onClick The action to perform when clicked.
 * @param delayMs The minimum time in milliseconds between clicks (default: 300ms).
 * @param coroutineScope The coroutine scope for launching click handlers.
 * @return A throttled click handler function.
 */

Consider Icon Buttons

Verify whether icon buttons (BitwardenStandardIconButton, BitwardenTonalIconButton, etc.) also need throttling for consistency.

Security & Performance

✅ No security issues identified - The throttling operates purely in the UI layer and doesn't affect the zero-knowledge architecture.

✅ Performance acceptable - Coroutine overhead is minimal, and the memory footprint (one Boolean per button) is negligible.

Overall Assessment

Status: ❌ NOT READY FOR MERGE

Reasoning: The race condition in the core throttling logic is a critical correctness issue. While it may not manifest frequently in normal usage, it defeats the purpose of this PR under heavy rapid clicking.

Effort to Fix: ~3-4 hours

Race condition fix: 30-60 minutes
Parameter reordering: 15 minutes
Test coverage: 1-2 hours

Recommendation: Address the race condition and parameter ordering before merge. Test coverage is highly recommended given this is critical functionality for preventing duplicate operations.

claude · 2025-11-28T11:01:54Z

...n/com/x8bit/bitwarden/ui/platform/feature/settings/folders/addedit/FolderAddEditViewModel.kt

            return@onContent
        }

+        if (state.dialog is FolderAddEditState.DialogState.Loading) return@onContent


💭 Consider guard clause placement for consistency

Details

The loading state guard is placed between validation and state update. While functionally correct, the other ViewModels in this PR (AddEditSendViewModel:561, VaultAddEditViewModel:403) place the guard immediately before state mutation, after all validations are complete.

Current pattern:

if (content.folderName.isEmpty()) { // validation error return@onContent } if (state.dialog is FolderAddEditState.DialogState.Loading) return@onContent // Line 106 mutableStateFlow.update { ... }

Consistent pattern (optional):

if (content.folderName.isEmpty()) { // validation error return@onContent } // Any other validations... if (state.dialog is FolderAddEditState.DialogState.Loading) return@onContent mutableStateFlow.update { ... }

This is a minor stylistic point - the current implementation is correct and safe.

github-actions · 2025-11-28T11:06:12Z

Checkmarx One – Scan Summary & Details – 2f0addeb-221b-40c4-abf9-d244d2d481a7

Great job! No new security vulnerabilities introduced in this pull request

codecov · 2025-11-28T16:22:49Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.38%. Comparing base (8bdbccd) to head (94be258).
⚠️ Report is 10 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6209      +/-   ##
==========================================
- Coverage   85.41%   85.38%   -0.03%     
==========================================
  Files         755      755              
  Lines       54114    54109       -5     
  Branches     7803     7795       -8     
==========================================
- Hits        46220    46203      -17     
- Misses       5180     5195      +15     
+ Partials     2714     2711       -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

david-livefront · 2025-12-01T16:30:21Z

...m/x8bit/bitwarden/ui/platform/feature/settings/folders/addedit/FolderAddEditViewModelTest.kt

        )
    }

+    @Suppress("MaxLineLength")


This suppression is not needed

david-livefront · 2025-12-01T16:30:41Z

app/src/test/kotlin/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditViewModelTest.kt

        } returns mockProviderCreateCredentialRequest
    }
+
+    @Suppress("MaxLineLength")


We don't need this suppression

david-livefront · 2025-12-01T16:32:17Z

app/src/main/kotlin/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditScreen.kt

                            onClick = remember(viewModel) {
                                { viewModel.trySendAction(VaultAddEditAction.Common.SaveClick) }
                            },
+                            isEnabled = state.dialog !is VaultAddEditState.DialogState.Loading,


Realistically, can't we just say:

isEnabled = state.dialog == null,

david-livefront · 2025-12-01T16:43:40Z

...tlin/com/x8bit/bitwarden/ui/platform/feature/settings/folders/addedit/FolderAddEditScreen.kt

                        onClick = remember(viewModel) {
                            { viewModel.trySendAction(FolderAddEditAction.SaveClick) }
                        },
+                        isEnabled = state.dialog !is FolderAddEditState.DialogState.Loading,


Thoughts on handling this inside the button? Adding some throttling inside the various BitwardenButtons?

I have tested this and just before navigating back I am able to create a duplicate item if I am spamming the save button.
On current main I can even create two duplicates. The throttle mitigates this a little bit but is not as consistent as current approach.

However... I believe throttle would solve the real world scenarios where this happens

🤔 should we keep this for save operations (in order to prevent duplicates) but still add throttling for all the buttons?

If the throttling works, I don't think we need to add this extra code, right?

I was playing around with this and something like this should work and we can generically apply it to all of the button components:

@Composable fun throttledClick( coroutineScope: CoroutineScope = rememberCoroutineScope(), delayMs: Long = 300, onClick: () -> Unit, ): () -> Unit { var isEnabled by remember { mutableStateOf(value = true) } return { coroutineScope.launch { if (isEnabled) { isEnabled = false onClick() delay(timeMillis = delayMs) isEnabled = true } } } }

It does not completely fix it, I have been able to create a duplicate. But unless someone intentionally tries to do it should be fine.

👍

If this bug ever re-appears, we can always increase the delay, but I am hesitant to go much higher than ~300

ui/src/main/kotlin/com/bitwarden/ui/platform/components/util/ThrottledClickHandler.kt

ui/src/main/kotlin/com/bitwarden/ui/platform/components/util/ThrottledClick.kt

ui/src/main/kotlin/com/bitwarden/ui/platform/components/util/ThrottledClickHandler.kt

app/src/main/kotlin/com/x8bit/bitwarden/ui/tools/feature/send/addedit/AddEditSendViewModel.kt

david-livefront · 2025-12-03T15:42:54Z

app/src/main/kotlin/com/x8bit/bitwarden/ui/tools/feature/send/addedit/AddEditSendViewModel.kt

                }
                return@onContent
            }
+


Can we drop these unnecessary changes

david-livefront · 2025-12-03T15:42:58Z

app/src/test/kotlin/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditViewModelTest.kt

            ProviderCreateCredentialRequest.fromBundle(any())
        } returns mockProviderCreateCredentialRequest
    }
+


david-livefront · 2025-12-03T15:44:02Z

ui/src/main/kotlin/com/bitwarden/ui/platform/components/util/ThrottledClickHandler.kt

+ * @param onClick The action to perform when clicked.
+ * @return A throttled click handler function.
+ */
+


Can we drop this newline

david-livefront · 2025-12-03T15:44:48Z

ui/src/main/kotlin/com/bitwarden/ui/platform/components/util/ThrottledClick.kt

@@ -0,0 +1,39 @@
+package com.bitwarden.ui.platform.components.util


Can we rename the file to match the function: ThrottledClick

…o match the function

aj-rosado · 2025-12-03T17:46:10Z

Thank you @david-livefront

Added validations to prevent duplicate press on buttons

7afddc3

aj-rosado requested review from a team, brian-livefront and david-livefront as code owners November 28, 2025 10:58

claude bot reviewed Nov 28, 2025

View reviewed changes

aj-rosado changed the title ~~[PM-28837] Added validations to prevent duplicate press on buttons~~ [PM-28835] Added validations to prevent duplicate press on buttons Nov 28, 2025

remove unnecessary test

0de7310

david-livefront reviewed Dec 1, 2025

View reviewed changes

aj-rosado added 3 commits December 2, 2025 10:41

removed unnecessary suppress

dabbbb4

Add throttle to button click

f841dda

removed isEnabled state check from save buttons

5b4ab93

david-livefront reviewed Dec 2, 2025

View reviewed changes

ui/src/main/kotlin/com/bitwarden/ui/platform/components/util/ThrottledClickHandler.kt Outdated Show resolved Hide resolved

david-livefront reviewed Dec 2, 2025

View reviewed changes

ui/src/main/kotlin/com/bitwarden/ui/platform/components/util/ThrottledClick.kt Show resolved Hide resolved

david-livefront reviewed Dec 2, 2025

View reviewed changes

ui/src/main/kotlin/com/bitwarden/ui/platform/components/util/ThrottledClickHandler.kt Outdated Show resolved Hide resolved

david-livefront reviewed Dec 2, 2025

View reviewed changes

app/src/main/kotlin/com/x8bit/bitwarden/ui/tools/feature/send/addedit/AddEditSendViewModel.kt Outdated Show resolved Hide resolved

aj-rosado added 3 commits December 3, 2025 11:28

Updated throttleClick method

a1b01b8

Removed unnecessary state validations on ViewModel

2ef188e

remove unnecessary test

ad8a557

aj-rosado requested a review from david-livefront December 3, 2025 13:15

david-livefront reviewed Dec 3, 2025

View reviewed changes

Removed unnecessary breaklines and changed ThrottledClick file name t…

94be258

…o match the function

aj-rosado requested a review from david-livefront December 3, 2025 16:07

david-livefront approved these changes Dec 3, 2025

View reviewed changes

aj-rosado added this pull request to the merge queue Dec 3, 2025

Merged via the queue into main with commit 26e7178 Dec 3, 2025
15 checks passed

aj-rosado deleted the PM-28833/save-creates-duplicate-entry branch December 3, 2025 18:20

		@@ -0,0 +1,39 @@
		package com.bitwarden.ui.platform.components.util

[PM-28835] Added validations to prevent duplicate press on buttons #6209

[PM-28835] Added validations to prevent duplicate press on buttons #6209

Uh oh!

Conversation

aj-rosado commented Nov 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎟️ Tracking

📔 Objective

⏰ Reminders before review

🦮 Reviewer guidelines

Uh oh!

claude bot commented Nov 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Complete

Tasks

Review Summary

Critical Issues

1. Race Condition in Throttle Implementation ❌ CRITICAL

2. Parameter Ordering Violates Kotlin Conventions ⚠️ MODERATE

3. Missing Test Coverage ⚠️ MODERATE

Positive Observations ✅

Additional Recommendations

Documentation Enhancement

Consider Icon Buttons

Security & Performance

Overall Assessment

Uh oh!

claude bot Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Great job! No new security vulnerabilities introduced in this pull request

Uh oh!

codecov bot commented Nov 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-livefront Dec 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-livefront Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aj-rosado commented Dec 3, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

aj-rosado commented Nov 28, 2025 •

edited

Loading

claude bot commented Nov 28, 2025 •

edited

Loading

github-actions bot commented Nov 28, 2025 •

edited

Loading

codecov bot commented Nov 28, 2025 •

edited

Loading

david-livefront Dec 1, 2025 •

edited

Loading

david-livefront Dec 2, 2025 •

edited

Loading