Skip to content
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

Enhance: Rework debug view #523

Merged
merged 5 commits into from
Sep 18, 2024
Merged

Conversation

cjellick
Copy link
Contributor

Signed-off-by: Craig Jellick craig@acorn.io

Signed-off-by: Craig Jellick <craig@acorn.io>
Signed-off-by: Craig Jellick <craig@acorn.io>
Signed-off-by: Craig Jellick <craig@acorn.io>
ryanhopperlowe
ryanhopperlowe previously approved these changes Sep 17, 2024
import { JSONTree } from 'react-json-tree';

const CallFrames = ({ calls }: { calls: Record<string, CallFrame> | null }) => {
if (!calls) return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

can you move this line to be placed right before the final return statement?

or add a TODO to do so?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dont follow. Isn't this a short-circuit, the point of which is to just quit this function early if calls is null/empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

spoke offline. i follow now

tylerslaton
tylerslaton previously approved these changes Sep 17, 2024
Copy link
Contributor

@tylerslaton tylerslaton left a comment

Choose a reason for hiding this comment

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

Pretty big PR but I couldn't see anything I'd personally change. I like the use of react json tree here, its a natural fit.

@tylerslaton
Copy link
Contributor

Side note - when I tested this locally I saw a severity high issue in the npm audit. Will have a follow-up PR that patches that up.

Signed-off-by: Craig Jellick <craig@acorn.io>
@cjellick
Copy link
Contributor Author

Ryan had approved, but wanted a TODO comment. I added that. I'm going to merge now just to close the loop here.

@cjellick cjellick merged commit 576ef7a into gptscript-ai:main Sep 18, 2024
1 check passed
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.

3 participants