Skip to content

Comments

Replace aw-qt with aw-tauri#1163

Closed
0xbrayo wants to merge 10 commits intoActivityWatch:masterfrom
0xbrayo:master
Closed

Replace aw-qt with aw-tauri#1163
0xbrayo wants to merge 10 commits intoActivityWatch:masterfrom
0xbrayo:master

Conversation

@0xbrayo
Copy link
Member

@0xbrayo 0xbrayo commented Jul 20, 2025

Important

Replace aw-qt with aw-tauri, updating build, submodules, and packaging scripts for Tauri integration.

  • Build System:
    • Add build-tauri.yml for Tauri builds, supporting multiple OS and Python/Node versions.
    • Update build.yml to reflect new OS and Node versions.
    • Modify Makefile to conditionally include aw-tauri and exclude aw-qt.
  • Submodules:
    • Add aw-tauri and awatcher submodules in .gitmodules.
    • Remove aw-qt from active submodules when TAURI_BUILD is true.
  • Scripts:
    • Add build_app_tauri.sh for macOS app bundle creation.
    • Add aw-tauri.iss for Windows installer creation.
    • Update package-all.sh to handle Tauri builds.
    • Add move-to-aw-modules.sh for module management.
  • Documentation:
    • Add README.txt in scripts/package for module setup instructions.
    • Update contributor information in changelog_contributors.csv and changelog_contributors_twitter.csv.

This description was created by Ellipsis for 7c07d6d. You can customize this summary. It will automatically update as commits are pushed.

@0xbrayo
Copy link
Member Author

0xbrayo commented Jul 20, 2025

Still WIP, tested Windows and MacOS both work. I haven't yet tested signing yet.

@0xbrayo
Copy link
Member Author

0xbrayo commented Jul 20, 2025

Windows build randomly fails sometimes idk why.

@0xbrayo 0xbrayo force-pushed the master branch 18 times, most recently from d4b124c to 3aacabe Compare July 29, 2025 15:21
@0xbrayo 0xbrayo marked this pull request as ready for review July 29, 2025 15:22
@0xbrayo
Copy link
Member Author

0xbrayo commented Jul 29, 2025

Adds arm releases for linux and macos. Windows won't compile for arm.

Copy link

@ellipsis-dev ellipsis-dev bot left a 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 3aacabe in 2 minutes and 18 seconds. Click for details.
  • Reviewed 1174 lines of code in 19 files
  • Skipped 0 files when reviewing.
  • Skipped posting 9 draft 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. .github/workflows/build.yml:72
  • Draft comment:
    The 'Set up Python for macOS' step only triggers when matrix.os == 'macOS-13', but the matrix also includes 'macos-latest'. Ensure both cases are handled consistently.
  • 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 code appears to be intentionally handling the two macOS environments differently. macOS-13 gets a special setup for compatibility with older macOS versions (10.9), while macos-latest uses the standard setup. This seems like a deliberate design choice rather than an oversight. The comment assumes both should be handled the same way but provides no evidence for why that would be better. I could be wrong about the intention - maybe there's a technical reason why both macOS environments should use the older-compatible Python setup. The code and comments clearly document that the special setup is for compatibility with older macOS versions. If this was needed for all macOS builds, the code would likely use runner.os == 'macOS' instead of the specific matrix.os check. The different handling of macOS environments appears intentional and well-documented. The comment assumes a problem without providing evidence.
2. .github/workflows/build.yml:60
  • Draft comment:
    Hard-coding 'RELEASE=true' bypasses dynamic detection from tag/branch names; confirm that this behavior 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 = 0% vs. threshold = 50% This is a clear change in behavior from dynamic to static configuration. However, asking for confirmation violates our rules - we shouldn't ask authors to confirm their intentions. The change appears deliberate since they preserved the old code in a comment. If this was unintentional, the build system would catch any issues. The change could have significant implications for the build process that I may not fully understand. Setting RELEASE=true for all builds could affect performance or resource usage. While the implications could be significant, asking for confirmation isn't helpful - the author clearly made this change intentionally by commenting out the old code rather than accidentally. If it causes problems, they'll be caught during testing. Delete the comment. The change appears intentional and asking for confirmation violates our rules about not asking authors to verify their intentions.
