Merged
Conversation
Migrated DetailViewModel navigation events to Channel pattern: - Changed _navigationEvent from StateFlow to Channel - Changed _openUrlEvent from StateFlow to Channel - Changed _shareIntent from StateFlow to Channel - Updated all emission sites to use .send() instead of .update()/.value - Removed manual consumption methods (onNavigationEventConsumed, etc.) - Updated BookmarkDetailScreen to use LaunchedEffect(Unit) with collectLatest This eliminates race conditions and potential double-navigation bugs. Refs: Phase 1 Item #1 (High severity) from post-refactor-code-review.md Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Added eager initialization to prevent race condition where network requests could be made before the base URL is loaded from DataStore: - Use runBlocking to eagerly load initial URL value during init - Then start reactive collection for future updates - Changed getBaseUrl() to return non-null String with clear error message This prevents "baseUrl is not set" IOException on cold start. Refs: Phase 1 Item #2 (High severity) from post-refactor-code-review.md Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Enhanced fromStringList converter to prevent silent data loss: - Added Timber.w logging when deserialization fails - Added CSV fallback for v6 compatibility and data recovery - Log includes the corrupted value for debugging This makes it possible to diagnose data corruption issues in production. Refs: Phase 1 Item #3 (Medium severity) from post-refactor-code-review.md Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Ensured consistent state collection pattern in BookmarkListScreen: - All state flows now use collectAsState() without immediate .value - Keeps State objects for proper smart cast support - Access .value only when needed (e.g., pendingActionCount.value > 0) This improves recomposition behavior and code consistency. Refs: Phase 1 Item #5 (Low severity) from post-refactor-code-review.md Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Removed test for onNavigationEventConsumed() method which was removed as part of the Channel migration. Manual consumption is no longer needed with the Channel pattern. Refs: Phase 1 Item #1 from post-refactor-code-review.md Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Fixed 12 failing tests by updating mock signatures and expectations: 1. Updated observeBookmarkListItems mocks to include new parameters: - Added `label` parameter (was missing) - Added `orderBy` parameter (was missing) - Changed `archived` from null to false to match default FilterState 2. Fixed test expectations to match current implementation: - onClickMyList() now sets FilterState(archived=false), not (unread=true) - Updated test assertions to match actual behavior 3. Added proper mock setup for state transitions: - Mocked both initial state and post-action states where needed All 190 tests now pass successfully. Fixes pre-existing test failures from baseline commit. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
The state collection pattern change in Phase 1 Item 5 introduced a bug where `uiState` and `createBookmarkUiState` were collected as State<T> objects but used in `when` expressions that expected the actual values. This broke all type checks (is checks), causing blank screens across all views in the app. Fixed by adding `.value` back to state collections that are used in `when/is` expressions, while keeping State objects for those only accessed via .value (bookmarkCounts, labelsWithCounts, filterState). Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Added checkboxes to all Phase 1 and Phase 2 items, Quick Wins, and Sprint Plan sections. Marked all Phase 1 items as complete: - Navigation pattern migration (DetailViewModel to Channel) - BaseUrlProvider race condition fix - Silent deserialization logging - openUrlEvent migration to Channel - State collection pattern standardization - Blank screen bug fix Phase 2 items remain pending for next sprint. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Add offline page (CloudOff icon + "Offline" text) to Original WebView when device has no connectivity, with reactive NetworkCallback updates - Add fast-fail connectivity check in LoadArticleUseCase to avoid OkHttp's ~10s timeout when offline - Switch connectivity check from NET_CAPABILITY_INTERNET to NET_CAPABILITY_VALIDATED for accurate detection of actual connectivity - Fix detail screen overflow menu order: Find in Article, View Article/ Original, Open in Browser, Share Link, Is Read, Delete - Fix pendingActionCount.toString() rendering State wrapper instead of value in navigation drawer Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Reduce OkHttp connect timeout from default 10s to 3s for faster offline failure. Remove unneeded pending-actions indicator from drawer header, keeping only the CloudOff offline icon. Change reading mode offline icon tint to red (error color) for consistency. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Replace Open in Browser icon from Language to OpenInNew (external link) - Add 60% opacity to search highlights (yellow matches and orange current) - Increase edit title pencil icon size from 16dp to 20dp for visibility - Improve pencil icon contrast (onSurface at 80% vs onSurfaceVariant at 60%) Note: Search scrolling issue deferred to Material Design 3 review. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Restore Icons.filled.Language import needed for other menu items that toggle between Language and Description/Movie icons. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR completes a comprehensive foundational cleanup of the MyDeck codebase, improving code quality, testability, and maintainability.
Earlier Branch (Merged Locally)
Code Cleanup & Refactoring
Functional Enhancements
Current Branch (This PR)
Code Cleanup & Refactoring
Functional Enhancements