feat(widget-cc-station-login): Spark 575845 login widget #239
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces several updates across multiple packages in the 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 (
|
Kesari3008
changed the title
Kesari3008
changed the title
packages/contact-center/station-login/src/station-login/station-login.types.ts
Outdated
Show resolved
Hide resolved
packages/contact-center/station-login/src/station-login/station-login.types.ts
Outdated
Show resolved
Hide resolved
packages/contact-center/station-login/src/station-login/styles.module.scss
Outdated
Show resolved
Hide resolved
packages/contact-center/station-login/src/station-login/styles.module.scss
Outdated
Show resolved
Hide resolved
packages/contact-center/station-login/src/station-login/station-login.presentational.tsx
Outdated
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 22
🧹 Outside diff range and nitpick comments (14)
packages/contact-center/store/src/store.ts (1)
12-17: Consider externalizingwebexConfigfor flexibilityCurrently,
webexConfigis hardcoded within the constructor. For better maintainability and to allow for dynamic configuration (e.g., different environments, testing), consider externalizing this configuration or accepting it as a parameter.packages/contact-center/station-login/src/station-login/station-login.presentational.tsx (1)
6-6: Destructurenamefrompropsfor consistencyCurrently,
nameis accessed viaprops.namein the JSX. For consistency and improved readability, consider destructuringnamefrompropsalong with the other properties.Apply this diff to destructure
name:- const {selectLoginOption, login, logout} = props; + const {selectLoginOption, login, logout, name} = props;And update the usage in your JSX:
- <h1 data-testid="station-login-heading">{props.name}</h1> + <h1 data-testid="station-login-heading">{name}</h1>packages/contact-center/station-login/src/helper.ts (1)
66-66: Remove unnecessary semicolon at the endThere is an extraneous semicolon after the closing brace, which is unnecessary and can be removed for cleaner code.
Apply this diff:
return {name: 'StationLogin', selectLoginOption, login, logout}; -}; +}packages/contact-center/store/src/sdk.ts (1)
7-7: Provide specific type annotations forinitfunction parametersThe parameters
webexConfig,updateTeamsList, andupdateLoginVoiceOptionsare typed asany. Specifying more precise types for these parameters will enhance type safety, improve code readability, and help catch errors during development.packages/contact-center/station-login/webpack.config.js (2)
12-29: SCSS module configuration looks good, but needs essential improvements.The basic setup is correct, but consider these important additions:
- Add source maps for development debugging
- Add PostCSS loader for vendor prefixing
Apply this diff to enhance the configuration:
module: { rules: [ { test: /\.module\.scss$/, use: [ 'style-loader', { loader: 'css-loader', options: { modules: true, + sourceMap: process.env.NODE_ENV === 'development', }, }, + { + loader: 'postcss-loader', + options: { + postcssOptions: { + plugins: [ + 'autoprefixer', + ], + }, + }, + }, 'sass-loader', ], include: path.resolve(__dirname, 'src') }, ], }
12-29: Consider additional optimizations for production.These improvements would enhance the production build:
- Extract CSS into separate files
- Add support for non-modular SCSS files
- Configure SCSS optimization settings
Apply this diff to add production optimizations:
+const MiniCssExtractPlugin = require('mini-css-extract-plugin'); +const isProduction = process.env.NODE_ENV === 'production'; module: { rules: [ { test: /\.module\.scss$/, use: [ - 'style-loader', + isProduction ? MiniCssExtractPlugin.loader : 'style-loader', { loader: 'css-loader', options: { modules: true, + sourceMap: !isProduction, }, }, { loader: 'sass-loader', options: { + sassOptions: { + outputStyle: isProduction ? 'compressed' : 'expanded', + }, }, }, ], include: path.resolve(__dirname, 'src') }, + { + test: /(?<!module)\.scss$/, + use: [ + isProduction ? MiniCssExtractPlugin.loader : 'style-loader', + 'css-loader', + 'sass-loader', + ], + include: path.resolve(__dirname, 'src') + }, ], -} +}, +plugins: [ + ...(isProduction ? [new MiniCssExtractPlugin()] : []), +]packages/contact-center/station-login/src/station-login/index.tsx (1)
Line range hint
1-29: Consider adding error boundaries and loading statesThe component architecture looks good with proper separation of concerns. However, consider adding:
- Error boundaries to handle potential SDK initialization failures
- Loading states while teams and login options are being fetched
- Proper cleanup in the web component lifecycle
This will improve the robustness and user experience of the login widget.
webpack.config.js (1)
7-7: Fix inconsistent extension formatThe extensions array mixes formats with some having dots and others not:
- '.ts', '.tsx', '.js' have dots
- 'jsx' is missing a dot
- extensions: ['.ts', '.tsx', '.js', 'jsx'], + extensions: ['.ts', '.tsx', '.js', '.jsx'],packages/contact-center/user-state/src/user-state/user-state.presentational.tsx (1)
6-6: Use destructured props consistentlyThe
loginStateis destructured from props but then accessed directly viaprops.loginState. For consistency and cleaner code, use the destructured value.const {loginState, handleAgentStatus, setAgentStatus} = props; return ( <> <h1 data-testid="user-state-heading">{props.name}</h1> - <h4>User State: {props.loginState}</h4> + <h4>User State: {loginState}</h4>Also applies to: 11-11
packages/contact-center/station-login/tests/station-login/station-login.presentational.tsx (1)
Line range hint
9-26: Expand test coverage for new functionality.The test only verifies the heading text, missing coverage for the new login-related functionality.
Consider adding these test cases:
describe('StationLoginPresentational', () => { const mockSelectLoginOption = jest.fn(); const mockLogin = jest.fn(); const mockLogout = jest.fn(); const mockTeams = [{ id: '1', name: 'Team 1' }]; const mockLoginOptions = [{ id: '1', label: 'Option 1' }]; const defaultProps = { name: 'StationLogin', loginState: 'idle', setLoginState: jest.fn(), sdk: {}, teams: mockTeams, loginOptions: mockLoginOptions, selectLoginOption: mockSelectLoginOption, login: mockLogin, logout: mockLogout, }; beforeEach(() => { jest.clearAllMocks(); }); it('renders login options', () => { render(<StationLoginPresentational {...defaultProps} />); expect(screen.getByText('Option 1')).toBeInTheDocument(); }); it('calls selectLoginOption when an option is selected', () => { render(<StationLoginPresentational {...defaultProps} />); // Add interaction test based on actual UI }); it('calls login/logout functions appropriately', () => { render(<StationLoginPresentational {...defaultProps} />); // Add interaction tests based on actual UI }); });packages/contact-center/station-login/package.json (1)
32-37: Consider using fixed versions for consistencyThe new loader dependencies use caret ranges (
^) while other dependencies use fixed versions. This inconsistency could lead to unexpected updates and version mismatches.Consider fixing the versions:
- "css-loader": "^7.1.2", + "css-loader": "7.1.2", - "sass-loader": "^16.0.3", + "sass-loader": "16.0.3", - "style-loader": "^4.0.0" + "style-loader": "4.0.0"packages/contact-center/station-login/src/station-login/styles.module.scss (3)
1-6: Consider using CSS variables for colors and consistent unitsFor better maintainability and consistency:
- Use CSS variables for the background color
- Consider using rem for max-width
body { - max-width: 100%; + max-width: 100rem; margin: 1rem; padding: 0; - background: #eeeeee; + background: var(--md-background-color, #eeeeee); }
76-86: Enhance accessibility for collapsible sectionsThe collapsible sections should be enhanced with ARIA attributes for better accessibility. Ensure the corresponding React component implements:
aria-expandedaria-controls- Proper keyboard navigation
200-205: Convert margin-top to rem in arrow classFor consistency with the rest of the codebase:
.arrow { font-weight: 500; font-size: 1.75rem; float: right; - margin-top: -2px; + margin-top: -0.125rem; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (18)
package.json(1 hunks)packages/contact-center/station-login/package.json(1 hunks)packages/contact-center/station-login/src/declaration.d.ts(1 hunks)packages/contact-center/station-login/src/helper.ts(1 hunks)packages/contact-center/station-login/src/station-login/index.tsx(1 hunks)packages/contact-center/station-login/src/station-login/station-login.presentational.tsx(1 hunks)packages/contact-center/station-login/src/station-login/station-login.types.ts(1 hunks)packages/contact-center/station-login/src/station-login/styles.module.scss(1 hunks)packages/contact-center/station-login/tests/station-login/station-login.presentational.tsx(1 hunks)packages/contact-center/station-login/webpack.config.js(1 hunks)packages/contact-center/store/package.json(1 hunks)packages/contact-center/store/src/sdk.ts(1 hunks)packages/contact-center/store/src/store.ts(1 hunks)packages/contact-center/user-state/package.json(1 hunks)packages/contact-center/user-state/src/helper.ts(1 hunks)packages/contact-center/user-state/src/user-state/use-state.types.ts(1 hunks)packages/contact-center/user-state/src/user-state/user-state.presentational.tsx(1 hunks)webpack.config.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/contact-center/station-login/src/declaration.d.ts
🧰 Additional context used
🪛 Gitleaks
packages/contact-center/store/src/sdk.ts
5-5: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
🔇 Additional comments (20)
packages/contact-center/user-state/src/helper.ts (1)
1-10: Verify integration with station login functionality.
Based on the PR objectives, this hook should be related to station login functionality, but the current implementation seems focused only on agent status.
Let's check for related station login implementations:
packages/contact-center/station-login/src/station-login/station-login.types.ts (3)
11-13: Improve type safety and documentation accuracy.
- The
anytype should be replaced with a specific type for better type safety. - The JSDoc comment mentions "Customer Care SDK" but the property is named just "sdk", which could be confusing.
Let's find the actual SDK type:
#!/bin/bash
# Search for SDK type definitions or imports
rg -t ts "import.*SDK" --glob "!*.test.*" --glob "!*.spec.*"
# Search for SDK class/interface definitions
ast-grep --pattern 'interface $SDK {
$$$
}'15-18: Specify the array element type for better type safety.
The empty array type [] should be replaced with a proper typed array like string[] (if these are team ID strings) or a more specific type that represents a team.
Let's find how teams are used in the codebase:
#!/bin/bash
# Search for team type definitions
ast-grep --pattern 'type $Team = {
$$$
}'
# Search for team interface definitions
ast-grep --pattern 'interface $Team {
$$$
}'
# Search for usage of teams prop
rg -t ts "teams.*:" -A 2 --glob "!*.test.*" --glob "!*.spec.*"20-23: Define a type for login options.
The empty array type [] should be replaced with a proper typed array that represents the structure of login options.
Let's find the login options structure:
#!/bin/bash
# Search for login option type definitions
ast-grep --pattern 'type $LoginOption = {
$$$
}'
# Search for usage of loginOptions
rg -t ts "loginOptions.*:" -A 2 --glob "!*.test.*" --glob "!*.spec.*"packages/contact-center/station-login/src/station-login/index.tsx (3)
14-18: Verify presentational component prop types
The presentational component is receiving new props (sdk, teams, loginOptions). Please ensure:
- These props are properly typed in the presentational component's props interface
- The component handles cases where these props might be undefined
10-10: Verify store initialization of new properties
Please ensure that sdk, teams, and loginOptions are properly initialized in the store before being accessed.
✅ Verification successful
Store properties are properly typed and utilized
The verification shows that:
- All three properties (
sdk,teams,loginOptions) are properly defined in the types file with appropriate JSDoc comments - The properties are correctly utilized in both the main component and helper functions
- The
teamsandloginOptionsarrays are properly handled in theuseStationLoginhook with proper type safety - The implementation shows correct initialization and usage patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check store initialization of new properties
# Look for store property definitions and initializations
# Search for store property definitions
ast-grep --pattern 'class $_ {
$$$
sdk: $_
$$$
}'
# Search for teams property
ast-grep --pattern 'class $_ {
$$$
teams: $_
$$$
}'
# Search for loginOptions property
ast-grep --pattern 'class $_ {
$$$
loginOptions: $_
$$$
}'
Length of output: 57
Script:
#!/bin/bash
# Let's try a different approach to find store initialization
# Search for store-related files and their content
# Find store-related files
fd -t f "store" packages/contact-center/station-login/src
# Search for property initialization patterns using ripgrep
rg "sdk|teams|loginOptions" packages/contact-center/station-login/src -A 3 -B 3
Length of output: 7102
11-11: Verify hook parameter handling
The useStationLogin hook now accepts three parameters. Please ensure:
- All parameters are properly typed in the hook definition
- The hook handles cases where any of these parameters might be undefined
webpack.config.js (2)
18-22: Consider security implications of process polyfill
The ProvidePlugin automatically injects the process polyfill, which could expose sensitive environment variables in the browser. Ensure that:
- Only necessary process features are used
- No sensitive data is exposed through process.env
8-16:
Fix formatting and verify polyfill security
-
The fallback configuration has inconsistent spacing after colons (e.g.,
fs:falsevsprocess: require) -
Using polyfills for core Node.js modules in the browser can introduce security risks. Please ensure these are necessary and verify their versions.
Apply this formatting fix:
fallback: {
- fs:false,
+ fs: false,
process: require.resolve('process/browser'),
crypto: require.resolve("crypto-browserify"),
querystring: require.resolve("querystring-es3"),
os: require.resolve("os-browserify/browser"),
stream: require.resolve("stream-browserify"),
vm: require.resolve("vm-browserify")
},✅ Verification successful
Let me verify the package.json to check the actual versions being used in the project and compare them with the latest versions we found.
Fix formatting and update polyfill dependencies
The spacing inconsistency should be fixed as suggested. Regarding security, I've verified that:
-
All polyfill packages are using their latest stable versions:
- crypto-browserify: 3.12.1 (latest)
- querystring-es3: 0.2.1 (latest)
- os-browserify: 0.3.0 (latest)
- stream-browserify: 3.0.0 (latest)
- vm-browserify: 1.1.2 (latest)
-
No known security vulnerabilities were found in any of these polyfills according to GitHub's security advisory database.
Please proceed with the formatting fix:
fallback: {
- fs:false,
+ fs: false,
process: require.resolve('process/browser'),
crypto: require.resolve("crypto-browserify"),
querystring: require.resolve("querystring-es3"),
os: require.resolve("os-browserify/browser"),
stream: require.resolve("stream-browserify"),
vm: require.resolve("vm-browserify")
},🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for known vulnerabilities in the polyfill packages
echo "Checking package versions and vulnerabilities..."
for pkg in "crypto-browserify" "querystring-es3" "os-browserify" "stream-browserify" "vm-browserify"; do
echo "Checking $pkg..."
# Get latest version and security advisories
curl -s "https://registry.npmjs.org/$pkg" | jq '.["dist-tags"].latest'
gh api graphql -f query='{
securityVulnerabilities(first:5, ecosystem:NPM, package:"'"$pkg"'") {
nodes {
advisory {
summary
severity
}
vulnerableVersionRange
}
}
}'
done
Length of output: 2843
Script:
#!/bin/bash
# Check package.json for dependency versions
echo "Checking package.json for polyfill versions..."
cat package.json | jq '.dependencies, .devDependencies' 2>/dev/null || echo "No package.json found"
# Also check package-lock.json or yarn.lock for actual resolved versions
echo -e "\nChecking lock files..."
if [ -f package-lock.json ]; then
echo "Found package-lock.json:"
cat package-lock.json | jq '.dependencies | with_entries(select(.key | test("(crypto-browserify|querystring-es3|os-browserify|stream-browserify|vm-browserify|process/browser)")))' 2>/dev/null
elif [ -f yarn.lock ]; then
echo "Found yarn.lock:"
cat yarn.lock | grep -A 2 -E "(crypto-browserify|querystring-es3|os-browserify|stream-browserify|vm-browserify|process/browser)"
fi
Length of output: 2703
packages/contact-center/user-state/src/user-state/user-state.presentational.tsx (2)
Line range hint 1-24: Verify alignment with station login requirements
The PR objectives mention implementing station login functionality, but this component focuses on agent status management. Please clarify if this is part of the station login flow or if additional components will handle the login functionality.
#!/bin/bash
# Search for station login related components
rg -t typescript -t javascript "StationLogin|station.*login" --no-filename12-19:
Address functional and accessibility concerns in agent status controls
Several issues need attention:
- The "Set Agent Status" button is hardcoded as disabled, making it non-functional
- The dropdown lacks options beyond the placeholder
- Missing accessibility attributes
- Inconsistent spacing classes
<fieldset>
<legend>Agent status</legend>
- <select name="idleCodesDropdown" id="idleCodesDropdown" className="form-control w-auto my-3" onChange={handleAgentStatus}>
- <option value="" selected hidden>Select Idle Codes</option>
+ <select
+ name="idleCodesDropdown"
+ id="idleCodesDropdown"
+ className="form-control w-auto mb-3"
+ onChange={handleAgentStatus}
+ aria-label="Select agent idle status"
+ >
+ <option value="">Select Idle Codes</option>
+ {/* TODO: Add idle code options */}
</select>
- <button id="setAgentStatus" disabled className="btn btn-primary my-3 ml-2" onClick={setAgentStatus}>Set Agent
- Status</button>
+ <button
+ id="setAgentStatus"
+ className="btn btn-primary mb-3 ms-2"
+ onClick={setAgentStatus}
+ disabled={!handleAgentStatus}
+ aria-label="Update agent status"
+ >
+ Set Agent Status
+ </button>
</fieldset>Let's verify if the idle codes are defined elsewhere:
packages/contact-center/store/package.json (2)
14-14: Verify version compatibility between webex and plugin-cc
The webex package (3.6.0-wxcc.3) and @webex/plugin-cc (3.5.0-wxcc.2) have different major versions. This version mismatch could potentially lead to compatibility issues.
#!/bin/bash
# Description: Check compatibility between webex and plugin-cc versions
# Check if there are any known compatibility issues or version constraints
rg -A 5 "peerDependencies.*webex"
rg -A 5 "@webex/plugin-cc.*dependencies"
# Look for version-related issues in commit history
git log -p --grep="wxcc" -- "**/package.json"Also applies to: 19-19
14-14: Verify custom wxcc versions against main releases
The -wxcc suffixed versions (3.5.0-wxcc.2, 3.6.0-wxcc.3) appear to be custom builds. Please ensure these versions:
- Are published and accessible in the registry
- Contain all necessary security patches from their base versions
- Are approved for production use
Also applies to: 19-19
✅ Verification successful
Custom wxcc versions are published and accessible
The verification shows that:
- Both versions (3.5.0-wxcc.2 and 3.6.0-wxcc.3) are published and accessible in the npm registry
- They were published recently (6 hours ago) by the official webex-jenkins maintainer
- No security vulnerabilities were found for the webex package
- The packages are under official Cisco maintenance with proper licensing (Cisco EULA)
The versions appear to be legitimate custom builds maintained by the Webex team specifically for Contact Center (wxcc) usage, as evidenced by:
- Dedicated 'wxcc' dist-tag in both packages
- Official maintainers (webex-jenkins, Cisco team members)
- Proper dependency chain with other wxcc-suffixed packages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the custom wxcc versions
# Check if these versions exist in the registry
npm view @webex/[email protected]
npm view [email protected]
# Look for any security advisories
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "webex") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 3125
packages/contact-center/user-state/package.json (2)
32-32: LGTM! The additions align with the PR objectives
The addition of CSS/SCSS loaders as dev dependencies is appropriate for:
- Supporting UI implementation for the station login widget
- Maintaining consistency with similar updates in the cc-station-login package
- Following the project's webpack build configuration
Also applies to: 36-37
32-32: Verify loader versions for compatibility and security
The added style-related loaders are necessary for UI implementation, but let's verify their versions:
Also applies to: 36-37
✅ Verification successful
Let me verify the webpack version to ensure compatibility before making the final response.
Loader versions are compatible and secure
The added loaders are using their latest stable versions and are fully compatible with webpack 5.94.0:
- [email protected] (latest)
- [email protected] (latest)
- [email protected] (latest)
No security vulnerabilities were found for any of these packages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check compatibility and latest versions of the added loaders
# Check latest versions and compatibility
echo "Checking latest versions from npm registry..."
for pkg in css-loader sass-loader style-loader; do
echo "=== $pkg ==="
curl -s "https://registry.npmjs.org/$pkg" | jq -r '.["dist-tags"].latest'
done
# Check for any security advisories
echo -e "\nChecking for security advisories..."
for pkg in css-loader sass-loader style-loader; do
echo "=== $pkg ==="
gh api graphql -f query='{
securityVulnerabilities(first:5, ecosystem: NPM, package:"'"$pkg"'") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
done
Length of output: 2145
Script:
#!/bin/bash
# Check webpack version in the package
cat packages/contact-center/user-state/package.json | jq -r '.dependencies.webpack // .devDependencies.webpack'
Length of output: 120
packages/contact-center/station-login/package.json (1)
32-37: Verify loader versions compatibility with webpack 5
Let's verify that the specified loader versions are compatible with webpack 5.94.0.
✅ Verification successful
Loader versions are compatible with webpack 5.94.0
The verification shows that all loaders are compatible with the project's webpack version (5.94.0):
- css-loader ^7.1.2 requires webpack ^5.27.0
- sass-loader ^16.0.3 requires webpack ^5.0.0
- style-loader ^4.0.0 requires webpack ^5.27.0
The webpack configuration file also shows these loaders are being used correctly in the module rules.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the compatibility of loaders with webpack 5
# Expected: The specified versions should be compatible with webpack 5
# Check package.json files in node_modules for peer dependency requirements
for pkg in css-loader sass-loader style-loader; do
if [ -f "node_modules/$pkg/package.json" ]; then
echo "Checking $pkg peer dependencies:"
cat "node_modules/$pkg/package.json" | jq '.peerDependencies'
fi
done
# Search for any webpack version requirements in READMEs
for pkg in css-loader sass-loader style-loader; do
if [ -f "node_modules/$pkg/README.md" ]; then
echo "Checking $pkg README for webpack version requirements:"
rg -i "webpack" "node_modules/$pkg/README.md"
fi
done
Length of output: 590
Script:
#!/bin/bash
# Let's try a different approach by checking the package.json for webpack version
cat packages/contact-center/station-login/package.json | jq '.dependencies.webpack, .devDependencies.webpack'
# Let's also check the versions of loaders in package.json
echo "Checking loader versions:"
cat packages/contact-center/station-login/package.json | jq '.dependencies["css-loader"], .dependencies["sass-loader"], .dependencies["style-loader"], .devDependencies["css-loader"], .devDependencies["sass-loader"], .devDependencies["style-loader"]'
# Let's search for any webpack config files that might show loader configurations
echo "Searching for webpack config files:"
fd -e js -e ts -e json webpack.config
Length of output: 901
Script:
#!/bin/bash
# Let's check the webpack config file for the station-login package
cat packages/contact-center/station-login/webpack.config.js
# Let's also check the official compatibility information for these loaders
echo "Checking css-loader compatibility:"
curl -s https://raw.githubusercontent.com/webpack-contrib/css-loader/v7.1.2/package.json | jq '.peerDependencies'
echo "Checking sass-loader compatibility:"
curl -s https://raw.githubusercontent.com/webpack-contrib/sass-loader/v16.0.3/package.json | jq '.peerDependencies'
echo "Checking style-loader compatibility:"
curl -s https://raw.githubusercontent.com/webpack-contrib/style-loader/v4.0.0/package.json | jq '.peerDependencies'
Length of output: 1577
package.json (2)
30-36: 🛠️ Refactor suggestion
Consider moving polyfills to devDependencies
These Node.js core module polyfills (crypto-browserify, os-browserify, process, querystring-es3, stream-browserify, vm-browserify) are typically build-time dependencies used by webpack to enable Node.js modules in the browser. They should be moved to devDependencies since they're not runtime dependencies.
{
"devDependencies": {
"html-webpack-plugin": "^5.6.3",
"node-gyp": "^10.2.0",
"typescript": "^5.6.3",
"webpack": "^5.96.1",
"webpack-cli": "^5.1.4",
"webpack-dev-server": "^5.1.0",
+ "crypto-browserify": "^3.12.1",
+ "os-browserify": "^0.3.0",
+ "process": "^0.11.10",
+ "querystring-es3": "^0.2.1",
+ "stream-browserify": "^3.0.0",
+ "vm-browserify": "^1.1.2"
},
- "dependencies": {
- "crypto-browserify": "^3.12.1",
- "os-browserify": "^0.3.0",
- "process": "^0.11.10",
- "querystring-es3": "^0.2.1",
- "stream-browserify": "^3.0.0",
- "vm-browserify": "^1.1.2"
- }
}30-36: Optimize bundle size by configuring polyfills selectively
Including all these Node.js polyfills could significantly increase your bundle size. Consider:
- Using webpack's
resolve.fallbackto only include necessary polyfills - Analyzing which Node.js core modules are actually needed by your dependencies
- Using lighter alternatives where possible (e.g.,
bufferinstead of fullstream-browserify)
Let's check which of these polyfills are actually being used:
packages/contact-center/station-login/src/station-login/styles.module.scss (2)
116-153: Regarding the use of !important in utility classes
The use of !important in utility classes is actually a valid pattern, as it ensures these utilities always override other styles. This is similar to how frameworks like Tailwind CSS implement their utilities.
However, consider adding a comment explaining this intentional use to prevent future confusion:
/* Margin utils */
+ /* Using !important is intentional for utility classes to ensure they always override other styles */
.u-m {
margin: 1rem !important;
}165-173: 🛠️ Refactor suggestion
Convert px units to rem for consistency
As mentioned in previous reviews, we should stick to rem units for consistency:
.device-type-label {
- width: 125px;
+ width: 7.8125rem;
display: inline-block;
}
.context-info {
- width: 200px;
+ width: 12.5rem;
display: inline-block;
}
packages/contact-center/station-login/src/station-login/station-login.presentational.tsx
Outdated
Show resolved
Hide resolved
packages/contact-center/station-login/src/station-login/station-login.presentational.tsx
Outdated
Show resolved
Hide resolved
packages/contact-center/station-login/tests/station-login/station-login.presentational.tsx
Outdated
Show resolved
Hide resolved
packages/contact-center/station-login/src/station-login/styles.module.scss
Outdated
Show resolved
Hide resolved
packages/contact-center/station-login/src/station-login/styles.module.scss
Outdated
Show resolved
Hide resolved
packages/contact-center/station-login/src/station-login/station-login.types.ts
Outdated
Show resolved
Hide resolved
packages/contact-center/station-login/tests/station-login/index.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| render(<StationLogin />); | ||
| console.log(useStationLoginSpy.mock.results[0].value); | ||
| expect(useStationLoginSpy).toHaveBeenCalledWith({webex: {}, loginReqParam}); |
There was a problem hiding this comment.
We should check number of times called. Can we create toBeCalledOnceWith() like in calling sdk or web client?
There was a problem hiding this comment.
toBeCalledOnceWith gives error as below and even after adding the jest macther like how we have done it in Calling SDK, issue doesn't resolve.
Property 'toBeCalledOnceWith' does not exist on type 'JestMatchers<SpyInstance<{ name: string; login: () => void; logout: () => void; loginSuccess: any; loginFailure: Error | undefined; logoutSuccess: any; }, [props: UseStationLoginProps], any>>'.
We might need to look into this separately to troubleshoot it.
There was a problem hiding this comment.
Can we create a jira to track this in the widget epic please? @Kesari3008
| loginOptions: ['EXTENSION', 'AGENT_DN', 'BROWSER'], | ||
| setDeviceType: jest.fn(), | ||
| setDialNumber: jest.fn(), | ||
| setTeam: jest.fn() | ||
| }; | ||
| render(<StationLoginPresentational {...props} />); | ||
| const heading = screen.getByTestId('station-login-heading'); | ||
| expect(heading).toHaveTextContent('StationLogin'); |
There was a problem hiding this comment.
Check number of times rendered.
There was a problem hiding this comment.
This code is temporary so I have not added proper testcases for the presentational component. Do you suggest we handle it differently?
There was a problem hiding this comment.
I think we should have a check for it right now so this acts as a template @Kesari3008
There was a problem hiding this comment.
There is no test file for this? Is it going to be added in a future PR
There was a problem hiding this comment.
I wasn't sure if we would be testing this file. @Shreyas281299 and @mkesavan13 might be able to suggest.
packages/contact-center/user-state/src/user-state/use-state.types.ts
Outdated
Show resolved
Hide resolved
packages/contact-center/user-state/src/user-state/use-state.types.ts
Outdated
Show resolved
Hide resolved
packages/contact-center/station-login/src/station-login/station-login.presentational.tsx
Show resolved
Hide resolved
packages/contact-center/station-login/src/station-login/station-login.presentational.tsx
Outdated
Show resolved
Hide resolved
| }); | ||
| }; | ||
|
|
||
| return {name: 'StationLogin', selectLoginOption, login, logout}; |
There was a problem hiding this comment.
Return the required props from helper file and add comments in presentational layer to help understanding the use of those props
There was a problem hiding this comment.
This is addressed
|
|
||
| useEffect(() => { | ||
| const teamsDropdown = document.getElementById('teamsDropdown') as HTMLSelectElement; | ||
| const agentLogin = document.querySelector('#LoginOption') as HTMLSelectElement; |
There was a problem hiding this comment.
Move the UI update logic in presentational layer
There was a problem hiding this comment.
This is addressed
| @@ -1,29 +1,40 @@ | |||
| /** | |||
| * Interface representing the properties for the Station Login component. | |||
| */ | |||
| export interface IStationLoginProps { | |||
| export interface IStationLoginPresentationalProps { | |||
There was a problem hiding this comment.
Revert back to old name, use the same type in presentational as well as helper layer
There was a problem hiding this comment.
This is addressed
| }, | ||
| token: 'eyJhbGciOiJSUzI1NiJ9.eyJjbHVzdGVyIjoiUDBBMSIsInByaXZhdGUiOiJleUpqZEhraU9pSktWMVFpTENKbGJtTWlPaUpCTVRJNFEwSkRMVWhUTWpVMklpd2lZV3huSWpvaVpHbHlJbjAuLlpTcC1aWmd2R0Z6ei1ZOXpRX21QbHcuWWhEa1hQQVRqS0RIUVQ4UGQxY3d6OUZUTW1IdmhJdUtDNEhxMGx0blh1TS01WWJDN2ltd1NaVHEwYzd0UVRidVRDalJpZjZqYTlkLUNSYTAzQ000Y3RTdjVuOFpZdS1qM0VCeXNlcS14dHdlQ3dOeTFTVllSZzlSM01qLU51S2tjRmFMSEFKelNmVlctUnk2WUJMLWUzS2g1REpkaEI5bDIwb0RIeVozYXF5dnFWZjgxSjRIU21zNElVSVhGZTVHeUV3RmxPTFQ3R2M3NUR4VkJYeUFyT0pXb1dXTHV3Y3lKa29oT2xsazdMcXNKY3hDNFNsQWxMQnpMR0M3VjlMb2xZWXM1T0lxV3hVWTdUV19wdkZKZFloUjFXUHhQMnRTTlRQUEVzQWRieEszeTZYZGhDRUxOQjBWWG9KakNHVlA1NUtET29hc09IUkctSFZ6dE9PNGZfdW9VLWNtVFRJV1BBVGYyU3J1ajF3dlR3eElKS3ljeU5pSUtFTUhaR2JXOHctMGNXMHk0QWc5SHZzSG5hMHlZNzRqY2MzNlVsbjhfMmdhNlh3WGlhMDd1eF9uSVpTeXNnNVhubzZFMGNEbFBzYmhseFN4SGlwWV80VVg3R0llaFhjSFRfRi05dmFFZ0wyZjlubDh5Wnpmb2FGUDNoM0IxZHdTWGlXQ0d6Sl9pcEVOeEtoLXNjNURXdVdBV1g4TjF4aFc0cUF6V1FXNGljb0NlejIzUkQ5WElsbHBFRjBpZ1Q3RmVpaHI3X3lTVHlTNFFMaDA0bXlzdTlfWjRuOWJjbElwWExvXzJMMnhrVVRvQ3h5YXNBMUc5YW51bGg3NmVLRFcwWVVaN1F0MXNhTkx2aVVIcmxsQmFjMEpTSEFXNmd3Y3R5RmpQZ2NfN3pOQ0RHZEk3amxDWXN5TFBqdWxxUm5GRkZxMmZxTncwT3JTQ1dkWUlhRjdFRnVyQlV4REZlZmFsSlJFbkVBZ2RpSDRIZzZHLVJnMTRiOVFMYlJQUTdNYXlJY1FLRjdKZ1UyRDJuR0pLY3NINUFGa1lOaFdtdTQyeC1NQTJUTEJsVjlydHF4YjRJbHJVdnBwajZQUndKX3EwNVh2dWhLcU4yeEdWbThXSEZHUjJScGhmWTI1TUlTdkpaYnBpLXNXeXp2RHlGUlVQTVFxM0JUbDFHelZGMTloR1BtZ2lBS1hQZnpoRXNKbGlJTmRZTVRacmN5VHRhSXpFenBZRF9QcUxOSlM4aDhOcEp3LlhWdVctRlhnVEQ0SWNXSUl5b2E0ckEiLCJ1c2VyX3R5cGUiOiJ1c2VyIiwidG9rZW5faWQiOiJBYVozcjBPREUyT0dGbFpEQXRaRGczTXkwME5HVXlMV0pqWVRJdE16a3dZV016WTJGaFpUVTFOR1F6TlRaak1qa3RZelJqIiwicmVmZXJlbmNlX2lkIjoiNWNkZDg1YzEtMGI1NC00ODc5LTk5NDAtOWJhNjE2OWRhZWFlIiwiaXNzIjoiaHR0cHM6XC9cL2lkYnJva2VyLWItdXMud2ViZXguY29tXC9pZGIiLCJ1c2VyX21vZGlmeV90aW1lc3RhbXAiOiIyMDI0MDkxMjA5MDkwOC45MDFaIiwicmVhbG0iOiI2ZWNlZjIwOS05YTM0LTRlZDEtYTA3YS03ZGRkMWRiZTkyNWEiLCJjaXNfdXVpZCI6IjZiMzEwZGZmLTU2OWUtNGFjNy1iMDY0LTcwZjgzNGVhNTZkOCIsInRva2VuX3R5cGUiOiJCZWFyZXIiLCJleHBpcnlfdGltZSI6MTczMTk3ODYyNTkyOCwiY2xpZW50X2lkIjoiQzU4MjU0ZjM4Y2UwMGVjN2FmZDFiNjA2NmY5N2UzYzI2ODBmODE5ZmFlZmU2OGE5NjUyMTk3YTNhOWUxODg3YzQifQ.Qhxd4En7IIhR2YQUNzxSaC6XEHLdAW-fxt3RO1U-ZSi1DtIIW-QAnXMIW3juAm30Q3UwvfJTZy9W3ox5Uu_yPJbjneFZ6ZbdKbXXzz8DJq_igOWRYX3oxWAWq1NjlBiP5Ov_qj3boRZEbddfg_IPPhJ1azz3UF0iVGhh_K9Yo9xb7SGvHRaZPSCE90MQmbWY61W99Ew2MLxzJOtB4LHfUZHaRoRDDlsONoxwSXiA_nihfr3wXRTlANTEsdI0u7g3G7oi8TxoCOE4icmcC2wtFOGhejGpSh-bcahPcWMVN3sV0VdPyG7T8P2TtqhsvypGL_i8TU_1Jtem83p9Nq-7_Q', | ||
| webex: {}, | ||
| init: async function(webexConfig: any, updateTeamsList: any, updateLoginVoiceOptions: any) { |
There was a problem hiding this comment.
Move this to store.ts file and delete sdk.ts
There was a problem hiding this comment.
THis is addressed
packages/contact-center/user-state/src/user-state/use-state.types.ts
Outdated
Show resolved
Hide resolved
| this.isAvailable = state; | ||
| }; | ||
| this.webex.once('ready', () => { | ||
| this.webex.cc.register(true).then((response: IAgentProfile) => { |
There was a problem hiding this comment.
Do we need to pass params to register? we removed it right?
There was a problem hiding this comment.
can we introduce type safety by utilising IcontactCenter?
| const [logoutSuccess, setLogoutSuccess] = useState<StationLogoutSuccess>(); | ||
|
|
||
| const login = () => { | ||
| webex.cc.stationLogin({teamId: loginReqParam.teamId, loginOption: loginReqParam.loginOption, dialNumber: loginReqParam.dialNumber}) |
There was a problem hiding this comment.
can we introduce type safety by utilising IcontactCenter?
JIRA: https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-575845
Description: Added code with basic UI and functional logic to invoke station login.
Testcase 1: Selected Extension as login option, entered dial number and clicked on login. Login successful.
Testcase 2: Selected Extension as login option clicked on login without dial number. Login fails with error: Agent_DN_Empty
Testcase 3: In logged in state, logged out the agent. Logout successful.
Testcase 4: Selected Browser as login option and clicked on login. Login Success.
Screenrecording:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores