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.

Sign up for GitHub

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

Fix copy link to logs functionality #15368

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4d84bdb
Fix copy link to logs functionality
Aug 5, 2022
7aefd6f
Update airbyte-webapp/src/components/JobItem/JobItem.tsx
Aug 5, 2022
4d393d2
Fix scrolling
Aug 8, 2022
71e82a3
Merge branch 'master' into tim/fix-link-to-logs
Aug 8, 2022
1527d19
Remove smooth scrolling
Aug 8, 2022
00f717a
Improve effect for better return statements
Aug 8, 2022
f57a686
Merge branch 'master' into tim/fix-link-to-logs
Aug 8, 2022
1a18a30
Merge branch 'master' into tim/fix-link-to-logs
Aug 9, 2022
60b0082
Better scroll
Aug 9, 2022
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
25 changes: 9 additions & 16 deletions airbyte-webapp/src/components/JobItem/JobItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ 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 +32,28 @@ const LoadLogs = styled.div`
`;

interface JobItemProps {
shortInfo?: boolean;
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 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
Contributor 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
Contributor 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 [isOpen, setIsOpen] = useState(linkedJobId === String(getJobId(job)));
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 the actual fix, that restores that behavior.

const scrollAnchor = useRef<HTMLDivElement>(null);

const didSucceed = didJobSucceed(job);
Expand All @@ -73,14 +73,7 @@ export const JobItem: React.FC<JobItemProps> = ({ shortInfo, job }) => {

return (
<Item isFailed={!didSucceed} ref={scrollAnchor}>
<MainInfo
shortInfo={shortInfo}
isOpen={isOpen}
isFailed={!didSucceed}
onExpand={onExpand}
job={job}
attempts={getJobAttemps(job)}
/>
<MainInfo isOpen={isOpen} isFailed={!didSucceed} onExpand={onExpand} job={job} attempts={getJobAttemps(job)} />
<ContentWrapper isOpen={isOpen}>
<div>
<Suspense
Expand Down
12 changes: 5 additions & 7 deletions airbyte-webapp/src/components/JobItem/components/MainInfo.tsx
View file
Open in desktop
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
1 change: 1 addition & 0 deletions airbyte-webapp/src/components/StatusIcon/StatusIcon.test.tsx
View file
Open in desktop
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
View file
Open in desktop
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
Contributor 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
Contributor 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
Contributor 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