feat(tray): persist module start/stop to autostart_modules

[ErikBjare]

A module started or stopped from the tray now stays that way after a restart. The click adds the module to, or removes it from, autostart_modules in aw-qt.toml. Until now Module.toggle() only changed the running state, and the only way to make it stick was to hand-edit the TOML.

This mirrors ActivityWatch/aw-tauri#285, so both apps behave the same in v0.14.1. A tray toggle means "on or off from now on". To stop everything for now, quit ActivityWatch. There is no pause state.

Behaviour

• Only explicit menu clicks persist. The click goes through TrayIcon._on_module_clicked, which toggles the module and then saves the new state. Crash auto-restarts, the "Restart" button in the crash dialog, and shutdown call Module.start/stop directly and never write the config. A start that raises (e.g. exec failure) is not saved.
• The server is never persisted. The Modules menu lists every discovered module, including aw-server/aw-server-rust. Toggling the server stays useful (restarting a crashed server, switching to an external one), so I kept those menu items and skip writing them instead of hiding them. Stopping the server from the tray therefore lasts only for this run, and the next start always has a server.
• A shadowed system copy is not persisted. When a module is found both bundled and on PATH, the menu lists both, but autostart_modules holds only names and autostart runs the bundled copy. A click on the bundled copy, or on a system module with no bundled twin, is saved. A click on a system copy that a bundled one shadows only changes what is running, since saving it would start the other copy next time.
• --autostart-modules disables persisting. That run isn't using the configured list, so toggles only change what is running.
• Right file, right section. The write targets the same file and section AwQtSettings reads for the profile: [aw-qt] in the profile's aw-qt.toml, [aw-qt-testing] in the legacy shared-root testing layout, and [aw-qt-<profile>] in the shared root for a named profile that still uses the pre-isolation section.
• Edited in place. The file is re-read from disk right before each write, so edits made while aw-qt runs are kept, and it is edited with tomlkit, so comments and layout are kept. It is read and written as UTF-8, and line endings are kept. A file that fails to decode or parse, or whose list isn't a list of strings, is left untouched, and the failure is logged. Writes go through a temp file + rename and follow a symlinked config. Errors from saving are caught in the tray slot, because an exception escaping a PyQt6 slot aborts the app.
• Stable order. Enable appends, disable removes every occurrence, and nothing is duplicated. If the section doesn't set the key, as in the commented-out file aw-qt writes on first run, the effective default list is written with the change applied. The commented-out defaults stay in place for reference.

Module.toggle() now returns whether the module is meant to be running. Manager.resolve() is the bundled-first lookup that Manager.start() already used.

Test isolation fix (separate commit)

The xdg_tmp fixture in tests/test_profile_config.py only sets XDG_*_HOME, which platformdirs ignores on macOS and Windows. Running the suite there wrote fixture files into the real ActivityWatch config dirs, overwriting aw-qt.toml, aw-server.toml and aw-server-rust/config*.toml. A new autouse fixture in tests/conftest.py points platformdirs into tmp_path on every platform. Its layout matches the XDG dirs, so existing tests are unchanged.

Tests

tests/test_persist_toggles.py covers:

• comments and layout preserved, append/remove order, no duplicates, no-op when already in state
• the first-run commented-out file
• re-reading before each write
• the server guard, both directions, with the file not even rewritten
• a malformed file left untouched (and no temp file left behind)
• a symlinked config, UTF-8 content with CRLF line endings, and an undecodable file
• every profile layout (default, isolated testing, legacy testing, named isolated, named pre-isolation), round-tripped through AwQtSettings
• the tray click path, including the CLI-override guard, a failed start, a shadowed system copy, and a persist error that must not escape the slot
• main() passing persist_toggles=False with --autostart-modules

Locally (macOS, Python 3.9): pytest tests (166 passed), make typecheck and make test pass.

Note that CI runs make test (an import check) and make typecheck, but not pytest, so the unit tests in tests/ aren't run in CI today.

[ErikBjare]

@greptileai review

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-07T10:40:22.852613Z	b028ca4	New commits

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 46e073b964

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[greptile-apps]

Confidence Score: 5/5

[Medium risk] Saves tray module toggles to the autostart configuration.

The PR appears safe to merge; no new blocking issue was found.

What we checked:

• Tray saves the wrong copy: Manager.resolve selects the bundled copy first. The tray skips saving when that choice differs from the clicked module.

