-
-
Notifications
You must be signed in to change notification settings - Fork 112
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add top browser titles to activity visualization #631
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Reviewed everything up to 3e332b1 in 2 minutes and 16 seconds
More details
- Looked at
132
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
5
drafted comments based on config settings.
1. src/components/SelectableVisualization.vue:46
- Draft comment:
The PR description mentions a fix for timezone usage in date(), but no changes to timestamp formatting or ISO conversion are apparent. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is asking the PR author to update the PR description or confirm their intention, which violates the rules. It doesn't provide a specific code suggestion or request for tests. Therefore, it should be removed.
2. src/components/SelectableVisualization.vue:46
- Draft comment:
New 'top_browser_titles' block uses :colorfunc="e => e.data.$domain". Verify that using the domain for color is intended vs. using a title‐based color. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
3. src/queries.ts:355
- Draft comment:
Aggregation for browser titles using merge_events_by_keys(browser_events, ["title"]) is consistent with other groups. Consider filtering out events missing a title if that could occur. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
4. src/queries.ts:360
- Draft comment:
The issue description mentions a bug with timestamp (using ISO formatted dates) but no changes related to date formatting or a dateStart() method are visible in the diff. Please confirm if the timestamp fix is intended in this PR. - Reason this comment was not posted:
Marked as duplicate.
5. src/stores/activity.ts:676
- Draft comment:
In start_loading, 'this.browser.top_domains' is reset twice. The second assignment (line 676) likely should set 'this.browser.top_titles' to null. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_w5S3WMr2iuD69qyE
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Reviewed everything up to 00d0bc7 in 2 minutes and 8 seconds
More details
- Looked at
132
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
10
drafted comments based on config settings.
1. src/components/SelectableVisualization.vue:46
- Draft comment:
Addition of the 'top_browser_titles' visualization block and its computed property are clear, but please ensure that the timestamp/tz fixes mentioned in the issue are also addressed. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is asking the PR author to ensure that certain fixes are addressed, which violates the rule against asking the author to ensure something is done. It doesn't provide a specific suggestion or point out a specific issue in the code.
2. src/queries.ts:355
- Draft comment:
The new logic for computing 'browser_titles' mirrors that for 'browser_urls' and 'browser_domains'. Confirm that merging on the 'title' field provides correct groupings and that the limit is appropriate. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to confirm their intention regarding merging on the 'title' field and the appropriateness of the limit. This violates the rule against asking the author to confirm their intention or ensure behavior is intended.
3. src/stores/activity.ts:604
- Draft comment:
Good integration of the 'top_titles' field for the browser category in the store; verify that the grouping logic in load_demo and query_browser_completed correctly sets the title data. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
4. src/stores/views.ts:42
- Draft comment:
The default browser view now includes 'top_browser_titles'. This is consistent with the new feature, but make sure it aligns with overall user expectations. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
5. General:1
- Draft comment:
The issue description mentions a fix for timestamp formatting and timezone handling, but this PR only adds support for top_browser_titles. Please confirm that changes addressing the timestamp bug are either included elsewhere or update the commit message. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. src/components/SelectableVisualization.vue:46
- Draft comment:
The new 'top_browser_titles' visualization is integrated in line with the other summary components. Ensure that the events always have a defined 'data.title' and 'data.$domain', and that 'with_limit' behaves as expected. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
7. src/components/SelectableVisualization.vue:1
- Draft comment:
Note: The PR description mentions a fix for a timestamp bug (using ISO formatted dates) but the changes here only add the top browser titles visualization. Please align the description and commit message with the actual changes. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. src/queries.ts:355
- Draft comment:
The query for browser titles is implemented here in a manner consistent with browser URLs and domains. The merge, sort, and limit functions are applied as expected. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
9. src/stores/activity.ts:87
- Draft comment:
The addition of the 'top_titles' property within the 'browser' state and its subsequent use in query_browser_completed and demo data generation is consistent with existing patterns. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
10. src/stores/views.ts:43
- Draft comment:
Including the 'top_browser_titles' element in the default desktop view is appropriate and aligns with the new visualization addition. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
Workflow ID: wflow_hlo2hVN9pim8xMfj
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Important
Add visualization for top browser titles in activity visualization, updating components, queries, and state management.
top_browser_titles
visualization inSelectableVisualization.vue
.desktopViews
inviews.ts
to includetop_browser_titles
.fullDesktopQuery
inqueries.ts
to includebrowser_titles
.State
interface inactivity.ts
to includetop_titles
forbrowser
.query_browser_completed()
inactivity.ts
to handletop_titles
.This description was created by
for 00d0bc7. It will automatically update as commits are pushed.