Port to Vite and Typescript & Manifest V3

[BelKed]

Based on work by @saghen in #103

---

I've fixed some bugs, cleaned up redundant code, and added GitHub action for linting.
Manually tested on both Firefox and Chrome, works flawlessly :)
We'll have to wait for proper support for Manifest V3 in Firefox, for now it's the Firefox extension built with Manifest V2.

---

• Closes Improved code #81
• Closes build(deps): bump minimatch from 3.0.4 to 3.1.2 #96
• Closes fix: added srczip target, now using aw-client-js from npm, updated deps #98
• Closes chore: add prettier and format project #100
• Closes Port to Vite and Typescript #103
• Closes build(deps): bump browserify-sign from 4.2.1 to 4.2.2 #119
• Closes fixed typo in README.md #144
• Closes Rewrite the Privacy Notice #151

---

Important

Refactor aw-watcher-web to use Vite, TypeScript, and Manifest V3, with updated build processes and improved code structure.

• Build System:
  • Migrated to Vite for build process (vite.config.ts).
  • Added GitHub Actions for build (build.yml), linting (lint.yml), and CodeQL analysis (codeql.yml).
  • Updated Makefile for new build commands build-chrome and build-firefox.
• Codebase:
  • Converted JavaScript to TypeScript in src/background, src/consent, and src/popup.
  • Removed aw-client-js submodule and replaced with aw-client package.
  • Added TypeScript types and configuration (vite-env.d.ts, tsconfig.json).
• Manifest and Configuration:
  • Updated to Manifest V3 for Chrome and maintained Manifest V2 for Firefox (src/manifest.json).
  • Added vite-plugin-web-extension for manifest generation.
• Miscellaneous:
  • Added .prettierignore and updated .editorconfig for consistent formatting.
  • Updated README.md with new build instructions.
  • Removed redundant files and code, such as old static files and eventPage.js.

^{This description was created by for c262196. It will automatically update as commits are pushed.}

[ellipsis-dev]

❌ Changes requested. Reviewed everything up to 70aa760 in 2 minutes and 14 seconds

More details

• Looked at 1944 lines of code in 33 files
• Skipped 3 files when reviewing.
• Skipped posting 9 drafted comments based on config settings.

1. Makefile:78

• Draft comment:
  Test reproducibility for Firefox uses a different zip path than Chrome. Please verify that 'aw-watcher-web.zip' is the correct artifact reference and aligns with the output of build-firefox.
• Reason this comment was not posted:
  Comment did not seem useful.

2. src/background/client.ts:44

• Draft comment:
  Consider handling the promise returned by ensureBucket more explicitly in onFailedAttempt; the empty .then(() => {}) may hide errors.
• Reason this comment was not posted:
  Confidence changes required: 50%
  None

3. src/background/main.ts:56

• Draft comment:
  Chained .then calls could be refactored to use async/await for better readability and error handling.
• Reason this comment was not posted:
  Confidence changes required: 40%
  None

4. src/storage.ts:15

• Draft comment:
  Consider defining and documenting interfaces for storage keys to improve type safety and maintainability.
• Reason this comment was not posted:
  Confidence changes required: 40%
  None

5. src/manifest.json:21

• Draft comment:
  Verify that the use of templated keys (e.g. '{{chrome}}.manifest_version') is compatible with all target browsers and tooling. Document this convention if not already.
• Reason this comment was not posted:
  Confidence changes required: 50%
  None

6. vite.config.ts:12

• Draft comment:
  Loading the web extension config with require() in a try/catch is fine; consider logging a warning when config is undefined for easier debugging.
• Reason this comment was not posted:
  Confidence changes required: 30%
  None

7. Makefile:79

• Draft comment:
  Inconsistent artifact naming in the reproducibility test for Firefox. It compares 'aw-watcher-web.zip' instead of the expected artifact (like artifacts/firefox.zip).
• Reason this comment was not posted:
  Marked as duplicate.

8. src/consent/main.ts:6

• Draft comment:
  Calling browser.management.uninstallSelf() without error handling may fail on browsers where the API is unavailable. Consider adding a fallback or proper error handling.
• Reason this comment was not posted:
  Confidence changes required: 50%
  None

9. src/storage.ts:31

• Draft comment:
  Consider providing explicit default values for sync status keys when they are not set, to avoid ambiguity in type conversion.
• Reason this comment was not posted:
  Confidence changes required: 50%
  None

Workflow ID: wflow_yczkyCIZDrMxrLjV

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Reviewed everything up to 9c6d835 in 2 minutes and 21 seconds

More details

• Looked at 1972 lines of code in 34 files
• Skipped 3 files when reviewing.
• Skipped posting 6 drafted comments based on config settings.

1. src/background/helpers.ts:20

• Draft comment:
  Consider verifying the icon URL path. Manifest uses 'logo-128.png' at root, but helper uses 'media/logo/logo-128.png'.
• Reason this comment was not posted:
  Comment did not seem useful.

2. vite.config.ts:37

• Draft comment:
  Use import.meta.env.VITE_TARGET_BROWSER for consistency with config.ts instead of process.env.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  While the comment suggests a specific change, I can't verify if there's actually an inconsistency with config.ts since I don't have access to that file. In Vite, both process.env and import.meta.env can be valid ways to access environment variables. Without seeing config.ts, I can't be certain this change is necessary or beneficial.
  I might be too cautious - consistency in environment variable access across a codebase is generally good practice. The reviewer might have valid knowledge about the codebase standards.
  However, without being able to verify the claimed inconsistency in config.ts, keeping this comment would violate the principle of needing strong evidence that the comment is correct.
  Delete the comment since we lack the necessary context to verify if there's actually an inconsistency with config.ts.

3. .github/workflows/codeql.yml:37

• Draft comment:
  The CodeQL job manually runs 'make build-chrome' instead of using CodeQL's autobuild. Verify that this manual build correctly includes all TS/JS sources for analysis, or consider using the official autobuild action.
• Reason this comment was not posted:
  Comment did not seem useful.

4. Makefile:78

• Draft comment:
  The test-reproducibility-firefox target uses file paths that seem inconsistent (e.g. 'aw-watcher-web.zip' vs 'build/aw-watcher-web/aw-watcher-web.zip'). Please verify and correct these paths to ensure the reproducibility check compares the correct artifacts.
• Reason this comment was not posted:
  Marked as duplicate.

5. src/storage.ts:28

• Draft comment:
  The ternary operator in getSyncStatus returns undefined if 'lastSyncSuccess' is undefined. Consider defaulting it explicitly (e.g. to false) to avoid unexpected undefined values in downstream consumers.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The code seems to intentionally handle undefined as a valid state throughout - the type definition, the getter, and the watcher all consistently treat undefined as a possible value. This suggests the undefined case is a valid business logic state, perhaps indicating "never synced" vs failed/successful sync. Changing this behavior could break existing code relying on this distinction.
  I might be over-interpreting the intentionality of the undefined handling. The code could be accidentally inconsistent.
  The consistent handling of undefined across multiple related functions and type definitions strongly suggests this is intentional design rather than an oversight.
  The comment should be deleted as it suggests changing what appears to be intentional undefined handling that is consistently implemented across the codebase.

6. src/manifest.json:2

• Draft comment:
  The manifest uses templated keys like '{{chrome}}.manifest_version'. Ensure that the build process (via vite-plugin-web-extension) correctly transforms these keys; otherwise, browsers might reject the manifest.
• Reason this comment was not posted:
  Comment did not seem useful.

Workflow ID: wflow_UBZYG8qCJlIYYK8O

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Reviewed everything up to c262196 in 1 minute and 21 seconds

More details

• Looked at 1966 lines of code in 34 files
• Skipped 3 files when reviewing.
• Skipped posting 14 drafted comments based on config settings.

1. Makefile:79

• Draft comment:
  Inconsistent zip artifact naming for Firefox reproducibility test.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

2. src/manifest.json:8

• Draft comment:
  Icon path may be incorrect.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The comment is speculative ("may be incorrect") rather than definitive. It doesn't provide any specific reason why the path might be wrong or what the correct path should be. The path is used consistently throughout the file, suggesting it was intentionally set this way. Without additional context showing the actual file structure or evidence of a problem, this seems like an unnecessary speculation.
  The icon path could potentially be wrong if the logo-128.png file doesn't exist in the expected location relative to the manifest.
  Even if that were true, this would be caught immediately during build/testing, and the comment doesn't provide any concrete evidence of an actual problem.
  The comment should be deleted as it's speculative, lacks evidence of an actual problem, and any real path issues would be caught by the build process.

3. vite.config.ts:37

• Draft comment:
  Environment variable usage inconsistency.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The comment is vague and doesn't explain what the inconsistency is. Since this is the only env var used in the file, any inconsistency would have to be with other files we can't see. We're told to ignore cross-file issues. The usage of VITE_TARGET_BROWSER seems standard for Vite.
  There could be inconsistencies in how environment variables are accessed across the codebase that would make this a valid comment.
  We are explicitly told to ignore cross-file issues and only consider the current file. Within this file alone, there is no inconsistency to fix.
  The comment should be deleted as it's vague, doesn't point to a clear issue within this file, and any potential issues would require cross-file context we're told to ignore.

