[8790] Update rxjs NEVER vs EMPTY usages

[jvega190]

craftercms/craftercms#8790

Summary by CodeRabbit

• Bug Fixes
  • Improved several “no action needed” flows so dialogs, navigation, and editor interactions now finish cleanly instead of lingering in the background.
  • Fixed early-exit cases in guest editing and drag-and-drop handling to complete immediately when an action can’t proceed.
  • Smoothed refresh and redirect behavior across content, project, and version screens, reducing the chance of stuck or unresponsive states.

[coderabbitai]

Walkthrough

Changes

The PR replaces selected RxJS NEVER no-op returns with EMPTY across application epics, navigator refresh flows, guest interaction handlers, TinyMCE initialization, and write utilities. These branches now complete immediately without dispatching emissions.

RxJS no-op observable completion

Layer / File(s)	Summary
Application epic guard branches ui/app/src/state/epics/dialogs.ts, itemVersions.ts, mappedDialogs.ts, misc.ts, system.ts	Dialog, content-type, history-dialog, and project-redirect branches return EMPTY for non-dispatching exits.
Navigator refresh exits ui/app/src/state/epics/pathNavigator*.ts	Navigator refresh and event epics return EMPTY when no requests or follow-up actions are produced.
Guest interaction exits ui/guest/src/store/epics/root.ts	Drag, drop, click, and content-tree selection handlers return EMPTY for blocked or actionless paths.
Guest runtime and write defaults ui/guest/src/controls/rte.ts, ui/guest/src/store/util.ts	TinyMCE initialization and beforeWrite$ use EMPTY for missing-document or stopped-write paths.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related issues

• Update rxjs NEVER vs EMPTY usages craftercms#8790 — Covers replacing RxJS NEVER with EMPTY so selected observables complete instead of remaining active.

Possibly related PRs

• craftercms/studio-ui#4349 — Updates the same dialog epic duplicate-opening branches.
• craftercms/studio-ui#4809 — Touches the TinyMCE missing-document.doctype early-exit path.

Suggested reviewers: rart

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title matches the main change: replacing RxJS NEVER with EMPTY across several epics and helpers.
Description check	✅ Passed	The description includes the required ticket reference, though it is minimal.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

ui/app/src/state/epics/mappedDialogs.ts (1)

186-186: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Use switchMap here so EMPTY is flattened

ui/app/src/state/epics/mappedDialogs.ts:175 still returns an EMPTY Observable from map, so the epic emits the Observable object itself. Switch this to switchMap and wrap pushDialog(...) in of(...), matching dialogs.ts.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@ui/app/src/state/epics/mappedDialogs.ts` at line 186, The mappedDialogs epic
is returning an EMPTY Observable from a map-like branch, so the stream emits the
Observable itself instead of flattening it. Update the epic logic around
showEditDialog.type and showCodeEditorDialog.type to use switchMap, and wrap the
pushDialog(...) result in of(...) so EMPTY is properly flattened, matching the
pattern used in dialogs.ts.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@ui/app/src/state/epics/mappedDialogs.ts`:
- Line 186: The mappedDialogs epic is returning an EMPTY Observable from a
map-like branch, so the stream emits the Observable itself instead of flattening
it. Update the epic logic around showEditDialog.type and
showCodeEditorDialog.type to use switchMap, and wrap the pushDialog(...) result
in of(...) so EMPTY is properly flattened, matching the pattern used in
dialogs.ts.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 95838f93-95f3-4d4e-8946-8800fbd29f58

📥 Commits

Reviewing files that changed from the base of the PR and between 7d8ec52 and b31c648.

📒 Files selected for processing (10)

• ui/app/src/state/epics/dialogs.ts
• ui/app/src/state/epics/itemVersions.ts
• ui/app/src/state/epics/mappedDialogs.ts
• ui/app/src/state/epics/misc.ts
• ui/app/src/state/epics/pathNavigator.ts
• ui/app/src/state/epics/pathNavigatorTree.ts
• ui/app/src/state/epics/system.ts
• ui/guest/src/controls/rte.ts
• ui/guest/src/store/epics/root.ts
• ui/guest/src/store/util.ts

[jvega190]

@coderabbitai can you check if everything looks good? Please give it a final review before I hand over for human review

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail
gh pr diff 4915 --repo craftercms/studio-ui > /tmp/pr.diff
wc -l /tmp/pr.diff
cat /tmp/pr.diff

Length of output: 15012

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail
sed -n '90,160p' ui/app/src/state/epics/dialogs.ts 2>/dev/null || echo "file not found, need to clone"

Length of output: 2812

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail
for f in ui/app/src/state/epics/dialogs.ts ui/app/src/state/epics/itemVersions.ts ui/app/src/state/epics/mappedDialogs.ts ui/app/src/state/epics/misc.ts ui/app/src/state/epics/pathNavigator.ts ui/app/src/state/epics/pathNavigatorTree.ts ui/app/src/state/epics/system.ts ui/guest/src/controls/rte.ts ui/guest/src/store/epics/root.ts ui/guest/src/store/util.ts; do
  echo "== $f =="
  rg -n 'NEVER' "$f" || echo "no NEVER usages left"
done

Length of output: 1226

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail
rg -n "\bNEVER\b" --type=ts -g '!**/node_modules/**' ui | rg -v '\.spec\.' || echo "none found"

Length of output: 275

---

@jvega190: I gave this a final pass and it looks good to me. A few notes from the review:

• All NEVER → EMPTY replacements are consistent, and each file's rxjs import was correctly updated (no leftover NEVER references anywhere in the repo, confirmed via search).
• The changes are semantically appropriate for how these observables are consumed: in every case, NEVER/EMPTY is returned from inside a switchMap (or similar) within a long-lived epic, so switching from "never completes" to "completes immediately with no emission" doesn't change the epic's overall lifecycle — it just avoids an observable that lingers forever for no reason.
• Nice catch in ui/app/src/state/epics/dialogs.ts: the branch where dialogState exists but isMinimized !== true previously had no explicit return at all (implicit undefined), which would have been an actual bug feeding switchMap. Adding return EMPTY; there is a genuine fix, not just a cosmetic swap.
• In ui/guest/src/store/epics/root.ts, the click-handling comment was also updated to accurately describe the new behavior (EMPTY completes immediately, and switchMap still unsubscribes any prior inner stream on a new click) — good, this keeps the comment truthful for future readers.
• ui/guest/src/store/util.ts's beforeWrite$ default for stop$ and its doc comment were updated together, so behavior and documentation stay in sync.

I didn't find any remaining NEVER usages, missed import cleanups, or spots where the mechanical replacement could change intended behavior (e.g., no cases mixing EMPTY with logic that depended on the observable never completing, such as race()/takeUntil() patterns).

This looks solid and ready for human review.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.