Custom-formatter #305
Custom-formatter #305
Conversation
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 2f17a91 in 2 minutes and 20 seconds
More details
- Looked at
7173lines of code in25files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. frontend/lib/flow/utils.ts:347
- Draft comment:
Typo in property name 'isCondtional'. It should be 'isConditional'. This typo is present in multiple node types. - Reason this comment was not posted:
Comment was on unchanged code.
2. frontend/lib/flow/utils.ts:374
- Draft comment:
Typo in property name 'isCondtional'. It should be 'isConditional'. This typo is present in multiple node types. - Reason this comment was not posted:
Marked as duplicate.
3. frontend/lib/flow/utils.ts:408
- Draft comment:
Typo in property name 'isCondtional'. It should be 'isConditional'. This typo is present in multiple node types. - Reason this comment was not posted:
Marked as duplicate.
4. frontend/lib/flow/utils.ts:517
- Draft comment:
Typo in property name 'isCondtional'. It should be 'isConditional'. This typo is present in multiple node types. - Reason this comment was not posted:
Marked as duplicate.
5. frontend/lib/flow/utils.ts:639
- Draft comment:
Typo in property name 'isCondtional'. It should be 'isConditional'. This typo is present in multiple node types. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_Cr2BOFJ2y5xuXTNs
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| {/* {isChatMessageList(span.input) ? ( | ||
| <ChatMessageListTab messages={span.input} /> | ||
| ) : ( | ||
| <Formatter | ||
| className="max-h-1/3" | ||
| collapsible | ||
| value={JSON.stringify(span.input)} | ||
| /> | ||
| )} | ||
| ) : ( */} | ||
| <Formatter | ||
| className="max-h-1/3" | ||
| collapsible | ||
| value={JSON.stringify(span.input)} | ||
| /> | ||
| {/* )} */} |
There was a problem hiding this comment.
this should be restored to before changes. I only did this while testing
|
Also, where does the default template come from? Can we add some comments on what the user must fill in? We'd need to fix indentation as well. Also, HTML syntax highlighting would be good |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 9ee640b in 1 minute and 56 seconds
More details
- Looked at
1876lines of code in6files - Skipped
1files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. frontend/components/ui/alert-dialog.tsx:3
- Draft comment:
Organize import statements by placing theReactimport first, followed by third-party libraries, and then local imports for better readability and maintainability. - Reason this comment was not posted:
Confidence changes required:50%
The import statements in thealert-dialog.tsxfile are not consistently ordered. TheReactimport should be the first import statement, followed by third-party libraries, and then local imports. This is a common best practice for readability and maintainability.
2. frontend/components/ui/code-editor.tsx:60
- Draft comment:
ThelineWrappingextension is conditionally added based on thelineWrappingprop, which is a good practice to ensure the editor behaves as expected. - Reason this comment was not posted:
Confidence changes required:0%
In thecode-editor.tsxfile, thelineWrappingprop is set totrueby default, which is a good practice. However, thelineWrappingextension is only added iflineWrappingis true. This is correct and ensures that the editor behaves as expected based on the prop value.
3. frontend/lib/db/migrations/relations.ts:3
- Draft comment:
Format the import statement for better readability by ensuring consistent spacing between imported items. - Reason this comment was not posted:
Confidence changes required:30%
In therelations.tsfile, the import statement for the schema is not well-formatted. The items are not consistently spaced, which can affect readability. It's a good practice to format import statements for better readability.
4. frontend/lib/db/migrations/schema.ts:14
- Draft comment:
RemovedefaultRandom()from theprojectIdfield in therenderTemplatestable as it is a foreign key and should not have a default value. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_Ag2UhWmYmtDVTs03
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.
| <CodeEditor | ||
| value={jsonData} | ||
| onChange={setJsonData} | ||
| language="html" |
There was a problem hiding this comment.
The language prop for the CodeEditor handling jsonData should be set to json instead of html to match the data type.
There was a problem hiding this comment.
this file still needs to be removed
| {/* {isChatMessageList(span.input) ? ( | ||
| <ChatMessageListTab messages={span.input} /> | ||
| ) : ( | ||
| <Formatter | ||
| className="max-h-1/3" | ||
| collapsible | ||
| value={JSON.stringify(span.input)} | ||
| /> | ||
| )} | ||
| ) : ( */} | ||
| <Formatter | ||
| className="max-h-1/3" | ||
| collapsible | ||
| value={JSON.stringify(span.input)} | ||
| /> | ||
| {/* )} */} |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 5d2959c in 1 minute and 31 seconds
More details
- Looked at
222lines of code in5files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. frontend/components/ui/custom-renderer.tsx:273
- Draft comment:
Consider using a more secure and consistent method than localStorage for storing template IDs, especially in a multi-user environment. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The use of localStorage here seems reasonable. The stored data is just an ID that gets validated against server data before use. Even if someone tampered with the localStorage value, it would just result in a failed template lookup. The actual template data and permissions are still controlled server-side. This appears to be a reasonable use of client-side storage for UI state persistence.
I could be underestimating security risks. Perhaps there are edge cases where template IDs could be sensitive information that shouldn't be stored client-side.
The template IDs appear to be used purely for UI state persistence, with all actual template data and access controls handled server-side. The security risk seems minimal given the architecture.
The comment should be deleted as it raises a speculative security concern without strong evidence of an actual issue. The current implementation appears to be a reasonable and safe use of localStorage.
2. frontend/components/ui/formatter.tsx:43
- Draft comment:
Consider using a more secure and consistent method than localStorage for storing formatter modes, especially in a multi-user environment. - Reason this comment was not posted:
Marked as duplicate.
3. frontend/components/ui/custom-renderer.tsx:279
- Draft comment:
Ensure to handle non-200 HTTP responses in fetch requests to improve error handling and user experience. - Reason this comment was not posted:
Comment did not seem useful.
4. frontend/components/ui/custom-renderer.tsx:319
- Draft comment:
Ensure to handle non-200 HTTP responses in fetch requests to improve error handling and user experience. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_vqvecWEvgiokXJZs
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 3024085 in 1 minute and 41 seconds
More details
- Looked at
192lines of code in2files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. frontend/components/queue/queue.tsx:163
- Draft comment:
Ensure thatdatais not empty before accessingdata[0]to avoid potential runtime errors.
{data && data.length > 0 && <span>Item {data[0]?.position} of {data[0]?.count}</span>}
- Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The code already handles empty data cases gracefully. The UI buttons that would trigger actions are disabled when data is empty. The suggested change would add redundant checks that make the code more verbose without adding real safety. The current optional chaining with ?. already provides null/undefined safety.
I could be underestimating the importance of defensive programming. There could be edge cases where data is an empty array that I'm not considering.
The component has comprehensive handling of all data states - null, empty array, and populated array. The existing optional chaining provides sufficient safety.
This comment should be deleted as it suggests adding redundant checks when the code already handles empty data cases appropriately through other means.
2. frontend/components/queue/queue.tsx:168
- Draft comment:
Ensure thatdatais not empty before accessingdata[0]to avoid potential runtime errors.
onClick={() => data && data.length > 0 && data[0]?.queueData && next(data[0].queueData.createdAt, 'prev')}
- Reason this comment was not posted:
Marked as duplicate.
3. frontend/components/queue/queue.tsx:176
- Draft comment:
Ensure thatdatais not empty before accessingdata[0]to avoid potential runtime errors.
onClick={() => data && data.length > 0 && data[0]?.queueData && next(data[0].queueData.createdAt, 'next')}
- Reason this comment was not posted:
Marked as duplicate.
4. frontend/app/api/projects/[projectId]/queues/[queueId]/move/route.ts:66
- Draft comment:
Ensure thatnextItemandstatsare not empty before accessing their first elements to avoid potential runtime errors.
if (nextItem.length > 0 && stats.length > 0) {
return Response.json([{
...nextItem[0],
count: stats[0].count,
position: stats[0].position
}]);
}
- Reason this comment was not posted:
Comment did not seem useful.
5. frontend/app/api/projects/[projectId]/queues/[queueId]/move/route.ts:112
- Draft comment:
Ensure thatprevItemandstatsare not empty before accessing their first elements to avoid potential runtime errors.
if (prevItem.length > 0 && stats.length > 0) {
return Response.json([{
...prevItem[0],
count: stats[0].count,
position: stats[0].position
}]);
}
- Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_mRBvFazB1sJtwcDh
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
Skipped PR review on 91882b5 because no changed files had a supported extension. If you think this was in error, please contact us and we'll fix it right away.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 634b0f0 in 48 seconds
More details
- Looked at
101lines of code in3files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. frontend/components/traces/chat-message-list-tab.tsx:98
- Draft comment:
EnsurepresetKeyis passed toContentPartTextwithinContentPartsfor consistency.
<ContentPartText text={contentPart.text} presetKey={presetKey} />
- Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_FyuNmDM67ikofJVc
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on af1e1ec in 2 minutes and 15 seconds
More details
- Looked at
588lines of code in7files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. frontend/components/traces/span-view.tsx:121
- Draft comment:
Ensurespan.eventsis not null or undefined before passing toJSON.stringifyto avoid runtime errors. - Reason this comment was not posted:
Comment did not seem useful.
2. frontend/components/traces/span-view.tsx:111
- Draft comment:
Ensurespan.attributesis not null or undefined before passing toJSON.stringifyto avoid runtime errors. - Reason this comment was not posted:
Comment did not seem useful.
3. frontend/components/traces/span-view.tsx:110
- Draft comment:
Ensurespan.attributesis not null or undefined before passing toJSON.stringifyto avoid runtime errors. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_XFklugzmBxNyTpvn
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.
| collapsible | ||
| value={JSON.stringify(span.input) + "a".repeat(100) + "\n".repeat(100)} |
There was a problem hiding this comment.
The use of "a".repeat(100) + "\n".repeat(100) in the Formatter component's value prop seems unintended. Consider removing these repeated characters and newlines.
There was a problem hiding this comment.
Skipped PR review on a1b5071 because no changed files had a supported extension. If you think this was in error, please contact us and we'll fix it right away.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 136a902 in 27 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. frontend/components/traces/span-view-span.tsx:1
- Draft comment:
It's a best practice to keep the import statement for React hooks at the top of the file. Consider movingimport { useEffect, useRef, useState } from 'react';to the top. - Reason this comment was not posted:
Confidence changes required:50%
The import statement for React hooks should be at the top of the file for better readability and organization.
Workflow ID: wflow_veRFIf3Z6eCxOhpN
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on dfded09 in 1 minute and 17 seconds
More details
- Looked at
53lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. frontend/components/traces/span-view-span.tsx:39
- Draft comment:
Thestyleprop forwidthshould be a string with units, not a number. Consider usingstyle={{ width:${contentWidth}px}}. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
In React, style properties that expect a length can accept either a number (which is interpreted as pixels) or a string with units. When a number is provided, React automatically adds 'px'. So while using${contentWidth}pxwould work, using just contentWidth is equally valid and will have the exact same result. The comment is technically incorrect in saying it "should" be a string with units.
Maybe there's a specific reason they were using string units before, like supporting different units than pixels? Maybe there's a browser compatibility issue I'm not aware of?
React's automatic handling of pixel units is well-documented and widely supported. The previous code was using pixels explicitly anyway, so there's no evidence of needing different units.
The comment should be deleted because it suggests a change that isn't necessary - React handles number values for the width style property correctly by automatically adding 'px' units.
Workflow ID: wflow_ZzMxJnKloFq9ToEf
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.
|
|
||
| resizeObserver.observe(scrollAreaRef.current); | ||
| return () => resizeObserver.disconnect(); | ||
| }, [scrollAreaRef.current]); |
There was a problem hiding this comment.
Using scrollAreaRef.current in the dependency array is incorrect because it is mutable and does not trigger re-renders. Use scrollAreaRef instead.
| collapsible | ||
| value={JSON.stringify(span.input) + "a".repeat(100) + "\n".repeat(100)} |
There was a problem hiding this comment.
The addition of repeated characters and newlines in the JSON string seems like a debugging artifact. Consider removing + "a".repeat(100) + "\n".repeat(100) before merging.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 65a47ff in 25 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_WhNWQ1aEYXk9HWmI
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
* Merge pull request #311 from lmnr-ai/fix/dashboard-tooltip fix double conversion date in chart tooltip too * Collapsable trace tree (#312) * Add ability to rename projects (#306) * Add ability to rename projects * Added user feedback notifiaction and convert PUT to POST * change to router.refresh * add close dialog after rename * Custom-formatter (#305) --------- Co-authored-by: skull8888888 <skull8888888@gmail.com> Co-authored-by: Nidhi Singh <120259299+nidhi-wa@users.noreply.github.com>
Important
Introduces a custom formatter feature, updates database schema with new tables, and enhances UI components and dependencies.
CustomRenderercomponent incustom-renderer.tsxfor rendering custom templates.Formattercomponent informatter.tsxto support custom rendering mode.render_templatesandmachinestables in0012_lying_mandrill.sql.schema.tsandrelations.tsto include new tables.AlertDialogandConfirmDialogcomponents inalert-dialog.tsxandconfirm-dialog.tsx.Sheetcomponent insheet.tsxto improve styling and functionality.@codemirror/lang-htmltopackage.jsonfor HTML support in code editor.0012_snapshot.jsonand_journal.json.This description was created by
for 65a47ff. It will automatically update as commits are pushed.