feat: playing around with aw-sync on android and misc #139
Conversation
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
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 975df0b in 2 minutes and 54 seconds. Click for details.
- Reviewed
3377lines of code in25files - Skipped
1files when reviewing. - Skipped posting
9draft 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. .idea/compiler.xml:4
- Draft comment:
The is updated to 21. Ensure that all modules and dependencies are compatible with JDK 21. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to ensure compatibility with JDK 21, which falls under the rule of not asking the author to ensure behavior is intended or tested. It does not provide a specific suggestion or point out a specific issue.
2. mobile/build.gradle:10
- Draft comment:
minSdkVersion has been increased to 25. Confirm that dropping support for API 24 is intentional. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This comment violates multiple rules. It's asking for confirmation of intention, which is explicitly forbidden. It's also about platform/dependency support changes, which we're told to ignore. The change is small (24 to 25) and likely intentional given it was committed. Perhaps dropping API support could have major implications for users that we should be concerned about? Maybe this needs discussion? The rules explicitly state not to ask for confirmations or make speculative comments. Platform support changes are intentional decisions that don't need review confirmation. Delete this comment as it violates rules against asking for confirmations and commenting on dependency/platform support changes.
3. mobile/src/main/AndroidManifest.xml:30
- Draft comment:
Screen orientation is locked to 'portrait'. Consider whether supporting 'unspecified' or 'fullSensor' might improve user experience on devices like Chrome OS. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. mobile/src/main/java/net/activitywatch/android/RustInterface.kt:56
- Draft comment:
The server startup logic checks port availability via binding a socket. Consider refining error handling for concurrency – e.g., ensuring proper reset of the serverStarted flag if startup fails unexpectedly. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. mobile/src/main/java/net/activitywatch/android/SyncInterface.kt:19
- Draft comment:
Using the public Downloads directory (via Environment.getExternalStoragePublicDirectory) for sync storage exposes data. Verify that this directory choice meets your privacy and security requirements. - 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% The comment raises a valid security concern, but it's phrased as "verify that..." which violates our rules. The code comment indicates this was an intentional choice for user accessibility. The security implications are likely already known and accepted by the developers. This seems more like asking for confirmation rather than pointing out a definite issue that needs fixing. The security concern is legitimate and could be serious if the developers haven't considered it. Maybe this should be kept as a warning? While the security concern is valid, the comment is phrased as a verification request rather than an actionable issue. The code comment shows this was a deliberate choice for user accessibility. Delete the comment because it violates our rule about verification requests ("Verify that...") and appears to be questioning an intentional design decision rather than pointing out a definite issue requiring changes.
6. mobile/src/main/java/net/activitywatch/android/SyncScheduler.kt:19
- Draft comment:
The sync scheduler uses a fixed 15‐minute interval. Consider making this interval configurable for different deployment scenarios. - 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% While making the interval configurable could add flexibility, there's no evidence this is actually needed. The 15-minute interval seems like a reasonable default. Without knowing the requirements or use cases, suggesting configuration adds unnecessary complexity. The comment is speculative about "different deployment scenarios" that aren't demonstrated to exist. The suggestion could be valuable if there are indeed different deployment needs. Making it configurable early could prevent future refactoring. We should follow YAGNI principle - don't add configuration without clear evidence it's needed. If different intervals are needed later, it's a simple refactor. Delete the comment as it suggests adding complexity without clear evidence of need, violating the rule about speculative comments.
7. mobile/src/main/java/net/activitywatch/android/parser/SessionParser.kt:148
- Draft comment:
The strict RESUME→PAUSE matching loop skips sessions if an extra RESUME occurs. Make sure this strict mode meets your intended session detection requirements and consider logging additional context for debugging. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =50%<= threshold50%The comment is asking the PR author to ensure that the strict mode meets their intended requirements, which violates the rule against asking for confirmation of intention. However, it also suggests considering logging additional context for debugging, which is a specific suggestion. The first part of the comment should be removed, but the second part is useful.
8. mobile/src/main/java/net/activitywatch/android/watcher/UsageStatsWatcher.kt:124
- Draft comment:
In setupAlarm(), the PendingIntent uses FLAG_IMMUTABLE which is good; ensure the chosen ELAPSED_REALTIME alarm type fits your scheduling needs even under Doze conditions. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. mobile/src/main/java/net/activitywatch/android/RustInterface.kt:95
- Draft comment:
Typo suggestion: The log message in the createBucketHelper method reads "Bucket with ID '$bucket_id', already existed. Not creating." Consider rephrasing it to something like "Bucket with ID '$bucket_id' already exists. Not creating." to improve clarity. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_TuLG6TnCI6KMpSnF
You can customize 
| callback: (Boolean, String) -> Unit, | ||
| syncFn: () -> String | ||
| ) { | ||
| val executor = Executors.newSingleThreadExecutor() |
There was a problem hiding this comment.
A new single-thread executor is created for each async sync call. Consider using a shared executor to avoid thread churn if sync operations are frequent.
| } | ||
| } | ||
|
|
||
| private inner class SendSessionEventTask : AsyncTask<Void, AppSession, Int>() { |
There was a problem hiding this comment.
The AsyncTask SendSessionEventTask is an inner class and may leak the Context. Consider refactoring it to a static inner class or using coroutines to prevent memory leaks.
| DEFAULT_JVM_OPTS='"-Xmx64m" "-Xms64m"' | ||
|
|
||
| # Collect all arguments for the java command: | ||
| # * DEFAULT_JVM_OPTS, JAVA_OPTS, JAVA_OPTS, and optsEnvironmentVar are not allowed to contain shell fragments, |
There was a problem hiding this comment.
Typo: In the comment on collecting java command arguments (line 206), JAVA_OPTS is mentioned twice and optsEnvironmentVar appears. This seems like an error; likely it should list DEFAULT_JVM_OPTS, JAVA_OPTS, and GRADLE_OPTS instead.
Suggested change
| # * DEFAULT_JVM_OPTS, JAVA_OPTS, JAVA_OPTS, and optsEnvironmentVar are not allowed to contain shell fragments, | |
| # * DEFAULT_JVM_OPTS, JAVA_OPTS, and GRADLE_OPTS are not allowed to contain shell fragments, |
Member
Author
|
@ellipsis-dev review |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 234b123 in 2 minutes and 33 seconds. Click for details.
- Reviewed
4368lines of code in34files - Skipped
1files 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. gradlew:206
- Draft comment:
Typo: The comment on line 206 repeats 'JAVA_OPTS' twice. Please remove the duplicate or clarify if two distinct options were intended. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This is the Gradle wrapper script (gradlew), which is a generated file from the Gradle project itself. The comment at line 57-58 explicitly states "This script is generated from the Groovy template https://github.com/gradle/gradle/blob/HEAD/subprojects/plugins/src/main/resources/org/gradle/api/internal/plugins/unixStartScript.txt within the Gradle project." This means the file is auto-generated and shouldn't be manually edited. If there's truly a typo, it would need to be fixed upstream in the Gradle project itself, not in this repository. The comment is technically correct that "JAVA_OPTS" appears twice on line 206, but suggesting a fix here is not appropriate since this is generated code. The PR author is likely just updating the Gradle wrapper version, and any issues in the generated script should be reported to Gradle, not fixed locally. However, I should verify if this is actually a typo or intentional. Looking at the comment, it mentions "DEFAULT_JVM_OPTS, JAVA_OPTS, JAVA_OPTS, and optsEnvironmentVar" - the duplicate JAVA_OPTS does seem odd. But even if it's a real typo, the key issue is that this is generated code from Gradle itself, so the comment is misdirected. Even if there is a genuine typo in the generated Gradle wrapper script, the comment should be deleted because: 1) This is auto-generated code that shouldn't be manually edited, and 2) The comment violates the rule about not commenting on things that should be obvious or that are outside the scope of this PR (the PR is updating the wrapper, not fixing Gradle's upstream issues). Delete this comment. The gradlew file is an auto-generated Gradle wrapper script that shouldn't be manually edited. Any issues with the generated content should be reported to the Gradle project upstream, not fixed in this repository.
2. mobile/build.gradle:48
- Draft comment:
Typo: The line 'coreLibraryDesugaringEnabled true' is missing an '='. It should be written as 'coreLibraryDesugaringEnabled = true' to match the assignment syntax. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This comment is flagging what it perceives as a syntax inconsistency. However, in Gradle's Groovy DSL, bothproperty = valueandproperty valueare valid syntaxes. The comment is treating this as a typo or error, but it's actually valid syntax. While consistency might be nice, this is not a bug or error that needs fixing. The comment is making a style suggestion at best, not identifying a real issue. According to the rules, I should not keep comments about code quality/style unless they are clearly actionable and important. This is a minor style inconsistency at best, and the code will work fine as-is. The build would not fail because of this. Could there be a specific reason why the equals sign is preferred in this context? Perhaps certain Gradle versions or Android Gradle Plugin versions require the equals sign for this specific property? I might be missing domain-specific knowledge about Android Gradle configuration. Even if there were version-specific requirements, the comment doesn't provide that context and simply calls it a "typo" to match "assignment syntax." If this were a real issue that would cause build failures, it would be caught by the build system. The comment is framed as a style/consistency issue, not a functional requirement. This comment should be deleted. It's flagging valid Gradle/Groovy syntax as a "typo" when bothproperty valueandproperty = valuesyntaxes are acceptable. This is at most a minor style preference, not a real issue, and doesn't meet the bar for a useful review comment.
Workflow ID: wflow_IunrW1SkDnKSlb1z
You can customize
- Created SyncInterface class to interact with native aw-sync library - Implemented JNI bindings for sync operations: * syncPullAll - pull from all hosts * syncPush - push local data * syncBoth - full bidirectional sync * getSyncDir - get sync directory path - Async wrappers with callbacks for all sync operations - Automatic sync directory setup in external storage - Proper error handling and logging Phase 4 implementation: Android app integration
- Modified SyncInterface to use Downloads/ActivityWatch/ directory * Static path for easy access by sync apps (Syncthing, etc.) * Falls back gracefully if storage not available - Created SyncScheduler for automatic periodic syncing * First sync runs 1 minute after app starts * Subsequent syncs every 15 minutes while app is open * Runs syncBoth (pull + push) for full bidirectional sync * Logs all sync operations and results - Integrated into MainActivity lifecycle * Starts scheduler in onCreate after server initialization * Stops scheduler in onDestroy to clean up resources No UI needed - syncs automatically in background. Users can access sync directory at: /sdcard/Download/ActivityWatch/ Phase 4 complete: Automatic background sync implementation
0xbrayo
force-pushed
the
investigate-aw-sync
branch
from
af35a44 to
7a52366
Compare
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 7a52366 in 3 minutes and 36 seconds. Click for details.
- Reviewed
3984lines of code in34files - Skipped
0files when reviewing. - Skipped posting
12draft 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. .idea/compiler.xml:4
- Draft comment:
Bytecode target level updated to 21; ensure your build and target devices support JDK 21 features. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to ensure compatibility with JDK 21 features, which is a form of asking them to double-check or verify something. This violates the rule against asking the author to ensure compatibility or verify intentions.
2. .idea/misc.xml:54
- Draft comment:
The ProjectRootManager now uses languageLevel JDK_21; ensure your IDE settings and environment are aligned. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is purely informative, reminding the PR author to align their IDE settings with the new language level. It doesn't provide a specific code suggestion or ask for a test to be written. It also doesn't point out a potential issue or ask for confirmation of an intentional change.
3. Makefile:174
- Draft comment:
New linking rules for libaw_sync.so have been added for multiple architectures; double-check these symlinks work in your CI and local environments. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to double-check the symlinks in their CI and local environments, which violates the rule against asking the author to double-check things. It does not provide a specific code suggestion or ask for a specific test to be written.
4. Makefile:239
- Draft comment:
Replaced the 'convert' command with 'magick' for image processing; ensure that ImageMagick (magick) is installed on all build systems. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to ensure that ImageMagick is installed on all build systems. This falls under the category of asking the author to ensure something is done, which is against the rules.
5. mobile/build.gradle:10
- Draft comment:
minSdkVersion has been updated from 24 to 26; confirm that this change is acceptable for your user base. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to confirm if the change in minSdkVersion is acceptable for their user base. This falls under asking the author to confirm their intention or ensure the behavior is intended, which is against the rules.
6. mobile/build.gradle:31
- Draft comment:
Release build type now includes an applicationIdSuffix '.debug'. Verify that this is intentional since release builds typically should not include debug suffixes. - 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% The comment is asking the PR author to "verify that this is intentional" which directly violates the rule: "Do NOT ask the PR author to confirm their intention, to explain, to double-check things, to ensure the behavior is intended". The comment is not pointing out a definite bug - it's speculating that this might be wrong and asking for confirmation. While it's true that release builds typically don't have debug suffixes, the fact that both debug and release have the same suffix suggests this might be intentional (perhaps for testing purposes or to allow both builds to coexist). Without more context about why this change was made, I cannot say this is definitely wrong. However, this could actually be a legitimate bug - having a ".debug" suffix on a release build is unusual and could cause real issues in production. The comment might be catching a genuine mistake that the author didn't intend to make. While it's unusual, the rules are clear: comments that ask to "verify" or "ensure" something are not useful. If I'm not certain this is wrong (and I'm not - it could be intentional for allowing side-by-side installation), I should delete it. The comment doesn't provide actionable feedback; it just asks for confirmation. This comment should be deleted because it explicitly asks the PR author to "verify that this is intentional" rather than stating a definite issue. This violates the rule against asking authors to confirm their intentions. Without strong evidence that this is definitely wrong, the comment should be removed.
7. mobile/src/androidTest/java/net/activitywatch/android/ScreenshotTest.kt:52
- Draft comment:
Using uiAutomation.takeScreenshot() is a good update for API ≥28. Ensure that lower API level devices are handled appropriately if needed. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to ensure that lower API level devices are handled appropriately. This falls under the category of asking the author to ensure behavior, which is against the rules. The comment does not provide a specific suggestion or point out a specific issue.
8. mobile/src/main/java/net/activitywatch/android/RustInterface.kt:40
- Draft comment:
The mutable 'serverStarted' flag in the companion object is accessed from multiple threads. Consider synchronizing access or using an atomic variable to avoid race conditions. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. mobile/src/main/java/net/activitywatch/android/RustInterface.kt:69
- Draft comment:
Consider shutting down the executor in startServerTask() after the server task completes to prevent potential thread leaks. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. mobile/src/main/java/net/activitywatch/android/parser/SessionParser.kt:150
- Draft comment:
The nested loops for pairing RESUME and PAUSE events may impact performance with high event volumes. Consider potential optimizations if performance becomes a bottleneck. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 15% vs. threshold = 50% The comment violates the rule about speculative comments ("If X, then Y is an issue"). It uses phrases like "may impact" and "if performance becomes a bottleneck" which are classic speculative language. The rules explicitly state: "Do NOT make speculative comments, such as 'If X, then Y is an issue', Only comment if it is definitely an issue." This is a new file being added, and there's no evidence that performance is actually a problem. The comment doesn't provide a concrete, actionable suggestion - it just mentions "potential optimizations" without specifics. According to the rules, I should only keep comments with STRONG EVIDENCE that they are correct and necessary. Could this actually be a legitimate performance concern that the author should address before merging? The nested loop could theoretically be problematic with thousands of events, and perhaps this is important enough to warrant attention even if speculative. While the O(n²) complexity is technically true, the comment is still speculative because it doesn't demonstrate that this is actually a problem in practice. For typical daily usage events (even heavy users might have hundreds to low thousands of events per day), this algorithm would likely perform fine. The comment doesn't provide concrete evidence of a performance issue or a clear actionable fix. The rules are explicit about removing speculative comments. This comment should be deleted because it's speculative ("may impact", "if performance becomes a bottleneck") and doesn't provide concrete evidence of an actual problem or a clear actionable solution. It violates the rule against speculative comments.
11. gradlew:136
- Draft comment:
Typographical issue: The error message starting on this line seems to span multiple lines without a proper line-continuation (or merging into a single line). In POSIX shell scripts, string literals cannot span lines without a backslash at the end. Please fix this so that the error message is correctly a single string. - Reason this comment was not posted:
Comment looked like it was already resolved.
12. gradlew:206
- Draft comment:
Typographical issue in the comment: It lists 'JAVA_OPTS' twice and refers to 'optsEnvironmentVar'. Likely, this should read something like 'DEFAULT_JVM_OPTS, JAVA_OPTS, and GRADLE_OPTS' to match the intended variables. Please update the comment accordingly. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This is a generated Gradle wrapper script. The comment is pointing out a legitimate typo in a code comment. However, I need to consider: 1) This is a generated file from Gradle itself (as noted in line 57-58), 2) The rules say not to comment on things that are obvious or unimportant, 3) This is just a typo in a comment, not in actual code, 4) If this is a generated file, the PR author likely didn't write this and can't fix it without modifying the upstream Gradle template. The comment is technically correct about the typo, but it may not be actionable if this is a generated file that shouldn't be manually edited. The file header explicitly states this script is "generated from the Groovy template" from the Gradle project (lines 57-59). If this is a generated file, asking the PR author to fix a typo in it would be inappropriate - they should report it upstream to Gradle instead. Also, a typo in a comment (not code) is relatively low importance. While the typo is real, this appears to be a generated file from Gradle's official template. The PR author likely just updated the Gradle wrapper version, which brought in this file. Asking them to manually edit a generated file would be incorrect - any changes would be overwritten on the next Gradle wrapper update. This comment is not actionable for the PR author. This comment should be deleted. The gradlew file is a generated file from Gradle's official template (as stated in the file itself), so the PR author cannot and should not manually fix typos in it. Any issues should be reported to the Gradle project upstream, not fixed in individual projects.
Workflow ID: wflow_6UYy9i4tOh6b9ldA
You can customize
| return eventsSent | ||
| } | ||
|
|
||
| private inner class SendSessionEventTask : AsyncTask<Void, AppSession, Int>() { |
There was a problem hiding this comment.
Usage of a non-static inner AsyncTask (SendSessionEventTask) can lead to memory leaks. Consider using a static inner class or, preferably, Kotlin coroutines.
| return (0 until info.childCount) | ||
| .mapNotNull { info.getChild(it) } | ||
| .firstNotNullOfOrNull { child -> | ||
| findWebView(child).also { child.recycle() } |
There was a problem hiding this comment.
The recursive call using child.recycle() inside firstNotNullOfOrNull may prematurely recycle nodes. Verify that recycling is safe and does not affect subsequent processing.
Suggested change
| findWebView(child).also { child.recycle() } | |
| findWebView(child).also { if (it == null) child.recycle() } |
Member
Author
|
@ellipsis-dev review |
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 38b89bf in 2 minutes and 52 seconds. Click for details.
- Reviewed
4006lines of code in35files - Skipped
0files when reviewing. - Skipped posting
5draft 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. Makefile:154
- Draft comment:
The linking rules for JNI libraries are repeated for each architecture. Consider refactoring these rules into a pattern rule to reduce repetition and improve maintainability. - 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% The comment is about code that was added in this PR (the libaw_sync.so rules). It's a code quality suggestion about refactoring. However, there's already a comment in the file (line 158) that explicitly acknowledges this exact issue: "# There must be a better way to do this without repeating almost the same rule over and over?". This means the author is already aware of the repetition problem. The PR is adding more of the same pattern that already exists - it's not introducing the repetition, just extending it. The comment is technically correct but the author clearly already knows about this issue and chose to follow the existing pattern. This makes the comment not particularly useful. The comment is technically a valid code quality suggestion and is about code that was changed. The author might appreciate the suggestion even if they're aware of the issue, as it could prompt them to finally refactor it. The existing comment might be old and the author might have forgotten about it. While the comment is technically valid, the existing comment at line 158 shows the author is explicitly aware of this issue. The PR is simply following the existing pattern consistently. If the author wanted to refactor this, they would have done it before adding more rules. This comment doesn't add new information and asking them to refactor now when they're just adding a new library following the existing pattern is not actionable in the context of this PR. The comment should be deleted. The author is already aware of the repetition issue (as evidenced by the comment at line 158), and this PR is simply following the existing pattern. The suggestion to refactor is not actionable in the context of this PR which is adding a new library.
2. mobile/build.gradle:31
- Draft comment:
Release build type specifies an applicationIdSuffix ".debug". Ensure this is intentional, as release builds normally should not include a debug suffix. - 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% The comment is asking the author to "Ensure this is intentional" which is essentially asking them to confirm or verify their intention. According to the rules, I should NOT keep comments that ask the PR author to confirm their intention, to explain, to double-check things, or to ensure the behavior is intended. The comment starts with "Ensure this is intentional" which directly violates this rule. While it's unusual to have a ".debug" suffix on a release build, the fact that both debug and release have the same suffix suggests this might be intentional (perhaps to allow both builds to be installed side-by-side with a production version). The comment is speculative ("normally should not") rather than definitively pointing out an error. However, this could be a genuine mistake that would cause problems - having a debug suffix on release builds is highly unusual and could indicate a copy-paste error. The comment might be catching a real bug that would affect the production app's identity. While it could be a mistake, the rule explicitly states not to ask authors to verify or ensure their intentions. The comment doesn't definitively state this is wrong - it uses "normally should not" which is speculative. If this were truly wrong, it would be caught during testing or deployment. The author clearly made this change deliberately as shown in the diff. This comment should be deleted because it explicitly asks the author to "Ensure this is intentional," which violates the rule against asking PR authors to confirm their intention or verify their changes. The comment is speculative rather than definitively identifying an error.
3. mobile/src/androidTest/java/net/activitywatch/android/ScreenshotTest.kt:49
- Draft comment:
Avoid using Thread.sleep for synchronization; consider using a proper wait mechanism or IdlingResources to wait for UI readiness. - Reason this comment was not posted:
Comment was on unchanged code.
4. mobile/src/main/java/net/activitywatch/android/parser/SessionParser.kt:147
- Draft comment:
The strict RESUME/PAUSE event pairing logic may miss sessions when events are out of order. Consider implementing a more robust method to handle out-of-order events. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment is suggesting a potential issue with out-of-order events. However, looking at the extractRawEvents method (line 101-122), I can clearly see that events are sorted by timestamp at line 121 before being returned. This sorted list is then passed to parseEventsIntoSessions. So the concern about out-of-order events is not valid - the code already handles this. Additionally, the comment is speculative ("may miss sessions") rather than pointing to a definite issue. The file header also mentions this achieved 99.1% accuracy vs Digital Wellbeing, suggesting the approach is working well. This comment appears to be incorrect and not useful. Could there be a scenario where events from the Android system itself come out of order even after sorting by timestamp, such as if timestamps are identical or if there's some other ordering issue I'm not considering? Maybe the comment is referring to a more subtle issue than simple chronological ordering. Even if there were edge cases with identical timestamps, the comment doesn't specify this and suggests implementing "a more robust method to handle out-of-order events" without acknowledging that events are already sorted. The comment is too vague and doesn't provide actionable guidance. If there were a specific edge case, the comment should identify it clearly. The comment should be deleted. The code already sorts events by timestamp at line 121 before processing them, so the concern about out-of-order events is not valid. The comment is speculative and doesn't identify a specific issue that needs to be addressed.
5. mobile/src/main/java/net/activitywatch/android/watcher/UsageStatsWatcher.kt:182
- Draft comment:
AsyncTask (SendHeartbeatsTask) is used, but AsyncTask is deprecated. Consider replacing it with coroutines or WorkManager to handle background operations. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_kM0lRnkag8ISLVrz
You can customize
mobile/src/main/java/net/activitywatch/android/watcher/WebWatcher.kt
Outdated
Show resolved
Hide resolved
mobile/src/main/java/net/activitywatch/android/watcher/SessionEventWatcher.kt
Show resolved
Hide resolved
…her.kt Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
0xbrayo
force-pushed
the
investigate-aw-sync
branch
from
2ee3e31 to
8b6e2f0
Compare
Member
Author
|
@greptileai review |
Member
Author
|
@ErikBjare review, still not ready for merging but feedback would be appreciated. The PR is quite large unfortunately |
|
really looking forward to this PR. right now there is no way to get your android screen time data into your own database without building your own background service that polls UsageStats directly. RescueTime used to collect this data which you could then pull from their API, but they no longer support their Android app. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Important
Enhances Android app with session-based event tracking, background sync services, and updated configurations for improved data accuracy and synchronization.
SessionEventWatcher.ktandUsageStatsWatcher.kt, replacing heartbeat-based tracking for improved accuracy.BackgroundService.ktto manage server and sync operations in the background.SyncScheduler.ktandSyncInterface.ktfor periodic data synchronization.MainActivity.ktto startBackgroundServicefor continuous operation.build.gradleandgradle.propertiesto use Kotlin 1.9.0 and Gradle 8.14.minSdkVersionto 26 andtargetSdkVersionto 34 inmobile/build.gradle.AndroidManifest.xmlfor foreground services and data sync.WebWatcherTest.ktandScreenshotTest.ktfor testing web activity tracking and UI.CustomTabsWrapper.ktfor handling custom tab interactions in tests.README.mdwith new build instructions.EventParsingWorker.ktfor scheduled event parsing using WorkManager.ChromeWatcher.ktand replaces it withWebWatcher.ktfor broader browser support.This description was created by
for 38b89bf. You can customize this summary. It will automatically update as commits are pushed.