From ddec1d4ca6bb450598d853e2ac6b0bfcec3dc0e8 Mon Sep 17 00:00:00 2001 From: jianwei1 Date: Tue, 31 Mar 2026 22:30:42 +0000 Subject: [PATCH 1/2] fix: preserve custom chat widget URL and icon on deselect (NES-1522) Use stable React keys on custom ChatOption slots to prevent unmount/remount when the backing button is deleted. Add render-time state reset to sync local state when a different button shifts into the same slot. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../JourneyAppearance/Chat/Chat.tsx | 4 +- .../Chat/ChatOption/ChatOption.spec.tsx | 87 ++++++++ .../Chat/ChatOption/ChatOption.tsx | 10 + ...widget-clears-url-icon-on-deselect-plan.md | 194 ++++++++++++++++++ 4 files changed, 293 insertions(+), 2 deletions(-) create mode 100644 docs/plans/2026-03-31-001-fix-custom-chat-widget-clears-url-icon-on-deselect-plan.md diff --git a/apps/journeys-admin/src/components/Editor/Slider/Settings/CanvasDetails/JourneyAppearance/Chat/Chat.tsx b/apps/journeys-admin/src/components/Editor/Slider/Settings/CanvasDetails/JourneyAppearance/Chat/Chat.tsx index 54b97fe4710..d3fa23394c8 100644 --- a/apps/journeys-admin/src/components/Editor/Slider/Settings/CanvasDetails/JourneyAppearance/Chat/Chat.tsx +++ b/apps/journeys-admin/src/components/Editor/Slider/Settings/CanvasDetails/JourneyAppearance/Chat/Chat.tsx @@ -116,7 +116,7 @@ export function Chat(): ReactElement { disableSelection={maxSelection} /> {customButtons.length === 2 && ( { fireEvent.click(screen.getByText('Snapchat')) expect(screen.getByRole('combobox')).toHaveTextContent('Snapchat') }) + + it('should sync state when chatButton changes to a different button', () => { + const initialProps = { + title: 'Custom', + chatButton: { + id: 'button-1', + link: 'https://first.com', + platform: MessagePlatform.tikTok + } as unknown as ChatButton, + active: true, + journeyId: 'journeyId', + disableSelection: false, + enableIconSelect: true + } + + const { rerender } = render( + + + + + + ) + + expect(screen.getByRole('textbox')).toHaveValue('https://first.com') + expect(screen.getByRole('combobox')).toHaveTextContent('TikTok') + + rerender( + + + + + + ) + + expect(screen.getByRole('textbox')).toHaveValue('https://second.com') + expect(screen.getByRole('combobox')).toHaveTextContent('Snapchat') + }) + + it('should preserve state when chatButton becomes undefined', () => { + const initialProps = { + title: 'Custom', + chatButton: { + id: 'button-1', + link: 'https://preserved.com', + platform: MessagePlatform.tikTok + } as unknown as ChatButton, + active: true, + journeyId: 'journeyId', + disableSelection: false, + enableIconSelect: true + } + + const { rerender } = render( + + + + + + ) + + expect(screen.getByRole('textbox')).toHaveValue('https://preserved.com') + expect(screen.getByRole('combobox')).toHaveTextContent('TikTok') + + rerender( + + + + + + ) + + expect(screen.getByRole('textbox')).toHaveValue('https://preserved.com') + expect(screen.getByRole('combobox')).toHaveTextContent('TikTok') + }) }) diff --git a/apps/journeys-admin/src/components/Editor/Slider/Settings/CanvasDetails/JourneyAppearance/Chat/ChatOption/ChatOption.tsx b/apps/journeys-admin/src/components/Editor/Slider/Settings/CanvasDetails/JourneyAppearance/Chat/ChatOption/ChatOption.tsx index d04c3aff2e5..047b8677faf 100644 --- a/apps/journeys-admin/src/components/Editor/Slider/Settings/CanvasDetails/JourneyAppearance/Chat/ChatOption/ChatOption.tsx +++ b/apps/journeys-admin/src/components/Editor/Slider/Settings/CanvasDetails/JourneyAppearance/Chat/ChatOption/ChatOption.tsx @@ -26,11 +26,21 @@ export function ChatOption({ disableSelection, enableIconSelect = false }: ChatOptionProps): ReactElement { + const [trackedId, setTrackedId] = useState(chatButton?.id) const [currentLink, setCurrentLink] = useState(chatButton?.link ?? '') const [currentPlatform, setCurrentPlatform] = useState( platform ?? chatButton?.platform ?? MessagePlatform.custom ) + // Sync local state when a different button shifts into this slot. + // When chatButton is undefined (deselected), the guard preserves + // local state so Summary can reuse it to re-create the button. + if (chatButton != null && chatButton.id !== trackedId) { + setTrackedId(chatButton.id) + setCurrentLink(chatButton.link ?? '') + setCurrentPlatform(chatButton.platform ?? MessagePlatform.custom) + } + return ( <> "Specifying a key tells React to use the key itself as part of the component's position. Every time a component with a specific key appears on the screen, its state is created fresh. Every time it is removed, its state is destroyed." + +**Git history**: The dynamic key pattern was introduced in PR #8865 (NES-1452, 2026-03-20) by jianwei1. A reviewer explicitly flagged the need for keys on the second custom slot. The key pattern exists specifically because `ChatOption` uses `useState` initializers that only run on mount. + +## Acceptance Criteria + +### Change 1: Stable keys in Chat.tsx + +- [ ] `ChatOption` for custom slot 0 uses `key="custom-0"` (static) +- [ ] `ChatOption` for custom slot 1 uses `key="custom-1"` (static) + +### Change 2: Render-time state reset in ChatOption.tsx + +- [ ] Add render-time state reset that syncs `currentLink` and `currentPlatform` from `chatButton` prop +- [ ] Reset only fires when `chatButton?.id` changes (tracked via `trackedId` state) +- [ ] When `chatButton` is `undefined` (deselected), the guard prevents state reset — preserving the user's data +- [ ] When `chatButton` changes to a different button (shift case), state syncs to the new button's data immediately (no stale-state frame) +- [ ] No additional imports needed (`useEffect` not used) + +### Change 3: Tests + +- [ ] **ChatOption.spec.tsx** — add test: when `chatButton` prop changes to a different button (rerender), local state syncs to new button's link and platform +- [ ] **ChatOption.spec.tsx** — add test: when `chatButton` prop becomes `undefined` (rerender), local state is preserved (not reset) +- [ ] **Chat.spec.tsx** — verify existing tests still pass with stable keys (no functional change expected) + +## Implementation + +### ChatOption.tsx (Recommended: Render-Time State Reset) + +```tsx +// apps/journeys-admin/src/components/Editor/Slider/Settings/CanvasDetails/JourneyAppearance/Chat/ChatOption/ChatOption.tsx + +import { ReactElement, useState } from 'react' +// ... existing imports + +export function ChatOption({ ... }: ChatOptionProps): ReactElement { + const [trackedId, setTrackedId] = useState(chatButton?.id) + const [currentLink, setCurrentLink] = useState(chatButton?.link ?? '') + const [currentPlatform, setCurrentPlatform] = useState( + platform ?? chatButton?.platform ?? MessagePlatform.custom + ) + + // Sync local state when a different button shifts into this slot. + // Runs during render (before commit) — no stale-state frame. + // When chatButton is undefined (deselected), the guard preserves + // local state so Summary.tsx can reuse it to re-create the button. + if (chatButton != null && chatButton.id !== trackedId) { + setTrackedId(chatButton.id) + setCurrentLink(chatButton.link ?? '') + setCurrentPlatform(chatButton.platform ?? MessagePlatform.custom) + } + + return ( + // ... unchanged + ) +} +``` + +**Why render-time reset instead of useEffect:** + +The React docs explicitly flag `useState` + `useEffect` prop sync as an anti-pattern (react.dev/learn/you-might-not-need-an-effect). The `useEffect` approach causes a double-render: one with stale state (before the effect fires), then another with correct state. The render-time pattern eliminates this — React processes the `setState` calls synchronously before committing, so children never see stale data. + +Additionally, the `TextFieldForm` component uses `key={field-${id}-${initialValue}}`, which means any change to `initialValue` triggers a full Formik remount. With `useEffect`, this remount would happen twice per identity change (once with stale value, once with correct value). The render-time pattern avoids this. + +### Chat.tsx + +```tsx +// Line 118-119: Change dynamic key to stable key + + +// Line 128-129: Change dynamic key to stable key + +``` + +### Alternative: useEffect approach (from original ticket) + +If the team prefers the more familiar `useEffect` pattern (matching Label.tsx/Placeholder.tsx), this is the lint-safe version: + +```tsx +const chatButtonId = chatButton?.id + +useEffect(() => { + if (chatButton != null) { + setCurrentLink(chatButton.link ?? '') + setCurrentPlatform(chatButton.platform ?? MessagePlatform.custom) + } + // eslint-disable-next-line react-hooks/exhaustive-deps -- intentionally sync only on identity change +}, [chatButtonId]) +``` + +**Trade-off**: Simpler and more recognizable, but causes one stale-state render frame per identity change. Acceptable for a settings panel (not a performance-critical path). + +## Verification Scenarios + +| # | Scenario | Expected | +|---|----------|----------| +| A | 1 custom: deselect then re-select | URL and icon preserved | +| B | 2 customs: deselect first | Second button's data shifts into slot 1 correctly | +| C | 2 customs: deselect second | First button unchanged | +| D | Deselect then re-select then refresh page | Button re-created on server with preserved URL (verify via network or refresh) | +| E | Toggle dedicated platforms off and on | Regression check — unchanged behavior | + +### Research Insights on Verification + +**No Apollo-to-state race condition**: The mutation response, cache update, re-render, and state sync all happen within a single browser frame. There is no gap where the user could interact with stale state. + +**Rapid toggle safety**: `Summary.tsx` has `createLoading || removeLoading` guards that prevent double-mutation from rapid clicking. User intent may be swallowed (they click uncheck during create loading, it blocks), but data is never corrupted. + +**TextFieldForm interaction**: The Formik form uses `key={field-${id}-${initialValue}}` and `enableReinitialize`. When `currentLink` changes via the state reset, the Formik instance reinitializes correctly. No additional work needed. + +## Context + +- **Codebase pattern**: `useState` + `useEffect` prop-to-state sync is established in `Label.tsx:81-83` and `Placeholder.tsx:61-63` in the same editor area — but those sync on VALUE changes, not identity changes. The render-time reset pattern is closer to the React-recommended "adjusting state during render" approach. +- **Summary.tsx already passes `currentLink` and `currentPlatform`** to the create mutation (lines 72-73), so re-created buttons get the preserved values from local state +- **TextFieldForm uses `key={field-${id}-${initialValue}}`** internally, so it reinitializes correctly when `currentLink` changes via the state reset — but this also means any state change causes a full Formik remount (a known amplification vector) +- **React 18+ automatic batching**: Multiple `setState` calls in the render-time reset (or `useEffect`) are batched into a single re-render +- **No infinite re-render risk**: The `trackedId` tracking variable ensures the reset condition is false after the synchronous re-render, preventing loops + +## Edge Cases Discovered During Research + +| Edge Case | Impact | Handling | +|---|---|---| +| Server normalizes link (e.g. lowercases hostname) | Low — currently server returns link as-is | State reset would write back the normalized value. Add a comment documenting this assumption. | +| `chatButton` transitions from undefined to `{id: undefined}` | Negligible — IDs are always UUIDs in practice | `chatButton.id !== trackedId` → `undefined !== undefined` → false → no sync. Technically correct (no button identity change). | +| User types in TextFieldForm but hasn't submitted | Unsubmitted input exists only in Formik's internal state | Deselecting destroys the Formik instance. This is consistent with dedicated platforms — user must commit (blur/enter) before toggling. | +| External update to chatButton fields (e.g. Google Sync) | State not synced if ID unchanged | By design — syncing on value changes would overwrite local edits. The ID-based approach prioritizes local state preservation. | + +## Sources + +- Linear ticket: [NES-1522](https://linear.app/jesus-film-project/issue/NES-1522) +- Related (done): [NES-1452](https://linear.app/jesus-film-project/issue/NES-1452) — introduced the dynamic key pattern in PR #8865 +- Existing sync pattern: `apps/journeys-admin/src/components/Editor/Slider/Settings/CanvasDetails/Properties/blocks/TextResponse/TextResponseFields/Label/Label.tsx:81-83` +- React docs: react.dev/learn/you-might-not-need-an-effect (adjusting state during render) +- React docs: react.dev/learn/preserving-and-resetting-state (key-based reset) From f99ca7b614dce1dea42aa30b648db0bbc23a1e3c Mon Sep 17 00:00:00 2001 From: "autofix-ci[bot]" <114827586+autofix-ci[bot]@users.noreply.github.com> Date: Tue, 31 Mar 2026 22:36:28 +0000 Subject: [PATCH 2/2] fix: lint issues --- .../Chat/ChatOption/ChatOption.spec.tsx | 6 +--- ...widget-clears-url-icon-on-deselect-plan.md | 32 +++++++++++-------- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/apps/journeys-admin/src/components/Editor/Slider/Settings/CanvasDetails/JourneyAppearance/Chat/ChatOption/ChatOption.spec.tsx b/apps/journeys-admin/src/components/Editor/Slider/Settings/CanvasDetails/JourneyAppearance/Chat/ChatOption/ChatOption.spec.tsx index c0a252988e0..4e085a18466 100644 --- a/apps/journeys-admin/src/components/Editor/Slider/Settings/CanvasDetails/JourneyAppearance/Chat/ChatOption/ChatOption.spec.tsx +++ b/apps/journeys-admin/src/components/Editor/Slider/Settings/CanvasDetails/JourneyAppearance/Chat/ChatOption/ChatOption.spec.tsx @@ -168,11 +168,7 @@ describe('ChatOption', () => { rerender( - + ) diff --git a/docs/plans/2026-03-31-001-fix-custom-chat-widget-clears-url-icon-on-deselect-plan.md b/docs/plans/2026-03-31-001-fix-custom-chat-widget-clears-url-icon-on-deselect-plan.md index f3cc7f71727..09ed0d3b9c3 100644 --- a/docs/plans/2026-03-31-001-fix-custom-chat-widget-clears-url-icon-on-deselect-plan.md +++ b/docs/plans/2026-03-31-001-fix-custom-chat-widget-clears-url-icon-on-deselect-plan.md @@ -1,5 +1,5 @@ --- -title: "fix: Custom chat widget clears URL and icon on deselect" +title: 'fix: Custom chat widget clears URL and icon on deselect' type: fix status: active date: 2026-03-31 @@ -15,12 +15,14 @@ linear: NES-1522 **Research agents used:** Frontend races reviewer, TypeScript reviewer, Code simplicity reviewer, Pattern recognition specialist, Performance oracle, Framework docs researcher, Git history analyzer, ReactFlow learning check ### Key Improvements + 1. Replaced `useEffect` sync with render-time state reset — eliminates the double-render anti-pattern flagged by React docs 2. Added ESLint compliance guidance — extract `chatButton?.id` to a local const 3. Documented the TextFieldForm Formik key interaction as a known amplification vector 4. Added verification that the shift case is handled without stale-state frames ### New Considerations Discovered + - The `useState` + `useEffect` sync pattern is explicitly called an anti-pattern by the React docs ("You Might Not Need an Effect") - The `useEffect` approach causes a one-frame stale-state render before the effect fires — render-time reset eliminates this - The `TextFieldForm` key pattern `key={field-${id}-${initialValue}}` amplifies any state change into a full Formik remount @@ -39,6 +41,7 @@ key={customButtons[0]?.id ?? 'custom-0'} ``` When the button is deleted from the server and Apollo cache: + 1. `customButtons[0]` becomes `undefined` 2. The key changes from the button's ID (e.g. `"abc123"`) to the fallback `"custom-0"` 3. React unmounts the old component and mounts a new one @@ -50,6 +53,7 @@ Dedicated platforms have no `key` prop, so React preserves their component ident ### Research Insights **React docs on keys** (react.dev/learn/preserving-and-resetting-state): + > "Specifying a key tells React to use the key itself as part of the component's position. Every time a component with a specific key appears on the screen, its state is created fresh. Every time it is removed, its state is destroyed." **Git history**: The dynamic key pattern was introduced in PR #8865 (NES-1452, 2026-03-20) by jianwei1. A reviewer explicitly flagged the need for keys on the second custom slot. The key pattern exists specifically because `ChatOption` uses `useState` initializers that only run on mount. @@ -152,13 +156,13 @@ useEffect(() => { ## Verification Scenarios -| # | Scenario | Expected | -|---|----------|----------| -| A | 1 custom: deselect then re-select | URL and icon preserved | -| B | 2 customs: deselect first | Second button's data shifts into slot 1 correctly | -| C | 2 customs: deselect second | First button unchanged | -| D | Deselect then re-select then refresh page | Button re-created on server with preserved URL (verify via network or refresh) | -| E | Toggle dedicated platforms off and on | Regression check — unchanged behavior | +| # | Scenario | Expected | +| --- | ----------------------------------------- | ------------------------------------------------------------------------------ | +| A | 1 custom: deselect then re-select | URL and icon preserved | +| B | 2 customs: deselect first | Second button's data shifts into slot 1 correctly | +| C | 2 customs: deselect second | First button unchanged | +| D | Deselect then re-select then refresh page | Button re-created on server with preserved URL (verify via network or refresh) | +| E | Toggle dedicated platforms off and on | Regression check — unchanged behavior | ### Research Insights on Verification @@ -178,12 +182,12 @@ useEffect(() => { ## Edge Cases Discovered During Research -| Edge Case | Impact | Handling | -|---|---|---| -| Server normalizes link (e.g. lowercases hostname) | Low — currently server returns link as-is | State reset would write back the normalized value. Add a comment documenting this assumption. | -| `chatButton` transitions from undefined to `{id: undefined}` | Negligible — IDs are always UUIDs in practice | `chatButton.id !== trackedId` → `undefined !== undefined` → false → no sync. Technically correct (no button identity change). | -| User types in TextFieldForm but hasn't submitted | Unsubmitted input exists only in Formik's internal state | Deselecting destroys the Formik instance. This is consistent with dedicated platforms — user must commit (blur/enter) before toggling. | -| External update to chatButton fields (e.g. Google Sync) | State not synced if ID unchanged | By design — syncing on value changes would overwrite local edits. The ID-based approach prioritizes local state preservation. | +| Edge Case | Impact | Handling | +| ------------------------------------------------------------ | -------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------- | +| Server normalizes link (e.g. lowercases hostname) | Low — currently server returns link as-is | State reset would write back the normalized value. Add a comment documenting this assumption. | +| `chatButton` transitions from undefined to `{id: undefined}` | Negligible — IDs are always UUIDs in practice | `chatButton.id !== trackedId` → `undefined !== undefined` → false → no sync. Technically correct (no button identity change). | +| User types in TextFieldForm but hasn't submitted | Unsubmitted input exists only in Formik's internal state | Deselecting destroys the Formik instance. This is consistent with dedicated platforms — user must commit (blur/enter) before toggling. | +| External update to chatButton fields (e.g. Google Sync) | State not synced if ID unchanged | By design — syncing on value changes would overwrite local edits. The ID-based approach prioritizes local state preservation. | ## Sources