Skip to content

Conversation

@adalpari
Copy link
Contributor

@adalpari adalpari commented Jan 9, 2026

Description

Updates the "Application Password Required" dialog to use automatic Application Password generation instead of redirecting to the browser for manual creation. When auto-generation fails, it falls back to the existing manual browser-based flow.

Key changes:

  • ApplicationPasswordRequiredDialogActivity now uses ApplicationPasswordAutoAuthDialogViewModel for auto-generation
  • Button text changed from "Get Started" to "Create"
  • Experimental feature flag is automatically enabled when the user initiates the flow
  • On auto-generation failure, falls back to manual browser-based Application Password creation

Testing instructions

Auto-generation success flow:

  1. Log in with a self-hosted site
  2. Try to create a new post using the new block editor
  3. See the "Application Password Required" dialog appear
  4. Tap "Create"
  • Verify the Application Password is created automatically and you can create a post
Screen.Recording.2026-01-09.at.13.01.50.mov

Auto-generation fallback flow:

  1. Disable the Application Password feature
  2. Add a fail line to ApplicationPasswordAutoAuthDialogViewModel:createApplicationPassword()
Screenshot 2026-01-09 at 12 58 39
  1. Try to create a new post using the new block editor
  2. See the "Application Password Required" dialog appear
  3. Tap "Create"
  • Verify the browser opens with the manual Application Password creation flow
Screen.Recording.2026-01-09.at.13.00.30.mov

@dangermattic
Copy link
Collaborator

1 Warning
⚠️ PR is not assigned to a milestone.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jan 9, 2026

App Icon📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress
FlavorJalapeno
Build TypeDebug
Versionpr22470-be1c4de
Commitbe1c4de
Direct Downloadwordpress-prototype-build-pr22470-be1c4de.apk
Note: Google Login is not supported on these builds.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jan 9, 2026

App Icon📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack
FlavorJalapeno
Build TypeDebug
Versionpr22470-be1c4de
Commitbe1c4de
Direct Downloadjetpack-prototype-build-pr22470-be1c4de.apk
Note: Google Login is not supported on these builds.

@codecov
Copy link

codecov bot commented Jan 9, 2026

Codecov Report

❌ Patch coverage is 20.40816% with 39 lines in your changes missing coverage. Please review.
✅ Project coverage is 39.12%. Comparing base (19d61b5) to head (be1c4de).
⚠️ Report is 5 commits behind head on trunk.

Files with missing lines Patch % Lines
...sword/ApplicationPasswordRequiredDialogActivity.kt 0.00% 36 Missing ⚠️
...java/org/wordpress/android/ui/ActivityNavigator.kt 0.00% 2 Missing ⚠️
...word/ApplicationPasswordAutoAuthDialogViewModel.kt 90.90% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            trunk   #22470      +/-   ##
==========================================
- Coverage   39.13%   39.12%   -0.01%     
==========================================
  Files        2204     2205       +1     
  Lines      106640   106684      +44     
  Branches    15134    15139       +5     
==========================================
+ Hits        41734    41743       +9     
- Misses      61407    61442      +35     
  Partials     3499     3499              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@adalpari
Copy link
Contributor Author

adalpari commented Jan 9, 2026

@claude

@claude
Copy link
Contributor

claude bot commented Jan 9, 2026

EDIT: Outdated

adalpari and others added 2 commits January 9, 2026 13:48
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
@adalpari
Copy link
Contributor Author

adalpari commented Jan 9, 2026

@claude

@claude
Copy link
Contributor

claude bot commented Jan 9, 2026

EDIT: Outdated

@adalpari
Copy link
Contributor Author

adalpari commented Jan 9, 2026

@claude

@claude
Copy link
Contributor

claude bot commented Jan 9, 2026

Claude finished @adalpari's task in 2m 37s —— View job


Comprehensive Code Review Complete ✅

I've completed a thorough review of this PR. Here's my analysis across the five focus areas:


1. Code Quality ⭐

Positive Changes:

  • Clean refactoring from inheritance (ApplicationPasswordDialogActivity) to composition (ComponentActivity with ApplicationPasswordAutoAuthDialogViewModel)
  • Good separation of concerns between UI (Compose) and business logic (ViewModel)
  • Proper use of Jetpack Compose modern UI patterns
  • Loading state management is well implemented

Issues Found:

🔴 Critical: Missing Error Toast on Dialog Dismissal
Location: ApplicationPasswordRequiredDialogActivity.kt:77 (view)
When users tap "Create" and auto-generation fails, they get a generic error toast. This doesn't explain what failed or what to do next. See inline comment for specific suggestion.

⚠️ Line Length Violations
Location: ApplicationPasswordRequiredDialogActivity.kt:120-121 (view)
The codebase standard is 120 characters max (per CLAUDE.md). See inline comment for formatting suggestions.

⚠️ Exception Message Logging
Location: ApplicationPasswordAutoAuthDialogViewModel.kt:97 (view)
Exception messages might contain sensitive data. Line 118 was addressed in a previous review, but line 97 should follow the same pattern for consistency.


2. Security 🔒

✅ No Major Security Issues Found

Minor Observations:

  • The experimental feature is auto-enabled when users click "Create" - this is intentional per the PR author's response, and is acceptable since the user explicitly initiates the action
  • Application Password credentials are properly stored through the existing ApplicationPasswordLoginHelper
  • No sensitive data is exposed in UI or logs beyond the logging concern noted above

3. Performance ⚡

✅ No Performance Concerns

  • Coroutines are properly scoped to lifecycleScope (cancels automatically on destroy)
  • State flows are used correctly for reactive UI updates
  • No memory leaks detected - ViewModel lifecycle is properly managed
  • Loading indicator prevents multiple simultaneous API calls (button is disabled during loading)

4. Testing 🧪

Excellent Test Coverage Additions!

The test file ApplicationPasswordAutoAuthDialogViewModelTest.kt was significantly enhanced (+99 lines):

  • ✅ Tests experimental feature enablement logic
  • ✅ Tests error scenarios with proper fallback
  • ✅ Tests loading state transitions
  • ✅ Tests blank username/password validation
  • ✅ Tests error handling when fallback also fails

Codecov Report Shows 20.40% Patch Coverage

The main gap is in ApplicationPasswordRequiredDialogActivity.kt (36 missing lines). This is the new Compose UI activity, which is harder to test with standard unit tests.

Recommendation:
Consider adding UI tests (Espresso or Compose UI tests) to cover the dialog interaction flow, or accept that UI code will have lower test coverage (which is common for Compose-based activities).


5. Documentation 📚

✅ Adequate Documentation

  • PR description clearly explains the changes and fallback behavior
  • Testing instructions are comprehensive with video demonstrations
  • Code comments are minimal but appropriate (code is self-documenting)
  • Function names clearly indicate intent

Minor Enhancement Suggestion:
Consider adding a KDoc comment to createApplicationPassword() explaining the flow (success → stores credentials, failure → fallback, fallback failure → error).


Summary of Review

Strengths:

  • ✅ Modern architecture with clean separation of concerns
  • ✅ Excellent error handling with graceful fallback
  • ✅ Comprehensive unit test coverage for ViewModel logic
  • ✅ Proper lifecycle management and coroutine usage
  • ✅ Good user experience with loading states

Action Items:

  1. 🔴 High Priority: Improve error message specificity - see inline comment
  2. ⚠️ Medium Priority: Fix line length violations - see inline comment
  3. ⚠️ Low Priority: Consistent exception logging - see inline comment
  4. 💡 Optional: Add UI tests for dialog interactions
  5. 💡 Optional: Add KDoc to main public functions

Overall Assessment: This is a solid implementation that successfully introduces auto-generation with proper fallback handling. The architecture is clean, tests are comprehensive, and the user flow is well thought out. The main improvements needed are around error messaging clarity and code style compliance.


Previous Review Comments Status:

  • ✅ Error feedback added (Toast on Error event) - though could be more specific
  • 🔄 Security logging concern partially addressed (line 118 fixed, line 97 remains)
  • ✅ Feature auto-enablement explained by author - acceptable with explicit user action

) {
val description = if (featureName != null) {
stringResource(R.string.application_password_required_description, featureName) +
stringResource(R.string.application_password_experimental_feature_note)
Copy link
Contributor

Choose a reason for hiding this comment

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

Code Style: Line Length Violation

This line exceeds the 120 character limit specified in CLAUDE.md. Consider breaking it into multiple lines:

val description = if (featureName != null) {
    stringResource(R.string.application_password_required_description, featureName) +
        " " + stringResource(R.string.application_password_experimental_feature_note)
} else {
    stringResource(R.string.application_password_info_description_1) +
        " " + stringResource(R.string.application_password_experimental_feature_note)
}

Or use a more readable format:

val baseDescription = if (featureName != null) {
    stringResource(R.string.application_password_required_description, featureName)
} else {
    stringResource(R.string.application_password_info_description_1)
}
val description = "$baseDescription ${
    stringResource(R.string.application_password_experimental_feature_note)
}"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

False positive.

is ApplicationPasswordAutoAuthDialogViewModel.NavigationEvent.Error -> {
ToastUtils.showToast(
this@ApplicationPasswordRequiredDialogActivity,
R.string.error_generic
Copy link
Contributor

Choose a reason for hiding this comment

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

User Experience: Improve Error Message Specificity

The generic error message doesn't help users understand what went wrong or what to do next. Consider using a more specific message:

ToastUtils.showToast(
    this@ApplicationPasswordRequiredDialogActivity,
    R.string.application_password_creation_failed, // Create a new, specific string resource
    ToastUtils.Duration.LONG
)

The message should explain that automatic Application Password creation failed and that they may need to try the manual process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is expected as the user tried to generate an Application Password but something was unexpected with no extra information.

}
}
} catch (e: Exception) {
appLogWrapper.e(AppLog.T.API, "Exception creating application password: ${e.message}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Security: Potential Sensitive Data in Logs

Exception messages could contain sensitive information (usernames, partial credentials, site-specific data). Consider sanitizing or logging just the exception type:

appLogWrapper.e(AppLog.T.API, "Exception creating application password", e)

This logs the stack trace for debugging without potentially exposing sensitive data in the message.

(Note: This was addressed in line 118 but not here - consider applying the same pattern for consistency)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a false positive as it's already addressed. See the error is e.message

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jan 9, 2026

@adalpari
Copy link
Contributor Author

adalpari commented Jan 9, 2026

Action Items:

  1. 🔴 High Priority: Improve error message specificity - see inline comment
  2. ⚠️ Medium Priority: Fix line length violations - see inline comment
  3. ⚠️ Low Priority: Consistent exception logging - see inline comment
  1. Expected. Not a real high priority. It's just an exoected generic error message
  2. False positive
  3. False posiotive. It's already solved

@adalpari adalpari requested a review from nbradbury January 9, 2026 13:38
@nbradbury
Copy link
Contributor

@adalpari When I followed the steps above, the "Authenticate using application password" banner appeared after the password was created.

Screen_recording_20260109_085051.mp4

Also, after the password is created, shouldn't it navigate to the edit post screen?

@adalpari
Copy link
Contributor Author

adalpari commented Jan 9, 2026

@adalpari When I followed the steps above, the "Authenticate using application password" banner appeared after the password was created.

This seems to be a race condition (not always happening), but not related with the PR. I've noted it down to solve it in a different PR.

Also, after the password is created, shouldn't it navigate to the edit post screen?

Same, this is not in the scope of this PR. I can have a look when fixing the banner bug though.

Copy link
Contributor

@nbradbury nbradbury left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

@adalpari adalpari merged commit dbbbe71 into trunk Jan 9, 2026
24 checks passed
@adalpari adalpari deleted the feat/CMM-1125-Be-sure-to-auto-generate-Application-Pasword-when-asked-fromt-he-editor branch January 9, 2026 14:05
@adalpari adalpari changed the title CMM-1125 be sure to auto generate application pasword when asked fromt he editor CMM-1125 be sure to auto generate application pasword when asked from the editor Jan 12, 2026
@adalpari adalpari changed the title CMM-1125 be sure to auto generate application pasword when asked from the editor CMM-1125 be sure to auto generate application password when asked from the editor Jan 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants