Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Address persistence approach (localStorage vs IPC), toggle window resize math, transition-during-drag conflict, collapsed state drag target, and inline style migration. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Custom drag-to-resize sidebar with snap-to-close behavior, localStorage persistence, and a blue hover indicator line. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
katinthehatsite
left a comment
There was a problem hiding this comment.
Generally, the changes look good to me 👍
I noticed one thing where in some languages that use longer transations, you can see the running sites being mushed together:
In my test, I switched to Ukrainian and resized the sidebar to as narrow as I could before it hides.
Additionally, I would be curious to test this on Windows. I can take a look into that tomorrow.
|
@Automattic/yolo could you please review? I made some changes based in my recommendations but I would appreciate another pair of eyes. |
| } from 'src/constants'; | ||
|
|
||
| export function getSavedSidebarWidth(): number { | ||
| const saved = localStorage.getItem( LOCAL_STORAGE_SIDEBAR_WIDTH_KEY ); |
There was a problem hiding this comment.
Why did we decide on a local storage approach?
We already have some UI properties (window size) living in the app.json config file.
I don't have anything against local storage, I just believe it should be consistent with our other properties.
There was a problem hiding this comment.
I don't have the answer specifically for that bit since it was not originally my PR but it makes sense to be consistent with our other properties. I will check how we can align our approach here 👀
There was a problem hiding this comment.
I had a chance to check this a bit further and I think the localStorage approach would be all right here. The sidebar width uses localStorage rather than app.json because it's renderer-side state: it's read synchronously at component init (allowing it to be passed directly as a useState initializer) and never needed by the Main process. windowBounds lives in app.json because Electron's Main process must restore the window size before the renderer exists. Moving sidebar width to app.json would introduce an async IPC to initialize a CSS value, requiring a loading state and added complexity.
There was a problem hiding this comment.
That makes a lot of sense, thanks for the explanation! 👍
gcsecsey
left a comment
There was a problem hiding this comment.
The changes make sense to me, and it overall works well. I added a comment on the snapping.
| if ( finalWidth < SIDEBAR_SNAP_THRESHOLD ) { | ||
| // Snap closed — restore the last good width for when it reopens | ||
| setSidebarWidth( | ||
| dragStartWidth.current > SIDEBAR_MIN_WIDTH ? dragStartWidth.current : SIDEBAR_WIDTH | ||
| ); | ||
| if ( isSidebarVisible ) { | ||
| toggleSidebar(); | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
To me, the UX for snapping the sidebar closed feels a bit strange. When trying to snap it closed, the current threshold feels a bit too high, the user needs to drag the line almost half the way before it registers as snapped. The content beneath also reflows when this happens.
CleanShot.2026-04-14.at.14.41.14-converted.mp4
I'm wondering if we could omit this snap-to-close behavior completely and just rely on the toggle button on top?
There was a problem hiding this comment.
I ended up removing the snap behaviour so that now it looks like this:
Screen.Recording.2026-04-21.at.11.23.51.AM.mov
Let me know what you think!
📊 Performance Test ResultsComparing 16a9189 vs trunk app-size
site-editor
site-startup
Results are median values from multiple test runs. Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change (<50ms diff) |
katinthehatsite
left a comment
There was a problem hiding this comment.
The changes look good, I also re-requested the review from two other reviewers.
| isMac() && 'pt-[10px]', | ||
| ! isMac() && 'pt-[38px]', |
There was a problem hiding this comment.
Let me double-check, if I am not mistaken this was related to resolving conflicts with trunk 👀
There was a problem hiding this comment.
I removed it as it seems to be an unintended leftover after the conflict resolution
Co-authored-by: Volodymyr Makukha <nei.css@gmail.com>
nightnei
left a comment
There was a problem hiding this comment.
Great improvement, I will increase sidebar width right after this PR is merged :)
Co-authored-by: Bernardo Cotrim <bmmcotrim@gmail.com>
bcotrim
left a comment
There was a problem hiding this comment.
LGTM 👍
Thanks for completing the changes @katinthehatsite

Proposed Changes
useSidebarResizehooklocalStorageacross sessionstoggleMinWindowWidthIPC handler to use the current sidebar width instead of the hardcoded constantTesting Instructions