Skip to content

Conversation

@piotr-iohk
Copy link
Collaborator

Description

The PR adds an E2E-only "Enter QR Code String" helper form (mirroring Android) so settings and lnurl e2e specs can inject manual "QR" payloads. As a result the @settings_10 iOS test in the current suite is enabled.

Linked Issues/Tasks

Add any links to GitHub issues or Asana tasks that are relevant to this pull request.

Screenshot / Video

Screenshot 2025-11-25 at 17 51 29 Screenshot 2025-11-25 at 17 51 38

@claude
Copy link

claude bot commented Nov 25, 2025

Code Review: E2E Manual QR Entry Feature

Overview

This PR adds a test-only feature to manually enter QR code strings during E2E testing, mirroring Android functionality. The implementation is clean and well-scoped to E2E builds only.


✅ Strengths

  1. Proper E2E Scoping: The feature is correctly gated behind Env.isE2E checks, ensuring it doesn't appear in production builds.

  2. Consistent Patterns: The implementation follows the existing ScannerManager patterns and reuses the same handleScan logic that other entry methods use (paste, image selection).

  3. Good Accessibility: All new UI elements have appropriate accessibilityIdentifier attributes for E2E test automation.

  4. Code Reuse: The new handleManualEntry method properly delegates to the existing handleScan logic, avoiding duplication.

  5. Proper State Management: Uses @State for local view state (isManualEntryPresented, manualEntry) which is appropriate for this use case.


🔍 Issues & Suggestions

1. Missing @MainActor for State Mutation (Minor)

Location: ScannerManager.swift:237-247

The handleManualEntry method calls onSuccess() which likely mutates view state (isManualEntryPresented, manualEntry), but there's no guarantee it runs on the main actor. While the class is marked @MainActor, the callback should be explicitly dispatched.

Current:

func handleManualEntry(
    _ value: String,
    context: ScannerContext,
    onSuccess: () -> Void
) async {
    let trimmed = value.trimmingCharacters(in: .whitespacesAndNewlines)
    guard !trimmed.isEmpty else { return }

    await handleScan(trimmed, context: context)
    onSuccess()  // ⚠️ May mutate state
}

Suggested Fix:

func handleManualEntry(
    _ value: String,
    context: ScannerContext,
    onSuccess: @MainActor () -> Void
) async {
    let trimmed = value.trimmingCharacters(in: .whitespacesAndNewlines)
    guard !trimmed.isEmpty else { return }

    await handleScan(trimmed, context: context)
    await MainActor.run { onSuccess() }
}

2. Empty String Handling is Silent (Minor)

Location: ScannerManager.swift:243

When the user submits an empty/whitespace-only string, the method silently returns without feedback. This could confuse E2E test developers.

Suggestion: Consider adding a toast or error message for empty input to improve the testing experience.


3. Duplicated Code Between ScannerScreen and ScannerSheet (Minor Refactoring Opportunity)

The manual entry UI and handling logic is duplicated in both ScannerScreen.swift and ScannerSheet.swift. While not critical for E2E-only code, extracting the sheet presentation and handler into a reusable component or view modifier would improve maintainability.

Example Pattern:

extension View {
    @ViewBuilder
    func e2eManualEntrySheet(
        isPresented: Binding<Bool>,
        text: Binding<String>,
        context: ScannerContext,
        scanner: ScannerManager
    ) -> some View {
        if Env.isE2E {
            self.sheet(isPresented: isPresented) {
                ScannerManualEntryPrompt(
                    text: text,
                    onSubmit: { /* ... */ },
                    onCancel: { /* ... */ }
                )
                .presentationDetents([.fraction(0.35)])
                .presentationDragIndicator(.visible)
            }
        } else {
            self
        }
    }
}

4. Inconsistent Button Title Formatting (Very Minor)

Location: ScannerScreen.swift:48 and ScannerSheet.swift:48

The button title "Enter QRCode String" uses inconsistent capitalization ("QRCode" should be "QR Code"). While this is E2E-only, consistent terminology improves readability.


🔒 Security

No security concerns: The feature is properly gated to E2E builds only and doesn't introduce any vulnerabilities.


⚡ Performance

No performance concerns: The implementation is lightweight and uses appropriate async patterns.


🧪 Test Coverage