3. .gitmodules:26
  • Draft comment:
    The aw-tauri submodule URL lacks a '.git' extension. For consistency with other submodules, consider appending '.git'.
  • 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 the comment is technically correct about inconsistency, adding .git to GitHub URLs is optional - they work the same either way. This seems like a very minor stylistic issue that doesn't affect functionality. The comment doesn't point out a real problem that needs fixing. The inconsistency could make the file harder to maintain or cause confusion for future contributors. Having a consistent style throughout a codebase is generally good practice. While consistency is good, this is an extremely minor issue. The .git suffix is optional in GitHub URLs and this kind of nitpick creates unnecessary noise in the review process. Delete the comment. The suggested change is too minor to warrant a review comment and doesn't affect functionality.
4. scripts/package/package-appimage.sh:15
  • Draft comment:
    The generated AppRun script still references 'aw-qt'. Update it to call 'aw-tauri' to match the new executable.
  • 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. scripts/package/README.txt:3
  • Draft comment:
    A space appears to be missing after the period in the guidance text; consider changing "deb binaries.Choose" to "deb binaries. Choose" for clarity.
  • Reason this comment was not posted:
    Confidence changes required: 50% <= threshold 50% None
6. Makefile:24
  • Draft comment:
    Typo suggestion: The comment on line 24 reads "#Include awatcher on linux". For consistency and readability, consider adding a space after '#' (e.g., "# Include awatcher on Linux").
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While the suggestion is technically correct from a style perspective, our rules explicitly state to focus on logic issues and not make purely informative or obvious comments. Comment formatting is a minor style issue that doesn't affect functionality. The rules emphasize focusing on substantive issues that require code changes. The comment does point out an inconsistency in the codebase's style. Consistent formatting can improve readability. However, the rules clearly state not to make purely informative comments or focus on obvious/unimportant issues. Comment formatting falls into this category. This comment should be deleted as it addresses a minor style issue rather than a substantive code change requirement.
7. scripts/package/README.txt:3
  • Draft comment:
    There seems to be a missing space after the period. The text "deb binaries.Choose" should be "deb binaries. Choose".
  • 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% While the comment is technically correct about the missing space, it's a very minor formatting issue in a documentation file. The text is still perfectly readable and understandable. According to the rules, we shouldn't make comments that are obvious or unimportant. Documentation formatting nitpicks generally fall into this category. The missing space could be seen as affecting readability, and since this is documentation, clarity is important. Also, it's a very simple fix. While it's an easy fix, it's still an extremely minor issue that doesn't impact the understanding of the documentation. We should focus on more substantial issues. Delete this comment as it's too minor and doesn't meaningfully improve the documentation.
8. scripts/package/README.txt:3
  • Draft comment:
    Typo in the phrase "copy it a permanent folder" – it appears to be missing the preposition "to" (should be "copy it to a permanent folder").
  • 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% While the comment is technically correct about the grammar, we need to consider if this is important enough to warrant a comment. Documentation clarity is important, but this is a minor grammatical issue that doesn't affect understanding. The rules state we should not make purely informative comments or unimportant ones. The missing "to" could potentially cause confusion for non-native English speakers. Documentation should be held to high standards of clarity. While clarity is important, this error is minor enough that readers would still understand the meaning perfectly. The comment doesn't point out a significant issue that would impact usability. Delete this comment as it points out a very minor grammatical issue that doesn't significantly impact the documentation's clarity or usefulness.
9. scripts/package/package-appimage.sh:21
  • Draft comment:
    There is a potential inconsistency in naming: the executable is referenced as "aw-tauri" (with a hyphen) while the desktop file and icon file paths use "aw_tauri" (with an underscore). Please confirm that this 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% 1. The comment is about changed code - the entire line is new. 2. It's asking for confirmation of intention, which violates our rules. 3. If this was a real issue, it would likely cause the build to fail. 4. File paths and executable names can legitimately use different conventions. 5. Without access to the actual files and build system, we can't know if this is actually a problem. The naming inconsistency could potentially cause real issues on Linux systems if the paths don't match the actual filesystem structure. If the paths were incorrect, the build would fail immediately. The PR author would have tested this before submitting. Delete the comment. It asks for confirmation of intention and speculates about a potential issue that would be caught by testing.

Workflow ID: wflow_7dvICAT5HX76LPHz

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@0xbrayo 0xbrayo force-pushed the master branch 4 times, most recently from 6066207 to ccd6b0c Compare July 30, 2025 17:18
@0xbrayo
Copy link
Member Author

0xbrayo commented Jul 31, 2025

@ErikBjare review

@ErikBjare
Copy link
Member

Looks good, but I'd like to keep the old-style aw-qt builds. Haven't reviewed thoroughly (still on vacation).

Maybe build the new aw-tauri bundle in a separate CI workflow/job?

