fix: address AI code review concerns — security, memory, stability, and bug fixes#47
Merged
fix: address AI code review concerns — security, memory, stability, and bug fixes#47
Conversation
Addresses concerns from code review council (issue #46): Phase 1 - Debug route gated behind production check Phase 2 - Rooms provider invalidation on sign-out to prevent state leak Phase 3 - Remove keepAlive from family providers to prevent memory leaks Phase 4 - Break circular dependency between provider files Phase 5 - Replace mock data with real device status in room statistics Phase 6 - Add error logging instead of silent error swallowing Phase 7 - Show error UI for SharedPreferences initialization failure Phase 8 - Document async onDispose behavior with unawaited() Phase 9 - Enable riverpod_lint and custom_lint Phase 10 - Move stream subscription after auth check Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove re-export from websocket_providers.dart to break circular dependency - Add direct import of websocket_sync_providers in repository_providers.dart - Add custom_lint plugin to analysis_options.yaml for riverpod_lint - Sort dev_dependencies alphabetically in pubspec.yaml Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> Co-Authored-By: OpenAI Codex <noreply@openai.com>
After removing the re-export from websocket_providers.dart, all files that use webSocketCacheIntegrationProvider, webSocketDataSyncServiceProvider, or webSocketDeviceLastUpdateProvider need to directly import websocket_sync_providers.dart. Updated files: - main.dart - initialization_provider.dart - settings_screen.dart - speed_test_providers.dart - image_upload_provider.dart - settings_riverpod_provider.dart - device_onboarding_provider.dart - health_notices_provider.dart - auth_notifier.dart - speed_test_card.dart - room_speed_test_selector.dart - device_speed_test_section.dart - network_overview_section.dart Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Addresses Issue #1 from code review - Insecure Credential Storage. Changes: - Add flutter_secure_storage package for platform-native encryption - Create SecureStorageService with iOS Keychain and Android Keystore - Migrate sensitive data (token, sessionToken, authSignature) to secure storage - Keep non-sensitive settings (theme, filters) in SharedPreferences - Add atomic migration: read -> write secure -> verify -> delete plaintext - Add migration flag to prevent re-migration - Update all callers to use async getToken() instead of sync getter - Handle FutureProvider properly for apiKeyProvider Security improvements: - iOS: Uses Keychain with afterFirstUnlock accessibility - Android: Uses EncryptedSharedPreferences backed by Keystore - Migration verifies writes before deleting plaintext - Failed migration allows retry on next launch See: #46 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Fixes from Codex review: 1. [High] Fix authSignature async access - auth_notifier.dart:157 now uses `await storage.getAuthSignature()` - Previously used deprecated sync getter that returned null 2. [Medium] Add verification to ATT legacy migration - Verify secure storage write before deleting legacy keys - Prevents credential loss if write fails 3. [High] Add migration for legacy api_url/api_token keys - Previously just deleted without migrating to secure storage - Now migrates token to secure storage before deletion - Verifies write before removing legacy keys Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Addresses Issue #2 from code review - Credentials/API keys logged. Changes: - Replace truncated API key logging with "[REDACTED, length=N]" - Add _redactUri() helper to deeplink_service.dart - Redact sensitive params (api_key, token, etc) from URI logs - Remove URL logging in rest_image_upload_service (contains api_key) Files changed: - lib/features/splash/presentation/screens/splash_screen.dart - lib/features/auth/presentation/providers/auth_notifier.dart - lib/features/auth/presentation/screens/auth_screen.dart - lib/core/services/deeplink_service.dart - lib/features/devices/data/services/rest_image_upload_service.dart Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Addresses Issue #3 from code review - keepAlive on family providers. Family providers with keepAlive: true create persistent instances for every unique parameter, causing unbounded memory growth. Changed from @riverpod(keepAlive: true) to @riverpod: - SpeedTestResultsNotifier (speed_test_providers.dart:98) - Parameters: speedTestId, accessPointId - RoomDeviceNotifier (room_device_view_model.dart:43) - Parameters: roomId Now auto-disposes when no longer watched, preventing memory leaks. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Addresses Issue #4 from code review - print() statements in production. print() calls bypass LoggerService and don't log in release builds. Replaced all print() statements with LoggerService calls or removed redundant debug logging. Files changed: - lib/features/rooms/presentation/providers/room_device_view_model.dart - Removed verbose device filtering debug logs - lib/features/devices/data/repositories/device_repository.dart - Removed redundant print() (already had logger.i()) - lib/features/issues/presentation/providers/health_notices_provider.dart - Replaced print() with LoggerService.debug() - lib/features/issues/data/datasources/health_notices_remote_data_source.dart - Rewrote with proper LoggerService calls Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Added roomLocalDataSourceProvider cache clearing to the sign-out cleanup to ensure room data from previous user doesn't persist after logout. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Pin flutter_cache_manager version to ^3.4.1 (Phase 11) - Use useSyntheticData flag instead of isDevelopment for mock data (Phase 12) - Remove analysis exclusions for mock data files (Phase 13) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
CacheManager now: - Runs periodic cleanup every 5 minutes to remove expired entries - Enforces max 100 entries with LRU eviction when exceeded - Properly disposes timer when provider is disposed Prevents unbounded memory growth from cache accumulation. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Clean up unused imports identified by flutter analyze. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add debug logging to websocket_data_sync_service when date parsing fails - Add explanatory comments to defensive catch blocks in room_readiness_data_source The defensive catches handle dynamic type access where objects may not have expected properties - this is intentional behavior, now documented. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add try-catch around WebSocket send() calls to handle race condition where connection can close between isConnected check and send() call. Note: Catches StateError (triggers lint info) because WebSocketService throws StateError for "not connected" - a future refactor could change this to a regular Exception. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add removeXxxCallback() methods to WebSocketCacheIntegration for: - deviceDataCallbacks - roomDataCallbacks - speedTestConfigCallbacks - speedTestResultCallbacks Previously callbacks could only be added (append-only), potentially causing memory issues if added repeatedly without cleanup. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove unused _reconstructDeviceId function from image_upload_provider - Remove unused _logger getter from device_onboarding_provider - Remove unused logger import Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove unused shared_preferences import from device_onboarding_provider - Remove unused websocket_providers import from settings_screen - Remove unused websocket_providers import from main.dart - Remove duplicate logger_service imports from main_development.dart - Remove duplicate logger_service imports from main_production.dart Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The WebSocket update_resource action silently ignores the images parameter, so delete requests never reached the backend. Switch to HTTP PUT via RestImageUploadService, matching ATT-FE-Tool's approach. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
zew-rgn
changed the title
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
Comprehensive fixes identified through multi-LLM code review (Codex, Gemini) addressing security vulnerabilities, memory leaks, stability issues, and a critical bug in device image deletion.
Security
Memory & Performance
keepAlivefrom family providers — prevents unbounded memory growth from parameterized Riverpod providers that were never disposedStability
send()against connection race conditions — prevents crashes when sending on a closing/closed connectionprint()withLoggerService— consistent, level-filtered logging across the appwebsocket_providers.dartandwebsocket_sync_providers.dartto resolve circular dependency issuesBug Fix
update_resourceaction silently ignores theimagesparameter, so delete requests never reached the backend. Switched to HTTP PUT viaRestImageUploadService, matching the ATT-FE-Tool reference implementationCleanup
Commits
f813921fix(security): implement code review council fixes (phases 1-10)c47b055fix(providers): address Codex review feedback069dcdafix(imports): add websocket_sync_providers imports to all consumersbb40f45fix(security): migrate credentials to secure storage972768cfix(security): address Codex review feedback on secure storagecd42782fix(security): remove credential logging (Issue Zew/development #2)88d01c2fix(memory): remove keepAlive from family providers (Issue Zew/web socket #3)3037363fix(logging): replace print() with LoggerService (Issue Fixed to websockets #4)4b5b118fix(auth): clear room cache on sign-out (Issue Dlp/riverpod #6)9703263fix(errors): add logging/comments to empty catch blocks (Issue Draft Dlp/add speedtest #7)17fda5ffix(config): address medium-priority review issues12f5b0dfix(cache): add periodic cleanup and max size limit (Issue Add deeplink functionality for FDK app login #9)d27a587chore: remove unused imports1ec4adffix(websocket): add callback removal methods to prevent memory leaks81566e2fix(websocket): guard send() calls against connection race conditionfbdf073chore: remove unused code elements1a14b2cchore: remove unused and duplicate imports56e6dacfix(devices): use REST API for image deletion instead of WebSocketTest plan
🤖 Generated with Claude Code