Skip to content

Conversation

@olzhik11
Copy link
Member

@olzhik11 olzhik11 commented Aug 25, 2025

  • resizable messages wrapper for span view
  • refactor of custom heights, collapsed messages state -> zustand
  • refactor of span input/output component

Important

Adds resizable messages to span view, refactors state management to Zustand, and consolidates span input/output components.

  • Behavior:
    • Introduces ResizableWrapper in common.tsx for resizable message components in span view.
    • Refactors span input/output handling into SpanContent in span-content.tsx.
    • Adds height state management to queue-store.tsx and span-view-store.tsx using Zustand.
  • State Management:
    • Moves custom heights and collapsed message states to Zustand in span-view-store.tsx.
    • Persists height in queue-store.tsx and span-view-store.tsx.
  • Refactoring:
    • Replaces SpanInput and SpanOutput with SpanMessages in span-view.tsx and human-evaluator-span-view.tsx.
    • Consolidates message rendering logic in messages.tsx and generic-parts.tsx.
  • Misc:
    • Updates jsx-renderer.tsx to change background color to #0A0A0A.
    • Removes span-input.tsx and span-output.tsx files.

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

@olzhik11 olzhik11 self-assigned this Aug 25, 2025
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.

Caution

Changes requested ❌

Reviewed everything up to bb57149 in 1 minute and 34 seconds. Click for details.
  • Reviewed 1172 lines of code in 14 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 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. frontend/components/shared/traces/span-view.tsx:90
  • Draft comment:
    Refactored SpanInput/SpanOutput to use a unified SpanMessages component. Ensure that the new component preserves all legacy side‐effects (e.g. data fetching, error handling).
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to ensure that the new component preserves all legacy side-effects, which is a form of asking them to double-check their work. This violates the rule against asking the author to ensure behavior is intended or tested.
2. frontend/components/traces/span-view/common.tsx:187
  • Draft comment:
    The MessageWrapper uses a function 'collapsed' (obtained from state.isCollapsed) to determine collapse state. Consider renaming it to 'isCollapsed' or 'checkCollapsed' for clarity.
  • 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% This code section is new - it's replacing useLocalStorage with useSpanViewStore. The naming suggestion is technically valid since 'collapsed' is a function while 'isCollapsed' would better indicate it's a boolean check. However, this is a relatively minor naming preference that doesn't impact functionality. The naming could be slightly confusing since 'collapsed' sounds like a state while it's actually a function. However, the code is still quite readable and the function's purpose is clear from context. While better naming could marginally improve code clarity, this is a minor stylistic preference that doesn't warrant a PR comment. The current name is not incorrect or misleading enough to require a change. This comment should be removed as it suggests a minor naming preference change that isn't significant enough to warrant a PR comment.
3. frontend/components/traces/span-view/common.tsx:25
  • Draft comment:
    In ResizableWrapper, the customHeight is derived using a null fallback. Consider using undefined or a more explicit default value to improve clarity and consistency.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
4. frontend/components/traces/trace-view/index.tsx:450
  • Draft comment:
    Empty catch block in the localStorage set should be avoided. Consider logging the error to ease debugging.
  • 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.
5. frontend/components/traces/span-view/span-view-store.tsx:64
  • Draft comment:
    The persist middleware converts Sets and Maps to arrays for storage. Ensure that this serialization/deserialization maintains the intended data types and that consumers handle the reconstructed Set/Map correctly.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to ensure that the serialization/deserialization process maintains the intended data types and that consumers handle the reconstructed Set/Map correctly. This falls under the category of asking the author to ensure behavior is intended, which is not allowed according to the rules.

Workflow ID: wflow_nts71mL2spkmvc0R

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

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 e8c9132 in 1 minute and 4 seconds. Click for details.
  • Reviewed 623 lines of code in 10 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. frontend/components/queue/queue-store.tsx:36
  • Draft comment:
    New 'height' state added and persisted via partialize. Verify its intended usage and consider adding a comment for clarity.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 50% <= threshold 50% The comment asks the PR author to verify the intended usage of the new 'height' state, which violates the rule against asking for confirmation of intention. However, it also suggests adding a comment for clarity, which is a valid suggestion. The comment is partially useful, but the part asking for verification should be removed.
2. frontend/components/queue/queue.tsx:279
  • Draft comment:
    ResizableWrapper now receives 'height' and 'setHeight'; integration looks correct. Ensure that the parent state updates trigger appropriate re-renders.
  • Reason this comment was not posted:
    Confidence changes required: 20% <= threshold 50% None
3. frontend/components/traces/span-view/span-content.tsx:15
  • Draft comment:
    The fetch in useEffect lacks error handling. Consider adding error handling (e.g., catch) to handle non-OK responses.
  • 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.
4. frontend/components/ui/template-renderer/jsx-renderer.tsx:122
  • Draft comment:
    Dynamic template execution using new Function can be risky. Ensure that the template code is trustworthy and properly sanitized to prevent injection issues.
  • 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.

Workflow ID: wflow_9O90Vqv3BlBvYTX8

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

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 1fe1996 in 1 minute and 51 seconds. Click for details.
  • Reviewed 22 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 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. frontend/components/ui/template-renderer/jsx-renderer.tsx:44
  • Draft comment:
    Dark background (#0A0A0A) in the iframe content differs from the light background (#FAFAFA) in the error content. Ensure consistent theming.
  • 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 from light to dark background appears intentional as part of a broader theme change (evidenced by both the direct color change and the switch to bg-background class). The error state having a different background color from the normal state is likely intentional - error states often have distinct styling to draw attention. The comment assumes the backgrounds should match, but there's no strong evidence for this requirement. I might be missing design system requirements that mandate consistent backgrounds. The error state's visibility might be compromised on a dark background. The error state uses a distinct red color scheme with its own background, making it stand out regardless of the main content's background. The change appears to be an intentional dark mode implementation. Delete the comment. The background color difference appears intentional, and there's no strong evidence that this is a problem rather than a deliberate design choice.
2. frontend/components/ui/template-renderer/jsx-renderer.tsx:256
  • Draft comment:
    The iframe className change removed 'rounded-md'. Confirm if the omission of rounded corners and use of 'bg-background' 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 = 0% vs. threshold = 50% The rules explicitly state "Do NOT comment on any pure frontend UI or styling changes, only logic. Assume that if the author changed the UI, they did it correctly." This comment is purely about UI styling - both the rounded corners and background color. The author likely made these changes intentionally as part of a UI update. Maybe the rounded corners are functionally important for consistency across the application? Maybe bg-background is an undefined class that would cause issues? Even if there were styling consistency issues, the rules clearly state to trust the author on UI changes. We have to assume they know the styling system and made these changes deliberately. This comment should be deleted as it relates purely to UI styling changes, which we are instructed to assume were made correctly by the author.

Workflow ID: wflow_8ua41glfEHsczkTv

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

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 a40a93e in 37 seconds. Click for details.
  • Reviewed 13 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 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. frontend/components/ui/template-renderer/jsx-renderer.tsx:256
  • Draft comment:
    Removing 'bg-background' from the iframe's class may affect the intended background styling. Confirm that this change is intentional and that the iframe's background is handled appropriately elsewhere.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_l6m9I3FILFs5hhBq

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

@olzhik11 olzhik11 merged commit be83f7f into dev Aug 25, 2025
3 checks passed
@dinmukhamedm dinmukhamedm deleted the feat/resizable-messages branch September 11, 2025 16:11
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.

2 participants