-
Notifications
You must be signed in to change notification settings - Fork 131
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
Fixes input dict display in UI #1254
Conversation
The UI would display [Object object]. This fixes that and makes any object print nicely. It includes a 10000 char limit just in case someone puts something massive in there.
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 c7bc549 in 33 seconds
More details
- Looked at
27
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. ui/frontend/src/components/dashboard/Runs/Run/Run.tsx:147
- Draft comment:
The logic for processing variables in InputsDisplay, ConfigDisplay, and TagDisplay components is repeated. Consider refactoring this into a reusable function to adhere to the DRY principle. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
2. ui/frontend/src/components/dashboard/Runs/Run/Run.tsx:185
- Draft comment:
Ensure that the json_string variable does not contain sensitive data before displaying it. Avoid logging or displaying sensitive information without proper sanitization. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is speculative and does not point to a specific issue in the code. It asks to ensure something without evidence of a problem. The code change is about displaying data, not logging it, and there's no indication that sensitive data is involved. The comment does not provide actionable advice or point to a definite issue.
I might be overlooking the potential for sensitive data to be included in the json_string, but without evidence or context indicating that this is a concern, the comment remains speculative.
While it's always good to be cautious about sensitive data, the comment does not provide evidence or context that this is a concern in this specific case.
The comment should be deleted as it is speculative and does not point to a definite issue in the code.
Workflow ID: wflow_8xwgUVT2laLakXbC
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.
@@ -147,6 +147,13 @@ const VariableTable = (props: { | |||
<code>{variableKey}</code> | |||
</td> | |||
{values.map((value, i) => { | |||
let json_string = ""; | |||
if (value) { | |||
json_string = JSON.stringify(value, null, 2); |
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.
Consider checking if the value is already a string before using JSON.stringify to avoid unnecessary quotes around string values.
json_string = JSON.stringify(value, null, 2); | |
json_string = typeof value === 'string' ? value : JSON.stringify(value, null, 2); |
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.
I don't think you need to do this unless you want strings to show up as not quoted (which is odd, the type is a string so a quote makes sense).
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.
yeah I'm not going to do the typeof value thing -- since quotes seem fine above (see screenshot).
@@ -147,6 +147,13 @@ const VariableTable = (props: { | |||
<code>{variableKey}</code> | |||
</td> | |||
{values.map((value, i) => { | |||
let json_string = ""; | |||
if (value) { | |||
json_string = JSON.stringify(value, null, 2); |
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.
I don't think you need to do this unless you want strings to show up as not quoted (which is odd, the type is a string so a quote makes sense).
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! Incremental review on c7e6f44 in 58 minutes and 9 seconds
More details
- Looked at
48
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. tests/test_node.py:63
- Draft comment:
Typo in comment: "greater that" -> "greater than"
if major == 2 and minor > 1: # greater than 2.1
- Reason this comment was not posted:
Confidence changes required:33%
Looking at the PR description and changes, this PR is about fixing UI display issues with input dictionaries. However, the changes in test_node.py seem to be about handling numpy array type annotations differently based on numpy version.
The change appears to be fixing a compatibility issue with numpy array type annotations in newer numpy versions (>2.1). The change is reasonable since it's adapting to numpy's type annotation changes, but there's a small typo in the comment.
Workflow ID: wflow_O1a84PfDMT2TsqQG
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
The UI would display [Object object]. This fixes that and makes any object print nicely. It includes a 10000 char limit just in case someone puts something massive in there.
Changes
How I tested this
Notes
Checklist