feat: showcase app with interactive demos and theme palettes#33
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Changes
Sequence Diagram(s)(Section omitted — conditions for generating diagrams not met.) Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 59
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/showcase/src/demos/ai-chat/components/ArtifactPanel.tsx`:
- Around line 37-41: The AIArtifactAction "Apply to editor" is rendered without
a handler; either wire it to a real callback or disable/remove it. In
ArtifactPanel, add an onClick/onAction prop to the AIArtifactAction (e.g., call
a new applyToEditor handler or an existing onApplyToEditor prop) that performs
the editor-apply logic (or delegates to the parent), or mark the control
disabled/hidden until implementation is available; ensure you reference the
AIArtifactAction component and implement the applyToEditor function or pass
through a prop so the button is not a dead action.
In `@apps/showcase/src/demos/ai-chat/components/MessageRenderer.tsx`:
- Around line 56-59: The AIMessageActions component is being rendered with a
no-op onRegenerate handler in MessageRenderer; remove the empty onRegenerate
prop or wire it to a real handler passed via props (e.g., propagate a
onRegenerate prop from MessageRenderer's props and pass that through) so the UI
does not show a non-functional Regenerate action; update the MessageRenderer
render of AIMessageActions to either omit onRegenerate or forward a concrete
handler (and update the component props signature accordingly).
- Around line 38-40: handleCopy currently always passes message.content to
onCopy, but the UI sometimes renders branch-specific text (the branch rendering
logic at the block around lines 93–105), so users copy the wrong text; update
handleCopy to compute the exact displayed text (use the same source/logic as the
branch rendering — e.g., derive displayedText from
message.branches/selectedBranch or the same helper used to render branch
content) and pass that displayedText to onCopy instead of message.content,
ensuring the copy payload matches what the user sees.
In `@apps/showcase/src/demos/ai-chat/hooks/useScriptedReplay.ts`:
- Around line 30-43: The hook useScriptedReplay currently schedules timeouts
(via schedule and timeoutsRef) but never cleans them up on unmount; add a
useEffect that imports useEffect and returns a cleanup function which calls
clearAllTimeouts to cancel pending timers when the component unmounts (e.g.
useEffect(() => () => clearAllTimeouts(), [clearAllTimeouts])); this ensures
timeouts scheduled by schedule are cleared and prevents setState from being
called after unmount.
In `@apps/showcase/src/demos/ai-chat/index.tsx`:
- Around line 150-224: The disabled exhaustive-deps lint for the useCallback
renderItem is risky because staticMessages and replayItems are recomputed each
render; either add a short inline comment next to the eslint-disable explaining
why the rule is intentionally suppressed (e.g., staticMessages/replayItems are
stable/derived from state and including them would cause unnecessary
re-creates), or refactor renderItem to derive/staticize staticMessages and
replayItems outside the render (or include them in the dependency array) so you
can remove the eslint-disable; reference the renderItem callback, useCallback,
staticMessages, replayItems, state, showThinking, showToolCall, showStreaming,
and the eslint-disable-next-line react-hooks/exhaustive-deps marker when making
the change.
- Line 119: The boolean expression computing isReplaying is correct but unclear
due to mixed && and || precedence; update the assignment for isReplaying (the
variable) to add explicit parentheses around the grouped conditions involving
state.phase and replayUserMessage so the intent is obvious—for example, group
(state.phase !== 'idle' && state.phase !== 'done') and (state.phase === 'done'
&& replayUserMessage != null) with parentheses around each before combining with
|| to make the logical grouping explicit.
In `@apps/showcase/src/demos/chat-app/components/ChannelHeader.tsx`:
- Around line 25-33: The header renders three IconButton controls in
ChannelHeader.tsx (the Search, Pinned messages, and Channel info buttons) with
no handlers; mark them as disabled or add noop/real onClick handlers to avoid
dead actions. Update the IconButton instances (the ones wrapping LuSearch,
LuPin, LuInfo) to either set the disabled prop (e.g., isDisabled or disabled
depending on your UI lib) and provide an aria-disabled label, or attach
appropriate onClick callbacks (even a no-op that opens a placeholder menu) so
the buttons are not clickable with no feedback. Ensure accessibility attributes
(aria-label remains accurate) are preserved.
In `@apps/showcase/src/demos/chat-app/components/ChannelSidebar.tsx`:
- Around line 48-50: The DM peer lookup is incorrectly using
workspace.users[0]?.id as the local user; change the logic in the dms.map block
(where otherUserId is computed and otherUser resolved via getUserById) to use
the actual current user id instead of workspace.users[0]?.id — obtain this id
from the real current user (e.g., a currentUser or currentUserId prop,
auth/context getter) and compute otherUserId = dm.members.find(id => id !==
currentUserId) then call getUserById(workspace.users, otherUserId); ensure the
component receives or reads the true current user id reliably.
In `@apps/showcase/src/demos/chat-app/components/MessageComposer.tsx`:
- Around line 38-43: The Enter key handler in handleKeyDown currently sends on
Enter without checking IME composition state, which can send partial text for
IME users; add an isComposing boolean state (e.g., via useState or useRef)
updated by onCompositionStart and onCompositionEnd handlers on the textarea, and
modify handleKeyDown to skip sending when isComposing is true (i.e., require
!isComposing && e.key === 'Enter' && !e.shiftKey before calling handleSend),
ensuring composition events (onCompositionStart/onCompositionEnd) are wired to
the textarea component.
In `@apps/showcase/src/demos/chat-app/components/MessageItem.tsx`:
- Around line 207-222: The thread indicator button in MessageItem (the JSX that
renders when message.replyCount > 0) lacks an explicit type and may act as a
submit button when inside a form; update the <button> that uses
styles.threadIndicator and onClick={() => onOpenThread(message.id)} to include
type="button" so it does not trigger form submission unexpectedly. Ensure you
add the attribute to the same button element that renders
threadAvatarUsers/Avatar and message.replyCount.
- Around line 132-139: The render prop for Menu.Trigger currently destructures
the color prop as `_` which ESLint still flags; update the render prop to avoid
extracting an unused `color` — either drop the explicit destructuring and accept
the whole props object (e.g., render={(props) => ...}) or rename the variable to
an ESLint-recognized unused name like `_color`; apply this change where
Menu.Trigger's render prop is defined so IconButton receives the forwarded props
(IconButton and LuEllipsis referenced).
In `@apps/showcase/src/demos/chat-app/components/MessageList.tsx`:
- Around line 62-67: The header visibility currently only uses
shouldShowHeader(msg, messages[i - 1]) which allows a message that starts a new
date (dateKey !== lastDateKey / showDateSep) to still render compact; change the
showHeader logic to force a header when showDateSep is true by computing
showHeader as showDateSep || shouldShowHeader(msg, messages[i - 1]) (references:
dateKey, showDateSep, lastDateKey, shouldShowHeader, messages, showHeader).
In `@apps/showcase/src/demos/chat-app/components/ReactionBar.tsx`:
- Around line 18-30: In ReactionBar.tsx update the two button elements used for
reactions and the add button to be explicit buttons and accessible: add
type="button" to both the reaction buttons (the button rendered in the map that
uses r.emoji, r.userIds and onToggle) and the add button (className
styles.reactionAdd, onClick={() => onToggle('👍')}), add aria-pressed to the
reaction toggle buttons (set to true when r.userIds.includes(currentUserId)),
and give the icon-only add button a clear accessible label (e.g.,
aria-label="Add reaction") so screen readers can identify it.
In `@apps/showcase/src/demos/chat-app/components/ThreadPanel.tsx`:
- Around line 33-35: The useEffect that calls endRef.current?.scrollIntoView({
behavior: 'smooth' }) triggers on any change to replies.length (including
initial mount), so update the effect in ThreadPanel to only auto-scroll when
there are replies: add a guard like checking replies.length > 0 before calling
scrollIntoView, or implement a small previous-length check (store
prevRepliesLength in a ref and compare to replies.length) to ensure scroll only
runs on new replies rather than on initial render; keep references to useEffect,
endRef, and replies.length to locate and modify the logic.
In `@apps/showcase/src/demos/chat-app/components/TypingIndicator.tsx`:
- Around line 16-24: The typing indicator lacks live-region semantics so screen
readers may miss updates; update the TypingIndicator component's container (the
div with className styles.typingIndicator or the containing span) to include a
live region such as aria-live="polite" and aria-atomic="true" (or role="status")
so changes to {names} and {verb} are announced; ensure the attributes are
applied to the element that updates text (the span containing
<strong>{names}</strong> {verb} typing…) so assistive tech receives the dynamic
updates.
In `@apps/showcase/src/demos/chat-app/components/WorkspaceRail.tsx`:
- Around line 43-45: The Add workspace button in WorkspaceRail (the <button
className={styles.railAddButton} aria-label="Add workspace"> element) lacks an
explicit type and will default to submit inside forms; update that button to
include type="button" to prevent unintended form submissions and preserve its
click-only behavior.
In `@apps/showcase/src/demos/chat-app/data.ts`:
- Around line 336-343: formatDateSeparator miscalculates "Yesterday" across
month/year boundaries; update it to compare date-only boundaries by normalizing
times to the start of day. In formatDateSeparator, create todayStart = new
Date(now.getFullYear(), now.getMonth(), now.getDate()).getTime() and
yesterdayStart = todayStart - 24*60*60*1000, then get targetStart = new
Date(date.getFullYear(), date.getMonth(), date.getDate()).getTime(); return
'Today' if targetStart === todayStart, 'Yesterday' if targetStart ===
yesterdayStart, otherwise return date.toLocaleDateString(...). This replaces the
diffDays/getDate subtraction logic and ensures correct behavior across
month/year boundaries.
In `@apps/showcase/src/demos/chat-app/hooks/useChat.ts`:
- Around line 32-35: The module-level mutable counter nextMsgId used by
makeMsgId causes shared state across hook instances and tests; change the hook
to accept an ID generator parameter or move the counter into hook-local state
(e.g., useRef) so each useChat gets its own sequence. Specifically, replace the
global nextMsgId/makeMsgId usage in useChat with either an injected idGen
callback parameter (used by message creation logic) or create a const idRef =
useRef(startingValue) inside the hook and derive IDs via idRef.current++ where
makeMsgId is referenced.
- Around line 57-65: Replace the unsafe non-null assertions by making
activeWorkspace and activeChannel handle empty arrays: change the logic in
useMemo for activeWorkspace (which currently uses workspaces and
activeWorkspaceId) to return workspaces.find(...) ?? workspaces[0] ?? undefined
(i.e., allow undefined instead of using !) and similarly for activeChannel (use
activeWorkspace?.channels.find(...) ?? activeWorkspace?.channels?.[0] ??
undefined); then update any consuming code to handle the possible undefined
values (or provide a clear documented invariant/fallback) so you no longer
assume non-empty workspaces or channels in useChat.
In `@apps/showcase/src/demos/chat-app/index.module.css`:
- Line 301: Replace the deprecated declaration "word-break: break-word;" with
the modern equivalent (e.g., "overflow-wrap: anywhere;" and, if needed for
compatibility, keep "word-break: break-all;" as a considered fallback) in the
message-wrapping CSS so lint no longer flags the property; also rename the
`@keyframes` declarations referenced around lines 529 and 540 to conform to the
repository's keyframe naming rule (use the project's required
kebab-case/lowercase format) and update all corresponding
animation-name/animation shorthand usages to the new keyframe names so the
linter accepts them.
- Line 414: The rule mismatch is caused by CSS Modules composition usage
"composes: reaction;" in the CSS (class using composes) conflicting with
stylelint; either enable CSS Modules support in stylelint (install and configure
stylelint-css-modules-plugin or set "ignoreFiles"/disable property validation in
.stylelintrc.json) so composes is accepted, or remove the composes usage and
update the JSX/TSX to apply both classes explicitly (use both .reaction and
.reactionAdd on the element) — locate the composes line in index.module.css and
choose one of these fixes, ensuring the class names reaction and reactionAdd are
kept consistent between CSS and TSX.
In `@apps/showcase/src/demos/container-management/components/ContainerList.tsx`:
- Around line 115-123: Replace the non-semantic clickable <div> used in
ContainerList.tsx with a <button type="button"> so keyboard users and screen
readers can activate the container; keep the className={styles.nameCell}, inline
style, the onClick handler that calls onSelectRef.current(row), and the
StatusDot + <span>{row.name}</span> children, and ensure there is an appropriate
accessible label (e.g., aria-label or visible text) so
containerStatusColor(row.status)/StatusDot remain intact and activation works
via keyboard.
In `@apps/showcase/src/demos/container-management/components/tabs/FilesTab.tsx`:
- Around line 19-23: expandedKeys and selectedKeys are only initialized from
container once and can become stale when container changes; add an effect in
FilesTab that watches the container (or container.id) and resets
setExpandedKeys(new Set(container.filesystem.map(n => n.id))) and
setSelectedKeys(new Set()) whenever the container changes; update the component
to import/use useEffect and reference the expandedKeys/setExpandedKeys and
selectedKeys/setSelectedKeys state setters so the tree state is re-derived from
the new container.
In `@apps/showcase/src/demos/container-management/components/tabs/StatsTab.tsx`:
- Around line 67-69: Guard against division-by-zero when computing the CPU
average in the Card.KeyValue rendering: check stats.cpuHistory.length before
dividing (use a conditional or fallback) and only call toFixed(1) on a numeric
value (or render a placeholder like "0.0" or "-" when length is 0). Update the
expression that computes (stats.cpuHistory.reduce(...) /
stats.cpuHistory.length).toFixed(1) to handle the empty-array case safely while
keeping the percent sign and label unchanged.
In `@apps/showcase/src/demos/container-management/data.ts`:
- Around line 206-214: Narrow the input type of containerStatusColor to a union
of the valid container status strings instead of string so the compiler can
check exhaustiveness; change the status parameter type in the
containerStatusColor function signature to the union 'running' | 'stopped' |
'paused' | 'restarting' | 'exited' (or an exported type alias like
ContainerStatus used across callers), keep returns as StatusDotStatus, and
update any call sites that pass arbitrary strings to ensure they supply one of
the valid union members.
- Around line 563-572: clusterStats currently computes avgCpu by dividing the
sum of container.cpu by containers.filter((c) => c.status === 'running').length
which will divide by zero when no containers are running; update the avgCpu
calculation in clusterStats to guard against zero running containers (using
containers and the status filter) by computing the runningCount first and
returning 0 (or NaN per project convention) when runningCount is 0, otherwise
divide the cpu sum by runningCount so avgCpu is always valid.
In `@apps/showcase/src/demos/container-management/types.ts`:
- Around line 41-47: The FileSystemNode interface allows invalid combinations
(e.g., a node with type 'file' having children); replace it with a discriminated
union such as defining a FileNode and DirectoryNode (e.g., interface FileNode {
id: string; name: string; type: 'file'; size?: number } and interface
DirectoryNode { id: string; name: string; type: 'directory'; children?:
DirectoryNode[] | FileNode[] }) and export type FileSystemNode = FileNode |
DirectoryNode; update any code that inspects node.type to narrow by the literal
type and adjust references to FileSystemNode, FileNode, and DirectoryNode
accordingly so children exist only on directories and size only on files.
In `@apps/showcase/src/demos/file-explorer/components/CommandLog.tsx`:
- Around line 26-30: The toggle button in the CommandLog component lacks an
explicit type and may submit a parent form; update the button element (the one
using className={styles.toggle} with onClick={() => setCollapsed((c) => !c)} and
aria-expanded={!collapsed}) to include type="button" to prevent accidental form
submission.
In `@apps/showcase/src/demos/file-explorer/components/FilePane.tsx`:
- Around line 242-244: Replace the incorrect LuArrowUp icon used for the Rename
action in the FilePane component: update the IconButton with a semantic
rename/edit icon (e.g., LuTextCursor or LuPencil) and ensure the chosen icon is
imported at the top of the file; specifically, change the icon instance inside
the IconButton whose onClick calls handleAction('Rename') and add the
corresponding import for LuTextCursor (or reuse LuPencil if preferred) so the UI
accurately reflects the Rename action.
- Around line 196-214: The column accessors in FileTable.Column use the unsafe
type "any"; update them to use the concrete FileNode type so TypeScript can
check fields like owner, group, and permissions. Locate the two FileTable.Column
declarations (the ones with id="owner" and id="permissions") in FilePane.tsx and
change the accessor signatures from (item: any) => ... to (item: FileNode) =>
... and import or reference the FileNode type if needed so owner, group, and
permissions are properly type-checked.
In `@apps/showcase/src/demos/file-explorer/components/TransferPanel.tsx`:
- Around line 31-40: The table header cells in TransferPanel (the <th> elements
for "Source", "Direction", "Destination", "Size", "Priority", "Status",
"Remaining", "Speed", "Progress") need explicit scope attributes for
accessibility; update each <th> in the TransferPanel component to include
scope="col" so screen readers correctly identify them as column headers, keeping
all other markup and text unchanged.
- Around line 74-83: The current code runs multiple full-array filters (counts
plus queued/failed/completed) causing unnecessary O(n) repeats; replace the
separate useMemo calls with a single useMemo over transfers that performs one
reduce/pass to build queued, failed, completed arrays and counts object,
returning both grouped lists and counts (e.g., an object {queued, failed,
completed, counts}) and update usages of the existing queued, failed, completed,
and counts variables to read from that single memoized result (referencing the
existing symbols counts, queued, failed, completed, useMemo, and transfers).
In `@apps/showcase/src/demos/file-explorer/components/TreePane.module.css`:
- Around line 9-20: Replace the hardcoded px values with design tokens and
remove the TODO: change the padding declaration that currently mixes "4px" and
"var(--ov-space-stack-sm)" to use tokens e.g. "padding: var(--ov-space-stack-xs)
var(--ov-space-stack-sm);" (update the existing TODO comment removal), change
".label { font-size: 10px; }" to use the font-size token (e.g. "font-size:
var(--ov-font-size-xxs);" and change "margin-bottom: 2px;" to a spacing token
(e.g. "margin-bottom: var(--ov-space-stack-xxs);"); update any token names to
match your theme variables and ensure .label and the padding rule are the only
places modified.
In `@apps/showcase/src/demos/file-explorer/components/TreePane.tsx`:
- Around line 81-83: The outer container div that renders the pane uses
aria-label={label} but lacks an explicit ARIA role; update the element rendered
in TreePane (the div with className={styles.pane} and aria-label={label}) to
include a role attribute (e.g., role="region") so the aria-label is recognized
by assistive tech; ensure you add the role directly on that div in the TreePane
component.
In `@apps/showcase/src/demos/ide-editor/components/IconStrip.tsx`:
- Line 25: Replace the non-semantic wrapper in IconStrip.tsx: change the outer
element that currently reads as <div className={styles.strip} role="navigation"
aria-label="Sidebar panels"> to use a semantic <nav> element instead, preserving
className={styles.strip} and aria-label="Sidebar panels" (remove the redundant
role attribute); update the JSX in the IconStrip component so the navigation
container is a <nav> with the same props.
In `@apps/showcase/src/demos/ide-editor/components/SidebarPanel.tsx`:
- Around line 93-127: SearchPanel exposes interactive result rows but drops the
onOpenFile prop and leaves click/keyboard handlers as no-ops; wire the passed-in
onOpenFile (SearchPanel's onOpenFile prop) into the result item handlers so
clicking a result calls onOpenFile(result.file, result.line) (or similar
signature used elsewhere), and update the onKeyDown handler to invoke the same
call when e.key is 'Enter' or ' ' (and call e.preventDefault() as needed);
ensure the element conveys its action via an aria-label or replace the div with
a button if appropriate and remove the noop handlers so keyboard and
screen-reader users can open files from the SearchPanel.
In `@apps/showcase/src/demos/ide-editor/index.tsx`:
- Around line 63-66: The keyboard shortcut check in the keydown handler
currently matches only lowercase 'k' and can miss uppercase input; update the
condition that checks e.key (where you toggle setShowPalette) to compare a
normalized value like e.key.toLowerCase() === 'k' (and guard that e.key exists)
so Ctrl/Cmd+K works regardless of keyboard state.
- Around line 96-109: handleCloseTab computes the next active tab using the
outer `tabs` variable which can be stale; change it to derive the new active id
from the same functional update used to update tabs so both use the same
snapshot. In practice, inside handleCloseTab use setTabs(prev => { const next =
prev.filter(t => t.id !== tabId); setActiveTabId(current => { const idx =
prev.findIndex(t => t.id === tabId); const remaining = next; if (current !==
tabId) return current; if (remaining.length === 0) return ''; const nextIdx =
Math.min(idx, remaining.length - 1); return remaining[nextIdx]?.id ?? ''; });
return next; }); also remove `tabs` from the useCallback dependency list so
handleCloseTab doesn't capture a stale `tabs` reference (referencing
handleCloseTab, setTabs, setActiveTabId, tabs).
In `@apps/showcase/src/demos/ide-editor/types.ts`:
- Line 5: The IDEFile.language field is currently typed as string; narrow it to
an explicit union of supported Monaco language IDs (e.g. "typescript" |
"javascript" | "css" | "json" | "markdown", etc.) to improve type safety when
wiring editor modes—update the IDEFile type definition and/or introduce a
SupportedLanguage type/alias and then fix any usages (constructors, factory
functions, props, and editor-mode mapping code) to use that union so the
compiler enforces only supported language IDs.
In `@apps/showcase/src/demos/web-browser/components/BrowserToolbar.tsx`:
- Line 31: The code hardcodes the new-tab sentinel string when computing
displayUrl; import and use the shared sentinel constant instead of the literal.
Replace the expression url === 'about:newtab' in the BrowserToolbar component
(where displayUrl is set) with a comparison against the shared constant (e.g.,
NEW_TAB_SENTINEL or the project's named export) and add the corresponding import
for that constant so the check stays in sync if the sentinel value changes.
In `@apps/showcase/src/demos/web-browser/components/ErrorPage.tsx`:
- Around line 13-16: The JSX in ErrorPage.tsx uses an unescaped apostrophe in
the heading text and a button without an explicit type; change the heading
content to avoid unescaped characters (e.g., use "cannot", use an escaped
apostrophe ', or wrap the string in braces like {"can't"}) and add
type="button" to the button element that uses the onNewTab handler (identify the
<h2 className={styles.errorTitle}> and <button className={styles.errorButton}
onClick={onNewTab}> elements to locate the changes).
In `@apps/showcase/src/demos/web-browser/components/NewTabPage.tsx`:
- Around line 38-42: Add an explicit type="button" attribute to the rendered
button in NewTabPage.tsx (the element using key={site.id},
className={styles.speedDialCard}, and onClick={() => onNavigate(site.url)}) so
the button's intent is unambiguous and follows semantic best practices; update
that JSX button element to include type="button".
In `@apps/showcase/src/demos/web-browser/data.ts`:
- Line 10: SPEED_DIAL_SITES currently aliases DEFAULT_BOOKMARKS, creating a
shared mutable reference; change the export to provide an independent copy
instead (e.g., create a shallow copy or deep-clone the array/objects) so
mutations to SPEED_DIAL_SITES won’t affect DEFAULT_BOOKMARKS; update the export
of SPEED_DIAL_SITES to return a cloned array (or cloned items) rather than
referencing DEFAULT_BOOKMARKS directly.
- Around line 20-23: The function ensureProtocol currently only checks for
http(s) and will incorrectly rewrite valid non-HTTP schemes like "about:newtab";
update ensureProtocol so it first returns input unchanged when the string
already has any URI scheme (match /^[a-zA-Z][\w+\-.]*:/) or is protocol-relative
(starts with "//"), otherwise if it lacks a scheme prepend "https://" as before;
adjust the logic in ensureProtocol to check these cases (scheme and
protocol-relative) before adding https://.
In `@apps/showcase/src/demos/web-browser/hooks/useBrowser.ts`:
- Around line 101-116: goBack (and similarly goForward) currently calls
startBlockedTimeout unconditionally after updateActiveTab, causing the timeout
to start even when navigating to NEW_TAB_URL which sets loading:false; fix by
only calling startBlockedTimeout when the new tab will actually be loading
(e.g., compute the target url inside the updater or capture it from the updater
return and call startBlockedTimeout only if url !== NEW_TAB_URL or loading would
be true), or alternatively move the timeout invocation into the updateActiveTab
callback so the decision is made based on the computed new state; update both
goBack and goForward and reference the updateActiveTab, startBlockedTimeout,
NEW_TAB_URL, goBack and goForward symbols.
- Line 21: INITIAL_TAB is created at module scope with createTab(), so all
useBrowser() hook instances share the same initial tab object; move creation of
the initial tab inside the useBrowser hook (e.g., call createTab() within the
hook and pass it to useState via an initializer function) so each hook instance
gets a fresh object reference; update any other module-scope initial tab usages
(the other occurrence using INITIAL_TAB) to use the hook-local initializer
instead, referencing INITIAL_TAB, createTab, and useBrowser to locate the
changes.
- Around line 171-176: In the reorderTabs callback, remove the non-null
assertion on map.get(id) — change map.get(id)! to map.get(id) — since you
already use .filter(Boolean) to drop undefined values; update the return in
reorderTabs inside setTabs to return orderedIds.map(id =>
map.get(id)).filter(Boolean) so the code is clearer and avoids misleading use of
the `!` operator (refer to reorderTabs and the Map created from prev.map((t) =>
[t.id, t])).
In `@apps/showcase/src/demos/web-browser/index.module.css`:
- Around line 114-116: The hover-only style on .speedDialCard (and the similar
hover rule at the later block) lacks a keyboard-visible focus state; add
matching :focus-visible rules for .speedDialCard:focus-visible (and the other
interactive selector's :focus-visible) that apply the same background/visual
treatment as the :hover rule and include an accessible focus indicator (e.g.,
outline or box-shadow) so keyboard users see focus. Update the CSS to include
these :focus-visible selectors alongside the existing :hover selectors and
ensure they target the same class names used in the diff.
In `@apps/showcase/src/demos/web-browser/index.tsx`:
- Around line 28-33: The handleReorder callback currently declares an unused
parameter _meta causing an ESLint warning; update the handleReorder signature to
drop the unused parameter (keep it as (nextTabs: TabDescriptor[]) => { ... })
or, if you must preserve the second parameter for onReorder type compatibility,
rename it to _ or _meta: ReorderMeta and ensure it's referenced only as a
placeholder (or annotate via a comment) so ESLint stops flagging it; adjust the
dependency array and the call to browser.reorderTabs(nextTabs.map(t => t.id))
remains unchanged.
In `@apps/showcase/src/Dock.tsx`:
- Around line 77-79: Replace the non-semantic list markup in Dock.tsx by using
real list elements: change the outer container that uses className={styles.apps}
and role="list" to a <ul> and change the inner elements that use
className={styles.iconWrapper} and role="listitem" to <li>, keeping the existing
key={app.id} and className assignments; ensure any layout or CSS targeting .apps
and .iconWrapper still applies to ul/li (adjust selectors if needed) so the apps
map renders as a semantic list.
In `@apps/showcase/src/global.css`:
- Around line 16-21: Update the global body CSS rule (the body selector) to
include Firefox/macOS font smoothing by adding -moz-osx-font-smoothing:
grayscale; alongside the existing -webkit-font-smoothing: antialiased; so both
WebKit and Firefox benefit from improved text rendering.
In `@apps/showcase/src/Home.module.css`:
- Line 35: Replace the hardcoded border-radius: 8px in Home.module.css with the
shared CSS variable used across the project (e.g.,
var(--ov-primitive-radius-sm)) so it matches other modules; locate the rule that
currently sets "border-radius: 8px" and swap the value to the token
(var(--ov-primitive-radius-sm) or the appropriate project token) to maintain
consistent design tokens.
In `@docs/superpowers/plans/2026-03-12-showcase-demos.md`:
- Around line 1458-1461: The ResizableSplitPane usage sets defaultSize={60}
which the docs state is interpreted as pixels; change this to use a
percentage-based value per the plan (e.g., "60%") or the component's percent
prop (e.g., defaultSizePercent={60}) so the pane is 60% width rather than 60px;
locate the ResizableSplitPane instance in the file and replace the numeric pixel
value with the appropriate percent representation (either a string with '%' or
the component's percent prop name) to align with the documented sizing rule.
- Around line 2396-2398: Replace the machine-specific absolute path command `cd
/Users/joshuapare/Repos/omniviewdev/ui/feat/demos && pnpm build` with a
repo-relative command so others/CI can run it; for example use `cd ui/feat/demos
&& pnpm build` (or even prefer a workspace-aware invocation like `pnpm --filter
./ui/feat/demos run build`), updating the bash snippet so it references the
repo-relative path instead of the absolute path.
In `@docs/superpowers/plans/2026-03-12-showcase-shell.md`:
- Around line 57-58: Replace the absolute, machine-specific cd command ("cd
/Users/joshuapare/Repos/omniviewdev/ui/feat/demos && pnpm install") with a
repo-relative invocation (for example "cd ui/feat/demos && pnpm install" or use
a variable like "$REPO_ROOT/ui/feat/demos && pnpm install") so the steps are
portable in other dev environments and CI; update all other identical
occurrences referenced in the comment (the other command blocks showing absolute
paths) to the same repo-relative pattern and ensure the expected outcomes text
(lockfile updates, workspace links resolve) still apply.
In `@docs/superpowers/plans/2026-03-15-chat-app.md`:
- Around line 18-19: Replace ambiguous workspace filter occurrences of
"showcase" with the exact workspace name "@omniview/showcase" in all plan
commands (for example change the command string "pnpm --filter showcase build
2>&1 | tail -5" to use "@omniview/showcase"); search for and update every
occurrence of the literal filter " --filter showcase " (including the other
listed occurrences) so commands target the specific workspace.
In `@docs/superpowers/specs/2026-03-12-showcase-app-design.md`:
- Around line 17-19: Add a language identifier (e.g., text or plaintext) to all
fenced code blocks in this document so the linter stops flagging them;
specifically update the inline example block containing "main/apps/showcase/"
and the longer file structure block (around the file structure section) and the
layout diagram block (the layout ASCII art) by changing their opening fences
from ``` to ```text (or ```plaintext) so they are treated as plain text by the
linter.
In `@docs/superpowers/specs/2026-03-12-showcase-demos-design.md`:
- Line 5: Update the "Scope:" line in the spec to reflect the demos actually
shipped in this PR by replacing the stale text that lists only four demos and
defers Web Browser, Notes, and Chat App; locate the "Scope:" paragraph in
docs/superpowers/specs/2026-03-12-showcase-demos-design.md (the line starting
with "Scope:") and enumerate all demos shipped in this PR (File Explorer, IDE
Editor, AI Chat, Container Management, Web Browser, Notes, Chat App) or
otherwise adjust the wording to indicate those additional demos are included
rather than deferred.
- Around line 35-51: Add explicit language tags to the fenced code blocks that
contain ASCII diagrams (e.g., the block starting with the box/Toolbar diagram
beginning "┌────────────────" and lines containing "Toolbar: ◀ ▶ ↑ | Search |
Copy Move Delete New Folder"); replace each opening triple backtick ``` with a
language-specified fence like ```text (also update the other diagram blocks
referenced in the review such as the blocks around lines 101-113, 196-216,
297-314, 333-351). Ensure every fenced block that is an ASCII diagram uses
```text so MD040 linting passes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f50ac34d-5cb7-46ff-b777-dc7f4be2eaca
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (147)
.gitignoreapps/showcase/index.htmlapps/showcase/package.jsonapps/showcase/src/App.module.cssapps/showcase/src/App.tsxapps/showcase/src/Dock.module.cssapps/showcase/src/Dock.tsxapps/showcase/src/Home.module.cssapps/showcase/src/Home.tsxapps/showcase/src/demos/ai-chat/components/ArtifactPanel.module.cssapps/showcase/src/demos/ai-chat/components/ArtifactPanel.tsxapps/showcase/src/demos/ai-chat/components/MessageRenderer.module.cssapps/showcase/src/demos/ai-chat/components/MessageRenderer.tsxapps/showcase/src/demos/ai-chat/data.tsapps/showcase/src/demos/ai-chat/hooks/useScriptedReplay.tsapps/showcase/src/demos/ai-chat/index.module.cssapps/showcase/src/demos/ai-chat/index.tsxapps/showcase/src/demos/ai-chat/types.tsapps/showcase/src/demos/chat-app/components/ChannelHeader.tsxapps/showcase/src/demos/chat-app/components/ChannelSidebar.tsxapps/showcase/src/demos/chat-app/components/MessageComposer.tsxapps/showcase/src/demos/chat-app/components/MessageItem.tsxapps/showcase/src/demos/chat-app/components/MessageList.tsxapps/showcase/src/demos/chat-app/components/ReactionBar.tsxapps/showcase/src/demos/chat-app/components/ThreadPanel.tsxapps/showcase/src/demos/chat-app/components/TypingIndicator.tsxapps/showcase/src/demos/chat-app/components/WorkspaceRail.tsxapps/showcase/src/demos/chat-app/data.tsapps/showcase/src/demos/chat-app/hooks/useChat.tsapps/showcase/src/demos/chat-app/index.module.cssapps/showcase/src/demos/chat-app/index.tsxapps/showcase/src/demos/chat-app/types.tsapps/showcase/src/demos/container-management/components/ContainerDetail.module.cssapps/showcase/src/demos/container-management/components/ContainerDetail.tsxapps/showcase/src/demos/container-management/components/ContainerList.module.cssapps/showcase/src/demos/container-management/components/ContainerList.tsxapps/showcase/src/demos/container-management/components/tabs/FilesTab.tsxapps/showcase/src/demos/container-management/components/tabs/InspectTab.module.cssapps/showcase/src/demos/container-management/components/tabs/InspectTab.tsxapps/showcase/src/demos/container-management/components/tabs/LogsTab.module.cssapps/showcase/src/demos/container-management/components/tabs/LogsTab.tsxapps/showcase/src/demos/container-management/components/tabs/StatsTab.module.cssapps/showcase/src/demos/container-management/components/tabs/StatsTab.tsxapps/showcase/src/demos/container-management/data.tsapps/showcase/src/demos/container-management/index.module.cssapps/showcase/src/demos/container-management/index.tsxapps/showcase/src/demos/container-management/types.tsapps/showcase/src/demos/file-explorer/components/CommandLog.module.cssapps/showcase/src/demos/file-explorer/components/CommandLog.tsxapps/showcase/src/demos/file-explorer/components/DetailPanel.module.cssapps/showcase/src/demos/file-explorer/components/DetailPanel.tsxapps/showcase/src/demos/file-explorer/components/ExplorerToolbar.module.cssapps/showcase/src/demos/file-explorer/components/ExplorerToolbar.tsxapps/showcase/src/demos/file-explorer/components/FilePane.module.cssapps/showcase/src/demos/file-explorer/components/FilePane.tsxapps/showcase/src/demos/file-explorer/components/TransferPanel.module.cssapps/showcase/src/demos/file-explorer/components/TransferPanel.tsxapps/showcase/src/demos/file-explorer/components/TreePane.module.cssapps/showcase/src/demos/file-explorer/components/TreePane.tsxapps/showcase/src/demos/file-explorer/data.tsapps/showcase/src/demos/file-explorer/index.module.cssapps/showcase/src/demos/file-explorer/index.tsxapps/showcase/src/demos/file-explorer/types.tsapps/showcase/src/demos/ide-editor/components/IconStrip.module.cssapps/showcase/src/demos/ide-editor/components/IconStrip.tsxapps/showcase/src/demos/ide-editor/components/SidebarPanel.module.cssapps/showcase/src/demos/ide-editor/components/SidebarPanel.tsxapps/showcase/src/demos/ide-editor/data.tsapps/showcase/src/demos/ide-editor/index.module.cssapps/showcase/src/demos/ide-editor/index.tsxapps/showcase/src/demos/ide-editor/types.tsapps/showcase/src/demos/notes/index.tsxapps/showcase/src/demos/web-browser/components/BookmarksBar.tsxapps/showcase/src/demos/web-browser/components/BrowserToolbar.tsxapps/showcase/src/demos/web-browser/components/BrowserViewport.tsxapps/showcase/src/demos/web-browser/components/ErrorPage.tsxapps/showcase/src/demos/web-browser/components/NewTabPage.tsxapps/showcase/src/demos/web-browser/data.tsapps/showcase/src/demos/web-browser/hooks/useBrowser.tsapps/showcase/src/demos/web-browser/index.module.cssapps/showcase/src/demos/web-browser/index.tsxapps/showcase/src/demos/web-browser/types.tsapps/showcase/src/global.cssapps/showcase/src/main.tsxapps/showcase/src/registry.tsapps/showcase/tsconfig.jsonapps/showcase/vite.config.tsdocs/superpowers/plans/2026-03-12-showcase-demos.mddocs/superpowers/plans/2026-03-12-showcase-shell.mddocs/superpowers/plans/2026-03-13-base-ui-components.mddocs/superpowers/plans/2026-03-13-web-browser-demo.mddocs/superpowers/plans/2026-03-15-chat-app.mddocs/superpowers/plans/2026-03-16-theme-palettes.mddocs/superpowers/specs/2026-03-12-showcase-app-design.mddocs/superpowers/specs/2026-03-12-showcase-demos-design.mddocs/superpowers/specs/2026-03-13-base-ui-components-design.mddocs/superpowers/specs/2026-03-13-web-browser-demo-design.mddocs/superpowers/specs/2026-03-15-chat-app-design.mddocs/superpowers/specs/2026-03-16-theme-palettes-design.mdpackage.jsonpackages/ai-ui/package.jsonpackages/ai-ui/src/components/chat/ChatBubble.module.csspackages/ai-ui/src/components/chat/ChatInput.tsxpackages/ai-ui/src/components/index.tspackages/ai-ui/vite.config.tspackages/base-ui/package.jsonpackages/base-ui/src/components/accordion/Accordion.module.csspackages/base-ui/src/components/accordion/Accordion.tsxpackages/base-ui/src/components/data-table/DataTableVirtualBody.tsxpackages/base-ui/src/components/data-table/constants.tspackages/base-ui/src/components/data-table/index.tspackages/base-ui/src/components/editor-tabs/EditorTabs.module.csspackages/base-ui/src/components/editor-tabs/EditorTabs.test.tsxpackages/base-ui/src/components/editor-tabs/EditorTabs.tsxpackages/base-ui/src/components/file-table/FileTable.module.csspackages/base-ui/src/components/file-table/FileTable.test.tsxpackages/base-ui/src/components/file-table/FileTable.tsxpackages/base-ui/src/components/file-table/FileTableContext.tsxpackages/base-ui/src/components/file-table/index.tspackages/base-ui/src/components/file-table/utils.tspackages/base-ui/src/components/index.tspackages/base-ui/src/components/list/List.module.csspackages/base-ui/src/components/menu/Menu.tsxpackages/base-ui/src/components/resizable-split-pane/ResizableSplitPane.module.csspackages/base-ui/src/components/resizable-split-pane/ResizableSplitPane.tsxpackages/base-ui/src/components/sortable-table/SortableHeader.module.csspackages/base-ui/src/components/sortable-table/SortableHeader.test.tsxpackages/base-ui/src/components/sortable-table/SortableHeader.tsxpackages/base-ui/src/components/sortable-table/index.tspackages/base-ui/src/components/sortable-table/useSortableTable.test.tspackages/base-ui/src/components/sortable-table/useSortableTable.tspackages/base-ui/src/components/status-bar/StatusBar.module.csspackages/base-ui/src/components/tabs/Tabs.module.csspackages/base-ui/src/components/tabs/Tabs.stories.tsxpackages/base-ui/src/components/tabs/Tabs.test.tsxpackages/base-ui/src/components/tabs/Tabs.tsxpackages/base-ui/src/components/tree-list/TreeList.module.csspackages/base-ui/src/theme/ThemeProvider.tsxpackages/base-ui/src/theme/styles.csspackages/base-ui/src/theme/types.tspackages/base-ui/vite.config.tspackages/editors/package.jsonpackages/editors/src/components/code-editor/CodeEditor.module.csspackages/editors/src/components/code-editor/CodeEditor.tsxpackages/editors/src/components/diff-viewer/DiffViewer.module.csspackages/editors/src/components/diff-viewer/DiffViewer.tsxpackages/editors/vite.config.ts
Comment on lines
+38
to
+40
| const handleCopy = useCallback(() => { | ||
| onCopy?.(message.content); | ||
| }, [message.content, onCopy]); |
There was a problem hiding this comment.
Copy action uses the wrong payload for branched answers.
Line 39 always copies message.content, but Lines 93–105 render branch-specific content. Users will copy text different from what they are reading.
💡 Proposed fix
export function MessageRenderer({ message, onCopy }: MessageRendererProps) {
const [activeBranch, setActiveBranch] = useState(0);
+ const hasBranches = Boolean(message.branches && message.branches.length > 0);
+ const copyContent = hasBranches
+ ? (message.branches?.[activeBranch]?.content ?? message.content)
+ : message.content;
const handleCopy = useCallback(() => {
- onCopy?.(message.content);
- }, [message.content, onCopy]);
+ onCopy?.(copyContent);
+ }, [copyContent, onCopy]);
@@
- const hasBranches = message.branches && message.branches.length > 0;
+ // hasBranches defined aboveAlso applies to: 93-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/showcase/src/demos/ai-chat/components/MessageRenderer.tsx` around lines
38 - 40, handleCopy currently always passes message.content to onCopy, but the
UI sometimes renders branch-specific text (the branch rendering logic at the
block around lines 93–105), so users copy the wrong text; update handleCopy to
compute the exact displayed text (use the same source/logic as the branch
rendering — e.g., derive displayedText from message.branches/selectedBranch or
the same helper used to render branch content) and pass that displayedText to
onCopy instead of message.content, ensuring the copy payload matches what the
user sees.
Comment on lines
+56
to
+59
| <AIMessageActions | ||
| onCopy={handleCopy} | ||
| onRegenerate={() => {}} | ||
| /> |
There was a problem hiding this comment.
Avoid shipping a no-op “Regenerate” action.
Line 58 exposes regenerate UI but does nothing. Remove it until implemented or pass a real handler via props.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/showcase/src/demos/ai-chat/components/MessageRenderer.tsx` around lines
56 - 59, The AIMessageActions component is being rendered with a no-op
onRegenerate handler in MessageRenderer; remove the empty onRegenerate prop or
wire it to a real handler passed via props (e.g., propagate a onRegenerate prop
from MessageRenderer's props and pass that through) so the UI does not show a
non-functional Regenerate action; update the MessageRenderer render of
AIMessageActions to either omit onRegenerate or forward a concrete handler (and
update the component props signature accordingly).
Comment on lines
+30
to
+43
| export function useScriptedReplay(options: UseScriptedReplayOptions) { | ||
| const [state, setState] = useState<ReplayState>(INITIAL_STATE); | ||
| const timeoutsRef = useRef<ReturnType<typeof setTimeout>[]>([]); | ||
|
|
||
| const clearAllTimeouts = useCallback(() => { | ||
| timeoutsRef.current.forEach(clearTimeout); | ||
| timeoutsRef.current = []; | ||
| }, []); | ||
|
|
||
| const schedule = useCallback((fn: () => void, delay: number) => { | ||
| const id = setTimeout(fn, delay); | ||
| timeoutsRef.current.push(id); | ||
| return id; | ||
| }, []); |
There was a problem hiding this comment.
Consider adding cleanup on unmount to prevent memory leaks.
If the component using this hook unmounts while a replay is in progress, the scheduled timeouts will still fire and attempt to call setState on an unmounted component. While React 18+ ignores this more gracefully, it's still a memory leak and may cause warnings in StrictMode.
🛡️ Proposed fix using useEffect cleanup
export function useScriptedReplay(options: UseScriptedReplayOptions) {
const [state, setState] = useState<ReplayState>(INITIAL_STATE);
const timeoutsRef = useRef<ReturnType<typeof setTimeout>[]>([]);
const clearAllTimeouts = useCallback(() => {
timeoutsRef.current.forEach(clearTimeout);
timeoutsRef.current = [];
}, []);
+ // Cleanup on unmount
+ useEffect(() => {
+ return () => {
+ clearAllTimeouts();
+ };
+ }, [clearAllTimeouts]);
+
const schedule = useCallback((fn: () => void, delay: number) => {Note: You'll need to add useEffect to the import on line 1.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/showcase/src/demos/ai-chat/hooks/useScriptedReplay.ts` around lines 30 -
43, The hook useScriptedReplay currently schedules timeouts (via schedule and
timeoutsRef) but never cleans them up on unmount; add a useEffect that imports
useEffect and returns a cleanup function which calls clearAllTimeouts to cancel
pending timers when the component unmounts (e.g. useEffect(() => () =>
clearAllTimeouts(), [clearAllTimeouts])); this ensures timeouts scheduled by
schedule are cleared and prevents setState from being called after unmount.
|
|
||
| // Build combined item list for the virtual list | ||
| // Static messages + replay items (cumulative, once visible they stay) | ||
| const isReplaying = state.phase !== 'idle' && state.phase !== 'done' || (state.phase === 'done' && replayUserMessage != null); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add explicit parentheses for clarity.
The boolean expression relies on operator precedence (&& binds tighter than ||), which works correctly but could be confusing for future readers.
✏️ Suggested clarification
- const isReplaying = state.phase !== 'idle' && state.phase !== 'done' || (state.phase === 'done' && replayUserMessage != null);
+ const isReplaying = (state.phase !== 'idle' && state.phase !== 'done') || (state.phase === 'done' && replayUserMessage != null);📝 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
| const isReplaying = state.phase !== 'idle' && state.phase !== 'done' || (state.phase === 'done' && replayUserMessage != null); | |
| const isReplaying = (state.phase !== 'idle' && state.phase !== 'done') || (state.phase === 'done' && replayUserMessage != null); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/showcase/src/demos/ai-chat/index.tsx` at line 119, The boolean
expression computing isReplaying is correct but unclear due to mixed && and ||
precedence; update the assignment for isReplaying (the variable) to add explicit
parentheses around the grouped conditions involving state.phase and
replayUserMessage so the intent is obvious—for example, group (state.phase !==
'idle' && state.phase !== 'done') and (state.phase === 'done' &&
replayUserMessage != null) with parentheses around each before combining with ||
to make the logical grouping explicit.
| justify-content: center; | ||
| width: 40px; | ||
| height: 40px; | ||
| border-radius: 8px; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using a CSS variable for border-radius consistency.
Other CSS modules in this PR use var(--ov-primitive-radius-sm) or similar tokens. Using a hardcoded 8px here breaks the pattern.
Suggested change
.iconBox {
display: flex;
align-items: center;
justify-content: center;
width: 40px;
height: 40px;
- border-radius: 8px;
+ border-radius: var(--ov-primitive-radius-md, 8px);
background: var(--ov-color-bg-surface-raised);
color: var(--ov-color-brand-400);
font-size: 20px;
flex-shrink: 0;
}📝 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
| border-radius: 8px; | |
| .iconBox { | |
| display: flex; | |
| align-items: center; | |
| justify-content: center; | |
| width: 40px; | |
| height: 40px; | |
| border-radius: var(--ov-primitive-radius-md, 8px); | |
| background: var(--ov-color-bg-surface-raised); | |
| color: var(--ov-color-brand-400); | |
| font-size: 20px; | |
| flex-shrink: 0; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/showcase/src/Home.module.css` at line 35, Replace the hardcoded
border-radius: 8px in Home.module.css with the shared CSS variable used across
the project (e.g., var(--ov-primitive-radius-sm)) so it matches other modules;
locate the rule that currently sets "border-radius: 8px" and swap the value to
the token (var(--ov-primitive-radius-sm) or the appropriate project token) to
maintain consistent design tokens.
Comment on lines
+17
to
+19
| ``` | ||
| main/apps/showcase/ | ||
| ``` |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add language identifiers to fenced code blocks.
The linter flags code blocks without language specifiers. For ASCII diagrams and file trees, use text or plaintext.
Example fix for one block
-```
+```text
main/apps/showcase/
Apply similarly to the file structure block (line 35) and layout diagram (line 69).
</details>
Also applies to: 35-61, 69-76
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.21.0)</summary>
[warning] 17-17: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @docs/superpowers/specs/2026-03-12-showcase-app-design.md around lines 17 -
19, Add a language identifier (e.g., text or plaintext) to all fenced code
blocks in this document so the linter stops flagging them; specifically update
the inline example block containing "main/apps/showcase/" and the longer file
structure block (around the file structure section) and the layout diagram block
(the layout ASCII art) by changing their opening fences from totext (or
Comment on lines
+35
to
+51
| ``` | ||
| ┌────────────────────────────────────────────────────────┐ | ||
| │ Toolbar: ◀ ▶ ↑ | Search | Copy Move Delete New Folder│ | ||
| ├──────────────┬──────────────┬──────────────────────────┤ | ||
| │ │ │ Detail Panel │ | ||
| │ Local │ Remote │ ┌──────────────────────┐│ | ||
| │ TreeList │ TreeList │ │ DescriptionList ││ | ||
| │ + Breadcrumbs│ + Breadcrumbs│ │ (name, size, type, ││ | ||
| │ │ │ │ modified, perms) ││ | ||
| │ │ │ ├──────────────────────┤│ | ||
| │ │ │ │ CodeBlock ││ | ||
| │ │ │ │ (file preview) ││ | ||
| │ │ │ └──────────────────────┘│ | ||
| ├──────────────┴──────────────┴──────────────────────────┤ | ||
| │ StatusBar: 7 items • 3 folders, 4 files • 12.8 KB │ | ||
| └────────────────────────────────────────────────────────┘ | ||
| ``` |
There was a problem hiding this comment.
Add language tags to fenced code blocks (MD040).
These fences are missing a language identifier; docs lint will keep warning/failing until they use a language tag (e.g. text for ASCII diagrams).
🛠️ Suggested patch pattern
-```
+```text
...
-```
+```Also applies to: 101-113, 196-216, 297-314, 333-351
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 35-35: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/specs/2026-03-12-showcase-demos-design.md` around lines 35 -
51, Add explicit language tags to the fenced code blocks that contain ASCII
diagrams (e.g., the block starting with the box/Toolbar diagram beginning
"┌────────────────" and lines containing "Toolbar: ◀ ▶ ↑ | Search | Copy Move
Delete New Folder"); replace each opening triple backtick ``` with a
language-specified fence like ```text (also update the other diagram blocks
referenced in the review such as the blocks around lines 101-113, 196-216,
297-314, 333-351). Ensure every fenced block that is an ASCII diagram uses
```text so MD040 linting passes.
Comment on lines
+132
to
+139
| <Menu.Root> | ||
| <Menu.Trigger | ||
| render={({ color: _, ...props }) => ( | ||
| <IconButton {...props} variant="ghost" size="sm" aria-label="More actions"> | ||
| <LuEllipsis size={14} /> | ||
| </IconButton> | ||
| )} | ||
| /> |
There was a problem hiding this comment.
Unused destructured variable _.
The color prop is destructured and discarded with _, but ESLint still flags it as unused. Consider using a more explicit approach.
🔧 Proposed fix using rest destructuring
<Menu.Trigger
- render={({ color: _, ...props }) => (
+ render={(triggerProps) => {
+ const { color: _color, ...props } = triggerProps;
+ return (
<IconButton {...props} variant="ghost" size="sm" aria-label="More actions">
<LuEllipsis size={14} />
</IconButton>
- )}
+ );
+ }}
/>Alternatively, if your ESLint config supports it, prefix with underscore in a way ESLint recognizes: _color instead of _.
📝 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
| <Menu.Root> | |
| <Menu.Trigger | |
| render={({ color: _, ...props }) => ( | |
| <IconButton {...props} variant="ghost" size="sm" aria-label="More actions"> | |
| <LuEllipsis size={14} /> | |
| </IconButton> | |
| )} | |
| /> | |
| <Menu.Root> | |
| <Menu.Trigger | |
| render={(triggerProps) => { | |
| const { color: _color, ...props } = triggerProps; | |
| return ( | |
| <IconButton {...props} variant="ghost" size="sm" aria-label="More actions"> | |
| <LuEllipsis size={14} /> | |
| </IconButton> | |
| ); | |
| }} | |
| /> |
🧰 Tools
🪛 ESLint
[error] 134-134: '_' is defined but never used.
(@typescript-eslint/no-unused-vars)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/showcase/src/demos/chat-app/components/MessageItem.tsx` around lines 132
- 139, The render prop for Menu.Trigger currently destructures the color prop as
`_` which ESLint still flags; update the render prop to avoid extracting an
unused `color` — either drop the explicit destructuring and accept the whole
props object (e.g., render={(props) => ...}) or rename the variable to an
ESLint-recognized unused name like `_color`; apply this change where
Menu.Trigger's render prop is defined so IconButton receives the forwarded props
(IconButton and LuEllipsis referenced).
Comment on lines
+207
to
+222
| {message.replyCount > 0 && onOpenThread && ( | ||
| <button | ||
| className={styles.threadIndicator} | ||
| onClick={() => onOpenThread(message.id)} | ||
| > | ||
| <span className={styles.threadAvatars}> | ||
| {threadAvatarUsers.length > 0 | ||
| ? threadAvatarUsers.map((u) => ( | ||
| <Avatar key={u.id} name={u.name} shape="circle" size="sm" /> | ||
| )) | ||
| : <Avatar name={user.name} shape="circle" size="sm" /> | ||
| } | ||
| </span> | ||
| <span>{message.replyCount} {message.replyCount === 1 ? 'reply' : 'replies'}</span> | ||
| </button> | ||
| )} |
There was a problem hiding this comment.
Add explicit type="button" to prevent unintended form submission.
The thread indicator <button> element lacks an explicit type attribute. Buttons default to type="submit", which can cause unexpected form submissions if this component is ever rendered inside a form.
🔧 Proposed fix
{message.replyCount > 0 && onOpenThread && (
<button
className={styles.threadIndicator}
onClick={() => onOpenThread(message.id)}
+ type="button"
>📝 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
| {message.replyCount > 0 && onOpenThread && ( | |
| <button | |
| className={styles.threadIndicator} | |
| onClick={() => onOpenThread(message.id)} | |
| > | |
| <span className={styles.threadAvatars}> | |
| {threadAvatarUsers.length > 0 | |
| ? threadAvatarUsers.map((u) => ( | |
| <Avatar key={u.id} name={u.name} shape="circle" size="sm" /> | |
| )) | |
| : <Avatar name={user.name} shape="circle" size="sm" /> | |
| } | |
| </span> | |
| <span>{message.replyCount} {message.replyCount === 1 ? 'reply' : 'replies'}</span> | |
| </button> | |
| )} | |
| {message.replyCount > 0 && onOpenThread && ( | |
| <button | |
| className={styles.threadIndicator} | |
| onClick={() => onOpenThread(message.id)} | |
| type="button" | |
| > | |
| <span className={styles.threadAvatars}> | |
| {threadAvatarUsers.length > 0 | |
| ? threadAvatarUsers.map((u) => ( | |
| <Avatar key={u.id} name={u.name} shape="circle" size="sm" /> | |
| )) | |
| : <Avatar name={user.name} shape="circle" size="sm" /> | |
| } | |
| </span> | |
| <span>{message.replyCount} {message.replyCount === 1 ? 'reply' : 'replies'}</span> | |
| </button> | |
| )} |
🧰 Tools
🪛 Biome (2.4.7)
[error] 208-211: Provide an explicit type prop for the button element.
(lint/a11y/useButtonType)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/showcase/src/demos/chat-app/components/MessageItem.tsx` around lines 207
- 222, The thread indicator button in MessageItem (the JSX that renders when
message.replyCount > 0) lacks an explicit type and may act as a submit button
when inside a form; update the <button> that uses styles.threadIndicator and
onClick={() => onOpenThread(message.id)} to include type="button" so it does not
trigger form submission unexpectedly. Ensure you add the attribute to the same
button element that renders threadAvatarUsers/Avatar and message.replyCount.
Comment on lines
+57
to
+65
| const activeWorkspace = useMemo( | ||
| () => workspaces.find((w) => w.id === activeWorkspaceId) ?? workspaces[0]!, | ||
| [workspaces, activeWorkspaceId], | ||
| ); | ||
|
|
||
| const activeChannel = useMemo( | ||
| () => activeWorkspace.channels.find((c) => c.id === activeChannelId) ?? activeWorkspace.channels[0]!, | ||
| [activeWorkspace, activeChannelId], | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Non-null assertions assume non-empty arrays.
The ! assertions on workspaces[0] and channels[0] will throw if arrays are empty. This is likely safe given the hardcoded demo data, but defensive fallbacks would be more robust.
Defensive alternative
const activeWorkspace = useMemo(
- () => workspaces.find((w) => w.id === activeWorkspaceId) ?? workspaces[0]!,
+ () => workspaces.find((w) => w.id === activeWorkspaceId) ?? workspaces[0],
[workspaces, activeWorkspaceId],
);Then handle the potential undefined case in consuming code, or ensure data invariants are documented.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/showcase/src/demos/chat-app/hooks/useChat.ts` around lines 57 - 65,
Replace the unsafe non-null assertions by making activeWorkspace and
activeChannel handle empty arrays: change the logic in useMemo for
activeWorkspace (which currently uses workspaces and activeWorkspaceId) to
return workspaces.find(...) ?? workspaces[0] ?? undefined (i.e., allow undefined
instead of using !) and similarly for activeChannel (use
activeWorkspace?.channels.find(...) ?? activeWorkspace?.channels?.[0] ??
undefined); then update any consuming code to handle the possible undefined
values (or provide a clear documented invariant/fallback) so you no longer
assume non-empty workspaces or channels in useChat.
Comment on lines
+28
to
+33
| const handleReorder = useCallback( | ||
| (nextTabs: TabDescriptor[], _meta: ReorderMeta) => { | ||
| browser.reorderTabs(nextTabs.map((t) => t.id)); | ||
| }, | ||
| [browser.reorderTabs], | ||
| ); |
There was a problem hiding this comment.
Remove unused _meta parameter to fix ESLint warning.
The _meta parameter is defined but never used. Either omit it or use a comment to suppress the warning if it's intentionally reserved for future use.
Proposed fix
const handleReorder = useCallback(
- (nextTabs: TabDescriptor[], _meta: ReorderMeta) => {
+ (nextTabs: TabDescriptor[]) => {
browser.reorderTabs(nextTabs.map((t) => t.id));
},
[browser.reorderTabs],
);If ReorderMeta is needed for type compatibility with onReorder, keep the parameter but verify the callback signature.
📝 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
| const handleReorder = useCallback( | |
| (nextTabs: TabDescriptor[], _meta: ReorderMeta) => { | |
| browser.reorderTabs(nextTabs.map((t) => t.id)); | |
| }, | |
| [browser.reorderTabs], | |
| ); | |
| const handleReorder = useCallback( | |
| (nextTabs: TabDescriptor[]) => { | |
| browser.reorderTabs(nextTabs.map((t) => t.id)); | |
| }, | |
| [browser.reorderTabs], | |
| ); |
🧰 Tools
🪛 ESLint
[error] 29-29: '_meta' is defined but never used.
(@typescript-eslint/no-unused-vars)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/showcase/src/demos/web-browser/index.tsx` around lines 28 - 33, The
handleReorder callback currently declares an unused parameter _meta causing an
ESLint warning; update the handleReorder signature to drop the unused parameter
(keep it as (nextTabs: TabDescriptor[]) => { ... }) or, if you must preserve the
second parameter for onReorder type compatibility, rename it to _ or _meta:
ReorderMeta and ensure it's referenced only as a placeholder (or annotate via a
comment) so ESLint stops flagging it; adjust the dependency array and the call
to browser.reorderTabs(nextTabs.map(t => t.id)) remains unchanged.
Comment on lines
+1458
to
+1461
| <ResizableSplitPane direction="horizontal" defaultSize={60}> | ||
| {chatContent} | ||
| <ArtifactPanel artifact={artifact} onClose={() => setArtifact(null)} /> | ||
| </ResizableSplitPane> |
There was a problem hiding this comment.
defaultSize={60} conflicts with the plan’s own pixel-based sizing rule.
The doc states ResizableSplitPane.defaultSize is in pixels; 60 here implies a 60px pane, not 60%.
Proposed fix
- <ResizableSplitPane direction="horizontal" defaultSize={60}>
+ <ResizableSplitPane direction="horizontal" defaultSize={640}>📝 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
| <ResizableSplitPane direction="horizontal" defaultSize={60}> | |
| {chatContent} | |
| <ArtifactPanel artifact={artifact} onClose={() => setArtifact(null)} /> | |
| </ResizableSplitPane> | |
| <ResizableSplitPane direction="horizontal" defaultSize={640}> | |
| {chatContent} | |
| <ArtifactPanel artifact={artifact} onClose={() => setArtifact(null)} /> | |
| </ResizableSplitPane> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-03-12-showcase-demos.md` around lines 1458 -
1461, The ResizableSplitPane usage sets defaultSize={60} which the docs state is
interpreted as pixels; change this to use a percentage-based value per the plan
(e.g., "60%") or the component's percent prop (e.g., defaultSizePercent={60}) so
the pane is 60% width rather than 60px; locate the ResizableSplitPane instance
in the file and replace the numeric pixel value with the appropriate percent
representation (either a string with '%' or the component's percent prop name)
to align with the documented sizing rule.
Comment on lines
+2396
to
+2398
| ```bash | ||
| cd /Users/joshuapare/Repos/omniviewdev/ui/feat/demos && pnpm build | ||
| ``` |
There was a problem hiding this comment.
Use repo-relative commands instead of a machine-specific absolute path.
The command on Line 2397 only works on one local machine and is not runnable by contributors or CI.
Proposed fix
-cd /Users/joshuapare/Repos/omniviewdev/ui/feat/demos && pnpm build
+cd . && pnpm build🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-03-12-showcase-demos.md` around lines 2396 -
2398, Replace the machine-specific absolute path command `cd
/Users/joshuapare/Repos/omniviewdev/ui/feat/demos && pnpm build` with a
repo-relative command so others/CI can run it; for example use `cd ui/feat/demos
&& pnpm build` (or even prefer a workspace-aware invocation like `pnpm --filter
./ui/feat/demos run build`), updating the bash snippet so it references the
repo-relative path instead of the absolute path.
Comment on lines
+57
to
+58
| Run: `cd /Users/joshuapare/Repos/omniviewdev/ui/feat/demos && pnpm install` | ||
| Expected: Lockfile updates, workspace links resolve. |
There was a problem hiding this comment.
Replace absolute local paths with repo-relative commands.
These commands are machine-specific and not portable for other developers or CI execution.
Suggested pattern
-Run: `cd /Users/joshuapare/Repos/omniviewdev/ui/feat/demos && pnpm --filter `@omniview/showcase` dev`
+Run from repo root: `pnpm --filter `@omniview/showcase` dev`Also applies to: 177-178, 704-705, 764-765, 769-770, 785-786, 790-791
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-03-12-showcase-shell.md` around lines 57 - 58,
Replace the absolute, machine-specific cd command ("cd
/Users/joshuapare/Repos/omniviewdev/ui/feat/demos && pnpm install") with a
repo-relative invocation (for example "cd ui/feat/demos && pnpm install" or use
a variable like "$REPO_ROOT/ui/feat/demos && pnpm install") so the steps are
portable in other dev environments and CI; update all other identical
occurrences referenced in the comment (the other command blocks showing absolute
paths) to the same repo-relative pattern and ensure the expected outcomes text
(lockfile updates, workspace links resolve) still apply.
Comment on lines
+18
to
+19
| - Verify builds with: `pnpm --filter showcase build 2>&1 | tail -5` | ||
| - Components use compound patterns: `NavList.Root`, `NavList.Item`, `TextArea.Root`, `TextArea.Control`, etc. |
There was a problem hiding this comment.
Use exact workspace filter in plan commands.
Prefer @omniview/showcase over showcase to avoid ambiguous filter matches across the monorepo.
Suggested replacement
- pnpm --filter showcase build
+ pnpm --filter `@omniview/showcase` buildAlso applies to: 79-80, 459-460, 704-705, 1318-1319, 1403-1404, 1552-1553, 1615-1616, 1705-1706, 1956-1957, 2208-2209, 2316-2317, 2424-2425
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/plans/2026-03-15-chat-app.md` around lines 18 - 19, Replace
ambiguous workspace filter occurrences of "showcase" with the exact workspace
name "@omniview/showcase" in all plan commands (for example change the command
string "pnpm --filter showcase build 2>&1 | tail -5" to use
"@omniview/showcase"); search for and update every occurrence of the literal
filter " --filter showcase " (including the other listed occurrences) so
commands target the specific workspace.
There was a problem hiding this comment.
Actionable comments posted: 17
♻️ Duplicate comments (5)
apps/showcase/src/demos/chat-app/index.module.css (1)
523-540:⚠️ Potential issue | 🟠 MajorRename keyframe to kebab-case and update animation reference.
typingBouncestill violates the keyframe naming rule (see Line 539), and the shorthand at Line 528 must be updated in lockstep.Proposed fix
.typingDot { width: 4px; height: 4px; border-radius: 50%; background: var(--ov-color-fg-muted); - animation: typingBounce 1.4s infinite; + animation: typing-bounce 1.4s infinite; } @@ -@keyframes typingBounce { +@keyframes typing-bounce { 0%, 60%, 100% { opacity: 0.3; transform: translateY(0); } 30% { opacity: 1; transform: translateY(-3px); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/showcase/src/demos/chat-app/index.module.css` around lines 523 - 540, Rename the `@keyframes` identifier typingBounce to kebab-case (e.g., typing-bounce) and update all references to it in the CSS (the animation shorthand on .typingDot and any animation-delay rules such as .typingDot:nth-child(2)/.typingDot:nth-child(3)) so the animation name in the shorthand matches the new kebab-case keyframe name; ensure `@keyframes` typing-bounce contains the same keyframe rules and the animation property on .typingDot still uses the same duration/iteration values.apps/showcase/src/demos/web-browser/hooks/useBrowser.ts (1)
21-21:⚠️ Potential issue | 🟡 MinorMove initial tab creation into the hook instance.
Line [21] still creates a shared
INITIAL_TABat module scope, and Line [47]-Line [48] seed state from it. MultipleuseBrowser()instances will start from the same tab object/id reference.Suggested fix
-const INITIAL_TAB = createTab(); - export function useBrowser(): UseBrowserReturn { - const [tabs, setTabs] = useState<BrowserTab[]>([INITIAL_TAB]); - const [activeTabId, setActiveTabId] = useState<string>(INITIAL_TAB.id); + const [initialTab] = useState<BrowserTab>(() => createTab()); + const [tabs, setTabs] = useState<BrowserTab[]>(() => [initialTab]); + const [activeTabId, setActiveTabId] = useState<string>(() => initialTab.id); const timeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null); const activeTab = useMemo( - () => tabs.find((t) => t.id === activeTabId) ?? tabs[0] ?? INITIAL_TAB, + () => tabs.find((t) => t.id === activeTabId) ?? tabs[0] ?? initialTab, [tabs, activeTabId], );#!/bin/bash # Verify module-scope initial tab sharing pattern still exists in this file. rg -n "const INITIAL_TAB|useState<BrowserTab\\[]>\\(\\[INITIAL_TAB\\]\\)|useState<string>\\(INITIAL_TAB.id\\)|\\?\\? INITIAL_TAB" apps/showcase/src/demos/web-browser/hooks/useBrowser.tsAlso applies to: 47-48, 52-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/showcase/src/demos/web-browser/hooks/useBrowser.ts` at line 21, The module currently defines a shared INITIAL_TAB via createTab() which is then used to seed state in useBrowser, causing multiple hook instances to share the same tab object/id; move the initial tab creation into the hook so each useBrowser call gets its own tab instance (e.g., call createTab() inside useBrowser or useMemo within useBrowser) and change state initializers that reference INITIAL_TAB (useState<BrowserTab[]> initialization, useState<string>(INITIAL_TAB.id), and any "?? INITIAL_TAB" fallbacks) to use the per-instance tab returned from createTab(); remove the module-scope INITIAL_TAB and update all references to use the locally created tab/id inside useBrowser.apps/showcase/src/demos/ide-editor/components/SidebarPanel.tsx (1)
119-127:⚠️ Potential issue | 🟡 MinorUse a native
<button>for search result actions.Line 123 still uses
role="button"on adiv; this keeps the accessibility lint violation and adds avoidable keyboard handling complexity.Proposed fix
- <div + <button // eslint-disable-next-line react/no-array-index-key key={`${result.file}-${result.line}-${i}`} className={styles.resultItem} - role="button" - tabIndex={0} + type="button" onClick={() => onOpenFile(result.file)} - onKeyDown={(e) => { if (e.key === 'Enter' || e.key === ' ') { e.preventDefault(); onOpenFile(result.file); } }} > @@ - </div> + </button>Also applies to: 133-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/showcase/src/demos/ide-editor/components/SidebarPanel.tsx` around lines 119 - 127, Replace the interactive divs in SidebarPanel (the element with className styles.resultItem that uses role="button", tabIndex and custom onKeyDown) with native <button type="button"> elements so browser accessibility and keyboard handling are correct; keep the existing className and ensure onClick calls onOpenFile(result.file) (remove role, tabIndex and the custom Enter/Space onKeyDown logic), and apply the same change to the other identical occurrence around the second result item (the duplicate at lines ~133) so both use buttons.apps/showcase/src/demos/web-browser/components/ErrorPage.tsx (1)
13-13:⚠️ Potential issue | 🟡 MinorEscape the apostrophe or reword the heading.
Line 13 still trips
react/no-unescaped-entities, which can fail lint/CI.Suggested fix
- <h2 className={styles.errorTitle}>This site can't be displayed in a frame</h2> + <h2 className={styles.errorTitle}>This site can't be displayed in a frame</h2>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/showcase/src/demos/web-browser/components/ErrorPage.tsx` at line 13, The heading string in ErrorPage.tsx (<h2 className={styles.errorTitle}>) contains an unescaped apostrophe that triggers react/no-unescaped-entities; update the text to either escape the apostrophe (e.g., "This site can't be displayed in a frame") or reword it (e.g., "This site cannot be displayed in a frame") to satisfy the linter while leaving the element and className (styles.errorTitle) unchanged.apps/showcase/src/demos/web-browser/data.ts (1)
10-10: 🧹 Nitpick | 🔵 TrivialGive
SPEED_DIAL_SITESits own copy.Line 10 still aliases
DEFAULT_BOOKMARKS, so any future speed-dial mutation will also rewrite the defaults.Suggested fix
-export const SPEED_DIAL_SITES = DEFAULT_BOOKMARKS; +export const SPEED_DIAL_SITES: Bookmark[] = DEFAULT_BOOKMARKS.map((bookmark) => ({ + ...bookmark, +}));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/showcase/src/demos/web-browser/data.ts` at line 10, SPEED_DIAL_SITES is currently a direct alias to DEFAULT_BOOKMARKS which causes mutations to SPEED_DIAL_SITES to also mutate DEFAULT_BOOKMARKS; change the initialization of SPEED_DIAL_SITES to create an independent copy of DEFAULT_BOOKMARKS (e.g., shallow copy for array of primitives/objects or deep clone if items are nested) so future mutations to SPEED_DIAL_SITES do not affect DEFAULT_BOOKMARKS; update the export of SPEED_DIAL_SITES accordingly and ensure any code expecting the original DEFAULT_BOOKMARKS reference still imports DEFAULT_BOOKMARKS.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/showcase/src/demos/ai-chat/components/ArtifactPanel.tsx`:
- Around line 22-24: The clipboard write in handleCopy currently swallows
rejections (void navigator.clipboard.writeText(...)); update the handleCopy
function in ArtifactPanel.tsx to attach a .catch(...) to
navigator.clipboard.writeText(artifact.code) and surface failures via the
existing useToast hook (show an error toast with a helpful message and error
details). Also search for the same pattern around index.tsx line ~159 and apply
the same fix: add .catch(...) and call useToast to notify users on
permission/availability failures rather than ignoring the error.
In `@apps/showcase/src/demos/chat-app/components/WorkspaceRail.tsx`:
- Around line 23-38: The workspace items in WorkspaceRail are not
keyboard-accessible because Tooltip.Trigger is being used as a non-interactive
element; update the workspace item to be an actual interactive control
(preferably a <button>) or ensure Tooltip.Trigger renders as a button and add
keyboard handlers: make the trigger focusable (tabIndex=0 if not already),
implement onKeyDown to call onSwitchWorkspace(ws.id) when Enter or Space is
pressed, and expose proper ARIA state (e.g., aria-current or aria-pressed) tied
to activeWorkspaceId; update the Tooltip.Trigger usage around workspaces.map,
the onClick handler, and include accessibility attributes so keyboard users can
tab and activate each workspace.
In `@apps/showcase/src/demos/chat-app/data.ts`:
- Around line 328-330: getUserById currently hides missing users by returning a
synthetic "Unknown" User object; change getUserById(users: User[], id: string):
User to return undefined when not found (i.e., adjust its return type to User |
undefined and return the result of users.find(...) directly), and update any
callers of getUserById to handle the undefined case (either by conditional
checks or explicit errors) so missing IDs are not silently masked.
In `@apps/showcase/src/demos/chat-app/hooks/useChat.ts`:
- Around line 167-200: toggleReaction currently only updates messages in
messagesByChannel[activeChannelId] and threadReplies[activeThreadId], so passing
a messageId from another channel/thread is silently ignored; update
toggleReaction to detect whether toggleInList actually changed any message in
those stores and, if neither update touched a message, log a console.warn (or
process logger) including the messageId and activeChannelId/activeThreadId for
debugging, or alternatively return a boolean indicating success; reference the
existing functions/vars toggleReaction, toggleInList, setMessagesByChannel,
setThreadReplies, activeChannelId and activeThreadId to implement the
guard/warning where the two setState calls are made.
In `@apps/showcase/src/demos/container-management/components/ContainerList.tsx`:
- Around line 172-185: Guard against division-by-zero when computing pct for the
Meter: replace the naive (row.memory / row.memoryLimit) * 100 with a safe
calculation (compute pct only if row.memoryLimit > 0, otherwise use 0), and
clamp the result to the 0–100 range before passing it to the Meter.value; update
the computed pct used for Meter and keep the aria-label/label using row.memory
and row.memoryLimit as-is. This change should be made around the pct calculation
in ContainerList (where const row = info.row.original and Meter is rendered).
In `@apps/showcase/src/demos/container-management/data.ts`:
- Around line 563-572: The avgCpu calculation in clusterStats incorrectly
divides the sum of all containers' cpu by the count of running containers;
change avgCpu to compute the sum only over running containers and divide by the
running count (or 0 guard) instead. Specifically, in the clusterStats object
update the avgCpu IIFE to filter containers to runningContainers (e.g., const
runningContainers = containers.filter(c => c.status === 'running')), sum
runningContainers' cpu values and divide by runningContainers.length with a
zero-check fallback so numerator and denominator use the same population.
In `@apps/showcase/src/demos/file-explorer/components/FilePane.tsx`:
- Around line 26-42: Extract the duplicated PathEntry type and buildPathMap
function into a shared helper module and import it into both FilePane and
TreePane to avoid divergence; specifically, create a new exported helper that
declares the PathEntry interface and the buildPathMap(root: FileNode):
Map<string, PathEntry> function, move the implementation currently in FilePane
(the walk recursion and map.set logic) there, then update FilePane and TreePane
to import and use the shared buildPathMap and PathEntry symbols instead of their
local duplicates.
- Line 1: The pane keeps internal state (e.g., pathMap-derived selection, table
rows, breadcrumbs) that is only set from the initial root; add a useEffect
watching the root prop to reset those derived states when root changes.
Specifically, when root changes clear/recompute pathMap and reset any state
variables like selectedPath/currentPath, table state (rows/pagination/sort), and
breadcrumbs so they are rebuilt from the new root; also ensure any useMemo or
useCallback that depends on root includes root in its dependency list so derived
values are recomputed. This will prevent the table and breadcrumbs from
remaining pinned to stale nodes after the parent supplies a refreshed tree.
In `@apps/showcase/src/demos/file-explorer/components/TreePane.tsx`:
- Around line 36-40: The prop name onSelectFile is misleading because
handleSelectedKeysChange (and places around lines 60-68) forwards folder nodes
as well; rename onSelectFile to onSelectNode in the TreePaneProps interface and
everywhere this component emits selections (including the handler
handleSelectedKeysChange and any JSX props passed to child components) so the
prop accurately reflects it can receive both files and folders, and update any
consumer callsites to accept (node: FileNode, path: string[]) instead of
assuming files only.
In `@apps/showcase/src/demos/ide-editor/components/IconStrip.tsx`:
- Line 14: The code references the React type ReactNode but doesn't import it;
update IconStrip.tsx to import ReactNode from 'react' (e.g., add ReactNode to
the existing imports) so the type annotation for the icon prop (icon: ReactNode)
compiles under the react-jsx setting; ensure the import is added alongside any
existing React imports or as a standalone named import.
In `@apps/showcase/src/demos/ide-editor/index.tsx`:
- Around line 74-94: handleOpenFile uses the stale closure variable tabs causing
duplicate tabs on rapid opens; move the deduplication into the setTabs
functional updater to read the previous state snapshot. Inside handleOpenFile,
call setTabs(prev => { check prev.find(t => t.file.id === fileId) and return
prev unchanged if found; otherwise append a new IDETab (id `tab-${fileId}`,
file, type 'code') and return the new array }), and ensure setActiveTabId is
called with the resolved tab id (existing tab's id if found or the newTab id) so
active tab is set deterministically without relying on the outer tabs variable.
In `@apps/showcase/src/demos/web-browser/components/BrowserToolbar.tsx`:
- Around line 80-88: The address bar Input.Control currently only uses a
placeholder so screen readers won't announce it; update the Input.Control in
BrowserToolbar.tsx (the Input.Root / Input.Control that uses inputValue,
setInputValue, displayUrl and handleKeyDown) to include an accessible
label—either add a visible <label> tied to the input or add an aria-label (e.g.,
aria-label="Address bar" or aria-label={`Address bar, ${displayUrl}`} ) and
ensure the label is associated with the control (using id on Input.Control and
htmlFor on the label) so screen readers announce it.
In `@apps/showcase/src/demos/web-browser/components/NewTabPage.tsx`:
- Around line 14-18: The key handler handleKeyDown currently treats any
non-empty input as a URL via ensureProtocol(search.trim()), causing plain search
queries to become invalid URLs; update handleKeyDown to detect whether the input
is a URL (e.g., has a scheme or contains a domain-like pattern such as a dot or
startsWith http/https) and only call ensureProtocol/onNavigate for URL-like
inputs, otherwise construct a search URL (encodeURIComponent) and navigate to
that (or call a search function); alternatively, if you prefer not to implement
search, change the UI copy/placeholder text referenced around lines 32-33 to
explicitly say “Enter URL” instead of advertising search so users aren’t misled
(refer to handleKeyDown, ensureProtocol, and onNavigate to locate the logic and
the placeholder/copy state).
- Around line 26-33: The input in the NewTabPage component (Input.Control) lacks
an accessible name; update the Input.Control usage to provide one by adding an
aria-label (e.g., aria-label="Search or enter URL") or by rendering/associating
a visible <label> and using aria-labelledby pointing to that label's id so the
text field has a programmatic name; modify the Input.Control props in NewTabPage
to include the chosen aria attribute or wire up a label component to ensure
accessibility.
In `@apps/showcase/src/demos/web-browser/data.ts`:
- Around line 20-23: The function ensureProtocol currently allows any scheme
(e.g., javascript: or data:) to pass; change its check to only allow safe
schemes and the internal new-tab sentinel. In ensureProtocol, replace the
general scheme test /^[a-zA-Z][\w+\-.]*:/ with a strict check for http and https
(e.g., /^https?:/i) while still allowing input.startsWith('//'); for any other
input, prepend "https://". This ensures only http/https and the existing '//'
sentinel are accepted unchanged.
In `@apps/showcase/src/demos/web-browser/hooks/useBrowser.ts`:
- Around line 139-142: The refresh callback should skip marking a new-tab as
loading and not start the blocked timeout when the active tab is the new-tab;
change refresh to first check the current active tab's URL (use the existing
activeTab state or an activeTabRef) and if activeTab.url === NEW_TAB_URL return
early, otherwise call updateActiveTab(t => ({ ...t, loading: true, error:
undefined })) and then call startBlockedTimeout(); ensure you reference refresh,
updateActiveTab, startBlockedTimeout and NEW_TAB_URL when making this change.
In `@apps/showcase/src/demos/web-browser/index.module.css`:
- Around line 160-165: The .errorUrl rule currently allows long URLs to
overflow; update the .errorUrl CSS to constrain and allow wrapping by adding
max-width: 100% and wrap rules (e.g., overflow-wrap: anywhere; word-break:
break-word; white-space: normal;) so long query strings wrap within the centered
error page instead of overflowing.
---
Duplicate comments:
In `@apps/showcase/src/demos/chat-app/index.module.css`:
- Around line 523-540: Rename the `@keyframes` identifier typingBounce to
kebab-case (e.g., typing-bounce) and update all references to it in the CSS (the
animation shorthand on .typingDot and any animation-delay rules such as
.typingDot:nth-child(2)/.typingDot:nth-child(3)) so the animation name in the
shorthand matches the new kebab-case keyframe name; ensure `@keyframes`
typing-bounce contains the same keyframe rules and the animation property on
.typingDot still uses the same duration/iteration values.
In `@apps/showcase/src/demos/ide-editor/components/SidebarPanel.tsx`:
- Around line 119-127: Replace the interactive divs in SidebarPanel (the element
with className styles.resultItem that uses role="button", tabIndex and custom
onKeyDown) with native <button type="button"> elements so browser accessibility
and keyboard handling are correct; keep the existing className and ensure
onClick calls onOpenFile(result.file) (remove role, tabIndex and the custom
Enter/Space onKeyDown logic), and apply the same change to the other identical
occurrence around the second result item (the duplicate at lines ~133) so both
use buttons.
In `@apps/showcase/src/demos/web-browser/components/ErrorPage.tsx`:
- Line 13: The heading string in ErrorPage.tsx (<h2
className={styles.errorTitle}>) contains an unescaped apostrophe that triggers
react/no-unescaped-entities; update the text to either escape the apostrophe
(e.g., "This site can't be displayed in a frame") or reword it (e.g., "This
site cannot be displayed in a frame") to satisfy the linter while leaving the
element and className (styles.errorTitle) unchanged.
In `@apps/showcase/src/demos/web-browser/data.ts`:
- Line 10: SPEED_DIAL_SITES is currently a direct alias to DEFAULT_BOOKMARKS
which causes mutations to SPEED_DIAL_SITES to also mutate DEFAULT_BOOKMARKS;
change the initialization of SPEED_DIAL_SITES to create an independent copy of
DEFAULT_BOOKMARKS (e.g., shallow copy for array of primitives/objects or deep
clone if items are nested) so future mutations to SPEED_DIAL_SITES do not affect
DEFAULT_BOOKMARKS; update the export of SPEED_DIAL_SITES accordingly and ensure
any code expecting the original DEFAULT_BOOKMARKS reference still imports
DEFAULT_BOOKMARKS.
In `@apps/showcase/src/demos/web-browser/hooks/useBrowser.ts`:
- Line 21: The module currently defines a shared INITIAL_TAB via createTab()
which is then used to seed state in useBrowser, causing multiple hook instances
to share the same tab object/id; move the initial tab creation into the hook so
each useBrowser call gets its own tab instance (e.g., call createTab() inside
useBrowser or useMemo within useBrowser) and change state initializers that
reference INITIAL_TAB (useState<BrowserTab[]> initialization,
useState<string>(INITIAL_TAB.id), and any "?? INITIAL_TAB" fallbacks) to use the
per-instance tab returned from createTab(); remove the module-scope INITIAL_TAB
and update all references to use the locally created tab/id inside useBrowser.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0b156882-1dad-4a57-8375-ac4f7195fa35
📒 Files selected for processing (25)
apps/showcase/src/demos/ai-chat/components/ArtifactPanel.tsxapps/showcase/src/demos/chat-app/components/ReactionBar.tsxapps/showcase/src/demos/chat-app/components/WorkspaceRail.tsxapps/showcase/src/demos/chat-app/data.tsapps/showcase/src/demos/chat-app/hooks/useChat.tsapps/showcase/src/demos/chat-app/index.module.cssapps/showcase/src/demos/container-management/components/ContainerList.module.cssapps/showcase/src/demos/container-management/components/ContainerList.tsxapps/showcase/src/demos/container-management/components/tabs/FilesTab.tsxapps/showcase/src/demos/container-management/components/tabs/StatsTab.tsxapps/showcase/src/demos/container-management/data.tsapps/showcase/src/demos/file-explorer/components/CommandLog.tsxapps/showcase/src/demos/file-explorer/components/FilePane.tsxapps/showcase/src/demos/file-explorer/components/TransferPanel.tsxapps/showcase/src/demos/file-explorer/components/TreePane.tsxapps/showcase/src/demos/ide-editor/components/IconStrip.tsxapps/showcase/src/demos/ide-editor/components/SidebarPanel.tsxapps/showcase/src/demos/ide-editor/index.tsxapps/showcase/src/demos/web-browser/components/BrowserToolbar.tsxapps/showcase/src/demos/web-browser/components/ErrorPage.tsxapps/showcase/src/demos/web-browser/components/NewTabPage.tsxapps/showcase/src/demos/web-browser/data.tsapps/showcase/src/demos/web-browser/hooks/useBrowser.tsapps/showcase/src/demos/web-browser/index.module.cssapps/showcase/src/global.css
Comment on lines
+22
to
+24
| const handleCopy = useCallback(() => { | ||
| void navigator.clipboard.writeText(artifact.code); | ||
| }, [artifact.code]); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether clipboard failures are handled consistently in the repo.
# Expected: clipboard write calls should have explicit error handling and user feedback paths.
rg -n -C3 --type=tsx --type=ts 'clipboard\.writeText\('
rg -n -C3 --type=tsx --type=ts 'writeText\([^)]*\)\.catch|try\s*\{[^}]*writeText'
rg -n -C3 --type=tsx --type=ts 'toast|notify|snackbar|alert'Repository: omniviewdev/ui
Length of output: 141
🏁 Script executed:
#!/bin/bash
# Corrected search for clipboard.writeText patterns
echo "=== Searching for clipboard.writeText calls ==="
rg -n -C3 'clipboard\.writeText\(' --glob='*.ts' --glob='*.tsx'
echo -e "\n=== Searching for error handling patterns with writeText ==="
rg -n -C3 'writeText.*catch|\.catch.*writeText' --glob='*.ts' --glob='*.tsx'
echo -e "\n=== Searching for toast/notification patterns ==="
rg -n 'toast|notify|snackbar' --glob='*.ts' --glob='*.tsx' | head -20
echo -e "\n=== Looking at ArtifactPanel.tsx file ==="
find . -name 'ArtifactPanel.tsx' -type fRepository: omniviewdev/ui
Length of output: 8074
🏁 Script executed:
#!/bin/bash
# Check CodeBlock error handling context
echo "=== CodeBlock.tsx context (lines 85-95) ==="
sed -n '85,95p' packages/base-ui/src/components/code-block/CodeBlock.tsx
# Check if CodeBlock is wrapped in try/catch
echo -e "\n=== CodeBlock.tsx - check for error handling around handleCopy ==="
sed -n '82,105p' packages/base-ui/src/components/code-block/CodeBlock.tsx
# Check ClipboardText for comparison
echo -e "\n=== ClipboardText.tsx - complete error handling pattern ==="
sed -n '28,45p' packages/base-ui/src/components/clipboard-text/ClipboardText.tsx
# Check if toast is used in showcase
echo -e "\n=== Check if useToast exists in showcase ==="
grep -r 'useToast\|toast' apps/showcase/src --include='*.tsx' --include='*.ts'Repository: omniviewdev/ui
Length of output: 4938
Handle clipboard write failures instead of silently dropping them.
Line 23 ignores rejected clipboard writes (void ...writeText(...)), causing permission/availability failures to silently fail for users. This pattern also appears in index.tsx:159. Add .catch() with error feedback using the useToast hook that's already available in the showcase.
const handleCopy = useCallback(() => {
- void navigator.clipboard.writeText(artifact.code);
+ navigator.clipboard.writeText(artifact.code).catch(() => {
+ // Optionally notify user if clipboard API fails
+ });
}, [artifact.code]);📝 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
| const handleCopy = useCallback(() => { | |
| void navigator.clipboard.writeText(artifact.code); | |
| }, [artifact.code]); | |
| const handleCopy = useCallback(() => { | |
| navigator.clipboard.writeText(artifact.code).catch(() => { | |
| // Optionally notify user if clipboard API fails | |
| }); | |
| }, [artifact.code]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/showcase/src/demos/ai-chat/components/ArtifactPanel.tsx` around lines 22
- 24, The clipboard write in handleCopy currently swallows rejections (void
navigator.clipboard.writeText(...)); update the handleCopy function in
ArtifactPanel.tsx to attach a .catch(...) to
navigator.clipboard.writeText(artifact.code) and surface failures via the
existing useToast hook (show an error toast with a helpful message and error
details). Also search for the same pattern around index.tsx line ~159 and apply
the same fix: add .catch(...) and call useToast to notify users on
permission/availability failures rather than ignoring the error.
Comment on lines
+23
to
+38
| {workspaces.map((ws) => ( | ||
| <Tooltip.Root key={ws.id} lazy> | ||
| <Tooltip.Trigger | ||
| className={styles.railWorkspace} | ||
| data-active={ws.id === activeWorkspaceId ? '' : undefined} | ||
| onClick={() => onSwitchWorkspace(ws.id)} | ||
| > | ||
| <span className={styles.railIndicator} /> | ||
| <Avatar name={ws.name} shape="rounded" size="md" /> | ||
| </Tooltip.Trigger> | ||
| <Tooltip.Portal> | ||
| <Tooltip.Positioner> | ||
| <Tooltip.Popup>{ws.name}</Tooltip.Popup> | ||
| </Tooltip.Positioner> | ||
| </Tooltip.Portal> | ||
| </Tooltip.Root> |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding keyboard support for workspace selection.
The workspace items use Tooltip.Trigger which may not provide full keyboard navigation. Users should be able to tab through workspaces and activate with Enter/Space for accessibility compliance.
Potential enhancement using a button element
<Tooltip.Trigger
+ as="button"
+ type="button"
className={styles.railWorkspace}
data-active={ws.id === activeWorkspaceId ? '' : undefined}
onClick={() => onSwitchWorkspace(ws.id)}
>Alternatively, ensure Tooltip.Trigger renders as an interactive element by default, or add tabIndex={0} and onKeyDown handlers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/showcase/src/demos/chat-app/components/WorkspaceRail.tsx` around lines
23 - 38, The workspace items in WorkspaceRail are not keyboard-accessible
because Tooltip.Trigger is being used as a non-interactive element; update the
workspace item to be an actual interactive control (preferably a <button>) or
ensure Tooltip.Trigger renders as a button and add keyboard handlers: make the
trigger focusable (tabIndex=0 if not already), implement onKeyDown to call
onSwitchWorkspace(ws.id) when Enter or Space is pressed, and expose proper ARIA
state (e.g., aria-current or aria-pressed) tied to activeWorkspaceId; update the
Tooltip.Trigger usage around workspaces.map, the onClick handler, and include
accessibility attributes so keyboard users can tab and activate each workspace.
Comment on lines
+328
to
+330
| export function getUserById(users: User[], id: string): User { | ||
| return users.find((u) => u.id === id) ?? { id, name: 'Unknown', avatar: '?', status: 'offline' as const }; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider returning undefined instead of a fallback object.
getUserById silently returns a synthetic "Unknown" user when the ID doesn't exist. This can mask bugs where an invalid user ID is passed. Returning undefined (or throwing) would make misuse more obvious during development.
Alternative: return undefined and let callers handle it
-export function getUserById(users: User[], id: string): User {
- return users.find((u) => u.id === id) ?? { id, name: 'Unknown', avatar: '?', status: 'offline' as const };
+export function getUserById(users: User[], id: string): User | undefined {
+ return users.find((u) => u.id === id);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/showcase/src/demos/chat-app/data.ts` around lines 328 - 330, getUserById
currently hides missing users by returning a synthetic "Unknown" User object;
change getUserById(users: User[], id: string): User to return undefined when not
found (i.e., adjust its return type to User | undefined and return the result of
users.find(...) directly), and update any callers of getUserById to handle the
undefined case (either by conditional checks or explicit errors) so missing IDs
are not silently masked.
Comment on lines
+167
to
+200
| const toggleReaction = useCallback((messageId: string, emoji: string) => { | ||
| const toggleInList = (msgs: Message[]): Message[] => | ||
| msgs.map((m) => { | ||
| if (m.id !== messageId) return m; | ||
| const existing = m.reactions.find((r) => r.emoji === emoji); | ||
| if (existing) { | ||
| const hasUser = existing.userIds.includes(CURRENT_USER.id); | ||
| const updatedUserIds = hasUser | ||
| ? existing.userIds.filter((uid) => uid !== CURRENT_USER.id) | ||
| : [...existing.userIds, CURRENT_USER.id]; | ||
| return { | ||
| ...m, | ||
| reactions: updatedUserIds.length > 0 | ||
| ? m.reactions.map((r) => (r.emoji === emoji ? { ...r, userIds: updatedUserIds } : r)) | ||
| : m.reactions.filter((r) => r.emoji !== emoji), | ||
| }; | ||
| } | ||
| return { ...m, reactions: [...m.reactions, { emoji, userIds: [CURRENT_USER.id] }] }; | ||
| }); | ||
|
|
||
| // Try channel messages first | ||
| setMessagesByChannel((prev) => ({ | ||
| ...prev, | ||
| [activeChannelId]: toggleInList(prev[activeChannelId] ?? []), | ||
| })); | ||
|
|
||
| // Also try thread replies | ||
| if (activeThreadId) { | ||
| setThreadReplies((prev) => ({ | ||
| ...prev, | ||
| [activeThreadId]: toggleInList(prev[activeThreadId] ?? []), | ||
| })); | ||
| } | ||
| }, [activeChannelId, activeThreadId]); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Reaction toggle may miss messages outside the active channel/thread.
toggleReaction only searches messagesByChannel[activeChannelId] and threadReplies[activeThreadId]. If a message ID from a different channel is passed (e.g., via a stale reference), the reaction silently does nothing. For a demo this is acceptable, but consider adding a guard or console warning for debugging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/showcase/src/demos/chat-app/hooks/useChat.ts` around lines 167 - 200,
toggleReaction currently only updates messages in
messagesByChannel[activeChannelId] and threadReplies[activeThreadId], so passing
a messageId from another channel/thread is silently ignored; update
toggleReaction to detect whether toggleInList actually changed any message in
those stores and, if neither update touched a message, log a console.warn (or
process logger) including the messageId and activeChannelId/activeThreadId for
debugging, or alternatively return a boolean indicating success; reference the
existing functions/vars toggleReaction, toggleInList, setMessagesByChannel,
setThreadReplies, activeChannelId and activeThreadId to implement the
guard/warning where the two setState calls are made.
Comment on lines
+172
to
+185
| const row = info.row.original; | ||
| const pct = (row.memory / row.memoryLimit) * 100; | ||
| return ( | ||
| <div className={styles.meterCell}> | ||
| <Meter | ||
| value={pct} | ||
| min={0} | ||
| max={100} | ||
| high={85} | ||
| optimum={50} | ||
| size="sm" | ||
| aria-label={`Memory: ${row.memory} MB / ${row.memoryLimit} MB`} | ||
| /> | ||
| <span className={styles.meterLabel}>{row.memory} / {row.memoryLimit} MB</span> |
There was a problem hiding this comment.
Guard memory percentage against zero memory limits.
At Line 173, row.memoryLimit can theoretically be 0; this yields Infinity/NaN for Meter.value and can break rendering semantics.
Proposed fix
- const pct = (row.memory / row.memoryLimit) * 100;
+ const pct = row.memoryLimit > 0 ? (row.memory / row.memoryLimit) * 100 : 0;📝 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
| const row = info.row.original; | |
| const pct = (row.memory / row.memoryLimit) * 100; | |
| return ( | |
| <div className={styles.meterCell}> | |
| <Meter | |
| value={pct} | |
| min={0} | |
| max={100} | |
| high={85} | |
| optimum={50} | |
| size="sm" | |
| aria-label={`Memory: ${row.memory} MB / ${row.memoryLimit} MB`} | |
| /> | |
| <span className={styles.meterLabel}>{row.memory} / {row.memoryLimit} MB</span> | |
| const row = info.row.original; | |
| const pct = row.memoryLimit > 0 ? (row.memory / row.memoryLimit) * 100 : 0; | |
| return ( | |
| <div className={styles.meterCell}> | |
| <Meter | |
| value={pct} | |
| min={0} | |
| max={100} | |
| high={85} | |
| optimum={50} | |
| size="sm" | |
| aria-label={`Memory: ${row.memory} MB / ${row.memoryLimit} MB`} | |
| /> | |
| <span className={styles.meterLabel}>{row.memory} / {row.memoryLimit} MB</span> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/showcase/src/demos/container-management/components/ContainerList.tsx`
around lines 172 - 185, Guard against division-by-zero when computing pct for
the Meter: replace the naive (row.memory / row.memoryLimit) * 100 with a safe
calculation (compute pct only if row.memoryLimit > 0, otherwise use 0), and
clamp the result to the 0–100 range before passing it to the Meter.value; update
the computed pct used for Meter and keep the aria-label/label using row.memory
and row.memoryLimit as-is. This change should be made around the pct calculation
in ContainerList (where const row = info.row.original and Meter is rendered).
Comment on lines
+14
to
+18
| const handleKeyDown = useCallback( | ||
| (e: KeyboardEvent<HTMLInputElement>) => { | ||
| if (e.key === 'Enter' && search.trim()) { | ||
| onNavigate(ensureProtocol(search.trim())); | ||
| } |
There was a problem hiding this comment.
Don't advertise search until this field handles search queries.
Line 16 treats every non-empty value as a URL, so queries like react hooks become invalid https://... targets. Either add a search fallback or change Line 32 to URL-only copy.
Also applies to: 32-33
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/showcase/src/demos/web-browser/components/NewTabPage.tsx` around lines
14 - 18, The key handler handleKeyDown currently treats any non-empty input as a
URL via ensureProtocol(search.trim()), causing plain search queries to become
invalid URLs; update handleKeyDown to detect whether the input is a URL (e.g.,
has a scheme or contains a domain-like pattern such as a dot or startsWith
http/https) and only call ensureProtocol/onNavigate for URL-like inputs,
otherwise construct a search URL (encodeURIComponent) and navigate to that (or
call a search function); alternatively, if you prefer not to implement search,
change the UI copy/placeholder text referenced around lines 32-33 to explicitly
say “Enter URL” instead of advertising search so users aren’t misled (refer to
handleKeyDown, ensureProtocol, and onNavigate to locate the logic and the
placeholder/copy state).
Comment on lines
+26
to
+33
| <Input.Root size="md"> | ||
| <Input.Control | ||
| startDecorator={<LuSearch size={16} />} | ||
| value={search} | ||
| onChange={(e) => setSearch(e.target.value)} | ||
| onKeyDown={handleKeyDown} | ||
| placeholder="Search or enter URL" | ||
| /> |
There was a problem hiding this comment.
Give the new-tab input an accessible name.
Line 27 renders the only text input on the page without a label or aria-label. Placeholder text is not a reliable accessible name.
Suggested fix
<Input.Root size="md">
<Input.Control
+ aria-label="Search or enter URL"
startDecorator={<LuSearch size={16} />}
value={search}
onChange={(e) => setSearch(e.target.value)}
onKeyDown={handleKeyDown}
placeholder="Search or enter URL"📝 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
| <Input.Root size="md"> | |
| <Input.Control | |
| startDecorator={<LuSearch size={16} />} | |
| value={search} | |
| onChange={(e) => setSearch(e.target.value)} | |
| onKeyDown={handleKeyDown} | |
| placeholder="Search or enter URL" | |
| /> | |
| <Input.Root size="md"> | |
| <Input.Control | |
| aria-label="Search or enter URL" | |
| startDecorator={<LuSearch size={16} />} | |
| value={search} | |
| onChange={(e) => setSearch(e.target.value)} | |
| onKeyDown={handleKeyDown} | |
| placeholder="Search or enter URL" | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/showcase/src/demos/web-browser/components/NewTabPage.tsx` around lines
26 - 33, The input in the NewTabPage component (Input.Control) lacks an
accessible name; update the Input.Control usage to provide one by adding an
aria-label (e.g., aria-label="Search or enter URL") or by rendering/associating
a visible <label> and using aria-labelledby pointing to that label's id so the
text field has a programmatic name; modify the Input.Control props in NewTabPage
to include the chosen aria attribute or wire up a label component to ensure
accessibility.
Comment on lines
+20
to
+23
| export function ensureProtocol(input: string): string { | ||
| if (/^[a-zA-Z][\w+\-.]*:/.test(input) || input.startsWith('//')) return input; | ||
| return `https://${input}`; | ||
| } |
There was a problem hiding this comment.
Whitelist safe schemes in ensureProtocol.
Line 21 now passes any absolute scheme through unchanged. Because the toolbar and new-tab page feed raw user input through this helper, values like javascript: and data: are accepted too. Keep the internal new-tab sentinel, but restrict user-entered navigation to the schemes this demo is prepared to render.
Suggested fix
export function ensureProtocol(input: string): string {
- if (/^[a-zA-Z][\w+\-.]*:/.test(input) || input.startsWith('//')) return input;
- return `https://${input}`;
+ const value = input.trim();
+ if (!value || value === NEW_TAB_URL || /^https?:\/\//i.test(value)) return value;
+ if (value.startsWith('//')) return `https:${value}`;
+ return `https://${value}`;
}📝 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
| export function ensureProtocol(input: string): string { | |
| if (/^[a-zA-Z][\w+\-.]*:/.test(input) || input.startsWith('//')) return input; | |
| return `https://${input}`; | |
| } | |
| export function ensureProtocol(input: string): string { | |
| const value = input.trim(); | |
| if (!value || value === NEW_TAB_URL || /^https?:\/\//i.test(value)) return value; | |
| if (value.startsWith('//')) return `https:${value}`; | |
| return `https://${value}`; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/showcase/src/demos/web-browser/data.ts` around lines 20 - 23, The
function ensureProtocol currently allows any scheme (e.g., javascript: or data:)
to pass; change its check to only allow safe schemes and the internal new-tab
sentinel. In ensureProtocol, replace the general scheme test
/^[a-zA-Z][\w+\-.]*:/ with a strict check for http and https (e.g., /^https?:/i)
while still allowing input.startsWith('//'); for any other input, prepend
"https://". This ensures only http/https and the existing '//' sentinel are
accepted unchanged.
Comment on lines
+139
to
+142
| const refresh = useCallback(() => { | ||
| updateActiveTab((t) => ({ ...t, loading: true, error: undefined })); | ||
| startBlockedTimeout(); | ||
| }, [updateActiveTab, startBlockedTimeout]); |
There was a problem hiding this comment.
Guard refresh() for New Tab to avoid false blocked errors.
Line [140]-Line [142] always force loading: true and timeout. If active URL is NEW_TAB_URL, this can incorrectly end in error: 'blocked'.
Suggested fix
const refresh = useCallback(() => {
- updateActiveTab((t) => ({ ...t, loading: true, error: undefined }));
- startBlockedTimeout();
+ let shouldStartTimeout = false;
+ updateActiveTab((t) => {
+ if (t.url === NEW_TAB_URL) {
+ return { ...t, loading: false, error: undefined };
+ }
+ shouldStartTimeout = true;
+ return { ...t, loading: true, error: undefined };
+ });
+ if (shouldStartTimeout) startBlockedTimeout();
}, [updateActiveTab, startBlockedTimeout]);📝 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
| const refresh = useCallback(() => { | |
| updateActiveTab((t) => ({ ...t, loading: true, error: undefined })); | |
| startBlockedTimeout(); | |
| }, [updateActiveTab, startBlockedTimeout]); | |
| const refresh = useCallback(() => { | |
| let shouldStartTimeout = false; | |
| updateActiveTab((t) => { | |
| if (t.url === NEW_TAB_URL) { | |
| return { ...t, loading: false, error: undefined }; | |
| } | |
| shouldStartTimeout = true; | |
| return { ...t, loading: true, error: undefined }; | |
| }); | |
| if (shouldStartTimeout) startBlockedTimeout(); | |
| }, [updateActiveTab, startBlockedTimeout]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/showcase/src/demos/web-browser/hooks/useBrowser.ts` around lines 139 -
142, The refresh callback should skip marking a new-tab as loading and not start
the blocked timeout when the active tab is the new-tab; change refresh to first
check the current active tab's URL (use the existing activeTab state or an
activeTabRef) and if activeTab.url === NEW_TAB_URL return early, otherwise call
updateActiveTab(t => ({ ...t, loading: true, error: undefined })) and then call
startBlockedTimeout(); ensure you reference refresh, updateActiveTab,
startBlockedTimeout and NEW_TAB_URL when making this change.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
apps/showcase/src/demos/web-browser/index.module.css (1)
169-183:⚠️ Potential issue | 🟠 MajorAdd
:focus-visiblestate for keyboard accessibility.The
.errorButtonhas a:hoverstate but still lacks a:focus-visiblestate. This was flagged in the previous review (the "Also applies to: 173-175" note). Keyboard users won't see a focus indicator when tabbing to this button.🎯 Proposed fix
.errorButton:hover { background: var(--ov-color-bg-surface-raised); } + +.errorButton:focus-visible { + background: var(--ov-color-bg-surface-raised); + outline: 2px solid var(--ov-color-border-focus); + outline-offset: -2px; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/showcase/src/demos/web-browser/index.module.css` around lines 169 - 183, The .errorButton rule lacks a :focus-visible style for keyboard users; add a matching .errorButton:focus-visible selector (mirroring the hover visual language) that applies a clear visible focus indicator (e.g., a visible outline or adjusted background/border color) to the existing .errorButton styles so keyboard navigation shows focus when tabbing to the button; update the CSS near the .errorButton and .errorButton:hover blocks to include .errorButton:focus-visible with accessible contrast and not rely solely on :hover.apps/showcase/src/demos/ai-chat/components/ArtifactPanel.tsx (1)
22-25:⚠️ Potential issue | 🟠 MajorGuard Clipboard API availability and don’t swallow copy failures.
At Line 23,
navigator.clipboardcan be unavailable (e.g., insecure context), which throws before.catch()runs. The current empty catch also hides failure from users.🔧 Suggested fix
export function ArtifactPanel({ artifact, onClose }: ArtifactPanelProps) { const handleCopy = useCallback(() => { - navigator.clipboard.writeText(artifact.code).catch(() => { - // Clipboard API can fail in insecure contexts - }); + if (!navigator.clipboard?.writeText) { + // Surface via your app's toast/notification system + return; + } + navigator.clipboard.writeText(artifact.code).catch((error) => { + // Surface via your app's toast/notification system + console.error('Failed to copy artifact code:', error); + }); }, [artifact.code]);#!/bin/bash # Verify clipboard writes that are unguarded or silently swallowed. rg -n -C2 'navigator\.clipboard\.writeText\(' --glob='*.ts' --glob='*.tsx' rg -n -C2 'writeText\([^)]*\)\.catch\(\(\)\s*=>\s*\{\s*\}\)' --glob='*.ts' --glob='*.tsx'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/showcase/src/demos/ai-chat/components/ArtifactPanel.tsx` around lines 22 - 25, The handleCopy function currently calls navigator.clipboard.writeText(artifact.code) without guarding for navigator.clipboard being undefined and swallows failures; update handleCopy to first check for navigator.clipboard and only call writeText when available, wrap the call in a try/catch (or use the returned promise .then/.catch) to surface errors (e.g., set a copy error state, call an onCopyError handler, show a toast/snackbar, and console.error) instead of silently ignoring them, and implement a fallback path (e.g., use a temporary textarea + document.execCommand('copy') or prompt the user to manually copy) that also reports success/failure so callers of handleCopy get proper feedback.apps/showcase/src/demos/ide-editor/components/SidebarPanel.tsx (1)
119-125:⚠️ Potential issue | 🟠 MajorNormalize search hits to the canonical file id before calling
onOpenFile.Line 124 forwards
result.file, but the rest of this demo treatsonOpenFileas a file-id callback:FileTreePanelpasses item ids, andapps/showcase/src/demos/ide-editor/index.tsx, Lines 74-94, resolves the value withprojectFiles.find((f) => f.id === fileId). Any search result whosefilefield is a display path/name will still no-op here unless you translate it back to the matching project file first.💡 Suggested fix
import { fileTree, + projectFiles, searchResults, gitStatus, type FileTreeNode, } from '../data'; @@ <button type="button" // eslint-disable-next-line react/no-array-index-key key={`${result.file}-${result.line}-${i}`} className={styles.resultItem} - onClick={() => onOpenFile(result.file)} + onClick={() => { + const file = projectFiles.find( + (candidate) => + candidate.id === result.file || candidate.path === result.file, + ); + if (file) onOpenFile(file.id); + }} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/showcase/src/demos/ide-editor/components/SidebarPanel.tsx` around lines 119 - 125, Search result buttons currently pass result.file directly to onOpenFile, but onOpenFile expects a canonical file id (as used by FileTreePanel and the projectFiles.find((f) => f.id === fileId) lookup). Before calling onOpenFile from the button in SidebarPanel (where result.file is used), normalize the hit by resolving the display path/name back to the corresponding project file id (e.g., look up projectFiles.find(f => matches result.file on f.path or f.name) and use f.id). Then call onOpenFile with that canonical id so the existing resolver logic in index.tsx and FileTreePanel works correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/showcase/src/demos/ai-chat/components/ArtifactPanel.tsx`:
- Around line 22-25: The handleCopy function currently calls
navigator.clipboard.writeText(artifact.code) without guarding for
navigator.clipboard being undefined and swallows failures; update handleCopy to
first check for navigator.clipboard and only call writeText when available, wrap
the call in a try/catch (or use the returned promise .then/.catch) to surface
errors (e.g., set a copy error state, call an onCopyError handler, show a
toast/snackbar, and console.error) instead of silently ignoring them, and
implement a fallback path (e.g., use a temporary textarea +
document.execCommand('copy') or prompt the user to manually copy) that also
reports success/failure so callers of handleCopy get proper feedback.
In `@apps/showcase/src/demos/ide-editor/components/SidebarPanel.tsx`:
- Around line 119-125: Search result buttons currently pass result.file directly
to onOpenFile, but onOpenFile expects a canonical file id (as used by
FileTreePanel and the projectFiles.find((f) => f.id === fileId) lookup). Before
calling onOpenFile from the button in SidebarPanel (where result.file is used),
normalize the hit by resolving the display path/name back to the corresponding
project file id (e.g., look up projectFiles.find(f => matches result.file on
f.path or f.name) and use f.id). Then call onOpenFile with that canonical id so
the existing resolver logic in index.tsx and FileTreePanel works correctly.
In `@apps/showcase/src/demos/web-browser/index.module.css`:
- Around line 169-183: The .errorButton rule lacks a :focus-visible style for
keyboard users; add a matching .errorButton:focus-visible selector (mirroring
the hover visual language) that applies a clear visible focus indicator (e.g., a
visible outline or adjusted background/border color) to the existing
.errorButton styles so keyboard navigation shows focus when tabbing to the
button; update the CSS near the .errorButton and .errorButton:hover blocks to
include .errorButton:focus-visible with accessible contrast and not rely solely
on :hover.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 61439897-03b7-42b8-986e-a205832831ae
📒 Files selected for processing (5)
apps/showcase/src/demos/ai-chat/components/ArtifactPanel.tsxapps/showcase/src/demos/ide-editor/components/SidebarPanel.module.cssapps/showcase/src/demos/ide-editor/components/SidebarPanel.tsxapps/showcase/src/demos/web-browser/components/ErrorPage.tsxapps/showcase/src/demos/web-browser/index.module.css
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/showcase/src/demos/ide-editor/components/SidebarPanel.tsx`:
- Around line 26-34: The handler handleSelectedKeysChange currently spreads the
ReadonlySet into an array to get the first element (const id = [...keys][0]),
which creates an unnecessary allocation; change it to read the first entry
directly from the iterator (use keys.values().next()) and extract .value (and
guard against .done if you want explicit safety) before calling
onOpenFile(String(id)) so the behavior remains identical but without allocating
an intermediate array.
- Around line 145-151: The switch in statusToStatusDot lacks an exhaustive
default branch for GitStatusEntry['status']; add a final default case that
accepts a value typed as never (e.g., default: const _exhaustiveCheck: never =
status;) and throw or return a safe fallback so the compiler will error if the
union is extended. This change should be implemented inside function
statusToStatusDot to ensure future additions to GitStatusEntry['status'] are
caught at compile time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 209d9d6b-6296-47ca-b4e0-5d1ad6f20042
📒 Files selected for processing (2)
apps/showcase/src/demos/ide-editor/components/SidebarPanel.tsxapps/showcase/src/demos/web-browser/index.module.css
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/showcase/src/demos/ide-editor/components/SidebarPanel.tsx (1)
26-34: 🧹 Nitpick | 🔵 TrivialMinor: Prefer iterator over spread for single-element extraction.
Spreading the entire
ReadonlySetinto an array just to grab the first element allocates an intermediate array. Using the iterator directly is more efficient.♻️ Suggested improvement
const handleSelectedKeysChange = useCallback( (keys: ReadonlySet<Key>) => { - const id = [...keys][0]; + const id = keys.values().next().value; if (id != null) { onOpenFile(String(id)); } }, [onOpenFile], );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/showcase/src/demos/ide-editor/components/SidebarPanel.tsx` around lines 26 - 34, The handler handleSelectedKeysChange currently spreads the ReadonlySet into an array to get the first element which allocates an intermediate array; change it to use the set's iterator (e.g., keys.values().next()) or keys[Symbol.iterator]().next() to obtain the first Key without creating an array, then call onOpenFile(String(id)) as before; update references to handleSelectedKeysChange and onOpenFile accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/showcase/src/demos/ide-editor/components/SidebarPanel.tsx`:
- Around line 180-189: The switch in SidebarPanel may return undefined for
future SidebarPanelType values; add an exhaustiveness check like the
statusToStatusDot pattern to ensure compile-time safety: after the existing
cases add a default that assigns the unhandled value to a never-typed variable
(e.g., const _exhaustive: never = panel) and throw or return a sensible fallback
to satisfy the compiler. Update the SidebarPanel function to include this
default branch so any new union members cause a type error until handled.
---
Duplicate comments:
In `@apps/showcase/src/demos/ide-editor/components/SidebarPanel.tsx`:
- Around line 26-34: The handler handleSelectedKeysChange currently spreads the
ReadonlySet into an array to get the first element which allocates an
intermediate array; change it to use the set's iterator (e.g.,
keys.values().next()) or keys[Symbol.iterator]().next() to obtain the first Key
without creating an array, then call onOpenFile(String(id)) as before; update
references to handleSelectedKeysChange and onOpenFile accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5a1809fc-1263-4035-995b-e90fea2d60f9
📒 Files selected for processing (1)
apps/showcase/src/demos/ide-editor/components/SidebarPanel.tsx
All three library packages (base-ui, ai-ui, editors) generated declarations under dist/src/ instead of dist/, causing TS7016 errors for consumers. Adding rollupTypes: true to vite-plugin-dts bundles them to the expected path.
…er with icon toggle - Cards now use Card.ActionArea, Card.Header, Card.Title, Card.Description and Card.Group (which provides correct min-width-aware grid) - Replace ThemeSwitcher (3-dropdown form) with useTheme() + IconButton dark/light toggle suitable for the 52px dock - Use Tooltip compound API (Root/Trigger/Portal/Positioner/Popup) - Use correct IconButton props: 'solid'/'ghost' variants, 'brand'/'neutral' colors
- Add Monaco worker setup section to IDE Editor - Add scripted replay timing guidance to AI Chat - Commit to DockLayout (remove AppShell option) - Add CSS import requirements for ai-ui and editors - Fix Typography compound API usage (no .Code sub-component) - Reduce mock containers from 50 to 15-20 with full data - Add FilterBar fallback strategy - Fix coverage matrix (add Separator, ChatAvatar; remove Dialog/Drawer, AppShell) - Add navigation state management and component API conventions - Note remaining 3 demos deferred to future spec
- Fix Typography.Code — it does exist, use it for inline code - Clarify AIArtifact/AIBranch use separate named exports, not dot-notation - Restore 50 container count (15-20 unique + ~30 generated) for virtual scrolling - Split NavList/List into separate coverage matrix rows - Add coverage notes for uncovered ai-ui (~20) and base-ui (~60) components - Add DockLayout fallback strategy in gaps table - Add editor package note for Container Mgmt (Terminal uses xterm, not Monaco) - Add 'use no memo' convention for DataTable MemoizedRow
Minor fixes from review round 3 suggestions: - Add AIArtifactDescription to AI Chat component table - Add NavList to IDE Editor component table (aligns with coverage matrix)
Four-chunk plan covering File Explorer, IDE Editor, AI Chat, and Container Management demos. All component APIs verified from source and corrected after 4 parallel review passes.
Implements TreePane component wrapping TreeList with breadcrumb header, and FileExplorer root layout using nested ResizableSplitPane for dual local/remote trees with a detail placeholder.
Implements Task 5 — full IDE editor layout with: - IconStrip: vertical panel switcher using Tooltip.Root compound API - SidebarPanel: files (TreeList), search (SearchInput + results), git (StatusDot) - Main IDE layout: ResizableSplitPane (horizontal sidebar + vertical editor/terminal) - EditorTabs tab bar driving CodeEditor, DiffViewer, MarkdownPreview based on tab type - Terminal via ref + TerminalHandle, writing terminalOutput in onReady - CommandPalette wired to Ctrl+K/Cmd+K with toggle-terminal command - Monaco workers set up at module level via setupMonacoWorkers() Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…treaming and artifacts Implements Tasks 8 and 9 — the full AI Chat demo with: - MessageRenderer component rendering ChatBubble, ThinkingBlock, ToolCall/ToolResult, AIMarkdown, AIBranch navigation, AISources, AIContextIndicator, and AIMessageActions - ArtifactPanel component using AIArtifact* named exports with AICodeBlock content - useScriptedReplay hook driving a state machine through typing → thinking → tool-call → streaming (character-by-character) → artifact → follow-up → done phases - Main layout using ResizableSplitPane for split chat/artifact view when an artifact is open, with AIConversationHeader, AIModelSelector, ChatMessageList (virtualised), ChatSuggestions, AIFollowUp, AIStopButton, and ChatInput Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Spec for Chrome-style browser chrome simulator demo, including EditorTabs variant="pill" base-ui enhancement and iframe-based page loading with error handling.
9-task plan across 4 chunks: EditorTabs pill variant, types/data/CSS, demo components (toolbar, bookmarks, new-tab, viewport), and assembly.
Extends the EditorTabsProps variant type (local Omit union, not touching ComponentVariant) and adds pill CSS rules: rounded top corners, no bottom border indicator, transparent dividers — matching Chrome/JetBrains tab aesthetics for the showcase browser demo.
- EditorTabs pill variant: use --ov-color-bg-* tokens instead of non-existent --ov-color-surface-* tokens - BrowserToolbar: remove dense prop from IconButtons so they match Input sm height (28px) - CSS module: fix all surface token references to actual --ov-color-bg-*
…ctionBar, TypingIndicator
…Panel, final assembly
Add three new dark theme presets with deeper backgrounds and crisper contrast. Extend ThemeMode type and isThemeMode guard, add CSS token blocks for all three themes, and replace the showcase dock's light/dark toggle with a Menu-based theme picker showing all 7 available themes.
- Fix ResizableSplitPane reverse prop, List/NavList avatar accommodation - Chat app: DM avatar shape, status dot overlay, thread resize - AI chat, IDE editor, container management, file explorer refinements - Accordion, DataTable, Menu, TreeList, StatusBar style fixes - Add ChatInput component to ai-ui package - Design specs and plans for theme palettes and base-ui components
- useChat: move global ID counter to useRef, fix date boundary calc - ContainerList: clickable div → semantic button, guard div-by-zero - containerStatusColor: use ContainerStatus type instead of string - FilePane: fix rename icon (LuTextCursor), type accessor params - TransferPanel: add scope="col" to th elements - TreePane: add role="region", CommandLog: add type="button" - IconStrip: div[role=navigation] → semantic nav element - SidebarPanel: wire SearchPanel onOpenFile handlers - IDE editor: case-insensitive Cmd+K, fix stale tabs in close handler - BrowserToolbar: use NEW_TAB_URL constant, fix ensureProtocol schemes - useBrowser: conditional timeout in goBack/goForward, remove ! assertion - Add focus-visible to speedDialCard, type="button" to misc buttons - Remove unwired "Apply to editor" action from ArtifactPanel - CSS: overflow-wrap instead of word-break, remove composes, font smoothing
- ArtifactPanel: add .catch to clipboard write for insecure contexts - SidebarPanel: replace div[role=button] with native button + CSS reset - ErrorPage: reword to avoid unescaped apostrophe - errorUrl: add overflow-wrap for long URLs
…ible - SidebarPanel: resolve result.file path to canonical fileId before calling onOpenFile so the tab open logic works correctly - errorButton: add :focus-visible for keyboard accessibility
Merging this PR will not alter performance
Comparing 
Footnotes
|
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.
Summary
apps/showcase) — a Vite-powered demo harness with a macOS-style dock for switching between interactive demosFileTable,SortableHeader/useSortableTable,Tabs.TabendDecorator,EditorTabspill variant,ResizableSplitPanereverse propThemeModetype extension,isThemeModeguard updateTest plan
pnpm --filter @omniview/base-ui build— no errorscd apps/showcase && pnpm exec tsc --noEmit— no type errorsSummary by CodeRabbit
New Features
Chores
Documentation