-
Notifications
You must be signed in to change notification settings - Fork 3
Add custom user agent to GutenbergKit WebViews (CMM-982) #255
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
base: trunk
Are you sure you want to change the base?
Conversation
android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt
Outdated
Show resolved
Hide resolved
dcalhoun
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot be sure to format (make format) and lint (make lint-js) the code. Also, address any CI failures.
Addressed in commit 6dbe307. The Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot CI tasks are still failing. Fix them. |
Fixed in commit 08c19e3. The issue was that the generated iOS version file used |
|
@copilot the Swift CI task is failing. Fix it. |
Fixed in commit 6f3aef8. The issue was a module name conflict - the generated enum was named |
jkmassel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Swift, could we use applicationNameForUserAgent?
|
@copilot address the feedback in #255 (review) and fix the failing CI tasks. |
Addressed in commit 3cfa4e4. Implemented @jkmassel's suggestion to use
Great suggestion! Implemented in commit 3cfa4e4. Now using |
|
@copilot please use Swift Testing instead of XCTest and evaluate that running `navigator.userAgent in the web view actually emits the expected value |
|
@copilot address #255 (comment). Also, the CI tests are still failing. Please review the details of the test failures via the logs and fix the failures. |
|
Claude finished @dcalhoun's task in 1m 20s —— View job Code Review CompleteI've conducted a comprehensive code review of PR #255. Overall, the implementation is well-designed and follows best practices. The code quality is excellent, and CI is passing. Below are my detailed findings: ✅ Strengths
🔍 Issues Found1. Critical: Test Logic Error (ios/Tests/GutenbergKitTests/GBWebViewTests.swift:16)The #expect(GBKVersion.version.isEmpty, "Version constant should not be empty")This test expects the version to be empty, but the message says it should NOT be empty. The test will fail when the version string is correctly populated. Fix: #expect(!GBKVersion.version.isEmpty, "Version constant should not be empty")2. Minor: Missing Test Coverage (Android)Android tests verify the user agent string is set during initialization, but don't actually evaluate Recommendation: Consider adding a JavaScript evaluation test similar to iOS's 3. Documentation: Auto-Generated Files (bin/generate-version.js)The generated files have comments saying "Do not edit manually", but there's no Consideration: Document whether generated files should be committed (current approach) or added to 🎯 Code Quality Assessment
📋 Recommendations
🎉 SummaryThis is a high-quality implementation that solves the problem elegantly. The single critical issue is the test logic error which will cause false positives. Once that's fixed, this PR is ready to merge. Recommendation: Fix the test logic error, then approve and merge. | |
Co-authored-by: dcalhoun <[email protected]>
Co-authored-by: dcalhoun <[email protected]>
Co-authored-by: dcalhoun <[email protected]>
Co-authored-by: dcalhoun <[email protected]>
Co-authored-by: dcalhoun <[email protected]>
Co-authored-by: dcalhoun <[email protected]>
Co-authored-by: dcalhoun <[email protected]>
Co-authored-by: dcalhoun <[email protected]>
Co-authored-by: dcalhoun <[email protected]>
Co-authored-by: dcalhoun <[email protected]>
Co-authored-by: dcalhoun <[email protected]>
Co-authored-by: dcalhoun <[email protected]>
Co-authored-by: dcalhoun <[email protected]>
This is a value on the WebView configuration, not the WebView itself.
Address iOS build failures.
Configuration updates have no impact on existing WebViews.
62ab681 to
5996461
Compare
The npm postinstall hook generates the required native files. Also, it does not appear the Swift tests need the build output, so it's replaced with solely the npm dependencies target.
Ignoring these files caused CI failures with CodeQL managed by GitHub. We could possibly add custom CodeQL configuration to ensure the version files are generated for these CI tasks, but the simplest fix is tracking them for the time being.
What?
Adds
GutenbergKit/{version}identifier to HTTP requests on iOS and Android, covering both WebView requests and native HTTP requests. Version is sourced frompackage.json.Why?
Requests from the editor are indistinguishable from regular browser traffic, making debugging and analytics difficult. A custom user agent enables identification of GutenbergKit requests in server logs and network traces.
How?
Version Management:
bin/generate-version.jsscript that reads version frompackage.json(single source of truth)GutenbergKitVersion.swiftfor iOS andGutenbergKitVersion.ktfor Androidnpm installvia postinstall hook.gitignoreiOS WebView Requests:
applicationNameForUserAgentonWKWebViewConfigurationbefore WebView creationEditorViewController) and HTML preview WebViews (HTMLWebViewRenderer)Mozilla/5.0 (...) GutenbergKit/0.11.1iOS Native HTTP Requests:
User-Agentheader inEditorHTTPClient.configureRequest()iOS/18.0.0 GutenbergKit/0.11.1Android WebView Requests:
GutenbergKit/{version}touserAgentStringduring WebView initializationMozilla/5.0 (...) GutenbergKit/0.11.1Android Native HTTP Requests:
User-Agentheader inEditorAssetsLibraryfor manifest and asset fetching{defaultUserAgent} GutenbergKit/0.11.1Testing:
navigator.userAgentincludes GutenbergKit via JavaScript evaluationEditorHTTPClientTestsverify native HTTP requests include proper User-Agent headerGutenbergViewTestverifies WebView user agent string includes GutenbergKitFiles changed:
bin/generate-version.js- Script to generate version files from package.json.gitignore- Exclude generated version files from version controlios/Sources/GutenbergKit/Sources/EditorViewController.swift- Set applicationNameForUserAgent before WebView creationios/Sources/GutenbergKit/Sources/Views/HTMLPreview/HTMLWebViewRenderer.swift- Set applicationNameForUserAgent on preview WebViewsios/Sources/GutenbergKit/Sources/EditorHTTPClient.swift- Add User-Agent header to native requestsandroid/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt- Append to userAgentStringandroid/Gutenberg/src/main/java/org/wordpress/gutenberg/EditorAssetsLibrary.kt- Add User-Agent to native requestsios/Tests/GutenbergKitTests/GBWebViewTests.swift- WebView user agent testsios/Tests/GutenbergKitTests/EditorHTTPClientTests.swift- Native HTTP client testsandroid/Gutenberg/src/test/java/org/wordpress/gutenberg/GutenbergViewTest.kt- Android user agent testspackage.json- Added generate-version script and postinstall hookTesting Instructions
npm installto generate version filesGutenbergKit/0.11.1in the User-Agent headermake test-swift-packagemake test-androidAccessibility Testing Instructions
No UI changes—accessibility testing not required.
Screenshots or screencast
Not applicable (network-level change only).
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.