Repository navigation
Conversation
The shared SecureStore preference store wrote fire-and-forget: set() moved the value the app shows and started a write in the background, so a burst of toggles queued one write per toggle and the newest choice sat behind the older ones. A process teardown right after a choice left the app showing a value the disk did not hold. The store now keeps one write in flight, writes only the newest requested target, and applies a value only when its write lands. A failed write toasts and leaves the value on the last stored one. clear() fences an in-flight write so a sign-out cannot repaint the reset value, and mergeOnLoad still reconciles a choice made before the disk read settles.
A preference now becomes visible when its write lands, so a read that derives the next value has to start from the newest requested one instead of the rendered one. Getters and list mutators (chat backends, remote MCP servers and their OAuth state, trusted hosts and images, the agent model map, the settings registry) read that value, and the two settings switches resolve it at press time through PreferenceRow's new resolver, so rapid taps each flip the state the person last asked for.
iscekic
added
drain
Collaborator
Author
|
bot: Android end-to-end proof on emulator Exercised on the branch head:
The uiautomator state read at each capture: on / on / off / off / off / on / on. The model-catalogue block below the switch in the voice-input screen is an unrelated local-stack error, cropped out of those two captures.
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Contributor
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryThe incremental change only renames the nullable-store test so its name matches the behavior it exercises; the failure-release path remains covered by the following case, and all prior findings are resolved at HEAD ( Files Reviewed (1 file)
Previous Review Summaries (2 snapshots, latest commit 2ef918f)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 2ef918f)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryThe four previously reported issues (stranded queued choice, nullable Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (5 files)
Fix these issues in Kilo Cloud Previous review (commit 8f1b17e)Status: 4 Issues Found | Recommendation: Address before merge Executive SummaryThe new durable SecureStore write queue can strand the newest queued choice and permanently hang Overview
Issue Details (click to expand)WARNING
SUGGESTION
Fix these issues in Kilo Cloud Files Reviewed (31 files)
Reviewed by deepseek-v4.1-flash-1017 · Input: 33.9K · Output: 1.7K · Cached: 174.1K Review guidance: REVIEW.md from base branch |
Review follow-ups on the preference store: - a failed write now drops the queued target as well as the in-flight one, so the queue releases its waiters instead of stranding a newer choice forever; - the newest-value lookup compares undefined explicitly, so a nullable store (the voice-input language "auto", no run-on destination) reports a requested null instead of the previous stored value; - revokeHost builds from the newest value like trustHost already does; - the row and the native switch share one flip, so both controls move the value the person last asked for. Each edge has a case in secure-store-preference.test.ts or preference-row.mounted.test.tsx.
Collaborator
Author
|
bot: Re-verified on the review-fix head
Same emulator, local dev stack, and owned fake account as the earlier captures; switch state read with
|
The nullable test asserts the requested null; the failure release is covered by the following case, so the name no longer claims both.
This branch has not been deployed
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.
Change
A persisted preference now reaches the disk before the app shows it, and a burst of choices no longer queues one write per choice.
createSecureStorePreference(apps/mobile/src/lib/hooks/secure-store-preference.ts) is the store behind every SecureStore preference, including the tool-summary translation switch and the voice-input gateway transcription switch. It now keeps:getRequested(), the newest requested value (queued or being written), for code that derives the next value from the current one;whenPersisted(), so a caller or a test can await durability instead of polling;clear()(sign-out) cannot be repainted by a write already on its way, andmergeOnLoadstill reconciles a choice made before the disk read settles.PreferenceRowacceptsresolveValue, and the tool-summary translation and voice-input gateway switches pass the store's newest requested value, so a burst of taps flips once per tap instead of repeating the value of the last render.Getters and list mutators that build a value from the current one (chat backends, remote MCP servers and their OAuth state, trusted hosts and images, the agent model map, the settings registry) read
getRequested(); React snapshots keep the durable value.Verification
pnpm --filter kilo-app test— 1194 files, 17900 tests pass.apps/mobile/src/lib/hooks/secure-store-preference.test.ts:pnpm --filter kilo-app typecheckandpnpm --filter kilo-app lintare clean.omp36, local dev stack, dev client, owned fake account):am force-stopand relaunch: the switch reads on;am force-stopand relaunch: the switch reads off;am force-stopand relaunch: the switch reads on.Proxy answers
The owner's proxy gave these answers in the owner's place.
Answer: Coalesce + durability-first
Confidence: 0.65
createSecureStorePreference, which also backs about 15 other preferences. Where should the fix land?Answer: Shared store
Confidence: 0.90
!valuefrom its last render, so taps that land before a re-render can repeat the same value. Should the row compute from the store's latest requested value instead?Answer: Compute from the store
Confidence: 0.85
Answer: Store exposes pending target; PreferenceRow uses it
Confidence: 0.85
Answer: Toast and drop the failed target
Confidence: 0.60
set()is used by about 15 modules and their tests. What signature should it keep?Answer:
void set()pluswhenPersisted()Confidence: 0.65