8.1 KiB
Code Review: dialog.tsx
Summary
The file is reasonably clean but has several style guide violations and readability issues: unnecessary destructuring, use of any, a for loop where a functional method works, unnecessary async on handlers that don't await, and some naming/inlining opportunities.
Issues
1. Unnecessary destructuring of theme (line 16)
Style guide says: avoid unnecessary destructuring, use dot notation.
// Before (line 16)
const { theme } = useTheme()
// After
const theme = useTheme().theme
Or, even better, inline theme directly where used (line 41) since it's only accessed once:
// Before (lines 16, 41)
const { theme } = useTheme()
...
backgroundColor={theme.backgroundPanel}
// After (line 41, remove line 16)
backgroundColor={useTheme().theme.backgroundPanel}
This removes a variable and the destructuring in one step. However, if the hook shouldn't be called inside JSX (reactive context matters in Solid), keeping a const theme = useTheme() and using theme.theme.backgroundPanel is the safe alternative.
2. Use of any type (line 100)
Style guide says: avoid any.
// Before (line 100)
replace(input: any, onClose?: () => void) {
// After
replace(input: JSX.Element, onClose?: () => void) {
JSX.Element is already imported and is the correct type for what gets stored as element in the stack.
3. Unnecessary async on mouse handlers (lines 21, 35)
Neither handler uses await. The async keyword is pointless.
// Before (line 21)
onMouseUp={async () => {
if (renderer.getSelection()) return
props.onClose?.()
}}
// After
onMouseUp={() => {
if (renderer.getSelection()) return
props.onClose?.()
}}
// Before (line 35)
onMouseUp={async (e) => {
if (renderer.getSelection()) return
e.stopPropagation()
}}
// After
onMouseUp={(e) => {
if (renderer.getSelection()) return
e.stopPropagation()
}}
4. for loops in clear and replace (lines 91, 105)
Style guide says: prefer functional array methods over for loops.
// Before (lines 91-93)
for (const item of store.stack) {
if (item.onClose) item.onClose()
}
// After
store.stack.forEach((item) => item.onClose?.())
Same fix applies to lines 105-107 in replace.
5. let used for focus (line 71)
focus is declared with let and mutated in multiple places, which is acceptable here since it genuinely needs reassignment. However, the type annotation Renderable | null is unnecessary since TypeScript can infer from the initial value and usage. Unfortunately, the initial value is just null so the type cannot be inferred. This one is fine as-is.
No change needed, but worth noting it was considered.
6. Recursive find function uses for loop (lines 76-82)
The nested find function uses a for loop. This is a recursive tree search where early return matters for performance, so a for loop is acceptable here. A .some() call would work too and be slightly more idiomatic:
// Before (lines 76-82)
function find(item: Renderable) {
for (const child of item.getChildren()) {
if (child === focus) return true
if (find(child)) return true
}
return false
}
// After
function find(item: Renderable): boolean {
return item.getChildren().some((child) => child === focus || find(child))
}
This collapses 7 lines into 1 and is equally readable. The return type annotation is needed here because of the recursive call.
7. Verbose onMouseUp handler in DialogProvider (lines 141-149)
The handler mixes await with .then()/.catch() chaining, which is inconsistent. Pick one style.
// Before (lines 141-149)
onMouseUp={async () => {
const text = renderer.getSelection()?.getSelectedText()
if (text && text.length > 0) {
await Clipboard.copy(text)
.then(() => toast.show({ message: "Copied to clipboard", variant: "info" }))
.catch(toast.error)
renderer.clearSelection()
}
}}
// After
onMouseUp={async () => {
const text = renderer.getSelection()?.getSelectedText()
if (!text || text.length === 0) return
try {
await Clipboard.copy(text)
toast.show({ message: "Copied to clipboard", variant: "info" })
} catch (e) {
toast.error(e)
}
renderer.clearSelection()
}}
Or, if we want to avoid try/catch per the style guide, keep the .then()/.catch() but drop async/await since the await does nothing useful when chained with .then():
// Alternative (no try/catch, no async)
onMouseUp={() => {
const text = renderer.getSelection()?.getSelectedText()
if (!text || text.length === 0) return
Clipboard.copy(text)
.then(() => toast.show({ message: "Copied to clipboard", variant: "info" }))
.catch(toast.error)
.finally(() => renderer.clearSelection())
}}
This version is cleaner: no async, no mixed styles, and clearSelection always runs via .finally().
8. text && text.length > 0 is redundant (line 143)
If text is a non-empty string, text.length > 0 is sufficient. If text is undefined, text?.length handles it. But since text comes from optional chaining it could be undefined, so text && text.length > 0 can be simplified:
// Before (line 143)
if (text && text.length > 0) {
// After
if (text?.length) {
Truthy check on length covers both undefined and 0.
9. store.stack passed directly to setStore loses reactivity safety (line 63)
// Before (line 63)
setStore("stack", store.stack.slice(0, -1))
This is fine functionally, but using a function form is more idiomatic for store updates derived from current state:
// After
setStore("stack", (s) => s.slice(0, -1))
Minor, but more consistent with Solid store patterns.
10. Unnecessary variable current used only once (line 61)
Style guide says: inline when a value is only used once.
// Before (lines 61-62)
const current = store.stack.at(-1)!
current.onClose?.()
// After
store.stack.at(-1)!.onClose?.()
11. Unnecessary variable value in useDialog (lines 162-166)
Style guide says: inline when a value is only used once. However, the conditional check requires the variable. This is fine as-is but could use a one-liner pattern:
// Before (lines 161-167)
export function useDialog() {
const value = useContext(ctx)
if (!value) {
throw new Error("useDialog must be used within a DialogProvider")
}
return value
}
// This is already clean. No change needed.
12. Batch in clear but not in replace (lines 94 vs 108)
In clear (line 94), setStore calls are wrapped in batch(). In replace (lines 108-114), two setStore calls are made without batch(). This is inconsistent. Either both should use batch or neither should (Solid batches synchronous updates in event handlers automatically, but clear and replace might be called outside event handlers).
// Before (lines 108-114)
setStore("size", "medium")
setStore("stack", [
{
element: input,
onClose,
},
])
// After
batch(() => {
setStore("size", "medium")
setStore("stack", [{ element: input, onClose }])
})
Summary of Changes
| # | Line(s) | Severity | Issue |
|---|---|---|---|
| 1 | 16 | Low | Unnecessary destructuring |
| 2 | 100 | Medium | any type |
| 3 | 21, 35 | Low | Unnecessary async |
| 4 | 91, 105 | Low | for loop instead of functional method |
| 5 | 71 | — | let is justified here |
| 6 | 76-82 | Low | Verbose recursive search |
| 7 | 141-149 | Medium | Mixed async/then style |
| 8 | 143 | Low | Redundant truthiness check |
| 9 | 63 | Low | Could use function form for store update |
| 10 | 61 | Low | Unnecessary intermediate variable |
| 11 | 162 | — | Fine as-is |
| 12 | 108 | Medium | Inconsistent batch usage |