Extract out JS code across both pages. #55
Conversation
This also improves our sending of platform messages as we check that we have got a correct response (not an error) when setting data.
There was a problem hiding this comment.
Pull Request Overview
This PR extracts duplicated JavaScript code for LTI PostMessage communication into a shared library class (LtiPostMessageClient), improving code maintainability and error handling. The changes also extend timeout values and add better error checking when setting data via platform messages.
Key Changes:
- Created a reusable
LtiPostMessageClientclass to handle LTI PostMessage communication - Updated both HTML redirect pages to use the new client library instead of inline code
- Improved error handling with promise-based error catching and more informative error messages
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| library.js | New JavaScript library implementing LtiPostMessageClient class for standardized LTI PostMessage communication |
| step-1-redirect.html | Refactored to use LtiPostMessageClient and added error handling for setData operations |
| step-3-redirect.html | Refactored to use LtiPostMessageClient, increased timeout from 5s to 10s, and added debug logging |
| StateAuthorizationRedirectHandler.java | Updated to load and inject the library.js content into HTML templates |
| StateCheckingAuthenticationSuccessHandler.java | Updated to load and inject the library.js content into HTML templates |
| StringReader.java | Simplified charset handling by directly using StandardCharsets.UTF_8 |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| library = StringReader.readString(getClass().getResourceAsStream(libraryName)); | ||
| } catch (IOException e) { | ||
| throw new IllegalStateException("Failed to read "+ name, e); | ||
| throw new IllegalStateException("Failed to resource. "+e.getMessage() , e); |
There was a problem hiding this comment.
The error message 'Failed to resource.' is incomplete and unclear. It should specify what resource failed to load, such as 'Failed to read resource: ' + e.getMessage().
There was a problem hiding this comment.
Typo? Suggestion seems sensible.
| library = StringReader.readString(getClass().getResourceAsStream(libraryName)); | ||
| } catch (IOException e) { | ||
| throw new IllegalStateException("Failed to read " + name, e); | ||
| throw new IllegalStateException("Failed to read. " + e.getMessage(), e); |
There was a problem hiding this comment.
The error message 'Failed to read.' is incomplete and unclear. It should specify what failed to be read, such as 'Failed to read resource: ' + e.getMessage().
| throw new IllegalStateException("Failed to read. " + e.getMessage(), e); | |
| throw new IllegalStateException("Failed to read resources '" + htmlName + "' or '" + libraryName + "': " + e.getMessage(), e); |
This also improves our sending of platform messages as we check that we have got a correct response (not an error) when setting data.