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

UI: change summary under prefix #2744

Merged
merged 9 commits into from
Nov 30, 2021
Merged
Show file tree
Hide file tree
Changes from 7 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
108 changes: 108 additions & 0 deletions webui/src/lib/components/repository/ChangeSummary.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
import React, {useEffect, useState} from "react";
import {
ClockIcon,
DiffAddedIcon,
DiffIgnoredIcon,
DiffModifiedIcon,
DiffRemovedIcon,
XCircleIcon
} from "@primer/octicons-react";
import {OverlayTrigger, Tooltip} from "react-bootstrap";
import {humanSize} from "./tree";

const MAX_NUM_OBJECTS = 10_000;
const PAGE_SIZE = 1_000;

class SummaryEntry {
constructor() {
this.count = 0
this.sizeBytes = 0
}
add(count, sizeBytes) {
this.count += count
Copy link
Contributor

Choose a reason for hiding this comment

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

guess it will be always 1

this.sizeBytes += sizeBytes
}
}

class SummaryData {
constructor() {
this.added = new SummaryEntry()
this.changed = new SummaryEntry()
this.removed = new SummaryEntry()
this.conflict = new SummaryEntry()
}
}

/**
* Widget to display a summary of a change: the number of added/changed/deleted/conflicting objects.
* Shows an error if the change is has more than {@link MAX_NUM_OBJECTS} entries.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Shows an error if the change is has more than {@link MAX_NUM_OBJECTS} entries.
* Shows an error if the change has more than {@link MAX_NUM_OBJECTS} entries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


* @param {string} prefix - prefix to display summary for.
* @param {(after : string, path : string, useDelimiter :? boolean, amount :? number) => Promise<any> } getMore - function to use to get the change entries.
*/
export default ({prefix, getMore}) => {
const [resultsState, setResultsState] = useState({results: [], pagination: {}, tooManyPages: false});
const [loading, setLoading] = useState(true);
useEffect(async () => {
// get pages until reaching the max change size
if (resultsState.results && resultsState.results.length >= MAX_NUM_OBJECTS) {
setResultsState({results: null, pagination: {}, tooManyPages: true})
setLoading(false)
return
}
if (!loading) {
return
}
const {results, pagination} = await getMore(resultsState.pagination.next_offset || "", prefix, false, PAGE_SIZE)
if (!pagination.has_more) {
setLoading(false)
}
setResultsState({results: resultsState.results.concat(results), pagination: pagination, tooManyPages: false})
}, [resultsState.results, loading])
Comment on lines +46 to +61
Copy link
Contributor

Choose a reason for hiding this comment

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

This is inconsistent with file viewer. You may show statistics for 3 files but present 2 in the ui.
This happens because:

  1. Listing happens separately.
  2. Files are cached and statistics are not.

Copy link
Contributor Author

@johnnyaug johnnyaug Nov 28, 2021

Choose a reason for hiding this comment

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

Solving this basically means I have to refresh all expanded paths in the tree, together with all the stats that are already shown (see explanation below).

I'm not sure it's worth the effort and the performance risk. Since it is clearly shown to the user that the summary is calculated, I think inconsistencies can be expected.

Explanation

Since summaries are calculated on demand - the only way for me to be consistent with the expanded tree is to refresh all expanded paths under the given path.

After doing so, shown summaries on upper levels should be updated to be consistent with their children.
In turn, all expanded paths under these summaries should be updated to be consistent with them.

This potentially requires updating the whole tree.

Copy link
Contributor

Choose a reason for hiding this comment

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

@johnnyaug to clarify - are you referring to a situation where you added let's say 100 objects to partition "a", and at a time 50 objects are loaded to the tree but the stats show that 100 objects were added?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is described here is a scenario where data is constantly written to the repo, and you are viewing the change at different points in time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to make sure I understand, this limitation is due to the way we built the tree structure. "Refreshing" means we need to go and fetch everything again, while the summary already did it. I don't want to block on it because I don't think it would be that common. Can we open an issue for consolidation of the summary and the tree fetching?


if (loading) return <ClockIcon/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why take this approach instead of showing results so far and keep update it forever..?

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 it's not a good experience to show an updating number of objects.

if (resultsState.tooManyPages) {
return (
<OverlayTrigger placement="bottom"
overlay={
<Tooltip>
<span className={"small font-weight-bold"}>
Can&apos;t show summary for a change with more than {MAX_NUM_OBJECTS} objects
</span>
</Tooltip>
}>
<span><XCircleIcon className="text-danger"/></span>
</OverlayTrigger>
)
}
const summaryData = resultsState.results.reduce((prev, current) => {
prev[current.type].add(1, current.size_bytes)
return prev
}, new SummaryData())
const detailsTooltip = <Tooltip>
<div className={"m-1 small text-left"}>
{summaryData.added.count > 0 &&
<><span className={"color-fg-added"}>{summaryData.added.count}</span> objects added (total {humanSize(summaryData.added.sizeBytes)})<br/></>}
{summaryData.removed.count > 0 &&
<><span className={"color-fg-removed"}>{summaryData.removed.count}</span> objects removed<br/></>}
{summaryData.changed.count > 0 &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please explain your choice not to show the size for the items removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

(I was under the false impression that the size wasn't returned for "deleted" type diffs)

<><span className={"color-fg-changed"}>{summaryData.changed.count}</span> objects changed<br/></>}
{summaryData.conflict.count > 0 &&
<><span className={"color-fg-conflict"}>{summaryData.conflict.count}</span> conflicts<br/></>}
</div>
</Tooltip>
return (
<OverlayTrigger placement="right-end" overlay={detailsTooltip}>
<div className={"m-1 small text-right"}>
{summaryData.added.count > 0 &&
<span className={"color-fg-added"}><DiffAddedIcon className={"change-summary-icon"}/>{summaryData.added.count}</span>}
{summaryData.removed.count > 0 &&
<span className={"color-fg-removed"}><DiffRemovedIcon className={"change-summary-icon"}/>{summaryData.removed.count}</span>}
{summaryData.changed.count > 0 &&
<span className={"font-weight-bold"}><DiffModifiedIcon className={"change-summary-icon"}/>{summaryData.changed.count}</span>}
{summaryData.conflict.count > 0 &&
<span className={"color-fg-conflict"}><DiffIgnoredIcon className={"change-summary-icon"}/>{summaryData.conflict.count}</span>}
</div>
</OverlayTrigger>
)
}
218 changes: 104 additions & 114 deletions webui/src/lib/components/repository/changes.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,67 +2,74 @@ import React, {useState} from "react";

import {OverlayTrigger} from "react-bootstrap";
import Tooltip from "react-bootstrap/Tooltip";
import Button from "react-bootstrap/Button";
import {
ChevronDownIcon,
ChevronRightIcon,
CircleSlashIcon,
ClockIcon,
FileDiffIcon,
FileDirectoryIcon,
GraphIcon,
HistoryIcon,
PencilIcon,
PlusIcon,
TrashIcon,
FileDiffIcon
TrashIcon
} from "@primer/octicons-react";

import {ConfirmationModal} from "../modals";
import {Link} from "../nav";
import {useAPIWithPagination} from "../../hooks/api";
import {Error} from "../controls";
import {ObjectsDiff} from "./ObjectsDiff";
import {ConfirmationModal} from "../modals";
import ChangeSummary from "./ChangeSummary";

class RowAction {
/**
* @param {JSX.Element} icon
* @param {string} tooltip
* @param {boolean} visible
* @param {()=>void} onClick
*/
constructor(icon, tooltip, visible, onClick) {
this.tooltip = tooltip
this.visible = visible
this.onClick = onClick
this.icon = icon
}
}

const ChangeRowActions = ({ entry, onRevert }) => {
const [show, setShow] = useState(false);
const revertConfirmMsg = `Are you sure you wish to revert "${entry.path}" (${entry.type})?`;
const onSubmit = () => {
onRevert(entry)
setShow(false)
};

return (
<>
<OverlayTrigger key={"bottom"} overlay={(<Tooltip id={"revert-entry"}>Revert change</Tooltip>)}>
<Button variant="link" disabled={false} onClick={(e) => {
e.preventDefault();
setShow(true)
}} >
<HistoryIcon/>
</Button>
</OverlayTrigger>

<ConfirmationModal show={show} onHide={() => setShow(false)} msg={revertConfirmMsg} onConfirm={onSubmit}/>
</>
);
};
/**
* @param {[RowAction]} actions
*/
const ChangeRowActions = ({actions}) => <>
{
actions.map(action => (
<><OverlayTrigger placement="bottom" overlay={<Tooltip>{action.tooltip}</Tooltip>}>
<Link className={"btn-link"} disabled={false} style={{visibility: action.visible ? "visible" : ""}}
onClick={(e) => {
e.preventDefault();
action.onClick()
}}>
{action.icon}
</Link>
</OverlayTrigger>&#160;&#160;</>
))}
</>;

/*
Tree item is a node in the tree view. It can be expanded to multiple TreeEntryRow:
1. A single TreeEntryRow for the current prefix (or entry for leaves).
2. Multiple TreeItem as children, each representing another tree node.
entry: The entry the TreeItem is representing, could be either an object or a prefix.
repo: Repository
reference: commitID / branch
leftDiffRefID: commitID / branch
rightDiffRefID: commitID / branch
internalRefresh: to be called when the page refreshes manually
onRevert: to be called when an object/prefix is requested to be reverted
delimiter: objects delimiter ('' or '/')
after: all entries must be greater than after
relativeTo: prefix of the parent item ('' for root elements)
getMore: callback to be called when more items need to be rendered
depth: the item's depth withing the tree
/**
* Tree item is a node in the tree view. It can be expanded to multiple TreeEntryRow:
* 1. A single TreeEntryRow for the current prefix (or entry for leaves).
* 2. Multiple TreeItem as children, each representing another tree node.
* @param entry The entry the TreeItem is representing, could be either an object or a prefix.
* @param repo Repository
* @param reference commitID / branch
* @param leftDiffRefID commitID / branch
* @param rightDiffRefID commitID / branch
* @param internalRefresh to be called when the page refreshes manually
* @param onRevert to be called when an object/prefix is requested to be reverted
* @param delimiter objects delimiter ('' or '/')
* @param after all entries must be greater than after
* @param relativeTo prefix of the parent item ('' for root elements)
* @param {(after : string, path : string, useDelimiter :? boolean, amount :? number) => Promise<any> } getMore callback to be called when more items need to be rendered
*/
export const TreeItem = ({ entry, repo, reference, leftDiffRefID, rightDiffRefID, internalRefresh, onRevert, onNavigate, delimiter, relativeTo, getMore, depth=0 }) => {
const [dirExpanded, setDirExpanded] = useState(false); // state of a non-leaf item expansion
Expand All @@ -89,65 +96,79 @@ export const TreeItem = ({ entry, repo, reference, leftDiffRefID, rightDiffRefID
return <Error error={error}/>

if (loading && results.length === 0)
return <TreeEntryRow key={entry.path+"entry-row"} entry={entry} showActions={true} loading={true} relativeTo={relativeTo} depth={depth} onRevert={onRevert} onNavigate={onNavigate} repo={repo} reference={reference}/>
return <TreeEntryRow key={entry.path+"entry-row"} entry={entry} loading={true} relativeTo={relativeTo} depth={depth} onRevert={onRevert} onNavigate={onNavigate}repo={repo} reference={reference}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return <TreeEntryRow key={entry.path+"entry-row"} entry={entry} loading={true} relativeTo={relativeTo} depth={depth} onRevert={onRevert} onNavigate={onNavigate}repo={repo} reference={reference}
return <TreeEntryRow key={entry.path+"entry-row"} entry={entry} loading={true} relativeTo={relativeTo} depth={depth} onRevert={onRevert} onNavigate={onNavigate} repo={repo} reference={reference}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

getMore={getMore}/>

// When the entry represents a tree leaf
if (!entry.path.endsWith(delimiter))
return <>
<TreeEntryRow key={entry.path+"entry-row"} entry={entry} showActions={true} leaf={true} relativeTo={relativeTo} depth={depth === 0 ? 0 : depth + 1} onRevert={onRevert} onNavigate={onNavigate} repo={repo}
reference={reference} diffExpanded={diffExpanded} onClick={() => setDiffExpanded(!diffExpanded)}/>
{diffExpanded && <ExpandedLeafRow entry={entry} repoId={repo.id} leftDiffRef={leftDiffRefID} rightDiffRef={rightDiffRefID} depth={depth} loading={loading}/>
}
</>
<TreeEntryRow key={entry.path + "entry-row"} entry={entry} leaf={true} relativeTo={relativeTo} depth={depth === 0 ? 0 : depth + 1} onRevert={onRevert} onNavigate={onNavigate} repo={repo}
reference={reference} diffExpanded={diffExpanded} onClickExpandDiff={() => setDiffExpanded(!diffExpanded)} getMore={getMore}/>
{diffExpanded && <tr key={"row-" + entry.path} className={"leaf-entry-row"}>
<td className="objects-diff" colSpan={4}>
<ObjectsDiff
diffType={entry.type}
repoId={repo.id}
leftRef={leftDiffRefID}
rightRef={rightDiffRefID}
path={entry.path}
/>
{loading && <ClockIcon/>}
</td>
</tr>
}
</>

return <>
<TreeEntryRow key={entry.path+"entry-row"} entry={entry} showActions={true} dirExpanded={dirExpanded} relativeTo={relativeTo} depth={depth} onClick={() => setDirExpanded(!dirExpanded)} onRevert={onRevert} onNavigate={onNavigate} repo={repo} reference={reference}/>
{dirExpanded && results &&
results.map(child =>
( <TreeItem key={child.path+"-item"} entry={child} repo={repo} reference={reference} leftDiffRefID={leftDiffRefID} rightDiffRefID={rightDiffRefID} onRevert={onRevert} onNavigate={onNavigate}
internalReferesh={internalRefresh} delimiter={delimiter} depth={depth+1} relativeTo={entry.path} getMore={getMore}/>))}
{(!!nextPage || loading) &&
<TreeEntryPaginator path={entry.path} depth={depth} loading={loading} nextPage={nextPage} setAfterUpdated={setAfterUpdated}/>
}
</>
<TreeEntryRow key={entry.path + "entry-row"} entry={entry} dirExpanded={dirExpanded} relativeTo={relativeTo} depth={depth} onClick={() => setDirExpanded(!dirExpanded)} onRevert={onRevert} onNavigate={onNavigate} repo={repo} reference={reference} getMore={getMore}/>
{dirExpanded && results &&
results.map(child =>
(<TreeItem key={child.path + "-item"} entry={child} repo={repo} reference={reference} leftDiffRefID={leftDiffRefID} rightDiffRefID={rightDiffRefID} onRevert={onRevert} onNavigate={onNavigate}
internalReferesh={internalRefresh} delimiter={delimiter} depth={depth + 1}
relativeTo={entry.path} getMore={getMore}/>))}
{(!!nextPage || loading) &&
<TreeEntryPaginator path={entry.path} depth={depth} loading={loading} nextPage={nextPage}
setAfterUpdated={setAfterUpdated}/>
}
</>
}

export const TreeEntryRow = ({ entry, showActions, relativeTo="", leaf=false, dirExpanded, depth=0, onClick, loading=false, onRevert, onNavigate}) => {
export const TreeEntryRow = ({entry, relativeTo = "", leaf = false, dirExpanded, diffExpanded, depth = 0, onClick, loading = false, onRevert, onNavigate, onClickExpandDiff = null, getMore}) => {
const [showRevertConfirm, setShowRevertConfirm] = useState(false)
let rowClass = 'tree-entry-row ' + diffType(entry);
let pathSection = extractPathText(entry, relativeTo);
let diffIndicator = diffIndicatorIcon(entry);
let actions = entryActions(showActions, entry, onRevert);

if (entry.path_type === "common_prefix"){
const [showSummary, setShowSummary] = useState(false);
if (entry.path_type === "common_prefix") {
pathSection = <Link href={onNavigate(entry)}>{pathSection}</Link>
}
const rowActions = []
if (onClickExpandDiff && entry.type !== 'conflict') {
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't have to but consider exporting this and other diff types to the new constants.jsx file

rowActions.push(new RowAction(<FileDiffIcon/>, diffExpanded ? "Hide changes" : "Show changes", diffExpanded, onClickExpandDiff))
Copy link
Contributor

Choose a reason for hiding this comment

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

The FileDiffIcon should be visible by default. I think that you have a bug here because the icon becomes visible only while hovering a leaf row.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Couldn't find a reasonable way to do it, I don't want all the side-actions to be visible by default.

}
if (!leaf) {
rowActions.push(new RowAction(<GraphIcon/>, showSummary ? "Hide summary" : "Calculate change summary", showSummary, () => setShowSummary(!showSummary)))
}
rowActions.push(new RowAction(<HistoryIcon/>, "Revert changes", false, () => {setShowRevertConfirm(true)}))
return (
<tr className={rowClass} >
<td className="diff-indicator">{diffIndicator}</td>
<td className="tree-path">
<span style={{marginLeft: depth * 20 + "px"}}>
<tr className={rowClass}>
<td className="pl-4 col-auto p-2">{diffIndicator}</td>
<td className="col-9 tree-path">
<span style={{marginLeft: (depth * 20) + "px"}}>
<span onClick={onClick}>
{leaf
? entry.type === 'conflict'
johnnyaug marked this conversation as resolved.
Show resolved Hide resolved
? ""
: <OverlayTrigger placement="bottom" overlay={<Tooltip>Show changes</Tooltip>}>
<Link style={{verticalAlign: "revert"}} disabled={false} onClick={(e) => {
e.preventDefault();
setShow(true)
}} >
<FileDiffIcon/>
</Link>
</OverlayTrigger>
: dirExpanded ? <ChevronDownIcon/> : <ChevronRightIcon/>}
{!leaf && (dirExpanded ? <ChevronDownIcon/> : <ChevronRightIcon/>)}
</span>
{loading ? <ClockIcon/> : ""}
{pathSection}
</span>
</td>
{ actions === null ?
<td/> :
<td className={"change-entry-row-actions"}>{actions}</td>
}
<td className={"col-2 p-0 text-right"}>{showSummary && <ChangeSummary prefix={entry.path} getMore={getMore}/>}</td>
<td className={"col-1 change-entry-row-actions"}>
<ChangeRowActions actions={rowActions} />
<ConfirmationModal show={showRevertConfirm} onHide={() => setShowRevertConfirm(false)}
msg={`Are you sure you wish to revert "${entry.path}" (${entry.type})?`}
onConfirm={() => onRevert(entry)}/>
</td>
</tr>
);
};
Expand Down Expand Up @@ -176,23 +197,6 @@ export const TreeEntryPaginator = ({ path, setAfterUpdated, nextPage, depth=0, l
);
};

const ExpandedLeafRow = ({entry, repoId, leftDiffRef, rightDiffRef, loading}) => {
return (
<tr key={"row-" + entry.path} className={"leaf-entry-row"}>
<td className="objects-diff" colSpan={3}>
<ObjectsDiff
diffType={entry.type}
repoId={repoId}
leftRef={leftDiffRef}
rightRef={rightDiffRef}
path={entry.path}
/>
{loading && <ClockIcon/>}
</td>
</tr>
);
}

function extractPathText(entry, relativeTo) {
let pathText = entry.path;
if (pathText.startsWith(relativeTo)) {
Expand Down Expand Up @@ -254,17 +258,3 @@ function diffIndicatorIcon(entry) {
return '';
}
}

function entryActions(showActions, entry, onRevert) {
if (!!!onRevert){
return null
}
let actions;
if (showActions) {
actions = <ChangeRowActions
entry={entry}
onRevert={onRevert}
/>;
}
return actions;
}
4 changes: 2 additions & 2 deletions webui/src/pages/repositories/repository/changes.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -281,8 +281,8 @@ const ChangesBrowser = ({repo, reference, prefix, onSelectRef, }) => {
internalReferesh={internalRefresh}
onNavigate={onNavigate} onRevert={onRevert} delimiter={delimiter}
relativeTo={prefix}
getMore={(afterUpdated, path) => {
return refs.changes(repo.id, reference.id, afterUpdated, path, delimiter)
getMore={(afterUpdated, path, useDelimiter= true, amount = -1) => {
return refs.changes(repo.id, reference.id, afterUpdated, path, useDelimiter ? delimiter : "", amount > 0 ? amount : undefined)
}}/>
)
})}
Expand Down
Loading