🐛 Fixed tag results in admin search not opening the tag - #30518
Conversation
no ref
Clicking a tag in the Cmd-K search modal did nothing.
The Ember `tag` route aborts its transition when `tagDetailsReact` is on so
React can own the URL, but an aborted transition never reaches `updateURL` —
so a transition Ember itself started left the shared hash untouched and React
never rendered anything. The search modal navigates by route name
(`transitionTo('tag', slug)`), which is exactly the case that has no URL of
its own.
Write the React URL for named intents and park on `react-fallback`, matching
what the posts/pages and editor routes already do behind their own flags. The
tag route was the first flag-gated route and predates that handling. Parking
also keeps `currentRouteName` honest, so returning to the same tag URL later
is a real transition rather than a silent no-op.
The existing search acceptance tests run with mirage labs `{}`, so they only
covered the Ember-owned branch no production user reaches. The new acceptance
test drives the real router with the flag on, mirroring the editor and posts
equivalents.
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx run @tryghost/admin:test:acceptance |
✅ Succeeded | 10m 17s | View ↗ |
nx run-many -t test:unit -p @tryghost/admin |
✅ Succeeded | 3m 54s | View ↗ |
nx run ghost-admin:test |
✅ Succeeded | 2m 52s | View ↗ |
nx run ghost-monorepo:lint:boundaries |
✅ Succeeded | 27s | View ↗ |
nx run @tryghost/admin:build |
✅ Succeeded | 1m 58s | View ↗ |
nx run-many -t lint -p ghost-admin,@tryghost/ad... |
✅ Succeeded | 1m 49s | View ↗ |
nx run-many --target=build --projects=tag:publi... |
✅ Succeeded | 1s | View ↗ |
nx run @tryghost/e2e:test:fixtures |
✅ Succeeded | <1s | View ↗ |
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗
☁️ Nx Cloud last updated this comment at 2026-09-03 20:24:37 UTC
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: QUIET Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (8)
🧰 Additional context used📓 Path-based instructions (5)Review whether tests prove changed behaviour, meaningful error/edge paths, and externally observable contracts without coupling to implementation details.⚙️ CodeRabbit configuration file Files:
New source files must be TypeScript: flag new JS files as a required change unless exempt (DB migrations, apps/ember-admin/, tool/config files, scripts/, docker/, generated code).⚙️ CodeRabbit configuration file Files:
Prioritise concrete correctness, security, data-integrity, compatibility, and regression risks.⚙️ CodeRabbit configuration file Files:
New files are TypeScript: Fail if the PR adds a new .js/.jsx/.cjs/.mjs source file, unless it is: a DB migration (ghost/core/core/server/data/migrations/), under apps/ember-admin/, a tool/config file, under scripts/ or docker/, or generated...📄 CodeRabbit inference engine (Custom checks) Files:
Always use `pnpm`, never npm or Yarn.📄 CodeRabbit inference engine (AGENTS.md) Files:
🧠 Learnings (1)📚 Learning: 2026-08-24T15:30:14.342ZApplied to files:
🔇 Additional comments (1)
WalkthroughThe tag route now leaves Ember in control when Merge Risk: ⚪ Minimal · up to With the feature enabled, tag navigation now delegates to the React tag pages while preserving URL and router state; the covered transition paths leave no concrete merge-blocking risk. 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
Full details: Type-Safe BoundariesExplanation PASS. The PR adds navigation logic in an existing JavaScript route. It does not add Full details: New Files Are TypescriptExplanation The PR adds one new JavaScript file: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #30518 +/- ##
=======================================
Coverage 67.53% 67.54%
=======================================
Files 1670 1670
Lines 60154 60179 +25
Branches 10403 10409 +6
=======================================
+ Hits 40626 40648 +22
- Misses 17237 17238 +1
- Partials 2291 2293 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Clicking a tag in the Cmd-K search modal did nothing — the result was found and highlighted, but selecting it left you on the current screen.
Cause
gh-search-input.jsnavigates tag results by Ember route name:The Ember
tagroute aborts its transition whentagDetailsReactis on, so React can own/tags/:tagSlug. But an aborted transition never reachesupdateURL, and the two apps sharewindow.location.hash— so a transition Ember itself started leaves the hash untouched and React never navigates. A named intent is exactly the case with no URL of its own; a URL intent (cold load, React-driven navigation) already has the browser pointing at the right place.tagDetailsReactis inGA_FEATURES, so this affects everyone.Fix
Write the React URL for named intents, then park on the
react-fallbackcatch-all — the same handlingposts.jsandlexical-editor.jsalready have behind their own flags. The tag route was the first flag-gated route and predates that handling; it was never backfilled.Parking also keeps
currentRouteNamehonest. Without it Ember still believes it is on the route it came from, so returning to the same tag URL later is compared against that stale route, found identical, and runs no transition at all./tags/newgets the same treatment viatag.new, which extends this route.Tests
The existing search acceptance tests run with mirage labs
{}, sotagDetailsReactis false there — they only covered the Ember-owned branch that no production user reaches, and would keep passing if this fix were reverted. Addedtag-react-flag-test.js, which drives the real router with the flag on, mirroring the editor and posts equivalents. Reverting the fix fails 3 of its 6 tests.Unit tests in
tag-test.jscover the URL-intent vs named-intent split,tag.new, the parked-path guard, and URL/history-state restoration.Verification
ember test --filter "tag"— 75 passingember test --filter "search"— 77 passingember test --filter "Unit: Route"— 13 passingeslint app testscleanOut of scope
Navigating away from a dirty React screen via any Ember-initiated hash write skips React's unsaved-changes blocker: the write creates a history entry with
history.state === null, andisOnRouterHistoryEntry()returns false, souseBlockerdeclines. This is not introduced here — Ember's ownHashLocation.setURListhis.location.hash = path, so Cmd-K results for Staff and the delete-tag-modal's/tagstransition already behave this way. Worth a separate fix.