test(new-webui): Add React Storybook for UI testing & docs; Extract TimeRangeInputBase and set up tests for it. - #832
Conversation
…imeRangeInputBase and set up tests for it.
WalkthroughThis set of changes introduces Storybook support, visual regression testing, and a new reusable Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TimeRangeInputBase
participant SearchStore
User->>TimeRangeInputBase: Selects preset from dropdown
TimeRangeInputBase->>TimeRangeInputBase: Updates selected option state
TimeRangeInputBase->>SearchStore: Calls onChange with mapped date range
User->>TimeRangeInputBase: Selects "Custom" from dropdown
TimeRangeInputBase->>TimeRangeInputBase: Shows date range picker
User->>TimeRangeInputBase: Picks custom date range
TimeRangeInputBase->>TimeRangeInputBase: Validates range
TimeRangeInputBase->>SearchStore: Calls onChange with custom range
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
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:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. 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 (3)
components/log-viewer-webui/client/src/utils/string.ts (1)
1-7: AddsafeStringutility.
This new helper cleanly normalizesundefinedinputs to empty strings. For stricter nullish handling and to avoid masking other falsy values, consider using the nullish coalescing operator (value ?? "").components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput.tsx (1)
5-9: Complete the JSDoc return statementThe JSDoc comment is missing a description for the @return tag.
/** * Renders an input to setting the time range filter for the query. * * @return + * A React component for selecting time ranges in the search interface. */components/log-viewer-webui/client/vitest.workspace.ts (1)
13-33: Comprehensive Vitest workspace configurationThe workspace configuration properly integrates Storybook testing with headless Chromium via Playwright. This setup aligns well with the PR objectives of adding UI testing capabilities.
Consider adding a brief comment explaining why only Chromium is specified for browser testing (if there's a specific reason).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
components/log-viewer-webui/client/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
components/log-viewer-webui/client/.gitignore(1 hunks)components/log-viewer-webui/client/.storybook/main.ts(1 hunks)components/log-viewer-webui/client/.storybook/preview.ts(1 hunks)components/log-viewer-webui/client/.storybook/vitest.setup.ts(1 hunks)components/log-viewer-webui/client/eslint.config.mjs(1 hunks)components/log-viewer-webui/client/package.json(2 hunks)components/log-viewer-webui/client/src/components/TimeRangeInputBase/index.tsx(1 hunks)components/log-viewer-webui/client/src/components/TimeRangeInputBase/utils.tsx(3 hunks)components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput.tsx(1 hunks)components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/index.tsx(0 hunks)components/log-viewer-webui/client/src/pages/SearchPage/SearchState.tsx(2 hunks)components/log-viewer-webui/client/src/stories/TimeRangeInputBase.stories.ts(1 hunks)components/log-viewer-webui/client/src/typings/time.ts(1 hunks)components/log-viewer-webui/client/src/utils/string.ts(1 hunks)components/log-viewer-webui/client/tsconfig/tsconfig.node.json(1 hunks)components/log-viewer-webui/client/vitest.workspace.ts(1 hunks)
💤 Files with no reviewable changes (1)
- components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput/index.tsx
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ` rather than `!`.
**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}: - Preferfalse == <expression>rather than!<expression>.
components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput.tsxcomponents/log-viewer-webui/client/src/utils/string.tscomponents/log-viewer-webui/client/src/typings/time.tscomponents/log-viewer-webui/client/vitest.workspace.tscomponents/log-viewer-webui/client/src/components/TimeRangeInputBase/utils.tsxcomponents/log-viewer-webui/client/src/stories/TimeRangeInputBase.stories.tscomponents/log-viewer-webui/client/src/pages/SearchPage/SearchState.tsxcomponents/log-viewer-webui/client/src/components/TimeRangeInputBase/index.tsx
🧬 Code Graph Analysis (2)
components/log-viewer-webui/client/src/components/TimeRangeInputBase/utils.tsx (2)
components/log-viewer-webui/client/src/components/TimeRangeInputBase/index.tsx (1)
TIME_RANGE_OPTION(103-103)components/log-viewer-webui/client/src/typings/time.ts (1)
TimeRange(9-9)
components/log-viewer-webui/client/src/pages/SearchPage/SearchState.tsx (1)
components/log-viewer-webui/client/src/typings/time.ts (1)
TimeRange(9-9)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: lint-check (macos-latest)
- GitHub Check: build (macos-latest)
🔇 Additional comments (26)
components/log-viewer-webui/client/.gitignore (1)
10-12: Appropriate exclusion of Storybook logs.
The addition of the “# Logs” section and the*storybook.logrule cleanly prevents transient Storybook log files from being committed.components/log-viewer-webui/client/package.json (1)
31-49:Details
✅ Verification successful
Verify React version compatibility and package correctness.
You’ve addedreact@^19.0.0andreact-dom@^19.0.0, along with other Storybook and Vitest dependencies. Please confirm these major versions are published and compatible with your environment.
🏁 Script executed:
#!/usr/bin/env bash # Verify the latest published versions of React and React DOM npm view react version npm view react-dom versionLength of output: 67
React 19.x Verified (19.1.0) Availability
The^19.0.0specifier for both React and React DOM resolves to the published version 19.1.0, so these packages are available on npm.• components/log-viewer-webui/client/package.json (devDependencies: lines 31–49)
components/log-viewer-webui/client/tsconfig/tsconfig.node.json (1)
4-6: Include new config files in Node tsconfig.
Adding../.storybook/**/*.ts,../vite.config.ts, and../vitest.workspace.tsensures TypeScript will type-check your Storybook setup and Vitest workspace alongside the existing configuration.components/log-viewer-webui/client/src/typings/time.ts (1)
1-9: DefineTimeRangealias.
Centralizing the tuple type[Nullable<Dayjs>, Nullable<Dayjs>]underTimeRangeimproves consistency and reusability across UI components and utilities.components/log-viewer-webui/client/eslint.config.mjs (1)
24-28: ESLint configuration expanded to support Storybook filesThe configuration has been properly updated to include the new Storybook TypeScript files and Vitest workspace file under the Node-specific TypeScript configuration. This change correctly supports the new Storybook and testing infrastructure being added.
components/log-viewer-webui/client/.storybook/main.ts (1)
1-20: Well-structured Storybook configurationThis is a clean and appropriate Storybook configuration for a React project using Vite. The stories pattern is correctly set up to find TypeScript stories, and the necessary addons are included for testing and documentation purposes.
components/log-viewer-webui/client/.storybook/vitest.setup.ts (1)
1-11: Correct setup for Storybook/Vitest integrationThe integration between Storybook and Vitest is properly configured. The code correctly applies the project annotations and hooks into Vitest's lifecycle, following best practices for testing Storybook stories.
components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput.tsx (1)
10-18: Clean wrapper for TimeRangeInputBase componentThe component implementation is clean and correctly uses the extracted TimeRangeInputBase component with appropriate props. This follows good separation of concerns by delegating the UI logic to the base component while this wrapper handles the connection to the search state.
components/log-viewer-webui/client/.storybook/preview.ts (1)
1-16: Well-configured Storybook preview fileThis configuration correctly sets up the parameter matchers for automatic recognition of color and date properties in Storybook controls. The file is properly typed with the
Previewtype from@storybook/react.components/log-viewer-webui/client/vitest.workspace.ts (1)
8-11: Directory path handling looks goodThe code correctly handles directory path determination in a way that's compatible with both CommonJS and ES modules.
components/log-viewer-webui/client/src/pages/SearchPage/SearchState.tsx (3)
6-7: Properly updated import pathsThe imports are correctly updated to reference the new location of time range utilities and the new
TimeRangetype.
20-20: Improved type consistencyReplacing the explicit tuple type with the
TimeRangetype alias improves code consistency.
22-22: Consistent parameter typingThe parameter type for
updateTimeRangeis correctly updated to match thetimeRangeproperty type.components/log-viewer-webui/client/src/components/TimeRangeInputBase/utils.tsx (3)
3-4: Proper type importThe
TimeRangetype is correctly imported from the shared typing module.
23-62: Enhanced type safety and readabilityThe changes to
TIME_RANGE_OPTION_DAYJS_MAPbring several improvements:
- Using
Readonly<Record<...>>withObject.freezeensures immutability- Adopting the
TimeRangetype improves type consistency across the codebase- The consistent formatting with each date range on separate lines enhances readability
These changes align well with the PR's goal of creating a more reusable and testable component.
79-80: Updated function signature for type consistencyThe
isValidDateRangefunction signature is properly updated to use theTimeRangetype, maintaining type consistency throughout the codebase.components/log-viewer-webui/client/src/stories/TimeRangeInputBase.stories.ts (4)
18-41: Well-structured Storybook meta configuration.The meta configuration is well organized with appropriate categorization under "Search", centered layout, autodocs tag for automatic documentation, and properly configured controls for the component props.
46-64: Excellent test case for Custom time range input.This story effectively tests the custom time range input functionality by:
- Setting the default value to the CUSTOM option
- Simulating user interactions with date inputs
- Verifying that onChange is not triggered until Enter is pressed
- Confirming the callback is called exactly once after proper input
This provides good coverage for the custom date range entry workflow.
73-92: Good test coverage for preset selection.This story effectively tests selecting a preset from the dropdown by:
- Verifying all preset options are rendered in the dropdown
- Confirming that onChange is not called until an option is selected
- Validating that the callback is triggered exactly once after selection
The comprehensive verification of all available options ensures the component renders the complete set of time range presets.
94-108: Proper testing of Custom option selection behavior.This story correctly verifies that selecting the "Custom" option from the dropdown:
- Does not immediately trigger the onChange callback
- Allows the user to proceed to enter custom dates (which would be verified in the other Custom story)
This test complements the other stories by covering another important interaction path.
components/log-viewer-webui/client/src/components/TimeRangeInputBase/index.tsx (6)
25-34: Well-defined props interface with helpful documentation.The TimeRangeInputProps interface is clearly defined with appropriate JSDoc comments explaining the purpose of each prop and their default behaviors.
36-49: Comprehensive component documentation.The JSDoc comments effectively describe the component's purpose, behavior, and props, which will be valuable for automatic documentation generation in Storybook and for other developers using this component.
50-60: Well-implemented state management for option selection.The state management approach is solid:
- Tracking the selected option with useState
- Using a ref to maintain the last non-custom selection for proper initialization
- Correctly handling option changes with appropriate side effects
The option selection logic properly updates the time range through the onChange callback when selecting standard presets while deferring updates for custom ranges until the user specifies dates.
62-69: Proper validation in range picker change handler.This handler correctly:
- Follows the coding guideline using
false === isValidDateRange(newRange)rather than negation- Prevents invalid date ranges from being passed to the onChange callback
- Only updates when a valid range is selected
71-73: Smart default picker value calculation.The logic for determining the default value for the date range picker is well thought out:
- When the last option was CUSTOM, it defaults to TODAY
- Otherwise, it uses the corresponding date range for the last selected option
This provides a good user experience when switching between preset and custom options.
75-97: Clean, accessible UI implementation.The component's rendering logic is well structured:
- The Select component is always visible with appropriate styling
- The DatePicker.RangePicker is conditionally rendered only when CUSTOM is selected
- Both components use the "large" size for consistency and better usability
- CSS classes are applied conditionally for styling different states
| "antd": "VITE_USE_ANTD_APP=true npm run start", | ||
| "storybook": "storybook dev -p 6006", | ||
| "storybook:build": "storybook build" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Introduce ANTD and Storybook scripts.
- The
antdscript enables launching the app with the ANTD design system. - The
storybookandstorybook:buildscripts facilitate component development and static builds.
Note: On Windows, settingVITE_USE_ANTD_APP=truemay require a cross-platform tool likecross-env.
There was a problem hiding this comment.
Changes:
- Refactor the interface to make the component reusable & testable.
- Add inline docs for props.
- Fix a bug where the datetime picker does not sync with the last selected range when "Custom" option is first selected.
- Add util
safeStringto streamline unsafe CSS module class names handling.
junhaoliao
changed the title
junhaoliao
changed the title
junhaoliao
changed the title
hoophalab
left a comment
hoophalab
left a comment
There was a problem hiding this comment.
Storybook is neat and everything is working nicely.
The Search page still looks the same after the refactor.
I have a few comments on the code style.
| * @param props.defaultValue | ||
| * @return | ||
| */ | ||
| const TimeRangeInputBase = ({ |
There was a problem hiding this comment.
We should treat "components" as extensions to antd.
Instead of naming this as TimeRangeInputBase, how about
- name this as
TimeRangeInput, - remove
components/log-viewer-webui/client/src/pages/SearchPage/SearchControls/TimeRangeInput.tsx - supply parameters directly in
components/log-viewer-webui/client/src/pages/SearchPage/SearchControls
There was a problem hiding this comment.
Naming things as utils often turns into a dumping ground over time.
We could rename this one to something more specific such as timeRangeOption.ts, and maintain it as a generic time range library.
There was a problem hiding this comment.
We've been using a separate typings directory in both log-viewer and this repo, and it might be a bit late to bring it up. I have a personal take on it:
-
typingsdirectory hurts modularity.
For instance, we wouldn’t moveinterface SearchStatetotypingssince it’s only used inSearchControls.
Same logic applies here: thisTimeRangeis only used by the users ofTimeRangeInput. -
We're writing TypeScript from the ground up, not just adding types to existing JavaScript code.
How about:
- move everything into the proposed
timeRangeOption.tsfile.
There was a problem hiding this comment.
Review on storybook setup. Overall setup looks reasonable
three questions not in code:
- Do you know if there is a way to get rid of docs strings from docs? The "@PARAM" looks weird
-
I tried the run tests feature, and it dosen't work. It says missing dependency but I ran npm ci?
-
It looks like there is a way to run all the tests from the CLI with vitest. We should probably set that up in this PR. I dont think will be difficult
Looking at actual tests now, but will be a second review
There was a problem hiding this comment.
You should add the reference for this?
I believe here.
https://storybook.js.org/docs/configure/integration/typescript
There was a problem hiding this comment.
When does it generated logs? I couldnt get it to generate any
There was a problem hiding this comment.
You should add the docs reference for this
There was a problem hiding this comment.
Do we need this chromatic, i dont believe it is being used
There was a problem hiding this comment.
Again I would add the reference for this. https://storybook.js.org/docs/writing-tests/test-addon
Also why did you pick this instead of the simple one?
import { defineConfig, mergeConfig } from 'vitest/config';
import { storybookTest } from '@storybook/experimental-addon-test/vitest-plugin';
import path from 'node:path';
import { fileURLToPath } from 'node:url';
const dirname =
typeof __dirname !== 'undefined' ? __dirname : path.dirname(fileURLToPath(import.meta.url));
import viteConfig from './vite.config';
export default mergeConfig(
viteConfig,
defineConfig({
plugins: [
storybookTest({
// The location of your Storybook config, main.js|ts
configDir: path.join(dirname, '.storybook'),
// This should match your package.json script to run Storybook
// The --ci flag will skip prompts and not open a browser
storybookScript: 'yarn storybook --ci',
}),
],
test: {
// Enable browser mode
browser: {
enabled: true,
name: 'chromium',
// Make sure to install Playwright
provider: 'playwright',
headless: true,
},
setupFiles: ['./.storybook/vitest.setup.ts'],
},
})
);
There was a problem hiding this comment.
Can you add a reference for this?
davemarco
left a comment
davemarco
left a comment
There was a problem hiding this comment.
Review for the stories themselves. Next will put one for the changes you made to my existing code
| }, | ||
| play: async ({canvasElement, args}) => { | ||
| const canvas = within(canvasElement); | ||
| const startDateInput = await canvas.findByPlaceholderText("Start date"); |
There was a problem hiding this comment.
I think you can use GetByPlaceholderText and then can remove the await. Same comment for endDateInput.
| type Story = StoryObj<typeof meta>; | ||
|
|
||
|
|
||
| const Custom: Story = { |
There was a problem hiding this comment.
We should probably have a comment at the top of each of these stories saying what it does.
There was a problem hiding this comment.
I think this should be next to the component itself (in its directory) and not in its own folder. See their docs.
https://storybook.js.org/docs/writing-stories#where-to-put-stories
3 participants
Description
Checklist
breaking change.
TODO: @junhaoliao submit another PR for CI; then another one for docs. The plan is to resolve (close) new-webui: Add Storybook support for custom components. #825 when the docs are up.
Validation performed
cd components/log-viewer-webui/client.npm cinpm run storybookand observed Storybook popped up in a browser with URL http://localhost:6006 .Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests