feat(apps): track direct download URLs via HEAD polling#630
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
✅ Files skipped from review due to trivial changes (10)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThis PR implements direct URL tracking: schema v18 adds polling metadata, repository polls HEAD (ETag/Last-Modified) to detect changes and persists state, presentation adds sheet/state/actions/viewmodel logic, UI components (menu, bottom sheet, badge) and localized strings/release notes. ChangesDirect URL Tracking Feature
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as DirectUrlBottomSheet
participant VM as AppsViewModel
participant Repo as InstalledAppsRepositoryImpl
participant HTTP as httpClient
participant DAO as InstalledAppDao
User->>UI: Fill URL/name/version/icon
UI->>VM: OnConfirmAddDirectUrl
VM->>Repo: saveDirectUrlApp(pollUrl,...)
Repo->>HTTP: HEAD pollUrl
HTTP-->>Repo: ETag/Last-Modified
Repo->>DAO: getAppByDirectUrl(pollUrl)
DAO-->>Repo: InstalledAppEntity?
Repo->>DAO: updateDirectUrlPollState(...)
Repo-->>VM: InstalledApp
VM-->>UI: close sheet + show snackbar
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
core/presentation/src/commonMain/composeResources/values/strings.xml (1)
738-738: 💤 Low valueOptional: Consider adding a comment header for documentation.
The new direct-URL strings are well-organized but lack a preceding XML comment (like
<!-- Direct URL tracking feature -->). While thedirect_url_prefix makes the strings self-documenting, a comment would improve maintainability for future contributors.📝 Suggested comment header
<string name="add_from_starred_title">Add from starred</string> + <!-- Direct URL tracking feature --> <string name="direct_url_menu_title">Track a direct URL</string>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/presentation/src/commonMain/composeResources/values/strings.xml` at line 738, Add an XML comment header above the block of direct-URL related strings to document the feature for future readers; locate the block using the direct_url_ prefix (and nearby string name add_from_starred_title) and insert a brief comment such as "Direct URL tracking feature" immediately before that group of <string> entries to improve maintainability.core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/InstalledAppsRepositoryImpl.kt (1)
893-899: 💤 Low valueConsider adding a brief comment explaining the change-detection priority.
The
whenblock correctly prioritizes ETag over Last-Modified and treats newly-appearing headers as baseline establishment rather than changes. This is sound HTTP caching semantics, but the multi-branch logic isn't immediately obvious to future readers.A one-line comment like
// ETag takes precedence; new headers establish baseline without flagging changewould make the intent clearer without requiring full KDoc.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/InstalledAppsRepositoryImpl.kt` around lines 893 - 899, Add a one-line comment above the val changed = when { ... } block in InstalledAppsRepositoryImpl explaining the change-detection priority, e.g. note that ETag takes precedence over Last-Modified and that newly-appearing headers (etag or lastModified when the previous value is null) are treated as baseline establishment rather than a detected change; keep it concise (for example: "// ETag takes precedence; new headers establish baseline without flagging change").
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsRoot.kt`:
- Around line 987-1023: Rows for DIRECT_URL apps (InstallSource.DIRECT_URL)
currently still trigger repo navigation which calls
OnNavigateToRepo(appItem.installedApp.repoId) with repoId=0L; update the click
handling in AppItemCard and CompactAppRow so that before invoking
OnNavigateToRepo you check installedApp.installSource !=
InstallSource.DIRECT_URL and either skip the click or call an alternative
handler (e.g., showSnackbar or noop) for DIRECT_URL items; alternatively, when
building lists filter out items with InstallSource.DIRECT_URL so they never
receive the clickable modifier. Ensure you reference AppItemCard, CompactAppRow,
OnNavigateToRepo and installedApp.repoId when making the change.
In
`@feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsViewModel.kt`:
- Around line 657-667: Handlers for AppsAction.OnDirectUrlNameChanged,
OnDirectUrlVersionChanged and OnDirectUrlIconChanged don't clear stale
directUrlError, so after a validation failure the error remains until submit;
update each _state.update call in these handlers (the ones setting
directUrlNameDraft, directUrlVersionDraft and directUrlIconDraft) to also set
directUrlError = null (mirroring the existing URL-change handler) so any
previous direct-URL validation error is cleared when the user edits
name/version/icon.
In
`@feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/components/DirectUrlBottomSheet.kt`:
- Line 59: The three hardcoded placeholder literals in the DirectUrlBottomSheet
composable (the Text placeholders currently set to "https://example.com/app.apk"
at the three placeholder= { Text(...) } sites) must be replaced with localized
string resources; create a resource key (e.g., placeholder_direct_url) in your
i18n files and load it inside DirectUrlBottomSheet using the
platform-appropriate lookup (e.g.,
stringResource(R.string.placeholder_direct_url) or your project's i18n helper)
and use that value for each placeholder Text call so the placeholder is
localized for all locales.
---
Nitpick comments:
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/InstalledAppsRepositoryImpl.kt`:
- Around line 893-899: Add a one-line comment above the val changed = when { ...
} block in InstalledAppsRepositoryImpl explaining the change-detection priority,
e.g. note that ETag takes precedence over Last-Modified and that newly-appearing
headers (etag or lastModified when the previous value is null) are treated as
baseline establishment rather than a detected change; keep it concise (for
example: "// ETag takes precedence; new headers establish baseline without
flagging change").
In `@core/presentation/src/commonMain/composeResources/values/strings.xml`:
- Line 738: Add an XML comment header above the block of direct-URL related
strings to document the feature for future readers; locate the block using the
direct_url_ prefix (and nearby string name add_from_starred_title) and insert a
brief comment such as "Direct URL tracking feature" immediately before that
group of <string> entries to improve maintainability.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c620cea0-7200-49f9-8c8d-31fff223e373
📒 Files selected for processing (44)
core/data/schemas/zed.rainxch.core.data.local.db.AppDatabase/18.jsoncore/data/src/androidMain/kotlin/zed/rainxch/core/data/local/db/initDatabase.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/local/db/migrations/MIGRATION_17_18.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/local/db/AppDatabase.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/local/db/dao/InstalledAppDao.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/local/db/entities/InstalledAppEntity.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/mappers/InstalledAppsMappers.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/InstalledAppsRepositoryImpl.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/model/InstallSource.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/model/InstalledApp.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/repository/InstalledAppsRepository.ktcore/presentation/src/commonMain/composeResources/files/whatsnew/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/ar/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/bn/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/es/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/fr/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/hi/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/it/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/ja/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/ko/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/pl/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/ru/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/tr/18.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/zh-CN/18.jsoncore/presentation/src/commonMain/composeResources/values-ar/strings-ar.xmlcore/presentation/src/commonMain/composeResources/values-bn/strings-bn.xmlcore/presentation/src/commonMain/composeResources/values-es/strings-es.xmlcore/presentation/src/commonMain/composeResources/values-fr/strings-fr.xmlcore/presentation/src/commonMain/composeResources/values-hi/strings-hi.xmlcore/presentation/src/commonMain/composeResources/values-it/strings-it.xmlcore/presentation/src/commonMain/composeResources/values-ja/strings-ja.xmlcore/presentation/src/commonMain/composeResources/values-ko/strings-ko.xmlcore/presentation/src/commonMain/composeResources/values-pl/strings-pl.xmlcore/presentation/src/commonMain/composeResources/values-ru/strings-ru.xmlcore/presentation/src/commonMain/composeResources/values-tr/strings-tr.xmlcore/presentation/src/commonMain/composeResources/values-zh-rCN/strings-zh-rCN.xmlcore/presentation/src/commonMain/composeResources/values/strings.xmlfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsAction.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsRoot.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsState.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/AppsViewModel.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/components/CompactAppRow.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/components/DirectUrlBadge.ktfeature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/components/DirectUrlBottomSheet.kt
| value = state.directUrlDraft, | ||
| onValueChange = { onAction(AppsAction.OnDirectUrlChanged(it)) }, | ||
| label = { Text(stringResource(Res.string.direct_url_field_url)) }, | ||
| placeholder = { Text("https://example.com/app.apk") }, |
There was a problem hiding this comment.
Localize placeholder text instead of hardcoding English literals.
The placeholders on Line 59, Line 77, and Line 86 are hardcoded, so non-English locales will still show English text.
Suggested fix
OutlinedTextField(
value = state.directUrlDraft,
onValueChange = { onAction(AppsAction.OnDirectUrlChanged(it)) },
label = { Text(stringResource(Res.string.direct_url_field_url)) },
- placeholder = { Text("https://example.com/app.apk") },
+ placeholder = { Text(stringResource(Res.string.direct_url_placeholder_url)) },
singleLine = true,
isError = state.directUrlError != null,
modifier = Modifier.fillMaxWidth(),
)
@@
OutlinedTextField(
value = state.directUrlVersionDraft,
onValueChange = { onAction(AppsAction.OnDirectUrlVersionChanged(it)) },
label = { Text(stringResource(Res.string.direct_url_field_version)) },
- placeholder = { Text("1.0.0") },
+ placeholder = { Text(stringResource(Res.string.direct_url_placeholder_version)) },
singleLine = true,
modifier = Modifier.fillMaxWidth(),
)
@@
OutlinedTextField(
value = state.directUrlIconDraft,
onValueChange = { onAction(AppsAction.OnDirectUrlIconChanged(it)) },
label = { Text(stringResource(Res.string.direct_url_field_icon)) },
- placeholder = { Text("https://example.com/icon.png") },
+ placeholder = { Text(stringResource(Res.string.direct_url_placeholder_icon)) },
singleLine = true,
modifier = Modifier.fillMaxWidth(),
)Also applies to: 77-77, 86-86
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@feature/apps/presentation/src/commonMain/kotlin/zed/rainxch/apps/presentation/components/DirectUrlBottomSheet.kt`
at line 59, The three hardcoded placeholder literals in the DirectUrlBottomSheet
composable (the Text placeholders currently set to "https://example.com/app.apk"
at the three placeholder= { Text(...) } sites) must be replaced with localized
string resources; create a resource key (e.g., placeholder_direct_url) in your
i18n files and load it inside DirectUrlBottomSheet using the
platform-appropriate lookup (e.g.,
stringResource(R.string.placeholder_direct_url) or your project's i18n helper)
and use that value for each placeholder Text call so the placeholder is
localized for all locales.
Greptile SummaryThis PR implements direct-download-URL tracking as a new
Confidence Score: 4/5Safe to merge after resolving the open items from prior review threads; no new blocking issues were found in this pass. Several non-trivial issues flagged in earlier rounds (packageName timestamp collision, repoId=0 sentinel conflict, CancellationException swallowed by runCatching, isUpdateAvailable never cleared) remain unaddressed. The new code in this cycle introduces only a minor form-validation UX inaccuracy. InstalledAppsRepositoryImpl.kt — the saveDirectUrlApp / checkDirectUrlForUpdates implementations still carry the open items from prior threads. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant DirectUrlBottomSheet
participant AppsViewModel
participant InstalledAppsRepository
participant HTTP as HEAD (remote URL)
participant DAO as InstalledAppDao
User->>DirectUrlBottomSheet: paste URL + name, tap Track
DirectUrlBottomSheet->>AppsViewModel: OnConfirmAddDirectUrl
AppsViewModel->>AppsViewModel: validate URL scheme + name
AppsViewModel->>InstalledAppsRepository: saveDirectUrlApp(url, name, version, icon)
InstalledAppsRepository->>DAO: getAppByDirectUrl(url)
InstalledAppsRepository->>HTTP: HEAD pollUrl
HTTP-->>InstalledAppsRepository: ETag / Last-Modified (or null)
InstalledAppsRepository->>DAO: insertApp(entity)
InstalledAppsRepository-->>AppsViewModel: InstalledApp
AppsViewModel-->>DirectUrlBottomSheet: dismiss + ShowSuccess snackbar
Note over InstalledAppsRepository,DAO: Update cycle (UpdateCheckWorker)
InstalledAppsRepository->>InstalledAppsRepository: checkForUpdates(entity)
InstalledAppsRepository->>HTTP: HEAD pollUrl
HTTP-->>InstalledAppsRepository: ETag / Last-Modified
InstalledAppsRepository->>DAO: updateDirectUrlPollState(...)
Reviews (3): Last reviewed commit: "Merge branch 'main' into feat/direct-url..." | Re-trigger Greptile |
| } | ||
|
|
||
| val now = System.currentTimeMillis() | ||
| val packageName = "direct-url:$now" |
There was a problem hiding this comment.
packageName collision on same-millisecond saves
packageName = "direct-url:$now" where now = System.currentTimeMillis() gives two rapid saves within the same millisecond an identical primary key. InstalledAppDao.insertApp uses OnConflictStrategy.REPLACE, so the second insert will silently DELETE the first row and replace it, permanently losing the earlier-added app. On some lower-resolution clock devices (or under emulator conditions), same-millisecond collisions are reproducible. Adding a UUID suffix or using the URL hash removes the race entirely.
| val changed = when { | ||
| etag != null && previousEtag != null -> etag != previousEtag | ||
| lastModified != null && previousLastModified != null -> lastModified != previousLastModified | ||
| etag != null && previousEtag == null -> false | ||
| lastModified != null && previousLastModified == null -> false | ||
| else -> false | ||
| } | ||
|
|
||
| installedAppsDao.updateDirectUrlPollState( | ||
| packageName = app.packageName, | ||
| etag = etag ?: previousEtag, | ||
| lastModified = lastModified ?: previousLastModified, | ||
| isUpdateAvailable = changed || app.isUpdateAvailable, | ||
| timestamp = now, | ||
| ) | ||
|
|
||
| return changed |
There was a problem hiding this comment.
isUpdateAvailable flag can never be cleared by polling
updateDirectUrlPollState is always called with isUpdateAvailable = changed || app.isUpdateAvailable. Once the flag is set to true (ETag changed), subsequent polls where changed == false still pass false || true = true, so the update badge is permanent. There is no user-facing action in onAction or DAO path to dismiss/acknowledge the update for a DIRECT_URL app (no clearUpdateMetadata call, no "mark installed" path), so the badge will stick indefinitely after the first detected change. If the user then taps the "Update" row action, updateSingleApp calls appsRepository.getLatestRelease(owner = "", repo = appName), which makes a malformed GitHub API request and produces a confusing error snackbar.
| packageName = packageName, | ||
| repoId = 0L, |
There was a problem hiding this comment.
All direct-URL apps share
repoId = 0L, breaking repo-based lookups
repoId = 0L is a sentinel that doesn't conflict with any real GitHub repo ID, but InstalledAppDao.getAppByRepoId(0) (used by isAppInstalled, getAppByRepoIdAsFlow, getAppsByRepoId) will return an arbitrary DIRECT_URL app — whichever SQLite happens to surface first — rather than something app-specific. After a second DIRECT_URL entry is added, isAppInstalled(0) returns true for any caller that checks repo 0, and getAppByRepoIdAsFlow(0) used by the Details screen would show whichever row comes back first. Consider using a different sentinel (e.g. -1) or a dedicated column/query that callers can distinguish from a real repo lookup.
| Button( | ||
| onClick = { onAction(AppsAction.OnConfirmAddDirectUrl) }, | ||
| enabled = !state.directUrlSaving, | ||
| ) { | ||
| if (state.directUrlSaving) { | ||
| CircularProgressIndicator( | ||
| modifier = Modifier.padding(end = 8.dp), | ||
| strokeWidth = 2.dp, | ||
| ) | ||
| } | ||
| Text(stringResource(Res.string.direct_url_add_button)) | ||
| } |
There was a problem hiding this comment.
CircularProgressIndicator has no explicit size, so it uses the 40 dp default and will overflow the button's internal layout when saving is in progress. Adding Modifier.size(18.dp) keeps it flush with the text height.
| Button( | |
| onClick = { onAction(AppsAction.OnConfirmAddDirectUrl) }, | |
| enabled = !state.directUrlSaving, | |
| ) { | |
| if (state.directUrlSaving) { | |
| CircularProgressIndicator( | |
| modifier = Modifier.padding(end = 8.dp), | |
| strokeWidth = 2.dp, | |
| ) | |
| } | |
| Text(stringResource(Res.string.direct_url_add_button)) | |
| } | |
| Button( | |
| onClick = { onAction(AppsAction.OnConfirmAddDirectUrl) }, | |
| enabled = !state.directUrlSaving, | |
| ) { | |
| if (state.directUrlSaving) { | |
| CircularProgressIndicator( | |
| modifier = Modifier.size(18.dp).padding(end = 8.dp), | |
| strokeWidth = 2.dp, | |
| ) | |
| } | |
| Text(stringResource(Res.string.direct_url_add_button)) | |
| } |
| val (initialEtag, initialLastModified) = runCatching { | ||
| val response = httpClient.head(trimmedUrl) | ||
| response.headers[HttpHeaders.ETag] to response.headers[HttpHeaders.LastModified] | ||
| }.getOrElse { null to null } |
There was a problem hiding this comment.
runCatching catches all Throwable, including CancellationException, which breaks structured concurrency. If the coroutine is cancelled while httpClient.head() is in flight, the exception is swallowed and (null, null) is returned, causing the app to be inserted with no initial ETag/Last-Modified — the first poll will then always see headers as "new" and never detect a change because the comparison falls to the else -> false branch. The private checkDirectUrlForUpdates in the same class already shows the correct pattern.
| val (initialEtag, initialLastModified) = runCatching { | |
| val response = httpClient.head(trimmedUrl) | |
| response.headers[HttpHeaders.ETag] to response.headers[HttpHeaders.LastModified] | |
| }.getOrElse { null to null } | |
| val (initialEtag, initialLastModified) = try { | |
| val response = httpClient.head(trimmedUrl) | |
| response.headers[HttpHeaders.ETag] to response.headers[HttpHeaders.LastModified] | |
| } catch (e: CancellationException) { | |
| throw e | |
| } catch (e: Throwable) { | |
| null to null | |
| } |
Summary
Sprint 3 Task #3. Paste any direct download URL (APK/installer) in the Apps overflow menu; GHS stores it as a new
InstallSource.DIRECT_URLrow and polls the URL each update cycle via HEAD requests. Update is flagged when ETag / Last-Modified changes vs. the previously observed values.Schema: 3 new nullable columns on
installed_apps(directUrlPollUrl,directUrlLastEtag,directUrlLastModified). DB v17 → v18 withMIGRATION_17_18. Synthetic packageNamedirect-url:<ts>keeps the existing PK contract; dedupe by URL before insert.UI: new overflow menu item "Track a direct URL" +
DirectUrlBottomSheet(URL / name / version / optional icon). Apps withinstallSource == DIRECT_URLrender a tertiary "URL" chip in both compact + full rows.checkForUpdates()dispatches tocheckDirectUrlForUpdates(app)when the source is direct-url;checkAllForUpdates()picks it up unchanged →UpdateCheckWorkerpolls direct-URL apps alongside GitHub repos. HEAD failures soft-fail (no badge flap, just bumpslastCheckedAt).Test plan
lastCheckedAtupdates, ETag/Last-Modified persisted.isUpdateAvailable = true.Summary by CodeRabbit
New Features
Improved
Documentation