Document in-app feedback sheet design and implementation plan#467
Document in-app feedback sheet design and implementation plan#467rainxchzed merged 23 commits intomainfrom
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughAdds a complete in-app "Send feedback" feature: platform helpers for OS/locale, MVI (state/action/event/viewmodel), feedback models/enums, a FeedbackComposer utility, Compose UI bottom sheet and components, DI registration, i18n strings, build dependency, and documentation. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as FeedbackBottomSheet
participant FVM as FeedbackViewModel
participant Composer as FeedbackComposer
participant BH as BrowserHelper
participant Browser as External Browser/Email
User->>UI: Fill form & tap Send
UI->>FVM: FeedbackAction.OnSendViaEmail/Github
FVM->>Composer: composeUrl(state, channel)
Composer->>Composer: build body + truncate
Composer-->>FVM: return URL
FVM->>BH: openUrl(url, onFailure)
BH->>Browser: launch intent / mailto
Browser-->>BH: success / exception
BH-->>FVM: success / onFailure(message)
FVM->>FVM: emit FeedbackEvent.OnSent / OnSendError
FVM-->>UI: event observed -> call onSent/onError
UI->>User: show snackbar / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/feedback/components/TopicSelector.kt (1)
34-47: Add selection-group semantics for topic chips.Line 34 defines a single-choice topic group; adding
selectableGroup()improves accessibility semantics for assistive tech.Proposed diff
import androidx.compose.foundation.layout.fillMaxWidth import androidx.compose.foundation.layout.padding +import androidx.compose.foundation.selection.selectableGroup @@ FlowRow( modifier = Modifier .fillMaxWidth() + .selectableGroup() .padding(top = 8.dp), horizontalArrangement = Arrangement.spacedBy(8.dp), verticalArrangement = Arrangement.spacedBy(8.dp), ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/feedback/components/TopicSelector.kt` around lines 34 - 47, Wrap the FlowRow used to render the topic chips with selectableGroup() to expose single-selection semantics to assistive tech: locate the FlowRow in TopicSelector.kt that iterates FeedbackTopic.entries and add the selectableGroup() modifier on its Modifier chain (the one currently calling fillMaxWidth() and padding()). This ensures the FilterChip group (each FilterChip using selected and onSelected) is announced as a selectable single-choice group to accessibility services.feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/feedback/components/FeedbackBottomSheet.kt (1)
63-66: Deduplicate sheet-dismiss behavior.Both dismiss paths execute identical logic. Extract a local
dismissSheet()lambda to avoid drift between the system dismiss and icon-button dismiss flows.Also applies to: 89-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/feedback/components/FeedbackBottomSheet.kt` around lines 63 - 66, Duplicate dismiss logic exists in the sheet's onDismissRequest and the icon-button dismiss flow: both call viewModel.onAction(FeedbackAction.OnDismiss) then onDismiss(). Extract a local dismissSheet() lambda (or val) that calls viewModel.onAction(FeedbackAction.OnDismiss) and onDismiss(), then replace the body of onDismissRequest and the icon-button's onClick/handler with a call to dismissSheet() to ensure a single source of truth.docs/superpowers/specs/2026-04-29-tweaks-feedback-design.md (1)
39-59: Add language identifiers to fenced code blocks.These fences currently violate markdownlint MD040 and should be tagged (for example,
text,kotlin, ormermaidwhere appropriate).Also applies to: 63-70, 167-196, 226-246
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/specs/2026-04-29-tweaks-feedback-design.md` around lines 39 - 59, The markdown in the spec file contains fenced code blocks without language identifiers (violating MD040); update each fence shown (the file tree and the other blocks at lines noted) to include an appropriate language tag (e.g., use "text" for the directory tree, "kotlin" for filenames/snippets like FeedbackViewModel.kt, FeedbackComposer.kt, FeedbackState.kt, and "mermaid" where diagrams apply) so all fenced blocks are tagged accordingly and linting passes.feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/feedback/components/DiagnosticsPreview.kt (1)
78-88: Avoid duplicated diagnostics formatting logic.
formatDiagnosticsduplicates composer-side diagnostics rendering, so preview and actual payload can diverge over time. Consider moving this to a shared formatter used by bothDiagnosticsPreviewandFeedbackComposer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/feedback/components/DiagnosticsPreview.kt` around lines 78 - 88, formatDiagnostics in DiagnosticsPreview duplicates rendering logic used by FeedbackComposer; extract this logic into a single shared formatter (e.g., a top-level function or a DiagnosticsFormatter class) in a common/shared module and have both DiagnosticsPreview (the current formatDiagnostics) and FeedbackComposer call that shared formatter; specifically, move the StringBuilder construction and field concatenation (appVersion, platform, osVersion, locale, installerType, githubUsername conditional on FeedbackChannel.GITHUB) into the shared function (e.g., formatDiagnosticsShared or DiagnosticsFormatter.format), replace the current private formatDiagnostics in DiagnosticsPreview with a call to that shared function, and update FeedbackComposer to use the same shared API so there is a single source of truth for diagnostics formatting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/plans/2026-04-29-tweaks-feedback.md`:
- Around line 1590-1591: Standardize the spelling of "prefilled/pre-filled"
across the plan by choosing one form (e.g., "prefilled") and updating both
occurrences in the two list items (Step 5: "Type a title and description, tap
'Send Email' → your default mail client opens with a prefilled draft to
`hello@github-store.org`" and Step 6: "Reopen the sheet, tap 'Open as GitHub
Issue' → browser opens to a pre-filled `OpenHub-Store/GitHub-Store` new-issue
page with labels") so both use the same chosen spelling consistently.
In `@docs/superpowers/specs/2026-04-29-tweaks-feedback-design.md`:
- Around line 155-158: The FeedbackEvent sealed interface currently declares a
channel-agnostic success object (FeedbackEvent.OnSent) which conflicts with the
implementation's channel-specific success handling; update the spec to make the
success event carry the channel (e.g., replace data object OnSent with a data
class OnSent(val channel: FeedbackChannel) : FeedbackEvent or similar) and
ensure the spec still includes OnSendError(message: String) so implementers can
wire channel-aware success messaging consistently with the code paths that
dispatch per-channel success.
In
`@feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/feedback/FeedbackViewModel.kt`:
- Around line 37-40: In the init block where viewModelScope.launch updates
_state with diagnostics, guard the call to collectDiagnostics() using
runCatching so any exception doesn’t cancel initialization; on success set
diagnostics to the collected value, and on failure set diagnostics = null (i.e.,
_state.update { it.copy(diagnostics = result.getOrNull()) } or use
runCatching(...).getOrNull()), ensuring collectDiagnostics() failures are
swallowed and diagnostics stays null rather than propagating.
- Around line 73-95: The send() coroutine should be made exception-safe: wrap
the work after setting _state.update { isSending = true } in a try/finally so
_state.update { isSending = false } always runs, catch exceptions from
FeedbackComposer.composeUrl(...) and browserHelper.openUrl(...) and route them
to _events.send(FeedbackEvent.OnSendError(...)) and set a local failed flag so
the OnSent path is not taken; ensure you still call resetForm() only on success.
Also harden AndroidBrowserHelper so its startActivity/openUrl path invokes the
provided onFailure callback when startActivity throws or fails (and surface that
error back to the FeedbackViewModel via the same onFailure callback used by
browserHelper.openUrl).
In
`@feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/feedback/util/FeedbackComposer.kt`:
- Around line 67-70: The truncateToCap extension currently slices to
BODY_MAX_CHARS and then appends the suffix, exceeding the cap; change
String.truncateToCap to compute the suffix (e.g., "\n\n…[truncated]") and, when
length > BODY_MAX_CHARS, return either suffix.take(BODY_MAX_CHARS) if
BODY_MAX_CHARS <= suffix.length or substring(0, BODY_MAX_CHARS - suffix.length)
+ suffix otherwise, so the final result from truncateToCap never exceeds
BODY_MAX_CHARS.
---
Nitpick comments:
In `@docs/superpowers/specs/2026-04-29-tweaks-feedback-design.md`:
- Around line 39-59: The markdown in the spec file contains fenced code blocks
without language identifiers (violating MD040); update each fence shown (the
file tree and the other blocks at lines noted) to include an appropriate
language tag (e.g., use "text" for the directory tree, "kotlin" for
filenames/snippets like FeedbackViewModel.kt, FeedbackComposer.kt,
FeedbackState.kt, and "mermaid" where diagrams apply) so all fenced blocks are
tagged accordingly and linting passes.
In
`@feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/feedback/components/DiagnosticsPreview.kt`:
- Around line 78-88: formatDiagnostics in DiagnosticsPreview duplicates
rendering logic used by FeedbackComposer; extract this logic into a single
shared formatter (e.g., a top-level function or a DiagnosticsFormatter class) in
a common/shared module and have both DiagnosticsPreview (the current
formatDiagnostics) and FeedbackComposer call that shared formatter;
specifically, move the StringBuilder construction and field concatenation
(appVersion, platform, osVersion, locale, installerType, githubUsername
conditional on FeedbackChannel.GITHUB) into the shared function (e.g.,
formatDiagnosticsShared or DiagnosticsFormatter.format), replace the current
private formatDiagnostics in DiagnosticsPreview with a call to that shared
function, and update FeedbackComposer to use the same shared API so there is a
single source of truth for diagnostics formatting.
In
`@feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/feedback/components/FeedbackBottomSheet.kt`:
- Around line 63-66: Duplicate dismiss logic exists in the sheet's
onDismissRequest and the icon-button dismiss flow: both call
viewModel.onAction(FeedbackAction.OnDismiss) then onDismiss(). Extract a local
dismissSheet() lambda (or val) that calls
viewModel.onAction(FeedbackAction.OnDismiss) and onDismiss(), then replace the
body of onDismissRequest and the icon-button's onClick/handler with a call to
dismissSheet() to ensure a single source of truth.
In
`@feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/feedback/components/TopicSelector.kt`:
- Around line 34-47: Wrap the FlowRow used to render the topic chips with
selectableGroup() to expose single-selection semantics to assistive tech: locate
the FlowRow in TopicSelector.kt that iterates FeedbackTopic.entries and add the
selectableGroup() modifier on its Modifier chain (the one currently calling
fillMaxWidth() and padding()). This ensures the FilterChip group (each
FilterChip using selected and onSelected) is announced as a selectable
single-choice group to accessibility services.
🪄 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: a15514e7-044b-4f51-b7ad-78eb25a76e50
📒 Files selected for processing (28)
composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/di/ViewModelsModule.ktcore/domain/src/androidMain/kotlin/zed/rainxch/core/domain/Platform.android.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/Platform.ktcore/domain/src/jvmMain/kotlin/zed/rainxch/core/domain/Platform.jvm.ktcore/presentation/src/commonMain/composeResources/values/strings.xmldocs/superpowers/plans/2026-04-29-tweaks-feedback.mddocs/superpowers/specs/2026-04-29-tweaks-feedback-design.mdfeature/tweaks/presentation/build.gradle.ktsfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/TweaksAction.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/TweaksRoot.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/TweaksState.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/TweaksViewModel.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/components/sections/About.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/feedback/FeedbackAction.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/feedback/FeedbackEvent.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/feedback/FeedbackState.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/feedback/FeedbackViewModel.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/feedback/components/CategorySelector.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/feedback/components/ConditionalFields.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/feedback/components/DiagnosticsPreview.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/feedback/components/FeedbackBottomSheet.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/feedback/components/SendActions.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/feedback/components/TopicSelector.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/feedback/model/DiagnosticsInfo.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/feedback/model/FeedbackCategory.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/feedback/model/FeedbackChannel.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/feedback/model/FeedbackTopic.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/feedback/util/FeedbackComposer.kt
| 5. Type a title and description, tap "Send Email" → your default mail client opens with a prefilled draft to `hello@github-store.org`. | ||
| 6. Reopen the sheet, tap "Open as GitHub Issue" → browser opens to a pre-filled `OpenHub-Store/GitHub-Store` new-issue page with labels. |
There was a problem hiding this comment.
Use one spelling consistently for “prefilled/pre-filled”.
Line 1590 uses “prefilled” while Line 1591 uses “pre-filled”. Please standardize to one form in this plan.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~1590-~1590: Do not mix variants of the same word (‘prefilled’ and ‘pre-filled’) within a single text.
Context: ...→ your default mail client opens with a prefilled draft to hello@github-store.org. 6. R...
(EN_WORD_COHERENCY)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-04-29-tweaks-feedback.md` around lines 1590 -
1591, Standardize the spelling of "prefilled/pre-filled" across the plan by
choosing one form (e.g., "prefilled") and updating both occurrences in the two
list items (Step 5: "Type a title and description, tap 'Send Email' → your
default mail client opens with a prefilled draft to `hello@github-store.org`"
and Step 6: "Reopen the sheet, tap 'Open as GitHub Issue' → browser opens to a
pre-filled `OpenHub-Store/GitHub-Store` new-issue page with labels") so both use
the same chosen spelling consistently.
| init { | ||
| viewModelScope.launch { | ||
| _state.update { it.copy(diagnostics = collectDiagnostics()) } | ||
| } |
There was a problem hiding this comment.
Guard diagnostics initialization against repository/platform failures.
collectDiagnostics() is called in init without failure handling. Any exception here can crash or silently cancel init work. Wrap it with runCatching and keep diagnostics = null on failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/feedback/FeedbackViewModel.kt`
around lines 37 - 40, In the init block where viewModelScope.launch updates
_state with diagnostics, guard the call to collectDiagnostics() using
runCatching so any exception doesn’t cancel initialization; on success set
diagnostics to the collected value, and on failure set diagnostics = null (i.e.,
_state.update { it.copy(diagnostics = result.getOrNull()) } or use
runCatching(...).getOrNull()), ensuring collectDiagnostics() failures are
swallowed and diagnostics stays null rather than propagating.
| private fun send(channel: FeedbackChannel) { | ||
| val current = _state.value | ||
| if (!current.canSend) return | ||
| _state.update { it.copy(isSending = true) } | ||
| viewModelScope.launch { | ||
| var failed = false | ||
| val url = FeedbackComposer.composeUrl(current, channel) | ||
| browserHelper.openUrl(url) { error -> | ||
| failed = true | ||
| viewModelScope.launch { | ||
| _events.send(FeedbackEvent.OnSendError(error)) | ||
| } | ||
| } | ||
| // Hold the disabled state briefly so the user sees the | ||
| // buttons disable and can't double-tap; long enough to | ||
| // also let any synchronous onFailure invocation arrive. | ||
| delay(250) | ||
| _state.update { it.copy(isSending = false) } | ||
| if (!failed) { | ||
| _events.send(FeedbackEvent.OnSent(channel)) | ||
| resetForm() | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== BrowserHelper contract =="
fd -i "BrowserHelper.kt" .
rg -n -C4 --type=kt '\binterface\s+BrowserHelper\b|\bfun\s+openUrl\s*\('
echo
echo "== Implementations / overrides =="
rg -n -C4 --type=kt ':\s*BrowserHelper\b|override\s+fun\s+openUrl\s*\('
echo
echo "== Async dispatch around onFailure/openUrl =="
rg -n -C4 --type=kt 'openUrl\s*\(|onFailure|launch\s*\{|async\s*\{|withContext\s*\('Repository: OpenHub-Store/GitHub-Store
Length of output: 397
🏁 Script executed:
#!/bin/bash
echo "=== BrowserHelper Interface Definition ==="
cat ./core/domain/src/commonMain/kotlin/zed/rainxch/core/domain/utils/BrowserHelper.kt
echo
echo "=== AndroidBrowserHelper Implementation ==="
cat ./core/data/src/androidMain/kotlin/zed/rainxch/core/data/utils/AndroidBrowserHelper.kt
echo
echo "=== DesktopBrowserHelper Implementation ==="
cat ./core/data/src/jvmMain/kotlin/zed/rainxch/core/data/utils/DesktopBrowserHelper.ktRepository: OpenHub-Store/GitHub-Store
Length of output: 2081
🏁 Script executed:
#!/bin/bash
echo "=== FeedbackViewModel Full Content ==="
cat ./feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/feedback/FeedbackViewModel.ktRepository: OpenHub-Store/GitHub-Store
Length of output: 5816
send() method lacks exception safety and doesn't handle failures from AndroidBrowserHelper.
The send() method has two reliability issues:
-
Unhandled exceptions: If
FeedbackComposer.composeUrl()orbrowserHelper.openUrl()throw an exception, the coroutine crashes and leavesisSending = truepermanently. -
AndroidBrowserHelper never invokes
onFailure: It only callscontext.startActivity()without error handling. If the intent fails to start, the callback is never invoked, leaving the UI in a sending state indefinitely.
Wrap the send logic in try/finally and ensure isSending is reset even when exceptions occur:
💡 Suggested fix
private fun send(channel: FeedbackChannel) {
val current = _state.value
if (!current.canSend) return
_state.update { it.copy(isSending = true) }
viewModelScope.launch {
- var failed = false
- val url = FeedbackComposer.composeUrl(current, channel)
- browserHelper.openUrl(url) { error ->
- failed = true
- viewModelScope.launch {
- _events.send(FeedbackEvent.OnSendError(error))
- }
- }
- delay(250)
- _state.update { it.copy(isSending = false) }
- if (!failed) {
- _events.send(FeedbackEvent.OnSent(channel))
- resetForm()
- }
+ var failureMessage: String? = null
+ try {
+ val url = FeedbackComposer.composeUrl(current, channel)
+ browserHelper.openUrl(url) { error ->
+ failureMessage = error
+ }
+ delay(250)
+ val error = failureMessage
+ if (error != null) {
+ _events.send(FeedbackEvent.OnSendError(error))
+ } else {
+ _events.send(FeedbackEvent.OnSent(channel))
+ resetForm()
+ }
+ } catch (t: Throwable) {
+ _events.send(FeedbackEvent.OnSendError(t.message ?: "Failed to open feedback channel"))
+ } finally {
+ _state.update { it.copy(isSending = false) }
+ }
}
}Also harden AndroidBrowserHelper to invoke the onFailure callback when startActivity() fails or throws an exception.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/feedback/FeedbackViewModel.kt`
around lines 73 - 95, The send() coroutine should be made exception-safe: wrap
the work after setting _state.update { isSending = true } in a try/finally so
_state.update { isSending = false } always runs, catch exceptions from
FeedbackComposer.composeUrl(...) and browserHelper.openUrl(...) and route them
to _events.send(FeedbackEvent.OnSendError(...)) and set a local failed flag so
the OnSent path is not taken; ensure you still call resetForm() only on success.
Also harden AndroidBrowserHelper so its startActivity/openUrl path invokes the
provided onFailure callback when startActivity throws or fails (and surface that
error back to the FeedbackViewModel via the same onFailure callback used by
browserHelper.openUrl).
Summary by CodeRabbit
New Features
Bug Fixes
Documentation