fix(research): classify by longest matching pattern, not map order - #138
fix(research): classify by longest matching pattern, not map order#138TimeToBuildBob wants to merge 3 commits into
Conversation
Study category maps list both a generic and a specific form of the same host: `google.com` -> Search & Navigation appears well before `docs.google.com` -> Work & Productivity. Because classify_title matched in insertion order, every Google Workspace URL (Docs, Drive, Calendar, Meet) was filed under Search & Navigation. Reported from a Research Edition field test: the participant's Top Applications view showed Workspace time as Search. Select the longest matching pattern instead, in both the Python filter and its Swift twin used by the macOS capture path. Ties resolve to map order, so existing single-form maps behave exactly as before. Empty pattern keys are now ignored rather than matching everything. 6 regression tests added; all fail against the previous implementation.
Greptile SummaryThe PR changes research classification to select the longest matching URL or title pattern while preserving map order for ties.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Captured browser activity] --> B{URL has a configured match?}
B -->|Yes| C[Select longest URL pattern]
B -->|No| D[Select longest title pattern]
C --> E[Resolve equal lengths by map order]
D --> E
E --> F[Store configured category]
E -->|No match| G[Store excluded]
Reviews (2): Last reviewed commit: "fix(tests): use cross-platform config di..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| pattern_lower = pattern.lower() | ||
| if pattern_lower and pattern_lower in haystack and len(pattern_lower) > best_len: | ||
| best_category = category |
There was a problem hiding this comment.
If a category map contains overlapping non-ASCII patterns, Python ranks lowercased code points while Swift ranks original extended grapheme clusters and uses different case-folding semantics, producing different research categories for the same activity across platforms.
Knowledge Base Used:
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed in 4906753. replaces in so Swift matches Python's code-point length semantics. For the study maps in use today (ASCII domain names) there is no behavioural difference, but the fix prevents divergence when non-ASCII patterns are added.
| transform(window, self.CATEGORY_MAP, self.APP_CATEGORY_MAP) | ||
| self.assertEqual(window, original) | ||
|
|
||
| class TestSpecificHostBeatsGenericHost(unittest.TestCase): |
There was a problem hiding this comment.
The new longest-match regression tests are not run by make test or the pull-request workflow, so CI remains green when these classification cases fail.
Knowledge Base Used: Quality automation
There was a problem hiding this comment.
Fixed in 4906753. Added poetry run python -m pytest tests/ to the Makefile test target — the same target the CI workflow calls with make test. Both the 44 pre-existing tests and the 6 new regression cases now run on every push across ubuntu, macOS, and Windows.
Two findings from review: - Swift's String.count measures grapheme clusters while Python's len() measures code points, so the two longest-match twins could pick different winners for non-ASCII patterns of equal visual length. Compare unicodeScalars.count instead. - 'make test' only ran '--help' and mypy, so tests/ has never executed in CI — not the new regression cases, and not the 44 that predate them. Add pytest to the test target.
|
Both review findings addressed in Unicode length mismatch — Swift's Tests not run in CI — correct, and the gap is wider than this PR: Local: 50/50 green, mypy clean. |
…_HOME XDG_CONFIG_HOME is Linux-only; appdirs ignores it on macOS and Windows, so the two config tests that needed a controlled config dir were failing on those platforms (StopIteration / False is True assertions). Replace the env-var approach with a monkeypatch of aw_core.dirs.get_config_dir that redirects to tmp_path on all platforms. Apply consistently across all three XDG_CONFIG_HOME test sites.
|
@greptileai review |
1 participant
Problem
classify_titlematched study category patterns in insertion order, firstmatch wins. Study maps routinely list both a generic and a specific form of the
same host:
google.comdocs.google.comgoogle.comis a substring ofdocs.google.com, so every Google Workspace URL(Docs, Drive, Calendar, Meet) classified as Search & Navigation.
Found during a Research Edition field test — a participant's "Top Applications"
view showed Workspace time filed as Search. The macOS
swiftstrategy capturesURLs via accessibility APIs, so macOS participants hit this on every Workspace
page.
Fix
Select the longest matching pattern instead of the first, in both twins:
aw_watcher_window/research_filter.py— new_longest_match()helper, usedfor both the URL and title passes.
aw_watcher_window/macos.swift—longestResearchMatch(), same semantics, sothe macOS capture path stays consistent with the Python filter.
Behaviour notes:
are unaffected.
"" in haystackwas alwaystrue, so a stray empty key swallowed every window.
Tests
6 regression tests in
tests/test_research_filter.py, covering specific-hostprecedence on both the URL and title paths, plain
google.com/searchstillmatching the generic entry, tie resolution, and the empty-key guard.
All 6 fail against the previous implementation; the full 50-test suite passes
with the fix.
mypy aw_watcher_window/ --ignore-missing-importsclean.