Dev notifications#84
Open
theSoberSobber wants to merge 16 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a notifications subsystem (dm-notifier container + dm-backend), CI/CD workflows and remote deploy tooling, Flutter subscription/pairing/notification services and screens, platform plugin/build changes, new localization keys, and supporting Docker/Node artifacts. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App as Flutter App
participant Sub as SubscriptionService
participant Pair as PairingService
participant Backend as dm-backend
participant Docker as Docker Host
participant Notif as dm-notifier
participant FCM as Firebase Cloud Messaging
User->>App: enable notifications for server
App->>Sub: ensure Pro / show paywall
Sub-->>App: isPro=true
App->>Pair: generatePairingToken(serverId)
Pair->>Backend: POST /api/pairing/generate {rc_user_id, server_id}
Backend-->>Pair: pairing_token
Pair->>Docker: SSH deploy dm-notifier container with pairing_token
Docker-->>Pair: container started & healthy
Pair-->>App: deployment success
Docker->>Notif: Docker events (start/stop/...)
Notif->>Backend: POST /api/events {pairing_token, event}
Backend->>FCM: send notifications
FCM-->>App: delivers notification
App-->>User: display notification
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (13)
android/settings.gradle.kts-24-24 (1)
24-24:⚠️ Potential issue | 🟡 MinorUpdate Google Services plugin to version 4.4.4.
The plugin declaration pattern is correct with
apply falsefor project-level declaration. However, version 4.4.2 is outdated—update to the latest version 4.4.4.id("com.google.gms.google-services") version "4.4.4" apply false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/settings.gradle.kts` at line 24, Update the Google Services Gradle plugin version: locate the plugin declaration using the id "com.google.gms.google-services" (the line that currently sets version "4.4.2" with apply false) and change its version to "4.4.4" so the project-level plugin declaration reads the newer 4.4.4 release.android/app/build.gradle.kts-92-95 (1)
92-95:⚠️ Potential issue | 🟡 MinorUpdate desugar_jdk_libs to the latest stable version.
The
coreLibraryDesugaringdependency is correctly added and required by theisCoreLibraryDesugaringEnabled = trueflag. However, version 2.1.4 is outdated; the latest stable version is 2.1.5 (released Feb 25, 2025). Update the dependency tocom.android.tools:desugar_jdk_libs:2.1.5.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/build.gradle.kts` around lines 92 - 95, Update the coreLibraryDesugaring dependency version from 2.1.4 to 2.1.5: locate the dependencies block where coreLibraryDesugaring("com.android.tools:desugar_jdk_libs:2.1.4") is declared (the same block used alongside isCoreLibraryDesugaringEnabled = true) and change the artifact version to "com.android.tools:desugar_jdk_libs:2.1.5" so the project uses the latest stable desugar_jdk_libs.dm-notifier/.gitattributes-1-2 (1)
1-2:⚠️ Potential issue | 🟡 MinorFix path patterns for nested
.gitattributesscope.
Fromdm-notifier/.gitattributes, Line 1 and Line 2 should be relative to that directory; current patterns won’t apply to the intended files.💡 Proposed fix
-dm-notifier/notifier.sh text eol=lf -dm-notifier/healthcheck.sh text eol=lf +notifier.sh text eol=lf +healthcheck.sh text eol=lf🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dm-notifier/.gitattributes` around lines 1 - 2, The .gitattributes entries are using the repository-relative prefix ("dm-notifier/notifier.sh" and "dm-notifier/healthcheck.sh") even though the file sits inside that directory, so the patterns won't match; update the lines to use paths relative to the file (e.g. "notifier.sh text eol=lf" and "healthcheck.sh text eol=lf") so that the notifier.sh and healthcheck.sh entries take effect.lib/data/services/notification_service.dart-99-100 (1)
99-100:⚠️ Potential issue | 🟡 MinorAvoid logging FCM tokens in production.
FCM tokens are sensitive — anyone with a token can send push notifications to that device. Logging them (even with
debugPrint) may expose them in crash reports or debug logs.🛡️ Mask or remove token from logs
_fcmToken = await FirebaseMessaging.instance.getToken(); - debugPrint('NotificationService: FCM token: $_fcmToken'); + debugPrint('NotificationService: FCM token obtained (length: ${_fcmToken?.length ?? 0})');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/data/services/notification_service.dart` around lines 99 - 100, Remove or redact logging of FCM tokens: in NotificationService where _fcmToken is assigned from FirebaseMessaging.instance.getToken() and currently printed via debugPrint('NotificationService: FCM token: $_fcmToken'), stop printing the raw token; either remove the debugPrint call entirely or replace it with a non-sensitive message (e.g., log that a token was retrieved without including _fcmToken, or log a hashed/masked version). Update any related logging in the class to never emit the full _fcmToken.dm-notifier/Dockerfile-1-29 (1)
1-29:⚠️ Potential issue | 🟡 MinorContainer runs as root user — consider adding a non-root user.
The container currently runs all processes as root. While the notifier only needs read access to the Docker socket, running as a non-root user improves security posture and limits potential damage from container escapes.
🛡️ Proposed fix to add a non-root user
FROM docker:24-cli AS base # Use Alpine for minimal footprint RUN apk add --no-cache curl jq +# Create a non-root user for running the notifier +RUN addgroup -S notifier && adduser -S notifier -G notifier + WORKDIR /app COPY notifier.sh /app/notifier.sh COPY healthcheck.sh /app/healthcheck.sh # Fix Windows CRLF -> Unix LF line endings RUN sed -i 's/\r$//' /app/notifier.sh /app/healthcheck.sh && \ chmod +x /app/notifier.sh /app/healthcheck.sh +# Note: The container needs access to docker.sock which is typically owned by root:docker +# The user running the container must ensure proper socket permissions or run with appropriate group +USER notifier + # Environment variables (set by the app during deployment)Note: If the Docker socket requires root access on the target system, this change may require adjusting socket permissions or running with
--group-addto add the docker group.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dm-notifier/Dockerfile` around lines 1 - 29, Add a non-root user in the Dockerfile and run the notifier as that user: create a dedicated group/user (e.g., "notifier"), chown /app/notifier.sh and /app/healthcheck.sh to that user and set USER notifier before ENTRYPOINT; ensure the HEALTHCHECK still runs under that user. Also add guidance to handle access to the Docker socket by either making the docker group available at runtime (recommend running container with --group-add docker or passing the docker group GID via build ARG and adding the user to that group) or documenting that socket permission adjustments are required so the non-root user can read /var/run/docker.sock.lib/data/services/notification_service.dart-102-103 (1)
102-103:⚠️ Potential issue | 🟡 MinorToken refresh listener may be registered multiple times.
If
requestPermissionAndToken()is called more than once, a new listener is added each time viaonTokenRefresh.listen(...). This could cause_onTokenRefreshto fire multiple times per actual refresh event.🐛 Track subscription to prevent duplicates
+ StreamSubscription<String>? _tokenRefreshSubscription; + Future<bool> requestPermissionAndToken() async { try { // ... existing code ... // Listen for token refreshes - FirebaseMessaging.instance.onTokenRefresh.listen(_onTokenRefresh); + _tokenRefreshSubscription?.cancel(); + _tokenRefreshSubscription = FirebaseMessaging.instance.onTokenRefresh.listen(_onTokenRefresh);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/data/services/notification_service.dart` around lines 102 - 103, The token refresh listener is being registered each time requestPermissionAndToken() runs causing duplicate handlers; modify notification_service.dart to store the StreamSubscription returned by FirebaseMessaging.instance.onTokenRefresh.listen(...) (e.g., a private field like _tokenRefreshSub) and before calling listen again cancel and null-check the existing subscription (or check if already active) so _onTokenRefresh is only subscribed once; ensure you also cancel the subscription in the service's dispose/close method to avoid leaks.dm-backend/.env.example-1-5 (1)
1-5:⚠️ Potential issue | 🟡 MinorPort mismatch with Dockerfile and docker-compose.
The example sets
PORT=3000, butDockerfileexposes port7847anddocker-compose.ymlmaps7847:7847. This inconsistency will cause confusion or connection failures when developers copy this example.Proposed fix
# Port to run the server on -PORT=3000 +PORT=7847 # Firebase service account JSON (alternative to placing firebase-service-account.json file) # FIREBASE_SERVICE_ACCOUNT={"type":"service_account","project_id":"..."}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dm-backend/.env.example` around lines 1 - 5, The PORT in .env.example is set to 3000 but the container image and compose map use 7847; update the example to set PORT=7847 (and adjust the accompanying comment) so the environment example matches the Dockerfile/docker-compose mapping; ensure the symbolic name PORT is consistent across .env.example, Dockerfile EXPOSE and docker-compose service port mappings (7847) to avoid runtime mismatches.dm-backend/README.md-29-40 (1)
29-40:⚠️ Potential issue | 🟡 MinorPort documentation mismatch.
The Docker run example maps port 3000 and the configuration table states
PORTdefaults to3000, butDockerfileexposes7847and the healthcheck pingslocalhost:7847. Update the documentation to reflect the actual port.Proposed fix
## Docker ```bash docker build -t dm-backend . -docker run -d -p 3000:3000 \ +docker run -d -p 7847:7847 \ -v ./firebase-service-account.json:/app/firebase-service-account.json:ro \ dm-backendConfiguration
Env Var Default Description - PORT3000+ PORT7847FIREBASE_SERVICE_ACCOUNT— Firebase service account JSON string (alternative to file) </details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@dm-backend/README.mdaround lines 29 - 40, The README's Docker run example
and PORT default are inconsistent with the actual service port; update the
Docker run command to map host port 7847 to container port 7847 (replace "-p
3000:3000" with "-p 7847:7847") and change the configuration table default for
PORTfrom3000to7847so the README matches the Dockerfile/healthcheck
that use port 7847.</details> </blockquote></details> <details> <summary>lib/presentation/screens/notification_setup_screen.dart-76-76 (1)</summary><blockquote> `76-76`: _⚠️ Potential issue_ | _🟡 Minor_ **Localize deployment step messages.** These progress labels are hardcoded English and bypass localization. Also applies to: 82-82, 85-85 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@lib/presentation/screens/notification_setup_screen.dartat line 76, The
hardcoded progress messages assigned to _deployStep (e.g., "Requesting
notification permission..." and the other two occurrences) must be replaced with
localized strings from your app's localization class; locate the assignments in
_NotificationSetupScreenState (where _deployStep is set around the three noted
spots) and call the appropriate localization getters (e.g.,
AppLocalizations.of(context).requestingNotificationPermission) instead of
literal English text, ensuring you have access to BuildContext (use
mounted/context from the State) and import the localization file; update all
three assignments to use the corresponding localized keys.</details> </blockquote></details> <details> <summary>lib/presentation/screens/notification_setup_screen.dart-4-6 (1)</summary><blockquote> `4-6`: _⚠️ Potential issue_ | _🟡 Minor_ **Remove unused imports and state flagged by CI.** `SubscriptionService`, `SshConnectionService`, and `_logs` are unused per pipeline warnings. Also applies to: 35-35 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@lib/presentation/screens/notification_setup_screen.dartaround lines 4 - 6,
Remove the unused imports SubscriptionService and SshConnectionService from
lib/presentation/screens/notification_setup_screen.dart and delete the unused
state member _logs (and any code that only references it) from the
NotificationSetupScreen's State (e.g., NotificationSetupScreenState or
_NotificationSetupScreenState); keep the NotificationService import if it is
used, run the analyzer to ensure no other unused symbols remain, and update any
constructors or methods that referenced _logs to avoid dangling references.</details> </blockquote></details> <details> <summary>lib/presentation/screens/server_list_screen.dart-288-297 (1)</summary><blockquote> `288-297`: _⚠️ Potential issue_ | _🟡 Minor_ **Refresh server data after returning from notification setup.** This navigation isn’t awaited, so notification state changes made in `NotificationSetupScreen` can remain stale in the list until manual refresh. <details> <summary>Suggested fix</summary> ```diff - onTap: () { - Navigator.push( + onTap: () async { + await Navigator.push( context, MaterialPageRoute( builder: (context) => NotificationSetupScreen( server: server, ), ), ); + if (mounted) { + await _loadServers(); + } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/presentation/screens/server_list_screen.dart` around lines 288 - 297, Make the onTap handler async and await the Navigator.push call to NotificationSetupScreen, then trigger the server-list refresh when it returns; i.e., change the onTap closure to async, await Navigator.push(context, MaterialPageRoute(builder: (_) => NotificationSetupScreen(server: server))), and after the await call your screen's server reload method (e.g., _loadServers(), _fetchServers(), or call setState to re-query/update the servers) so any notification changes are reflected when control returns.lib/presentation/screens/server_list_screen.dart-323-340 (1)
323-340:⚠️ Potential issue | 🟡 MinorLocalize popup menu labels.
EditandDeleteare hardcoded and bypass i18n, unlike the rest of this screen.Suggested fix
- Text('Edit'), + Text('common.edit'.tr()), @@ - Text('Delete'), + Text('common.delete'.tr()),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/presentation/screens/server_list_screen.dart` around lines 323 - 340, The PopupMenuItem labels for the edit/delete actions are hardcoded; update the two PopupMenuItem children in server_list_screen.dart (the items with value 'edit' and value 'delete') to use the app's localization strings instead of Text('Edit') and Text('Delete') — e.g., obtain the localized values via your localization helper (such as AppLocalizations.of(context).edit and .delete or the project's equivalent) and replace those Text nodes so the menu respects i18n..github/workflows/deploy-backend.yml-37-37 (1)
37-37:⚠️ Potential issue | 🟡 MinorQuote the git clone URL to address shellcheck SC2086.
Unquoted variable expansion in
https://${PAT}@github.com/theSoberSobber/envs.gitcan cause word splitting if the PAT contains spaces or special characters.Suggested fix
- git clone https://${PAT}@github.com/theSoberSobber/envs.git envs-repo + git clone "https://${PAT}@github.com/theSoberSobber/envs.git" envs-repo🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/deploy-backend.yml at line 37, The shellcheck warning SC2086 is triggered by unquoted variable expansion in the git clone command; update the git clone invocation (the line containing "git clone" and the PAT variable) to quote the URL so the PAT is not subject to word splitting/special-character expansion—e.g., wrap the entire URL with double quotes (so the token becomes "https://${PAT}@github.com/theSoberSobber/envs.git") ensuring the git clone command receives a single argument.
🧹 Nitpick comments (10)
.gitignore (1)
68-69: Fix typo and avoid overlappingrefignore intent.On Line 68, “refrence” should be “reference”. Also, Line 69 (
**/ref) appears to overlap with existingref*(Line 48), which can make ignore behavior harder to reason about. Consider keeping one clear rule with a precise comment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore around lines 68 - 69, Fix the typo in the comment by changing "refrence" to "reference" and consolidate the overlapping ignore rules: remove the ambiguous "ref*" entry and replace the "**/ref" pattern with a single precise rule that matches the intended target (for example use "**/ref/" to ignore any directory named "ref" at any depth) so the .gitignore contains one clear rule and an accurate "reference" comment.dm-notifier/healthcheck.sh (1)
4-7: Harden liveness check against PID reuse.
Line 6 only checks that some process with that PID exists; this can report healthy for the wrong process.💡 Proposed fix
if [ -f /tmp/notifier.pid ]; then PID=$(cat /tmp/notifier.pid) - if kill -0 "$PID" 2>/dev/null; then + if kill -0 "$PID" 2>/dev/null \ + && [ -r "/proc/$PID/cmdline" ] \ + && tr '\0' ' ' < "/proc/$PID/cmdline" | grep -q "notifier.sh"; then exit 0 # Process is alive fi fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dm-notifier/healthcheck.sh` around lines 4 - 7, The liveness check only uses kill -0 on the PID from /tmp/notifier.pid which can match a different process due to PID reuse; after reading PID from /tmp/notifier.pid (the PID variable) and verifying kill -0 "$PID" succeeds, also validate the process identity by checking /proc/$PID/cmdline (or /proc/$PID/exe) contains the expected notifier binary/name (e.g., "notifier" or your daemon name) before returning healthy; if the identity check fails, treat the PID file as stale (remove it) and report unhealthy.dm-backend/package.json (1)
14-14: Remove deprecatedcryptonpm package and use Node's built-in module instead.The npm "crypto" ^1.0.1 package is explicitly deprecated per its registry listing: "This package is no longer supported. It's now a built-in Node module." The backend code only uses
crypto.randomBytes(), which is fully available in Node.js's built-in crypto. Removing this dependency eliminates unnecessary risk without any impact.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dm-backend/package.json` at line 14, Remove the deprecated "crypto" dependency from package.json and update code to use Node's built-in crypto module instead: delete the "crypto": "^1.0.1" entry from package.json and ensure any places that import the npm package (e.g., require('crypto') or import crypto from 'crypto') continue to work by relying on Node's built-in crypto; confirm usages such as crypto.randomBytes() remain unchanged and run tests/install to verify no further references to the npm package remain.lib/data/services/subscription_service.dart (3)
56-56: UnusedBuildContextparameter inshowPaywall.Similar to
showCustomerCenter, thecontextparameter is not used by the method body.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/data/services/subscription_service.dart` at line 56, The showPaywall method currently accepts an unused BuildContext parameter (similar to showCustomerCenter); remove the unused parameter from the method signature (and any callers) or, if context is intended to be used, update showPaywall to use the passed BuildContext appropriately; locate the function named showPaywall in subscription_service.dart and either delete the BuildContext context parameter from Future<bool> showPaywall(...) and adjust all call sites, or implement the required UI logic that uses context inside showPaywall so the parameter is actually used.
74-80: UnusedBuildContextparameter inshowCustomerCenter.The
contextparameter is declared but never used. If it's not needed byRevenueCatUI.presentCustomerCenter(), consider removing it to avoid confusion.♻️ Remove unused parameter
- Future<void> showCustomerCenter(BuildContext context) async { + Future<void> showCustomerCenter() async {Note: If callers are already passing context, this is a breaking change to the API. Alternatively, prefix with underscore:
BuildContext _context.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/data/services/subscription_service.dart` around lines 74 - 80, The showCustomerCenter method declares an unused BuildContext parameter; either remove the parameter from the signature and update all callers, or keep it and rename to _context to signal it's intentionally unused. Locate the method showCustomerCenter in SubscriptionService and apply the chosen change (remove BuildContext context or rename to BuildContext _context) and then update any call sites that pass a context if you remove the parameter.
31-32: Debug log level may be too verbose for production.
LogLevel.debugwill produce verbose RevenueCat logs in production. Consider usingLogLevel.warnorLogLevel.errorfor release builds, or make it conditional.♻️ Conditional log level
- await Purchases.setLogLevel(LogLevel.debug); + await Purchases.setLogLevel(kDebugMode ? LogLevel.debug : LogLevel.warn);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/data/services/subscription_service.dart` around lines 31 - 32, The code currently sets Purchases.setLogLevel(LogLevel.debug) unconditionally; change it to set a less-verbose level in release builds by checking the build mode (e.g., using Flutter's kReleaseMode or an env flag) and call Purchases.setLogLevel(LogLevel.warn or LogLevel.error) in release, while keeping LogLevel.debug for debug/development; update the try block that calls Purchases.setLogLevel and reference the Purchases and LogLevel symbols to implement the conditional selection so production logs are not overly verbose..github/deploy/remote_deploy.sh (1)
39-48: Old app directory is removed before verifying the new clone will succeed.If
git clonefails afterrm -rf "${APP_DIR}", the deployment is left in an unrecoverable state with no application directory. Consider cloning to a temporary directory first, then swapping.♻️ Safer clone pattern
-echo "==> Removing old app dir: ${APP_DIR}" -rm -rf "${APP_DIR}" - -echo "==> Cloning app repo (${APP_REPO_URL}) into ${APP_DIR}" -if [ -n "${BRANCH_NAME}" ]; then - echo "Cloning branch ${BRANCH_NAME}" - git clone --depth=1 --branch "${BRANCH_NAME}" "${APP_REPO_URL}" "${APP_DIR}" -else - git clone --depth=1 "${APP_REPO_URL}" "${APP_DIR}" -fi +APP_DIR_TMP="${APP_DIR}.tmp.$$" +echo "==> Cloning app repo (${APP_REPO_URL}) into ${APP_DIR_TMP}" +if [ -n "${BRANCH_NAME}" ]; then + echo "Cloning branch ${BRANCH_NAME}" + git clone --depth=1 --branch "${BRANCH_NAME}" "${APP_REPO_URL}" "${APP_DIR_TMP}" +else + git clone --depth=1 "${APP_REPO_URL}" "${APP_DIR_TMP}" +fi + +echo "==> Removing old app dir and replacing with new clone" +rm -rf "${APP_DIR}" +mv "${APP_DIR_TMP}" "${APP_DIR}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/deploy/remote_deploy.sh around lines 39 - 48, The script currently rm -rfs APP_DIR before cloning, which can leave the deployment broken if git clone fails; change the flow in remote_deploy.sh to clone into a temporary directory (use mktemp -d) using APP_REPO_URL and BRANCH_NAME (preserve the existing branch logic), verify the git clone exits successfully, and only then atomically replace APP_DIR (rm -rf old APP_DIR and mv TMP_DIR to APP_DIR or use a rename/swap) and ensure TMP_DIR is cleaned up on failure so the original APP_DIR is not lost if clone fails.lib/data/services/notification_service.dart (1)
129-136: HTTP request has no timeout — can hang indefinitely.The
http.postcall to register the token lacks a timeout. If the backend is unreachable, this could block for a long time.♻️ Add timeout
- final response = await http.post( + final response = await http.post( Uri.parse('${AppConfig.backendBaseUrl}/register-token'), headers: {'Content-Type': 'application/json'}, body: jsonEncode({ 'fcm_token': _fcmToken, 'rc_user_id': subscriptionService.appUserId, }), - ); + ).timeout(const Duration(seconds: 10));Don't forget to handle
TimeoutExceptionin the catch block.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/data/services/notification_service.dart` around lines 129 - 136, The POST call that registers the FCM token can hang because it has no timeout; modify the http.post in the method that contains _fcmToken and subscriptionService.appUserId (e.g., registerDeviceToken / registerToken) to use .timeout(Duration(seconds: X)) on the Future returned by http.post, and update the existing catch block to explicitly catch TimeoutException (and handle it like other failures or log it via processLogger / your logger). Ensure the timeout value is reasonable (e.g., 5–10s) and that the error handling branch distinguishes timeout errors from other exceptions.lib/data/services/pairing_service.dart (1)
46-54: HTTP request lacks timeout.The
http.postcall to generate a pairing token has no timeout. Consider adding a timeout to prevent indefinite hangs.♻️ Add timeout
final response = await http.post( Uri.parse('${AppConfig.backendBaseUrl}/pairing/generate'), headers: {'Content-Type': 'application/json'}, body: jsonEncode({ 'rc_user_id': subscriptionService.appUserId, 'server_id': serverId, }), - ); + ).timeout(const Duration(seconds: 15));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/data/services/pairing_service.dart` around lines 46 - 54, The HTTP POST in generatePairingToken (the call to http.post with Uri.parse('${AppConfig.backendBaseUrl}/pairing/generate') using subscriptionService.appUserId and serverId) has no timeout; add a .timeout(Duration(seconds: <reasonable_seconds>)) to the Future returned by http.post and handle TimeoutException in the surrounding try/catch so the method fails fast instead of hanging (ensure you import dart:async if needed and return or surface a clear error when a timeout occurs).lib/main.dart (1)
13-20: Consider error handling for service initialization.If
Firebase.initializeApp(),SubscriptionService().init(), orNotificationService().init()throws, the app will crash without user feedback. Consider wrapping these in try-catch to handle failures gracefully, especially for optional features like notifications.Proposed approach
// Initialize Firebase (required before FCM) - await Firebase.initializeApp(); - - // Initialize subscription service (RevenueCat) - await SubscriptionService().init(); - - // Initialize notification channels (does NOT request permission) - await NotificationService().init(); + try { + await Firebase.initializeApp(); + await SubscriptionService().init(); + await NotificationService().init(); + } catch (e) { + // Log error but continue - notifications are optional + debugPrint('Service initialization failed: $e'); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/main.dart` around lines 13 - 20, Wrap the three service initializations (Firebase.initializeApp(), SubscriptionService().init(), NotificationService().init()) inside try-catch blocks in main (or a small initServices helper) so failures don't crash the app; catch and log the error (use your existing logger or debugPrint) and apply a sensible fallback per service (e.g., continue without subscriptions or notifications, set a flag to disable related UI) and for Firebase decide whether to continue in degraded mode or show a user-facing error, ensuring each catch references which call failed for actionable logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/deploy/remote_deploy.sh:
- Around line 110-115: The current health check in remote_deploy.sh uses
num_running from a global `docker ps --filter "status=running"` which counts all
containers on the host; change it to only count containers belonging to this
deployment by filtering on the compose project or service labels (e.g., the
compose project name label or service-specific label) or by using `docker
compose ps`/`docker-compose ps` for this project, then reassign `num_running`
from that scoped command and keep the same error/exit logic; update the command
that sets `num_running` and any related echo/log lines to reference the scoped
check so only the current deployment's containers are counted.
In @.github/workflows/deploy-backend.yml:
- Around line 173-175: The CI deploy step currently disables SSH host-key
verification (uses -o StrictHostKeyChecking=no and UserKnownHostsFile=/dev/null)
which allows MITM; instead add the server's host key to a known_hosts file and
point SSH at that file or rely on the Actions known_hosts mechanism before
connecting: run ssh-keyscan "${SERVER_HOST}" >> "$HOME/.ssh/known_hosts" (or
create a temp file and set SSH to use it), ensure correct permissions (chmod
600), then remove the -o StrictHostKeyChecking and UserKnownHostsFile options
and use the existing ssh_cmd/sshpass invocation (or better, replace sshpass with
SSH_PRIVATE_KEY agent if available) so the connection verifies the server
identity when executing remote_deploy.sh.
- Around line 112-119: The workflow currently suppresses failures from the
Tailscale bootstrap by appending "|| true" to the "sudo tailscale up" and "sudo
tailscale status" commands; remove those "|| true" suppressions so that failures
from the tailscale commands (in the block that runs curl ... | sh and then calls
sudo tailscale up --authkey "${TAILSCALE_AUTHKEY}" and sudo tailscale status)
cause the job to fail immediately, surfacing bootstrap errors rather than
allowing the workflow to continue without a working VPN route.
In `@dm-backend/Dockerfile`:
- Around line 1-18: The Dockerfile currently runs the image as root; update it
to create and switch to a non-root user: add commands after WORKDIR /app to
create a dedicated group/user (e.g., app or use the existing node user), chown
the /app directory so files created by earlier steps (after RUN npm ci
--omit=dev and COPY server.js /COPY firebase-service-account.jso[n]) are owned
by that user, and set USER to that non-root account before
EXPOSE/HEALTHCHECK/CMD. Ensure ownership is changed after npm ci so node_modules
are writable by the non-root user and that the HEALTHCHECK and CMD run under the
non-root USER.
In `@dm-backend/server.js`:
- Around line 51-74: Both endpoints accept arbitrary clients; add authentication
and authorization checks to the POST /api/register-token handler and POST
/api/pairing/generate handler: require a valid auth token (extract via a
reusable middleware or helper like authenticateToken/verifyJwt) and ensure the
authenticated user's id matches the rc_user_id in register-token (or that the
user has permission to register for that rc_user_id) before adding to
userTokens, and when generating pairing tokens verify that the authenticated
user owns or is authorized to manage the provided server_id (e.g., call a server
ownership check like getServerOwner(server_id) or
serverStore.isOwner(req.user.id, server_id)); if checks fail return 401/403 and
log the denial.
- Line 154: The line building the body uses action.charAt(...) without
validating action which throws if action is missing; before using it, ensure
action is a non-empty string (e.g., compute a safeAct fallback like 'Unknown' or
'' when typeof action !== 'string' or action.length === 0) and then build body
from that safeAct (reference the const body and the action variable in
server.js); replace the direct usage of action.charAt(...) with the safeAct
variant so malformed events no longer cause a 500.
- Around line 130-139: Ensure events cannot spoof server_id by binding the
pairing token to its server_id: stop trusting server_id from req.body—use
pairingData.server_id (or compare and reject if body.server_id exists and
mismatches). In the event handler around registeredServers and last_seen
(symbols: registeredServers, pairingData, server_id, req.body), upsert a
registeredServers entry for the canonical server id when an event arrives
(create if missing) and set last_seen = Date.now() and pairing status to
paired/registered; if body.server_id is present but different, ignore it or
return an error to enforce the token↔server binding.
- Around line 221-241: The webhook route app.post('/api/webhook/revenuecat')
must verify the HMAC-SHA256 signature using REVENUECAT_WEBHOOK_SECRET before
parsing/acting on the payload: retrieve the raw request body (ensure this route
uses a raw/body-parser middleware or captures req.rawBody), read the RevenueCat
signature header (e.g., "RevenueCat-Signature"), compute HMAC-SHA256 over the
raw body with process.env.REVENUECAT_WEBHOOK_SECRET, compare using a timing-safe
equality check, and if the signature is missing or invalid respond with 401 and
do not process event/type/app_user_id; only continue to log/handle
EXPIRATION/CANCELLATION when the signature verification succeeds.
In `@dm-notifier/notifier.sh`:
- Around line 45-55: The current curl payload in notifier.sh sends a flat JSON
and risks invalid JSON via string interpolation and doesn't fail on HTTP
4xx/5xx; change payload shape to match backend expectations by nesting under
"event.event.type" (i.e. send {"server_id": SERVER_ID, "event": {"event":
{"type": EVENT_TYPE}, "action": EVENT_ACTION, "actor": {"name":
EVENT_ACTOR_NAME, "image": EVENT_ACTOR_IMAGE}, "timestamp": EVENT_TIME}}), build
the JSON with a safe serializer (e.g., use jq -n or printf to create JSON and
pipe to curl --data `@-` so you avoid manual quoting), and make the request fail
on HTTP errors by adding --fail (-f) to curl and returning a non-zero exit code
on failure so the script can handle unreachable/backend errors reliably; update
the curl invocation that currently references BACKEND_URL, PAIRING_TOKEN and the
interpolated fields to use this safe serialization and --fail.
In `@lib/config/app_config.dart`:
- Around line 8-10: Change the constant debugForcePro to false (replace static
const bool debugForcePro = true with false) so Pro features are not granted in
production; additionally, make this flag non-hardcoded for safety (e.g., use
bool.fromEnvironment or a build-time config) and add a startup check like
assert(!debugForcePro) or a CI/build validation that fails if
AppConfig.debugForcePro is true to prevent merging a release with it enabled.
- Around line 22-23: The hard-coded test RevenueCat API key in app_config.dart
(static const String revenueCatApiKey) must be replaced with the appropriate
production key(s): use the appl_ key for iOS/App Store builds and the goog_ key
for Android/Google Play; update revenueCatApiKey to reference the correct
production value (or switch to platform-specific configuration
loading/environment variables) so production builds do not use the test_ key.
Ensure the new key(s) are stored securely (not committed in plaintext) and that
any platform-specific selection logic uses the correct symbol (revenueCatApiKey)
at runtime.
In `@lib/data/services/pairing_service.dart`:
- Around line 122-131: The runCommand currently interpolates pairingToken
directly into the shell command (used by ssh.executeCommand), which risks shell
injection; update pairing token handling in pairing_service.dart by validating
the pairingToken with a strict whitelist (e.g., alphanumeric, hyphen,
underscore) before building runCommand and return a DeployResult failure on
invalid tokens, or alternatively ensure the token is safely escaped/quoted when
inserted (use single quotes and escape any embedded single quotes) so the
constructed runCommand and environment variable (-e PAIRING_TOKEN=...) cannot be
used for command injection; reference runCommand, pairingToken,
ssh.executeCommand, AppConfig.notifierImage and return a DeployResult on
validation failure.
In `@lib/presentation/screens/notification_setup_screen.dart`:
- Around line 72-143: The _deployNotifier flow must be wrapped in
try/catch/finally so any thrown exception sets _error (with a localized message
or exception.toString()), ensures _isDeploying is set to false and _deployStep
cleared in the finally block, and uses mounted checks when updating state; do
the same for the corresponding remove flow (e.g., _removeNotifier) to guarantee
_isRemoving is reset on error/exit. Locate _deployNotifier and _removeNotifier,
surround the async work (permission/token generation, deployNotifier call and
post-success persistence) with try { ... } catch (e, s) { if (mounted)
setState(() { _error = e.toString(); _isDeploying = false; _deployStep = ''; });
/* optionally log e/s */ } finally { if (mounted) setState(() { _isDeploying =
false; _deployStep = ''; }); } and analogously clear _isRemoving/_removeStep in
the remove flow.
- Around line 53-70: The _checkStatus method can leave _isChecking true if
checkContainerStatus throws; wrap the async call to
_pairingService.checkContainerStatus (and the await _getDockerPath) in a
try/catch/finally inside _checkStatus so that errors are caught and _isChecking
is always cleared in the finally block; inside catch set _error with the
thrown/parsed error and in finally call setState (guarded by mounted) to set
_isChecking = false (and update _status when successful), referencing the
_checkStatus method, _isChecking, _error, _status, and
_pairingService.checkContainerStatus to locate the change.
In `@lib/presentation/widgets/pro_card.dart`:
- Around line 33-56: _wrap the async flows in _handleSubscribe and
_handleRestore with try/finally so _isLoading is always cleared: set _isLoading
= true before calling _subscriptionService.showPaywall or restorePurchases,
await the call inside a try block, and in a finally block call setState (if
mounted) to set _isLoading = false; keep existing success handling inside the
try after the awaited call. Ensure you reference the existing methods
_handleSubscribe, _handleRestore, _subscriptionService.showPaywall,
restorePurchases, and the _isLoading field when making the changes.
---
Minor comments:
In @.github/workflows/deploy-backend.yml:
- Line 37: The shellcheck warning SC2086 is triggered by unquoted variable
expansion in the git clone command; update the git clone invocation (the line
containing "git clone" and the PAT variable) to quote the URL so the PAT is not
subject to word splitting/special-character expansion—e.g., wrap the entire URL
with double quotes (so the token becomes
"https://${PAT}@github.com/theSoberSobber/envs.git") ensuring the git clone
command receives a single argument.
In `@android/app/build.gradle.kts`:
- Around line 92-95: Update the coreLibraryDesugaring dependency version from
2.1.4 to 2.1.5: locate the dependencies block where
coreLibraryDesugaring("com.android.tools:desugar_jdk_libs:2.1.4") is declared
(the same block used alongside isCoreLibraryDesugaringEnabled = true) and change
the artifact version to "com.android.tools:desugar_jdk_libs:2.1.5" so the
project uses the latest stable desugar_jdk_libs.
In `@android/settings.gradle.kts`:
- Line 24: Update the Google Services Gradle plugin version: locate the plugin
declaration using the id "com.google.gms.google-services" (the line that
currently sets version "4.4.2" with apply false) and change its version to
"4.4.4" so the project-level plugin declaration reads the newer 4.4.4 release.
In `@dm-backend/.env.example`:
- Around line 1-5: The PORT in .env.example is set to 3000 but the container
image and compose map use 7847; update the example to set PORT=7847 (and adjust
the accompanying comment) so the environment example matches the
Dockerfile/docker-compose mapping; ensure the symbolic name PORT is consistent
across .env.example, Dockerfile EXPOSE and docker-compose service port mappings
(7847) to avoid runtime mismatches.
In `@dm-backend/README.md`:
- Around line 29-40: The README's Docker run example and PORT default are
inconsistent with the actual service port; update the Docker run command to map
host port 7847 to container port 7847 (replace "-p 3000:3000" with "-p
7847:7847") and change the configuration table default for `PORT` from `3000` to
`7847` so the README matches the Dockerfile/healthcheck that use port 7847.
In `@dm-notifier/.gitattributes`:
- Around line 1-2: The .gitattributes entries are using the repository-relative
prefix ("dm-notifier/notifier.sh" and "dm-notifier/healthcheck.sh") even though
the file sits inside that directory, so the patterns won't match; update the
lines to use paths relative to the file (e.g. "notifier.sh text eol=lf" and
"healthcheck.sh text eol=lf") so that the notifier.sh and healthcheck.sh entries
take effect.
In `@dm-notifier/Dockerfile`:
- Around line 1-29: Add a non-root user in the Dockerfile and run the notifier
as that user: create a dedicated group/user (e.g., "notifier"), chown
/app/notifier.sh and /app/healthcheck.sh to that user and set USER notifier
before ENTRYPOINT; ensure the HEALTHCHECK still runs under that user. Also add
guidance to handle access to the Docker socket by either making the docker group
available at runtime (recommend running container with --group-add docker or
passing the docker group GID via build ARG and adding the user to that group) or
documenting that socket permission adjustments are required so the non-root user
can read /var/run/docker.sock.
In `@lib/data/services/notification_service.dart`:
- Around line 99-100: Remove or redact logging of FCM tokens: in
NotificationService where _fcmToken is assigned from
FirebaseMessaging.instance.getToken() and currently printed via
debugPrint('NotificationService: FCM token: $_fcmToken'), stop printing the raw
token; either remove the debugPrint call entirely or replace it with a
non-sensitive message (e.g., log that a token was retrieved without including
_fcmToken, or log a hashed/masked version). Update any related logging in the
class to never emit the full _fcmToken.
- Around line 102-103: The token refresh listener is being registered each time
requestPermissionAndToken() runs causing duplicate handlers; modify
notification_service.dart to store the StreamSubscription returned by
FirebaseMessaging.instance.onTokenRefresh.listen(...) (e.g., a private field
like _tokenRefreshSub) and before calling listen again cancel and null-check the
existing subscription (or check if already active) so _onTokenRefresh is only
subscribed once; ensure you also cancel the subscription in the service's
dispose/close method to avoid leaks.
In `@lib/presentation/screens/notification_setup_screen.dart`:
- Line 76: The hardcoded progress messages assigned to _deployStep (e.g.,
"Requesting notification permission..." and the other two occurrences) must be
replaced with localized strings from your app's localization class; locate the
assignments in _NotificationSetupScreenState (where _deployStep is set around
the three noted spots) and call the appropriate localization getters (e.g.,
AppLocalizations.of(context).requestingNotificationPermission) instead of
literal English text, ensuring you have access to BuildContext (use
mounted/context from the State) and import the localization file; update all
three assignments to use the corresponding localized keys.
- Around line 4-6: Remove the unused imports SubscriptionService and
SshConnectionService from
lib/presentation/screens/notification_setup_screen.dart and delete the unused
state member _logs (and any code that only references it) from the
NotificationSetupScreen's State (e.g., NotificationSetupScreenState or
_NotificationSetupScreenState); keep the NotificationService import if it is
used, run the analyzer to ensure no other unused symbols remain, and update any
constructors or methods that referenced _logs to avoid dangling references.
In `@lib/presentation/screens/server_list_screen.dart`:
- Around line 288-297: Make the onTap handler async and await the Navigator.push
call to NotificationSetupScreen, then trigger the server-list refresh when it
returns; i.e., change the onTap closure to async, await Navigator.push(context,
MaterialPageRoute(builder: (_) => NotificationSetupScreen(server: server))), and
after the await call your screen's server reload method (e.g., _loadServers(),
_fetchServers(), or call setState to re-query/update the servers) so any
notification changes are reflected when control returns.
- Around line 323-340: The PopupMenuItem labels for the edit/delete actions are
hardcoded; update the two PopupMenuItem children in server_list_screen.dart (the
items with value 'edit' and value 'delete') to use the app's localization
strings instead of Text('Edit') and Text('Delete') — e.g., obtain the localized
values via your localization helper (such as AppLocalizations.of(context).edit
and .delete or the project's equivalent) and replace those Text nodes so the
menu respects i18n.
---
Nitpick comments:
In @.github/deploy/remote_deploy.sh:
- Around line 39-48: The script currently rm -rfs APP_DIR before cloning, which
can leave the deployment broken if git clone fails; change the flow in
remote_deploy.sh to clone into a temporary directory (use mktemp -d) using
APP_REPO_URL and BRANCH_NAME (preserve the existing branch logic), verify the
git clone exits successfully, and only then atomically replace APP_DIR (rm -rf
old APP_DIR and mv TMP_DIR to APP_DIR or use a rename/swap) and ensure TMP_DIR
is cleaned up on failure so the original APP_DIR is not lost if clone fails.
In @.gitignore:
- Around line 68-69: Fix the typo in the comment by changing "refrence" to
"reference" and consolidate the overlapping ignore rules: remove the ambiguous
"ref*" entry and replace the "**/ref" pattern with a single precise rule that
matches the intended target (for example use "**/ref/" to ignore any directory
named "ref" at any depth) so the .gitignore contains one clear rule and an
accurate "reference" comment.
In `@dm-backend/package.json`:
- Line 14: Remove the deprecated "crypto" dependency from package.json and
update code to use Node's built-in crypto module instead: delete the "crypto":
"^1.0.1" entry from package.json and ensure any places that import the npm
package (e.g., require('crypto') or import crypto from 'crypto') continue to
work by relying on Node's built-in crypto; confirm usages such as
crypto.randomBytes() remain unchanged and run tests/install to verify no further
references to the npm package remain.
In `@dm-notifier/healthcheck.sh`:
- Around line 4-7: The liveness check only uses kill -0 on the PID from
/tmp/notifier.pid which can match a different process due to PID reuse; after
reading PID from /tmp/notifier.pid (the PID variable) and verifying kill -0
"$PID" succeeds, also validate the process identity by checking
/proc/$PID/cmdline (or /proc/$PID/exe) contains the expected notifier
binary/name (e.g., "notifier" or your daemon name) before returning healthy; if
the identity check fails, treat the PID file as stale (remove it) and report
unhealthy.
In `@lib/data/services/notification_service.dart`:
- Around line 129-136: The POST call that registers the FCM token can hang
because it has no timeout; modify the http.post in the method that contains
_fcmToken and subscriptionService.appUserId (e.g., registerDeviceToken /
registerToken) to use .timeout(Duration(seconds: X)) on the Future returned by
http.post, and update the existing catch block to explicitly catch
TimeoutException (and handle it like other failures or log it via processLogger
/ your logger). Ensure the timeout value is reasonable (e.g., 5–10s) and that
the error handling branch distinguishes timeout errors from other exceptions.
In `@lib/data/services/pairing_service.dart`:
- Around line 46-54: The HTTP POST in generatePairingToken (the call to
http.post with Uri.parse('${AppConfig.backendBaseUrl}/pairing/generate') using
subscriptionService.appUserId and serverId) has no timeout; add a
.timeout(Duration(seconds: <reasonable_seconds>)) to the Future returned by
http.post and handle TimeoutException in the surrounding try/catch so the method
fails fast instead of hanging (ensure you import dart:async if needed and return
or surface a clear error when a timeout occurs).
In `@lib/data/services/subscription_service.dart`:
- Line 56: The showPaywall method currently accepts an unused BuildContext
parameter (similar to showCustomerCenter); remove the unused parameter from the
method signature (and any callers) or, if context is intended to be used, update
showPaywall to use the passed BuildContext appropriately; locate the function
named showPaywall in subscription_service.dart and either delete the
BuildContext context parameter from Future<bool> showPaywall(...) and adjust all
call sites, or implement the required UI logic that uses context inside
showPaywall so the parameter is actually used.
- Around line 74-80: The showCustomerCenter method declares an unused
BuildContext parameter; either remove the parameter from the signature and
update all callers, or keep it and rename to _context to signal it's
intentionally unused. Locate the method showCustomerCenter in
SubscriptionService and apply the chosen change (remove BuildContext context or
rename to BuildContext _context) and then update any call sites that pass a
context if you remove the parameter.
- Around line 31-32: The code currently sets
Purchases.setLogLevel(LogLevel.debug) unconditionally; change it to set a
less-verbose level in release builds by checking the build mode (e.g., using
Flutter's kReleaseMode or an env flag) and call
Purchases.setLogLevel(LogLevel.warn or LogLevel.error) in release, while keeping
LogLevel.debug for debug/development; update the try block that calls
Purchases.setLogLevel and reference the Purchases and LogLevel symbols to
implement the conditional selection so production logs are not overly verbose.
In `@lib/main.dart`:
- Around line 13-20: Wrap the three service initializations
(Firebase.initializeApp(), SubscriptionService().init(),
NotificationService().init()) inside try-catch blocks in main (or a small
initServices helper) so failures don't crash the app; catch and log the error
(use your existing logger or debugPrint) and apply a sensible fallback per
service (e.g., continue without subscriptions or notifications, set a flag to
disable related UI) and for Firebase decide whether to continue in degraded mode
or show a user-facing error, ensuring each catch references which call failed
for actionable logs.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
dm-backend/package-lock.jsonis excluded by!**/package-lock.jsonpubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (36)
.github/CD-backend.yml.github/deploy/remote_deploy.sh.github/workflows/build-notifier.yml.github/workflows/deploy-backend.yml.gitignoreandroid/app/build.gradle.ktsandroid/app/src/main/kotlin/com/pavit/docker/MainActivity.ktandroid/settings.gradle.ktsassets/i18n/en-US.jsonassets/i18n/es.jsonassets/i18n/fr-FR.jsondm-backend/.env.exampledm-backend/Dockerfiledm-backend/README.mddm-backend/docker-compose.ymldm-backend/package.jsondm-backend/server.jsdm-notifier/.gitattributesdm-notifier/Dockerfiledm-notifier/README.mddm-notifier/healthcheck.shdm-notifier/notifier.shlib/config/app_config.dartlib/data/services/notification_service.dartlib/data/services/pairing_service.dartlib/data/services/subscription_service.dartlib/domain/models/server.dartlib/main.dartlib/presentation/screens/notification_setup_screen.dartlib/presentation/screens/server_list_screen.dartlib/presentation/screens/settings_screen.dartlib/presentation/widgets/pro_card.dartmacos/Flutter/GeneratedPluginRegistrant.swiftpubspec.yamlwindows/flutter/generated_plugin_registrant.ccwindows/flutter/generated_plugins.cmake
Comment on lines
+110
to
+115
| num_running=$(sudo docker ps --filter "status=running" --format '{{.Names}}' | wc -l | tr -d ' ') | ||
| if [ "${num_running}" -lt 1 ]; then | ||
| echo "ERROR: No running containers detected after compose up." >&2 | ||
| sudo docker ps -a || true | ||
| exit 2 | ||
| fi |
There was a problem hiding this comment.
Container health check counts all running containers, not just the deployed ones.
The validation checks docker ps --filter "status=running" which counts all running containers on the host, not specifically the containers from this deployment. If another container is running, the check passes even if the current deployment failed.
🐛 Proposed fix to scope container check to the deployment
-num_running=$(sudo docker ps --filter "status=running" --format '{{.Names}}' | wc -l | tr -d ' ')
+if sudo docker compose version >/dev/null 2>&1; then
+ num_running=$(sudo docker compose -f "${COMPOSE_FILE}" ps --status running -q 2>/dev/null | wc -l | tr -d ' ')
+else
+ num_running=$(sudo docker-compose -f "${COMPOSE_FILE}" ps -q --filter "status=running" 2>/dev/null | wc -l | tr -d ' ')
+fi
if [ "${num_running}" -lt 1 ]; then
echo "ERROR: No running containers detected after compose up." >&2
- sudo docker ps -a || true
+ if sudo docker compose version >/dev/null 2>&1; then
+ sudo docker compose -f "${COMPOSE_FILE}" ps -a || true
+ sudo docker compose -f "${COMPOSE_FILE}" logs --tail 50 || true
+ else
+ sudo docker-compose -f "${COMPOSE_FILE}" ps || true
+ sudo docker-compose -f "${COMPOSE_FILE}" logs --tail 50 || true
+ fi
exit 2
fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/deploy/remote_deploy.sh around lines 110 - 115, The current health
check in remote_deploy.sh uses num_running from a global `docker ps --filter
"status=running"` which counts all containers on the host; change it to only
count containers belonging to this deployment by filtering on the compose
project or service labels (e.g., the compose project name label or
service-specific label) or by using `docker compose ps`/`docker-compose ps` for
this project, then reassign `num_running` from that scoped command and keep the
same error/exit logic; update the command that sets `num_running` and any
related echo/log lines to reference the scoped check so only the current
deployment's containers are counted.
Comment on lines
+112
to
+119
| if [ -z "${TAILSCALE_AUTHKEY:-}" ]; then | ||
| echo "ERROR: TAILSCALE_AUTHKEY not found in environment variables." >&2 | ||
| exit 1 | ||
| fi | ||
| curl -fsSL https://tailscale.com/install.sh | sh | ||
| sudo tailscale up --authkey "${TAILSCALE_AUTHKEY}" || true | ||
| sleep 3 | ||
| sudo tailscale status || true |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/deploy-backend.yml | head -150Repository: theSoberSobber/Docker-Manager
Length of output: 6528
🏁 Script executed:
cat -n .github/workflows/deploy-backend.yml | sed -n '150,200p'Repository: theSoberSobber/Docker-Manager
Length of output: 1641
🏁 Script executed:
if [ -f .github/deploy/remote_deploy.sh ]; then
wc -l .github/deploy/remote_deploy.sh
fiRepository: theSoberSobber/Docker-Manager
Length of output: 108
🏁 Script executed:
cat -n .github/deploy/remote_deploy.shRepository: theSoberSobber/Docker-Manager
Length of output: 4748
Fail fast when Tailscale bootstrap fails.
The target server is at a Tailscale-reserved IP (100.112.185.9 in range 100.64.0.0/10). Suppressing tailscale up and tailscale status errors masks setup failures and allows the workflow to proceed without a valid VPN route, leading to confusing SSH connection failures downstream. Remove error suppression to surface Tailscale issues immediately.
Suggested fix
- sudo tailscale up --authkey "${TAILSCALE_AUTHKEY}" || true
+ sudo tailscale up --authkey "${TAILSCALE_AUTHKEY}"
sleep 3
- sudo tailscale status || true
+ sudo tailscale status📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if [ -z "${TAILSCALE_AUTHKEY:-}" ]; then | |
| echo "ERROR: TAILSCALE_AUTHKEY not found in environment variables." >&2 | |
| exit 1 | |
| fi | |
| curl -fsSL https://tailscale.com/install.sh | sh | |
| sudo tailscale up --authkey "${TAILSCALE_AUTHKEY}" || true | |
| sleep 3 | |
| sudo tailscale status || true | |
| if [ -z "${TAILSCALE_AUTHKEY:-}" ]; then | |
| echo "ERROR: TAILSCALE_AUTHKEY not found in environment variables." >&2 | |
| exit 1 | |
| fi | |
| curl -fsSL https://tailscale.com/install.sh | sh | |
| sudo tailscale up --authkey "${TAILSCALE_AUTHKEY}" | |
| sleep 3 | |
| sudo tailscale status |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/deploy-backend.yml around lines 112 - 119, The workflow
currently suppresses failures from the Tailscale bootstrap by appending "||
true" to the "sudo tailscale up" and "sudo tailscale status" commands; remove
those "|| true" suppressions so that failures from the tailscale commands (in
the block that runs curl ... | sh and then calls sudo tailscale up --authkey
"${TAILSCALE_AUTHKEY}" and sudo tailscale status) cause the job to fail
immediately, surfacing bootstrap errors rather than allowing the workflow to
continue without a working VPN route.
Comment on lines
+173
to
+175
| echo "==> Streaming .github/deploy/remote_deploy.sh to ${SERVER_USER}@${SERVER_HOST} and executing" | ||
| ssh_cmd="${SSH_ENV} bash -s" | ||
| sshpass -p "${SSH_PASSWORD}" ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null "${SERVER_USER}@${SERVER_HOST}" "${ssh_cmd}" < .github/deploy/remote_deploy.sh |
There was a problem hiding this comment.
Avoid disabling SSH host-key verification in CI.
StrictHostKeyChecking=no + /dev/null known_hosts permits MITM on a credentialed deployment channel.
Suggested fix
+ mkdir -p ~/.ssh
+ ssh-keyscan -H "${SERVER_HOST}" >> ~/.ssh/known_hosts
echo "==> Streaming .github/deploy/remote_deploy.sh to ${SERVER_USER}@${SERVER_HOST} and executing"
ssh_cmd="${SSH_ENV} bash -s"
- sshpass -p "${SSH_PASSWORD}" ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null "${SERVER_USER}@${SERVER_HOST}" "${ssh_cmd}" < .github/deploy/remote_deploy.sh
+ sshpass -p "${SSH_PASSWORD}" ssh "${SERVER_USER}@${SERVER_HOST}" "${ssh_cmd}" < .github/deploy/remote_deploy.sh📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| echo "==> Streaming .github/deploy/remote_deploy.sh to ${SERVER_USER}@${SERVER_HOST} and executing" | |
| ssh_cmd="${SSH_ENV} bash -s" | |
| sshpass -p "${SSH_PASSWORD}" ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null "${SERVER_USER}@${SERVER_HOST}" "${ssh_cmd}" < .github/deploy/remote_deploy.sh | |
| mkdir -p ~/.ssh | |
| ssh-keyscan -H "${SERVER_HOST}" >> ~/.ssh/known_hosts | |
| echo "==> Streaming .github/deploy/remote_deploy.sh to ${SERVER_USER}@${SERVER_HOST} and executing" | |
| ssh_cmd="${SSH_ENV} bash -s" | |
| sshpass -p "${SSH_PASSWORD}" ssh "${SERVER_USER}@${SERVER_HOST}" "${ssh_cmd}" < .github/deploy/remote_deploy.sh |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/deploy-backend.yml around lines 173 - 175, The CI deploy
step currently disables SSH host-key verification (uses -o
StrictHostKeyChecking=no and UserKnownHostsFile=/dev/null) which allows MITM;
instead add the server's host key to a known_hosts file and point SSH at that
file or rely on the Actions known_hosts mechanism before connecting: run
ssh-keyscan "${SERVER_HOST}" >> "$HOME/.ssh/known_hosts" (or create a temp file
and set SSH to use it), ensure correct permissions (chmod 600), then remove the
-o StrictHostKeyChecking and UserKnownHostsFile options and use the existing
ssh_cmd/sshpass invocation (or better, replace sshpass with SSH_PRIVATE_KEY
agent if available) so the connection verifies the server identity when
executing remote_deploy.sh.
Comment on lines
+1
to
+18
| FROM node:20-alpine | ||
|
|
||
| WORKDIR /app | ||
|
|
||
| COPY package*.json ./ | ||
| RUN npm ci --omit=dev | ||
|
|
||
| COPY server.js ./ | ||
|
|
||
| # Copy Firebase service account if present (glob trick avoids COPY failure when missing) | ||
| COPY firebase-service-account.jso[n] ./ | ||
|
|
||
| EXPOSE 7847 | ||
|
|
||
| HEALTHCHECK --interval=30s --timeout=5s --retries=3 \ | ||
| CMD wget --no-verbose --tries=1 --spider http://localhost:7847/health || exit 1 | ||
|
|
||
| CMD ["node", "server.js"] |
There was a problem hiding this comment.
Add a non-root user for security hardening.
The container runs as root by default, which Trivy flagged as a security concern (DS-0002). Running Node.js applications as a non-root user limits the blast radius if the application is compromised.
Proposed fix
FROM node:20-alpine
WORKDIR /app
COPY package*.json ./
RUN npm ci --omit=dev
COPY server.js ./
# Copy Firebase service account if present (glob trick avoids COPY failure when missing)
COPY firebase-service-account.jso[n] ./
+# Run as non-root user for security
+RUN addgroup -g 1001 -S nodejs && \
+ adduser -S nodejs -u 1001 -G nodejs && \
+ chown -R nodejs:nodejs /app
+USER nodejs
+
EXPOSE 7847
HEALTHCHECK --interval=30s --timeout=5s --retries=3 \
CMD wget --no-verbose --tries=1 --spider http://localhost:7847/health || exit 1
CMD ["node", "server.js"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| FROM node:20-alpine | |
| WORKDIR /app | |
| COPY package*.json ./ | |
| RUN npm ci --omit=dev | |
| COPY server.js ./ | |
| # Copy Firebase service account if present (glob trick avoids COPY failure when missing) | |
| COPY firebase-service-account.jso[n] ./ | |
| EXPOSE 7847 | |
| HEALTHCHECK --interval=30s --timeout=5s --retries=3 \ | |
| CMD wget --no-verbose --tries=1 --spider http://localhost:7847/health || exit 1 | |
| CMD ["node", "server.js"] | |
| FROM node:20-alpine | |
| WORKDIR /app | |
| COPY package*.json ./ | |
| RUN npm ci --omit=dev | |
| COPY server.js ./ | |
| # Copy Firebase service account if present (glob trick avoids COPY failure when missing) | |
| COPY firebase-service-account.jso[n] ./ | |
| # Run as non-root user for security | |
| RUN addgroup -g 1001 -S nodejs && \ | |
| adduser -S nodejs -u 1001 -G nodejs && \ | |
| chown -R nodejs:nodejs /app | |
| USER nodejs | |
| EXPOSE 7847 | |
| HEALTHCHECK --interval=30s --timeout=5s --retries=3 \ | |
| CMD wget --no-verbose --tries=1 --spider http://localhost:7847/health || exit 1 | |
| CMD ["node", "server.js"] |
🧰 Tools
🪛 Trivy (0.69.1)
[error] 1-1: Image user should not be 'root'
Specify at least 1 USER command in Dockerfile with non-root user as argument
Rule: DS-0002
(IaC/Dockerfile)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dm-backend/Dockerfile` around lines 1 - 18, The Dockerfile currently runs the
image as root; update it to create and switch to a non-root user: add commands
after WORKDIR /app to create a dedicated group/user (e.g., app or use the
existing node user), chown the /app directory so files created by earlier steps
(after RUN npm ci --omit=dev and COPY server.js /COPY
firebase-service-account.jso[n]) are owned by that user, and set USER to that
non-root account before EXPOSE/HEALTHCHECK/CMD. Ensure ownership is changed
after npm ci so node_modules are writable by the non-root user and that the
HEALTHCHECK and CMD run under the non-root USER.
Comment on lines
+122
to
+131
| final runCommand = '$docker run -d ' | ||
| '--name $container ' | ||
| '--restart unless-stopped ' | ||
| '-v /var/run/docker.sock:/var/run/docker.sock:ro ' | ||
| '-e PAIRING_TOKEN=$pairingToken ' | ||
| '-e BACKEND_URL=${AppConfig.backendBaseUrl} ' | ||
| '-e SERVER_ID=$serverId ' | ||
| '--memory=64m ' | ||
| '--cpus=0.1 ' | ||
| '${AppConfig.notifierImage}'; |
There was a problem hiding this comment.
Potential shell injection via pairingToken in command string.
The pairingToken is interpolated directly into a shell command passed to ssh.executeCommand(). If the backend is compromised or the token contains shell metacharacters (e.g., $, backticks, ;, quotes), this could lead to command injection on the user's server.
🛡️ Proposed fix: escape or validate the token
Option 1: Validate token format (if tokens are expected to be alphanumeric/hex):
// Add near line 78
if (!RegExp(r'^[a-zA-Z0-9_-]+$').hasMatch(pairingToken)) {
return DeployResult(
success: false,
error: 'Invalid pairing token format',
);
}Option 2: Use single quotes and escape embedded quotes:
final runCommand = '$docker run -d '
'--name $container '
'--restart unless-stopped '
'-v /var/run/docker.sock:/var/run/docker.sock:ro '
- '-e PAIRING_TOKEN=$pairingToken '
+ "-e PAIRING_TOKEN='${pairingToken.replaceAll("'", "'\\''")}' "
'-e BACKEND_URL=${AppConfig.backendBaseUrl} '🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/data/services/pairing_service.dart` around lines 122 - 131, The
runCommand currently interpolates pairingToken directly into the shell command
(used by ssh.executeCommand), which risks shell injection; update pairing token
handling in pairing_service.dart by validating the pairingToken with a strict
whitelist (e.g., alphanumeric, hyphen, underscore) before building runCommand
and return a DeployResult failure on invalid tokens, or alternatively ensure the
token is safely escaped/quoted when inserted (use single quotes and escape any
embedded single quotes) so the constructed runCommand and environment variable
(-e PAIRING_TOKEN=...) cannot be used for command injection; reference
runCommand, pairingToken, ssh.executeCommand, AppConfig.notifierImage and return
a DeployResult on validation failure.
Comment on lines
+53
to
+70
| Future<void> _checkStatus() async { | ||
| setState(() { | ||
| _isChecking = true; | ||
| _error = null; | ||
| }); | ||
|
|
||
| final dockerPath = await _getDockerPath(); | ||
| final status = await _pairingService.checkContainerStatus( | ||
| dockerCliPath: dockerPath, | ||
| ); | ||
|
|
||
| if (mounted) { | ||
| setState(() { | ||
| _status = status; | ||
| _isChecking = false; | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Always clear _isChecking even when status check fails.
If checkContainerStatus throws, the screen can get stuck in checking state.
Suggested fix
Future<void> _checkStatus() async {
- setState(() {
- _isChecking = true;
- _error = null;
- });
-
- final dockerPath = await _getDockerPath();
- final status = await _pairingService.checkContainerStatus(
- dockerCliPath: dockerPath,
- );
-
- if (mounted) {
- setState(() {
- _status = status;
- _isChecking = false;
- });
- }
+ if (mounted) {
+ setState(() {
+ _isChecking = true;
+ _error = null;
+ });
+ }
+ try {
+ final dockerPath = await _getDockerPath();
+ final status = await _pairingService.checkContainerStatus(
+ dockerCliPath: dockerPath,
+ );
+ if (mounted) setState(() => _status = status);
+ } catch (e) {
+ if (mounted) setState(() => _error = e.toString());
+ } finally {
+ if (mounted) setState(() => _isChecking = false);
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/presentation/screens/notification_setup_screen.dart` around lines 53 -
70, The _checkStatus method can leave _isChecking true if checkContainerStatus
throws; wrap the async call to _pairingService.checkContainerStatus (and the
await _getDockerPath) in a try/catch/finally inside _checkStatus so that errors
are caught and _isChecking is always cleared in the finally block; inside catch
set _error with the thrown/parsed error and in finally call setState (guarded by
mounted) to set _isChecking = false (and update _status when successful),
referencing the _checkStatus method, _isChecking, _error, _status, and
_pairingService.checkContainerStatus to locate the change.
Comment on lines
+72
to
+143
| Future<void> _deployNotifier() async { | ||
| setState(() { | ||
| _isDeploying = true; | ||
| _error = null; | ||
| _deployStep = 'Requesting notification permission...'; | ||
| }); | ||
|
|
||
| // Request notification permission (only asked here, when user enables the feature) | ||
| await _notificationService.requestPermissionAndToken(); | ||
|
|
||
| if (mounted) setState(() => _deployStep = 'Registering device...'); | ||
| await _notificationService.registerTokenWithBackend(); | ||
|
|
||
| if (mounted) setState(() => _deployStep = 'Generating security token...'); | ||
| final token = await _pairingService.generatePairingToken(widget.server.id); | ||
| if (token == null) { | ||
| if (mounted) { | ||
| setState(() { | ||
| _error = 'notifications.token_generation_failed'.tr(); | ||
| _isDeploying = false; | ||
| _deployStep = ''; | ||
| }); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| // Deploy via SSH with step-by-step progress | ||
| final dockerPath = await _getDockerPath(); | ||
| final result = await _pairingService.deployNotifier( | ||
| serverId: widget.server.id, | ||
| pairingToken: token, | ||
| dockerCliPath: dockerPath, | ||
| onProgress: (step) { | ||
| if (mounted) setState(() => _deployStep = step); | ||
| }, | ||
| ); | ||
|
|
||
| if (mounted) { | ||
| setState(() { | ||
| _isDeploying = false; | ||
| _deployStep = ''; | ||
| if (result.success) { | ||
| _status = PairingStatus.running; | ||
| } else { | ||
| _error = result.error; | ||
| _status = PairingStatus.error; | ||
| } | ||
| }); | ||
|
|
||
| if (result.success) { | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| SnackBar( | ||
| content: Row( | ||
| children: [ | ||
| const Icon(Icons.check_circle, color: Colors.white, size: 20), | ||
| const SizedBox(width: 8), | ||
| Expanded(child: Text( | ||
| result.alreadyRunning | ||
| ? 'notifications.already_active'.tr() | ||
| : 'notifications.deploy_success'.tr(), | ||
| )), | ||
| ], | ||
| ), | ||
| backgroundColor: Colors.green, | ||
| ), | ||
| ); | ||
| // Persist enabled flag | ||
| final updated = widget.server.copyWith(notificationsEnabled: true); | ||
| await _serverRepo.updateServer(updated); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Wrap deploy/remove flows with try/catch/finally to avoid stuck loaders.
Unhandled exceptions in these paths can leave _isDeploying/_isRemoving true and block the UI.
Also applies to: 145-188
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/presentation/screens/notification_setup_screen.dart` around lines 72 -
143, The _deployNotifier flow must be wrapped in try/catch/finally so any thrown
exception sets _error (with a localized message or exception.toString()),
ensures _isDeploying is set to false and _deployStep cleared in the finally
block, and uses mounted checks when updating state; do the same for the
corresponding remove flow (e.g., _removeNotifier) to guarantee _isRemoving is
reset on error/exit. Locate _deployNotifier and _removeNotifier, surround the
async work (permission/token generation, deployNotifier call and post-success
persistence) with try { ... } catch (e, s) { if (mounted) setState(() { _error =
e.toString(); _isDeploying = false; _deployStep = ''; }); /* optionally log e/s
*/ } finally { if (mounted) setState(() { _isDeploying = false; _deployStep =
''; }); } and analogously clear _isRemoving/_removeStep in the remove flow.
Comment on lines
+33
to
+56
| Future<void> _handleSubscribe() async { | ||
| setState(() => _isLoading = true); | ||
|
|
||
| final success = await _subscriptionService.showPaywall(context); | ||
|
|
||
| if (mounted) { | ||
| setState(() => _isLoading = false); | ||
|
|
||
| if (success) { | ||
| ScaffoldMessenger.of(context).showSnackBar( | ||
| SnackBar( | ||
| content: Row( | ||
| children: [ | ||
| const Icon(Icons.check_circle, color: Colors.white, size: 20), | ||
| const SizedBox(width: 12), | ||
| Text('pro.purchase_success'.tr()), | ||
| ], | ||
| ), | ||
| backgroundColor: Colors.green, | ||
| ), | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Protect loading state with try/finally in subscribe/restore flows.
If showPaywall or restorePurchases throws, _isLoading can stay true and disable actions indefinitely.
Suggested fix
Future<void> _handleSubscribe() async {
- setState(() => _isLoading = true);
-
- final success = await _subscriptionService.showPaywall(context);
-
- if (mounted) {
- setState(() => _isLoading = false);
+ if (mounted) setState(() => _isLoading = true);
+ try {
+ final success = await _subscriptionService.showPaywall(context);
+ if (!mounted) return;
if (success) {
ScaffoldMessenger.of(context).showSnackBar(
@@
);
}
+ } catch (e) {
+ if (!mounted) return;
+ ScaffoldMessenger.of(context).showSnackBar(
+ SnackBar(content: Text('pro.purchase_failed'.tr(args: [e.toString()]))),
+ );
+ } finally {
+ if (mounted) setState(() => _isLoading = false);
}
}Apply the same pattern to _handleRestore().
Also applies to: 58-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/presentation/widgets/pro_card.dart` around lines 33 - 56, _wrap the async
flows in _handleSubscribe and _handleRestore with try/finally so _isLoading is
always cleared: set _isLoading = true before calling
_subscriptionService.showPaywall or restorePurchases, await the call inside a
try block, and in a finally block call setState (if mounted) to set _isLoading =
false; keep existing success handling inside the try after the awaited call.
Ensure you reference the existing methods _handleSubscribe, _handleRestore,
_subscriptionService.showPaywall, restorePurchases, and the _isLoading field
when making the changes.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/presentation/widgets/pro_card.dart (1)
34-54:⚠️ Potential issue | 🟠 MajorProtect both async purchase flows with
try/finallyso loading state is always released.If either async call throws,
_isLoadingcan remaintrueand keep actions disabled. Move loading reset tofinallyin both handlers.Suggested patch
Future<void> _handleSubscribe() async { - setState(() => _isLoading = true); - final success = await _subscriptionService.showPaywall(context); - if (mounted) { - setState(() => _isLoading = false); + if (mounted) setState(() => _isLoading = true); + try { + final success = await _subscriptionService.showPaywall(context); + if (!mounted) return; if (success) { ScaffoldMessenger.of(context).showSnackBar( SnackBar( @@ ), ); } + } finally { + if (mounted) setState(() => _isLoading = false); } } Future<void> _handleRestore() async { - setState(() => _isLoading = true); - final success = await _subscriptionService.restorePurchases(); - if (mounted) { - setState(() => _isLoading = false); + if (mounted) setState(() => _isLoading = true); + try { + final success = await _subscriptionService.restorePurchases(); + if (!mounted) return; ScaffoldMessenger.of(context).showSnackBar( SnackBar( @@ backgroundColor: success ? Colors.green : Colors.orange, ), ); + } finally { + if (mounted) setState(() => _isLoading = false); } }Also applies to: 56-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/presentation/widgets/pro_card.dart` around lines 34 - 54, The _isLoading flag isn't guaranteed to be cleared if an exception is thrown; wrap the asynchronous flow in _handleSubscribe (and the other purchase handler, e.g. _handleRestore) with try/finally: setState(() => _isLoading = true) before the try, run the async call(s) in the try block, and in finally call if (mounted) setState(() => _isLoading = false); keep the success SnackBar logic inside the try after the await so the finally always releases loading state even on errors.
🧹 Nitpick comments (2)
lib/presentation/widgets/pro_card.dart (1)
199-204: Use at least a 48x48 touch target for the GitHub action.The current
BoxConstraints(minWidth: 36, minHeight: 36)makes the button harder to tap on mobile and hurts accessibility.Suggested patch
IconButton( icon: const FaIcon(FontAwesomeIcons.github, size: 20), tooltip: 'settings.github_repo'.tr(), onPressed: widget.onOpenGitHub, padding: EdgeInsets.zero, - constraints: const BoxConstraints(minWidth: 36, minHeight: 36), + constraints: const BoxConstraints(minWidth: 48, minHeight: 48), ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/presentation/widgets/pro_card.dart` around lines 199 - 204, The GitHub IconButton in ProCard uses BoxConstraints(minWidth: 36, minHeight: 36) which is below the recommended 48x48 touch target; update the IconButton's constraints (the instance with icon: FaIcon(...), tooltip: 'settings.github_repo'.tr(), onPressed: widget.onOpenGitHub) to use at least BoxConstraints(minWidth: 48, minHeight: 48) or otherwise ensure a 48x48 hit area (e.g., wrap with SizedBox or adjust padding) so the touch target meets accessibility guidelines.lib/presentation/screens/notification_setup_screen.dart (1)
24-26: Consider dependency injection for testability.Direct instantiation of services makes unit testing this screen difficult. You could pass these as constructor parameters with defaults, or use a service locator pattern.
♻️ Optional: Constructor injection pattern
class NotificationSetupScreen extends StatefulWidget { final Server server; + final PairingService? pairingService; + final NotificationService? notificationService; + final ServerRepositoryImpl? serverRepo; - const NotificationSetupScreen({super.key, required this.server}); + const NotificationSetupScreen({ + super.key, + required this.server, + this.pairingService, + this.notificationService, + this.serverRepo, + }); `@override` State<NotificationSetupScreen> createState() => _NotificationSetupScreenState(); } class _NotificationSetupScreenState extends State<NotificationSetupScreen> { - final PairingService _pairingService = PairingService(); - final NotificationService _notificationService = NotificationService(); - final ServerRepositoryImpl _serverRepo = ServerRepositoryImpl(); + late final PairingService _pairingService; + late final NotificationService _notificationService; + late final ServerRepositoryImpl _serverRepo; + `@override` + void initState() { + super.initState(); + _pairingService = widget.pairingService ?? PairingService(); + _notificationService = widget.notificationService ?? NotificationService(); + _serverRepo = widget.serverRepo ?? ServerRepositoryImpl(); + _checkStatus(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/presentation/screens/notification_setup_screen.dart` around lines 24 - 26, Replace the direct instantiation of PairingService, NotificationService, and ServerRepositoryImpl inside NotificationSetupScreen with constructor injection: add optional constructor parameters (e.g., PairingService pairingService, NotificationService notificationService, ServerRepositoryImpl serverRepo) that default to new instances when null, assign them to the existing fields (_pairingService, _notificationService, _serverRepo), and update callers/tests to pass mocks or fakes as needed so the screen becomes testable via injected dependencies.
🤖 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/presentation/screens/notification_setup_screen.dart`:
- Around line 187-216: The _viewLogs function currently calls _getDockerPath()
and pairingService.getNotifierLogs(...) without any loading UI or error
handling; wrap the async fetch in a try/catch and show a loading indicator while
awaiting results (e.g., set a local/loading state or show a modal with
CircularProgressIndicator before the await), ensure you check mounted before
calling setState or showing/closing dialogs, and on error display a user-facing
message (AlertDialog or SnackBar) including the error message; update references
in this flow to _getDockerPath, getNotifierLogs, mounted, and Navigator.pop to
properly close the loader before showing logs or the error dialog.
---
Duplicate comments:
In `@lib/presentation/widgets/pro_card.dart`:
- Around line 34-54: The _isLoading flag isn't guaranteed to be cleared if an
exception is thrown; wrap the asynchronous flow in _handleSubscribe (and the
other purchase handler, e.g. _handleRestore) with try/finally: setState(() =>
_isLoading = true) before the try, run the async call(s) in the try block, and
in finally call if (mounted) setState(() => _isLoading = false); keep the
success SnackBar logic inside the try after the await so the finally always
releases loading state even on errors.
---
Nitpick comments:
In `@lib/presentation/screens/notification_setup_screen.dart`:
- Around line 24-26: Replace the direct instantiation of PairingService,
NotificationService, and ServerRepositoryImpl inside NotificationSetupScreen
with constructor injection: add optional constructor parameters (e.g.,
PairingService pairingService, NotificationService notificationService,
ServerRepositoryImpl serverRepo) that default to new instances when null, assign
them to the existing fields (_pairingService, _notificationService,
_serverRepo), and update callers/tests to pass mocks or fakes as needed so the
screen becomes testable via injected dependencies.
In `@lib/presentation/widgets/pro_card.dart`:
- Around line 199-204: The GitHub IconButton in ProCard uses
BoxConstraints(minWidth: 36, minHeight: 36) which is below the recommended 48x48
touch target; update the IconButton's constraints (the instance with icon:
FaIcon(...), tooltip: 'settings.github_repo'.tr(), onPressed:
widget.onOpenGitHub) to use at least BoxConstraints(minWidth: 48, minHeight: 48)
or otherwise ensure a 48x48 hit area (e.g., wrap with SizedBox or adjust
padding) so the touch target meets accessibility guidelines.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/config/app_config.dartlib/presentation/screens/notification_setup_screen.dartlib/presentation/screens/settings_screen.dartlib/presentation/widgets/pro_card.dart
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/config/app_config.dart
- lib/presentation/screens/settings_screen.dart
Comment on lines
+187
to
+216
| Future<void> _viewLogs() async { | ||
| final dockerPath = await _getDockerPath(); | ||
| final logs = await _pairingService.getNotifierLogs( | ||
| dockerCliPath: dockerPath, | ||
| ); | ||
|
|
||
| if (mounted) { | ||
| showDialog( | ||
| context: context, | ||
| builder: (context) => AlertDialog( | ||
| title: Text('notifications.logs_title'.tr()), | ||
| content: SizedBox( | ||
| width: double.maxFinite, | ||
| child: SingleChildScrollView( | ||
| child: SelectableText( | ||
| logs, | ||
| style: const TextStyle(fontFamily: 'monospace', fontSize: 11), | ||
| ), | ||
| ), | ||
| ), | ||
| actions: [ | ||
| TextButton( | ||
| onPressed: () => Navigator.pop(context), | ||
| child: Text('common.close'.tr()), | ||
| ), | ||
| ], | ||
| ), | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing error handling and loading feedback for log viewing.
If getNotifierLogs() throws, the user sees no feedback. Also, there's no loading indicator while logs are being fetched, which could take time over SSH.
🛡️ Proposed fix with loading and error handling
Future<void> _viewLogs() async {
+ // Show loading dialog
+ showDialog(
+ context: context,
+ barrierDismissible: false,
+ builder: (context) => const Center(child: CircularProgressIndicator()),
+ );
+
+ String logs;
+ try {
final dockerPath = await _getDockerPath();
- final logs = await _pairingService.getNotifierLogs(
+ logs = await _pairingService.getNotifierLogs(
dockerCliPath: dockerPath,
);
+ } catch (e) {
+ if (mounted) {
+ Navigator.pop(context); // dismiss loading
+ setState(() => _error = e.toString());
+ }
+ return;
+ }
+
+ if (!mounted) return;
+ Navigator.pop(context); // dismiss loading
- if (mounted) {
showDialog(
context: context,
builder: (context) => AlertDialog(
title: Text('notifications.logs_title'.tr()),
// ... rest unchanged
),
);
- }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Future<void> _viewLogs() async { | |
| final dockerPath = await _getDockerPath(); | |
| final logs = await _pairingService.getNotifierLogs( | |
| dockerCliPath: dockerPath, | |
| ); | |
| if (mounted) { | |
| showDialog( | |
| context: context, | |
| builder: (context) => AlertDialog( | |
| title: Text('notifications.logs_title'.tr()), | |
| content: SizedBox( | |
| width: double.maxFinite, | |
| child: SingleChildScrollView( | |
| child: SelectableText( | |
| logs, | |
| style: const TextStyle(fontFamily: 'monospace', fontSize: 11), | |
| ), | |
| ), | |
| ), | |
| actions: [ | |
| TextButton( | |
| onPressed: () => Navigator.pop(context), | |
| child: Text('common.close'.tr()), | |
| ), | |
| ], | |
| ), | |
| ); | |
| } | |
| } | |
| Future<void> _viewLogs() async { | |
| // Show loading dialog | |
| showDialog( | |
| context: context, | |
| barrierDismissible: false, | |
| builder: (context) => const Center(child: CircularProgressIndicator()), | |
| ); | |
| String logs; | |
| try { | |
| final dockerPath = await _getDockerPath(); | |
| logs = await _pairingService.getNotifierLogs( | |
| dockerCliPath: dockerPath, | |
| ); | |
| } catch (e) { | |
| if (mounted) { | |
| Navigator.pop(context); // dismiss loading | |
| setState(() => _error = e.toString()); | |
| } | |
| return; | |
| } | |
| if (!mounted) return; | |
| Navigator.pop(context); // dismiss loading | |
| showDialog( | |
| context: context, | |
| builder: (context) => AlertDialog( | |
| title: Text('notifications.logs_title'.tr()), | |
| content: SizedBox( | |
| width: double.maxFinite, | |
| child: SingleChildScrollView( | |
| child: SelectableText( | |
| logs, | |
| style: const TextStyle(fontFamily: 'monospace', fontSize: 11), | |
| ), | |
| ), | |
| ), | |
| actions: [ | |
| TextButton( | |
| onPressed: () => Navigator.pop(context), | |
| child: Text('common.close'.tr()), | |
| ), | |
| ], | |
| ), | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/presentation/screens/notification_setup_screen.dart` around lines 187 -
216, The _viewLogs function currently calls _getDockerPath() and
pairingService.getNotifierLogs(...) without any loading UI or error handling;
wrap the async fetch in a try/catch and show a loading indicator while awaiting
results (e.g., set a local/loading state or show a modal with
CircularProgressIndicator before the await), ensure you check mounted before
calling setState or showing/closing dialogs, and on error display a user-facing
message (AlertDialog or SnackBar) including the error message; update references
in this flow to _getDockerPath, getNotifierLogs, mounted, and Navigator.pop to
properly close the loader before showing logs or the error dialog.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pubspec.yaml`:
- Around line 45-50: Add the missing Firebase platform config files and Android
notification permissions: place the Android google-services.json at the Android
app module root (matching the build.gradle.kts google-services plugin), add the
iOS GoogleService-Info.plist into the iOS Runner project, and update
AndroidManifest.xml to declare POST_NOTIFICATIONS,
SCHEDULE_EXACT_ALARM/USE_EXACT_ALARM and VIBRATE permissions so
firebase_messaging and flutter_local_notifications can initialize; keep
notification_service.dart as-is but re-run a clean build to verify Firebase
initializes with the new files and permissions.
Comment on lines
+45
to
+50
| purchases_flutter: ^8.5.1 | ||
| purchases_ui_flutter: ^8.5.1 | ||
| firebase_core: ^3.12.1 | ||
| firebase_messaging: ^15.2.4 | ||
| flutter_local_notifications: ^18.0.1 | ||
| crypto: ^3.0.3 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, explore repository structure
git ls-files | head -30Repository: theSoberSobber/Docker-Manager
Length of output: 1149
🏁 Script executed:
# Find and read the pubspec.yaml file
fd pubspec.yamlRepository: theSoberSobber/Docker-Manager
Length of output: 85
🏁 Script executed:
# Read the pubspec.yaml file
cat -n pubspec.yamlRepository: theSoberSobber/Docker-Manager
Length of output: 5744
🏁 Script executed:
# Check for Android build configuration
cat -n android/app/build.gradle.kts | head -60Repository: theSoberSobber/Docker-Manager
Length of output: 2873
🏁 Script executed:
# Check for google-services.json
fd -HI 'google-services.json' .Repository: theSoberSobber/Docker-Manager
Length of output: 55
🏁 Script executed:
# Check AndroidManifest for notification permissions
cat -n android/app/src/main/AndroidManifest.xmlRepository: theSoberSobber/Docker-Manager
Length of output: 3778
🏁 Script executed:
# Check for iOS Podfile
fd -HI 'Podfile' .Repository: theSoberSobber/Docker-Manager
Length of output: 123
🏁 Script executed:
# Check iOS Podfile
cat -n ios/PodfileRepository: theSoberSobber/Docker-Manager
Length of output: 1764
🏁 Script executed:
# Check for GoogleService-Info.plist (iOS Firebase config)
fd -HI 'GoogleService-Info.plist' .Repository: theSoberSobber/Docker-Manager
Length of output: 55
🏁 Script executed:
# Search for Firebase initialization in Dart code
rg -n 'FirebaseApp\.configure|Firebase\.initializeApp|firebase_core' --type dartRepository: theSoberSobber/Docker-Manager
Length of output: 190
🏁 Script executed:
# Check for flutter_local_notifications initialization
rg -n 'FlutterLocalNotifications|flutter_local_notifications|initializationSettings' --type dartRepository: theSoberSobber/Docker-Manager
Length of output: 497
🏁 Script executed:
# Check the notification service implementation
cat -n lib/data/services/notification_service.dart | head -100Repository: theSoberSobber/Docker-Manager
Length of output: 4381
🏁 Script executed:
# Check the rest of the notification service to see complete init
cat -n lib/data/services/notification_service.dart | tail -50Repository: theSoberSobber/Docker-Manager
Length of output: 1967
🏁 Script executed:
# Search for iOS notification initialization
rg -n 'UNUserNotificationCenter|didFinishLaunchingWithOptions|didRegisterForRemoteNotifications' --type swift --type objcRepository: theSoberSobber/Docker-Manager
Length of output: 311
🏁 Script executed:
# Check AppDelegate.swift
cat -n ios/Runner/AppDelegate.swiftRepository: theSoberSobber/Docker-Manager
Length of output: 554
🏁 Script executed:
# Check for Info.plist in iOS
cat -n ios/Runner/Info.plist | head -50Repository: theSoberSobber/Docker-Manager
Length of output: 2070
Add missing Firebase config files and Android notification permissions—Firebase initialization will fail without them.
Critical setup gaps found:
-
Android:
google-services.jsonis missing. The build.gradle.kts applies the google-services plugin, but the JSON configuration file required for Firebase initialization is absent. Without it, Firebase will not initialize at runtime. -
iOS:
GoogleService-Info.plistis missing. Required for iOS Firebase initialization. -
Android AndroidManifest.xml: Missing required notification permissions:
android.permission.POST_NOTIFICATIONS(mandatory for Firebase Messaging and local notifications on Android 13+)android.permission.SCHEDULE_EXACT_ALARMandandroid.permission.USE_EXACT_ALARM(flutter_local_notifications may need these)android.permission.VIBRATE(notification feedback)
The notification service implementation itself is sound (notification_service.dart has proper Firebase Messaging and flutter_local_notifications initialization), but platform-level wiring is incomplete. Add the missing config files and update AndroidManifest.xml with notification permissions before merging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pubspec.yaml` around lines 45 - 50, Add the missing Firebase platform config
files and Android notification permissions: place the Android
google-services.json at the Android app module root (matching the
build.gradle.kts google-services plugin), add the iOS GoogleService-Info.plist
into the iOS Runner project, and update AndroidManifest.xml to declare
POST_NOTIFICATIONS, SCHEDULE_EXACT_ALARM/USE_EXACT_ALARM and VIBRATE permissions
so firebase_messaging and flutter_local_notifications can initialize; keep
notification_service.dart as-is but re-run a clean build to verify Firebase
initializes with the new files and permissions.
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 by CodeRabbit
New Features
Localization
Chores
Documentation