Summary

Tray clicks save module choices to autostart_modules, except for servers, CLI overrides, and copies that autostart would not select.

• The latest changes use UTF-8, keep line endings, and log unexpected save errors.
• Both prior unnumbered findings are fixed: unreadable files are left untouched, and the symlink test skips when symlinks cannot be created.
• No new actionable issues were found.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Tray module click] --> B[Toggle selected module]
  B --> C{Persistence enabled?}
  C -->|No| D[Keep change for this run]
  C -->|Yes| E{Autostart selects this copy?}
  E -->|No| D
  E -->|Yes| F{Server module?}
  F -->|Yes| D
  F -->|No| G[Read current config as UTF-8]
  G --> H[Update module list]
  H --> I[Write temporary file and replace target]

Loading

_{Reviews (2) · Last reviewed commit: "fix(tray): address review: UTF-8 I/O, sh..." · Reviewed by Greptile}

[ErikBjare]

@greptileai review

[TimeToBuildBob]

🤖 AI code review

This PR makes tray start/stop of modules persist to autostart_modules in aw-qt.toml. It adds set_module_autostart and persist_module_autostart functions in config.py, with atomic write and profile-aware location resolution. It changes Module.toggle to return the new state, adds Manager.resolve, and wires TrayIcon._on_module_clicked to persist toggles unless --autostart-modules overrides. It adds tests for config editing, persistence, profile locations, tray click behavior, and main wiring.

Not safe to merge — 1 P1 open

Confidence 3/5

2 findings · ❌ 1 P1 · ⚠️ 1 P2

❌ P1 high — tests/conftest.py:28

In tests/conftest.py, the fixture _isolated_platform_dirs monkeypatches platformdirs.user_config_dir etc. However, aw_core.dirs may have already imported and cached the original functions. The fixture patches platformdirs attributes, but aw_core.dirs might have done from platformdirs import user_config_dir at import time, so the patch won't affect the already-bound reference. This means the tests may not actually isolate the config dir, and could write to the real user config. The test suite would then be non-hermetic and could overwrite the developer's real aw-qt.toml. This is a test defect. The fix is to patch aw_core.dirs functions directly, or ensure aw_core.dirs uses the platformdirs functions at call time (which it likely does, but need to verify).

Instead of patching platformdirs, patch aw_core.dirs.get_config_dir etc. or set XDG_* env vars and ensure platformdirs reads them at call time.

