Add scope option to grantAccessIfNeeded for page-level tools#28
Closed
sebastianchristopher wants to merge 1 commit into
Closed
Add scope option to grantAccessIfNeeded for page-level tools#28sebastianchristopher wants to merge 1 commit into
sebastianchristopher wants to merge 1 commit into
Conversation
- Add GrantAccessOptions typedef with scope ('lti'|'page') and promptTimeoutMs
- Default to 'lti' for backward compatibility
- When scope is 'page', operate directly on the page instead of the LTI iframe
- Use .first() on button locator to avoid strict-mode failures
- Make Authorize regex case-insensitive
- Update JSDoc types to accept FrameLocator | Page
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
sebastianchristopher
requested review from
buckett,
Copilot,
lardo-de-arnaud and
nicholaswilson100
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an optional scope to grantAccessIfNeeded so deployment tests can support tools that render at the top-level page (not inside the LTI iframe), while keeping the existing default LTI-iframe behavior.
Changes:
- Add
optionstograntAccessIfNeededwithscope: 'lti' | 'page'andpromptTimeoutMs. - Make the LTI “Loading…” wait conditional on
scope === 'lti'. - Improve locator robustness (
.first()on the grant-access button) and broaden matching (/ifor Authori[sz]e); update JSDoc types to acceptFrameLocator | Page.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
43
to
51
| const needsGrantAccess = await Promise.race([ | ||
| ltiToolFrame.getByText('Please Grant Access').waitFor() | ||
| scopeRoot.getByText('Please Grant Access').waitFor({ | ||
| state: 'visible', | ||
| timeout: promptTimeoutMs | ||
| }) | ||
| .then(() => { return true }), | ||
| waitForNoSpinners(ltiToolFrame, 3000) | ||
| waitForNoSpinners(scopeRoot, 3000) | ||
| .then(() => { return false }) | ||
| ]) |
buckett
reviewed
Jun 24, 2026
|
|
||
| await page.goto(toolUrl) | ||
| const ltiToolFrame = getLtiIFrame(page) | ||
| const scopeRoot = scope === 'lti' ? getLtiIFrame(page) : page |
Member
There was a problem hiding this comment.
I'm slightly confused here as the LTI tool is still displayed in a iframe. Isn't it just the getLtiIFrame() method is looking for a specific iframe which doesn't match this one iframe[data-lti-launch="true"]:
Contributor
Author
There was a problem hiding this comment.
Yeah this was the wrong solution. Will do another PR!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Some tools render directly in the page rather than inside an LTI iframe. Previously
grantAccessIfNeededalways assumed an iframe context, making it unusable for those tools.This adds an optional
optionsparameter with two fields:scope('lti' | 'page') -- controls whether the helper looks inside the LTI iframe (default, backward-compatible) or operates on the top-level page.promptTimeoutMs-- configurable timeout for detecting the "Please Grant Access" prompt (defaults to 3000 ms).Other small improvements bundled in:
.first()on the grant-access button locator to avoid Playwright strict-mode failures when multiple buttons exist./i) so it matches both "Authorize" and "Authorise" regardless of casing.grantAccessandwaitForNoSpinnersto acceptFrameLocator | Page.