Test/backup

[piotr-iohk]

Description

• Enable backup tests.
• Also adjusting behavior on clear date in activity list as per comment

⚠️ please merge with Android counterpart (synonymdev/bitkit-android#481)

Linked Issues/Tasks

synonymdev/bitkit-e2e-tests#54

Screenshot / Video

Insert relevant screenshot / recording

[claude]

Code Review

Thank you for enabling the backup E2E tests! I've reviewed the changes and here's my feedback:

✅ Positive Aspects

1. Clear test enablement: Adding the @backup tag to the E2E workflow is straightforward and follows existing patterns.
2. Proper E2E gating: Using Env.isE2E to conditionally show the "AllSynced" indicator ensures this UI element only appears during tests.
3. Accessibility support: Proper use of .accessibilityIdentifier("AllSynced") enables automated testing to detect sync completion.
4. Consistent patterns: The AddTagSheet changes follow the existing pattern of passing test IDs to reusable components.

📝 Code Quality & Best Practices

BackupSettings.swift (Lines 58-63)

The allSynced computed property is well-implemented:

• ✅ Uses allSatisfy for clean, functional-style checking
• ✅ Properly checks both !status.running and !status.isRequired
• ✅ Handles all backup categories via BackupCategory.allCases

Minor Suggestions

1. Consider extracting the icon as a separate view (Lines 88-94):

   // Current implementation embeds the icon inline
   if Env.isE2E, allSynced {
       Image("check")
           .resizable()
           .frame(width: 16, height: 16)
           .foregroundColor(.greenAccent)
           .accessibilityIdentifier("AllSynced")
   }

   For better reusability and testability, consider:

   private struct SyncStatusIndicator: View {
       let allSynced: Bool
       
       var body: some View {
           if allSynced {
               Image("check")
                   .resizable()
                   .frame(width: 16, height: 16)
                   .foregroundColor(.greenAccent)
                   .accessibilityIdentifier("AllSynced")
           }
       }
   }

   However, this is not required for this PR - the current implementation is fine for a test-only indicator.

2. Alignment consideration (Line 84): The HStack(alignment: .center) may cause slight visual misalignment if the label has padding. Consider testing visually or using .firstTextBaseline alignment if the check icon doesn't align well with the text.

🔒 Security Concerns

None identified. The changes:

• Only affect test builds (gated by Env.isE2E)
• Don't expose sensitive backup data
• Don't modify backup logic itself

⚡ Performance Considerations

The allSynced computed property runs on every view update. This is acceptable because:

• ✅ It's a simple iteration over a small enum (7 cases in BackupCategory)
• ✅ Only accesses cached data from BackupViewModel.backupStatuses
• ✅ No async operations or heavy computations

However, if performance becomes a concern, consider memoization:

@State private var cachedAllSynced: Bool = false

// Update in .onChange(of: viewModel.backupStatuses)

This is not necessary at this stage but worth noting for future optimization.

🐛 Potential Issues

None identified. The code is clean and follows best practices.

🧪 Test Coverage

Question: Does the linked E2E test PR (synonymdev/bitkit-e2e-tests#54) include:

• Tests that verify the "AllSynced" indicator appears after all backups complete?
• Tests for each backup category to ensure comprehensive coverage?
• Edge case testing (e.g., one category fails while others succeed)?

These would strengthen confidence in the changes.

📐 Architecture & Conventions

The changes align well with the project's architecture guidelines:

• ✅ Uses @StateObject for BackupViewModel (appropriate for view-owned state)
• ✅ Follows SwiftUI best practices with computed properties
• ✅ Properly uses conditional compilation/environment checks
• ✅ Maintains separation of concerns (view logic vs. business logic)

🎯 Recommendations

1. Verify visual alignment: Test the "AllSynced" icon alignment with the "Latest" label across different font sizes/accessibility settings.
2. Consider SwiftFormat: Ensure code passes formatting checks (though the diff looks clean).
3. Document the helper in comments: Consider adding a brief comment explaining why allSynced checks !running && !isRequired to help future maintainers.

Summary

This is a clean, focused PR that enables backup E2E tests with minimal, well-scoped changes. The code quality is good, follows project conventions, and introduces no security or performance concerns. The accessibility support is properly implemented for automated testing.

Recommendation: ✅ Approve (pending verification of visual alignment and E2E test coverage)

[Copilot]

Pull request overview

This PR enables @backup e2e tests by adding necessary test identifiers to UI components and including the backup tests in the CI workflow.

Key Changes:

• Added test identifiers to TagInputForm component in AddTagSheet for e2e test automation
• Introduced an "AllSynced" visual indicator in BackupSettings that appears only during e2e tests to confirm all backup categories are synced
• Updated the GitHub Actions e2e test workflow to include @backup tests in the test execution pattern

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
Bitkit/Views/Wallets/Sheets/AddTagSheet.swift	Added test identifiers (TagInput, ActivityTagsSubmit) to the TagInputForm component for e2e test automation
Bitkit/Views/Settings/Backup/BackupSettings.swift	Added allSynced computed property and conditional check icon indicator for e2e testing to verify all backup categories are synced
.github/workflows/e2e-tests.yml	Added @backup tag to the test grep pattern to enable backup tests in the e2e test suite

[claude]

PR Review: Enable Backup Tests

Overview

This PR enables backup tests in the E2E test suite and adds testing helpers to the backup UI. The changes are minimal and focused on test enablement.

---

✅ Positive Findings

1. Clean Test Infrastructure

• Good use of Env.isE2E flag to conditionally show test-only UI elements
• Proper accessibility identifiers (AllSynced, ActivityTagsSubmit, TagInput) for E2E test selectors
• Changes are non-invasive and do not affect production behavior

2. Solid Logic
The allSynced computed property correctly checks backup status - verifies all categories are not running AND not required using Swift's allSatisfy idiom appropriately

3. Consistent Pattern

• The TagInputForm changes follow existing patterns with optional test IDs
• Uses .accessibilityIdentifierIfPresent() helper correctly

---

🔍 Code Quality Issues

1. Architecture Violation (BackupSettings.swift:55-56)
The view still uses @StateObject with a legacy ViewModel pattern. According to CLAUDE.md, this project is transitioning away from traditional ViewModels to modern SwiftUI patterns using @Observable classes. The BackupViewModel should be converted to @Observable class and injected via .environment().

2. Missing Accessibility Context (BackupSettings.swift:87-93)
The "AllSynced" indicator lacks an accessibility label. Recommendation: Add .accessibilityLabel("All backups synced") for screen reader support.

3. Non-Localized Status Text (BackupViewModel.swift:53-58)
The status strings in formatStatusText() are hardcoded ("Running", "Required"). While not directly changed in this PR, these should use the t() localization function for consistency.

---

🐛 Potential Issues

1. Computed Property Performance (BackupSettings.swift:58-63)
The allSynced computed property performs 7 dictionary lookups per render. For E2E-only visual feedback, this is acceptable but not optimal. If performance becomes an issue, consider computing this in the ViewModel and exposing it as a published property.

---

🔒 Security & Performance

• ✅ No security concerns - E2E flag prevents production exposure
• ✅ No sensitive data exposed
• ✅ Performance impact negligible (E2E builds only)

---

📋 Test Coverage

The PR description links to E2E test suite changes. Suggestion: Consider adding unit tests for allSynced computed property logic (edge cases: all running, some required, etc.) and backup status state transitions.

---

🎯 Recommendations

High Priority:

1. ⚠️ Architecture Alignment: Plan migration of BackupViewModel from ObservableObject to @Observable to align with project standards (CLAUDE.md)
2. Add .accessibilityLabel() to the "AllSynced" indicator for screen reader support

Medium Priority:
3. Localize hardcoded strings in BackupViewModel.formatStatusText()

Low Priority:
4. Consider caching allSynced computation if backup categories grow significantly

---

✅ Approval Status

LGTM with minor suggestions. The changes are well-structured for E2E testing and do not introduce bugs. The architecture concerns are pre-existing and can be addressed in a follow-up refactor.

Suggested follow-up issue: Track migration of BackupViewModel to @Observable pattern per project architecture guidelines.

---

Review conducted with reference to CLAUDE.md project guidelines.

[claude]

PR Review: Enable backup tests

Summary

This PR enables backup E2E tests by adding test identifiers and visual indicators for test automation. The changes are focused and purposeful for E2E testing enablement.

---

✅ Code Quality & Best Practices

Strong Points:

• Clean, minimal changes that are well-scoped to the task
• Proper use of accessibility identifiers for E2E test automation
• E2E-specific UI indicator properly gated behind Env.isE2E flag
• Follows SwiftUI naming conventions and patterns

Suggestions:

1. BackupSettings.swift:58-63 - Consider adding a comment explaining the sync logic:

   // E2E helper: Determines if all backup categories are synced and not running
   private var allSynced: Bool {
       BackupCategory.allCases.allSatisfy { category in
           let status = viewModel.getStatus(for: category)
           return !status.running && !status.isRequired
       }
   }

2. BackupSettings.swift:87-93 - The conditional E2E UI could be extracted to a separate component for better testability and reusability:

   if Env.isE2E, allSynced {
       SyncStatusIndicator()
   }

---

🐛 Potential Issues

DateRangeSelectorSheet.swift:184 - ⚠️ Removed line may cause UX regression

The removal of isPresented = false when clearing the date range means the sheet will no longer auto-dismiss after clearing. This changes user behavior:

Before: Clear button → clears dates → closes sheet
After: Clear button → clears dates → sheet stays open

Question: Is this intentional? If users expect the sheet to close after clearing (similar to how many filter UIs work), this could be confusing. Consider:

• If intentional for E2E testing: Add a comment explaining why
• If unintentional: Restore the line or add explicit "Close" button

---

🔒 Security Concerns

No security issues identified. The E2E-specific UI additions are properly gated and don't expose sensitive information.

---

⚡ Performance Considerations

BackupSettings.swift:58-63 - The allSynced computed property recalculates on every body render. This is acceptable given:

• Small data set (BackupCategory.allCases is typically small)
• Simple boolean operations
• Already following SwiftUI best practices

If BackupCategory grows significantly, consider caching or @State optimization.

---

🧪 Test Coverage

Good:

• Adds accessibility identifiers for E2E tests: AllSynced, TagInput, ActivityTagsSubmit
• Visual indicator (AllSynced) helps E2E tests verify backup completion
• Enables @backup tag in CI workflow

Consider:

• Unit tests for allSynced computed property logic
• Verify the date range sheet behavior change doesn't break existing E2E tests that rely on auto-dismiss

---

📋 Additional Notes

1. Workflow change (.github/workflows/e2e-tests.yml:111): Adding @backup to the E2E test grep pattern is correct and aligns with the PR objective.

2. AddTagSheet.swift: The addition of explicit test identifiers (textFieldTestId, buttonTestId) is good practice for E2E stability. These were previously optional parameters in TagInputForm, now being explicitly set.

3. Architecture alignment: Changes follow the repository's SwiftUI patterns. Note that BackupViewModel uses @StateObject/ObservableObject (legacy pattern) rather than the newer @Observable pattern mentioned in CLAUDE.md, but this is consistent with existing code.

---

🎯 Recommendations

Priority 1 (Must Address):

• Clarify the intentional behavior change in DateRangeSelectorSheet (line 184)

Priority 2 (Should Address):

• Add comment explaining allSynced logic
• Verify E2E tests handle the date range sheet behavior change

Priority 3 (Nice to Have):

• Consider extracting E2E sync indicator into reusable component
• Add unit tests for allSynced logic

---

✨ Conclusion

Overall, this is a well-executed PR with focused changes that enable backup E2E tests. The code quality is good and follows repository conventions. The main concern is the behavioral change in DateRangeSelectorSheet that should be clarified or reverted.

Recommendation: Approve with minor changes requested for the date range sheet behavior.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.