feat: web UX parity — hasUnread pipeline + Auto Run tab enhancements#739
feat: web UX parity — hasUnread pipeline + Auto Run tab enhancements#739pedramamini merged 4 commits intorcfrom
Conversation
📝 WalkthroughWalkthroughAdded optional Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as AutoRunTabContent
participant Hook as useAutoRun
participant WS as WebSocket (send/sendRequest)
participant Server as Server
User->>UI: Open AutoRun tab / Tap refresh
UI->>Hook: fetchDocuments(sessionId)
Hook->>WS: sendRequest(fetchDocuments)
WS->>Server: request documents
Server-->>WS: documents list
WS-->>Hook: documents payload
Hook-->>UI: documents
UI->>User: render DocumentCard list
User->>UI: Tap DocumentCard(filename)
UI->>WS: send(openDocument, filename)
WS->>Server: open document request
Server-->>WS: ack
WS-->>UI: ack -> UI invokes onOpenDocument(filename)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR wires Confidence Score: 5/5Safe to merge; both remaining findings are P2 style issues with no runtime impact The
Important Files Changed
Sequence DiagramsequenceDiagram
participant Desktop as Renderer (useRemoteIntegration)
participant Factory as web-server-factory
participant WS as WebSocket Broadcast
participant Web as Web/Mobile Client
Note over Desktop: interval tick (500ms)
Desktop->>Desktop: build tabsHash (id:name:starred:state:hasUnread)
Desktop->>WS: broadcastTabsChange(sessionId, tabs[], activeTabId)
WS->>Web: tabs_changed { aiTabs[{id, state, hasUnread, ...}], activeTabId }
Web->>Web: render red dot if any tab.hasUnread === true
Note over Factory,Web: On initial connect / getSessions
Factory->>Factory: serialize aiTabs (hasUnread ?? false)
Factory->>Web: sessions payload with hasUnread in each AITabData
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| onTap: (filename: string) => void; | ||
| } | ||
|
|
||
| export function DocumentCard({ document, onTap }: DocumentCardProps) { |
There was a problem hiding this comment.
document shadows the global DOM object
Destructuring a prop called document shadows window.document inside the function body. The component doesn't currently need the DOM global, but it's a footgun for future edits. A clearer name like doc avoids the collision.
| export function DocumentCard({ document, onTap }: DocumentCardProps) { | |
| export function DocumentCard({ document: doc, onTap }: DocumentCardProps) { |
(and rename all uses of document to doc inside the function body)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/hooks/remote/useRemoteIntegration.ts (1)
860-871:⚠️ Potential issue | 🟡 MinorUpdate
global.d.tsto includehasUnreadfield inbroadcastTabsChangedeclaration.The
broadcastTabsChangetype insrc/renderer/global.d.ts(lines 542–556) does not include thehasUnreadfield that is being passed at line 870 ofuseRemoteIntegration.ts. While TypeScript allows extra properties, this type mismatch creates inconsistency with the actual implementation types (src/main/web-server/types.tsandsrc/web/hooks/useWebSocket.ts), which do includehasUnread?: boolean;.Add
hasUnread?: boolean;to theaiTabsarray type in thebroadcastTabsChangedeclaration to keep types in sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/hooks/remote/useRemoteIntegration.ts` around lines 860 - 871, Add the missing optional hasUnread property to the broadcastTabsChange declaration in src/renderer/global.d.ts: update the aiTabs array item type within the broadcastTabsChange type to include hasUnread?: boolean so it matches the actual payload constructed in useRemoteIntegration.ts (the tabsForBroadcast object) and the implementations in src/main/web-server/types.ts and src/web/hooks/useWebSocket.ts; ensure the property is optional to match existing usages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/web/mobile/AutoRunDocumentCard.tsx`:
- Around line 28-49: The button in AutoRunDocumentCard (the element using
handleTap and aria-label with document.filename) currently removes the native
focus ring via outline: 'none'; restore a visible focus indicator by removing
that outline: 'none' and instead add an accessible focus style (e.g., a clear
outline or focus-visible box-shadow/border) so keyboard users can see focus when
tabbing the card; implement this as a focus/focus-visible style on the same
button element (or via a CSS class applied to the component) so the card remains
fully keyboard operable.
In `@src/web/mobile/RightDrawer.tsx`:
- Around line 922-937: Replace the hardcoded Auto Run directory strings with a
single exported constant (e.g., AUTO_RUN_DIR) and use it from both components:
add the constant to a shared module (e.g., export const AUTO_RUN_DIR =
'.maestro/playbooks/' or the canonical path) and import it into RightDrawer
(where the hint currently renders ".maestro/playbooks/") and AutoRunPanel (where
it currently renders ".maestro/auto-run/"); update the JSX in both
RightDrawer.tsx and AutoRunPanel.tsx to render the imported AUTO_RUN_DIR inside
the <code> element so both screens use one source of truth.
- Around line 716-723: The stop flow is silently swallowing unexpected
errors—update the hook and the handler so recoverable errors are surfaced to the
UI while unexpected errors propagate to Sentry: change stopAutoRun in
useAutoRun.ts to return/throw a discriminated result (e.g., { ok: boolean,
error?: 'NETWORK_ERROR' | ... } or throw for unexpected exceptions) instead of
catching all exceptions and returning false, and update handleStop in
RightDrawer.tsx to inspect that result and set UI state / show a user-facing
error for expected recoverable cases (e.g., NETWORK_ERROR) while NOT
catching/re-suppressing unexpected exceptions so they bubble up. Reference
symbols: stopAutoRun (useAutoRun.ts), handleStop (RightDrawer.tsx), and
setIsStopping/triggerHaptic to maintain existing UX.
---
Outside diff comments:
In `@src/renderer/hooks/remote/useRemoteIntegration.ts`:
- Around line 860-871: Add the missing optional hasUnread property to the
broadcastTabsChange declaration in src/renderer/global.d.ts: update the aiTabs
array item type within the broadcastTabsChange type to include hasUnread?:
boolean so it matches the actual payload constructed in useRemoteIntegration.ts
(the tabsForBroadcast object) and the implementations in
src/main/web-server/types.ts and src/web/hooks/useWebSocket.ts; ensure the
property is optional to match existing usages.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5e1a96cf-1032-4601-876f-0e27028bfb1a
📒 Files selected for processing (7)
src/main/web-server/types.tssrc/main/web-server/web-server-factory.tssrc/renderer/hooks/remote/useRemoteIntegration.tssrc/web/mobile/AutoRunDocumentCard.tsxsrc/web/mobile/AutoRunPanel.tsxsrc/web/mobile/RightDrawer.tsxsrc/web/mobile/RightPanel.tsx
| const handleStop = useCallback(async () => { | ||
| triggerHaptic(HAPTIC_PATTERNS.interrupt); | ||
| setIsStopping(true); | ||
| const success = await stopAutoRun(sessionId); | ||
| if (!success) { | ||
| setIsStopping(false); | ||
| } | ||
| }, [sessionId, stopAutoRun]); |
There was a problem hiding this comment.
Don't let stop failures disappear silently.
Line 719 only gets a boolean back, so a network/backend error just flips the button from "Stopping..." back to "Stop". src/web/hooks/useAutoRun.ts:136-146 currently catches every exception and returns false, which means this new control neither tells the user what happened nor preserves unexpected failures for Sentry. Please surface a recoverable error here, and let the unexpected path bubble out of the hook.
As per coding guidelines, "Do not silently swallow errors. Let unhandled exceptions bubble up to Sentry for error tracking in production. Handle expected/recoverable errors explicitly (e.g., NETWORK_ERROR). For unexpected errors, re-throw them to allow Sentry to capture them."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/web/mobile/RightDrawer.tsx` around lines 716 - 723, The stop flow is
silently swallowing unexpected errors—update the hook and the handler so
recoverable errors are surfaced to the UI while unexpected errors propagate to
Sentry: change stopAutoRun in useAutoRun.ts to return/throw a discriminated
result (e.g., { ok: boolean, error?: 'NETWORK_ERROR' | ... } or throw for
unexpected exceptions) instead of catching all exceptions and returning false,
and update handleStop in RightDrawer.tsx to inspect that result and set UI state
/ show a user-facing error for expected recoverable cases (e.g., NETWORK_ERROR)
while NOT catching/re-suppressing unexpected exceptions so they bubble up.
Reference symbols: stopAutoRun (useAutoRun.ts), handleStop (RightDrawer.tsx),
and setIsStopping/triggerHaptic to maintain existing UX.
| <p style={{ fontSize: '13px', color: colors.textDim, margin: '0 0 6px 0' }}> | ||
| No Auto Run documents found | ||
| </p> | ||
| <p style={{ fontSize: '12px', color: colors.textDim, margin: 0, opacity: 0.7 }}> | ||
| Add documents to{' '} | ||
| <code | ||
| style={{ | ||
| fontSize: '11px', | ||
| backgroundColor: `${colors.textDim}15`, | ||
| padding: '1px 4px', | ||
| borderRadius: '3px', | ||
| }} | ||
| > | ||
| .maestro/playbooks/ | ||
| </code>{' '} | ||
| directory |
There was a problem hiding this comment.
Use one source of truth for the Auto Run directory hint.
These lines now tell users to add files under .maestro/playbooks/, but src/web/mobile/AutoRunPanel.tsx:401-416 still points at .maestro/auto-run/. Because both screens render the same Auto Run document list, one of these instructions is wrong and will send users to the wrong folder. Please drive this copy from a shared constant/message.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/web/mobile/RightDrawer.tsx` around lines 922 - 937, Replace the hardcoded
Auto Run directory strings with a single exported constant (e.g., AUTO_RUN_DIR)
and use it from both components: add the constant to a shared module (e.g.,
export const AUTO_RUN_DIR = '.maestro/playbooks/' or the canonical path) and
import it into RightDrawer (where the hint currently renders
".maestro/playbooks/") and AutoRunPanel (where it currently renders
".maestro/auto-run/"); update the JSX in both RightDrawer.tsx and
AutoRunPanel.tsx to render the imported AUTO_RUN_DIR inside the <code> element
so both screens use one source of truth.
|
Thanks for the contribution, @chr1syy! The hasUnread pipeline and Auto Run tab enhancements look solid overall — nice work extracting the shared A few things to address before we can merge: CI failuresBoth lint-and-format and test checks are failing. Please fix those so we get a clean CI run. Stale directory path in
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/web/mobile/AutoRunPanel.tsx (1)
107-114: Consider extracting the repeated stopping-state check.The expression
isStopping || autoRunState?.isStoppingis repeated 7 times throughout the JSX. Extracting it to a single variable alongside the other derived state would improve clarity and reduce duplication.♻️ Suggested extraction
const isRunning = autoRunState?.isRunning ?? false; + const isStopPending = isStopping || autoRunState?.isStopping; const totalTasks = autoRunState?.totalTasks;Then replace all occurrences of
isStopping || autoRunState?.isStoppingwithisStopPending.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/web/mobile/AutoRunPanel.tsx` around lines 107 - 114, Extract the repeated stopping-state check into a single derived variable (e.g., isStopPending) near the other computed state values (alongside isRunning, totalTasks, completedTasks, currentTaskIndex, progress, totalDocs, currentDocIndex) by computing isStopPending = isStopping || autoRunState?.isStopping, and then replace every occurrence of isStopping || autoRunState?.isStopping in the AutoRunPanel JSX with isStopPending to remove duplication and improve clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/web/mobile/AutoRunPanel.tsx`:
- Around line 107-114: Extract the repeated stopping-state check into a single
derived variable (e.g., isStopPending) near the other computed state values
(alongside isRunning, totalTasks, completedTasks, currentTaskIndex, progress,
totalDocs, currentDocIndex) by computing isStopPending = isStopping ||
autoRunState?.isStopping, and then replace every occurrence of isStopping ||
autoRunState?.isStopping in the AutoRunPanel JSX with isStopPending to remove
duplication and improve clarity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fac3a4af-b9a4-424f-b6af-1e94fd356f61
📒 Files selected for processing (3)
src/renderer/global.d.tssrc/web/mobile/AutoRunDocumentCard.tsxsrc/web/mobile/AutoRunPanel.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/web/mobile/AutoRunDocumentCard.tsx
Add hasUnread field to AITabData type, web-server aiTabs mapping, and useRemoteIntegration broadcast hash + payload so unread indicators propagate to web clients. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…op controls Extracted DocumentCard into shared AutoRunDocumentCard.tsx and rewrote AutoRunTabContent to show a document list with progress cards, running status with task/doc counters and progress bar, stop button, and proper loading/empty states. Threaded sessionId, sendRequest, send, and onOpenDocument props through RightDrawer and RightPanel. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…sion Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…, rename document prop - Add hasUnread?: boolean to broadcastTabsChange type in global.d.ts - Fix stale .maestro/auto-run/ path to .maestro/playbooks/ in AutoRunPanel - Rename document prop to doc to avoid shadowing window.document - Remove outline: 'none' from DocumentCard button for keyboard accessibility Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
chr1syy
force-pushed
the
feat/feat-web-ux-parity
branch
from
fbda513 to
70b0aa8
Compare
|
Yay loving me some auto run improvements 👍 |
3 participants
Summary
hasUnreadtoAITabDatatype, serialize it in web-server factory, and include it in the tabs broadcast hash so web clients receive unread notification state changes (red dots)DocumentCardcomponent extracted fromAutoRunPanelisStoppedexpressionChanged files
src/main/web-server/types.tshasUnreadtoAITabDatasrc/main/web-server/web-server-factory.tshasUnreadin aiTabs serializationsrc/renderer/hooks/remote/useRemoteIntegration.tshasUnreadin tabs hash + broadcastsrc/web/mobile/AutoRunDocumentCard.tsxsrc/web/mobile/AutoRunPanel.tsxsrc/web/mobile/RightDrawer.tsxsrc/web/mobile/RightPanel.tsxTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor