Skip to content

fix(settings): restore mounted flag on remount so auto-save can settle (MUL-4962) - #5613

Merged
Bohan-J merged 1 commit into
multica-ai:mainfrom
yyclaw:fix/auto-save-strictmode-mounted-flag
Jul 20, 2026
Merged

fix(settings): restore mounted flag on remount so auto-save can settle (MUL-4962)#5613
Bohan-J merged 1 commit into
multica-ai:mainfrom
yyclaw:fix/auto-save-strictmode-mounted-flag

Conversation

@yyclaw

@yyclaw yyclaw commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes the shared settings auto-save indicator getting stranded on "Saving…" forever, even though the save succeeded.

The useAutoSave mount effect (packages/views/settings/components/use-auto-save.ts) only cleared mountedRef in its cleanup and never re-set it to true on setup. Under React StrictMode (the Next.js dev default), the initial setup → cleanup → setup cycle leaves mountedRef.current pinned to false for the component's whole life. A successful save then never reaches its terminal branch (succeeded && mountedRef.current), so setStatus("saved") and onSuccess never fire — the indicator spins forever and no success toast appears, even though PATCH /api/workspaces/{id} returned 200.

The fix restores the invariant by setting mountedRef.current = true on every mount. This is the standard mounted-ref pattern and keeps the flag reflecting the live component across StrictMode's simulated remount (and any genuine remount).

Shared by all auto-saved settings surfaces (Repositories, GitHub, etc.), so this fixes the whole set.

Related Issue

Closes #5612

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Refactor / code improvement (no behavior change)
  • Documentation update
  • Tests (adding or improving test coverage)
  • CI / infrastructure

Changes Made

  • packages/views/settings/components/use-auto-save.ts: set mountedRef.current = true in the mount effect's setup so StrictMode's setup/cleanup/setup cycle no longer strands the flag at false.
  • packages/views/settings/components/use-auto-save.test.tsx: add a StrictMode regression test asserting a successful save still transitions to success (onSuccess fires).

How to Test

  1. Before the fix: run the web app in dev, go to Settings → Code repositories, add a repo URL and blur. The PATCH /api/workspaces/{id} returns 200 but the indicator stays on "Saving…" indefinitely.
  2. After the fix: the indicator transitions to "Saved" and the success toast fires.
  3. Automated: pnpm --filter @multica/views test -- use-auto-save. The added StrictMode test fails on the old code and passes with the fix.

Checklist

  • I have included a thinking path that traces from project context to this change
  • I have run tests locally and they pass
  • I have added or updated tests where applicable
  • If this change affects the UI, I have included before/after screenshots
  • I have updated relevant documentation to reflect my changes
  • If I added a new runtime / coding tool / UI tab, I synced the change to landing copy and relevant docs
  • If this PR touches Chinese product copy, I checked it against conventions.zh.mdx
  • I have considered and documented any risks above
  • I will address all reviewer comments before requesting merge

AI Disclosure

AI tool used: Claude Code

Prompt / approach: Started from a user-reported "Saving… spins forever" bug with a screenshot. Traced the auto-save state machine end to end, confirmed via DevTools that the PATCH returned 200 (ruling out a hung request), then pinpointed the broken mounted-ref pattern that only misfires under StrictMode's double-invoked mount. Wrote a failing StrictMode regression test first, verified it fails on the old code and passes with the one-line invariant fix.

@vercel

vercel Bot commented Jul 18, 2026

Copy link
Copy Markdown

@yyclaw is attempting to deploy a commit to the IndexLabs Team on Vercel.

A member of the Team first needs to authorize it.

The auto-save mount effect only cleared `mountedRef` in its cleanup and never
re-set it to true on setup. Under React StrictMode the initial
setup/cleanup/setup cycle therefore leaves `mountedRef.current` pinned to false
for the component's whole life. A successful save then never reaches the
terminal branch (`succeeded && mountedRef.current`), so the status is stranded
on "saving" and the success toast never fires — the settings save indicator
spins forever even though the PATCH returned 200.

Set `mountedRef.current = true` on every mount so the flag reflects the live
component. Shared by all auto-saved settings (repositories, GitHub, etc.).

Add a StrictMode regression test asserting onSuccess still fires.
@yyclaw
yyclaw force-pushed the fix/auto-save-strictmode-mounted-flag branch from 2cec5df to 993c0be Compare July 18, 2026 07:11
@Bohan-J Bohan-J changed the title fix(settings): restore mounted flag on remount so auto-save can settle fix(settings): restore mounted flag on remount so auto-save can settle (MUL-4962) Jul 20, 2026
@Bohan-J

Bohan-J commented Jul 20, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the great fix, @yyclaw! 🙏

The root-cause analysis was spot on: mountedRef initialized to true with a cleanup-only mount effect gets pinned to false by StrictMode's setup → cleanup → setup cycle, so the terminal succeeded && mountedRef.current branch never fires and the indicator spins forever despite a 200. Restoring the flag in the effect's setup is exactly the right, minimal fix, and the StrictMode regression test (which fails on the old code and passes on the new) is a nice touch that will keep this from regressing.

Merging now. Thanks again for the clear write-up and for catching this — it improves every auto-saved settings surface. 🎉

Minor non-blocking follow-up (no action needed): the pnpm --filter @multica/views test -- use-auto-save command in the description runs the whole @multica/views suite rather than just this file under the current Vitest arg parsing — pnpm --filter @multica/views exec vitest run settings/components/use-auto-save.test.tsx targets it directly. Doesn't affect the code or this merge.

@Bohan-J
Bohan-J merged commit a0ec1da into multica-ai:main Jul 20, 2026
5 checks passed
ba0f3 pushed a commit to r2d-ai/multica that referenced this pull request Jul 22, 2026
multica-ai#5613)

The auto-save mount effect only cleared `mountedRef` in its cleanup and never
re-set it to true on setup. Under React StrictMode the initial
setup/cleanup/setup cycle therefore leaves `mountedRef.current` pinned to false
for the component's whole life. A successful save then never reaches the
terminal branch (`succeeded && mountedRef.current`), so the status is stranded
on "saving" and the success toast never fires — the settings save indicator
spins forever even though the PATCH returned 200.

Set `mountedRef.current = true` on every mount so the flag reflects the live
component. Shared by all auto-saved settings (repositories, GitHub, etc.).

Add a StrictMode regression test asserting onSuccess still fires.
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.

[Bug]: Settings auto-save indicator spins forever under StrictMode despite a successful save

2 participants