Resolve outstanding webui TODO/FIXME markers by 0xbrayo · Pull Request #979 · ActivityWatch/aw-webui

0xbrayo · 2026-09-10T11:48:21Z

Clears out a batch of long-standing TODO/FIXME markers in the webui. Each
commit addresses one tracked issue and is independently reviewable and
mergeable; the test suite passes at every commit (308 → 505 tests), with
tsc --noEmit clean throughout.

Several of these markers turned out to be documenting real bugs rather than
pending work, so where a comment claimed something was unfinished I verified
the current behaviour before either removing the comment or fixing the defect.

Stale comments (no behaviour change)

TimelineBarChart — the FIXME asked for access to the timeperiod start;
labels() already assigns const start = this.timeperiod_start and derives
the month length from it.
CategoryEditModal — the FIXME asked for the store's category ID to be
used so saves are uniquely targeted; resetModal() (on @show) already
loads cat.id and handleSubmit() passes it to updateClass(). The id: 0
initializer is never read and is now labelled as the placeholder it is.

Correctness fixes

Sunburst clock — the Now marker was drawn unconditionally, so
historical and future day views showed a marker for the current wall-clock
time. Now guarded by the whole-day bounds already computed above it,
start-inclusive/end-exclusive, which also makes 02:00 correctly belong to the
activity day that began at 04:00 the previous date.
Alerts date range — check() built its range as
moment().subtract(1, 'days').startOf('day') plus one day, i.e. yesterday's
complete calendar day, despite the neighbouring "Get start of today" comment.
Now derives the current activity-day boundary from the user's startOfDay
setting via the existing helpers, and ends at a single captured instant.
Alerts persistence — goals lived only in component data, so every add or
delete was lost on reload. Persisted through the settings store (localStorage
is deprecated there), with a hasStoredAlerts getter so a saved empty list is
distinguishable from a first run and deleting every goal stays deleted.
Malformed stored data is dropped rather than crashing the view.
EventEditor — save()/delete_() emitted success before awaiting the
request, and the button handlers called close() immediately, so a failed
request closed the editor and told parents the operation had succeeded. The
old FIXMEs noted that moving the emit later stopped it firing at all; the
cause was the lifecycle, not the ordering — consumers render the editor behind
a v-if on the event being edited, so the component could already be gone.
The modal is now hidden before the success event is emitted, with the ref
access guarded, and a failure keeps the editor, its input and a visible error.
Bar-chart durations — durations were dropped when a category definition
was missing.
Calendar — fit-to-active produced unsafe bounds for short and overnight
events.
Timeline tooltips — fields were sanitized individually and then
interpolated into markup, which is the wrong tool: the URL went into an
unquoted <a href=${url}>, so a URL ending in onmouseover=alert(1) parsed
as a separate handler attribute; a javascript: URL was kept as the href; and
because DOMPurify is an HTML sanitizer, <img src=x onerror=…> in a text
field lost the handler but kept the element. getSwimlane also called
new URL() unguarded, so a malformed URL aborted the whole visualization.
Text is now escaped for its insertion context, URLs are restricted to http(s)
and emitted in a quoted href, and the assembled markup is sanitized once as a
boundary check.

Worth recording for reviewers: vis-timeline's own filter did mitigate all of
this end-to-end — its esnext build constructs a FilterXSS with the xss
library's defaults and defaults to xss: { disabled: false }, which stripped
the injected handler and emptied the javascript: href. The builder output
was unsafe on its own but not exploitable through this sink. The fix removes
the reliance on that second line of defence, and the new tests assert both
layers agree.
Usage-chart period identity — each bar's identity was derived from
events[0].timestamp, which fails for an empty period (no event to read),
for a period whose first activity starts late, and for rolling ranges, which
aren't identified by their start day at all. Period metadata is now carried
through explicitly; Today marks the period containing the current instant;
durations are summed rather than taken from an arbitrary first event; and
clicking a last7d/last30d bar no longer shifts the range backwards by its
own length. Degenerate inputs (0/1 bars, all-zero durations) keep finite SVG
geometry, where padding of 100/(count-1) was previously Infinity.
Active-duration semantics — the history query was AFK-only, so it
disagreed with the main activity view whenever always_active_pattern or
audible-as-active mattered. It now derives active evidence from
canonicalEvents per source so the two cannot drift, and unions across
sources so overlapping evidence and devices count once. AFK-only hosts still
contribute, and with AFK filtering disabled the history measures window
coverage, matching what the main view then reports.
Active-history cache — three defects compounded, leaving the cache
simultaneously never hit and never cleared. Lookup used
_.includes(this.active.history, tp_str), and on an object lodash checks
values (which are event arrays), so it was always false and every navigation
refetched every period. Fixing only that would have exposed the second
problem: nothing invalidated the cache — start_loading's guard only cleared
an already-empty map, so data from a previous host or settings context
survived indefinitely. The third was that the open period would then be cached
forever, freezing a duration that is still growing. Cache identity now covers
host, platform, multidevice mode, the startOfDay boundary, the resolved
source buckets and the semantic settings above; the open period is always
refetched; future periods are never queried (previously desktop-only, now
consistent with Android/iOS); no request is sent when nothing is missing; and
an invalidation generation stops a delayed response for the previous host from
landing in the new context.

Testing

npm test — 48 suites, 505 tests, 3 snapshots. tsc --noEmit clean;
vue-cli-service lint reports only 3 pre-existing warnings in
vite.config.js / vue.config.js. Production build verified.

Behavioural fixes were checked against the pre-fix code to confirm the new
tests actually fail without them, rather than only passing with them.

One deliberate snapshot update: the fullDesktopQuery active-duration snapshot
changes because the AFK-only path now floods the AFK bucket, matching
canonicalEvents.

Known adjacent issues, left alone

The Alerts category <select> offers "All" with a null value, which the
view's alertTime() cannot sum. Validation now rejects it with a message
instead of persisting an entry that would crash the view, but the option
itself is still wrong and wants its own fix.
periodLengthConvertMoment logs Invalid periodLength last7d and defaults to
'day' on each rolling-range click. That default happens to be the correct
normalization for an anchor date, and the function is shared with other
callers, so it is untouched here.

codecov · 2026-09-10T11:50:42Z

Codecov Report

❌ Patch coverage is 98.89503% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.88%. Comparing base (2ddc69f) to head (1e73859).
⚠️ Report is 5 commits behind head on master.

Files with missing lines	Patch %	Lines
src/stores/settings.ts	77.77%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master     #979       +/-   ##
===========================================
+ Coverage   53.11%   64.88%   +11.76%     
===========================================
  Files          48       51        +3     
  Lines        2984     3087      +103     
  Branches      681      761       +80     
===========================================
+ Hits         1585     2003      +418     
+ Misses       1379     1078      -301     
+ Partials       20        6       -14

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

greptile-apps · 2026-09-10T11:53:42Z

Confidence Score: 5/5

The PR appears safe to merge; no new issue was introduced after the previous review and no outstanding finding remains.

Summary

Aligns active-duration history with canonical activity semantics and adds context-aware cache invalidation.
Persists validated alert goals and queries the current configured activity day.
Makes event mutations wait for server success while preserving retryable editor state on failure.
Hardens timeline tooltip generation and malformed-URL handling.
Corrects usage-period identity, duration aggregation, chart geometry, calendar bounds, and sunburst markers.
No production changes were made after the previous review, and the previously reported cache-key defect is fixed.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Buckets[Activity buckets] --> Sources[Resolve activity sources]
  Settings[Activity settings] --> CacheKey[Build history cache identity]
  Sources --> CacheKey
  CacheKey --> Selection[Select missing or open periods]
  Selection --> Query[Canonical active-duration query]
  Query --> Generation{Generation still current?}
  Generation -->|Yes| History[Merge results into history cache]
  Generation -->|No| Discard[Discard stale response]
  History --> Usage[Period-usage visualization]

_{Reviews (3) · Last reviewed commit: "fix(activity): include the AFK filter in..."}

0xbrayo · 2026-09-10T11:59:49Z

@greptile review

0xbrayo · 2026-09-10T12:11:58Z

@greptile review

The FIXME claimed the month branch of labels() needs access to the timeperiod start to know which month to label. It already has it: the computed assigns `const start = this.timeperiod_start` and the branch derives the month length from that date. Comment-only change; rendered labels are unchanged.

@show

…Modal The FIXME asked for the store's category ID to be used so saves are uniquely targeted. That already happens: the modal's @show calls resetModal(), which reads the category via get_category_by_id() and assigns `editing.id = cat.id`, and handleSubmit() passes that ID to categoryStore.updateClass(). The `id: 0` initializer is never read, so it is now labelled as the placeholder it is. Comment-only change; no runtime behavior is affected.

drawClock(..., 'Now') ran unconditionally, so historical and future day views got a marker for the current wall-clock time even though that instant falls outside the period on screen. Guard it with the whole-day bounds already computed just above, using a start-inclusive/end-exclusive comparison. Because those bounds respect the startOfDay offset, 02:00 correctly belongs to the activity day that began at 04:00 the previous calendar date, rather than being rejected for having a different date. Adds test/unit/SunburstClock.test.js, covering current/historical/future periods, both boundaries, a fractional-hour day start, and that switching periods leaves no stale marker behind.

check() built its range as `moment().subtract(1, 'days').startOf('day')` plus one day, which is yesterday's complete calendar day — not the day start the "Get start of today" comment and the current-goal display both imply. Goals were therefore measured against the wrong 24h window. Derive the boundary from the user's startOfDay setting (default 04:00) using the existing get_offset_duration() and get_day_start_with_offset() helpers, and end the interval at the same instant the boundary was derived from, so a query that straddles the boundary stays consistent. Feeding the helper a startOf('day') moment also zeroes the seconds and milliseconds it would otherwise carry over from its input. mounted() now awaits settingsStore.ensureLoaded(), since the boundary depends on a setting that may not have loaded yet. Adds test/unit/Alerts.test.js with a frozen clock and a mocked $aw.query, asserting the submitted timeperiods before, after, and exactly at the boundary, for a fractional-hour boundary, and that the response is still processed.

Alert goals lived only in component data: the Work/Media examples were hardcoded in data(), addAlert() concatenated into an in-memory array and deleteAlert() filtered it, so every change was lost on reload. Persist them through the settings store rather than localStorage, which is deprecated there. A new typed `alerts` field holds the definitions, and a `hasStoredAlerts` getter (mirroring hasStoredCategories) reads _storedKeys so a saved empty list is distinguishable from a first run: deleting every goal stays deleted instead of resurrecting the examples. The samples are seeded in mounted() only when nothing was ever stored, and are never written to settings by initialization alone. Stored data is untrusted, so both the server and localStorage paths run it through cleanAlertGoals(): malformed entries are dropped rather than crashing the view. New src/util/alerts.ts holds the AlertGoal type and the cleaning helpers, which also normalize the string a number input produces and reject structurally invalid goals — including the category select's "All" option, whose null value the view cannot sum time for. Add and delete now surface a failure with a retryable error and restore the previously shown list, so a failed write never looks durable, and both are blocked while a save is in flight. Only definitions are stored; query results, timers and the in-progress form are not.

…cceeds save() emitted 'save' before awaiting $aw.replaceEvent and delete_() emitted 'delete' before awaiting $aw.deleteEvent, while the button handlers called close() immediately after starting the async method. A failed request therefore closed the editor and told parents the operation had succeeded when the server had not changed. The old FIXMEs noted that moving the emit later stopped it happening at all. The cause was the lifecycle, not the ordering: close() hid the modal synchronously, and consumers render the editor behind a v-if on the event being edited (VisTimeline, EventList), so by the time the request resolved the component could already be gone. Hence the modal is now hidden before the success event is emitted, and the ref access is guarded, so a parent destroying the editor in its handler cannot throw. Drop the optimistic close() from the Save/Delete handlers and route both through runMutation(), which: - emits the existing event with its existing payload exactly once, and only after the request resolves (save emits the edited event, delete the original) - keeps the editor, the user's input and a visible error on failure, and allows a retry - ignores a repeat click while a request is pending, so one click is one request - discards a late response if the editor has since been pointed at a different event Dismissal is disabled while pending, and Cancel still closes when idle without sending a mutation. Adds test/unit/EventEditor.test.js using deferred promises, including a mounted parent that destroys the editor when it hears success.

…re missing

…ght events

…abels buildTooltip sanitized each field with DOMPurify and then interpolated the result into markup, which is the wrong tool in both places it was used. Confirmed defects in the builder's output, all now covered by tests: - the URL went into an unquoted `<a href=${url}>`, so a URL ending in ` onmouseover=alert(1)` parsed as a separate event-handler attribute - a `javascript:` URL was kept verbatim as the href - DOMPurify is an HTML sanitizer, so `<img src=x onerror=alert(1)>` in a text field lost the handler but kept the element, and a title of `"><script>...` rendered as the mangled `">` — unsafe and lossy - getSwimlane called new URL() unguarded, and buildTooltip read bucket.type and e.data unguarded, so a malformed URL, a missing bucket type or an event without data threw and aborted the whole visualization Text fields are now escaped for their insertion context, so they render exactly as recorded and cannot introduce elements. URLs are parsed, restricted to http(s) and emitted in a quoted, escaped href; anything else stays readable as plain text rather than becoming a link. The assembled table is sanitized once at the end as a boundary check. Swimlane values are escaped rather than stripped: they are subgroup keys, and escaping is reversible, so two values differing only inside markup cannot collapse onto the same group. Documented finding for the two removed FIXMEs: vis-timeline's own filter did mitigate these end-to-end. Its esnext build creates a FilterXSS with the xss library's defaults and defaults to `xss: { disabled: false }`, which stripped the injected handler, emptied the javascript: href, and reduced `<img src=x onerror=...>` to `<img src>`. The builder output was unsafe on its own but not exploitable through this sink; the fix removes the reliance on that second line of defence, and the new sink tests assert both layers agree. test/unit/timelineHtml.test.js covers quote and whitespace injection, javascript:/data: schemes, markup in every displayed field, benign URLs containing & and quoted characters, malformed and missing URLs, missing bucket types and missing event data, grouping-key distinctness, and the downstream sink.

…nd navigation The renderer derived each bar's identity from events[0].timestamp, which fails three ways: an empty period has no event to read (the date became ''), the first activity of a period need not begin at the period boundary, and a rolling range is not identified by its start day at all. The store getter returned bare IEvent[][], so the period boundaries that timeperiodsAroundTimeperiod had already computed were thrown away. The getter now returns each period alongside its events, and that metadata is what the chart labels, marks and navigates from. - Navigation hands the whole TimePeriod to the view, which derives the route anchor: rolling ranges advance to start + (length - 1) days, since Activity.vue computes start = anchor - (length - 1) days. Passing the start day made that subtract the length a second time, so clicking the next last7d bar moved 6 days back instead of 7 forward. Covered by a mounted-Activity route test. - Today marks the period whose half-open interval contains the current instant, instead of comparing a derived date against today, so it respects a non-midnight boundary (02:00 belongs to the activity day that began at 04:00 the previous date) and historical windows get no marker. The selected-period highlight stays independent, and the marker label is centred over its bar. - Durations are summed over all active events. The old code took _.head of the status-filtered list and silently dropped the rest. - Geometry stays finite for degenerate inputs: padding of 100/(count-1) was Infinity for a single bar, and an all-zero maximum made every height NaN. A null or empty array renders the No data status rather than throwing. - The component renders on mount, not only on prop change, so an already-populated history is drawn immediately.

…active settings activityQuery accepted only AFK bucket ids, filtered status=not-afk and unioned those periods. canonicalEvents, which the main activity view uses, additionally treats app/title matches of always_active_pattern and audible browser events as active. The navigation history therefore disagreed with the main view whenever either setting mattered. activeDurationQuery now derives active evidence from canonicalEvents per source, so the two cannot drift, and unions the results with union_no_overlap so overlapping evidence and devices are counted once. Sources carry the host's window and browser buckets when it has them; bid_afk stays the only requirement, so AFK-only hosts still contribute and audible handling is skipped where there is no browser bucket. With AFK filtering disabled the history measures window coverage instead, matching what the main view then reports. Category filters stay out of it, keeping the duration category-independent. Android and iOS behavior is untouched. The AFK-only fallback is built inline rather than by reusing activityQuery with its RETURN line stripped. That reuse left activityQuery's trailing not_afk = merge_events_by_keys(not_afk, ["status"]); in the emitted query, and merge_events_by_keys collapses every not-afk period into a single event carrying the first timestamp and the summed duration. The result is no longer an interval, so unioning it against another source's real periods trimmed time that was never concurrent: multidevice with an AFK-only host — one running aw-watcher-afk without a window watcher — under-counted active time. Emitting flood + filter directly keeps real intervals, and the union semantics hold. Adds query-generation and store-wiring tests in test/unit/activeDuration.test.node.ts, including regressions that pin the AFK-only interval structure for single, multidevice and mixed source sets.

…refresh Three defects compounded here, so the cache was simultaneously never hit and never cleared. Lookup used `_.includes(this.active.history, tp_str)`. On an object lodash checks values, and the values are event arrays, so this was always false — every navigation refetched every period. Fixing only that would have exposed the second problem: nothing ever invalidated the cache. start_loading's guard read if (Object.keys(this.active.history).length === 0) { this.active.history = {}; } which only clears an already-empty map, and completion merges into the existing one, so data from a previous host or settings context survived indefinitely. The third was that the current period would then be cached forever, freezing a duration that is still growing. Lookup, invalidation and freshness are therefore treated as one change, with the policy in the new src/util/activeHistory.ts: - Cache identity covers everything that changes the answer: platform, host, multidevice mode, the startOfDay boundary (which defines the periods themselves), the resolved source bucket set, and the semantic inputs from the preceding audible/always-active commit (include_audible, always_active_pattern). Bucket ids are sorted, so source discovery order cannot spuriously invalidate. Date navigation within one context still reuses completed periods. - Force reload clears the cache rather than reading through it. - Freshness: a closed period is served from the cache, and a completed empty result counts as cached — nothing happened then and that does not change. The period containing now is always refetched so its duration can grow. Periods starting at or after now are never queried; that was already the desktop rule and now applies to Android/iOS too, making the future-period policy explicit and consistent across platforms. - When no periods remain, no request is sent at all. - Invalidation bumps a generation that a response must still match to be written, so a delayed response for the previous host cannot populate the new one. This complements the existing getClient().abort() in ensure_loaded rather than replacing it. Failed requests leave the cache untouched and stay retryable. start_loading now documents that preserving active.history across date changes is deliberate, with clearing decided by cache identity instead. Adds 30 policy tests in test/unit/activeHistory.test.node.ts and 19 store tests in test/unit/store/activeHistoryCache.test.node.ts, asserting actual request counts and payloads for both the desktop and Android paths.

`make lint` runs eslint over src/ and test/ with --max-warnings=0, which the added tests did not satisfy: - empty arrow callbacks tripped no-empty-function; replaced with jest.fn(), which is a better test double anyway - `pending` and `closed` shadow browser globals; renamed to `inFlight` and `closedPeriod` - a non-null assertion on the deferred release hook is replaced by a default that throws if it is called before the mock installs it - jest/expect-expect cannot see assertions inside the expectFinite helper, so that test gets the same explicit bar-count assertion as its siblings - prettier formatting No test behaviour changes; 505 tests still pass.

Greptile review, P1: toggling "Filter AFK" changes what the active-duration query measures — AFK-filtered intervals versus full window coverage — but filter_afk was missing from the cache key, so closed periods stayed cached under the previous setting and the period-usage chart showed durations for the wrong measure until some other change happened to clear the cache. This was a seam between two commits. The cache identity was written while filter_afk was deliberately inert for this query, which made omitting it correct at the time; the audible/always-active commit then gave filter_afk a real effect on the result, and the identity was not updated to match. The key now carries filter_afk, normalized as `!== false` so it agrees with how activeDurationQuery reads it and an omitted value still shares the default-on cache. Only the desktop path passes it, since activityQueryAndroid does not use it and would otherwise invalidate on an unrelated toggle. Tests cover toggling the filter in both directions, and that an omitted value still hits the default-on cache. Four of them fail without the fix.

greptile-apps Bot reviewed Sep 10, 2026

View reviewed changes

Comment thread src/stores/activity.ts

Comment thread src/util/activeHistory.ts

0xbrayo force-pushed the todos_n_fixes branch from 167717a to 770e7a8 Compare September 10, 2026 12:11

0xbrayo added 14 commits September 11, 2026 01:20

fix(charts): preserve bar-chart durations when category definitions a…

0d4cada

…re missing

fix(calendar): compute safe fit-to-active bounds for short and overni…

6b76b8a

…ght events

0xbrayo force-pushed the todos_n_fixes branch from 770e7a8 to 1e73859 Compare September 10, 2026 22:21

Navigation Menu

Uh oh!

Uh oh!

Resolve outstanding webui TODO/FIXME markers - #979

Resolve outstanding webui TODO/FIXME markers#979
0xbrayo wants to merge 14 commits into
ActivityWatch:masterfrom
0xbrayo:todos_n_fixes

0xbrayo commented Sep 10, 2026

Uh oh!

codecov Bot commented Sep 10, 2026 •

edited

Loading

Uh oh!

greptile-apps Bot commented Sep 10, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

0xbrayo commented Sep 10, 2026

Uh oh!

0xbrayo commented Sep 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Uh oh!

Conversation

0xbrayo commented Sep 10, 2026

Stale comments (no behaviour change)

Correctness fixes

Testing

Known adjacent issues, left alone

Uh oh!

codecov Bot commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

greptile-apps Bot commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Confidence Score: 5/5

Summary

Diagram

Uh oh!

Uh oh!

Uh oh!

0xbrayo commented Sep 10, 2026

Uh oh!

0xbrayo commented Sep 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

codecov Bot commented Sep 10, 2026 •

edited

Loading

greptile-apps Bot commented Sep 10, 2026 •

edited

Loading