Conversation
- Automatic browser history detection is much better - Improve tests - MacOS menu bar support, looks like hackerbar
There was a problem hiding this comment.
Pull request overview
This PR expands HackerTray from a Linux-only tray app into a cross-platform app by adding a native macOS status bar implementation and replacing per-browser history handling with unified, multi-browser history discovery and lookup. It also modernizes and strengthens the test suite with network-blocking and fixtures.
Changes:
- Add native macOS status bar UI via PyObjC, including settings toggles and icon rendering.
- Introduce unified browser-history discovery/search across Chromium/Firefox/Safari families (multi-browser, multi-profile).
- Improve tests by adding fixtures, blocking network access by default, and adding coverage for history + macOS menu behavior.
Reviewed changes
Copilot reviewed 24 out of 26 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
uv.lock |
Locks new runtime/dev dependencies (PyObjC + pytest tooling). |
pyproject.toml |
Adds macOS-only PyObjC deps and a dev dependency group for pytest tooling. |
hackertray/__init__.py |
Refactors entrypoint: config-file defaults + OS dispatch to macOS vs Linux implementation. |
hackertray/macos.py |
New native macOS status bar app implementation using PyObjC. |
hackertray/linux.py |
Extracts Linux GTK/AppIndicator app into a dedicated module and integrates new history scanning. |
hackertray/history.py |
New cross-browser history discovery + URL lookup via SQLite. |
hackertray/hackernews.py |
Minor formatting change to URL list / module structure. |
hackertray/version.py |
Small formatting adjustments (no functional change intended). |
README.md |
Updates docs to reflect cross-platform support + automatic multi-browser history discovery. |
CHANGELOG.md |
Updates 5.0.0 notes to include macOS + improved history scanning. |
.python-version |
Pins local Python version used by tooling. |
.gitmodules |
Removes existing submodule config. |
.github/workflows/test.yml |
Expands CI matrix to macOS + multiple Python versions; updates uv setup action usage. |
.github/workflows/release.yml |
Sets checkout to not persist credentials. |
test/conftest.py |
Adds autouse pytest fixture to block all network access unless explicitly mocked. |
test/hn_test.py |
Reworks HN parsing test to use mocked urlopen + JSON fixture. |
test/version_test.py |
Reworks version test to mock PyPI JSON response deterministically. |
test/history_test.py |
Adds comprehensive tests for history discovery and URL lookup across schemas/browsers. |
test/macos_ui_test.py |
Adds macOS-only unit/integration-style tests for menu construction and actions. |
test/news_fixture.json |
Adds deterministic fixture data for HN items used by tests. |
test/safari/History.db |
Adds Safari SQLite fixture for history tests. |
test/chrome_test.py |
Removes legacy Chrome-specific history test (superseded by unified history tests). |
test/firefox_test.py |
Removes legacy Firefox-specific history test (superseded by unified history tests). |
hackertray/chrome.py |
Removes legacy Chrome history implementation (replaced by hackertray.history). |
hackertray/firefox.py |
Removes legacy Firefox history implementation (replaced by hackertray.history). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Redraw to show visited checkmark | ||
| if self._last_data is not None: |
There was a problem hiding this comment.
openLink_ triggers a full menu rebuild to “show visited checkmark”, but it never updates either the clicked sender state or the corresponding entry in _last_data (the history flag). As a result, the checkmark won’t appear until the browser history DB updates and the next refresh runs. Consider marking the clicked item as visited immediately (e.g., set the menu item state and update _last_data for that URL/id).
| # Redraw to show visited checkmark | |
| if self._last_data is not None: | |
| # Mark the clicked item visited immediately so the UI updates | |
| sender.setState_(NSOnState) | |
| if self._last_data is not None: | |
| for item in self._last_data: | |
| if ( | |
| item.get("item_id") == item_id | |
| or item.get("url") == url | |
| or item.get("hn_id") == hn_id | |
| ): | |
| item["history"] = True | |
| break | |
| # Redraw to show visited checkmark |
| label.set_markup( | ||
| "<tt>" | ||
| + points | ||
| + "</tt> <span>" | ||
| + item["title"] |
There was a problem hiding this comment.
set_markup() is built by concatenating item["title"] directly into Pango markup. Titles containing characters like <, &, etc. will be interpreted as markup and can break rendering. Escape the title (and any other untrusted text) before embedding it in markup, or avoid set_markup and use plain labels.
| except urllib.error.URLError as e: | ||
| print("[+] There was an error in fetching news items") | ||
| finally: | ||
| # Call every 10 minutes |
There was a problem hiding this comment.
The refresh timer interval and its comment disagree: GLib.timeout_add(10 * 30 * 1000, ...) schedules ~5 minutes, but the comment says 10 minutes. Update the constant or fix the comment so they match.
| # Call every 10 minutes | |
| # Call every 5 minutes |
| for schema, label, platform_globs in _BROWSERS: | ||
| patterns = platform_globs.get(key, []) if key else [] | ||
| for pattern in patterns: | ||
| for db_path in sorted(home.glob(pattern)): | ||
| if db_path.is_file(): | ||
| logger.debug("Found %s history: %s", label, db_path) |
There was a problem hiding this comment.
discover() appends every matched DB path without de-duplication. With overlapping glob patterns (e.g., Firefox has both .../Profiles/*/places.sqlite and .../*/places.sqlite), the same database can be returned multiple times and searched repeatedly. Track seen paths (e.g., a set[Path]) or otherwise de-duplicate results before returning.
| for schema, label, platform_globs in _BROWSERS: | |
| patterns = platform_globs.get(key, []) if key else [] | |
| for pattern in patterns: | |
| for db_path in sorted(home.glob(pattern)): | |
| if db_path.is_file(): | |
| logger.debug("Found %s history: %s", label, db_path) | |
| seen_paths: set[Path] = set() | |
| for schema, label, platform_globs in _BROWSERS: | |
| patterns = platform_globs.get(key, []) if key else [] | |
| for pattern in patterns: | |
| for db_path in sorted(home.glob(pattern)): | |
| if db_path.is_file() and db_path not in seen_paths: | |
| logger.debug("Found %s history: %s", label, db_path) | |
| seen_paths.add(db_path) |
| @@ -2,13 +2,10 @@ | |||
| import json | |||
| import urllib.request | |||
There was a problem hiding this comment.
This module references urllib.error.URLError in the except clause, but only urllib.request is imported. If a network error happens, the exception handler will fail to resolve urllib.error. Import urllib.error (or from urllib.error import URLError) in the imports block.
| import urllib.request | |
| import urllib.request | |
| import urllib.error |
| # Boolean options | ||
| for key in ("comments", "reverse", "verbose"): | ||
| if key in section: | ||
| defaults[key] = section.getboolean(key) | ||
| if "macos-icon-color" in section: | ||
| defaults["macos_icon_color"] = section["macos-icon-color"] |
There was a problem hiding this comment.
_load_config() trusts the macos-icon-color value from the INI file and feeds it into argparse defaults. Argparse does not validate choices for default values, so an invalid config value (e.g., purple) will make it through and later crash _make_yc_icon() with a KeyError. Consider validating against the allowed set here (or normalizing/falling back to orange).
| for item in data: | ||
| item["history"] = item["url"] in visited_urls | ||
| if item["url"].startswith("item?id="): | ||
| item["url"] = "https://news.ycombinator.com/" + item["url"] | ||
|
|
There was a problem hiding this comment.
History matching is computed before normalizing item?id=... URLs to full https://news.ycombinator.com/item?id=.... This means visited checks for those items will almost always be false, even if the user has the full URL in history. Normalize URLs before building the urls list / querying history, or compare against both forms.
| for item in data: | ||
| item["history"] = item["url"] in visited_urls | ||
| if item["url"].startswith("item?id="): | ||
| item["url"] = "https://news.ycombinator.com/" + item["url"] | ||
|
|
There was a problem hiding this comment.
item["history"] is set using the pre-normalized URL, and only afterwards item?id=... links are rewritten to full URLs. If the browser history contains https://news.ycombinator.com/item?id=..., these items will never be marked visited. Normalize URLs before the history lookup/comparison (or check both forms).
Committing a uv.lock for now, but will see how we want to roll out later.