feat: add refresh ability to GitHub RHS sidebar#1026
Conversation
- Add refresh button in RHS header with spinner animation - Auto-refresh data when RHS is opened - Wire getSidebarContent action to SidebarRight component Resolves mattermost#131
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a manual refresh capability to the GitHub plugin's right-hand sidebar. Sidebar Right Refresh Feature
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
|
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
|
|
||
| componentDidMount() { | ||
| // Auto-refresh on open to guarantee latest data (issue #131) | ||
| this.handleRefresh(); |
There was a problem hiding this comment.
The auto refresh on mount is redundant because getSidebarContent() is already dispatched from sidebar_buttons.jsx:42 and websocket/index.js:74,84. By the time the RHS opens the data is fresh.
This adds an extra GitHub search call every time the user opens the RHS, which is expensive. I'd suggest removing the auto refresh entirely and relying on the manual button and existing WS-driven updates. If you want to keep it, please mirror the E2E guard from sidebar_buttons.jsx.
| this.state = {refreshing: false}; | ||
| } | ||
|
|
||
| handleRefresh = async (e) => { |
There was a problem hiding this comment.
I see a couple of issues here. setState is async so two fast clicks can both pass the this.state.refreshing check before either update lands. Use an instance flag for the gate. We also need unmount safety.
Suggested rewrite:
componentDidMount() {
this._mounted = true;
// ...
}
componentWillUnmount() {
this._mounted = false;
}
handleRefresh = async (e) => {
e?.preventDefault();
if (this._refreshing) {
return;
}
this._refreshing = true;
this.setState({refreshing: true});
try {
await this.props.actions.getSidebarContent();
} finally {
this._refreshing = false;
if (this._mounted) {
this.setState({refreshing: false});
}
}
};
| placement='left' | ||
| overlay={<Tooltip id='rhsRefreshTooltip'>{'Refresh'}</Tooltip>} | ||
| > | ||
| <a |
There was a problem hiding this comment.
| alignItems: 'center', | ||
| }, | ||
| refreshButton: { | ||
| color: 'rgba(0, 0, 0, 0.4)', |
There was a problem hiding this comment.
Hard-coded color will be invisible on dark themes. this.props.theme is already available on the component please derive the color the same way sidebar_buttons.jsx does. Please convert the bottom-of-file style object to a memoized factory:
import {makeStyleFromTheme, changeOpacity} from 'mattermost-redux/utils/theme_utils';
const getStyle = makeStyleFromTheme((theme) => ({
sectionHeader: {
padding: '15px',
display: 'flex',
justifyContent: 'space-between',
alignItems: 'center',
},
refreshButton: {
color: changeOpacity(theme.centerChannelColor, 0.6),
cursor: 'pointer',
background: 'transparent',
border: 'none',
padding: 0,
},
}));
| const style = { | ||
| sectionHeader: { | ||
| padding: '15px', | ||
| display: 'flex', |
There was a problem hiding this comment.
This switches every RHS state's header to flex layout not just the ones with the refresh button. Could you attach screenshots of all four states (PRS, REVIEWS, UNREADS, ASSIGNMENTS) on both light and dark themes? Want to make sure the title alignment doesn't regress anywhere.
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
3 participants
Summary
Adds the ability to refresh data in the GitHub plugin RHS sidebar, as requested in #131.
Two improvements:
getSidebarContentis called automatically to guarantee the latest data is available.getSidebarContentwith a spinner animation while loading.Resolves #131
Changes
sidebar_right/index.jsx: WiregetSidebarContentaction toSidebarRightsidebar_right/sidebar_right.jsx: Add refresh state, handler, auto-refresh on mount, and refresh button in headerRelease Notes
Change Impact: 🟡 Medium
Reasoning: The update is localized to the GitHub plugin sidebar, but it changes sidebar loading behavior by triggering refresh on open and adding a new manual refresh action with async state handling. That creates some regression risk around rendering, loading indicators, and repeated fetch behavior, though it does not affect shared auth, persistence, or core application flows.
Regression Risk: Moderate. The affected path is user-facing and includes new lifecycle-driven network requests plus UI state transitions, so edge cases like repeated opens, concurrent refresh clicks, or loading state cleanup should be validated.
QA Recommendation: Perform focused manual QA on the sidebar open flow and refresh button behavior, including spinner display, tooltip, repeated clicks, and ensuring existing sidebar content still loads correctly. Skipping manual QA is not recommended.
Generated by CodeRabbitAI