Skip to content

OUT-3669 | Save button appears broken on Home app IU - #212

Merged
arpandhakal merged 1 commit into
mainfrom
OUT-3669
Jun 12, 2026
Merged

OUT-3669 | Save button appears broken on Home app IU#212
arpandhakal merged 1 commit into
mainfrom
OUT-3669

Conversation

@arpandhakal

@arpandhakal arpandhakal commented Jun 12, 2026

Copy link
Copy Markdown
Collaborator

Changes

  • Fix the "Save Changes" button staying visible after a successful save of sidebar-only changes (e.g. Actions), even though the change was persisted (a refresh was previously needed to clear it). Closes OUT-3669.

Root cause: Change detection (useAppControls) compares the live settings against the saved baseline (initialSettings). After a save, the post-save sync (useSegmentSettings, which re-runs via the settings query) set the live content to the raw server value, but set the baseline content to the editor's normalized/canonical serialization (editor.getHTML()). When the stored body content wasn't already in canonical form, the two diverged, so the content comparison always read as dirty and the button never disappeared.

This only surfaced for changes that don't touch the body (Actions), because editing the body (e.g. inserting dynamic fields) normalizes the content first — which is exactly why dynamic-field changes worked but Actions didn't.

Fix: Use the editor's normalized content for both the live value and the baseline, in:

  • useSegmentSettings.ts — the post-save sync (the actual culprit).
  • useSettingsMutation.ts — the save handler itself, for consistency / no flicker.

Testing Criteria

  • In the Home app IU, toggle and/or reorder Actions → click Save Changes → button disappears and stays gone (no refresh needed).
  • Insert a dynamic field → Save → button disappears (regression check — already worked).
  • Edit heading / subheading / background color / body content → Save → button disappears.
  • Make a change → Cancel → reverts and button disappears.
  • (If applicable) switch segments → no false "dirty" state / Save button on load.
  • Loom: TODO

Notes

  • No API/schema changes; purely client-side change-detection sync.

Impact & Surface Area of Change

  • Touches the post-save settings sync and the settings mutation. Watch for: the editor body content rendering correctly after save/segment switch, and change detection across all home settings (heading, subheading, background color, body, actions, dynamic fields).

🤖 Generated with Claude Code

The Save Changes button stayed visible after saving sidebar-only changes
(e.g. Actions). Change detection compares the live settings against the
saved baseline, and the post-save sync set the live `content` to the raw
server value while setting the baseline to the editor's normalized
(canonical) serialization. When stored content wasn't already canonical,
the two diverged so `content` always read as dirty and the button never
went away. It only surfaced for changes that don't touch the body (Actions),
since editing the body normalizes the content first.

Use the editor's normalized content for both the live value and the
baseline in useSegmentSettings (the post-save sync) and useSettingsMutation.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@linear-code

linear-code Bot commented Jun 12, 2026

Copy link
Copy Markdown

OUT-3669

@vercel

vercel Bot commented Jun 12, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
client-home-v3 Ready Ready Preview, Comment Jun 12, 2026 8:54am

Request Review

@greptile-apps

greptile-apps Bot commented Jun 12, 2026

Copy link
Copy Markdown

Greptile Summary

Fixes the "Save Changes" button not disappearing after saving sidebar-only settings (e.g. Actions) by ensuring the editor-normalized HTML is used for both the live value and the change-detection baseline wherever settings are synced from the server.

  • useSegmentSettings.ts: setSettings is moved to after setContent, so the live state receives the TipTap-normalized HTML rather than the raw server string — eliminating the divergence that kept the dirty flag set.
  • useSettingsMutation.ts: onSuccess now reads editor?.getHTML() after setContent and passes the normalized value into both setSettings and setInitialSettings, adding consistency and preventing a brief flicker between the optimistic update and the useSegmentSettings effect that would otherwise re-normalize.

Confidence Score: 4/5

Safe to merge; the core fix in useSegmentSettings.ts is clean and the mutation path degrades gracefully via the subsequent segment-settings re-sync.

The useSegmentSettings.ts change is a correct, minimal reordering that solves the root cause. useSettingsMutation.ts applies the same normalization pattern but uses editor from the component closure rather than a ref; if editor is null at callback time the fix silently falls back to raw content, relying on the useSegmentSettings effect to heal the state. That path works in practice but is fragile compared to the ref-based approach used elsewhere.

Both files are small and focused; useSettingsMutation.ts is worth a second look for the editor nullability fallback on the getHTML call.

Important Files Changed

Filename Overview
src/features/settings/hooks/useSegmentSettings.ts Reorders setSettings to after setContent so both the live value and the baseline receive the editor-normalized content; the core fix for the false-dirty state on segment load/save.
src/features/settings/hooks/useSettingsMutation.ts Adds normalized content to both setSettings and setInitialSettings in onSuccess; correct when editor is non-null but silently falls back to raw server content if editor is null, leaving the bug temporarily present until useSegmentSettings heals the state.

Sequence Diagram

sequenceDiagram
    participant U as User
    participant SM as useSettingsMutation
    participant QC as QueryClient
    participant SS as useSegmentSettings
    participant E as TipTap Editor
    participant ST as SettingsStore

    U->>SM: Save Changes clicked
    SM->>ST: setSettings(variables) [optimistic]
    SM->>QC: PATCH /settings
    QC-->>SM: onSuccess(data)
    SM->>QC: setQueryData(raw data.data)
    SM->>E: setContent(data.data.content)
    SM->>E: getHTML()
    E-->>SM: normalizedContent
    SM->>ST: "setSettings({...data.data, content: normalizedContent})"
    SM->>ST: "setInitialSettings({...data.data, content: normalizedContent})"

    Note over QC,SS: setQueryData triggers useQuery re-render
    SS->>E: setContent(data.content)
    SS->>E: getHTML()
    E-->>SS: normalizedContent
    SS->>ST: "setSettings({...data, content: normalizedContent})"
    SS->>ST: "setInitialSettings({...data, content: normalizedContent})"

    Note over ST: live content == baseline content → button hidden
Loading

Reviews (1): Last reviewed commit: "fix(OUT-3669): keep Save button in sync ..." | Re-trigger Greptile

Comment on lines +45 to +47
const normalizedContent = editor?.getHTML() ?? data.data.content
setSettings({ ...data.data, content: normalizedContent })
setInitialSettings({ ...data.data, content: normalizedContent })

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Silent fallback to raw content when editor is unavailable

editor?.getHTML() evaluates to undefined if editor is null at the time onSuccess fires, so normalizedContent falls back to the raw server string (data.data.content). In that situation both setSettings and setInitialSettings are called with un-normalized content, which is the exact condition this PR is fixing. In practice the subsequent useSegmentSettings effect (triggered by setQueryData on the same line above) will heal the state, but the mutation path silently degrades and the flicker window remains. Using a ref for the editor — as useSegmentSettings already does with editorRef.current — would keep the reference fresh and make the fallback path unnecessary.

@SandipBajracharya SandipBajracharya left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@arpandhakal
arpandhakal merged commit 519562b into main Jun 12, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants