Skip to content

fix: Fix diff view not clearing on sidebar tab switch#199

Merged
backnotprop merged 2 commits intobacknotprop:mainfrom
flex-yj-kim:fix/clear-diff-on-tab-switch
Mar 2, 2026
Merged

fix: Fix diff view not clearing on sidebar tab switch#199
backnotprop merged 2 commits intobacknotprop:mainfrom
flex-yj-kim:fix/clear-diff-on-tab-switch

Conversation

@flex-yj-kim
Copy link
Copy Markdown
Contributor

@flex-yj-kim flex-yj-kim commented Mar 1, 2026

Summary

  • Clear diff view state when switching from Versions tab to Contents tab in the sidebar
  • Dismiss diff view overlay on Escape key press
  • Add dev mock API for version history to support local development

sidebar.activeTab and isPlanDiffActive were independent states with
no synchronization. Switching to TOC tab left the diff viewer active
in the main area. Add useEffect to reset isPlanDiffActive when tab
changes to "toc".
…story

- Add keydown listener to clear isPlanDiffActive on Escape
- Add Vite dev plugin (dev-mock-api.ts) serving mock /api/plan endpoints
  with 3 plan versions so the Versions tab works during local development
- Guard setMarkdown against undefined plan from API response
@flex-yj-kim flex-yj-kim force-pushed the fix/clear-diff-on-tab-switch branch from ebf265b to 69a6728 Compare March 1, 2026 15:51
@flex-yj-kim flex-yj-kim marked this pull request as ready for review March 1, 2026 15:51
@backnotprop
Copy link
Copy Markdown
Owner

Code review

Found 1 issue:

  1. isPlanDiffActive is read inside the tab-switch useEffect but missing from its dependency array, creating a stale closure. The Escape key effect directly below correctly includes [isPlanDiffActive] in its deps. This is the same class of bug that was caught in PR feat: Plan Diff marketing dialog, Pi origin, and docs #177.

// Clear diff view when switching away from versions tab
useEffect(() => {
if (sidebar.activeTab === 'toc' && isPlanDiffActive) {
setIsPlanDiffActive(false);
}
}, [sidebar.activeTab]);

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

@backnotprop
Copy link
Copy Markdown
Owner

Correction to my earlier review — disregard the previous comment.

After pulling the branch locally and tracing the data flow more carefully, the missing isPlanDiffActive dependency is intentional, not a bug.

The effect's purpose is to clear the diff view when activeTab transitions to toc. The isPlanDiffActive check inside the body is just a guard to skip a redundant state update. If isPlanDiffActive were added to the deps array, it would create a worse bug: activating diff view while already on the TOC tab would immediately get cleared by the effect re-firing.

The closure captures isPlanDiffActive from the render where activeTab changed, which is always fresh at that point. No issue here.

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

@backnotprop backnotprop merged commit 99c0666 into backnotprop:main Mar 2, 2026
3 checks passed
@flex-yj-kim flex-yj-kim deleted the fix/clear-diff-on-tab-switch branch March 2, 2026 14:22
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