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

feat: Display scale as number of browser windows #2057

Merged
merged 18 commits into from
Sep 6, 2024

Conversation

SuaYoo
Copy link
Member

@SuaYoo SuaYoo commented Aug 30, 2024

Resolves #2048

Changes

  • Returns numBrowsers from API app settings
  • Displays scale/crawler instances as "Browser Windows" in workflow editor, workflow detail, and crawl settings
  • Fixes info text for edit scale dialog

Manual testing

  1. Log in as crawler
  2. Go to "Crawl Workflows" > edit workflow
  3. Click "Limits". Verify "Crawler Instances" is not shown
  4. Click "Browser Settings". Verify "Browser Windows" is shown
  5. Update browser windows and save. Verify workflow saves as expected
  6. Run crawl and wait for "Edit Browser Windows" to be enabled
  7. Click "Edit Browser Windows". Verify dialog is shown with the correct info text (see screenshot)

Screenshots

Page Image/video
Workflow Editor - Browser Settings Screenshot 2024-08-30 at 12 08 12 PM
Workflow Detail - Watch Crawl Screenshot 2024-08-30 at 12 07 47 PM
Workflow Detail / Archived Item Detail - Crawl Settings Screenshot 2024-08-30 at 12 07 56 PM

Follow-ups

There's a bug where the "Edit Browser Settings" button isn't enabled until the crawl is running, even though the value can be updated during any active crawl state. Should be addressed with #2059

@SuaYoo SuaYoo marked this pull request as ready for review August 30, 2024 19:15
SuaYoo and others added 4 commits August 30, 2024 12:16
I think we can use "crawling" here!
@SuaYoo IDK if your version of mkdocs is doing this (I think it was released in a newer version, I'm on 1.6), but it will show you in the terminal if you have broken links!  Useful for nailing down stuff like this.
Copy link
Member

@Shrinks99 Shrinks99 left a comment

Choose a reason for hiding this comment

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

Looks good to me! I think this will be a small but helpful bit of de-jargoning! :)

When I was testing earlier, I observed NaN values in the workflow settings tab but I'm pretty sure this was a cache issue. I can't reproduce it and it seems to work as expected.

Screenshot 2024-08-30 at 10 18 46 PM

Only one real thought (see comment on the docs link), but can leave for later.

href="https://docs.browsertrix.com/user-guide/workflow-setup/#browser-windows"
class="text-blue-600 hover:text-blue-500"
target="_blank"
>${msg("See caveats")}</a
Copy link
Member

Choose a reason for hiding this comment

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

Don't have to do it for this PR, but it would be really cool if docs links we link to in the app opened up in the sidebar!

docs/user-guide/overview.md Outdated Show resolved Hide resolved
Copy link
Member

@tw4l tw4l left a comment

Choose a reason for hiding this comment

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

Very nice, and thanks for the thorough job updating copy through the app and in the docs!

@ikreymer ikreymer merged commit 4c36c80 into main Sep 6, 2024
4 checks passed
@ikreymer ikreymer deleted the issue-2048-num-browsers branch September 6, 2024 00:32
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.

"Crawler Instances" → "Browser Windows"
4 participants