From d0e169eba27f1d48cdaa5ab8e9165c2ca6df85af Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Sun, 31 May 2026 16:16:55 +0200 Subject: [PATCH] open the decision reader without re-contributing its tab MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The decisions extension tore down and re-contributed the decisions.detail context-panel tab on every selection, racing the view's own subscription and leaving the panel unrevealed — so clicking a decision often did nothing. Match the working ticket panel: contribute the tab once (static), and on selection just reveal the context panel and activateTab; DecisionDetailView loads via its existing subscription. T-188. Co-Authored-By: Claude --- CHANGELOG.md | 4 + lib/builtin/decisions/src/extension.dart | 11 +- .../decisions/decision_reader_test.dart | 320 ++++++++++++++++++ 3 files changed, 325 insertions(+), 10 deletions(-) create mode 100644 test/builtin/decisions/decision_reader_test.dart diff --git a/CHANGELOG.md b/CHANGELOG.md index ed3a1ed1..df94c62b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,10 @@ heading, and (b) bumping `pubspec.yaml` `version:` in the same commit. ### Fixed +- Clicking a decision opens it in the decision reader again — the decisions + extension no longer tears down and re-contributes its panel tab on every + selection; it activates a static tab and reveals the panel like the ticket + panel. (T-188) - Clicking a markdown file opens it in the right-side markdown reader again — the files panel, the Claude Config tab, and wiki `.md` links now publish to the reader instead of the editor. (T-187) diff --git a/lib/builtin/decisions/src/extension.dart b/lib/builtin/decisions/src/extension.dart index fc20e653..3cf9ea81 100644 --- a/lib/builtin/decisions/src/extension.dart +++ b/lib/builtin/decisions/src/extension.dart @@ -21,16 +21,7 @@ class DecisionsExtension extends ClideExtension { @override Future activate(ClideExtensionContext ctx) async { _sub = ctx.messages.subscribe(publisher: id, channel: 'selection').listen((msg) { - final selectedId = msg.data['id'] as String?; - if (selectedId == null) return; - ctx.panels.uncontribute('decisions.detail'); - ctx.panels.contribute(TabContribution( - id: 'decisions.detail', - slot: Slots.contextPanel, - title: 'Decision', - icon: PhosphorIcons.lightbulb, - build: (_) => DecisionDetailView(initialId: selectedId), - )); + if (msg.data['id'] is! String) return; ctx.arrangement.setVisible(Slots.contextPanel, true); ctx.arrangement.setCollapsed(Slots.contextPanel, false); ctx.panels.activateTab(Slots.contextPanel, 'decisions.detail'); diff --git a/test/builtin/decisions/decision_reader_test.dart b/test/builtin/decisions/decision_reader_test.dart new file mode 100644 index 00000000..3164a0e0 --- /dev/null +++ b/test/builtin/decisions/decision_reader_test.dart @@ -0,0 +1,320 @@ +/// T-188: Decision reader — static-tab fix. +/// +/// Verifies that clicking a decision opens it in the static `decisions.detail` +/// context-panel tab (no per-click uncontribute/contribute churn), that a +/// second click switches the content, that clicking the same id twice is +/// idempotent, and that rapid sequential selections all resolve. +/// +/// Extension-level tests use [KernelFixture] + the real [ExtensionManager]. +/// Widget-level tests use the [harness] wrapper to pump [DecisionDetailView] +/// and assert IPC-stub + message-bus wiring. +library; + +import 'package:clide/builtin/decisions/src/decision_detail_view.dart'; +import 'package:clide/builtin/decisions/src/extension.dart'; +import 'package:clide/extension/extension.dart' show LayoutPresetContribution, LayoutSlot; +import 'package:clide/clide.dart'; +import 'package:clide/kernel/kernel.dart'; +import 'package:flutter/widgets.dart'; +import 'package:flutter_test/flutter_test.dart'; + +import '../../helpers/kernel_fixture.dart'; +import '../../helpers/widget_harness.dart'; + +// --------------------------------------------------------------------------- +// Helpers +// --------------------------------------------------------------------------- + +/// Registers and activates [DecisionsExtension] against [f], and seeds the +/// [Slots.contextPanel] slot into the arrangement so that +/// [setVisible]/[setCollapsed] have a state entry to mutate. +Future _bootExtension(KernelFixture f) async { + f.services.panels.registerSlot( + const SlotDefinition( + id: Slots.contextPanel, + position: SlotPosition.right, + ), + ); + // Seed the ARRANGEMENT with the context-panel slot (visible:false) so the + // extension's setVisible/setCollapsed reveal actually round-trips — in + // production the default-layout preset does this; LayoutArrangement.setVisible + // is a no-op for a slot it has no state entry for. + f.services.arrangement.applyPreset( + const LayoutPresetContribution( + id: 'test.preset', + displayName: 'test', + slots: [ + LayoutSlot(slot: Slots.contextPanel, position: SlotPosition.right, visible: false), + ], + ), + ); + f.services.extensions.register(DecisionsExtension()); + await f.services.extensions.activate('builtin.decisions'); +} + +/// Publishes a `builtin.decisions / selection` message on [f]'s bus. +void _select(KernelFixture f, String id) { + f.services.messages.publish('builtin.decisions', 'selection', {'id': id}); +} + +// --------------------------------------------------------------------------- +// IPC stub: pql.decisions.read +// --------------------------------------------------------------------------- + +IpcResponse _decisionResponse(String id) => IpcResponse.ok( + id: '', + data: { + 'id': id, + 'title': 'Decision $id', + 'type': 'confirmed', + 'domain': 'architecture', + 'status': 'active', + 'date': '2026-01-01', + 'body': 'Body of $id.', + 'refs': [], + }, + ); + +// --------------------------------------------------------------------------- +// Extension-level unit tests (no Flutter widgets, no IPC) +// --------------------------------------------------------------------------- + +void main() { + group('DecisionsExtension — static-tab wiring (T-188)', () { + late KernelFixture f; + + setUp(() async { + f = await KernelFixture.create(); + await _bootExtension(f); + }); + tearDown(() => f.dispose()); + + test('activate contributes decisions.detail as a static tab', () { + final tabs = f.services.panels.tabsFor(Slots.contextPanel); + expect(tabs.any((t) => t.id == 'decisions.detail'), isTrue, reason: 'decisions.detail must be contributed once at activate'); + }); + + test('decisions.detail tab count stays at 1 after multiple selections', () async { + _select(f, 'D-1'); + await Future.delayed(Duration.zero); + _select(f, 'D-2'); + await Future.delayed(Duration.zero); + + final tabs = f.services.panels.tabsFor(Slots.contextPanel); + expect(tabs.where((t) => t.id == 'decisions.detail').length, 1, reason: 'no per-click re-contribution — exactly one decisions.detail tab'); + }); + + test('selection activates decisions.detail tab', () async { + _select(f, 'D-1'); + await Future.delayed(Duration.zero); + + expect(f.services.panels.activeTabIn(Slots.contextPanel), 'decisions.detail'); + }); + + test('second selection switches to decisions.detail (already active, stays)', () async { + _select(f, 'D-1'); + await Future.delayed(Duration.zero); + _select(f, 'D-2'); + await Future.delayed(Duration.zero); + + expect(f.services.panels.activeTabIn(Slots.contextPanel), 'decisions.detail'); + }); + + test('clicking the same decision twice leaves decisions.detail active', () async { + _select(f, 'D-5'); + await Future.delayed(Duration.zero); + _select(f, 'D-5'); + await Future.delayed(Duration.zero); + + expect(f.services.panels.activeTabIn(Slots.contextPanel), 'decisions.detail'); + expect(f.services.panels.tabsFor(Slots.contextPanel).where((t) => t.id == 'decisions.detail').length, 1); + }); + + test('selection reveals the context panel', () async { + // The panel starts with visible=false for this slot (no preset applied, + // but registerSlot sets the default state; setVisible is a no-op when + // visible is already true, so we flip it first). + f.services.arrangement.setVisible(Slots.contextPanel, false); + f.services.arrangement.setCollapsed(Slots.contextPanel, true); + + _select(f, 'D-3'); + await Future.delayed(Duration.zero); + + expect(f.services.arrangement.isVisible(Slots.contextPanel), isTrue, reason: 'panel must be made visible on selection'); + expect(f.services.arrangement.isCollapsed(Slots.contextPanel), isFalse, reason: 'panel must be un-collapsed on selection'); + }); + + test('rapid sequential selections all leave decisions.detail active', () async { + for (var i = 1; i <= 10; i++) { + _select(f, 'D-$i'); + } + await Future.delayed(Duration.zero); + + expect(f.services.panels.activeTabIn(Slots.contextPanel), 'decisions.detail'); + expect(f.services.panels.tabsFor(Slots.contextPanel).where((t) => t.id == 'decisions.detail').length, 1); + }); + + test('null id in selection message is ignored', () async { + // Seed a valid tab selection first. + _select(f, 'D-1'); + await Future.delayed(Duration.zero); + + // Then send a bad message. + f.services.messages.publish('builtin.decisions', 'selection', {'id': null}); + await Future.delayed(Duration.zero); + + // Tab still active, still only one. + expect(f.services.panels.activeTabIn(Slots.contextPanel), 'decisions.detail'); + expect(f.services.panels.tabsFor(Slots.contextPanel).where((t) => t.id == 'decisions.detail').length, 1); + }); + + test('deactivate cancels subscription — selections after deactivate are no-ops', () async { + await f.services.extensions.deactivate('builtin.decisions'); + + // After deactivation contributions are removed, so no decisions.detail + // tab at all — but the panel activation path must not fire either. + f.services.panels.registerSlot( + const SlotDefinition( + id: Slots.contextPanel, + position: SlotPosition.right, + ), + ); + f.services.messages.publish('builtin.decisions', 'selection', {'id': 'D-99'}); + await Future.delayed(Duration.zero); + + expect(f.services.panels.activeTabIn(Slots.contextPanel), isNot('decisions.detail'), + reason: 'deactivated extension must not respond to selection messages'); + }); + }); + + // ------------------------------------------------------------------------- + // Widget tests for DecisionDetailView + // ------------------------------------------------------------------------- + + group('DecisionDetailView — widget (T-188)', () { + late KernelFixture f; + + setUp(() async { + f = await KernelFixture.create(); + // Stub the IPC call made by _load(). + f.ipc.stub('pql.decisions.read', (args) async { + final id = args['id'] as String? ?? 'unknown'; + return _decisionResponse(id); + }); + }); + tearDown(() => f.dispose()); + + Future pumpView(WidgetTester tester, {String? initialId}) async { + tester.view.physicalSize = const Size(600, 800); + tester.view.devicePixelRatio = 1.0; + addTearDown(() { + tester.view.resetPhysicalSize(); + tester.view.resetDevicePixelRatio(); + }); + await tester.pumpWidget( + harness(f, DecisionDetailView(initialId: initialId)), + ); + await pumpAsync(tester); + } + + testWidgets('shows placeholder when no decision selected', (tester) async { + await pumpView(tester); + expect(find.text('Select a decision to view details.'), findsOneWidget); + }); + + testWidgets('loads and renders initialId on first mount', (tester) async { + await pumpView(tester, initialId: 'D-1'); + expect(find.text('D-1'), findsOneWidget); + expect(find.text('Decision D-1'), findsOneWidget); + }); + + testWidgets('selection message loads a decision into the view', (tester) async { + await pumpView(tester); + // Starts empty. + expect(find.text('Select a decision to view details.'), findsOneWidget); + + // Publish a selection. + f.services.messages.publish('builtin.decisions', 'selection', {'id': 'D-7'}); + // Give the broadcast stream a microtask to deliver. + await pumpAsync(tester); + + expect(find.text('D-7'), findsOneWidget); + expect(find.text('Decision D-7'), findsOneWidget); + }); + + testWidgets('second selection switches the displayed decision', (tester) async { + await pumpView(tester, initialId: 'D-1'); + expect(find.text('Decision D-1'), findsOneWidget); + + f.services.messages.publish('builtin.decisions', 'selection', {'id': 'D-2'}); + await pumpAsync(tester); + + expect(find.text('Decision D-2'), findsOneWidget); + expect(find.text('Decision D-1'), findsNothing); + }); + + testWidgets('clicking the same decision twice leaves view stable', (tester) async { + await pumpView(tester, initialId: 'D-5'); + expect(find.text('Decision D-5'), findsOneWidget); + + f.services.messages.publish('builtin.decisions', 'selection', {'id': 'D-5'}); + await pumpAsync(tester); + + // Still shows D-5, no crash, no duplicate. + expect(find.text('Decision D-5'), findsOneWidget); + }); + + testWidgets('rapid sequential selections resolve to the last one', (tester) async { + await pumpView(tester); + + for (var i = 1; i <= 5; i++) { + f.services.messages.publish('builtin.decisions', 'selection', {'id': 'D-$i'}); + } + await pumpAsync(tester); + + // The last resolved data should be for D-5 (stub is synchronous so + // each load completes before the next, but all 5 fire in order). + expect(find.text('Decision D-5'), findsOneWidget); + }); + + testWidgets('cross-reference tap publishes a new selection', (tester) async { + // Stub: D-10 has a ref pointing to D-11. + f.ipc.stub('pql.decisions.read', (args) async { + final id = args['id'] as String? ?? 'unknown'; + if (id == 'D-10') { + return IpcResponse.ok( + id: '', + data: { + 'id': 'D-10', + 'title': 'Decision D-10', + 'type': 'confirmed', + 'domain': 'architecture', + 'status': 'active', + 'date': '2026-01-01', + 'body': '', + 'refs': [ + {'target_id': 'D-11', 'ref_type': 'implements'}, + ], + }, + ); + } + return _decisionResponse(id); + }); + + await pumpView(tester, initialId: 'D-10'); + expect(find.text('D-10'), findsOneWidget); + expect(find.text('D-11'), findsOneWidget); + + // Tap the ref card — it should publish selection for D-11. + Message? received; + final sub = f.services.messages.subscribe(publisher: 'builtin.decisions', channel: 'selection').listen((m) => received = m); + addTearDown(sub.cancel); + + await tester.tap(find.text('D-11').first); + await pumpAsync(tester); + + expect(received, isNotNull); + expect(received!.data['id'], 'D-11'); + }); + }); +}