From 5caa6057f17d063060c61c5dcddde4cb705ac5ad Mon Sep 17 00:00:00 2001 From: choiminyeong Date: Mon, 28 Sep 2026 13:17:32 +0900 Subject: [PATCH] TASK-046: commit history load performance and loading feedback See tasks/046-commit-history-load-performance.md. Co-Authored-By: Claude Fable 5.1 --- .../ubgitea/core/network/HttpTransport.kt | 11 +- .../repositories/GiteaRepositoryBrowserApi.kt | 7 ++ .../ubgitea/feature/history/HistoryScreen.kt | 38 +++++- app/src/main/res/values/strings_history.xml | 1 + .../GiteaRepositoryBrowserApiTest.kt | 1 + docs/development-log.md | 46 ++++++++ tasks/046-commit-history-load-performance.md | 108 ++++++++++++++++++ 7 files changed, 207 insertions(+), 5 deletions(-) create mode 100644 tasks/046-commit-history-load-performance.md diff --git a/app/src/main/java/com/ubisam/ubgitea/core/network/HttpTransport.kt b/app/src/main/java/com/ubisam/ubgitea/core/network/HttpTransport.kt index a703ee7..be5a338 100644 --- a/app/src/main/java/com/ubisam/ubgitea/core/network/HttpTransport.kt +++ b/app/src/main/java/com/ubisam/ubgitea/core/network/HttpTransport.kt @@ -40,10 +40,19 @@ interface HttpTransport { // being misreported as offline, and sharing one connection pool across every feature's API calls // (instead of each of the ~8 call sites opening its own pool) lets repeated calls to the same // Gitea host reuse an already-open TCP/TLS connection rather than renegotiating one per feature. +// +// readTimeout raised again, 30s -> 60s (TASK-045 follow-up): live-reproduced against a real +// repository whose history contains a single vendored-dependency commit touching ~5.8 million +// lines. Gitea's commit-list endpoint computes per-commit file/stat data server-side regardless of +// requested page size, so that one commit's cost is paid on any page containing it -- consistently +// took long enough on the Synology NAS host to still trip the previous 30s ceiling. This is a +// client-side patience increase, not a fix to Gitea's own response cost; if a host or repository +// ever makes even 60s insufficient, the real fix is asking Gitea to omit per-file stats from the +// list endpoint (if it supports that), not raising this value indefinitely. private val sharedOkHttpClient: OkHttpClient by lazy { OkHttpClient.Builder() .connectTimeout(15, TimeUnit.SECONDS) - .readTimeout(30, TimeUnit.SECONDS) + .readTimeout(60, TimeUnit.SECONDS) .writeTimeout(15, TimeUnit.SECONDS) .build() } diff --git a/app/src/main/java/com/ubisam/ubgitea/data/repositories/GiteaRepositoryBrowserApi.kt b/app/src/main/java/com/ubisam/ubgitea/data/repositories/GiteaRepositoryBrowserApi.kt index 99aac63..c84a182 100644 --- a/app/src/main/java/com/ubisam/ubgitea/data/repositories/GiteaRepositoryBrowserApi.kt +++ b/app/src/main/java/com/ubisam/ubgitea/data/repositories/GiteaRepositoryBrowserApi.kt @@ -243,6 +243,13 @@ class GiteaRepositoryBrowserApi( .addQueryParameter("sha", ref.value) .addQueryParameter("page", page.toString()) .addQueryParameter("limit", pageSize.toString()) + // mapCommit below only ever reads sha/message/author name+date -- never the per-commit + // file list or added/removed line stats Gitea computes by default for this endpoint. + // That computation is expensive: live-measured at 31s vs 0.4s for the same request + // against a real repository with one huge (~5.8M line) commit in its history, entirely + // server-side compute time, not payload size (both responses were near-identical size). + // stat=false skips work the app already throws away. + .addQueryParameter("stat", "false") path?.takeUnless(RepositoryPath::isRoot)?.let { builder.addQueryParameter("path", it.value) } val response = get(builder.build()) val items = parseList(response.body).map(::mapCommit) diff --git a/app/src/main/java/com/ubisam/ubgitea/feature/history/HistoryScreen.kt b/app/src/main/java/com/ubisam/ubgitea/feature/history/HistoryScreen.kt index dd58728..37444a6 100644 --- a/app/src/main/java/com/ubisam/ubgitea/feature/history/HistoryScreen.kt +++ b/app/src/main/java/com/ubisam/ubgitea/feature/history/HistoryScreen.kt @@ -16,6 +16,7 @@ import androidx.compose.foundation.layout.fillMaxSize import androidx.compose.foundation.layout.fillMaxWidth import androidx.compose.foundation.layout.offset import androidx.compose.foundation.layout.padding +import androidx.compose.foundation.layout.width import androidx.compose.foundation.clickable import androidx.compose.foundation.rememberScrollState import androidx.compose.foundation.lazy.LazyColumn @@ -24,8 +25,8 @@ import androidx.compose.foundation.lazy.rememberLazyListState import androidx.compose.foundation.text.selection.SelectionContainer import androidx.compose.foundation.verticalScroll import androidx.compose.material3.Button -import androidx.compose.material3.CircularProgressIndicator import androidx.compose.material3.HorizontalDivider +import androidx.compose.material3.LinearProgressIndicator import androidx.compose.material3.MaterialTheme import androidx.compose.material3.OutlinedTextField import androidx.compose.material3.Scaffold @@ -51,6 +52,7 @@ import androidx.compose.ui.text.font.FontFamily import androidx.compose.ui.text.rememberTextMeasurer import androidx.compose.ui.unit.IntOffset import androidx.compose.ui.unit.dp +import kotlinx.coroutines.delay import com.ubisam.ubgitea.R import com.ubisam.ubgitea.domain.history.CommitDetail import com.ubisam.ubgitea.domain.history.DiffLine @@ -86,7 +88,7 @@ fun HistoryScreen( is HistoryUiState.Loading -> { BackHandler(onBack = onBack) Header(state.target) - CenterMessage { CircularProgressIndicator() } + CenterMessage { EstimatedProgressIndicator() } } is HistoryUiState.CommitList -> { BackHandler(onBack = onBack) @@ -105,7 +107,7 @@ fun HistoryScreen( is HistoryUiState.CommitDetailLoading -> { BackHandler(onBack = onBackToList) Header(state.target) - CenterMessage { CircularProgressIndicator() } + CenterMessage { EstimatedProgressIndicator() } } is HistoryUiState.CommitDetailContent -> { CommitDetailScreen(state, onRetryPatch, onFileSelected, onClearFileSelection, onBackToList) @@ -386,7 +388,7 @@ private val DiffAddedColor = Color(0xFF2E7D32) @Composable private fun PatchSection(patch: PatchUiState, onRetry: () -> Unit) { when (patch) { - PatchUiState.Loading -> CenterMessage { CircularProgressIndicator() } + PatchUiState.Loading -> CenterMessage { EstimatedProgressIndicator() } is PatchUiState.Loaded -> PatchText(patch.text, patch.isTruncated) PatchUiState.Failed -> CenterMessage { Text(stringResource(R.string.history_patch_failed)) @@ -433,6 +435,34 @@ private fun CenterMessage(content: @Composable ColumnScope.() -> Unit) { ) } +// Gitea's commit-list response is chunked (no Content-Length -- confirmed against the live server), +// so there is no real byte count to report a true download percentage from. This instead estimates +// progress from elapsed time against a typical-case duration, capped short of 100% so a request that +// runs long never visually lies about being finished -- it just holds near the cap until the actual +// response arrives and this composable is replaced by the real content/error state. Purely +// decorative/local state: nothing here is persisted or read by the ViewModel. +private const val ESTIMATED_LOAD_DURATION_MILLIS = 20_000L +private const val ESTIMATED_LOAD_PROGRESS_CAP = 0.94f + +@Composable +private fun EstimatedProgressIndicator() { + var progress by remember { mutableFloatStateOf(0f) } + LaunchedEffect(Unit) { + val start = System.currentTimeMillis() + while (progress < ESTIMATED_LOAD_PROGRESS_CAP) { + val elapsed = System.currentTimeMillis() - start + progress = (elapsed.toFloat() / ESTIMATED_LOAD_DURATION_MILLIS).coerceAtMost(ESTIMATED_LOAD_PROGRESS_CAP) + delay(100) + } + } + LinearProgressIndicator(progress = { progress }, modifier = Modifier.width(160.dp)) + Text( + stringResource(R.string.history_loading_progress, (progress * 100).toInt()), + style = MaterialTheme.typography.bodySmall, + modifier = Modifier.padding(top = 8.dp), + ) +} + private val FileChangeStatus.labelResource: Int get() = when (this) { FileChangeStatus.ADDED -> R.string.history_status_added diff --git a/app/src/main/res/values/strings_history.xml b/app/src/main/res/values/strings_history.xml index 5788565..e2af000 100644 --- a/app/src/main/res/values/strings_history.xml +++ b/app/src/main/res/values/strings_history.xml @@ -20,6 +20,7 @@ 이름 변경됨 기타 변경 서버에 연결할 수 없습니다. 네트워크를 확인해 주세요. + 불러오는 중… %1$d%% 로그인이 만료되었습니다. 다시 로그인해 주세요. 이 저장소의 커밋 내역을 볼 권한이 없습니다. 커밋이나 기준을 찾을 수 없습니다. diff --git a/app/src/test/java/com/ubisam/ubgitea/data/repositories/GiteaRepositoryBrowserApiTest.kt b/app/src/test/java/com/ubisam/ubgitea/data/repositories/GiteaRepositoryBrowserApiTest.kt index fa3f957..0133603 100644 --- a/app/src/test/java/com/ubisam/ubgitea/data/repositories/GiteaRepositoryBrowserApiTest.kt +++ b/app/src/test/java/com/ubisam/ubgitea/data/repositories/GiteaRepositoryBrowserApiTest.kt @@ -274,6 +274,7 @@ class GiteaRepositoryBrowserApiTest { val url = transport.urls.single().toHttpUrl() assertEquals("refs/heads/a+b", url.queryParameter("sha")) assertEquals("src/a b.kt", url.queryParameter("path")) + assertEquals("false", url.queryParameter("stat")) } @Test diff --git a/docs/development-log.md b/docs/development-log.md index 5fc02ca..1adc5c9 100644 --- a/docs/development-log.md +++ b/docs/development-log.md @@ -1060,3 +1060,49 @@ This is the append-only implementation and handoff record shared by Codex and Cl - Next owner: Claude, continuing per the standing direction; human product owner for the device verification and the logo-asset decision. +## 2026-09-08 (continued) — TASK-046: commit history load performance and loading feedback + +- Agent and role: Claude, incident responder -- found live while the product owner was testing + TASK-045's redesign on-device (wireless adb eventually reconnected after several failed attempts; + see TASK-045's own known limitations for that saga). +- Commit/diff: same working tree as TASK-045 (main checkout, branch `claude/task-015-notifications-main`), + not yet committed; new `tasks/046-commit-history-load-performance.md`. +- Root cause and fix: the product owner reported the 커밋 (commit history) tab failing with + "서버에 연결할 수 없습니다" on a real, large repository. Root-caused via live screenshots (adb + screencap, requiring per-display capture on the product owner's foldable -- the default display + argument returns corrupted output on multi-display devices) plus direct `curl` timing against the + live Gitea server: the failing endpoint (`GET .../commits`) computes full per-commit file/stat data + server-side by default, and this repository's history contains one commit touching ~5.8 million + lines (a vendored `packages/` folder committed to git), making even a 5-item page take 31+ seconds -- + tripping the client's 30s read timeout. `stat=false` skips that computation; measured 31.3s -> 0.4s + for the identical request, with near-identical response size either way (confirming the cost was + compute, not payload). `GiteaRepositoryBrowserApi.listCommits` never reads the `files`/`stats` + fields it was implicitly paying for, so this is a pure win. +- How it was implemented: `listCommits` now sends `stat=false`. `HttpTransport`'s shared OkHttp + client's `readTimeout` raised 30s -> 60s as defense in depth (this exact class of "slow-but-working + request misreported as offline" was already a documented, previously-fixed issue in that file's own + comment -- this is its second escalation). A new `EstimatedProgressIndicator` composable in + `HistoryScreen.kt` replaces the plain spinner across all three of History's loading states with a + percentage progress bar; confirmed via a live response-header check that Gitea serves this endpoint + chunked with no `Content-Length`, so the percentage is a time-based estimate (ramped toward a 94% + cap, never claiming 100% before real data arrives), not a true byte count -- disclosed to the + product owner as such. +- Files changed: `data/repositories/GiteaRepositoryBrowserApi.kt`, `core/network/HttpTransport.kt`, + `feature/history/HistoryScreen.kt`, `res/values/strings_history.xml`, + `GiteaRepositoryBrowserApiTest.kt`, `tasks/046-commit-history-load-performance.md`. +- Checks and results: `./gradlew compileDebugKotlin compileDebugUnitTestKotlin testDebugUnitTest assembleDebug lintDebug` + BUILD SUCCESSFUL, all unit tests passing (including a new assertion that `listCommits` sends + `stat=false`), 0 lint errors. +- Live verification: installed over wireless `adb` (after a lengthy reconnection struggle -- the + connection kept dropping when the product owner's phone screen turned off, requiring several fresh + pairing codes). Product owner navigated to the exact repository/commit tab that had failed before + and confirmed it now works well themselves ("내가 확인했어"). +- Review findings and resolutions: None from a separate review pass yet. +- Known limitations: the progress percentage is a time estimate, not real byte progress (inherent to + the endpoint's chunked response, see task file). Commit *detail* view for an equally huge commit + would still legitimately be slow, since showing file changes is that view's entire purpose -- + not addressed by this fix, and not fixable the same way. +- Next recommended task: none queued from this incident. TASK-045's own remaining items (device + confirmation of the visual redesign specifically, and the real logo asset) are still open. +- Next owner: Claude, continuing per the standing direction. + diff --git a/tasks/046-commit-history-load-performance.md b/tasks/046-commit-history-load-performance.md new file mode 100644 index 0000000..17aed50 --- /dev/null +++ b/tasks/046-commit-history-load-performance.md @@ -0,0 +1,108 @@ +# TASK-046: Commit history load performance and loading feedback + +Status: implemented; all required checks pass; live-confirmed on the human product owner's device +Owner: Claude +Reviewer: Codex + +## Outcome + +Live-testing TASK-045's redesign surfaced a real, pre-existing bug: opening the 커밋 (commit history) +tab on a real, large company repository (`RobotAutomationDevelopment-Dept/LGES-OC5-PtoP-Welder`) +consistently failed with "서버에 연결할 수 없습니다" (offline error), even though the device had a +normal network connection throughout. Root-caused, fixed, and confirmed live with the product owner. + +## Scope + +- Included: `GiteaRepositoryBrowserApi.listCommits` now requests `stat=false`; `HttpTransport`'s + shared OkHttp client's `readTimeout` raised 30s -> 60s as a safety margin; a new + `EstimatedProgressIndicator` (percentage progress bar) replaces the plain spinner on History's + three loading states. +- Excluded: no change to any other API call's stat/file-list behavior (only the commit-list + endpoint was touched); no change to commit detail, which legitimately needs per-file data and + correctly keeps returning it. + +## Root cause + +Gitea's `GET /repos/{owner}/{repo}/commits` endpoint computes and returns full per-commit file-change +lists and add/remove line stats by default, regardless of what the client asked for. This particular +repository's history contains one commit (a vendored `packages/` folder checked into git) touching +~5.8 million lines. Computing that commit's stats made the *entire request* -- even a 5-item page -- +take 31+ seconds server-side, tripping the client's then-30s read timeout. Live-measured directly +against the real endpoint: + +```text +default (stat computed): 31.3s, 678653 bytes +stat=false: 0.4s, 678567 bytes +``` + +Note the response size is nearly identical either way -- the cost is server-side compute time, not +payload size. `GiteaRepositoryBrowserApi.mapCommit` only ever reads `sha`/`commit.message`/ +`commit.author.{name,date}` from this response; it never reads `files` or `stats`. `stat=false` is +therefore a pure win: Gitea skips work the app already threw away, with no loss of data the app used. + +## Design notes + +- **`stat=false` is the real fix; the timeout bump and progress bar are defense in depth**, not the + primary fix -- kept anyway since OkHttp's previous 30s ceiling was already documented (see + `HttpTransport.kt`'s existing comment) as too tight for other genuinely-slow cases against this + same home-NAS-hosted Gitea instance (e.g. very large directory listings), and a percentage + indicator is a real UX improvement for any request that legitimately takes a while. +- **The progress bar is a time-based estimate, not a byte-accurate percentage.** Confirmed via a live + header check that Gitea serves this endpoint `Transfer-Encoding: chunked` with no `Content-Length`, + so there is no real total-bytes figure to compute a true percentage from. `EstimatedProgressIndicator` + ramps linearly toward a 94% cap over an assumed ~20s typical duration and holds there until the real + response arrives (replacing the whole composable with actual content/error) -- it never visually + claims 100% before the data is actually in. This trade-off (an honest estimate, not a precise + measurement) was not explicitly re-confirmed with the product owner after implementing, since the + underlying request became near-instant once `stat=false` was added, making the estimate's accuracy + largely moot in practice. +- **Applied to all three of History's loading states** (commit list, commit detail, patch), not just + the one that was reported, since all three hit the same class of Gitea host and the fix is a + same-shape, low-risk composable swap. + +## Acceptance criteria + +- [x] `listCommits` requests `stat=false`. +- [x] Live-measured against the real failing repository: request completes in under a second instead + of 30+. +- [x] Loading states show an estimated percentage progress bar instead of a bare spinner. +- [x] No change to what `CommitSummary` exposes or how it's built. +- [x] Build, unit tests (including a new query-parameter assertion), and lint pass. +- [x] Live-confirmed on the human product owner's device, against the exact repository that + originally failed. + +## Required checks + +```text +./gradlew compileDebugKotlin compileDebugUnitTestKotlin testDebugUnitTest assembleDebug lintDebug +``` + +## Evidence + +- `data/repositories/GiteaRepositoryBrowserApi.kt`: `listCommits` adds `.addQueryParameter("stat", "false")`. +- `core/network/HttpTransport.kt`: `readTimeout` 30s -> 60s, comment extended with this task's + measured numbers. +- `feature/history/HistoryScreen.kt`: new `EstimatedProgressIndicator` + `history_loading_progress` + string, replacing `CircularProgressIndicator()` at all three loading call sites (commit list, + commit detail, patch). +- `GiteaRepositoryBrowserApiTest.kt`: existing `listCommits` test extended to assert + `url.queryParameter("stat") == "false"`. +- Direct `curl` measurement against the live server (see Root cause above): 31.3s -> 0.4s, near-identical + response size either way. +- `./gradlew compileDebugKotlin compileDebugUnitTestKotlin testDebugUnitTest assembleDebug lintDebug`: + BUILD SUCCESSFUL, all unit tests passing, 0 lint errors. +- Live device: installed over wireless `adb`. Human product owner navigated to the previously-failing + repository's commit tab themselves and confirmed it now works well ("내가 확인했어"). + +## Known limitations + +- The progress percentage is a time-based estimate (see Design notes), not derived from real bytes + transferred -- inherent to Gitea serving this endpoint as chunked with no `Content-Length`. +- Only `listCommits` (the list endpoint) was changed. If a future repository has a similarly huge + commit and a user opens that commit's *detail* view, the detail fetch still legitimately needs and + requests full file/stat data, and could be slow for the same underlying reason -- not fixable the + same way, since that view's entire purpose is showing the file changes. + +## Review findings + +None yet -- pending Codex/human review.