General: Fix an issue in Chrome when uploading files
#9766
Conversation
github-actions
bot
added
the
client
eceeeren
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
WalkthroughThe changes involve modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachmentUnit.service.ts (2)
29-31: Enhance the temporary fix documentation.While the comment clearly indicates this is temporary and links to the Chromium issue, it would be helpful to add information about when this workaround can be removed (e.g., which Chromium version will contain the fix).
/** Ngsw-worker is bypassed temporarily to fix Chromium file upload issue * See: https://issues.chromium.org/issues/374550348 + * TODO: Remove this workaround once Chromium versions > 132 are widely adopted **/
33-36: Consider tracking this temporary fix.Since this is a temporary workaround, we should track it for future removal.
Would you like me to create a GitHub issue to track the removal of this workaround once Chromium fixes the underlying issue? The issue would include:
- Link to this PR and the Chromium issue
- Criteria for removal (Chromium version requirements)
- List of affected files that need updating
src/main/webapp/app/lecture/attachment.service.ts (1)
38-40: Enhance the documentation for future maintainability.While the comment explains the temporary nature of the bypass, consider adding:
- Affected browser versions (Chrome 130-132)
- Link to our tracking issue (Can't upload files for Lecture and Exercise #9732)
- Criteria for when this workaround can be removed
- /** Ngsw-worker is bypassed temporarily to fix Chromium file upload issue - * See: https://issues.chromium.org/issues/374550348 - **/ + /** + * Temporarily bypass ngsw-worker to fix file upload 404 errors in Chrome 130-132 + * @see https://issues.chromium.org/issues/374550348 (Chromium issue) + * @see https://github.com/your-repo/issues/9732 (Tracking issue) + * @todo Remove this workaround once the Chromium fix is released and widely adopted + */
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/main/webapp/app/lecture/attachment.service.ts(1 hunks)src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachmentUnit.service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/webapp/app/lecture/attachment.service.ts (1)
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachmentUnit.service.ts (1)
🔇 Additional comments (3)
src/main/webapp/app/lecture/lecture-unit/lecture-unit-management/attachmentUnit.service.ts (1)
33-36: Verify consistent implementation across services.
The implementation looks correct for addressing the Chromium file upload issue. However, since this is a temporary fix, we should ensure:
- The same header is consistently implemented in
AttachmentService. - The bypass doesn't affect other browsers negatively.
Let's verify the consistency of implementation:
src/main/webapp/app/lecture/attachment.service.ts (2)
38-42: Consider architectural implications of service worker bypass.
While this fix addresses the immediate issue, consider the following:
- Impact on offline functionality: Bypassing the service worker means these requests won't be cached for offline use.
- Monitoring: Consider adding telemetry to track if/when the Chromium issue is resolved.
- Related services: Other file upload services (e.g., AttachmentUnitService) might need similar changes.
Let's check for similar file upload implementations:
#!/bin/bash
# Look for similar file upload services that might need the same fix
ast-grep --pattern 'class $_Service {
$$$
create($_: $_) {
$$$
formData.append("file", $_)
$$$
}
}'42-42: Implementation looks good, verify scope of bypass.
The service worker bypass is correctly implemented using Angular's supported header mechanism. The change is focused and doesn't affect other functionality.
Let's verify that the bypass is only applied to file uploads and not other requests:
✅ Verification successful
Service worker bypass correctly scoped.
The ngsw-bypass header is only applied to relevant file upload services.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of ngsw-bypass to ensure the bypass is scoped appropriately
rg -g '*.ts' "'ngsw-bypass'"
# Check for similar file upload endpoints that might need the same fix
ast-grep --pattern 'formData.append("file", $_)'
Length of output: 494
Script:
#!/bin/bash
# Search for other instances of ngsw-bypass in TypeScript files only
rg -t ts 'ngsw-bypass'
# Check for similar file upload endpoints that might need the same fix in TypeScript files
ast-grep --pattern 'formData.append("file", $_)' --lang typescript
Length of output: 426
eceeeren
temporarily deployed
to
artemis-test4.artemis.cit.tum.de
GitHub Actions
Inactive
|
github-actions
bot
added
the
deployment-error
github-actions
bot
removed
the
deployment-error
vinceclifford
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
HawKhiem
temporarily deployed
to
artemis-test5.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
Tested on TS5. Almost everything worked. I just noticed two bugs but I am not sure if they are related to this PR though:
- When clicking twice on the upload button for the unit while it's loading, it is added twice. Is this intended?
- The tooltip says you can add png files as file. But when trying to add one, I receive an error.
Hello, these are not related to this PR but I think both of them are not intended :) You can open an issue with them |
krusche
changed the title
General: Temporarily bypass ngsw-worker when uploading a fileGeneral: Fix an issue in Chrome when uploading files
Note: Please test it first on Chromium Browsers (Google Chrome, Arc etc.)!
Checklist
General
Server
Motivation and Context
Right now when users want to add an attachment on Chromium browsers (such as Chrome and Arc), they get a 404 Not Found error and their attachment is not uploaded. This is an issue occurring in Chromium's version 130, 132 and it is fix in version 132. However since it may take time until the release, a temporary solution was needed to tackle this problem.
See: https://issues.chromium.org/issues/374550348
Description
In one of the Chromium forum comments, it is stated that a work around can be used by bypassing the service workers.
See: https://issues.chromium.org/issues/374550348#comment20
This is first tried on Chrome by bypassing service workers manually and shown that it is working.
After that Angular's "ngsw-bypass" header flag is found to fix the problem.
See: https://angular.dev/ecosystem/service-workers/devops#bypassing-the-service-worker
Fixes #9732 and #9168 and #9646
Steps for Testing
Prerequisites:
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Code Review
Manual Tests
Summary by CodeRabbit
These changes enhance the file upload experience while maintaining existing functionality.