fix: read port from server config instead of hardcoding 5600#122
fix: read port from server config instead of hardcoding 5600#122ErikBjare merged 4 commits intoActivityWatch:masterfrom
Conversation
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 2c498cf in 10 seconds. Click for details.
- Reviewed
104lines of code in3files - Skipped
0files when reviewing. - Skipped posting
0draft 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.
Workflow ID: wflow_e0Zla4YJDPj4l74g
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Greptile SummaryThis PR successfully fixes the hardcoded port issue by reading the port configuration from server config files. The implementation cleanly refactors port reading into separate helper functions with proper priority (aw-server-rust → aw-server → defaults) and includes appropriate error handling and fallbacks.
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[aw-qt starts] --> B[AwQtSettings.__init__]
B --> C[_read_server_port]
C --> D{Check aw-server-rust config}
D -->|port found| E[Return rust port]
D -->|not found| F{Check aw-server config}
F -->|port found| G[Return Python port]
F -->|not found| H[Return default<br/>5600 or 5666]
E --> I[config.port set]
G --> I
H --> I
I --> J[main.py passes port to trayicon.run]
J --> K[TrayIcon.__init__ receives port]
K --> L[Constructs root_url with port]
L --> M[Open Dashboard uses correct URL]
Last reviewed commit: a0cbfda |
|
Only reads aw-server-rust config. Servers should probably manage these defaults themselves (with --testing support), not have aw-qt reading server configs. |
|
Fair point. The ideal approach would be for aw-server-rust to write a runtime port file on startup (e.g. However, that requires changes to aw-server-rust, and this PR at least fixes the concrete problem (hardcoded 5600) with a reasonable fallback chain. Should I:
Option 2 is cleaner architecturally but requires cross-repo coordination. Option 1 works today and can be replaced later. |
|
Oh, I misunderstood when I wrote:
I thought this was setting explicit Given my misunderstanding, complete this PR (should support both aw-server and aw-server-rust). |
|
@TimeToBuildBob Get going |
Reads the port from aw-server-rust's config.toml (or config-testing.toml) so that users running on a non-default port get the correct dashboard URL when clicking "Open Dashboard" in the tray icon. Falls back to the defaults (5600/5666) if the config file doesn't exist or can't be parsed. Fixes ActivityWatch#79
Extends port detection to support both aw-server and aw-server-rust: - aw-server-rust: ~/.config/activitywatch/aw-server-rust/config.toml - aw-server: ~/.config/activitywatch/aw-server/aw-server.toml ([server]/[server-testing] sections) Tries aw-server-rust first, falls back to aw-server, then to hardcoded defaults.
2c498cf to
136dcfb
Compare
|
Done — extended to support both server types:
Priority is aw-server-rust → aw-server → default. Also rebased onto master. |
|
@greptileai review |
Summary
Fixes #79
Changes
config.py: Refactored into_read_server_rust_port()and_read_aw_server_port()helpers, coordinated by_read_server_port(); exposed asAwQtSettings.port~/.config/activitywatch/aw-server-rust/config.toml(orconfig-testing.toml)~/.config/activitywatch/aw-server/aw-server.toml([server]or[server-testing]section)main.py: Passesconfig.porttotrayicon.run()trayicon.py: Acceptsportparameter instead of hardcoding the valueTest plan
port = 5700in~/.config/activitywatch/aw-server-rust/config.tomland verify tray icon opens correct URLport = 5700in[server]section of~/.config/activitywatch/aw-server/aw-server.tomland verify tray icon opens correct URL