Diff between AI Clean arch branch and main#21
Conversation
…ide effects, will migrate to init state
WalkthroughRemoves iOS and macOS native project/workspace/resources and many Xcode config/includes; adds GetIt service locator and registers services; introduces Docker domain interfaces and implementations, CLI config and error-state models; refactors presentation to a BaseResourceScreen pattern and updates screens to use DI and domain services. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as main()
participant DI as Service Locator (GetIt)
participant UI as UI Screens
participant Domain as Domain Services
participant Repo as Repositories
participant SSH as SSHConnectionService
App->>DI: setupServiceLocator()
DI->>Repo: register repositories (Docker, Server, Registry)
DI->>SSH: register SSHConnectionService
DI->>Domain: register domain impls (Container/Image/Operations)
UI->>DI: getIt<T>()
DI-->>UI: instance
UI->>Domain: fetchItems / createContainer / pullImage
Domain->>Repo: construct/resolve command
Repo->>SSH: executeCommand(command)
SSH-->>Repo: stdout / stderr
Repo-->>Domain: parsed result or error
Domain-->>UI: data or error
UI->>UI: BaseResourceScreen renders loading / success / error
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Areas to focus during review:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/presentation/screens/server_list_screen.dart (1)
20-60: Guard asyncsetStatecalls withmountedto avoidsetState-after-dispose
_loadCurrentServerIdand_loadServersbothawaitrepository calls and then callsetStatewithout checkingmounted, so if the screen is popped while those futures are in flight you can hitsetState()after dispose. The new DI wiring is fine; this is just an existing async safety gap worth tightening.You can harden both methods like this:
Future<void> _loadCurrentServerId() async { try { - final currentServerId = await _serverRepository.getLastUsedServerId(); - setState(() { - _currentServerId = currentServerId; - }); + final currentServerId = await _serverRepository.getLastUsedServerId(); + if (!mounted) return; + setState(() { + _currentServerId = currentServerId; + }); } catch (e) { // Handle error silently } } Future<void> _loadServers() async { try { setState(() => _isLoading = true); final servers = await _serverRepository.getServers(); - setState(() { - _servers = servers; - _isLoading = false; - }); + if (!mounted) return; + setState(() { + _servers = servers; + _isLoading = false; + }); } catch (e) { - setState(() => _isLoading = false); + if (!mounted) return; + setState(() => _isLoading = false); if (mounted) { ScaffoldMessenger.of(context).showSnackBar( SnackBar(content: Text('Failed to load servers: $e')), ); } } }lib/presentation/screens/images/build_image_screen.dart (1)
19-138: Addmountedchecks around asyncsetStatein_buildImageThe new service-based build flow looks good, but
_buildImagecallssetStateafter awaiting_imageManagementService.buildImageand again in thecatchblock without checkingmounted, which can triggersetState()after dispose if the user navigates away mid-build.A minimal fix:
Future<void> _buildImage() async { @@ - // Use service to build image - final result = await _imageManagementService.buildImage(config); - - setState(() { - _buildLogs += result; - }); - _scrollToBottom(); + // Use service to build image + final result = await _imageManagementService.buildImage(config); + + if (!mounted) return; + + setState(() { + _buildLogs += result; + }); + _scrollToBottom(); @@ - } catch (e) { - setState(() { - _buildLogs += '\nError: $e\n'; - _isBuilding = false; - }); - _scrollToBottom(); - - if (mounted) { + } catch (e) { + if (!mounted) return; + setState(() { + _buildLogs += '\nError: $e\n'; + _isBuilding = false; + }); + _scrollToBottom(); + + if (mounted) { ScaffoldMessenger.of(context).showSnackBar( SnackBar( content: Text('Error: $e'),
🧹 Nitpick comments (22)
lib/data/services/ssh_connection_service.dart (1)
41-42: Consider making the setter private or removing it.The public setter allows external code to set
_currentServerwithout establishing an actual connection, which could lead to inconsistent state where_currentServeris set but_currentConnectionand_statusdon't match.If the setter is needed for specific use cases, consider documenting when and why it should be used, or restrict its visibility:
- // Setter for currentServer (used when server is selected before connection) - set currentServer(Server? server) => _currentServer = server;Alternatively, if this is truly needed, add validation or documentation explaining the intended usage pattern.
lib/presentation/screens/containers/create_container_screen.dart (1)
638-651: Consider reusing domain models for UI state.The local data classes
PortMapping,EnvVariable, andVolumeMount(lines 638-651) duplicate the structure of domain models defined inlib/domain/services/container_creation_service.dart. The only difference is mutability for UI binding.Consider one of these approaches to reduce duplication:
- Make domain models mutable (if appropriate for your architecture)
- Use a ValueNotifier or similar pattern to wrap immutable domain models
- Keep the current approach if the separation between mutable UI models and immutable domain models is intentional
The current implementation works but creates maintenance overhead when domain models change.
lib/data/services/docker_operations_service_impl.dart (1)
15-99: Commands and wiring look correct; consider small DRY helper laterThe per-method command strings and use of
DockerCliConfig.getCliPath()andsshService.executeCommandall look correct and consistent. There is some repetition in fetching the CLI and interpolating commands, but that’s a minor DRY opportunity you can defer (e.g., a small private helper that returns'$dockerCli $subcommand').lib/data/repositories/docker_repository_impl.dart (2)
12-42: Error parsing helper is useful but has some platform-specific assumptionsThe
_parseDockerErrorheuristics provide much more user-friendly messages, which is great. A couple of caveats to keep in mind:
- Matching on very broad substrings like
'no such file or directory'may misclassify unrelated file errors as “Docker not found”.- The remediation text (docker group,
sudo systemctl start docker) assumes a Linux/systemd environment and correct group-based permissions; if you ever support non-Linux or non-systemd targets, you may want to gate these messages on detected platform/connection metadata or generalize the wording.
52-73: CLI path centralization and error wrapping look good; consider a small shared helperUsing
DockerCliConfig.getCliPath()everywhere and routing all exceptions through_parseDockerErroris a solid improvement for consistency and UX. The only minor thing you might consider later is extracting a small helper likeFuture<String> _runDockerCommand(String args)that:
- Resolves the CLI path once.
- Executes the command.
- Applies the common “null/empty output” and error wrapping logic.
This would remove a bit of repetition across
getContainers,getContainerStats,getImages,getVolumes, andgetNetworks, and make it easier to adjust quoting/escaping ofdockerCliin one place if needed.Also applies to: 83-115, 124-135, 144-155, 164-175
lib/presentation/screens/images/pull_image_screen.dart (1)
161-200: Pull flow delegation is clean; consider future reuse of error presentationDelegating the pull to
_imageManagementService.pullImage(fullImageName)and handling success via a green SnackBar +Navigator.of(context).pop(true)is straightforward and keeps the UI logic simple. As you roll out the shared error/empty-state widgets elsewhere, you might eventually want to align the error handling here with that pattern (e.g., surfacing richer messages from the domain service), but the current flow is perfectly fine.lib/core/utils/error_state.dart (1)
3-76: ErrorState is a solid central model; just be awareemptyis server-specificThis is a nice, compact way to standardize error/empty/permission configurations across screens. One small semantic detail:
ErrorState.emptycurrently hard-codes the headline to'No Servers Found', which makes it tightly coupled to the server use case. If you later want to reuse this for other “no data” resources, consider either:
- Renaming this factory to something server-specific, or
- Making the headline a parameter with a sensible default.
lib/presentation/widgets/resource_state_widgets.dart (1)
4-193: Reusable state widgets are clear; consider surfacing the search queryThe loading, error, and empty widgets are concise and will help keep individual screens lean and consistent. In
NoSearchResultsWidget, you already accept aquerystring but don’t show it in the UI; it could be helpful to users to see something likeNo results found for "foo"instead of the generic text, or else drop the parameter if you don’t plan to use it.lib/domain/services/docker_operations_service.dart (1)
1-25: Domain service contract looks coherent and matches the operations you needThe
DockerOperationsServiceabstraction cleanly groups container/image/volume/network operations and gives the UI a single domain-level entry point. ReturningFuture<String>for the “get*Command” methods makes sense if implementations need to compose commands with async config (e.g., CLI path). Just ensure call sites clearly treat those as shell commands (vs. logs/JSON content) so the naming doesn’t cause confusion later.lib/presentation/screens/base/base_resource_screen.dart (4)
64-181: Connection/error classification works but relies on a magic message markerThe
_loadItems/_isConnectionError/_isPermissionErrorflow is a nice centralization of error handling, but a couple of implementation details are brittle:
- Using
ErrorState.connection(message: '__RECONNECT__', ...)and then checkingerror?.message == '__RECONNECT__'in multiple places overloads themessagefield with control semantics. It would be more robust to model this explicitly (e.g., a dedicated enum/flag onErrorStateor a small sealed result type likeLoadResult.success/connectionError/otherError).- The string heuristics (
'connection','ssh','timeout','refused','closed'/'permission denied','docker group', etc.) are reasonable, but they are broad; keep in mind that they may catch unrelated messages in edge cases. Centralizing them here mitigates that risk, though.If this logic starts to grow, extracting a small “error classifier” helper or type would make it easier to reason about and test.
Also applies to: 183-201
296-397: Build-triggered connect/load is fine, but the debug SnackBar is easy to forgetThe
needsConnection/needsLoadchecks insidebuildcombined withisLoadinggating are a pragmatic way to bootstrap connection + loading without extra lifecycle hooks, and the async chaining looks sound.For the debug section:
- The SnackBar with a random number is helpful while diagnosing the state machine, but because it’s wired from
buildviaaddPostFrameCallback, it will fire on every rebuild in debug and can flood the UI. It’d be safer to either guard it behind an additional debug flag or remove it once you’re confident in the flow.
455-507: Connection failure / no-server UX could offer a direct settings/action affordanceWhen
_connectToServerfinds no stored server, it returnsErrorState.emptywith a message instructing the user to add a server via the settings menu, butonRetryisnull, so the error UI (Lines 414–449) ends up with no primary action. Consider:
- Providing an
onRetry(or dedicatedonAction) callback that navigates toSettingsScreen, or- Using a more specialized
ErrorStatevariant for this case that the UI can render with a “Go to Settings” button.This would restore the direct affordance that existed in the older, commented-out
buildErrorStatelogic.Also applies to: 414-449
203-245: Legacy stubs and commented blocks are fine short-term but worth cleaning up later
loadItems,refreshItems,_startServerChangeDetection,didChangeAppLifecycleState,buildBody, andbuildErrorStateare effectively dead code now (either no-op or entirely commented). Keeping them during migration is understandable, but once the new flow is stable it’d be good to either:
- Remove them, or
- Reintroduce them in terms of the new
errorState+_loadItems/_connectToServermodel.That will reduce confusion for future readers trying to understand which paths are actually in use.
Also applies to: 254-293, 509-641
lib/presentation/screens/containers_screen.dart (4)
22-65: Background stats loading can race with repeated refreshes
fetchItemsschedules_loadContainerStats()on a delayed future, and_loadContainerStatsthen merges stats into whateveritemshappens to be at execution time. IfloadItems()is triggered again while a previous stats fetch is in flight, an older stats result can overwrite neweritems/filteredItems.Consider capturing the
containerslist (or a version/token) insidefetchItemsand having_loadContainerStatsoperate only if the state still corresponds to that fetch, to avoid stale overwrites.
67-109: Filtering and stack-chip behavior look solid; consider clarifying the 'no-stack' sentinelThe combined stack + text filtering logic is straightforward and efficient, and
_getAvailableStackscorrectly derives sorted unique stack names fromitems. One minor UX edge case is overloading the literal'no-stack'as a sentinel; if a real stack were ever named"no-stack", it would be indistinguishable from the synthetic “No Stack” filter.If that’s a plausible scenario, consider using a private constant or enum-like type for the sentinel instead of a raw string.
117-359: Item card implementation is robust; think about failure-state handling for statsThe card rendering is well-structured, with defensive ID truncation (
substringguarded bylength > 12), clear stack labeling, and a good use of helper builders for detail rows and stats columns.When stats loading fails, though, running containers will show the “Loading stats...” spinner indefinitely. If stats failures are common (e.g., remote daemon issues), it may be worth tracking a “statsFailed” flag (per list or per container) so you can either hide the stats section or show an error message instead of a permanent spinner.
537-661: Action routing via stringly-typedaction.commandis brittle; loading SnackBar duration may be shortThe overall flow—special-casing logs/inspect/exec, then showing a progress SnackBar and delegating state changes to
operationsService—is clean and well-guarded withmountedchecks.Two improvement points:
- Using
action.commandstring literals ('docker stop','docker rm', etc.) as the switch key is fragile: adding a newDockerActionwithout updating this switch will silently result in a no-op but still show success UI. Consider switching on an enum or dedicatedaction.typefield, and/or adding adefaultcase that logs unexpected commands.- The loading SnackBar uses a fixed 2-second duration; long-running operations may complete after it disappears, leaving a period with no visual feedback. You could make it longer or use an indefinite SnackBar and explicitly dismiss it once the operation finishes (before showing success/error).
lib/presentation/screens/images_screen.dart (2)
42-219: Guard image ID truncation and unify display naming for unnamed imagesTwo small improvements in the card builder:
'ID: ${image.imageId.substring(0, 12)}'assumesimage.imageId.length >= 12. Docker IDs are typically long enough, but for consistency with the containers screen (where you already guard on length) and to avoid potentialRangeErrors from unexpected data, consider:- 'ID: ${image.imageId.substring(0, 12)}', + 'ID: ${image.imageId.length > 12 ? image.imageId.substring(0, 12) : image.imageId}',
- For unnamed images (
repository == '<none>'), the card uses'Unnamed Image', but the actionresourceNameand the inspect ShellScreen title display${image.repository}:${image.tag}, which will show<none>:<none>. You may want to reuse the same human-friendly name used in the card to keep titles and dialogs consistent.
305-374: Image action handler is solid; consider reducing reliance on command stringsThe inspect/delete flows are correctly funneled through
operationsService, with confirmation for deletions andmountedguarding around SnackBars. Behavior-wise this looks good.Similar to the containers screen, routing based on
action.commandstring literals ('docker image inspect','docker image rm') is somewhat brittle. Introducing an enum/actionTypeonDockerActionand switching on that would make adding new actions safer and avoid silent no-ops if command strings drift.lib/presentation/screens/volumes_screen.dart (1)
212-277: Volume action handler is correct; consider standardizing refresh behavior and action routingThe inspect and delete flows correctly use
operationsService, guard SnackBars withmounted, and refresh the list after deletion vialoadItems().Two small consistency improvements:
- Here you call
loadItems();withoutawait, whereas_handleContainerActionawaitsloadItems(). Since callers don’t currently await_handleVolumeAction, this isn’t a bug, but standardizing on either awaiting or fire-and-forget across all handlers will make behavior more predictable (and tests easier to reason about).- As with other screens, switching on
action.commandstring values ('docker volume inspect','docker volume rm') is a bit fragile; a typedactionTypewould be more robust.lib/presentation/screens/networks_screen.dart (2)
41-201: Guard network ID truncation and centralize system-network detectionThe card rendering is clear, especially the visual distinction for system networks.
Two small robustness improvements:
'ID: ${network.networkId.substring(0, 12)}'assumesnetwork.networkId.length >= 12. For defensive coding and consistency with the containers screen, consider guarding the substring:- 'ID: ${network.networkId.substring(0, 12)}', + 'ID: ${network.networkId.length > 12 ? network.networkId.substring(0, 12) : network.networkId}',
- The list of system networks
['bridge', 'host', 'none']is defined here to color the icon and duplicated in_handleNetworkActionto block deletions. Extracting this into a shared helper/constant (e.g.,bool isSystemNetwork(String name)) would avoid drift if you ever adjust that list.
287-363: Network actions correctly guard system networks; minor refactor opportunitiesThe action handler appropriately:
- Uses
operationsServicefor inspect/delete.- Prevents deletion of system networks with an immediate SnackBar.
- Confirms destructive deletes and refreshes the list via
loadItems().- Wraps everything in a
try/catchwithmounted-guarded error SnackBars.As in the other screens, you might consider:
- Sharing the system-network detection logic with the card (
isSystemNetworkhelper/constant).- Replacing the string-based
action.commandswitch with a typed discriminator onDockerActionto avoid silent no-ops if commands change.Functionally, though, this is solid as-is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (32)
ios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-1024x1024@1x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-20x20@1x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-20x20@2x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-20x20@3x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-29x29@1x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-29x29@2x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-29x29@3x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-40x40@1x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-40x40@2x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-40x40@3x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-50x50@1x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-50x50@2x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-57x57@1x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-57x57@2x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-60x60@2x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-60x60@3x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-72x72@1x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-72x72@2x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-76x76@1x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-76x76@2x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-83.5x83.5@2x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/LaunchImage.imageset/LaunchImage.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/LaunchImage.imageset/LaunchImage@2x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/LaunchImage.imageset/LaunchImage@3x.pngis excluded by!**/*.pngmacos/Runner/Assets.xcassets/AppIcon.appiconset/app_icon_1024.pngis excluded by!**/*.pngmacos/Runner/Assets.xcassets/AppIcon.appiconset/app_icon_128.pngis excluded by!**/*.pngmacos/Runner/Assets.xcassets/AppIcon.appiconset/app_icon_16.pngis excluded by!**/*.pngmacos/Runner/Assets.xcassets/AppIcon.appiconset/app_icon_256.pngis excluded by!**/*.pngmacos/Runner/Assets.xcassets/AppIcon.appiconset/app_icon_32.pngis excluded by!**/*.pngmacos/Runner/Assets.xcassets/AppIcon.appiconset/app_icon_512.pngis excluded by!**/*.pngmacos/Runner/Assets.xcassets/AppIcon.appiconset/app_icon_64.pngis excluded by!**/*.pngpubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (68)
ios/.gitignore(0 hunks)ios/Flutter/AppFrameworkInfo.plist(0 hunks)ios/Flutter/Debug.xcconfig(0 hunks)ios/Flutter/Release.xcconfig(0 hunks)ios/Runner.xcodeproj/project.pbxproj(0 hunks)ios/Runner.xcodeproj/project.xcworkspace/contents.xcworkspacedata(0 hunks)ios/Runner.xcodeproj/project.xcworkspace/xcshareddata/IDEWorkspaceChecks.plist(0 hunks)ios/Runner.xcodeproj/project.xcworkspace/xcshareddata/WorkspaceSettings.xcsettings(0 hunks)ios/Runner.xcodeproj/xcshareddata/xcschemes/Runner.xcscheme(0 hunks)ios/Runner.xcworkspace/contents.xcworkspacedata(0 hunks)ios/Runner.xcworkspace/xcshareddata/IDEWorkspaceChecks.plist(0 hunks)ios/Runner.xcworkspace/xcshareddata/WorkspaceSettings.xcsettings(0 hunks)ios/Runner/AppDelegate.swift(0 hunks)ios/Runner/Assets.xcassets/AppIcon.appiconset/Contents.json(0 hunks)ios/Runner/Assets.xcassets/LaunchImage.imageset/Contents.json(0 hunks)ios/Runner/Assets.xcassets/LaunchImage.imageset/README.md(0 hunks)ios/Runner/Base.lproj/LaunchScreen.storyboard(0 hunks)ios/Runner/Base.lproj/Main.storyboard(0 hunks)ios/Runner/Info.plist(0 hunks)ios/Runner/Runner-Bridging-Header.h(0 hunks)ios/RunnerTests/RunnerTests.swift(0 hunks)lib/core/di/service_locator.dart(1 hunks)lib/core/utils/docker_cli_config.dart(1 hunks)lib/core/utils/error_state.dart(1 hunks)lib/data/repositories/docker_repository_impl.dart(11 hunks)lib/data/services/container_creation_service_impl.dart(1 hunks)lib/data/services/docker_operations_service_impl.dart(1 hunks)lib/data/services/image_management_service_impl.dart(1 hunks)lib/data/services/ssh_connection_service.dart(4 hunks)lib/domain/services/container_creation_service.dart(1 hunks)lib/domain/services/docker_operations_service.dart(1 hunks)lib/domain/services/image_management_service.dart(1 hunks)lib/main.dart(1 hunks)lib/presentation/screens/base/base_resource_screen.dart(1 hunks)lib/presentation/screens/containers/create_container_screen.dart(4 hunks)lib/presentation/screens/containers_screen.dart(7 hunks)lib/presentation/screens/home_screen.dart(5 hunks)lib/presentation/screens/images/build_image_screen.dart(3 hunks)lib/presentation/screens/images/pull_image_screen.dart(4 hunks)lib/presentation/screens/images_screen.dart(5 hunks)lib/presentation/screens/networks_screen.dart(5 hunks)lib/presentation/screens/server_list_screen.dart(2 hunks)lib/presentation/screens/settings_screen.dart(4 hunks)lib/presentation/screens/shell_screen.dart(2 hunks)lib/presentation/screens/volumes_screen.dart(4 hunks)lib/presentation/widgets/resource_state_widgets.dart(1 hunks)macos/.gitignore(0 hunks)macos/Flutter/Flutter-Debug.xcconfig(0 hunks)macos/Flutter/Flutter-Release.xcconfig(0 hunks)macos/Flutter/GeneratedPluginRegistrant.swift(0 hunks)macos/Runner.xcodeproj/project.pbxproj(0 hunks)macos/Runner.xcodeproj/project.xcworkspace/xcshareddata/IDEWorkspaceChecks.plist(0 hunks)macos/Runner.xcodeproj/xcshareddata/xcschemes/Runner.xcscheme(0 hunks)macos/Runner.xcworkspace/contents.xcworkspacedata(0 hunks)macos/Runner.xcworkspace/xcshareddata/IDEWorkspaceChecks.plist(0 hunks)macos/Runner/AppDelegate.swift(0 hunks)macos/Runner/Assets.xcassets/AppIcon.appiconset/Contents.json(0 hunks)macos/Runner/Base.lproj/MainMenu.xib(0 hunks)macos/Runner/Configs/AppInfo.xcconfig(0 hunks)macos/Runner/Configs/Debug.xcconfig(0 hunks)macos/Runner/Configs/Release.xcconfig(0 hunks)macos/Runner/Configs/Warnings.xcconfig(0 hunks)macos/Runner/DebugProfile.entitlements(0 hunks)macos/Runner/Info.plist(0 hunks)macos/Runner/MainFlutterWindow.swift(0 hunks)macos/Runner/Release.entitlements(0 hunks)macos/RunnerTests/RunnerTests.swift(0 hunks)pubspec.yaml(2 hunks)
💤 Files with no reviewable changes (42)
- macos/Runner/Configs/Debug.xcconfig
- macos/Runner.xcworkspace/xcshareddata/IDEWorkspaceChecks.plist
- ios/Runner/Base.lproj/LaunchScreen.storyboard
- macos/RunnerTests/RunnerTests.swift
- macos/.gitignore
- ios/Runner.xcodeproj/project.xcworkspace/contents.xcworkspacedata
- ios/Runner/Assets.xcassets/LaunchImage.imageset/README.md
- ios/Runner/Assets.xcassets/AppIcon.appiconset/Contents.json
- macos/Runner/DebugProfile.entitlements
- macos/Flutter/GeneratedPluginRegistrant.swift
- macos/Runner/Configs/Warnings.xcconfig
- macos/Runner.xcworkspace/contents.xcworkspacedata
- ios/RunnerTests/RunnerTests.swift
- macos/Runner/Info.plist
- ios/Runner/Runner-Bridging-Header.h
- macos/Runner/Assets.xcassets/AppIcon.appiconset/Contents.json
- macos/Runner/Configs/AppInfo.xcconfig
- ios/Runner.xcodeproj/project.xcworkspace/xcshareddata/IDEWorkspaceChecks.plist
- macos/Flutter/Flutter-Debug.xcconfig
- macos/Runner/AppDelegate.swift
- macos/Runner.xcodeproj/xcshareddata/xcschemes/Runner.xcscheme
- ios/Runner/Info.plist
- ios/Runner.xcodeproj/project.pbxproj
- ios/Runner/AppDelegate.swift
- macos/Flutter/Flutter-Release.xcconfig
- ios/Runner.xcworkspace/contents.xcworkspacedata
- ios/Runner.xcodeproj/xcshareddata/xcschemes/Runner.xcscheme
- ios/Flutter/Release.xcconfig
- macos/Runner.xcodeproj/project.pbxproj
- ios/Runner/Base.lproj/Main.storyboard
- ios/Flutter/Debug.xcconfig
- ios/.gitignore
- ios/Runner.xcworkspace/xcshareddata/IDEWorkspaceChecks.plist
- ios/Flutter/AppFrameworkInfo.plist
- macos/Runner/Base.lproj/MainMenu.xib
- ios/Runner.xcodeproj/project.xcworkspace/xcshareddata/WorkspaceSettings.xcsettings
- ios/Runner.xcworkspace/xcshareddata/WorkspaceSettings.xcsettings
- macos/Runner/Release.entitlements
- macos/Runner.xcodeproj/project.xcworkspace/xcshareddata/IDEWorkspaceChecks.plist
- macos/Runner/MainFlutterWindow.swift
- ios/Runner/Assets.xcassets/LaunchImage.imageset/Contents.json
- macos/Runner/Configs/Release.xcconfig
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build Windows Release
🔇 Additional comments (20)
lib/domain/services/container_creation_service.dart (1)
1-72: LGTM!Clean domain service abstraction with well-structured configuration models. The separation of concerns between domain interfaces and implementation is good architecture.
pubspec.yaml (1)
19-19: LGTM!Version bump and GetIt dependency addition are appropriate for introducing dependency injection infrastructure.
Also applies to: 40-40
lib/presentation/screens/settings_screen.dart (1)
33-43: LGTM!Clean migration to centralized Docker CLI configuration. The consistent use of
DockerCliConfiginstead of direct SharedPreferences access improves maintainability.Also applies to: 62-78, 136-146
lib/presentation/screens/shell_screen.dart (1)
8-8: LGTM on DockerCliConfig usage, but verify command escaping.The migration to
DockerCliConfig.getCliPath()is consistent with the broader refactoring.However, verify that line 375's command construction is safe:
actualCommand = '$dockerCli exec $containerId $executable -c "$command"';The
$commandvariable contains user input that's wrapped in double quotes. If the user enters a command containing double quotes or other shell metacharacters, this could lead to command injection. For example, input like"; rm -rf /; "would break out of the quotes.Consider using proper shell escaping or parameterized command execution if available in your SSH library.
Also applies to: 370-376
lib/main.dart (1)
6-11: No changes required — DI setup is confirmed synchronous.Verification confirms that
setupServiceLocator()contains only synchronous operations with no async/await patterns or Future returns. All service registrations use direct instantiation, so services are immediately available after the function completes. The original concern about potential race conditions does not apply to this implementation.lib/core/di/service_locator.dart (1)
15-53: DI wiring via GetIt is cohesive and consistentThe service locator setup cleanly registers core infrastructure, repositories, and domain services as singletons, and the dependency graph (e.g., DockerOperationsServiceImpl needing DockerRepository + SSHConnectionService) is wired correctly. The “call once at startup” contract is clear and appropriate for this pattern.
lib/domain/services/image_management_service.dart (1)
1-22: Image management domain interface is clean and focusedThe
ImageManagementServicecontract andImageBuildConfigshape the image APIs nicely and should keep the UI decoupled from SSH/CLI details. Good separation of concerns.lib/core/utils/docker_cli_config.dart (1)
3-32: Centralized Docker CLI configuration utility looks solid
DockerCliConfigcleanly encapsulates persistence of the CLI path and command construction logic, which should make future switches (e.g., to podman) localized and easy to manage.lib/presentation/screens/home_screen.dart (1)
26-102: HomeScreen DI and server-selection flow look well-structuredResolving
ServerRepositoryandSSHConnectionServicevia the service locator ininitState, reloading the last used server on resume, and explicitly updating_sshService.currentServer/didServerChangein_selectServergive you a clear, DI-friendly lifecycle. The_pagesgetter using fresh screen instances on rebuild also plays nicely with the shared SSH state.lib/presentation/screens/images/pull_image_screen.dart (1)
3-5: DI usage for registry and image management services looks consistentResolving
DockerRegistryServiceandImageManagementServiceviagetItininitStatealigns with the new DI setup and keeps the widget free of direct SSH/CLI concerns. Thelate finalpattern here is appropriate and plays nicely with hot reload and testability (you can override registrations in tests as needed).Also applies to: 14-15, 24-29
lib/presentation/screens/base/base_resource_screen.dart (1)
19-37: Base state and DI wiring look aligned with the new architectureThe base state class cleanly injects
SSHConnectionService,DockerRepository,DockerOperationsService, andServerRepositoryviagetIt, and the abstract methods (fetchItems,filterItems,getResourceName,getEmptyIcon,buildItemCard) give concrete screens a clear contract. This should significantly reduce per-screen boilerplate once consumers are migrated.Also applies to: 55-60
lib/presentation/screens/containers_screen.dart (2)
361-535: Empty / no-results / list composition is consistent and leverages the base APIThe empty state, no-results state, and main item list all consistently reuse
SearchBarWithSettings, the stack filter chips, and the baseloadItems/onSearchChangedwiring. The composition looks correct and matches the BaseResourceScreen contract; no issues from a behavior or UX standpoint.
663-762: Shell executable dialog is nicely designed; controller lifecycle is acceptable hereThe shell selection dialog with pre-populated common executables and integration with
_openInteractiveShellprovides flexible UX and clean navigation (closing the dialog before pushing a new route). Creating theTextEditingControllerinside_showShellExecutableDialogis fine given the dialog’s short lifetime; there’s no pressing need to promote it to state just to dispose it.lib/presentation/screens/images_screen.dart (2)
18-33: Filtering implementation is straightforward and correctThe image filtering logic (repository/tag/ID/size, all lowercased) is clear and efficient, and returning the original list when
query.isEmptyavoids unnecessary allocations. No issues here.
221-303: Empty, no-results, and list states align well with the base patternThe empty state, no-search-results state, and main
buildItemListimplementation are consistent with other screens and correctly hook intoonSearchChangedandfilteredItems. The “Pull down to refresh” hint is clear given a pull-to-refresh container; no functional concerns.lib/presentation/screens/volumes_screen.dart (3)
18-31: Volume fetching and filtering are straightforward and correct
fetchItemsdelegating todockerRepository.getVolumes()andfilterItemsmatching onvolumeNameanddriver(with early return for empty query) are both idiomatic and efficient. No functional issues here.
40-126: Volume card UI is clear and idiomaticThe volume card cleanly displays the name and driver with appropriate theming and makes good use of
DockerResourceActions. The use ofconst Iconand constTextStylewhere possible is a nice touch for small performance wins. No changes needed.
128-210: Empty/no-results/list implementations are consistent with other resource screensThe volume-specific empty state, filtered list, and search bar wiring correctly follow the BaseResourceScreen pattern and reuse
SearchBarWithSettings. UX and behavior look consistent with images/networks.lib/presentation/screens/networks_screen.dart (2)
18-32: Network fetching and filtering logic is soundThe networks are fetched via
dockerRepository.getNetworks(), andfilterItemscorrectly matches on name, driver, or ID with an early return when the query is empty. No issues here.
203-285: Empty/no-results/list implementations are consistent and correctThe networks empty state, no-results variant, and
buildItemListcorrectly reuseSearchBarWithSettings,filteredItems, and the base callbacks. UX and wiring look aligned with the other resource screens.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
lib/presentation/screens/base/base_resource_screen.dart (3)
18-18: Unused mixin:AutomaticKeepAliveClientMixin.The
AutomaticKeepAliveClientMixinis included butwantKeepAlivereturnsfalse(line 34), which means the mixin has no effect. This mixin is typically used to keep the state alive when a tab or page is not visible.If you don't need to keep the state alive when navigating away, remove the mixin:
-abstract class BaseResourceScreenState<T, W extends BaseResourceScreen<T>> - extends State<W> with AutomaticKeepAliveClientMixin, WidgetsBindingObserver { +abstract class BaseResourceScreenState<T, W extends BaseResourceScreen<T>> + extends State<W> with WidgetsBindingObserver {And remove the override:
- @override - bool get wantKeepAlive => false;
39-39: UnusedWidgetsBindingObserverregistration.
WidgetsBindingObserveris added ininitStateand removed indispose, but no observer methods (likedidChangeAppLifecycleState) are overridden in the class. This registration has no effect.If you don't need app lifecycle observation, remove the mixin and registration:
abstract class BaseResourceScreenState<T, W extends BaseResourceScreen<T>> - extends State<W> with AutomaticKeepAliveClientMixin, WidgetsBindingObserver { + extends State<W> {@override void initState() { super.initState(); - WidgetsBinding.instance.addObserver(this); _initializeScreen(); } @override void dispose() { - WidgetsBinding.instance.removeObserver(this); super.dispose(); }Also applies to: 46-46
133-136: State mutation outsidesetState.Lines 134-135 mutate
itemsandfilteredItemsdirectly without wrapping insetState. While this is technically allowed for fields that don't affect the current widget tree, it's inconsistent with Flutter best practices and can lead to confusion.Wrap state mutations in
setStatefor consistency:if (mounted) { + setState(() { items = fetchedItems; filteredItems = filterItems(fetchedItems, searchQuery); + }); }Note: This is safe to do here because the caller (
_loadItemsAndUpdateState) already handles thesetStateforisLoadinganderrorStateafter this method returns.lib/presentation/screens/containers_screen.dart (2)
26-30: Potential race condition with delayed stats loading.The delayed stats loading (lines 26-30) only checks
mountedbut doesn't verify that the screen hasn't been refreshed or reinitialized. If the user triggers a refresh before the delay completes,_loadContainerStatswill execute with potentially stale data.Add a generation counter to track refresh cycles:
+ int _dataGeneration = 0; + @override Future<List<DockerContainer>> fetchItems() async { final containers = await dockerRepository.getContainers(); + final currentGeneration = ++_dataGeneration; // Fetch stats asynchronously in the background after showing containers Future.delayed(const Duration(milliseconds: 100), () { - if (mounted) { + if (mounted && _dataGeneration == currentGeneration) { _loadContainerStats(); } });
591-609: Loading SnackBar not cleared on error.Lines 591-609 show a loading SnackBar with a 2-second duration. However, if the operation fails quickly (within 2 seconds), the loading SnackBar will overlap with the error SnackBar shown at lines 648-661. While
clearSnackBarsis called at line 648, this creates visual noise.Show the loading SnackBar with an indefinite duration and clear it explicitly:
if (mounted) { ScaffoldMessenger.of(context).showSnackBar( SnackBar( content: Row( children: [ const SizedBox( width: 16, height: 16, child: CircularProgressIndicator(strokeWidth: 2), ), const SizedBox(width: 12), Text('${action.label}...'), ], ), - duration: const Duration(seconds: 2), + duration: const Duration(days: 365), // Indefinite ), ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
lib/presentation/screens/base/base_resource_screen.dart(1 hunks)lib/presentation/screens/containers_screen.dart(8 hunks)lib/presentation/screens/images_screen.dart(5 hunks)lib/presentation/screens/networks_screen.dart(5 hunks)lib/presentation/screens/volumes_screen.dart(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: Build Windows Release
| Future<void> _initializeScreen() async { | ||
| setState(() { | ||
| isLoading = true; | ||
| errorState = null; | ||
| }); | ||
|
|
||
| // Check if we need to connect | ||
| final needsConnection = sshService.didServerChange || !sshService.isConnected; | ||
|
|
||
| if (needsConnection) { | ||
| // Reset server change flag | ||
| sshService.didServerChange = false; | ||
|
|
||
| // Try to connect | ||
| final connectionError = await _connectToServer(); | ||
|
|
||
| if (connectionError != null) { | ||
| // Connection failed | ||
| if (mounted) { | ||
| setState(() { | ||
| isLoading = false; | ||
| errorState = connectionError; | ||
| }); | ||
| } | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| // Connection good (or already connected), load data | ||
| await _loadItemsAndUpdateState(); | ||
| } | ||
|
|
||
| // Public method for child classes to trigger refresh | ||
| void refresh() { | ||
| _initializeScreen(); | ||
| } | ||
|
|
||
| // Common refresh logic for pull-to-refresh | ||
| Future<void> refreshItems() async { | ||
| await _initializeScreen(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Missing concurrency guard for initialization.
_initializeScreen can be called multiple times concurrently from:
initState(line 41)refresh(line 85)refreshItems(line 90)- Server change detection in
build(line 244)
This can lead to race conditions where multiple connection attempts and data fetches occur simultaneously, potentially causing inconsistent state or duplicate operations.
Add a guard to prevent concurrent initialization:
+ bool _isInitializing = false;
+
Future<void> _initializeScreen() async {
+ if (_isInitializing) return;
+ _isInitializing = true;
+
setState(() {
isLoading = true;
errorState = null;
});
// ... rest of method
// Connection good (or already connected), load data
await _loadItemsAndUpdateState();
+ _isInitializing = false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Future<void> _initializeScreen() async { | |
| setState(() { | |
| isLoading = true; | |
| errorState = null; | |
| }); | |
| // Check if we need to connect | |
| final needsConnection = sshService.didServerChange || !sshService.isConnected; | |
| if (needsConnection) { | |
| // Reset server change flag | |
| sshService.didServerChange = false; | |
| // Try to connect | |
| final connectionError = await _connectToServer(); | |
| if (connectionError != null) { | |
| // Connection failed | |
| if (mounted) { | |
| setState(() { | |
| isLoading = false; | |
| errorState = connectionError; | |
| }); | |
| } | |
| return; | |
| } | |
| } | |
| // Connection good (or already connected), load data | |
| await _loadItemsAndUpdateState(); | |
| } | |
| // Public method for child classes to trigger refresh | |
| void refresh() { | |
| _initializeScreen(); | |
| } | |
| // Common refresh logic for pull-to-refresh | |
| Future<void> refreshItems() async { | |
| await _initializeScreen(); | |
| } | |
| bool _isInitializing = false; | |
| Future<void> _initializeScreen() async { | |
| if (_isInitializing) return; | |
| _isInitializing = true; | |
| try { | |
| setState(() { | |
| isLoading = true; | |
| errorState = null; | |
| }); | |
| // Check if we need to connect | |
| final needsConnection = sshService.didServerChange || !sshService.isConnected; | |
| if (needsConnection) { | |
| // Reset server change flag | |
| sshService.didServerChange = false; | |
| // Try to connect | |
| final connectionError = await _connectToServer(); | |
| if (connectionError != null) { | |
| // Connection failed | |
| if (mounted) { | |
| setState(() { | |
| isLoading = false; | |
| errorState = connectionError; | |
| }); | |
| } | |
| return; | |
| } | |
| } | |
| // Connection good (or already connected), load data | |
| await _loadItemsAndUpdateState(); | |
| } finally { | |
| _isInitializing = false; | |
| } | |
| } | |
| // Public method for child classes to trigger refresh | |
| void refresh() { | |
| _initializeScreen(); | |
| } | |
| // Common refresh logic for pull-to-refresh | |
| Future<void> refreshItems() async { | |
| await _initializeScreen(); | |
| } |
🤖 Prompt for AI Agents
In lib/presentation/screens/base/base_resource_screen.dart around lines 51 to
91, _initializeScreen lacks a concurrency guard and can be re-entered from
initState, refresh, refreshItems, or build, causing race conditions; add a
private boolean flag (e.g., _isInitializing) checked at the top to immediately
return if true, set it to true before any async work starts, and clear it in a
finally block so it always resets even on errors; ensure setState calls still
check mounted and that the flag prevents overlapping connection attempts and
data loads.
| if (loadError?.message == '__RECONNECT__') { | ||
| // Connection error detected, retry initialization | ||
| _initializeScreen(); |
There was a problem hiding this comment.
Potential infinite reconnection loop.
When a connection error is detected, _loadItems returns a special '__RECONNECT__' message (line 154), which triggers a recursive call to _initializeScreen (line 102). Without a retry limit or backoff strategy, this can lead to an infinite loop if the connection continues to fail.
Add retry limits and exponential backoff:
+ int _reconnectAttempts = 0;
+ static const int _maxReconnectAttempts = 3;
+
Future<void> _loadItemsAndUpdateState() async {
final loadError = await _loadItems();
if (!mounted) return;
// Check if it's a reconnect signal
if (loadError?.message == '__RECONNECT__') {
+ if (_reconnectAttempts >= _maxReconnectAttempts) {
+ setState(() {
+ isLoading = false;
+ errorState = ErrorState.connection(
+ message: 'Connection failed after $_maxReconnectAttempts attempts',
+ onRetry: () {
+ _reconnectAttempts = 0;
+ _initializeScreen();
+ },
+ );
+ });
+ return;
+ }
+ _reconnectAttempts++;
// Connection error detected, retry initialization
_initializeScreen();
} else {
+ _reconnectAttempts = 0;
setState(() {
isLoading = false;
errorState = loadError;
});
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (loadError?.message == '__RECONNECT__') { | |
| // Connection error detected, retry initialization | |
| _initializeScreen(); | |
| int _reconnectAttempts = 0; | |
| static const int _maxReconnectAttempts = 3; | |
| Future<void> _loadItemsAndUpdateState() async { | |
| final loadError = await _loadItems(); | |
| if (!mounted) return; | |
| // Check if it's a reconnect signal | |
| if (loadError?.message == '__RECONNECT__') { | |
| if (_reconnectAttempts >= _maxReconnectAttempts) { | |
| setState(() { | |
| isLoading = false; | |
| errorState = ErrorState.connection( | |
| message: 'Connection failed after $_maxReconnectAttempts attempts', | |
| onRetry: () { | |
| _reconnectAttempts = 0; | |
| _initializeScreen(); | |
| }, | |
| ); | |
| }); | |
| return; | |
| } | |
| _reconnectAttempts++; | |
| // Connection error detected, retry initialization | |
| _initializeScreen(); | |
| } else { | |
| _reconnectAttempts = 0; | |
| setState(() { | |
| isLoading = false; | |
| errorState = loadError; | |
| }); | |
| } | |
| } |
🤖 Prompt for AI Agents
In lib/presentation/screens/base/base_resource_screen.dart around lines 100 to
102, the code calls _initializeScreen() unconditionally when loadError?.message
== '__RECONNECT__', which can create an infinite recursive reconnect loop;
modify this flow to track retry attempts (e.g., an int _reconnectAttempts field)
and a maxRetries constant, and replace the direct recursive call with a
scheduled retry using exponential backoff (e.g., delay = baseDelay *
2^_reconnectAttempts) via Future.delayed before calling _initializeScreen();
increment _reconnectAttempts on each retry, reset it on a successful init, and
when _reconnectAttempts >= maxRetries stop retrying and surface a persistent
error state or notification so the UI stops looping.
| // Debug toast with random number | ||
| if (kDebugMode) { | ||
| final randomNum = Random().nextInt(10000); | ||
| final serverName = sshService.currentServer?.name ?? 'NULL'; | ||
| final isConnected = sshService.isConnected; | ||
|
|
||
| WidgetsBinding.instance.addPostFrameCallback((_) { | ||
| if (mounted) { | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| SnackBar( | ||
| content: Text('[DEBUG] ${getResourceName()} - Random: $randomNum | Server: $serverName | Connected: $isConnected | Loading: $isLoading'), | ||
| duration: const Duration(seconds: 3), | ||
| behavior: SnackBarBehavior.floating, | ||
| ), | ||
| ); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove debug code or make it optional.
Lines 220-237 show a debug SnackBar on every build with a random number. This creates UI noise during development and should either be removed or made optional via a flag.
Remove the debug toast or add a flag to control it:
super.build(context);
- // Debug toast with random number
- if (kDebugMode) {
- final randomNum = Random().nextInt(10000);
- final serverName = sshService.currentServer?.name ?? 'NULL';
- final isConnected = sshService.isConnected;
-
- WidgetsBinding.instance.addPostFrameCallback((_) {
- if (mounted) {
- ScaffoldMessenger.of(context).showSnackBar(
- SnackBar(
- content: Text('[DEBUG] ${getResourceName()} - Random: $randomNum | Server: $serverName | Connected: $isConnected | Loading: $isLoading'),
- duration: const Duration(seconds: 3),
- behavior: SnackBarBehavior.floating,
- ),
- );
- }
- });
- }
-
// CHECK: Server change detection📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Debug toast with random number | |
| if (kDebugMode) { | |
| final randomNum = Random().nextInt(10000); | |
| final serverName = sshService.currentServer?.name ?? 'NULL'; | |
| final isConnected = sshService.isConnected; | |
| WidgetsBinding.instance.addPostFrameCallback((_) { | |
| if (mounted) { | |
| ScaffoldMessenger.of(context).showSnackBar( | |
| SnackBar( | |
| content: Text('[DEBUG] ${getResourceName()} - Random: $randomNum | Server: $serverName | Connected: $isConnected | Loading: $isLoading'), | |
| duration: const Duration(seconds: 3), | |
| behavior: SnackBarBehavior.floating, | |
| ), | |
| ); | |
| } | |
| }); | |
| } | |
| super.build(context); | |
| // CHECK: Server change detection |
🤖 Prompt for AI Agents
lib/presentation/screens/base/base_resource_screen.dart lines 220-237: the
current code shows a debug SnackBar on every build with a random number causing
UI noise; either remove the debug block entirely or make it opt-in by
introducing a boolean flag (e.g., showDebugSnackBar) read from a debug/config
value, only show the SnackBar when that flag is true, and move the SnackBar
logic out of the build path (e.g., trigger it in initState or behind an explicit
user action) to avoid firing on every rebuild; also remove the per-build
Random() usage or generate the debug value once when the debug notification is
created.
| void _showShellExecutableDialog(DockerContainer container) { | ||
| final TextEditingController executableController = TextEditingController(text: '/bin/bash'); | ||
|
|
||
| final commonExecutables = [ | ||
| '/bin/bash', | ||
| '/bin/sh', | ||
| '/bin/ash', | ||
| '/bin/zsh', | ||
| '/bin/fish', | ||
| 'redis-cli', | ||
| 'mysql', | ||
| 'psql', | ||
| 'mongo', | ||
| 'python', | ||
| 'node', | ||
| ]; | ||
|
|
||
| showDialog( | ||
| context: context, | ||
| builder: (BuildContext context) { | ||
| return AlertDialog( | ||
| title: Text('Choose Shell - ${container.names}'), | ||
| content: Column( | ||
| mainAxisSize: MainAxisSize.min, | ||
| children: [ | ||
| TextField( | ||
| controller: executableController, | ||
| autofocus: true, | ||
| decoration: const InputDecoration( | ||
| labelText: 'Executable', | ||
| hintText: 'Enter executable path or command', | ||
| border: OutlineInputBorder(), | ||
| ), | ||
| onSubmitted: (value) { | ||
| if (value.trim().isNotEmpty) { | ||
| Navigator.of(context).pop(); | ||
| _openInteractiveShell(container, value.trim()); | ||
| } | ||
| }, | ||
| ), | ||
| const SizedBox(height: 16), | ||
| const Text( | ||
| 'Common executables:', | ||
| style: TextStyle(fontWeight: FontWeight.w500), | ||
| ), | ||
| const SizedBox(height: 8), | ||
| Container( | ||
| padding: const EdgeInsets.all(8), | ||
| decoration: BoxDecoration( | ||
| color: Colors.blue.withOpacity(0.05), | ||
| borderRadius: BorderRadius.circular(8), | ||
| border: Border.all(color: Colors.blue.withOpacity(0.2)), | ||
| SizedBox( | ||
| height: 150, | ||
| child: SingleChildScrollView( | ||
| child: Wrap( | ||
| spacing: 8, | ||
| runSpacing: 4, | ||
| children: commonExecutables.map((executable) { | ||
| return ActionChip( | ||
| label: Text(executable), | ||
| onPressed: () { | ||
| executableController.text = executable; | ||
| }, | ||
| ); | ||
| }).toList(), | ||
| ), | ||
| ), | ||
| child: container.hasStats | ||
| ? Row( | ||
| children: [ | ||
| Expanded( | ||
| child: _buildStatColumn( | ||
| icon: Icons.speed, | ||
| label: 'CPU', | ||
| value: container.cpuPerc ?? 'N/A', | ||
| ), | ||
| ), | ||
| Expanded( | ||
| child: _buildStatColumn( | ||
| icon: Icons.memory, | ||
| label: 'Memory', | ||
| value: container.memPerc ?? 'N/A', | ||
| ), | ||
| ), | ||
| Expanded( | ||
| child: _buildStatColumn( | ||
| icon: Icons.cloud_queue, | ||
| label: 'Network', | ||
| value: container.netIO ?? 'N/A', | ||
| ), | ||
| ), | ||
| Expanded( | ||
| child: _buildStatColumn( | ||
| icon: Icons.format_list_numbered, | ||
| label: 'PIDs', | ||
| value: container.pids ?? 'N/A', | ||
| ), | ||
| ), | ||
| ], | ||
| ) | ||
| : Row( | ||
| mainAxisAlignment: MainAxisAlignment.center, | ||
| children: [ | ||
| SizedBox( | ||
| width: 16, | ||
| height: 16, | ||
| child: CircularProgressIndicator( | ||
| strokeWidth: 2, | ||
| valueColor: AlwaysStoppedAnimation<Color>( | ||
| Colors.blue[700]!, | ||
| ), | ||
| ), | ||
| ), | ||
| const SizedBox(width: 8), | ||
| Text( | ||
| 'Loading stats...', | ||
| style: TextStyle( | ||
| fontSize: 12, | ||
| color: Colors.grey[600], | ||
| ), | ||
| ), | ||
| ], | ||
| ), | ||
| ), | ||
| ], | ||
| ], | ||
| ), | ||
| ), | ||
| ); | ||
| } | ||
|
|
||
| Widget _buildDetailRow(String label, String value) { | ||
| return Padding( | ||
| padding: const EdgeInsets.symmetric(vertical: 2), | ||
| child: Row( | ||
| crossAxisAlignment: CrossAxisAlignment.start, | ||
| children: [ | ||
| SizedBox( | ||
| width: 80, | ||
| child: Text( | ||
| '$label:', | ||
| style: Theme.of(context).textTheme.bodySmall?.copyWith( | ||
| fontWeight: FontWeight.w500, | ||
| color: Colors.grey[600], | ||
| ), | ||
| ), | ||
| ), | ||
| Expanded( | ||
| child: Text( | ||
| value, | ||
| style: Theme.of(context).textTheme.bodySmall, | ||
| actions: [ | ||
| TextButton( | ||
| onPressed: () => Navigator.of(context).pop(), | ||
| child: const Text('Cancel'), | ||
| ), | ||
| ), | ||
| ], | ||
| ), | ||
| ElevatedButton( | ||
| onPressed: () { | ||
| final executable = executableController.text.trim(); | ||
| if (executable.isNotEmpty) { | ||
| Navigator.of(context).pop(); | ||
| _openInteractiveShell(container, executable); | ||
| } | ||
| }, | ||
| child: const Text('Connect'), | ||
| ), | ||
| ], | ||
| ); | ||
| }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Memory leak: TextEditingController not disposed.
Line 667 creates a TextEditingController that is never disposed. This causes a memory leak each time the dialog is shown.
Dispose the controller when the dialog is dismissed:
void _showShellExecutableDialog(DockerContainer container) {
final TextEditingController executableController = TextEditingController(text: '/bin/bash');
final commonExecutables = [
'/bin/bash',
'/bin/sh',
'/bin/ash',
'/bin/zsh',
'/bin/fish',
'redis-cli',
'mysql',
'psql',
'mongo',
'python',
'node',
];
showDialog(
context: context,
builder: (BuildContext context) {
return AlertDialog(
title: Text('Choose Shell - ${container.names}'),
content: Column(
mainAxisSize: MainAxisSize.min,
children: [
TextField(
controller: executableController,
autofocus: true,
decoration: const InputDecoration(
labelText: 'Executable',
hintText: 'Enter executable path or command',
border: OutlineInputBorder(),
),
onSubmitted: (value) {
if (value.trim().isNotEmpty) {
Navigator.of(context).pop();
_openInteractiveShell(container, value.trim());
}
},
),
// ... rest of dialog
],
),
actions: [
TextButton(
onPressed: () => Navigator.of(context).pop(),
child: const Text('Cancel'),
),
ElevatedButton(
onPressed: () {
final executable = executableController.text.trim();
if (executable.isNotEmpty) {
Navigator.of(context).pop();
_openInteractiveShell(container, executable);
}
},
child: const Text('Connect'),
),
],
);
},
- );
+ ).then((_) => executableController.dispose());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void _showShellExecutableDialog(DockerContainer container) { | |
| final TextEditingController executableController = TextEditingController(text: '/bin/bash'); | |
| final commonExecutables = [ | |
| '/bin/bash', | |
| '/bin/sh', | |
| '/bin/ash', | |
| '/bin/zsh', | |
| '/bin/fish', | |
| 'redis-cli', | |
| 'mysql', | |
| 'psql', | |
| 'mongo', | |
| 'python', | |
| 'node', | |
| ]; | |
| showDialog( | |
| context: context, | |
| builder: (BuildContext context) { | |
| return AlertDialog( | |
| title: Text('Choose Shell - ${container.names}'), | |
| content: Column( | |
| mainAxisSize: MainAxisSize.min, | |
| children: [ | |
| TextField( | |
| controller: executableController, | |
| autofocus: true, | |
| decoration: const InputDecoration( | |
| labelText: 'Executable', | |
| hintText: 'Enter executable path or command', | |
| border: OutlineInputBorder(), | |
| ), | |
| onSubmitted: (value) { | |
| if (value.trim().isNotEmpty) { | |
| Navigator.of(context).pop(); | |
| _openInteractiveShell(container, value.trim()); | |
| } | |
| }, | |
| ), | |
| const SizedBox(height: 16), | |
| const Text( | |
| 'Common executables:', | |
| style: TextStyle(fontWeight: FontWeight.w500), | |
| ), | |
| const SizedBox(height: 8), | |
| Container( | |
| padding: const EdgeInsets.all(8), | |
| decoration: BoxDecoration( | |
| color: Colors.blue.withOpacity(0.05), | |
| borderRadius: BorderRadius.circular(8), | |
| border: Border.all(color: Colors.blue.withOpacity(0.2)), | |
| SizedBox( | |
| height: 150, | |
| child: SingleChildScrollView( | |
| child: Wrap( | |
| spacing: 8, | |
| runSpacing: 4, | |
| children: commonExecutables.map((executable) { | |
| return ActionChip( | |
| label: Text(executable), | |
| onPressed: () { | |
| executableController.text = executable; | |
| }, | |
| ); | |
| }).toList(), | |
| ), | |
| ), | |
| child: container.hasStats | |
| ? Row( | |
| children: [ | |
| Expanded( | |
| child: _buildStatColumn( | |
| icon: Icons.speed, | |
| label: 'CPU', | |
| value: container.cpuPerc ?? 'N/A', | |
| ), | |
| ), | |
| Expanded( | |
| child: _buildStatColumn( | |
| icon: Icons.memory, | |
| label: 'Memory', | |
| value: container.memPerc ?? 'N/A', | |
| ), | |
| ), | |
| Expanded( | |
| child: _buildStatColumn( | |
| icon: Icons.cloud_queue, | |
| label: 'Network', | |
| value: container.netIO ?? 'N/A', | |
| ), | |
| ), | |
| Expanded( | |
| child: _buildStatColumn( | |
| icon: Icons.format_list_numbered, | |
| label: 'PIDs', | |
| value: container.pids ?? 'N/A', | |
| ), | |
| ), | |
| ], | |
| ) | |
| : Row( | |
| mainAxisAlignment: MainAxisAlignment.center, | |
| children: [ | |
| SizedBox( | |
| width: 16, | |
| height: 16, | |
| child: CircularProgressIndicator( | |
| strokeWidth: 2, | |
| valueColor: AlwaysStoppedAnimation<Color>( | |
| Colors.blue[700]!, | |
| ), | |
| ), | |
| ), | |
| const SizedBox(width: 8), | |
| Text( | |
| 'Loading stats...', | |
| style: TextStyle( | |
| fontSize: 12, | |
| color: Colors.grey[600], | |
| ), | |
| ), | |
| ], | |
| ), | |
| ), | |
| ], | |
| ], | |
| ), | |
| ), | |
| ); | |
| } | |
| Widget _buildDetailRow(String label, String value) { | |
| return Padding( | |
| padding: const EdgeInsets.symmetric(vertical: 2), | |
| child: Row( | |
| crossAxisAlignment: CrossAxisAlignment.start, | |
| children: [ | |
| SizedBox( | |
| width: 80, | |
| child: Text( | |
| '$label:', | |
| style: Theme.of(context).textTheme.bodySmall?.copyWith( | |
| fontWeight: FontWeight.w500, | |
| color: Colors.grey[600], | |
| ), | |
| ), | |
| ), | |
| Expanded( | |
| child: Text( | |
| value, | |
| style: Theme.of(context).textTheme.bodySmall, | |
| actions: [ | |
| TextButton( | |
| onPressed: () => Navigator.of(context).pop(), | |
| child: const Text('Cancel'), | |
| ), | |
| ), | |
| ], | |
| ), | |
| ElevatedButton( | |
| onPressed: () { | |
| final executable = executableController.text.trim(); | |
| if (executable.isNotEmpty) { | |
| Navigator.of(context).pop(); | |
| _openInteractiveShell(container, executable); | |
| } | |
| }, | |
| child: const Text('Connect'), | |
| ), | |
| ], | |
| ); | |
| }, | |
| ); | |
| } | |
| void _showShellExecutableDialog(DockerContainer container) { | |
| final TextEditingController executableController = TextEditingController(text: '/bin/bash'); | |
| final commonExecutables = [ | |
| '/bin/bash', | |
| '/bin/sh', | |
| '/bin/ash', | |
| '/bin/zsh', | |
| '/bin/fish', | |
| 'redis-cli', | |
| 'mysql', | |
| 'psql', | |
| 'mongo', | |
| 'python', | |
| 'node', | |
| ]; | |
| showDialog( | |
| context: context, | |
| builder: (BuildContext context) { | |
| return AlertDialog( | |
| title: Text('Choose Shell - ${container.names}'), | |
| content: Column( | |
| mainAxisSize: MainAxisSize.min, | |
| children: [ | |
| TextField( | |
| controller: executableController, | |
| autofocus: true, | |
| decoration: const InputDecoration( | |
| labelText: 'Executable', | |
| hintText: 'Enter executable path or command', | |
| border: OutlineInputBorder(), | |
| ), | |
| onSubmitted: (value) { | |
| if (value.trim().isNotEmpty) { | |
| Navigator.of(context).pop(); | |
| _openInteractiveShell(container, value.trim()); | |
| } | |
| }, | |
| ), | |
| const SizedBox(height: 16), | |
| const Text( | |
| 'Common executables:', | |
| style: TextStyle(fontWeight: FontWeight.w500), | |
| ), | |
| const SizedBox(height: 8), | |
| SizedBox( | |
| height: 150, | |
| child: SingleChildScrollView( | |
| child: Wrap( | |
| spacing: 8, | |
| runSpacing: 4, | |
| children: commonExecutables.map((executable) { | |
| return ActionChip( | |
| label: Text(executable), | |
| onPressed: () { | |
| executableController.text = executable; | |
| }, | |
| ); | |
| }).toList(), | |
| ), | |
| ), | |
| ), | |
| ], | |
| ), | |
| actions: [ | |
| TextButton( | |
| onPressed: () => Navigator.of(context).pop(), | |
| child: const Text('Cancel'), | |
| ), | |
| ElevatedButton( | |
| onPressed: () { | |
| final executable = executableController.text.trim(); | |
| if (executable.isNotEmpty) { | |
| Navigator.of(context).pop(); | |
| _openInteractiveShell(container, executable); | |
| } | |
| }, | |
| child: const Text('Connect'), | |
| ), | |
| ], | |
| ); | |
| }, | |
| ).then((_) => executableController.dispose()); | |
| } |
🤖 Prompt for AI Agents
In lib/presentation/screens/containers_screen.dart around lines 666 to 750 a
TextEditingController is created for the dialog and never disposed, causing a
memory leak; fix by ensuring the controller is disposed after the dialog is
dismissed (for example, call showDialog(...) and then dispose the controller in
the returned Future's then/catchError or use async/await with try/finally), i.e.
create the controller as now, pass it to the dialog, and after showDialog
completes always call executableController.dispose().
| child: Row( | ||
| mainAxisSize: MainAxisSize.min, | ||
| children: [ | ||
| children: const [ |
There was a problem hiding this comment.
Incorrect const on children list.
Line 194 marks the children list as const, but the list contains SizedBox widgets that are not compile-time constants. This will cause a compilation error or inconsistent behavior.
Remove the const keyword:
child: Row(
mainAxisSize: MainAxisSize.min,
- children: const [
+ children: [
Icon(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| children: const [ | |
| children: [ |
🤖 Prompt for AI Agents
In lib/presentation/screens/images_screen.dart around line 194, the children
list is incorrectly declared as const even though it contains non-constant
widgets (SizedBox), which causes compile errors; remove the const keyword before
the children list so the list and its widget entries are evaluated at runtime,
then run a build to verify the error is resolved.
theSoberSobber
force-pushed
the
clean-arch
branch
from
75b43d5 to
516681f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/presentation/screens/base/base_resource_screen.dart (1)
270-287: Debug SnackBar on every build is noisy even in debugThe debug block shows a SnackBar with a random number on every
buildwhilekDebugModeis true. This can easily spam SnackBars on rebuilds and makes it harder to see more meaningful messages.Consider either removing this entirely, or guarding it behind a dedicated debug flag / developer setting and triggering it from a one‑off action (e.g., a button) rather than every build.
If you want to keep some form of debug feedback, you might switch to simple
debugPrintcalls or a singleSnackBartriggered frominitStateinstead of every rebuild.
🧹 Nitpick comments (2)
lib/presentation/screens/base/base_resource_screen.dart (2)
202-240: Clean up or re‑introduce legacy helpers to avoid confusion
loadItems,refreshItems,didChangeAppLifecycleState,buildBody, andbuildErrorStateare currently effectively dead code (logic commented out or replaced withContainer()), while the new flow is entirely driven by_loadItems,_connectToServer, and thebuildmethod.This is harmless at runtime but increases cognitive load for anyone extending the base class.
Options to simplify:
- If the new CHECK‑based flow is the only supported path, remove these unused methods (or at least mark them
@deprecated) to avoid accidental use.- If you plan to re‑enable them (e.g., for pull‑to‑refresh or lifecycle reload), consider wiring them to the new
_loadItems/errorStatemodel instead of leaving commented blocks in place.It’d be good to grep for
refreshItems()andbuildBody()usages in the project to ensure nothing still depends on the old behaviour before removing or refactoring them.Also applies to: 253-263, 479-546
289-382: Avoid starting async work and mutating state directly inbuildThe
buildmethod currently:
- Computes
needsConnection/needsLoad.- Mutates
isLoadinganderrorStatedirectly.- Starts async operations (
_connectToServer,_loadItems) from withinbuild.Functionally this works due to the
!isLoadingguard and the completion handlers callingsetState, but triggering side‑effects frombuildis a Flutter anti‑pattern and can make behaviour harder to reason about as the widget tree evolves.A more robust structure would be to:
- Move the “connect & load if needed” orchestration into a private method (e.g.,
_ensureConnectedAndLoaded()) invoked frominitStateand from explicit events (server change, manual refresh).- Let
buildreadisLoading,errorState, anditemsonly, without starting new work.This keeps rendering and side‑effects separated and reduces the chances of subtle re‑entrancy issues as more logic is added.
If you refactor this, please confirm that server‑change detection (via
sshService.didServerChange) still reliably triggers a reload path without relying onbuildside‑effects.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/presentation/screens/base/base_resource_screen.dart(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: Build Windows Release
🔇 Additional comments (1)
lib/presentation/screens/base/base_resource_screen.dart (1)
18-36: Base abstraction & DI wiring look coherentThe generic
BaseResourceScreenState<T, W>plus GetIt‑backed service fields provide a clear, reusable foundation for resource screens; state fields cover loading, error, and filtering concerns cleanly. No functional issues spotted here.Please just confirm that
service_locator.dartalways initializes GetIt before anyBaseResourceScreenis created (e.g., inmain()), so theselatefields are never accessed before registration.
| Future<ErrorState?> _loadItems() async { | ||
| if (kDebugMode) { | ||
| print('[CHECK 2] Loading ${getResourceName()}...'); | ||
| } | ||
|
|
||
| try { | ||
| final fetchedItems = await fetchItems(); | ||
|
|
||
| if (kDebugMode) { | ||
| print('[CHECK 2] Loaded ${fetchedItems.length} ${getResourceName()}'); | ||
| } | ||
|
|
||
| if (mounted) { | ||
| items = fetchedItems; | ||
| filteredItems = filterItems(fetchedItems, searchQuery); | ||
| hasTriedLoading = true; | ||
| } | ||
|
|
||
| return null; // Success | ||
| } catch (e) { | ||
| if (kDebugMode) { | ||
| print('[CHECK 2] Error loading ${getResourceName()}: $e'); | ||
| } | ||
|
|
||
| final errorMessage = e.toString(); | ||
|
|
||
| // Check if it's a connection error | ||
| if (_isConnectionError(errorMessage)) { | ||
| if (kDebugMode) { | ||
| print('[CHECK 2.1] Connection error detected - forcing reconnection'); | ||
| } | ||
|
|
||
| // Force reconnection by resetting flag AND keeping isLoading true | ||
| // Caller will call setState which triggers CHECK 1 on next build | ||
| if (mounted) { | ||
| hasTriedConnecting = false; | ||
| // Return special marker so caller knows to keep loading | ||
| return ErrorState.connection( | ||
| message: '__RECONNECT__', // Special marker | ||
| onRetry: null, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| // Check if it's a permission error | ||
| if (_isPermissionError(errorMessage)) { | ||
| if (kDebugMode) { | ||
| print('[CHECK 2.2] Permission error detected'); | ||
| } | ||
|
|
||
| if (mounted) { | ||
| hasTriedLoading = true; // Mark as tried to prevent infinite loop | ||
| } | ||
|
|
||
| return ErrorState.permission( | ||
| message: errorMessage, | ||
| onRetry: () { | ||
| // For permission errors, just retry loading (don't reconnect) | ||
| setState(() { | ||
| errorState = null; | ||
| isLoading = true; | ||
| }); | ||
|
|
||
| // Reset flag and load again | ||
| hasTriedLoading = false; | ||
| _loadItems().then((error) { | ||
| if (mounted) { | ||
| // Check if it's a reconnect signal | ||
| if (error?.message == '__RECONNECT__') { | ||
| setState(() { | ||
| isLoading = false; | ||
| }); | ||
| } else { | ||
| setState(() { | ||
| isLoading = false; | ||
| errorState = error; | ||
| }); | ||
| } | ||
| } | ||
| }); | ||
| }, | ||
| ); | ||
| } | ||
|
|
||
| // Other error - return error state | ||
| if (mounted) { | ||
| hasTriedLoading = true; // Mark as tried to prevent infinite loop | ||
| } | ||
|
|
||
| return ErrorState.general( | ||
| message: errorMessage, | ||
| onRetry: () { | ||
| // For general errors, retry loading directly | ||
| setState(() { | ||
| errorState = null; | ||
| isLoading = true; | ||
| }); | ||
|
|
||
| hasTriedLoading = false; | ||
| _loadItems().then((error) { | ||
| if (mounted) { | ||
| // Check if it's a reconnect signal | ||
| if (error?.message == '__RECONNECT__') { | ||
| setState(() { | ||
| isLoading = false; | ||
| }); | ||
| } else { | ||
| setState(() { | ||
| isLoading = false; | ||
| errorState = error; | ||
| }); | ||
| } | ||
| } | ||
| }); | ||
| }, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| // Helper to detect connection errors | ||
| bool _isConnectionError(String error) { | ||
| final lowerError = error.toLowerCase(); | ||
| return lowerError.contains('connection') || | ||
| lowerError.contains('ssh') || | ||
| lowerError.contains('timeout') || | ||
| lowerError.contains('refused') || | ||
| lowerError.contains('closed'); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Unbounded reconnect/reload loop risk with '__RECONNECT__' marker
When _loadItems classifies an error as a connection error, it returns an ErrorState.connection whose message is '__RECONNECT__'. All callers treat this as a special “retry” signal and simply clear isLoading, letting the build logic re‑enter the connect/load path on the next frame. If fetchItems() continues to throw connection‑style errors (e.g., remote Docker down, flaky SSH tunnel), this creates an effectively infinite reconnect/reload loop with no max attempt cap or backoff.
This can:
- Hammer the remote server / network.
- Keep the UI stuck in a loading cycle with no stable error screen.
- Make it hard for users to intervene (no surfaced error once the loop starts).
I’d strongly recommend introducing a bounded retry strategy here, for example:
- Track a
_reconnectAttemptscounter and a_maxReconnectAttemptsconstant on the state. - Increment the counter in the
_isConnectionErrorbranch; only return the'__RECONNECT__'marker while_reconnectAttempts < _maxReconnectAttempts. - Once the limit is hit, set
hasTriedLoading = trueand return a normalErrorState.connectionwith a user‑facing message and anonRetrythat resets the counter and triggers a single explicit reconnect + load cycle (without further auto‑looping).
You might also consider replacing the string sentinel with a dedicated flag/type on ErrorState to avoid brittle message == '__RECONNECT__' checks.
Please verify behaviour manually by forcing fetchItems() to always throw a connection‑like error (e.g., by mocking DockerRepository/DockerOperationsService) and observe that the screen doesn’t loop indefinitely after adding a retry cap. I can help sketch a concrete diff if you want to move to a counter‑based approach.
I'll verify the reconnect loop issue by examining the codebase. Let me start by exploring the relevant files.
Now let me examine the actual codebase to verify the reconnect loop concern:
I'll now examine the codebase directly to verify the reconnect loop issue:
Now searching the codebase:
[/run_scripts]
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for base_resource_screen.dart and examine the full file
fd -t f 'base_resource_screen.dart' -exec head -300 {} \;
Length of output: 230
🏁 Script executed:
#!/bin/bash
# Find the file and examine it
find . -name 'base_resource_screen.dart' 2>/dev/null | head -1Length of output: 57
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Read the full file to understand the complete structure
cat ./lib/presentation/screens/base/base_resource_screen.dartLength of output: 18833
🏁 Script executed:
#!/bin/bash
# Also search for ErrorState definition
rg -n 'class ErrorState' --type=dartLength of output: 157
🏁 Script executed:
#!/bin/bash
# Search for __RECONNECT__ usage patterns
rg -n '__RECONNECT__' --type=dart -A 2 -B 2Length of output: 2581
🏁 Script executed:
#!/bin/bash
# Search for any existing attempt counters
rg -n '_reconnectAttempts|_loadAttempts|_maxRetry|maxAttempts' --type=dartLength of output: 0
🏁 Script executed:
#!/bin/bash
# Search for the build method in base_resource_screen
rg -n 'Widget build' ./lib/presentation/screens/base/base_resource_screen.dart -A 50Length of output: 7715
The infinite reconnect loop issue is confirmed and accurately described
The code exhibits the exact unbounded retry problem identified in the review. The control flow confirms:
-
Loop mechanism verified: When
_loadItems()detects a connection error, it resetshasTriedConnecting = false(line 98) and returns the__RECONNECT__marker. The next build cycle's CHECK 1 logic (line 290-291) evaluatesneedsConnection = !sshService.isConnected && !hasTriedConnectingas true, retriggering the load cycle indefinitely iffetchItems()continues throwing connection errors. -
No attempt counters exist: Search confirms no
_reconnectAttempts,_loadAttempts,_maxRetry, ormaxAttemptscounters in the codebase. -
No user intervention possible: When the
__RECONNECT__marker is returned,errorStateis cleared (line 302) andisLoadingset to false briefly. The next build has no error UI to display (noerrorStateobject), so users see only a loading spinner and cannot tap a retry button. -
String sentinel is problematic: The
'__RECONNECT__'marker is checked via string equality at 5 locations (lines 131, 165, 327, 352, and implicit in retry callbacks). This pattern is fragile and couples internal logic to a magic string.
The recommendation to add a _reconnectAttempts counter with a _maxReconnectAttempts cap is appropriate and necessary to prevent indefinite server hammering and UI hangs.
🤖 Prompt for AI Agents
In lib/presentation/screens/base/base_resource_screen.dart around lines 63-190,
the connection-reconnect flow uses a magic string '__RECONNECT__' and resets
hasTriedConnecting to false without any attempt cap, causing an infinite
reconnect loop; add a private int _reconnectAttempts and a const int
_maxReconnectAttempts (e.g. 3-5), replace the string sentinel with a typed
signal (add a dedicated ErrorState.reconnect factory or boolean flag on
ErrorState) and increment _reconnectAttempts each time you return the reconnect
ErrorState; when _reconnectAttempts >= _maxReconnectAttempts, stop forcing
reconnection (set hasTriedConnecting = true and return a normal connection
ErrorState with a retry callback that allows the user to manually retry), and
reset _reconnectAttempts to 0 on a successful load or when the user manually
retries so the cycle is bounded and no longer depends on a fragile string check.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
lib/presentation/screens/base/base_resource_screen.dart (2)
229-246: Per-build debug SnackBar is noisy and should be removed or gated
buildcurrently shows a debugSnackBarwith a random number on every rebuild inkDebugMode. This is very noisy once multiple resource screens are in use and tightly couples UI rebuilds to side effects.I’d either:
- Remove this block entirely, or
- Gate it behind a dedicated debug/config flag and trigger it only on explicit user action or a one-off lifecycle hook (not on every build).
If you opt to remove it now:
-import 'dart:math'; @@ - // Debug toast with random number - if (kDebugMode) { - final randomNum = Random().nextInt(10000); - final serverName = sshService.currentServer?.name ?? 'NULL'; - final isConnected = sshService.isConnected; - - WidgetsBinding.instance.addPostFrameCallback((_) { - if (mounted) { - ScaffoldMessenger.of(context).showSnackBar( - SnackBar( - content: Text('[DEBUG] ${getResourceName()} - Random: $randomNum | Server: $serverName | Connected: $isConnected | Loading: $isLoading'), - duration: const Duration(seconds: 3), - behavior: SnackBarBehavior.floating, - ), - ); - } - }); - } -
62-104: Unbounded reconnect loop and fragile'__RECONNECT__'sentinel are still presentThe current reconnect flow still relies on the magic string
'__RECONNECT__'and continuously resetshasTriedConnectingwithout any attempt cap or backoff. IffetchItems()keeps producing connection-style errors, the screen will keep auto-retrying via CHECK 1 with no upper bound, potentially hammering the server and keeping the UI in a loop with no stable error state.I recommend:
- Tracking reconnect attempts with a counter and a max limit.
- Returning a normal
ErrorState.connection(with user-facing message +onRetry) once the limit is reached, so the UI stops auto-looping.- Resetting the counter on a successful load or when the user explicitly retries.
- (Optional) Replacing the magic
'__RECONNECT__'string with a typed signal onErrorStatein a follow-up.Example of how you could bound the reconnects inside this class:
@@ bool hasTriedConnecting = false; // Track if we've attempted connection to prevent infinite retry loops bool hasTriedLoading = false; + int _reconnectAttempts = 0; + static const int _maxReconnectAttempts = 3; @@ - // Check if it's a connection error - if (_isConnectionError(errorMessage)) { - if (kDebugMode) { - print('[CHECK 2.1] Connection error detected - forcing reconnection'); - } - - // Force reconnection by resetting flag AND keeping isLoading true - // Caller will call setState which triggers CHECK 1 on next build - if (mounted) { - hasTriedConnecting = false; - // Return special marker so caller knows to keep loading - return ErrorState.connection( - message: '__RECONNECT__', // Special marker - onRetry: null, - ); - } - } + // Check if it's a connection error + if (_isConnectionError(errorMessage)) { + if (kDebugMode) { + print('[CHECK 2.1] Connection error detected - forcing reconnection'); + } + + if (mounted) { + if (_reconnectAttempts >= _maxReconnectAttempts) { + hasTriedLoading = true; // stop auto-looping + return ErrorState.connection( + message: + 'Failed to load ${getResourceName()} after $_maxReconnectAttempts attempts. Please check your connection and try again.', + onRetry: () { + setState(() { + errorState = null; + isLoading = true; + hasTriedConnecting = false; // allow CHECK 1 to reconnect + hasTriedLoading = false; + _reconnectAttempts = 0; + }); + }, + ); + } + + _reconnectAttempts++; + hasTriedConnecting = false; + + // Return special marker so caller knows to keep loading + return ErrorState.connection( + message: '__RECONNECT__', // Special marker + onRetry: null, + ); + } + } @@ - if (mounted) { + if (mounted) { items = fetchedItems; filteredItems = filterItems(fetchedItems, searchQuery); hasTriedLoading = true; + _reconnectAttempts = 0; // reset on success }You’d also want to reset
_reconnectAttemptsin any other “hard stop” branches where you surface a non-retryingErrorStateso future manual retries start clean.Also applies to: 126-140, 160-176, 283-299, 308-323
🧹 Nitpick comments (1)
lib/presentation/screens/base/base_resource_screen.dart (1)
201-222: Clean up or formally deprecate no-op “deprecated” helpers
loadItems,refreshItems,didChangeAppLifecycleState,buildBody, andbuildErrorStateare left as empty “deprecated” stubs. Keeping them around without annotations makes the base API surface larger and less clear for consumers.Either:
- Remove these methods if they’re no longer used anywhere, or
- Mark them with
@Deprecated('Use _loadItems / buildItemList / errorState UI instead')and plan their removal in a later breaking change.This keeps the base class focused and reduces confusion for implementers.
Also applies to: 438-447
1 participant
Summary by CodeRabbit
Chores
New Features
Bug Fixes / UX