feat(onboarding): rework lesson flow, better styles, better focus#2158
Merged
feat(onboarding): rework lesson flow, better styles, better focus#2158
Conversation
Contributor
sedson
commented
Mar 24, 2026
sedson
commented
- [wip]
- [wip]
- feat(onboarding): rework lesson flow, better styles, better focus
Contributor
WalkthroughThe interactive onboarding system undergoes substantial expansion and refactoring. Hotkey navigation now includes Vim-style alternatives (hjkl) alongside arrow keys. The command menu gains an Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
js/app/packages/app/component/interactive-onboarding/sandbox/sandbox-store.ts (1)
361-376: 🧹 Nitpick | 🔵 TrivialSame recommendation: use
matchfromts-patternforentityToBucket.This switch also has a
defaultfallback that could hide issues if new entity types are added.Suggested refactor
function entityToBucket(entity: EntityData): EntityBucketType { - switch (entity.type) { - case 'document': - return entity.subType?.type === 'task' ? 'task' : 'note'; - case 'email': - return 'email'; - case 'channel': - return entity.channelType === 'direct_message' ? 'dm' : 'channel'; - case 'chat': - return 'chat'; - case 'project': - return 'project'; - default: - return 'note'; - } + return match(entity.type) + .with('document', () => entity.subType?.type === 'task' ? 'task' : 'note') + .with('email', () => 'email' as const) + .with('channel', () => (entity.channelType === 'direct_message' ? 'dm' : 'channel') as const) + .with('chat', () => 'chat' as const) + .with('project', () => 'project' as const) + .otherwise(() => 'note' as const); }As per coding guidelines: "For exhaustive switch statements use
matchfromts-pattern".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/app/component/interactive-onboarding/sandbox/sandbox-store.ts` around lines 361 - 376, Replace the switch in entityToBucket(entity: EntityData): EntityBucketType with a ts-pattern match on entity to make the match exhaustive: use match(entity) and branch on entity.type cases ('document' with nested check on entity.subType?.type === 'task' => 'task' or else 'note', 'email' => 'email', 'channel' with check entity.channelType === 'direct_message' => 'dm' or else 'channel', 'chat' => 'chat', 'project' => 'project'), and remove the generic default branch; for the unmatched case return or throw an explicit unreachable/assert (e.g., assertNever) so new EntityData types fail at compile-time rather than being silently mapped to 'note'. Ensure references to EntityData and EntityBucketType remain unchanged.js/app/packages/entity/mocks/mockEntityData.ts (1)
265-280:⚠️ Potential issue | 🟡 MinorNaming inconsistency:
MOCK_CHANNEL_PUBLICnow haschannelType: 'private'.The constant name suggests a public channel, but the
channelTypewas changed to'private'. This is misleading for consumers of this mock data. Consider renaming the constant to match its actual type, or if backward compatibility is needed, introduce a new constant and deprecate this one.Suggested fix
-export const MOCK_CHANNEL_PUBLIC: ChannelEntity = { +export const MOCK_CHANNEL_GENERAL: ChannelEntity = { type: 'channel', - id: 'channel_public_1', + id: 'channel_general_1', name: 'general', ownerId: MOCK_USER_IDS.owner, channelType: 'private',Then update references accordingly, or add a deprecated alias:
/** `@deprecated` Use MOCK_CHANNEL_GENERAL instead */ export const MOCK_CHANNEL_PUBLIC = MOCK_CHANNEL_GENERAL;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/app/packages/entity/mocks/mockEntityData.ts` around lines 265 - 280, MOCK_CHANNEL_PUBLIC's data is inconsistent: its name implies a public channel but the object has channelType: 'private'; update the mock to be consistent by either changing channelType to 'public' (in the exported MOCK_CHANNEL_PUBLIC constant) or rename the constant to reflect its actual type (e.g., MOCK_CHANNEL_PRIVATE or MOCK_CHANNEL_GENERAL) and update all references; if you need to preserve the old identifier for compatibility, add a deprecated alias (exporting the new constant under the old name with a deprecation note) and update callers to use the new name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@js/app/packages/app/component/interactive-onboarding/components-lib.tsx`:
- Around line 151-155: ContinueButton currently only toggles CSS when disabled;
update the ContinueButton (and its ContinueButtonProps) so the native <button>
element receives the disabled boolean (pass disabled to the button attribute),
prevent onClick when disabled (guard the onClick handler), and add aria-disabled
and tabIndex adjustments (aria-disabled={disabled}, tabIndex={disabled ? -1 :
0}) so it is not focusable when disabled; ensure the ref handling remains
compatible (use forwardRef or remove custom ref prop if necessary) so the native
button can still receive a proper ref.
- Around line 142-145: The completion badge's class list in the Show block of
components-lib.tsx uses "rounded xs" which is parsed as two separate classes and
prevents the corner radius from applying; update the div's class string to use
the proper token "rounded-xs" (replace "rounded xs" with "rounded-xs") so the
badge renders with the intended border radius (the div containing CheckIcon
inside the Show when={props.completed} block).
In
`@js/app/packages/app/component/interactive-onboarding/components/MockAppChrome.tsx`:
- Around line 117-129: The icon-only filter buttons in MockAppChrome lack
explicit accessible names; update the button elements that call setFilter (e.g.,
the button using onClick={(e) => { e.preventDefault(); setFilter(null); }} and
the other filter button(s) around the later block) to include descriptive
aria-label attributes (for example aria-label="Show all" or aria-label="Show
favorites" as appropriate) so screen readers and keyboard users get a clear
name; keep existing title/tooltips but add aria-label directly on the button
elements referenced in the MockAppChrome component and ensure labels match the
filter action.
In
`@js/app/packages/app/component/interactive-onboarding/InteractiveOnboarding.tsx`:
- Around line 349-356: The ContinueButton consumer passes a disabled prop but
the implementation in components-lib.tsx doesn't forward disabled/aria-disabled
(and likely tabIndex) to the underlying element, leaving the CTA tabbable and
clickable; update the ContinueButton component in components-lib.tsx (the
function/component that renders the button for ContinueButton) to accept and
forward the disabled boolean to the native button element (buttonProps.disabled
= disabled), set aria-disabled={disabled}, and if you manually manage tabIndex
ensure it becomes -1 when disabled so it is not tabbable; also
preserve/ref-forward the ref used by the caller (the ref passed into
ContinueButton) so the existing ref usage in InteractiveOnboarding
(continueButtonRef) continues to work.
In
`@js/app/packages/app/component/interactive-onboarding/lessons/choose-plan.tsx`:
- Around line 85-94: The plan buttons currently have no click handler, so when
hideContinue is true the step becomes uncompletable; add an onClick to the
button (in the render that maps over plan) that calls props.onComplete() and
then triggers the lesson to advance (call the component's existing advance
method/prop — e.g., props.onAdvance() or the shell advance function used
elsewhere in this component) so selecting a plan both arms Continue and
immediately advances the lesson; apply the same fix to the other plan button
group around lines 106-112.
In
`@js/app/packages/app/component/interactive-onboarding/lessons/email-invite.tsx`:
- Around line 5-25: In EmailInviteContent, the deferred setTimeout and the two
requestAnimationFrame calls can run after the component is cleaned up; store the
timer ID returned by setTimeout and the two rAF IDs returned by
requestAnimationFrame when you schedule them, and in onCleanup call
clearTimeout(timerId) and cancelAnimationFrame for each rAFId (and null out
textareaRef if needed) so props.onComplete('Send Invites') and
textareaRef?.focus() are not invoked after unmount; update the scheduling code
around props.onComplete, requestAnimationFrame, and textareaRef to capture those
handles and cancel them in onCleanup.
In `@js/app/packages/app/component/interactive-onboarding/lessons/index.ts`:
- Line 8: The file contains a commented-out import and array entry for
composersPreviewLesson in index.ts; either remove both commented lines (import
and any commented array reference) if the lesson is no longer needed, or replace
the commented code with a short TODO explaining why composersPreviewLesson is
disabled and under what conditions it should be re-enabled (reference the import
statement for composersPreviewLesson and the lessons array in this module so the
change is applied consistently).
In
`@js/app/packages/app/component/interactive-onboarding/lessons/navigate-list.tsx`:
- Around line 85-89: The delayed removal uses setTimeout with
REMOVE_ANIMATION_MS and can run after the component unmounts; fix by capturing
the timeout id when calling setTimeout in the handlers that call
setRemovingIds(...) and removeSandboxEntity(focused.id) (references:
setRemovingIds, removeSandboxEntity, REMOVE_ANIMATION_MS, focused) and clear
that timeout on unmount (or when the lesson changes) using a cleanup (e.g.,
store the timer id in a ref and call clearTimeout(timerRef.current) in a
useEffect cleanup); apply the same pattern to the other occurrence around lines
100-105 so no stale timeouts mutate sandbox state after teardown.
In
`@js/app/packages/app/component/interactive-onboarding/lessons/sidebar-nav.tsx`:
- Around line 12-18: Snapshot the current global sidebarFilter() when
SidebarNavContent mounts, temporarily ensure it's not already 'mail' (if prev
=== 'mail' set sidebarFilter('') then set sidebarFilter('mail') to trigger the
lesson), then run the existing createEffect that watches sidebarFilter() and
calls setDone(true) and props.onComplete() when it becomes 'mail'; after
completion and in cleanup restore the original snapshot value. Implement the
same snapshot/reset/restore pattern used in create-entity.tsx and
markdown-mentions.tsx: capture prev = sidebarFilter(), adjust to avoid immediate
auto-complete, trigger the lesson, and always restore sidebarFilter(prev) on
onCleanup or after props.onComplete().
In
`@js/app/packages/app/component/interactive-onboarding/OnboardingEntityList.tsx`:
- Around line 15-26: The inline CSS in OnboardingEntityList.tsx defines a
keyframe animation onboarding-entity-remove and the .onboarding-entity-removing
rule; extract these into the project's shared stylesheet (or the project's
CSS-in-JS location) and replace the <style> block with a class reference.
Specifically, move the `@keyframes` onboarding-entity-remove and
.onboarding-entity-removing declarations into the shared CSS/CSS-in-JS module,
import or apply that stylesheet/module in the OnboardingEntityList component,
and ensure the component continues to add the className
"onboarding-entity-removing" where needed so the animation still applies.
- Around line 64-127: The email rendering block repeats the same type assertions
for isRead, senderName, and snippet; narrow the entity once at the top of the
Show branch to a typed local (e.g., const e = entity as EntityData & { isRead:
boolean; senderName: string; snippet: string }) and replace all repeated (entity
as ...) usages with e when computing classes and rendering values; keep usages
of Entity.Icon, Entity.Title, and Entity.Timestamp but pass e (or e cast back to
EntityData where needed) and remove duplicate assertions to simplify the JSX.
In
`@js/app/packages/app/component/interactive-onboarding/sandbox/sandbox-store.ts`:
- Around line 231-254: The matchesFilter function currently uses a switch with a
default that masks unhandled SandboxSidebarFilter values; replace the switch
with ts-pattern's match to get exhaustiveness checking: import { match } from
'ts-pattern', convert the switch in matchesFilter to a match(filter) that maps
each case ('empty','agents','mail','documents','tasks','channels','folders') to
the same boolean expressions (using entity.type and entity.subType?.type) and
omit a fallback so the compiler will flag any new/unhandled SandboxSidebarFilter
variants (or explicitly handle unknown with a sensible return if needed).
---
Outside diff comments:
In
`@js/app/packages/app/component/interactive-onboarding/sandbox/sandbox-store.ts`:
- Around line 361-376: Replace the switch in entityToBucket(entity: EntityData):
EntityBucketType with a ts-pattern match on entity to make the match exhaustive:
use match(entity) and branch on entity.type cases ('document' with nested check
on entity.subType?.type === 'task' => 'task' or else 'note', 'email' => 'email',
'channel' with check entity.channelType === 'direct_message' => 'dm' or else
'channel', 'chat' => 'chat', 'project' => 'project'), and remove the generic
default branch; for the unmatched case return or throw an explicit
unreachable/assert (e.g., assertNever) so new EntityData types fail at
compile-time rather than being silently mapped to 'note'. Ensure references to
EntityData and EntityBucketType remain unchanged.
In `@js/app/packages/entity/mocks/mockEntityData.ts`:
- Around line 265-280: MOCK_CHANNEL_PUBLIC's data is inconsistent: its name
implies a public channel but the object has channelType: 'private'; update the
mock to be consistent by either changing channelType to 'public' (in the
exported MOCK_CHANNEL_PUBLIC constant) or rename the constant to reflect its
actual type (e.g., MOCK_CHANNEL_PRIVATE or MOCK_CHANNEL_GENERAL) and update all
references; if you need to preserve the old identifier for compatibility, add a
deprecated alias (exporting the new constant under the old name with a
deprecation note) and update callers to use the new name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 67de2227-fedb-44ec-892c-95095ac8ce90
📒 Files selected for processing (19)
js/app/packages/app/component/Launcher.tsxjs/app/packages/app/component/command/CommandMenu.tsxjs/app/packages/app/component/interactive-onboarding/InteractiveOnboarding.tsxjs/app/packages/app/component/interactive-onboarding/OnboardingEntityList.tsxjs/app/packages/app/component/interactive-onboarding/components-lib.tsxjs/app/packages/app/component/interactive-onboarding/components/MockAppChrome.tsxjs/app/packages/app/component/interactive-onboarding/lessons/choose-plan.tsxjs/app/packages/app/component/interactive-onboarding/lessons/command-k.tsxjs/app/packages/app/component/interactive-onboarding/lessons/create-entity.tsxjs/app/packages/app/component/interactive-onboarding/lessons/email-invite.tsxjs/app/packages/app/component/interactive-onboarding/lessons/home.tsxjs/app/packages/app/component/interactive-onboarding/lessons/index.tsjs/app/packages/app/component/interactive-onboarding/lessons/markdown-mentions.tsxjs/app/packages/app/component/interactive-onboarding/lessons/navigate-list.tsxjs/app/packages/app/component/interactive-onboarding/lessons/sidebar-nav.tsxjs/app/packages/app/component/interactive-onboarding/lessons/welcome.tsxjs/app/packages/app/component/interactive-onboarding/sandbox/sandbox-store.tsjs/app/packages/app/component/interactive-onboarding/types.tsjs/app/packages/entity/mocks/mockEntityData.ts
Comment on lines
+142
to
+145
| <Show when={props.completed}> | ||
| <div class="bg-accent text-panel size-5 rounded xs flex items-center justify-center ml-auto"> | ||
| <CheckIcon class="size-4" /> | ||
| </div> |
Contributor
There was a problem hiding this comment.
Fix the rounded-xs class typo on the completion badge.
rounded xs is parsed as two classes, so the tokenized corner radius never applies here.
🎨 Proposed fix
- <div class="bg-accent text-panel size-5 rounded xs flex items-center justify-center ml-auto">
+ <div class="bg-accent text-panel size-5 rounded-xs flex items-center justify-center ml-auto">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Show when={props.completed}> | |
| <div class="bg-accent text-panel size-5 rounded xs flex items-center justify-center ml-auto"> | |
| <CheckIcon class="size-4" /> | |
| </div> | |
| <Show when={props.completed}> | |
| <div class="bg-accent text-panel size-5 rounded-xs flex items-center justify-center ml-auto"> | |
| <CheckIcon class="size-4" /> | |
| </div> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/app/component/interactive-onboarding/components-lib.tsx`
around lines 142 - 145, The completion badge's class list in the Show block of
components-lib.tsx uses "rounded xs" which is parsed as two separate classes and
prevents the corner radius from applying; update the div's class string to use
the proper token "rounded-xs" (replace "rounded xs" with "rounded-xs") so the
badge renders with the intended border radius (the div containing CheckIcon
inside the Show when={props.completed} block).
Comment on lines
151
to
155
| interface ContinueButtonProps { | ||
| onClick: () => void; | ||
| label?: string; | ||
| ghost?: boolean; | ||
| disabled?: boolean; | ||
| ref?: (el: HTMLButtonElement) => void; |
Contributor
There was a problem hiding this comment.
Apply disabled to the native button, not just the styles.
Right now disabled only changes classes. The button is still clickable and focusable, so onboarding can advance even when the lesson is not ready.
🔒 Proposed fix
<button
ref={props.ref}
type="button"
+ disabled={props.disabled}
class={cn(
'w-full px-3 py-2.5 text-lg font-bold rounded-xs flex items-center justify-between gap-2 bracket-never',
{Also applies to: 160-172
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/app/component/interactive-onboarding/components-lib.tsx`
around lines 151 - 155, ContinueButton currently only toggles CSS when disabled;
update the ContinueButton (and its ContinueButtonProps) so the native <button>
element receives the disabled boolean (pass disabled to the button attribute),
prevent onClick when disabled (guard the onClick handler), and add aria-disabled
and tabIndex adjustments (aria-disabled={disabled}, tabIndex={disabled ? -1 :
0}) so it is not focusable when disabled; ensure the ref handling remains
compatible (use forwardRef or remove custom ref prop if necessary) so the native
button can still receive a proper ref.
Comment on lines
+117
to
+129
| <button | ||
| type="button" | ||
| class={cn( | ||
| 'size-6 text-ink rounded-xs p-1 transition-colors cursor-default hover:bg-ink/10', | ||
| sidebarFilter() === null | ||
| ? 'opacity-100 bg-ink/10 text-ink' | ||
| : 'opacity-50 hover:opacity-80' | ||
| )} | ||
| onClick={(e) => { | ||
| e.preventDefault(); | ||
| setFilter(null); | ||
| }} | ||
| title="All" |
Contributor
There was a problem hiding this comment.
Give the icon-only filter buttons explicit accessible names.
These controls rely on icons plus tooltip/title text, which is not a reliable button name for assistive tech. Add aria-label directly on the buttons so keyboard and screen-reader users can tell what each filter does.
♿ Proposed fix
<button
type="button"
+ aria-label="All items"
class={cn(
@@
<button
type="button"
+ aria-label={link.label}
class={cn(Also applies to: 166-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@js/app/packages/app/component/interactive-onboarding/components/MockAppChrome.tsx`
around lines 117 - 129, The icon-only filter buttons in MockAppChrome lack
explicit accessible names; update the button elements that call setFilter (e.g.,
the button using onClick={(e) => { e.preventDefault(); setFilter(null); }} and
the other filter button(s) around the later block) to include descriptive
aria-label attributes (for example aria-label="Show all" or aria-label="Show
favorites" as appropriate) so screen readers and keyboard users get a clear
name; keep existing title/tooltips but add aria-label directly on the button
elements referenced in the MockAppChrome component and ensure labels match the
filter action.
Comment on lines
+349
to
+356
| <ContinueButton | ||
| ref={(el) => { | ||
| continueButtonRef = el; | ||
| }} | ||
| onClick={handleContinue} | ||
| label={continueLabel()} | ||
| disabled={!readyToContinue()} | ||
| /> |
Contributor
There was a problem hiding this comment.
ContinueButton is still exposed as enabled here.
This call site now relies on a disabled prop, but components-lib.tsx still renders a plain <button> without forwarding disabled or aria-disabled. The CTA stays tabbable/clickable and won’t be announced as disabled until the lesson is ready. Please wire the actual disabled state in ContinueButton before relying on this prop here.
Minimal fix in components-lib.tsx
export function ContinueButton(props: ContinueButtonProps) {
return (
<button
ref={props.ref}
type="button"
+ disabled={props.disabled}
class={cn(
'w-full px-3 py-2.5 text-lg font-bold rounded-xs flex items-center justify-between gap-2 bracket-never',
{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@js/app/packages/app/component/interactive-onboarding/InteractiveOnboarding.tsx`
around lines 349 - 356, The ContinueButton consumer passes a disabled prop but
the implementation in components-lib.tsx doesn't forward disabled/aria-disabled
(and likely tabIndex) to the underlying element, leaving the CTA tabbable and
clickable; update the ContinueButton component in components-lib.tsx (the
function/component that renders the button for ContinueButton) to accept and
forward the disabled boolean to the native button element (buttonProps.disabled
= disabled), set aria-disabled={disabled}, and if you manually manage tabIndex
ensure it becomes -1 when disabled so it is not tabbable; also
preserve/ref-forward the ref used by the caller (the ref passed into
ContinueButton) so the existing ref usage in InteractiveOnboarding
(continueButtonRef) continues to work.
Comment on lines
+85
to
+94
| <button | ||
| type="button" | ||
| class="w-full py-2 rounded-xs text-base font-semibold" | ||
| classList={{ | ||
| 'bg-accent text-panel': !!plan.popular, | ||
| 'bg-ink/8 text-ink hover:bg-ink/12': !plan.popular, | ||
| }} | ||
| > | ||
| Get {plan.name} | ||
| </button> |
Contributor
There was a problem hiding this comment.
hideContinue leaves this step without any visible way to finish.
props.onComplete() only arms the shell’s Continue action; it does not advance the lesson. Once you hide the Continue/Skip controls here, the flow depends on the undiscoverable cmd+enter hotkey because the plan buttons in Lines 85-94 do not call back into the shell. From the visible UI, onboarding can no longer complete.
Minimal fix
export const choosePlanLesson: LessonDefinition = {
id: 'choose-plan',
title: 'Choose your plan',
content: ChoosePlanContent,
demo: ChoosePlanDemo,
order: 80,
- hideContinue: true,
};Also applies to: 106-112
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/app/component/interactive-onboarding/lessons/choose-plan.tsx`
around lines 85 - 94, The plan buttons currently have no click handler, so when
hideContinue is true the step becomes uncompletable; add an onClick to the
button (in the render that maps over plan) that calls props.onComplete() and
then triggers the lesson to advance (call the component's existing advance
method/prop — e.g., props.onAdvance() or the shell advance function used
elsewhere in this component) so selecting a plan both arms Continue and
immediately advances the lesson; apply the same fix to the other plan button
group around lines 106-112.
Comment on lines
+85
to
+89
| setRemovingIds(new Set<string>([focused.id])); | ||
| setTimeout(() => { | ||
| removeSandboxEntity(focused.id); | ||
| setRemovingIds(new Set<string>()); | ||
| }, REMOVE_ANIMATION_MS); |
Contributor
There was a problem hiding this comment.
Cancel the delayed removal when the lesson unmounts.
This setTimeout can fire after teardown. If the user presses E and immediately continues or skips, the old lesson still calls removeSandboxEntity(...) 180ms later and mutates shared sandbox state from a dead component.
🧹 Proposed fix
+ let pendingRemovalTimer: number | undefined;
+
onMount(() => {
registerHotkey({
scopeId: props.scopeId,
hotkey: 'e',
description: 'Mark done',
keyDownHandler: () => {
+ if (pendingRemovalTimer !== undefined) return false;
if (!hasNavigated()) return false;
const focused = soup.focus.item();
if (!focused) return false;
const currentIndex = soup.focus.index();
const data = soup.items.data();
@@
// Animate the row away, then remove from the store
setRemovingIds(new Set<string>([focused.id]));
- setTimeout(() => {
+ pendingRemovalTimer = window.setTimeout(() => {
removeSandboxEntity(focused.id);
setRemovingIds(new Set<string>());
+ pendingRemovalTimer = undefined;
}, REMOVE_ANIMATION_MS);
@@
onCleanup(() => {
+ if (pendingRemovalTimer !== undefined) {
+ window.clearTimeout(pendingRemovalTimer);
+ }
group.dispose();
setSharedSoup(undefined);
setRemovingIds(new Set<string>());
setSidebarFilter(previousFilter);
});Also applies to: 100-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@js/app/packages/app/component/interactive-onboarding/lessons/navigate-list.tsx`
around lines 85 - 89, The delayed removal uses setTimeout with
REMOVE_ANIMATION_MS and can run after the component unmounts; fix by capturing
the timeout id when calling setTimeout in the handlers that call
setRemovingIds(...) and removeSandboxEntity(focused.id) (references:
setRemovingIds, removeSandboxEntity, REMOVE_ANIMATION_MS, focused) and clear
that timeout on unmount (or when the lesson changes) using a cleanup (e.g.,
store the timer id in a ref and call clearTimeout(timerRef.current) in a
useEffect cleanup); apply the same pattern to the other occurrence around lines
100-105 so no stale timeouts mutate sandbox state after teardown.
Comment on lines
+12
to
+18
| function SidebarNavContent(props: LessonContentProps) { | ||
| const [done, setDone] = createSignal(false); | ||
| createEffect(() => { | ||
| if (sidebarFilter() === 'mail') { | ||
| setDone(true); | ||
| props.onComplete(); | ||
| } |
Contributor
There was a problem hiding this comment.
Reset and restore sidebarFilter() around this lesson.
This lesson now uses the global sidebar filter as both its completion trigger and its data source. If onboarding enters this step with 'mail' already selected, it auto-completes immediately; if the user leaves it on 'mail', that state leaks into the next lesson. Add the same snapshot/reset/restore pattern used in create-entity.tsx and markdown-mentions.tsx.
Also applies to: 40-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/app/packages/app/component/interactive-onboarding/lessons/sidebar-nav.tsx`
around lines 12 - 18, Snapshot the current global sidebarFilter() when
SidebarNavContent mounts, temporarily ensure it's not already 'mail' (if prev
=== 'mail' set sidebarFilter('') then set sidebarFilter('mail') to trigger the
lesson), then run the existing createEffect that watches sidebarFilter() and
calls setDone(true) and props.onComplete() when it becomes 'mail'; after
completion and in cleanup restore the original snapshot value. Implement the
same snapshot/reset/restore pattern used in create-entity.tsx and
markdown-mentions.tsx: capture prev = sidebarFilter(), adjust to avoid immediate
auto-complete, trigger the lesson, and always restore sidebarFilter(prev) on
onCleanup or after props.onComplete().
Comment on lines
+15
to
+26
| <style>{` | ||
| @keyframes onboarding-entity-remove { | ||
| 0% { opacity: 1; transform: translateX(0); max-height: 48px; } | ||
| 60% { opacity: 0; transform: translateX(-12px); max-height: 48px; } | ||
| 100% { opacity: 0; transform: translateX(-12px); max-height: 0; padding-top: 0; padding-bottom: 0; } | ||
| } | ||
| .onboarding-entity-removing { | ||
| overflow: hidden; | ||
| pointer-events: none; | ||
| animation: onboarding-entity-remove 180ms ease-in forwards; | ||
| } | ||
| `}</style> |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inline styles work but could be extracted.
The @keyframes and .onboarding-entity-removing styles are defined inline. For better maintainability, consider moving these to a shared CSS file or using a CSS-in-JS solution if the project has one.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@js/app/packages/app/component/interactive-onboarding/OnboardingEntityList.tsx`
around lines 15 - 26, The inline CSS in OnboardingEntityList.tsx defines a
keyframe animation onboarding-entity-remove and the .onboarding-entity-removing
rule; extract these into the project's shared stylesheet (or the project's
CSS-in-JS location) and replace the <style> block with a class reference.
Specifically, move the `@keyframes` onboarding-entity-remove and
.onboarding-entity-removing declarations into the shared CSS/CSS-in-JS module,
import or apply that stylesheet/module in the OnboardingEntityList component,
and ensure the component continues to add the className
"onboarding-entity-removing" where needed so the animation still applies.
Comment on lines
+64
to
+127
| <Show | ||
| when={entity.type === 'email'} | ||
| fallback={ | ||
| <> | ||
| <span class="size-1.5 shrink-0" /> | ||
| <div class="size-4 shrink-0"> | ||
| <Entity.Icon entity={entity as EntityData} /> | ||
| </div> | ||
| <Entity.Title entity={entity as EntityData} /> | ||
| <span class="ml-auto font-mono font-light uppercase tracking-wide text-xs text-ink/40 shrink-0"> | ||
| <Entity.Timestamp entity={entity as EntityData} /> | ||
| </span> | ||
| </> | ||
| } | ||
| > | ||
| {/* Fixed-width: unread dot + icon + sender */} | ||
| <div class="w-[22ch] shrink-0 flex items-center gap-2 min-w-0"> | ||
| <span | ||
| class={cn('size-1.5 rounded-full bg-accent shrink-0', { | ||
| 'opacity-0': ( | ||
| entity as EntityData & { isRead: boolean } | ||
| ).isRead, | ||
| })} | ||
| /> | ||
| <div class="size-4 shrink-0"> | ||
| <Entity.Icon entity={entity as EntityData} /> | ||
| </div> | ||
| <span | ||
| class={cn('truncate', { | ||
| 'text-ink font-semibold': !( | ||
| entity as EntityData & { isRead: boolean } | ||
| ).isRead, | ||
| 'text-ink/60 font-normal': ( | ||
| entity as EntityData & { isRead: boolean } | ||
| ).isRead, | ||
| })} | ||
| > | ||
| { | ||
| (entity as EntityData & { senderName: string }) | ||
| .senderName | ||
| } | ||
| </span> | ||
| </div> | ||
| <span class="flex-1 flex items-baseline gap-2 min-w-0 truncate"> | ||
| <span | ||
| class={cn('truncate shrink-0 max-w-[40%]', { | ||
| 'font-semibold': !( | ||
| entity as EntityData & { isRead: boolean } | ||
| ).isRead, | ||
| 'font-normal text-ink/70': ( | ||
| entity as EntityData & { isRead: boolean } | ||
| ).isRead, | ||
| })} | ||
| > | ||
| <Entity.Title entity={entity as EntityData} /> | ||
| </span> | ||
| <span class="truncate text-ink/50 font-normal"> | ||
| {(entity as EntityData & { snippet: string }).snippet} | ||
| </span> | ||
| </span> | ||
| <span class="ml-auto font-mono font-light uppercase tracking-wide text-xs text-ink/40 shrink-0"> | ||
| <Entity.Timestamp entity={entity as EntityData} /> | ||
| </span> | ||
| </Show> |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Reduce repetitive type assertions by narrowing once.
The email-specific rendering block has multiple identical type assertions for isRead, senderName, and snippet. Consider narrowing the type once at the start of the block.
Suggested refactor
<Show
when={entity.type === 'email'}
fallback={
// ... non-email fallback
}
>
+ {(() => {
+ // Type narrow for email entity
+ const email = entity as EntityData & {
+ isRead: boolean;
+ senderName: string;
+ snippet: string;
+ };
+ return (
{/* Fixed-width: unread dot + icon + sender */}
<div class="w-[22ch] shrink-0 flex items-center gap-2 min-w-0">
<span
class={cn('size-1.5 rounded-full bg-accent shrink-0', {
- 'opacity-0': (
- entity as EntityData & { isRead: boolean }
- ).isRead,
+ 'opacity-0': email.isRead,
})}
/>
{/* ... rest of the email rendering using `email.isRead`, `email.senderName`, `email.snippet` */}
+ );
+ })()}
</Show>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@js/app/packages/app/component/interactive-onboarding/OnboardingEntityList.tsx`
around lines 64 - 127, The email rendering block repeats the same type
assertions for isRead, senderName, and snippet; narrow the entity once at the
top of the Show branch to a typed local (e.g., const e = entity as EntityData &
{ isRead: boolean; senderName: string; snippet: string }) and replace all
repeated (entity as ...) usages with e when computing classes and rendering
values; keep usages of Entity.Icon, Entity.Title, and Entity.Timestamp but pass
e (or e cast back to EntityData where needed) and remove duplicate assertions to
simplify the JSX.
Comment on lines
+231
to
+254
| function matchesFilter( | ||
| entity: EntityData, | ||
| filter: SandboxSidebarFilter | ||
| ): boolean { | ||
| if (!filter) return true; | ||
| switch (filter) { | ||
| case 'empty': | ||
| return false; | ||
| case 'agents': | ||
| return entity.type === 'chat'; | ||
| case 'mail': | ||
| return entity.type === 'email'; | ||
| case 'documents': | ||
| return entity.type === 'document' && entity.subType?.type !== 'task'; | ||
| case 'tasks': | ||
| return entity.type === 'document' && entity.subType?.type === 'task'; | ||
| case 'channels': | ||
| return entity.type === 'channel'; | ||
| case 'folders': | ||
| return entity.type === 'project'; | ||
| default: | ||
| return true; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using match from ts-pattern for exhaustive switch handling.
Per coding guidelines, exhaustive switch statements should use match from ts-pattern. The default case here silently returns true for any unhandled filter values, which could mask bugs if new filter types are added.
Suggested refactor using ts-pattern
+import { match } from 'ts-pattern';
+
function matchesFilter(
entity: EntityData,
filter: SandboxSidebarFilter
): boolean {
if (!filter) return true;
- switch (filter) {
- case 'empty':
- return false;
- case 'agents':
- return entity.type === 'chat';
- case 'mail':
- return entity.type === 'email';
- case 'documents':
- return entity.type === 'document' && entity.subType?.type !== 'task';
- case 'tasks':
- return entity.type === 'document' && entity.subType?.type === 'task';
- case 'channels':
- return entity.type === 'channel';
- case 'folders':
- return entity.type === 'project';
- default:
- return true;
- }
+ return match(filter)
+ .with('empty', () => false)
+ .with('agents', () => entity.type === 'chat')
+ .with('mail', () => entity.type === 'email')
+ .with('documents', () => entity.type === 'document' && entity.subType?.type !== 'task')
+ .with('tasks', () => entity.type === 'document' && entity.subType?.type === 'task')
+ .with('channels', () => entity.type === 'channel')
+ .with('folders', () => entity.type === 'project')
+ .exhaustive();
}As per coding guidelines: "For exhaustive switch statements use match from ts-pattern".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function matchesFilter( | |
| entity: EntityData, | |
| filter: SandboxSidebarFilter | |
| ): boolean { | |
| if (!filter) return true; | |
| switch (filter) { | |
| case 'empty': | |
| return false; | |
| case 'agents': | |
| return entity.type === 'chat'; | |
| case 'mail': | |
| return entity.type === 'email'; | |
| case 'documents': | |
| return entity.type === 'document' && entity.subType?.type !== 'task'; | |
| case 'tasks': | |
| return entity.type === 'document' && entity.subType?.type === 'task'; | |
| case 'channels': | |
| return entity.type === 'channel'; | |
| case 'folders': | |
| return entity.type === 'project'; | |
| default: | |
| return true; | |
| } | |
| } | |
| import { match } from 'ts-pattern'; | |
| function matchesFilter( | |
| entity: EntityData, | |
| filter: SandboxSidebarFilter | |
| ): boolean { | |
| if (!filter) return true; | |
| return match(filter) | |
| .with('empty', () => false) | |
| .with('agents', () => entity.type === 'chat') | |
| .with('mail', () => entity.type === 'email') | |
| .with('documents', () => entity.type === 'document' && entity.subType?.type !== 'task') | |
| .with('tasks', () => entity.type === 'document' && entity.subType?.type === 'task') | |
| .with('channels', () => entity.type === 'channel') | |
| .with('folders', () => entity.type === 'project') | |
| .exhaustive(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@js/app/packages/app/component/interactive-onboarding/sandbox/sandbox-store.ts`
around lines 231 - 254, The matchesFilter function currently uses a switch with
a default that masks unhandled SandboxSidebarFilter values; replace the switch
with ts-pattern's match to get exhaustiveness checking: import { match } from
'ts-pattern', convert the switch in matchesFilter to a match(filter) that maps
each case ('empty','agents','mail','documents','tasks','channels','folders') to
the same boolean expressions (using entity.type and entity.subType?.type) and
omit a fallback so the compiler will flag any new/unhandled SandboxSidebarFilter
variants (or explicitly handle unknown with a sensible return if needed).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.