Mobile UX consolidated pass#4503
Open
iscekic wants to merge 142 commits into
Open
Conversation
…y, money-role gating, session-detail recovery - code-reviewer manual review: show retryable QueryError on provider status-query failure instead of the connect empty state - security-agent setup: clear the tab bar so the connect screen centers - agents platform-filter sheet: mark backdrop + content pressables non-accessible so VoiceOver reaches individual controls - profile credits: gate the org zero-balance Add credits CTA on money-role - agent session detail: add Back to sessions to the personal-scope load error - login: use toast.success for the resend confirmation - code-reviewer platform error: use the server QueryError variant
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Contributor
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Fix these issues in Kilo Cloud Files Reviewed (1 file)
Previous Review Summaries (5 snapshots, latest commit 9f2cdad)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 9f2cdad)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Fix these issues in Kilo Cloud Files Reviewed (3 files)
Previous review (commit 3f0f446)Status: No Issues Found | Recommendation: Merge Files Reviewed (9 files)
Previous review (commit 504ca27)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Fix these issues in Kilo Cloud Files Reviewed (10 files)
Previous review (commit 58a0bf2)Status: 6 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Fix these issues in Kilo Cloud Files Reviewed (31 files)
Previous review (commit 63f995d)Status: 12 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Fix these issues in Kilo Cloud Files Reviewed (282 files)
Reviewed by gpt-5.6-sol-20260709 · Input: 80.4K · Output: 7.8K · Cached: 720K Review guidance: REVIEW.md from base branch |
…ting them Retry-send previously composed a brand-new message from the failed one: attachment blocks reused already-linked attachment IDs (server rejects with 'Attachment is already linked', so retries with attachments always failed), and the original row kept delivery_failed=1 server-side, resurfacing next to the successful retry on any refetch. New conversation-DO redeliverMessage op re-enqueues delivery of the existing row: clears delivery_failed, reuses the stored content and reply context, touches no attachments, and is an idempotent no-op when the message is not in a failed state. Human members receive a message.redelivered event (mirror of message.delivery_failed) so all clients clear the flag; the mobile retry action now calls it via useRedeliverMessage with an optimistic flag clear.
Sentry.init in @sentry/react-native 7.x binds a new client without closing the previous one, and replay sampling is only read at init — so revoking consent left an in-flight replay recording and accumulated clients on every transition. The SDK exposes no runtime replay start/stop (native bridge only has captureReplay/getCurrentReplayId), so consent transitions now await Sentry.close() before re-initing, serialized through a lifecycle promise so a fast accept-revoke cannot interleave. Also drops the redundant duplicate init on mount.
…law, security agent, login - chat composer: lock text input and attachment controls while a send is in flight so the success path cannot wipe a draft typed during the await - session mutations: key the pending guard by operation (and rename title) instead of bare session id, so a delete during a settling rename is no longer silently dropped - new session: only replace the repo/model selectors with an error state when there is no cached data; background refetch failures keep selectors - version pin: treat cached null (valid unpinned state) as data — only data === undefined is an initial-load failure - instance controls: block Start/Redeploy during recovering/restoring, which the backend rejects anyway - security-agent scope list: an organizations.list failure degrades only the org section; the Personal scope row stays reachable - remediation cancel: distinguish 'Cancellation requested' (attempt already running) from 'Remediation cancelled' (queued attempt removed) - kiloclaw onboarding: retry from the instance_stopped terminal state now fires the start mutation — previously it looped back to the same state - device auth: cancel aborts the in-flight device-code POST and a resolving request can no longer revive a cancelled flow; the 15s timeout owns its error state
… targets - failed session search shows a retryable QueryError (Clear search kept below) instead of a dead-end no-match state; pull-to-refresh retries the search query too while one is active - kiloclaw provisioning breathing pulse goes static under Reduce Motion, matching Skeleton/SpinningIcon - header icon buttons (session-list +/filter, dashboard rename, device pairing refresh) get hitSlop to reach 44pt effective targets
…t, session ops - sentry consent: the close+init chain promise never rejects (resolved via a gate in finally), so a failed transition can't poison later ones; the caller un-marks the applied consent state on failure so the next change re-attempts teardown. Effect extracted to useSentryConsentSync. - session ops: per-session promise chain serializes rename/delete so optimistic snapshots can't interleave and an older request can't overwrite a newer result; identical duplicate taps still dedupe - redeliver: DO requires exactly one eligible bot (zero bots no longer clears the flag and fakes success; multiple would double-deliver) and publishes message.redelivered BEFORE enqueueing the delivery attempt, so a fresh delivery failure can't be clobbered by a delayed clear event; the hooks-side retry now always invalidates after a rollback because a lost HTTP response can hide a committed redelivery - synced plugin copy of the kilo-chat event contract regenerated
Backdrop and content pressables default to accessible=true, collapsing the
whole sheet into one VoiceOver node with the Switch unreachable — same bug
class as the platform-filter sheet, same accessible={false} fix.
- 'Copy Command' / 'Destroy Instance' → sentence case per the copy convention - device-pairing header refresh icon gets a role + label for screen readers
Even a disabled organizations.list observer surfaces the shared cache entry's error state, so an org-list failure rendered the personal scope entry as a full-screen error although every personal securityAgent call succeeds. The role query now masks query state entirely for the personal scope, where the org list is irrelevant.
…ped to the POST
- SheetHeader was invisible on formSheet pickers: react-native-screens lays
out a formSheet's scroll view natively and only honors a header when
[header, scroll view] are the screen content's direct children — the
wrapping View made it pin the list over the header. PickerSheet drops the
wrapper (fragment) and marks the header collapsable={false} so view
flattening can't recreate the bug; mode-picker migrates to PickerSheet.
Verified on-device: mode + model pickers now show title + Done.
- device-auth: the 15s start timeout now clears as soon as the POST
resolves. start() stays suspended on the auth browser await for as long
as the sheet is open, so the timer previously fired mid-sign-in, stomped
the live pending/idle state with a bogus error, and left a zombie poll
running behind it.
The bots.length !== 1 guard already narrows bots[0] to ActiveMemberRow; the root oxlint config forbids the non-null assertion and flags it as unnecessary.
…event-driven clear - session mutations: dedup only an ADJACENT identical op (repeat of the chain tail) instead of any op in a global set, so rename A -> B -> A runs the final A and the user's latest intent wins; double-taps still coalesce - kilo-chat redeliver (DO): reserve the webhook-chain slot synchronously with no external await first, so a concurrent request can't slot a later message ahead of the redelivery or run delivery out of order; the redelivered event is pushed inside the serialized task, strictly before deliverToBot - kilo-chat redeliver (client): drop the optimistic clear + guarded restore + invalidate; the server's message.redelivered event is the single source of truth for the flag, which removes both the rollback ambiguity on an unclear HTTP error and the refetch race that could overwrite a newer delivery failure with a stale pre-failure snapshot
…via event pushEventToHumanMembers is best-effort, so a requesting client whose socket dropped would miss message.redelivered and keep showing Retry indefinitely even though the server cleared the flag and retried. Apply the clear on HTTP success as a targeted single-message cache write (not optimistic, not a refetch); a strictly-later message.delivery_failed event still restores failure if the retry ultimately fails.
| old | ||
| ? updateMessageInPages(old, variables.messageId, msg => ({ | ||
| ...msg, | ||
| deliveryFailed: false, |
Contributor
There was a problem hiding this comment.
WARNING: The HTTP success write can erase a newer failure event
The HTTP response and websocket events are independent delivery channels. Even though the server orders message.redelivered before the retry attempt, a fast message.delivery_failed event can reach this handler before the mutation response; this unconditional onSuccess write then changes the cache back to deliveryFailed: false. The server row remains failed, but the UI hides Retry until another refetch. Guard the success write against a failure event observed after this mutation started, or carry an attempt/version through both paths.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Contributor
Author
There was a problem hiding this comment.
Valid in theory, but this specific ordering is practically unreachable given the current DO structure: redeliverMessage returns its HTTP response immediately after synchronously reserving the webhook slot — before deliverToBot runs. A message.delivery_failed event can only be produced by that delivery attempt exhausting its retries (2 retries with 500ms/1s backoff), so it is always generated seconds after the HTTP ack has already been sent. For it to clobber this onSuccess write, the WS event would have to overtake an already-in-flight HTTP response by several seconds.
The residual is also self-healing (a later refetch/reconnect reconciles from the server row) and non-data-loss — worst case the Retry affordance is briefly hidden.
Fully closing the HTTP-ack-vs-WS-event tie does require carrying a monotonic attempt/version through both channels (message row + both event payloads + client merge), which is a schema-migration-scale change I judged disproportionate to a practically-unreachable, self-healing UI race inside this UX PR. Leaving this thread open rather than resolving it so a maintainer can decide whether to prioritize the attempt-token work as a follow-up.
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.
Implements the consolidated mobile UX pass: every finding from the mobile UX audit (dead ends, eternal skeletons, invisible errors, missing CTAs, tab-bar/inset defects, disabled-state lies, silent mutations, a11y gaps), built as shared primitives first, then per-domain migrations, then cross-app sweeps.
Correctness & privacy (Phase 0)
parseTimestampeverywhere (Hermes-safe); code-reviewer domain failures ({success:false}) now route toonErrorShared primitives (Phase 1)
TabScreenScrollView/useTabBarBottomPadding(single source for tab-bar clearance, ~40 screens migrated),useDetailScreenBottomPadding,QueryErrorvariants + placement + retry spinner,EmptyStateplacement,Button44pt targets +loading,ConfigureRowdisabled/chevron truth,SheetHeader/PickerSheet, sharedRenameModal,openExternalUrl(P7),FormField(uncontrolled, a11y error announcement),ChoiceRow(radio/checkbox semantics, single haptic owner),SpinningIcon+ reduced-motion support (incl. skeleton shimmer), toaster ordering + Kilo-token styling, form-sheet detent helpers.Domain passes (Phases 2–6)
canCreategating, session-detail recovery + message-shaped skeletons, picker fallback headers + synchronous bridge init, kilo-chat cached-data-wins routing, token-failure retry, inline rename errors, bot-offline CTA, message retry-send (resolves the failed original), favorites with latest-write-wins optimistic updates, behaviorally-modal reaction picker, single creation affordance on empty surfacesisLoading || !datatraps), org boundary discriminates transient failure from stale org (retry vs re-select), sheets own their errors inline (toasts render behind iOS form sheets), field validation viaFormField, role guards fail closed, role-aware empty-state CTAs, notifications-card failure feedback, forced-update store fallbacks, Kilo Pass inline feedback + StoreKit timeout, consent card caught mutations + pending states, login field labelingOptionListsaves await outcomes with serialized config writes, review list/detail states (UNAUTHORIZED/FORBIDDEN/NOT_FOUND classified as permanent, no un-retriable retry)Cross-app sweeps (Phase 7)
Empty/error placement per convention (centered full-screen with tab clearance; top-aligned inline), disabled-vs-loading visual contract, skeleton geometry matched to loaded content, press feedback on inert-feeling controls, KVRow overflow, status colors migrated to Focus tokens (new
infofamily added in the same style), sentence-case copy pass,formatDateconsolidation.Product notes
@kilocode/app-sharedhelpers with Intl grouping:$1,234.50(was$1234.50at a few residual sites). Signed off.toLocaleDateStringsites consolidated onto sharedformatDate, which follows the device locale (numeric date style). Four sites moved from long month names to the numeric form.--muted-foreground#7a756b#6f6a61--destructive#c25647#be4e3f--good#2f9a5f#278150--warn#b27214#9f6612--inputborder (light)--inputborder (dark)Foreground-on-fill pairs (
--good/--warn/--destructivebadges) all clear 4.5:1 as well; dark text tokens already passed and were left unchanged. Tinted tile tokens updated to match. RootDESIGN.mdnow documents that mobile intentionally uses the Focus palette.Verification
pnpm format && pnpm typecheck && pnpm lint && pnpm check:unusedclean; 530 mobile + 192 app-shared + 91 kilo-chat-hooks tests green||→??autofix regressions, no backend-stringnew Date(), error-before-skeleton branch ordering everywhere, single feedback owner per mutation (no double toast+inline, no silent mutations)kilo-chat-hookschanged (injected package) — re-inject and clear the Metro cache before device testing.Known follow-ups (deliberately left)
flyRegioninstances (crossfaded, not popped)Linking.openURL(system handoff arguably better UX than the in-app browser helper)useConnectFlowdedupe remain deferred per the planUpdate — review round + second e2e pass (2026-07-12)
Review comments (all 18 kilobot findings valid, fixed, threads resolved)
redeliverMessageop on the conversation DO — re-enqueues delivery of the existing row (no new message, no attachment re-linking), clearsdelivery_failed, publishesmessage.redeliveredbefore enqueueing the attempt (so a fresh failure can't be clobbered by a delayed clear), requires exactly one eligible bot (no fake success with zero bots, no double-delivery with many). Fixes the "attachment already linked" and duplicate-row findings.Sentry.close()before re-init, serialized through a chain promise that never rejects (a failed transition no longer poisons later ones); the applied-consent ref is un-marked on failure so the next change retries teardown.null(unpinned) as valid; instance controls block Start/Redeploy duringrecovering/restoring; security-agent scope list keeps Personal reachable on an org-list outage (and the personal scope entry no longer inherits the shared org-list error); remediation cancel distinguishes "Cancellation requested" from "Remediation cancelled"; onboardinginstance_stoppedretry actually starts the instance; device-auth cancel aborts the in-flight POST.Fixes from the second on-device pass (iOS)
react-native-screensonly honors a formSheet header when[header, scrollview]are the screen content's direct children; removed the wrapper View and marked the headercollapsable={false}. Verified on-device.start()including the auth-browser await, firing mid-sign-in and stomping state with a bogus error + zombie poll; now cleared as soon as the POST resolves. Verified on-device.accessible={false}fix as the filter sheet); session search retry; provisioning pulse gated on Reduce Motion; header touch targets to 44pt via hitSlop; kiloclaw copy sentence-case + pairing-refresh a11y label.Verification
format/typecheck/lint/check:unused) across apps/mobile; 530 mobile + 91 kilo-chat-hooks + 107 kilo-chat pkg + 415 kilo-chat service tests pass.