Unify server refresh interval handling across auto-refresh views#1160
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds utilities to normalize and retrieve the server-status refresh interval: Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/view/page/container/actions.dart`:
- Around line 179-182: Replace the literal fallback 'null' shown in user dialogs
with a localized fallback using libL10n (prefer libL10n.fail or libL10n.unknown)
rather than a raw string; specifically update calls to context.showRoundDialog
where you build the child Text(e ?? 'null') so it becomes Text(e ??
libL10n.fail) (or Text(e ?? l10n.unknown if libL10n is unavailable), keeping
libL10n.error as the title). Apply the same change to the other dialog sites
referenced (the similar Text(e ?? 'null') occurrences around the later blocks)
so all user-facing error texts use localization instead of the literal "null".
- Around line 309-314: The _initAutoRefresh() method must cancel any existing
timer before any early returns to avoid leaving a running periodic timer; update
_initAutoRefresh() so that at the top (or immediately before each return) you
call _autoRefreshTimer?.cancel() and set _autoRefreshTimer = null when
Stores.setting.containerAutoRefresh.fetch() is false or when
serverStatusRefreshInterval() returns null, then only create a new
Timer.periodic(duration, ...) when you actually proceed.
In `@lib/view/page/pve.dart`:
- Around line 557-558: The current check uses pveState.release!.compareTo('8.0')
which does lexicographic comparison and fails for multi-digit majors (e.g.,
"10.0" < "8.0"); change this to a proper semantic numeric comparison by parsing
pveState.release into version parts (or use a Version/semver parser) and compare
major, minor (and patch if present) numerically; update the conditional around
pveState.release (the block where pveState.release != null and compareTo is
used) to parse safely (handle non-numeric or malformed strings) and then test
whether the parsed version is less than 8.0 using numeric comparisons or
Version.compareTo from a semver library.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 85bff823-cdfe-4c2f-8fd8-d81d46050db0
📒 Files selected for processing (7)
lib/core/utils/refresh_interval.dartlib/data/provider/server/all.dartlib/view/page/container/actions.dartlib/view/page/container/container.dartlib/view/page/process.dartlib/view/page/pve.darttest/refresh_interval_test.dart
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/view/page/container/actions.dart (1)
72-75:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse localized fallback instead of
e.toString()in error dialogs.At Line 74 and Line 102,
Text(e.toString())can still show literal"null"to users. Use a localized fallback (e ?? libL10n.fail) to keep UI strings consistent.💡 Suggested patch
if (err != null || result != null) { final e = result?.message ?? err?.toString(); context.showRoundDialog( title: libL10n.error, - child: Text(e.toString()), + child: Text(e ?? libL10n.fail), ); }if (err != null || result != null) { final e = result?.message ?? err?.toString(); context.showRoundDialog( title: libL10n.error, - child: Text(e.toString()), + child: Text(e ?? libL10n.fail), ); }As per coding guidelines
lib/**/*.dart: UselibL10nandl10nfor localization strings, prioritizinglibL10nfromfl_libpackage to avoid duplication.Also applies to: 100-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/view/page/container/actions.dart` around lines 72 - 75, Replace the raw e.toString() used in the error dialogs with a localized fallback using libL10n: where you call context.showRoundDialog (the title uses libL10n.error) replace Text(e.toString()) with Text(e?.toString() ?? libL10n.fail) (or equivalent) so null/empty errors show the localized libL10n.fail; apply the same change to both occurrences around the context.showRoundDialog calls in this file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/core/utils/version.dart`:
- Around line 1-8: The parseVersionParts function currently uses int.parse on
match.group(1) which can throw for oversized integers; change it to use
int.tryParse for the major version (the same pattern used for minor/patch) and
if that returns null make parseVersionParts return null to preserve the
function's nullable contract (update the logic around match.group(1) in
parseVersionParts accordingly).
---
Duplicate comments:
In `@lib/view/page/container/actions.dart`:
- Around line 72-75: Replace the raw e.toString() used in the error dialogs with
a localized fallback using libL10n: where you call context.showRoundDialog (the
title uses libL10n.error) replace Text(e.toString()) with Text(e?.toString() ??
libL10n.fail) (or equivalent) so null/empty errors show the localized
libL10n.fail; apply the same change to both occurrences around the
context.showRoundDialog calls in this file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5848d17d-364c-4eed-90cb-15bcf1d186e7
📒 Files selected for processing (4)
lib/core/utils/version.dartlib/view/page/container/actions.dartlib/view/page/pve.darttest/version_test.dart
✅ Files skipped from review due to trivial changes (1)
- test/version_test.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/view/page/pve.dart
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/view/page/container/actions.dart (1)
146-146:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid rendering literal
"null"in user-facing snackbar text.At Line 146, fallback text still shows raw
"null"when no message is returned.💡 Suggested fix
- context.showSnackBar(result.message ?? 'null'); + final msg = result.message; + context.showSnackBar(msg?.isNotEmpty == true ? msg! : libL10n.fail);As per coding guidelines
lib/**/*.dart: UselibL10nandl10nfor localization strings, prioritizinglibL10nfromfl_libpackage to avoid duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/view/page/container/actions.dart` at line 146, Replace the literal "null" fallback in the context.showSnackBar call so user-facing text never renders "null": when calling context.showSnackBar with result.message (in actions.dart, the context.showSnackBar and result.message usage), use a localized fallback from fl_lib's libL10n (e.g., libL10n.of(context).<appropriateKey>) and if libL10n is not available fall back to the app l10n (l10n.of(context).<appropriateKey>); ensure the necessary import for libL10n is present, handle the nullable result.message safely, and pick a suitable localization key like "unknownMessage" or "emptyMessage" to display instead of the literal "null".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/view/page/container/actions.dart`:
- Around line 179-182: The dialog content currently uses a null-coalescing
fallback (e ?? libL10n.fail) which still shows blank when e is an empty string;
update all calls to context.showRoundDialog (including the instances around the
contexts currently using e at lines 179–182, 239–242, 254–257, 266–269, 278–281)
to guard for empty strings instead of only null by using a non-empty check on e
(e.g., use e if non-empty, otherwise libL10n.fail) so the dialog always displays
the localized failure message when e is null or empty.
---
Duplicate comments:
In `@lib/view/page/container/actions.dart`:
- Line 146: Replace the literal "null" fallback in the context.showSnackBar call
so user-facing text never renders "null": when calling context.showSnackBar with
result.message (in actions.dart, the context.showSnackBar and result.message
usage), use a localized fallback from fl_lib's libL10n (e.g.,
libL10n.of(context).<appropriateKey>) and if libL10n is not available fall back
to the app l10n (l10n.of(context).<appropriateKey>); ensure the necessary import
for libL10n is present, handle the nullable result.message safely, and pick a
suitable localization key like "unknownMessage" or "emptyMessage" to display
instead of the literal "null".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ed08c4bd-ab0c-4511-a056-39729f160feb
📒 Files selected for processing (3)
lib/core/utils/version.dartlib/view/page/container/actions.darttest/version_test.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/core/utils/version.dart
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
serverStatusUpdateIntervalin one place.0now consistently means manual refresh and no timer is started.Testing
flutter test test/refresh_interval_test.dartdart analyzeon the changed filesflutter analyze lib testSummary by CodeRabbit
Bug Fixes
New Features
Tests