Note: This PR doesn't include Swift unit tests for the new handleManualEntry method, but since this is E2E infrastructure code that will be exercised by the E2E test suite (particularly @settings_10), this is acceptable.


📝 Code Style

✅ The code follows the project's SwiftUI patterns:

  • Uses @State for local view state
  • Uses .task would be unnecessary here (synchronous operations)
  • Proper component decomposition with ScannerManualEntryPrompt
  • Consistent with existing scanner patterns

Summary

Recommendation: ✅ Approve with minor suggestions

This is a well-implemented E2E testing feature that follows the project's architectural patterns and is properly scoped. The identified issues are minor and don't block approval:

  • Must Fix: None
  • Should Consider: Add @MainActor guarantee for onSuccess callback
  • Nice to Have: Empty string feedback, refactor duplication

Great work enabling the @settings_10 test! 🎉

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds an E2E testing helper that allows manual QR code string entry during automated tests, mirroring existing Android functionality. The feature is gated behind Env.isE2E to ensure it only appears in test builds, not production. This enables the previously disabled @settings_10 iOS E2E test to run.

Key changes:

  • Added manual entry UI components to scanner screens for E2E testing
  • Implemented handleManualEntry method in ScannerManager to process manual input
  • Enabled @settings_10 test by removing exclusion filter in CI workflow

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
Bitkit/Views/Scanner/ScannerSheet.swift Added E2E-only manual QR entry button, state management, and sheet presentation
Bitkit/Views/Scanner/ScannerScreen.swift Added E2E-only manual QR entry button, state management, and sheet presentation (parallel to ScannerSheet)
Bitkit/Views/Scanner/ManualScanPrompt.swift New reusable prompt component for manual QR code string input with proper accessibility identifiers
Bitkit/Managers/ScannerManager.swift Added handleManualEntry method to process manually entered QR strings
Bitkit/Components/TabBar/ScanButton.swift Added accessibility identifier for E2E test targeting
.github/workflows/e2e-tests.yml Removed exclusion of @settings_10 test to enable previously disabled iOS test

@piotr-iohk piotr-iohk force-pushed the test/scan-prompt-test-form branch from 9b4e4b1 to 5256919 Compare November 25, 2025 17:35
@claude
Copy link

claude bot commented Nov 25, 2025

Code Review for PR #242: E2E Test Helper for QR Code String Entry

Overview

This PR adds an E2E-only manual QR code entry form to enable automated testing without requiring actual QR code scanning. This mirrors the Android implementation and enables the @settings_10 iOS test.


Strengths

  1. E2E-only feature flag: Properly gated behind Env.isE2E to ensure this debug feature doesn't appear in production builds.

  2. Consistent implementation: The feature is consistently implemented across both ScannerScreen and ScannerSheet, ensuring comprehensive coverage.

  3. Good accessibility support: Proper use of .accessibilityIdentifier() modifiers throughout for E2E test automation.

  4. Reusable component: ScannerManualEntryPrompt is a well-structured, reusable SwiftUI component.

  5. Code reuse: The handleManualEntry method in ScannerManager properly delegates to the existing handleScan method, avoiding code duplication.


🔍 Code Quality Observations

1. Minor Inconsistency in Button Titles

  • ScannerScreen.swift:48: "Enter QR Code String"
  • ScannerSheet.swift:48: "Enter QRCode String" (missing space)

Recommendation: Standardize to "Enter QR Code String" in both files.

2. Redundant MainActor.run in handleManualEntry

Location: ScannerManager.swift:246-248

await MainActor.run {
    onSuccess()
}

Since ScannerManager is already annotated with @MainActor (line 11), and the onSuccess closure is also marked @MainActor, the explicit MainActor.run is redundant.

Recommendation: Simplify to:

onSuccess()

3. Empty String Handling

Location: ScannerManager.swift:242-243

The empty string guard is good, but it fails silently. Consider providing user feedback when an empty string is submitted.

Recommendation:

guard !trimmed.isEmpty else {
    app?.toast(
        type: .warning,
        title: "Empty Input",
        description: "Please enter a QR code string"
    )
    await MainActor.run { onSuccess() } // Still close the sheet
    return
}

🏗️ Architecture & Best Practices

