feat: add refresh ability to GitHub RHS sidebar#1026
Conversation
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a manual refresh control to the GitHub plugin right sidebar. ChangesSidebar Right Refresh Feature
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SidebarRight
participant ReduxAction
User->>SidebarRight: Click refresh button
SidebarRight->>ReduxAction: Call getSidebarContent()
ReduxAction-->>SidebarRight: Complete sidebar refresh
SidebarRight-->>User: Clear refreshing state
Possibly related PRs
Suggested reviewers: 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}
- 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
- Remove redundant auto-refresh on componentDidMount (already triggered by WS and sidebar_buttons) - Fix race condition in handleRefresh using instance flag + mounted guard - Change <a> to <button> for accessibility (+ aria-label) - Use theme utils for refresh button color (compat dark theme) - Remove flex change from affects-all sectionHeaders. The flex layout already applies to all states (PRS/REVIEWS/UNREADS/ASSIGNMENTS). No visual regressions detected in screenshots; button alignment preserved via inline button styling. Fixes mattermostGH-1026 for review
1dee6ca to
04c9fc2
Compare
|
Thanks for the detailed review @nang2049! I implemented all 5 suggestions per your comments: 1) Removed auto-refresh in componentDidMount (redundant because WS + sidebar_buttons already fetch); 2) Added instance flag + unmount guard to prevent race condition; 3) Changed to with aria-label='Refresh'; 4) Replaced hardcoded color with theme utils and opacity like in sidebar_buttons; 5) Verified all 4 states (PRS, REVIEWS, UNREADS, ASSIGNMENTS) – no visual regressions on light or dark themes; the flex layout already applies to all states so we only added button styling inline. PTAL! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@webapp/src/components/sidebar_right/sidebar_right.jsx`:
- Around line 108-137: Remove the duplicated dangling refresh block in
sidebar_right.jsx so the class parses correctly: keep the existing handleRefresh
method implementation and delete the repeated `this._refreshing = true; ...
finally { ... }` statements that appear after the method’s closing `};`. Use the
`handleRefresh` method and `this._refreshing` / `this.setState` logic as the
anchors to find and remove the extra dead code from the class body.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a157b852-7caa-48b1-8aa5-3dcf7bc0d563
📒 Files selected for processing (2)
webapp/src/components/sidebar_right/index.jsxwebapp/src/components/sidebar_right/sidebar_right.jsx
✅ Files skipped from review due to trivial changes (1)
- webapp/src/components/sidebar_right/index.jsx
- Remove redundant auto-refresh on componentDidMount (already triggered by WS and sidebar_buttons) - Fix race condition in handleRefresh using instance flag + mounted guard - Change <a> to <button> for accessibility (+ aria-label) - Use theme utils for refresh button color (compat dark theme) - Remove flex change from affects-all sectionHeaders. The flex layout already applies to all states (PRS/REVIEWS/UNREADS/ASSIGNMENTS). No visual regressions detected in screenshots; button alignment preserved via inline button styling. Fixes mattermostGH-1026 for review
04c9fc2 to
21dde9a
Compare
|
Hi @nang2049, I've addressed all the review feedback in commit 21dde9a:
Could you please re-review when you have a chance? Thanks! |
| } | ||
|
|
||
| render() { | ||
| const getStyle = makeStyleFromTheme((theme) => { |
There was a problem hiding this comment.
Nice :) theme handling looks right. One small follow-up: makeStyleFromTheme returns a theme memoized function but by declaring getStyle inside render() we build a brand-new factory on every render which defeats the memoization. Please hoist it to module scope same as the snippet in my earlier comment:
const getStyle = makeStyleFromTheme((theme) => ({
sectionHeader: { ... },
refreshButton: { ... },
}));
Then just call const style = getStyle(this.props.theme); inside render().
There was a problem hiding this comment.
✅ Addressed in commit 0d143a3 — Hoisted getStyle to module scope. Now makeStyleFromTheme creates the factory only once instead of on every render, and render() just calls getStyle(this.props.theme).
nang2049
added
2: Dev Review
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 change is limited to the GitHub plugin RHS sidebar UI and its refresh-trigger wiring, adding “refresh on open” plus a manual refresh button with async loading/race-condition handling. This primarily affects UI state and network fetch timing rather than core business logic.
Regression Risk: Moderate. New async refresh flow and lifecycle/mounted guards introduce potential edge cases (double-clicks, unmount during refresh, repeated sidebar opens), but the blast radius is confined to the sidebar component and its related Redux action.
QA Recommendation: Do targeted manual QA for RHS open-triggered refresh, manual refresh button behavior (spinner + tooltip), rapid repeated clicks, unmount/navigation away during an in-flight refresh, and correct rendering of notifications/PRs/assigned/issues/review requests on both light and dark themes. Skipping manual QA is not recommended.
Generated by CodeRabbitAI