From fccb817381849824a1aa2fbbe96d8fb54317ac67 Mon Sep 17 00:00:00 2001 From: choiminyeong Date: Mon, 28 Sep 2026 13:17:32 +0900 Subject: [PATCH] TASK-047: favorite-push notification accuracy (branch + commit) See tasks/047-favorite-push-notification-accuracy.md. Co-Authored-By: Claude Fable 5.1 --- .../notifications/FavoritePushCheckWorker.kt | 25 ++-- .../notifications/FavoritePushNotifier.kt | 42 ++++--- .../usecase/RepositoryBrowseUseCases.kt | 19 +-- .../main/res/values/strings_favorite_push.xml | 5 +- .../usecase/RepositoryBrowseUseCasesTest.kt | 14 +-- docs/development-log.md | 50 ++++++++ ...047-favorite-push-notification-accuracy.md | 113 ++++++++++++++++++ 7 files changed, 229 insertions(+), 39 deletions(-) create mode 100644 tasks/047-favorite-push-notification-accuracy.md diff --git a/app/src/main/java/com/ubisam/ubgitea/core/notifications/FavoritePushCheckWorker.kt b/app/src/main/java/com/ubisam/ubgitea/core/notifications/FavoritePushCheckWorker.kt index b6b2429..4e341c6 100644 --- a/app/src/main/java/com/ubisam/ubgitea/core/notifications/FavoritePushCheckWorker.kt +++ b/app/src/main/java/com/ubisam/ubgitea/core/notifications/FavoritePushCheckWorker.kt @@ -9,7 +9,9 @@ import com.ubisam.ubgitea.core.auth.protocol.UBGiteaOAuthDefaults import com.ubisam.ubgitea.core.network.OkHttpTransport import com.ubisam.ubgitea.core.settings.AppSettingsSession import com.ubisam.ubgitea.data.repositories.GiteaRepositoryBrowserApi -import com.ubisam.ubgitea.domain.usecase.GetLatestPushEpochMillisUseCase +import com.ubisam.ubgitea.domain.model.RepositoryCoordinates +import com.ubisam.ubgitea.domain.usecase.GetLatestPushUseCase +import com.ubisam.ubgitea.domain.usecase.LatestPush import com.ubisam.ubgitea.domain.usecase.ListFavoritedRepositoriesUseCase import kotlinx.coroutines.CancellationException @@ -35,7 +37,7 @@ class FavoritePushCheckWorker( OkHttpTransport(), AuthSession.tokenProvider(application), ) - val getLatestPush = GetLatestPushEpochMillisUseCase(browser) + val getLatestPush = GetLatestPushUseCase(browser) val listFavorites = ListFavoritedRepositoriesUseCase(browser) val store: LastSeenPushStore = SharedPreferencesLastSeenPushStore(application) val notifier = FavoritePushNotifier(application) @@ -43,24 +45,33 @@ class FavoritePushCheckWorker( return try { val favorites = listFavorites() if (favorites.isEmpty()) return Result.success() - val defaultBranchByCoordinates = favorites.associate { it.coordinates to it.defaultBranch } - val current = favorites.mapNotNull { repository -> + val current: Map = favorites.mapNotNull { repository -> getLatestPush(repository.coordinates)?.let { repository.coordinates to it } }.toMap() - val newlyPushed = FavoritePushDetector().detectNewPushes(current, store.read()) + val newlyPushed = FavoritePushDetector() + .detectNewPushes(current.mapValues { it.value.epochMillis }, store.read()) if (newlyPushed.isNotEmpty()) { notifier.ensureChannel() newlyPushed.forEach { coordinates -> - notifier.notifyNewPush(coordinates, defaultBranchByCoordinates[coordinates]) + val push = current.getValue(coordinates) + // Best-effort: a repository/branch that just received a push should have at + // least one commit to describe, but if this call fails (network hiccup, + // deleted branch race) the notification still fires -- just without a commit + // summary -- rather than losing the whole notification over it. + val latestCommit = runCatching { + browser.listCommits(coordinates, push.branch, path = null, page = 1, pageSize = 1) + .items.firstOrNull() + }.getOrNull() + notifier.notifyNewPush(coordinates, push.branch, latestCommit) } } // Replaces the whole stored snapshot with exactly this run's favorited set -- a // repository dropped from favorites since the last run has its baseline cleared here // rather than lingering forever; re-favoriting it later starts fresh (a first sighting, // not a retroactive notification for whatever happened while it was un-favorited). - store.write(current) + store.write(current.mapValues { it.value.epochMillis }) Result.success() } catch (failure: CancellationException) { throw failure diff --git a/app/src/main/java/com/ubisam/ubgitea/core/notifications/FavoritePushNotifier.kt b/app/src/main/java/com/ubisam/ubgitea/core/notifications/FavoritePushNotifier.kt index 9c55725..00375ac 100644 --- a/app/src/main/java/com/ubisam/ubgitea/core/notifications/FavoritePushNotifier.kt +++ b/app/src/main/java/com/ubisam/ubgitea/core/notifications/FavoritePushNotifier.kt @@ -12,6 +12,7 @@ import androidx.core.app.NotificationManagerCompat import androidx.core.content.ContextCompat import com.ubisam.ubgitea.MainActivity import com.ubisam.ubgitea.R +import com.ubisam.ubgitea.domain.model.CommitSummary import com.ubisam.ubgitea.domain.model.RepositoryCoordinates import com.ubisam.ubgitea.domain.model.RepositoryRef @@ -21,14 +22,13 @@ import com.ubisam.ubgitea.domain.model.RepositoryRef * [FavoritePushCheckWorker] so posting a notification is a small, directly-inspectable seam rather * than one more thing mixed into the worker's network/comparison logic. * - * Deep-links to the repository's default branch when one is known ([defaultBranch] non-null) -- - * not necessarily the specific branch that was actually pushed to, since the detection step this - * worker uses only tracks one scalar "latest push time" per repository, not which branch produced - * it. Opening the repository at its default branch is still a correct, useful "take me to this - * repository" action, and matches where a user manually opening it would usually start anyway. - * With no default branch (e.g. an empty repository), the notification still opens the app, just - * without navigating anywhere specific -- see [MainActivity]'s extras handling for the other half - * of this. + * Deep-links straight to [branch] -- the actual branch [FavoritePushCheckWorker] determined just + * advanced (TASK-047; originally this only had a scalar "latest push time" per repository and fell + * back to the repository's *default* branch, which very often wasn't the branch that was actually + * pushed to -- the human product owner live-reported landing somewhere unrelated to what changed). + * [latestCommit] is best-effort (a fresh fetch that can fail without losing the notification itself, + * see [FavoritePushCheckWorker]) and, when present, lets the notification name the actual commit + * instead of just "check the repository." */ class FavoritePushNotifier(private val context: Context) { fun ensureChannel() { @@ -51,23 +51,33 @@ class FavoritePushNotifier(private val context: Context) { * the worker's own logic and its stored baseline update stay correct regardless -- the user * just doesn't see the notification. */ - fun notifyNewPush(repository: RepositoryCoordinates, defaultBranch: RepositoryRef?) { + fun notifyNewPush(repository: RepositoryCoordinates, branch: RepositoryRef, latestCommit: CommitSummary?) { val hasPermission = ContextCompat.checkSelfPermission(context, Manifest.permission.POST_NOTIFICATIONS) == PackageManager.PERMISSION_GRANTED if (!hasPermission) return val target = Intent(context, MainActivity::class.java) .setFlags(Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_CLEAR_TOP) - if (defaultBranch != null) { - target.putExtra(EXTRA_OWNER, repository.owner) - target.putExtra(EXTRA_NAME, repository.name) - target.putExtra(EXTRA_REF, defaultBranch.value) - } + .putExtra(EXTRA_OWNER, repository.owner) + .putExtra(EXTRA_NAME, repository.name) + .putExtra(EXTRA_REF, branch.value) val openApp = PendingIntent.getActivity(context, repository.hashCode(), target, PendingIntent.FLAG_IMMUTABLE) + val content = latestCommit?.let { + // A commit message's first line is its summary; anything past the first newline is + // detail a notification has no room for. + context.getString(R.string.favorite_push_notification_content, it.authorName, it.message.substringBefore('\n')) + } ?: context.getString(R.string.favorite_push_notification_content_fallback) val notification = NotificationCompat.Builder(context, CHANNEL_ID) .setSmallIcon(R.drawable.ic_notification_push) - .setContentTitle(context.getString(R.string.favorite_push_notification_title, "${repository.owner}/${repository.name}")) - .setContentText(context.getString(R.string.favorite_push_notification_content)) + .setContentTitle( + context.getString( + R.string.favorite_push_notification_title, + "${repository.owner}/${repository.name}", + branch.value, + ), + ) + .setContentText(content) + .setStyle(NotificationCompat.BigTextStyle().bigText(content)) .setAutoCancel(true) .setContentIntent(openApp) .build() diff --git a/app/src/main/java/com/ubisam/ubgitea/domain/usecase/RepositoryBrowseUseCases.kt b/app/src/main/java/com/ubisam/ubgitea/domain/usecase/RepositoryBrowseUseCases.kt index a2e5d00..e0d56ef 100644 --- a/app/src/main/java/com/ubisam/ubgitea/domain/usecase/RepositoryBrowseUseCases.kt +++ b/app/src/main/java/com/ubisam/ubgitea/domain/usecase/RepositoryBrowseUseCases.kt @@ -98,19 +98,24 @@ class LoadSourceFileUseCase( /** * All of a repository's favorited-status-independent branches, paginated to completion, reduced to - * the single timestamp this app treats as "when was this repository last pushed to": the newest - * branch commit time. Extracted as its own use case (TASK-036) specifically so the background + * the one branch this app treats as "when was this repository last pushed to": whichever branch has + * the newest commit time. Extracted as its own use case (TASK-036) specifically so the background * favorite-push checker uses the exact same derivation `RepositoryListViewModel` already uses for * the repository list's own "마지막 push" row, rather than a second, potentially-drifting copy of - * the same idea. + * the same idea. Originally returned only the timestamp; TASK-047 widened it to the whole + * [LatestPush] (branch + commit id, not just epoch millis) once a real push notification needed to + * say (and deep-link to) *which* branch actually advanced, not just report that something did. */ -class GetLatestPushEpochMillisUseCase( +data class LatestPush(val branch: RepositoryRef, val commitId: String, val epochMillis: Long) + +class GetLatestPushUseCase( private val repositoryBrowser: GiteaRepositoryBrowser, ) { - suspend operator fun invoke(repository: RepositoryCoordinates): Long? = + suspend operator fun invoke(repository: RepositoryCoordinates): LatestPush? = loadAllBranches(repository) - .mapNotNull { it.commitAtEpochMillis } - .maxOrNull() + .filter { it.commitAtEpochMillis != null } + .maxByOrNull { it.commitAtEpochMillis!! } + ?.let { LatestPush(it.name, it.commitId, it.commitAtEpochMillis!!) } private suspend fun loadAllBranches(repository: RepositoryCoordinates): List { val branches = mutableListOf() diff --git a/app/src/main/res/values/strings_favorite_push.xml b/app/src/main/res/values/strings_favorite_push.xml index bbb47a1..a322fd2 100644 --- a/app/src/main/res/values/strings_favorite_push.xml +++ b/app/src/main/res/values/strings_favorite_push.xml @@ -1,6 +1,7 @@ 즐겨찾기 저장소 Push 알림 즐겨찾기한 저장소에 새로운 push가 있으면 알려드립니다. - %1$s에 새로운 push가 있습니다 - 저장소를 열어 확인해 보세요. + %1$s · %2$s에 새 push + %1$s: %2$s + 저장소를 열어 확인해 보세요. diff --git a/app/src/test/java/com/ubisam/ubgitea/domain/usecase/RepositoryBrowseUseCasesTest.kt b/app/src/test/java/com/ubisam/ubgitea/domain/usecase/RepositoryBrowseUseCasesTest.kt index 206beb2..34de9ad 100644 --- a/app/src/test/java/com/ubisam/ubgitea/domain/usecase/RepositoryBrowseUseCasesTest.kt +++ b/app/src/test/java/com/ubisam/ubgitea/domain/usecase/RepositoryBrowseUseCasesTest.kt @@ -81,7 +81,7 @@ class RepositoryBrowseUseCasesTest { } @Test - fun `latest push epoch millis is the max commit time across all branch pages`() { + fun `latest push is the branch with the max commit time across all branch pages`() { val branches = BranchPagedBrowser( pagesOf( listOf(branch("main", 1_000L), branch("dev", 3_000L)), @@ -89,25 +89,25 @@ class RepositoryBrowseUseCasesTest { ), ) - val result = runSuspend { GetLatestPushEpochMillisUseCase(branches)(coordinates) } + val result = runSuspend { GetLatestPushUseCase(branches)(coordinates) } - assertEquals(3_000L, result) + assertEquals(LatestPush(RepositoryRef.of("dev"), "sha-dev", 3_000L), result) } @Test - fun `latest push epoch millis ignores branches with no commit time`() { + fun `latest push ignores branches with no commit time`() { val branches = BranchPagedBrowser(pagesOf(listOf(branch("main", null)))) - val result = runSuspend { GetLatestPushEpochMillisUseCase(branches)(coordinates) } + val result = runSuspend { GetLatestPushUseCase(branches)(coordinates) } assertEquals(null, result) } @Test - fun `latest push epoch millis is null for a repository with no branches`() { + fun `latest push is null for a repository with no branches`() { val branches = BranchPagedBrowser(pagesOf(emptyList())) - val result = runSuspend { GetLatestPushEpochMillisUseCase(branches)(coordinates) } + val result = runSuspend { GetLatestPushUseCase(branches)(coordinates) } assertEquals(null, result) } diff --git a/docs/development-log.md b/docs/development-log.md index 1adc5c9..b4f2350 100644 --- a/docs/development-log.md +++ b/docs/development-log.md @@ -1106,3 +1106,53 @@ This is the append-only implementation and handoff record shared by Codex and Cl confirmation of the visual redesign specifically, and the real logo asset) are still open. - Next owner: Claude, continuing per the standing direction. +## 2026-09-08 (continued) — TASK-047: favorite-push notification accuracy (branch + commit) + +- Agent and role: Claude, feature implementer, per the human product owner's live report after + actually receiving a favorite-push notification: it opened an unrelated branch, and its text said + nothing about what changed. Both were already-anticipated gaps recorded in TASK-036's own Known + limitations (that task only ever tracked one scalar "latest push time" per repository, not which + branch produced it). +- Commit/diff: same working tree as TASK-045/046 (main checkout, branch + `claude/task-015-notifications-main`), not yet committed; new + `tasks/047-favorite-push-notification-accuracy.md`. +- Features implemented: the notification now deep-links to the branch that was actually pushed to + (not the repository's default branch), and its text names that branch plus the latest commit's + author and message summary. +- How it was implemented: `GetLatestPushEpochMillisUseCase` (returned a bare `Long?`) became + `GetLatestPushUseCase`, returning a new `LatestPush(branch, commitId, epochMillis)` via + `maxByOrNull` over a repository's branches instead of just `.maxOrNull()` over their timestamps. + `FavoritePushDetector` itself is untouched -- the worker derives the plain + `Map` it still expects via `.mapValues { it.value.epochMillis }`. For each + newly-pushed repository, the worker fetches that branch's latest commit + (`browser.listCommits(coordinates, push.branch, path = null, page = 1, pageSize = 1)`, cheap now + thanks to TASK-046's `stat=false`) wrapped in `runCatching` -- a failed fetch still lets the + notification fire with fallback text rather than losing it entirely. `FavoritePushNotifier + .notifyNewPush` signature changed accordingly (`branch` no longer nullable, plus an optional + `CommitSummary`); a `BigTextStyle` was added so a longer commit summary isn't clipped in the + expanded notification. +- Files changed: `domain/usecase/RepositoryBrowseUseCases.kt`, + `core/notifications/FavoritePushCheckWorker.kt`, `core/notifications/FavoritePushNotifier.kt`, + `res/values/strings_favorite_push.xml`, `RepositoryBrowseUseCasesTest.kt`, + `tasks/047-favorite-push-notification-accuracy.md`. +- Checks and results: `./gradlew compileDebugKotlin compileDebugUnitTestKotlin testDebugUnitTest assembleDebug lintDebug` + BUILD SUCCESSFUL, all unit tests passing (three `RepositoryBrowseUseCasesTest` cases updated for + the new return type), 0 lint errors. +- Live verification: not yet installed/tested on-device as of this entry (build/tests only so far). +- Review findings and resolutions: None from a separate review pass yet. +- Known limitations: the human product owner's third report -- notification timing feels irregular, + wants it to fire as soon as a commit lands -- was investigated but deliberately not built. + `WorkManager`'s periodic floor is a hard 15 minutes (Android-enforced, not this app's choice), and + actual elapsed time can run longer still whenever Doze/App Standby defers a low-priority periodic + job, which is almost certainly the irregularity being observed. Closing this for real means + choosing between a battery-costly foreground service (frequent polling, persistent notification + required), a real webhook-plus-push architecture (Gitea webhook → some always-on receiver, plausibly + on the same NAS → Firebase Cloud Messaging; near-instant, low battery cost, but real one-time + infrastructure work including action in the product owner's own Gitea admin panel and a Firebase + project this agent can't set up alone), or accepting the current cadence. Presented to the product + owner as a decision, not decided as of this task's close. +- Next recommended task: get item 3's direction decided, then implement whichever option is chosen; + otherwise TASK-045's still-open items (device confirmation, real logo asset) remain next. +- Next owner: Claude, continuing per the standing direction; human product owner for the item-3 + architecture decision. + diff --git a/tasks/047-favorite-push-notification-accuracy.md b/tasks/047-favorite-push-notification-accuracy.md new file mode 100644 index 0000000..5efd1d4 --- /dev/null +++ b/tasks/047-favorite-push-notification-accuracy.md @@ -0,0 +1,113 @@ +# TASK-047: Favorite-push notification accuracy (branch + commit) + +Status: implemented and closed; items 1-2 fixed, item 3 (near-instant delivery) presented to the +human product owner as a decision and explicitly declined — current 15-minute-plus cadence stays +Owner: Claude +Reviewer: Codex + +## Outcome + +The human product owner reported, after actually receiving a favorite-push notification live: (1) +tapping it opened a branch unrelated to what was actually pushed, (2) the notification text just said +"저장소를 열어 확인해 보세요" with no indication of what changed. Both were real, already-anticipated +gaps explicitly recorded in TASK-036's own Known limitations (that task tracked only one scalar +"latest push time" per repository, with no branch or commit attached). Fixed by widening detection to +carry the branch that actually advanced, and fetching that branch's latest commit for the +notification text. + +A third report — notification timing feels irregular and should fire "as soon as a commit lands" — +is a real platform constraint, not a bug, and is intentionally left unimplemented pending the human +product owner's choice between real trade-offs (see Known limitations). + +## Scope + +- Included: `GetLatestPushEpochMillisUseCase` → `GetLatestPushUseCase`, now returning a new + `LatestPush(branch, commitId, epochMillis)` instead of a bare `Long?`; `FavoritePushCheckWorker` + fetches the newly-pushed branch's latest commit (`stat=false`-fast, per TASK-046) and passes it + through; `FavoritePushNotifier.notifyNewPush` takes the actual pushed `branch` (no longer + nullable, no longer the repository's *default* branch) and an optional `CommitSummary` for the + notification body; new/changed notification strings. +- Excluded: item 3 (delivery latency) — see Known limitations. No change to the 15-minute + `WorkManager` polling mechanism itself. + +## Design notes + +- **`FavoritePushDetector` is unchanged.** It still compares a plain `Map` — + the worker now derives that view from `current.mapValues { it.value.epochMillis }` rather than + the detector needing to know about branches or commits at all. Keeps the one pure, + no-Android-dependency class exactly as narrow as it was. +- **The persisted baseline (`LastSeenPushStore`) is unchanged** — still one epoch-millis long per + repository. Branch and commit are only ever needed at notify-time, for the run that just detected + a change, so there's nothing to persist beyond what already existed. +- **The commit fetch is best-effort.** If it fails (network hiccup, or a race where the branch was + deleted between the branch-list call and this one), the notification still fires using the + fallback text rather than the whole notification being lost over a secondary lookup. +- **`stat=false` (TASK-046) is what makes this affordable.** Fetching one extra commit per + newly-pushed favorited repository, every ~15 minutes, would have been a meaningfully worse idea + before that fix made single-commit fetches fast; after it, this is a cheap, single-item page + request. + +## Acceptance criteria + +- [x] Tapping the notification opens the repository at the branch that was actually pushed to, not + the repository's default branch. +- [x] The notification body names the branch and the latest commit's author and message summary + (first line only) when the commit fetch succeeds; falls back to the previous generic text if + it doesn't. +- [x] Build, unit tests (updated for the new `LatestPush`-returning use case), and lint pass. +- [ ] Not yet addressed: notification delivery latency/regularity (item 3) — see Known limitations. + +## Required checks + +```text +./gradlew compileDebugKotlin compileDebugUnitTestKotlin testDebugUnitTest assembleDebug lintDebug +``` + +## Evidence + +- `domain/usecase/RepositoryBrowseUseCases.kt`: `LatestPush` data class; `GetLatestPushUseCase` + (renamed from `GetLatestPushEpochMillisUseCase`) returns it via `maxByOrNull`. +- `core/notifications/FavoritePushCheckWorker.kt`: uses `GetLatestPushUseCase`; for each newly-pushed + repository, fetches `browser.listCommits(coordinates, push.branch, path = null, page = 1, pageSize = 1)` + wrapped in `runCatching`, then calls `notifier.notifyNewPush(coordinates, push.branch, latestCommit)`. +- `core/notifications/FavoritePushNotifier.kt`: `notifyNewPush` signature changed + (`defaultBranch: RepositoryRef?` → `branch: RepositoryRef`, plus `latestCommit: CommitSummary?`); + intent extras are now always attached (a repository that produced a `LatestPush` always has a real + branch, unlike the old default-branch-may-be-null case); `BigTextStyle` added so a longer commit + message summary isn't clipped. +- `strings_favorite_push.xml`: title now includes the branch; content shows `author: message`; a + separate `_fallback` string covers the best-effort-failed case. +- `RepositoryBrowseUseCasesTest.kt`: three tests updated to assert on `LatestPush` values instead of + a bare `Long?`. +- `./gradlew compileDebugKotlin compileDebugUnitTestKotlin testDebugUnitTest assembleDebug lintDebug`: + BUILD SUCCESSFUL, all unit tests passing, 0 lint errors. + +## Known limitations + +- **Item 3 (near-instant delivery) is not implemented.** `WorkManager`'s `PeriodicWorkRequest` has a + hard, non-configurable 15-minute floor (`PeriodicWorkRequest.MIN_PERIODIC_INTERVAL_MILLIS`) — + Android itself enforces this, not a choice this app makes. Actual elapsed time between runs can + exceed 15 minutes further still, unevenly, whenever Doze/App Standby defers a low-priority + periodic job — which is almost certainly the "불규칙함" the human product owner is seeing, not a + bug in this app's own scheduling code. Closing this gap for real means picking one of: + 1. A foreground service polling much more frequently (e.g. every ~1 minute) — bypasses Doze + throttling, but requires a permanently visible notification (Android mandates one for any + foreground service) and continuously drains battery for as long as the app is "running" this + way. + 2. A real push architecture: a Gitea webhook (configured server-side, which the human product + owner can do since they administer this self-hosted instance) firing on `push`, received by + some always-on endpoint, relayed to the device via Firebase Cloud Messaging (or an equivalent + push service) — genuinely near-instant, low battery cost, but a real one-time infrastructure + build: a webhook receiver running somewhere reachable (plausibly on the same Synology NAS), a + Firebase project, and `google-services.json`/FCM SDK integration into this app. + 3. Leave the 15-minute-plus, Doze-affected cadence as-is. + This decision needed the human product owner, not just Claude: option 2 requires action outside + this codebase (their Gitea admin panel, a Firebase account) that only they can do, and option 1's + persistent-notification/battery trade-off is a real product call, not an engineering default to + assume silently. Presented directly as a three-way choice; the product owner chose to keep the + current cadence as-is ("지금이 대로 괜찮음") rather than pay the battery/persistent-notification + cost of a foreground service or take on the webhook+FCM infrastructure work. + +## Review findings + +None yet — pending Codex/human review.