_{How this was verified: Checked aw_core.dirs source (not provided) but typical implementation does from platformdirs import user_config_dir at module level, so patching platformdirs after import won't affect the reference. This is a common pitfall.}

_{Consensus: 3/3 passes agreed}
Distinct keys: 1 (general)

⚠️ P2 medium — aw_qt/config.py:416

In _atomic_write, the temp file is created with tempfile.mkstemp, which creates the file with mode 0o600. After writing, the code does os.chmod(tmp, os.stat(target).st_mode & 0o777) only if os.path.exists(target). If the target does not exist (first run, or config file was deleted), the temp file remains with mode 0o600, so the new config file is created with restrictive permissions (owner-only read/write). This is a behavior change from a normal open/write which would respect umask and typically create 0o644. The consequence: the config file may be unreadable by other processes or users, and on some systems the tray or other components running under a different user could fail to read it. Also, if the file is later read by a service running as another user, it would fail. The fix is to apply a default mode (e.g., 0o666 & ~umask) when target doesn't exist, or use os.open with mode. This is a real bug in normal use when the config file doesn't exist yet (e.g., first run after upgrade).

if os.path.exists(target):
            os.chmod(tmp, os.stat(target).st_mode & 0o777)
        else:
            os.chmod(tmp, 0o644 & ~umask)  # or just 0o644

_{How this was verified: Checked persist_module_autostart: except FileNotFoundError sets source="", then set_module_autostart writes, then _atomic_write on line 408 uses tempfile.mkstemp(dir=os.path.dirname(target)). If dirname doesn't exist, mkstemp raises FileNotFoundError, caught on line 389 as OSError and returns False. There is no os.makedirs call anywhere in the new code.}

_{Consensus: 3/3 passes agreed}
Distinct keys: 1 (general)

1 advisory finding (summary-only, not scored)

These P2 guard, heuristic, trade-off, or documentation claims are retained for judgment without opening review threads.

⚠️ P2 medium — aw_qt/config.py:277

In _autostart_location, for a named profile, it checks if _raw_toml_section(path, "aw-qt") is None. If the isolated file has no uncommented [aw-qt] section, it looks for a shared section. However, if the isolated file exists but has a commented-out [aw-qt] (like the first-run file), _raw_toml_section returns None because the section has no uncommented keys. Then it checks the shared root. If the shared root has a [aw-qt-<profile>] section, it returns that. This means that a named profile that has an isolated file with only commented-out defaults will write to the shared root's pre-isolation section, which may be unexpected. But this mirrors AwQtSettings behavior: if the isolated file has no uncommented [aw-qt], it uses the shared section. So it's consistent. However, if the user has a named profile and the isolated file exists with commented-out defaults, and the shared root also has a [aw-qt-<profile>] section, the toggle will write to the shared file, not the isolated one. This could be surprising but is consistent with how settings are read. Not a bug.

_{How this was verified: Traced _default_autostart_modules: defaults = tomlkit.parse(default_config), table = defaults.get(section, defaults["aw-qt"]). default_config has only [aw-qt] and [aw-qt-testing]. For a named profile section like "aw-qt-research", defaults.get returns None and falls back to [aw-qt] defaults, which may not match the profile's configured list. But AwQtSettings has already read the effective list, so writing defaults that differ is a divergence only in corner cases.}

_{Consensus: 3/3 passes agreed}
Distinct keys: 1 (general)

Files changed (6) — the diff as I read it

• aw_qt/config.py — Adds SERVER_MODULES, _autostart_location, _default_autostart_modules, set_module_autostart, persist_module_autostart, and _atomic_write for editing autostart_modules in TOML.
• aw_qt/main.py — Computes autostart_overridden from --autostart-modules and passes persist_toggles=not autostart_overridden to trayicon.run.
• aw_qt/manager.py — Changes Module.toggle to return self.started, adds Manager.resolve, and refactors Manager.start to use resolve.
• aw_qt/trayicon.py — Adds persist_toggles parameter, _on_module_clicked handler that toggles and persists, and passes persist_toggles through run to TrayIcon.
• tests/conftest.py — Adds autouse fixture that redirects platformdirs user_*_dir and user_cache_path to a temp directory.
• tests/test_persist_toggles.py — Adds tests for set_module_autostart, persist_module_autostart, profile location selection, tray click persistence, and main wiring.

_{Reviewed b028ca4793c0 · openrouter/deepseek/deepseek-v4-flash-0731 · llm engine · 496s · about this reviewer}

_{Maintainer commands}

_{@TimeToBuildBob review (own line) — fresh review · @TimeToBuildBob fix — a worker acts on the findings. Once per comment; 👀 = received.}

[TimeToBuildBob]

❌ P1 — In tests/conftest.py, the fixture _isolated_platform_dirs monkeypatches platformdirs.user_config_dir etc. However, aw_core.dirs may have already imported and cached the original functions. The fixture patches platformdirs attributes, but aw_core.dirs might have done from platformdirs import user_config_dir at import time, so the patch won't affect the already-bound reference. This means the tests may not actually isolate the config dir, and could write to the real user config. The test suite would then be non-hermetic and could overwrite the developer's real aw-qt.toml. This is a test defect. The fix is to patch aw_core.dirs functions directly, or ensure aw_core.dirs uses the platformdirs functions at call time (which it likely does, but need to verify). (3/3 passes)

[TimeToBuildBob]

⚠️ P2 — In _atomic_write, the temp file is created with tempfile.mkstemp, which creates the file with mode 0o600. After writing, the code does os.chmod(tmp, os.stat(target).st_mode & 0o777) only if os.path.exists(target). If the target does not exist (first run, or config file was deleted), the temp file remains with mode 0o600, so the new config file is created with restrictive permissions (owner-only read/write). This is a behavior change from a normal open/write which would respect umask and typically create 0o644. The consequence: the config file may be unreadable by other processes or users, and on some systems the tray or other components running under a different user could fail to read it. Also, if the file is later read by a service running as another user, it would fail. The fix is to apply a default mode (e.g., 0o666 & ~umask) when target doesn't exist, or use os.open with mode. This is a real bug in normal use when the config file doesn't exist yet (e.g., first run after upgrade). (3/3 passes)