Skip to content

.pr_agent_accepted_suggestions

Jump to bottom
qodo-merge-bot edited this page Jun 9, 2026 · 19 revisions
                     PR 917 (2026-06-03)                    
[maintainability] OTA logic in `FirmwareUpdateView`
OTA logic in `FirmwareUpdateView` The OTA update feature adds non-trivial business logic directly in the view (building API params from providers and parsing/formatting hardware/MAC values). This violates the required feature layering and makes the UI harder to test and maintain.

Issue description

New OTA update business logic is implemented in FirmwareUpdateView (fetching device/system/wan data, formatting MAC, parsing hardware version, assembling FirmwareOtaCheckParams). Per compliance, feature business logic should live under lib/page/[feature]/services/ (or be orchestrated from the notifier using services), not in views.

Issue Context

The view currently:

  • reads devicesDataProvider, systemInfoDataProvider, and wanDataProvider
  • parses hardware version (_parseHardwareVersion)
  • formats MAC (_formatMacAddress)
  • constructs FirmwareOtaCheckParams This should be moved into a firmware-update service (or into the notifier, backed by services) so UI remains mostly declarative.

Fix Focus Areas

  • lib/page/firmware_update/views/firmware_update_view.dart[357-424]
[security] PII leaked in logs
PII leaked in logs The OTA check logs the full request URI which contains mac_address and ip_address query parameters, and LinksysHttpClient also logs `request.url`, leaking PII into logs.

Issue description

The OTA check URI includes MAC address and WAN IP as query parameters and is logged verbatim. Additionally, the shared LinksysHttpClient logs request.url for every request, which will include these query parameters.

Issue Context

MAC and WAN IP are sensitive identifiers; logging them verbatim can violate privacy expectations/policies and increases blast radius when logs are shared.

Fix Focus Areas

  • lib/page/firmware_update/services/firmware_ota_check_service.dart[33-41]
  • lib/core/cloud/http/linksys_http_client.dart[278-286]

Suggested direction

  • In FirmwareOtaCheckService, log a sanitized URI (e.g., only scheme/host/path) or omit the URI entirely.
  • In LinksysHttpClient._logRequest, consider logging request.url.replace(query: '') (omit all query values) or a redacted query for known sensitive keys.
[correctness] Non-deterministic releaseDate
Non-deterministic releaseDate FirmwareOtaInfo.fromJson falls back to DateTime.now() when release_date is missing/invalid, making the parsed model (and any Equatable comparison/state diff) non-deterministic across calls.

Issue description

FirmwareOtaInfo.fromJson() uses DateTime.now() as the fallback when release_date is missing/invalid. Because FirmwareOtaInfo is Equatable and includes releaseDate in props, two parses of the same payload can compare unequal and cause unnecessary UI/state churn.

Issue Context

This is especially problematic if the backend omits release_date for some responses (or returns an unexpected format).

Fix Focus Areas

  • lib/page/firmware_update/models/firmware_ota_info.dart[24-44]

Suggested direction

Prefer one of:

  • Make releaseDate nullable (DateTime?) and handle null in UI.
  • Use a deterministic sentinel (e.g. DateTime.fromMillisecondsSinceEpoch(0, isUtc: true)) instead of DateTime.now().
                     PR 915 (2026-06-03)                    
[maintainability] `DeviceRow` reimplements `AppListTile`
`DeviceRow` reimplements `AppListTile` A new `DeviceRow` widget is implemented using custom `Row`/`Container` layout even though `ui_kit_library` already provides `AppListTile` for standardized list rows. This bypasses the shared UI Kit component and risks inconsistent UI/UX and accessibility behavior across the app.

Issue description

DeviceRow is a custom list-row component built with Row/Container, but ui_kit_library already provides AppListTile for this use-case. This violates the UI Kit First rule and can lead to inconsistent styling/behavior.

Issue Context

AppListTile is already used in the codebase (and comes from package:ui_kit_library/ui_kit.dart). The new DeviceRow should be refactored to compose AppListTile (or another existing UI Kit list-row component) rather than reimplementing the pattern.

Fix Focus Areas

  • lib/page/_shared/components/layout_blocks/row_blocks.dart[14-75]
