feat: UI improvements in Query Management UI #4930
Conversation
|
Warning Rate limit exceeded@omkarK06 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 21 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis pull request introduces several changes across multiple Vue components related to query management. Key modifications include the addition of new components for managing running queries, updates to existing components to enhance data handling via props and events, and adjustments to routing logic. Additionally, new localization strings have been added to support the query interface. The overall functionality remains consistent, with a focus on improving the user interface and data management capabilities. Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (11)
web/src/composables/shared/useManagementRoutes.ts (1)
Line range hint
6-6: Consider improving type safety with proper TypeScript types.The routes array and route guard parameters are using
anytype. This reduces TypeScript's ability to catch potential issues at compile time.Consider using Vue Router's built-in types:
import { RouteRecordRaw, RouteLocationNormalized, NavigationGuardNext } from 'vue-router' const routes: RouteRecordRaw[] = [ // ... existing routes ] // For route guards: beforeEnter( to: RouteLocationNormalized, from: RouteLocationNormalized, next: NavigationGuardNext ) { routeGuard(to, from, next); }Also applies to: 44-51
web/src/components/queries/QueryList.vue (2)
95-95: Consider using named constants for time unit conversions.The division by 1000000 for microseconds conversion could be more readable with a named constant.
+const MICROSECONDS_PER_SECOND = 1000000; const getDuration = (createdAt: number) => { const currentTime = localTimeToMicroseconds(); - const durationInSeconds = Math.floor((currentTime - createdAt) / 1000000); + const durationInSeconds = Math.floor((currentTime - createdAt) / MICROSECONDS_PER_SECOND);
Line range hint
80-95: Consider consolidating time handling logic into a utility module.The component handles multiple time-related calculations with different units (microseconds, milliseconds, seconds). This complexity could be simplified by moving the time conversion logic into a dedicated utility module.
Consider creating a TimeUtils module that encapsulates these conversions:
// timeUtils.ts export class TimeUtils { static readonly MICROSECONDS_PER_SECOND = 1000000; static readonly MICROSECONDS_PER_MILLISECOND = 1000; static getCurrentMicroseconds(): number { return Date.now() * this.MICROSECONDS_PER_MILLISECOND; } static microsecondsToDuration(microseconds: number): number { return Math.floor(microseconds / this.MICROSECONDS_PER_SECOND); } }web/src/locales/languages/en.json (1)
936-936: Consider making the abbreviated text more clear.The string "Total Exec. Duration" uses an abbreviation that might not be immediately clear to all users. Consider either:
- Using the full word: "Total Execution Duration"
- Adding a period after "Exec" for consistency: "Total Exec. Duration"
- "totalDuration": "Total Exec. Duration", + "totalDuration": "Total Execution Duration",web/src/components/queries/RunningQueriesList.vue (3)
Line range hint
430-431: Remove Duplicate Entry inotherFieldOptionsThe
otherFieldOptionscomputed property contains a duplicate entry for"> 1 Month". This can cause confusion and should be corrected.Apply this diff to remove the duplicate entry:
const otherFieldOptions = computed(() => { filterQuery.value = ""; if (selectedSearchField.value === "exec_duration") { return [ { label: "> 1 second", value: "gt_1s" }, { label: "> 5 seconds", value: "gt_5s" }, { label: "> 15 seconds", value: "gt_15s" }, { label: "> 30 seconds", value: "gt_30s" }, { label: "> 1 minute", value: "gt_1m" }, { label: "> 5 minutes", value: "gt_5m" }, { label: "> 10 minutes", value: "gt_10m" }, ]; } else if (selectedSearchField.value === "query_range") { return [ { label: "> 5 minutes", value: "gt_5m" }, { label: "> 10 minutes", value: "gt_10m" }, { label: "> 15 minutes", value: "gt_15m" }, { label: "> 1 hour", value: "gt_1h" }, { label: "> 1 day", value: "gt_1d" }, { label: "> 1 week", value: "gt_1w" }, - { label: "> 1 Month", value: "gt_1M" }, { label: "> 1 Month", value: "gt_1M" }, ]; } });
Line range hint
271-277: UseconstInstead ofvarinlocalTimeToMicrosecondsFunctionUsing
constfor variable declarations where the value does not change enhances code clarity and prevents accidental reassignments.Apply this diff to update the variable declarations:
const localTimeToMicroseconds = () => { - var date = new Date(); + const date = new Date(); - var timestampMilliseconds = date.getTime(); + const timestampMilliseconds = date.getTime(); // Convert milliseconds to microseconds - var timestampMicroseconds = timestampMilliseconds * 1000; + const timestampMicroseconds = timestampMilliseconds * 1000; return timestampMicroseconds; };
266-268: Update Comment to Reflect Current FunctionalityThe comment inside the
listSchemafunction is outdated. It mentions passing data toschemaData, but the function emits an event instead.Update the comment to accurately reflect the code:
const listSchema = (props: any) => { - //pass whole props.row to schemaData + // Emit event to show schema for the selected row emit("show:schema", props.row); };web/src/components/queries/SummaryList.vue (1)
276-285: Use 'const' instead of 'var' for variable declarationsIn the
localTimeToMicrosecondsfunction,varis used to declare variables. It's recommended to useconstorletin modern JavaScript and TypeScript to prevent scope-related issues and enhance readability.Update the variable declarations to use
const:const localTimeToMicroseconds = () => { - var date = new Date(); - var timestampMilliseconds = date.getTime(); - var timestampMicroseconds = timestampMilliseconds * 1000; + const date = new Date(); + const timestampMilliseconds = date.getTime(); + const timestampMicroseconds = timestampMilliseconds * 1000; return timestampMicroseconds; };web/src/components/queries/RunningQueries.vue (3)
546-546: Remove unnecessaryconsole.logstatementThe
console.logstatement on line 546 appears to be for debugging purposes. It's best practice to remove such statements from production code to prevent unwanted console output and potential performance issues.Apply this diff to fix:
- console.log("item", item);
419-424: Ensure consistency between field names and labelsThe field
user_idis labeled ast("user.email"), which implies that it represents an email address. Ifuser_idactually contains email addresses, consider renaming the field to
565-574: Add missing duration option tootherFieldOptionsThe
timeMapincludes agt_10h(greater than 10 hours) key, but this option is missing from theotherFieldOptionsarray forexec_duration. Including this option will allow users to filter queries with execution durations exceeding 10 hours.Apply this diff to add the missing option:
{ label: "> 10 minutes", value: "gt_10m" }, + { + label: "> 10 hours", + value: "gt_10h" + }, { label: "> 15 minutes", value: "gt_15m" },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- web/src/components/queries/QueryList.vue (3 hunks)
- web/src/components/queries/RunningQueries.vue (1 hunks)
- web/src/components/queries/RunningQueriesList.vue (9 hunks)
- web/src/components/queries/SummaryList.vue (1 hunks)
- web/src/composables/shared/useManagementRoutes.ts (1 hunks)
- web/src/locales/languages/en.json (1 hunks)
🔇 Additional comments (5)
web/src/composables/shared/useManagementRoutes.ts (1)
44-44: Verify the new component path and functionality.The component import path has been updated from
RunningQueriesList.vuetoRunningQueries.vue. While this aligns with the PR's UI improvements, let's verify the new component exists and maintains backward compatibility.✅ Verification successful
Both old and new components exist, potential duplicate implementation
The verification reveals that both
RunningQueriesList.vueandRunningQueries.vuecurrently exist in the codebase, withRunningQueries.vueimporting and usingRunningQueriesList.vueas a child component. This is not a simple rename but rather a new parent component that wraps the existing functionality. The route change is correct, but there are some considerations:
web/src/components/queries/RunningQueries.vue- New parent componentweb/src/components/queries/RunningQueriesList.vue- Still used as a child componentweb/src/components/queries/SummaryList.vue- Has references to the list component🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the new component exists and check for any remaining references to the old component # Test 1: Verify new component exists fd RunningQueries.vue # Test 2: Check for any lingering references to old component name that might need updating rg -l "RunningQueriesList"Length of output: 235
Script:
#!/bin/bash # Let's check the content of these files to understand the relationship and potential issues # Check content of RunningQueries.vue to see if it's a renamed version rg -A 5 -B 5 "RunningQueriesList" web/src/components/queries/RunningQueries.vue # Check if RunningQueriesList.vue is still being used or just needs to be removed rg -A 5 -B 5 "RunningQueriesList" web/src/components/queries/SummaryList.vue # Check if the old component file exists and its content cat web/src/components/queries/RunningQueriesList.vueLength of output: 19147
web/src/components/queries/QueryList.vue (1)
67-67: LGTM!The addition of trailing commas improves code consistency.
Also applies to: 75-75
web/src/locales/languages/en.json (1)
934-938: LGTM! New localization strings align with Query Management UI improvements.The added strings follow consistent naming patterns and provide clear, meaningful labels for the query management interface.
web/src/components/queries/SummaryList.vue (1)
563-565: Verify compatibility of '::v-deep' in scoped stylesThe
::v-deepcombinator is used in the scoped styles. Depending on the version of Vue and your build setup, the syntax for deep selectors might differ.Please ensure that the usage of
::v-deepis compatible with your project's Vue version and tooling. If there are compatibility issues, consider updating the syntax as per the official Vue documentation.web/src/components/queries/RunningQueries.vue (1)
448-451: Verify the correctness of thework_groupfield in Query TypeThe column labeled "Query Type" uses the field
work_group. Please ensure that thework_groupfield properly represents the query type. If there's a more appropriate field for query type, consider updating the field reference to reflect accurate data.
| if (config.isEnterprise == "true") { | ||
| routes[0].children.push( | ||
| ...[ | ||
| { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving the enterprise feature flag check.
The string comparison config.isEnterprise == "true" is prone to errors. Using a boolean would be more robust.
Consider converting the config value to a boolean:
if (config.isEnterprise === "true" || config.isEnterprise === true) {Or better yet, update the aws-exports configuration to return a boolean instead of a string.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
web/src/components/queries/RunningQueriesList.vue (1)
80-80: Simplify optional chainingThe optional chaining operator (
?.) is unnecessary here sinceselectedRowis a computed property that always returns an array fromprops.selectedRows.-:disable="selectedRow?.length === 0" +:disable="selectedRow.length === 0"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- web/src/components/queries/RunningQueries.vue (1 hunks)
- web/src/components/queries/RunningQueriesList.vue (10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/components/queries/RunningQueries.vue
🔇 Additional comments (6)
web/src/components/queries/RunningQueriesList.vue (6)
134-150: LGTM! Well-structured component definitionThe component setup follows Vue 3 best practices with properly typed props and clearly defined emit events.
164-230: Remove unused variables and mock dataThis code segment contains unused variables and mock data that should be removed.
266-266: LGTM! Clean event emissionThe schema handling is properly implemented using event emission.
468-473: Rename selectedRow to selectedRows for consistencyThe naming inconsistency was previously identified and should be addressed.
513-515: Remove unused functionThe
onChangeQueryTabfunction is not used in the component.
553-559: LGTM! Well-structured stylesThe styles are properly scoped and follow Vue best practices.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
web/src/components/queries/RunningQueriesList.vue (2)
80-80: Simplify the disable condition.
The optional chaining operator (?.) is unnecessary here since selectedRowsModel is a computed property that will always return an array (even if empty).
-:disable="selectedRowsModel?.length === 0"
+:disable="selectedRowsModel.length === 0"124-140: Great architectural improvements!
The component has been effectively refactored to:
- Use props and events instead of internal state management
- Follow a clear event-driven architecture
- Maintain separation of concerns
These changes improve maintainability and reusability.
Also applies to: 247-251, 255-259
web/src/components/queries/SummaryList.vue (1)
274-286: Remove unused styles to simplify the stylesheet.
The styles defined for .query-management-tabs, .label-container, .search-input, and .search-field-select are not used in the template. Consider removing them to keep the codebase clean and maintainable.
Also applies to: 288-302
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- web/src/components/queries/QueryList.vue (3 hunks)
- web/src/components/queries/RunningQueries.vue (1 hunks)
- web/src/components/queries/RunningQueriesList.vue (6 hunks)
- web/src/components/queries/SummaryList.vue (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- web/src/components/queries/QueryList.vue
- web/src/components/queries/RunningQueries.vue
🧰 Additional context used
📓 Learnings (1)
web/src/components/queries/SummaryList.vue (2)
Learnt from: omkarK06
PR: openobserve/openobserve#4930
File: web/src/components/queries/SummaryList.vue:476-478
Timestamp: 2024-10-29T08:49:45.513Z
Learning: In `SummaryList.vue`, the data rows contain `trace_ids`, which is a list of grouped queries.
Learnt from: omkarK06
PR: openobserve/openobserve#4930
File: web/src/components/queries/SummaryList.vue:480-482
Timestamp: 2024-10-29T08:47:07.675Z
Learning: In `SummaryList.vue` (Vue.js component), the `handleMultiQueryCancel` function emits the `delete:queries` event without passing the `trace_id`s because the parent component has access to `selectedRows` and can obtain the `trace_id`s from them. Therefore, it's unnecessary to pass `trace_id`s explicitly when emitting the `delete:queries` event.
🔇 Additional comments (1)
web/src/components/queries/RunningQueriesList.vue (1)
126-134: LGTM! Well-structured component communication.
The component follows Vue.js best practices:
- Props are properly typed and documented
- Events are clearly defined
- Two-way binding is correctly implemented using computed property
Also applies to: 135-140, 247-251
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (4)
web/src/components/queries/RunningQueriesList.vue (3)
80-80: Simplify disable condition
The optional chaining operator is unnecessary here as selectedRowsModel is a computed property that will always return an array (either the provided selectedRows or undefined).
-:disable="selectedRowsModel?.length === 0"
+:disable="!selectedRowsModel.length"126-134: Add default value and improve type safety for props
The selectedRows prop should have a default value to ensure consistent behavior when the prop is not provided.
rows: {
type: Array,
required: true,
+ default: () => []
},
selectedRows: {
type: Array,
required: false,
+ default: () => [],
+ validator: (value: unknown[]) => Array.isArray(value)
},289-293: Avoid using !important in CSS
Using !important is generally discouraged as it makes styles harder to override and maintain. Consider increasing selector specificity or restructuring the CSS.
.query-management-tabs {
:deep(.q-btn:before) {
- border: none !important;
+ border: none;
}
}web/src/components/queries/SummaryList.vue (1)
219-221: Rename getAllUserQueries to filterQueries for clarity
The function getAllUserQueries emits a filter event but doesn't fetch queries. Renaming it to filterQueries improves readability.
Apply this diff to rename the function and update its usage:
- const getAllUserQueries = (event: any, row: any) => {
+ const filterQueries = (event: any, row: any) => {
emit("filter:queries", row);
}; <q-table
data-test="running-queries-table"
ref="qTable"
:rows="rows"
:columns="columns"
:pagination="pagination"
row-key="row_id"
style="width: 100%"
selection="multiple"
v-model:selected="selectedRow"
- @row-click="getAllUserQueries"
+ @row-click="filterQueries"
>Also applies to: 29-29
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- web/src/components/queries/RunningQueriesList.vue (6 hunks)
- web/src/components/queries/SummaryList.vue (1 hunks)
🧰 Additional context used
📓 Learnings (2)
web/src/components/queries/RunningQueriesList.vue (1)
Learnt from: omkarK06
PR: openobserve/openobserve#4930
File: web/src/components/queries/RunningQueriesList.vue:38-38
Timestamp: 2024-10-29T10:34:25.361Z
Learning: In `web/src/components/queries/RunningQueriesList.vue`, the padding of rows in the Query Management tables has been reduced, so `size="xs"` is used for checkboxes to match the reduced row padding.
web/src/components/queries/SummaryList.vue (3)
Learnt from: omkarK06
PR: openobserve/openobserve#4930
File: web/src/components/queries/SummaryList.vue:476-478
Timestamp: 2024-10-29T08:49:45.513Z
Learning: In `SummaryList.vue`, the data rows contain `trace_ids`, which is a list of grouped queries.
Learnt from: omkarK06
PR: openobserve/openobserve#4930
File: web/src/components/queries/SummaryList.vue:480-482
Timestamp: 2024-10-29T08:47:07.675Z
Learning: In `SummaryList.vue` (Vue.js component), the `handleMultiQueryCancel` function emits the `delete:queries` event without passing the `trace_id`s because the parent component has access to `selectedRows` and can obtain the `trace_id`s from them. Therefore, it's unnecessary to pass `trace_id`s explicitly when emitting the `delete:queries` event.
Learnt from: omkarK06
PR: openobserve/openobserve#4930
File: web/src/components/queries/RunningQueriesList.vue:38-38
Timestamp: 2024-10-29T10:34:25.361Z
Learning: In `web/src/components/queries/RunningQueriesList.vue`, the padding of rows in the Query Management tables has been reduced, so `size="xs"` is used for checkboxes to match the reduced row padding.
| const confirmDeleteAction = (props: any) => { | ||
| deleteDialog.value.data = [props.row.trace_id]; | ||
| deleteDialog.value.show = true; | ||
| emit("delete:query", props.row); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper typing for event handlers
The event handler parameters should be properly typed for better type safety.
-const confirmDeleteAction = (props: any) => {
+interface QueryRowProps {
+ row: {
+ trace_id: string;
+ [key: string]: unknown;
+ };
+}
+
+const confirmDeleteAction = (props: QueryRowProps) => {
emit("delete:query", props.row);
};
const handleMultiQueryCancel = () => {
emit("delete:queries");
};Also applies to: 258-260
| emits: [ | ||
| "update:selectedRows", | ||
| "delete:queries", | ||
| "delete:query", | ||
| "show:schema", | ||
| ], |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add TypeScript types for emitted events
Define types for the emitted values to improve type safety and documentation.
-emits: [
- "update:selectedRows",
- "delete:queries",
- "delete:query",
- "show:schema",
-],
+emits: {
+ 'update:selectedRows': (rows: unknown[]) => Array.isArray(rows),
+ 'delete:queries': () => true,
+ 'delete:query': (query: unknown) => !!query,
+ 'show:schema': (schema: unknown) => !!schema
+},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| emits: [ | |
| "update:selectedRows", | |
| "delete:queries", | |
| "delete:query", | |
| "show:schema", | |
| ], | |
| emits: { | |
| 'update:selectedRows': (rows: unknown[]) => Array.isArray(rows), | |
| 'delete:queries': () => true, | |
| 'delete:query': (query: unknown) => !!query, | |
| 'show:schema': (schema: unknown) => !!schema | |
| }, |
| const qTable: Ref<InstanceType<typeof QTable> | null> = ref(null); | ||
| const { t } = useI18n(); | ||
| const showListSchemaDialog = ref(false); |
There was a problem hiding this comment.
Remove unused variable showListSchemaDialog
The variable showListSchemaDialog is defined but never used in the component. It can be removed to reduce clutter.
Apply this diff to remove the unused variable:
- const showListSchemaDialog = ref(false); return {
t,
columns,
confirmDeleteAction,
perPageOptions,
changePagination,
outlinedCancel,
loadingState,
isMetaOrg,
resultTotal,
selectedPerPage,
qTable,
selectedRow,
handleMultiQueryCancel,
pagination,
getAllUserQueries,
- showListSchemaDialog,
};Also applies to: 229-229
omkarK06
force-pushed
the
feat/query_management_ui_improvements
branch
from
00569c2 to
fed1949
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
web/src/components/queries/RunningQueriesList.vue (3)
Line range hint 45-63: Add accessibility attributes to action buttons.
The action buttons lack proper accessibility attributes which could make it difficult for screen reader users.
Apply this diff to improve accessibility:
<q-btn
icon="list_alt"
:title="t('queries.queryList')"
+ :aria-label="t('queries.queryList')"
class="q-ml-xs"
padding="sm"
unelevated
size="sm"
round
flat
+ :loading="loadingState"
@click="listSchema(props)"
data-test="queryList-btn"
/>
<q-btn
:icon="outlinedCancel"
:title="t('queries.cancelQuery')"
+ :aria-label="t('queries.cancelQuery')"
class="q-ml-xs"
padding="sm"
unelevated
size="sm"
style="color: red"
round
flat
+ :loading="loadingState"
@click="confirmDeleteAction(props)"
data-test="cancelQuery-btn"
/>Line range hint 166-181: Improve type safety and configuration of pagination options.
The pagination options are currently using any type and have hardcoded values. Consider making these configurable via props and adding proper typing.
Apply this diff to improve the implementation:
+interface PaginationOption {
+ label: string;
+ value: number;
+}
+
+const DEFAULT_PAGINATION_OPTIONS: PaginationOption[] = [
+ { label: "5", value: 5 },
+ { label: "10", value: 10 },
+ { label: "20", value: 20 },
+ { label: "50", value: 50 },
+ { label: "100", value: 100 },
+];
+
-const perPageOptions: any = [
- { label: "5", value: 5 },
- { label: "10", value: 10 },
- { label: "20", value: 20 },
- { label: "50", value: 50 },
- { label: "100", value: 100 },
-];
+const perPageOptions = computed(() =>
+ props.paginationOptions || DEFAULT_PAGINATION_OPTIONS
+);Also update the props:
props: {
// ... existing props
paginationOptions: {
type: Array as PropType<PaginationOption[]>,
required: false,
},
},Line range hint 158-164: Consider moving schema dialog state to props/emits.
The schema dialog state is managed internally but could be controlled by the parent component for better flexibility.
Apply this diff to improve component composition:
props: {
// ... existing props
+ showSchemaDialog: {
+ type: Boolean,
+ required: false,
+ default: false,
+ },
},
emits: [
// ... existing emits
+ "update:showSchemaDialog",
],
setup(props, { emit }) {
- const showListSchemaDialog = ref(false);
+ const showListSchemaDialog = computed({
+ get: () => props.showSchemaDialog,
+ set: (value) => emit("update:showSchemaDialog", value)
+ });
const listSchema = (props: any) => {
emit("show:schema", props.row);
};📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- web/src/components/queries/QueryList.vue (3 hunks)
- web/src/components/queries/RunningQueries.vue (1 hunks)
- web/src/components/queries/RunningQueriesList.vue (7 hunks)
- web/src/components/queries/SummaryList.vue (1 hunks)
- web/src/composables/shared/useManagementRoutes.ts (1 hunks)
- web/src/locales/languages/en.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- web/src/components/queries/QueryList.vue
- web/src/components/queries/RunningQueries.vue
- web/src/components/queries/SummaryList.vue
- web/src/composables/shared/useManagementRoutes.ts
- web/src/locales/languages/en.json
🧰 Additional context used
📓 Learnings (1)
web/src/components/queries/RunningQueriesList.vue (1)
Learnt from: omkarK06
PR: openobserve/openobserve#4930
File: web/src/components/queries/RunningQueriesList.vue:38-38
Timestamp: 2024-10-29T10:34:25.361Z
Learning: In `web/src/components/queries/RunningQueriesList.vue`, the padding of rows in the Query Management tables has been reduced, so `size="xs"` is used for checkboxes to match the reduced row padding.
🔇 Additional comments (2)
web/src/components/queries/RunningQueriesList.vue (2)
1-15: LGTM: License header is properly formatted.
The GNU Affero General Public License v3 header is complete and correctly formatted.
Line range hint 295-324: LGTM: Styles are clean and well-organized.
The styles are properly scoped where needed and use the correct selector syntax.
omkarK06
force-pushed
the
feat/query_management_ui_improvements
branch
from
2958b3c to
0b0c501
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
web/src/components/queries/SummaryList.vue (3)
76-86: Consider adding type safety to pagination props.
The pagination implementation uses any type for scope. Consider defining an interface for the pagination scope to improve type safety.
- <q-table-pagination
- data-test="query-stream-table-pagination"
- :scope="scope"
- :resultTotal="rows.length"
- :perPageOptions="perPageOptions"
- position="bottom"
- @update:changeRecordPerPage="changePagination"
- class="fit"
- />
+ <q-table-pagination
+ data-test="query-stream-table-pagination"
+ :scope="scope as PaginationScope"
+ :resultTotal="rows.length"
+ :perPageOptions="perPageOptions"
+ position="bottom"
+ @update:changeRecordPerPage="changePagination"
+ class="fit"
+ />
interface PaginationScope {
pagination: {
page: number;
rowsPerPage: number;
rowsNumber: number;
};
pagesNumber: number;
}104-113: Define specific interfaces for props.
The rows and selectedRows props use generic Array type. Consider defining specific interfaces for better type safety.
+ interface QueryRow {
+ row_id: string;
+ user_id: string;
+ search_type_label: string;
+ numOfQueries: number;
+ duration: string;
+ queryRange: string;
+ trace_ids: string[];
+ }
props: {
rows: {
- type: Array,
+ type: Array as PropType<QueryRow[]>,
required: true,
},
selectedRows: {
- type: Array,
+ type: Array as PropType<QueryRow[]>,
required: true,
},
},219-221: Add type safety to event handler parameters.
The getAllUserQueries function uses any type for its parameters. Consider adding specific types.
- const getAllUserQueries = (event: any, row: any) => {
+ const getAllUserQueries = (event: Event, row: QueryRow) => {
emit("filter:queries", row);
};web/src/components/queries/RunningQueriesList.vue (1)
80-80: Add null check for selectedRowsModel
The disable condition could be more robust by adding a null check.
-:disable="selectedRowsModel?.length === 0"
+:disable="!selectedRowsModel?.length"📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- web/src/components/queries/QueryList.vue (3 hunks)
- web/src/components/queries/RunningQueries.vue (1 hunks)
- web/src/components/queries/RunningQueriesList.vue (8 hunks)
- web/src/components/queries/SummaryList.vue (1 hunks)
- web/src/composables/shared/useManagementRoutes.ts (1 hunks)
- web/src/locales/languages/en.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- web/src/components/queries/QueryList.vue
- web/src/components/queries/RunningQueries.vue
- web/src/composables/shared/useManagementRoutes.ts
- web/src/locales/languages/en.json
🧰 Additional context used
📓 Learnings (2)
web/src/components/queries/RunningQueriesList.vue (1)
Learnt from: omkarK06
PR: openobserve/openobserve#4930
File: web/src/components/queries/RunningQueriesList.vue:38-38
Timestamp: 2024-10-29T10:34:25.361Z
Learning: In `web/src/components/queries/RunningQueriesList.vue`, the padding of rows in the Query Management tables has been reduced, so `size="xs"` is used for checkboxes to match the reduced row padding.
web/src/components/queries/SummaryList.vue (3)
Learnt from: omkarK06
PR: openobserve/openobserve#4930
File: web/src/components/queries/SummaryList.vue:476-478
Timestamp: 2024-10-29T08:49:45.513Z
Learning: In `SummaryList.vue`, the data rows contain `trace_ids`, which is a list of grouped queries.
Learnt from: omkarK06
PR: openobserve/openobserve#4930
File: web/src/components/queries/SummaryList.vue:480-482
Timestamp: 2024-10-29T08:47:07.675Z
Learning: In `SummaryList.vue` (Vue.js component), the `handleMultiQueryCancel` function emits the `delete:queries` event without passing the `trace_id`s because the parent component has access to `selectedRows` and can obtain the `trace_id`s from them. Therefore, it's unnecessary to pass `trace_id`s explicitly when emitting the `delete:queries` event.
Learnt from: omkarK06
PR: openobserve/openobserve#4930
File: web/src/components/queries/RunningQueriesList.vue:38-38
Timestamp: 2024-10-29T10:34:25.361Z
Learning: In `web/src/components/queries/RunningQueriesList.vue`, the padding of rows in the Query Management tables has been reduced, so `size="xs"` is used for checkboxes to match the reduced row padding.
🔇 Additional comments (5)
web/src/components/queries/SummaryList.vue (2)
1-16: LGTM: License header is properly included.
The AGPL v3 license header is complete and correctly formatted.
17-90: LGTM: Well-structured template with proper component organization.
The template follows Vue.js and Quasar best practices:
- Proper conditional rendering
- Well-organized table slots
- Consistent data-test attributes for testing
- Appropriate checkbox size as per UI requirements
web/src/components/queries/RunningQueriesList.vue (3)
1-15: LGTM: License header is properly added
The GNU AGPL v3 license header is complete and correctly formatted.
204-210: LGTM: New search_type column is properly added
The search_type column is correctly integrated with proper alignment and sorting capabilities.
295-301: LGTM: Styles follow Vue 3 conventions
The :deep() selector is correctly used, replacing the deprecated ::v-deep syntax.
| const deleteDialog = ref({ | ||
| show: false, | ||
| title: "Delete Running Query", | ||
| message: "Are you sure you want to delete this running query?", | ||
| data: null as any, | ||
| }); |
There was a problem hiding this comment.
Remove unused variables.
The following variables are defined but never used:
deleteDialogshowListSchemaDialog
These can be safely removed to improve code maintainability.
- const deleteDialog = ref({
- show: false,
- title: "Delete Running Query",
- message: "Are you sure you want to delete this running query?",
- data: null as any,
- });
- const showListSchemaDialog = ref(false);Also applies to: 135-135
| props: { | ||
| rows: { | ||
| type: Array, | ||
| required: true, | ||
| }, | ||
| selectedRows: { | ||
| type: Array, | ||
| required: false, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add specific types for props
The props could benefit from more specific typing.
props: {
rows: {
- type: Array,
+ type: Array as PropType<RunningQuery[]>,
required: true,
},
selectedRows: {
- type: Array,
+ type: Array as PropType<RunningQuery[]>,
required: false,
+ default: () => [],
},
},Add this interface above the component:
interface RunningQuery {
trace_id: string;
user_id: string;
org_id: string;
search_type: string;
duration: string;
queryRange: string;
work_group: string;
status: string;
stream_type: string;
}
omkarK06
force-pushed
the
feat/query_management_ui_improvements
branch
from
0b0c501 to
8924e56
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
web/src/components/queries/RunningQueriesList.vue (2)
21-27: Consider adding type for pagination prop.The pagination prop lacks type information which could lead to runtime errors.
:rows="rows" :columns="columns" -:pagination="pagination" +:pagination="pagination as QTableProps['pagination']" row-key="trace_id"
163-163: Add type safety and validation for schema data.The schema handling could be improved with proper typing and validation.
-const listSchema = (props: any) => { +interface SchemaProps { + row: { + trace_id: string; + query?: { + sql?: string; + }; + }; +} + +const listSchema = (props: SchemaProps) => { + if (!props.row?.query?.sql) { + console.warn('Invalid schema data'); + return; + } emit("show:schema", props.row); };web/src/components/queries/RunningQueries.vue (3)
67-68: Use distinct data-test attributes for different inputs.The same data-test attribute is used for both the text input and select components. This could make test maintenance and debugging more difficult.
- data-test="running-queries-search-input" + data-test="running-queries-search-text-input"And for the select input:
- data-test="running-queries-search-input" + data-test="running-queries-search-select-input"Also applies to: 85-86
82-82: Use i18n for the select label.The label "Select option" is hardcoded in English. Use the translation system for consistency.
- label="Select option" + :label="t('queries.selectOption')"
669-698: Improve error handling in deleteQuery.The function could benefit from more specific error handling and better type safety.
const deleteQuery = () => { + const traceIds = deleteDialog.value.data; + if (!traceIds?.length) { + q.notify({ + message: "No queries selected for deletion", + color: "warning", + position: "bottom", + timeout: 1500, + }); + return; + } SearchService.delete_running_queries( store.state.zoConfig.meta_org, - deleteDialog.value.data, + traceIds, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
web/src/components/queries/QueryList.vue(3 hunks)web/src/components/queries/RunningQueries.vue(1 hunks)web/src/components/queries/RunningQueriesList.vue(8 hunks)web/src/components/queries/SummaryList.vue(1 hunks)web/src/composables/shared/useManagementRoutes.ts(1 hunks)web/src/locales/languages/en.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- web/src/components/queries/QueryList.vue
- web/src/components/queries/SummaryList.vue
- web/src/composables/shared/useManagementRoutes.ts
- web/src/locales/languages/en.json
🧰 Additional context used
📓 Learnings (2)
web/src/components/queries/RunningQueries.vue (1)
Learnt from: omkarK06
PR: openobserve/openobserve#4930
File: web/src/components/queries/RunningQueriesList.vue:38-38
Timestamp: 2024-10-29T10:34:25.361Z
Learning: In `web/src/components/queries/RunningQueriesList.vue`, the padding of rows in the Query Management tables has been reduced, so `size="xs"` is used for checkboxes to match the reduced row padding.
web/src/components/queries/RunningQueriesList.vue (1)
Learnt from: omkarK06
PR: openobserve/openobserve#4930
File: web/src/components/queries/RunningQueriesList.vue:38-38
Timestamp: 2024-10-29T10:34:25.361Z
Learning: In `web/src/components/queries/RunningQueriesList.vue`, the padding of rows in the Query Management tables has been reduced, so `size="xs"` is used for checkboxes to match the reduced row padding.
🔇 Additional comments (3)
web/src/components/queries/RunningQueriesList.vue (3)
1-15: LGTM: License header is properly added.
The GNU Affero General Public License header is correctly formatted and includes all required sections.
Line range hint 80-90: LGTM: Multi-cancel button implementation.
The multi-cancel button correctly:
- Uses computed property for disable state
- Handles pagination updates
- Emits events for query cancellation
204-210: LGTM: Search type column properly defined.
The new column follows the established pattern and includes all necessary properties.
#4936
Summary by CodeRabbit
Release Notes
New Features
Improvements
Localization