- Notifications
You must be signed in to change notification settings - Fork1k
fix(site): updateuseAgentLogs to make it more testable and add more tests#19126
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| @@ -1,60 +1,147 @@ | |||
| import{renderHook,waitFor}from"@testing-library/react"; | |||
ParkreinerAug 2, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Enough changed with the test file that it honestly might not even be worth looking at the old version of the code
useAgentLogs to make it more testable and add more testsuseAgentLogs to make it more testable and add more tests| publishError:(event:Event)=>void; | ||
| publishClose:(event:CloseEvent)=>void; | ||
| publishOpen:(event:Event)=>void; | ||
| readonlyisConnectionOpen:boolean; |
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.
Setup is 99% the same from the old file, just with the addition of this getter method
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 think this mock WebSocket implementation needs some tests of its own, and is probably worth its own PR as well.@jaaydenh has been working on a similar refactor of some other tests that I think uses a copy of this WebSocket mock. It'd be nice to sync on that so that we don't duplicate effort, andthen we can get everyones tests reviewed. 😄
| exportconstDestructive:Story={ | ||
| args:{ | ||
| variant:"destructive", | ||
| }, | ||
| }; | ||
| exportconstInfo:Story={ | ||
| args:{ | ||
| variant:"info", | ||
| }, | ||
| }; | ||
| exportconstGreen:Story={ | ||
| args:{ | ||
| variant:"green", | ||
| }, | ||
| }; |
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.
Some of our styling for theBadge component was jacked, and we just never caught it because we never made stories for the variants
| <spanclassName="ml-auto"> | ||
| <spanclassName="sr-only">{notAvailableText}</span> | ||
| <CircleHelpIconclassName="size-icon-sm"style={{ color}}/> | ||
| </span> |
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.
Biome kept hitting me with a linter violation that this fragment was useless, even though it's needed for the MUI types. The fix script also refused to run. Figured splitting things up like this was a decent compromise
| <CircleHelpIconclassName="ml-auto size-icon-sm"style={{ color}}/> | ||
| </> | ||
| <spanclassName="ml-auto"> |
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.
why does it needmargin-left: auto?
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 had it because the previous icon had it. Let me see if it's safe to remove
ParkreinerSep 16, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Okay, it wasn't a drop-in replacement, but I updated the component and where it was used, and added an extra story. The margin values never should've been baked into the component, but now it's fully gone
The component itself is still a bit janky, but (1) this PR has already ballooned in scope from just trying to fix a simple flake, and (2) I'm legitimately running out of time to squeeze a bunch of stuff in before vacation. It's already midnight – I'm not done for today yet
I fully understand if you still have problems with the component, but what we have now should still be an improvement. We can take care of the rest of the problems in a separate PR
| width={14} | ||
| height={14} |
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.
why remove these?
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.
They're not removed – they got turned into thesize-3.5 class. This is a native HTML element – width andheight aren't special props, so we can afford to just turn them into normal Tailwind
| return()=>{ | ||
| socket.close(); | ||
| }; |
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.
can we revert this? it's clearer that the longer form doesn't return anything meaningful, whereas the second version returns whateverclose() returns.
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 prefer the changed version aesthetically (and the type signature for useEffect ensures that there won't ever be any problems), but I just reverted it
759746c intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Take 2
Closescoder/internal#644
Changes made
useAgentLogswas defined to make it easier to inject specific data dependencies (basically making the hook more unit-testable)