Follows SwiftUI Patterns

  • Uses @State for local view state (isManualEntryPresented, manualEntry)
  • Properly uses .sheet(isPresented:) for modal presentation
  • Delegates business logic to ScannerManager
  • Follows the repository's modern SwiftUI architecture (no ViewModels, uses @Observable pattern via @EnvironmentObject)

Proper Async/Await Usage

  • Correctly wraps async operations in Task blocks
  • Proper use of await for scanner operations

🔒 Security Considerations

No Security Concerns

  • Feature is properly gated behind Env.isE2E flag
  • Input is trimmed and validated before processing
  • Delegates to existing, tested handleScan method
  • No direct Bitcoin/Lightning operations in views (follows architecture guidelines)

🧪 Testing Considerations

Good Test Support

  • Comprehensive accessibility identifiers:
    • "ScanPrompt" - Button to open dialog
    • "QRDialog" - Dialog container
    • "QRInput" - Text field
    • "DialogConfirm" - Submit button
    • "DialogCancel" - Cancel button
    • "Scan" - Scan button (added in ScanButton.swift)

⚠️ Missing Unit Tests

While this is an E2E testing feature, consider adding unit tests for ScannerManager.handleManualEntry to verify:

  • Trimming behavior
  • Empty string handling
  • Proper delegation to handleScan

Performance Considerations

No Performance Issues

  • Minimal overhead (only adds UI elements when Env.isE2E is true)
  • No expensive operations in view body
  • Proper state management

📝 Minor Suggestions

  1. Localization: The button title "Enter QR Code String" is hardcoded. Since this is E2E-only, this is acceptable, but consider adding a comment explaining why it's not localized.

  2. Error handling: The handleManualEntry method could benefit from error propagation if handleScan fails.

  3. TextField placeholder: The TextField in ManualScanPrompt.swift:22 has an empty placeholder. Consider adding a descriptive placeholder like "bitcoin:...".


🎯 Final Verdict

✅ APPROVED - This is a well-implemented E2E testing feature that follows the repository's architecture and conventions.

Summary:

  • ✅ Clean, focused implementation
  • ✅ Properly gated behind E2E flag
  • ✅ Good accessibility support
  • ✅ Follows SwiftUI best practices
  • ✅ No security concerns
  • ⚠️ Minor inconsistencies and opportunities for improvement (see above)

The PR is ready to merge after addressing the button title inconsistency. Other suggestions are optional enhancements.

@piotr-iohk piotr-iohk marked this pull request as ready for review November 25, 2025 17:42
@piotr-iohk piotr-iohk requested review from jvsena42 and pwltr November 25, 2025 17:43
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@piotr-iohk piotr-iohk force-pushed the test/scan-prompt-test-form branch from 5256919 to 668f8d2 Compare November 25, 2025 18:29
@claude
Copy link

claude bot commented Nov 25, 2025

PR Review: E2E Manual QR Entry Feature

Overview

This PR adds an E2E-only manual QR code entry form, mirroring Android functionality. The implementation is clean and focused, enabling the @settings_10 iOS test. Overall, this is a well-structured addition that follows the project's conventions effectively.


✅ Strengths

  1. E2E-Only Scope: Properly gated behind Env.isE2E check, ensuring test-only functionality doesn't leak into production builds.

  2. Consistent Architecture: Follows SwiftUI patterns correctly with:

    • @State for local view state
    • Async/await for operations
    • Proper separation of concerns (UI vs. business logic)
  3. Accessibility: Good use of accessibilityIdentifier for E2E testing ("QRDialog", "QRInput", "DialogConfirm", "ScanPrompt").

  4. Code Reuse: The new handleManualEntry method properly delegates to existing handleScan logic.

  5. Consistency Across Contexts: Implementation in both ScannerScreen and ScannerSheet ensures the feature works in all QR scanning contexts.


🐛 Issues Found

1. Inconsistent Button Title (Minor)

  • ScannerScreen.swift:48: "Enter QR Code String"
  • ScannerSheet.swift:48: "Enter QRCode String" (missing space)

Recommendation: Standardize to "Enter QR Code String" in both files.

// ScannerSheet.swift:48
title: "Enter QR Code String",  // Add space between "QR" and "Code"

2. Missing Input Validation Feedback (Moderate)

