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

Refactor Rapid integration to load the editor in an iframe #2403

Merged
merged 10 commits into from
Sep 9, 2024

Conversation

jake-low
Copy link
Contributor

@jake-low jake-low commented Aug 13, 2024

This implements @bhousel's suggestion to mount Rapid in an iframe rather than directly into the main document tree. This has a few advantages:

  • MapRoulette will always run the latest published version of Rapid (more specifically rapid@2 as specified in the CDN URL in the public/rapid-editor.html file; see the diff).
  • Rapid and MapRoulette run in different Window contexts which removes the risk of conflicts arising from pollution of the global namespace. This could happen e.g. if both Rapid and MapRoulette loaded the same script resource from a CDN (which usually results in symbols being injected into the global namespace) but each expected different versions of that dependency.
  • MapRoulette's CSS rules won't affect Rapid (resolving RapiD integration: poor readability of selected text #2368 and preventing similar issues in the future).
  • MapRoulette can still interact with Rapid, including configuring the editor at startup, observing editor events and state changes, and issuing commands to the editor (all of which are demonstrated by this PR).

One drawback worth noting is that since Rapid is now being loaded from a CDN, if the CDN is down, Rapid won't load, even if the MapRoulette servers are up and running. However, the jsDelivr CDN has an excellent track record.

This is a draft. I have a couple of tasks before I put this up for proper review:

  • figure out how to minify public/rapid-editor.html as part of the build step
  • check with the Rapid team to see if it's okay to use the client ID and client secret values from their example, or if we ought to generate our own

Fixes #2368

@jake-low jake-low force-pushed the jlow/rapid-iframe branch 3 times, most recently from cf52452 to 7e8a885 Compare August 13, 2024 01:45
Copy link

codecov bot commented Aug 13, 2024

Codecov Report

Attention: Patch coverage is 5.17241% with 55 lines in your changes missing coverage. Please review.

Project coverage is 24.02%. Comparing base (8eaf220) to head (b51c5fa).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...s/Widgets/TaskMapWidget/RapidEditor/RapidEditor.js 2.70% 32 Missing and 4 partials ⚠️
src/hooks/UseHash.js 7.69% 11 Missing and 1 partial ⚠️
.../components/Widgets/TaskMapWidget/TaskMapWidget.js 0.00% 2 Missing and 4 partials ⚠️
...skDetails/ActiveTaskControls/ActiveTaskControls.js 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2403      +/-   ##
==========================================
+ Coverage   23.95%   24.02%   +0.07%     
==========================================
  Files         649      650       +1     
  Lines       22483    22406      -77     
  Branches     6894     6888       -6     
==========================================
- Hits         5385     5384       -1     
+ Misses      14312    14254      -58     
+ Partials     2786     2768      -18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jake-low
Copy link
Contributor Author

Following up on the TODOs I wrote in the initial post:

figure out how to minify public/rapid-editor.html as part of the build step

I tried this but it would require changes to our webpack config which we can't make without 'ejecting' from create react app, so I don't think it's worth worrying about.

check with the Rapid team to see if it's okay to use the client ID and client secret values from their example, or if we ought to generate our own

In retrospect this was silly, what we want to do is use the access token for the current MapRoulette user, since that means they don't have to log into OSM twice (once in MR and again in the Rapid iframe). This is what the old code was doing, I just hadn't read it carefully enough.

With those two things resolved, I think this is ready for review.

@jake-low jake-low marked this pull request as ready for review August 15, 2024 23:17
@CollinBeczak CollinBeczak self-requested a review September 2, 2024 18:35
@jake-low
Copy link
Contributor Author

jake-low commented Sep 3, 2024

Tested this against the staging server backend, using the following .env.development.local file:

REACT_APP_MAP_ROULETTE_SERVER_URL='https://staging.maproulette.org/'
REACT_APP_MAP_ROULETTE_SERVER_WEBSOCKET_URL='wss://staging.maproulette.org/ws'
REACT_APP_MAP_ROULETTE_SERVER_GRAPHQL_URL='https://staging.maproulette.org/graphql'
REACT_APP_SERVER_API_KEY='<my staging api key>'
REACT_APP_SERVER_OAUTH_URL='http://127.0.0.1:9000/auth/authenticate?redirect=http://127.0.0.1:3000'

Everything seems to work, including:

  1. hash synchronization from Rapid to MR
  2. pre-selection of task element when editor loads
  3. proper (re-)initialization when advancing to the next task, toggling Edit Mode off + on, or refreshing the page
  4. saving edits

@CollinBeczak
Copy link
Collaborator

Should also resolve:
#2369

@jake-low
Copy link
Contributor Author

jake-low commented Sep 5, 2024

Collin and I tracked down the source of a bug in this branch which caused Rapid not to select the OSM element specified in the MR task when it loaded. We only observed this bug when running a local development backend, but not when running the frontend against staging.maproulette.org.

The selection bug was actually just a side effect; the root cause was that Rapid was failing to authenticate (preauth) to the OSM API server. When this happens it fires an authchange event which resets the editor state (the same thing that would happen if the user explicitly logged out), and thus deselects the selected element.

Rapid was failing to auth when MapRoulette was running against the master.apis.dev.openstreetmap.org server. Internally Rapid is hardcoded to assume it's talking to the real OSM API at api.openstreetmap.org. There is an open issue tracking this.

I've modified this PR to hopefully make this mistake harder to make in the future.

  • The MR frontend will only pass an auth token to Rapid if it is running against the prod OSM API (specifically if REACT_APP_OSM_API_SERVER === "https://api.openstreetmap.org"). Note that this assumes the MapRoulette backend is configured to run against the same API server - the frontend can't tell if this is the case and can't inspect the token to see which server it's valid for, so it must assume.
  • The Rapid iframe component will only enable preauth if a token is provided, and if none is, it'll log a console warning and initialize without credentials (so the user won't be logged in and won't be able to save edits without logging in first).

@jake-low
Copy link
Contributor Author

jake-low commented Sep 5, 2024

This is now deployed to staging and seems to be working correctly. The root cause of the issue Collin described in Slack was ultimately caused by Cloudflare rewriting rapid-editor.html and changing which scripts were render-blocking, which caused the iframe's load event to fire too early. That issue is resolved by adding a Cache-Control header which I've done in this PR.

@CollinBeczak
Copy link
Collaborator

LGTM

@CollinBeczak CollinBeczak mentioned this pull request Sep 9, 2024
@jake-low jake-low merged commit df07149 into main Sep 9, 2024
5 of 6 checks passed
@jake-low jake-low deleted the jlow/rapid-iframe branch September 9, 2024 15:47
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.

RapiD integration: poor readability of selected text
2 participants