Undo: express delay constants in seconds

[ujjwal-devzy]

UNDO_DELAY and ACCESSIBLE_UNDO_DELAY were defined as raw millisecond literals (3000L / 15000L), which took a moment of mental math to read as human time. Express them in seconds and convert to milliseconds at the point where kotlinx.coroutines' delay() needs it.

Also tidies up a couple of similarly-shaped hardcoded millisecond duration constants in DefaultMetricsStorage to be defined in seconds for the same readability reason.

Pull Request checklist

• Tests: This PR includes thorough tests or an explanation of why it does not
• Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
• Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features. In addition, it includes a screenshot of a successful accessibility scan to ensure no new defects are added to the product.

QA

• QA Needed

To download an APK when reviewing a PR (after all CI tasks finished running):

1. Click on Checks at the top of the PR page.
2. Click on the firefoxci-taskcluster group on the left to expand all tasks.
3. Click on the build-debug task.
4. Click on View task in Taskcluster in the new DETAILS section.
5. The APK links should be on the right side of the screen, named for each CPU architecture.

GitHub Automation

Used by GitHub Actions.

[neatcod-simulator-dev]

⚠️ Issues Identified — 1 High = 1 Total

Severity	File	Description
🟠 High	…/utils/Undo.kt	getUndoDelay() returns seconds but consumer expects millisec

📖 Walkthrough

This refactoring improves code readability by defining delay constants in seconds rather than milliseconds. The MetricsStorage and Undo utility classes now express timing values in human-readable seconds, then convert to milliseconds where needed. New unit tests verify the accessibility-aware undo delay behavior, ensuring the system correctly returns different timeout values based on accessibility settings while maintaining the same runtime behavior.

🔀 Sequence

sequenceDiagram
    participant App
    participant Undo
    participant AccessibilitySettings
    participant MetricsStorage
    App->>Undo: Request undo delay
    Undo->>AccessibilitySettings: Check accessibility enabled
    AccessibilitySettings-->>Undo: Return setting status
    Undo->>Undo: Calculate delay (seconds → ms)
    Undo-->>App: Return delay in milliseconds
    App->>MetricsStorage: Store metrics with delay
    MetricsStorage->>MetricsStorage: Convert seconds to ms

Loading

📂 File Changes

📊 Changes by Category (1 category)

⏱️ Accessibility & Timing Configuration

Refactors delay constants to use seconds as base unit for improved readability and maintainability

Files	Summary
**app/src/main/java/org/mozilla/fenix/components/metrics/MetricsStorage.kt app/src/main/java/org/mozilla/fenix/utils/Undo.kt	** Refactors delay constants to be defined in seconds, then converted to milliseconds for improved readability.
**app/src/test/java/org/mozilla/fenix/utils/UndoTest.kt	** Adds unit tests for undo delay constants returning values in seconds based on accessibility settings.

[neatcod-simulator-dev]

Review complete

DevzyAi finished this review for this commit. Feedback is in the inline review comments on this diff.

Commits

Files that changed from the base of the PR and between 9f12e38 and 3a98b0d commits.

Files selected (3)

• app/src/main/java/org/mozilla/fenix/components/metrics/MetricsStorage.kt (2)
• app/src/main/java/org/mozilla/fenix/utils/Undo.kt (2)
• app/src/test/java/org/mozilla/fenix/utils/UndoTest.kt (1)

Review comments generated (1)

• Review: 1
• LGTM: 0

---

Tips

Chat with DevzyAi Bot (@DevzyAi)

• Reply on review comments left by this bot to ask follow-up questions. A review comment is a comment on a diff or a file.
• Invite the bot into a review comment chain by tagging @DevzyAi in a reply.

See More

Interact with @DevzyAi in any bot review thread (Files changed tab):

Command	Description
@DevzyAi explain	Get a detailed explanation of the code or issue
@DevzyAi fix	Generate a code fix suggestion
@DevzyAi suggest	Get alternative implementations
@DevzyAi ignore	Mark this as a false positive
@DevzyAi review	Trigger a full PR review (overrides ignore)
@DevzyAi test	Generate unit tests for file(s)
@DevzyAi help	Show this help message

Code suggestions

• The bot may make code suggestions, but please review them carefully before committing since the line number ranges may be misaligned.
• You can edit the comment made by the bot and manually tweak the suggestion if it is slightly off.

Pausing incremental reviews

• Add @DevzyAi: ignore anywhere in the PR description to pause further reviews from the bot.

---

Models: code-review → claude-sonnet-4-5-20250929 · summary → claude-haiku-4-5-20251001

[neatcod-simulator-dev]

🟠 High

getUndoDelay() returns seconds but consumer expects milliseconds

The function getUndoDelay() now returns raw seconds (3L or 15L) instead of milliseconds. The conversion TimeUnit.SECONDS.toMillis() happens only inside allowUndo() at line 88, but getUndoDelay() is also called from Core.kt:246 where it's passed to UndoMiddleware(context.getUndoDelay()). UndoMiddleware will receive 3 or 15 instead of 3000 or 15000, causing undo operations to expire almost immediately (3-15ms instead of 3-15 seconds).

Fix: Either (1) make getUndoDelay() return milliseconds by applying the conversion inside it, or (2) update UndoMiddleware to accept seconds and do its own conversion.

🤖 AI Fix Prompt - Copy this into your AI coding agent

VALIDATION
Open app/src/main/java/org/mozilla/fenix/utils/Undo.kt and app/src/main/java/org/mozilla/fenix/Core.kt. Confirm that getUndoDelay() returns 3L or 15L (seconds) and that Core.kt passes this value directly to UndoMiddleware without converting to milliseconds. If UndoMiddleware already handles seconds internally or if the conversion happens elsewhere, stop here.

PROBLEM & LOCATION
In Undo.kt, the function getUndoDelay() returns raw seconds (3L or 15L). In Core.kt around line 246, this value is passed to UndoMiddleware(context.getUndoDelay()). UndoMiddleware expects milliseconds, so it will receive 3 or 15 instead of 3000 or 15000, causing undo timeouts to expire in milliseconds instead of seconds.

FIX
Change getUndoDelay() to return milliseconds by applying TimeUnit.SECONDS.toMillis() before returning. The function should return 3000L when accessibility is enabled and 15000L otherwise. Remove the duplicate conversion inside allowUndo() since getUndoDelay() will now return milliseconds directly.

VERIFY
Run tests that exercise undo functionality, especially any tests covering UndoMiddleware behavior and timeout expiration. Check that undo operations in the UI wait the full 3 or 15 seconds before expiring.

---

↑ Back to Summary