The handleManualEntry method silently returns on empty input (ScannerManager.swift:243). Users may not understand why nothing happens if they submit an empty string.

Recommendation: Add user feedback for empty/invalid input:

func handleManualEntry(
    _ value: String,
    context: ScannerContext,
    onSuccess: @MainActor () -> Void
) async {
    let trimmed = value.trimmingCharacters(in: .whitespacesAndNewlines)
    
    guard \!trimmed.isEmpty else {
        await MainActor.run {
            app?.toast(
                type: .warning,
                title: "Invalid Input",
                description: "Please enter a valid QR code string"
            )
        }
        return
    }

    await handleScan(trimmed, context: context)
    await MainActor.run {
        onSuccess()
    }
}

3. Redundant MainActor.run (Minor Performance)

In ScannerManager.swift:246, wrapping onSuccess() in MainActor.run is redundant since onSuccess is already marked @MainActor.

Recommendation:

await handleScan(trimmed, context: context)
onSuccess()  // Already @MainActor, no need to wrap

4. Missing Localization (Minor)

The "Enter QR Code String" text (and button title) is hardcoded in English and not using the t() function for localization.

Recommendation: Add translation keys:

// ManualScanPrompt.swift:20
BodyMSBText(t("scanner__manual_entry_title"))

// ScannerScreen.swift:48
title: t("scanner__manual_entry_button"),

Then add to localization files. However, since this is E2E-only and not user-facing in production, this is low priority.


📊 Code Quality Assessment

Aspect Rating Notes
Code Style ⭐⭐⭐⭐⭐ Follows SwiftFormat conventions
Architecture ⭐⭐⭐⭐⭐ Proper SwiftUI patterns, good separation
Error Handling ⭐⭐⭐☆☆ Missing feedback for empty input
Security ⭐⭐⭐⭐⭐ E2E-only, no security concerns
Accessibility ⭐⭐⭐⭐⭐ All E2E identifiers present
Performance ⭐⭐⭐⭐☆ Minor redundancy with MainActor.run
Testability ⭐⭐⭐⭐⭐ Enables E2E tests effectively

🔍 Additional Observations

  1. Sheet Presentation: The .presentationDetents([.fraction(0.35)]) is well-chosen for the simple input form.

  2. State Management: Proper cleanup of manualEntry state on cancel/submit prevents stale data.

  3. Context Handling: Both ScannerScreen (dynamic context based on navigation) and ScannerSheet (fixed .main context) are handled correctly.

  4. Workflow Changes: The E2E workflow improvement (removing @settings_10 exclusion) is a positive outcome. The regex simplification is clean.


🧪 Test Coverage Recommendations

Since this is for E2E testing, consider:

  1. Testing empty string submission (currently silent failure)
  2. Testing various QR payload formats (addresses, invoices, LNURLs)
  3. Testing cancel behavior

🔐 Security Considerations

No security concerns – This is E2E-only code properly gated behind Env.isE2E, which evaluates to true only when:

  • Compiled with E2E_BUILD flag, OR
  • Environment variable E2E=true is set

✨ Suggestions for Future Enhancement (Optional)

  1. Multi-line support: For longer payloads, consider TextEditor instead of TextField
  2. Paste button: Quick paste from clipboard within the dialog
  3. Recent entries: E2E test payload history (if needed for test efficiency)

📝 Summary

This is a solid, well-implemented feature that successfully brings parity with Android E2E testing capabilities. The code is clean, follows project conventions, and introduces no regressions.

Recommended Actions Before Merge:

  1. ✅ Fix inconsistent button title ("QRCode" → "QR Code")
  2. ⚠️ Consider adding empty input feedback (optional but recommended)
  3. ✅ Remove redundant MainActor.run wrapper

Approval Status: ✅ Approved with minor suggestions

Great work on the implementation! 🚀

Copy link
Collaborator

@ovitrif ovitrif left a comment

Choose a reason for hiding this comment

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

utAck

@piotr-iohk piotr-iohk merged commit 5437871 into master Nov 26, 2025
15 of 24 checks passed
@piotr-iohk piotr-iohk deleted the test/scan-prompt-test-form branch November 26, 2025 12:34
BitcoinErrorLog pushed a commit to BitcoinErrorLog/bitkit-ios that referenced this pull request Dec 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants