Skip to content

next: password import conflict count and Firefox consent text - #18229

Merged
teamleaderleo merged 3 commits into
feat-cmux-nextfrom
op-next-password-strings
Oct 7, 2026
Merged

teamleaderleo merged 3 commits into
feat-cmux-nextfrom
op-next-password-strings

Conversation

@teamleaderleo

@teamleaderleo teamleaderleo commented Oct 7, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Onboarding strings requested by the password-import lane, which doesn't edit our onboarding files:

  1. Conflicts on their own line. When an imported sign-in matches one cmux already has but with a different password, cmux keeps the saved one. The finished line now says so: "Already saved with a different password: N. cmux kept the saved password." It appears only when PasswordImportReport.conflicts > 0. "Passwords skipped: N" now uses notImportedOtherThanConflicts, so a conflict isn't counted twice.
  2. Firefox-only consent text. When the chosen profiles involve no Keychain item (only Firefox), the consent screen shows onboarding.passwords.firefoxOnly instead of onboarding.passwords.keychain2.
  3. Mixed choice. When the choice mixes Chromium browsers and Firefox, onboarding.passwords.firefoxPrimary follows the Keychain text.

PasswordImportReport.conflicts and notImportedOtherThanConflicts are copied byte for byte from the password lane's wip/nx-passwords-firefox, so the two branches merge cleanly. The text logic lives in OnboardingStrings.passwordsSummary and passwordsConsent, which both views call and the tests cover.

Testing

  • Failing test first: 781bd44 (PasswordImportStringsTests: the conflict count, each summary sentence shown only when its count is above zero, and the consent-text choice for Chromium only, mixed, and Firefox only). Fix in e2eb176. CI runs the Swift tests.
  • Local: check-l10n.sh passes (all three strings in 21 locales; en and the supplied ja are translated, the rest needs_review), and package conventions, concurrency and crash-safety checks pass.
  • No fleet build or capture: string and count changes, covered by unit tests.

Changelog

Changed: Password import says how many saved passwords it kept when the imported one differed, and explains Firefox's confirmation without a Keychain prompt.

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Summary by cubic

Improves password import reporting and clarifies the onboarding consent text for Firefox.

The finished import line now says how many saved passwords cmux kept when the imported one differed, and "Passwords skipped" no longer double-counts those conflicts. On the consent screen, Firefox-only imports no longer mention a Keychain item, and mixed Chromium/Firefox imports append a sentence explaining Firefox's primary password prompt.

Bug Fixes

  • Adds conflicts and notImportedOtherThanConflicts to PasswordImportReport so the summary counts are accurate.
  • Adds unit tests covering the summary sentences and consent text for Chromium-only, mixed, and Firefox-only choices.

Written for commit e99eb1c. Summary will update on new commits.

Review in cubic Turn on auto-fix


Note

Low Risk
User-facing copy and reporting only; import logic unchanged aside from exposing existing conflict counts for summaries.

Overview
Password import onboarding now reports password conflicts separately from other skips, and adjusts consent copy when Firefox is involved.

The finished import line uses passwordsSummary instead of lumping all non-imports into “Passwords skipped.” When cmux already has a sign-in for the same site/username with a different password, it keeps the saved one and shows “Already saved with a different password: N. cmux kept the saved password.” PasswordImportReport adds conflicts and notImportedOtherThanConflicts so conflicts are not double-counted in the skipped total.

On the password consent screen, copy is chosen by browser mix: Firefox-only uses text that explains no Keychain item (key lives in the profile) and optional primary password; Chromium + Firefox keeps the Keychain paragraph and appends the Firefox primary-password note. ImportStepModel.passwordsIncludeFirefox drives that via OnboardingStrings.passwordsConsent.

New localized strings and PasswordImportStringsTests cover summary sentences and consent variants.

Reviewed by Cursor Bugbot for commit e99eb1c. Bugbot is set up for automated code reviews on this repo. Configure here.

teamleaderleo and others added 2 commits October 6, 2026 21:35
Red: PasswordImportStringsTests needs PasswordImportReport.conflicts,
notImportedOtherThanConflicts, OnboardingStrings.passwordsSummary and
passwordsConsent, which don't exist yet.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Onboarding's finished line now counts a conflict (a sign-in cmux already
has with another password; the saved one is kept) on its own line,
"Already saved with a different password: N. cmux kept the saved
password.", and "Passwords skipped" uses notImportedOtherThanConflicts so
a conflict is not counted twice. The consent screen shows
onboarding.passwords.firefoxOnly when no Keychain item is involved (only
Firefox profiles), and appends onboarding.passwords.firefoxPrimary to the
Keychain text when the choice mixes Chromium browsers and Firefox.
PasswordImportReport.conflicts and notImportedOtherThanConflicts match
the password-import lane's wip/nx-passwords-firefox lines exactly. All
three strings are in 21 locales; en and ja are translated, the rest
needs_review.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration
  • Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: ba594498-42da-4bf0-a660-843567f904b8

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Not verified: macOS jobs did not run: compile and app tests were skipped on e99eb1c944; ci-status is green without them.

