-
Notifications
You must be signed in to change notification settings - Fork 3
.pr_agent_accepted_suggestions
| PR 626 (2026-02-11) |
[correctness] Missing invalidHostname l10n
Missing invalidHostname l10n
`invalidHostname` was removed from the ARB template, but multiple Dart files still call `loc(...).invalidHostname`, which will break build/localization generation. This also breaks integration tests that assert that removed string.The PR removed the invalidHostname localization key from the ARB files, but several Dart files and integration tests still reference loc(...).invalidHostname. Since the localization API is generated from the template ARB, this will cause compilation failures.
-
invalidHostnameis no longer present inlib/l10n/app_en.arb. - Remaining references exist in DDNS forms and integration tests.
- Local network hostname validation now emits more specific errors (
hostNameLengthError,hostNameStartWithHyphen,hostNameEndWithHyphen,hostNameInvalidCharacters). Integration tests should be updated accordingly.
- lib/page/advanced_settings/apps_and_gaming/ddns/views/no_ip_ddns_form.dart[71-79]
- lib/page/advanced_settings/apps_and_gaming/ddns/views/dyn_ddns_form.dart[94-102]
- lib/page/advanced_settings/apps_and_gaming/ddns/views/tzo_ddns_form.dart[71-79]
- integration_test/actions/local_network_settings_actions.dart[60-63]
- integration_test/local_network_settings_test.dart[55-66]
- lib/l10n/app_en.arb[510-526]
[correctness] Invalid-chars error not prioritized
Invalid-chars error not prioritized
`updateHostName()` returns `hostNameLengthError`/hyphen errors before checking for invalid characters, so inputs containing disallowed characters may not display the required invalid-characters message. This can violate the requirement to show a specific invalid-character validation error whenever disallowed characters are present.Hostname validation checks length/start/end hyphen before checking for invalid characters, which can prevent the UI from showing the required specific invalid-character error when disallowed characters are present.
Compliance requires that when any character outside [A-Za-z0-9-] is present, the UI displays a specific invalid-characters message.
- lib/page/advanced_settings/local_network_settings/providers/local_network_settings_provider.dart[125-151]
[reliability] Invalid char ordering brittle
Invalid char ordering brittle
Invalid characters are deduplicated via `toSet()` then joined; this implicitly relies on Set iteration order for the user-facing string and a unit test asserts a specific order. This is a robustness/code-smell issue (low likelihood today) and can be made deterministic.updateHostName formats invalid characters using .toSet().join(', '), which depends on Set iteration order. The UI string and the unit test currently assume a specific order.
This is unlikely to fail in current Dart implementations, but it’s a robustness/code-smell issue and couples visible output + tests to a collection implementation detail.
- lib/page/advanced_settings/local_network_settings/providers/local_network_settings_provider.dart[137-141]
- test/page/advanced_settings/local_network_settings/providers/local_network_settings_provider_test.dart[82-93]
| PR 623 (2026-02-10) |
[correctness] Unused `ref` parameter
Unused `ref` parameter
• `_showServerSelectionDialog` declares a `WidgetRef ref` parameter that is never used. • This typically triggers an analyzer hint (`unused_parameter`) and can fail CI if analyzer warnings are treated as violations. • Keeping unused parameters also reduces code clarity and invites confusion about intended behavior._showServerSelectionDialog has an unused WidgetRef ref parameter, which can trigger analyzer warnings.
The repository’s compliance requires no new analyzer/lint violations in modified Dart code.
- lib/page/health_check/shared_widgets/speed_test_widget.dart[68-99]
- lib/page/health_check/shared_widgets/speed_test_widget.dart[670-699]
[correctness] Go disable contract mismatch
Go disable contract mismatch
• `SpeedTestWidget` documents that when `showServerSelectionDialog` is false, the Go button should be disabled if no server is selected; however the Go button is always enabled and the handler always runs. • `SpeedTestView` explicitly sets `showServerSelectionDialog: false` and allows `selectedServer` to be null (dropdown shows a hint), so users can tap Go without selecting a server despite the widget contract.SpeedTestWidget’s showServerSelectionDialog contract says the Go button is disabled when no server is selected and the dialog is disabled. Current implementation always enables Go and always runs.
SpeedTestView sets showServerSelectionDialog: false and allows selectedServer to remain null (dropdown hint), so the user can tap Go in a state the widget contract says should be disabled.
- lib/page/health_check/shared_widgets/speed_test_widget.dart[51-66]
- lib/page/health_check/shared_widgets/speed_test_widget.dart[541-606]
- lib/page/health_check/shared_widgets/speed_test_widget.dart[72-99]
- lib/page/health_check/views/speed_test_view.dart[26-67]
- In
_startButton, computeisGoEnabledbased on: -
showServerSelectionDialog == true=> enabled -
showServerSelectionDialog == false=> enabled only ifref.read(healthCheckProvider).selectedServer != null(and/orservers.isEmpty) - Alternatively, keep current behavior but update the docstring to reflect reality.
- If disabled, consider showing helper text near the dropdown (“Select a server to start”).
| PR 622 (2026-02-10) |
[correctness] `errorMsg` allows `"null"`
`errorMsg` allows `"null"`
• The snackbar guard only checks for `null` and empty string, but it does not handle the literal string `"null"`. • If `errorMsg` is the string `"null"`, the UI will still display `"null"` to the user, which the compliance item explicitly aims to prevent. • This leaves a remaining edge case where Instant-Safety can surface a non-meaningful error message despite the new guard.The Instant-Safety snackbar message guard does not prevent displaying the literal string "null".
Compliance requires that the UI never displays blank, null, or "null" snackbar messages. The current guard only checks for null and empty, so errorMsg == "null" will still be shown.
- lib/page/instant_safety/views/instant_safety_view.dart[176-178]
[correctness] jsonDecode crash on result
jsonDecode crash on result
• JNAPError.fromJson now sets `error` to the raw `result` code when both `error` and `output` are absent; this value is not JSON. • Some downstream code unconditionally calls `jsonDecode(error.error!)`; if it receives a non-JSON result code string (the exact scenario added in the new unit test), it will throw `FormatException` and can crash the error-handling flow.JNAPError.error can now legitimately be a non-JSON result code string (e.g., ErrorInvalidHostName) when the router omits both error and output. Some code paths still do jsonDecode(error.error!) without guarding; this will throw FormatException (or TypeError if decoded type isn’t a map) and can crash error-handling.
The PR added a test asserting the new behavior (fallback to result code). Existing login and password-recovery flows assume error.error is JSON.
- lib/page/login/views/login_local_view.dart[132-154]
- lib/page/instant_admin/providers/router_password_provider.dart[106-116]
- Replace
jsonDecode(error.error!) as Map<String,dynamic>?with safe parsing: - if
error.error == null=> treat as null try { final decoded = jsonDecode(error.error!); if (decoded is Map<String,dynamic>) ... } catch (_) { ... }- Ensure subsequent indexing (
['delayTimeRemaining'],['attemptsRemaining']) is null-safe. - Consider logging/telemetry when JSON parsing fails (optional).
[reliability] Output-null still yields "null"
Output-null still yields "null"
• The new fallback uses `containsKey(keyJnapOutput)`; if a response contains an explicit `"output": null`, it will still take the `jsonEncode(output)` branch. • That recreates the same undesirable `"null"` error string called out in the new tests, just via a slightly different payload shape (explicit null vs missing key).Current fallback logic branches on containsKey(keyJnapOutput). If the response includes "output": null, the key is present and the code will still jsonEncode(null), producing the string "null"—the same bad error value this PR aims to avoid.
A router may omit output entirely or explicitly return it as null. The PR only handles the missing-key case.
- lib/core/jnap/result/jnap_result.dart[156-164]
- lib/core/jnap/result/jnap_result.dart[173-179]
- test/core/jnap/result/jnap_result_test.dart[91-124]
- Change the condition from
containsKey(keyJnapOutput)to checking the actual value (json[keyJnapOutput] != null). - Apply the same adjustment in both transaction and non-transaction branches.
- Add a unit test asserting that
{result: 'ErrorX', output: null}falls back to'ErrorX'(not"null").
| PR 609 (2026-01-29) |
[high-level] Decouple PWA logic from model number
✅ Decouple PWA logic from model number
Relocate the PWA banner's model number check from the InstallPromptBanner UI component to the PwaInstallService. This change centralizes all business rules for displaying the banner, improving code structure and maintainability.
lib/page/components/pwa/install_prompt_banner.dart [26-28]
if (!modelNumber.contains('DU-')) {
return const SizedBox.shrink();
}lib/core/pwa/pwa_install_service.dart [29-49]
Future _checkPlatformAndPersistence() async {
// If installed (standalone), never show
if (isStandalone) {
state = PwaMode.none;
return;
}
final dismissed = await isDismissedRecently();
if (dismissed) {
state = PwaMode.none;
... (clipped 11 lines)// In lib/page/components/pwa/install_prompt_banner.dart
class InstallPromptBanner extends ConsumerWidget {
Widget build(BuildContext context, WidgetRef ref) {
final modelNumber = ref.watch(globalModelNumberProvider);
// Business logic is coupled with the UI component
if (!modelNumber.contains('DU-')) {
return const SizedBox.shrink();
}
final mode = ref.watch(pwaInstallServiceProvider);
if (mode == PwaMode.none) {
return const SizedBox.shrink();
}
// ... build banner
}
}
// In lib/page/components/pwa/install_prompt_banner.dart
class InstallPromptBanner extends ConsumerWidget {
Widget build(BuildContext context, WidgetRef ref) {
// UI component only depends on the service's final state
final mode = ref.watch(pwaInstallServiceProvider);
if (mode == PwaMode.none) {
return const SizedBox.shrink();
}
// ... build banner
}
}
// In lib/core/pwa/pwa_install_service.dart
class PwaInstallService extends Notifier {
Future _checkPlatformAndPersistence() async {
// Business logic is centralized in the service
final modelNumber = ref.read(globalModelNumberProvider);
if (!modelNumber.contains('DU-')) {
state = PwaMode.none;
return;
}
// ... other checks
}
}
Suggestion importance[1-10]: 7
__
Why: This is a strong architectural suggestion that correctly identifies business logic (modelNumber check) misplaced in a UI component, improving maintainability by centralizing all display conditions within PwaInstallService.
[possible issue] Reuse PwaLogic instance for consistency
✅ Reuse PwaLogic instance for consistency
Refactor PwaInstallService to use a single, cached instance of PwaLogic instead of creating new instances in multiple methods.
lib/core/pwa/pwa_install_service.dart [102-114]
-Future<void> promptInstall() async {
- if (_deferredPrompt == null) {
- debugPrint('PWA: No deferred prompt available');
- return;
+class PwaInstallService extends Notifier<PwaMode> {
+ final _pwaLogic = PwaLogic();
+ // ... (rest of the class)
+
+ // ... in _initListeners()
+ // _pwaLogic.initListeners(...)
+
+ // ... in promptInstall()
+ Future<void> promptInstall() async {
+ if (_deferredPrompt == null) {
+ debugPrint('PWA: No deferred prompt available');
+ return;
+ }
+
+ // Show the install prompt
+ await _pwaLogic.prompt(_deferredPrompt!);
+
+ // We've used the prompt, so we can't use it again.
+ _deferredPrompt = null;
+ state = PwaMode.none;
}
- // Show the install prompt
- await PwaLogic().prompt(_deferredPrompt!);
-
- // We've used the prompt, so we can't use it again.
- _deferredPrompt = null;
- state = PwaMode.none;
+ // ... similar changes for isIOS, isMacSafari, isStandalone
}Suggestion importance[1-10]: 7
__
Why: The suggestion correctly identifies that creating new instances of PwaLogic is inefficient and proposes a good refactoring to use a single instance, improving code quality and consistency.
[possible issue] Fix malformed head tags
✅ Fix malformed head tags
Correct the malformed HTML in index.html by moving the viewport tag inside the section and removing the duplicate closing tag.
-</head>
<meta content="width=device-width, initial-scale=1.0, maximum-scale=1.0, user-scalable=no" name="viewport">
</head>Suggestion importance[1-10]: 5
__
Why: The suggestion correctly identifies and fixes malformed HTML where a tag is placed between two tags, which improves the structural correctness of the document.
[general] Eliminate unused helper file
✅ Eliminate unused helper file
Remove the unused pwa_helpers_web.dart and pwa_helpers_stub.dart files to eliminate dead code and duplication.
lib/core/pwa/pwa_helpers_web.dart [6-19]
-extension type BeforeInstallPromptEvent._(JSObject _) implements Event {
- external JSPromise prompt();
- external JSPromise<UserChoice> get userChoice;
-}
+// (file removed; definitions consolidated in pwa_logic_web.dart)
-extension type UserChoice._(JSObject _) implements JSObject {
- external String get outcome;
- external String get platform;
-}
-
-extension WindowExtension on Window {
- external JSObject? get deferredBeforeInstallPromptEvent;
-}
-Suggestion importance[1-10]: 7
__
Why: This suggestion correctly identifies that pwa_helpers_web.dart and its stub are unused and contain duplicated code, so removing them is a good cleanup that improves maintainability.
| PR 608 (2026-01-28) |
[general] Use a more appropriate banner icon
✅ Use a more appropriate banner icon
Change the banner icon from Icons.shield_outlined to Icons.info_outline to match the test expectation and better convey a warning status.
lib/page/components/views/remote_read_only_banner.dart [26-58]
return Container(
width: double.infinity,
padding: const EdgeInsets.symmetric(vertical: 12, horizontal: 16),
decoration: BoxDecoration(
color: colorScheme.errorContainer,
border: Border(
bottom: BorderSide(
- color: colorScheme.error.withValues(alpha: 0.3),
+ color: colorScheme.error.withOpacity(0.3),
width: 2,
),
),
),
child: Row(
children: [
Icon(
- Icons.shield_outlined,
+ Icons.info_outline,
color: colorScheme.onErrorContainer,
size: 24,
),
const SizedBox(width: 12),
Expanded(
child: Text(
loc(context).remoteViewModeActive,
style: textTheme.bodyLarge?.copyWith(
color: colorScheme.onErrorContainer,
fontWeight: FontWeight.w600,
),
),
),
],
),
);Suggestion importance[1-10]: 7
__
Why: The suggestion aligns the banner's icon with its corresponding test, which expects Icons.info_outline instead of Icons.shield_outlined, fixing a test failure introduced in the PR.
| PR 607 (2026-01-28) |
[possible issue] Fix incorrect null handling for server ID
✅ Fix incorrect null handling for server ID
Fix the runHealthCheck payload by ensuring targetServerID is null (not the string "null") when no server is selected, allowing it to be correctly removed from the request. Also, simplify the ID retrieval to avoid unnecessary type parsing.
lib/page/health_check/providers/health_check_provider.dart [51-65]
-final targetServerId = serverId ??
- (state.selectedServer?.serverID != null
- ? int.tryParse(state.selectedServer!.serverID)
- : null);
+final targetServerId = serverId?.toString() ?? state.selectedServer?.serverID;
final result = await repo.send(
JNAPAction.runHealthCheck,
data: {
"runHealthCheckModule": module.value,
- "targetServerID": '$targetServerId',
+ "targetServerID": targetServerId,
}..removeWhere((key, value) => value == null),
auth: true,
fetchRemote: true,
cacheLevel: CacheLevel.noCache,
);Suggestion importance[1-10]: 8
__
Why: This suggestion correctly identifies a bug where a null server ID is converted to the string "null" and sent in the payload, violating the requirement to omit the parameter. The fix is accurate and improves the code's correctness and robustness.