Copilot commented on code in PR #16534: URL: https://github.com/apache/pinot/pull/16534#discussion_r2259081673
########## pinot-controller/src/main/resources/app/components/Homepage/PackageVersions.tsx: ########## @@ -0,0 +1,66 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import React, { useEffect, useState } from 'react'; +import { TableData } from 'Models'; +import AppLoader from '../AppLoader'; +import CustomizedTables from '../Table'; +import PinotMethodUtils from '../../utils/PinotMethodUtils'; + +const PackageVersions = () => { + + const [fetching, setFetching] = useState(true); + const [error, setError] = useState<string | null>(null); + const [tableData, setTableData] = useState<TableData>({ + columns: [], + records: [] + }); + + const fetchData = async () => { + try { + const result = await PinotMethodUtils.getPackageVersionsData(); + setTableData(result); + setError(null); + } catch (err: any) { + setError(err?.message || 'Failed to fetch package versions'); + setTableData({ + columns: ['Package', 'Version'], + records: [], + error: err?.message || 'Failed to fetch package versions' Review Comment: Using 'any' type for error handling reduces type safety. Consider using a more specific error type or 'unknown' and then type-checking the error properties. ```suggestion } catch (err: unknown) { const errorMessage = typeof err === 'object' && err !== null && 'message' in err && typeof (err as any).message === 'string' ? (err as { message: string }).message : 'Failed to fetch package versions'; setError(errorMessage); setTableData({ columns: ['Package', 'Version'], records: [], error: errorMessage ``` ########## pinot-controller/src/main/resources/app/components/Homepage/PackageVersions.tsx: ########## @@ -0,0 +1,66 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import React, { useEffect, useState } from 'react'; +import { TableData } from 'Models'; +import AppLoader from '../AppLoader'; +import CustomizedTables from '../Table'; +import PinotMethodUtils from '../../utils/PinotMethodUtils'; + +const PackageVersions = () => { + Review Comment: [nitpick] There's an extra blank line after the function declaration. While not critical, removing it would improve code consistency. ```suggestion ``` ########## pinot-controller/src/main/resources/app/utils/PinotMethodUtils.ts: ########## @@ -1351,6 +1352,23 @@ const getAuthUserEmailFromAccessToken = ( return email; }; +// This method is used to display package versions in tabular format on cluster manager home page +// API: /version +// Expected Output: {columns: [], records: []} Review Comment: The comment should include information about error handling and the expected data structure from the API response. Consider documenting what happens when the API returns an empty object or fails. ```suggestion /** * This method is used to display package versions in tabular format on the cluster manager home page. * API: /version * * Expected Output: { columns: string[], records: string[][] } * - columns: An array of column names, e.g., ['Package', 'Version']. * - records: An array of [packageName, version] pairs as strings. * * Error Handling: * - If the API returns an empty object, the function returns { columns: ['Package', 'Version'], records: [] }. * - If the API call fails (e.g., network error, server error), the returned promise will reject with the error from getVersions(). */ ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
