Add new webgl broken modal #367 #364 #390
Merged
Conversation
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
This comment has been minimized.
This comment has been minimized.
Contributor
|
⏳ Code review in progress. Analyzing for code quality issues and best practices. You can monitor the review status in the checks section at the bottom of this pull request. Detailed findings will be posted upon completion. Using Amazon Q Developer for GitHubAmazon Q Developer1 is an AI-powered assistant that integrates directly into your GitHub workflow, enhancing your development process with intelligent features for code development, review, and transformation. Slash Commands
FeaturesAgentic Chat Code Review CustomizationYou can create project-specific rules for Amazon Q Developer to follow:
Example rule: FeedbackTo provide feedback on Amazon Q Developer, create an issue in the Amazon Q Developer public repository. For more detailed information, visit the Amazon Q for GitHub documentation. Footnotes
|
Contributor
|
❌ Visual Regression Testing Failed @supervoidcoder Something went wrong during the screenshot capture or comparison. Check the logs for details: View Logs |
Contributor
|
❌ MEGA PR Test Failed @supervoidcoder Something went wrong during the build or analysis process. Check the logs for details: View Logs |
wellcode-ai
bot
added
review-effort-3
🔍 General Code Quality Feedback🔍 Comprehensive Code ReviewConsolidated Feedback
Overall Assessment: The pull request introduces a new modal to handle WebGL support detection, which is a necessary feature given recent browser issues. However, it lacks test coverage and has some architectural concerns that could affect maintainability. Critical Issues:
Improvements:
Positive Notes:
Next Steps:
By addressing these issues and suggestions, the code quality, maintainability, and user experience of the application will significantly improve. 🤖 Generated by Wellcode.ai |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review Summary
This PR adds a WebGL broken modal to handle cases where browsers falsely report WebGL support. The implementation addresses a real issue with iOS/iPadOS 18.7.2 where WebGL appears supported but fails at runtime.
Critical Issues Found:
- File Structure Error:
src/containers/stage-wrapper.jsxcontains JSX fragments instead of a proper React component - Missing Props:
canActuallyUseWebGLprop is used but not defined in PropTypes - Missing Required Props: WebGlBrokenModal requires
onBackprop but it's not provided - Missing Asset: Referenced
unsupported.pngimage file is not included in the PR - Resource Leaks: WebGL resources in the smoke test are not properly cleaned up
Additional Issues:
- Hardcoded inline styles should use CSS classes
- Date reference in comment is incorrect (mentions 2025 instead of 2024)
- Navigation behavior using
window.history.back()may be unpredictable
The core concept is sound and addresses a legitimate browser compatibility issue, but the implementation needs fixes before merge.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| </Box> | ||
| { | ||
| !isRendererSupported ? null : | ||
| !props.canActuallyUseWebGL ? ( |
Contributor
There was a problem hiding this comment.
Missing PropType definition for canActuallyUseWebGL prop. The component uses props.canActuallyUseWebGL but it's not declared in the PropTypes validation.
| { | ||
| !isRendererSupported ? null : | ||
| !props.canActuallyUseWebGL ? ( | ||
| <WebGlBrokenModal isRtl={isRtl} /> |
Contributor
There was a problem hiding this comment.
Missing required onBack prop for WebGlBrokenModal. The modal component expects an onBack function but none is provided.
Suggested change
| <WebGlBrokenModal isRtl={isRtl} /> | |
| <WebGlBrokenModal isRtl={isRtl} onBack={() => {}} /> |
| </p> | ||
|
|
||
| {isIOS18_7 && ( | ||
| <p style={{marginTop: '1rem', padding: '0.75rem', backgroundColor: '#FFF3CD', borderRadius: '0.25rem'}}> |
Contributor
There was a problem hiding this comment.
Hardcoded inline styles should be moved to CSS classes for maintainability and consistency with the project's styling approach.
Suggested change
| <p style={{marginTop: '1rem', padding: '0.75rem', backgroundColor: '#FFF3CD', borderRadius: '0.25rem'}}> | |
| <p className={styles.iosWarning}> |
Comment on lines
+8
to
+21
| // In the component, replace the stage canvas section: | ||
| <Box className={styles.stageCanvasWrapper}> | ||
| { | ||
| !isRendererSupported ? null : | ||
| !props.canActuallyUseWebGL ? ( | ||
| <WebGlBrokenModal isRtl={isRtl} /> | ||
| ) : ( | ||
| <Stage | ||
| stageSize={stageSize} | ||
| vm={vm} | ||
| /> | ||
| ) | ||
| } | ||
| </Box> No newline at end of file |
Contributor
There was a problem hiding this comment.
🛑 File Structure Error: This file contains JSX code instead of a proper container component. Container files should export React components, not contain JSX fragments.
Suggested change
| // In the component, replace the stage canvas section: | |
| <Box className={styles.stageCanvasWrapper}> | |
| { | |
| !isRendererSupported ? null : | |
| !props.canActuallyUseWebGL ? ( | |
| <WebGlBrokenModal isRtl={isRtl} /> | |
| ) : ( | |
| <Stage | |
| stageSize={stageSize} | |
| vm={vm} | |
| /> | |
| ) | |
| } | |
| </Box> | |
| import PropTypes from 'prop-types'; | |
| import React from 'react'; | |
| import VM from 'scratch-vm'; | |
| import {STAGE_DISPLAY_SIZES} from '../lib/layout-constants.js'; | |
| import StageWrapperComponent from '../components/stage-wrapper/stage-wrapper.jsx'; | |
| const StageWrapper = props => <StageWrapperComponent {...props} />; | |
| StageWrapper.propTypes = { | |
| isRendererSupported: PropTypes.bool.isRequired, | |
| stageSize: PropTypes.oneOf(Object.keys(STAGE_DISPLAY_SIZES)).isRequired, | |
| vm: PropTypes.instanceOf(VM).isRequired, | |
| canActuallyUseWebGL: PropTypes.bool | |
| }; | |
| export default StageWrapper; |
|
|
||
| class WebGlBrokenModal extends React.Component { | ||
| handleCancel () { | ||
| window.history.back(); |
Contributor
There was a problem hiding this comment.
Using window.history.back() may cause unexpected navigation behavior. Consider providing a more controlled navigation mechanism or accepting an onBack callback prop.
| if (cachedWebGLActuallyWorks === null) { | ||
| // First check if renderer thinks it's supported | ||
| if (!isRendererSupported()) { | ||
| cachedWebGLActuallyWorks = false; // apple recently (as of oct-nov 2025) broke ALL of webgl somehow, but OmniBlocks vm crashes instead of reporting a modal because the browser claims it's supported (which its not) |
Contributor
There was a problem hiding this comment.
Comment contains incorrect date reference. The comment mentions "oct-nov 2025" but we're currently in 2024. This should be corrected to reflect the actual timeframe.
Suggested change
| cachedWebGLActuallyWorks = false; // apple recently (as of oct-nov 2025) broke ALL of webgl somehow, but OmniBlocks vm crashes instead of reporting a modal because the browser claims it's supported (which its not) | |
| cachedWebGLActuallyWorks = false; // apple recently (as of oct-nov 2024) broke ALL of webgl somehow, but OmniBlocks vm crashes instead of reporting a modal because the browser claims it's supported (which its not) |
Member
Author
There was a problem hiding this comment.
/q omg bruh you stupid clanker you're stuck in the past it's literally 2025 are you dum o something
Comment on lines
+101
to
+125
| const vertexShader = gl.createShader(gl.VERTEX_SHADER); | ||
| gl.shaderSource(vertexShader, 'attribute vec4 a;void main(){gl_Position=a;}'); | ||
| gl.compileShader(vertexShader); | ||
| if (!gl.getShaderParameter(vertexShader, gl.COMPILE_STATUS)) { | ||
| cachedWebGLActuallyWorks = false; | ||
| return false; | ||
| } | ||
|
|
||
| const fragmentShader = gl.createShader(gl.FRAGMENT_SHADER); | ||
| gl.shaderSource(fragmentShader, 'void main(){gl_FragColor=vec4(1.0);}'); | ||
| gl.compileShader(fragmentShader); | ||
| if (!gl.getShaderParameter(fragmentShader, gl.COMPILE_STATUS)) { | ||
| cachedWebGLActuallyWorks = false; | ||
| return false; | ||
| } | ||
|
|
||
| // Test 5: Link program | ||
| const program = gl.createProgram(); | ||
| gl.attachShader(program, vertexShader); | ||
| gl.attachShader(program, fragmentShader); | ||
| gl.linkProgram(program); | ||
| if (!gl.getProgramParameter(program, gl.LINK_STATUS)) { | ||
| cachedWebGLActuallyWorks = false; | ||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
Missing resource cleanup. WebGL resources (shaders, programs, buffers) should be cleaned up after testing to prevent memory leaks.
Suggested change
| const vertexShader = gl.createShader(gl.VERTEX_SHADER); | |
| gl.shaderSource(vertexShader, 'attribute vec4 a;void main(){gl_Position=a;}'); | |
| gl.compileShader(vertexShader); | |
| if (!gl.getShaderParameter(vertexShader, gl.COMPILE_STATUS)) { | |
| cachedWebGLActuallyWorks = false; | |
| return false; | |
| } | |
| const fragmentShader = gl.createShader(gl.FRAGMENT_SHADER); | |
| gl.shaderSource(fragmentShader, 'void main(){gl_FragColor=vec4(1.0);}'); | |
| gl.compileShader(fragmentShader); | |
| if (!gl.getShaderParameter(fragmentShader, gl.COMPILE_STATUS)) { | |
| cachedWebGLActuallyWorks = false; | |
| return false; | |
| } | |
| // Test 5: Link program | |
| const program = gl.createProgram(); | |
| gl.attachShader(program, vertexShader); | |
| gl.attachShader(program, fragmentShader); | |
| gl.linkProgram(program); | |
| if (!gl.getProgramParameter(program, gl.LINK_STATUS)) { | |
| cachedWebGLActuallyWorks = false; | |
| return false; | |
| } | |
| // Test 4: Compile minimal shaders | |
| const vertexShader = gl.createShader(gl.VERTEX_SHADER); | |
| gl.shaderSource(vertexShader, 'attribute vec4 a;void main(){gl_Position=a;}'); | |
| gl.compileShader(vertexShader); | |
| if (!gl.getShaderParameter(vertexShader, gl.COMPILE_STATUS)) { | |
| gl.deleteShader(vertexShader); | |
| cachedWebGLActuallyWorks = false; | |
| return false; | |
| } | |
| const fragmentShader = gl.createShader(gl.FRAGMENT_SHADER); | |
| gl.shaderSource(fragmentShader, 'void main(){gl_FragColor=vec4(1.0);}'); | |
| gl.compileShader(fragmentShader); | |
| if (!gl.getShaderParameter(fragmentShader, gl.COMPILE_STATUS)) { | |
| gl.deleteShader(vertexShader); | |
| gl.deleteShader(fragmentShader); | |
| cachedWebGLActuallyWorks = false; | |
| return false; | |
| } | |
| // Test 5: Link program | |
| const program = gl.createProgram(); | |
| gl.attachShader(program, vertexShader); | |
| gl.attachShader(program, fragmentShader); | |
| gl.linkProgram(program); | |
| if (!gl.getProgramParameter(program, gl.LINK_STATUS)) { | |
| gl.deleteProgram(program); | |
| gl.deleteShader(vertexShader); | |
| gl.deleteShader(fragmentShader); | |
| cachedWebGLActuallyWorks = false; | |
| return false; | |
| } | |
| // Clean up resources | |
| gl.deleteProgram(program); | |
| gl.deleteShader(vertexShader); | |
| gl.deleteShader(fragmentShader); | |
| gl.deleteBuffer(buffer); | |
| gl.deleteFramebuffer(framebuffer); |
| width: 100%; | ||
| height: 208px; | ||
| background-color: $looks-secondary; | ||
| background-image: url('./unsupported.png'); |
Contributor
There was a problem hiding this comment.
Missing image file unsupported.png. The CSS references this image but it's not included in the PR. This will cause a broken image display.
Member
Author
|
@coderabbitai hello coderabbit why did you skip the review? |
Contributor
🧩 Analysis chain🏁 Script executed: Length of output: 2991 🏁 Script executed: Length of output: 80 🏁 Script executed: Length of output: 1482 🏁 Script executed: Length of output: 328 @supervoidcoder Hey void! I apologize for missing this one. Looking at the timeline, I can see I was triggered when the PR was created (there's a mention event at 16:22:02Z), but I never posted a review - looks like something failed on my end. Other recent PRs like #385, #360, #261, and #226 all got reviews from me successfully, so this appears to be an isolated technical issue with PR #390 specifically. I'll review it now for you. The PR adds the WebGL broken modal to handle the Apple WebGL bug you encountered (iOS/iPadOS 18.7.2 build 22H123). I see Amazon Q already left some feedback, but let me give you a proper CodeRabbit review. 🧠 Learnings used✅ Actions performedReview triggered.
|
Contributor
Summary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughIntroduces WebGL capability detection with a smoke-test function, creates a new modal component to handle broken WebGL scenarios, and updates the stage-wrapper to conditionally render the modal instead of the Stage when WebGL capability detection fails. Changes
Sequence DiagramsequenceDiagram
participant User
participant StageWrapper
participant canActuallyUseWebGL
participant WebGLContext
participant Modal as WebGlBrokenModal
User->>StageWrapper: Loads stage
StageWrapper->>canActuallyUseWebGL: Check WebGL capability
canActuallyUseWebGL->>WebGLContext: Create canvas & test operations
alt Smoke test fails
WebGLContext-->>canActuallyUseWebGL: Error/validation fails
canActuallyUseWebGL-->>StageWrapper: false (cached)
StageWrapper->>Modal: Render modal
Modal-->>User: Show WebGL broken message
User->>Modal: Click Back / Close
Modal->>User: Navigate back
else Smoke test succeeds
WebGLContext-->>canActuallyUseWebGL: All operations valid
canActuallyUseWebGL-->>StageWrapper: true (cached)
StageWrapper->>User: Render Stage normally
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes
Possibly related issues
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
🧰 Additional context used🧠 Learnings (6)📓 Common learnings📚 Learning: 2025-11-15T22:03:54.980ZApplied to files:
📚 Learning: 2025-11-20T00:25:31.489ZApplied to files:
📚 Learning: 2025-10-07T19:27:27.244ZApplied to files:
📚 Learning: 2025-10-24T02:08:35.312ZApplied to files:
📚 Learning: 2025-10-22T19:25:31.991ZApplied to files:
🪛 Biome (2.1.2)src/components/webgl-broken-modal/webgl-broken-modal.css[error] 12-12: unexpected character (parse) [error] 13-13: unexpected character (parse) [error] 19-19: unexpected character (parse) [error] 21-21: unexpected character (parse) [error] 25-25: unexpected character (parse) [error] 32-32: unexpected character (parse) [error] 42-42: unexpected character (parse) [error] 56-56: unexpected character (parse) [error] 59-59: unexpected character (parse) [error] 68-68: unexpected character (parse) [error] 72-72: unexpected character (parse) 🪛 GitHub Actions: CodeQL Advancedsrc/containers/webgl-broken-modal.jsx[error] 1-1: ModuleNotFoundError: Can't resolve '../../containers/webgl-broken-modal.jsx' during Webpack build. The file may be missing or the import path is incorrect. 🪛 GitHub Actions: Mega PR Test & Analysissrc/containers/webgl-broken-modal.jsx[error] 1-1: Module not found: Can't resolve '../../containers/webgl-broken-modal.jsx' in '/home/runner/work/scratch-gui/scratch-gui/src/containers'. ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Member
Author
|
@coderabbitai haha lol why did amazon q say it's 2024 |
This was
linked to
issues
Nov 21, 2025
Closed
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 49133 @supervoidcoder 😂 Looks like Amazon Q's calendar needs an update! In one of its review comments (on Line 60 of
But... we're literally in November 2025! Your comment was correct all along. Amazon Q is apparently running on a Y2K-style bug where it thinks we're still in 2024. Classic AI moment. 🤖📅 Your WebGL smoke test implementation is actually addressing the Apple bug that happened in late 2024/early 2025, so the timeline in your comment is accurate! 🧠 Learnings used |
This was referenced Nov 21, 2025
Open
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Resolves
#367 #364
Proposed Changes
Describe what this Pull Request does
it makes a new modal that checks if webgl is actually supported to prevent browsers from bluffing about it
Reason for Changes
apple broke all of webgl (classic apple) on iOS/iPadOS 18.7.2 Build 22H123 and fixed it silently in the next build
Explain why these changes should be made. Why is this helpful or necessary? Why should this be added?
because if the browser says that webgl is supported but it's just bluffing, scratch-gui will believe it and try to load the VM. Unfortunately, as soon as the VM spawns into existence, it crashes instantly as the browser doesn't actually support WebGL
Test Coverage
Please show how you have added tests to cover your changes
what's a test
Browser Coverage
Check the OS/browser combinations tested (At least 2)
Mac
Windows
Chromebook
iPad
Android Tablet