Conversation
PM-2524 Fix empty submissions tab for DS F2F
…odes PM-3204 - handle utm codes from cookies
Fix package lock
Dockerfile: use ci instead of install
fix package-lock
package-lock
update package-lock
Master -> dev
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| /* Provide globalThis in older Node runtimes (e.g. Node 10). */ | ||
| if (typeof globalThis === 'undefined') { | ||
| global.globalThis = global; |
There was a problem hiding this comment.
[💡 maintainability]
Assigning globalThis to global is a workaround for older Node versions. Consider documenting this workaround in a separate documentation file or README to ensure future maintainability and clarity for developers unfamiliar with this context.
|
|
||
| /* Provide crypto.sign/crypto.verify in older Node runtimes (e.g. Node 10). */ | ||
| const crypto = require('crypto'); | ||
| if (typeof crypto.sign !== 'function') { |
There was a problem hiding this comment.
[💡 maintainability]
The polyfill for crypto.sign and crypto.verify is a workaround for older Node versions. Ensure that this is well-documented elsewhere in the project documentation to avoid confusion for developers who may not expect this behavior.
|
|
||
| /* Provide TextEncoder/TextDecoder in older Node runtimes (e.g. Node 10). */ | ||
| const { TextDecoder, TextEncoder } = require('util'); | ||
| if (typeof global.TextEncoder === 'undefined') { |
There was a problem hiding this comment.
[💡 maintainability]
The polyfill for TextEncoder and TextDecoder is a workaround for older Node versions. Consider documenting this in a separate documentation file or README to ensure future maintainability and clarity for developers unfamiliar with this context.
| @@ -1,8 +1,12 @@ | |||
| module.exports = { | |||
| SEGMENT_IO_API_KEY: 'QBtLgV8vCiuRX1lDikbMjcoe9aCHkF6n', | |||
There was a problem hiding this comment.
[❗❗ security]
Storing API keys directly in the codebase is a security risk. Consider using environment variables or a secure vault to manage sensitive information.
| @@ -1,8 +1,12 @@ | |||
| module.exports = { | |||
| SEGMENT_IO_API_KEY: 'QBtLgV8vCiuRX1lDikbMjcoe9aCHkF6n', | |||
| SERVER_API_KEY: '79b2d5eb-c1fd-42c4-9391-6b2c9780d591', | |||
There was a problem hiding this comment.
[❗❗ security]
Storing API keys directly in the codebase is a security risk. Consider using environment variables or a secure vault to manage sensitive information.
| @@ -1,8 +1,12 @@ | |||
| module.exports = { | |||
| SEGMENT_IO_API_KEY: 'QBtLgV8vCiuRX1lDikbMjcoe9aCHkF6n', | |||
There was a problem hiding this comment.
[❗❗ security]
The SEGMENT_IO_API_KEY is hardcoded in the configuration file. Consider using environment variables to manage sensitive information securely.
| @@ -1,8 +1,12 @@ | |||
| module.exports = { | |||
| SEGMENT_IO_API_KEY: 'QBtLgV8vCiuRX1lDikbMjcoe9aCHkF6n', | |||
| SERVER_API_KEY: '79b2d5eb-c1fd-42c4-9391-6b2c9780d591', | |||
There was a problem hiding this comment.
[❗❗ security]
The SERVER_API_KEY is hardcoded in the configuration file. Consider using environment variables to manage sensitive information securely.
| @@ -1,3 +1,9 @@ | |||
| module.exports = { | |||
| SERVER_API_KEY: '79b2d5eb-c1fd-42c4-9391-6b2c9780d591', | |||
There was a problem hiding this comment.
[❗❗ security]
Storing sensitive information like SERVER_API_KEY directly in the codebase can lead to security vulnerabilities. Consider using environment variables or a secure vault to manage sensitive data.
| module.exports = { | ||
| SERVER_API_KEY: '79b2d5eb-c1fd-42c4-9391-6b2c9780d591', | ||
| API: { | ||
| ENGAGEMENTS: 'https://api.topcoder-dev.com/v6/engagements/engagements', |
There was a problem hiding this comment.
[maintainability]
Ensure that the API endpoint https://api.topcoder-dev.com/v6/engagements/engagements is not hardcoded for production use. Consider using environment-specific configurations to manage different API endpoints for development, testing, and production environments.
| ENGAGEMENTS: 'https://api.topcoder-dev.com/v6/engagements/engagements', | ||
| }, | ||
| URL: { | ||
| ENGAGEMENTS_APP: 'https://engagements.topcoder-dev.com', |
There was a problem hiding this comment.
[maintainability]
The URL https://engagements.topcoder-dev.com should be configurable based on the environment to avoid hardcoding production URLs in the codebase. This will improve the maintainability and flexibility of the application.
| return { uuid, page, filters }; | ||
| } | ||
|
|
||
| async function getEngagementsDone(uuid, page, filters, tokenV3) { |
There was a problem hiding this comment.
[correctness]
Consider adding input validation for uuid, page, filters, and tokenV3 to ensure they are of the expected types and values before proceeding with the function logic. This can prevent unexpected errors and improve the robustness of the function.
|
|
||
| async function getEngagementsDone(uuid, page, filters, tokenV3) { | ||
| try { | ||
| const { engagements, meta } = await getEngagements(page, PAGE_SIZE, filters, tokenV3); |
There was a problem hiding this comment.
[performance]
The getEngagements function call should handle potential errors that could arise from network issues or unexpected responses. Consider implementing retry logic or more granular error handling to improve resilience.
| } else if (typeof error === 'string') { | ||
| message = error; | ||
| } | ||
| fireErrorMessage('Error Loading Engagements', message); |
There was a problem hiding this comment.
[💡 maintainability]
The fireErrorMessage function is used here to display an error message, but it might be beneficial to log the error details as well for debugging purposes. Consider adding a logging mechanism to capture the full error stack or details.
| value={normalizedTerms} | ||
| onChange={(value) => { | ||
| onChange(value ? (value.value || '') : ''); | ||
| onChange(value || null); |
There was a problem hiding this comment.
[❗❗ correctness]
Changing onChange(value ? (value.value || '') : '') to onChange(value || null) alters the behavior when value is an object with a value property. Previously, it would pass an empty string if value.value was falsy, but now it passes null. Ensure this change is intentional and that downstream logic can handle null appropriately.
| })); | ||
| const suggestedOptions = (response || []) | ||
| .map((skillItem) => { | ||
| const label = skillItem && (skillItem.name || skillItem.label || skillItem.title); |
There was a problem hiding this comment.
[correctness]
The mapping logic for label and value is flexible but could lead to unexpected results if skillItem has multiple properties like name, label, and title. Consider explicitly prioritizing these properties to ensure consistent behavior.
| .map((skillItem) => { | ||
| const label = skillItem && (skillItem.name || skillItem.label || skillItem.title); | ||
| if (!label) return null; | ||
| const value = skillItem.id || skillItem.skillId || skillItem.value || label; |
There was a problem hiding this comment.
[correctness]
The logic for determining value is potentially problematic. If skillItem has both id and skillId, the choice of id over skillId might not be intentional. Clarify the priority or document the expected structure of skillItem.
| onChange={(newSkill) => { | ||
| setSkills(newSkill); | ||
| onSearch(newSkill); | ||
| const nextSkill = newSkill || ''; |
There was a problem hiding this comment.
[correctness]
Using newSkill || '' ensures nextSkill is always a string, which is good for consistency. However, if newSkill can be an object, this might mask potential issues. Ensure newSkill is always the expected type before this assignment.
|
|
||
| const ProfileHeader = ({ getLookerDone, lookerInfo, info }) => { | ||
| const ProfileHeader = ({ info }) => { | ||
| const [imageUrl, setimageUrl] = useState(); |
There was a problem hiding this comment.
[💡 readability]
The setimageUrl function should be named setImageUrl to follow camelCase naming conventions, which improves readability and consistency with React's useState convention.
| prizeSets, | ||
| prizeSet => ((prizeSet && prizeSet.type) || '').toLowerCase() === 'checkpoint', | ||
| ); | ||
| const checkpointPrizeType = _.toUpper(_.get(checkpointPrizes, 'prizes[0].type', 'USD')); |
There was a problem hiding this comment.
[correctness]
Using _.toUpper on a potentially undefined value from _.get could lead to unexpected behavior if checkpointPrizes.prizes[0].type is not a string. Consider providing a default string value to _.get to ensure _.toUpper always receives a string.
| numberOfCheckpointsPrizes = checkpointPrizes.prizes.length; | ||
| topCheckPointPrize = checkpointPrizes.prizes[0].value; | ||
| } | ||
| const topCheckPointPrizeDisplay = checkpointPrizeType === 'POINT' |
There was a problem hiding this comment.
[💡 readability]
The logic for determining topCheckPointPrizeDisplay could be simplified by using a conditional expression directly in the template string. This would improve readability by reducing the number of lines and making the logic more concise.
| const { challenge } = this.props; | ||
| const trackName = getTrackName(challenge); | ||
| return (trackName || '').toLowerCase() === 'data science' || checkIsMM(challenge); | ||
| return checkIsMM(challenge); |
There was a problem hiding this comment.
[❗❗ correctness]
The removal of getTrackName(challenge) and the associated check for 'data science' may change the behavior of the isMM method. Ensure that checkIsMM(challenge) alone is sufficient to determine if the challenge is a marathon match, as this could affect logic that depends on the challenge type.
| const past = isPastBucket(activeBucket); | ||
| const [currentSelected, setCurrentSelected] = useState(past); | ||
| const [isTabClosed, setIsTabClosed] = useState(true); | ||
| const pathname = _.get(location, 'pathname', ''); |
There was a problem hiding this comment.
[💡 performance]
Using _.get to access location.pathname is unnecessary here since location is a required prop and pathname is a required field. Direct access would be more straightforward and performant.
| }, [activeBucket]); | ||
|
|
||
| const onActiveClick = () => { | ||
| if (externalTabLabel) { |
There was a problem hiding this comment.
[maintainability]
The check if (history && history.push) is repeated in both onActiveClick and onPastChallengesClick. Consider extracting this logic into a helper function to reduce code duplication and improve maintainability.
| {TAB_NAME.PAST_CHALLENGES} | ||
| </li> | ||
| {TAB_LINKS.map(link => ( | ||
| <li |
There was a problem hiding this comment.
[correctness]
The key for list items should be unique and stable. Using link.to as a key could cause issues if to is not unique across different links. Consider using a unique identifier for each link if available.
| @@ -1,5 +1,10 @@ | |||
| @import "~styles/mixins"; | |||
|
|
|||
| .item-link { | |||
There was a problem hiding this comment.
[maintainability]
The .item-link class is defined multiple times with different properties. Consider consolidating these definitions to improve maintainability and avoid potential conflicts.
| } | ||
| } | ||
|
|
||
| .item-link { |
There was a problem hiding this comment.
[maintainability]
The .item-link class is defined multiple times with different properties. Consider consolidating these definitions to improve maintainability and avoid potential conflicts.
| @@ -0,0 +1,547 @@ | |||
| import React from 'react'; | |||
| import PT from 'prop-types'; | |||
| import moment from 'moment-timezone'; | |||
There was a problem hiding this comment.
[performance]
Consider using native JavaScript Intl API for timezone handling instead of moment-timezone to reduce bundle size, as moment is a large library and Intl is now widely supported.
| <img src={iconBlackSkills} alt="role-icon" /> {getRoleDisplay(role)} | ||
| </div> | ||
| <div styleName="icon-val"> | ||
| <img src={iconBlackSkills} alt="skills-icon" /> {limitedSkillsText} |
There was a problem hiding this comment.
[💡 accessibility]
The alt attribute for the skills icon is set to skills-icon, which is not descriptive for accessibility purposes. Consider using a more descriptive text.
| }; | ||
|
|
||
| EngagementCard.propTypes = { | ||
| engagement: PT.shape(), |
There was a problem hiding this comment.
[maintainability]
The engagement prop type is defined as PT.shape(), which does not specify any expected properties. Consider defining the shape of the engagement object to improve type safety and maintainability.
|
|
||
| &:disabled, | ||
| &:hover:disabled { | ||
| background-color: #e9e9e9 !important; |
There was a problem hiding this comment.
[maintainability]
Using !important can make the CSS harder to maintain and override in the future. Consider refactoring to avoid using !important if possible.
| const UPDATED_DATE_FIELDS = ['updatedAt', 'updated_at', 'updatedOn', 'updated']; | ||
| const UUID_PATTERN = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; | ||
|
|
||
| function getTimestamp(engagement, fields) { |
There was a problem hiding this comment.
[correctness]
The getTimestamp function returns 0 when no valid timestamp is found. This could lead to incorrect sorting if engagements have missing or invalid date fields. Consider handling this case more explicitly, perhaps by returning null or a distinct value that can be checked separately.
| <div styleName="cards"> | ||
| {sortedEngagements.map((engagement, index) => ( | ||
| <EngagementCard | ||
| key={engagement.nanoId || engagement.id || engagement.engagementId || index} |
There was a problem hiding this comment.
[correctness]
The key for EngagementCard falls back to the index if none of the other identifiers are available. This can lead to issues with React's reconciliation process if the order of engagements changes. Consider ensuring that each engagement has a unique identifier to use as a key.
| } | ||
| } | ||
|
|
||
| .filters.loading { |
There was a problem hiding this comment.
[design]
Consider adding a visual indicator (e.g., a spinner) when .filters.loading is active to improve user experience by providing feedback that the interface is in a loading state.
| cursor: pointer; | ||
|
|
||
| &:disabled { | ||
| opacity: 0.6; |
There was a problem hiding this comment.
[accessibility]
Ensure that the opacity change for the disabled state of .load-more-button provides sufficient contrast to meet accessibility standards. This is important for users with visual impairments.
| getEngagements, | ||
| } = this.props; | ||
|
|
||
| const filterChanged = !_.isEqual(prevProps.filter, filter); |
There was a problem hiding this comment.
[performance]
Using _.isEqual from lodash for deep comparison of filter objects is fine, but consider the performance implications if filter becomes large or complex. If performance becomes an issue, you might want to implement a more efficient comparison strategy.
|
|
||
| return { | ||
| getEngagements: (page, filters, tokenV3) => { | ||
| const uuid = shortId(); |
There was a problem hiding this comment.
[security]
The shortId library is used to generate UUIDs for requests. Be aware that shortId is not suitable for generating cryptographically secure IDs. If security is a concern, consider using a more robust library like uuid.
| }; | ||
| } | ||
|
|
||
| const page = typeof payload.page === 'number' ? payload.page : state.lastRequestedPage; |
There was a problem hiding this comment.
[correctness]
The check for typeof payload.page === 'number' is good for ensuring the type, but consider also checking if payload.page is a valid page number (e.g., non-negative) to prevent potential logical errors.
| ...state.meta, | ||
| ...(payload.meta || {}), | ||
| }; | ||
| const totalCount = typeof nextMeta.totalCount === 'number' ? nextMeta.totalCount : null; |
There was a problem hiding this comment.
[correctness]
The variable totalCount is set to null if nextMeta.totalCount is not a number. This could lead to unexpected behavior in the subsequent logic that checks totalCount !== null. Consider defaulting to a safe number or handling this case explicitly.
| @@ -0,0 +1,32 @@ | |||
| import LoadingIndicator from 'components/LoadingIndicator'; | |||
| import path from 'path'; | |||
There was a problem hiding this comment.
[💡 maintainability]
The path module is imported but not used in the code. Consider removing it to clean up the imports.
| renderPlaceholder={() => <LoadingIndicator />} | ||
| renderServer={(renderProps) => { | ||
| const p = webpack.resolveWeak('containers/engagement-listing'); | ||
| const EngagementListing = webpack.requireWeak(path.resolve(__dirname, p)); |
There was a problem hiding this comment.
[correctness]
Using path.resolve(__dirname, p) in a server-side rendering context might not behave as expected if the module resolution path is not correctly set up. Ensure that __dirname resolves to the correct directory in all environments where this code will run.
|
|
||
| function buildEngagementsUrl(page, pageSize, filters = {}) { | ||
| const normalizedPage = Number.isFinite(page) ? Math.max(1, page + 1) : 1; | ||
| const url = new URL(engagementsApiUrl); |
There was a problem hiding this comment.
[💡 maintainability]
Consider using URLSearchParams directly for constructing query parameters instead of appending them one by one. This can improve readability and maintainability.
| }); | ||
|
|
||
| if (!res.ok) { | ||
| throw new Error(res.statusText); |
There was a problem hiding this comment.
[maintainability]
Consider including more detailed error handling here, such as logging the error or providing more context in the error message. This can help with debugging and understanding the failure.
| }); | ||
|
|
||
| if (!res.ok) { | ||
| throw new Error(res.statusText); |
There was a problem hiding this comment.
[maintainability]
Consider including more detailed error handling here, such as logging the error or providing more context in the error message. This can help with debugging and understanding the failure.
| }); | ||
|
|
||
| if (!res.ok) { | ||
| throw new Error(res.statusText); |
There was a problem hiding this comment.
[maintainability]
Consider including more detailed error handling here, such as logging the error or providing more context in the error message. This can help with debugging and understanding the failure.
| fullFooter = false; | ||
| } | ||
|
|
||
| if (url.includes('/engagements')) { |
There was a problem hiding this comment.
[maintainability]
The condition url.includes('/engagements') is very similar to the condition for /challenges above. Consider refactoring to avoid duplication and improve maintainability.
jmgasper
changed the title
4 participants
No description provided.