opt: Better performance on container parsing #1001
Conversation
… content Adjust the return type of the `execWithPwd` method to `(int?, String)` so that it can simultaneously return the exit code and output content Fix the issue in ContainerNotifier where the return result of execWithPwd is not handled correctly Ensure that server operations (shutdown/restart/suspend) are correctly pending until the command execution is completed
…ists Support the simultaneous display of multiple container operation errors, enhancing error handling capabilities
…text overflow Adjust the width calculation for the container page layout, changing from subtracting a fixed value to subtracting a smaller value to improve the layout Add overflow ellipsis processing to the text to prevent anomalies when the text is too long
… error lists" This reverts commit 72aaa17.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe SSH client's Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai Review. |
|
@GT-610: I'll review the changes in this PR focused on container parsing optimization. ✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
lib/core/extension/ssh_client.dart (2)
115-151: Potential parsing regression: returnedoutputmay include stderr (e.g., sudo password prompt).
Ifexec()aggregates stderr into the returned string, consumers that parse stdout (like container output split/JSON) can break when sudo prompts/errors appear. Consider excluding stderr from the aggregated output forexecWithPwdand relying ononStderrfor stderr handling.Proposed fix (stdout-only output)
Future<(int?, String)> execWithPwd( String script, { String? entry, BuildContext? context, OnStdout? onStdout, OnStdout? onStderr, required String id, }) async { var isRequestingPwd = false; final (session, output) = await exec( (sess) { sess.stdin.add('$script\n'.uint8List); sess.stdin.close(); }, onStderr: (data, session) async { onStderr?.call(data, session); if (isRequestingPwd) return; if (data.contains('[sudo] password for ')) { isRequestingPwd = true; final user = Miscs.pwdRequestWithUserReg.firstMatch(data)?.group(1); final ctx = context ?? WidgetsBinding.instance.focusManager.primaryFocus?.context; if (ctx == null) return; final pwd = ctx.mounted ? await ctx.showPwdDialog(title: user, id: id) : null; if (pwd == null || pwd.isEmpty) { session.stdin.close(); } else { session.stdin.add('$pwd\n'.uint8List); } isRequestingPwd = false; } }, onStdout: onStdout, entry: entry, + stderr: false, ); return (session.exitCode, output); }
115-151: Update allexecWithPwdcall sites to consistently handle the record return type.The SDK supports Dart 3.9.0+ (which supports record types), but call sites are inconsistent. Three calls in
lib/view/page/server/tab/utils.dart(lines 52, 66, 80) discard the return value entirely, while two calls inlib/data/provider/container.dart(lines 91, 236) properly destructure it as(code, raw)and(code, _)respectively. Either explicitly destructure the record at all call sites or assign to variables if the return values are unused, for consistency and clarity.lib/data/provider/container.dart (1)
87-116: Handleclient == nullexplicitly to avoid misleadingsegmentsNotMatcherrors.
Lines 88-92: ifclientis null,rawstays empty and you’ll fall into the segments-length error path (Lines 106-115). Prefer early-return with a clearer error.Proposed tweak
final cmd = _wrap(ContainerCmdType.execAll(state.type, sudo: sudo, includeStats: includeStats)); int? code; String raw = ''; if (client != null) { (code, raw) = await client!.execWithPwd(cmd, context: context, id: hostId); + } else { + state = state.copyWith( + isBusy: false, + error: ContainerErr(type: ContainerErrType.unknown, message: 'SSH client not available'), + ); + return; } if (!ref.mounted) return; state = state.copyWith(isBusy: false);
🧹 Nitpick comments (4)
lib/view/page/server/tab/utils.dart (2)
45-57: Avoidawaiton a nullable client; make the null-path explicit (and optionally handle the result).
Line 52: preferfinal client = srv.client; if (client == null) return; await client.execWithPwd(...)for clearer typing/flow, and consider handling non-zero exit codes now that execWithPwd returns them.Proposed tweak
func: () async { if (Stores.setting.showSuspendTip.fetch()) { await context.showRoundDialog(title: libL10n.attention, child: Text(l10n.suspendTip)); Stores.setting.showSuspendTip.put(false); } - await srv.client?.execWithPwd( + final client = srv.client; + if (client == null) return; + await client.execWithPwd( ShellFunc.suspend.exec(srv.spi.id, systemType: srv.status.system, customDir: null), context: context, id: srv.id, ); },
63-75: Consider making_askFor.funcawaitable so errors/ordering are observable.
Lines 65-71 and 79-85: these are now async, but_askForcurrently invokesfunc()without awaiting (Line 132), so failures are effectively “fire-and-forget”. If the intent is “wait for completion” for these ops, making_askFortakeFuture<void> Function()and awaiting it in the OK handler will align behavior.Proposed tweak (signature + call site)
- void _askFor({required void Function() func, required String typ, required String name}) { + void _askFor({required Future<void> Function() func, required String typ, required String name}) { context.showRoundDialog( title: libL10n.attention, child: Text(libL10n.askContinue('$typ ${l10n.server}($name)')), actions: Btn.ok( - onTap: () { + onTap: () async { context.pop(); - func(); + await func(); }, ).toList, ); }Also applies to: 77-89
lib/view/page/container/container.dart (1)
233-258: Clamp computedwidthto avoid negative sizes on very small layouts.
Line 237:cons.maxWidth / 2 - 6.5can go negative; prefer clamping to>= 0.Proposed tweak
builder: (_, cons) { - final width = cons.maxWidth / 2 - 6.5; + final width = math.max(0.0, cons.maxWidth / 2 - 6.5); return Column(lib/data/provider/container.dart (1)
234-252: Tuple destructuring + fallback looks good; consider whether discarding output is intended.
Lines 236-245:(code, _) = ... ?? (null, null)is clean. Just double-check whether some failures only surface on stdout (you currently only return stderr-derivederrswhencode != 0).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/core/extension/ssh_client.dartlib/data/provider/container.dartlib/view/page/container/container.dartlib/view/page/server/tab/utils.dart
🧰 Additional context used
📓 Path-based instructions (2)
lib/view/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
lib/view/**/*.dart: Prefer widgets/utilities from fl_lib for common UI (e.g., CustomAppBar, context.showRoundDialog, Input, Btnx.cancelOk)
Prefer using libL10n strings before adding new ones to project l10n
Split UI into build, actions, and utils; use extension on to separate concerns
Files:
lib/view/page/container/container.dartlib/view/page/server/tab/utils.dart
lib/data/provider/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Organize Riverpod providers by feature under lib/data/provider/
Files:
lib/data/provider/container.dart
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: lollipopkit/flutter_server_box PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-08T09:36:41.682Z
Learning: Applies to lib/data/model/container/**/*.dart : Place container/Docker models under lib/data/model/container/
📚 Learning: 2025-10-08T09:36:41.682Z
Learnt from: CR
Repo: lollipopkit/flutter_server_box PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-08T09:36:41.682Z
Learning: Applies to lib/data/model/container/**/*.dart : Place container/Docker models under lib/data/model/container/
Applied to files:
lib/view/page/container/container.dartlib/data/provider/container.dart
⏰ 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: check
🔇 Additional comments (1)
lib/view/page/container/container.dart (1)
261-283:Expanded+TextOverflow.ellipsisis a solid fix for overflow here.
Lines 271-277: good change to keep stats values single-line without layout exceptions on narrow widths.
|
Noticed. I'll check these comments later. |
Add detection for Podman Docker emulation in the container module. When detected, a prompt message will be displayed and users will be advised to switch to Podman settings. Updated the multilingual translation files to support the new features.
|
Still a draft? |
Yeah. I haven't reviewed CodeRabbit issues yet. |
2 participants
Part of #1000.
This PR focuses on container parsing. It will undergo continuous submission and improvement, and will be transitioned from draft to open once it is fully completed.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.