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

PRfD - List Pull Requests UI #8171

Merged
merged 5 commits into from
Sep 18, 2024
Merged
Show file tree
Hide file tree
Changes from 3 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
3 changes: 3 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,9 @@ $(UI_DIR)/node_modules:
gen-ui: $(UI_DIR)/node_modules ## Build UI web app
cd $(UI_DIR) && $(NPM) run build

gen-ui-watch: $(UI_DIR)/node_modules ## Build UI web app and watch for changes
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Helpful for UI dev.

Copy link
Member

Choose a reason for hiding this comment

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

We have npm run dev which achieves the same result

cd $(UI_DIR) && $(NPM) run build-watch

gen-proto: ## Build Protocol Buffers (proto) files using Buf CLI
go run github.com/bufbuild/buf/cmd/buf@$(BUF_CLI_VERSION) generate

Expand Down
15 changes: 15 additions & 0 deletions webui/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions webui/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
"scripts": {
"dev": "vite",
"build": "cross-env NODE_OPTIONS=--max-old-space-size=4096 vite build",
"build-watch": "cross-env NODE_OPTIONS=--max-old-space-size=4096 vite build --watch",
"serve": "vite preview",
"test": "vitest run",
"test-watch": "vitest",
Expand Down Expand Up @@ -41,6 +42,7 @@
"react-router-dom": "^6.20.1",
"react-simple-code-editor": "^0.13.1",
"react-syntax-highlighter": "^15.5.0",
"react-timeago": "^7.2.0",
"react-toggle-dark-mode": "^1.1.1",
"rehype-raw": "^7.0.0",
"rehype-react": "^8.0.0",
Expand Down
59 changes: 58 additions & 1 deletion webui/src/lib/api/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -567,6 +567,62 @@ class Tags {

}

class Pulls {
async list(repoId, state = "open", prefix = "", after = "", amount = DEFAULT_LISTING_AMOUNT) {
// const query = qs({prefix, after, amount});
// const response = await apiRequest(`/repositories/${encodeURIComponent(repoId)}/pulls?` + query);
// if (response.status !== 200) {
// throw new Error(`could not list pulls: ${await extractError(response)}`);
// }
// return response.json();

// TODO: this is for development purposes only
console.log("list pulls", {repoId, state, prefix, after, amount})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As written - this is temp, until the API will be ready.

let results = [
{
"id": "test-pull-1",
"title": "Test PR 1",
"status": "open",
"created_at": 1726575741,
"author": "test-user-1",
"description": "This is a test PR",
"source_branch": "feature-branch-1",
"destination_branch": "main"
},
{
"id": "test-pull-2",
"title": "Next Test PR 2",
"status": "closed",
"created_at": 1726402941,
"author": "test-user-2",
"description": "This is a another test PR",
"source_branch": "feature-branch-2",
"destination_branch": "main"
},
{
"id": "test-pull-3",
"title": "Another Test PR 3",
"status": "open",
"created_at": 1718454141,
"author": "test-user-1",
"description": "This is also a test PR",
"source_branch": "feature-branch-3",
"destination_branch": "feature-branch-1"
}
];
results = results.filter(pull => pull.status === state);
return {
"pagination": {
"has_more": false,
"max_per_page": 1000,
"next_offset": "",
"results": results.length
},
"results": results
}
}
}

// uploadWithProgress uses good ol' XMLHttpRequest because progress indication in fetch() is
// still not well supported across browsers (see https://stackoverflow.com/questions/35711724/upload-progress-indicators-for-fetch).
export const uploadWithProgress = (url, file, method = 'POST', onProgress = null, additionalHeaders = null) => {
Expand Down Expand Up @@ -652,7 +708,7 @@ class Objects {
if (response.status === 401) {
return false;
}

// This is not one of the expected responses
throw new Error(await extractError(response));
}
Expand Down Expand Up @@ -1112,6 +1168,7 @@ class Import {
export const repositories = new Repositories();
export const branches = new Branches();
export const tags = new Tags();
export const pulls = new Pulls();
export const objects = new Objects();
export const commits = new Commits();
export const refs = new Refs();
Expand Down
4 changes: 2 additions & 2 deletions webui/src/lib/components/controls.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -105,10 +105,10 @@ export const FormattedDate = ({ dateValue, format = "MM/DD/YYYY HH:mm:ss" }) =>
};


export const ActionGroup = ({ children, orientation = "left" }) => {
export const ActionGroup = ({ children, orientation = "left", className = "" }) => {
const side = (orientation === 'right') ? 'ms-auto' : '';
return (
<div role="toolbar" className={`${side} mb-2 btn-toolbar action-group-${orientation}`}>
<div role="toolbar" className={`${side} mb-2 btn-toolbar action-group-${orientation} ${className}`}>
{children}
</div>
);
Expand Down
2 changes: 1 addition & 1 deletion webui/src/lib/components/repository/changes.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ export const ChangesTreeContainer = ({results, delimiter, uriNavigator,
<div>{changesTreeMessage}</div>
<Card>
<Card.Header>
<span className="float-start">
<span className="float-start w-100">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason, width: 100% was added globally to float-start, which is wrong.
Removed it, and added w-100 to all the places using it (and not overriding).

Copy link
Member

Choose a reason for hiding this comment

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

Can you explain please what was wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

float-start is a utility class meant to, well, make elements float to the left.
By overriding it with other attributes, it makes it unusable when needed without the extras.

Basically, overriding bootstrap stuff should only be in case of something related to the theme of the app or something like this, things that are flavors of the app. Should almost never happen.

{(delimiter !== "") && uriNavigator}
</span>
</Card.Header>
Expand Down
8 changes: 7 additions & 1 deletion webui/src/lib/components/repository/tabs.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@ import {Link, NavItem} from "../nav";
import {useRouter} from "../../hooks/router";
import {RefTypeBranch} from "../../../constants";


// TODO: this is temp, until PRfD will be ready
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// TODO: this is temp, until PRfD will be ready
// TODO (gilo): this is temp, until PRfD will be ready

const showPulls = false;

export const RepositoryNavTabs = ({ active }) => {
const { reference } = useRefs();
Expand Down Expand Up @@ -59,6 +60,11 @@ export const RepositoryNavTabs = ({ active }) => {
<Link active={active === 'tags'} href={`/repositories/${repoId}/tags`} component={NavItem}>
<TagIcon/> Tags
</Link>
{showPulls &&
Copy link
Member

Choose a reason for hiding this comment

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

Please add the TODO here as well

<Link active={active === 'pulls'} href={`/repositories/${repoId}/pulls`} component={NavItem}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note - I've used pulls (and not pull-requests) pretty much everywhere.

<GitCompareIcon/> Pull Requests
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using the same icon as Compare. We'll need to change one of them.

Copy link
Member

Choose a reason for hiding this comment

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

Can we add a TODO here or open an issue

</Link>
}
<Link active={active === 'compare'} href={withRefAndCompareContext(`/repositories/${repoId}/compare`)} component={NavItem}>
<GitCompareIcon/> Compare
</Link>
Expand Down
2 changes: 2 additions & 0 deletions webui/src/pages/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import RepositoryCommitsPage from "./repositories/repository/commits";
import RepositoryCommitPage from "./repositories/repository/commits/commit";
import RepositoryBranchesPage from "./repositories/repository/branches";
import RepositoryTagsPage from "./repositories/repository/tags";
import RepositoryPullsPage from "./repositories/repository/pulls/pulls";
import RepositoryComparePage from "./repositories/repository/compare";
import RepositoryActionsPage from "./repositories/repository/actions";
import RepositoryGeneralSettingsPage from "./repositories/repository/settings/general";
Expand Down Expand Up @@ -62,6 +63,7 @@ export const IndexPage = () => {
</Route>
<Route path="branches" element={<RepositoryBranchesPage/>}/>
<Route path="tags" element={<RepositoryTagsPage/>}/>
<Route path="pulls" element={<RepositoryPullsPage/>}/>
<Route path="compare/*" element={<RepositoryComparePage/>}/>
<Route path="actions">
<Route index element={<RepositoryActionsPage/>}/>
Expand Down
2 changes: 1 addition & 1 deletion webui/src/pages/repositories/repository/commits/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ const CommitWidget = ({ repo, commit }) => {
return (
<ListGroup.Item>
<div className="clearfix">
<div className="float-start">
<div className="float-start w-100">
<h6>
<Link href={{
pathname: '/repositories/:repoId/commits/:commitId',
Expand Down
159 changes: 159 additions & 0 deletions webui/src/pages/repositories/repository/pulls/pulls.jsx
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is based on the Tags page -
I changed mainly what I needed, and left the rest very similar.

Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
import React, {useEffect, useState} from "react";
import {useOutletContext} from "react-router-dom";
import Card from "react-bootstrap/Card";
import ListGroup from "react-bootstrap/ListGroup";
import Alert from "react-bootstrap/Alert";
import {Tab, Tabs} from "react-bootstrap";
import Button from "react-bootstrap/Button";
import TimeAgo from "react-timeago";

import {ActionGroup, AlertError, Loading, PrefixSearchWidget, RefreshButton} from "../../../../lib/components/controls";
import {pulls} from "../../../../lib/api";
import {useRefs} from "../../../../lib/hooks/repo";
import {useAPIWithPagination} from "../../../../lib/hooks/api";
import {Paginator} from "../../../../lib/components/pagination";
import {useRouter} from "../../../../lib/hooks/router";
import {RepoError} from "../error";
import {Link} from "../../../../lib/components/nav";


const PullWidget = ({repo, pull}) => {
const authorLink =
<Link href={{
// TODO: to where this link should lead?
Copy link
Member

Choose a reason for hiding this comment

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

Should lead to the user page in lakeFS IMO

pathname: '/repositories/:repoId/user/:userId',
params: {repoId: repo.id, userId: pull.author}
}}>
{pull.author}
</Link>;
return (
<ListGroup.Item className="pull-row">
Copy link
Member

Choose a reason for hiding this comment

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

Look at the example of commits tab, it should look the same IMO.
Font size, alignment hyperlink marking and the visuals (images and colors) should have the same style.
This tab should look and feel like the other tabs

<div className="clearfix">
<div className="float-start pt-1 pb-2">
<Link className="pull-title fs-4"
href={{
pathname: '/repositories/:repoId/pulls/:pullId',
params: {repoId: repo.id, pullId: pull.id},
}}
>
{pull.title}
</Link>
<div className="pull-info mt-1">
Opened <TimeAgo date={new Date(pull.created_at * 1000)}/> ago by {authorLink}
</div>
</div>
<div className="pull-branches mt-3 float-end">
<Button variant="secondary" size="sm" disabled={true}>{pull.source_branch}</Button>
<span className="m-2">&#8680;</span>
<Button variant="secondary" size="sm" disabled={true}>{pull.destination_branch}</Button>
</div>
</div>
</ListGroup.Item>
);
};

// TODO: is there a nicer place for this?
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// TODO: is there a nicer place for this?
// TODO (gilo): is there a nicer place for this?

const PullStatus = {
open: "open",
closed: "closed",
merged: "merged",
}

const PullsList = ({repo, after, prefix, onPaginate}) => {
const router = useRouter()
const [refresh, setRefresh] = useState(true);
// TODO: pullState should be persistent in the url and saved as a url param?
Copy link
Member

Choose a reason for hiding this comment

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

state is a query param in GitHub

const [pullsState, setPullsState] = useState(PullStatus.open);
const {results, error, loading, nextPage} = useAPIWithPagination(async () => {
return pulls.list(repo.id, pullsState, prefix, after);
}, [repo.id, pullsState, prefix, refresh, after]);

const doRefresh = () => setRefresh(true);

let content;

if (loading) content = <Loading/>;
else if (error) content = <AlertError error={error}/>;
else content = (results && !!results.length ?
<>
<Card>
<ListGroup variant="flush">
{results.map(pull => (
<PullWidget key={pull.id} repo={repo} pull={pull}/>
))}
</ListGroup>
</Card>
<Paginator onPaginate={onPaginate} nextPage={nextPage} after={after}/>
</> : <Alert variant="info">There aren&apos;t any pull requests yet.</Alert>
)

return (
<div className="mb-5">
<div className="position-relative clearfix">
<div className="">
<Tabs
defaultActiveKey={pullsState}
id="pulls-tabs"
onSelect={key => setPullsState(key)}
className="mb-3 pt-2"
>
<Tab eventKey={PullStatus.open} title="Open"/>
<Tab eventKey={PullStatus.closed} title="Closed"/>
</Tabs>
</div>
<ActionGroup orientation="right" className="position-absolute top-0 end-0 pb-2">
<PrefixSearchWidget
defaultValue={router.query.prefix}
text="Find Pull Request"
onFilter={prefix => router.push({
pathname: '/repositories/:repoId/pulls',
params: {repoId: repo.id},
query: {prefix}
})}/>

<RefreshButton onClick={doRefresh}/>
<Button variant="success">Create Pull Request</Button>
</ActionGroup>
</div>
{content}
</div>
);
};


const PullsContainer = () => {
const router = useRouter()
const {repo, loading, error} = useRefs();
const {after} = router.query;
const routerPfx = router.query.prefix || "";

if (loading) return <Loading/>;
if (error) return <RepoError error={error}/>;

return (
<PullsList
repo={repo}
after={after || ""}
prefix={routerPfx}
onPaginate={after => {
const query = {after};
if (router.query.prefix) {
query.prefix = router.query.prefix;
}
router.push({
pathname: '/repositories/:repoId/pulls',
params: {repoId: repo.id},
query
});
}}/>
);
};


const RepositoryPullsPage = () => {
const [setActivePage] = useOutletContext();
useEffect(() => setActivePage("pulls"), [setActivePage]);
return <PullsContainer/>;
}

export default RepositoryPullsPage;
2 changes: 1 addition & 1 deletion webui/src/pages/repositories/repository/tags.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ const TagWidget = ({ repo, tag, onDelete }) => {
return (
<ListGroup.Item>
<div className="clearfix">
<div className="float-start">
<div className="float-start w-100">
<h6>
<Link href={{
pathname: '/repositories/:repoId/objects',
Expand Down
Loading
Loading