-
Notifications
You must be signed in to change notification settings - Fork 150
feat: add resizable messages to span view, refactor #813
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
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.
Caution
Changes requested ❌
Reviewed everything up to bb57149 in 1 minute and 34 seconds. Click for details.
- Reviewed
1172lines of code in14files - Skipped
0files when reviewing. - Skipped posting
5draft 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%<= threshold50%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%<= threshold50%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%<= threshold50%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 by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Important
Looks good to me! 👍
Reviewed e8c9132 in 1 minute and 4 seconds. Click for details.
- Reviewed
623lines of code in10files - Skipped
0files when reviewing. - Skipped posting
4draft 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%<= threshold50%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%<= threshold50%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 by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Important
Looks good to me! 👍
Reviewed 1fe1996 in 1 minute and 51 seconds. Click for details.
- Reviewed
22lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft 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 by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Important
Looks good to me! 👍
Reviewed a40a93e in 37 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft 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%<= threshold50%None
Workflow ID: wflow_l6m9I3FILFs5hhBq
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Important
Adds resizable messages to span view, refactors state management to Zustand, and consolidates span input/output components.
ResizableWrapperincommon.tsxfor resizable message components in span view.SpanContentinspan-content.tsx.heightstate management toqueue-store.tsxandspan-view-store.tsxusing Zustand.span-view-store.tsx.heightinqueue-store.tsxandspan-view-store.tsx.SpanInputandSpanOutputwithSpanMessagesinspan-view.tsxandhuman-evaluator-span-view.tsx.messages.tsxandgeneric-parts.tsx.jsx-renderer.tsxto change background color to#0A0A0A.span-input.tsxandspan-output.tsxfiles.This description was created by
for a40a93e. You can customize this summary. It will automatically update as commits are pushed.