[maintainability] `NetworkBadgeWidget` duplicates `AppBadge`
`NetworkBadgeWidget` duplicates `AppBadge` The PR adds `NetworkBadgeWidget` implemented with a custom `Container` badge even though `ui_kit_library` already provides `AppBadge` for badges/chips. This bypasses the shared UI Kit component and risks inconsistent badge styling.

Issue description

NetworkBadgeWidget implements a custom badge UI with Container/BoxDecoration, but the UI Kit already provides AppBadge. This violates the UI Kit First rule.

Issue Context

AppBadge is used elsewhere in the app via package:ui_kit_library/ui_kit.dart. The new badge widget should be refactored to use AppBadge (and extend via UI Kit-supported properties) rather than creating a bespoke badge.

Fix Focus Areas

  • lib/page/_shared/components/layout_blocks/row_blocks.dart[124-155]
                     PR 864 (2026-05-08)                    
[correctness] `UspDeviceListTile` missing type icon
`UspDeviceListTile` missing type icon Device list tiles still render only WiFi/Ethernet connection icons and do not show OUI-derived manufacturer/vendor info, so the list remains generic. This does not meet the redesign requirement to use `DeviceClassifier` for device-type icons and display OUI vendor/manufacturer on the tile.

Issue description

UspDeviceListTile currently shows only connection-type icons (WiFi/Ethernet) and does not display OUI-derived manufacturer/vendor information, but the compliance checklist requires device-type icons driven by DeviceClassifier plus OUI vendor display.

Issue Context

The PR introduces DeviceClassifier and OuiLookup, but the device list tile UI is not yet using them.

Fix Focus Areas

  • lib/page/devices/views/components/usp_device_list_tile.dart[55-156]
[correctness] Signal label bucket mismatch
Signal label bucket mismatch `UspDeviceDetailView` derives the signal-quality text from `DeviceUIModel.signalLevel` while the signal indicator and signal filter use `getWifiSignalLevel()`; these use different RSSI thresholds, so the detail page can display the wrong quality label for a given RSSI. Example: RSSI -66 is treated as “Good” by `getWifiSignalLevel()` but becomes level=1 (“Fair”) via `DeviceUIModel.signalLevel`.

Issue description

UspDeviceDetailView uses DeviceUIModel.signalLevel to generate the signal-quality text label, but the redesigned signal indicator and device filtering now use getWifiSignalLevel() (project-wide thresholds). This produces inconsistent and sometimes incorrect quality text on the detail page.

Issue Context

  • DeviceUIModel.signalLevel still buckets RSSI with legacy thresholds (-50/-65/-80).
  • The rest of the redesigned surfaces (indicator + filters) bucket via getWifiSignalLevel() thresholds (-65/-71/-78).

Fix Focus Areas

  • lib/page/devices/views/usp_device_detail_view.dart[276-302]
  • lib/page/_shared/models/device_ui_model.dart[63-70]
  • lib/core/utils/wifi.dart[19-40]

Suggested fix

  • In UspDeviceDetailView._buildSignalSection, compute the label from getWifiSignalLevel(device.signalStrength) (and ideally reuse NodeSignalLevelExt.resolveLabel(context) for consistency/localization) instead of _getSignalQualityText(device.signalLevel).
  • Consider updating/removing DeviceUIModel.signalLevel (or re-implementing it via getWifiSignalLevel) so analytics/stats and any other consumers don’t continue using legacy thresholds.
                     PR 813 (2026-04-08)                    
[correctness] SVG path loaded as bitmap
SVG path loaded as bitmap BrandUtils.resolveAsset now returns a .svg path first, but BrandAsset.logo (TopBar) and BrandAsset.imgSup (FAQ) are rendered with Image.asset without an extension check, which will throw at runtime if an SVG variant exists in the asset bundle. The PR already demonstrates correct SVG handling for BrandAsset.textLogo, but the same safeguard is missing for other BrandAsset call sites.

Issue description

BrandUtils.resolveAsset() now prefers .svg assets, but some UI call sites still assume the returned path is a raster image and render it with Image.asset(). If an SVG variant is present for those assets, Flutter will fail at runtime trying to decode SVG as an image.

