feat(categorization): add category sets — named rule profiles with multi-set combining#804
feat(categorization): add category sets — named rule profiles with multi-set combining#804TimeToBuildBob wants to merge 4 commits intoActivityWatch:masterfrom
Conversation
…lti-set combining Implements category sets from ActivityWatch#369 (Erik's 2022 WIP) with an extended data model supporting priority-ordered combining of multiple active sets. ## What's new - **CategorySet type** (`src/util/classes.ts`): `{ id: string; categories: Category[] }` - **mergeCategorySets()**: merges sets in priority order (first = highest); more specific rules still win within a set, set order breaks ties - **Settings store**: new `category_sets: CategorySet[]` + `active_set_ids: string[]` fields persisted alongside the legacy `classes` field (backwards compatible) - **Categories store**: full CRUD for sets (`createSet`, `deleteSet`, `switchToSet`, `setActiveSets`, `renameSet`); `classes` always reflects the merged effective categories from all active sets; migration path for existing users (wraps legacy `classes` in a "default" set on first load) - **CategorizationSettings UI**: set switcher dropdown, New/Delete buttons, per-set export ## Design notes - `active_set_ids: string[]` data model supports combining; UI v1 exposes single-set switching — multi-set combining UI can follow in a subsequent PR - Legacy `classes` field kept in sync for backwards compat with external readers - `save()` syncs edits back to the primary active set before persisting all sets - Import handles both legacy `{ categories: [] }` and new `{ id, categories }` formats Closes / supersedes ActivityWatch#369
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #804 +/- ##
==========================================
- Coverage 29.96% 29.06% -0.90%
==========================================
Files 32 32
Lines 1829 1920 +91
Branches 317 351 +34
==========================================
+ Hits 548 558 +10
- Misses 1260 1284 +24
- Partials 21 78 +57 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Greptile SummaryThis PR picks up and completes the category-sets feature (#369), adding named rule profiles with multi-set merging, full CRUD store actions, a set-switcher UI, and a migration path that wraps legacy flat Confidence Score: 5/5Safe to merge — all prior P1 issues are resolved and remaining findings are P2. The three P1 issues from previous review rounds are confirmed fixed: discardChanges() is wired correctly, syncToPrimarySet early-returns for multi-set state, and the active-set import path uses discardChanges() to avoid overwriting imported data. Remaining findings are a redundant double-persist and an edge-case import dedup collision, both P2 and non-blocking. src/stores/categories.ts (redundant saveClasses call in save()), src/views/settings/CategorizationSettings.vue (import dedup while-loop edge case) Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as CategorizationSettings
participant CS as categoriesStore
participant SS as settingsStore
Note over UI,SS: Switch sets (with unsaved changes)
UI->>CS: discardChanges()
CS->>CS: classes = computeEffectiveClasses(sets, activeIds)
UI->>CS: switchToSet(id)
CS->>CS: syncToPrimarySet() [no-op: classes already clean]
CS->>CS: active_set_ids = [id]
CS->>CS: classes = computeEffectiveClasses(sets, [id])
Note over UI,SS: Save
UI->>CS: save()
CS->>CS: syncToPrimarySet() → primarySet.categories = classes.map(clean)
CS->>SS: saveCategories(sets, activeIds) [writes category_sets + active_set_ids + classes]
SS->>SS: $patch + save() → HTTP POST per changed key
Note over UI,SS: Load (migration path)
UI->>CS: load()
CS->>SS: loadCategories()
SS-->>CS: sets=[{id:'default', categories: legacyClasses}], activeIds=['default']
CS->>CS: computeEffectiveClasses → createMissingParents + assignIds
Reviews (3): Last reviewed commit: "fix(categorization): fix P2 bugs in impo..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| onSetChange: function (setId: string) { | ||
| if (this.classes_unsaved_changes) { | ||
| if (!confirm('You have unsaved changes. Switch sets anyway? (Changes will be discarded)')) { | ||
| // Revert the select back to current active | ||
| this.activeSetId = this.categoryStore.active_set_ids[0] || 'default'; | ||
| return; | ||
| } | ||
| } | ||
| this.categoryStore.switchToSet(setId); |
There was a problem hiding this comment.
When the user confirms "Changes will be discarded", switchToSet is called, which immediately invokes syncToPrimarySet — copying state.classes (the unsaved edits) back into the primary set's in-memory categories array. The edits are not discarded; they live in the in-memory store. If the user later saves from any set, save() calls syncToPrimarySet again and then saveCategories which persists all sets — including the "discarded" changes from the old primary set.
The fix is to not call syncToPrimarySet during a discard, e.g. by adding a discard: boolean parameter:
// In categories.ts — switchToSet
switchToSet(this: State, id: string, discard = false) {
if (!this.category_sets.find(s => s.id === id)) { ... }
if (!discard) syncToPrimarySet(this);
this.active_set_ids = [id];
this.classes = computeEffectiveClasses(this.category_sets, this.active_set_ids);
this.classes_unsaved_changes = false;
},And in the Vue component:
| onSetChange: function (setId: string) { | |
| if (this.classes_unsaved_changes) { | |
| if (!confirm('You have unsaved changes. Switch sets anyway? (Changes will be discarded)')) { | |
| // Revert the select back to current active | |
| this.activeSetId = this.categoryStore.active_set_ids[0] || 'default'; | |
| return; | |
| } | |
| } | |
| this.categoryStore.switchToSet(setId); | |
| this.categoryStore.switchToSet(setId, true); |
There was a problem hiding this comment.
Fixed in c9273ad: added a discardChanges() action to the store that resets state.classes from the stored sets without calling syncToPrimarySet. The onSetChange discard path now calls discardChanges() before switchToSet(), so unsaved edits are wiped from memory before the set switch happens.
| /** Copy current effective classes back into the primary active set (no-op if no sets). */ | ||
| function syncToPrimarySet(state: State) { | ||
| if (state.active_set_ids.length === 0 || state.category_sets.length === 0) return; | ||
| const primaryId = state.active_set_ids[0]; | ||
| const primarySet = state.category_sets.find(s => s.id === primaryId); | ||
| if (primarySet) { | ||
| primarySet.categories = state.classes.map(cleanCategory); | ||
| } | ||
| } |
There was a problem hiding this comment.
syncToPrimarySet corrupts primary set when multiple sets are active
When active_set_ids has more than one entry, state.classes holds the merged result of all active sets. Writing this merged list back to only the primary set permanently injects every category from secondary sets into it. The next save() call then persists this corruption.
setActiveSets is a public store action, so the multi-set path is reachable today even without a UI. Any caller that does store.setActiveSets(['a','b']) → user edits → store.save() will silently merge all of set B's categories into set A.
The fix is to strip secondary-set categories before syncing:
function syncToPrimarySet(state: State) {
if (state.active_set_ids.length === 0 || state.category_sets.length === 0) return;
const primaryId = state.active_set_ids[0];
const primarySet = state.category_sets.find(s => s.id === primaryId);
if (!primarySet) return;
const secondaryIds = new Set(state.active_set_ids.slice(1));
const secondaryNames = new Set<string>();
for (const s of state.category_sets.filter(s => secondaryIds.has(s.id))) {
for (const c of s.categories) secondaryNames.add(JSON.stringify(c.name));
}
primarySet.categories = state.classes
.map(cleanCategory)
.filter(c => !secondaryNames.has(JSON.stringify(c.name)));
}There was a problem hiding this comment.
Fixed in c9273ad: syncToPrimarySet now early-returns when active_set_ids.length > 1. In multi-set mode, state.classes is the merged result of all active sets, so writing it back to only the primary set would corrupt data. The function is now a no-op in that mode until proper de-merge logic is added.
| createSet: function () { | ||
| const name = prompt('Name for the new category set:'); | ||
| if (!name) return; | ||
| if (this.categoryStore.category_sets.find(s => s.id === name)) { | ||
| alert(`A set named "${name}" already exists.`); | ||
| return; | ||
| } | ||
| // Create the new set and switch to it (switchToSet syncs current state first) | ||
| this.categoryStore.createSet(name); | ||
| this.categoryStore.switchToSet(name); |
There was a problem hiding this comment.
After createSet(name) + switchToSet(name), switchToSet sets classes_unsaved_changes = false. The new (empty) set exists only in memory and is never persisted until the user explicitly saves. But because classes_unsaved_changes is false, the beforeunload guard won't fire and there's no "unsaved changes" banner — so navigating away silently drops the new set.
Consider marking classes_unsaved_changes = true after the switch since the set list itself has changed:
| createSet: function () { | |
| const name = prompt('Name for the new category set:'); | |
| if (!name) return; | |
| if (this.categoryStore.category_sets.find(s => s.id === name)) { | |
| alert(`A set named "${name}" already exists.`); | |
| return; | |
| } | |
| // Create the new set and switch to it (switchToSet syncs current state first) | |
| this.categoryStore.createSet(name); | |
| this.categoryStore.switchToSet(name); | |
| this.categoryStore.createSet(name); | |
| this.categoryStore.switchToSet(name); | |
| this.categoryStore.classes_unsaved_changes = true; |
| const existing = this.categoryStore.category_sets.find(s => s.id === setId); | ||
| if (existing) { | ||
| existing.categories = import_obj.categories; | ||
| } else { | ||
| this.categoryStore.category_sets.push({ id: setId, categories: import_obj.categories }); | ||
| } | ||
| this.categoryStore.switchToSet(setId); | ||
| this.categoryStore.classes_unsaved_changes = true; | ||
| } else { | ||
| console.error('Unrecognized import format'); | ||
| } |
There was a problem hiding this comment.
importCategories
Lines 188 and 191 mutate Pinia store state directly from the component (category_sets.push(...) and classes_unsaved_changes = true). This is inconsistent with using createSet() elsewhere and makes it harder to add validation/side-effects to the set-creation path in the future. The classes_unsaved_changes = true assignment is also immediately overwritten by the subsequent switchToSet call.
Consider routing through the store action instead:
this.categoryStore.createSet(setId);
const set = this.categoryStore.category_sets.find(s => s.id === setId);
if (set) set.categories = import_obj.categories;- syncToPrimarySet: skip when multiple sets are active When active_set_ids.length > 1, state.classes is the merged result of all active sets. Writing it back to only the primary set would absorb all secondary sets' categories into it (data corruption). Now a no-op in multi-set mode until proper de-merge logic is added. - onSetChange discard bypass: call discardChanges() before switchToSet() Without this, confirming "discard" still called switchToSet() which called syncToPrimarySet() first, writing the unsaved edits back into the primary set's in-memory state. Now we explicitly reset in-memory classes from the stored sets before switching. - Add discardChanges() action to categories store Resets state.classes from stored sets without syncing back. Separates the "reload from sets" operation from "switch to set", enabling clean discard semantics.
|
@greptileai review |
| const existing = this.categoryStore.category_sets.find(s => s.id === setId); | ||
| if (existing) { | ||
| existing.categories = import_obj.categories; | ||
| } else { | ||
| this.categoryStore.category_sets.push({ id: setId, categories: import_obj.categories }); | ||
| } | ||
| this.categoryStore.switchToSet(setId); | ||
| this.categoryStore.classes_unsaved_changes = true; |
There was a problem hiding this comment.
When the imported file's ID matches the currently active set, existing.categories is assigned the imported data (line 186), but the immediately following switchToSet(setId) call invokes syncToPrimarySet, which writes state.classes.map(cleanCategory) — the old in-memory classes — back over the just-assigned import data. The net result is that importing a file for the current active set is a no-op; the imported categories are lost.
Repro: export set "work" → import the same file back → categories unchanged.
The cleanest fix is to set the categories after switchToSet (so the sync runs first) and then recompute classes:
this.categoryStore.switchToSet(setId);
// Overwrite now, after syncToPrimarySet has already run inside switchToSet
const targetSet = this.categoryStore.category_sets.find(s => s.id === setId);
if (targetSet) {
targetSet.categories = import_obj.categories;
// Force classes to reflect the newly imported categories
this.categoryStore.discardChanges(); // recomputes from updated set
}
this.categoryStore.classes_unsaved_changes = true;
|
@TimeToBuildBob Get on it |
- exportClasses: use classes_clean instead of activeSet.categories so unsaved in-memory edits are included in the export (was silently omitting any edits made since last save/switch) - importCategories (active-set case): call discardChanges() instead of switchToSet() when importing into the currently active set. switchToSet calls syncToPrimarySet first which overwrites the just-imported categories with stale in-memory classes, making the import a silent no-op. discardChanges recomputes classes from the updated set data without the destructive sync.
|
@greptileai review |
|
Ready to merge. All P1 and P2 bugs identified by Greptile are fixed:
CI: all green. Greptile: 5/5, safe to merge. Needs merge by a maintainer — I don't have merge permissions on this repo. |
2 participants
Picks up #369 (Erik's 2022 WIP) and extends it with an updated data model and a complete implementation against the current codebase.
What changed since #369
The codebase has evolved significantly since 2022:
classesis now stored in the settings store (which syncs to aw-server) rather than directly inlocalStorage. This PR adapts the design accordingly.What's new
src/util/classes.tsCategorySettype:{ id: string; categories: Category[] }mergeCategorySets(sets): merges sets in priority order (first = highest priority); more specific rules still win within a set — set order only breaks ties at equal specificitysaveCategories/loadCategories: persist sets + active IDs to the settings store, keeping the legacyclassesfield in sync for backwards compatibility with external readerssrc/stores/settings.tscategory_sets: CategorySet[]+active_set_ids: string[], persisted alongside the existingclassesfieldsrc/stores/categories.tscreateSet,deleteSet,switchToSet,setActiveSets,renameSetclassesalways reflects the merged effective categories from all active setsclassesare wrapped in a"default"set automaticallysave()syncs current edits back to the primary active set before persisting all setssrc/views/settings/CategorizationSettings.vue{ id, categories }format (with set ID in filename); import handles both legacy{ categories }and new{ id, categories }formatsDesign decisions
active_set_ids: string[](not a single string) — the data model already supports combining multiple sets in priority order; this PR's UI exposes single-set switching. Multi-set combining UI can follow as a small incremental change.classesfield in the settings store is always kept in sync, so external tools reading settings are unaffected.Tests
All 43 tests that ran on master still pass. The 3 test suite failures (
categories.test,activity.test,views.test) are pre-existing — they fail withvueDemi.hasInjectionContext is not a function, unrelated to this PR.Closes / supersedes #369