Skip to content

Commit 17e9ada

Browse files
committed
fix: ProfileExplorer UI can be racy w.r.t selections and initial render
I don't think this PR fixes all of the issues, but it's a lot more stable now.
1 parent 9cbfd41 commit 17e9ada

File tree

3 files changed

+63
-31
lines changed

3 files changed

+63
-31
lines changed

plugins/plugin-codeflare/src/components/ProfileExplorer.tsx

+47-17
Original file line numberDiff line numberDiff line change
@@ -55,21 +55,43 @@ export default class ProfileExplorer extends React.PureComponent<Props, State> {
5555
this.init()
5656
}
5757

58+
private updateDebouncer: null | ReturnType<typeof setTimeout> = null
59+
5860
private readonly updateFn = () => {
59-
// slice to force a render; TODO we could do a comparison to avoid
60-
// false re-renders if we want to get fancy
61-
this.setState((curState) => {
62-
const profiles = curState.watcher.profiles.slice()
63-
if (!curState || !curState.profiles || curState.profiles.length === 0) {
64-
// sort the first time we get a list of profiles; TODO should
65-
// we re-sort if the list changes? what we want to avoid is
66-
// resorting simply because the selection changed
67-
profiles.sort((a, b) => b.lastUsedTime - a.lastUsedTime)
68-
}
69-
return {
70-
profiles,
71-
}
72-
})
61+
if (this.updateDebouncer) {
62+
clearTimeout(this.updateDebouncer)
63+
}
64+
65+
// hmm, this is imperfect... the watcher seems to give us [A],
66+
// then [A,B], then [A,B,C] in quick succession. is there any way
67+
// to know that we are done with the initial batch? for now, we do
68+
// some debouncing.
69+
this.updateDebouncer = setTimeout(() => {
70+
this.setState((curState) => {
71+
if (JSON.stringify(curState.watcher.profiles) === JSON.stringify(curState.profiles)) {
72+
return null
73+
}
74+
75+
const profiles = curState.watcher.profiles.slice()
76+
77+
let selectedProfile = curState.selectedProfile
78+
if (!curState || !curState.profiles || curState.profiles.length === 0) {
79+
// sort the first time we get a list of profiles; TODO should
80+
// we re-sort if the list changes? what we want to avoid is
81+
// resorting simply because the selection changed
82+
profiles.sort((a, b) => b.lastUsedTime - a.lastUsedTime)
83+
84+
// also emit an initial profile selection event
85+
selectedProfile = profiles[0].name
86+
emitSelectProfile(selectedProfile)
87+
}
88+
89+
return {
90+
profiles,
91+
selectedProfile,
92+
}
93+
})
94+
}, 100)
7395
}
7496

7597
private async init() {
@@ -103,6 +125,14 @@ export default class ProfileExplorer extends React.PureComponent<Props, State> {
103125
}
104126
}
105127

128+
private prettyMillis(duration: number) {
129+
if (duration < 1000) {
130+
return "just now"
131+
} else {
132+
return prettyMillis(duration, { compact: true }) + " ago"
133+
}
134+
}
135+
106136
public render() {
107137
if (this.state && this.state.catastrophicError) {
108138
return "Internal Error"
@@ -111,16 +141,16 @@ export default class ProfileExplorer extends React.PureComponent<Props, State> {
111141
} else {
112142
return (
113143
<Grid className="codeflare--gallery-grid flex-fill sans-serif top-pad left-pad right-pad bottom-pad" hasGutter>
114-
{this.state.profiles.map((_, idx) => (
144+
{this.state.profiles.map((_) => (
115145
<GridItem key={_.name}>
116146
<Tile
117147
className="codeflare--tile"
118148
data-profile={_.name}
119149
title={_.name}
120-
isSelected={!this.state.selectedProfile ? idx === 0 : this.state.selectedProfile === _.name}
150+
isSelected={this.state.selectedProfile === _.name}
121151
onClick={this.onSelect}
122152
>
123-
{`Last used ${prettyMillis(Date.now() - _.lastUsedTime, { compact: true })} ago`}
153+
{`Last used ${this.prettyMillis(Date.now() - _.lastUsedTime)}`}
124154
</Tile>
125155
</GridItem>
126156
))}

plugins/plugin-codeflare/src/components/RestartableTerminal.tsx

+11-5
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,8 @@ type State = {
5353
* tne pty and recreates the terminal if this happens.
5454
*/
5555
export default class RestartableTerminal extends React.PureComponent<Props, State> {
56+
private mounted = false
57+
5658
private async initPty() {
5759
try {
5860
// we need this to wire the pty output through to the Terminal
@@ -72,11 +74,13 @@ export default class RestartableTerminal extends React.PureComponent<Props, Stat
7274
passthrough.write(_)
7375
},
7476
onReady: (job) => {
75-
this.setState((curState) => ({
76-
job,
77-
watch: () => watch(passthrough, job),
78-
startCount: !curState || curState.startCount === undefined ? 0 : curState.startCount + 1,
79-
}))
77+
if (this.mounted) {
78+
this.setState((curState) => ({
79+
job,
80+
watch: () => watch(passthrough, job),
81+
startCount: !curState || curState.startCount === undefined ? 0 : curState.startCount + 1,
82+
}))
83+
}
8084
},
8185
})
8286
} catch (err) {
@@ -86,10 +90,12 @@ export default class RestartableTerminal extends React.PureComponent<Props, Stat
8690
}
8791

8892
public componentDidMount() {
93+
this.mounted = true
8994
this.initPty()
9095
}
9196

9297
public componentWillUnmount() {
98+
this.mounted = false
9399
if (this.state.job) {
94100
this.state.job.abort()
95101
}

plugins/plugin-codeflare/src/components/SelectedProfileTerminal.tsx

+5-9
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@
1515
*/
1616

1717
import React from "react"
18-
import { Profiles } from "madwizard"
1918
import { Loading } from "@kui-shell/plugin-client-common"
2019

2120
import Terminal, { Props } from "./RestartableTerminal"
@@ -31,7 +30,7 @@ export default class SelectedProfileTerminal extends React.PureComponent<Props,
3130
public constructor(props: Props) {
3231
super(props)
3332
onSelectProfile(this.onSelect)
34-
this.init()
33+
// this.init()
3534
}
3635

3736
public componentWillUnmount() {
@@ -43,16 +42,13 @@ export default class SelectedProfileTerminal extends React.PureComponent<Props,
4342
this.setState({ cmdline })
4443
}
4544

46-
private async init() {
45+
/* private async init() {
4746
const cmdline = await this.cmdline()
4847
this.setState({ cmdline })
49-
}
48+
} */
5049

51-
private async cmdline(selectedProfile?: string) {
52-
return this.props.cmdline.replace(
53-
SelectedProfileTerminal.selectedProfilePattern,
54-
selectedProfile || (await Profiles.lastUsed())
55-
)
50+
private async cmdline(selectedProfile: string) {
51+
return this.props.cmdline.replace(SelectedProfileTerminal.selectedProfilePattern, selectedProfile)
5652
}
5753

5854
public render() {

0 commit comments

Comments
 (0)