feat: add in-app guest browser for localhost pages - #221
Conversation
Adds a WebView-based Guest Browser screen reachable from Settings. It lets users open pages served inside the guest runtime (e.g. http://127.0.0.1:PORT/) without leaving the app: URL bar, back/forward/reload, progress indicator and in-app link handling.
|
🔍 OpenCodeReview found 3 issue(s) in this PR.
ℹ️ All inline comments overlapped with existing reviews; nothing new was posted. |
| canGoForward = forward | ||
| }, | ||
| ) | ||
| webView = this |
There was a problem hiding this comment.
[bug · medium]
WebViewはネイティブリソースを保持するため、画面離脱時にdestroy()を呼ぶ必要がありますが、本実装ではDisposableEffect等による破棄処理がありません。ComposeのAndroidViewはcomposableツリーから外れても自動的にdestroy()を呼ばないため、画面を開くたびにWebViewとそのネイティブリソースがリークします。また破棄後もwebViewのstate参照が残るため、BackHandler等で破棄済みインスタンスを参照する恐れもあります。DisposableEffectでonDispose内にstopLoading()→webChromeClient/webViewClientをnull化→destroy()→webView=nullの一連の処理を追加することを推奨します。
| IconButton(enabled = canGoBack, onClick = onHistoryBack) { | ||
| Icon( | ||
| Icons.AutoMirrored.Filled.ArrowBack, | ||
| contentDescription = stringResource(R.string.guest_browser_back), | ||
| ) | ||
| } |
There was a problem hiding this comment.
[style · low]
navigationIcon(画面を閉じる)とactions内の履歴戻るボタン(WebView履歴を戻る)の両方に同一のIcons.AutoMirrored.Filled.ArrowBackを使用しているため、視覚的に操作を区別できません。履歴戻る側のアイコンを別のもの(例: Icons.Default.UndoやArrowUpward)に差し替える等して、視覚的・アクセシビリティ上で「画面を閉じる」と「履歴を戻る」を区別できるようにすることを推奨します。
| } | ||
| } | ||
|
|
||
| private fun normalizeUrl(raw: String): String { |
There was a problem hiding this comment.
[bug · medium]
normalizeUrlは空文字や空白のみの入力を "http://" として返すため、WebViewが不正なURLをロードしてエラーページを表示します。また javascript: 等のスキームを含む入力は先頭に http:// が付与され、予期しない挙動になります。入力の空チェック(空の場合はGoボタンを無効化する等)と、Uri.parse等によるスキーム/ホストの検証を行い、不正な入力はloadUrlしないようにすることを推奨します。
| onNavigationStateChange: (url: String, canGoBack: Boolean, canGoForward: Boolean) -> Unit, | ||
| ) { | ||
| // Guest pages (dev servers, dashboards, tool UIs) are interactive web apps that need JS. | ||
| settings.javaScriptEnabled = true |
There was a problem hiding this comment.
[security · high]
この画面はゲストランタイム用(例: http://127.0.0.1:PORT/)ですが、URLバーから入力された任意のURLをバリデーションなしでloadUrlし、かつjavaScriptEnabled=true / domStorageEnabled=trueを無条件で有効化しています。またページ内リンクやリダイレクトによる外部サイトへの遷移を制限するshouldOverrideUrlLoadingの実装もないため、悪意のあるページ上でJavaScriptが実行され、フィッシングやローカルデータへの不正アクセスにつながるリスクがあります。プロジェクトには既にOpenCodeUrl.normalize()(クリアテキストHTTPを信頼済みホストに制限)というセキュリティ検証があるため、URLバー入力時はスキーム/ホストを検証し、shouldOverrideUrlLoadingで信頼できるローカルオリジン(localhost/127.0.0.1)以外への遷移を遮断するか、外部オリジンではJSを無効化するなどの対策を推奨します。
| } | ||
| } | ||
| webViewClient = object : WebViewClient() { | ||
| override fun onPageFinished(view: WebView?, url: String?) { |
There was a problem hiding this comment.
[bug · medium]
canGoBack/canGoForward/urlInputの更新がonPageFinished時のみのため、ページ読み込み中やgoBack()実行直後はUIの履歴状態が実際の状態と食い違います。またonPageFinishedでurlInputが強制的に現在URLへ上書きされるため、ユーザーがURL欄を編集中に入力テキストが失われることがあります。onPageStartedやonProgressChangedで早期に履歴状態を更新するか、編集中のurlInputは上書きしない等の対策を推奨します。
|
|
||
| composable(ROUTE_GUEST_BROWSER) { | ||
| GuestBrowserScreen( | ||
| initialUrl = app.localRuntimeManager.installedPort()?.let { "http://127.0.0.1:$it/" }.orEmpty(), |
There was a problem hiding this comment.
[performance · low]
installedPort() は内部で readMetadata() を呼び、metadata.json を同期的にディスクから読み込みます。この式は composable ブロック内で評価されるため、Navigation がこのディスティネーションを再コンポーズするたびにメインスレッド上でファイル I/O が再実行されます。一方で URL の値は GuestBrowserScreen の WebView factory で一度だけ消費されるため、再読み込みは実質無駄になります。remember で一度だけ計算することを推奨します(ナビゲーション時に毎回最新のポートを読む必要がある場合は、LocalRuntimeManager.state の LocalRuntimeStatus.Ready.port を利用する方が Compose の状態と整合します)。
Suggestion:
| initialUrl = app.localRuntimeManager.installedPort()?.let { "http://127.0.0.1:$it/" }.orEmpty(), | |
| val initialUrl = remember { app.localRuntimeManager.installedPort()?.let { "http://127.0.0.1:$it/" }.orEmpty() } | |
| GuestBrowserScreen( | |
| initialUrl = initialUrl, | |
| onBack = { navController.popBackStack() }, | |
| ) |
| <string name="cd_pull_request_collapse">Show fewer pull requests</string> | ||
| <string name="guest_browser_title">Guest Browser</string> | ||
| <string name="guest_browser_row">Guest Browser</string> | ||
| <string name="guest_browser_url_hint">http://127.0.0.1:port</string> |
There was a problem hiding this comment.
[maintainability · low]
URL形式の例示文字列(ヒント)が通常の翻訳対象になっています。その結果、他言語版では「http://127.0.0.1:ポート」「http://127.0.0.1:端口」のようにURL中に非ASCIIの「port」訳語が埋め込まれ、ユーザーが実際のURLを入力する際の見本として不自然になります(「端口」等をそのまま入力してしまう懸念もあります)。技術的な書式の見本は translatable="false" にするか、プレースホルダー(例: http://127.0.0.1:%1$s)を使うのが望ましいです。
| webView = this | ||
| if (initialUrl.isNotBlank()) { | ||
| loadUrl(normalizeUrl(initialUrl)) | ||
| } |
There was a problem hiding this comment.
[bug · high]
AndroidView の factory 内で生成した WebView を Compose の state(webView)に保持していますが、画面離脱時の破棄処理(AndroidView の onRelease での WebView.destroy()、または DisposableEffect によるクリーンアップ)がありません。Compose のライフサイクルと同期しないため、GuestBrowserScreen を離れても WebView のネイティブリソースやレンダリングスレッドが解放されず、メモリリークや破棄後の WebView 操作によるクラッシュにつながります。また factory 内(Composition 中)に mutableStateOf への書込みを行うため、余分な再コンポジションも発生します。AndroidView に onRelease = { it.destroy() } を追加し、破棄を明示することを推奨します。
Suggestion:
| webView = this | |
| if (initialUrl.isNotBlank()) { | |
| loadUrl(normalizeUrl(initialUrl)) | |
| } | |
| factory = { context -> | |
| WebView(context).apply { | |
| layoutParams = ViewGroup.LayoutParams( | |
| ViewGroup.LayoutParams.MATCH_PARENT, | |
| ViewGroup.LayoutParams.MATCH_PARENT, | |
| ) | |
| configure( | |
| onProgress = { progress = it }, | |
| onNavigationStateChange = { url, back, forward -> | |
| urlInput = url | |
| canGoBack = back | |
| canGoForward = forward | |
| }, | |
| ) | |
| webView = this | |
| if (initialUrl.isNotBlank()) { | |
| loadUrl(normalizeUrl(initialUrl)) | |
| } | |
| } | |
| }, | |
| onRelease = { it.destroy() }, |
| private fun normalizeUrl(raw: String): String { | ||
| val trimmed = raw.trim() | ||
| return if (trimmed.startsWith("http://") || trimmed.startsWith("https://")) { | ||
| trimmed | ||
| } else { | ||
| "http://$trimmed" | ||
| } | ||
| } |
There was a problem hiding this comment.
[bug · low]
normalizeUrl() は空文字や空白のみの入力を「http://」に変換するため、アドレスバーを空にして Go ボタンを押すと不正な URL で loadUrl() が呼ばれエラーになります。また http/https 以外のスキーム(例: javascript:)にも先頭へ http:// を付与した誤った URL を生成します。isBlank() チェックで入力を検証し、不正な入力は無視するかデフォルト URL にフォールバックしてください。
Suggestion:
| private fun normalizeUrl(raw: String): String { | |
| val trimmed = raw.trim() | |
| return if (trimmed.startsWith("http://") || trimmed.startsWith("https://")) { | |
| trimmed | |
| } else { | |
| "http://$trimmed" | |
| } | |
| } | |
| private fun normalizeUrl(raw: String): String { | |
| val trimmed = raw.trim() | |
| return when { | |
| trimmed.isBlank() -> trimmed | |
| trimmed.startsWith("http://") || trimmed.startsWith("https://") -> trimmed | |
| else -> "http://$trimmed" | |
| } | |
| } |
| // Guest pages (dev servers, dashboards, tool UIs) are interactive web apps that need JS. | ||
| settings.javaScriptEnabled = true | ||
| settings.domStorageEnabled = true |
There was a problem hiding this comment.
[security · medium]
この画面はゲストランタイムのローカルページ(http://127.0.0.1:PORT/)の表示を想定していますが、アドレスバーから任意の外部サイトへ遷移でき、かつ JavaScript / DOM Storage を無条件で有効化しています。想定外の外部サイトが読み込まれた場合、JS 経由で WebView の Cookie や DOM Storage にアクセスでき、フィッシングやアプリ内データへの攻撃面になります。外部サイトへの遷移を意図しないのであれば WebViewClient.shouldOverrideUrlLoading 等でゲストランタイムのオリジンに遷移を制限するか、少なくとも遷移先のホスト検証と allowFileAccess の無効化を検討してください。
| override fun onPageFinished(view: WebView?, url: String?) { | ||
| onProgress(100) | ||
| onNavigationStateChange( | ||
| url.orEmpty(), | ||
| view?.canGoBack() == true, | ||
| view?.canGoForward() == true, | ||
| ) | ||
| } |
There was a problem hiding this comment.
[bug · medium]
canGoBack / canGoForward と URL バーの表示は onPageFinished でのみ更新されます。ページ読込中やリダイレクト中はボタンの有効状態が古いままになり、読込失敗(onReceivedError 等)で onPageFinished が呼ばれないケースでは状態が更新されず、進む/戻るの可否やアドレスバーの URL が実際のページと食い違います。onPageStarted(または doUpdateVisitedHistory)でもナビゲーション状態を更新し、onReceivedError 時のハンドリングを追加してください。
| initialUrl: String, | ||
| onBack: () -> Unit, | ||
| ) { | ||
| var urlInput by remember { mutableStateOf(initialUrl) } |
There was a problem hiding this comment.
[bug · medium]
remember で保持しているため、画面回転などの設定変更で URL 入力値・進捗・履歴状態がすべてリセットされ、ブラウザが初期 URL に戻ってしまいます。urlInput は rememberSaveable に変更するか、WebView の onSaveInstanceState/restoreState を利用して設定変更後も状態を復元できるようにしてください。
Suggestion:
| var urlInput by remember { mutableStateOf(initialUrl) } | |
| var urlInput by rememberSaveable { mutableStateOf(initialUrl) } |
| AndroidView( | ||
| modifier = Modifier.weight(1f), | ||
| factory = { context -> | ||
| WebView(context).apply { |
There was a problem hiding this comment.
[bug · high]
WebView のライフサイクル管理が欠落しています。AndroidView に onRelease を指定していないため、この画面が composition から離脱しても WebView が destroy() されず、画面遷移を繰り返すたびに WebView インスタンスがリークします。また、Activity の onPause/onResume に追従する webView.onPause()/onResume() も呼ばれていないため、バックグラウンド時も CPU・バッテリーを消費し続けるリスクがあります。onRelease = { it.destroy() } を指定し、必要に応じて LifecycleEventObserver 等で onPause/onResume を連携してください。
Suggestion:
| AndroidView( | |
| modifier = Modifier.weight(1f), | |
| factory = { context -> | |
| WebView(context).apply { | |
| AndroidView( | |
| modifier = Modifier.weight(1f), | |
| factory = { context -> | |
| WebView(context).apply { | |
| // ... | |
| } | |
| }, | |
| onRelease = { it.destroy() }, | |
| ) |
| TopAppBar( | ||
| title = { Text(stringResource(R.string.guest_browser_title)) }, | ||
| navigationIcon = { | ||
| IconButton(onClick = onBack) { |
There was a problem hiding this comment.
[bug · medium]
システムバックと画面上部の戻るボタンで動作が不統一です。BackHandler は履歴があれば goBack() しますが、TopAppBar の navigationIcon は常に onBack() を呼ぶため、履歴が残っている状態で上部の戻るボタンを押すと画面が即座に閉じてしまいます。両者を同じ挙動(履歴があれば戻る、なければ画面を閉じる)に統一してください。
Suggestion:
| IconButton(onClick = onBack) { | |
| IconButton(onClick = { if (canGoBack) onHistoryBack() else onBack() }) { |
| composable(ROUTE_GUEST_BROWSER) { | ||
| GuestBrowserScreen( | ||
| initialUrl = app.localRuntimeManager.installedPort()?.let { "http://127.0.0.1:$it/" }.orEmpty(), |
There was a problem hiding this comment.
[bug · medium]
このファイルのコメント(32〜34行目)にも記載されている通り、NavHost は destination のラムダを記憶するため、グラフ構築時に読み込んだ値は固定されてしまいます。ここで installedPort() を即座に評価すると、ユーザーが後からゲストブラウザを開いた時点では古い(あるいはまだインストール前で null の)ポートがキャプチャされ、http://127.0.0.1:/ のように無効な URL を読み込む恐れがあります。他の箇所と同じく getter(例: app.localRuntimeManager::installedPort)を渡し、画面側で読み取るのが安全です。
Adds a WebView-based Guest Browser screen (Settings > Guest Browser) for opening pages served inside the guest runtime (e.g. http://127.0.0.1:PORT/).