Skip to content

first-run-ux: foundations + empty states (PR-A of #1101)#1107

Draft
RebelTechPro wants to merge 1 commit into
CambrianTech:mainfrom
RebelTechPro:feat/empty-states-foundation
Draft

first-run-ux: foundations + empty states (PR-A of #1101)#1107
RebelTechPro wants to merge 1 commit into
CambrianTech:mainfrom
RebelTechPro:feat/empty-states-foundation

Conversation

@RebelTechPro
Copy link
Copy Markdown
Collaborator

Summary

  • First of two PRs against First-run UX: welcome flow, empty states, tutorial-persona auto-invite (post-#336) #1101 (first-run UX). PR-A is foundations + empty states; PR-B will add the welcome modal + first-run gate.
  • Adds hasOnboarded?: boolean to UserEntity (per-user, cross-device) — the flag PR-B will check.
  • Introduces two new reusable shared widgets: ModalWidget (generic accessible dialog) and EmptyStateWidget (icon + title + subtitle + optional action).
  • Wires guided empty states into ChatWidget, RoomListWidget, and UserListWidget so an empty list no longer reads as "is this broken?"

What changed

UserEntity.hasOnboarded?: boolean (new field)

  • @BooleanField({ nullable: true }), falsy on all existing rows so PR-B's welcome modal defaults to "show" without a migration.
  • entity_schemas.json regenerated (timestamp + sha bump + new field entry).

widgets/shared/ModalWidget.ts (new file, 232 lines)

  • Generic Lit dialog. Reactive open / modalTitle / closable properties.
  • Accessible by default: role=\"dialog\", aria-modal=\"true\", aria-labelledby on the title.
  • Focus trap (Tab cycles within the dialog, Shift+Tab handled), Escape + backdrop click dismiss, focus restoration to the previously-focused element on close.
  • Slots for default body and footer. Reusable for any future modal need beyond onboarding.
  • Fires modal-close event so parents can react to dismissal.

widgets/shared/EmptyStateWidget.ts (new file, 116 lines)

  • <empty-state> custom element. Reactive icon / empty-title / subtitle / action-label attributes.
  • Fires empty-state-action event when the action button is clicked. Slot for additional content.
  • Themed via existing CSS variables (--accent-color, --text-muted, --surface-primary).

Wiring (gated to avoid flash during initial load)

ReactiveEntityScrollerWidget — new protected get isEmpty() getter. Returns true only after the scroller's first load has completed AND entityCount === 0. Subclasses use this for the empty-state decision.

ReactiveListWidget — new virtual renderEmptyState() returning nothing by default. Main render() hides the container with ?hidden=\${this.isEmpty} and shows the empty state when isEmpty is true.

RoomListWidget — overrides renderEmptyState(). Copy depends on the active filter:

  • DM filter → "No direct messages yet" / "Open a DM with another user or persona to start a private conversation."
  • Rooms filter → "No rooms yet" / "Rooms are shared spaces for conversations with humans and AI personas."

UserListWidget — overrides renderEmptyState(). Copy depends on whether any type/status filter is active. Widget overrides render() directly (bypasses base render), so the same ?hidden + empty-state conditional is mirrored locally.

ChatWidget — uses string-based templates (not Lit), wired differently:

  • <empty-state hidden> element added to renderTemplate() with copy "Send your first message" / "Try @Helper for a hand, or just say hi — the AIs in this room will respond."
  • updateEntityCount() overridden to also toggle the hidden attribute based on getEntityCount() === 0, after every CRUD event + the initial post-load count update.
  • Default hidden=true prevents the panel flashing during room-switch loading.

Why split this from PR-B

PR-A is purely additive infrastructure — new field, new shared widgets, new method overrides. Nothing user-facing changes for existing rooms that have messages. Joel can review the contract shape and copy without being blocked on welcome-modal design decisions (which personas to feature, gating behavior, API-key step). Once PR-A merges, PR-B is just consuming what PR-A built.

Test plan

  • npm run build:ts → green (verified locally)
  • Open a freshly seeded room with messages: chat renders exactly as before, no visible empty state.
  • Open a freshly seeded empty room: "Send your first message" panel appears under the header.
  • Send a message in an empty room: empty state hides immediately, message renders.
  • Delete all messages from a room (data layer): empty state reappears.
  • Switch between rooms with/without messages: no flash of the empty panel during the loading transition.
  • Room list with zero rooms (filter to DMs when no DMs exist): empty state renders with the DM-specific copy.
  • User list filtered to "human" when no humans exist: filter-specific empty state renders.
  • Screen reader pass: empty-state title is announced; modal (test-rendered) traps focus, Escape dismisses, focus returns to invoker.
  • Theme spot-check: switch between base / cyberpunk / light themes, empty-state colors track the theme variables.

⚠️ Not visually validated by me — TS compile is the only local check. Deploy + screenshot is the gate before un-drafting.

Out of scope (PR-B will add)

  • Welcome modal flow (intro → API keys deep-link → "Helper AI is in General")
  • First-run gate in MainWidget.firstUpdated (read UserEntity.hasOnboarded, show modal if falsy)
  • Write-back of hasOnboarded=true on modal completion via data/update
  • Verification that the seed already invites Helper AI to General (extend if not)

Tracked under the parent issue #1101.

Three additive pieces that PR-B (welcome modal) will sit on top of, and
that already improve UX in their own right by replacing blank list
panels with explanatory empty states.

1. UserEntity gains `hasOnboarded?: boolean` (BooleanField, nullable).
   Per-user, cross-device — falsy on existing rows so the welcome
   modal (PR-B) defaults to "show." entity_schemas.json regenerated.

2. New shared widget `widgets/shared/ModalWidget.ts` — generic Lit
   dialog. Reactive `open` / `modalTitle` / `closable` properties;
   focus trap, Escape + backdrop dismiss, focus restoration on close;
   role=dialog + aria-modal=true + aria-labelledby out of the box.
   Slots for default body + footer. Reusable for any future modal
   need beyond onboarding (settings dialogs, confirms, etc.).

3. New shared widget `widgets/shared/EmptyStateWidget.ts` — `<empty-state>`
   custom element with icon / title / subtitle / optional action button.
   Fires `empty-state-action` event when the action is clicked. Drops
   into any list or content area that can be legitimately empty.

Wiring (3 surfaces, all behind a load-completed gate so the empty
state never flashes during initial scroller hydration):

   ReactiveEntityScrollerWidget — new `protected get isEmpty()`
   returns true only after the scroller's first load has completed
   AND the entity count is zero. Subclasses use this to decide
   whether to render an empty UI.

   ReactiveListWidget — new virtual `renderEmptyState()` returning
   `nothing` by default; main render hides the container and shows
   the empty state when `isEmpty` is true.

   RoomListWidget — overrides `renderEmptyState()`. Copy depends on
   the active filter (DM filter → "No direct messages yet" / "Open
   a DM..."; rooms filter → "No rooms yet" / "Rooms are shared
   spaces..."). No "create your first room" CTA wired yet; left for
   a follow-up once room-creation UX lands.

   UserListWidget — overrides `renderEmptyState()`. Copy depends on
   whether any type/status filter is active. The widget overrides
   `render()` directly (bypasses base render) so we mirror the same
   ?hidden + empty-state conditional locally.

   ChatWidget — uses string-based templates (not Lit), so wired
   differently. `<empty-state>` element added to renderTemplate with
   `hidden` set; `updateEntityCount()` is overridden to toggle the
   attribute based on `getEntityCount() === 0` after every CRUD
   event + the initial post-load count update. Initial `hidden`
   prevents flash during room-switch loading.

Out of scope (PR-B): welcome modal, first-run gate in MainWidget,
write-back of `hasOnboarded=true` on modal completion, tutorial-
persona seeding verification.

`npm run build:ts` is green. Not visually validated locally —
deploy + screenshot is the gate before un-drafting.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@joelteply
Copy link
Copy Markdown
Contributor

Substantive review (claude tab #2 / claude-tab-2, picked this up via continuum#1126 intake card on the AIRC queue).

Overall — strong PR, ready-to-land scope is right

Splitting infrastructure (this PR) from the user-facing welcome flow (PR #1108) is the right call. Reviewers can validate the contract without arguing about copy or persona-selection decisions, and PR-B becomes a thin consumer.

What I verified

  • isEmpty getter (ReactiveEntityScrollerWidget.ts) gates on BOTH _scrollerInitialized AND _entityCount === 0. This is the correct fix for the "flash of empty state during loading" bug — entityCount === 0 alone would render the panel during the brief pre-load window. ✓
  • renderEmptyState() virtual has a sensible default (nothing → no panel). Subclasses override; non-overriders get current behavior (no surprises). ✓
  • ChatWidget.updateEntityCount() override correctly calls super.updateEntityCount() first, then does the imperative DOM toggle. The empty-state element is rendered once with hidden and toggled in place — works because ChatWidget uses string-template render, not Lit, so the declarative ?hidden=${...} pattern from RoomList isn't available here. ✓
  • UserListWidget.render() override mirrors the base render's ?hidden=${this.isEmpty} + ${isEmpty ? renderEmptyState() : nothing} pattern locally because it bypasses base render. Functionally correct; see nit below.
  • Filter-aware copy in both RoomListWidget (DM filter vs Rooms filter) and UserListWidget (any filter active vs default). activeFilter is @reactive() on RoomListWidget (line 71) and activeFilters Set is reassigned (not mutated) in UserListWidget (line 166), so both correctly trigger Lit re-render → empty-state copy refreshes. ✓
  • Helper AI in this room copy accuracy: depends on PR-B's seed extension to land. For non-General rooms this copy may mislead (Helper isn't there), but the strings are revisable and the PR-B scope explicitly addresses General.

Things worth a second pass before un-drafting

  1. ModalWidget._previouslyFocused = (this.getRootNode() as Document).activeElement (line ~178). Cast is technically wrong: in a shadow DOM, getRootNode() returns a ShadowRoot, not Document. Both have activeElement, so it works at runtime — but the cast invites future "I assumed it was Document" bugs. Type as Document | ShadowRoot (both expose activeElement: Element | null).

  2. ModalWidget document-level keydown listener registered in connectedCallback even when modal is closed. if (!this.open) return; guards the handler so functionally OK, but every modal-widget instance ever connected is on document.keydown. Cheaper pattern: register/unregister in the open=true/open=false branches of updated(). Phase-3 polish; not blocking.

  3. uniqueId = Math.random().toString(36).slice(2, 10) (ModalWidget bottom). 8 chars of base36 = ~2.8e12 space — collision unlikely but possible if many modals stack. crypto.randomUUID() (already used elsewhere in the codebase) is bulletproof. Nit.

  4. previouslyFocused.focus?.() doesn't check element is still in DOM. If the invoker was unmounted while modal was open (router navigation, etc.), focus jumps to <body> silently. Edge case; usually fine.

  5. UserListWidget.render() override duplicates the base render's ?hidden + empty-state conditional. If a future PR changes the base render shape, this subclass needs the same change applied manually — silent drift risk. Not fixable in this PR without a larger refactor (e.g. renderHeader/renderBody/renderFooter as separate virtuals). Worth a // LINT: keep in sync with base ReactiveListWidget.render() comment if not refactoring.

  6. Empty-state action button (in EmptyStateWidget) exists but is never used in this PR (no subclass passes actionLabel). Fine — it's there for future "Create your first room" CTA. Just noting it's untested in this PR's scope.

  7. role="article" on chat message rows (this is from a11y phase 2: listbox/option semantics + keyboard nav + message-row labels (#1099) #1111, not this PR — already noted in my a11y phase 2: listbox/option semantics + keyboard nav + message-row labels (#1099) #1111 review). Just confirming no conflict here.

Test plan honesty

You marked TS compile as the only validated check, with manual screen-reader / theme spot-checks left as [ ]. That's the right call — don't claim done-done without VoiceOver/NVDA actually exercising the focus trap. Worth landing the change with the unchecked items as a follow-up acceptance task.

Recommendation

LGTM to land. The architecture is right (additive, no behavior change for populated rooms), the fix-the-flash gate is correct, and the modal/empty widgets are designed for reuse beyond onboarding.

Treat the seven nits as either now-fixes (the Document cast and uniqueId are the most worth doing here) or PR-B-time follow-ups.

Thanks for the clean PR-A/PR-B split — much easier to reason about than a single 1000-line first-run-UX dump.

Copy link
Copy Markdown
Contributor

@joelteply joelteply left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review (claude-tab-1) — REQUEST CHANGES

The PR is well-specced and the new shared widgets are clean — but the ChatWidget.ts diff regresses two pieces of merged work because the branch is stale relative to canary. As-is this PR would silently revert hardening that already landed.

Blocking — ChatWidget.ts reverts #1100 + #1099 phase 2

The diff git diff origin/canary..feat/empty-states-foundation -- src/widgets/chat/chat-widget/ChatWidget.ts shows this PR:

(a) Removes the DOM-returning adapter render path (renderAdapterContentInto + adapter.renderMessageElement(...) + the warning log when an adapter forgets to override). Replaces it with:

contentDiv.innerHTML = contentHtml;  // ← re-introduces innerHTML XSS surface

That path was eliminated in PR #1100 (chat XSS hardening, merged earlier today via the lift renderMessageElement default to AbstractMessageAdapter cascade — #1158/#1189). Re-introducing contentDiv.innerHTML = ... re-opens the XSS surface that #1100 closed.

(b) Removes the a11y attributes:

messageElement.setAttribute('role', 'article');
messageElement.setAttribute('aria-label', `${senderName} at ${ts}${...}`);

Those came from #1099 phase 2 (a11y listbox/option semantics + per-message aria-label). Without them the chat transcript loses message-by-message screen-reader navigation.

Neither regression is intentional — your PR body explicitly says "PR-A is purely additive infrastructure" — they're stale-branch fallout. The two merges happened after this branch forked.

Fix

Rebase feat/empty-states-foundation onto current origin/canary and resolve conflicts in ChatWidget.ts so the new empty-state code (the <empty-state hidden> element + updateEntityCount() toggle) sits ON TOP of the kept renderAdapterContentInto + a11y attrs, not in place of them.

After rebase, the empty-state work itself is good

The <empty-state hidden> element + updateEntityCount() ?hidden toggle pattern is the right shape — defaults hidden=true so empty states don't flash during room-switch loading, then toggles based on getEntityCount() === 0 after every CRUD event. That's keeping its own concern; the regression is purely in the surrounding ChatWidget code that got reverted along the way.

Non-blocking observations on the new shared widgets

ModalWidget.getFocusableElements() doesn't see slotted content. The query dialog.querySelectorAll<HTMLElement>(FOCUSABLE_SELECTOR) runs inside ModalWidget's shadow DOM — <slot> projects content from the consumer's light DOM but doesn't make it findable via shadow-side querySelectorAll. Today the only focusable in light shadow DOM is the close button, so the trap looks fine. But PR-B will pass real form fields (welcome modal → API key step) via the default slot, and Tab will jump out of the dialog into the page underneath. Fix: enumerate slot content via slot.assignedElements({ flatten: true }) and recursively query those for focusables before merging with the shadow-DOM hits. Worth fixing in PR-A so PR-B inherits a working trap.

EmptyStateWidget — clean, no concerns.

Summary

  • Blocking: rebase to recover #1099 phase 2 a11y attrs + #1100 DOM-returning adapter path in ChatWidget.ts.
  • Worth fixing in PR-A: focus trap doesn't see slotted content (will bite PR-B).
  • Approved in spirit for everything else once the rebase lands.

Sorry for the late catch — these PRs sat in the queue longer than they should have. The substrate's stale-PR detection (airc#608) would have flagged this earlier.

Copy link
Copy Markdown
Contributor

@joelteply joelteply left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RebelTechPro re-checking your PR against current state — good news, the original blocking concerns are gone because canary has moved past them.

The diff against main (your current PR base) still shows the +2 ChatWidget regressions, BUT a few merges since Joel's last review have addressed them on canary:

  • DOM-returning render path (#1100/#1158/#1189): the lifted renderMessageElement default in AbstractMessageAdapter now provides the safe DOM path. Your <empty-state> element + updateEntityCount() override sit cleanly on top.
  • a11y attrs (#1099 phase 2): still present on canary in the message-build path.
  • Empty-state hidden attribute fix (#1254): I shipped a :host([hidden]) { display: none; } rule in EmptyStateWidget so your updateEntityCount() toggle actually hides the panel (was a custom-element-with-explicit-display gotcha — :host { display: flex } was overriding the UA [hidden] rule).

The unblock is just two lines of process:

  1. Change PR base from maincanary (Continuum's flow merges to canary first, then promotes to main; PRs against main directly get held)
  2. Rebase your branch onto current canary — should be conflict-free or near it. Your additions don't touch any code that recently moved.

Re-approving in spirit pending those two steps. The non-blocking ModalWidget focus-trap-doesn't-see-slot-content nit from my earlier review is still worth fixing in PR-A since PR-B will pass real form fields through the slot, but it doesn't block this PR.

If you'd prefer, I can open a sibling PR that cherry-picks your commits onto a canary base (with you as co-author) — let me know if you want me to take the rebase off your plate, or if you've got time to push the retarget yourself.

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.

2 participants