feat: in-app Issues and Security viewers (#633)#704
Open
rainxchzed wants to merge 3 commits into
Open
Conversation
Contributor
WalkthroughThe PR introduces a complete "Repo Pages" feature enabling users to browse and view repository issues (with filtering and full detail threads) and per-repository security information (advisories with severity badges plus inline SECURITY.md policy). It includes domain models, data layer with GitHub API integration, presentation UI screens, navigation routing, multilingual localization, and WhatsNew release notes. ChangesRepository Issues and Security Browsing Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 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 docstrings
🧪 Generate unit tests (beta)
|
Greptile SummaryThis PR introduces a new
Confidence Score: 4/5Safe to merge; the new module is read-only and GitHub-only, with no writes to local storage or the GitHub API. All three issues found are non-blocking quality observations. The feature is self-contained and additive. The
Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as DetailsScreen
participant Nav as AppNavigation
participant IVM as IssuesViewModel
participant DVM as IssueDetailViewModel
participant SVM as SecurityViewModel
participant Repo as RepoPagesRepositoryImpl
participant GH as GitHub REST API
UI->>Nav: onNavigateToIssues(owner, repo)
Nav->>IVM: create(owner, repo)
IVM->>Repo: "getIssues(owner, repo, OPEN, page=1)"
Repo->>GH: "GET /repos/{owner}/{repo}/issues?state=open&per_page=30"
GH-->>Repo: "List<IssueDto> (PRs filtered out)"
Repo-->>IVM: "Result.success(List<RepoIssue>)"
IVM-->>UI: IssuesUiState(issues, endReached)
UI->>Nav: onOpenIssue(number)
Nav->>DVM: create(owner, repo, number)
par fetch issue + comments
DVM->>Repo: getIssueDetail(owner, repo, number)
Repo->>GH: "GET /repos/{owner}/{repo}/issues/{number}"
GH-->>Repo: IssueDto
Repo->>GH: "GET /repos/{owner}/{repo}/issues/{number}/comments"
GH-->>Repo: "List<IssueCommentDto>"
end
Repo-->>DVM: Result.success(RepoIssueDetail)
UI->>Nav: onNavigateToSecurity(owner, repo)
Nav->>SVM: create(owner, repo)
SVM->>Repo: getSecurityOverview(owner, repo)
par fetch advisories + policy
Repo->>GH: "GET /repos/{owner}/{repo}/security-advisories"
GH-->>Repo: "List<SecurityAdvisoryDto>"
Repo->>GH: "GET /repos/{owner}/{repo}/contents/SECURITY.md"
GH-->>Repo: RepoContentDto (base64)
end
Repo-->>SVM: Result.success(SecurityOverview)
Reviews (1): Last reviewed commit: "whatsnew: add version 20 for Issues and ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+149
to
+151
| androidx.compose.runtime.LaunchedEffect(shouldLoadMore) { | ||
| if (shouldLoadMore) onLoadMore() | ||
| } |
There was a problem hiding this comment.
LaunchedEffect import — FQN used instead
LaunchedEffect is referenced by its fully-qualified name (androidx.compose.runtime.LaunchedEffect) while every other androidx.compose.runtime.* symbol in this file (remember, derivedStateOf, getValue, Composable) is properly imported at the top. The FQN works but is inconsistent with the file's style and breaks IDE "optimize imports" tooling.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| isLoadingMore = false, | ||
| issues = merged.toPersistentList(), | ||
| page = page, | ||
| endReached = newIssues.size < PER_PAGE, |
There was a problem hiding this comment.
PER_PAGE constant creates silent pagination drift risk
IssuesViewModel.PER_PAGE = 30 is a private constant that drives endReached = newIssues.size < PER_PAGE. The actual page size sent to the API is RepoPagesRepositoryImpl.PER_PAGE = 30, a separate private constant. If someone changes the repository constant (say, to 50) without updating the ViewModel, the ViewModel will incorrectly mark pages as complete when it receives 30–49 items, silently truncating infinite scroll. The ViewModel's constant should reflect the repository's actual request size — ideally the repository should expose the page size or the constant should be shared at the domain layer.
Comment on lines
+184
to
+189
| val response = httpClient.get("/repos/$owner/$repo/contents/$path") | ||
| if (response.status == HttpStatusCode.NotFound) continue | ||
| if (!response.status.isSuccess()) continue | ||
| val dto: RepoContentDto = response.body() | ||
| val raw = dto.content ?: continue | ||
| val decoded = Base64.Mime.decode(raw).decodeToString() |
There was a problem hiding this comment.
encoding field not checked before Base64 decode
The GitHub Contents API always returns encoding: "base64" for files under 1 MB, so Base64.Mime.decode(raw) works in practice. However, the RepoContentDto already includes an encoding field — if a repo's SECURITY.md is unusually large and the API ever returns a non-base64 payload (or a future API version changes defaults), the decode will silently fail or produce garbage, caught by the surrounding try-catch and skipped as if the file doesn't exist. A guard if (dto.encoding != "base64") continue before the decode would make the intent explicit and the failure observable.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
feature/repo-pages/presentation/src/commonMain/kotlin/zed/rainxch/repopages/presentation/security/SecurityRoot.kt (1)
182-207: ⚖️ Poor tradeoffSeverity colors are hard-coded and not theme-aware.
These fixed hues (e.g.
0xFFCF222E) are applied both as the text color and, at 16% alpha, as the badge background. On a dark background the solid foreground hue over a faint same-hue fill can land below the WCAG 4.5:1 contrast threshold, hurting legibility. Consider deriving these fromMaterialTheme.colorScheme(or providing light/dark variants) so the badge adapts to the active theme.🤖 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/repo-pages/presentation/src/commonMain/kotlin/zed/rainxch/repopages/presentation/security/SecurityRoot.kt` around lines 182 - 207, The severity badge currently uses hard-coded Color literals (in the when for color) for both foreground and translucent background, which is not theme-aware; modify the mapping in the severity -> color decision (the variable named color) to pick colors from MaterialTheme.colorScheme (or from a new severityColorProvider that returns light/dark variants) instead of raw hex values, then use color.copy(alpha = 0.16f) for the Surface background and color for Text as before; ensure the chosen color tokens (e.g., MaterialTheme.colorScheme.error, secondary, or custom severity tokens) provide sufficient contrast on both light and dark themes and update any helper (severity label mapping) if you add new theme tokens.
🤖 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 `@core/presentation/src/commonMain/composeResources/values-ru/strings-ru.xml`:
- Around line 1330-1334: The Russian resource file contains English or mixed
strings for issue-related keys; update the values for repo_pages_issues_title to
"Проблемы", repo_pages_issues_empty_open to "Нет открытых проблем.", and
repo_pages_issues_empty_closed to "Нет закрытых проблем." (leave
repo_pages_issues_filter_open/closed as appropriate if already Russian), and
make the same replacements for the duplicate occurrence referenced around line
1352 to keep translations consistent.
In `@core/presentation/src/commonMain/composeResources/values-tr/strings-tr.xml`:
- Around line 1332-1336: Replace the inconsistent Turkish term "Issue'lar" with
the project-standard "Sorunlar" across the listed string resources: update
repo_pages_issues_title, repo_pages_issues_filter_open,
repo_pages_issues_filter_closed, repo_pages_issues_empty_open, and
repo_pages_issues_empty_closed to use "Sorunlar" (and the corresponding
localized words for open/closed if needed) so they match the existing `issues`
translations elsewhere; also apply the same change to the duplicate occurrence
referenced at line 1354.
In
`@feature/repo-pages/data/src/commonMain/kotlin/zed/rainxch/repopages/data/repository/RepoPagesRepositoryImpl.kt`:
- Around line 85-87: getIssueDetail (and other places) only requests one
comments page (parameter per_page=COMMENTS_PER_PAGE) causing threads >100
comments to be truncated; implement paginated fetching: add a helper like
fetchAllIssueComments(owner, repo, number) that loops pages (parameter page
starting at 1), requests /repos/$owner/$repo/issues/$number/comments with
per_page=COMMENTS_PER_PAGE and page, accumulates dtos.map { it.toIssueComment()
} into a list, breaks when returned dto count < COMMENTS_PER_PAGE or on
non-success status, and return the full list; replace the direct
httpClient.get(...) calls in RepoPagesRepositoryImpl (the locations around
getIssueDetail and the other noted call sites) with calls to this helper.
- Around line 80-95: The code launches commentsDeferred before knowing the issue
exists, wasting requests when the issue fetch fails; modify the coroutineScope
in RepoPagesRepositoryImpl so you first async/await the issue call
(issueDeferred → await issueResponse) and only if
issueResponse.status.isSuccess() create and await the comments async
(commentsDeferred) using the same
httpClient.get("/repos/$owner/$repo/issues/$number/comments") with
parameter("per_page", COMMENTS_PER_PAGE); keep existing error handling
(return@coroutineScope Result.failure(...)) when the issue fetch fails.
In
`@feature/repo-pages/presentation/src/commonMain/kotlin/zed/rainxch/repopages/presentation/issuedetail/IssueDetailViewModel.kt`:
- Around line 14-46: The ViewModel must adopt the State/Action/Event pattern:
add a sealed interface Action (e.g., object Load and object Retry) and replace
public methods like retry() with a single public fun accept(action: Action) that
dispatches actions (calling load() for Load/Retry); add a private Channel<Event>
and expose events as val events = channel.receiveAsFlow(), define an Event
sealed class (e.g., ShowError(message: String)); update load() to be private
and, onFailure of repository.getIssueDetail(...), send an Event into the Channel
(instead of only setting errorMessage) and/or set state as needed; keep state as
MutableStateFlow and ensure accept(action) is invoked from UI.
In
`@feature/repo-pages/presentation/src/commonMain/kotlin/zed/rainxch/repopages/presentation/issues/IssuesViewModel.kt`:
- Around line 17-93: The ViewModel must adopt the State/Action/Event contract:
create a sealed interface (e.g., IssuesAction) for inputs (Open, Closed, Retry,
LoadNextPage, SetFilter(filter: IssueState)) and a sealed interface (e.g.,
IssuesEvent) for one‑off outputs, add a private Channel<IssuesEvent> and expose
events as eventChannel.receiveAsFlow(), then replace public methods retry(),
setFilter(), loadNextPage(), and any direct calls to load(...) with a single
public function handle(action: IssuesAction) that dispatches actions (via when
on IssuesAction) and calls the existing private helpers (load, loadPage) or
sends events into the channel (e.g., error events); keep the MutableStateFlow
_state and its updates intact and ensure eventChannel usage is done from
viewModelScope when emitting events.
- Around line 42-77: The loadPage results can arrive out of order and overwrite
newer UI state; add a generation token to the ViewModel (e.g., a private var
requestGeneration = 0) that you increment in load(filter: IssueState) before
calling loadPage, capture the current generation at the start of loadPage's
coroutine (val gen = requestGeneration), and before any _state.update after
repository.getIssues completes, check that gen == requestGeneration (or return
early) to discard stale responses; update references to load, loadPage,
viewModelScope.launch, and repository.getIssues accordingly so only the latest
request can modify _state.
In
`@feature/repo-pages/presentation/src/commonMain/kotlin/zed/rainxch/repopages/presentation/security/SecurityRoot.kt`:
- Around line 74-78: The when expression in SecurityRoot (where state.isLoading,
state.errorMessage, state.overview are checked) can produce an empty body for
the initial SecurityUiState() because all three conditions can be false/null;
add a fallback branch to the when (e.g., else -> RepoPagesLoading()) so the UI
always renders something, or alternatively initialize SecurityUiState with
isLoading=true in the SecurityViewModel/constructor; update either the when in
SecurityRoot.kt to include an else -> RepoPagesLoading() or change the
SecurityUiState default to isLoading=true to ensure a non-empty initial render.
In
`@feature/repo-pages/presentation/src/commonMain/kotlin/zed/rainxch/repopages/presentation/security/SecurityViewModel.kt`:
- Around line 14-45: Refactor SecurityViewModel to follow State/Action/Event
pattern: create a sealed interface SecurityAction (e.g., Load/Retry/Refresh) and
expose a sendAction(Action) entrypoint instead of public methods; add a private
Channel<SecurityEvent> and expose events via events = channel.receiveAsFlow();
keep state as _state/ state StateFlow (SecurityUiState) but move the loading
logic from load() into a coroutine that collects actions (e.g., in init launch a
coroutine that receives actions from a Channel or a Flow and handles
SecurityAction.Load by calling repository.getSecurityOverview(owner, repo));
emit UI events (success/error) to the events Channel instead of only setting
errorMessage when appropriate; remove public retry(), map UI retry to sending
SecurityAction.Retry, and ensure viewModelScope is used for action processing
and repository calls (refer to SecurityViewModel, load,
repository.getSecurityOverview, _state, and viewModelScope).
---
Nitpick comments:
In
`@feature/repo-pages/presentation/src/commonMain/kotlin/zed/rainxch/repopages/presentation/security/SecurityRoot.kt`:
- Around line 182-207: The severity badge currently uses hard-coded Color
literals (in the when for color) for both foreground and translucent background,
which is not theme-aware; modify the mapping in the severity -> color decision
(the variable named color) to pick colors from MaterialTheme.colorScheme (or
from a new severityColorProvider that returns light/dark variants) instead of
raw hex values, then use color.copy(alpha = 0.16f) for the Surface background
and color for Text as before; ensure the chosen color tokens (e.g.,
MaterialTheme.colorScheme.error, secondary, or custom severity tokens) provide
sufficient contrast on both light and dark themes and update any helper
(severity label mapping) if you add new theme tokens.
🪄 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: a20110e2-550d-42f5-b73d-4799757c47b8
📒 Files selected for processing (57)
composeApp/build.gradle.ktscomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/di/ViewModelsModule.ktcomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/di/initKoin.ktcomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/navigation/AdaptiveDetailPaneContent.ktcomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/navigation/AppNavigation.ktcomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/navigation/GithubStoreGraph.ktcomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/whatsnew/WhatsNewLoaderImpl.ktcore/presentation/src/commonMain/composeResources/files/whatsnew/20.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/ar/20.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/bn/20.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/es/20.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/fr/20.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/hi/20.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/it/20.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/ja/20.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/ko/20.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/pl/20.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/ru/20.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/tr/20.jsoncore/presentation/src/commonMain/composeResources/files/whatsnew/zh-CN/20.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/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsRoot.ktfeature/repo-pages/data/build.gradle.ktsfeature/repo-pages/data/src/commonMain/kotlin/zed/rainxch/repopages/data/di/SharedModule.ktfeature/repo-pages/data/src/commonMain/kotlin/zed/rainxch/repopages/data/dto/IssueDto.ktfeature/repo-pages/data/src/commonMain/kotlin/zed/rainxch/repopages/data/dto/SecurityAdvisoryDto.ktfeature/repo-pages/data/src/commonMain/kotlin/zed/rainxch/repopages/data/mappers/RepoPagesMappers.ktfeature/repo-pages/data/src/commonMain/kotlin/zed/rainxch/repopages/data/repository/RepoPagesRepositoryImpl.ktfeature/repo-pages/domain/build.gradle.ktsfeature/repo-pages/domain/src/commonMain/kotlin/zed/rainxch/repopages/domain/model/RepoIssue.ktfeature/repo-pages/domain/src/commonMain/kotlin/zed/rainxch/repopages/domain/model/RepoIssueDetail.ktfeature/repo-pages/domain/src/commonMain/kotlin/zed/rainxch/repopages/domain/model/SecurityOverview.ktfeature/repo-pages/domain/src/commonMain/kotlin/zed/rainxch/repopages/domain/repository/RepoPagesRepository.ktfeature/repo-pages/presentation/build.gradle.ktsfeature/repo-pages/presentation/src/commonMain/kotlin/zed/rainxch/repopages/presentation/components/RepoPagesComponents.ktfeature/repo-pages/presentation/src/commonMain/kotlin/zed/rainxch/repopages/presentation/issuedetail/IssueDetailRoot.ktfeature/repo-pages/presentation/src/commonMain/kotlin/zed/rainxch/repopages/presentation/issuedetail/IssueDetailUiState.ktfeature/repo-pages/presentation/src/commonMain/kotlin/zed/rainxch/repopages/presentation/issuedetail/IssueDetailViewModel.ktfeature/repo-pages/presentation/src/commonMain/kotlin/zed/rainxch/repopages/presentation/issues/IssuesRoot.ktfeature/repo-pages/presentation/src/commonMain/kotlin/zed/rainxch/repopages/presentation/issues/IssuesUiState.ktfeature/repo-pages/presentation/src/commonMain/kotlin/zed/rainxch/repopages/presentation/issues/IssuesViewModel.ktfeature/repo-pages/presentation/src/commonMain/kotlin/zed/rainxch/repopages/presentation/security/SecurityRoot.ktfeature/repo-pages/presentation/src/commonMain/kotlin/zed/rainxch/repopages/presentation/security/SecurityUiState.ktfeature/repo-pages/presentation/src/commonMain/kotlin/zed/rainxch/repopages/presentation/security/SecurityViewModel.ktsettings.gradle.kts
Comment on lines
+1330
to
+1334
| <string name="repo_pages_issues_title">Issues</string> | ||
| <string name="repo_pages_issues_filter_open">Открытые</string> | ||
| <string name="repo_pages_issues_filter_closed">Закрытые</string> | ||
| <string name="repo_pages_issues_empty_open">Нет открытых issue.</string> | ||
| <string name="repo_pages_issues_empty_closed">Нет закрытых issue.</string> |
Contributor
There was a problem hiding this comment.
Localize remaining English “Issues” strings in Russian block.
These entries are still English in values-ru, causing mixed-language UI. Please translate them consistently with existing Russian wording (e.g., “Проблемы” / “Нет открытых проблем.” / “Нет закрытых проблем.”).
Also applies to: 1352-1352
🤖 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-ru/strings-ru.xml`
around lines 1330 - 1334, The Russian resource file contains English or mixed
strings for issue-related keys; update the values for repo_pages_issues_title to
"Проблемы", repo_pages_issues_empty_open to "Нет открытых проблем.", and
repo_pages_issues_empty_closed to "Нет закрытых проблем." (leave
repo_pages_issues_filter_open/closed as appropriate if already Russian), and
make the same replacements for the duplicate occurrence referenced around line
1352 to keep translations consistent.
Comment on lines
+1332
to
+1336
| <string name="repo_pages_issues_title">Issue'lar</string> | ||
| <string name="repo_pages_issues_filter_open">Açık</string> | ||
| <string name="repo_pages_issues_filter_closed">Kapalı</string> | ||
| <string name="repo_pages_issues_empty_open">Açık issue yok.</string> | ||
| <string name="repo_pages_issues_empty_closed">Kapalı issue yok.</string> |
Contributor
There was a problem hiding this comment.
Use consistent Turkish terminology for “Issues”.
This block uses Issue'lar, but the locale already uses Sorunlar elsewhere (issues). Please align these new keys to the same term for consistency.
Also applies to: 1354-1354
🤖 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-tr/strings-tr.xml`
around lines 1332 - 1336, Replace the inconsistent Turkish term "Issue'lar" with
the project-standard "Sorunlar" across the listed string resources: update
repo_pages_issues_title, repo_pages_issues_filter_open,
repo_pages_issues_filter_closed, repo_pages_issues_empty_open, and
repo_pages_issues_empty_closed to use "Sorunlar" (and the corresponding
localized words for open/closed if needed) so they match the existing `issues`
translations elsewhere; also apply the same change to the duplicate occurrence
referenced at line 1354.
Comment on lines
+80
to
+95
| coroutineScope { | ||
| val issueDeferred = async { | ||
| httpClient.get("/repos/$owner/$repo/issues/$number") | ||
| } | ||
| val commentsDeferred = async { | ||
| httpClient.get("/repos/$owner/$repo/issues/$number/comments") { | ||
| parameter("per_page", COMMENTS_PER_PAGE) | ||
| } | ||
| } | ||
|
|
||
| val issueResponse = issueDeferred.await() | ||
| if (!issueResponse.status.isSuccess()) { | ||
| return@coroutineScope Result.failure( | ||
| Exception("Failed to fetch issue: ${issueResponse.status.description}"), | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid firing comments request before confirming the issue exists.
If the issue fetch fails (e.g., 404), the comments request was already sent, adding avoidable latency and API usage on an error path.
Proposed fix
- coroutineScope {
- val issueDeferred = async {
- httpClient.get("/repos/$owner/$repo/issues/$number")
- }
- val commentsDeferred = async {
- httpClient.get("/repos/$owner/$repo/issues/$number/comments") {
- parameter("per_page", COMMENTS_PER_PAGE)
- }
- }
-
- val issueResponse = issueDeferred.await()
+ coroutineScope {
+ val issueResponse = httpClient.get("/repos/$owner/$repo/issues/$number")
if (!issueResponse.status.isSuccess()) {
return@coroutineScope Result.failure(
Exception("Failed to fetch issue: ${issueResponse.status.description}"),
)
}
val issueDto: IssueDto = issueResponse.body()
+ val commentsDeferred = async {
+ httpClient.get("/repos/$owner/$repo/issues/$number/comments") {
+ parameter("per_page", COMMENTS_PER_PAGE)
+ }
+ }🤖 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/repo-pages/data/src/commonMain/kotlin/zed/rainxch/repopages/data/repository/RepoPagesRepositoryImpl.kt`
around lines 80 - 95, The code launches commentsDeferred before knowing the
issue exists, wasting requests when the issue fetch fails; modify the
coroutineScope in RepoPagesRepositoryImpl so you first async/await the issue
call (issueDeferred → await issueResponse) and only if
issueResponse.status.isSuccess() create and await the comments async
(commentsDeferred) using the same
httpClient.get("/repos/$owner/$repo/issues/$number/comments") with
parameter("per_page", COMMENTS_PER_PAGE); keep existing error handling
(return@coroutineScope Result.failure(...)) when the issue fetch fails.
Comment on lines
+85
to
+87
| httpClient.get("/repos/$owner/$repo/issues/$number/comments") { | ||
| parameter("per_page", COMMENTS_PER_PAGE) | ||
| } |
Contributor
There was a problem hiding this comment.
Issue comments are truncated at 100 entries.
getIssueDetail fetches only one comments page (per_page=100), so large threads are silently incomplete.
Proposed direction
- val commentsDeferred = async {
- httpClient.get("/repos/$owner/$repo/issues/$number/comments") {
- parameter("per_page", COMMENTS_PER_PAGE)
- }
- }
+ val commentsDeferred = async { fetchAllIssueComments(owner, repo, number) }
- val comments = if (commentsResponse.status.isSuccess()) {
- val commentDtos: List<IssueCommentDto> = commentsResponse.body()
- commentDtos.map { it.toIssueComment() }
- } else {
- emptyList()
- }
+ val comments = commentsDeferred.await()private suspend fun fetchAllIssueComments(
owner: String,
repo: String,
number: Int,
): List<RepoIssueComment> {
val out = mutableListOf<RepoIssueComment>()
var page = 1
while (true) {
val response = httpClient.get("/repos/$owner/$repo/issues/$number/comments") {
parameter("per_page", COMMENTS_PER_PAGE)
parameter("page", page)
}
if (!response.status.isSuccess()) return out
val dtos: List<IssueCommentDto> = response.body()
out += dtos.map { it.toIssueComment() }
if (dtos.size < COMMENTS_PER_PAGE) break
page++
}
return out
}Also applies to: 98-104, 204-204
🤖 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/repo-pages/data/src/commonMain/kotlin/zed/rainxch/repopages/data/repository/RepoPagesRepositoryImpl.kt`
around lines 85 - 87, getIssueDetail (and other places) only requests one
comments page (parameter per_page=COMMENTS_PER_PAGE) causing threads >100
comments to be truncated; implement paginated fetching: add a helper like
fetchAllIssueComments(owner, repo, number) that loops pages (parameter page
starting at 1), requests /repos/$owner/$repo/issues/$number/comments with
per_page=COMMENTS_PER_PAGE and page, accumulates dtos.map { it.toIssueComment()
} into a list, breaks when returned dto count < COMMENTS_PER_PAGE or on
non-success status, and return the full list; replace the direct
httpClient.get(...) calls in RepoPagesRepositoryImpl (the locations around
getIssueDetail and the other noted call sites) with calls to this helper.
Comment on lines
+14
to
+46
| class IssueDetailViewModel( | ||
| private val owner: String, | ||
| private val repo: String, | ||
| private val number: Int, | ||
| private val repository: RepoPagesRepository, | ||
| ) : ViewModel() { | ||
| private val _state = MutableStateFlow(IssueDetailUiState()) | ||
| val state = _state.asStateFlow() | ||
|
|
||
| init { | ||
| load() | ||
| } | ||
|
|
||
| fun retry() = load() | ||
|
|
||
| private fun load() { | ||
| viewModelScope.launch { | ||
| _state.update { it.copy(isLoading = true, errorMessage = null) } | ||
| repository.getIssueDetail(owner, repo, number) | ||
| .onSuccess { detail -> | ||
| _state.update { it.copy(isLoading = false, detail = detail, errorMessage = null) } | ||
| } | ||
| .onFailure { e -> | ||
| _state.update { | ||
| it.copy( | ||
| isLoading = false, | ||
| errorMessage = e.message ?: getString(Res.string.failed_to_load), | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Align this ViewModel with the required Action/Event API shape.
Current API uses direct public methods only; it should consume sealed Actions and publish one-off Events via Channel.receiveAsFlow().
As per coding guidelines, “Every ViewModel must follow the State/Action/Event pattern with State exposed via StateFlow, Actions as sealed interface for user input, and Events sent via Channel.receiveAsFlow()”.
🤖 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/repo-pages/presentation/src/commonMain/kotlin/zed/rainxch/repopages/presentation/issuedetail/IssueDetailViewModel.kt`
around lines 14 - 46, The ViewModel must adopt the State/Action/Event pattern:
add a sealed interface Action (e.g., object Load and object Retry) and replace
public methods like retry() with a single public fun accept(action: Action) that
dispatches actions (calling load() for Load/Retry); add a private Channel<Event>
and expose events as val events = channel.receiveAsFlow(), define an Event
sealed class (e.g., ShowError(message: String)); update load() to be private
and, onFailure of repository.getIssueDetail(...), send an Event into the Channel
(instead of only setting errorMessage) and/or set state as needed; keep state as
MutableStateFlow and ensure accept(action) is invoked from UI.
Comment on lines
+17
to
+93
| class IssuesViewModel( | ||
| private val owner: String, | ||
| private val repo: String, | ||
| private val repository: RepoPagesRepository, | ||
| ) : ViewModel() { | ||
| private val _state = MutableStateFlow(IssuesUiState()) | ||
| val state = _state.asStateFlow() | ||
|
|
||
| init { | ||
| load(IssueState.OPEN) | ||
| } | ||
|
|
||
| fun retry() = load(_state.value.filter) | ||
|
|
||
| fun setFilter(filter: IssueState) { | ||
| if (_state.value.filter == filter && _state.value.errorMessage == null) return | ||
| load(filter) | ||
| } | ||
|
|
||
| fun loadNextPage() { | ||
| val s = _state.value | ||
| if (s.isLoading || s.isLoadingMore || s.endReached || s.errorMessage != null) return | ||
| loadPage(s.filter, s.page + 1, append = true) | ||
| } | ||
|
|
||
| private fun load(filter: IssueState) { | ||
| _state.update { | ||
| it.copy( | ||
| filter = filter, | ||
| issues = persistentListOf(), | ||
| page = 1, | ||
| endReached = false, | ||
| errorMessage = null, | ||
| ) | ||
| } | ||
| loadPage(filter, page = 1, append = false) | ||
| } | ||
|
|
||
| private fun loadPage( | ||
| filter: IssueState, | ||
| page: Int, | ||
| append: Boolean, | ||
| ) { | ||
| viewModelScope.launch { | ||
| _state.update { | ||
| if (append) it.copy(isLoadingMore = true) else it.copy(isLoading = true, errorMessage = null) | ||
| } | ||
| repository.getIssues(owner, repo, filter, page) | ||
| .onSuccess { newIssues -> | ||
| _state.update { st -> | ||
| val merged = if (append) st.issues + newIssues else newIssues | ||
| st.copy( | ||
| isLoading = false, | ||
| isLoadingMore = false, | ||
| issues = merged.toPersistentList(), | ||
| page = page, | ||
| endReached = newIssues.size < PER_PAGE, | ||
| errorMessage = null, | ||
| ) | ||
| } | ||
| } | ||
| .onFailure { e -> | ||
| _state.update { | ||
| it.copy( | ||
| isLoading = false, | ||
| isLoadingMore = false, | ||
| errorMessage = e.message ?: getString(Res.string.failed_to_load), | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| companion object { | ||
| private const val PER_PAGE = 30 | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Adopt the required State/Action/Event ViewModel contract.
This ViewModel exposes methods directly and does not define sealed Action input + Event output via Channel.receiveAsFlow().
As per coding guidelines, “Every ViewModel must follow the State/Action/Event pattern with State exposed via StateFlow, Actions as sealed interface for user input, and Events sent via Channel.receiveAsFlow()”.
🤖 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/repo-pages/presentation/src/commonMain/kotlin/zed/rainxch/repopages/presentation/issues/IssuesViewModel.kt`
around lines 17 - 93, The ViewModel must adopt the State/Action/Event contract:
create a sealed interface (e.g., IssuesAction) for inputs (Open, Closed, Retry,
LoadNextPage, SetFilter(filter: IssueState)) and a sealed interface (e.g.,
IssuesEvent) for one‑off outputs, add a private Channel<IssuesEvent> and expose
events as eventChannel.receiveAsFlow(), then replace public methods retry(),
setFilter(), loadNextPage(), and any direct calls to load(...) with a single
public function handle(action: IssuesAction) that dispatches actions (via when
on IssuesAction) and calls the existing private helpers (load, loadPage) or
sends events into the channel (e.g., error events); keep the MutableStateFlow
_state and its updates intact and ensure eventChannel usage is done from
viewModelScope when emitting events.
Comment on lines
+42
to
+77
| private fun load(filter: IssueState) { | ||
| _state.update { | ||
| it.copy( | ||
| filter = filter, | ||
| issues = persistentListOf(), | ||
| page = 1, | ||
| endReached = false, | ||
| errorMessage = null, | ||
| ) | ||
| } | ||
| loadPage(filter, page = 1, append = false) | ||
| } | ||
|
|
||
| private fun loadPage( | ||
| filter: IssueState, | ||
| page: Int, | ||
| append: Boolean, | ||
| ) { | ||
| viewModelScope.launch { | ||
| _state.update { | ||
| if (append) it.copy(isLoadingMore = true) else it.copy(isLoading = true, errorMessage = null) | ||
| } | ||
| repository.getIssues(owner, repo, filter, page) | ||
| .onSuccess { newIssues -> | ||
| _state.update { st -> | ||
| val merged = if (append) st.issues + newIssues else newIssues | ||
| st.copy( | ||
| isLoading = false, | ||
| isLoadingMore = false, | ||
| issues = merged.toPersistentList(), | ||
| page = page, | ||
| endReached = newIssues.size < PER_PAGE, | ||
| errorMessage = null, | ||
| ) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Prevent stale async responses from overriding newer state.
loadPage() can complete out of order (e.g., quick filter switches), causing older results to overwrite current UI state.
Suggested guard using request generation
class IssuesViewModel(
@@
) : ViewModel() {
private val _state = MutableStateFlow(IssuesUiState())
val state = _state.asStateFlow()
+ private var requestGeneration: Long = 0
@@
private fun load(filter: IssueState) {
+ requestGeneration += 1
+ val generation = requestGeneration
_state.update {
@@
}
- loadPage(filter, page = 1, append = false)
+ loadPage(filter, page = 1, append = false, generation = generation)
}
@@
fun loadNextPage() {
val s = _state.value
if (s.isLoading || s.isLoadingMore || s.endReached || s.errorMessage != null) return
- loadPage(s.filter, s.page + 1, append = true)
+ loadPage(s.filter, s.page + 1, append = true, generation = requestGeneration)
}
@@
private fun loadPage(
filter: IssueState,
page: Int,
append: Boolean,
+ generation: Long,
) {
viewModelScope.launch {
@@
repository.getIssues(owner, repo, filter, page)
.onSuccess { newIssues ->
+ if (generation != requestGeneration) return@onSuccess
_state.update { st ->
@@
}
.onFailure { e ->
+ if (generation != requestGeneration) return@onFailure
_state.update {🤖 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/repo-pages/presentation/src/commonMain/kotlin/zed/rainxch/repopages/presentation/issues/IssuesViewModel.kt`
around lines 42 - 77, The loadPage results can arrive out of order and overwrite
newer UI state; add a generation token to the ViewModel (e.g., a private var
requestGeneration = 0) that you increment in load(filter: IssueState) before
calling loadPage, capture the current generation at the start of loadPage's
coroutine (val gen = requestGeneration), and before any _state.update after
repository.getIssues completes, check that gen == requestGeneration (or return
early) to discard stale responses; update references to load, loadPage,
viewModelScope.launch, and repository.getIssues accordingly so only the latest
request can modify _state.
Comment on lines
+74
to
+78
| when { | ||
| state.isLoading -> RepoPagesLoading() | ||
| state.errorMessage != null -> RepoPagesError(message = state.errorMessage, onRetry = onRetry) | ||
| state.overview != null -> SecurityContent(overview = state.overview) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect SecurityUiState defaults to confirm the initial state cannot leave all three branches unmatched.
fd -t f 'SecurityUiState.kt' --exec cat {}Repository: OpenHub-Store/GitHub-Store
Length of output: 331
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect SecurityRoot.kt around the flagged when block
SEC_ROOT="feature/repo-pages/presentation/src/commonMain/kotlin/zed/rainxch/repopages/presentation/security/SecurityRoot.kt"
if [ -f "$SEC_ROOT" ]; then
echo "---- $SEC_ROOT (context around lines 60-95) ----"
nl -ba "$SEC_ROOT" | sed -n '60,105p'
else
echo "Missing file: $SEC_ROOT" >&2
exit 1
fi
# 2) Find where SecurityUiState is created/updated (viewmodel/store/collectAsState usage)
echo "---- Searching for SecurityUiState usage in security presentation ----"
fd -t f --glob '*security*' 'feature/repo-pages' | wc -l
fd -t f --glob '*security*' 'feature/repo-pages' | xargs rg -n "SecurityUiState" || true
echo "---- Searching for isLoading assignments in security presentation ----"
fd -t f --glob '*security*' 'feature/repo-pages' | xargs rg -n "isLoading\s*=" || trueRepository: OpenHub-Store/GitHub-Store
Length of output: 284
🏁 Script executed:
#!/bin/bash
set -euo pipefail
SEC_ROOT="feature/repo-pages/presentation/src/commonMain/kotlin/zed/rainxch/repopages/presentation/security/SecurityRoot.kt"
if [ -f "$SEC_ROOT" ]; then
echo "---- $SEC_ROOT (lines 55-110) ----"
cat -n "$SEC_ROOT" | sed -n '55,110p'
else
echo "Missing file: $SEC_ROOT" >&2
exit 1
fi
echo "---- Searching for SecurityUiState usage in security presentation ----"
# Print matching lines with file+line for accuracy; keep output bounded
fd -t f 'SecurityUiState.kt' 'feature/repo-pages' -x cat >/dev/null || true
fd -t f --glob '*security*' 'feature/repo-pages' \
| xargs rg -n "SecurityUiState" || true
echo "---- Searching for isLoading default/assignments in security presentation ----"
fd -t f --glob '*security*' 'feature/repo-pages' \
| xargs rg -n "isLoading\s*=" || trueRepository: OpenHub-Store/GitHub-Store
Length of output: 3764
🏁 Script executed:
#!/bin/bash
set -euo pipefail
SEC_VM="feature/repo-pages/presentation/src/commonMain/kotlin/zed/rainxch/repopages/presentation/security/SecurityViewModel.kt"
SEC_ROOT="feature/repo-pages/presentation/src/commonMain/kotlin/zed/rainxch/repopages/presentation/security/SecurityRoot.kt"
echo "---- $SEC_VM (lines 1-120) ----"
cat -n "$SEC_VM" | sed -n '1,120p'
echo "---- $SEC_ROOT (lines 1-120) ----"
cat -n "$SEC_ROOT" | sed -n '1,120p'Repository: OpenHub-Store/GitHub-Store
Length of output: 8263
Avoid empty body by handling the all-false/null state
SecurityUiState() defaults to isLoading=false, errorMessage=null, overview=null, and SecurityScreen’s when (no else) therefore matches none of the branches until SecurityViewModel’s coroutine flips isLoading=true, leading to an empty body (top bar only) for the initial frame. Add an else fallback (e.g., RepoPagesLoading()) or set the initial SecurityUiState to isLoading=true.
🤖 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/repo-pages/presentation/src/commonMain/kotlin/zed/rainxch/repopages/presentation/security/SecurityRoot.kt`
around lines 74 - 78, The when expression in SecurityRoot (where
state.isLoading, state.errorMessage, state.overview are checked) can produce an
empty body for the initial SecurityUiState() because all three conditions can be
false/null; add a fallback branch to the when (e.g., else -> RepoPagesLoading())
so the UI always renders something, or alternatively initialize SecurityUiState
with isLoading=true in the SecurityViewModel/constructor; update either the when
in SecurityRoot.kt to include an else -> RepoPagesLoading() or change the
SecurityUiState default to isLoading=true to ensure a non-empty initial render.
Comment on lines
+14
to
+45
| class SecurityViewModel( | ||
| private val owner: String, | ||
| private val repo: String, | ||
| private val repository: RepoPagesRepository, | ||
| ) : ViewModel() { | ||
| private val _state = MutableStateFlow(SecurityUiState()) | ||
| val state = _state.asStateFlow() | ||
|
|
||
| init { | ||
| load() | ||
| } | ||
|
|
||
| fun retry() = load() | ||
|
|
||
| private fun load() { | ||
| viewModelScope.launch { | ||
| _state.update { it.copy(isLoading = true, errorMessage = null) } | ||
| repository.getSecurityOverview(owner, repo) | ||
| .onSuccess { overview -> | ||
| _state.update { it.copy(isLoading = false, overview = overview, errorMessage = null) } | ||
| } | ||
| .onFailure { e -> | ||
| _state.update { | ||
| it.copy( | ||
| isLoading = false, | ||
| errorMessage = e.message ?: getString(Res.string.failed_to_load), | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Refactor to the project-standard State/Action/Event ViewModel pattern.
This class currently exposes direct method-based inputs and has no event stream backed by Channel.receiveAsFlow().
As per coding guidelines, “Every ViewModel must follow the State/Action/Event pattern with State exposed via StateFlow, Actions as sealed interface for user input, and Events sent via Channel.receiveAsFlow()”.
🤖 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/repo-pages/presentation/src/commonMain/kotlin/zed/rainxch/repopages/presentation/security/SecurityViewModel.kt`
around lines 14 - 45, Refactor SecurityViewModel to follow State/Action/Event
pattern: create a sealed interface SecurityAction (e.g., Load/Retry/Refresh) and
expose a sendAction(Action) entrypoint instead of public methods; add a private
Channel<SecurityEvent> and expose events via events = channel.receiveAsFlow();
keep state as _state/ state StateFlow (SecurityUiState) but move the loading
logic from load() into a coroutine that collects actions (e.g., in init launch a
coroutine that receives actions from a Channel or a Flow and handles
SecurityAction.Load by calling repository.getSecurityOverview(owner, repo));
emit UI events (success/error) to the events Channel instead of only setting
errorMessage when appropriate; remove public retry(), map UI retry to sending
SecurityAction.Retry, and ensure viewModelScope is used for action processing
and repository calls (refer to SecurityViewModel, load,
repository.getSecurityOverview, _state, and viewModelScope).
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.
Part of #633. New
feature/repo-pagesmodule: browse a GitHub repo's Issues (open/closed filter, labels, paginated list → detail with comments, images, Markdown) and Security (published advisories + SECURITY.md), reached via action-row buttons on Details. GitHub-only, read-only, direct REST — no backend/GraphQL.Adds 20 strings across 12 locales + whats-new v20 (1.9.1). Compiles on Android + Desktop.
Deferred: PRs/Actions/Wiki/Projects/Insights; long-repo perf profiling (#1/#4). Already-shipped #2/#3/#6 verified, not rebuilt.
Summary by CodeRabbit
New Features
Localization