[feat] add NetworkRequestDetailBottomSheet #76

Manabu-GT · 2025-12-05T00:52:14Z

add NetworkRequestDetailBottomSheet to show captured network request details

…equest details

claude · 2025-12-05T00:52:29Z

Claude finished @Manabu-GT's task —— View job

Code Review Complete ✅

I've reviewed this PR which adds a comprehensive NetworkRequestDetailBottomSheet to display captured network request details. Overall, this is a well-structured and feature-rich implementation with good attention to performance and UX. The code quality is high with clear separation of concerns.

Summary

What's New:

7 new utility files (TextType, UrlParts, Badges, Texts, Colors, Formatters, HttpStatusCodes)
Comprehensive bottom sheet UI with Overview, Headers, and Body tabs
Smart text rendering with 4-tier performance optimization
Syntax highlighting for JSON/HTML responses
Copy-to-clipboard functionality throughout

Architecture: The refactoring cleanly separates concerns—badge components, text utilities, and formatters are well-organized and reusable.

Issues Found

🔴 Security Issues

WebView File Access (Texts.kt:163)
- JavaScript is properly disabled ✓
- Missing: allowFileAccess = false and allowContentAccess = false
- Risk: Potential file:// access vulnerabilities when loading HTML from network responses
- Recommendation: Add these settings to harden WebView security
HTML Escaping Incomplete (Formatters.kt:44)
- Missing single quote escaping (' → ')
- Could lead to attribute injection in edge cases
- Recommendation: Add single quote escaping or use a well-tested HTML escaping library

🟡 Major Issues

Regex Performance Concerns (Formatters.kt:53)
- Pattern [^&]+ could cause catastrophic backtracking with adversarial input
- Will fail on JSON keys containing & (e.g., "foo&bar")
- Running 5 regex replacements on large JSON (up to 500KB) may be slow
- Recommendation: Use a proper JSON formatter library (kotlinx.serialization) or limit syntax highlighting to smaller payloads
HTML Detection Case Sensitivity (TextType.kt:28)
- Checks "<!DOCTYPE html" but HTML is case-insensitive
- Will miss <!DOCTYPE HTML>, <HTML>, etc.
- Recommendation: Use ignoreCase = true in startsWith() calls

🟢 Minor Issues

Code Duplication (NetworkRequestDetailBottomSheet.kt:196)
- Clipboard copying logic repeated 5+ times
- Recommendation: Extract to helper function for maintainability
Performance Threshold Validation (Texts.kt:46)
- LARGE_TEXT_THRESHOLD = 500K but DEFAULT_MAX_BODY_SIZE = 2MB
- May want to validate these thresholds with real-world usage
- Consider telemetry to understand if truncation tier is commonly hit

Positive Observations ✨

Excellent performance strategy with 4-tier text rendering (Compose → WebView → TextView → Truncated)
Security-conscious: JavaScript disabled in WebView, header/query param redaction
Good error handling: Network errors displayed with stack traces
Accessibility: Clear visual hierarchy, proper color semantics for status codes
Well-documented: Helpful comments explaining design decisions (e.g., LazyColumn scrolling)
Consistent styling: Uses Material3 theming throughout
Memory-conscious: Proper truncation for very large responses

Testing Recommendations

Per CLAUDE.md section 4, for core runtime changes:

./gradlew :debugoverlay-core:check

Manual Testing Checklist:

Test with various response types (JSON, HTML, XML, plain text, binary)
Test with very large responses (> 500KB, > 2MB)
Verify truncation UI and copy functionality works
Test WebView rendering with malformed JSON/HTML
Verify header redaction works correctly
Test error display with network failures
Verify all 3 tabs (Overview, Headers, Body) render correctly

Recommendations Priority

High Priority (Security):

Add allowFileAccess = false and allowContentAccess = false to WebView
Fix HTML escaping to include single quotes

Medium Priority (Robustness):
3. Fix regex patterns in JSON syntax highlighting or use proper formatter
4. Make HTML detection case-insensitive

Low Priority (Maintainability):
5. Extract clipboard helper function
6. Consider renaming performance threshold constants

Verdict

Approve with minor fixes recommended. The implementation is solid and production-ready. The security issues identified are low-risk in this debug-only context, but should be addressed as a best practice. The regex performance concerns are worth monitoring but unlikely to cause issues with typical API responses.

Great work on the comprehensive feature implementation! 🎉

claude · 2025-12-05T00:54:09Z

debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/ui/Texts.kt