Issue Context

  • BrandUtils.resolveAsset() returns ...svg first.
  • TopBar (BrandAsset.logo) and FaqListView (BrandAsset.imgSup) use Image.asset(path) without checking file type.
  • TopBar already contains correct SVG-vs-raster branching for BrandAsset.textLogo; reuse that approach.

Fix Focus Areas

  • lib/utils.dart[720-739]
  • lib/page/components/styled/top_bar.dart[81-103]
  • lib/page/support/faq_list_view.dart[58-73]

Suggested fix

Option A (preferred): update all brand asset renderers to branch on extension:

  • If path.endsWith('.svg'), render with SvgPicture.asset(...).
  • Else render with Image.asset(...). Option B: constrain SVG priority to only the asset types whose renderers support SVG (e.g., keep .svg priority only for textLogo), until other call sites are updated.
                     PR 777 (2026-04-01)                    
[correctness] Refresh action never handled
Refresh action never handled The demo widget `chart_integration_demo_simple.json` uses `$action: "refresh_chart_data"` for its Refresh button, but `PackageWidgetRenderer` only handles the `refresh_data` action type, so tapping Refresh will be treated as an unknown action and won’t trigger any reload logic.

Issue description

The chart demo widget’s Refresh button uses an action name (refresh_chart_data) that PackageWidgetRenderer does not handle. This makes the button a no-op (it will be logged as an unknown action).

Issue Context

PackageWidgetRenderer currently supports refresh_data and routes it to _handleRefreshDataAction, which refreshes either the USP GET or HTTP fetch depending on the template.

Fix Focus Areas

  • assets/a2ui/widgets/chart_integration_demo_simple.json[63-72]
  • lib/page/dashboard/widgets/package_widget_renderer.dart[200-236]

Suggested fix

Either:

  1. Change the demo JSON to use {"$action": "refresh_data"} for the Refresh button, or
  2. Add a handler case for refresh_chart_data that delegates to _handleRefreshDataAction (or otherwise performs the intended refresh).
[correctness] HTTP widget bindings mismapped
HTTP widget bindings mismapped `demo_network_traffic.json` maps HTTP response fields into keys like `current_download_rate`, but the template binds to `traffic.download.rate`; since `applyMapping` uses the mapping key as the output key, the template’s `$bind` paths will not exist in the data map and the widget will render empty/incorrect values if used.

Issue description

demo_network_traffic.json’s HTTP mapping cannot satisfy the template’s $bind paths with the current applyMapping behavior, because applyMapping outputs a flat map keyed by the mapping keys (LHS), while the template binds to the mapping values (RHS).

Issue Context

  • Renderer behavior:
  • _fetchHttpData calls applyMapping(json, ds.mapping) and stores the returned map.
  • applyMapping sets result[entry.key] = resolvePath(json, entry.value).
  • Therefore, if the template binds to traffic.download.rate, the mapping must produce an entry with key traffic.download.rate.
  • The demo JSON also includes headers, timeout, and errorHandling fields which are not parsed by HttpDataSourceConfig.fromJson and are not used by _fetchHttpData.

Fix Focus Areas

  • assets/a2ui/widgets/demo_network_traffic.json[13-43]
  • lib/page/dashboard/widgets/package_widget_renderer.dart[176-183]
  • lib/page/dashboard/widgets/package_widget_renderer.dart[534-542]
  • lib/page/dashboard/models/package_widget_template.dart[87-113]

Suggested fix

Option A (preferred for this repo’s flat-key binding style):

  • Change mapping keys to exactly match the $bind paths used in the template, e.g.
  • "traffic.download.rate": "traffic.download.rate"
  • "traffic.upload.rate": "traffic.upload.rate"
  • "traffic.hourly_stats": "traffic.hourly_stats" (and update the remaining entries similarly). Option B:
  • Update the template $bind paths to reference the mapping keys (current_download_rate, etc.). Additionally:
  • Either remove unsupported dataSource fields (headers, timeout, errorHandling) from the demo JSON to avoid implying behavior that doesn’t exist, or extend HttpDataSourceConfig + _fetchHttpData to actually honor them.
                     PR 716 (2026-03-20)                    
