-
Notifications
You must be signed in to change notification settings - Fork 547
feat: simplify message steps display and enhance timing system #252
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
feat: simplify message steps display and enhance timing system #252
Conversation
|
The following comment was made by an LLM, it may be inaccurate: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request simplifies the message steps display in the OpenWork UI to show only essential information, making it cleaner and easier to scan. It also adds a send/stop button to the composer and implements an enhanced timing system to track message durations and completion states.
Changes:
- Simplified message steps display to single-line format with smart tool-specific formatting
- Added send/stop button in composer with abort functionality
- Implemented per-message timing tracking with human-readable duration display and end reason labels
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/app/src/app/utils/index.ts | Enhanced normalizeSessionStatus to handle more status types; refactored summarizeStep with tool-specific formatting and GitHub icon detection; added helper functions for path shortening and formatting |
| packages/app/src/app/types.ts | Added MessageEndReason and MessageTiming types for timing tracking |
| packages/app/src/app/context/session.ts | Implemented timing tracking system with start/end timestamps, end reasons, and finalization logic for interrupted/terminated/error states |
| packages/app/src/app/components/session/message-list.tsx | Refactored steps display to single-line format; added timing display with Clock icon; moved copy button outside user bubbles; reduced padding and adjusted styling |
| packages/app/src/app/components/session/composer.tsx | Added conditional send/stop button with red square stop icon when busy |
| packages/app/src/app/pages/session.tsx | Added formatElapsedTime function; modified scroll behavior to only trigger on new messages; added cancelRun handler and messageTimings prop |
| packages/app/src/app/app.tsx | Implemented cancelRun function to abort sessions with "interrupted" end reason; added messageTimings prop passing |
| .gitignore | Added local development notes files to ignore list |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const formatElapsedTime = (ms: number): string => { | ||
| const seconds = Math.floor(ms / 1000); | ||
| const minutes = Math.floor(seconds / 60); | ||
| const hours = Math.floor(minutes / 60); | ||
|
|
||
| if (hours > 0) { | ||
| const remainingMinutes = minutes % 60; | ||
| return remainingMinutes > 0 ? `${hours}h ${remainingMinutes}m` : `${hours}h`; | ||
| } | ||
| if (minutes > 0) { | ||
| const remainingSeconds = seconds % 60; | ||
| return remainingSeconds > 0 ? `${minutes}m ${remainingSeconds}s` : `${minutes}m`; | ||
| } | ||
| if (seconds > 0) { | ||
| return `${seconds}s`; | ||
| } | ||
| return `${ms}ms`; | ||
| }; |
Copilot
AI
Jan 25, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The formatElapsedTime function is duplicated in both session.tsx (lines 325-342) and message-list.tsx (lines 10-27). This is a code duplication issue that violates the DRY principle. Consider extracting this function to a shared utility file (e.g., packages/app/src/app/utils/index.ts) to avoid maintaining identical logic in multiple places.
packages/app/src/app/app.tsx
Outdated
| try { | ||
| markSessionEndReason(sessionID, "interrupted"); | ||
| await c.session.abort({ sessionID }); | ||
| } catch (e) { | ||
| const message = e instanceof Error ? e.message : safeStringify(e); | ||
| setError(message); | ||
| } finally { | ||
| setBusy(false); | ||
| setBusyLabel(null); | ||
| setBusyStartedAt(null); |
Copilot
AI
Jan 25, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cancelRun function sets busy state to false in the finally block, but there's a potential race condition. If the abort() call fails or takes time, and the session.status event arrives before the finally block executes, the busy state might be inconsistent. The session.status event handling (which may arrive from the server after abort) might not properly synchronize with the local busy state. Consider whether the busy state should be cleared before calling abort(), or whether the busy state management should be entirely driven by server-side session.status events for consistency.
| try { | |
| markSessionEndReason(sessionID, "interrupted"); | |
| await c.session.abort({ sessionID }); | |
| } catch (e) { | |
| const message = e instanceof Error ? e.message : safeStringify(e); | |
| setError(message); | |
| } finally { | |
| setBusy(false); | |
| setBusyLabel(null); | |
| setBusyStartedAt(null); | |
| // Clear local busy state immediately on user cancel to avoid races with | |
| // server-driven status updates that may arrive while abort is in flight. | |
| setBusy(false); | |
| setBusyLabel(null); | |
| setBusyStartedAt(null); | |
| try { | |
| markSessionEndReason(sessionID, "interrupted"); | |
| await c.session.abort({ sessionID }); | |
| } catch (e) { | |
| const message = e instanceof Error ? e.message : safeStringify(e); | |
| setError(message); |
| <Show when={timing().reason && timing().reason !== "completed"}> | ||
| <span class="text-gray-9/80">· {formatReasonLabel(timing().reason!)}</span> |
Copilot
AI
Jan 25, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The non-null assertion operator (!) is used on timing().reason without verifying that the reason is defined within the nested Show condition. While the outer condition checks timing().reason && timing().reason !== "completed", TypeScript may not narrow the type in the nested scope due to how SolidJS reactivity works. The condition already ensures reason is truthy and not "completed", so the non-null assertion should be safe, but it would be clearer to extract the reason to a variable or use optional chaining (?.) for better type safety.
| <Show when={timing().reason && timing().reason !== "completed"}> | |
| <span class="text-gray-9/80">· {formatReasonLabel(timing().reason!)}</span> | |
| <Show | |
| when={timing().reason && timing().reason !== "completed" ? timing().reason : null} | |
| > | |
| {(reason) => ( | |
| <span class="text-gray-9/80"> | |
| · {formatReasonLabel(reason() as MessageEndReason)} | |
| </span> | |
| )} |
| {(resolved) => ( | ||
| <div class="flex items-center gap-1.5 opacity-60"> | ||
| <Clock size={12} /> | ||
| <span>{formatElapsedTime(resolved().duration)}</span> | ||
| <Show when={resolved().reason && resolved().reason !== "completed"}> | ||
| <span class="text-gray-9/80">· {formatReasonLabel(resolved().reason!)}</span> | ||
| </Show> | ||
| </div> | ||
| )} |
Copilot
AI
Jan 25, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same non-null assertion issue as line 346: timing().reason! is used without proper type narrowing. The condition checks timing().reason && timing().reason !== "completed", but TypeScript may not properly narrow the type within the nested Show scope due to SolidJS reactivity. Consider extracting to a variable or using optional chaining for better type safety.
| {(resolved) => ( | |
| <div class="flex items-center gap-1.5 opacity-60"> | |
| <Clock size={12} /> | |
| <span>{formatElapsedTime(resolved().duration)}</span> | |
| <Show when={resolved().reason && resolved().reason !== "completed"}> | |
| <span class="text-gray-9/80">· {formatReasonLabel(resolved().reason!)}</span> | |
| </Show> | |
| </div> | |
| )} | |
| {(resolved) => { | |
| const reason = resolved().reason; | |
| return ( | |
| <div class="flex items-center gap-1.5 opacity-60"> | |
| <Clock size={12} /> | |
| <span>{formatElapsedTime(resolved().duration)}</span> | |
| <Show when={reason && reason !== "completed"}> | |
| {(visibleReason) => ( | |
| <span class="text-gray-9/80">· {formatReasonLabel(visibleReason)}</span> | |
| )} | |
| </Show> | |
| </div> | |
| ); | |
| }} |
| const duration = end - start; | ||
| if (duration <= 0) return null; |
Copilot
AI
Jan 25, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The resolveTimingRange function rejects timing data when duration <= 0 (line 202), which is reasonable for filtering out invalid timings. However, this might hide legitimate edge cases where start and end timestamps are equal (duration = 0), such as very fast operations or timing resolution issues. Consider whether duration === 0 should be allowed and displayed as "0s" or "0ms" to provide more visibility into these edge cases, or document why such timings are intentionally filtered out.
| // Read file | ||
| if (["read", "Read"].includes(toolName)) { | ||
| detail = formatReadInfo(input); | ||
| } | ||
| // List directory | ||
| else if (["LS", "ls", "list", "List"].includes(toolName)) { | ||
| detail = formatListInfo(input); | ||
| } | ||
| // Search (grep/glob) | ||
| else if (["grep", "Grep", "glob", "Glob", "find", "Find"].includes(toolName)) { | ||
| detail = formatSearchInfo(toolName, input); | ||
| } | ||
| // Command/Execute | ||
| else if (["bash", "Bash", "Execute", "execute", "shell", "Shell"].includes(toolName)) { | ||
| detail = formatCommandInfo(input); | ||
| } | ||
| return { title }; | ||
| // Edit/Write/Create - show file path | ||
| else if (["edit", "Edit", "write", "Write", "Create", "create", "patch", "Patch", "multiedit", "MultiEdit"].includes(toolName)) { | ||
| const filePath = input.file_path ?? input.path; | ||
| if (typeof filePath === "string" && filePath.trim()) { | ||
| detail = shortenPath(filePath.trim()); | ||
| } | ||
| } | ||
| // Fetch/WebSearch - show URL or query | ||
| else if (["webfetch", "FetchUrl", "WebSearch"].includes(toolName)) { | ||
| const url = input.url; | ||
| const query = input.query; | ||
| if (typeof url === "string" && url.trim()) { | ||
| const u = url.trim(); | ||
| detail = u.length > 50 ? `${u.slice(0, 50)}…` : u; | ||
| } else if (typeof query === "string" && query.trim()) { | ||
| const q = query.trim(); | ||
| detail = q.length > 40 ? `"${q.slice(0, 40)}…"` : `"${q}"`; | ||
| } | ||
| } |
Copilot
AI
Jan 25, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tool name matching uses case-sensitive array lookups (e.g., ["read", "Read"].includes(toolName)) but also checks toolLower for GitHub detection. This creates inconsistency in how tool names are matched. Some tools might be missed if they use unexpected casing like "READ" or "Read" when the arrays only check for specific cases. Consider normalizing toolName to toolLower throughout the function and updating all the arrays to use lowercase strings for consistent case-insensitive matching, or use toolLower.includes() or a Map for lookups.
| const [prevMessageCount, setPrevMessageCount] = createSignal(0); | ||
| createEffect(() => { | ||
| props.messages.length; | ||
| props.todos.length; | ||
| messagesEndEl?.scrollIntoView({ behavior: "smooth" }); | ||
| const currentCount = props.messages.length; | ||
| const prev = prevMessageCount(); | ||
| if (currentCount > prev) { | ||
| messagesEndEl?.scrollIntoView({ behavior: "smooth" }); | ||
| } | ||
| setPrevMessageCount(currentCount); | ||
| }); |
Copilot
AI
Jan 25, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The scroll behavior fix that tracks prevMessageCount only scrolls when the message count increases. However, this may not prevent scroll jumps when toggling "View steps" as described in the PR description. When steps are expanded/collapsed, the message count doesn't change, but the DOM height changes, which could still cause scroll position shifts. The original code tracked both messages.length and todos.length. Consider also tracking expandedStepIds changes or using a more robust scroll position preservation technique when DOM content changes without new messages being added.
|
|
||
| const copyButton = ( | ||
| <button | ||
| class="text-gray-9 hover:text-gray-11 p-1 rounded hover:bg-black/5 dark:hover:bg-white/10 transition-colors opacity-0 group-hover:opacity-100 transition-opacity" |
Copilot
AI
Jan 25, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The class string on line 384 contains "transition-opacity" twice. This is redundant and should be simplified to include it only once. The correct class string should be: "text-gray-9 hover:text-gray-11 p-1 rounded hover:bg-black/5 dark:hover:bg-white/10 transition-colors opacity-0 group-hover:opacity-100 transition-opacity"
- Show tool operations in single line: status icon + tool name + key param - Add TOOL_LABELS mapping for friendly tool names (e.g., 'Fetch' instead of 'webfetch') - Extract key parameters (URL, file path, command) for each tool type - Color-coded status: green (completed), red (error), blue pulse (running) - Hide verbose output/details, todowrite shows only in sidebar Progress Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Only scroll to bottom when messages count actually increases, not on every re-render or state change. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
- List (LS): Show only last 3 path segments instead of full path - Read: Show file path + line range info (e.g., 'from L100, 50 lines') - Grep/Glob: Show pattern in quotes + search path context - Execute: Show first line of command, truncated - Edit/Write/Create: Show shortened file path - GitHub icon: Display for git/gh commands (MCP or CLI) - Add StepSummary type with icon field for extensibility Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Only show View steps when there are actual tool or reasoning parts, ignore step-start/step-finish metadata markers that have no content. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Remove w-full from user bubbles so they shrink to fit content, assistant messages keep w-full for consistent width. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
- px-6 py-4 → px-5 py-3 (smaller padding) - rounded-[24px] → rounded-[20px] (proportional) - leading-relaxed → leading-normal (tighter line height) Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
- User messages: copy button on left side of bubble - Assistant messages: copy button inside at bottom right - Both show on hover only Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
- Running time: show as seconds (e.g., '30s'), minutes ('2m 15s'), hours ('1h 30m')
- Completed messages: show total duration with clock icon
- Remove milliseconds display for cleaner look
Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Calculate total duration from first assistant message creation to last assistant message completion, display only once at the end. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
- Show send button (arrow) when idle - Show stop button (square) when busy/running - Stop button calls session.abort() to cancel the run - Red color for stop button to indicate destructive action Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Change opacity from 0 to 30% so button is always visible. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
Each assistant message now shows its own duration (created to completed), not the total time from first to last message. Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
- Add MessageEndReason type (completed/interrupted/terminated/error) - Add MessageTiming type with startAt/lastTokenAt/endAt/endReason - Track timing from part updates for more accurate duration - Show end reason labels for non-completed messages - Support cluster timing aggregation for grouped steps - Mark session end reason on abort for proper interruption tracking Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com>
a2b57fd to
9f6d0e3
Compare


Summary
Simplify the message steps display in OpenWork to show only essential information, making the UI cleaner and easier to scan. Also adds a send/stop button and enhanced timing tracking.
Changes
Message Steps Simplification
UI Polish
Send/Stop Button
session.abort()to cancel the runTime Display
Commits
cbe7a75 feat: enhance message timing system with end reasons
47a05d2 fix: show duration per message instead of total session time
e282c08 fix: make send button visible when disabled
8772174 feat: add send/stop button in composer
b47468a fix: show duration only on last assistant message
7cf72da feat: improve time display format
a97c66a fix: move copy button outside user bubble
40e6a31 fix: reduce user bubble padding for more compact look
bd4edab fix: user message bubble auto-width based on content
a5ffd75 fix: hide empty steps (step-start/step-finish markers)
a3c1710 feat: enhance tool step display with smarter formatting
bee3d7d fix: prevent scroll jump when toggling View steps
2768da9 feat: simplify message steps display