first-run-ux: foundations + empty states (PR-A of #1101)#1107
first-run-ux: foundations + empty states (PR-A of #1101)#1107RebelTechPro wants to merge 1 commit into
Conversation
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>
|
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 rightSplitting 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
Things worth a second pass before un-drafting
Test plan honestyYou marked TS compile as the only validated check, with manual screen-reader / theme spot-checks left as RecommendationLGTM 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 Thanks for the clean PR-A/PR-B split — much easier to reason about than a single 1000-line first-run-UX dump. |
joelteply
left a comment
There was a problem hiding this comment.
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 surfaceThat 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.
joelteply
left a comment
There was a problem hiding this comment.
@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
renderMessageElementdefault inAbstractMessageAdapternow 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
hiddenattribute fix (#1254): I shipped a:host([hidden]) { display: none; }rule inEmptyStateWidgetso yourupdateEntityCount()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:
- Change PR base from
main→canary(Continuum's flow merges to canary first, then promotes to main; PRs againstmaindirectly get held) - 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.
Summary
hasOnboarded?: booleantoUserEntity(per-user, cross-device) — the flag PR-B will check.ModalWidget(generic accessible dialog) andEmptyStateWidget(icon + title + subtitle + optional action).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.jsonregenerated (timestamp + sha bump + new field entry).widgets/shared/ModalWidget.ts(new file, 232 lines)open/modalTitle/closableproperties.role=\"dialog\",aria-modal=\"true\",aria-labelledbyon the title.modal-closeevent so parents can react to dismissal.widgets/shared/EmptyStateWidget.ts(new file, 116 lines)<empty-state>custom element. Reactiveicon/empty-title/subtitle/action-labelattributes.empty-state-actionevent when the action button is clicked. Slot for additional content.--accent-color,--text-muted,--surface-primary).Wiring (gated to avoid flash during initial load)
ReactiveEntityScrollerWidget— newprotected get isEmpty()getter. Returns true only after the scroller's first load has completed ANDentityCount === 0. Subclasses use this for the empty-state decision.ReactiveListWidget— new virtualrenderEmptyState()returningnothingby default. Mainrender()hides the container with?hidden=\${this.isEmpty}and shows the empty state whenisEmptyis true.RoomListWidget— overridesrenderEmptyState(). Copy depends on the active filter:UserListWidget— overridesrenderEmptyState(). Copy depends on whether any type/status filter is active. Widget overridesrender()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 torenderTemplate()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 thehiddenattribute based ongetEntityCount() === 0, after every CRUD event + the initial post-load count update.hidden=trueprevents 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)Out of scope (PR-B will add)
MainWidget.firstUpdated(readUserEntity.hasOnboarded, show modal if falsy)hasOnboarded=trueon modal completion viadata/updateTracked under the parent issue #1101.