-
Notifications
You must be signed in to change notification settings - Fork 3
.pr_agent_accepted_suggestions
| 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.