refactor(web): address current-platform-assistant store PR feedback (LUM-1718)#31435
Conversation
- Stop deleting other-org localStorage keys on persist write. The in-memory `byOrg` snapshot is only as fresh as the last rehydrate, so a stale tab could clobber selections written by other tabs for unrelated orgs. The storage adapter now writes per-org keys additively, and `setAssistantId(orgId, null)` removes only its own key directly. - Call `setAssistantId` via `useCurrentPlatformAssistantStore.getState()` inside `useEffect` / `useCallback` per `STATE_MANAGEMENT.md` — drops the unnecessary subscription and the dep-array entry. - Drop the unused `getAssistantId` action; the hook reads `byOrg` directly and no other caller exists.
![vex-assistant-bot[bot]](https://avatars.githubusercontent.com/in/3399303?s=60&v=4){kind=small}
There was a problem hiding this comment.
✦ APPROVE
All three follow-up items are correctly resolved.
Fix 1 — Multi-tab additive writes (Codex P1) ✅
The root cause was real: writeByOrgToLocalStorage(byOrg) deleted every prefixed key absent from the in-memory snapshot, but that snapshot is only as fresh as the last rehydrate(). A stale tab could clobber live writes from another tab for unrelated orgs.
New setItem writes additively — only when the stored value differs from the incoming value — and never deletes:
if (window.localStorage.getItem(key) !== id) {
window.localStorage.setItem(key, id);
}The if-differs guard also suppresses spurious storage events, which is a nice side-effect.
Deletions for setAssistantId(orgId, null) are issued imperatively in the action via removeStoredAssistantId(orgId) — which fires a storage event in other tabs → they call persist.rehydrate() → readByOrgFromLocalStorage() no longer sees the deleted key → cross-tab propagation is correct. The order in the action (localStorage remove before set()) is right.
removeItem no-op is fine — persist.clearStorage() isn't exposed by this store, so it's never called.
Fix 2 — .getState() for actions in effects/callbacks (Devin) ✅
Per STATE_MANAGEMENT.md, calling store actions via useStore.getState().action() from effects and callbacks is correct — getState() is called at invocation time (not render), so it always resolves the current action. Removing setAssistantIdAction from the subscription and from the dep array is right: subscribing to an action via .use.setAssistantId() was unnecessary overhead (actions are stable refs), and the dep array entry was vestigial.
Fix 3 — getAssistantId dead code (Devin) ✅
No callers inside or outside the hook. The get parameter is correctly dropped from the store factory since nothing calls it anymore.
storageKeyForOrg helper ✅
Good extraction — used in both setItem (write path) and removeStoredAssistantId (imperative delete path), eliminating the inline template literal duplication.
CI — 7/7 green. ✅
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
Codex Review: Didn't find any major issues. 👍 ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
…orm-assistant-hook-DRjFS-followup
ashleeradka
deleted the
claude/refactor-platform-assistant-hook-DRjFS-followup
branch
2 participants
Follow-up to #31433. Addresses all three review comments:
Changes
1. Fix multi-tab clobbering of other-org keys (codex P1)
The previous
writeByOrgToLocalStoragetreated the in-memorybyOrgsnapshot as authoritative and removed every prefixed localStorage key not present in it. The snapshot is only as fresh as the lastrehydrate(), so a stale tab could delete selections that another tab had just written for unrelated orgs.The storage adapter now writes per-org keys additively — only when the in-memory value differs from what's already in localStorage — and never removes keys absent from the snapshot. Deletions (
setAssistantId(orgId, null)) are issued imperatively by the action against the specific affected key, so they don't get conflated with the additive write path.2. Call store action via
.getState()inside effects/callbacks (devin)Per
docs/STATE_MANAGEMENT.md, actions are stable references and should be called viauseStore.getState().action()from event handlers, callbacks, and effects — not subscribed via.use.action(). The hook now does that, dropping the unnecessary subscription and the dep-array entry.3. Drop unused
getAssistantIdaction (devin)getAssistantIdwas unused — the hook readsbyOrgdirectly and there are no other callers. Removed per the repo's dead-code rule.Linear: https://linear.app/vellum/issue/LUM-1718
Test plan
bunx tsc --noEmitpassesbun run lintpasses (no new warnings)setAssistantId(orgId, null)removes only that org's localStorage keyGenerated by Claude Code