-
-
Notifications
You must be signed in to change notification settings - Fork 32
Generic web watcher #138
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Generic web watcher #138
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caution
Changes requested ❌
Reviewed everything up to 9b94818 in 1 minute and 35 seconds. Click for details.
- Reviewed
897lines of code in14files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. mobile/src/androidTest/java/net/activitywatch/android/watcher/utils/CustomTabsWrapper.kt:44
- Draft comment:
Binding the CustomTabs service relies on CompletableFuture without explicit error handling. It may help to handle service disconnection or timeout exceptions explicitly. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is test code, which typically doesn't need the same level of robust error handling as production code. The code already has timeout handling and a fallback mechanism. The empty onServiceDisconnected handler is fine for tests. The CompletableFuture is used in a relatively straightforward way with timeout handling. I might be underestimating the importance of handling service disconnection errors in test code. There could be race conditions or edge cases I'm not seeing. While better error handling is always possible, the existing timeout and fallback mechanisms are sufficient for test code. Adding more error handling would make the test code more complex without providing significant benefits. The comment should be deleted as the existing error handling is adequate for test code, and adding more complexity would not be beneficial.
2. mobile/build.gradle:72
- Draft comment:
Dependency and JVM target updates look good; double-check that switching from kotlin-stdlib-jdk7 to jdk8 and Gradle version bumps are fully compatible with all modules. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_ZvN1gE6Qp9htdXGH
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
| return (0 until info.childCount) | ||
| .mapNotNull { info.getChild(it) } | ||
| .firstNotNullOfOrNull { child -> | ||
| findWebView(child).also { child.recycle() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recycling the child in the recursive findWebView() may lead to returning a recycled node. Consider deferring recycling until after you’ve determined the returned node is not needed further.
9b94818 to
b1fcaa4
Compare
b1fcaa4 to
7f94a2a
Compare
|
hi, been doing some testing with this and it worked for google chrome initially then stopped. I tried samsung internet and firefox, no data in the timeline view, no new data in the bucket. Haven't tested on the physical device yet. |
Hi @0xbrayo , thank you for feedback. I have been testing on emulated device as well. Are you able to dump logs from Also: what's the version of Android build and device model? |
|
I have yet been able to get the logs, will post them soon. The android version is 16, generic small phone on Android studio emulator. Also tested on pixel 9 pro, android 16, in the emulator. |
Changes introduced within this PR:
aw-watcher-android-webbrowserproperty to web activity eventswindowIdand ignores events withoutpackageNamebut with identicalwindowIdas previous event - this issue has been discovered during Firefox testingonBackPressedcallbackDiscovered issues to be addressed within next commits:
android.webkit.WebViewnode is not present)Important
Introduce a generic web watcher to track browser activities across multiple browsers, update dependencies, and add test coverage.
ChromeWatcherwithWebWatcherinWebWatcher.ktto track activities in Chrome, Firefox, Samsung Internet, Opera, and Edge.browserproperty to web activity events.windowIdand ignores events withoutpackageNamebut with identicalwindowId.WebWatcherTest.ktfor instrumentation test coverage across all available browsers.CustomTabsWrapper.ktfor handling custom tab interactions in tests.ScreenshotTest.ktto useInstrumentationRegistryfor screenshots.build.gradleandgradle-wrapper.properties.mobile/build.gradle.aw-watcher-android-web.onBackPressedinMainActivity.ktandOnboardingActivity.kt.This description was created by
for 9b94818. You can customize this summary. It will automatically update as commits are pushed.