browsertrix icon indicating copy to clipboard operation
browsertrix copied to clipboard

Published 20 hours ago • verified publisher icon webrecorder

QA review Replay embed improvements

Open SuaYoo opened this issue 1 year ago • 4 comments

Resolves https://github.com/webrecorder/browsertrix/issues/1758

Changes

  • Adds button to refresh current QA review Replay
  • Shows loading indicator when Replay window is loading
  • Prevents clicking links within Replay
  • Fixes document scrollbar gutter
  • Adds scroll snap to QA review

Manual testing

  1. Log in as crawler
  2. Navigate to crawl QA review
  3. Click "Replay" tab. Verify Replay window loads as expected
  4. Click link within Replay. Verify click is prevented and message is shown
  5. Click page in page list. Verify loading indicator and page content shows as expected
  6. Change QA run ID. Verify loading indicator and page content shows as expected

Screenshots

Page Image/video
QA Review - Replay Screenshot 2024-04-30 at 6 31 59 PM
QA Review - Replay (new page loading) Screenshot 2024-04-30 at 6 30 43 PM
QA Review - Replay (click prevented) Screenshot 2024-04-30 at 6 30 56 PM

SuaYoo avatar May 01 '24 01:05 SuaYoo

Tested, only thing I might change is the additional label, looks good!!

Shrinks99 avatar May 01 '24 17:05 Shrinks99

Feedback from Ilya: Links could potentially handled by allowing navigation and then checking for 404?

SuaYoo avatar May 01 '24 21:05 SuaYoo

Nice! Looking good! Only issue I noticed is that clicking the refresh button also shows the 'navigation prevention' dialog, and then ends up going back to screenshots. Probably want to ignore the visibilityChange handling, when refreshing

ikreymer avatar May 07 '24 14:05 ikreymer

Nice! Looking good! Only issue I noticed is that clicking the refresh button also shows the 'navigation prevention' dialog, and then ends up going back to screenshots. Probably want to ignore the visibilityChange handling, when refreshing

Can you link me to URL where you're seeing this? Right now visibility change checks if the URL after _mp is the same and ignores if so, so it should work for most cases--some cases might be falling through, though I haven't seen it so far with my crawls.

SuaYoo avatar May 07 '24 16:05 SuaYoo

Some weird things going on here: http://localhost:9870/orgs/default-org/items/crawl/manual-20240502135410-e49523be-866/review/resources?qaRunId=qa-20240502135628-e49523be-866&itemPageId=2f31a23a-4dc3-4e2c-abd2-8a068487ec24

Try clicking on the replay tab — for me, it loads and then sends the browser back one entry, in this case to the resources tab, but it also does the same thing if you're viewing another page on the replay tab and then click to this page.

emma-sg avatar May 08 '24 02:05 emma-sg

Try clicking on the replay tab — for me, it loads and then sends the browser back one entry, in this case to the resources tab, but it also does the same thing if you're viewing another page on the replay tab and then click to this page.

Hmm, good catch, looks like it's because the page adds /en... Added a fix, maybe we should start on https://github.com/webrecorder/browsertrix/issues/1758 next to add a navigation event in RWP to more reliably know where the navigation occurred @ikreymer?

SuaYoo avatar May 08 '24 03:05 SuaYoo

Hmm, good catch, looks like it's because the page adds /en... Added a fix, maybe we should start on #1758 next to add a navigation event in RWP to more reliably know where the navigation occurred @ikreymer?

Yes, can do some improvements in RWP (though the linked issue is fixed by this PR also I think?) Edit: I think the issue is #1780

ikreymer avatar May 10 '24 09:05 ikreymer