TASK-035: remove redundant per-file Commit button from the code browser
The header's 'Commit' button (onViewHistory) just switched BranchDetailScreen's pager to page 1 -- the exact same action the top-level 'Commit' tab's own onClick already performs. No path/file info ever flowed through it, so it was never actually file-specific history, just a duplicate shortcut that TASK-031's rename made obvious. Removed the button, its parameter threading, and the now-unused codebrowser_history string. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
parent
bec12e4f46
commit
9e4cbe3e89
@ -26,7 +26,7 @@ import kotlinx.coroutines.launch
|
|||||||
|
|
||||||
/**
|
/**
|
||||||
* One branch/tag/commit ref, shown as two swipeable tabs — Files (the code browser, default) and
|
* One branch/tag/commit ref, shown as two swipeable tabs — Files (the code browser, default) and
|
||||||
* History — over the same ref. Each tab keeps its own existing ViewModel and internal drill-down
|
* Commit — over the same ref. Each tab keeps its own existing ViewModel and internal drill-down
|
||||||
* (a subfolder, a commit's detail) entirely as before; this screen only adds the tab chrome and
|
* (a subfolder, a commit's detail) entirely as before; this screen only adds the tab chrome and
|
||||||
* the pager, and only intercepts "back" once a tab's own internal back-handling has nothing left
|
* the pager, and only intercepts "back" once a tab's own internal back-handling has nothing left
|
||||||
* to unwind (see `onCodeBrowserBack`/`onHistoryBack`).
|
* to unwind (see `onCodeBrowserBack`/`onHistoryBack`).
|
||||||
@ -97,7 +97,6 @@ fun BranchDetailScreen(
|
|||||||
onRetry = onRetryCodeBrowser,
|
onRetry = onRetryCodeBrowser,
|
||||||
onBack = onCodeBrowserBack,
|
onBack = onCodeBrowserBack,
|
||||||
onAuthenticationExpired = onAuthenticationExpired,
|
onAuthenticationExpired = onAuthenticationExpired,
|
||||||
onViewHistory = { scope.launch { pagerState.animateScrollToPage(1) } },
|
|
||||||
onNavigateImage = onNavigateImage,
|
onNavigateImage = onNavigateImage,
|
||||||
)
|
)
|
||||||
else -> HistoryScreen(
|
else -> HistoryScreen(
|
||||||
|
|||||||
@ -96,7 +96,6 @@ fun CodeBrowserScreen(
|
|||||||
onRetry: () -> Unit,
|
onRetry: () -> Unit,
|
||||||
onBack: () -> Unit,
|
onBack: () -> Unit,
|
||||||
onAuthenticationExpired: () -> Unit,
|
onAuthenticationExpired: () -> Unit,
|
||||||
onViewHistory: (CodeBrowserTarget) -> Unit,
|
|
||||||
onNavigateImage: (Int) -> Unit,
|
onNavigateImage: (Int) -> Unit,
|
||||||
modifier: Modifier = Modifier,
|
modifier: Modifier = Modifier,
|
||||||
) {
|
) {
|
||||||
@ -106,11 +105,11 @@ fun CodeBrowserScreen(
|
|||||||
when (state) {
|
when (state) {
|
||||||
CodeBrowserUiState.Idle -> Unit
|
CodeBrowserUiState.Idle -> Unit
|
||||||
is CodeBrowserUiState.Loading -> {
|
is CodeBrowserUiState.Loading -> {
|
||||||
BrowserHeader(state.target, state.path, state.isFile, onBreadcrumbSelected, onViewHistory)
|
BrowserHeader(state.target, state.path, state.isFile, onBreadcrumbSelected)
|
||||||
CenterMessage { CircularProgressIndicator() }
|
CenterMessage { CircularProgressIndicator() }
|
||||||
}
|
}
|
||||||
is CodeBrowserUiState.Directory -> {
|
is CodeBrowserUiState.Directory -> {
|
||||||
BrowserHeader(state.target, state.path, false, onBreadcrumbSelected, onViewHistory)
|
BrowserHeader(state.target, state.path, false, onBreadcrumbSelected)
|
||||||
if (state.entries.isEmpty()) {
|
if (state.entries.isEmpty()) {
|
||||||
CenterMessage { Text(stringResource(R.string.codebrowser_directory_empty)) }
|
CenterMessage { Text(stringResource(R.string.codebrowser_directory_empty)) }
|
||||||
} else {
|
} else {
|
||||||
@ -122,11 +121,11 @@ fun CodeBrowserScreen(
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
is CodeBrowserUiState.File -> {
|
is CodeBrowserUiState.File -> {
|
||||||
BrowserHeader(state.target, state.path, true, onBreadcrumbSelected, onViewHistory)
|
BrowserHeader(state.target, state.path, true, onBreadcrumbSelected)
|
||||||
FileContent(state.content, state.path, state.siblingImagePaths, onNavigateImage)
|
FileContent(state.content, state.path, state.siblingImagePaths, onNavigateImage)
|
||||||
}
|
}
|
||||||
is CodeBrowserUiState.UnsupportedEntry -> {
|
is CodeBrowserUiState.UnsupportedEntry -> {
|
||||||
BrowserHeader(state.target, state.path, true, onBreadcrumbSelected, onViewHistory)
|
BrowserHeader(state.target, state.path, true, onBreadcrumbSelected)
|
||||||
CenterMessage {
|
CenterMessage {
|
||||||
Text(
|
Text(
|
||||||
stringResource(
|
stringResource(
|
||||||
@ -137,7 +136,7 @@ fun CodeBrowserScreen(
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
is CodeBrowserUiState.Error -> {
|
is CodeBrowserUiState.Error -> {
|
||||||
BrowserHeader(state.target, state.path, state.isFile, onBreadcrumbSelected, onViewHistory)
|
BrowserHeader(state.target, state.path, state.isFile, onBreadcrumbSelected)
|
||||||
CenterMessage {
|
CenterMessage {
|
||||||
Text(stringResource(state.type.messageResource))
|
Text(stringResource(state.type.messageResource))
|
||||||
if (state.type == CodeBrowserError.AUTHENTICATION_EXPIRED) {
|
if (state.type == CodeBrowserError.AUTHENTICATION_EXPIRED) {
|
||||||
@ -160,21 +159,15 @@ private fun BrowserHeader(
|
|||||||
path: RepositoryPath,
|
path: RepositoryPath,
|
||||||
leafIsFile: Boolean,
|
leafIsFile: Boolean,
|
||||||
onBreadcrumbSelected: (RepositoryPath) -> Unit,
|
onBreadcrumbSelected: (RepositoryPath) -> Unit,
|
||||||
onViewHistory: (CodeBrowserTarget) -> Unit,
|
|
||||||
) {
|
) {
|
||||||
Column(Modifier.fillMaxWidth().padding(16.dp)) {
|
Column(Modifier.fillMaxWidth().padding(16.dp)) {
|
||||||
Row(verticalAlignment = Alignment.CenterVertically) {
|
Text("${target.repository.owner}/${target.repository.name}", style = MaterialTheme.typography.titleLarge)
|
||||||
Column(Modifier.weight(1f)) {
|
val refKind = when (target.kind) {
|
||||||
Text("${target.repository.owner}/${target.repository.name}", style = MaterialTheme.typography.titleLarge)
|
CodeReferenceKind.BRANCH -> stringResource(R.string.codebrowser_ref_branch)
|
||||||
val refKind = when (target.kind) {
|
CodeReferenceKind.TAG -> stringResource(R.string.codebrowser_ref_tag)
|
||||||
CodeReferenceKind.BRANCH -> stringResource(R.string.codebrowser_ref_branch)
|
CodeReferenceKind.COMMIT -> stringResource(R.string.codebrowser_ref_commit)
|
||||||
CodeReferenceKind.TAG -> stringResource(R.string.codebrowser_ref_tag)
|
|
||||||
CodeReferenceKind.COMMIT -> stringResource(R.string.codebrowser_ref_commit)
|
|
||||||
}
|
|
||||||
Text(stringResource(R.string.codebrowser_ref, refKind, target.ref.value))
|
|
||||||
}
|
|
||||||
OutlinedButton(onClick = { onViewHistory(target) }) { Text(stringResource(R.string.codebrowser_history)) }
|
|
||||||
}
|
}
|
||||||
|
Text(stringResource(R.string.codebrowser_ref, refKind, target.ref.value))
|
||||||
Row(Modifier.horizontalScroll(rememberScrollState()).padding(top = 12.dp)) {
|
Row(Modifier.horizontalScroll(rememberScrollState()).padding(top = 12.dp)) {
|
||||||
val breadcrumbs = CodeBrowserNavigation.breadcrumbs(path)
|
val breadcrumbs = CodeBrowserNavigation.breadcrumbs(path)
|
||||||
breadcrumbs.forEachIndexed { index, crumb ->
|
breadcrumbs.forEachIndexed { index, crumb ->
|
||||||
|
|||||||
@ -25,7 +25,6 @@
|
|||||||
<string name="codebrowser_server_error">서버 응답을 처리하지 못했습니다.</string>
|
<string name="codebrowser_server_error">서버 응답을 처리하지 못했습니다.</string>
|
||||||
<string name="codebrowser_retry">다시 시도</string>
|
<string name="codebrowser_retry">다시 시도</string>
|
||||||
<string name="codebrowser_sign_in_again">다시 로그인</string>
|
<string name="codebrowser_sign_in_again">다시 로그인</string>
|
||||||
<string name="codebrowser_history">커밋</string>
|
|
||||||
<string name="codebrowser_image_index">%1$d / %2$d</string>
|
<string name="codebrowser_image_index">%1$d / %2$d</string>
|
||||||
<string name="codebrowser_image_decode_failed">이미지를 표시할 수 없습니다.</string>
|
<string name="codebrowser_image_decode_failed">이미지를 표시할 수 없습니다.</string>
|
||||||
<string name="codebrowser_csv_empty">이 CSV 파일은 비어 있습니다.</string>
|
<string name="codebrowser_csv_empty">이 CSV 파일은 비어 있습니다.</string>
|
||||||
|
|||||||
@ -605,3 +605,16 @@ This is the append-only implementation and handoff record shared by Codex and Cl
|
|||||||
- Known limitations: unchanged from earlier in this task — still optimistic rather than server-authoritative in both directions (accepted tradeoff), and the underlying "can't tell if already starred on load" limitation from TASK-018/TASK-030 is untouched.
|
- Known limitations: unchanged from earlier in this task — still optimistic rather than server-authoritative in both directions (accepted tradeoff), and the underlying "can't tell if already starred on load" limitation from TASK-018/TASK-030 is untouched.
|
||||||
- Next recommended task: get live confirmation that starring/un-starring now feels instant.
|
- Next recommended task: get live confirmation that starring/un-starring now feels instant.
|
||||||
- Next owner: Claude, continuing per the standing direction.
|
- Next owner: Claude, continuing per the standing direction.
|
||||||
|
|
||||||
|
## 2026-08-29 (continued) — TASK-035: remove the redundant per-file "커밋" button in the code browser
|
||||||
|
|
||||||
|
- Agent and role: Claude, direct continuation of the same live session — "현재 보고있는 페이지 우상단에 커밋 버튼은 이제 필요 없지 않나" (the Commit button in the top-right of the current page isn't needed anymore, is it).
|
||||||
|
- Commit/diff: on `claude/task-015-notifications`; `feature/codebrowser/CodeBrowserScreen.kt`, `feature/branchdetail/BranchDetailScreen.kt`, `strings_codebrowser.xml`, `tasks/035-remove-redundant-code-browser-commit-button.md`.
|
||||||
|
- Confirmed the observation was correct before acting on it: traced the header's "커밋" button (`onViewHistory`) all the way to its single call site in `BranchDetailScreen.kt` and found it just switched the pager to page 1 — the exact same action the top-level "커밋" tab's own `onClick` already performs a few lines above it. TASK-031's rename (both controls used to be independently labeled "히스토리"/differently-purposed before that) is what made the duplication visually obvious.
|
||||||
|
- How it was implemented: removed the button and the `onViewHistory` parameter threaded through `CodeBrowserScreen`/`BrowserHeader`, the call site in `BranchDetailScreen.kt`, and the now-unused `codebrowser_history` string resource (confirmed unused via a repo-wide search first). `BrowserHeader`'s title/ref layout simplified from a `Row`-wrapped `Column` to a plain `Column` now that there's no trailing button to share the row with. Also fixed a stale "History" word in a doc comment one line above (predating TASK-031's rename).
|
||||||
|
- Files changed: `CodeBrowserScreen.kt`, `BranchDetailScreen.kt`, `strings_codebrowser.xml`, `tasks/035-remove-redundant-code-browser-commit-button.md`.
|
||||||
|
- Checks and results: `./gradlew compileDebugKotlin compileDebugUnitTestKotlin testDebugUnitTest assembleDebug lintDebug` BUILD SUCCESSFUL, all unit tests passing, 0 lint errors. Installed on the human product owner's device over wireless `adb`.
|
||||||
|
- Review findings and resolutions: None from a separate review pass.
|
||||||
|
- Known limitations: None identified.
|
||||||
|
- Next recommended task: get live confirmation the header looks right without the button; otherwise continue per the standing direction on the rest of this session's queued items.
|
||||||
|
- Next owner: Claude, continuing per the standing direction.
|
||||||
|
|||||||
84
tasks/035-remove-redundant-code-browser-commit-button.md
Normal file
84
tasks/035-remove-redundant-code-browser-commit-button.md
Normal file
@ -0,0 +1,84 @@
|
|||||||
|
# TASK-035: Remove the redundant per-file "커밋" button in the code browser
|
||||||
|
|
||||||
|
Status: implemented; all required checks pass; live-installed, awaiting the human product owner's confirmation
|
||||||
|
Owner: Claude
|
||||||
|
Reviewer: Codex
|
||||||
|
|
||||||
|
## Outcome
|
||||||
|
|
||||||
|
Live-reported: "현재 보고있는 페이지 우상단에 커밋 버튼은 이제 필요 없지 않나" (the Commit button
|
||||||
|
in the top-right of the current page isn't needed anymore, is it). Confirmed correct: the code
|
||||||
|
browser's header showed an `OutlinedButton` (originally labeled "히스토리", renamed to "커밋" by
|
||||||
|
TASK-031) that just switched `BranchDetailScreen`'s pager to page 1 — the exact same destination
|
||||||
|
the top-level "커밋" tab (also renamed by TASK-031) already switches to with one tap, with no
|
||||||
|
file-specific filtering or other distinct behavior. Since TASK-031 gave both controls the identical
|
||||||
|
label doing the identical thing, the in-content button became pure duplication. Removed it.
|
||||||
|
|
||||||
|
## Scope
|
||||||
|
|
||||||
|
- Included: `feature/codebrowser/CodeBrowserScreen.kt` (`BrowserHeader`'s button and the
|
||||||
|
`onViewHistory` parameter threaded through `CodeBrowserScreen`), `feature/branchdetail/BranchDetailScreen.kt`
|
||||||
|
(the call site, plus a doc-comment word fix from a prior rename), `strings_codebrowser.xml`
|
||||||
|
(removed the now-unused `codebrowser_history` string).
|
||||||
|
- Excluded: The top-level "커밋" tab and `HistoryScreen` itself are untouched — this removes only
|
||||||
|
the duplicate shortcut, not the feature it duplicated. No change to any other button or gesture
|
||||||
|
in the code browser.
|
||||||
|
|
||||||
|
## Design notes
|
||||||
|
|
||||||
|
- **Confirmed it was truly redundant before removing it, not just probably**: traced
|
||||||
|
`onViewHistory` all the way from `BrowserHeader`'s button through `CodeBrowserScreen`'s parameter
|
||||||
|
to its one call site in `BranchDetailScreen.kt` —
|
||||||
|
`onViewHistory = { scope.launch { pagerState.animateScrollToPage(1) } }` — which is exactly the
|
||||||
|
same action the "커밋" `Tab`'s own `onClick` performs a few lines above it. No path/file
|
||||||
|
information ever flowed through `onViewHistory`, so this was never actually "view this specific
|
||||||
|
file's history," despite once being labeled that way before TASK-031's rename made the
|
||||||
|
duplication obvious.
|
||||||
|
- **Header simplified along with the removal**: `BrowserHeader`'s title/ref `Column` no longer
|
||||||
|
needs to share a `Row` with a trailing button, so it collapsed back to a plain `Column` — one
|
||||||
|
less nested layout, not just a deleted button.
|
||||||
|
|
||||||
|
## Acceptance criteria
|
||||||
|
|
||||||
|
- [x] The code browser's header (shown across Loading/Directory/File/UnsupportedEntry/Error states)
|
||||||
|
no longer shows a "커밋" button.
|
||||||
|
- [x] The top-level "커밋" tab in `BranchDetailScreen` is unaffected and remains the only way to
|
||||||
|
reach commit history from this screen.
|
||||||
|
- [x] No leftover references to `onViewHistory` or the removed `codebrowser_history` string
|
||||||
|
resource anywhere in the app.
|
||||||
|
- [x] Build, unit tests, and lint pass.
|
||||||
|
- [ ] Live confirmation from the human product owner that the header now looks right without it —
|
||||||
|
installed on the device but not yet re-viewed in this exact session.
|
||||||
|
|
||||||
|
## Required checks
|
||||||
|
|
||||||
|
```text
|
||||||
|
./gradlew compileDebugKotlin compileDebugUnitTestKotlin
|
||||||
|
./gradlew testDebugUnitTest
|
||||||
|
./gradlew assembleDebug lintDebug
|
||||||
|
```
|
||||||
|
|
||||||
|
## Evidence
|
||||||
|
|
||||||
|
- `CodeBrowserScreen.kt`: `onViewHistory` parameter removed from `CodeBrowserScreen` and
|
||||||
|
`BrowserHeader`; the `OutlinedButton` removed; `BrowserHeader`'s title/ref layout simplified from
|
||||||
|
a `Row`-wrapped `Column` to a plain `Column`.
|
||||||
|
- `BranchDetailScreen.kt`: the `onViewHistory = { ... }` argument removed from the one
|
||||||
|
`CodeBrowserScreen(...)` call site; a stale doc-comment reference to "History" (predating
|
||||||
|
TASK-031's tab rename) corrected to "Commit".
|
||||||
|
- `strings_codebrowser.xml`: `codebrowser_history` string removed (confirmed unused via a
|
||||||
|
repository-wide search before deleting it).
|
||||||
|
- `./gradlew compileDebugKotlin compileDebugUnitTestKotlin`: BUILD SUCCESSFUL.
|
||||||
|
- `./gradlew testDebugUnitTest`: BUILD SUCCESSFUL, all unit tests passing (no test referenced the
|
||||||
|
removed button/parameter/string).
|
||||||
|
- `./gradlew assembleDebug lintDebug`: BUILD SUCCESSFUL, 0 lint errors.
|
||||||
|
- Installed on the human product owner's device over wireless `adb` (`adb install -r`, streamed
|
||||||
|
install succeeded); not yet re-viewed live in this exact session.
|
||||||
|
|
||||||
|
## Known limitations
|
||||||
|
|
||||||
|
None identified.
|
||||||
|
|
||||||
|
## Review findings
|
||||||
|
|
||||||
|
None yet — pending Codex/human review.
|
||||||
Loading…
x
Reference in New Issue
Block a user