[BUGFIX] @model becomes undefined or changes to the wrong route's model during Glimmer component willDestroy

[johanrd]

Summary

• Fixes a long-standing issue Route @model argument value timing issue #18987 where @model becomes undefined (or changes to the wrong route's model) during Glimmer component willDestroy when transitioning between routes
• Adds a controller identity check alongside the existing lastState === state guard in the outlet helper's model compute ref
• Adds a smoke test in the scenario test suite (based on Add a test that verifies @model is stable between route transitions #20959 by @Windvis)

Details

The @model reference in route templates is built from a chain of compute refs that read from the shared outlet state tree via dynamic scope. When transitioning, _setOutlets() rebuilds this tree, and all refs in the chain immediately see the new state.

The existing guard (lastState === state) freezes the model value when the outlet changes at the same level. But when a parent outlet is torn down first, the child outlet's outer compute ref never re-evaluates, so lastState is never updated. The guard passes, andmodelRef reads from the wrong outlet state.

The fix compares the current outlet's controller identity against the expected one. Since each route has a unique controller singleton, this detects when the outletRef has been redirected — even when lastState hasn't been updated yet.

From git history, the bug may have existed since @model was introduced in v3.14.0 (16b74a5). The original implementation read directly from the outletRef chain with no guard at all. The lastState === state guard added in v4.0.0 (commit 7d334cf) partially mitigated same-level transitions but did not catch parent-level teardown.

Fixes #18987
Supersedes #20959

Test plan

• Smoke test in smoke-tests/scenarios/basic-test.ts: transitions through sibling, parent, cousin, and unrelated routes, verifying this.args.model in willDestroy Based on Add a test that verifies @model is stable between route transitions #20959 but moved to smoke-test to avoid the vite.config import of glimmer-component
• Full internal test suite passes (5907/5907)
• Scenario tests pass for both v1 (classic/embroider-webpack) and v2 (embroider-vite) app builds

Before the fix, the added smoke tests fail with:

not ok 1 - Acceptance | @model stability during route transitions: @model should be stable when transitioning out of the route                                                                                                                                                                                                            
      actual:   b,b,d,e,
      expected: b,b,b,b,b
      message:  The @model value should remain stable in willDestroy for all transition types

ember-source	Result	Notes
workspace:* (with fix)	PASS	Local build includes the bugfix
~6..11.0	FAIL	Last version
~5.12.0	FAIL	Last 5.x
~4.0.0	FAIL	First 4.x
~3.28.0	FAIL	Last 3.x

Cowritten by claude

[ef4]

I think something along this vein is the real fix:

Suggested change

	let modelRef = childRefFromParts(named['controller'], ['render', 'model']);

The modelRef should only depend on the constant controller, not the variable reference to the controller.

[johanrd]

@ef4 Thanks, tried now (if I understood correctly): I get two test failures with something like that: https://github.com/johanrd/ember.js/actions/runs/22914352893/job/66495351782?pr=11

I guess we can be okay with @model tracking controller.model mutations? If yes, we may update the tests (bugfix, or RFC)?

[NullVoxPopuli]

I got 33 failures https://github.com/emberjs/ember.js/actions/runs/22912061404/job/66486942647?pr=21215 🙈 ope

Concerns over @model updating during same-route transitions

[johanrd]

Concerns over @model updating during same-route transitions

@NullVoxPopuli tests are added for this now, but they could maybe be more specific?

[BoussonKarel]

We are also running into issues with @model being undefined during transitions.
We use template tags for all our routes, and thus we use @model for all our routes.

So this PR would be greatly appreciated :)

[NullVoxPopuli]

In a 2.7 million line app, we have 27818 tests... that passed

boring exploration, fumbling my package manager

As a point of (in)confidence, I tried this out on the 3.5 million line project at work, and got a ton of test failures 🤔
lots of

Cannot read properties of undefined (reading 'lookup')

(I don't have the app at work on the latest canary tho ... I should try that out as a baseline)

update: happens on main, so I need to figure out what's going on here
update2: looks like ember-simple-auth problem -- perhaps they don't test against canary. time for a PR over there!
update3: pnpm bug is creating duplicate ember-source's so I have multiple get/set owner pairs :(
update4: got big app against ember-source@main passing -- will try this branch tomorrow

[NullVoxPopuli]

Unless someone can (with code) prove the fix is wrong, I think I'm in favor of this moving forward. The biggest-app-I-have-access-to's test suite passed on this PR, so at the least there is no regression.

[johanrd]

@NullVoxPopuli ok, thanks a lot for running such a thorough test on the pr!

Just to not fully leave the the @ef4-inspired solution:

- let modelRef = childRefFromParts(outletRef, ['render', 'model']);
+ let modelRef = childRefFromParts(named['controller'], ['model']);

This only fails on the tests that check that @model should not rerender external mutations to the model via the controller:

ember.js/packages/@ember/-internals/glimmer/tests/integration/application/rendering-test.js

Lines 316 to 327 in 6c66561

	runTask(() => {
	this.controllerFor('color').model = 'blue';
	});
	assert.equal(this.currentURL, '/yellow');
	this.assertInnerHTML(strip`
	[@model: yellow]
	[this.model: blue]
	[model: blue]
	`);
	}

@model becomes more in sync with this.model (both change to blue, even if the route url still is still yellow):

        actual: >
            [@model: blue][this.model: blue][model: blue]
        expected: >
            [@model: yellow][this.model: blue][model: blue]

The current premise is from the RFC:

Just like the "reflected" named arguments in classic components, mutating this.model on the controller instance would not change the value of @model. In practice, this seems unlikely to be relied upon and probably considered a bad practice (it does not change the URL, does not affect what is returned by route.modelFor, etc). In any case, this is consistent with the general behavior for named arguments, in that they are immutable and should always reflect what was "passed in" from the caller.

I am not sure what is the real fix in the long term – changing it may require RFC, though

[kategengler]

I'd like @ef4 to re-review before any merge

[mansona]

So just to let you know we are about to open a draft PR with the Route Manager RFC implementation. I would recommend adding this test to that PR to double check if the problem still persists 🤔 I suspect that @ef4 's suggestion would be much clearer once we see the clear boundary between the classic route manager and the actual router

[johanrd]

@mansona checked now, both the tests and the fix seems to be included in #21460 it seems👍

[johanrd]

double check if the problem still persists

The problem persists in itself (but is fixed because the fix has been applied to #21460)

Tested #21460 now, but with restoring the logic to the current ember-7.0.0 bevavior:

   wrapperArgs['model'] = createComputeRef(() => {
     if (lastState === state) {
-      let currentOutlet = valueForRef(outletRef);
-      if (currentOutlet?.render?.controller === controller) {
-        model = valueForRef(modelRef);
-      }
+      model = valueForRef(modelRef);
     }
     return model;
   });

Test	Expected	#21460	#21460 with guard removed
@model stable when transitioning out of the route	b,b,b,b,b	✅ b,b,b,b,b	❌️ b,b,d,e, log
@model updates when the model changes on the same route	third	✅ third	✅ third
@model stable when grandparent outlet tears down	i	✅ i	❌️ (undefined) log

[evoactivity]

I would merge this.

I already pulled it's tests into the route manager work, bart has done a bunch of work there to change how outlets work, so if those tests are still passing on that branch this fix will be superseded by that work, but no reason to wait for the route manager stuff to land to get this fix in place now.