Support tracking loaded urls in authorization fragment

[fadidurah]

No description provided.

[github-actions]

❌ Work item link check failed. Description does not contain AB#{ID}.

Click here to Learn more.

[Copilot]

Pull request overview

This PR adds URL load tracking infrastructure to the AuthorizationFragment base class to monitor URLs loaded during OAuth 2.0/OIDC authorization flows. The implementation provides a mechanism to track each URL along with its load status and potential error messages, storing them in sequential order.

Changes:

• Added a LinkedHashMap-based tracking system with mUrlLoadTracker and mUrlLoadCounter fields to maintain ordered URL load events
• Introduced UrlLoadStatus inner class to encapsulate URL, success status, and error message with basic sanitization logic
• Provided protected trackUrlLoad() method for subclasses to record URL events and public getUrlLoadTracker() accessor to retrieve tracked events

[Copilot]

Missing null safety validation for the url parameter. The sanitizeUrl method will throw a NullPointerException if a null URL is passed to trackUrlLoad. This is problematic because:

1. WebView callbacks can potentially receive null URLs in edge cases
2. The method is protected, allowing subclasses to call it with any parameters
3. There's no defensive validation before calling url.replaceAll()

Add null checking at the start of sanitizeUrl or in the UrlLoadStatus constructor, returning either an empty string, "null", or "redacted" as appropriate to maintain tracking consistency.

