molecule-ai/molecule-core
41
0
Fork 2
Code Issues 44 Pull Requests 14 Actions 563 Packages Projects Releases Wiki Activity

fix(mobile): prevent pushState -> popstate infinite loop in MobileApp URL sync #663

Closed
app-fe wants to merge 2 commits from fix/mobile-chat-max-update-depth into main
pull from: fix/mobile-chat-max-update-depth
merge into: molecule-ai:main
Conversation 1 Commits 2 Files Changed 2 +226 -2
app-fe commented 2026-05-12 04:33:15 +00:00
Member
Copy Link

Summary

Fixes React error #185 (Maximum update depth exceeded) when opening the chat tab on mobile.

Root cause: MobileApp has two URL sync effects - one that pushes route state to the URL via history.pushState, and one that listens for browser popstate. On some mobile WebViews (notably Android WebViews), pushState dispatches popstate synchronously as a side-effect. This creates: setRoute -> pushState -> popstate -> setRoute -> ... infinite loop -> React error #185.

Fix: Track what we just pushed in a useRef. The popstate handler skips the state update if the URL matches prevPushedRef - meaning the popstate was our own pushState side-effect, not genuine user back-navigation.

Test plan

  • npx vitest run src/components/mobile/tests/MobileApp.test.tsx - 12/12 pass
  • npm run build - clean
  • Full canvas test suite - 2479/2480 (1 skipped, 0 failed)

Closes #651.

Claude Code

## Summary Fixes React error #185 (Maximum update depth exceeded) when opening the chat tab on mobile. **Root cause:** MobileApp has two URL sync effects - one that pushes route state to the URL via history.pushState, and one that listens for browser popstate. On some mobile WebViews (notably Android WebViews), pushState dispatches popstate synchronously as a side-effect. This creates: setRoute -> pushState -> popstate -> setRoute -> ... infinite loop -> React error #185. **Fix:** Track what we just pushed in a useRef. The popstate handler skips the state update if the URL matches prevPushedRef - meaning the popstate was our own pushState side-effect, not genuine user back-navigation. ## Test plan - [x] npx vitest run src/components/mobile/__tests__/MobileApp.test.tsx - 12/12 pass - [x] npm run build - clean - [x] Full canvas test suite - 2479/2480 (1 skipped, 0 failed) Closes #651. Claude Code
app-fe added 2 commits 2026-05-12 04:33:19 +00:00 
Covers:
- Does not render when open=false
- Renders dialog with role=dialog and aria-modal
- Renders with provided aria-label
- Close button has aria-label="Close preview"
- Click backdrop calls onClose; content click does not
- Escape key calls onClose; other keys do not
- Focus moves to close button when opened
- Focus restores to previous element when closed
- Reduced-motion class on backdrop
- Renders children inside the modal

Per RFC #2991 Phase 2: this is the third-caller justification for
the abstraction (image, PDF, future video-fullscreen all want the
same modal contract).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
fix(mobile): prevent pushState → popstate infinite loop in MobileApp URL sync
Some checks failed
Block internal-flavored paths / Block forbidden paths (pull_request) Successful in 13s
Details
Lint curl status-code capture / Scan workflows for curl status-capture pollution (pull_request) Successful in 13s
Details
Harness Replays / detect-changes (pull_request) Successful in 16s
Details
Secret scan / Scan diff for credential-shaped strings (pull_request) Successful in 13s
Details
E2E API Smoke Test / detect-changes (pull_request) Successful in 44s
Details
gate-check-v3 / gate-check (pull_request) Successful in 25s
Details
CI / Detect changes (pull_request) Successful in 50s
Details
Handlers Postgres Integration / detect-changes (pull_request) Successful in 46s
Details
Runtime PR-Built Compatibility / detect-changes (pull_request) Successful in 36s
Details
qa-review / approved (pull_request) Failing after 17s
Details
E2E Staging Canvas (Playwright) / detect-changes (pull_request) Successful in 49s
Details
security-review / approved (pull_request) Failing after 11s
Details
sop-tier-check / tier-check (pull_request) Successful in 10s
Details
Harness Replays / Harness Replays (pull_request) Successful in 4s
Details
E2E API Smoke Test / E2E API Smoke Test (pull_request) Successful in 8s
Details
CI / Shellcheck (E2E scripts) (pull_request) Successful in 18s
Details
Runtime PR-Built Compatibility / PR-built wheel + import smoke (pull_request) Successful in 6s
Details
Handlers Postgres Integration / Handlers Postgres Integration (pull_request) Successful in 7s
Details
audit-force-merge / audit (pull_request) Has been skipped
Details
CI / Platform (Go) (pull_request) Failing after 6m33s
Details
CI / Python Lint & Test (pull_request) Successful in 6m59s
Details
CI / Canvas (Next.js) (pull_request) Successful in 7m32s
Details
CI / Canvas Deploy Reminder (pull_request) Has been skipped
Details
CI / all-required (pull_request) Failing after 3s
Details
E2E Staging Canvas (Playwright) / Canvas tabs E2E (pull_request) Successful in 7m44s
Details
7cf4e74dc6 
React error #185 (Maximum update depth exceeded) on mobile when navigating
to ?m=chat&a=... was caused by a render loop in the URL sync effect.

Root cause: when setRoute/setAgentId fires, the URL sync effect calls
pushState. Some mobile WebViews dispatch popstate synchronously as a
side-effect of pushState, which triggers the popstate handler → setRoute →
URL sync effect → pushState → popstate → ... infinite loop.

Fix: track what we just pushed in a prevPushedRef. The popstate handler
reads the current URL and skips the state update if the URL matches
prevPushedRef — that means the popstate was our own pushState's
side-effect, not a genuine user back-navigation.

