-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
fix: added a filter on initial log fetch based on activelogid param #7109
base: main
Are you sure you want to change the base?
Conversation
Sahil seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
There was a problem hiding this 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 c2f4be4 in 1 minute and 40 seconds
More details
- Looked at
92
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
9
drafted comments based on config settings.
1. frontend/src/components/Logs/RawLogView/styles.ts:48
- Draft comment:
Updated raw log highlight colors now use Color.BG_ROBIN_600 and Color.BG_VANILLA_400. Ensure these design tokens reflect the intended design for dark/light mode consistency. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =30%
<= threshold50%
The comment is asking to ensure that the design tokens reflect the intended design for dark/light mode consistency. This is a request for confirmation, which violates the rule against asking the PR author to confirm their intention or ensure behavior. However, it does mention the use of design tokens, which aligns with the additional rules about using predefined color constants. Despite this, the comment's main focus is on ensuring intention, which is not allowed.
2. frontend/src/container/LogsExplorerViews/index.tsx:318
- Draft comment:
Appending a new filter for activeLogId using operator '≤'. Verify if '<=' is the correct operator for an exact match, and consider using a more explicit equality operator if intended. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
The use of '<=' seems intentional here as part of pagination logic. When paginating through logs, we want all logs with IDs less than or equal to the activeLogId to ensure we don't miss any logs. An exact match would only return one log. The comment appears to misunderstand the pagination purpose of this filter.
Could there be edge cases where an exact match would be more appropriate? Maybe the activeLogId has special semantics I'm not aware of?
The code context strongly suggests this is part of pagination logic - it's inside getPaginationQueryData and is used alongside page size parameters. The '<=' operator is a standard approach for cursor-based pagination.
The comment should be deleted. The '<=' operator appears to be intentionally used for pagination purposes, and changing it to '==' would break the pagination functionality.
3. frontend/src/container/LogsExplorerViews/index.tsx:361
- Draft comment:
Good update in dependency array for useCallback to include activeLogId. This helps ensure the filter is updated when the activeLogId changes. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative, explaining why a change was made. It doesn't provide a suggestion or ask for clarification, which violates the rules.
4. frontend/src/hooks/logs/configs.ts:1
- Draft comment:
Delay increased from 10s to 120s. Confirm that a 120-second HIGHLIGHTED_DELAY meets the application requirements. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is asking the PR author to confirm their intention regarding the change in delay from 10s to 120s. It is not a specific code suggestion or a request for a test, and it doesn't align with the additional rules provided. Therefore, it should be removed.
5. frontend/src/utils/logs.ts:12
- Draft comment:
Default log background now uses BG_VANILLA_400 instead of BG_VANILLA_300. Double-check that this change conforms to the updated design guidelines. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. frontend/src/components/Logs/RawLogView/styles.ts:48
- Draft comment:
Ensure the updated highlight colors (Color.BG_ROBIN_600 for dark mode and Color.BG_VANILLA_400 for light mode) align with the design guidelines and theming. Confirm that these design token values are the intended ones for highlighted logs. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
7. frontend/src/container/LogsExplorerViews/index.tsx:318
- Draft comment:
Review the activeLogId filter: using OPERATORS['<='] for the 'id' field seems unusual. Confirm if an equality operator was intended, unless range filtering is desired. - Reason this comment was not posted:
Marked as duplicate.
8. frontend/src/hooks/logs/configs.ts:1
- Draft comment:
The HIGHLIGHTED_DELAY value was increased from 10000 to 120000. Please confirm that this 120-second delay is intentional and aligns with the desired UX for log highlighting. - Reason this comment was not posted:
Marked as duplicate.
9. frontend/src/utils/logs.ts:41
- Draft comment:
getActiveLogBackground has been updated to use Color.BG_VANILLA_400 in non-dark mode instead of Color.BG_VANILLA_300. Confirm that this change matches the updated design specifications for log backgrounds. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
Workflow ID: wflow_t6teW1RUz4XxDJE2
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Summary
Resolved : 6740
Related Issues / PR's
Screenshots
NA
Affected Areas and Manually Tested Areas
Important
Adds
activeLogId
filter to log fetching and updates log background colors.activeLogId
ingetRequestData()
inindex.tsx
to fetch logs based on this parameter.styles.ts
andlogs.ts
.logs.ts
.HIGHLIGHTED_DELAY
to 120000 inconfigs.ts
.This description was created by
for c2f4be4. It will automatically update as commits are pushed.