4. .editorconfig:10

• Draft comment:
  Verify inclusion of TS and YML in the indentation rule is intentional.
• Reason this comment was not posted:
  Confidence changes required: 33%
  None

5. .github/workflows/build.yml:20

• Draft comment:
  Using Node.js 23.x could be ahead of the LTS; ensure dependencies work with this version.
• Reason this comment was not posted:
  Confidence changes required: 33%
  None

6. .github/workflows/codeql.yml:9

• Draft comment:
  Quote the cron expression to ensure YAML parses it correctly.
• Reason this comment was not posted:
  Confidence changes required: 33%
  None

7. Makefile:78

• Draft comment:
  Reproducibility test for Firefox uses different filename patterns than Chrome; verify artifact naming consistency.
• Reason this comment was not posted:
  Marked as duplicate.

8. src/background/client.ts:23

• Draft comment:
  Using 'forever: true' in retry could cause indefinite hangs if the bucket creation repeatedly fails; consider capping retries or adding exponential backoff.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  The comment raises a valid concern about potential indefinite hangs. However, this is a bucket creation operation that's fundamental to the application working - if it fails, nothing else can work. The 500ms minimum timeout prevents too aggressive retrying. The forever retry makes sense here as a design choice since the app can't function without a bucket.
  The comment might be right that indefinite retries could cause issues in some edge cases. What if the server is permanently down or misconfigured?
  In this case, indefinite retries are actually the desired behavior - the app needs the bucket to function, and users would want it to keep trying rather than give up. The notification system will inform users of connection issues.
  The forever retry appears to be an intentional design choice here, not a bug. The comment should be removed as it suggests changing what seems to be desired behavior.

9. src/background/main.ts:25

• Draft comment:
  The onInstalled event chain lacks error handling; adding a catch may help log or resolve initialization errors.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable:
  Error handling could be useful here since this is initialization code. However, the operations are mostly browser API calls and storage operations which are unlikely to fail catastrophically. The code already has logging. If something fails, the browser's built-in error handling will catch it and log to the console. Adding try/catch would mostly duplicate that functionality.
  I might be underestimating the importance of explicit error handling during extension initialization. A failed initialization could leave the extension in an inconsistent state.
  While initialization errors are important, the browser extension framework already provides error handling and logging. Adding explicit try/catch would add complexity without providing significant benefits.
  The comment should be deleted as it suggests adding complexity without clear benefits, given the browser's built-in error handling.

10. src/consent/index.html:44

• Draft comment:
  Ensure that 'main.ts' is compiled to 'main.js' and the script URL is correct in the production build.
• Reason this comment was not posted:
  Confidence changes required: 33%
  None

11. src/consent/main.ts:6

• Draft comment:
  browser.management.uninstallSelf may require explicit permission; ensure the manifest declares necessary management permissions.
• Reason this comment was not posted:
  Confidence changes required: 33%
  None

12. src/manifest.json:1

• Draft comment:
  Manifest uses templating keys (e.g. '{{chrome}}.manifest_version'); confirm your build process correctly transforms these for each target browser.
• Reason this comment was not posted:
  Comment did not seem useful.

13. src/manifest.json:8

• Draft comment:
  Ensure the icon file 'logo-128.png' is present in the expected location so it loads correctly.
• Reason this comment was not posted:
  Confidence changes required: 33%
  None

14. vite.config.ts:37

• Draft comment:
  Verify that process.env.VITE_TARGET_BROWSER is set appropriately in your build environment for correct browser targeting.
• Reason this comment was not posted:
  Confidence changes required: 33%
  None

Workflow ID: wflow_LSefA3ktnDfwB2zN

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ErikBjare]

Awesome work, I accidentally merged #150 before I saw this.

Rebasing on master should hopefully be easy :)

[ErikBjare]

Very nice, rebasing all of this onto master since the git commit history looks fairly clean and nicely documents the sequence of changes.

Great job getting this across the finish line! ❤️

[ErikBjare]

I don't see such a file?

[BelKed]

No idea what that should be... It was there so I didn't change it :D
Perhaps that's why the logos are missing

[ErikBjare]

Is changing this path really correct? I thought it should be media/logo/logo-128.png

[BelKed]

I guess you're right as I'm looking at the rest of the paths :)

[ErikBjare]

I've fixed it all in master, don't worry about it :)