CI passes on e99eb1c944 (run 37582830046 attempt 1).
macOS jobs did not run: compile and app tests were skipped: Fast static checks skipped (the macos job needs both to succeed). Re-run CI once they pass, before merging.

Written by scripts/ci/classify_failures.py (ci-failure-attribution.yml); signatures are its SIGNATURES table. A machine verdict is the runner's fault, not this PR's; yours means the failing file is one this PR changes, also red on main that main's latest full suite fails the same way, seen on other PRs that it failed on another pull request's run lately.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit e99eb1c. Configure here.

}
keychain.stringValue = OnboardingStrings.passwordsKeychain(Self.quotedList(model.passwordKeychainItems))
keychain.stringValue = OnboardingStrings.passwordsConsent(
keychainItems: model.passwordKeychainItems, includesFirefox: model.passwordsIncludeFirefox, quote: { Self.quotedList($0) })

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Mixed consent text exceeds label

Low Severity

passwordsConsent appends passwordsFirefoxPrimary to the full Keychain paragraph for a mixed Chromium and Firefox choice, then assigns that to the consent label, which is still capped at four lines. That cap was already sized for the Keychain paragraph alone, so the new Firefox primary-password sentence is clipped and mixed imports never show the warning in full.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit e99eb1c. Configure here.

@teamleaderleo
teamleaderleo merged commit 6a753c9 into feat-cmux-next Oct 7, 2026
73 of 84 checks passed
@teamleaderleo
teamleaderleo deleted the op-next-password-strings branch October 7, 2026 07:23
@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

The feat-cmux-next push run https://github.com/manaflow-ai/cmux/actions/runs/37587042315 at 6a753c9 failed: cmux-next checks (god files, concurrency, crash safety, l10n).
Those jobs last passed at 86c2c3b. This pull request is one of 3 merged since: #18282, #18152, #18229.
If the failure is in your change, fix forward on feat-cmux-next. Pull requests run only the tiers their paths reach (docs/ci/cmux-next-tiers.md); label a batch PR full-ci to run them all before merging.

@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

The feat-cmux-next push run https://github.com/manaflow-ai/cmux/actions/runs/37587935376 at a53b04b failed: cmux-next checks (god files, concurrency, crash safety, l10n).
Those jobs last passed at 86c2c3b. This pull request is one of 3 merged since: #18282, #18152, #18229.
If the failure is in your change, fix forward on feat-cmux-next. Pull requests run only the tiers their paths reach (docs/ci/cmux-next-tiers.md); label a batch PR full-ci to run them all before merging.

@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

The feat-cmux-next push run https://github.com/manaflow-ai/cmux/actions/runs/37588003146 at b472aa1 failed: cmux-next checks (god files, concurrency, crash safety, l10n).
Those jobs last passed at 86c2c3b. This pull request is one of 3 merged since: #18282, #18152, #18229.
If the failure is in your change, fix forward on feat-cmux-next. Pull requests run only the tiers their paths reach (docs/ci/cmux-next-tiers.md); label a batch PR full-ci to run them all before merging.

@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

The feat-cmux-next push run https://github.com/manaflow-ai/cmux/actions/runs/37587147561 at 9ec0d03 failed: cmux-next checks (god files, concurrency, crash safety, l10n).
Those jobs last passed at fa75842. This pull request is one of 4 merged since: #18282, #18147, #18152, #18229.
If the failure is in your change, fix forward on feat-cmux-next. Pull requests run only the tiers their paths reach (docs/ci/cmux-next-tiers.md); label a batch PR full-ci to run them all before merging.

@github-actions

github-actions Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

The feat-cmux-next push run https://github.com/manaflow-ai/cmux/actions/runs/37588244794 at 8eee20c failed: cmux-next checks (god files, concurrency, crash safety, l10n).
Those jobs last passed at 86c2c3b. This pull request is one of 3 merged since: #18282, #18152, #18229.
If the failure is in your change, fix forward on feat-cmux-next. Pull requests run only the tiers their paths reach (docs/ci/cmux-next-tiers.md); label a batch PR full-ci to run them all before merging.

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.

1 participant