nandi/frq · 4925e54

nandi/frqpublic⑂ Fork 0

⑂ 4925e54

The arrow goes to the message

A reply chip's arrow highlighted the message it points at and left the
view where it was. The core has marked that row with `scrollHere` since
the port, and `Jump to present` has counted up a `scrollToBottom` tick
for just as long; the renderer read neither prop. Both are handled now.

`scrollHere` needs a built element — `ensureVisible` cannot run during
the build that asks for it — so the row carries a GlobalKey and the
scroll happens after the frame. Two keys on one widget is not a thing,
so they nest: the ValueKey keeps the element across rebuilds and the
GlobalKey is how the frame finds it afterwards. It lands a fifth of the
way down rather than against the ceiling, so the answered message is
read with what came after it.

The renderer then tells the core it has arrived, and the core takes the
target off. A `jumpTo` left set would scroll back to that row on every
frame, which is scrolling taken away from the reader — the highlight
stays, since that is the part that says "this is the one you asked for".

`scrollToBottom` is a tick rather than a flag for the same reason: a
count that goes up needs nobody to clear it. A reverse scroll holds the
present at offset zero, which is why jumping to it is not
maxScrollExtent.

Both tests took a correction. The first asserted the offset moved with
the view already at the present, where the target is on screen and
`ensureVisible` is right to do nothing; and a single timed pump advances
the clock without running the scroll animation out, so the offset came
back unchanged as though the handler were missing. `pumpAndSettle`, from
a position away from the target.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

nandi committed 2026-09-19T17:56:49-07:00 Browse files

4925e54 parent: 91b0e1e

modified flutter/lib/nim_renderer.dart +56 -4 Viewed

@@ -60,6 +60,19 @@ class _NimAppState extends State<NimApp> {
60	60	// switching rooms is a different backlog at a different offset.
61	61	final _scrollers = <String, ScrollController>{};
62	62
	63	+ // Where a "go to that message" is pointing, for the one frame it is
	64	+ // pointing there. The core marks the row with `scrollHere`, this finds it
	65	+ // after the frame is laid out — `ensureVisible` needs a built element, so
	66	+ // it cannot happen during the build that asks for it — and then tells the
	67	+ // core it has arrived, which takes the mark off. Leaving it on would pin
	68	+ // the view to that row and take scrolling away from the reader.
	69	+ final _jumpKey = GlobalKey();
	70	+
	71	+ // The last `scrollToBottom` tick acted on, per scroll. The core counts up
	72	+ // when "Jump to present" is pressed; an unchanged count is a frame where
	73	+ // nobody asked to be moved.
	74	+ final _bottomTicks = <String, int>{};
	75	+
63	76	@override
64	77	void initState() {
65	78	super.initState();
@@ -230,7 +243,28 @@ class _NimAppState extends State<NimApp> {
230	243	// holds per element is at stake: text controllers, scroll offsets, and
231	244	// the selectables a live text selection is made of.
232	245	final key = n.prop('key', '');
233		- final w = _buildNode(n, axis);
	246	+ var w = _buildNode(n, axis);
	247	+
	248	+ // The row a reply's arrow is aiming at. Two keys on one widget is not a
	249	+ // thing, so they nest: the ValueKey keeps the element across rebuilds,
	250	+ // and the GlobalKey is how this frame finds it afterwards.
	251	+ if (n.prop('scrollHere', false)) {
	252	+ w = KeyedSubtree(key: _jumpKey, child: w);
	253	+ WidgetsBinding.instance.addPostFrameCallback((_) {
	254	+ final ctx = _jumpKey.currentContext;
	255	+ if (ctx == null \|\| !mounted) return;
	256	+ Scrollable.ensureVisible(
	257	+ ctx,
	258	+ duration: const Duration(milliseconds: 250),
	259	+ curve: Curves.easeOut,
	260	+ // A little down from the top, so the message that was replied to
	261	+ // is read with what came after it rather than alone against the
	262	+ // ceiling.
	263	+ alignment: 0.2,
	264	+ );
	265	+ _send('jump.done');
	266	+ });
	267	+ }
234	268	return key.isEmpty ? w : KeyedSubtree(key: ValueKey(key), child: w);
235	269	}
236	270
@@ -640,12 +674,30 @@ class _NimAppState extends State<NimApp> {
640	674	case 'scroll':
641	675	{
642	676	// Both ends of the same scroll, named so they are the same one.
643		- final c = _scrollers.putIfAbsent(
644		- n.prop('scrollKey', 'scroll'), ScrollController.new);
	677	+ final scrollKey = n.prop('scrollKey', 'scroll');
	678	+ final c = _scrollers.putIfAbsent(scrollKey, ScrollController.new);
	679	+ final stick = n.prop('stickToBottom', false);
	680	+
	681	+ // "Jump to present": a tick that goes up, rather than a flag that
	682	+ // would have to be cleared. A reverse scroll holds the present at
	683	+ // offset zero, which is why this is not maxScrollExtent.
	684	+ final tick = n.prop('scrollToBottom', 0);
	685	+ if (_bottomTicks[scrollKey] != tick) {
	686	+ _bottomTicks[scrollKey] = tick;
	687	+ WidgetsBinding.instance.addPostFrameCallback((_) {
	688	+ if (!c.hasClients) return;
	689	+ c.animateTo(
	690	+ stick ? c.position.minScrollExtent
	691	+ : c.position.maxScrollExtent,
	692	+ duration: const Duration(milliseconds: 250),
	693	+ curve: Curves.easeOut,
	694	+ );
	695	+ });
	696	+ }
645	697	Widget body = SingleChildScrollView(
646	698	controller: c,
647	699	// The backlog reads from the bottom; a settings list from the top.
648		- reverse: n.prop('stickToBottom', false),
	700	+ reverse: stick,
649	701	child: Column(
650	702	crossAxisAlignment: CrossAxisAlignment.start,
651	703	children: _spaced(kids, spacing, vertical: true)),

    // switching rooms is a different backlog at a different offset.
    final _scrollers = <String, ScrollController>{};
++  // Where a "go to that message" is pointing, for the one frame it is
++  // pointing there. The core marks the row with `scrollHere`, this finds it
++  // after the frame is laid out — `ensureVisible` needs a built element, so
++  // it cannot happen during the build that asks for it — and then tells the
++  // core it has arrived, which takes the mark off. Leaving it on would pin
++  // the view to that row and take scrolling away from the reader.
++  final _jumpKey = GlobalKey();
++
++  // The last `scrollToBottom` tick acted on, per scroll. The core counts up
++  // when "Jump to present" is pressed; an unchanged count is a frame where
++  // nobody asked to be moved.
++  final _bottomTicks = <String, int>{};
++
    @override
    void initState() {
      super.initState();
      // holds per element is at stake: text controllers, scroll offsets, and
      // the selectables a live text selection is made of.
      final key = n.prop('key', '');
--    final w = _buildNode(n, axis);
++    var w = _buildNode(n, axis);
++
++    // The row a reply's arrow is aiming at. Two keys on one widget is not a
++    // thing, so they nest: the ValueKey keeps the element across rebuilds,
++    // and the GlobalKey is how this frame finds it afterwards.
++    if (n.prop('scrollHere', false)) {
++      w = KeyedSubtree(key: _jumpKey, child: w);
++      WidgetsBinding.instance.addPostFrameCallback((_) {
++        final ctx = _jumpKey.currentContext;
++        if (ctx == null || !mounted) return;
++        Scrollable.ensureVisible(
++          ctx,
++          duration: const Duration(milliseconds: 250),
++          curve: Curves.easeOut,
++          // A little down from the top, so the message that was replied to
++          // is read with what came after it rather than alone against the
++          // ceiling.
++          alignment: 0.2,
++        );
++        _send('jump.done');
++      });
++    }
      return key.isEmpty ? w : KeyedSubtree(key: ValueKey(key), child: w);
    }
        case 'scroll':
          {
            // Both ends of the same scroll, named so they are the same one.
--          final c = _scrollers.putIfAbsent(
++          final scrollKey = n.prop('scrollKey', 'scroll');
--              n.prop('scrollKey', 'scroll'), ScrollController.new);
++          final c = _scrollers.putIfAbsent(scrollKey, ScrollController.new);
++          final stick = n.prop('stickToBottom', false);
++
++          // "Jump to present": a tick that goes up, rather than a flag that
++          // would have to be cleared. A reverse scroll holds the present at
++          // offset zero, which is why this is not maxScrollExtent.
++          final tick = n.prop('scrollToBottom', 0);
++          if (_bottomTicks[scrollKey] != tick) {
++            _bottomTicks[scrollKey] = tick;
++            WidgetsBinding.instance.addPostFrameCallback((_) {
++              if (!c.hasClients) return;
++              c.animateTo(
++                stick ? c.position.minScrollExtent
++                      : c.position.maxScrollExtent,
++                duration: const Duration(milliseconds: 250),
++                curve: Curves.easeOut,
++              );
++            });
++          }
            Widget body = SingleChildScrollView(
              controller: c,
              // The backlog reads from the bottom; a settings list from the top.
--            reverse: n.prop('stickToBottom', false),
++            reverse: stick,
              child: Column(
                  crossAxisAlignment: CrossAxisAlignment.start,
                  children: _spaced(kids, spacing, vertical: true)),

modified flutter/test/nim_layout_test.dart +45 -0 Viewed

@@ -241,6 +241,51 @@ void main() {
241	241	});
242	242	});
243	243
	244	+ group('going to a message', () {
	245	+ // The core has always marked the row a reply points at with
	246	+ // `scrollHere`, and the renderer ignored the prop — so the arrow on a
	247	+ // reply chip highlighted the message and left the view where it was.
	248	+ testWidgets('the target is scrolled into view', (tester) async {
	249	+ core.demoUi();
	250	+ // Short enough that the backlog does not fit, or a scroll has nothing
	251	+ // to do and the target is already on screen — which is what the first
	252	+ // version of this test proved: the offset stayed at zero because
	253	+ // `ensureVisible` was right not to move.
	254	+ await layOut(tester, const Size(700, 260));
	255	+ final c = tester
	256	+ .widget<Scrollable>(find.byType(Scrollable).first)
	257	+ .controller!;
	258	+
	259	+ // Away from the present, where the answered message is not.
	260	+ c.jumpTo(c.position.maxScrollExtent);
	261	+ await tester.pump();
	262	+ final before = c.offset;
	263	+ expect(before, greaterThan(0.0), reason: 'nothing to scroll here');
	264	+
	265	+ core.dispatch('goto:5');
	266	+ await tester.pump(const Duration(milliseconds: 150));
	267	+ // Settled, not a timed pump: one pump of 400ms advances the clock but
	268	+ // does not run the scroll animation out, and the offset comes back
	269	+ // unchanged as though nothing had happened.
	270	+ await tester.pumpAndSettle();
	271	+ expectLaidOut(tester, 'the backlog after going to a message');
	272	+ expect(c.offset, lessThan(before), reason: 'the view did not move');
	273	+ });
	274	+
	275	+ testWidgets('and the core is told, so the view is not pinned there',
	276	+ (tester) async {
	277	+ // A `jumpTo` left set would scroll back to that row on every frame,
	278	+ // which is scrolling taken away from the reader.
	279	+ core.demoUi();
	280	+ await layOut(tester, const Size(700, 260));
	281	+ await tester.tap(find.text('→').first);
	282	+ await tester.pumpAndSettle();
	283	+ expectLaidOut(tester, 'the backlog after the arrow');
	284	+ expect(core.dispatchFrame('noop').json.contains('"scrollHere":true'),
	285	+ isFalse, reason: 'the core still thinks it has somewhere to go');
	286	+ });
	287	+ });
	288	+
244	289	group('identity', () {
245	290	// Duplicate keys among siblings are an error Flutter throws at build
246	291	// time, so this is mostly a guard on the tree the core emits: every

      });
    });
++  group('going to a message', () {
++    // The core has always marked the row a reply points at with
++    // `scrollHere`, and the renderer ignored the prop — so the arrow on a
++    // reply chip highlighted the message and left the view where it was.
++    testWidgets('the target is scrolled into view', (tester) async {
++      core.demoUi();
++      // Short enough that the backlog does not fit, or a scroll has nothing
++      // to do and the target is already on screen — which is what the first
++      // version of this test proved: the offset stayed at zero because
++      // `ensureVisible` was right not to move.
++      await layOut(tester, const Size(700, 260));
++      final c = tester
++          .widget<Scrollable>(find.byType(Scrollable).first)
++          .controller!;
++
++      // Away from the present, where the answered message is not.
++      c.jumpTo(c.position.maxScrollExtent);
++      await tester.pump();
++      final before = c.offset;
++      expect(before, greaterThan(0.0), reason: 'nothing to scroll here');
++
++      core.dispatch('goto:5');
++      await tester.pump(const Duration(milliseconds: 150));
++      // Settled, not a timed pump: one pump of 400ms advances the clock but
++      // does not run the scroll animation out, and the offset comes back
++      // unchanged as though nothing had happened.
++      await tester.pumpAndSettle();
++      expectLaidOut(tester, 'the backlog after going to a message');
++      expect(c.offset, lessThan(before), reason: 'the view did not move');
++    });
++
++    testWidgets('and the core is told, so the view is not pinned there',
++        (tester) async {
++      // A `jumpTo` left set would scroll back to that row on every frame,
++      // which is scrolling taken away from the reader.
++      core.demoUi();
++      await layOut(tester, const Size(700, 260));
++      await tester.tap(find.text('→').first);
++      await tester.pumpAndSettle();
++      expectLaidOut(tester, 'the backlog after the arrow');
++      expect(core.dispatchFrame('noop').json.contains('"scrollHere":true'),
++          isFalse, reason: 'the core still thinks it has somewhere to go');
++    });
++  });
++
    group('identity', () {
      // Duplicate keys among siblings are an error Flutter throws at build
      // time, so this is mostly a guard on the tree the core emits: every

modified nim/src/frq/reducer.nim +7 -0 Viewed

@@ -503,6 +503,13 @@ proc dispatch*(event: JsonNode) =
503	503	app.jumpTo = arg
504	504	app.highlight = arg
505	505
	506	+ of "jump.done":
	507	+ # The renderer, saying it has scrolled there. The target is taken off
	508	+ # again because a `jumpTo` that stayed set would pin the view to that
	509	+ # message and take scrolling away from the reader — the highlight stays,
	510	+ # since that is what says "this is the one you asked for".
	511	+ app.jumpTo = ""
	512	+
506	513	of "overview.goto":
507	514	# The overview is the one place that moves the reader without their having
508	515	# asked to leave where they were, so it is the one place that owes them

      app.jumpTo = arg
      app.highlight = arg
++  of "jump.done":
++    # The renderer, saying it has scrolled there. The target is taken off
++    # again because a `jumpTo` that stayed set would pin the view to that
++    # message and take scrolling away from the reader — the highlight stays,
++    # since that is what says "this is the one you asked for".
++    app.jumpTo = ""
++
    of "overview.goto":
      # The overview is the one place that moves the reader without their having
      # asked to leave where they were, so it is the one place that owes them