Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: revamp the frontend architecture #6598

Open
wants to merge 41 commits into
base: develop
Choose a base branch
from

Conversation

vikrantgupta25
Copy link
Collaborator

@vikrantgupta25 vikrantgupta25 commented Dec 5, 2024

Refactor

This PR takes care of moving away from redux as much as possible and streamlining the routing , sync setup calls and making sure we have deterministic actions based on API results. All the test cases have been fixed in sync with the latest design architecture

Contributes to - #5621 , https://github.com/SigNoz/engineering-pod/issues/2068

@vikrantgupta25
Copy link
Collaborator Author

Copy link

github-actions bot commented Dec 5, 2024

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

@github-actions github-actions bot added the enhancement New feature or request label Dec 5, 2024
@vikrantgupta25 vikrantgupta25 changed the title feat: setup the app context to fetch users,licenses and feature flags feat: architectural changes for app routes and private routes [ deprecating redux ] Dec 5, 2024
@vikrantgupta25 vikrantgupta25 changed the title feat: architectural changes for app routes and private routes [ deprecating redux ] feat: architectural changes for app routes and private routes [deprecating redux] Dec 5, 2024
Copy link

github-actions bot commented Dec 5, 2024

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

1 similar comment
Copy link

github-actions bot commented Dec 5, 2024

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

@vikrantgupta25 vikrantgupta25 changed the title feat: architectural changes for app routes and private routes [deprecating redux] feat: revamp the frontend architecture Dec 5, 2024
Copy link

github-actions bot commented Dec 5, 2024

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

1 similar comment
Copy link

github-actions bot commented Dec 5, 2024

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

@vikrantgupta25 vikrantgupta25 changed the title feat: revamp the frontend architecture chore: revamp the frontend architecture Dec 5, 2024
Copy link

github-actions bot commented Dec 5, 2024

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

@github-actions github-actions bot added the chore label Dec 5, 2024
@vikrantgupta25 vikrantgupta25 removed the enhancement New feature or request label Dec 5, 2024
@vikrantgupta25 vikrantgupta25 marked this pull request as ready for review December 11, 2024 03:19
Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Reviewed everything up to 6eeab69 in 3 minutes and 58 seconds

More details
  • Looked at 8732 lines of code in 136 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. pkg/query-service/dao/sqlite/rbac.go:95
  • Draft comment:
    Ensure that the model.Organization struct includes IsAnonymous and HasOptedUpdates fields, and that these fields are correctly populated before calling CreateOrg.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The CreateOrg function has been updated to include is_anonymous and has_opted_updates fields in the INSERT statement. This change should be reflected in the corresponding model and any related logic to ensure consistency.
2. frontend/src/providers/App/App.tsx:243
  • Draft comment:
    Consider adding a check to ensure the AFTER_LOGIN event does not redundantly update the user state if the event is dispatched multiple times.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    In the AppProvider component, the useEffect hook for setting the user state on login might not handle cases where the AFTER_LOGIN event is dispatched multiple times. Consider adding a check to prevent redundant state updates.
3. frontend/src/providers/App/App.tsx:259
  • Draft comment:
    Ensure that any additional cleanup logic is handled in the LOGOUT event listener, if necessary.
  • Reason this comment was not posted:
    Confidence changes required: 33%
    In the AppProvider, the useEffect for LOGOUT event resets the state. Ensure that any additional cleanup logic is also handled here if needed.
4. frontend/src/providers/App/App.tsx:66
  • Draft comment:
    Consider adding error handling for the user data fetch to manage potential issues during the fetch process.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    In the AppProvider, the useEffect for fetching user data does not handle errors. Consider adding error handling to manage potential issues during the fetch.
5. pkg/query-service/dao/sqlite/rbac.go:92
  • Draft comment:
    Do not add non-ClickHouse related functions to the ClickHouseReader interface. Use the DAO in the telemetry instance to access such information instead.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_hhchr3f8ChOUxn1y


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@srikanthccv
Copy link
Member

https://app.intercom.com/a/inbox/wmkurvb7/inbox/admin/6745296/conversation/5813#part_id=comment-5813-17320 more and more users are facing this and perhaps it became more visible and frequent

Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on 4023a6c in 4 minutes and 56 seconds

More details
  • Looked at 6238 lines of code in 300 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. frontend/src/container/OnboardingContainer/Modules/AzureMonitoring/AppService/appService-installCentralCollector.md:40
  • Draft comment:
    Use design tokens or predefined color constants instead of hardcoding color values for the signoz-ingestion-key header. This applies to other similar occurrences in the markdown files.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_sYOlnInX7BdBsOJw


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@srikanthccv
Copy link
Member

https://github.com/SigNoz/engineering-pod/issues/2096

Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on 8763fe6 in 1 minute and 46 seconds

More details
  • Looked at 69 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. frontend/src/container/ResourceAttributesFilter/ResourceAttributesFilter.tsx:89
  • Draft comment:
    The data-testid attribute should be consistently named. Please ensure it is correctly capitalized throughout the codebase.
  • Reason this comment was not posted:
    Comment did not seem useful.
2. frontend/src/AppRoutes/Private.tsx:189
  • Draft comment:
    Ensure that location is necessary in the dependency array. If not, it could lead to unnecessary re-renders.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The location dependency was added to the useEffect dependency array, which is a good practice to ensure the effect runs when the location changes. However, ensure that location is indeed necessary for the effect logic.
3. frontend/src/container/ResourceAttributesFilter/ResourceAttributesFilter.tsx:90
  • Draft comment:
    Avoid using inline styles. Consider using CSS classes or styled components instead. This applies to other inline styles in this file as well.
  • Reason this comment was not posted:
    Comment was on unchanged code.
4. frontend/src/container/ResourceAttributesFilter/ResourceAttributesFilter.tsx:122
  • Draft comment:
    Avoid using inline styles. Consider using CSS classes or styled components instead. This applies to other inline styles in this file as well.
  • Reason this comment was not posted:
    Marked as duplicate.
5. frontend/src/container/ResourceAttributesFilter/ResourceAttributesFilter.tsx:128
  • Draft comment:
    Avoid using inline styles. Consider using CSS classes or styled components instead. This applies to other inline styles in this file as well.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_79OVsbiT6CaMoWb5


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id>

@vikrantgupta25
Copy link
Collaborator Author

contributes to - #5621

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Incremental review on a95d218 in 13 minutes and 33 seconds

More details
  • Looked at 109 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. frontend/src/container/LogsExplorerViews/index.tsx:292
  • Draft comment:
    Remove the unnecessary console.log statement to clean up the code.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
2. frontend/src/container/Login/index.tsx:86
  • Draft comment:
    Avoid using inline styles. Replace inline styles with CSS classes or styled components where applicable.
  • Reason this comment was not posted:
    Comment was not on a valid diff hunk.
3. frontend/src/container/Login/index.tsx:1
  • Draft comment:
    Avoid using index.tsx for component files. Use a more descriptive file name for better debugging and searching.
  • Reason this comment was not posted:
    Comment was on unchanged code.
4. frontend/src/container/LogsExplorerViews/index.tsx:292
  • Draft comment:
    Avoid using index.tsx for component files. Use a more descriptive file name for better debugging and searching.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_tYinoYp2xQoGc5Vq


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

if (isCloudUserVal || isEECloudUser()) {
// if the user is on basic plan then remove billing
if (isOnBasicPlan) {
updatedRoutes = updatedRoutes.filter(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vikrantgupta25: Billing should be should only to admins, do we have a check somewhere else ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes we have the same in SideNav.tsx

licenses.onTrial &&
!licenses.trialConvertedToSubscription &&
!licenses.workSpaceBlock &&
getRemainingDays(licenses.trialEnd) < 7
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: destruct licenses and move 7 to const

}
}, [isLoggedIn]);

const handleUpgrade = (): void => {
if (role === 'ADMIN') {
if (user.role === 'ADMIN') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

safety check

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not required as the user.role will always be present [ default is VIEWER ]


const { data: ingestionData, isFetching } = useQuery({
queryFn: getIngestionData,
queryKey: ['getIngestionData', user?.userId],
queryKey: ['getIngestionData', user?.id],
Copy link
Member

@YounixM YounixM Dec 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can add enabled check here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will check in follow up PR , didn't update the existing code logics

(state) => state.app,
);
const { user } = useAppContext();
// TODO[vikrantgupta25]: check with sagar on cleanup
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not required anymore, i cleaned it up ✅

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will remove the TODO in follow up

@@ -239,12 +225,6 @@ function ImportJSON({
>
{t('import_and_next')} &nbsp; <MoveRight size={14} />
</Button>

{isFeatureAlert && (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this not being consumed ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no we removed usage based features so removed the check for everywhere

Copy link
Member

@YounixM YounixM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor Comments, can be taken up in a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants