Elementary upgrade button #1625
Conversation
…entary needs an upgrade if it does, allow the user to upgrade through a button-click once they have pushed their updated packages.yml to their github repo
WalkthroughThe ElementaryStatus type in the DBT Elementary component was refactored to use nested objects for package and schema details, enabling richer upgrade detection. New state variables and UI logic were added to manage and display upgrade prompts, including a button-driven upgrade flow with loading indicators, completion messaging, and error handling. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI (Elementary)
participant Backend
User->>UI (Elementary): Load component
UI (Elementary)->>Backend: checkElementarySetupStatus()
Backend-->>UI (Elementary): status (with elementary_package.needs_upgrade)
alt Upgrade needed
UI (Elementary)->>User: Show upgrade message and button
User->>UI (Elementary): Click "Complete Upgrade"
UI (Elementary)->>Backend: gitPullAndMigrateElementaryTrackingTables()
Backend-->>UI (Elementary): Success or Error
UI (Elementary)->>User: Show upgrade completion or error message
else No upgrade needed
UI (Elementary)->>User: Normal flow
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/DBT/Elementary.tsx (2)
283-283: Remove commented debug code before production.The commented line appears to be for testing purposes and should be removed before merging to production.
- // response.exists.elementary_package.needs_upgrade = '0.20.0';
380-380: Consider using a more robust condition for button display.Using string matching (
includes('click the button when done')) to determine button visibility is fragile and tightly couples the UI logic to specific message text. Consider using a separate boolean state or enum to control button visibility.// Add a new state variable const [showUpgradeButton, setShowUpgradeButton] = useState(false); // In handleCheckDbtFiles, set both message and button state if (response?.exists?.elementary_package?.needs_upgrade) { setUpgradeMessage( `Please update the version of "elementary-data/elementary" in your packages.yml to ${response.exists.elementary_package.needs_upgrade} and push to your repository` ); setShowUpgradeButton(true); } // In the UI {showUpgradeButton && ( <Button // ... existing props >This approach makes the code more maintainable and less prone to breaking when message text changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/DBT/Elementary.tsx(6 hunks)
🔇 Additional comments (5)
src/components/DBT/Elementary.tsx (5)
26-34: Type definition correctly models the enhanced API response structure.The updated
ElementaryStatustype properly reflects the change from simple strings to nested objects, providing better type safety for the upgrade detection feature. The optional properties and nested structure align well with the API response format.
145-146: State management properly initialized for upgrade functionality.The new state variables are correctly typed and initialized with appropriate default values for managing upgrade messages and progress indicators.
287-291: Upgrade detection logic is well-implemented.The logic correctly checks for the
needs_upgradeproperty and sets an appropriate user message. The conditional check prevents unnecessary state updates when no upgrade is needed.
370-408: Upgrade UI provides excellent user experience with proper feedback.The upgrade message card implementation is well-designed with:
- Clear visual hierarchy using warning colors
- Conditional button display based on message content
- Loading state with spinner during upgrade process
- Proper button states (disabled during progress)
- Dismiss functionality for user control
The UI correctly handles all upgrade states and provides appropriate feedback to users.
208-208: Confirmed: No repeated API calls on re-renders
ThehandleCheckDbtFiles()invocation lives insidecheckElementarySetupStatus(), which itself is only called from auseEffecthooked to[session]. Sincesessiontypically changes once on init, the DBT file check happens only once per session rather than on every mount. No performance-related changes needed here.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/components/DBT/Elementary.tsx (1)
308-327: Function correctly handles upgrade process with proper state management.The implementation properly manages the upgrade flow with appropriate error handling and state cleanup. The finally block ensures
setUpgradeInProgress(false)is called even with early returns, maintaining consistent UI state.
🧹 Nitpick comments (2)
src/components/DBT/Elementary.tsx (2)
283-283: Remove commented debug code.The commented line appears to be debug code that artificially sets the upgrade requirement. This should be removed to avoid confusion and potential issues in production.
Apply this diff to remove the debug code:
- // response.exists.elementary_package.needs_upgrade = '0.20.0';
370-408: Well-implemented upgrade UI with minor suggestions for improvement.The upgrade message UI provides good user experience with appropriate visual styling, loading states, and dismiss functionality. The implementation follows Material-UI patterns consistently.
Consider these minor improvements:
Make button visibility more robust: Instead of string matching, use a separate boolean state or enum to determine when to show the button.
Add accessibility attributes: Include
aria-live="polite"for the upgrade message and properaria-labelattributes for buttons.<Typography variant="body1" sx={{ color: '#856404', fontWeight: 500 }}> + <Box component="div" aria-live="polite"> {upgradeMessage} + </Box> </Typography> </Box> - {upgradeMessage.includes('click the button when done') && ( + {upgradeMessage.includes('click the button when done') && ( <Button variant="contained" color="warning" onClick={gitPullAndMigrateElementaryTrackingTables} disabled={upgradeInProgress} sx={{ ml: 2, minWidth: '120px' }} + aria-label="Complete Elementary upgrade process" >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/DBT/Elementary.tsx(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: checks
🔇 Additional comments (4)
src/components/DBT/Elementary.tsx (4)
24-40: LGTM! Type definition properly supports upgrade detection.The refactoring from optional strings to nested objects enables richer upgrade detection by providing detailed package information including version and upgrade requirements. The structure is well-designed for the upgrade flow.
145-146: LGTM! State variables properly manage upgrade flow.The new state variables correctly track upgrade messaging and progress state with appropriate types and initial values.
208-208: LGTM! Correctly implements automatic upgrade checking.Adding the
handleCheckDbtFiles()call when Elementary is already set up properly implements the PR requirement to always check for upgrades. This ensures users are notified of available upgrades without manual intervention.
286-292: LGTM! Upgrade detection logic is well-implemented.The upgrade detection correctly uses optional chaining to safely access nested properties and provides clear, actionable messaging to users. The logic properly integrates with the existing setup flow.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/components/DBT/Elementary.tsx (1)
276-312: Fix error handling and improve upgrade detection logic.Several issues in this function:
- Line 280-282: Git pull failure doesn't prevent continued execution, which could lead to inconsistent state
- Line 289-293: The upgrade message is set regardless of whether there are missing items, which could confuse users
- Line 295-298: The condition checking for empty missing items could be more robust
Apply this diff to improve error handling and logic:
const handleCheckDbtFiles = async (firstTimeSetup: boolean) => { setLoading(true); try { const gitPullResponse: { success: boolean } = await httpPost(session, 'dbt/git_pull/', {}); if (!gitPullResponse.success) { errorToast('Something went wrong running git-pull', [], globalContext); + return; // Don't proceed if git pull fails } const checkDbtFilesResponse: ElementaryStatus = await httpGet(session, 'dbt/check-dbt-files'); setElementaryStatus(checkDbtFilesResponse); const needsUpgrade = checkDbtFilesResponse?.exists?.elementary_package?.needs_upgrade; - // Check for upgrade requirement and set message - if (needsUpgrade) { + // Check for upgrade requirement and set message only if no missing items + const hasMissingItems = checkDbtFilesResponse?.missing && + Object.keys(checkDbtFilesResponse.missing).length > 0; + + if (needsUpgrade && !hasMissingItems) { setUpgradeMessage( `Please update the version of "elementary-data/elementary" in your packages.yml to ${needsUpgrade} and click the button when done` ); } + if ( firstTimeSetup && - checkDbtFilesResponse?.missing && - Object.keys(checkDbtFilesResponse?.missing).length === 0 + !hasMissingItems ) {
🧹 Nitpick comments (1)
src/components/DBT/Elementary.tsx (1)
376-414: Enhance the upgrade UI for better UX.The upgrade UI implementation is functional but could be improved:
- Accessibility: Missing ARIA labels for screen readers
- Visual feedback: The close button could be more prominent
- Message specificity: Consider different styling for success vs. action-required messages
Apply this diff to improve accessibility and UX:
<Card sx={{ mb: 2, backgroundColor: '#fff3cd', border: '1px solid #ffeaa7' }}> <CardContent> - <Box sx={{ display: 'flex', alignItems: 'center', justifyContent: 'space-between' }}> + <Box + sx={{ display: 'flex', alignItems: 'center', justifyContent: 'space-between' }} + role="alert" + aria-live="polite" + > <Box sx={{ flex: 1 }}> <Typography variant="body1" sx={{ color: '#856404', fontWeight: 500 }}> {upgradeMessage} </Typography> </Box> {upgradeMessage.includes('click the button when done') && ( <Button variant="contained" color="warning" onClick={gitPullAndMigrateElementaryTrackingTables} disabled={upgradeInProgress} + aria-label="Complete Elementary package upgrade" sx={{ ml: 2, minWidth: '120px' }} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/DBT/Elementary.tsx(7 hunks)
🔇 Additional comments (5)
src/components/DBT/Elementary.tsx (5)
26-34: Approve the enhanced type definition structure.The refactoring of
ElementaryStatusto use nested objects provides better type safety and enables richer upgrade detection capabilities. The structure properly reflects the API response format with detailed package information including version and upgrade requirements.
145-146: Good addition of upgrade state management.The new state variables
upgradeMessageandupgradeInProgressappropriately manage the upgrade flow UI state and user feedback.
314-333: Approve the upgrade function with good error handling.The function correctly manages the upgrade flow with proper state management, error handling, and user feedback. The use of a finally block ensures the progress state is always reset, even with early returns.
435-435: Approve the updated Setup Elementary button.The button correctly passes
trueto indicate first-time setup, which aligns with the updated function signature and logic flow.
208-208: Impact of auto-running check-dbt-files on mountI confirmed via a repo-wide search that
handleCheckDbtFiles(and thedbt/check-dbt-filesendpoint) is only invoked insrc/components/DBT/Elementary.tsx—once automatically whenresponse.status === 'set-up'and again on the “Setup Elementary” button click.With the new
handleCheckDbtFiles(false)call in the “set-up” branch, the full Git-pull + DBT-file-check sequence now runs on every component mount for already-configured clients. Depending on project size, this could noticeably impact load times or backend load.Please verify that:
- Repeatedly invoking
dbt/git_pull/anddbt/check-dbt-fileson mount is acceptable in terms of performance and user experience.- If not, consider one or more of:
- Caching the last check result and skipping if unchanged.
- Throttling or debouncing the automatic run.
- Moving the background sync to a less frequent interval or explicit user action.
Always call "check-dbt-files" to see if the installed version of elementary needs an upgrade
If it does, allow the user to upgrade through a button-click once they have pushed their updated packages.yml to their github repo
Summary by CodeRabbit
New Features
Enhancements