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

Component table improvements #88

Merged
merged 8 commits into from
Jul 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 11 additions & 8 deletions src/js/components/CRUD/CRUD.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {
} from '..'
import { Columns } from '../../schema'
import { Context } from '../../state'
import { httpGet, httpDelete } from '../../utils'
import { httpGet, httpDelete, fetchPages } from '../../utils'

import { Form } from './Form'

Expand Down Expand Up @@ -131,14 +131,17 @@ function CRUD({
// Fetch the collection data
useEffect(() => {
if (fetchData === true) {
const newData = []
setFetchData(false)
const url = new URL(collectionPath, state.baseURL)
httpGet(
state.fetch,
url,
({ data }) => {
setData(data)
setReady(true)
fetchPages(
collectionPath,
state,
(data, finished) => {
newData.push(...data)
if (finished === true) {
setReady(true)
setData(newData)
}
},
({ message }) => {
setErrorMessage(message)
Expand Down
23 changes: 13 additions & 10 deletions src/js/components/Report/Report.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@ import { Column } from '../../schema'
import { useContext } from 'react'
import { Context } from '../../state'
import { useTranslation } from 'react-i18next'
import { httpGet } from '../../utils'
import { fetchPages, httpGet } from '../../utils'
import { Alert } from '../Alert/Alert'
import { Loading } from '../Loading/Loading'
import { ContentArea } from '../ContentArea/ContentArea'
import { Table } from '../Table'
import { NavigableTable, Table } from '../Table'
import { Markdown } from '../Markdown/Markdown'

/**
Expand Down Expand Up @@ -114,15 +114,18 @@ function Report({

useEffect(() => {
if (!fetched) {
httpGet(
state.fetch,
new URL(endpoint, state.baseURL),
({ data }) => {
setReportData(data)
setErrorMessage(null)
setFetched(true)
fetchPages(
endpoint,
state,
(data, isComplete) => {
setReportData((prevState) => prevState.concat(data))
if (isComplete) {
setFetched(true)
}
},
({ message }) => setErrorMessage(message)
(message) => {
setErrorMessage(message)
}
)
}
}, [fetched])
Expand Down
33 changes: 32 additions & 1 deletion src/js/components/Table/NavigableTable.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,13 @@ import { Icon } from '../Icon/Icon'
*
* @param columns passed as-is to Table
* @param data array of data objects, passed to Table and used for navigation
* @param defaultSort optional column name that the data is sorted by
* @param defaultSortDirection optional "sort direction" in use if defaultSort
* is included, defaults to "asc"
* @param extractSearchParam function that returns the property from data[n] used in
* the search params for tracking
* @param onSortChange function to call when the user selects a different sort order
* for the table.
* @param selectedIndex index of the current value in data
* @param setSelectedIndex setter used for next/previous
* @param slideOverElement element to use as the root of the content in the slide over
Expand All @@ -22,14 +27,21 @@ import { Icon } from '../Icon/Icon'
function NavigableTable({
columns,
data,
defaultSort,
defaultSortDirection,
extractSearchParam,
onSortChange,
selectedIndex,
setSelectedIndex,
slideOverElement,
title
}) {
const [searchParams, setSearchParams] = useSearchParams()

const [sort, setSort] = useState([
defaultSort || '',
defaultSortDirection || 'asc'
])
const [showArrows, setShowArrows] = useState(false)
const [slideOverOpen, setSlideOverOpen] = useState(false)
const arrowLeftRef = useRef(null)
Expand Down Expand Up @@ -63,10 +75,26 @@ function NavigableTable({
updateSearchParamsWith(extractSearchParam(data[index]))
}

function onSortDirection(column, direction) {
const [curCol, curDir] = sort
if (curCol !== column || curDir !== direction) {
setSort([column, direction])
onSortChange(column, direction)
}
}

const augmentedColumns = onSortChange
? columns.map((column) => ({
...column,
sortCallback: onSortDirection,
sortDirection: sort[0] === column.name ? sort[1] : null
}))
: columns

return (
<div className="m0">
<Table
columns={columns}
columns={augmentedColumns}
data={data}
onRowClick={({ index }) => {
updateSearchParamsWith(extractSearchParam(data[index]))
Expand Down Expand Up @@ -147,7 +175,10 @@ function NavigableTable({
NavigableTable.propTypes = {
columns: Columns,
data: PropTypes.arrayOf(PropTypes.object),
defaultSort: PropTypes.string,
defaultSortDirection: PropTypes.string,
extractSearchParam: PropTypes.func.isRequired,
onSortChange: PropTypes.func,
selectedIndex: PropTypes.number.isRequired,
setSelectedIndex: PropTypes.func.isRequired,
slideOverElement: PropTypes.node.isRequired,
Expand Down
15 changes: 15 additions & 0 deletions src/js/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,21 @@ export async function httpRequest(fetchMethod, path, options = requestOptions) {
}
}

export function fetchPages(path, { fetch, baseURL }, onDataReceived, onError) {
const localFetch = (resource) => {
httpGet(fetch, new URL(resource, baseURL), onSuccess, onError)
}
const onSuccess = ({ data, headers }) => {
const links = parseLinkHeader(headers.get('Link'))
const nextLink = Object.hasOwn(links, 'next') ? links.next[0] : null
onDataReceived(data, nextLink === null)
if (nextLink !== null) {
setTimeout(localFetch, 25, nextLink)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why after a timeout? Since this is onSuccess, the previous request has already completed. Is there a reason to wait a little longer on top of that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that I did this to avoid tight looping (eg, let the browser and the server breath a little). Not completely sure if it is necessary or not. I can't recall if there is a stack depth issue or not since localFetch results in onSuccess being called. I think that there is some async buried in there somewhere that prevents that though 😕

This should also sever the closure chain that would otherwise result... I think

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think there is a stack depth issue, setTimeout queues up the callback out of the main thread, so the enclosing onSuccess function finishes, and the localFetch callback gets executed outside of it.

}
}
localFetch(path)
}

export function isFunction(func) {
return func && {}.toString.call(func) === '[object Function]'
}
Expand Down
77 changes: 53 additions & 24 deletions src/js/views/Project/Components/ComponentList.jsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import React, { useContext, useEffect, useRef, useState } from 'react'
import React, { useContext, useEffect, useState } from 'react'
import PropTypes from 'prop-types'
import { Alert, Loading, ScoreBadge } from '../../../components'
import { NavigableTable } from '../../../components/Table'
import { useTranslation } from 'react-i18next'
import { Context } from '../../../state'
import { httpGet, parseLinkHeader } from '../../../utils'
import { fetchPages } from '../../../utils'
import { ViewComponent } from './ViewComponent'

function ComponentList({ project, urlPath }) {
Expand All @@ -26,29 +26,40 @@ function ComponentList({ project, urlPath }) {
}, [])

useEffect(() => {
let allData = []
let url = new URL(`/projects/${project.id}/components`, globalState.baseURL)
setFetching(true)
do {
const currUrl = url
url = null
httpGet(
globalState.fetch,
currUrl,
({ data, headers }) => {
const links = parseLinkHeader(headers.get('Link'))
url = Object.hasOwn(links, 'next') ? new URL(links.next[0]) : null
setComponents((prevState) => prevState.concat(data))
},
({ message }) => {
setErrorMessage(message)
fetchPages(
`/projects/${project.id}/components`,
globalState,
(data, isComplete) => {
if (isComplete) {
setFetching(false)
setComponents((prevState) =>
prevState
.concat(data)
.sort((a, b) => (a['name'] < b['name'] ? -1 : 1))
)
} else {
setComponents((prevState) => prevState.concat(data))
}
)
} while (url !== null)
setFetching(false)
},
(message) => {
setErrorMessage(message)
setFetching(false)
}
)
}, [])

function onSortChange(column, direction) {
setComponents(
[...components].sort((a, b) => {
if (a[column] == null || a[column] < b[column])
return direction === 'asc' ? -1 : 1
if (b[column] === null || b[column] < a[column])
return direction === 'asc' ? 1 : -1
})
)
}

if (fetching) return <Loading />
if (errorMessage) return <Alert level="error">{errorMessage}</Alert>

Expand All @@ -66,28 +77,44 @@ function ComponentList({ project, urlPath }) {
{
title: t('common.name'),
name: 'name',
type: 'text'
type: 'text',
tableOptions: {
headerClassName: 'w-3/12',
className: 'truncate'
}
},
{
title: t('terms.package'),
name: 'package_url',
type: 'text'
type: 'text',
tableOptions: {
headerClassName: 'w-4/12',
className: 'truncate'
}
},
{
title: t('terms.version'),
name: 'version',
type: 'text'
type: 'text',
tableOptions: {
headerClassName: 'w-1/12',
className: 'overflow-clip'
}
},
{
title: t('project.components.status'),
name: 'status',
type: 'text'
type: 'text',
tableOptions: {
headerClassName: 'w-1/12'
}
},
{
title: t('terms.healthScore'),
name: 'score',
type: 'text',
tableOptions: {
headerClassName: 'w-2/12',
lookupFunction: (value) => {
if (value !== null) {
return <ScoreBadge value={value} />
Expand All @@ -98,7 +125,9 @@ function ComponentList({ project, urlPath }) {
}
]}
data={components}
defaultSort="name"
extractSearchParam={(obj) => obj.name}
onSortChange={onSortChange}
title={t('project.components.singular')}
selectedIndex={selectedIndex}
setSelectedIndex={setSelectedIndex}
Expand Down
17 changes: 11 additions & 6 deletions src/js/views/Reports/ComponentUsage.jsx
Original file line number Diff line number Diff line change
@@ -1,19 +1,24 @@
import React from 'react'
import Report from '../../components/Report/Report'

function createColumn(name, type, headerClassName, className) {
const tableOptions = { headerClassName, className }
return { name, type, tableOptions }
}

function ComponentUsage() {
return (
<Report
endpoint="/reports/component-usage"
keyPrefix="reports.componentUsage"
pageIcon="fas cubes"
columns={[
{ name: 'name', type: 'text' },
{ name: 'package_url', type: 'text' },
{ name: 'status', type: 'text' },
{ name: 'active_version', type: 'text' },
{ name: 'version_count', type: 'number' },
{ name: 'project_count', type: 'number' }
createColumn('name', 'text', 'w-3/12', 'truncate'),
createColumn('package_url', 'text', 'w-4/12', 'truncate'),
createColumn('status', 'text', 'w-1/12', 'overflow-clip'),
createColumn('active_version', 'text', 'w-2/12', 'overflow-clip'),
createColumn('version_count', 'number', 'w-1/12'),
createColumn('project_count', 'number', 'w-1/12')
]}
/>
)
Expand Down
Loading