Postpone fallback warning notification when VpnWarningActivity starts successfully - #35
Conversation
…ns when VpnWarningActivity starts successfully
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Warning Review limit reached
Next review available in: 34 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe VPN warning fallback now waits one second after attempting to launch ChangesVPN warning fallback
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/com/tunnelguard/app/TunnelGuardVpnService.kt`:
- Around line 236-285: Add a shared warning lock accessible to both
TunnelGuardVpnService and VpnWarningActivity, and synchronize the fallback block
around the pendingWarningId check through manager.notify(1002,
warningNotification). Update the activity’s matching-ID validation, clearing of
pendingWarningId, and cancellation of notification 1002 to use the same lock,
preserving the existing behavior while preventing cancellation and fallback
posting from interleaving.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 57c81de0-4138-4913-ac96-f13e343e275c
📒 Files selected for processing (1)
app/src/main/java/com/tunnelguard/app/TunnelGuardVpnService.kt
| serviceScope.launch { | ||
| delay(1000) | ||
| if (pendingWarningId == warningId) { | ||
| config.addLog("VpnWarningActivity did not launch in time. Posting fallback warning notification.") | ||
| val options = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.UPSIDE_DOWN_CAKE) { | ||
| android.app.ActivityOptions.makeBasic().setPendingIntentCreatorBackgroundActivityStartMode( | ||
| android.app.ActivityOptions.MODE_BACKGROUND_ACTIVITY_START_ALLOWED | ||
| ).toBundle() | ||
| } else { | ||
| null | ||
| } | ||
|
|
||
| val pendingIntent = PendingIntent.getActivity( | ||
| this@TunnelGuardVpnService, | ||
| 1002, | ||
| warningIntent, | ||
| PendingIntent.FLAG_IMMUTABLE or PendingIntent.FLAG_UPDATE_CURRENT, | ||
| options | ||
| ) | ||
|
|
||
| var appLabel = currentApp | ||
| try { | ||
| val pm = packageManager | ||
| val appInfo = pm.getApplicationInfo(currentApp, 0) | ||
| appLabel = pm.getApplicationLabel(appInfo).toString() | ||
| } catch (e: Exception) { | ||
| // Ignore | ||
| } | ||
|
|
||
| val warningNotificationBuilder = NotificationCompat.Builder(this@TunnelGuardVpnService, ALERT_CHANNEL_ID) | ||
| .setContentTitle("Security Warning") | ||
| .setContentText("$appLabel opened without an active VPN connection!") | ||
| .setSmallIcon(android.R.drawable.ic_dialog_alert) | ||
| .setPriority(NotificationCompat.PRIORITY_HIGH) | ||
| .setCategory(NotificationCompat.CATEGORY_ALARM) | ||
| .setAutoCancel(true) | ||
| .setContentIntent(pendingIntent) | ||
|
|
||
| // Only call setFullScreenIntent when permission is available according to NotificationManagerCompat | ||
| val managerCompat = androidx.core.app.NotificationManagerCompat.from(this@TunnelGuardVpnService) | ||
| if (managerCompat.canUseFullScreenIntent()) { | ||
| warningNotificationBuilder.setFullScreenIntent(pendingIntent, true) | ||
| } | ||
|
|
||
| val warningNotification = warningNotificationBuilder.build() | ||
| val manager = getSystemService(Context.NOTIFICATION_SERVICE) as NotificationManager | ||
| manager.notify(1002, warningNotification) | ||
| } else { | ||
| config.addLog("VpnWarningActivity launched successfully. Skipping fallback notification.") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Synchronize warning acknowledgement with fallback posting.
@Volatile makes pendingWarningId visible. It does not make the check at Line 238 and manager.notify at Line 282 atomic.
VpnWarningActivity.kt:37-45 can clear the matching ID and cancel notification 1002 after this check but before this notification post. In that order, the cancellation occurs first and the fallback notification remains posted.
Use one shared warning lock in both files. Hold it while the activity validates and clears the ID and cancels notification 1002. Hold the same lock while this coroutine checks the ID and posts the notification.
🧰 Tools
🪛 detekt (1.23.8)
[warning] 261-261: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/com/tunnelguard/app/TunnelGuardVpnService.kt` around lines
236 - 285, Add a shared warning lock accessible to both TunnelGuardVpnService
and VpnWarningActivity, and synchronize the fallback block around the
pendingWarningId check through manager.notify(1002, warningNotification). Update
the activity’s matching-ID validation, clearing of pendingWarningId, and
cancellation of notification 1002 to use the same lock, preserving the existing
behavior while preventing cancellation and fallback posting from interleaving.
…and synchronization in TunnelGuardVpnServiceWarningTest
|
@coderabbitai do unit tests |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Candidate files ==='
fd -t f . | rg '(^|/)(TunnelGuardVpnService|VpnWarningActivity).*\.(kt|java)$|(^|/).*(Test|Spec)\.(kt|java)$|(^|/)(build\.gradle(\.kts)?|settings\.gradle(\.kts)?)$'
printf '%s\n' '=== Service outline ==='
ast-grep outline app/src/main/java/com/tunnelguard/app/TunnelGuardVpnService.kt --items all --view expanded || true
printf '%s\n' '=== Test declarations and framework usage ==='
rg -n -C 3 --glob '*.{kt,java,kts,gradle}' \
'org\.junit|kotlin\.test|MockK|Mockito|Robolectric|androidx\.test|testImplementation|androidTestImplementation' \
.Length of output: 30206 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Warning-flow references ==='
rg -n -C 8 --glob '*.{kt,java}' \
'pendingWarningId|VpnWarningActivity|NotificationCompat|Handler|postDelayed|warning' \
app/src
printf '%s\n' '=== Warning activity outline ==='
warning_activity="$(fd -t f -a '^VpnWarningActivity\.(kt|java)$' app/src | head -n 1)"
if [ -n "$warning_activity" ]; then
ast-grep outline "$warning_activity" --items all --view expanded
fiLength of output: 54543 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Existing warning tests ==='
sed -n '1,310p' app/src/test/java/com/tunnelguard/app/TunnelGuardVpnServiceWarningTest.kt
printf '%s\n' '=== Production fallback flow ==='
sed -n '86,120p;200,305p' app/src/main/java/com/tunnelguard/app/TunnelGuardVpnService.kt
printf '%s\n' '=== Pull request file changes ==='
gh pr view 35 --repo DisabledAbel/TunnelGuard --json files --jq '.files[] | "\(.path): +\(.additions) -\(.deletions)"'Length of output: 19656
The tests validate the fallback decision logic. They do not execute the 1000 ms coroutine delay or verify You are interacting with an AI system. |
1 participant
This change resolves the issue where the on-screen warning pop-up (VpnWarningActivity) triggers when a protected application is opened without VPN, but also posts/displays an unnecessary notification that is immediately cancelled on entry.
By postponing the fallback notification's delivery by 1000ms inside TunnelGuardVpnService, we verify if the VpnWarningActivity has successfully launched in the foreground and consumed the 'pendingWarningId'. If so, we completely skip posting the fallback notification, preventing redundant flashes, sounds, and icons on TV and mobile screens. If the direct launch is silently blocked (e.g., due to background activity limits), the fallback notification is successfully posted as intended after the delay.
PR created automatically by Jules for task 6792269727374199170 started by @DisabledAbel
Summary by CodeRabbit