+    AndroidView(
+      factory = { context ->
+        WebView(context).apply {
+          settings.javaScriptEnabled = false // Security: No JS needed


Security: JavaScript should remain disabled

Good security practice to disable JavaScript! However, consider also adding:

settings.allowFileAccess = false settings.allowContentAccess = false

This prevents potential file:// access vulnerabilities, especially since you're loading HTML from untrusted network responses.

debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/ui/Texts.kt

debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/data/TextType.kt

claude · 2025-12-05T00:54:12Z

debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/util/Formatters.kt

+/**
+ * Format JSON as syntax-highlighted HTML.
+ */
+@Suppress("MaxLineLength") // HTML template


Security: HTML escaping needs review

The HTML escaping here only handles basic entities (&, <, >, "). However:

Missing single quote: Should also escape ' → ' or '

Order matters: The & replacement should happen first (which it does ✓), otherwise you'd double-escape.

Consider using a well-tested HTML escaping library instead of manual regex, or at minimum add single quote escaping:

.replace("'", "'")

claude · 2025-12-05T00:54:12Z

debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/util/Formatters.kt

+    .replace("\"", "&quot;")
+
+  // Simple syntax highlighting with regex
+  val highlighted = escaped


Code Quality: Regex performance and correctness

These regex patterns for syntax highlighting have some issues:

Backtracking risk: The pattern """"[^&]+"""" could cause catastrophic backtracking with malicious input containing many & characters.

Incorrect matching: The key pattern """("[^&]+")\s*:""" will fail on keys like "foo&bar" because [^&] excludes ampersands.

Performance: Running 5 separate regex replacements on potentially large JSON (up to 500KB) could be slow.

Consider:

Use a proper JSON formatter library (e.g., kotlinx.serialization with pretty print)

Or use more robust regex patterns: """"([^&"]|&[^&]*?;)+""""

Or limit syntax highlighting to small JSON only

claude · 2025-12-05T00:54:13Z

...re/src/main/kotlin/com/ms/square/debugoverlay/internal/ui/NetworkRequestDetailBottomSheet.kt

+      Row(horizontalArrangement = Arrangement.spacedBy(4.dp)) {
+        IconButton(onClick = {
+          scope.launch {
+            val clipboardLabel = context.getString(R.string.debugoverlay_clipboard_label)


Consistency: Consider reusing clipboard logic

The clipboard copying logic appears multiple times (here, lines 244-248, 328-334, 364-370, 412-417, 437-442). Consider extracting to a helper function:

private fun CoroutineScope.copyToClipboard( clipboard: ClipboardManager, context: Context, text: String ) { launch { val clipboardLabel = context.getString(R.string.debugoverlay_clipboard_label) val clipEntry = ClipEntry(ClipData.newPlainText(clipboardLabel, text)) clipboard.setClipEntry(clipEntry) } }

This would reduce code duplication and make future changes easier.

claude · 2025-12-05T00:54:14Z

...re/src/main/kotlin/com/ms/square/debugoverlay/internal/ui/NetworkRequestDetailBottomSheet.kt

+          shape = RoundedCornerShape(8.dp),
+          color = MaterialTheme.colorScheme.surfaceContainerLowest
+        ) {
+          // No verticalScroll - LazyColumn parent handles vertical scrolling


Accessibility: Consider adding verticalScroll comment justification

Good comment explaining why verticalScroll is not used! For clarity, you might also mention that this is inside a LazyColumn item, which already handles scrolling.

claude · 2025-12-05T02:26:17Z

Claude finished @Manabu-GT's task —— View job

Code Review Complete ✅

I've reviewed the latest changes (commit 4e64214 "cr fixes") which addressed all the feedback from my previous review. Excellent work! 🎉

Summary of Changes in Latest Commit

The author made comprehensive improvements addressing all previous review feedback:

✅ Security fixes implemented
- ~~Removed WebView entirely~~ - simplified approach eliminates all WebView security concerns
✅ Code quality improvements
- Extracted clipboard logic to reusable helper function Clipboards.kt:9
- Replaced regex-based JSON syntax highlighting with kotlinx.serialization library (Formatters.kt:47-52)
- Made HTML detection case-insensitive (TextType.kt:28-31)
- Renamed performance thresholds for clarity (COMPOSE_TEXT_MAX_SIZE, TEXT_VIEW_MAX_SIZE)
✅ Architecture simplification
- Removed WebView-based formatted view entirely
- Simplified to 3-tier text rendering: Compose Text → TextView → Truncated
- JSON formatting now uses proper library instead of HTML rendering
- Removed unused HTML formatting code

Changes Analysis

New Files Added

Clipboards.kt - Clipboard helper function (addresses code duplication issue ✅)

Major Refactorings

Formatters.kt (simplified from ~90 lines to ~52 lines)

Removed formatJsonAsHtml() and formatPlainTextAsHtml()
Added proper JSON formatting using kotlinx.serialization.json library
Much more robust than regex-based approach ✅

Texts.kt (simplified from ~260 lines to ~173 lines)

Removed WebViewTextPreview() and StructuredTextPreview()
Simplified TextPreview() to 3-tier strategy
JSON formatting now happens via library before TextView rendering
All clipboard operations now use helper function ✅

TextType.kt

HTML detection now case-insensitive with ignoreCase = true ✅

NetworkRequestDetailBottomSheet.kt

All clipboard operations refactored to use copyToClipboard() helper ✅
Removed unused parameters

Verification of Previous Issues

Issue	Status	Notes
🔴 WebView security (file access)	✅ Fixed	WebView removed entirely - better solution!
🔴 HTML escaping incomplete	✅ Fixed	HTML formatting removed - no longer needed
🟡 Regex performance concerns	✅ Fixed	Replaced with `kotlinx.serialization` library
🟡 HTML detection case sensitivity	✅ Fixed	Now uses `ignoreCase = true`
🟢 Code duplication (clipboard)	✅ Fixed	Extracted to `copyToClipboard()` helper
🟢 Performance threshold naming	✅ Fixed	Renamed to `COMPOSE_TEXT_MAX_SIZE`, `TEXT_VIEW_MAX_SIZE`

Current Implementation Assessment

Architecture (Simplified & Improved):

Small text (< 10K)   → Compose Text
Medium/Large (< 500K) → TextView (with JSON formatting via library)
Very Large (≥ 500K)   → Truncated Compose Text + Copy button

Key Improvements:

Security: Eliminated WebView entirely - no JavaScript, file access, or HTML escaping concerns
Performance: kotlinx.serialization is battle-tested and efficient
Maintainability: ~180 lines of code removed, single clipboard helper
Robustness: Proper JSON parsing with graceful fallback for malformed JSON
Simplicity: Users no longer need to toggle between "Raw" and "Formatted" views

Remaining Observations (Minor)

No blocking issues remain! Just a few minor notes for consideration:

Dependency Added: kotlinx-serialization-json:1.9.0 (~1MB) - reasonable trade-off for robustness ✅
JSON-only Formatting: Only JSON gets formatted now (not HTML/XML). This is a reasonable design choice since JSON is the most common API response format. HTML is rarely pretty-printed anyway.
TextView Rendering: For very large JSON (100K-500K), the formatted output might be expensive to render in TextView. Consider:
- Adding a max size for JSON formatting (e.g., only format JSON < 100K)
- Or keeping as-is and monitoring performance in production
Current approach is reasonable - can optimize later if needed.

Testing Recommendations

Per CLAUDE.md section 4:

./gradlew :debugoverlay-core:check

Manual Testing Checklist:

Architecture simplified and code compiles ✓
Test JSON formatting with valid/malformed JSON
Test with very large JSON responses (> 100KB)
Test non-JSON responses (HTML, XML, plain text)
Verify all clipboard operations work correctly
Test truncation UI for very large responses

Final Verdict

✅ APPROVE - Ready to merge!

The refactoring addressed all previous feedback comprehensively and resulted in a simpler, more secure, and more maintainable implementation. The author made excellent architectural decisions:

Removing WebView eliminated an entire class of security concerns
Using kotlinx.serialization provides robust JSON handling
Code is now ~180 lines shorter and easier to understand
No behavioral regressions

This is exemplary code review follow-through. Great work! 🚀

Diff Summary

9 files changed, 70 insertions(+), 250 deletions(-)

Key changes:

Added: Clipboards.kt (clipboard helper)
Refactored: Formatters.kt, Texts.kt (simplified)
Fixed: TextType.kt (case-insensitive HTML detection)
Updated: build.gradle.kts (added kotlinx-serialization)

[feat] add NetworkRequestDetailBottomSheet to show captured network r…

da6fcc2

…equest details

claude bot reviewed Dec 5, 2025

View reviewed changes

debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/ui/Texts.kt Outdated Show resolved Hide resolved

claude bot reviewed Dec 5, 2025

View reviewed changes

debugoverlay-core/src/main/kotlin/com/ms/square/debugoverlay/internal/data/TextType.kt Outdated Show resolved Hide resolved

claude bot reviewed Dec 5, 2025

View reviewed changes

cr fixes

4e64214

Manabu-GT merged commit 1ea1854 into main Dec 5, 2025
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[feat] add NetworkRequestDetailBottomSheet #76

[feat] add NetworkRequestDetailBottomSheet #76

Uh oh!

Manabu-GT commented Dec 5, 2025

Uh oh!

claude bot commented Dec 5, 2025 •

edited

Loading

Uh oh!

claude bot Dec 5, 2025

Uh oh!

Uh oh!

Uh oh!

claude bot Dec 5, 2025

Uh oh!

claude bot Dec 5, 2025

Uh oh!

claude bot Dec 5, 2025

Uh oh!

claude bot Dec 5, 2025

Uh oh!

claude bot commented Dec 5, 2025 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

[feat] add NetworkRequestDetailBottomSheet #76

[feat] add NetworkRequestDetailBottomSheet #76

Uh oh!

Conversation

Manabu-GT commented Dec 5, 2025

Uh oh!

claude bot commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Complete ✅

Summary

Issues Found

🔴 Security Issues

🟡 Major Issues

🟢 Minor Issues

Positive Observations ✨

Testing Recommendations

Recommendations Priority

Verdict

Uh oh!

claude bot Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

claude bot Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

claude bot Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

claude bot Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

claude bot Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

claude bot commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Complete ✅

Summary of Changes in Latest Commit

Changes Analysis

New Files Added

Major Refactorings

Verification of Previous Issues

Current Implementation Assessment

Remaining Observations (Minor)

Testing Recommendations

Final Verdict

Diff Summary

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

claude bot commented Dec 5, 2025 •

edited

Loading

claude bot commented Dec 5, 2025 •

edited

Loading