Onboarding part 3 #6358
Onboarding part 3 #6358
Conversation
YounixM
force-pushed
the
onboarding-part-3
branch
from
43d822b
to
8354915
Compare
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 198ba59 in 2 minutes and 23 seconds
More details
- Looked at
2997lines of code in18files - Skipped
55files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. frontend/src/pages/OnboardingPageV2/OnboardingAddDataSource.tsx:183
- Draft comment:
Avoid usinganytype for theselectedEnvironmentparameter. Define a specific type for the environment to ensure type safety. - Reason this comment was not posted:
Marked as duplicate.
2. frontend/src/pages/OnboardingPageV2/OnboardingIngestionDetails.tsx:17
- Draft comment:
Ensure to handle cases whereingestionDatamight be undefined or null to prevent runtime errors. - Reason this comment was not posted:
Comment did not seem useful.
3. frontend/src/pages/OnboardingPageV2/OnboardingIngestionDetails.tsx:64
- Draft comment:
Ensure to handle cases whereinjectionDataPayloadmight be undefined or null to prevent runtime errors. - Reason this comment was not posted:
Marked as duplicate.
4. frontend/src/pages/OnboardingPageV2/QuestionBlock.tsx:67
- Draft comment:
Avoid using inline styles. Instead, use external stylesheets, CSS classes, or styled components. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_99RprrQxUCf7QXIv
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| const [hasMoreQuestions, setHasMoreQuestions] = useState<boolean>(true); | ||
|
|
||
| const [docsUrl, setDocsUrl] = useState<string>( | ||
| 'http://localhost:3000/docs/instrumentation/springboot#send-traces-to-signoz-cloud', |
There was a problem hiding this comment.
Consider externalizing the URL used in the useState hook to a configuration file or environment variable to avoid hardcoding.
| } | ||
| }; | ||
|
|
||
| const handleSelectFramework = (option: any): void => { |
There was a problem hiding this comment.
Avoid using any type for the option parameter. Define a specific type for the options to ensure type safety.
| const handleSelectFramework = (option: any): void => { | |
| const handleSelectFramework = (option: Option): void => { |
frontend/src/pages/OnboardingPageV2/OnboardingAddDataSource.tsx
Outdated
Show resolved
Hide resolved
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on c8ea8e7 in 44 seconds
More details
- Looked at
1078lines of code in2files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. frontend/src/container/OnboardingV2Container/AddDataSource/AddDataSource.tsx:97
- Draft comment:
Avoid using inline styles in React components. Consider using external stylesheets, CSS classes, or styled components instead. This is also applicable at line 345. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_NxVePD4uA8ahwrMV
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 762c36b in 1 minute and 0 seconds
More details
- Looked at
732lines of code in1files - Skipped
5files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. frontend/src/container/OnboardingV2Container/configs/onboarding-config-with-links.json:27
- Draft comment:
Unresolved merge conflict markers found. Please resolve the conflicts before merging. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_WIU3jEUHqFQhPozg
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| { | ||
| "key": "vm", | ||
| "label": "VM", | ||
| <<<<<<< Updated upstream:frontend/src/container/OnboardingV2Container/configs/onboarding-config-with-links.json |
There was a problem hiding this comment.
Unresolved merge conflict markers (e.g., <<<<<<<, =======, >>>>>>>) are present. These need to be resolved to ensure the JSON is valid. This issue appears multiple times throughout the file.
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on b33f72f in 1 minute and 7 seconds
More details
- Looked at
723lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. frontend/src/container/OnboardingV2Container/configs/onboarding-config-with-links.json:24
- Draft comment:
Merge conflict markers (e.g.,<<<<<<<,=======,>>>>>>>) are present in the JSON file. These need to be resolved to ensure the file is correctly formatted and functional. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_6wQ7CgF1LeGhOigf
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| "key": "vm", | ||
| "label": "VM", | ||
| "imgUrl": "/Logos/vm.svg", | ||
| "link": "http://localhost:3000/docs/instrumentation/opentelemetry-springboot/#send-traces-to-signoz-cloud" |
There was a problem hiding this comment.
Avoid using hardcoded URLs like http://localhost:3000. Replace them with production URLs or use environment variables to manage different environments.
| "link": "http://localhost:3000/docs/instrumentation/opentelemetry-springboot/#send-traces-to-signoz-cloud" | |
| "link": "https://signoz.io/docs/instrumentation/opentelemetry-springboot/#send-traces-to-signoz-cloud" |
- added header - questionnaire blocks - basic styling - search based on UI config method (basic for UI behaviour) - empty state result UI
filterByCategory styling spacing and styling fixes for options steps style for question block header styling update for sticky added actions on header
YounixM
force-pushed
the
onboarding-part-3
branch
from
b33f72f
to
8752021
Compare
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 8752021 in 1 minute and 4 seconds
More details
- Looked at
723lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_m8KtV8p0AfwOt4SJ
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| "key": "vm", | ||
| "label": "VM", | ||
| "imgUrl": "/Logos/vm.svg", | ||
| "link": "http://localhost:3000/docs/instrumentation/opentelemetry-springboot/#send-traces-to-signoz-cloud" |
There was a problem hiding this comment.
The link is set to a localhost URL, which is likely incorrect for production. Please update it to the correct external URL. This issue is also present in other parts of the file, such as lines 34, 40, 59, 65, 71, 90, 96, 102, 121, 127, 160, 166, 185, 191, 210, 216, 235, 241, 260, 266, 299, 305, 311, 330, 336, 342, 361, 367, 373, 392, 398, 404, 423, 429, 435, 459, 465, 471, 492, 498, 504, 525, 531, 537, 558, 564, 570, 591, 597, 603, 624, 630, 636, 657, 663, 669, 876, 1037, 1044, 1051, 1058.
| "link": "http://localhost:3000/docs/instrumentation/opentelemetry-springboot/#send-traces-to-signoz-cloud" | |
| "link": "https://signoz.io/docs/instrumentation/opentelemetry-springboot/#send-traces-to-signoz-cloud" |
| "type": "select", | ||
| "entityID": "environment", | ||
| "options": [ | ||
| { |
There was a problem hiding this comment.
Use design tokens or predefined constants for URLs instead of hardcoding them. This applies to all similar instances in this file.
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 44ca035 in 1 minute and 12 seconds
More details
- Looked at
1227lines of code in3files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. frontend/src/container/OnboardingV2Container/AddDataSource/AddDataSource.tsx:160
- Draft comment:
UsingsetTimeoutfor scrolling can lead to inconsistent behavior. Consider using a callback or event-driven mechanism to ensure the DOM is ready before scrolling. - Reason this comment was not posted:
Comment was on unchanged code.
2. frontend/src/container/OnboardingV2Container/AddDataSource/AddDataSource.tsx:298
- Draft comment:
Consider using a mapping object for routing based onselectedDataSource?.moduleto improve readability and maintainability. - Reason this comment was not posted:
Confidence changes required:50%
The switch statement for routing based onselectedDataSource?.moduleis clear, but it could be more concise using a mapping object. This would improve readability and maintainability.
3. frontend/src/container/OnboardingV2Container/configs/onboarding-config-with-links.json:6
- Draft comment:
Consider using a base URL and appending paths to reduce redundancy in similar environment links. - Reason this comment was not posted:
Confidence changes required:50%
The JSON configuration file is well-structured, but there are repeated URLs for similar environments. Consider using a base URL and appending paths to reduce redundancy and potential errors.
Workflow ID: wflow_e4sByCipTUKiRbc6
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 3310260 in 1 minute and 3 seconds
More details
- Looked at
1111lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. frontend/src/container/OnboardingV2Container/configs/onboarding-config-with-links.json:19
- Draft comment:
The links have been changed to a new domain (signoz-web-git-feat-onboarding-v2-p3-signoz.vercel.app). Ensure this is intentional and not a temporary change for testing purposes, as it could affect production if not intended. This applies to all similar link changes in this file. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_ScQZSyc2hIUIc8vv
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| "key": "springboot", | ||
| "label": "Spring Boot", | ||
| "imgUrl": "/Logos/springboot.svg", | ||
| "link": "https://signoz-web-git-feat-onboarding-v2-p3-signoz.vercel.app/docs/instrumentation/opentelemetry-springboot/#send-traces-to-signoz-cloud", |
There was a problem hiding this comment.
Avoid hardcoding URLs directly in the JSON. Consider using a configuration setting or constant to manage these URLs for better maintainability. This applies to all similar instances in this file.
|
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 11bd625 in 1 minute and 23 seconds
More details
- Looked at
560lines of code in2files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. frontend/src/container/OnboardingV2Container/AddDataSource/AddDataSource.tsx:187
- Draft comment:
Consider usingrequestAnimationFrameinstead ofsetTimeoutfor scrolling to ensure the element is in the DOM and to avoid potential timing issues. - Reason this comment was not posted:
Confidence changes required:50%
The use ofsetTimeoutfor scrolling is not ideal as it can lead to inconsistent behavior. UsingrequestAnimationFrameor ensuring the element is in the DOM before scrolling would be more reliable.
2. frontend/src/container/OnboardingV2Container/AddDataSource/AddDataSource.tsx:650
- Draft comment:
UsingreferrerPolicy="unsafe-url"can expose sensitive information. Consider using a safer policy likeno-referrerorstrict-origin-when-cross-origin. - Reason this comment was not posted:
Comment was on unchanged code.
3. frontend/src/container/OnboardingV2Container/AddDataSource/AddDataSource.tsx:206
- Draft comment:
Consider refactoringhandleSelectFrameworkandhandleSelectEnvironmentto reduce code duplication and improve maintainability. - Reason this comment was not posted:
Confidence changes required:40%
ThehandleSelectFrameworkandhandleSelectEnvironmentfunctions have similar logic for updating the URL and setting the state. This code can be refactored to reduce duplication and improve maintainability.
4. frontend/src/container/OnboardingV2Container/OnboardingV2.styles.scss:217
- Draft comment:
Use design tokens or predefined color constants instead of hardcoding color values like#e0e0e0and#0b0c0e. This applies to other hardcoded colors in this file as well. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_QRVNIfcA6HsbtjzK
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| <Flex gap="middle" vertical> | ||
| <Space align="end"> | ||
| <Skeleton.Avatar | ||
| style={{ width: 40, height: 40 }} |
There was a problem hiding this comment.
Avoid using inline styles in React components. Use external stylesheets, CSS classes, or styled components instead.
Summary
Related Issues / PR's
Screenshots
NA
Affected Areas and Manually Tested Areas
Important
Introduce
OnboardingV2component for enhanced onboarding flow with new routes, components, styles, and permissions.OnboardingV2component inAppRoutes/pageComponents.tsfor a new onboarding flow.GET_STARTED_V2inAppRoutes/routes.tsandconstants/routes.ts.AppLayout/index.tsxto render full screen forGET_STARTED_V2path.OnboardingAddDataSource,OnboardingIngestionDetails, andQuestionBlockcomponents inpages/OnboardingPageV2/.OnboardingAddDataSource.tsx.OnboardingPageV2.styles.scssfor the new onboarding components.AppLayout.styles.scssto adjust layout for new onboarding flow.GET_STARTED_V2title inlocales/en/titles.json.utils/permission/index.tsto includeGET_STARTED_V2route permissions.This description was created by
for 11bd625. It will automatically update as commits are pushed.