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

Fix copy link to logs functionality #15368

Merged
merged 9 commits into from
Aug 9, 2022
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
46 changes: 20 additions & 26 deletions airbyte-webapp/src/components/JobItem/JobItem.tsx
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
import React, { Suspense, useRef, useState } from "react";
import { useEffectOnce } from "react-use";
import React, { Suspense, useCallback, useRef, useState } from "react";
import styled from "styled-components";

import { Spinner } from "components";

import { SynchronousJobReadWithStatus } from "core/request/LogsRequestError";
import { JobsWithJobs } from "pages/ConnectionPage/pages/ConnectionItemPage/components/JobsList";

import { AttemptRead, JobStatus } from "../../core/request/AirbyteClient";
import { AttemptRead, CheckConnectionReadStatus, JobStatus } from "../../core/request/AirbyteClient";
import { useAttemptLink } from "./attemptLinkUtils";
import ContentWrapper from "./components/ContentWrapper";
import ErrorDetails from "./components/ErrorDetails";
Expand All @@ -32,28 +31,29 @@ const LoadLogs = styled.div`
`;

interface JobItemProps {
shortInfo?: boolean;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ℹ️ This was never passed into this component, so removed it.

job: SynchronousJobReadWithStatus | JobsWithJobs;
}

const didJobSucceed = (job: SynchronousJobReadWithStatus | JobsWithJobs) => {
return getJobStatus(job) !== "failed";
};

export const getJobStatus: (job: SynchronousJobReadWithStatus | JobsWithJobs) => JobStatus = (job) => {
return (job as JobsWithJobs).job?.status ?? (job as SynchronousJobReadWithStatus).status;
export const getJobStatus: (
job: SynchronousJobReadWithStatus | JobsWithJobs
) => JobStatus | CheckConnectionReadStatus = (job) => {
return "status" in job ? job.status : job.job.status;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ℹ️ The casting also caused TypeScript to lose the real return type here, so changed this as well.

};

export const getJobAttemps: (job: SynchronousJobReadWithStatus | JobsWithJobs) => AttemptRead[] | undefined = (job) => {
return "attempts" in job ? job.attempts : undefined;
};

export const getJobId = (job: SynchronousJobReadWithStatus | JobsWithJobs) =>
(job as SynchronousJobReadWithStatus).id ?? (job as JobsWithJobs).job.id;
export const getJobId = (job: SynchronousJobReadWithStatus | JobsWithJobs) => ("id" in job ? job.id : job.job.id);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ℹ️ Doing this without casting now causes this method to understand that its return type is number | string.

Copy link
Contributor

Choose a reason for hiding this comment

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

When I was initially going through here I thought there was a bug. But no, we unfortunately get back both from the backend :/


export const JobItem: React.FC<JobItemProps> = ({ shortInfo, job }) => {
export const JobItem: React.FC<JobItemProps> = ({ job }) => {
const { jobId: linkedJobId } = useAttemptLink();
const [isOpen, setIsOpen] = useState(linkedJobId === getJobId(job));
const alreadyScrolled = useRef(false);
const [isOpen, setIsOpen] = useState(() => linkedJobId === String(getJobId(job)));
const scrollAnchor = useRef<HTMLDivElement>(null);

const didSucceed = didJobSucceed(job);
Expand All @@ -62,26 +62,20 @@ export const JobItem: React.FC<JobItemProps> = ({ shortInfo, job }) => {
setIsOpen(!isOpen);
};

useEffectOnce(() => {
if (linkedJobId) {
scrollAnchor.current?.scrollIntoView({
behavior: "smooth",
block: "start",
});
const onDetailsToggled = useCallback(() => {
if (alreadyScrolled.current || linkedJobId !== String(getJobId(job))) {
return;
}
});
scrollAnchor.current?.scrollIntoView({
block: "start",
});
alreadyScrolled.current = true;
}, [job, linkedJobId]);

return (
<Item isFailed={!didSucceed} ref={scrollAnchor}>
<MainInfo
shortInfo={shortInfo}
isOpen={isOpen}
isFailed={!didSucceed}
onExpand={onExpand}
job={job}
attempts={getJobAttemps(job)}
/>
<ContentWrapper isOpen={isOpen}>
<MainInfo isOpen={isOpen} isFailed={!didSucceed} onExpand={onExpand} job={job} attempts={getJobAttemps(job)} />
<ContentWrapper isOpen={isOpen} onToggled={onDetailsToggled}>
<div>
<Suspense
fallback={
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,14 @@ import React from "react";
interface IProps {
children?: React.ReactNode;
isOpen?: boolean;
onToggled?: () => void;
}

const ContentWrapper: React.FC<IProps> = ({ children, isOpen }) => {
const ContentWrapper: React.FC<IProps> = ({ children, isOpen, onToggled }) => {
return (
<motion.div
animate={!isOpen ? "closed" : "open"}
onAnimationComplete={onToggled}
variants={{
open: {
height: "auto",
Expand Down
12 changes: 5 additions & 7 deletions airbyte-webapp/src/components/JobItem/components/MainInfo.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -34,25 +34,24 @@ interface MainInfoProps {
isOpen?: boolean;
onExpand: () => void;
isFailed?: boolean;
shortInfo?: boolean;
}

const MainInfo: React.FC<MainInfoProps> = ({ job, attempts = [], isOpen, onExpand, isFailed, shortInfo }) => {
const MainInfo: React.FC<MainInfoProps> = ({ job, attempts = [], isOpen, onExpand, isFailed }) => {
const jobStatus = getJobStatus(job);
const isPartialSuccess = partialSuccessCheck(attempts);

const statusIcon = () => {
switch (true) {
case jobStatus === JobStatus.cancelled:
return <StatusIcon />;
return <StatusIcon status="error" />;
case jobStatus === JobStatus.running:
return <StatusIcon status="loading" />;
case jobStatus === JobStatus.succeeded:
return <StatusIcon status="success" />;
case isPartialSuccess:
return <StatusIcon status="warning" />;
case !isPartialSuccess && isFailed && !shortInfo:
return <StatusIcon />;
case !isPartialSuccess && isFailed:
return <StatusIcon status="error" />;
default:
return null;
}
Expand All @@ -71,8 +70,7 @@ const MainInfo: React.FC<MainInfoProps> = ({ job, attempts = [], isOpen, onExpan
) : (
<FormattedMessage id={`sources.${getJobStatus(job)}`} />
)}
{shortInfo && <FormattedMessage id="sources.additionLogs" />}
{attempts.length && !shortInfo && (
{attempts.length && (
<>
{attempts.length > 1 && (
<div className={styles.lastAttempt}>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ describe("<StatusIcon />", () => {
{ status: "sleep", icon: "moon" },
{ status: "warning", icon: "triangle-exclamation" },
{ status: "loading", icon: "circle-loader" },
{ status: "error", icon: "xmark" },
];

test.each(statusCases)("renders $status status", ({ status, icon }) => {
Expand Down
24 changes: 13 additions & 11 deletions airbyte-webapp/src/components/StatusIcon/StatusIcon.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { faBan, faCheck, faExclamationTriangle, faTimes, IconDefinition } from "@fortawesome/free-solid-svg-icons";
import { faBan, faCheck, faExclamationTriangle, faTimes } from "@fortawesome/free-solid-svg-icons";
import { FontAwesomeIcon } from "@fortawesome/react-fontawesome";
import React from "react";
import styled from "styled-components";
Expand All @@ -8,7 +8,7 @@ import { MoonIcon } from "components/icons/MoonIcon";
import PauseIcon from "../icons/PauseIcon";
import CircleLoader from "./CircleLoader";

export type StatusIconStatus = "sleep" | "inactive" | "success" | "warning" | "loading";
export type StatusIconStatus = "sleep" | "inactive" | "success" | "warning" | "loading" | "error";

interface StatusIconProps {
className?: string;
Expand All @@ -20,20 +20,22 @@ interface StatusIconProps {

const getBadgeWidth = (props: StatusIconProps) => (props.big ? (props.value ? 57 : 40) : props.value ? 37 : 20);

const _iconByStatus: Partial<Record<StatusIconStatus, IconDefinition | undefined>> = {
const _iconByStatus = {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ℹ️ By changing this just to a as const, TypeScript understands which keys exactly exist in this, unless beforehand where it always suspected accessing something in here could also be undefined.

sleep: faBan,
success: faCheck,
warning: faExclamationTriangle,
};
error: faTimes,
} as const;

const _themeByStatus: Partial<Record<StatusIconStatus, string>> = {
const _themeByStatus = {
sleep: "lightTextColor",
inactive: "lightTextColor",
success: "successColor",
warning: "warningColor",
};
error: "dangerColor",
} as const;

const Container = styled.div<StatusIconProps>`
const Container = styled.div<Pick<StatusIconProps, "big" | "value">>`
width: ${(props) => getBadgeWidth(props)}px;
height: ${({ big }) => (big ? 40 : 20)}px;
margin-right: 10px;
Expand All @@ -44,8 +46,8 @@ const Container = styled.div<StatusIconProps>`
vertical-align: middle;
`;

const Badge = styled(Container)<StatusIconProps>`
background: ${(props) => props.theme[(props.status && _themeByStatus[props.status]) || "dangerColor"]};
const Badge = styled(Container)<{ status: Exclude<StatusIconStatus, "loading"> }>`
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ℹ️ Just require the properties that are actually used here (and in the Pick above), can simplify that code, since it understands that theme[...] cannot be undefined anymore.

background: ${({ theme, status }) => theme[_themeByStatus[status]]};
border-radius: ${({ value }) => (value ? "15px" : "50%")};
color: ${({ theme }) => theme.whiteColor};
padding-top: ${({ status }) => (status === "warning" || status === "inactive" ? 3 : 4)}px;
Expand All @@ -66,7 +68,7 @@ const Value = styled.span`
padding-left: 3px;
`;

const StatusIcon: React.FC<StatusIconProps> = ({ title, status, ...props }) => {
const StatusIcon: React.FC<StatusIconProps> = ({ title, status = "error", ...props }) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ℹ️ Make error the default status, to not need to adjust all places this was used without a status.

const valueElement = props.value ? <Value>{props.value}</Value> : null;

if (status === "loading") {
Expand All @@ -85,7 +87,7 @@ const StatusIcon: React.FC<StatusIconProps> = ({ title, status, ...props }) => {
) : status === "sleep" ? (
<MoonIcon title={title} />
) : (
<FontAwesomeIcon icon={(status && _iconByStatus[status]) || faTimes} title={title} />
<FontAwesomeIcon icon={_iconByStatus[status]} title={title} />
)}
{valueElement}
</Badge>
Expand Down