[correctness] setState during build
setState during build SseConnectionBanner calls _reconcile() via stateAsync.whenData() inside build(), and _reconcile() can call setState(), which can trigger Flutter's "setState() called during build" assertion when SSE state changes. This can cause runtime failures or unstable rendering during reconnect cycles.

Issue description

SseConnectionBanner currently triggers _reconcile() from inside build() using stateAsync.whenData(...). Because _reconcile() calls setState(), this can call setState() during the widget build phase and trigger Flutter assertions and unstable rebuild behavior.

Issue Context

This widget is driven by sseConnectionStateProvider (a StreamProvider), so state changes can happen frequently (connecting/reconnecting). State reconciliation should be done in a listener/effect rather than synchronously in build().

Fix Focus Areas

  • lib/page/_shared/components/sse_connection_banner.dart[44-91]

Suggested fix

  • Replace stateAsync.whenData(_reconcile) inside build() with a ref.listen(sseConnectionStateProvider, ...) callback.
  • In the listener callback, handle:
  • data: call _reconcile(realState).
  • loading/error: cancel the grace timer and hide the banner (or otherwise define desired behavior) so the banner cannot get stuck displaying a stale _visibleState.
  • Keep build() pure: render based only on _visibleState.
  • (Optional) Track the latest non-connected state so the grace timer shows the most recent connecting/reconnecting state rather than a stale captured value.
[reliability] Timeout escapes to settings
Timeout escapes to settings wifiDataProvider now times out after 15s and will throw, but UspWifiSettingsNotifier.performFetch awaits wifiDataProvider.future without try/catch, so a timeout can escape and prevent WifiSettingsStatus.errorMessage from being set. Because build() fires fetch() via an unhandled microtask, the WiFi Settings UI can remain stuck in loading and/or produce unhandled async errors.

Issue description

wifiDataProvider can now throw TimeoutException after 15 seconds. UspWifiSettingsNotifier.performFetch() awaits wifiDataProvider.future without catching errors, and the preservable fetch pipeline does not catch exceptions—so timeouts can escape and bypass the intended WifiSettingsStatus(errorMessage: ...) UI path.

Issue Context

UspWifiSettingsNotifier.build() kicks off fetch() in a microtask without .catchError(...), and the WiFi Settings UI relies on status.isLoading / status.errorMessage to leave loading and display errors.

Fix Focus Areas

  • lib/page/wifi_settings/providers/wifi_data_provider.dart[122-130]
  • lib/page/wifi_settings/providers/usp_wifi_settings_provider.dart[41-50]
  • lib/page/wifi_settings/providers/usp_wifi_settings_provider.dart[79-126]
  • lib/framework/preservable_notifier_mixin.dart[49-55]

Suggested fix

  • In UspWifiSettingsNotifier.performFetch() wrap await ref.read(wifiDataProvider.future) with try/catch (at least TimeoutException, ideally broad) and return:
  • (null, WifiSettingsStatus(errorMessage: '...')) so the UI exits loading and shows the error state.
  • In build(), change the microtask kickoff to handle errors:
  • Future.microtask(fetch).catchError((e, st) { ...set error status... }); or remove the microtask pattern and let the UI explicitly call fetch() with proper awaiting.
  • Optionally, consider aligning with the resilience approach used in devicesDataProvider (fallback) if WiFi settings should degrade rather than error.
                     PR 709 (2026-03-19)                    
[correctness] Cannot clear selected server
Cannot clear selected server HealthCheckState.copyWith() prevents setting selectedServer to null, so setSelectedServer(null) never clears an existing selection. SpeedTestView’s “empty” dropdown option attempts to clear the selection but the state retains the previous server, making the UI behavior inconsistent.

Issue description

HealthCheckState.copyWith() cannot clear selectedServer because it uses selectedServer ?? this.selectedServer. Calls like setSelectedServer(null) therefore keep the old value, which breaks the Speed Test dropdown's “empty” option behavior.

Issue Context

SpeedTestView includes an empty dropdown option that calls setSelectedServer(null), implying the UX supports clearing the selection.

