Skip to content

fix(explore): open the skill before closing the ☰ menu#125

Merged
jrosskopf merged 1 commit into
mainfrom
fix/skills-menu-async-nav
May 30, 2026
Merged

fix(explore): open the skill before closing the ☰ menu#125
jrosskopf merged 1 commit into
mainfrom
fix/skills-menu-async-nav

Conversation

@jrosskopf
Copy link
Copy Markdown
Contributor

Summary

Clicking a skill in the ☰ skills registry did nothing on the live (HTTP) gateway — the menu closed but the skill page never opened, with only an opaque release-mode Uncaught Error in the console. Every widget test passed, so the regression was invisible to the suite.

Cause. _SkillsPanel._open was close(); focusSkill(ref, skillId);. focusSkill is async — it awaits a real resolve round-trip, then navigateToInstance(ref, …). But close() tore down the OverlayPortal that owns ref first, so the post-await ref.read hit a defunct element and threw; navigation never ran. The fixture's microtask-fast resolve masked it locally; the gateway's network latency made it deterministic.

Fix. Await focusSkill before close(), so the overlay's ref is still mounted when navigateToInstance runs.

Verified live via chrome-devtools against the seeded gateway on :8080: with the clean release bundle the ☰ → skill click now opens and renders the skill manifest (frontmatter + body), minimizes the event pane, and shows the back button.

Test plan

  • test/crm/skill_page_render_test.dart (new, no-mock): the instance pane renders a skill page without throwing — pumps InstancePane over the real examples/crm-demo corpus with currentPageId set to a skill page.
  • test/crm/skills_menu_test.dart: existing the ☰ menu lists grouped skills and opens one on tap stays green.
  • flutter analyze clean; full flutter test suite green (93 tests).
  • docs/notes/discovered/2026-05-30-overlay-ref-disposed-during-async-nav.md documents the bug and why no widget test catches it (flutter_test keeps the container alive across the async gap, so the buggy ordering stays green — verified by reverting the fix and re-running). The real safety net is the rodney/verify-demo.sh browser check, not a unit test.

🤖 Generated with Claude Code

Clicking a skill in the ☰ registry did nothing on the live (HTTP)
gateway — the menu closed but the skill page never opened, with an
opaque release-mode `Uncaught Error` in the console.

`_SkillsPanel._open` closed the overlay *before* awaiting `focusSkill`,
which awaits a real `resolve` round-trip and then reads the overlay's
`ref`. `close()` disposed that overlay first, so the post-await
`ref.read` hit a defunct element and threw; navigation never ran. The
fixture's microtask-fast `resolve` hid it locally; the gateway's network
latency made it deterministic.

Await `focusSkill` first, close last, so `ref` is still mounted when
`navigateToInstance` runs.

Adds a no-mock `skill_page_render_test` (the instance pane renders a
skill page over the real corpus without throwing) and a discovered-note
explaining why no widget test can catch the ordering regression —
`flutter_test` keeps the container alive across the async gap, so the
buggy order stays green; the real net is the rodney browser check.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@jrosskopf jrosskopf merged commit 740721f into main May 30, 2026
1 check passed
@jrosskopf jrosskopf deleted the fix/skills-menu-async-nav branch May 30, 2026 15:57
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.

1 participant