Replace DevSessionUI footer with tabbed interface to display more app information #6121
Conversation
nickwesselman
changed the title
Coverage report
Show new covered files 🐣
Show files with reduced coverage 🔻
Test suite run success3043 tests passing in 1313 suites. Report generated by 🧪jest coverage report action from 65547ff |
|
/snapit |
|
🫰✨ Thanks @gonzaloriestra! Your snapshot has been published to npm. Test the snapshot by installing your package globally: pnpm i -g @shopify/[email protected]Tip If you get an Caution After installing, validate the version by running just |
There was a problem hiding this comment.
Nice work! I love the new UI, it works perfectly and the code LGTM. Have you tested it on Windows/Linux?
Some suggestions (not blocking):
- I'd enable the left/right arrows to switch between the tabs
- Not sure how to fix this, but the
(q) Quittab is actually not a usable tab, but more like a button... Maybe a different style or place? - The App URL and dev store should be hyperlinks (they appear underscored):
- The text on the disabled tabs is a bit hard to read with light themes (see above)
packages/app/src/cli/services/dev/ui/components/TabPanel.test.tsx
Outdated
Show resolved
Hide resolved
🤔 I'm not sure on this as it's meant to be informational vs clicked. Opening the App URL directly with our Remix template is actually quite confusing -- it lands you on a mock marketing page for the app. The dev store could be useful though, maybe even with an additional admin link. |
|
/snapit |
|
🫰✨ Thanks @nickwesselman! Your snapshot has been published to npm. Test the snapshot by installing your package globally: pnpm i -g @shopify/[email protected]Tip If you get an Caution After installing, validate the version by running just |
|
Implemented PR feedback and more:
I also tested on Windows and Linux (via Ubuntu on WSL). |
- Add appURL prop to DevSessionUI component interface - Display App URL in terminal footer when available - Pass development URL from config through renderDev to DevSessionUI - Show App URL above Preview URL and GraphiQL URL for consistent ordering 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
This commit introduces a comprehensive app information modal accessible via the 'i' key during `shopify app dev` sessions. The modal displays essential app details including app name, app URL, config file, dev store, and organization in a clean, formatted interface. Key features: - Modal triggered by 'i' key (when app preview is ready) - Escape key or 'i' key again to close - Uses Alert component from cli-kit for consistent styling - Hides preview/GraphiQL URLs when modal is active - Comprehensive test coverage with 21 test cases - Follows existing code patterns and conventions Files changed: - DevSessionUI: Added modal state management and keyboard handlers - New utility: formatAppInfoBody for consistent formatting - Enhanced prop threading through dev session components - Updated test snapshots and added extensive test coverage 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
The Box wrapper around the persistent dev alert was inadvertently removed.
This change restores the proper spacing and layout consistency by adding
back the Box component with marginTop={1} and flexDirection="column".
🤖 Generated with [Claude Code](https://claude.ai/code)
Co-Authored-By: Claude <[email protected]>
…aracters - Convert app info modal to persistent tabbed interface (Status, App Info, Quit) - Add connected box drawing character borders (╒═╤═╕, └─┴─┘) for visual tab connection - Refactor input handling to use object-based tab structure for cleaner code - Update tests to match new tabbed behavior 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Remove 'switches back to status tab when s is pressed after viewing info tab' test (covered by comprehensive tab switching test) - Remove 'hides preview and graphiql URLs when app info tab is shown' test (covered by tab switching test) - Maintain same functional coverage with cleaner test suite
- Only show tabbed interface when canUseShortcuts is true (raw mode supported) - Fall back to simple border layout when shortcuts are not available - Refactor non-interactive fallback to reuse status tab content directly - Add test for non-interactive fallback with proper useStdin mocking - Prevents confusing tabbed interface display in CI/non-interactive environments 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
These files were part of the old modal-based app info system that has been replaced by the tabbed interface using TabularData. The formatAppInfoBody function was designed for Alert components with "Press Esc to close" functionality, which is no longer needed. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Refactored the DevSessionUI to improve separation of concerns: - Created new TabPanel component for reusable tab functionality - Moved tab state management and input handling to TabPanel - Kept global concerns (Ctrl+C, error handling) in DevSessionUI - Cleaned up component APIs by only exporting necessary types - Improved code organization and maintainability The TabPanel component now handles: - Tab state management (activeTab) - Tab navigation input handling - Tab rendering with box drawing characters - Tab-specific shortcuts DevSessionUI maintains responsibility for: - Global input handling (Ctrl+C) - Application lifecycle management - Error state management - Business logic coordination 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Add full test coverage for TabPanel component with 9 test cases - Make initialActiveTab prop optional with fallback to first tab - Add runtime validation to handle empty tabs gracefully - Improve error handling with proper undefined state management - Use nullish coalescing operator for better type safety 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Remove redundant tests for tab switching, tab rendering, and content display - Keep simplified app info test focused on DevSessionUI behavior - Retain raw mode fallback test as it's DevSessionUI-specific UI logic - Improve test separation of concerns with comprehensive TabPanel coverage 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Add arrow key navigation to TabPanel for content tabs only - Navigate between tabs with left/right arrows - Skip action-only tabs during navigation - Prevent wraparound at tab boundaries - Enhance App Info tab with clickable store links - Convert Dev Store to clickable link - Add new Dev Store Admin entry with link - Filter out empty table rows - Update tab styling and improve UX - Change quit tab label from "Quit" to "to quit" - Improve active tab styling with inverse highlighting - Dim action tab labels for visual distinction - Add comprehensive test coverage for arrow key navigation These improvements were made in response to PR feedback. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
nickwesselman
force-pushed
the
feature/app-info-tabs
branch
from
67f55fa to
65547ff
Compare
Differences in type declarationsWe detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:
New type declarationsWe found no new type declarations in this PR Existing type declarationspackages/cli-kit/dist/public/node/ui/components.d.ts@@ -1,3 +1,4 @@
export { ConcurrentOutput, ConcurrentOutputContext, useConcurrentOutputContext, } from '../../../private/node/ui/components/ConcurrentOutput.js';
export { Alert } from '../../../private/node/ui/components/Alert.js';
-export { Link } from '../../../private/node/ui/components/Link.js';
\ No newline at end of file
+export { Link } from '../../../private/node/ui/components/Link.js';
+export { TabularData } from '../../../private/node/ui/components/TabularData.js';
\ No newline at end of file
|
|
/snapit |
|
🫰✨ Thanks @nickwesselman! Your snapshot has been published to npm. Test the snapshot by installing your package globally: pnpm i -g @shopify/[email protected]Tip If you get an Caution After installing, validate the version by running just |
I agree with this feedback. If the q tab is genuinely not navigable, but more a quick action you can take to terminate the process, I'd still house it in the main screen with the other short-keys to deal with the ongoing server process. Then the tabs can remain purely about accessing the different tabbed states in the CLI, and be cleanly and consistently arrow-navigated too. |
| export const TabPanel: React.FunctionComponent<TabPanelProps> = ({tabs, initialActiveTab}) => { | ||
| const {isRawModeSupported: canUseShortcuts} = useStdin() | ||
| const firstTabKey = Object.keys(tabs)[0] | ||
| const [activeTab, setActiveTab] = useState<string | undefined>(initialActiveTab ?? firstTabKey) |
There was a problem hiding this comment.
Can this really be undefined? I feel like TabPanelProps could have initialActiveTab as a mandatory property.
| let newIndex | ||
| // Right arrow | ||
| if (key?.rightArrow) { | ||
| newIndex = currentIndex + 1 < contentTabs.length ? currentIndex + 1 : currentIndex |
There was a problem hiding this comment.
no need to change it if you prefer your way, but is this easier to follow?:
newIndex = min(currentIndex + 1, contentTabs.length - 1)and
newIndex = max(currentIndex - 1, 0)Also I think we can simplify this code to have less indented ifs:
if (key?.leftArrow || key?.rightArrow) {
const contentTabs = Object.entries(tabs).filter(([_, tab]) => tab.content)
const currentIndex = contentTabs.findIndex(([tabKey]) => tabKey === activeTab)
if (currentIndex === -1) return
const modifier = key.leftArrow ? -1 : 1
const newIndex = Math.min(Math.max(currentIndex + modifier, 0), contentTabs.length - 1)
const newTabEntry = contentTabs[newIndex]
if (newTabEntry) {
setActiveTab(newTabEntry[0])
}
}Does this make sense? is it easier or harder to follow?
|
Is easy to break it by resizing the terminal. Should the tab container width be limited to the size of the tabs to prevent this? Since it now takes the whole width, any minor resize breaks it, with a smaller container we prevent that unless you make a big resize. |
Yeah @megthesmith has some ideas for the design that may help with this, I am going to try some iterations. |
The new
app devcommand for Dev Platform makes the app URL less accessible. This PR adds a new App Information tab accessible via theikey inapp dev, which includes the app URL and more. The tabs can also be navigated via arrow keys.It also adds the App URL as an output to the app dev logs, via dev session messaging on the app home component.
Demo / Screenshots
17-42-yn95m-c3ydv.mp4
Summary
Test plan
🤖 Generated with Claude Code