Skip to content

test(mobile): restore settings regression tests after UI module moves - #21280

Closed
luke2x wants to merge 2 commits into
pingdotgg:mainfrom
luke2x:test/mobile-settings-mock-paths
Closed

luke2x wants to merge 2 commits into
pingdotgg:mainfrom
luke2x:test/mobile-settings-mock-paths

Conversation

@luke2x

@luke2x luke2x commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Two existing mobile settings regression files fail on main at 699c1c5d4c before they can verify draft persistence and serialized writes. Their mocks still target components/AppText and components/ScreenScrollView, while production imports now use components/ui. The source-control route also eagerly imports the maintenance-only database notice and its Expo dependencies.

The same settings failures appear in this CI run. They were reproduced locally on fresh main before this change.

Change

Point the three stale mocks at the imported UI modules and stub DatabaseStorageNotice at the existing route-test component boundary. The actual branch-naming component and route write-queue logic remain under test. Every existing assertion is retained.

Scope and approval

A focused, test-only repair of broken settings test boundaries: four added lines and three removed lines across two existing test files. This is submitted under the small obvious-fix exception. It restores existing regression coverage for leaving a screen with unsaved text and saving a draft while another write is pending.

Verification

vp test run apps/mobile/src/features/settings/components/BranchNamingSettings.test.tsx apps/mobile/src/features/settings/SettingsServerControlsRouteScreen.test.tsx
Existing regression coverage Before on 699c1c5d4c After
Branch naming: save on leave, reopen, pending writes, unchanged values 9 failed (Text missing from native mock) 9 passed
Source-control settings: flush draft after pending write Suite fails during native-module loading 1 passed

After correcting the moved paths, the route still reached Expo and failed with __DEV__ is not defined; stubbing the unrelated database-notice leaf allows its write-queue assertion to run. No assertions were removed, no tests were skipped, and no native globals were added to hide the import failure.

Focused lint and formatting checks passed for both files. Mobile typecheck passed. These are React/jsdom tests; native UI behavior and full CI are not claimed as verified by this test-harness fix. Separate web test dependency failures are outside this PR.

Implemented and tested with OpenAI Codex (gpt-6.1-sol) in T3 Code. Review follow-ups by DeepSeek V4.1 Flash 1017.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 18, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-18T20:18:05.321654Z 7e19eed PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:XS 0-9 changed lines (additions + deletions). labels Oct 18, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 18, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 7e19eed

Macroscope's review found this PR approvable — The PR is limited to two ignored mobile test files and restores existing regression coverage through small mock-path adjustments. It does not change production behavior, product defaults, or static-analysis configuration.

Notes:

  • No code objects were reviewed. Approvability was decided on eligibility alone.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: ab8dfeca-2fa2-4f06-99d3-77715050993c
📥 Commits

Reviewing files that changed from the base of the PR and between 3adac94 and 7e19eed.

📒 Files selected for processing: 2
  • apps/mobile/src/features/settings/SettingsServerControlsRouteScreen.test.tsx
  • apps/mobile/src/features/settings/components/BranchNamingSettings.test.tsx

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.

📝 Walkthrough

Two settings test files update component mock paths. One test also adds a no-op mock for DatabaseStorageNotice.

Changes: Settings test mock updates

Layer / File(s) Summary
Update settings test mocks
apps/mobile/src/features/settings/SettingsServerControlsRouteScreen.test.tsx, apps/mobile/src/features/settings/components/BranchNamingSettings.test.tsx
The tests mock ScreenScrollView and AppText from their components/ui paths. The server controls test adds a no-op DatabaseStorageNotice mock.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~3 minutes

Change: Other

Suggested reviewers: adityavardhansharma

Merge Risk: ⚪ Minimal · up to 7e19e

This change updates settings tests to match current component imports and stubs a child component at the test boundary. No production behavior change or user-facing regression risk is identified.

Pre-merge checks: Passed 4
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Title check Passed The title clearly and concisely describes the main change: restoring mobile settings regression tests after UI module moves.
Description check Passed The description includes the required Problem, Change, Scope and approval, and Verification sections. It explains the failure, the focused test-only fix, the small-obvious-fix rationale, test results,…
Full details: Description check

Explanation

The description includes the required Problem, Change, Scope and approval, and Verification sections. It explains the failure, the focused test-only fix, the small-obvious-fix rationale, test results, limitations, and the tools used.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@juliusmarminge

Copy link
Copy Markdown
Member

Note

Grok responding on behalf of Julius.

Thanks for tracking this down. The same settings test fix (pointing the AppText and ScreenScrollView mocks at components/ui and stubbing DatabaseStorageNotice) just landed on main in #21293, so I'm closing this as superseded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XS 0-9 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants