Skip to content

feat: integrate update branch API and modify UI#1833

Merged
benjamin-747 merged 1 commit into
gitmono-dev:mainfrom
zyd123-cmd:main
Jan 23, 2026
Merged

feat: integrate update branch API and modify UI#1833
benjamin-747 merged 1 commit into
gitmono-dev:mainfrom
zyd123-cmd:main

Conversation

@zyd123-cmd

Copy link
Copy Markdown
Contributor

1对接了 update branch的接口 2 code部分在目录页面也可拖动调节宽度

@zyd123-cmd zyd123-cmd force-pushed the main branch 3 times, most recently from 938e40b to 02c4575 Compare January 22, 2026 14:58
@genedna

genedna commented Jan 22, 2026

Copy link
Copy Markdown
Collaborator

@claude

@github-actions

github-actions Bot commented Jan 22, 2026

Copy link
Copy Markdown

Claude finished @genedna's task in 3m 22s —— View job


Code Review Complete ✅

I've conducted a comprehensive review of this PR which integrates the update branch API and adds resizable code panels. Here are my findings:


Summary

This PR successfully implements two main features:

  1. Update Branch Integration: Adds API integration to check if a CL branch is outdated and provides an "Update branch" button
  2. Resizable Code Panel: Makes the code directory panel resizable via drag-and-drop

Code Quality & Best Practices

✅ Strengths

  1. Good separation of concerns: The new hooks (useGetClUpdateStatus, usePostClUpdateBranch) follow the existing pattern in the codebase
  2. Comprehensive documentation: Both hooks have excellent JSDoc comments explaining their purpose and business logic
  3. Proper error handling: Toast notifications for success/error cases in usePostClUpdateBranch
  4. Clean component decomposition: ChecksSection.tsx properly breaks down into smaller components (CheckStatus, CheckGroup, CheckList)
  5. Consistent code style: Follows React best practices with proper use of hooks, memoization, and TypeScript typing

⚠️ Issues & Suggestions

1. Missing Error Handling in Update Status Hook (See inline comment)

moon/apps/web/hooks/CL/useGetClUpdateStatus.ts:18

The hook doesn't handle errors. Consider adding an onError callback.

2. Potential Performance Issue with Polling (See inline comment)

moon/apps/web/components/ClBox/ChecksSection.tsx:195

The component polls the update status every 30 seconds unconditionally. This could cause unnecessary API calls. Consider only polling when the section is open or checks are in progress.

3. Accessibility: Missing Button Label (See inline comment)

moon/apps/web/components/ClBox/ChecksSection.tsx:101

The toggle button lacks an accessible label for screen readers.

4. Inline Styles Should Use Tailwind (See inline comment)

moon/apps/web/components/ClBox/ChecksSection.tsx:88

The component uses inline backgroundColor: '#f6f8fa' which doesn't match the Tailwind approach used elsewhere.

5. Resizer Handle Lacks Visual Affordance (See inline comment)

moon/apps/web/pages/[org]/code/tree/[version]/[...path]/index.tsx:164-168

The resizer is only 1px wide and might be hard to discover for users. Consider increasing width, adding a visible grip icon, and adding keyboard support for accessibility.


Security

✅ No Critical Issues Found

  • Input sanitization: Not applicable (no user input directly processed)
  • API calls: Properly typed and using established API client patterns
  • XSS: No dangerouslySetInnerHTML or unsafe rendering
  • Authentication: Relies on existing auth context (appropriate)

Performance

Observations

  1. Good: Proper use of useMemo for expensive calculations (summary, groupedChecks)
  2. Good: useCallback for event handlers to prevent re-renders
  3. Concern: Polling every 30s could add up with multiple open PRs (see inline comment)
  4. Good: Resizer's mouse move listener is properly cleaned up ✅
  5. Good: The resizer implementation prevents memory leaks with proper cleanup in useEffect

Suggestions

  • Consider debouncing the resize operation if performance issues arise
  • The 30-second staleTime in useGetClUpdateStatus is reasonable but ensure it aligns with backend cache strategy

Testing

Critical Gap: No Tests

Neither of the new hooks has test coverage:

  • moon/apps/web/hooks/CL/useGetClUpdateStatus.ts - No test file
  • moon/apps/web/hooks/CL/usePostClUpdateBranch.ts - No test file

Recommended test coverage:

  1. useGetClUpdateStatus:

    • Should fetch update status with correct parameters
    • Should respect enabled flag
    • Should handle polling interval
    • Should handle errors gracefully
  2. usePostClUpdateBranch:

    • Should call mutation with correct link
    • Should show success toast on success
    • Should show error toast on failure
    • Should invalidate correct query caches
    • Should handle backend errors (req_result: false)
  3. ChecksSection:

    • Should render update button only when outdated
    • Should disable button while updating
    • Should call updateBranch on button click
    • Should stop propagation to prevent collapsible toggle
  4. Resizable Panel:

    • Should respect min/max width constraints
    • Should update width on drag
    • Should clean up event listeners on unmount

Documentation

✅ Good Documentation

  • Both hooks have excellent JSDoc comments explaining purpose, parameters, and business logic
  • Inline comments explain complex logic (e.g., resizer constraints)

Suggestions

  1. Consider adding a comment explaining why polling interval is 30000ms specifically
  2. Document the resizer constraints (MIN_LEFT_WIDTH, MAX_LEFT_WIDTH_PERCENT) with rationale

Additional Observations

Type Safety

  • Excellent use of TypeScript throughout
  • Proper typing for all props and state
  • Generated types properly integrated

Code Organization

  • New hooks follow existing patterns
  • Component decomposition is clean and logical

User Experience ⚠️

  1. Good: Loading spinner while updating branch
  2. Good: Disabled state prevents double-clicks
  3. Improvement: Consider adding success/error inline feedback in addition to toasts
  4. Improvement: The resizer could be more discoverable with better visual affordance

Summary of Recommendations

High Priority 🔴:

  1. Add test coverage for new hooks and components
  2. Optimize polling to reduce unnecessary API calls

Medium Priority 🟡:
3. Add error handling to useGetClUpdateStatus
4. Improve resizer visual affordance and accessibility
5. Add accessibility label to expand/collapse button

Low Priority 🟢:
6. Replace inline style with Tailwind class
7. Add comments for magic numbers (30000ms, width constraints)


Conclusion

Overall, this is a solid PR with well-structured code that follows existing patterns. The implementation is clean and the functionality is thoughtfully designed.

Main concern: The lack of test coverage should be addressed before merging to ensure reliability and maintainability.

I've left 6 inline comments on specific files with detailed suggestions. Please review them and let me know if you have any questions!


export function ChecksSection({ checks, onStatusChange, additionalChecks }: ChecksSectionProps) {
export function ChecksSection({ checks, onStatusChange, additionalChecks, clLink }: ChecksSectionProps) {
// Get update status
const { data: updateStatus } = useGetClUpdateStatus(clLink, true, 30000)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performance Concern: This polls the update status every 30 seconds unconditionally, even when the section is collapsed or there are no pending checks.

Consider optimizing by only polling when relevant:

const shouldPoll = open || inProgressCount > 0
const { data: updateStatus } = useGetClUpdateStatus(clLink, shouldPoll, 30000)

This reduces unnecessary API calls when users aren't actively viewing this section.

)}
</button>
)}
<button>{open ? <ArrowUpIcon /> : <ArrowDownIcon />}</button>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accessibility: This button lacks an accessible label for screen readers.

Add an aria-label:

<button ? "Collapse checks" : "Expand checks"}>
  {open ? <ArrowUpIcon /> : <ArrowDownIcon />}
</button>

onUpdateBranch()
}}
disabled={isUpdating}
style={{ backgroundColor: '#f6f8fa' }}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Style: Inline style backgroundColor: '#f6f8fa' doesn't match the Tailwind approach used elsewhere in the component.

Consider using Tailwind's utility class instead:

className='... bg-gray-50 hover:bg-gray-100 ...'

Remove the style prop for consistency.

export const useGetClUpdateStatus = (link: string, enabled: boolean = true, refetchInterval?: number) => {
const query = legacyApiClient.v1.getApiClUpdateStatus()

return useQuery<GetApiClUpdateStatusData>({

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing Error Handling: This query doesn't handle errors, which could lead to silent failures.

Consider adding error handling:

return useQuery<GetApiClUpdateStatusData>({
  queryKey: query.requestKey(link),
  queryFn: () => query.request(link),
  enabled: enabled && !!link,
  refetchInterval,
  staleTime: 30000,
  onError: (error) => {
    console.error('Failed to fetch CL update status:', error)
    // Optionally show a toast notification
  }
})

</div>

{/* Resizer handle */}
<div

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UX & Accessibility Improvement: The resizer is only 1px wide (w-1), making it difficult to discover and click, especially on touch devices.

Suggestions:

  1. Increase width to at least 4-6px for better clickability
  2. Add visual affordance like a grip icon or pattern
  3. Add ARIA attributes for screen readers
  4. Consider keyboard support for accessibility

Example improvement:

<div
  onMouseDown={handleMouseDown}
  className='h-full w-1.5 flex-shrink-0 cursor-col-resize bg-gray-300 transition-colors hover:bg-blue-400 flex items-center justify-center'
  style={{ backgroundColor: isDragging ? '#60a5fa' : undefined }}
  role="separator"
  aria-orientation="vertical"
 
  tabIndex={0}
>
  <div className="w-0.5 h-8 bg-gray-500 rounded opacity-60" />
</div>


return mutation.request(link)
},
onSuccess: (data, link) => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good Error Handling: Nice work checking req_result and displaying appropriate error messages!

One minor suggestion: Consider logging the error details for debugging:

onSuccess: (data, link) => {
  if (data.req_result) {
    toast.success('Branch updated successfully')
  } else {
    console.error('Branch update failed:', data.err_message)
    toast.error(data.err_message || 'Failed to update branch')
  }
  // ... rest of the code
}

This helps with troubleshooting production issues.

@benjamin-747 benjamin-747 added this pull request to the merge queue Jan 23, 2026
Merged via the queue into gitmono-dev:main with commit ff49ba8 Jan 23, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants