-
-
Notifications
You must be signed in to change notification settings - Fork 113
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: enhance colorisation for Top Window Titles #654
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #654 +/- ##
==========================================
- Coverage 26.76% 26.68% -0.09%
==========================================
Files 28 28
Lines 1655 1660 +5
Branches 292 294 +2
==========================================
Hits 443 443
- Misses 1155 1160 +5
Partials 57 57 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 292da10 in 2 minutes and 57 seconds
More details
- Looked at
88
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
9
drafted comments based on config settings.
1. src/components/SelectableVisualization.vue:34
- Draft comment:
Verify that using e.data['$category'] for the color function always returns a valid category identifier. If not, consider a fallback. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the author to verify the behavior of a specific piece of code, which violates the rule against asking the author to confirm or verify intentions. It does not provide a specific suggestion or ask for a test to be written, which would be acceptable.
2. src/components/SelectableVisualization.vue:71
- Draft comment:
Double-check if removing the join(' > ') in the color function for top_categories is intentional, as it now passes an array directly. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The change appears intentional - other visualizations use simpler color keys (like app name or domain) rather than joined strings. Using the raw category array for coloring likely allows for more consistent color mapping across visualizations. The comment is asking for verification rather than pointing out a clear issue. The code will either work or fail immediately if there's a problem - no need for manual verification.
The code could break if the coloring component expects a string instead of an array. I don't have visibility into the aw-summary component's implementation.
Even if there was an issue, it would be immediately apparent during testing/building. The comment is just asking for verification rather than identifying a clear problem.
Delete this comment as it's asking for verification rather than pointing out a clear issue, and any problems would be caught during normal testing.
3. src/visualizations/summary.ts:74
- Draft comment:
The check for Array.isArray(app.colorKey) is good. Ensure that all events provide colorKey in a consistent type (array vs string). - Reason this comment was not posted:
Confidence changes required:30%
<= threshold50%
None
4. src/visualizations/summary.ts:154
- Draft comment:
Adding 'category' property is useful for further categorization—ensure downstream components properly use it. - Reason this comment was not posted:
Confidence changes required:20%
<= threshold50%
None
5. src/components/SelectableVisualization.vue:33
- Draft comment:
Changed the color function for top_titles to use e.data['$category'] instead of the title string. Ensure that '$category' is always provided as an array so that downstream processing (in summary.ts) works correctly. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the author to ensure that a specific data format is maintained for downstream processing. This is a request for confirmation of behavior, which violates the rules.
6. src/components/SelectableVisualization.vue:70
- Draft comment:
Updated the top_categories color function to return the raw category array rather than a joined string. Confirm that using the array directly aligns with getCategoryColor's expectations and that any UI relying on joined values (e.g. in linkfunc) remains consistent. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =40%
<= threshold50%
The comment is asking the PR author to confirm that the change aligns with expectations and that the UI remains consistent. This violates the rule against asking the author to confirm their intention or ensure behavior is intended. However, it does point out a specific area of concern (UI consistency with joined values), which could be useful for the author to consider.
7. src/util/color.ts:100
- Draft comment:
The new getCategoryColor function neatly handles an array input for category names by comparing via _.isEqual. Consider caching or memoizing the loadClasses() result to avoid performance issues on repeated calls. - Reason this comment was not posted:
Marked as duplicate.
8. src/visualizations/summary.ts:74
- Draft comment:
The update function now distinguishes if app.colorKey is an array to correctly use getCategoryColor. Verify that events always supply '$category' as an array when categorisation is applied. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to verify that events always supply'$category'
as an array. This falls under the rule of not asking the author to ensure behavior is intended or to double-check things. The comment does not provide a specific code suggestion or ask for a specific test to be written.
9. src/visualizations/summary.ts:154
- Draft comment:
A new 'category' property is added in updateSummedEvents. If not used elsewhere, consider either utilizing it in the UI or removing it to avoid confusion. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
Workflow ID: wflow_HcpcmQK2FYI8cdrA
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.
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 deec992 in 1 minute and 43 seconds
More details
- Looked at
67
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
8
drafted comments based on config settings.
1. src/components/SelectableVisualization.vue:32
- Draft comment:
Ensure that using e.data['$category'] directly as the color key is intentional. For top_titles, the data may be an array; if so, be sure that downstream logic handles arrays correctly. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The comment violates several rules: 1) It starts with "Ensure that..." which is asking for confirmation 2) It's speculative with "if so..." 3) There's no strong evidence that $category being used as a color key would cause issues - in fact, it's used similarly elsewhere in the code 4) The comment isn't clearly actionable - it just raises concerns without specific fixes.
Maybe there's some domain knowledge about $category that I'm missing that would make this a valid concern. The change from title to $category is significant enough that it could introduce issues.
Even if there are potential issues, the comment is phrased as a request for verification rather than pointing out a specific problem that needs fixing. The consistent use of $category elsewhere suggests this is an intentional pattern.
Delete the comment as it violates multiple comment rules - it's speculative, asks for verification, and doesn't point to a clear issue that needs fixing.
2. src/components/SelectableVisualization.vue:71
- Draft comment:
In top_categories, namefunc joins the category array but colorfunc passes the raw array. Confirm that this mismatch is intended and that get_category_color handles an array input. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The comment is asking for confirmation about intended behavior and speculating about potential issues with get_category_color. This violates the rule about not asking authors to confirm their intention. Additionally, without seeing the implementation of get_category_color, we can't know if this is actually a problem. The change appears intentional since it was made consistently in two places.
I could be wrong about the intentionality - maybe the consistent changes were a mistake. The handling of arrays vs strings for categories could be a real issue.
Even if there is a potential issue, asking for confirmation violates our rules. If there was a real problem with array handling, it would likely surface in testing or runtime errors.
Delete the comment as it violates the rule about asking authors to confirm their intention, and we don't have enough context to know if the array handling is actually problematic.
3. src/visualizations/summary.ts:75
- Draft comment:
The check for Array.isArray(app.colorKey) and use of get_category_color is good. Consider handling the case where get_category_color might return undefined, to avoid potential rendering issues. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
4. src/visualizations/summary.ts:156
- Draft comment:
The addition of the 'category' property in updateSummedEvents appears coherent. Verify that downstream components correctly utilize this property. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
5. src/components/SelectableVisualization.vue:33
- Draft comment:
Using e.data['$category'] for color, ensure this field is always defined or provide a fallback. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
- The code was intentionally changed from using title to using $category for coloring. 2. The same pattern is used elsewhere in the codebase without fallbacks. 3. If $category was undefined, it would likely be a data issue that should be fixed at the source. 4. Adding a fallback color would hide potential data issues. 5. The PR author deliberately made this change, suggesting they know the data structure.
The comment raises a valid concern about potential undefined values. Missing category data could cause rendering issues.
However, this appears to be an intentional pattern used throughout the codebase, and adding fallbacks would mask data integrity issues rather than fix them.
The comment should be deleted as it suggests masking potential data issues rather than fixing them at the source, and goes against the established pattern in the codebase.
6. src/components/SelectableVisualization.vue:71
- Draft comment:
In top_categories, colorfunc now returns the raw $category (an array) while namefunc joins it. Ensure this discrepancy is intentional. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The comment asks to "ensure this is intentional" which violates our rule about not asking authors to verify their intentions. The pattern of using simpler values for colorfunc is common in the codebase. This appears to be an intentional refactor to align with that pattern. The comment doesn't identify a clear issue that needs fixing.
I could be wrong about the pattern - maybe there's a technical reason why colorfunc needs the same format as namefunc for categories specifically.
Even if there was a technical requirement, the comment doesn't identify it - it just asks for verification. If there was a real issue, it should be stated directly.
Delete the comment as it only asks for verification of intent without identifying a concrete issue that needs fixing.
7. src/visualizations/summary.ts:75
- Draft comment:
When app.colorKey is an array, consider caching useCategoryStore outside the loop for performance and ensure type consistency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50%
The suggestion to cache the store outside the loop is technically correct - it would be more efficient. However, without knowing the implementation of useCategoryStore or how often colorKey is an array, it's hard to know if this is a meaningful optimization. The type consistency part of the comment is vague and not clearly actionable.
The performance impact might be negligible if colorKey is rarely an array or if useCategoryStore is lightweight. The comment doesn't provide clear guidance on how to implement the suggestion.
While the optimization could be beneficial, the comment would be more useful if it provided specific implementation guidance and the performance benefit was more clearly established.
The comment should be removed as it makes a speculative performance suggestion without clear evidence of benefit or specific implementation guidance.
8. src/visualizations/summary.ts:156
- Draft comment:
The new 'category' field is added to entries; verify its necessity and usage in downstream logic. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to verify the necessity and usage of a new field, which falls under asking the author to confirm their intention or ensure behavior. This violates the rules.
Workflow ID: wflow_8q4UHfo9trXNviTZ
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Colorises Top Window Titles based on the category of the event rather than just the event title. Reduces confusion for new users who may see a lot of "uncategorised" rectangles even if they have categorised everything correctly.
This is the first time I'm doing something bigger in the Vue, so I'm open to any constructive criticism :)
Important
Enhances colorization for top window titles by using event categories instead of titles in
SelectableVisualization.vue
andsummary.ts
.colorfunc
inSelectableVisualization.vue
fortop_titles
andtop_categories
to usee.data['$category']
instead ofe.data.title
.update()
insummary.ts
to use category-based color logic for entries withcolorKey
as an array.useCategoryStore
import insummary.ts
for category color retrieval.category
field toEntry
interface insummary.ts
.This description was created by
for deec992. It will automatically update as commits are pushed.