Fix React #185 in split thread views - #1253
Conversation
| }: UseGitDiffPanelParams) { | ||
| const selectedMergeBaseBranch = useAtomValue(selectedMergeBaseBranchAtom); | ||
| const setSelectedMergeBaseBranch = useSetAtom(selectedMergeBaseBranchAtom); | ||
| const [selectedMergeBaseBranch, setSelectedMergeBaseBranch] = |
There was a problem hiding this comment.
🚨 slopcop/review — The branch state has no environment identity.
A pane can change from environment A to B without a remount. The first B render still uses branch A.
The effect at line 82 clears the branch later. This can start B queries with the branch from A.
Store the environment ID with the branch. Ignore the branch when its environment does not match.
Add a test that renders the view again with environment B. Confirm that no B query uses branch A.
| const setSelectedMergeBaseBranch = useSetAtom(selectedMergeBaseBranchAtom); | ||
| const [selectedMergeBaseBranch, setSelectedMergeBaseBranch] = | ||
| useState<string>(); | ||
| const setPendingGitDiffScrollPath = useSetAtom(pendingGitDiffScrollPathAtom); |
There was a problem hiding this comment.
🚨 slopcop/review — The file and commit intents still use global atoms.
Every split pane reads these atoms. Pane B can consume or clear a request from pane A.
This event can reset the selection in pane B. It can also cancel the requested scroll in pane A.
Move both intents into the pane owner. An atom family with a pane and thread key also works.
Add a delayed scroll test. Confirm that only pane A consumes its request.
| setThreadSecondaryPanel: noop, | ||
| }); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
🚨 slopcop/review — This test copies the production synchronization effect.
The test does not use useEnvironmentMergeBase. It does not change an environment ID.
It also omits the split close and reopen sequence from issue #1250. Therefore, both medium findings can remain while this test passes.
Use the production hooks. Add the complete close and reopen regression test.
SawyerHood
left a comment
SawyerHood
left a comment
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
ELI5: Each split pane needs its own small box for branch and diff requests.
This change gives each pane a branch box. However, two request notes remain in one shared box.
Review result: I found two medium correctness issues and one low test gap.
First, the selected branch has no environment key. A pane can query the new environment with the old branch.
Bind the branch to its environment. Add a test that changes the environment without a remount.
Second, file-open and commit-open requests still use global atoms. Another pane can consume or clear these requests.
Move these requests into the pane owner, or key them by pane and thread.
The new ownership test copies the production synchronization effect. It omits the production hook composition and the split close and reopen sequence.
Security: I found no security issue. The diff adds no new permission, process, storage, HTML, dependency, server, or daemon boundary.
Performance: I found no material performance defect. The common offsetHeight measurement prevents alternating box models and repeated state updates.
Architecture: The local branch state is a good direction. The codebase already uses an atom family for per-thread panel state.
The two remaining intents should use the same ownership model.
Evidence: Three GPT-5.6 workers checked security, code quality, and performance. A separate GPT-5.6 reviewer checked their combined findings.
GitHub reports no checks for this SHA. I did not run the fork code because that action would execute untrusted code.
I also did not start a development server or use a browser for the same reason.
The pull request body claims successful tests and packaged browser checks. I could not verify those claims.
Two medium findings remain before merge.
2 participants
Fixes #1246.
Fixes #1250.
What changed
Why
Split panes could have different merge-base branches while reading and writing the same global selection. Each pane then restored its own value, causing the values to alternate and the panes to rerender until React threw error #185 and blanked the route.
The composer measurement path now also treats repeated layout and resize measurements as one update while split layout is settling.
Verification
@bb/appsuite: 326 files, 2,453 tests passed.@bb/app.bb-apptarball smoke test passed.