Skip to content

Conversation

@petrroll
Copy link

@petrroll petrroll commented Aug 11, 2025

For my virtual desktop name watcher (), I'm using custom event-type with custom key (which might be wrong?) which means the visualization doesn't pick up the events well. All events are just blue and don't have a title. The data is there in tooltip but that's not good for glancing.

image

I think that if there's only one piece of data, and there's no title, we can extract the single piece of data as event.

Happy to also be told that custom watchers should use title for that purse, if that's the design. And I can prepare PR for docs and abandon this one.


Important

Enhances getTitleAttr in color.ts to extract a single data piece as title for custom events without a title.

  • Behavior:
    • In getTitleAttr in color.ts, if e.data.title is missing, extract single data piece as title if only one key exists in e.data and its value is a string.
    • Truncate extracted title to 50 characters if necessary.
  • Misc:
    • No changes to existing behavior for events with a valid title or multiple data keys.

This description was created by Ellipsis for c528deb. You can customize this summary. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed everything up to c528deb in 1 minute and 53 seconds. Click for details.
  • Reviewed 26 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/util/color.ts:127
  • Draft comment:
    Using 'if (title && typeof title === "string")' will skip empty strings. Confirm if an empty title should be treated as absent.
  • 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 current code checks both for truthiness (title &&) and type. This means empty strings will be falsy and skip to the fallback logic. The function has a clear fallback strategy - it tries to get a title from a single key's value, and ultimately returns empty string if nothing works. This seems intentional - empty strings are treated the same as missing titles. Maybe empty strings should be valid titles and returned as-is rather than triggering the fallback logic? The comment raises a valid technical point about string handling. The code's behavior of treating empty strings as invalid titles appears intentional, as evidenced by the careful fallback logic. The comment is technically correct but suggests changing intentional behavior. Delete the comment. While technically accurate about string handling, it questions intentional behavior and doesn't identify a clear issue that needs fixing.
2. src/util/color.ts:131
  • Draft comment:
    Fallback logic: If no valid title exists and e.data has only one key, its string value is used. Consider adding a comment to document this assumption for custom events.
  • 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 about code that was just added in this PR. It's suggesting a documentation improvement which is actionable. However, this appears to be an internal utility function and the fallback behavior is quite straightforward from the code itself. The logic isn't really an "assumption" - it's just a fallback mechanism for extracting a displayable title. The fallback logic might be important for maintainers to understand. Not documenting it could lead to confusion about why this specific fallback was chosen. The code is self-documenting - it clearly shows the fallback logic in a straightforward way. The behavior isn't complex enough to warrant additional documentation. While the comment points out something real, the suggested documentation would be redundant given how clear and straightforward the code already is.
3. src/util/color.ts:134
  • Draft comment:
    Only string values are considered in fallback. If non-string (e.g. number or boolean) values might be meaningful, consider converting them with String(val).
  • 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 is a title attribute getter that falls back to different strategies. The current implementation deliberately chooses to only use string values and falls back to empty string otherwise. Converting non-string values could lead to undesirable titles like "true" or "123". The code's approach of being strict about string values seems intentional. The comment could be right - maybe there are legitimate use cases where numeric or boolean titles would be meaningful. The empty string fallback might be too restrictive. Without more context about the application's requirements and what kind of data typically flows through this function, suggesting to blindly convert non-string values is speculative. The strict string-only approach seems like a deliberate design choice. The comment should be deleted as it makes a speculative suggestion without strong evidence that non-string values should be handled.
4. src/util/color.ts:135
  • Draft comment:
    The fallback value is truncated to 50 characters. Consider making this limit configurable or document why 50 characters is chosen.
  • 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% This is a UI-related change that truncates display text. The 50-character limit seems arbitrary but is likely chosen for display purposes. Making it configurable would add complexity for minimal benefit since this is just a fallback case. The rules say not to comment on pure UI/styling changes. The truncation could affect functionality if other parts of the system rely on the full string value. The 50-character limit might need to be consistent with other truncation limits in the codebase. Since this is in getTitleAttr() which is clearly for display purposes, and it's a fallback case, the exact truncation length is not critical to functionality. This falls under UI styling decisions that we should trust the author on. Delete the comment as it relates to a UI display decision and doesn't highlight a clear code quality issue requiring action.

Workflow ID: wflow_dRlIyRDH9D5PeHKz

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

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.

1 participant