Feat update deps #141
Feat update deps #141
Conversation
Romakita
force-pushed
the
feat-update-deps
branch
4 times, most recently
from
eb2a6f7 to
ca98fa5
Compare
Romakita
force-pushed
the
feat-update-deps
branch
from
ca98fa5 to
0cbce8a
Compare
|
Warning Rate limit exceeded@Romakita has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 15 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (31)
WalkthroughThis pull request introduces comprehensive updates to the project's configuration, focusing on modernizing the development workflow, enhancing testing capabilities, and improving the Storybook integration. The changes span multiple configuration files across GitHub Actions, package management, and build processes. Key modifications include updating Node.js versions, introducing Storybook testing, streamlining workflow configurations, and refactoring type definitions in various components. The updates aim to improve type safety, testing infrastructure, and overall project maintainability. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 6
🧹 Nitpick comments (11)
packages/react-formio/src/components/form-control/formControl.component.tsx (1)
4-10: Enhance type specificity on theonChangecallbackCurrently,
onChange?: (name: string, value: any) => void;usesanyfor the value parameter. Consider specifying a more explicit type or leveraging the genericDatato enforce stricter type safety.packages/react-formio/src/components/form-access/formAccess.component.tsx (1)
98-98: Consider explicitly specifying a return type.While TypeScript can infer return types, some teams prefer explicit return types (e.g.,
JSX.Element) for clarity. Verify whether your style guide or team conventions recommend explicit return types for React components..github/workflows/chromatic.yml (1)
5-9: Confirm the intended workflow triggers.This workflow triggers on pushes to any branch and on pull requests to the
productionbranch. If you intend to run Chromatic checks on all PRs (including feature branches), consider expandingbranchesin pull_request or using a more permissive pattern.packages/react-formio/package.json (1)
42-43: Consider updating microbundle to latest version.[email protected] is outdated. The latest version (0.15.1) includes important bug fixes and improvements.
Consider updating to the latest version:
- "microbundle": "0.13.0" + "microbundle": "^0.15.1"packages/storybook/package.json (1)
59-59: Consider removing empty peerDependencies.The empty peerDependencies field serves no purpose and can be removed.
- "peerDependencies": {}.github/workflows/website.yml (2)
4-7: Verify branch configuration comment accuracyThe comment mentions
mainbranch but the configuration usesmaster. This inconsistency could be confusing for contributors.- # Runs on pushes targeting the `main` branch. Change this to `master` if you're - # using the `master` branch as the default branch. + # Runs on pushes targeting the `master` branch
32-34: Consider enabling commented package managersThe workflow includes commented configurations for pnpm and Bun. Consider documenting why these are commented out or remove them if they won't be used.
packages/react-formio-container/package.json (1)
98-99: Consider removing empty dependencies objectThe empty dependencies object (
"dependencies": {}) doesn't serve any purpose and can be removed.- }, - "dependencies": {} + }.github/workflows/build.yml (1)
42-62: Review Storybook test timeout and error handlingThe new test-storybook job has some potential issues:
- 60-minute timeout might be excessive for most cases
- The concurrent server start and test execution could be more robust
Consider these improvements:
test-storybook: - timeout-minutes: 60 + timeout-minutes: 30 runs-on: ubuntu-latest steps: # ... previous steps ... - name: Serve Storybook and run tests 🔍 run: | + # Start server in background + npx http-server packages/storybook/.out --port 6006 --silent & + # Wait for server with timeout + npx wait-on -t 30000 tcp:6006 + # Run tests + yarn storybook:test:ci + # Cleanup + kill $(lsof -t -i:6006) || true - npx concurrently -k -s first -n "SB,TEST" -c "magenta,blue" \ - "npx http-server packages/storybook/.out --port 6006 --silent" \ - "npx wait-on tcp:6006 && yarn storybook:test:ci"package.json (1)
26-30: Script naming convention improvements and build process enhancementsThe new script organization improves clarity and introduces better integration between Tailwind and Storybook builds. The sequential dependency is well handled (tailwind:build before storybook:build).
However, consider adding error handling for the case when tailwind:build fails:
- "storybook:build": "yarn tailwind:build && lerna run storybook:build --stream", + "storybook:build": "yarn tailwind:build || exit 1 && lerna run storybook:build --stream", - "storybook:test": "yarn tailwind:build && lerna run storybook:test --stream", + "storybook:test": "yarn tailwind:build || exit 1 && lerna run storybook:test --stream",packages/react-formio/src/components/__fixtures__/form-actions.json (1)
171-174: Standardize method handlers for similar integration actionsIntegration actions (Google Sheets, SQL Connector, Jira, Twilio) have inconsistent method handlers:
- Most support create/update/delete
- Twilio only supports create
Consider standardizing the method handlers for similar integration types or document why certain methods are excluded.
"defaults": { "handler": ["after"], - "method": ["create"], + "method": ["create", "update", "delete"], "priority": 0, "name": "twilioSMS",Also applies to: 185-188, 199-202, 232-235
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (32)
.github/workflows/build.yml(1 hunks).github/workflows/chromatic.yml(1 hunks).github/workflows/website.yml(1 hunks)package.json(1 hunks)packages/config/package.json(1 hunks)packages/react-formio-container/coverage.json(1 hunks)packages/react-formio-container/package.json(2 hunks)packages/react-formio-container/src/views/formAccess.view.tsx(2 hunks)packages/react-formio-stores/coverage.json(1 hunks)packages/react-formio-stores/package.json(1 hunks)packages/react-formio/coverage.json(1 hunks)packages/react-formio/package.json(2 hunks)packages/react-formio/src/components/__fixtures__/form-actions.json(1 hunks)packages/react-formio/src/components/actions-table/actionsTable.component.tsx(1 hunks)packages/react-formio/src/components/actions-table/actionsTable.stories.tsx(2 hunks)packages/react-formio/src/components/form-access/formAccess.component.tsx(2 hunks)packages/react-formio/src/components/form-access/formAccess.stories.tsx(2 hunks)packages/react-formio/src/components/form-control/formControl.component.tsx(1 hunks)packages/react-formio/src/components/select/select.component.tsx(3 hunks)packages/react-formio/src/components/table/components/defaultCellOperations.component.tsx(1 hunks)packages/react-formio/src/components/table/components/defaultOperationButton.component.tsx(3 hunks)packages/react-formio/src/components/table/hooks/useOperations.hook.tsx(2 hunks)packages/redux-utils/package.json(2 hunks)packages/storybook/.storybook/main.js(0 hunks)packages/storybook/.storybook/main.ts(1 hunks)packages/storybook/chromatic.config.json(1 hunks)packages/storybook/package.json(3 hunks)packages/storybook/tsconfig.json(1 hunks)packages/storybook/tsconfig.node.json(1 hunks)packages/storybook/vite.config.mts(1 hunks)packages/tailwind-formio/package.json(1 hunks)packages/tailwind/package.json(2 hunks)
💤 Files with no reviewable changes (1)
- packages/storybook/.storybook/main.js
✅ Files skipped from review due to trivial changes (8)
- packages/config/package.json
- packages/tailwind-formio/package.json
- packages/storybook/tsconfig.json
- packages/storybook/tsconfig.node.json
- packages/storybook/chromatic.config.json
- packages/react-formio/coverage.json
- packages/react-formio-container/coverage.json
- packages/react-formio-stores/coverage.json
🧰 Additional context used
🪛 Biome (1.9.4)
packages/react-formio/src/components/table/hooks/useOperations.hook.tsx
[error] 8-8: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🔇 Additional comments (54)
packages/react-formio/src/components/table/hooks/useOperations.hook.tsx (2)
1-1: Good use of type imports for clarity.
Importing both ComponentType and FunctionComponent helps ensure explicit type definitions and maintain clarity in the codebase.
17-17: Proper default assignment for CellOperations.
Casting DefaultCellOperations to FunctionComponent ensures a consistent type signature. Just confirm that DefaultCellOperations indeed satisfies the expected component contract.
packages/react-formio/src/components/table/components/defaultCellOperations.component.tsx (5)
3-3: Clear import structure.
Importing OperationButtonProps and DefaultOperationButton keeps the file self-contained and explicit about dependencies.
5-15: Thoughtful prop definition using a dedicated interface.
DefaultCellOperationsProps provides a clear contract for operations, row, onClick, ctx, and i18n. This offers improved type safety and enhances maintainability.
17-17: Descriptive function signature with typed props.
Using DefaultCellOperationsProps in the function signature promotes readability and ensures that callers provide proper data.
23-24: Effective permissions check.
Filtering operations based on permissionsResolver is a neat approach to conditionally enable or disable features. Continue ensuring that each permission logic is thoroughly tested.
29-29: Comprehensive test IDs.
Including the index and action in the data-testid attribute is a good practice for making tests more precise.
packages/react-formio/src/components/table/components/defaultOperationButton.component.tsx (6)
2-2: Explicit import for HTML attributes.
Using HTMLAttributes from React clarifies that the component extends standard HTML button capabilities, improving discoverability.
7-7: Smart usage of Omit to tailor button attributes.
Extending Omit<HTMLAttributes<HTMLButtonElement>, "onClick"> is an elegant way to fine-tune the props interface without losing general HTML button attributes.
17-17: Optional ctx prop included.
Allowing ctx? fosters flexibility for data or context passing without cluttering the component.
30-30: Acceptable default for i18n.
Using a fallback (f: string) => f ensures that, when no localization function is provided, the raw string is returned.
31-33: Props destructuring is well-structured and maintainable.
Including data, ctx, and any additional HTML attributes via ...otherProps provides a cohesive interface while preserving flexibility.
38-39: Spread operator for HTML attributes and improved accessibility.
Applying ...otherProps to the button is a best practice, and adding an aria-label helps with screen readers and a11y compliance.
packages/storybook/vite.config.mts (1)
5-11: Ensure consistent environment variable usage
Defining process.env.NODE_ENV in the configuration is a good start. However, verify that your build environment and other scripts rely on or set the same variable consistently, avoiding mismatched environment references in production vs. development.
packages/react-formio-container/src/views/formAccess.view.tsx (2)
7-7: Clarify component override usage
You are overriding the default FormAccess component with props.FormAccessComponent. Double-check if all usage sites handle cases where FormAccessComponent might be undefined or not provided, ensuring safe fallback behaviors.
20-20: Confirm prop coverage in spread operator
Review the spread operator ({...props}) to confirm you are not unintentionally passing irrelevant or sensitive data to <Component>. This helps avoid potential security or performance pitfalls if props contains large or sensitive objects.
packages/react-formio/src/components/actions-table/actionsTable.component.tsx (2)
38-38: Use of distinct test identifier for Select is clear and consistent.
Renaming the data-testid to "action-table-select" provides clarity for testing.
46-46: Consistent naming of the add button test identifier.
The data-testid="action-table-add" naming convention looks more aligned with the component’s purpose.
packages/react-formio/src/components/form-access/formAccess.stories.tsx (6)
1-2: Imports from Storybook/test utilities are correctly declared.
These imports facilitate interactive testing.
10-10: Story argType for onSubmit
Defining the action creator for onSubmit is useful for capturing interactions in Storybook.
19-19: Adoption of the satisfies operator
Using satisfies Meta<typeof FormAccess> ensures strong type consistency for the default export.
21-21: Type alias for Story
Defining type Story = StoryObj<typeof FormAccess> is a clean approach to strongly type the story object.
23-37: Interactive test setup in Sandbox story
The play function and usage of user actions are a good approach for verifying interactive behavior.
39-74: Comprehensive story arguments
Providing realistic form data and roles helps ensure thorough testing coverage.
packages/react-formio/src/components/select/select.component.tsx (6)
3-3: Importing HTMLAttributes for typed HTML props
This approach helps maintain consistent React typing throughout the component.
8-13: Extended interface for SelectProps
Combining FormControlProps with HTML attributes, while omitting onChange/prefix, is a neat way to avoid collisions and keep the API tidy.
17-17: Generic type parameter “Data”
Using <Data> clarifies expected select value types, improving maintainability.
32-33: Reusable ref typed as HTMLSelectElement
Ensuring ref is correctly typed avoids confusion with inputs of different types.
39-39: Conditional initialization of Choices.js
The approach ensures Choices is only instantiated when required, improving performance.
67-67: Data-testid usage
data-testid={\select_${name}`}` is consistent, enabling targeted test queries.
packages/react-formio/src/components/actions-table/actionsTable.stories.tsx (6)
1-2: Storybook & test import statements
Correct usage of Meta, StoryObj, and testing libraries fosters maintainable stories and interactive tests.
4-4: Importing fixture data
Using form-actions.json for availableActions centralizes data management and improves test clarity.
41-41: satisfies Meta for ActionsTable
This ensures the default export is accurately typed, leveraging Storybook’s TS definitions.
43-43: Type alias Story
type Story = StoryObj<typeof ActionsTable> standardizes the structure and is consistent with best practices.
45-81: Sandbox story’s interactive test
The play function thoroughly verifies selection, addition, and button states. This approach reinforces best practices in interactive testing.
82-122: Argument definitions for testing complex interactions
Populating args with realistic data, including availableActions, data, and operations, ensures robust test coverage.
packages/react-formio/src/components/form-access/formAccess.component.tsx (1)
1-1: Good removal of PropTypes in favor of TypeScript.
By removing the PropTypes import, you have fully embraced TypeScript’s type system. This simplifies the code and avoids duplication of type definitions. Great job!
.github/workflows/chromatic.yml (1)
23-28: Ensure Chromatic token is set as a secret.
This step relies on CHROMATIC_TOKEN. Ensure it’s properly configured in GitHub Settings → Secrets to avoid build failures. Confirm that the chromaui/action version is up-to-date with Chromatic’s recommendations.
packages/redux-utils/package.json (2)
10-15: Export map is aligned with modern bundling standards.
Providing ES and CJS paths under "exports" is great for better module resolution. If you need Node 12 or older environment support, ensure you test for fallback or confirm Node’s ESM implementation is sufficient.
31-31: Empty dependencies are acceptable; verify usage.
You’ve defined "dependencies": {}. If the package references external libraries internally, confirm they are declared under "dependencies". Otherwise, an empty object is fine.
packages/tailwind/package.json (2)
12-14: Script renames may require CI/call-site updates.
Renaming build:tailwind to tailwind:build (and similarly for the viewer script) is fine. Ensure downstream scripts or documentation referencing the old script names are also updated.
37-38: Empty dependencies and peerDependencies.
Currently, both fields are empty. Verify that all required packages are declared in dependencies and the appropriate libraries used at runtime appear in peerDependencies.
packages/react-formio-stores/package.json (1)
9-14: LGTM! Verify dual package hazard handling.
The exports field is correctly configured for dual module/commonjs support, following Node.js best practices.
Let's verify the package exports configuration:
✅ Verification successful
LGTM! Exports field configuration is consistent across packages
The exports field configuration is correctly implemented and consistent across all packages in the monorepo:
- All packages use the same dual module/commonjs configuration
- All packages use the correct paths with
./dist/prefix - No potential dual package hazards found in imports
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify package.json configurations across the monorepo
# Check for consistent exports field configuration and potential dual package hazards
# Find all package.json files and check their exports field configuration
fd package.json | xargs jq -r 'select(.exports != null) | "\(.name): \(.exports)"'
# Check for potential dual package hazards by looking for direct imports
rg -g '*.{js,jsx,ts,tsx}' "from ['\"](\.{1,2}/|@tsed/react-formio-stores)"
Length of output: 66424
packages/react-formio/package.json (2)
5-6: LGTM! Path updates follow best practices.
The explicit relative paths for main and module fields follow Node.js best practices.
8-11: LGTM! Exports field matches monorepo convention.
The exports field configuration is consistent with other packages in the monorepo.
packages/storybook/package.json (4)
8-12: LGTM! Enhanced testing infrastructure.
New scripts for Storybook testing with both local and CI configurations are well structured.
44-44: Major Vite upgrade requires attention.
Upgrading from Vite 4 to 5 includes breaking changes that might affect the build.
Key breaking changes to verify:
- Node.js 14/16 support dropped
- Changed plugin hooks
- Updated CSS handling
6-6: Verify module type compatibility.
Setting "type": "module" affects how Node.js treats .js files. Ensure all JavaScript files in the package use ESM syntax.
✅ Verification successful
Module type configuration is correctly set for the codebase
The codebase is already properly structured for ESM:
- All source files use ESM syntax with
import/exportstatements - The only CommonJS syntax (
require/module.exports) is correctly used in.cjsfiles (tailwind.config.cjs and postcss.config.cjs), which explicitly use the .cjs extension to denote CommonJS modules - All other files consistently use modern ESM imports and exports
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential CommonJS syntax in .js files
# Look for require() and module.exports in .js files
rg -g '*.js' -e 'require\(' -e 'module\.exports' ./packages/storybook/
Length of output: 72
Script:
#!/bin/bash
# Let's check for JavaScript files first to ensure we have files to analyze
fd '\.js$' ./packages/storybook/
# Also check for any potential CJS-style imports in any file type
# This covers cases where imports might be in .ts, .jsx, .tsx files
rg -g '!*.{json,md}' -e 'require\(' -e 'module\.exports' ./packages/storybook/
# Check for ESM-style imports to verify ESM usage
rg -g '!*.{json,md}' -e 'import .* from' -e 'export .*' ./packages/storybook/
Length of output: 3500
23-34:
Major Storybook upgrade requires careful testing.
Upgrading from Storybook v7 to v8.4.7 includes breaking changes. Ensure all stories are tested with the new version.
Key breaking changes to verify:
- React 18+ is now required
- New MDX syntax
- Updated addon APIs
- Changed webpack configuration
Run the following to check for potential issues:
✅ Verification successful
Let me gather more information about the Storybook upgrade by checking the current configuration and dependencies.
Let me check for any potential issues with the stories and components.
Storybook v8 upgrade appears compatible with the codebase
Based on the verification:
- React 18 is already in use (
react: ^18.2.0) - Stories are using the modern CSF3 format with
MetaandStoryObjtypes - No deprecated APIs (
storiesOf,addon-info,addDecorator) were found - MDX stories are present and using the correct configuration with
@storybook/addon-mdx-gfm - The Storybook configuration is using the recommended vite builder and properly configured addons
The upgrade appears safe, but as a precaution:
- Run
npm run storybookto verify all stories load correctly - Run
npm run test-storybookto execute the interaction tests - Check Chromatic builds if you're using it for visual testing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Storybook configuration and stories
# Look for deprecated Storybook APIs and configurations
rg -g '*.{js,jsx,ts,tsx,mdx}' -e 'storiesOf\(' -e '@storybook/addon-info' -e 'addDecorator' ./packages/storybook/
Length of output: 111
Script:
#!/bin/bash
# Check React version in package.json files
echo "=== Checking React version ==="
rg '"react":\s*"(.+)"' -g 'package.json'
# Check Storybook config files
echo -e "\n=== Checking Storybook config ==="
fd -e js -e ts -p 'main|preview' --base-directory packages/storybook/.storybook -x cat {}
# Check for MDX stories
echo -e "\n=== Looking for MDX stories ==="
fd -e mdx --base-directory packages/storybook/
Length of output: 2353
Script:
#!/bin/bash
# Check story files for potential compatibility issues
echo "=== Checking story files ==="
rg -g '*.{stories,story}.{js,jsx,ts,tsx}' -e 'ComponentStory|Story|Meta' --context 2
# Check for potential breaking changes in components
echo -e "\n=== Checking components for potential issues ==="
rg -g '*.{js,jsx,ts,tsx}' -e 'useEffect.*\[\]' -e 'React.FC' --context 2 packages/
# Check package.json for previous Storybook version
echo -e "\n=== Previous Storybook version ==="
git show HEAD^:packages/storybook/package.json | rg '@storybook'
Length of output: 3358
.github/workflows/website.yml (2)
12-16: LGTM: Proper permissions configuration
The permissions are correctly configured for GitHub Pages deployment with the minimum required scopes:
contents: readfor accessing repository contentpages: writefor deploying to Pagesid-token: writefor authentication
18-22: LGTM: Well-configured concurrency settings
The concurrency configuration is properly set to prevent deployment conflicts while allowing in-progress deployments to complete.
packages/react-formio-container/package.json (1)
9-14: LGTM: Proper dual module exports configuration
The exports field is correctly configured to support both ESM (import) and CommonJS (require) module systems, pointing to the appropriate bundle files.
.github/workflows/build.yml (2)
9-10: Verify pull request trigger configuration
The pull request trigger is configured only for the master branch and specific event types. Ensure this is intentional and won't miss important events.
64-84: LGTM: Well-structured deployment job
The deploy-packages job is properly configured with:
- Correct dependencies on previous jobs
- Appropriate branch conditions
- Required secrets for deployment
| import { Hooks } from "react-table"; | ||
|
|
||
| import { Operation } from "../../../interfaces"; | ||
| import { DefaultCellOperations } from "../components/defaultCellOperations.component"; | ||
|
|
||
| export type UseOperationsHookProps = { | ||
| CellOperations?: React.ComponentType; | ||
| CellOperations?: FunctionComponent | ComponentType<{}>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider avoiding ComponentType<{}> in favor of a more specific prop type.
Static analysis tools caution against using {} as a type, as it implies "any non-nullable value." It might be beneficial to define a dedicated interface for the props expected by CellOperations or use a broader type like ComponentType<any> if you need flexibility.
🧰 Tools
🪛 Biome (1.9.4)
[error] 8-8: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
| addons: [ | ||
| "@storybook/addon-a11y", | ||
| "@storybook/addon-links", | ||
| "@storybook/addon-interactions", | ||
| "@storybook/addon-essentials", | ||
| "@storybook/addon-mdx-gfm", | ||
| "@storybook/addon-links", | ||
| "@storybook/addon-interactions", | ||
| "@chromatic-com/storybook" | ||
| ], |
There was a problem hiding this comment.
Duplicate addon references
You have added @storybook/addon-links and @storybook/addon-interactions twice (lines 16 & 20, and lines 17 & 21). This can lead to confusing or unexpected behaviors.
Apply the following diff to remove duplicates:
addons: [
"@storybook/addon-a11y",
"@storybook/addon-links",
"@storybook/addon-interactions",
"@storybook/addon-essentials",
"@storybook/addon-mdx-gfm",
- "@storybook/addon-links",
- "@storybook/addon-interactions",
"@chromatic-com/storybook"
],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| addons: [ | |
| "@storybook/addon-a11y", | |
| "@storybook/addon-links", | |
| "@storybook/addon-interactions", | |
| "@storybook/addon-essentials", | |
| "@storybook/addon-mdx-gfm", | |
| "@storybook/addon-links", | |
| "@storybook/addon-interactions", | |
| "@chromatic-com/storybook" | |
| ], | |
| addons: [ | |
| "@storybook/addon-a11y", | |
| "@storybook/addon-links", | |
| "@storybook/addon-interactions", | |
| "@storybook/addon-essentials", | |
| "@storybook/addon-mdx-gfm", | |
| "@chromatic-com/storybook" | |
| ], |
.github/workflows/website.yml
Outdated
| - name: Build with Storybook | ||
| run: | | ||
| yarn storybook:build | ||
| touch packages/storybook/.out/.nojekyll | ||
| echo "formio.tsed.dev" > packages/storybook/.out/CNAME |
There was a problem hiding this comment.
💡 Codebase verification
CNAME configuration mismatch detected
The CNAME value in the workflow file (formio.tsed.dev) doesn't match the configured GitHub Pages domain (formio.tsed.io). This mismatch could cause GitHub Pages deployment issues.
- In
.github/workflows/website.yml: Changeformio.tsed.devtoformio.tsed.io
🔗 Analysis chain
Verify CNAME configuration
The hardcoded CNAME value formio.tsed.dev is being written to the build output. Ensure this domain is properly configured in DNS settings.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the domain is configured in GitHub Pages settings
gh api repos/{owner}/{repo}/pages --jq '.cname'
Length of output: 64
| "defaults": { | ||
| "handler": ["before"], | ||
| "method": ["create", "update"], | ||
| "priority": 10, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent priority handling between similar actions
The priority values need review:
- Save action: priority 10
- Login action: priority 2
- LDAP login: priority 3
Authentication-related actions should have consistent priorities. Consider standardizing authentication priorities.
"defaults": {
"handler": ["before"],
"method": ["create"],
- "priority": 3,
+ "priority": 2,
"name": "ldap",Also applies to: 84-87, 157-160
| "name": "sql", | ||
| "title": "SQL Query", | ||
| "description": "Allows you to execute a remote SQL Query.", | ||
| "priority": 0, | ||
| "defaults": { | ||
| "handler": ["after"], | ||
| "method": ["create"], | ||
| "priority": 0, | ||
| "name": "sql", | ||
| "title": "SQL Query" | ||
| } | ||
| }, |
There was a problem hiding this comment.
Security concern: SQL Query action needs additional safeguards
The SQL Query action allows direct SQL execution which could pose security risks. Consider:
- Adding validation constraints in the defaults
- Including a warning about SQL injection risks
- Adding access restrictions similar to other sensitive actions
"name": "sql",
"title": "SQL Query",
"description": "Allows you to execute a remote SQL Query.",
"priority": 0,
"defaults": {
"handler": ["after"],
"method": ["create"],
"priority": 0,
"name": "sql",
"title": "SQL Query"
- }
+ },
+ "access": {
+ "handler": false,
+ "method": false
+ },
+ "security": {
+ "requiresAdmin": true
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "name": "sql", | |
| "title": "SQL Query", | |
| "description": "Allows you to execute a remote SQL Query.", | |
| "priority": 0, | |
| "defaults": { | |
| "handler": ["after"], | |
| "method": ["create"], | |
| "priority": 0, | |
| "name": "sql", | |
| "title": "SQL Query" | |
| } | |
| }, | |
| "name": "sql", | |
| "title": "SQL Query", | |
| "description": "Allows you to execute a remote SQL Query.", | |
| "priority": 0, | |
| "defaults": { | |
| "handler": ["after"], | |
| "method": ["create"], | |
| "priority": 0, | |
| "name": "sql", | |
| "title": "SQL Query" | |
| }, | |
| "access": { | |
| "handler": false, | |
| "method": false | |
| }, | |
| "security": { | |
| "requiresAdmin": true | |
| } | |
| }, |
| "description": "Provides OAuth authentication behavior to this form.", | ||
| "priority": 20, | ||
| "defaults": { | ||
| "handler": ["after"], | ||
| "method": ["form", "create"], | ||
| "priority": 20, | ||
| "name": "oauth", | ||
| "title": "OAuth (Premium)" | ||
| }, | ||
| "premium": true | ||
| }, | ||
| { | ||
| "name": "ldap", | ||
| "title": "LDAP Login (Premium)", | ||
| "description": "Provides ldap login.", | ||
| "priority": 3, | ||
| "defaults": { | ||
| "handler": ["before"], | ||
| "method": ["create"], | ||
| "priority": 3, | ||
| "name": "ldap", | ||
| "title": "LDAP Login (Premium)" | ||
| }, | ||
| "premium": true | ||
| }, | ||
| { | ||
| "name": "googlesheet", | ||
| "title": "Google Sheets (Premium)", | ||
| "description": "Allows you to integrate data into Google sheets.", | ||
| "priority": 0, | ||
| "defaults": { | ||
| "handler": ["after"], | ||
| "method": ["create", "update", "delete"], | ||
| "priority": 0, | ||
| "name": "googlesheet", | ||
| "title": "Google Sheets (Premium)" | ||
| }, | ||
| "premium": true | ||
| }, | ||
| { | ||
| "name": "sqlconnector", | ||
| "title": "SQL Connector (Premium)", | ||
| "description": "Allows you to execute a remote SQL Query via Resquel.", | ||
| "priority": 0, | ||
| "defaults": { | ||
| "handler": ["after"], | ||
| "method": ["create", "update", "delete"], | ||
| "priority": 0, | ||
| "name": "sqlconnector", | ||
| "title": "SQL Connector (Premium)" | ||
| }, | ||
| "premium": true | ||
| }, | ||
| { | ||
| "name": "jira", | ||
| "title": "Jira (Premium)", | ||
| "description": "Allows you to create issues within Jira.", | ||
| "priority": 0, | ||
| "defaults": { | ||
| "handler": ["after"], | ||
| "method": ["create", "update", "delete"], | ||
| "priority": 0, | ||
| "name": "jira", | ||
| "title": "Jira (Premium)" | ||
| }, | ||
| "premium": true | ||
| }, | ||
| { | ||
| "name": "group", | ||
| "title": "Group Assignment (Premium)", | ||
| "premium": true, | ||
| "description": "Provides the Group Assignment capabilities.", | ||
| "priority": 5, | ||
| "defaults": { | ||
| "handler": ["after"], | ||
| "method": ["create", "update", "delete"], | ||
| "priority": 5, | ||
| "name": "group", | ||
| "title": "Group Assignment (Premium)" | ||
| }, | ||
| "access": { | ||
| "handler": false, | ||
| "method": false | ||
| } | ||
| }, | ||
| { | ||
| "name": "twilioSMS", | ||
| "title": "Twilio SMS (Premium)", | ||
| "premium": true, | ||
| "description": "Allows you to send SMS to phone numbers.", | ||
| "priority": 0, | ||
| "defaults": { | ||
| "handler": ["after"], | ||
| "method": ["create"], | ||
| "priority": 0, | ||
| "name": "twilioSMS", | ||
| "title": "Twilio SMS (Premium)" | ||
| } | ||
| } | ||
| ] |
There was a problem hiding this comment.
💡 Codebase verification
Based on the analysis of the codebase, I can now provide a final response:
Premium features are only marked in configuration without runtime validation
The form-actions.json file only contains premium feature flags in the configuration, but there's no evidence of actual runtime validation or access control implementation in the codebase. The ActionSchema interface and related components don't show any handling of the premium status of actions.
- The
premium: trueflag exists in form-actions.json for several actions (webhook, office365, hubspot, oauth, etc.) - The ActionSchema interface in
packages/react-formio/src/interfaces/ActionSchema.tsdoesn't enforce any premium-related validation - The ActionsTable and FormAction components don't implement any premium feature access control
- No error handling or validation logic was found related to premium feature access
🔗 Analysis chain
Verify premium feature access control implementation
The configuration defines several premium features. Ensure that the application code properly enforces these restrictions and that there's appropriate error handling when non-premium users attempt to access these features.
Let me gather more information to check how these premium features are used in the codebase.
Let me check the actual implementation of the actions table and form action components.
Let me try one more search with different file extensions since tsx wasn't recognized.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for premium feature access control implementation
# Look for premium feature checks in the codebase
echo "Searching for premium feature validation..."
rg -A 3 "premium.*true"
# Look for related error handling
echo "Checking error handling for premium features..."
ast-grep --pattern 'if ($_.premium) {
$$$
}'
Length of output: 4050
Script:
#!/bin/bash
# Search for files that import or use the form actions
echo "Searching for files using form actions..."
rg -l "form-actions.json"
# Search for premium-related code
echo "Searching for premium-related implementations..."
rg -A 5 -B 2 "premium" --type ts --type tsx --type js --type jsx
# Look for action handlers
echo "Searching for action handlers..."
rg -A 5 "handler.*\["
Length of output: 11231
Script:
#!/bin/bash
# Check implementation of actions table and form action components
echo "Checking actions table implementation..."
rg -A 10 "export.*ActionsTable" --type tsx
echo "Checking form action component implementation..."
rg -A 10 "export.*FormAction" --type tsx
# Look for any premium feature handling logic
echo "Checking premium feature handling..."
rg -A 5 "premium.*=.*" --type tsx
Length of output: 488
Script:
#!/bin/bash
# Check implementation files with various extensions
echo "Checking component implementations..."
rg -A 10 "export.*ActionsTable|export.*FormAction" --type js --type jsx --type ts
# Look for any premium feature handling in JavaScript/TypeScript files
echo "Checking premium feature handling..."
rg -A 5 "premium" --type js --type jsx --type ts
# Check for any action-related types or interfaces
echo "Checking action-related types..."
rg -A 5 "interface.*Action|type.*Action" --type ts
Length of output: 23145
Romakita
force-pushed
the
feat-update-deps
branch
4 times, most recently
from
34298e9 to
747bc06
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (15)
chromatic.config.json (1)
3-3: Consider using an environment variable for the project IDHardcoding the Chromatic project ID in the configuration file could lead to issues when deploying to different environments. Consider using an environment variable instead.
{ "onlyChanged": true, - "projectId": "Project:676abc3ef2ee519da9b21fb6", + "projectId": "${CHROMATIC_PROJECT_ID}", "storybookBaseDir": "packages/storybook", "zip": true }tsconfig.json (1)
7-9: Consider documenting the purpose of Node.js configurationWhile the change is structurally correct, it would be helpful to document why Node.js-specific configuration was introduced and what settings it contains, either in PR description or in the configuration files themselves.
packages/react-formio/src/components/actions-table/actionsTable.component.spec.tsx (7)
5-7: Use concrete types overanyfor imports and data structures.
Declaringargsasanywill reduce type safety. Consider specifying a more accurate interface or type forargs, ensuring stronger guarantees and better maintainability moving forward.
42-42: Avoid re-rendering the component within each test block if not strictly necessary.
Frequent re-renders can increase test duration and complexity. If the initial state and props are consistent, consider reusing the rendered instance to simplify tests and reduce overhead.
44-44: Use more descriptive test ID references.
action-table-addis functional, but a more descriptive data-testid can make tests clearer (e.g.,action-table-add-button).
59-59: DRY principle in test setup.
Repeated rendering calls with the same arguments can be refactored into abeforeEachblock or helper function, reducing duplication.
61-61: Consolidate repeated variables.
const btn = screen.getByTestId("action-table-add")appears multiple times across tests. DRY up by extracting these repeated references if feasible.
69-69: Consider combining “render” calls into a single reusable function.
Rendering with repeated props is repeated several times. A custom test utility function could improve maintainability and readability.
71-71: Break up logically distinct steps in test for clarity.
Reading more than one reference in a single test can complicate test logic. Splitting the reference assignment from the usage can make debugging simpler.packages/react-formio/src/components/actions-table/actionsTable.stories.tsx (4)
42-42: Use the “satisfies” operator carefully.
While helpful to ensureMetaconformance, be mindful that changes toActionsTablemight break type compliance unexpectedly if strict checks are not maintained.
44-44: Ensure type alias readability.
type Story = StoryObj<typeof ActionsTable>;is concise, but ensure fellow developers can easily follow the abstraction in context, e.g., by standardizing naming or doc comments.
46-82: Use consistent naming and test steps in Storybook “play” functions.
The asynchronousplayfunction is great for simulating user interactions, but ensure method names likeeditButton,deleteButtonfollow a consistent pattern for clarity.
83-112: Encapsulate pre-processed logic.
MappingavailableActions.map(({ name, title }) => ({ label: title, value: name }))is practical, but consider extracting into a helper if reused in tests or other stories, improving maintainability.vite.config.mts (1)
6-6: Consider removing// @ts-ignoreif not strictly necessary.
If there's no TypeScript error, please remove the@ts-ignoredirective for cleaner code. If a type error persists, consider providing a clearer type definition.stories/FormEdit.mdx (1)
2-2: Consider documenting the project restructuringThe consistent movement of components under the
packagesdirectory suggests a shift towards a monorepo structure. Consider:
- Updating the project's README to reflect this new structure
- Adding a MIGRATION.md if this is a breaking change
- Updating any CI/CD pipelines to account for the new paths
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
.storybook/styles/fonts/bxicons/boxicons.eotis excluded by!**/*.eot.storybook/styles/fonts/bxicons/boxicons.svgis excluded by!**/*.svg.storybook/styles/fonts/bxicons/boxicons.ttfis excluded by!**/*.ttf.storybook/styles/fonts/bxicons/boxicons.woffis excluded by!**/*.woff.storybook/styles/fonts/bxicons/boxicons.woff2is excluded by!**/*.woff2yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (31)
.github/workflows/build.yml(1 hunks).github/workflows/chromatic.yml(1 hunks).github/workflows/website.yml(1 hunks).gitignore(1 hunks).storybook/main.ts(1 hunks).storybook/styles/index.css(1 hunks)chromatic.config.json(1 hunks)lint-staged.config.js(0 hunks)package.json(7 hunks)packages/config/package.json(1 hunks)packages/react-formio-container/package.json(1 hunks)packages/react-formio-stores/package.json(2 hunks)packages/react-formio/package.json(3 hunks)packages/react-formio/src/components/actions-table/__fixtures__/data.json(1 hunks)packages/react-formio/src/components/actions-table/actionsTable.component.spec.tsx(1 hunks)packages/react-formio/src/components/actions-table/actionsTable.stories.tsx(2 hunks)packages/redux-utils/package.json(1 hunks)packages/storybook/.eslintignore(0 hunks)packages/storybook/.storybook/styles/index.css(0 hunks)packages/storybook/package.json(0 hunks)packages/tailwind-formio/package.json(1 hunks)packages/tailwind/package.json(1 hunks)stories/Form.mdx(1 hunks)stories/FormBuilder.mdx(1 hunks)stories/FormEdit.mdx(1 hunks)stories/FormTable.mdx(1 hunks)stories/Getting-started.mdx(1 hunks)stories/SubmissionsTable.mdx(1 hunks)tsconfig.json(1 hunks)tsconfig.node.json(1 hunks)vite.config.mts(1 hunks)
💤 Files with no reviewable changes (4)
- lint-staged.config.js
- packages/storybook/.storybook/styles/index.css
- packages/storybook/package.json
- packages/storybook/.eslintignore
✅ Files skipped from review due to trivial changes (5)
- .gitignore
- tsconfig.node.json
- .storybook/styles/index.css
- stories/Form.mdx
- stories/FormBuilder.mdx
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/config/package.json
- packages/redux-utils/package.json
- packages/tailwind-formio/package.json
- packages/tailwind/package.json
- packages/react-formio-stores/package.json
- .github/workflows/chromatic.yml
- packages/react-formio-container/package.json
- packages/react-formio/package.json
🔇 Additional comments (25)
chromatic.config.json (1)
4-4: Verify the Storybook directory structure
The configuration assumes Storybook files are located in packages/storybook. Let's verify this directory structure exists and contains the necessary Storybook files.
tsconfig.json (1)
7-9: Verify the existence of tsconfig.node.json
The addition of this reference suggests a new Node.js-specific TypeScript configuration has been introduced. Let's verify its existence and configuration.
✅ Verification successful
Let me verify the content of the other TypeScript configurations to ensure this reference is consistently used across the monorepo.
Let me check if each package has its own tsconfig.node.json since the references in package configs are using relative paths.
The tsconfig.node.json reference is correctly configured
The verification shows that:
- The root
tsconfig.node.jsonexists and is properly configured for Node.js-specific settings - Each package that references
tsconfig.node.jsonhas its own local version of the file - The configuration is consistently used across the monorepo packages
- The references are correctly positioned in the TypeScript project references array
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and basic structure of tsconfig.node.json
# Check if the file exists
if [ -f "tsconfig.node.json" ]; then
echo "File exists. Checking content:"
cat tsconfig.node.json
else
echo "Warning: tsconfig.node.json not found"
fi
# Look for any other TypeScript configs that might reference this file
rg -l "tsconfig.node.json" --type json
Length of output: 566
Script:
#!/bin/bash
# Check the content of all tsconfig files that reference tsconfig.node.json
for file in tsconfig.json packages/*/tsconfig.json; do
if grep -q "tsconfig.node.json" "$file"; then
echo "=== Content of $file ==="
cat "$file"
echo
fi
done
Length of output: 2878
Script:
#!/bin/bash
# Check for tsconfig.node.json files in packages
echo "=== Root tsconfig.node.json ==="
cat tsconfig.node.json
echo -e "\n=== Checking packages for tsconfig.node.json ==="
for dir in packages/*/; do
if [ -f "${dir}tsconfig.node.json" ]; then
echo -e "\nFound in ${dir}:"
cat "${dir}tsconfig.node.json"
fi
done
Length of output: 2837
packages/react-formio/src/components/actions-table/actionsTable.component.spec.tsx (5)
9-36: Ensure consistent naming and property handling in test fixtures.
The new object structure for args is a good approach, but confirm that the names (data, availableActions, operations) match the actual props expected by the component. Any mismatch could cause runtime or type errors.
51-51: Validate array length checks to avoid off-by-one errors.
When verifying options.length, ensure literal numbers aren’t more suitable, especially if available actions change in the future or differ across environments.
54-54: Maintain consistent labeling in fixtures and test expectations.
Changes like "Webhook (Premium)" are reflected here, but be mindful if fixture data changes again—it’s easy for label mismatches to break tests.
74-74: Double check string conversions in selectOptions.
You’re calling String(args.availableActions[1].value). Ensure that args.availableActions[1].value always has a valid string-like type or confirm it remains consistent.
79-79: Confirm test coverage for all “onAddAction” outcomes.
Currently, you test the “sql” case. If multiple distinct actions or branches exist, consider adding separate coverage for those other cases as well.
packages/react-formio/src/components/actions-table/__fixtures__/data.json (1)
1-12: Validate fixture structure to match production usage.
This new fixture includes fields like _id and machineName, which might affect production logic. Double-check that all properties are necessary and properly handled in associated components.
packages/react-formio/src/components/actions-table/actionsTable.stories.tsx (2)
1-2: Import the correct Storybook types and ensure they match the component’s props.
Inconsistent usage of Meta, StoryObj, or other type exports can cause type inference issues. Confirm that ActionsTable is typed correctly elsewhere in the project.
4-5: Keep fixture and data sources in sync.
availableActions and data are imported from JSON fixtures. Confirm the data structure aligns with your production environment or story needs to avoid misrepresenting real scenarios.
vite.config.mts (2)
5-5: Configuration setup looks good overall.
Kudos on using defineConfig for clearer Vite configuration.
9-11: Confirm environment variable usage.
"process.env.NODE_ENV": "'${process.env.NODE_ENV}'" can override or conflict with actual runtime environment variables. Make sure this behavior aligns with your deployment environment requirements.
.storybook/main.ts (1)
3-3: Storybook config successfully typesafe.
Using StorybookConfig interface helps ensure correct configuration and future maintainability.
.github/workflows/website.yml (1)
48-48: CNAME configuration mismatch detected
The domain is still set to formio.tsed.dev. The past review recommended switching to formio.tsed.io. If you intend to use formio.tsed.io, please update the CNAME accordingly.
- echo "formio.tsed.dev" > ./storybook-static/CNAME
+ echo "formio.tsed.io" > ./storybook-static/CNAME.github/workflows/build.yml (2)
9-10: Ensure branch naming matches your repository's default.
The workflow triggers on master. If your default branch is main, update these lines to reflect the correct branch name.
66-66: Deploy logic looks consistent, but consider verifying environment references.
Confirm that all referenced secrets (GH_TOKEN, NPM_TOKEN, SLACK_WEBHOOK) are properly set, especially if you’re deploying from multiple branches (master, alpha, beta, rc).
package.json (4)
27-38: LGTM: Script changes align with Storybook v8 migration
The script changes properly handle:
- Storybook v8 commands
- Integration with Tailwind builds
- Test runner configuration
- Development and CI workflows
159-179: LGTM: Well-structured configuration additions
The new configurations are well-structured:
- Browserslist targets are reasonable for both production and development
- Lint-staged configuration properly handles different file types
- Package manager version is explicitly specified
69-79: Verify Vite and Storybook configuration
The addition of Vite and Storybook v8 requires proper configuration files.
Run the following script to check for required configuration files:
Also applies to: 94-94, 137-138
✅ Verification successful
Let me check the content of these configuration files to ensure they are properly configured for Storybook v8 and Vite.
Storybook and Vite configurations are properly set up ✅
The configuration files are present and correctly configured for Storybook v8 with Vite:
-
.storybook/main.tshas:- Correct type imports from "@storybook/react-vite"
- All added addons are properly configured
- Framework correctly set to "@storybook/react-vite"
- Proper TypeScript and documentation settings
-
.storybook/preview.tshas:- Correct type imports
- Proper viewport configuration
- Required parameters for actions and controls
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for Storybook config
fd -t f "main\.(js|ts)" .storybook
# Check for Vite config
fd -t f "vite\.config\.(js|ts)"
# Verify Storybook addons are properly configured
fd -t f "preview\.(js|ts)" .storybook
Length of output: 151
Script:
#!/bin/bash
# Check Storybook main config content
cat .storybook/main.ts
# Check Storybook preview config content
cat .storybook/preview.ts
Length of output: 1523
7-7: Verify the impact of ES modules migration
Adding "type": "module" changes the default module system to ES modules. This is a breaking change that affects how imports work across the project.
Run the following script to check for potential CommonJS usage that might break:
✅ Verification successful
Let's get more context about the module system usage in the codebase to make a better assessment. We'll:
- See the actual CommonJS usage with context
- Check for explicit module system file extensions
- Look for ES module import statements
ES modules migration is safe to proceed
Based on the analysis of the codebase:
- The CommonJS usage is properly isolated to configuration files (
.cjsextension) and build tools:- Configuration files:
tailwind.config.cjs,postcss.config.cjs,commitlint.config.cjs - Build utilities:
jest.config.js,gulpfile.js, etc.
- Configuration files:
- The source code in
packages/*/srcdirectories already uses ES modules syntax extensively, as evidenced by the numerousimportstatements found. - The project correctly uses
.cjsextension for files that need to remain in CommonJS format.
The addition of "type": "module" in package.json is appropriate and aligns with the existing codebase structure. The configuration files that need CommonJS are properly marked with .cjs extension, while the source code is already using ES module syntax.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for CommonJS require/exports usage
rg "(?:require\(|module\.exports|exports\.)" -g '!{node_modules,dist}/**' -l
# Search for .js files with potential CommonJS syntax
ast-grep --pattern 'module.exports = $_' -l
Length of output: 936
Script:
#!/bin/bash
# Search for CommonJS require/exports usage with context
rg "(?:require\(|module\.exports|exports\.)" -g '!{node_modules,dist}/**' -A 3
# Check for .mjs or .cjs file extensions
fd "\.(mjs|cjs)$"
# Check for import statements to understand ES module usage
rg "^import\s+.*from\s+" -g "*.{js,jsx,ts,tsx}" -l
Length of output: 26711
stories/FormTable.mdx (1)
2-2: Verify the new import path structure
The import path has been updated to reflect the new project structure under packages. Let's ensure this restructuring is consistent across the project.
✅ Verification successful
Import path structure is consistent across stories
The new import path structure using ../packages/react-formio/src/components/ is consistently used across all story files in the project:
- FormTable.mdx
- SubmissionsTable.mdx
- FormBuilder.mdx
- FormEdit.mdx
- Form.mdx
- Getting-started.mdx
Additionally, we verified that the target file formsTable.stories.tsx exists at the specified location. The import path change aligns with the project's structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and consistency of the new path structure
# Check if the new path exists
fd --type f "formsTable.stories" packages
# Check for any remaining old-style imports that might have been missed
rg -l "from '\.\./\.\./react-formio/" stories/
Length of output: 165
Script:
#!/bin/bash
# Let's verify the import patterns in stories directory and check for consistency
rg -l "from.*react-formio" stories/
# Check all story imports to understand the common pattern
rg "^import.*from" stories/FormTable.mdx
# Let's also verify if there are other similar imports in the codebase
rg "from.*packages/react-formio" .
Length of output: 1213
stories/SubmissionsTable.mdx (1)
2-2: LGTM! Import path update is consistent
The import path update follows the same pattern as other story files, maintaining consistency with the project's new structure.
stories/FormEdit.mdx (1)
2-2: Verify Storybook navigation links
While the import path update is consistent, please verify that the story links in the documentation still work with the new structure:
/story/reactformio-formedit--sandbox/docs/documentation-formbuilder--docs
✅ Verification successful
Let me try another approach to verify the story links:
Story links are consistent with the new structure
Based on the verification results, I can confirm that:
- The story links in FormEdit.mdx are correctly structured and consistent with other MDX files in the codebase
- The specific links mentioned in the review:
/story/reactformio-formedit--sandbox/docs/documentation-formbuilder--docs
are both present and follow the same pattern used across all documentation files (Getting-started.mdx, FormBuilder.mdx, FormTable.mdx)
- The links align with the new import structure, where components are imported from the
packages/react-formio/src/components/directory
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any broken story links in MDX files
# Look for story links in MDX files
rg -l "\/story\/|\/docs\/" stories/
Length of output: 122
Script:
#!/bin/bash
# Description: Check for any broken story links in MDX files using proper regex escaping
# Look for story links in MDX files
rg -l "/story/|/docs/" stories/
# Show the actual content with context for better analysis
rg -A 2 "/story/|/docs/" stories/
Length of output: 2634
Script:
#!/bin/bash
# Check the actual content of FormEdit.mdx
cat stories/FormEdit.mdx
Length of output: 1585
stories/Getting-started.mdx (2)
7-8: LGTM: Image size adjustment improves visibility
The increased width of the Ts.ED logo from 150 to 200 pixels improves its visibility while maintaining the layout.
1-2: Verify import path consistency across story files
The import path has been updated to reflect the new project structure under packages/. Let's ensure this change is consistent across all story files.
✅ Verification successful
Import paths are consistently using the new structure
The verification shows that all story files in the stories/ directory are using the new import path structure (../packages/react-formio):
- stories/SubmissionsTable.mdx
- stories/Getting-started.mdx
- stories/FormEdit.mdx
- stories/Form.mdx
- stories/FormTable.mdx
- stories/FormBuilder.mdx
No files were found using the old import path structure (../../react-formio), confirming that the changes are consistent across all story files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if all story files use the new import path structure
echo "Checking story files for import consistency..."
rg -l "from '\.\./packages/react-formio" stories/
rg -l "from '\.\./\.\./react-formio" stories/
# This should show all story files using the new structure
# and hopefully no files using the old structure
Length of output: 349
| addons: [ | ||
| "@storybook/addon-a11y", | ||
| "@storybook/addon-links", | ||
| "@storybook/addon-interactions", | ||
| "@storybook/addon-essentials", | ||
| "@storybook/addon-mdx-gfm", | ||
| "@storybook/addon-links", | ||
| "@storybook/addon-interactions", | ||
| "@chromatic-com/storybook" | ||
| ], |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate addons to avoid potential conflicts.
@storybook/addon-links and @storybook/addon-interactions both appear twice (lines 16 & 20, 17 & 21). Consider removing duplicates to prevent confusion.
addons: [
"@storybook/addon-a11y",
"@storybook/addon-links",
"@storybook/addon-interactions",
"@storybook/addon-essentials",
"@storybook/addon-mdx-gfm",
- "@storybook/addon-links",
- "@storybook/addon-interactions",
"@chromatic-com/storybook"
],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| addons: [ | |
| "@storybook/addon-a11y", | |
| "@storybook/addon-links", | |
| "@storybook/addon-interactions", | |
| "@storybook/addon-essentials", | |
| "@storybook/addon-mdx-gfm", | |
| "@storybook/addon-links", | |
| "@storybook/addon-interactions", | |
| "@chromatic-com/storybook" | |
| ], | |
| addons: [ | |
| "@storybook/addon-a11y", | |
| "@storybook/addon-links", | |
| "@storybook/addon-interactions", | |
| "@storybook/addon-essentials", | |
| "@storybook/addon-mdx-gfm", | |
| "@chromatic-com/storybook" | |
| ], |
| "react": "^18.2.0", | ||
| "react-dom": "^18.2.0", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate React dependencies
React and ReactDOM are listed in both dependencies and devDependencies. This could lead to multiple versions being installed.
Remove these entries from devDependencies since they're already in dependencies:
- "react": "^18.2.0",
- "react-dom": "^18.2.0",
Romakita
force-pushed
the
feat-update-deps
branch
from
747bc06 to
eddb63e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
.storybook/main.ts (1)
14-23:⚠️ Potential issueRemove duplicate addons to avoid potential conflicts
The addons
@storybook/addon-linksand@storybook/addon-interactionsare listed twice. This could lead to initialization conflicts or unexpected behavior.Apply this diff to remove the duplicates:
addons: [ "@storybook/addon-a11y", "@storybook/addon-links", "@storybook/addon-interactions", "@storybook/addon-essentials", "@storybook/addon-mdx-gfm", - "@storybook/addon-links", - "@storybook/addon-interactions", "@chromatic-com/storybook" ],.github/workflows/website.yml (1)
46-48:⚠️ Potential issueCNAME configuration mismatch detected
The CNAME value in the workflow file (
formio.tsed.dev) doesn't match the configured GitHub Pages domain (formio.tsed.io). This mismatch could cause GitHub Pages deployment issues.Apply this diff to fix the CNAME:
yarn storybook:build touch ./storybook-static/.nojekyll - echo "formio.tsed.dev" > ./storybook-static/CNAME + echo "formio.tsed.io" > ./storybook-static/CNAME
🧹 Nitpick comments (3)
.github/workflows/build.yml (1)
42-62: Consider adjusting Storybook test timeout and server configurationThe Storybook testing setup has a few potential improvements:
- The 60-minute timeout might be excessive for most test runs
- The http-server doesn't have a fallback port if 6006 is in use
Consider these improvements:
test-storybook: - timeout-minutes: 60 + timeout-minutes: 30 runs-on: ubuntu-latest steps: # ... existing steps ... - name: Serve Storybook and run tests 🔍 run: | npx concurrently -k -s first -n "SB,TEST" -c "magenta,blue" \ - "npx http-server storybook-static --port 6006 --silent" \ + "npx http-server storybook-static --port 6006 --silent --fallback-port" \ "npx wait-on tcp:6006 && yarn storybook:test:ci"packages/react-formio/src/components/actions-table/actionsTable.component.spec.tsx (2)
9-9: Consider replacing 'any' with a proper type definitionUsing
anyreduces type safety. Consider defining an interface for the args object.-const args: any = { +interface ActionTableArgs { + data: typeof data; + availableActions: Array<{label: string; value: string}>; + operations: Array<{ + title?: string; + action: string; + alias?: string; + path: string; + icon: string; + buttonType?: string; + permissionsResolver: () => boolean; + }>; +} +const args: ActionTableArgs = {
41-81: Consider adding more test coverageThe test suite could benefit from additional test cases:
- Error handling scenarios
- Loading states
- Edge cases with empty or invalid data
- Permission denied scenarios
Would you like me to provide examples of these additional test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
.storybook/styles/fonts/bxicons/boxicons.eotis excluded by!**/*.eot.storybook/styles/fonts/bxicons/boxicons.svgis excluded by!**/*.svg.storybook/styles/fonts/bxicons/boxicons.ttfis excluded by!**/*.ttf.storybook/styles/fonts/bxicons/boxicons.woffis excluded by!**/*.woff.storybook/styles/fonts/bxicons/boxicons.woff2is excluded by!**/*.woff2yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (31)
.github/workflows/build.yml(1 hunks).github/workflows/chromatic.yml(1 hunks).github/workflows/website.yml(1 hunks).gitignore(1 hunks).storybook/main.ts(1 hunks).storybook/styles/index.css(1 hunks)chromatic.config.json(1 hunks)lint-staged.config.js(0 hunks)package.json(7 hunks)packages/config/package.json(1 hunks)packages/react-formio-container/package.json(1 hunks)packages/react-formio-stores/package.json(2 hunks)packages/react-formio/package.json(3 hunks)packages/react-formio/src/components/actions-table/__fixtures__/data.json(1 hunks)packages/react-formio/src/components/actions-table/actionsTable.component.spec.tsx(1 hunks)packages/react-formio/src/components/actions-table/actionsTable.stories.tsx(2 hunks)packages/redux-utils/package.json(1 hunks)packages/storybook/.eslintignore(0 hunks)packages/storybook/.storybook/styles/index.css(0 hunks)packages/storybook/package.json(0 hunks)packages/tailwind-formio/package.json(1 hunks)packages/tailwind/package.json(1 hunks)stories/Form.mdx(1 hunks)stories/FormBuilder.mdx(1 hunks)stories/FormEdit.mdx(1 hunks)stories/FormTable.mdx(1 hunks)stories/Getting-started.mdx(1 hunks)stories/SubmissionsTable.mdx(1 hunks)tsconfig.json(1 hunks)tsconfig.node.json(1 hunks)vite.config.mts(1 hunks)
💤 Files with no reviewable changes (4)
- packages/storybook/.eslintignore
- lint-staged.config.js
- packages/storybook/.storybook/styles/index.css
- packages/storybook/package.json
🚧 Files skipped from review as they are similar to previous changes (23)
- packages/tailwind-formio/package.json
- packages/config/package.json
- vite.config.mts
- .storybook/styles/index.css
- .gitignore
- stories/FormTable.mdx
- packages/redux-utils/package.json
- tsconfig.json
- chromatic.config.json
- packages/react-formio-container/package.json
- tsconfig.node.json
- stories/Form.mdx
- stories/Getting-started.mdx
- stories/FormEdit.mdx
- packages/react-formio-stores/package.json
- packages/tailwind/package.json
- stories/SubmissionsTable.mdx
- stories/FormBuilder.mdx
- .github/workflows/chromatic.yml
- packages/react-formio/src/components/actions-table/fixtures/data.json
- packages/react-formio/package.json
- packages/react-formio/src/components/actions-table/actionsTable.stories.tsx
- package.json
🔇 Additional comments (5)
.github/workflows/website.yml (1)
35-39: Ensure consistent Node.js setup across workflows
The Node.js setup matches the build workflow, using version 20 and Yarn cache. This is good for consistency.
.github/workflows/build.yml (1)
64-84: LGTM: Well-structured deployment job
The deployment job is well configured with:
- Proper job dependencies ensuring quality gates
- Correct conditional execution for release branches
- Required secrets for publishing
packages/react-formio/src/components/actions-table/actionsTable.component.spec.tsx (3)
5-7: LGTM! Good practice using external fixtures
Moving test data to external fixtures improves maintainability and reusability of test data.
44-51: LGTM! Good test structure and assertions
The test properly verifies the component's initial state and rendering. Using data-testid for button selection is a good practice.
74-79: Verify the expected action value
The test expects the action value to be "sql", but this value isn't immediately clear from the test setup. Consider making this expectation more explicit in the test setup or add a comment explaining the expected value.
✅ Verification successful
The test expectation is correct and clear from the context
Looking at the code and fixtures:
- The test uses
form-actions.jsonfixture which clearly shows "sql" as the second action in the list (index 1) - The test selects
args.availableActions[1].valuewhich maps to this "sql" action becauseavailableActionsis created by mapping over the fixture array - The expectation
toHaveBeenCalledWith("sql")correctly matches the selected action's name
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the action value in the fixtures
echo "Checking form-actions.json for the sql action:"
jq '.[] | select(.name=="sql")' "../__fixtures__/form-actions.json"
Length of output: 262
Script:
#!/bin/bash
# Let's first find the fixtures directory and any form-actions related files
fd "form-actions" --type f
# Then let's look for the test file and its context
rg -B10 -A5 'expect\(.*\)\.toHaveBeenCalledWith\("sql"\)' --type ts
Length of output: 1650
Script:
#!/bin/bash
# Let's check the content of the form-actions fixture to understand the available actions
cat packages/react-formio/src/components/__fixtures__/form-actions.json
# Let's also look at the test setup to see how args.availableActions is defined
rg -B20 "availableActions" packages/react-formio/src/components/actions-table/actionsTable.component.spec.tsx
Length of output: 7835
| import type {StorybookConfig} from "@storybook/react-vite"; | ||
|
|
||
| const config: StorybookConfig = { | ||
| staticDirs: ["../packages/tailwind/build"], |
There was a problem hiding this comment.
💡 Codebase verification
The Tailwind build directory path needs to be updated
The current path ../packages/tailwind/build in .storybook/main.ts is incorrect. Based on the package.json scripts and build configuration:
- The Tailwind package has a
tailwind:buildscript that outputs tobuild/tailwind.css - However, this build directory is relative to the package's root at
packages/tailwind/ - The current path in Storybook config uses
../packages/tailwind/buildwhich won't resolve correctly
The path should be updated to:
./packages/tailwind/build(relative to the project root)
🔗 Analysis chain
Verify the Tailwind build output path
Please ensure the static directory path ../packages/tailwind/build exists and contains the required Tailwind CSS build output.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the Tailwind build directory exists
fd -t d "tailwind/build$" packages/
Length of output: 37
Script:
#!/bin/bash
# Let's check if the tailwind package and its build directory structure exists
fd "tailwind" packages/
# Also check for any potential build or dist directories that might contain Tailwind output
fd "build|dist" packages/
# Let's also look for package.json files in tailwind-related directories to understand the build setup
fd "package.json" packages/ -x grep -l "tailwind" {}
Length of output: 1586
Script:
#!/bin/bash
# Let's check the package.json in the tailwind package to understand the build process
cat packages/tailwind/package.json
# Also check if there are any build scripts or configurations
cat packages/tailwind/build-colors.js
# Check the tailwind config file
cat packages/tailwind/tailwind.config.js
Length of output: 8755
| operations: [ | ||
| { | ||
| title: "Edit", | ||
| action: "edit", | ||
| alias: "row", | ||
| path: "/resources/:resourceId/submissions/:submissionId", | ||
| icon: "edit", | ||
| permissionsResolver() { | ||
| return true; | ||
| } | ||
| }, | ||
| { | ||
| action: "delete", | ||
| path: "/resources/:resourceId/submissions/:submissionId/delete", | ||
| icon: "trash", | ||
| buttonType: "danger", | ||
| permissionsResolver() { | ||
| return true; | ||
| } | ||
| } | ||
| ] |
There was a problem hiding this comment.
💡 Codebase verification
Permission testing scenarios need improvement
Based on the codebase analysis, I can confirm both concerns:
-
The operations configuration is indeed duplicated across multiple table components (actions-table, forms-table, submissions-table) and should be extracted to a shared location.
-
The permission testing is inadequate:
- The actual implementation in
defaultCellOperations.component.tsxshows thatpermissionsResolveris used to filter operations based on permissions, but the test files only usereturn trueimplementations - There are no test cases that verify the permission filtering logic
- The test file focuses only on action selection functionality, missing critical permission-based scenarios
Suggested improvements:
- Extract operation configurations to a shared constants/fixtures file
- Add test cases that verify:
- Operations are properly filtered when permissions are denied
- UI elements are hidden/disabled based on permission results
- The permission resolver receives correct data and context arguments
🔗 Analysis chain
Consider extracting operation configurations
The operations array contains sensitive paths and configurations that might be better moved to a separate fixture file or constants.
Additionally, the permissionsResolver functions always return true, which might not adequately test different permission scenarios. Consider adding test cases for denied permissions.
Let me try a different search strategy to verify the permissions testing patterns in the codebase.
Let me check how permissions are actually used in the table components to better understand the testing context.
Let me try one final search to understand the permission handling implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if permissions are properly tested across the codebase
rg -l "permissionsResolver.*return.*true" --type ts
Length of output: 53
Script:
#!/bin/bash
# Search for permissionsResolver patterns more broadly
rg "permissionsResolver" --type ts -A 3
# Also check for any test files related to permissions
fd ".*spec.*" -e ts -e tsx -X rg "permissions"
Length of output: 6566
Script:
#!/bin/bash
# Check the table components implementation for permissions handling
rg -t tsx "permissionsResolver" ./packages/react-formio/src/components/table -A 5
# Check if there are any actual test files that test permission scenarios
fd "test|spec" -e tsx -e ts -X rg "describe.*permission|test.*permission|it.*permission"
Length of output: 200
Script:
#!/bin/bash
# Search for permission-related code in table components
rg "permissionsResolver" ./packages/react-formio/src/components/table -A 5
# Look for any test setup or mock implementations
rg "describe|it" ./packages/react-formio/src/components/actions-table/actionsTable.component.spec.tsx -A 3
Length of output: 4807
Romakita
force-pushed
the
feat-update-deps
branch
from
eddb63e to
4dc812b
Compare
Romakita
force-pushed
the
feat-update-deps
branch
from
4dc812b to
bd5932b
Compare
|
🎉 This PR is included in version 2.3.2 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
.gitignoreto includestorybook-static.Style