Skip to content

fix(queries): keep comments out of generated AQL - #1001

Merged
ErikBjare merged 1 commit into
ActivityWatch:masterfrom
NikitaMGrimm:fix/aql-query-comments
Sep 26, 2026
Merged

ErikBjare merged 1 commit into
ActivityWatch:masterfrom
NikitaMGrimm:fix/aql-query-comments

Conversation

@NikitaMGrimm

@NikitaMGrimm NikitaMGrimm commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #999.

With "Use multidevice query" enabled, Activity fails because the generated AQL contains // comments. The activity-context query has the same problem.

Move those explanations outside the query strings and add a regression test for both builders.

Reproduced with synthetic events on a local aw-server: Activity shows the syntax error before the change and loads after it. Both queries execute successfully after the fix.

Before / after (with a short blank gap):

activitywatch-aql-before-after.mp4

@greptile-apps

greptile-apps Bot commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 4/5

The PR appears safe to merge; broadening the regression assertion would improve its protection.

Findings

  1. P2 Comment check misses other forms ▶

Summary

Moves explanatory comments out of the multidevice and activity-context AQL strings and adds a regression test for both builders.

  • The test detects the comments removed by this PR, but its assertion does not cover other JavaScript comment forms.

Reviews (1) · Last reviewed commit: "fix(queries): keep comments out of gener..."

Comment on lines +407 to +408
expect(multidevice.join('\n')).not.toMatch(/^\s*\/\//m);
expect(context.join('\n')).not.toMatch(/^\s*\/\//m);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Comment check misses other forms
The new assertions catch // only at the start of a line. An inline // comment or a /* ... */ comment in generated AQL would pass the test, and querystr_to_array would still send it as part of a statement. This leaves a gap in the regression check. Please cover those forms without matching comment-like text inside string literals.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in #1007: the test now strips string literals and checks for // and /* anywhere, across all query builders.

@codecov

codecov Bot commented Sep 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 57.69%. Comparing base (23c95ed) to head (86c8fd6).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1001      +/-   ##
==========================================
+ Coverage   57.66%   57.69%   +0.03%     
==========================================
  Files          51       51              
  Lines        3231     3231              
  Branches      794      754      -40     
==========================================
+ Hits         1863     1864       +1     
- Misses       1291     1351      +60     
+ Partials       77       16      -61     

☔ 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.

@ErikBjare
ErikBjare merged commit 54ac546 into ActivityWatch:master Sep 26, 2026
9 checks passed
ErikBjare added a commit that referenced this pull request Sep 26, 2026
* test(queries): catch inline and block comments in generated AQL

The check added in #1001 only matched // at the start of a line, so an
inline // or a /* */ comment would still slip into a generated query and
break it (#999). Strip string literals first, so comment-like text inside
them (such as https:// in a category regex) is allowed, and check every
query builder with realistic params.

Addresses the Greptile review on #1001.

* test(activity): check active history stays aligned across request chunks

Return distinct data per period so a period paired with another period's
result would fail, not just a missing one.

Follow-up to review on #1003.

* test(queries): use typed, tuple-shaped fixtures in the AQL comment check

Addresses Greptile review on #1007.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Multidevice query always fails with QueryParseException: generated AQL contains // comments, which AQL does not support (#988 regression)

2 participants