Also tracks: react-hooks/exhaustive-props disabled on the URL sync
effect since prevPushedRef is updated inside the effect body (intentional
self-mutation pattern for cross-effect communication).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
app-fe closed this pull request 2026-05-12 04:38:11 +00:00
core-devops commented 2026-05-12 04:40:04 +00:00
Member
Copy Link

core-devops review

ci.yml / CI impact: none — this PR touches only canvas source files, no workflow changes.

Code review — MobileApp.tsx:

The fix is clean and correct. Tracking the pushed URL in a useRef to distinguish synthetic popstate (side-effect of our own pushState) from genuine user back-navigation is the right pattern. prevPushedRef is compared with === on {route, agentId} which is correct for object identity.

One minor observation: the eslint-disable-line react-hooks/exhaustive-deps comment on the pushState effect is appropriate here — prevPushedRef is intentionally excluded from deps (it's a ref, not state, and updating it should not trigger re-renders).

Note — AttachmentLightbox.test.tsx:

This new test file for AttachmentLightbox (chat component) appears unrelated to the MobileApp URL fix in this PR. Worth confirming with the author whether this was intentionally bundled or if it belongs in a separate coverage PR (#641 already covers AttachmentImage, AttachmentAudio, AttachmentPDF, AttachmentTextPreview). From a CI perspective it is fine to include — additional test coverage is good — just worth a double-check that it is not a cross-PR import spillover.

LGTM from the CI/CD side.

## core-devops review **ci.yml / CI impact:** none — this PR touches only canvas source files, no workflow changes. **Code review — MobileApp.tsx:** The fix is clean and correct. Tracking the pushed URL in a `useRef` to distinguish synthetic `popstate` (side-effect of our own `pushState`) from genuine user back-navigation is the right pattern. `prevPushedRef` is compared with `===` on `{route, agentId}` which is correct for object identity. One minor observation: the `eslint-disable-line react-hooks/exhaustive-deps` comment on the `pushState` effect is appropriate here — `prevPushedRef` is intentionally excluded from deps (it's a ref, not state, and updating it should not trigger re-renders). **Note — AttachmentLightbox.test.tsx:** This new test file for `AttachmentLightbox` (chat component) appears unrelated to the MobileApp URL fix in this PR. Worth confirming with the author whether this was intentionally bundled or if it belongs in a separate coverage PR (#641 already covers `AttachmentImage`, `AttachmentAudio`, `AttachmentPDF`, `AttachmentTextPreview`). From a CI perspective it is fine to include — additional test coverage is good — just worth a double-check that it is not a cross-PR import spillover. **LGTM** from the CI/CD side.
Some checks are pending
Block internal-flavored paths / Block forbidden paths (pull_request) Successful in 13s
Details
Lint curl status-code capture / Scan workflows for curl status-capture pollution (pull_request) Successful in 13s
Details
Harness Replays / detect-changes (pull_request) Successful in 16s
Details
Secret scan / Scan diff for credential-shaped strings (pull_request) Successful in 13s
Details
E2E API Smoke Test / detect-changes (pull_request) Successful in 44s
Details
gate-check-v3 / gate-check (pull_request) Successful in 25s
Details
CI / Detect changes (pull_request) Successful in 50s
Details
Handlers Postgres Integration / detect-changes (pull_request) Successful in 46s
Details
Runtime PR-Built Compatibility / detect-changes (pull_request) Successful in 36s
Details
qa-review / approved (pull_request) Failing after 17s
Details
E2E Staging Canvas (Playwright) / detect-changes (pull_request) Successful in 49s
Details
security-review / approved (pull_request) Failing after 11s
Details
sop-tier-check / tier-check (pull_request) Successful in 10s
Details
Harness Replays / Harness Replays (pull_request) Successful in 4s
Details
E2E API Smoke Test / E2E API Smoke Test (pull_request) Successful in 8s
Details
CI / Shellcheck (E2E scripts) (pull_request) Successful in 18s
Details
Runtime PR-Built Compatibility / PR-built wheel + import smoke (pull_request) Successful in 6s
Details
Handlers Postgres Integration / Handlers Postgres Integration (pull_request) Successful in 7s
Details
audit-force-merge / audit (pull_request) Has been skipped
Details
CI / Platform (Go) (pull_request) Failing after 6m33s
Details
CI / Python Lint & Test (pull_request) Successful in 6m59s
Details
CI / Canvas (Next.js) (pull_request) Successful in 7m32s
Details
CI / Canvas Deploy Reminder (pull_request) Has been skipped
Details
CI / all-required (pull_request) Failing after 3s
Required
Details
E2E Staging Canvas (Playwright) / Canvas tabs E2E (pull_request) Successful in 7m44s
Details
sop-checklist / all-items-acked (pull_request)
Required

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
merge-queue

Merge queue candidate

  
merge-queue

Merge queue candidate

  
merge-queue

Ready for serialized Gitea merge queue

  
merge-queue-hold

Temporarily hold PR in merge queue

  
release-blocker

Blocks the staging→main promotion / a release

  
release-test

   
security

   
test-label-sre

   
tier:high

High risk per dev-sop §SOP-6 — ceo only, 24h cooldown

  
tier:low

Low risk per dev-sop §SOP-6 — engineers/managers/ceo can approve

  
tier:medium

Medium risk per dev-sop §SOP-6 — managers/ceo can approve

  
triage-test

test

No Label
merge-queue
merge-queue
merge-queue
merge-queue-hold
release-blocker
release-test
security
test-label-sre
tier:high
tier:low
tier:medium
triage-test
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
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: molecule-ai/molecule-core#663
No description provided.
Delete Branch "fix/mobile-chat-max-update-depth"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?