Fix Focus Areas

  • lib/page/health_check/providers/health_check_state.dart[59-82]
  • lib/page/health_check/providers/health_check_provider.dart[274-276]
  • lib/page/health_check/views/speed_test_view.dart[248-262]

Implementation notes

Update copyWith to support explicitly setting selectedServer to null (e.g., via a sentinel/Object? parameter pattern or an additional boolean like clearSelectedServer). Then keep setSelectedServer(null) working as intended.

[maintainability] Missing locale translations
Missing locale translations The new speedTestFailedToLoadServers string is only added to app_en.arb, but the UI now references it for all locales. Other locale ARB files do not define this key, leading to untranslated output being generated/tracked (and potentially incorrect UI text for non-English users).

Issue description

A new localization key is used in UI but is only defined in app_en.arb. Other locales lack the key, causing missing/untranslated strings for non-English users.

Issue Context

l10n.yaml uses app_en.arb as the template, and the UI references speedTestFailedToLoadServers.

Fix Focus Areas

  • lib/l10n/app_en.arb[900-914]
  • lib/l10n/app_es.arb[710-721]
  • l10n.yaml[1-6]

Implementation notes

Add "speedTestFailedToLoadServers": "..." to each lib/l10n/app_*.arb (with appropriate translations, or at minimum an English placeholder to unblock builds/automation if translations are handled later).

                     PR 694 (2026-03-18)                    
[reliability] Unhandled invalidation fetch
Unhandled invalidation fetch _PreservableDelegate.onSseInvalidation() calls fetch(forceRemote: true) without awaiting or handling errors, so exceptions from performFetch become unhandled asynchronous errors. Some notifiers (e.g., WiFi settings) perform awaits that can throw (wifiDataProvider.future), making this unhandled-error path reachable during normal SSE invalidation.

Issue description

onSseInvalidation() triggers fetch(forceRemote: true) without awaiting or handling errors. If performFetch throws, this becomes an unhandled asynchronous error.

Issue Context

At least one notifier (UspWifiSettingsNotifier) calls onSseInvalidation() from a provider listener and its performFetch awaits wifiDataProvider.future, which can throw.

Fix Focus Areas

  • lib/usp_page/_framework/preservable_notifier_mixin.dart[97-104]

Implementation direction

Option A (preferred):

  • Import dart:async and use unawaited(fetch(forceRemote: true).catchError((e, st) { /* log */ })); Option B:
  • Change onSseInvalidation() to Future<void> and update call sites to unawaited(onSseInvalidation()) with internal try/catch.
[correctness] Save stuck on fetch-fail
Save stuck on fetch-fail _PreservableDelegate.save() swallows post-save fetch(forceRemote:true) failures and returns the current state without clearing transient saving flags, so pages can remain stuck in isSaving=true after a successful mutation if the re-fetch fails. Notifiers like UspWifiSettingsNotifier set isSaving=true before calling super.save() and only clear it via the subsequent fetch, so swallowing that fetch failure can leave the UI permanently “saving”.

Issue description

_PreservableDelegate.save() catches failures from the post-save fetch(forceRemote: true) and returns the current state. If notifiers set status.isSaving=true before calling super.save(), and rely on the post-save fetch to reset it, a fetch failure leaves the UI stuck in a saving state.

Issue Context

UspWifiSettingsNotifier.save() sets isSaving: true and only clears it in the catch path. Because _PreservableDelegate.save() swallows the fetch error, the notifier never enters its catch, so isSaving is not cleared.

Fix Focus Areas

  • lib/usp_page/_framework/preservable_notifier_mixin.dart[78-92]
  • lib/usp_page/wifi_settings/providers/usp_wifi_settings_provider.dart[134-149]

Implementation direction

Choose one:

  1. Rethrow post-save fetch errors from _PreservableDelegate.save() after logging, so callers can reliably clear isSaving.
  2. Guarantee status cleanup: update all save() overrides to clear isSaving in a finally block (not only in catch), and/or add a framework callback/hook invoked when post-save fetch fails so the notifier can reset its status.
                     PR 684 (2026-03-13)                    
