Skip to content

[Dashboards] Fix page layout navigation edit mode sync#17478

Merged
ehconitin merged 2 commits intomainfrom
page-layout-unmount-cleanup
Jan 27, 2026
Merged

[Dashboards] Fix page layout navigation edit mode sync#17478
ehconitin merged 2 commits intomainfrom
page-layout-unmount-cleanup

Conversation

@ehconitin
Copy link
Copy Markdown
Contributor

@ehconitin ehconitin commented Jan 27, 2026

navigating away -

before -

CleanShot.2026-01-27.at.19.11.11.mp4

after -

CleanShot.2026-01-27.at.19.08.24.mp4

new dashbaord -

before -

CleanShot.2026-01-27.at.19.11.47.mp4

after -

CleanShot.2026-01-27.at.19.09.41.mp4

empty page layout -

before -

CleanShot.2026-01-27.at.19.13.13.mp4

after -

CleanShot.2026-01-27.at.19.10.24.mp4

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Jan 27, 2026

Greptile Overview

Greptile Summary

This PR fixes a synchronization issue with the dashboard edit mode by moving cleanup logic from the global location change handler to a component-specific unmount effect.

Key Changes:

  • Removed resetIsPageInEditMode from useExecuteTasksOnAnyLocationChange hook - this was resetting edit mode on any location change, which could happen before the PageLayout component actually unmounted
  • Created new PageLayoutUnmountCleanupEffect component that properly cleans up state during component unmount
  • The cleanup effect resets three states: draft page layout, edit mode flag, and initialization flag
  • Integrated the cleanup effect into PageLayoutRenderer to ensure it runs with the correct component lifecycle

Technical Approach:
The fix properly separates concerns by moving page-layout-specific cleanup from the global location change handler to a scoped effect component. This follows the React best practice of tying side effects to component lifecycle rather than global events, preventing race conditions where state is reset before the component has a chance to unmount properly.

Confidence Score: 4/5

  • This PR is safe to merge with proper testing of dashboard edit mode navigation
  • The changes follow React best practices and properly scope cleanup to component lifecycle. The implementation correctly uses useEffect cleanup functions and follows the codebase's *Effect.tsx component pattern. Score is 4 (not 5) because this fix changes state management timing which could have subtle edge cases that need testing in various navigation scenarios
  • Test PageLayoutUnmountCleanupEffect.tsx thoroughly with different navigation patterns to ensure edit mode synchronization works correctly

Important Files Changed

Filename Overview
packages/twenty-front/src/modules/app/hooks/useExecuteTasksOnAnyLocationChange.ts Removed edit mode reset logic from global location change handler - properly scoped cleanup moved to component-specific unmount effect
packages/twenty-front/src/modules/page-layout/components/PageLayoutRenderer.tsx Added PageLayoutUnmountCleanupEffect component to handle cleanup when page layout unmounts
packages/twenty-front/src/modules/page-layout/components/PageLayoutUnmountCleanupEffect.tsx New effect component that properly cleans up page layout state on unmount (edit mode, draft state, initialization flag)

Sequence Diagram

sequenceDiagram
    participant User
    participant PageLayoutRenderer
    participant UnmountCleanupEffect
    participant PageLayoutState
    participant LocationChangeHook
    
    Note over User,LocationChangeHook: Before PR: Global location change resets edit mode
    User->>LocationChangeHook: Navigate to different page
    LocationChangeHook->>PageLayoutState: Reset edit mode globally
    Note over PageLayoutState: Issue: Edit mode reset happens<br/>before component unmount
    
    Note over User,LocationChangeHook: After PR: Scoped cleanup on unmount
    User->>PageLayoutRenderer: Mount PageLayout component
    PageLayoutRenderer->>UnmountCleanupEffect: Mount cleanup effect
    UnmountCleanupEffect->>UnmountCleanupEffect: Setup useEffect cleanup
    
    User->>PageLayoutRenderer: Navigate away
    PageLayoutRenderer->>UnmountCleanupEffect: Component unmounting
    UnmountCleanupEffect->>PageLayoutState: resetDraftPageLayoutToPersistedPageLayout()
    UnmountCleanupEffect->>PageLayoutState: setIsPageLayoutInEditMode(false)
    UnmountCleanupEffect->>PageLayoutState: setIsInitialized(false)
    Note over UnmountCleanupEffect,PageLayoutState: Cleanup happens at proper time<br/>during component unmount
Loading

Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

No files reviewed, no comments

Edit Code Review Agent Settings | Greptile

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

No issues found across 3 files

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jan 27, 2026

🚀 Preview Environment Ready!

Your preview environment is available at: http://bore.pub:24728

This environment will automatically shut down when the PR is closed or after 5 hours.

@ehconitin ehconitin requested a review from bosiraphael January 27, 2026 14:06
@ehconitin ehconitin changed the title [Dashboards] Fix page layout unmount edit mode sync [Dashboards] Fix page layout navigation edit mode sync Jan 27, 2026
Copy link
Copy Markdown
Contributor

@bosiraphael bosiraphael left a comment

Choose a reason for hiding this comment

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

LGTM

@ehconitin ehconitin added this pull request to the merge queue Jan 27, 2026
Merged via the queue into main with commit 27e5b97 Jan 27, 2026
69 checks passed
@ehconitin ehconitin deleted the page-layout-unmount-cleanup branch January 27, 2026 15:36
@twenty-eng-sync
Copy link
Copy Markdown

Hey @ehconitin! After you've done the QA of your Pull Request, you can mark it as done here. Thank you!

1 similar comment
@twenty-eng-sync
Copy link
Copy Markdown

Hey @ehconitin! After you've done the QA of your Pull Request, you can mark it as done here. Thank you!

@github-actions
Copy link
Copy Markdown
Contributor

Fails
🚫

node failed.

Log

Details
�[31mError: �[39m SyntaxError: Unexpected token 'C', "Contributo"... is not valid JSON
    at JSON.parse (<anonymous>)
�[90m    at parseJSONFromBytes (node:internal/deps/undici/undici:4259:19)�[39m
�[90m    at successSteps (node:internal/deps/undici/undici:6882:27)�[39m
�[90m    at readAllBytes (node:internal/deps/undici/undici:5807:13)�[39m
�[90m    at process.processTicksAndRejections (node:internal/process/task_queues:103:5)�[39m
danger-results://tmp/danger-results-11dc6227.json

Generated by 🚫 dangerJS against 8daa65f

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