feat: add documentation link for logs explorer quick filters #6854
+237
−32
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to bb9f8e2 in 2 minutes and 59 seconds
More details
- Looked at
132lines of code in3files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. frontend/src/components/QuickFilters/FilterRenderers/Checkbox/Checkbox.tsx:426
- Draft comment:
Consider adding a null check forsectionbefore using it in the URL to prevent potential issues iffilter.attributeKey.keyis not found inQUICK_FILTER_DOC_PATHS. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
- The comment has a valid concern - if the key doesn't exist in QUICK_FILTER_DOC_PATHS, section would be undefined. 2. However, this is a UI element for documentation links - worst case is the link doesn't work perfectly. 3. The keys in QUICK_FILTER_DOC_PATHS seem to match the expected filter keys. 4. This is not a critical path that would break functionality.
The comment identifies a real edge case that could occur. Missing documentation links could be frustrating for users.
While valid, this is a low-risk UI enhancement feature. The worst case is a slightly malformed documentation URL, not a critical error.
The comment should be deleted as it suggests defensive programming for a non-critical UI feature. The potential issue is minor and doesn't warrant the added complexity.
2. frontend/src/components/QuickFilters/QuickFilters.tsx:128
- Draft comment:
Ensure that thekeyprop for theCheckboxandSlidercomponents is unique. Consider using a combination offilter.typeandfilter.titleor another unique identifier. - Reason this comment was not posted:
Comment did not seem useful.
3. frontend/src/components/QuickFilters/FilterRenderers/Checkbox/Checkbox.tsx:553
- Draft comment:
Avoid using inline styles likecolor={Color.BG_ROBIN_400}. Use CSS classes or styled components instead. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
While generally avoiding inline styles is good practice, this case is using a proper design token from a design system. The color is not hardcoded but rather referenced from a centralized token system. The styling is also quite minimal - just a single color property. Moving this to a CSS class would add complexity without much benefit.
The comment correctly identifies an inline style usage. Using CSS classes could make the styling more maintainable and consistent with other components.
However, since this is using a proper design token and is just a single color property, the benefit of moving it to CSS would be minimal. The current approach maintains the connection to the design system while being simple and clear.
The comment should be deleted as it's suggesting a change that would add complexity without significant benefit, especially since proper design tokens are already being used.
Workflow ID: wflow_PNmEAQdHqkiRp1ef
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
nityanandagohain
requested changes
Jan 20, 2025
|
Updated recording after the suggested changes 2025-01-20.17-21-58.mov |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 233e0eb in 58 seconds
More details
- Looked at
355lines of code in3files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. frontend/src/assets/CustomIcons/EmptyQuickFilterIcon.tsx:90
- Draft comment:
TheEmptyQuickFilterIconcomponent is well-structured and follows best practices. No issues found. - Reason this comment was not posted:
Confidence changes required:0%
The PR introduces a new icon componentEmptyQuickFilterIconwhich is used in theCheckboxFiltercomponent. The icon is a static SVG and does not have any dynamic behavior or props that could lead to issues. The component is well-structured and follows React best practices. No issues found here.
2. frontend/src/components/QuickFilters/FilterRenderers/Checkbox/Checkbox.styles.scss:164
- Draft comment:
The CSS changes are consistent with existing styles and follow best practices. No issues found. - Reason this comment was not posted:
Confidence changes required:0%
The CSS changes inCheckbox.styles.scssare mostly structural and stylistic, aligning with the new design requirements. The changes are consistent with the existing styles and do not introduce any obvious issues. The use of BEM-like naming conventions is appropriate.
3. frontend/src/components/QuickFilters/FilterRenderers/Checkbox/Checkbox.tsx:478
- Draft comment:
The logic for displaying the documentation link is correctly implemented and aligns with the PR's intent. No issues found. - Reason this comment was not posted:
Confidence changes required:0%
TheCheckboxFiltercomponent has been updated to include a new section for the documentation link when no attribute values are found. This aligns with the PR's intent to add a documentation link. The logic for displaying this section is correctly implemented, checking for!isLoading,!isInfraMonitoring, and!attributeValues.length. The URL construction for the documentation link is also correct.
4. frontend/src/components/QuickFilters/FilterRenderers/Checkbox/Checkbox.tsx:494
- Draft comment:
Avoid using inline styles for the ArrowUpRight icon's color property. Use a CSS class or styled component instead. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
While generally avoiding inline styles is good practice, in this case the color is using a design token which is the recommended approach for maintaining consistent theming. The comment seems to misunderstand that using a design token via props is actually a good pattern. The icon component likely expects a color prop as part of its API.
Could there be value in moving even design token colors to CSS classes for better maintainability? Would that make the code more complex for little benefit?
Using design tokens via props is a valid and common pattern, especially for icon components. Moving this to CSS would add complexity without clear benefits since we're already using the design system correctly.
Delete this comment since it incorrectly flags proper usage of design tokens through component props as an issue.
Workflow ID: wflow_QR0sCgfhZivLh9FC
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.
nityanandagohain
previously approved these changes
Jan 20, 2025
|
@ahmadshaheer the loading state shows the filter and no values found, which should not be the case. please take care of that |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on a103c97 in 51 seconds
More details
- Looked at
163lines of code in3files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. frontend/src/components/QuickFilters/FilterRenderers/Checkbox/QuickFilterEmptyState.tsx:22
- Draft comment:
Consider adding a default case forQUICK_FILTER_DOC_PATHSto handle unknownattributeKeyvalues gracefully. - Reason this comment was not posted:
Confidence changes required:50%
The PR introduces a new componentQuickFilterEmptyStateto handle the empty state of quick filters. This component is used inCheckbox.tsxto replace the inline JSX for the empty state. The component correctly uses theQUICK_FILTER_DOC_PATHSto generate the documentation link based on theattributeKey. The logic for opening the documentation link is encapsulated within thehandleLearnMoreClickfunction, which is a good practice for separation of concerns. The component is styled using the existing CSS classes, ensuring consistency with the rest of the application. Overall, the component is well-structured and integrates seamlessly with the existing code.
2. frontend/src/components/QuickFilters/FilterRenderers/Checkbox/Checkbox.tsx:460
- Draft comment:
Ensure that theattributeKeypassed toQuickFilterEmptyStateis always valid and present inQUICK_FILTER_DOC_PATHSto avoid potential issues with undefined links. - Reason this comment was not posted:
Confidence changes required:50%
The PR modifies theCheckbox.tsxfile to use the newQuickFilterEmptyStatecomponent. This change is appropriate as it encapsulates the empty state logic into a separate component, improving code readability and maintainability. TheQuickFilterEmptyStatecomponent is used when there are no attribute values and theisInfraMonitoringflag is false. This logic aligns with the existing behavior, ensuring that the empty state is displayed under the correct conditions. The removal of thehandleLearnMoreClickfunction fromCheckbox.tsxand its inclusion inQuickFilterEmptyStateis a good refactor, as it centralizes the logic related to the empty state in one place.
3. frontend/src/components/QuickFilters/FilterRenderers/Checkbox/QuickFilterEmptyState.tsx:1
- Draft comment:
Avoid usingcomponent/index.tsxfile structure forQuickFilterEmptyState. It makes it difficult to debug and find components using global search tools. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_Olb2ZgMvbk92p2dC
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.
frontend/src/components/QuickFilters/FilterRenderers/Checkbox/QuickFilterEmptyState.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to f9afdeb in 1 minute and 49 seconds
More details
- Looked at
411lines of code in7files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. frontend/src/components/QuickFilters/FilterRenderers/Checkbox/QuickFilterEmptyState.tsx:23
- Draft comment:
Consider adding a check to handle cases whereattributeKeyis not found inQUICK_FILTER_DOC_PATHSto avoid potential undefined behavior. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The issue is real - an undefined section would lead to a broken URL. However, looking at the QUICK_FILTER_DOC_PATHS object, it seems to be a fixed mapping for specific known attributes. The component is likely only used with these known keys. The PR rules say not to make speculative comments or ask for checks unless we're certain there's an issue.
I could be wrong about the component only being used with known keys - there might be dynamic attribute keys passed in that aren't in the mapping.
Without seeing the parent components and how this is used, we can't be certain this is a real issue. The PR rules specifically say to delete comments if we need more context.
Delete the comment since we don't have strong evidence that undefined keys are actually possible in practice.
2. frontend/src/components/QuickFilters/FilterRenderers/Checkbox/Checkbox.tsx:24
- Draft comment:
Consider makingsourcea required prop without a default value to ensure it is always provided. - Reason this comment was not posted:
Confidence changes required:50%
TheCheckboxFiltercomponent uses thesourceprop to determine if the empty state with documentation should be shown. This is a good approach, but it would be more robust to ensure thatsourceis always provided by making it a required prop without a default value. This will prevent any potential issues if thesourceis accidentally omitted.
3. frontend/src/components/QuickFilters/FilterRenderers/Checkbox/Checkbox.tsx:447
- Draft comment:
Consider storing the conditionattributeValues.length > 0in a variable to avoid repetition and improve readability. - Reason this comment was not posted:
Confidence changes required:30%
TheCheckboxFiltercomponent has a section where it checks ifattributeValues.length > 0to render the values. However, this check is repeated multiple times in the component. It would be more efficient to store this condition in a variable and reuse it, improving readability and maintainability.
4. frontend/src/pages/LogsExplorer/index.tsx:119
- Draft comment:
Avoid using thecomponent/index.tsxfile structure approach. It makes it difficult to debug and find components using global search tools. This applies to other similar files as well. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_2TxkJUHL0ZClGF2U
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.
…MONITORING for consistency
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 4d80c36 in 20 seconds
More details
- Looked at
76lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. frontend/src/container/InfraMonitoringK8s/InfraMonitoringK8s.tsx:117
- Draft comment:
The PR description mentions adding a documentation link to quick filters, but the code changes do not reflect this. The changes only modify thesourceproperty ofQuickFilterscomponents. This does not resolve the issue described in the PR. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_7pl7eUAd5HiFYCJ1
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
amlannandy
approved these changes
Jan 24, 2025
| <EmptyQuickFilterIcon /> | ||
| </div> | ||
| <div className="go-to-docs__container-message"> | ||
| {`You'd need to parse out this attribute to start getting them as a fast |
There was a problem hiding this comment.
Maybe remove the back ticks if it's a simple string?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Related Issues / PR's
close #6846
Screenshots
2025-01-20.13-23-32.mov
Updated recording: #6854 (comment)
Affected Areas and Manually Tested Areas
Important
Adds documentation links for quick filters in logs explorer with new empty state visuals and source differentiation.
QuickFilterEmptyStatecomponent.QuickFilterEmptyStateopens relevant documentation section based onattributeKey.EmptyQuickFilterIconinEmptyQuickFilterIcon.tsxfor empty state visuals.QuickFilterEmptyStatecomponent inQuickFilterEmptyState.tsx.Checkbox.styles.scssto include styles for the new documentation link section.QuickFiltersSourceenum inQuickFilters.tsxto differentiate between sources likeLOGS_EXPLORERandINFRA_MONITORING.This description was created by
for 4d80c36. It will automatically update as commits are pushed.