fix: don't override loadGraphData viewport on cache miss

[artokun]

Summary

Fix regression from #10247 where template workflows (e.g. LTX2.3) loaded with a broken viewport.

Problem

restoreViewport() called fitView() on every cache miss via rAF. This raced with loadGraphData's own viewport restore (extra.ds for saved workflows, or its own fitView() for templates at line 1266 of app.ts). The second fitView() overwrote the correct viewport, causing templates with subgraphs to display incorrectly.

Fix

On cache miss, check if any nodes are already visible in the current viewport before calling fitView(). If loadGraphData already positioned things correctly, we don't override it. Only intervene when the viewport is genuinely empty (first visit to a subgraph with no prior cached state AND no loadGraphData restore).

Review Focus

Single-file change in subgraphNavigationStore.ts. The visibility check mirrors the same pattern used in app.ts:1272-1281 where loadGraphData itself checks for visible nodes.

E2E Regression Test

The existing Playwright tests in browser_tests/tests/subgraphViewport.spec.ts (added in #10247) already cover viewport restoration after subgraph navigation. The specific regression (template load viewport race) is not practically testable in E2E because:

1. Template loading requires the backend's template API which returns different templates per environment
2. The race condition depends on exact timing between loadGraphData's viewport restore and the rAF-deferred restoreViewport — Playwright cannot reliably reproduce frame-level timing races
3. The fix is a guard condition (skip fitView if nodes visible) that makes the behavior idempotent regardless of timing

Alternative to #10790

This can replace the full revert in #10790 — it preserves the viewport persistence feature while fixing the template regression.

Fixes regression from #10247

[artokun]

@DrJKL This fixes the LTX2.3 template viewport regression from #10247 without a full revert. Can replace #10790.

The issue: restoreViewport() called fitView() on cache miss via rAF, which raced with loadGraphData's own viewport restore. Now checks if nodes are already visible before intervening.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: fa7e86e7-7ffc-4b71-8014-fda3daf5a48c

📥 Commits

Reviewing files that changed from the base of the PR and between df7c4c5 and 224e0c8.

📒 Files selected for processing (1)

• src/stores/subgraphNavigationStore.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• src/stores/subgraphNavigationStore.ts

---

📝 Walkthrough

Walkthrough

On viewport restore cache miss, the store now conditionally calls fitView() only after verifying the canvas and graph exist, the graph has nodes, the active graph id still matches, and no node bounding boxes intersect the computed visible canvas area.

Changes

Cohort / File(s)	Summary
Viewport Cache Miss Handler src/stores/subgraphNavigationStore.ts	On cache miss, replace unconditional scheduling of fitView() with conditional logic: re-check active graph id at rAF time, ensure canvas and canvas.graph exist, require graph.nodes.length > 0, compute visible canvas area from canvas.viewport, and call fitView() only if no node bounding boxes intersect that visible area.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 I twitch my whiskers, hop to see,

I check the bounds before I plea,
If nodes are near the view I stay,
No needless zoom to lead astray,
A careful hop and quiet glee.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main fix: preventing viewport override on cache miss, which is the core change in the changeset.
Description check	✅ Passed	The description includes all required sections (Summary, Problem, Fix) with clear context, review focus, and testing notes. The structure follows the template guidelines.
End-To-End Regression Coverage For Fixes	✅ Passed	PR uses bug-fix language in title and provides concrete explanation for lack of E2E tests citing impractical reproducibility due to environment-dependent templates and timing sensitivity.
Adr Compliance For Entity/Litegraph Changes	✅ Passed	The changed file src/stores/subgraphNavigationStore.ts is a Vue/Pinia state management store located in src/stores/, which falls outside the ADR compliance check scope that applies only to src/lib/litegraph/, src/ecs/, and graph entity files.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

🎨 Storybook: Building...

[github-actions]

🎭 Playwright: ⏳ Running...

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/stores/subgraphNavigationStore.ts (1)

144-144: Redundant variable assignment.

expectedGraphId is identical to graphId. The parameter is a const and doesn't change within the function scope.

♻️ Suggested fix

-      const expectedGraphId = graphId
       requestAnimationFrame(() => {
-        if (getActiveGraphId() !== expectedGraphId) return
+        if (getActiveGraphId() !== graphId) return

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/stores/subgraphNavigationStore.ts` at line 144, The variable
expectedGraphId is a redundant alias of the parameter graphId in the function
(remove the unnecessary assignment). Replace usages of expectedGraphId with
graphId and delete the const expectedGraphId = graphId line (locate the
assignment and any references to expectedGraphId in the same function or scope
and update them to use graphId directly, e.g., inside methods that reference
expectedGraphId).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/stores/subgraphNavigationStore.ts`:
- Around line 149-150: The code directly accesses canvas.graph._nodes; replace
that with the public getter canvas.graph.nodes to avoid touching internal
properties—locate the usage in subgraphNavigationStore where nodes is assigned
(currently using canvas.graph._nodes) and change it to use canvas.graph.nodes so
it matches other modules like appModeStore.ts and litegraphUtil.ts.

---

Nitpick comments:
In `@src/stores/subgraphNavigationStore.ts`:
- Line 144: The variable expectedGraphId is a redundant alias of the parameter
graphId in the function (remove the unnecessary assignment). Replace usages of
expectedGraphId with graphId and delete the const expectedGraphId = graphId line
(locate the assignment and any references to expectedGraphId in the same
function or scope and update them to use graphId directly, e.g., inside methods
that reference expectedGraphId).

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 44eb8fac-5f8a-418a-892e-631837f630f0

📥 Commits

Reviewing files that changed from the base of the PR and between d946694 and 650834d.

📒 Files selected for processing (1)

• src/stores/subgraphNavigationStore.ts