@0xbrayo
Copy link
Member Author

0xbrayo commented Aug 1, 2025

Cool, will do.

; NOTE: The value of AppId uniquely identifies this application. Do not use the same AppId value in installers for other applications.
; (To generate a new GUID, click Tools | Generate GUID inside the IDE.)
; NOTE: the double {{ are used to escape the { character (needed for the AppId)
AppId={{F226B8F4-3244-46E6-901D-0CE8035423E4}
Copy link
Member

@ErikBjare ErikBjare Aug 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be updated to not collide with the aw-qt installation.

Should probably mention in MyAppName or AppVerName that this is about the Tauri version (not sure which is best).

@0xbrayo 0xbrayo force-pushed the master branch 4 times, most recently from ebfb982 to 458cfea Compare August 7, 2025 16:23
@0xbrayo
Copy link
Member Author

0xbrayo commented Aug 8, 2025

Not very sure about the last commit, it works but maybe it will break later? Not sure how best to test it.

@0xbrayo 0xbrayo marked this pull request as ready for review August 8, 2025 07:19
@0xbrayo
Copy link
Member Author

0xbrayo commented Aug 8, 2025

@ellipsis-dev review

Copy link

@ellipsis-dev ellipsis-dev bot left a 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 be51352 in 2 minutes and 0 seconds. Click for details.
  • Reviewed 902 lines of code in 17 files
  • Skipped 0 files when reviewing.
  • Skipped posting 9 draft 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. .github/workflows/build-tauri.yml:19
  • Draft comment:
    Typo in comment: 'Wether' should read 'Whether'.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While the spelling correction is accurate, this is just a typo in a comment that doesn't affect functionality. The rules state we should not make purely informative comments or comments about obvious/unimportant issues. A minor spelling error in a comment falls into this category. The typo could theoretically cause confusion for future readers of the code, since "wether" is a different word with a different meaning (relating to a castrated ram). In practice, the meaning is completely clear from context and this level of nitpicking on comments is not productive. The benefit of fixing this is minimal compared to the overhead of the PR comment. This comment should be deleted as it points out a minor, non-functional issue that doesn't meaningfully impact code quality or understanding.
2. .gitmodules:31
  • Draft comment:
    Verify that the URL for the 'aw-tauri' submodule is complete; consider appending '.git' if required by your workflow.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to verify something, which is against the rules. It suggests appending '.git' if required, but this is not a specific code suggestion or a request for a test. It is more of a general cautionary note.
3. scripts/package/aw-tauri.iss:18
  • Draft comment:
    Confirm that the AppId formatting (using double '{{' braces) complies with Inno Setup guidelines.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is asking the PR author to confirm compliance with guidelines, which is against the rules. It doesn't provide a specific suggestion or point out a clear issue with the code.
4. scripts/package/move-to-aw-modules.sh:5
  • Draft comment:
    Ensure that 'aw-awatcher' is the intended binary name; it differs from the submodule name 'awatcher' and may be a typo.
  • 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 the rule about not asking authors to verify their intentions. The comment starts with "Ensure that..." which is explicitly called out as likely not useful. The difference in names might be intentional - it's common for a project directory to have a different name than its binary. Without more context about the codebase, we can't know if this is actually an issue. Maybe there's a real naming inconsistency here that could cause problems. The different names could indicate a mistake. Even if there is a naming issue, asking the author to "ensure" or verify something is not actionable. If there was strong evidence of a problem, the comment should state it directly. This comment should be deleted as it violates the rule about asking authors to verify their intentions, and we don't have strong evidence that the naming is actually incorrect.
5. scripts/package/package-all.sh:32
  • Draft comment:
    Verify that 'getversion.sh' reliably outputs the expected version string for packaging.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
6. .github/workflows/build-tauri.yml:19
  • Draft comment:
    Typo: "Wether" should be "Whether" (line 19).
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While this is technically correct - there is a typo - it's an extremely minor issue in a comment. Comments are not code and don't affect functionality. The rules say not to make comments that are obvious or unimportant. A typo in a comment is about as unimportant as it gets. The typo could be seen as unprofessional and might be worth fixing for code quality and professionalism reasons. While professionalism is important, this is an extremely minor issue in a comment that doesn't affect functionality or readability. The rules specifically say not to make unimportant comments. This comment should be deleted as it points out an extremely minor issue that doesn't affect functionality or understanding.
7. Makefile:35
  • Draft comment:
    Typographical suggestion: Consider adding a space after the # and capitalizing 'Linux' (i.e. # Include awatcher on Linux) for consistency.
  • 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% While the suggestion is technically correct (Linux should be capitalized and consistency in comment formatting is good), this is an extremely minor formatting issue in a Makefile. The code works perfectly fine either way. Such minor style issues in comments don't warrant a PR comment and could be seen as noise. The suggestion is technically correct and would improve consistency. Some might argue that maintaining consistent style even in comments is important for code quality. While consistency is good, this is too minor of an issue to warrant a PR comment. It's about comment formatting in a Makefile, not about any functional code changes. Delete this comment as it's too minor of a style issue to warrant a PR comment. It's about comment formatting that doesn't affect functionality.
8. scripts/package/README.txt:3
  • Draft comment:
    There are a couple of typographical issues on this line: 1. There is no space after the period in "deb binaries.Choose". Consider adding a space after "binaries.". 2. In the phrase "copy it a permanent folder", the word "to" appears to be missing. It should read "copy it to a permanent folder". Additionally, consider capitalizing "linux" to "Linux" if it's referring to the operating system.
  • 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 new README file, so these changes are about new content. The suggestions are technically correct and would improve readability. However, our rules state we should focus on logic issues, not pure documentation/text changes. Grammar and capitalization fixes, while helpful, are not critical code issues that require changes. The changes would make the documentation more professional and easier to read. Poor documentation can lead to user confusion. While better documentation is valuable, our review rules explicitly focus on logic and code quality issues. These are purely cosmetic text improvements. Delete this comment as it only addresses documentation formatting and grammar, not substantive logic or code quality issues.
9. scripts/package/aw-tauri.iss:18
  • Draft comment:
    Typo: The AppId line appears to be missing a closing '}'—it should likely read: AppId={{983D0855-08C8-46BD-AEFB-3924581C6703}} to properly escape the GUID as noted in the comment.
  • 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 appears to be incorrect. The file's own documentation explicitly states that double {{ are used to escape the { character, not that we need double }}. This is a new file being added, and the syntax follows the documented pattern. The comment author may have misunderstood the escaping rules for Inno Setup. Could there be some Inno Setup syntax requirement that I'm not aware of? Maybe the documentation comment is wrong and we do need double }}? The inline documentation is explicit about the escaping rules, and since this is a new file that was likely generated by the Inno Setup Script Wizard (as stated in line 1), it's very likely that the syntax is correct as-is. The comment should be deleted as it appears to be incorrect - the current syntax matches the documented escaping rules.

Workflow ID: wflow_cMkkzZ8MlKVnCBN5

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Makefile Outdated
ifeq ($(TAURI_BUILD),true)
ICON := "aw-tauri/aw-webui/media/logo/logo.png"
else
ICON := "aw-tauri/aw-webui/media/logo/logo.png"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both branches of the ICON assignment set the same value. Confirm if the else branch should instead use the legacy aw-qt icon (e.g. 'aw-qt/media/logo/logo.png') for non-TAURI builds.

Suggested change
ICON := "aw-tauri/aw-webui/media/logo/logo.png"
ICON := "aw-qt/media/logo/logo.png"

Makefile Outdated
# Exclude aw-server-rust if SKIP_SERVER_RUST is true
ifeq ($(SKIP_SERVER_RUST),true)
SUBMODULES := $(filter-out aw-server-rust,$(SUBMODULES))
SUBMODULES := $(filter-out aw-server-rust,$(SUBMODULES))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: Inconsistent indentation on this line. The Makefile recipe should be indented with a tab rather than spaces. Please update the indentation.

Copy link

@ellipsis-dev ellipsis-dev bot left a 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 be51352 in 2 minutes and 4 seconds. Click for details.
  • Reviewed 902 lines of code in 17 files
  • Skipped 0 files when reviewing.
  • Skipped posting 6 draft 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. .github/workflows/build.yml:56
  • Draft comment:
    The Python setup conditions only check for 'macOS-13', but the matrix also includes 'macos-latest'. This may skip macos-latest jobs from using the custom setup. Consider handling both cases consistently.
  • 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% Looking at the conditions, macos-latest will use the standard Python setup since it doesn't match 'macOS-13'. This appears intentional - the special setup is only needed for macOS-13 due to older macOS compatibility issues (as noted in the comment on line 61). The macos-latest runner doesn't need the special setup, so the current conditions are actually correct. I could be wrong about whether macos-latest needs special handling. There might be compatibility issues I'm not aware of. The code comment explicitly states this special setup is for older macOS compatibility. Since macos-latest is newer, it's reasonable that it doesn't need this special handling. The current setup conditions are correct - only macOS-13 needs special handling, while macos-latest can use the standard Python setup. This comment should be deleted.
2. Makefile:72
  • Draft comment:
    For the build target, the 'aw-server-rust' module triggers the 'aw-sync' target when TAURI_BUILD is true. Consider adding a comment or documentation to clarify this behavior.
  • 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 behavior is already quite clear from the code itself. The if-condition explicitly shows when aw-sync is used instead of build. The code is self-documenting and the logic is straightforward. Adding a comment would be redundant and wouldn't add significant value. Perhaps some developers unfamiliar with Tauri builds would benefit from understanding why aw-sync is used instead of build in this case. The code is clear enough that it uses a different target for Tauri builds. If developers need to understand why, that belongs in higher-level documentation about Tauri builds, not as an inline comment. The comment should be deleted as it requests redundant documentation for behavior that is already clear from the code.
3. scripts/package/build_app_tauri.sh:8
  • Draft comment:
    ICON_PATH is defined as a relative path. Ensure the script is invoked from the repository root or use a script-relative path to avoid issues.
  • 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 script already handles missing icons gracefully. While making paths script-relative is generally good practice, this seems more like a nice-to-have suggestion than a critical issue. The script is clearly meant to be run from the repo root given other paths like "dist/activitywatch". The comment is suggesting a change that isn't strictly necessary. The comment does point out a real potential improvement in script robustness. Making paths script-relative is a shell scripting best practice. While it's a valid suggestion, the script already handles missing icons gracefully and other paths in the script assume repo root context anyway. This change wouldn't meaningfully improve the script's functionality. Delete the comment. While technically correct, it suggests an optional improvement to a script that already handles the failure case appropriately.
4. scripts/package/package-all.sh:45
  • Draft comment:
    The script copies README.txt and move-to-aw-modules.sh into the dist/activitywatch directory on Linux. Confirm that including these files in the distribution 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% The comment violates our rules by asking for confirmation of intention. We should not ask authors to verify their changes - we should assume they made the change intentionally unless there's strong evidence of a problem. The files being copied are likely documentation and utility scripts that the author deliberately wants to include in the Linux distribution. Maybe these files contain critical information that needs review before being included in the distribution? Maybe they could contain sensitive information? Without seeing the actual contents of these files, we can't make assumptions about security risks. The files are in the repo already, so they've been previously reviewed. We should trust the author's judgment about including them. Delete this comment as it violates our rule against asking authors to confirm their intentions. The change appears deliberate and there's no clear evidence of a problem.
5. .github/workflows/build-tauri.yml:19
  • Draft comment:
    Typo: "Wether" should be "whether" in the comment.
  • 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% While this is technically correct - there is a typo - comments about typos in comments are generally very low value. The typo doesn't affect functionality and fixing it provides minimal value. The rules state we should not make comments that are obvious or unimportant. The typo could potentially cause confusion for future readers of the code since it's in a comment describing an environment variable's purpose. While technically true, the meaning is still clear from context and this is an extremely minor issue that doesn't warrant a PR comment. This comment should be deleted as it points out a trivial typo that doesn't impact functionality and falls under the category of "obvious or unimportant" comments that we should avoid.
6. Makefile:35
  • Draft comment:
    Typographical note: The comment "#Include awatcher on linux" might be revised for consistency. Consider adding a space after '#' (i.e., "# Include awatcher on Linux") to match the formatting and same capitalization as used in the ifeq comparison.
  • 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 about a newly added line in the diff. However, it's purely about code style and formatting, not about logic or functionality. The rules state we should focus on logic issues and not make purely informative comments. While the suggestion would improve consistency, it's a very minor formatting issue that doesn't impact functionality. The comment does point out an actual inconsistency in the codebase formatting. Consistent formatting can improve code readability. While consistent formatting is good, this is too minor of an issue to warrant a comment. The rules explicitly state not to make purely informative comments or obvious suggestions. Delete this comment as it's purely about formatting and doesn't suggest any necessary code changes that would impact functionality.

Workflow ID: wflow_saCbNkuUUwwxugnC

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Makefile Outdated
git submodule update --init --recursive

ICON := "aw-qt/media/logo/logo.png"
ifeq ($(TAURI_BUILD),true)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ICON variable is defined identically in both TAURI_BUILD true and false branches. If the paths should differ for tauri vs non-tauri builds, update one; otherwise, remove the conditional for clarity.

Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
@0xbrayo
Copy link
Member Author

0xbrayo commented Aug 8, 2025

@ellipsis-dev review

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

Looks good to me! 👍

Reviewed everything up to 7c07d6d in 1 minute and 46 seconds. Click for details.
  • Reviewed 891 lines of code in 17 files
  • Skipped 0 files when reviewing.
  • Skipped posting 16 draft 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. .github/workflows/build-tauri.yml:60
  • Draft comment:
    TAURI_BUILD is correctly set and propagated; verify downstream tasks handle it consistently.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
2. .github/workflows/build.yml:55
  • Draft comment:
    Updated matrix OS values and use of 'matrix.os' (vs runner.os) looks correct; just confirm that the changed APT packages (qt5-qmake, qtbase5-dev) satisfy build requirements.
  • Reason this comment was not posted:
    Confidence changes required: 20% <= threshold 50% None
3. .gitmodules:31
  • Draft comment:
    New submodules 'awatcher' and 'aw-tauri' are added; ensure TAURI builds use aw-tauri instead of aw-qt.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
4. Makefile:16
  • Draft comment:
    The TAURI_BUILD conditional correctly replaces aw-qt with aw-tauri in the SUBMODULES list.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
5. Makefile:153
  • Draft comment:
    The packaging target now conditionally invokes build_app_tauri.sh when TAURI_BUILD is true, which is appropriate for replacing aw-qt.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
6. Makefile:168
  • Draft comment:
    Conditional packaging logic for aw-server-rust (copying aw-sync) under TAURI_BUILD is correctly applied.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
7. scripts/changelog_contributors.csv:28
  • Draft comment:
    Removal of the 'brayo-pip' entry appears intentional; ensure it reflects correct contributor attribution.
  • Reason this comment was not posted:
    Confidence changes required: 10% <= threshold 50% None
8. scripts/changelog_contributors_twitter.csv:1
  • Draft comment:
    Username updated from 'brayo-pip' to '0xbrayo' is consistent with recent naming changes.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
9. scripts/package/README.txt:1
  • Draft comment:
    Documentation update clarifies module placement; the instructions now reflect the switch from aw-qt to aw-tauri.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
10. scripts/package/aw-tauri.iss:4
  • Draft comment:
    The Inno Setup script for aw-tauri appears correctly configured; verify the GUID and icon paths remain valid.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
11. scripts/package/build_app_tauri.sh:37
  • Draft comment:
    The TAURI macOS app bundle script correctly copies the aw-tauri binary and resources; ensure binary names and file paths match the new structure.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
12. scripts/package/move-to-aw-modules.sh:3
  • Draft comment:
    The rsync excludes for aw-tauri (and others) are correctly applied; this should ensure proper module placement for tauri builds.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
13. scripts/package/package-all.sh:45
  • Draft comment:
    The conditional copy of README.txt and move-to-aw-modules.sh for Linux TAURI builds is correctly added; verify this meets your deployment needs.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
14. .github/workflows/build-tauri.yml:19
  • Draft comment:
    Typo: "Wether" should be "Whether" in the comment.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% While this is technically correct - there is a typo in the comment - fixing typos in comments is generally not important enough to warrant a PR comment. The meaning is still clear despite the typo. This seems like an overly pedantic comment that doesn't improve code quality in any meaningful way. The typo could be seen as unprofessional and might be worth fixing since this is a new file being added. Documentation quality does matter. While documentation quality matters, this is just a minor typo in an internal comment that doesn't affect functionality. The comment rules specifically say not to make purely informative comments or unimportant changes. This comment should be deleted as it points out a minor typo that doesn't meaningfully impact code quality or understanding.
15. scripts/package/README.txt:3
  • Draft comment:
    Typographical error on this line: "... If you decide on the appimage copy it a permanent folder like ~/bin or /usr/local/bin, since autostart relies on the appimage being in the same location each time." Suggested fixes: - Insert 'to' after 'copy it' so it reads 'copy it to a permanent folder'. - Capitalize 'linux' to 'Linux' for consistency.
  • 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% Since this is a new documentation file, clarity and correctness are important. The suggested fixes do improve readability and follow standard conventions (capitalizing Linux). However, these are very minor issues that don't impact understanding. The rules say not to make purely informative comments or obvious/unimportant changes. Documentation quality is important for user experience. Poor grammar could reflect badly on the project. While documentation quality matters, these changes are too minor to warrant a PR comment. They don't significantly impact understanding. Delete this comment as it suggests only minor text changes that don't meaningfully improve the documentation's clarity or usefulness.
16. scripts/package/aw-tauri.iss:18
  • Draft comment:
    Typographical error: The AppId line appears to have mismatched braces. It currently reads: AppId={{983D0855-08C8-46BD-AEFB-3924581C6703} It likely should have a closing brace, e.g.: AppId={{983D0855-08C8-46BD-AEFB-3924581C6703}}
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The comment suggests adding another closing brace, but the file itself contains a comment explaining that the double {{ is intentional for escaping. This is a new file for the Tauri build system, and the syntax appears to be correct for Inno Setup. The comment is making an incorrect assumption about the syntax. Could there be a case where double braces are needed on both sides? Maybe I should verify Inno Setup's documentation. The inline comment in the code explicitly explains the syntax, and since this is a generated script from the Inno Setup Wizard, we can trust that the syntax is correct. The comment should be deleted as it incorrectly suggests fixing something that is actually correct syntax for Inno Setup scripts.

Workflow ID: wflow_m4qJXnxPcN42rEYJ

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@0xbrayo 0xbrayo requested a review from ErikBjare August 8, 2025 07:57
@0xbrayo 0xbrayo marked this pull request as draft August 16, 2025 11:45
@ErikBjare
Copy link
Member

@TimeToBuildBob take over this PR (open a new one that you can push to), read through the conversation and review comments carefully and address the remaining issues. End result should be CI building both aw-qt and aw-tauri versions.

@TimeToBuildBob
Copy link
Contributor

On it! I'll fork ActivityWatch/activitywatch and open a fresh PR that:

  1. Keeps existing aw-qt CI builds intact
  2. Adds a separate CI workflow/job for aw-tauri builds
  3. Addresses the remaining review comments (Makefile ICON conditional, move-to-aw-modules.sh rsync flags, aw-tauri.iss AppVerName collision with aw-qt, README.txt typos)

Will start in the next session with a full worktree setup.

@TimeToBuildBob
Copy link
Contributor

Took over as requested by @ErikBjare. New clean PR with all review comments addressed: #1208

Changes:

  • aw-tauri.iss: app name → "ActivityWatch Tauri" (prevents collision with aw-qt installer)
  • Makefile: aw-qt icon dependency is now conditional on TAURI_BUILD
  • move-to-aw-modules.sh: consistent rsync -a flags in both Wayland/X11 paths
  • README.txt: fixed typos ("copy it to", "AppImage", "Linux" capitalized)
  • build-tauri.yml, build.yml: fixed typo "Wether" → "Whether"

TimeToBuildBob added a commit to TimeToBuildBob/activitywatch that referenced this pull request Feb 20, 2026
Add a separate build-tauri.yml CI workflow that builds aw-tauri alongside
the existing aw-qt builds. The two build pipelines coexist independently.

Makefile changes:
- TAURI_BUILD=true flag switches between aw-qt and aw-tauri submodules
- aw-server-rust builds only aw-sync in Tauri mode (not full server)
- ICON points to aw-tauri icons when TAURI_BUILD is set
- Conditional packaging: aw-tauri app bundle on macOS, Inno Setup on Windows

Packaging:
- build_app_tauri.sh: macOS .app bundle creator for Tauri
- aw-tauri.iss: Windows Inno Setup with different AppId/AppName to avoid
  colliding with existing aw-qt installations (addresses review comment)
- move-to-aw-modules.sh: Linux module discovery helper (rsync -a, proper
  line continuation)
- README.txt: Linux end-user instructions (typos fixed)
- package-all.sh: Tauri-aware zip/installer selection

Build-tauri.yml features:
- Multi-OS: ubuntu-24.04, ubuntu-24.04-arm, windows-latest, macOS-13,
  macos-latest
- Retry wrapper (nick-fields/retry) for flaky builds
- Separate artifact names (builds-tauri-*) to not collide with aw-qt
- Tauri-specific APT deps (libgtk-3-dev, libwebkit2gtk-4.1-dev, etc.)

Also updates contributor CSV files (brayo-pip → 0xbrayo).

Based on work by @0xbrayo in PR ActivityWatch#1163.
@TimeToBuildBob
Copy link
Contributor

Superseded by #1209.

I've taken over this PR as requested by @ErikBjare. The new PR applies the changes cleanly on current master and addresses all outstanding review comments:

  • Windows installer collision: Different AppId, AppName="ActivityWatch (Tauri)", and separate install directory
  • Makefile ICON: Now correctly points to aw-tauri icons for Tauri builds
  • rsync issues: Proper line continuation and rsync -a consistently
  • Typos: Fixed in README.txt and CI workflow
  • Submodule: Uses upstream awatcher (not fork)

Thank you @0xbrayo for the original work on this — the core approach is preserved, just cleaned up and rebased.

@ErikBjare ErikBjare closed this Feb 22, 2026
ErikBjare pushed a commit that referenced this pull request Feb 22, 2026
* ci(build): modernize runners, node, cache keys for aw-qt CI

- Ubuntu 22.04 → 24.04, add macos-latest alongside macOS-13
- Node 20 → 22
- Fix macOS Python setup condition (matrix.os instead of runner.os)
- Use matrix.os in cache keys for unique per-variant caching
- Enable cargo cache on macOS (was disabled)
- Fix qt5-default → qt5-qmake + qtbase5-dev (ubuntu-24.04)
- Fix artifact upload name collision (matrix.os)
- Fix typo: 'Wether' → 'Whether'

* feat: add aw-tauri and awatcher submodules

aw-tauri is the Tauri-based replacement for aw-qt (system tray).
awatcher is used on Linux for Wayland-compatible window watching.

* feat: add aw-tauri CI workflow, Makefile support, and packaging

Add a separate build-tauri.yml CI workflow that builds aw-tauri alongside
the existing aw-qt builds. The two build pipelines coexist independently.

Makefile changes:
- TAURI_BUILD=true flag switches between aw-qt and aw-tauri submodules
- aw-server-rust builds only aw-sync in Tauri mode (not full server)
- ICON points to aw-tauri icons when TAURI_BUILD is set
- Conditional packaging: aw-tauri app bundle on macOS, Inno Setup on Windows

Packaging:
- build_app_tauri.sh: macOS .app bundle creator for Tauri
- aw-tauri.iss: Windows Inno Setup with different AppId/AppName to avoid
  colliding with existing aw-qt installations (addresses review comment)
- move-to-aw-modules.sh: Linux module discovery helper (rsync -a, proper
  line continuation)
- README.txt: Linux end-user instructions (typos fixed)
- package-all.sh: Tauri-aware zip/installer selection

Build-tauri.yml features:
- Multi-OS: ubuntu-24.04, ubuntu-24.04-arm, windows-latest, macOS-13,
  macos-latest
- Retry wrapper (nick-fields/retry) for flaky builds
- Separate artifact names (builds-tauri-*) to not collide with aw-qt
- Tauri-specific APT deps (libgtk-3-dev, libwebkit2gtk-4.1-dev, etc.)

Also updates contributor CSV files (brayo-pip → 0xbrayo).

Based on work by @0xbrayo in PR #1163.

* ci: fix macOS-13 runner label casing (macOS-13 -> macos-13)

GitHub Actions runner labels are case-sensitive. `macOS-13` (capital OS)
is not a valid runner label and causes CI failures. The correct label is
`macos-13` (all lowercase). Fix this in both build.yml and build-tauri.yml.

* ci: replace deprecated macos-13 with macos-14 runner

macos-13 (x86_64) is deprecated by GitHub Actions. Replace with macos-14
(ARM64/Apple Silicon) in both build.yml and build-tauri.yml. Also remove
the macos-13-specific Python workaround (custom pkg install for macOS 10.9
compatibility) since macos-14 uses standard actions/setup-python.

* ci: upgrade Poetry from 1.3.2 to 1.4.2 to fix Windows race condition

Poetry 1.3.x has a known Windows file-locking race condition when
installing packages (tilde-prefixed dist-info temp files cause OSError).
Version 1.4.0+ fixes this. Using 1.4.2 (last release in 1.4.x series)
to stay on same lockfile format as 1.3.x.

Fixes: Windows CI failure in jobs that run 'poetry install'

* ci: cache node_modules and aw-tauri cargo target to reduce build times

- Cache node_modules directly instead of npm global cache. The global
  cache only saves download time, but npm ci still does a full install
  into node_modules every run (~5-7 min per npm ci call). Caching
  node_modules directly skips this entirely on cache hit.

- Cache aw-tauri/src-tauri/target alongside aw-server-rust/target in
  the Tauri workflow. The Tauri Cargo release build takes ~8 min
  uncached and was not being cached previously.

- Also cache aw-tauri/node_modules in the Tauri workflow (has its own
  package-lock.json separate from aw-webui).

Expected improvement: ~15-20 min savings on cached Windows Tauri builds
(from ~34 min to ~15 min).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants