chore: move attachment link back to tree item, make it flash yellow #34353

Skn0tt · 2025-01-16T12:35:35Z

Screen.Recording.2025-01-16.at.13.33.42.mov

Screen.Recording.2025-01-16.at.15.03.10.mov

This reverts commit a33659f.

dgozman · 2025-01-16T14:16:01Z

packages/web/src/uiUtils.ts

@@ -224,3 +224,16 @@ export function scrollIntoViewIfNeeded(element: Element | undefined) {

 const kControlCodesRe = '\\u0000-\\u0020\\u007f-\\u009f';
 export const kWebLinkRe = new RegExp('(?:[a-zA-Z][a-zA-Z0-9+.-]{2,}:\\/\\/|www\\.)[^\\s' + kControlCodesRe + '"]{2,}[^\\s' + kControlCodesRe + '"\')}\\],:;.!?]', 'ug');
+
+// flash is retriggered whenever the value changes
+export function useFlash(flash: any | undefined) {


What's the semantic? Whenever flash is truthy it is going to "flash" and unflash in a second? Should this be flash: boolean instead?

whenever flash changes its value, useFlash returns true for two seconds. if it were flash: boolean then you couldn't retrigger it. it depends on something where it can compare references

This API signature should be const useFlash = (): [boolean, () => void]

You return a function that, when called, triggers the flash by updating state. You have a useState with an incrementing counter to trigger rerenders (i.e. the useEffect dependencies array contains the state number).

In particular it looks like you want your setFlash to take an ID object (we can just use the attachment itself), which is then cleared when the timer finishes. You then compare against that ID object to determine if you want to flash that particular attachment when it changes.

I believe you also trigger your scrollTo when you have a matching flash object.

How's that different from the current implementation, apart from the placement of the useEffect?

Suggested change

export function useFlash(flash: any | undefined) {

export function useFlash(idObject?: any) {

agg23

Visually it looks good. Left some comments on the React bits.

agg23 · 2025-01-16T18:07:57Z

packages/web/src/uiUtils.ts

@@ -224,3 +224,16 @@ export function scrollIntoViewIfNeeded(element: Element | undefined) {

 const kControlCodesRe = '\\u0000-\\u0020\\u007f-\\u009f';
 export const kWebLinkRe = new RegExp('(?:[a-zA-Z][a-zA-Z0-9+.-]{2,}:\\/\\/|www\\.)[^\\s' + kControlCodesRe + '"]{2,}[^\\s' + kControlCodesRe + '"\')}\\],:;.!?]', 'ug');
+
+// flash is retriggered whenever the value changes
+export function useFlash(flash: any | undefined) {


This API signature should be const useFlash = (): [boolean, () => void]

You return a function that, when called, triggers the flash by updating state. You have a useState with an incrementing counter to trigger rerenders (i.e. the useEffect dependencies array contains the state number).

agg23 · 2025-01-16T18:14:26Z

packages/web/src/uiUtils.ts

@@ -224,3 +224,16 @@ export function scrollIntoViewIfNeeded(element: Element | undefined) {

 const kControlCodesRe = '\\u0000-\\u0020\\u007f-\\u009f';
 export const kWebLinkRe = new RegExp('(?:[a-zA-Z][a-zA-Z0-9+.-]{2,}:\\/\\/|www\\.)[^\\s' + kControlCodesRe + '"]{2,}[^\\s' + kControlCodesRe + '"\')}\\],:;.!?]', 'ug');
+
+// flash is retriggered whenever the value changes
+export function useFlash(flash: any | undefined) {


In particular it looks like you want your setFlash to take an ID object (we can just use the attachment itself), which is then cleared when the timer finishes. You then compare against that ID object to determine if you want to flash that particular attachment when it changes.

I believe you also trigger your scrollTo when you have a matching flash object.

agg23 · 2025-01-16T18:15:21Z

packages/trace-viewer/src/ui/workbench.tsx

@@ -148,7 +148,7 @@ export const Workbench: React.FunctionComponent<{

  const revealAttachment = React.useCallback((attachment: AfterActionTraceEventAttachment) => {
    selectPropertiesTab('attachments');
-    setRevealedAttachment(attachment);
+    setRevealedAttachment({ ...attachment }); // copy to force re-render


I'm not seeing why this clone is necessary, and in general causing rerenders in this manner is bad practice as it's difficult to track down why stuff is behaving as it is.

It'll be passed to a useEffect down the line, so we need to do something to break Object.is equality to allow flashing the same attachment twice in a row.

Revisiting this, you're right. Cloning an object is hard to debug because both objects will look the same. I've refactored it to use an incrementing counter instead.

Skn0tt · 2025-01-17T10:42:45Z

I refactored things a little. It's now also handling the case where the animation is retriggered while it's still ongoing

https://github.com/user-attachments/assets/71432bf6-9b9b-436a-87f9-41c1671c9f46
https://github.com/user-attachments/assets/b59ab585-d2df-43ca-b9cc-35b442d836d5

dgozman · 2025-01-17T10:52:03Z

packages/web/src/uiUtils.ts

@@ -224,3 +225,22 @@ export function scrollIntoViewIfNeeded(element: Element | undefined) {

 const kControlCodesRe = '\\u0000-\\u0020\\u007f-\\u009f';
 export const kWebLinkRe = new RegExp('(?:[a-zA-Z][a-zA-Z0-9+.-]{2,}:\\/\\/|www\\.)[^\\s' + kControlCodesRe + '"]{2,}[^\\s' + kControlCodesRe + '"\')}\\],:;.!?]', 'ug');
+
+export function useFlash(): [boolean, EffectCallback] {


IIUC, this is only needed to re-trigger the animation mid-flight? Otherwise, we'd just set the css class when needed, and the animation will play by itself?

If so, please add a comment explaining this. I am not 100% sure it's worth the complexity, but I leave that up to you.

The important bit is that the css class is removed after a second, so the animation can be played multiple times.

I've added a comment to explain that.

dgozman · 2025-01-17T10:54:01Z

packages/trace-viewer/src/ui/attachmentsTab.tsx


 type Attachment = AfterActionTraceEventAttachment & { traceUrl: string };

 type ExpandableAttachmentProps = {
  attachment: Attachment;
-  reveal: boolean;
-  highlight: boolean;
+  reveal?: any;


It seems like you can revert this back to be a boolean? I don't see why it should be different now with the yellow flash compared to scrolling into view.

Just like with scrolling into view, we need this to be an ID object so we can retrigger the animation. I think there was a bug with scroll into view before, where it didn't retrigger when clicking the same button twice. But it wasn't as noticeable because you'd have to scroll around between clicking the button.

dgozman · 2025-01-17T10:54:19Z

packages/trace-viewer/src/ui/workbench.tsx

@@ -148,7 +148,12 @@ export const Workbench: React.FunctionComponent<{

  const revealAttachment = React.useCallback((attachment: AfterActionTraceEventAttachment) => {
    selectPropertiesTab('attachments');
-    setRevealedAttachment(attachment);
+    setRevealedAttachment(currentValue => {


It looks to me this can be reverted.

Nope, this is required that to re-trigger the animation. We're discussing the specific form in #34353 (comment). We don't need it in the HTML reporter because we have the URLSearchParams context, but that's not a thing in the trace viewer.

github-actions · 2025-01-17T12:01:56Z

Test results for "tests 1"

10 flaky

⚠️ [chromium-library] › tests/library/role-utils.spec.ts:442:5 › svg role=presentation @chromium-ubuntu-22.04-node20
⚠️ [firefox-page] › tests/page/page-evaluate.spec.ts:403:3 › should throw for too deep reference chain @firefox-ubuntu-22.04-node18
⚠️ [playwright-test] › tests/ui-mode-test-setup.spec.ts:98:5 › should show errors in config @macos-latest-node18-1
⚠️ [playwright-test] › tests/ui-mode-trace.spec.ts:341:5 › should work behind reverse proxy @ubuntu-latest-node22-1
⚠️ [webkit-library] › tests/library/browsercontext-clearcookies.spec.ts:92:3 › should remove cookies by domain @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/page-event-crash.spec.ts:67:1 › should cancel navigation when page crashes @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/page-event-crash.spec.ts:77:1 › should be able to close context when page crashes @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/proxy.spec.ts:291:3 › should use SOCKS proxy for websocket requests @webkit-ubuntu-22.04-node18
⚠️ [webkit-library] › tests/library/selector-generator.spec.ts:380:5 › selector generator › should work in dynamic iframes without navigation @webkit-ubuntu-22.04-node18
⚠️ [webkit-page] › tests/page/page-set-input-files.spec.ts:205:3 › should upload multiple large files @webkit-ubuntu-22.04-node18

37593 passed, 648 skipped
✔️✔️✔️

Merge workflow run.

Skn0tt added 4 commits January 16, 2025 13:14

Revert "chore: move attachments link to step body (microsoft#34196)"

2162ade

This reverts commit a33659f.

include attachment name in title

2b74e52

different labeling

269b0bb

add yellow flash

b191773

Skn0tt requested a review from dgozman January 16, 2025 12:35

Skn0tt self-assigned this Jan 16, 2025

This comment has been minimized.

Sign in to view

Skn0tt added 4 commits January 16, 2025 14:49

update next test

879ba72

remove text underlining

13d89e4

also flash trace-viewer

299a52e

extract useFlash

5dcff8f

This comment has been minimized.

Sign in to view

dgozman reviewed Jan 16, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

agg23 reviewed Jan 16, 2025

View reviewed changes

refactor

b75cf1f

Skn0tt added 2 commits January 17, 2025 11:45

express rerender by incrementing counter

05ca43b

name tuple

34df343

Skn0tt requested a review from dgozman January 17, 2025 10:46

This comment has been minimized.

Sign in to view

dgozman reviewed Jan 17, 2025

View reviewed changes

Skn0tt added 2 commits January 17, 2025 12:17

add comment

ef9f0ff

refactor

b4d900f

This comment has been minimized.

Sign in to view

dgozman approved these changes Jan 17, 2025

View reviewed changes

agg23 approved these changes Jan 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: move attachment link back to tree item, make it flash yellow #34353

chore: move attachment link back to tree item, make it flash yellow #34353

Skn0tt commented Jan 16, 2025 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

dgozman Jan 16, 2025

Skn0tt Jan 16, 2025

agg23 Jan 16, 2025

agg23 Jan 16, 2025

Skn0tt Jan 17, 2025

Skn0tt Jan 17, 2025

This comment has been minimized.

agg23 left a comment

agg23 Jan 16, 2025

agg23 Jan 16, 2025

agg23 Jan 16, 2025

Skn0tt Jan 17, 2025 •

edited

Loading

Skn0tt Jan 17, 2025

Skn0tt commented Jan 17, 2025

This comment has been minimized.

dgozman Jan 17, 2025

Skn0tt Jan 17, 2025

dgozman Jan 17, 2025

Skn0tt Jan 17, 2025

dgozman Jan 17, 2025

Skn0tt Jan 17, 2025

This comment has been minimized.

github-actions bot commented Jan 17, 2025

	export function useFlash(flash: any \| undefined) {
	export function useFlash(idObject?: any) {

chore: move attachment link back to tree item, make it flash yellow #34353

Are you sure you want to change the base?

chore: move attachment link back to tree item, make it flash yellow #34353

Conversation

Skn0tt commented Jan 16, 2025 • edited Loading

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

agg23 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skn0tt Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skn0tt commented Jan 17, 2025

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

github-actions bot commented Jan 17, 2025

Test results for "tests 1"

Skn0tt commented Jan 16, 2025 •

edited

Loading

Skn0tt Jan 17, 2025 •

edited

Loading