Suggested change

	private static String sanitizeUrl(String url) {
	private static String sanitizeUrl(@Nullable String url) {
	if (url == null) {
	// Return an empty string to maintain tracking consistency for null URLs
	return "";
	}

[Copilot]

Missing @NonNull and @Nullable nullability annotations on method parameters. Following the repository's convention (seen throughout AuthorizationFragment with annotations like @NonNull final Bundle instanceStateBundle), the parameters should be annotated:

• url: Should be @Nullable (and handle null defensively) or @NonNull (with validation that throws IllegalArgumentException)
• error: Should be @Nullable since the documentation states "null if successful"

Additionally, the UrlLoadStatus constructor parameters should have matching annotations.

[Copilot]

The UrlLoadStatus class lacks proper encapsulation and immutability markers. Following Java best practices and repository conventions (where similar data holder classes use final fields), all fields should be explicitly marked as final since they are never reassigned after construction. While the fields are already effectively final (no reassignment), explicitly declaring them as final makes the immutability contract clear and prevents accidental modification.

Additionally, consider whether this class should implement equals() and hashCode() if instances will be compared, or add a comment explaining why it doesn't need them.

[Copilot]

The UrlLoadStatus inner class should be static final rather than just static. Following repository conventions (seen in other inner classes like the IReceiverCallback implementation), the class should be marked as final because:

1. It's not designed for extension (no protected members, simple data holder)
2. Making it final prevents unintended subclassing
3. It documents the design intent clearly
4. It's a common pattern for immutable data holder classes

Additionally, the class is package-private but should consider being public or at least documented regarding its intended visibility, since getUrlLoadTracker() returns a map containing these objects, effectively exposing them publicly.

Suggested change

	private static class UrlLoadStatus {
	public static final class UrlLoadStatus {

[Copilot]

Fragment state restoration not handled for URL tracking state. Android fragments can be destroyed and recreated by the system (e.g., during configuration changes, low memory conditions). The current implementation doesn't save/restore the tracking state via onSaveInstanceState / onCreate(savedInstanceState). This means:

1. Tracked URLs will be lost if the fragment is recreated
2. The counter will reset to 0, potentially causing duplicate keys if tracking resumes
3. Partial authorization flows won't have complete URL history after recreation

If URL tracking is important for debugging or telemetry, consider:

1. Implementing state persistence in onSaveInstanceState() and restoration in onCreate()
2. Documenting that tracking is not preserved across configuration changes (if that's acceptable)
3. Resetting the tracker in a lifecycle method to ensure clean state after restoration

Review existing fragment state handling (lines 87-108) which already implements state preservation patterns for other fields.

[Copilot]

The documentation comment for the mUrlLoadTracker field is imprecise about the map's structure. The comment states "Key: Load order (int), Value: URL and success status (boolean)" which doesn't accurately reflect that the value is actually a UrlLoadStatus object containing three fields (url, isSuccess, error).

The comment should be updated to accurately describe the structure, for example:
"Tracks the URLs loaded in the WebView along with their load status. Key: Sequential load order (1-indexed int), Value: UrlLoadStatus containing the sanitized URL, success boolean, and optional error message."

Suggested change

	* Key: Load order (int), Value: URL and success status (boolean).
	* Key: Sequential load order (1-indexed int), Value: {@link UrlLoadStatus} containing the
	* sanitized URL, success boolean, and optional error message.

[Copilot]

No actual usage or integration of the tracking functionality is present in this PR. The trackUrlLoad() method is declared as protected to allow subclasses to call it, but:

1. No calls to trackUrlLoad() are added in any subclass (WebViewAuthorizationFragment, BrowserAuthorizationFragment, etc.)
2. No integration with WebView callbacks (onPageStarted, onPageFinished, onReceivedError) is shown
3. The public getUrlLoadTracker() method has no consumers

This creates "dead code" - functionality that exists but is never used. Consider either:

1. Adding the actual integration with WebView/browser callbacks in the appropriate subclasses as part of this PR
2. Documenting the intended usage pattern with examples for subclass implementers
3. If this is infrastructure for future work, document that clearly in the PR description and class Javadoc

Without actual usage, it's difficult to verify the implementation is correct and the design meets the real requirements.

[Copilot]

Missing Javadoc documentation for the public getUrlLoadTracker() method regarding thread safety and lifecycle considerations. The method documentation should clarify:

1. Thread safety: Whether it's safe to call while URLs are being tracked concurrently
2. Lifecycle: When it's appropriate to call this method (e.g., after authorization completes, during debugging)
3. Ownership: That the returned map is a defensive copy and modifications won't affect internal state
4. Return value guarantee: Whether the method can return an empty map (yes, initially)

Following repository patterns (like other public API methods in this class such as sendResult()), comprehensive documentation helps consumers understand proper usage.

Suggested change

	* Retrieves the tracked URL load events.
	*
	* @return A copy of the URL load tracker map.
	* Retrieves a snapshot of the tracked URL load events for diagnostic purposes.
	* <p>
	* This method is intended to be called after, or while, the authorization WebView flow
	* is in progress (for example, after authorization completes or when debugging navigation
	* issues) to inspect which URLs were loaded and whether they succeeded.
	* </p>
	* <p>
	* Thread-safety: {@code AuthorizationFragment} and its URL tracking are expected to be
	* accessed from a single thread (typically the main/UI thread). This method does not
	* perform any additional synchronization; callers should avoid invoking it concurrently
	* from multiple threads unless they provide their own external synchronization.
	* </p>
	* <p>
	* Ownership: the returned {@link Map} is a defensive copy of the internal tracker. Changes
	* made to the returned map or its contents will not affect the fragment's internal state.
	* The map may be empty, for example if no URL load events have been tracked yet.
	* </p>
	*
	* @return a new {@link LinkedHashMap} containing the currently tracked URL load events,
	* preserving their load order, or an empty map if no events have been recorded.

[Copilot]

The getUrlLoadTracker() method exposes UrlLoadStatus instances which are package-private, creating an inconsistent API surface. Since getUrlLoadTracker() is a public method returning Map<Integer, UrlLoadStatus>, the UrlLoadStatus class becomes part of the public API contract even though it's not explicitly marked public. This creates several issues:

1. Callers outside the package cannot access the UrlLoadStatus type explicitly in their code
2. They can only use it via the Map interface or with raw types (losing type safety)
3. It's unclear whether external consumers should be using this or if it's internal-only
4. Future changes to UrlLoadStatus might accidentally break external consumers

Either make UrlLoadStatus explicitly public if external consumption is intended, or make getUrlLoadTracker() package-private/protected if it's only for internal/subclass use. The visibility should be consistent with the intended API surface.

[Copilot]

The URL tracking fields mUrlLoadTracker and mUrlLoadCounter are not thread-safe, but may be accessed from multiple threads. In Android's WebView, callbacks like shouldOverrideUrlLoading, onPageStarted, and onPageFinished can be invoked from different threads (typically the UI thread for the WebView, but this can vary). The mUrlLoadCounter increment operation (++mUrlLoadCounter) is not atomic, and the LinkedHashMap is not synchronized, which can lead to:

1. Lost updates to the counter due to race conditions
2. Concurrent modification exceptions when accessing the map
3. Inconsistent state in the map structure

To ensure thread safety, the trackUrlLoad method should be synchronized, and mUrlLoadCounter should either be protected by the same lock or converted to an AtomicInteger. Additionally, getUrlLoadTracker() should also be synchronized to prevent concurrent access while copying the map.