-
Notifications
You must be signed in to change notification settings - Fork 71
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
Add support for viewing search results in context for text logs (clp-text). #489
Changes from 9 commits
0007fb4
c6d89d9
44c15eb
17bcd11
420126a
a6bf9d2
c1f6e19
18b9411
f684c09
2b6d83a
ba61aa6
302ac3a
7eed866
d8c6b42
8c860e5
c81aef8
d6a5e9b
fb6c7b6
ae526eb
6568b5a
3f6d40f
50b9f70
46d8a7a
db770f7
ec84389
af59797
8bb0e84
2b52fd5
a48588f
4cf54ad
a270ff4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,18 @@ | ||
import {CssVarsProvider} from "@mui/joy/styles/CssVarsProvider"; | ||
|
||
import Query from "./ui/QueryState.jsx"; | ||
|
||
|
||
/** | ||
* Renders the main application. | ||
* | ||
* @return {JSX.Element} | ||
*/ | ||
const App = () => { | ||
return ( | ||
<h1>Hello world!</h1> | ||
<CssVarsProvider modeStorageKey={"uiTheme"}> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. create a const for string "uiTheme" |
||
<Query/> | ||
</CssVarsProvider> | ||
); | ||
}; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
import axios, {AxiosError} from "axios"; | ||
|
||
|
||
/** | ||
* @typedef {number} QueryLoadState | ||
*/ | ||
let enumQueryLoadState; | ||
/** | ||
* Enum of query loading state. | ||
* | ||
* @enum {QueryLoadState} | ||
*/ | ||
const QUERY_LOAD_STATE = Object.freeze({ | ||
SUBMITTING: (enumQueryLoadState = 0), | ||
WAITING: ++enumQueryLoadState, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unused? |
||
LOADING: ++enumQueryLoadState, | ||
}); | ||
|
||
/** | ||
* Submits a job to extract a segment of the original file, which contains a given log event index, | ||
* into CLP's IR format for viewing in the Log Viewer. | ||
junhaoliao marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* | ||
* @param {number|string} origFileId The ID of the original file to extract IR from | ||
junhaoliao marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* @param {number} logEventIx The index of the log event | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's use |
||
* @param {Function} onQueryStateChange Callback to set query state. | ||
* @param {Function} onErrorMsg Callback to set error message. | ||
*/ | ||
const submitExtractIrJob = async (origFileId, logEventIx, onQueryStateChange, onErrorMsg) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method has side-effects (setting query state, changing the URL) that (1) aren't obvious from its name and (2) don't really seem specific to the api "module". Perhaps its better to simplify this method to just submitting the POST request and do the rest in the |
||
try { | ||
const {data} = await axios.post("/query/extract-ir", { | ||
msg_ix: logEventIx, | ||
orig_file_id: origFileId, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I missed this in the previous PR, but why are we using snake_case for these? I think the only place we actually need to use snake_case is when inserting the row into database, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In a similar vein, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Right, I was using the webui code as an example but forgot the search query args are directly insert into the DB. Will update accordingly. |
||
}); | ||
|
||
onQueryStateChange(QUERY_LOAD_STATE.LOADING); | ||
|
||
const innerLogEventIx = logEventIx - data.begin_msg_ix + 1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be |
||
window.location = `/log-viewer/index.html?filePath=/ir/${data.path}` + | ||
`#logEventIdx=${innerLogEventIx}`; | ||
} catch (e) { | ||
let errorMsg = "Unknown error."; | ||
if (e instanceof AxiosError) { | ||
errorMsg = e.message; | ||
if ("undefined" !== typeof e.response) { | ||
errorMsg = e.response.data.message ?? e.response.statusText; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This reads as if we're going to append to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Right, depending on whether the request was sent successfully and whether the server returns a valid response, we pick the error message (from one of the 3 places) that best represents the error. Let's continue the discussion with these as references:
Sure There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we're obsessed enough with syntactic sugar, we can write it as
(only if it doesn't give us more "?" when reading the code; pun intended lol There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 😛 I think in this case, fewer "?" is better, lol. |
||
} | ||
onErrorMsg(errorMsg); | ||
junhaoliao marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
}; | ||
|
||
export { | ||
QUERY_LOAD_STATE, | ||
submitExtractIrJob, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
.loading-sheet { | ||
height: 100%; | ||
|
||
display: flex; | ||
flex-direction: column; | ||
|
||
align-items: center; | ||
justify-content: center; | ||
} | ||
|
||
.loading-progress-container { | ||
width: 100%; | ||
} | ||
|
||
.loading-stepper-container { | ||
display: flex; | ||
flex-grow: 1; | ||
|
||
align-items: center; | ||
} | ||
|
||
.loading-stepper { | ||
--Stepper-verticalGap: 2rem!important; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,142 @@ | ||
import { | ||
Box, | ||
LinearProgress, | ||
Sheet, | ||
Step, | ||
StepIndicator, | ||
Stepper, | ||
Typography, | ||
} from "@mui/joy"; | ||
|
||
import "./Loading.css"; | ||
|
||
|
||
/** | ||
* Descriptions for query states. | ||
*/ | ||
const QUERY_STATE_DESCRIPTIONS = Object.freeze([ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we move this into the query.js and use the enums as indices (so that they don't go out of sync)? |
||
{ | ||
label: "Submitting query Job", | ||
description: "Parsing arguments and submitting job to the server.", | ||
}, | ||
{ | ||
label: "Waiting for job To finish", | ||
junhaoliao marked this conversation as resolved.
Show resolved
Hide resolved
|
||
description: "The job is running. Waiting for the job to finish.", | ||
}, | ||
{ | ||
label: "Loading up Log Viewer", | ||
junhaoliao marked this conversation as resolved.
Show resolved
Hide resolved
|
||
description: "The query has been completed and the results are being loaded.", | ||
}, | ||
]); | ||
|
||
|
||
/** | ||
* Renders a step with a label and a description text. | ||
junhaoliao marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* | ||
* @param {object} props | ||
* @param {string} props.description | ||
* @param {boolean} props.isActive | ||
* @param {boolean} props.isError whether an error message should be shown instead of a step. | ||
junhaoliao marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* @param {string} props.label | ||
* @param {number} props.stepNumber | ||
junhaoliao marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* @return {React.ReactElement} | ||
*/ | ||
const LoadingStep = ({ | ||
description, | ||
isActive, | ||
isError, | ||
label, | ||
stepNumber, | ||
}) => { | ||
let color = "danger"; | ||
if (false === isError) { | ||
color = isActive ? | ||
"primary" : | ||
"neutral"; | ||
} | ||
junhaoliao marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
return ( | ||
<Step | ||
indicator={ | ||
<StepIndicator | ||
color={color} | ||
variant={isActive ? | ||
"solid" : | ||
"outlined"} | ||
> | ||
{stepNumber} | ||
</StepIndicator> | ||
} | ||
> | ||
<Typography | ||
color={color} | ||
level={"title-lg"} | ||
> | ||
{label} | ||
</Typography> | ||
<Typography level={"body-sm"}> | ||
{description} | ||
</Typography> | ||
</Step> | ||
); | ||
}; | ||
|
||
/** | ||
* Displays status of a pending query job. | ||
* | ||
* @param {object} props | ||
* @param {QueryLoadState} props.currentState | ||
* @param {string} props.errorMsg | ||
* @return {React.ReactElement} | ||
*/ | ||
const Loading = ({currentState, errorMsg}) => { | ||
const steps = []; | ||
QUERY_STATE_DESCRIPTIONS.forEach((state, index) => { | ||
const isActive = (currentState === index); | ||
steps.push( | ||
<LoadingStep | ||
description={state.description} | ||
isActive={isActive} | ||
isError={false} | ||
key={index} | ||
label={state.label} | ||
stepNumber={index + 1}/> | ||
); | ||
if (isActive && null !== errorMsg) { | ||
steps.push( | ||
<LoadingStep | ||
description={errorMsg} | ||
isActive={isActive} | ||
isError={true} | ||
key={`${index}-error`} | ||
label={"Error"} | ||
stepNumber={"X"}/> | ||
); | ||
} | ||
}); | ||
|
||
return ( | ||
<> | ||
<Sheet className={"loading-sheet"}> | ||
<Box className={"loading-progress-container"}> | ||
<LinearProgress | ||
determinate={null !== errorMsg} | ||
color={null === errorMsg ? | ||
"primary" : | ||
"danger"}/> | ||
</Box> | ||
<Box className={"loading-stepper-container"}> | ||
<Stepper | ||
className={"loading-stepper"} | ||
orientation={"vertical"} | ||
size={"lg"} | ||
> | ||
{steps} | ||
</Stepper> | ||
</Box> | ||
</Sheet> | ||
</> | ||
); | ||
}; | ||
|
||
export default Loading; |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
import { | ||
useEffect, | ||
useRef, | ||
useState, | ||
} from "react"; | ||
|
||
import { | ||
QUERY_LOAD_STATE, | ||
submitExtractIrJob, | ||
} from "../api/query.js"; | ||
import Loading from "./Loading.jsx"; | ||
|
||
|
||
/** | ||
* Submits queries and renders the query states. | ||
* | ||
* @return {React.ReactElement} | ||
*/ | ||
const Query = () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we name this the same as the file? |
||
const [queryState, setQueryState] = useState(QUERY_LOAD_STATE.SUBMITTING); | ||
const [errorMsg, setErrorMsg] = useState(null); | ||
const isFirstRun = useRef(true); | ||
|
||
useEffect(() => { | ||
if (false === isFirstRun.current) { | ||
return; | ||
} | ||
isFirstRun.current = false; | ||
|
||
const searchParams = new URLSearchParams(window.location.search); | ||
const origFileId = searchParams.get("origFileId"); | ||
const logEventIx = searchParams.get("logEventIx"); | ||
if (null === origFileId || null === logEventIx) { | ||
const error = "Non-IR-Extraction queries are not supported at the moment. " + | ||
"Either origFileId or logEventIx is missing from the URL parameters."; | ||
junhaoliao marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
console.error(error); | ||
setErrorMsg(error); | ||
} | ||
|
||
submitExtractIrJob(origFileId, Number(logEventIx), setQueryState, setErrorMsg).then(); | ||
}, []); | ||
|
||
return ( | ||
<Loading | ||
currentState={queryState} | ||
errorMsg={errorMsg}/> | ||
); | ||
}; | ||
|
||
export default Query; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,14 @@ const plugins = [ | |
]; | ||
|
||
const config = { | ||
devServer: { | ||
proxy: [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Proxy added to avoid CORS issues during development. |
||
{ | ||
context: ["/"], | ||
target: "http://localhost:3000", | ||
kirkrodrigues marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}, | ||
], | ||
}, | ||
devtool: isProduction ? | ||
"source-map" : | ||
"eval-source-map", | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe just
LogViewerWebuiUrl
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sg