search: update exploration logic and UI empty states#438
Conversation
- Add `passthroughAttempted` to `SearchState` to track if the backend already performed a GitHub search. - Integrate detailed Kermit logging for the "Explore from Github" flow. - Refactor `SearchViewModel` to delegate empty state messaging to the UI and improve pagination logic during exploration. - Update `SearchRoot` to display a centralized "no repositories found" view. - Prevent redundant manual exploration if a passthrough search was already attempted by the backend.
…uests - Update `requestTimeoutMillis` from 20,000 to 30,000ms. - Explicitly set `socketTimeoutMillis` to 30,000ms.
- Update `BackendSearchResponse` DTO to include an optional `passthroughAttempted` boolean. - Update `PaginatedDiscoveryRepositories` domain model to include `passthroughAttempted`. - Map the new field from the data layer to the domain layer in `SearchRepositoryImpl`.
…g logic - Add documentation for the `CrashReporter` on desktop, including log rotation, crash file generation, and platform-specific log paths. - Detail the security constraints for the `X-GitHub-Token` header, specifying that it is only forwarded to specific search endpoints to utilize user-side rate limits and is never logged.
WalkthroughThe PR enhances the search feature by adding passthrough attempt tracking throughout the data layer, domain model, and presentation tier. It also introduces tagged logging support to the logging infrastructure and adjusts timeout configuration in the search explore endpoint. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/search/presentation/src/commonMain/kotlin/zed/rainxch/search/presentation/SearchViewModel.kt (1)
295-318:⚠️ Potential issue | 🟡 MinorReset
passthroughAttemptedwhen starting a new initial search.On
isInitial, the state update clearsrepositories,errorMessage, andtotalCount, butpassthroughAttemptedis left at its previous value until the new result is emitted incollect(line 396). If the new search errors out, is cancelled, or simply takes a while, the UI state still carries the prior search'spassthroughAttempted = true, which gates the Explore CTA inSearchRoot.kt. Today the empty-state branch is additionally gated by!isLoadinganderrorMessage == null, so the visible impact is limited, but it's easy to regress if any other UI branch starts reading this flag.Proposed fix
_state.update { it.copy( isLoading = isInitial, isLoadingMore = !isInitial, errorMessage = null, repositories = if (isInitial) { persistentListOf() } else { it.repositories }, totalCount = if (isInitial) null else it.totalCount, + passthroughAttempted = if (isInitial) null else it.passthroughAttempted, ) }Also applies to: 391-398
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/search/presentation/src/commonMain/kotlin/zed/rainxch/search/presentation/SearchViewModel.kt` around lines 295 - 318, When starting a new initial search in SearchViewModel (inside the isInitial branch before launching currentSearchJob and inside the _state.update call), reset the passthroughAttempted flag to false so the UI doesn't inherit the previous search's value; specifically update the state mutation (the _state.update block that sets isLoading/isLoadingMore/errorMessage/repositories/totalCount) to also set passthroughAttempted = false whenever isInitial is true (and mirror this same reset where you clear state around lines 391-398).
🧹 Nitpick comments (1)
CLAUDE.md (1)
162-162: Optional wording cleanup for brevity.In Line 162, “by accident” can be shortened to “accidentally” for tighter phrasing.
✏️ Suggested doc edit
-- **`X-GitHub-Token` header:** Forwarded to the backend *only* on `/v1/search` and `/v1/search/explore` (so the backend's live GitHub passthrough runs under the user's own 5000/hr quota). Sourced from `TokenStore.currentToken()` via `BackendApiClient.currentUserGithubToken()` — the helper is `private` so other endpoints can't leak it by accident. Never sent on other endpoints (`/v1/categories`, `/v1/topics`, `/v1/repo`, `/v1/events`), never logged (no Ktor `Logging` plugin installed). +- **`X-GitHub-Token` header:** Forwarded to the backend *only* on `/v1/search` and `/v1/search/explore` (so the backend's live GitHub passthrough runs under the user's own 5000/hr quota). Sourced from `TokenStore.currentToken()` via `BackendApiClient.currentUserGithubToken()` — the helper is `private` so other endpoints can't leak it accidentally. Never sent on other endpoints (`/v1/categories`, `/v1/topics`, `/v1/repo`, `/v1/events`), never logged (no Ktor `Logging` plugin installed).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` at line 162, Edit the paragraph that begins with "**`X-GitHub-Token` header:**" and replace the phrase "by accident" with "accidentally" so the sentence reads tighter (e.g., "...the helper is `private` so other endpoints can't leak it accidentally."). Keep the rest of the sentence and punctuation unchanged and ensure the surrounding phrasing remains consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/network/BackendApiClient.kt`:
- Around line 143-148: The per-request timeouts in the timeout block
(requestTimeoutMillis and socketTimeoutMillis) are set to 30_000ms, which can
leave the UI stuck in ExploreStatus.LOADING when searchExplore (called from
exploreFromGithub → performExplore() and consumed with getOrThrow()) waits the
full 30s; reduce these timeouts to a more user-friendly value (e.g.,
10_000–15_000ms) or implement a cancellable request path (expose a
CancellationToken/Job from performExplore and wire a cancel affordance on the
explore button) so users can abort slow calls; update the timeout values in the
timeout { ... } block or add cancellation support around
searchExplore/performExplore accordingly.
In
`@feature/search/presentation/src/commonMain/kotlin/zed/rainxch/search/presentation/SearchViewModel.kt`:
- Around line 724-743: The current loop silently advances explorePage when
exploreResult.repos.isEmpty() && exploreResult.hasMore, which can lead to
repeated no-op clicks; fix by tracking consecutive empty-but-has-more pages
(e.g., add a private exploreEmptyStreak counter in SearchViewModel) and reuse or
mirror the existing MAX_AUTO_SKIP_PAGES behavior from
SearchRepositoryImpl.fallbackGithubSearch: increment exploreEmptyStreak when
added == 0, reset it when added > 0, and if exploreEmptyStreak >=
MAX_AUTO_SKIP_PAGES set _state.exploreStatus to EXHAUSTED (or set a new explicit
"NO_NEW_RESULTS" status flag) + log the event instead of remaining IDLE; update
references in the block around appendExploreResults, explorePage, exploreLog,
and _state.update accordingly and ensure the counter is persisted/cleared on new
searches.
- Line 5: SearchViewModel currently imports co.touchlab.kermit.Logger as
KermitLogger and calls KermitLogger.withTag(...) for SearchExplore logs which
bypasses the injected GitHubStoreLogger; remove the direct Kermit import and
replace usages of KermitLogger.withTag(...) to route through the injected
logger: GitHubStoreLogger (e.g., add an optional tag parameter to
GitHubStoreLogger like fun debug(message: String, tag: String? = null) and use
logger.debug(..., tag) or simply call logger.debug(message) if you prefer no
tag), updating SearchViewModel's logging calls to use logger instead of
KermitLogger so all logging uses the GitHubStoreLogger abstraction.
---
Outside diff comments:
In
`@feature/search/presentation/src/commonMain/kotlin/zed/rainxch/search/presentation/SearchViewModel.kt`:
- Around line 295-318: When starting a new initial search in SearchViewModel
(inside the isInitial branch before launching currentSearchJob and inside the
_state.update call), reset the passthroughAttempted flag to false so the UI
doesn't inherit the previous search's value; specifically update the state
mutation (the _state.update block that sets
isLoading/isLoadingMore/errorMessage/repositories/totalCount) to also set
passthroughAttempted = false whenever isInitial is true (and mirror this same
reset where you clear state around lines 391-398).
---
Nitpick comments:
In `@CLAUDE.md`:
- Line 162: Edit the paragraph that begins with "**`X-GitHub-Token` header:**"
and replace the phrase "by accident" with "accidentally" so the sentence reads
tighter (e.g., "...the helper is `private` so other endpoints can't leak it
accidentally."). Keep the rest of the sentence and punctuation unchanged and
ensure the surrounding phrasing remains consistent.
🪄 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: e54fd0c3-69fb-476b-8f8d-60c9d90e883b
📒 Files selected for processing (9)
CLAUDE.mdcore/data/src/commonMain/kotlin/zed/rainxch/core/data/dto/BackendSearchResponse.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/network/BackendApiClient.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/model/PaginatedDiscoveryRepositories.ktfeature/search/data/src/commonMain/kotlin/zed/rainxch/search/data/repository/SearchRepositoryImpl.ktfeature/search/presentation/build.gradle.ktsfeature/search/presentation/src/commonMain/kotlin/zed/rainxch/search/presentation/SearchRoot.ktfeature/search/presentation/src/commonMain/kotlin/zed/rainxch/search/presentation/SearchState.ktfeature/search/presentation/src/commonMain/kotlin/zed/rainxch/search/presentation/SearchViewModel.kt
|
|
||
| timeout { | ||
| requestTimeoutMillis = 30_000 | ||
| socketTimeoutMillis = 30_000 | ||
| } | ||
|
|
There was a problem hiding this comment.
Consider user-perceived delay for the 30s timeout.
The per-request timeout raises both requestTimeoutMillis and socketTimeoutMillis to 30s, which is 6× the client default of 5s. Since searchExplore is only invoked from the user-triggered exploreFromGithub → performExplore() flow (with getOrThrow() and no retry), a slow/unreachable backend can keep the UI in ExploreStatus.LOADING for up to 30 seconds with no way for the user to cancel from the UI. If the backend is expected to take this long, that's fine; otherwise consider tightening to ~10–15s and/or providing an explicit cancel affordance on the explore button.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/network/BackendApiClient.kt`
around lines 143 - 148, The per-request timeouts in the timeout block
(requestTimeoutMillis and socketTimeoutMillis) are set to 30_000ms, which can
leave the UI stuck in ExploreStatus.LOADING when searchExplore (called from
exploreFromGithub → performExplore() and consumed with getOrThrow()) waits the
full 30s; reduce these timeouts to a more user-friendly value (e.g.,
10_000–15_000ms) or implement a cancellable request path (expose a
CancellationToken/Job from performExplore and wire a cancel affordance on the
explore button) so users can abort slow calls; update the timeout values in the
timeout { ... } block or add cancellation support around
searchExplore/performExplore accordingly.
| val before = _state.value.repositories.size | ||
| if (exploreResult.repos.isNotEmpty()) { | ||
| appendExploreResults(exploreResult.repos) | ||
| } | ||
| val added = _state.value.repositories.size - before | ||
| val dupes = exploreResult.repos.size - added | ||
|
|
||
| if (exploreResult.hasMore) { | ||
| explorePage++ | ||
| exploreLog.d { | ||
| "-> IDLE: appended=$added dupes=$dupes nextPage=$explorePage" | ||
| } | ||
| _state.update { it.copy(exploreStatus = SearchState.ExploreStatus.IDLE) } | ||
| } else { | ||
| exploreLog.d { | ||
| "-> EXHAUSTED: appended=$added dupes=$dupes " + | ||
| "rawItems=${exploreResult.repos.size}" | ||
| } | ||
| _state.update { it.copy(exploreStatus = SearchState.ExploreStatus.EXHAUSTED) } | ||
| } |
There was a problem hiding this comment.
Edge case: empty repos with hasMore = true silently drops the attempt.
If the backend returns exploreResult.repos.isEmpty() but hasMore = true, this block: (a) skips appendExploreResults, (b) increments explorePage, and (c) sets status back to IDLE. The user sees no visible change and no "no more results" terminal state, which can look like the button did nothing. A subsequent click could repeat indefinitely if the backend keeps returning empty pages with hasMore = true.
Consider either capping consecutive empty-but-has-more responses (similar to MAX_AUTO_SKIP_PAGES in SearchRepositoryImpl.fallbackGithubSearch) or surfacing a small "no new results on this page" message so the interaction doesn't feel dead. Not blocking, just brittle.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/search/presentation/src/commonMain/kotlin/zed/rainxch/search/presentation/SearchViewModel.kt`
around lines 724 - 743, The current loop silently advances explorePage when
exploreResult.repos.isEmpty() && exploreResult.hasMore, which can lead to
repeated no-op clicks; fix by tracking consecutive empty-but-has-more pages
(e.g., add a private exploreEmptyStreak counter in SearchViewModel) and reuse or
mirror the existing MAX_AUTO_SKIP_PAGES behavior from
SearchRepositoryImpl.fallbackGithubSearch: increment exploreEmptyStreak when
added == 0, reset it when added > 0, and if exploreEmptyStreak >=
MAX_AUTO_SKIP_PAGES set _state.exploreStatus to EXHAUSTED (or set a new explicit
"NO_NEW_RESULTS" status flag) + log the event instead of remaining IDLE; update
references in the block around appendExploreResults, explorePage, exploreLog,
and _state.update accordingly and ensure the counter is persisted/cleared on new
searches.
- Add `withTag` support to `GitHubStoreLogger` and provide a Kermit-based implementation in `TaggedKermitLogger`. - Remove direct Kermit dependency from `feature:search:presentation` in favor of the core logging abstraction. - Update `SearchViewModel` to use the tagged logger for exploration tracing. - Update search state logic to track and reset `passthroughAttempted` status during query changes and initial loads. - Clean up documentation and unused imports.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/search/presentation/src/commonMain/kotlin/zed/rainxch/search/presentation/SearchViewModel.kt (1)
687-758:⚠️ Potential issue | 🟠 MajorExplore coroutine isn’t tracked — stale results/status can leak into a new search.
performExplore()launches intoviewModelScopewithout assigning theJobanywhere, whileperformSearch(isInitial = true)only cancelscurrentSearchJoband resetsexploreStatus = IDLE/passthroughAttempted = null(lines 299–304). Concrete race:
- User clicks Explore → explore job runs,
exploreStatus = LOADING.- User changes query / filter / sort / hits IME search →
performSearch(isInitial = true)resets explore state and starts a new search; the in-flight explore is not cancelled.- The explore job resolves against the old query and calls
appendExploreResults(...)(line 732), appending stale repos into the new search’srepositorieslist, and then clobbersexploreStatusback toIDLE/EXHAUSTED(lines 742/748) after the new search has already settled it.Suggested fix: track the explore job the same way as
currentSearchJob, cancel it on new-query resets and onOnClearClick, and bail out onCancellationExceptionbefore touching state.🛠 Sketch
private var currentSearchJob: Job? = null + private var currentExploreJob: Job? = null private var currentPage = 1 private var explorePage = 1if (isInitial) { currentSearchJob?.cancel() + currentExploreJob?.cancel() currentPage = 1 explorePage = 1 lastExploreQuery = query _state.update { it.copy( exploreStatus = SearchState.ExploreStatus.IDLE, passthroughAttempted = null, ) } }- viewModelScope.launch { + currentExploreJob = viewModelScope.launch { _state.update { it.copy(exploreStatus = SearchState.ExploreStatus.LOADING) } try { ... } catch (e: CancellationException) { throw e } catch (e: Exception) { ... } }Also consider cancelling it from
OnClearClickandOnSearchChange(blank/URL-only) paths that already cancelcurrentSearchJob, so explore state doesn’t reappear after the results are cleared.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/search/presentation/src/commonMain/kotlin/zed/rainxch/search/presentation/SearchViewModel.kt` around lines 687 - 758, performExplore launches an untracked coroutine (viewModelScope.launch) so its in-flight Job can append stale results and clobber exploreStatus after a new search; create and store a Job property (e.g., exploreJob: Job?) and assign it to the coroutine returned by viewModelScope.launch in performExplore, cancel exploreJob wherever currentSearchJob is cancelled (the performSearch(isInitial = true) reset path) and also cancel it in OnClearClick/OnSearchChange handlers, and ensure the explore coroutine bails on CancellationException before mutating state (rethrow or return immediately) and only updates state/ calls appendExploreResults when not cancelled. Use the existing symbols performExplore, exploreJob, currentSearchJob, appendExploreResults, OnClearClick, and OnSearchChange to locate and apply the changes.
🧹 Nitpick comments (2)
core/domain/src/commonMain/kotlin/zed/rainxch/core/domain/logging/GitHubStoreLogger.kt (1)
14-15: DefaultwithTagreturningthisis a silent footgun for alternate implementations.Any
GitHubStoreLoggerimplementation that doesn’t explicitly overridewithTag(e.g., test doubles, no-op loggers, future platform impls) will silently drop the tag while callers assume tagging is in effect. SinceKermitLoggeris currently the only production impl this is low risk, but consider making it abstract (no default) so the contract is enforced, or documenting on the interface that the default is a best-effort no-op so implementors know to override it when they have a backing tagging mechanism.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/domain/src/commonMain/kotlin/zed/rainxch/core/domain/logging/GitHubStoreLogger.kt` around lines 14 - 15, The default implementation of GitHubStoreLogger.withTag returning this can silently drop tags for implementations that forget to override it; change the interface so withTag is abstract (remove the default implementation) to force implementors to provide tagging behavior, then update the concrete implementation(s) such as KermitLogger (and any test doubles/no-op loggers) to implement withTag appropriately (either applying the tag to the underlying logger or explicitly documenting/returning a best-effort no-op) so callers cannot assume tagging is applied when it is not.feature/search/presentation/src/commonMain/kotlin/zed/rainxch/search/presentation/SearchViewModel.kt (1)
294-324: Minor: redundantpassthroughAttemptedreset on initial search.When
isInitialis true,passthroughAttemptedis set tonulltwice — once at lines 299–304 and again at lines 321–322 — and on non-initial the explicitpassthroughAttempted = it.passthroughAttemptedis a no-op. Not incorrect, just noise. Consider dropping the second branch or collapsing both resets into the outerisInitialblock.♻️ Suggested simplification
_state.update { it.copy( isLoading = isInitial, isLoadingMore = !isInitial, errorMessage = null, repositories = if (isInitial) { persistentListOf() } else { it.repositories }, totalCount = if (isInitial) null else it.totalCount, - passthroughAttempted = - if (isInitial) null else it.passthroughAttempted, ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/search/presentation/src/commonMain/kotlin/zed/rainxch/search/presentation/SearchViewModel.kt` around lines 294 - 324, The passthroughAttempted field is redundantly reset to null for initial searches: it's already set inside the outer isInitial block and then again in the _state.update within the launched coroutine; remove the redundant assignment by eliminating passthroughAttempted from the inner _state.update branch (the one inside viewModelScope.launch) or make that inner assignment conditional only for non-initial paths so that passthroughAttempted is only set once when isInitial is true; update references to passthroughAttempted in the _state.update call in SearchViewModel (within viewModelScope.launch) to avoid the duplicate reset while preserving the existing behavior for non-initial searches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@feature/search/presentation/src/commonMain/kotlin/zed/rainxch/search/presentation/SearchViewModel.kt`:
- Around line 687-758: performExplore launches an untracked coroutine
(viewModelScope.launch) so its in-flight Job can append stale results and
clobber exploreStatus after a new search; create and store a Job property (e.g.,
exploreJob: Job?) and assign it to the coroutine returned by
viewModelScope.launch in performExplore, cancel exploreJob wherever
currentSearchJob is cancelled (the performSearch(isInitial = true) reset path)
and also cancel it in OnClearClick/OnSearchChange handlers, and ensure the
explore coroutine bails on CancellationException before mutating state (rethrow
or return immediately) and only updates state/ calls appendExploreResults when
not cancelled. Use the existing symbols performExplore, exploreJob,
currentSearchJob, appendExploreResults, OnClearClick, and OnSearchChange to
locate and apply the changes.
---
Nitpick comments:
In
`@core/domain/src/commonMain/kotlin/zed/rainxch/core/domain/logging/GitHubStoreLogger.kt`:
- Around line 14-15: The default implementation of GitHubStoreLogger.withTag
returning this can silently drop tags for implementations that forget to
override it; change the interface so withTag is abstract (remove the default
implementation) to force implementors to provide tagging behavior, then update
the concrete implementation(s) such as KermitLogger (and any test doubles/no-op
loggers) to implement withTag appropriately (either applying the tag to the
underlying logger or explicitly documenting/returning a best-effort no-op) so
callers cannot assume tagging is applied when it is not.
In
`@feature/search/presentation/src/commonMain/kotlin/zed/rainxch/search/presentation/SearchViewModel.kt`:
- Around line 294-324: The passthroughAttempted field is redundantly reset to
null for initial searches: it's already set inside the outer isInitial block and
then again in the _state.update within the launched coroutine; remove the
redundant assignment by eliminating passthroughAttempted from the inner
_state.update branch (the one inside viewModelScope.launch) or make that inner
assignment conditional only for non-initial paths so that passthroughAttempted
is only set once when isInitial is true; update references to
passthroughAttempted in the _state.update call in SearchViewModel (within
viewModelScope.launch) to avoid the duplicate reset while preserving the
existing behavior for non-initial searches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4124e307-cbc0-4a06-bab8-ccb59b4811a1
📒 Files selected for processing (4)
CLAUDE.mdcore/data/src/commonMain/kotlin/zed/rainxch/core/data/logging/KermitLogger.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/logging/GitHubStoreLogger.ktfeature/search/presentation/src/commonMain/kotlin/zed/rainxch/search/presentation/SearchViewModel.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- CLAUDE.md
Summary by CodeRabbit
New Features
Improvements