Automatically refresh Artifacts panel #468
Conversation
This comment has been minimized.
This comment has been minimized.
- Introduced a helper function to identify completed tool use messages, improving clarity and maintainability. - Added a timeout reference for handling session completion, ensuring artifacts are refreshed after a delay. - Implemented cleanup logic for the completion timeout to prevent memory leaks on component unmount or phase changes. - Updated dependencies in useEffect hooks for better performance and adherence to React best practices.
- Improved ProjectSessionDetailPage by implementing a debounced refresh mechanism for artifacts, ensuring users see updates immediately without manual refresh. - Refactored completion handling logic to enhance performance and maintainability. - Updated ArtifactsAccordion to include an accessible aria-label for file count, improving usability for screen readers.
- Added "peer": true to multiple dependencies in package-lock.json, indicating that these packages are peer dependencies. This change enhances compatibility and ensures proper dependency resolution in the project.
Claude Code ReviewSummaryThis PR implements automatic artifact refreshing in the frontend, improving UX by eliminating the need for manual refresh clicks. The implementation includes a file count badge in the accordion header and uses debounced refreshes triggered by completed tool use messages and session completion. Issues by Severity🚫 Blocker IssuesNone 🔴 Critical Issues1. useEffect Dependency Issue - Missing
2. Constants in Dependency Arrays (page.tsx:562, 587)
// Outside component (preferred)
const ARTIFACTS_DEBOUNCE_MS = 1000;
const COMPLETION_DELAY_MS = 2000;
// Or inside useEffect if needed
useEffect(() => {
const ARTIFACTS_DEBOUNCE_MS = 1000;
// ...
}, [completedToolCount]); // Remove constant from deps🟡 Major Issues3. Type Guard Function in useMemo Dependencies (page.tsx:534-536)
const completedToolCount = useMemo(() => {
return streamMessages.filter(msg =>
msg.type === 'tool_use_messages' &&
'resultBlock' in msg &&
msg.resultBlock?.content !== null
).length;
}, [streamMessages]);4. Ref Cleanup Logic Issue (page.tsx:575-577)
if (phase === 'Completed' && !hasRefreshedOnCompletionRef.current) {
// ... refresh logic
hasRefreshedOnCompletionRef.current = true;
} else if (phase !== 'Completed') {
// Reset flag whenever leaving Completed state
hasRefreshedOnCompletionRef.current = false;
}🔵 Minor Issues5. FileCount Computation on Every Render (artifacts-accordion.tsx:38)
const fileCount = useMemo(() => files.filter(f => !f.isDir).length, [files]);6. Badge Accessibility Could Be Enhanced (artifacts-accordion.tsx:47-54)
<Badge
variant="secondary"
className="ml-auto mr-2"
aria-label={`${fileCount} ${fileCount === 1 ? 'file' : 'files'}`}
aria-live="polite"
>
{fileCount}
</Badge>7. package-lock.json Peer Dependency Changes
Positive Highlights✅ Excellent UX Improvement: Auto-refresh eliminates friction and improves discoverability of artifacts ✅ Smart Debouncing: The 1s debounce for tool completions prevents API spam during rapid operations ✅ Good Documentation: Inline comments explain WHY decisions were made (debounce delays, completion delays) ✅ Type Safety: Proper type guard with ✅ Proper Cleanup: Both timeout refs have cleanup logic in useEffect returns ✅ Accessibility: Badge includes aria-label for screen readers ✅ Consistent with Patterns: Uses React Query's refetch pattern, follows Shadcn UI component usage RecommendationsPriority 1 (Before Merge)
Priority 2 (Nice to Have)
Priority 3 (Future Consideration)
VerdictRecommend: Merge after addressing Priority 1 items This is a valuable UX improvement with solid implementation. The critical issues are React best practices violations that should be fixed before merge to prevent potential bugs and maintain code quality standards. The fixes are straightforward and low-risk. Generated with Claude Code 🔍 View AI decision process (logs available for 90 days) 📋 View memory system files loaded (click to expand)What Amber Loaded for Code ReviewAmber automatically loaded these repository standards from the memory system:
Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines. |
… performance - Moved artifact auto-refresh debounce constants outside the component to prevent unnecessary re-renders. - Inlined the type guard for completed tool use messages to simplify the code and improve performance. - Updated useEffect dependencies to ensure proper behavior during session phase changes. - Memoized file count calculation in ArtifactsAccordion to optimize rendering and added an accessible aria-live attribute for better screen reader support.
Claude Code ReviewSummaryThis PR implements automatic artifact panel refreshing triggered by tool completion and session completion events. The implementation demonstrates solid UX thinking with debouncing and proper cleanup patterns. Overall the code quality is good, with memoization and accessibility improvements. However, there are several critical issues related to React best practices, type safety, and one performance concern that should be addressed before merge. Issues by Severity🚫 Blocker IssuesNone - No blocking issues that prevent merge. 🔴 Critical Issues
🟡 Major Issues
🔵 Minor Issues
Positive Highlights✅ Excellent UX improvement - Auto-refreshing artifacts is a significant user experience win RecommendationsMust Fix (Before Merge)
Should Fix (High Priority)
Nice to Have (Low Priority)
Testing Recommendations
Overall Assessment: This is good work that delivers real user value. The critical issues are fixable with small changes. Once the Recommendation: 🟡 Approve with changes - Address critical issues #1-3, explain package-lock changes, then merge. 🔍 View AI decision process (logs available for 90 days) 📋 View memory system files loaded (click to expand)What Amber Loaded for Code ReviewAmber automatically loaded these repository standards from the memory system:
Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines. |
…clarity and performance - Added useCallback to stabilize the refetch function for artifacts, preventing stale closures. - Extracted type guard for completed tool use messages to improve testability and validation. - Updated ArtifactsAccordion to wrap file count in a span with aria-live for better accessibility. - Refactored completed tool count calculation to utilize the new type guard for clarity.
Claude Code ReviewSummaryThis PR adds automatic artifact refresh functionality triggered by tool completions and session completion, plus a badge showing the artifact count. The implementation is generally well-structured with good attention to performance (memoization, debouncing) and proper cleanup. However, there are several code quality, type safety, and architectural issues that should be addressed. Issues by Severity🚫 Blocker IssuesNone - No blocking issues that prevent merge, but see Critical Issues below. 🔴 Critical Issues1. Type Safety Violation - Unsafe Type Assertion (page.tsx:109-134)Location: The type guard function isCompletedToolUseMessage(msg: MessageObject | ToolUseMessages): msg is ToolUseMessages {
if (msg.type !== "tool_use_messages") {
return false;
}
const toolMsg = msg as ToolUseMessages; // ⚠️ UNSAFE ASSERTION
if (!("resultBlock" in toolMsg)) {
return false;
}
if (!toolMsg.resultBlock || typeof toolMsg.resultBlock !== "object") {
return false;
}
return toolMsg.resultBlock.content !== null;
}Problem: After checking Impact: If a malformed message arrives with Fix: Remove the type assertion and use proper type narrowing: function isCompletedToolUseMessage(msg: MessageObject | ToolUseMessages): msg is ToolUseMessages {
if (msg.type !== "tool_use_messages") {
return false;
}
// Type narrowing without assertion
if (!("resultBlock" in msg)) {
return false;
}
const toolMsg = msg; // Now properly narrowed
if (!toolMsg.resultBlock || typeof toolMsg.resultBlock !== "object") {
return false;
}
return toolMsg.resultBlock.content !== null;
}Violates: CLAUDE.md frontend rule #1 - "Zero 2. Accessibility Issue - Dynamic Content Announcement (artifacts-accordion.tsx:53-56)Location: <span aria-live="polite" aria-atomic="true">
{fileCount}
</span>Problem: The
Impact: Screen reader users may not be notified when artifacts are added. Fix: Move <div className="flex items-center gap-2 w-full" aria-live="polite" aria-atomic="false">
<NotepadText className="h-4 w-4" />
<span>Artifacts</span>
{fileCount > 0 && (
<Badge
variant="secondary"
className="ml-auto mr-2"
aria-label={\`\${fileCount} \${fileCount === 1 ? 'file' : 'files'}\`}
>
{fileCount}
</Badge>
)}
</div>🟡 Major Issues3. Unnecessary
|
… clarity and performance - Removed unnecessary useCallback for refetch function in ProjectSessionDetailPage to simplify code. - Enhanced type guard for completed tool use messages to improve type safety and validation. - Updated artifact auto-refresh constants with clearer comments for better understanding of their purpose. - Improved accessibility in ArtifactsAccordion by adding aria-live attributes for dynamic content updates.
Claude Code ReviewSummaryThis PR implements automatic refresh of the Artifacts panel after tool completions and session completion, along with a file count badge. The implementation demonstrates good attention to UX, performance optimization through debouncing, and thoughtful cleanup patterns. However, there are several issues that need to be addressed before merging. Issues by Severity🚫 Blocker Issues1. Both Fix: Wrap // If refetchArtifactsFiles is NOT from React Query:
const refetchArtifactsFiles = useCallback(() => {
// your refetch logic
}, []); // Add proper dependencies2. package-lock.json Changes Unrelated to PR (package-lock.json) The
Fix: Separate package-lock.json changes into their own PR or document why they're necessary for this feature. 🔴 Critical Issues3. Type Guard Unsafe Type Narrowing (page.tsx:127-143) The // Type narrowing without assertion - check resultBlock exists before accessing
if (!("resultBlock" in msg)) {
return false;
}
// Now TypeScript knows msg has resultBlock property
if (!msg.resultBlock || typeof msg.resultBlock !== "object") {
return false;
}Issue: The Fix: Use proper type assertion or ensure the type definition is correct: function isCompletedToolUseMessage(msg: MessageObject | ToolUseMessages): msg is ToolUseMessages {
if (msg.type !== "tool_use_messages") {
return false;
}
const toolMsg = msg as ToolUseMessages;
return (
toolMsg.resultBlock !== undefined &&
toolMsg.resultBlock !== null &&
typeof toolMsg.resultBlock === "object" &&
toolMsg.resultBlock.content !== null
);
}🟡 Major Issues4. Accessibility: Incorrect aria-live Usage (artifacts-accordion.tsx:44) <div className="flex items-center gap-2 w-full" aria-live="polite" aria-atomic="false">Issues:
Fix: <div className="flex items-center gap-2 w-full">
<NotepadText className="h-4 w-4" />
<span>Artifacts</span>
{fileCount > 0 && (
<Badge
variant="secondary"
className="ml-auto mr-2"
aria-live="polite"
aria-atomic="true"
>
<span className="sr-only">{fileCount} {fileCount === 1 ? 'file' : 'files'}</span>
{fileCount}
</Badge>
)}
</div>5. Missing Error Handling for refetchArtifactsFiles (page.tsx:579, 600) The Fix: artifactsRefreshTimeoutRef.current = setTimeout(async () => {
try {
await refetchArtifactsFiles();
} catch (error) {
console.error('Failed to refresh artifacts:', error);
// Consider showing a subtle toast notification
}
}, ARTIFACTS_DEBOUNCE_MS);6. Race Condition on Rapid Phase Changes (page.tsx:594-614) If the session phase rapidly toggles between "Completed" and another state, the flag Fix: Clear the completion timeout when the flag is reset: } else if (phase !== "Completed") {
// Clear any pending completion refresh
if (completionTimeoutRef.current) {
clearTimeout(completionTimeoutRef.current);
completionTimeoutRef.current = null;
}
hasRefreshedOnCompletionRef.current = false;
}🔵 Minor Issues7. Magic Number for Initial Count Check (page.tsx:566) if (previousToolResultCount.current === 0 && completedToolCount > 0) {Issue: Using Fix: Use a boolean flag for clarity: const isInitialized = useRef(false);
useEffect(() => {
if (!isInitialized.current && completedToolCount > 0) {
isInitialized.current = true;
previousToolResultCount.current = completedToolCount;
return;
}
// ...
}, [completedToolCount, refetchArtifactsFiles]);8. Missing Type for WorkspaceFile (artifacts-accordion.tsx:10-15) The Fix: Import the type from a shared location or convert to a proper shared type definition. 9. Component Length Approaching 200 Lines The page.tsx file continues to grow with each feature. While the current additions are well-organized, the component is approaching the recommended 200-line limit per the frontend guidelines. Recommendation: Consider extracting the artifact auto-refresh logic into a custom hook: // hooks/use-artifact-auto-refresh.ts
export function useArtifactAutoRefresh(
streamMessages: Messages[],
sessionPhase: string | undefined,
refetchArtifactsFiles: () => void
) {
// Move all the auto-refresh logic here
}Positive Highlights✅ Excellent debouncing strategy - The 1000ms debounce prevents API rate limits while maintaining responsive UX RecommendationsPriority 1 (Before Merge)
Priority 2 (Follow-up PR)
Priority 3 (Nice to Have)
Overall Assessment: This is a well-thought-out UX improvement with good attention to performance and cleanup. The implementation demonstrates solid React patterns but has a few critical issues around type safety, accessibility, and dependency management that must be addressed before merging. 🔍 View AI decision process (logs available for 90 days) 📋 View memory system files loaded (click to expand)What Amber Loaded for Code ReviewAmber automatically loaded these repository standards from the memory system:
Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines. |
…functionality and accessibility - Added useCallback to stabilize the refetch function for artifacts, preventing unnecessary re-renders. - Improved type guard for completed tool use messages with clearer type assertions and validation. - Updated ArtifactsAccordion to enhance accessibility by refining aria-live attributes for dynamic content updates. - Implemented cleanup logic for completion handling to avoid race conditions during state transitions.
… streamline dependency management and improve compatibility.
Claude Code ReviewSummaryThis PR adds automatic refresh functionality to the Artifacts panel, eliminating the need for users to manually click the refresh button. It introduces a file count badge and implements debounced auto-refresh logic that triggers after tool completions and session completion. The implementation follows React best practices with proper cleanup, memoization, and accessibility features. Issues by Severity🚫 Blocker IssuesNone - the code is safe to merge. 🔴 Critical Issues1. Type Safety Issue in isCompletedToolUseMessage (page.tsx:127-141)
if (msg.type !== "tool_use_messages") {
return false;
}
const toolMsg = msg as ToolUseMessages; // Unnecessary cast
function isCompletedToolUseMessage(msg: MessageObject | ToolUseMessages): msg is ToolUseMessages {
return (
msg.type === "tool_use_messages" &&
msg.resultBlock !== undefined &&
msg.resultBlock !== null &&
typeof msg.resultBlock === "object" &&
msg.resultBlock.content !== null
);
}
2. Potential Race Condition with useCallback Dependencies (page.tsx:439)
🟡 Major Issues3. Silent Error Handling (page.tsx:442)
catch (error) {
console.error('Failed to refresh artifacts:', error);
// Silent fail - don't interrupt user experience
}
4. Missing Error Boundary Protection
5. No Loading State During Auto-Refresh
🔵 Minor Issues6. Magic Numbers for Timing Constants
7. Accessibility: Badge Screen Reader Text
<span className="sr-only">{fileCount} {fileCount === 1 ? 'file' : 'files'}</span>
{fileCount}
<span className="sr-only">{fileCount} {fileCount === 1 ? 'file' : 'files'} in artifacts</span>
<span aria-hidden="true">{fileCount}</span>8. Memoization Overkill
9. Ref Naming Inconsistency
10. Missing JSDoc for Exported Component
Positive Highlights✅ Excellent Documentation: Constants have thorough comments explaining timing choices, including testing notes ("500ms was too aggressive"). ✅ Proper Cleanup: All timeouts are properly cleaned up in useEffect return functions, preventing memory leaks. ✅ Accessibility First: Good use of ✅ Smart Debouncing: The debounce logic intelligently handles burst writes, preventing API rate limit issues. ✅ Memoization: Proper use of ✅ Type Guards: Extracted ✅ Follows Project Patterns: Adheres to React Query patterns from ✅ No ✅ Component Colocation: Changes are properly localized to the session detail page and accordion component. RecommendationsPriority 1 (Fix Before Merge)
Priority 2 (Should Fix)
Priority 3 (Nice to Have)
Final AssessmentOverall Grade: B+ (Good with minor improvements needed) This PR delivers a valuable UX improvement with solid implementation. The code follows project patterns, has proper cleanup, and good accessibility. The main concerns are:
Once the Priority 1 items are addressed, this is ready to merge. The Priority 2 and 3 items can be follow-up improvements. Recommendation: Request changes for Priority 1 items, then approve. 🔍 View AI decision process (logs available for 90 days) 📋 View memory system files loaded (click to expand)What Amber Loaded for Code ReviewAmber automatically loaded these repository standards from the memory system:
Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines. |
|
id like this to eventually be replaced by some kind of streaming / shared state. but for now looks good |
2 participants
This PR triggers an automatic refresh of the Artifacts panel after every completed tool call message and completed session so that users don't need to click the Refresh button manually.
It also adds a badge to the panel's header whenever artifacts exist, which should make it easier to spot that artifacts have been created when the panel is collapsed (like it is by default).
cc @Daniel-Warner-X
Demo
2025-12-16.12.27.54.mp4