[security] SSE logs auth token
SSE logs auth token UspBridgeClient emits `_debug` SSE events that include the Bearer token prefix and raw SSE chunks/frames, and SseConnectionManager logs those `_debug` events. This can leak credentials/notification payloads into logs and create extreme log volume on active SSE streams.

Issue description

SSE transport currently emits and logs _debug events that include credential material (Bearer token prefix) and raw SSE chunk/frame contents. This can leak secrets and sensitive payloads into logs and significantly increase log volume.

Issue Context

UspBridgeClient uses _debug events as internal diagnostics, and SseConnectionManager logs them unconditionally.

Fix Focus Areas

  • lib/usp/services/usp_bridge_client.dart[120-210]
  • lib/usp/services/sse_connection_manager.dart[137-141]
[reliability] Logout triggers reauth
Logout triggers reauth AuthNotifier.logout calls USP logout before unregistering SSE/OBUSPA subscriptions, but subscription deletion uses UspService auth-retry which can trigger re-auth via restoreSession. This can re-login during logout and leave sessions/subscriptions in an inconsistent state.

Issue description

During logout, USP is logged out before SSE/OBUSPA subscription cleanup. Cleanup performs authenticated deletes with automatic 401 retry that can trigger reauth() and restoreSession(), potentially re-authenticating while logging out.

Issue Context

The subscription registry delete path uses UspService._withAuthRetry, which calls reauth() on 401; UspAuthCoordinator wires onReauthRequired to restoreSession().

Fix Focus Areas

  • lib/providers/auth/auth_provider.dart[415-423]
  • lib/usp/services/sse_subscription_registry.dart[102-127]
  • lib/usp/services/usp_service.dart[105-155]
  • lib/usp/providers/usp_auth_coordinator.dart[25-27]
[correctness] Operate result cross-talk
Operate result cross-talk SseOperationAwaiter correlates OperationComplete results only by `command_name`, so concurrent operations of the same command can complete from the first matching event and return the wrong result. This breaks correctness for scenarios like overlapping ping/traceroute requests of the same type.

Issue description

OperationComplete correlation currently matches only on command_name, which is not unique per invocation. Concurrent operations of the same command can be completed by the wrong SSE event.

Issue Context

command_key is present in the OperationComplete payload and parsed into OperateResult, but is not used in the matching predicate.

Fix Focus Areas

  • lib/usp/services/sse_operation_awaiter.dart[72-109]
  • lib/usp/services/sse_operation_awaiter.dart[214-231]
                     PR 631 (2026-02-12)                    
[reliability] Null `error` in JNAPError
Null `error` in JNAPError `JNAPError.fromJson` can set `error` to null when a JNAP response contains `result` but lacks both `error` and `output`, instead of falling back to the `result` code string. This can still lead to empty/meaningless error messages reaching the UI and violates the required fallback behavior.

Issue description

JNAPError.fromJson can return error: null when a response has a result but no error and no output. Per requirements, it must instead fall back to the result code string so the UI never ends up with a meaningless/"null"-derived message.

Issue Context

This occurs for responses like {"result":"ErrorInvalidHostName"} (no error, no output). The code currently sets error to null in that scenario.

Fix Focus Areas

  • lib/core/jnap/result/jnap_result.dart[151-179]
                     PR 628 (2026-02-11)                    
[correctness] Unsorted gauge markers
Unsorted gauge markers For upperBound values between 600–700 Mbps, _generateMarkers() adds a 750 marker unconditionally, producing markers that exceed upperBound and are not sorted. This can cause incorrect AppGauge rendering and may trigger assertions/logic errors if the gauge assumes markers are within range and ascending.

Issue description

_generateMarkers() can return marker values larger than upperBound and out-of-order (e.g., upperBound=600 yields ... 750, 600). This can break gauge rendering/logic.

Issue Context

upperBound is computed by rounding historical max speed up to the nearest hundred, so values like 600 and 700 are expected.

Fix Focus Areas

  • lib/page/health_check/shared_widgets/speed_test_widget.dart[169-182]
  • lib/page/health_check/shared_widgets/speed_test_widget.dart[113-134]

Implementation notes

  • Only add 750 when upperBound >= 750.
  • Always append upperBound as the maximum marker.
  • Optionally: final sorted = {...markers}.toList()..sort(); and ensure all markers <= upperBound.
[correctness] Constitution date mismatch
Constitution date mismatch constitution.md now contains conflicting “Last Amended” dates between the header and the new footer, which can confuse contributors and any governance process relying on this metadata.

Issue description

constitution.md contains two conflicting “Last Amended” fields (header vs footer). This inconsistency was amplified by updating the footer date while leaving the header unchanged.

Issue Context

The header is typically treated as document metadata and should reflect the latest amendment date to avoid confusion.

Fix Focus Areas

  • constitution.md[3-8]
  • constitution.md[1160-1160]

Suggested approach

  • Set header **Last Amended:** to 2026-02-11 (to match the footer), or
  • Remove the footer date line and keep a single source of truth in the header.
                     PR 627 (2026-02-11)                    
[correctness] Constitution amendment date mismatch
Constitution amendment date mismatch constitution.md now states two different “Last Amended” dates (header vs footer). This makes the constitution’s revision metadata ambiguous for reviewers and any automation that may rely on it.

Issue description

constitution.md contains conflicting amendment metadata: the header lists Last Amended: 2025-12-22 while the footer lists Last Amended: 2026-02-11.

Issue Context

This PR adds Article XIV and updates the footer metadata, but does not update the header metadata.

Fix Focus Areas

  • constitution.md[1-8]
  • constitution.md[1158-1160]
[correctness] Skill vs constitution format rule
Skill vs constitution format rule The new implement-feature-with-checks skill instructs formatting entire feature directories, which can include unmodified files. This conflicts with the constitution’s review checklist requiring `dart format` only on modified files.

Issue description

The skill’s formatting step uses directory-level dart format, which can format unmodified files and violate the constitution’s checklist.

Issue Context

The constitution is explicit about limiting formatting to modified files to avoid noisy diffs.

Fix Focus Areas

  • .claude/skills/implement-feature-with-checks/SKILL.md[234-237]
  • constitution.md[838-840]
                     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.

Issue description

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.

Issue Context

  • invalidHostname is no longer present in lib/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.

Fix Focus Areas

  • 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.

Issue description

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.

Issue Context

Compliance requires that when any character outside [A-Za-z0-9-] is present, the UI displays a specific invalid-characters message.

Fix Focus Areas

  • 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.

Issue description

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.

Issue Context

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.

Fix Focus Areas

  • 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.

Issue description

_showServerSelectionDialog has an unused WidgetRef ref parameter, which can trigger analyzer warnings.

Issue Context

The repository’s compliance requires no new analyzer/lint violations in modified Dart code.

Fix Focus Areas

  • 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.

Issue description

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.

Issue Context

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.

Fix Focus Areas

  • 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]

Suggested approach

  • In _startButton, compute isGoEnabled based on:
  • showServerSelectionDialog == true => enabled
  • showServerSelectionDialog == false => enabled only if ref.read(healthCheckProvider).selectedServer != null (and/or servers.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.

Issue description

The Instant-Safety snackbar message guard does not prevent displaying the literal string "null".

Issue Context

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.

Fix Focus Areas

  • 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.

Issue description

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.

Issue Context

The PR added a test asserting the new behavior (fallback to result code). Existing login and password-recovery flows assume error.error is JSON.

Fix Focus Areas

  • lib/page/login/views/login_local_view.dart[132-154]
  • lib/page/instant_admin/providers/router_password_provider.dart[106-116]

Implementation notes

  • 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).

Issue description

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.

Issue Context

A router may omit output entirely or explicitly return it as null. The PR only handles the missing-key case.

Fix Focus Areas

  • 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]

Implementation notes

  • 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 620 (2026-02-09)                    
[reliability] Theme assets outside allowed dirs
Theme assets outside allowed dirs • `pubspec.yaml` adds `assets/theme/`, introducing a new asset directory outside the designated `assets/resources/` (and `assets/icons/`) locations. • This breaks the documented asset organization policy and can lead to inconsistent asset placement and harder-to-manage builds.

Issue description

The PR adds a new Flutter asset directory assets/theme/, but the compliance checklist requires general resources to live under assets/resources/.

Issue Context

Theme JSON files are general resources and should follow the repo’s asset directory conventions to keep asset management consistent.

Fix Focus Areas

  • pubspec.yaml[132-138]
  • lib/utils.dart[769-776]
[correctness] THEME_JSON ignored in normal
THEME_JSON ignored in normal • In the new flow, `deviceThemeConfigProvider` only calls `ThemeConfigLoader.load()` when `THEME_SOURCE` is *not* `normal`; otherwise it always uses `BrandUtils.getDeviceTheme(modelNumber)`. • `ThemeConfigLoader` still supports the prior “priority” behavior where `THEME_JSON` alone (non-empty) wins even when `THEME_SOURCE=normal`. • Result: builds that previously set only `THEME_JSON` (without forcing `THEME_SOURCE=cicd`) will silently stop applying the override theme.

Issue description

deviceThemeConfigProvider ignores THEME_JSON unless THEME_SOURCE is set to a non-normal value. Previously, ThemeConfigLoader would apply THEME_JSON in normal mode via priority resolution.

Issue Context

This is a behavioral regression for CI/CD and local builds that set only --dart-define=THEME_JSON=... (and rely on default THEME_SOURCE=normal).

Fix Focus Areas

  • lib/providers/device_theme_config_provider.dart[59-76]
  • lib/theme/theme_config_loader.dart[28-36]

Suggested implementation

  1. Add public getters in ThemeConfigLoader for env values (e.g., static String get themeJsonEnv => _themeJsonEnv; and similar for network url/asset path).
  2. In deviceThemeConfigProvider, update the branch condition to also use ThemeConfigLoader.themeJsonEnv.isNotEmpty (and optionally other env signals) to decide when to call ThemeConfigLoader.load().
  3. (Optional) Add a short migration note in docs/build scripts stating that THEME_JSON-only builds must set THEME_SOURCE=cicd if you choose not to support the old behavior.
[correctness] Log tags missing `]:` format
Log tags missing `]:` format • New log messages use bracketed tags like `"[DeviceThemeConfig] ..."` and `"[BrandUtils] ..."` but omit the `]:` delimiter expected by the app’s log tag parser. • This makes logs harder to parse/audit and prevents them from being categorized properly in the web log cache.

Issue description

New log statements do not follow the structured tag format expected by the logger ([TAG]:message). This prevents proper tag extraction and reduces audit/debug value.

Issue Context

lib/core/utils/logger.dart parses tags using a regex that expects a colon after the closing bracket.

Fix Focus Areas

  • lib/providers/device_theme_config_provider.dart[64-78]
  • lib/utils.dart[749-785]
  • lib/core/utils/logger.dart[81-84]
[reliability] build_web arg breaking change
build_web arg breaking change • `build_web.sh` changed positional args from a single `$7` theme value to `$7`=`themeSource` and `$8`=`themeJson`. • Repo documentation still shows invoking `./build_web.sh` without parameters or only documents 6 parameters, which makes it easy to call the script incorrectly. • Without argument validation/backward compatibility, existing callers that passed theme JSON as the 7th arg will silently produce builds with `THEME_SOURCE` set to a JSON blob and empty `THEME_JSON`.

Issue description

build_web.sh now expects 8 positional args but docs still show 0–6 args. This can silently misconfigure THEME_SOURCE/THEME_JSON, breaking theme overrides.

Issue Context

The script is used for web builds and now wires the theme override env defines.

Fix Focus Areas

  • build_web.sh[14-22]
  • README.md[69-76]
  • CLAUDE.md[21-25]

Suggested implementation

  1. Add usage() + check $# at script start; exit non-zero with help text when insufficient args.
  2. Backward compatibility:
  • If $# -eq 7: set themeSource=cicd and themeJson=$7 (or infer based on content), so existing callers keep working.
  • If $# -ge 8: use themeSource=$7, themeJson=$8.
  1. Update README/CLAUDE to document full parameter list and show an example for forcing theme via THEME_SOURCE/THEME_JSON.
                     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.

Examples:

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)

Solution Walkthrough:

Before:

// 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
  }
}

After:

// 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.

web/index.html [147-149]

-</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.

Clone this wiki locally