firebaseをパッケージから削除した，その他バグ回収 - #4
Merged
Merged
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughIDE設定の追加・更新、GradleからGoogle Servicesプラグイン削除、WebViewのエラー処理をメインフレームのみに制限、センシング関連でfileName正規化とRxJava除去（blocking呼び出しへ移行）を行う変更。 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as UI / ViewModel
participant Usecase as SensingUsecase
participant Repo as SensingRepository
participant Sensor as Sensor (各センサー)
UI->>Usecase: start(fileName)
Usecase->>Repo: sensorStart(resolvedFileName, params)
Repo->>Sensor: start(params)
Sensor-->>Repo: started (path)
Repo-->>Usecase: started (paths)
Usecase-->>UI: onStarted(paths)
rect rgba(100,150,255,0.5)
UI->>Usecase: stop()
Usecase->>Repo: sensorStop()
Repo->>Sensor: stop() (blocking)
Sensor-->>Repo: stopped or error
Repo-->>Usecase: listOf(paths or nulls)
Usecase-->>UI: onStopped(list)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.idea/deploymentTargetSelector.xml (1)
1-21: IDE設定ファイルはバージョン管理から除外を検討してください。
.idea/deploymentTargetSelector.xmlはIDEが自動生成するマシン固有の設定ファイルです。以下の理由から.gitignoreに追加することを推奨します：
- タイムスタンプやデバイス選択状態は開発者ごとに異なり、不要なマージコンフリクトの原因になります
- Line 10にデバイスシリアル番号（
serial=48221FDJH0021D）が含まれており、公開リポジトリでは避けるべき情報です📝 .gitignoreへの追加案
+.idea/deploymentTargetSelector.xml🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.idea/deploymentTargetSelector.xml around lines 1 - 21, The committed IDE-generated deploymentTargetSelector XML contains a machine-specific device serial (the DeviceId element with identifier="serial=48221FDJH0021D") and should not be versioned; update the repository ignore rules to exclude IDE-generated deployment target files, stop tracking this file (remove it from the index and commit the removal), and purge the exposed serial from repository history using a history-rewrite tool (e.g., git filter-repo or BFG) so the DeviceId identifier value is removed; after rewriting history, force-push and inform collaborators to rebase/clone to avoid reintroducing the SelectionState runConfigName="app" entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/net/kajilab/elpissender/repository/SensingRepository.kt`:
- Around line 49-59: 現在の sensors.map ブロックは sensor.stop().blockingGet() を Main
スレッドで呼んでおり ANR を誘発するため、blockingGet を使わないように修正してください: sensor.stop() をメインでブロックせずに
I/O スレッドで実行する（たとえば Rx の場合は sensor.stop().subscribeOn(Schedulers.io())
を使って非同期に集約するか、コルーチン化して suspend 関数に変換して await する）ように変更し、SensingUsecase.stop() を
suspend にして SettingViewModel.stopSensing() 側は viewModelScope.launch
内で呼ぶようにしてください（該当シンボル: sensor.stop().blockingGet(), sensors.map, files,
onStopped(files), SensingUsecase.stop(), SettingViewModel.stopSensing(),
viewModelScope.launch）。
In `@app/src/main/java/net/kajilab/elpissender/usecase/SensingUsecase.kt`:
- Around line 182-185: resolveFileName currently only trims and checks blank but
lets dangerous strings like "/" or ".." through; update resolveFileName to
sanitize the input by removing or replacing path separators and traversal
sequences (e.g., "/", "\", "..") and any characters outside a safe whitelist
(letters, digits, dot, underscore, dash), strip leading dots, collapse repeated
dots, then fallback to "sensing_${DateUtils.getNowDate()}" if the sanitized
result is empty; ensure you update the function resolveFileName to perform these
checks and sanitization before returning the filename.
---
Nitpick comments:
In @.idea/deploymentTargetSelector.xml:
- Around line 1-21: The committed IDE-generated deploymentTargetSelector XML
contains a machine-specific device serial (the DeviceId element with
identifier="serial=48221FDJH0021D") and should not be versioned; update the
repository ignore rules to exclude IDE-generated deployment target files, stop
tracking this file (remove it from the index and commit the removal), and purge
the exposed serial from repository history using a history-rewrite tool (e.g.,
git filter-repo or BFG) so the DeviceId identifier value is removed; after
rewriting history, force-push and inform collaborators to rebase/clone to avoid
reintroducing the SelectionState runConfigName="app" entry.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.idea/deploymentTargetSelector.xml.idea/material_theme_project_new.xmlapp/build.gradle.ktsapp/src/main/java/net/kajilab/elpissender/presenter/ui/view/components/WebViewComponent.ktapp/src/main/java/net/kajilab/elpissender/presenter/ui/view/setting/SettingViewModel.ktapp/src/main/java/net/kajilab/elpissender/repository/SensingRepository.ktapp/src/main/java/net/kajilab/elpissender/service/SensingWorker.ktapp/src/main/java/net/kajilab/elpissender/usecase/SensingUsecase.kt
💤 Files with no reviewable changes (1)
- app/build.gradle.kts
Comment on lines
+182
to
+185
| private fun resolveFileName(fileName: String): String { | ||
| val normalized = fileName.trim() | ||
| return normalized.ifBlank { "sensing_${DateUtils.getNowDate()}" } | ||
| } |
There was a problem hiding this comment.
ファイル名正規化が不十分で、危険文字列を通してしまいます。
Line 182-185 は空文字対策のみで、/ や .. などを含む入力を通します。sampleType 由来の値が来る経路もあるため、ファイル操作先の逸脱や失敗リスクがあります。
🔧 修正案（危険文字の除去を追加）
private fun resolveFileName(fileName: String): String {
- val normalized = fileName.trim()
- return normalized.ifBlank { "sensing_${DateUtils.getNowDate()}" }
+ val normalized = fileName.trim().ifBlank { "sensing_${DateUtils.getNowDate()}" }
+ return normalized
+ .replace(Regex("""[\\/:*?"<>|]"""), "_")
+ .replace("..", "_")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private fun resolveFileName(fileName: String): String { | |
| val normalized = fileName.trim() | |
| return normalized.ifBlank { "sensing_${DateUtils.getNowDate()}" } | |
| } | |
| private fun resolveFileName(fileName: String): String { | |
| val normalized = fileName.trim().ifBlank { "sensing_${DateUtils.getNowDate()}" } | |
| return normalized | |
| .replace(Regex("""[\\/:*?"<>|]"""), "_") | |
| .replace("..", "_") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/main/java/net/kajilab/elpissender/usecase/SensingUsecase.kt` around
lines 182 - 185, resolveFileName currently only trims and checks blank but lets
dangerous strings like "/" or ".." through; update resolveFileName to sanitize
the input by removing or replacing path separators and traversal sequences
(e.g., "/", "\", "..") and any characters outside a safe whitelist (letters,
digits, dot, underscore, dash), strip leading dots, collapse repeated dots, then
fallback to "sensing_${DateUtils.getNowDate()}" if the sanitized result is
empty; ensure you update the function resolveFileName to perform these checks
and sanitization before returning the filename.
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
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.
背景
com.google.gms.google-servicesプラグインが有効になっており、google-services.json不足でDebugビルドが失敗していた。fileNameが渡される経路があり、File name cannot be emptyでアプリがクラッシュしていた。Negativeモデルを送信するなどの短時間計測で、計測自体は終わっていても停止完了の通知が UI まで返らず、「計測中」のまま終了しないことがあった。ERR_CLEARTEXT_NOT_PERMITTED発生時にページ全体が見えなくなるケースがあった。実施内容
app/build.gradle.ktsから未使用のcom.google.gms.google-servicesプラグインを削除し、google-services.jsonなしでもDebugビルドできるようにした。SensingUsecaseで空のfileNameをそのまま流さず、空の場合は日時付きのデフォルト名に補完するようにした。SettingViewModel/SensingWorkerからのセンシング開始時に、用途が分かるファイル名を渡すように変更した。SensingRepositoryのsamplingFrequencyバリデーションを見直し、実装で利用している-1.0を有効値として扱うようにした。SensingRepositoryの停止処理を見直し、各センサー停止後に確実にonStoppedを返すように変更した。SensingUsecase側の不要なクリーンアップ呼び出しを外し、停止完了前に後続処理が切れる状態を解消した。WebViewComponentでは main frame の読込失敗のみを画面エラー扱いとし、サブリソース失敗でページ全体を閉じないようにした。Summary by CodeRabbit
リリースノート
バグ修正
その他