Mark/kimi_agent_kin_app_sync

Fork 0

[Review] Add comment to vaultSettings type warning future contributors away from useShallow (which reintroduces #162) #188

New issue

Closed

opened 2026-09-14 14:07:30 +00:00 by Mark · 1 comment

Mark commented

2026-09-14 14:07:30 +00:00

Owner

Finding

#162 was fixed by replacing useShallow(vaultSettings) with two primitive selectors (vaultEnabled, vaultAutoLockDelay) in src/pages/Settings.tsx. Root cause: useShallow returning a new object each time defeats Object.is re-render semantics. The fix is correct.

The hazard is that the pattern reads as "subscribe to all of vault state at once" — anyone adding a new field to vaultSettings in the future will reach for useShallow again because it looks natural, and re-introduce the same bug.

Suggested fix

Add a code comment at the vaultSettings type declaration in src/lib/store.ts (around line 253):

vaultLocked: boolean;
vaultEnabled: boolean;
// NOTE: Do NOT subscribe to multiple vaultSettings fields via useShallow.
//       #162 was caused by useShallow returning a new object each time,
//       which defeats Object.is re-render semantics. Subscribe to
//       individual primitives (e.g. useStore((s) => s.vaultEnabled))
//       and combine them via React's normal hook composition.

Severity

Documentation. Prevents the bug from being re-introduced.

Discovered by

Manual code review pass over #162, 2026-09-14.

## Finding `#162` was fixed by replacing `useShallow(vaultSettings)` with two primitive selectors (`vaultEnabled`, `vaultAutoLockDelay`) in `src/pages/Settings.tsx`. Root cause: `useShallow` returning a new object each time defeats `Object.is` re-render semantics. The fix is correct. The hazard is that the pattern reads as "subscribe to all of vault state at once" — anyone adding a new field to `vaultSettings` in the future will reach for `useShallow` again because it looks natural, and re-introduce the same bug. ## Suggested fix Add a code comment at the `vaultSettings` type declaration in `src/lib/store.ts` (around line 253): ```ts vaultLocked: boolean; vaultEnabled: boolean; // NOTE: Do NOT subscribe to multiple vaultSettings fields via useShallow. // #162 was caused by useShallow returning a new object each time, // which defeats Object.is re-render semantics. Subscribe to // individual primitives (e.g. useStore((s) => s.vaultEnabled)) // and combine them via React's normal hook composition. ``` ## Severity Documentation. Prevents the bug from being re-introduced. ## Discovered by Manual code review pass over #162, 2026-09-14.

Mark

2026-09-14 14:07:30 +00:00

closed this issue
added the
documentation
label

Mark referenced this issue from a commit

2026-09-14 14:25:50 +00:00

docs(store): warn against useShallow on vaultSettings (closes #188)

Mark referenced this issue from a commit

2026-09-14 14:25:50 +00:00

Apply changes from kilo-gsd.py run for Mark/kimi_agent_kin_app_sync#188

Mark referenced this issue

2026-09-14 14:32:01 +00:00

[Review] detectConflictPrecursors / detectExcitementCycles length filters (3/4) interact badly with vault-locked scrubbed interactions #180

Mark commented

2026-09-14 14:36:59 +00:00

Author

Owner

Closing as already-fixed.

Commit bf676a6 (docs(store): warn against useShallow on vaultSettings (closes #188)) landed on main at 2026-09-14 (today, before this triage session reviewed the file).

The chat-agent code review read against a stale HEAD and reported this issue as open. The fix was already in place when the review started — the agent failed to re-check HEAD between reads.

No work to do.

Closing as already-fixed. Commit `bf676a6` (`docs(store): warn against useShallow on vaultSettings (closes #188)`) landed on `main` at 2026-09-14 (today, before this triage session reviewed the file). The chat-agent code review read against a stale HEAD and reported this issue as open. The fix was already in place when the review started — the agent failed to re-check HEAD between reads. No work to do.

Mark referenced this issue

2026-09-14 14:37:39 +00:00

[Review] detectExcitementCycles bag-of-words picks up common English words ('time', 'meeting', 'just') as 'excitement keywords' #185

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Mark/kimi_agent_kin_app_sync#188

No description provided.

Rows
Columns