fix(panels): proportional absorber pins, stale-pin cleanup, double-click to reset#123
Conversation
|
Thank you very much for this! <3 I reviewed this with separate passes over the resize component, task layout wiring, and tiling reset behavior. I think there’s one blocker before this lands: The A few smaller follow-ups I noticed:
|
|
Thanks for the thorough review! This was mostly me experimenting to learn how things fit together, so appreciate the patience! On the follow-ups: I had similar concerns about the flex-grow weighting and index drift while writing it, but went with the simpler fix for now since I wasn't sure how far this panel pattern is intended to expand. Happy to revisit if you have a sense of where it's headed, especially with behaviour in combination with the Terminal/dev/e2e section, which I haven't really touched. A few questions before I tinker further:
Would love to hear how you're thinking about these sections long-term, mostly so I can work around your direction rather than against it. For now I'm just adding things I personally want haha, but if I make a bigger structural change, is it better to gate it behind a settings toggle (like the Send a Prompt panel)? |
You can use it for everything, but personally I mostly use it for keeping track I've want to evaluate with the agent later, when current work is done.
In terms of what gets send to the agent, there is no difference.
|
|
For this PR I would keep the scope conservative: either restore the notes I do not think we need to make Plans are not meant to be Claude-only long term, but the current implementation is Claude-biased: Claude is configured via On prompt surfaces: Notes, the prompt input, and review submission all send through the same running-agent prompt path. Notes are mainly a persistent scratchpad/backlog; the prompt box is the transient next-message composer. Inline "ask about code" is different: that is a separate one-shot Q&A flow, not the task agent session. For bigger structural changes, I would prefer good defaults over adding settings toggles by default. Add toggles when it is a lasting workflow preference or the UI would otherwise be noisy/controversial. |
…ick to reset ResizablePanel: - Switch wrapperRefs from an index-keyed array to an ID-keyed Map so drag measurement survives dynamic children changes (e.g. plan tab appearing). - Add flexGrow prop to PanelChild for weighted absorbers (notes-files gets a smaller weight so the AI terminal absorbs ~2/3 of remaining space). - Store absorber pins as flex-grow ratios instead of pixel values so pinned panels scale proportionally when the window is resized. - Detect and clear asymmetric absorber pins (one absorber pinned while siblings are not) — these are stale pixel values that would starve siblings. TilingLayout: - Double-click on a tiling resize handle calls deletePanelUserSize to unpin both neighbours, snapping them back to the default equal split. TaskNotesBody: - Remove 40vh max-height cap; notes-files is now a weighted absorber so its height is layout-driven, not content-driven. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
01029dd to
c932215
Compare
|
Thank you very much once more! There still seem to be some problems. Multi-Agent Review — additional findingsRan a parallel 6-agent review (correctness, security, architecture, alternatives, performance, simplicity). The four items you (@johannesjo) already flagged hold up — restating only the new findings below to avoid noise. Critical (new)
Warnings (new)
Suggestions (new)
What I'd merge as-isNothing yet — confidence high on the structural items above. The smallest viable path is still your suggested scope: wire |
- Rename flexGrow -> absorberWeight on PanelChild; wire notes-files as a weighted absorber (absorberWeight: 0.5) in TaskPanel and add it to absorberIds alongside ai-terminal. Height is now layout-driven so the max-height: 40vh cap on TaskNotesBody is removed. - Add onCleanup(() => wrapperRefs.delete(child.id)) to fix Map leak on child unmount - Add Number.isFinite + > 0 guard on persisted pin reads so a corrupt store value can't emit broken CSS - Gate double-click reset on dragging() !== null to prevent drag-preview flash race; inline unpinHandle body while there - Remove unused idx param from childStyle (side effect of ID-keyed dragOverride from prior commit) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Apologies for the force pushes earlier it seems like some changes in the origin triggered an update which then lead to rebasing to stay current with upstream. I understand that rewrites the branch history and makes incremental review harder and very sorry for the confusion. All updates from here are regular commits. Addressed everything flagged: Blocker (from first review)
From the parallel review
Left out of scope (as suggested)
|
|
Thank you very much! |
What
ResizablePanel reliability fixes
wrapperRefsfrom an index-keyed array to an ID-keyedMap— drag measurement now survives dynamic children changes (e.g. the plan tab appearing mid-drag)flexGrowprop toPanelChildso absorbers can be weighted (notes-files gets a smaller weight, leaving the AI terminal ~2/3 of the remaining space)Double-click on tiling resize handles to unpin
Layout cleanup
40vhmax-heightfromTaskNotesBody— it was a workaround for content-size bubbling up the flex chain;notes-filesis now a proper weighted absorber so height is layout-driven, not content-drivenWhy
Pinned panels previously stored pixel values, which go stale when the window is resized or when an absorber configuration changes. Storing ratios instead keeps the layout self-healing. The double-click reset gives users a quick escape hatch without needing to drag.
Testing
PS. just cosmetic fixes! just wanted to test what submitting a PR on parallel code would look like :)