Skip to content

Commit c0130e5

Browse files
committed
feat(FR-2496): EduAppLauncher new-window fallback + launch-flow hardening (#6498)
Resolves #6497 ([FR-2496](https://lablup.atlassian.net/browse/FR-2496)) — sub-task of [FR-2470](https://lablup.atlassian.net/browse/FR-2470). Follow-up polish on the EduAppLauncher refactor stack. Three related improvements surfaced during manual testing of the completed FR-2483FR-2487 work. ## 1. New-window fallback UI (`EduAppLauncher.tsx`) When the launch completes, the original code only auto-opens the app window. Browsers with popup blockers silently drop the `window.open` call. This PR: - Stores the resolved `appConnectUrl` in the `done` stage so the UI knows where to navigate - Adds a visible primary button ("If a new window did not open automatically, click here.") on the completion card - Adds a secondary "Refresh page" action ## 2. Migrate to React Compiler + `useEffectEvent` - Remove manual `useCallback` / `useMemoizedFn` wrappers now that the file uses the `'use memo'` directive (React Compiler memoizes automatically) - Wrap the launch-effect body in `useEffectEvent` so the `useEffect` deps stay precisely `[active, apiEndpoint]` — no more `// eslint-disable-next-line react-hooks/exhaustive-deps` - The `onLaunchEffect` block is placed **after** `_launch` is declared to respect the temporal-dead-zone rule (caught by `react-hooks/immutability` lint) ## 3. wsproxy `proxyURL` truthy-value hardening (`useBackendAIAppLauncher.tsx`) Root-cause fix: the Backend.AI client initializes `_config.proxyURL = null` by default, and `config.toml` can ship `wsproxy.proxyURL = ""` for environments that rely on the local default. The previous `!== undefined` check let both null and empty-string pass through, which later crashed on `url.endsWith(...)`. Switch to a truthy check so both fall through to the default `http://127.0.0.1:5050/`. Also adds structured debug logging through the wsproxy launch path (`getWSProxyVersion`, `getProxyURL`, `_launchApp`) so the version/URL/wsproxy response chain is observable from the browser console. ## 4. i18n New keys `eduAppLauncher.OpenAppInNewWindow` and `eduAppLauncher.RefreshPage` added in all 21 supported languages. ## 5. New rule: `.claude/rules/use-effect-event.md` Codifies when to reach for `useEffectEvent` over `useCallback` under the `'use memo'` directive. Captures the temporal-dead-zone gotcha that this PR itself tripped on. ## Out of scope SSH/VS Code Desktop modal apps, Edu API runtime state handling (inherited from FR-2470 out-of-scope list). ## Test plan - [ ] With a popup-blocker enabled: completion card shows the fallback button and it navigates correctly on click - [ ] Without a popup-blocker: original `window.open` path still works; completion card still renders the fallback as a recovery option - [ ] `config.toml` with `wsproxy.proxyURL = ""` no longer crashes; falls through to `http://127.0.0.1:5050/` - [ ] `config.toml` with a real proxy URL still honors it - [ ] Browser console shows the new `[wsproxy]` / `[_launchApp]` logs at `info` level - [ ] No `useCallback` / `useMemoizedFn` regressions in `EduAppLauncher.tsx` [FR-2496]: https://lablup.atlassian.net/browse/FR-2496?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ [FR-2470]: https://lablup.atlassian.net/browse/FR-2470?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
1 parent 64f61cb commit c0130e5

24 files changed

Lines changed: 705 additions & 199 deletions

.claude/rules/use-effect-event.md

Lines changed: 154 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,154 @@
1+
---
2+
description: Use React 19.2's useEffectEvent to keep useEffect dependencies precise
3+
paths:
4+
- "react/**/*.{tsx,ts}"
5+
- "packages/backend.ai-ui/**/*.{tsx,ts}"
6+
---
7+
8+
# `useEffectEvent` Convention
9+
10+
This project targets **React 19.2+** which exposes `useEffectEvent` as a stable
11+
API (`@version 19.2.0` in `@types/react`). Use it to keep `useEffect` dependency
12+
arrays focused on the values that genuinely drive synchronization, while still
13+
reading the latest props/state/closures inside the effect body.
14+
15+
## Why
16+
17+
`useEffect` re-runs whenever any value in its dep array changes by identity.
18+
When you call helper callbacks (`onSomething`, `logger`, parent props) from
19+
inside an effect:
20+
21+
- Including them in deps causes spurious re-runs every time the parent
22+
re-renders, because their identity changes.
23+
- Omitting them with `// eslint-disable-next-line react-hooks/exhaustive-deps`
24+
silently captures stale closures.
25+
- Wrapping each one in `useCallback` and propagating that everywhere creates
26+
cascading dep arrays that are hard to maintain.
27+
- `ahooks` `useMemoizedFn` was a popular workaround pre-19.2 but is now
28+
redundant.
29+
30+
`useEffectEvent` solves all of these. It returns a stable function whose body
31+
always reads the *latest* values from the surrounding closure. The dep array
32+
of the surrounding `useEffect` only needs the values that actually represent
33+
"things this effect should re-synchronize on."
34+
35+
## Rules
36+
37+
1. Whenever an `useEffect` body needs to call a helper that closes over props,
38+
state, or context but is **not** part of the synchronization key, wrap that
39+
helper in `useEffectEvent` and call it from the effect.
40+
2. The `useEffect` dep array should contain **only** the values the effect
41+
genuinely synchronizes against — typically 0–2 entries: primary inputs
42+
like a fetched ID, a query parameter, or `active`/`apiEndpoint` flags.
43+
Helper callbacks, loggers, and parent props that you only *call* from
44+
inside the effect must not appear in deps; move them into a
45+
`useEffectEvent` callback instead.
46+
3. Do **not** disable `react-hooks/exhaustive-deps` to omit deps. Either:
47+
include the value in deps (because the effect should re-react to it), or
48+
move the access into a `useEffectEvent` callback.
49+
4. `useEffectEvent` callbacks are **not** safe to call outside of effects.
50+
Treat them as effect-internal helpers, not as event handlers passed to JSX
51+
(use a regular function or `useCallback` for that — though under the
52+
`'use memo'` directive even those are unnecessary).
53+
5. Prefer `useEffectEvent` over `ahooks` `useMemoizedFn`. Do not introduce new
54+
`useMemoizedFn` usage; remove it when touching nearby code.
55+
56+
## Pattern
57+
58+
### ❌ Without `useEffectEvent`: stale closure or spurious re-runs
59+
60+
```tsx
61+
const Child: React.FC<{ data: Data; onLoaded: (d: Data) => void }> = ({
62+
data,
63+
onLoaded,
64+
}) => {
65+
useEffect(() => {
66+
if (data) {
67+
onLoaded(data); // closure over `onLoaded`
68+
}
69+
// eslint-disable-next-line react-hooks/exhaustive-deps
70+
}, [data]); // ← omitted `onLoaded`; stale closure if parent updates it
71+
};
72+
```
73+
74+
```tsx
75+
useEffect(() => {
76+
if (data) onLoaded(data);
77+
}, [data, onLoaded]); // ← `onLoaded` re-fires the effect on every parent
78+
// re-render; effect re-runs even when `data`
79+
// hasn't changed
80+
```
81+
82+
### ✅ With `useEffectEvent`: precise deps, latest closure
83+
84+
```tsx
85+
import { useEffect, useEffectEvent } from 'react';
86+
87+
const Child: React.FC<{ data: Data; onLoaded: (d: Data) => void }> = ({
88+
data,
89+
onLoaded,
90+
}) => {
91+
// Read the latest `onLoaded` from the closure without forcing it
92+
// into the dep array.
93+
const onResolved = useEffectEvent(() => {
94+
if (data) {
95+
onLoaded(data);
96+
}
97+
});
98+
99+
useEffect(() => {
100+
onResolved();
101+
}, [data]); // ← only the genuine reactive dep
102+
};
103+
```
104+
105+
### Pattern for "fire once on mount" with closures
106+
107+
```tsx
108+
const onMount = useEffectEvent(() => {
109+
// Reads latest props/closures every time it is called.
110+
doExpensiveSetup({ foo, bar, onDone });
111+
});
112+
113+
useEffect(() => {
114+
onMount();
115+
}, []); // ← only when mount key changes (and `key` prop on JSX
116+
// guarantees a fresh mount, not in-place updates)
117+
```
118+
119+
**StrictMode caveat**: empty-dep effects fire **twice** in dev StrictMode.
120+
If the side effect is not idempotent (starts a network request, opens a
121+
window, etc.), guard with a `useRef` flag, or rely on the parent's `key`
122+
prop to remount cleanly so identity is the synchronization key.
123+
124+
## When NOT to use `useEffectEvent`
125+
126+
- **For values the effect *should* react to**. Example: a query parameter that,
127+
when changed, must trigger a re-fetch. Keep these in deps.
128+
- **For event handlers passed to JSX** (`onClick`, `onChange`, etc.). The
129+
React Compiler under `'use memo'` already memoizes regular functions; or
130+
use `useCallback` if you must. `useEffectEvent` callbacks are documented
131+
as effect-internal and have undefined behavior outside of effects.
132+
- **In components without `'use memo'`**: prefer adding the directive (per
133+
`react.instructions.md`) before adopting `useEffectEvent`. The two work
134+
together; using one without the other is a partial solution and you will
135+
still be re-creating helper functions on every render.
136+
137+
## Verification
138+
139+
After editing a file that uses `useEffect`, ensure:
140+
141+
- No `useMemoizedFn` from `ahooks` remains (project-wide convention).
142+
- No `// eslint-disable-next-line react-hooks/exhaustive-deps` introduced just
143+
to drop a callback dep — convert to `useEffectEvent` instead.
144+
- `bash scripts/verify.sh` passes; `react-hooks/exhaustive-deps` lint should be
145+
satisfied without disable comments.
146+
147+
## Related
148+
149+
- `'use memo'` directive (`react.instructions.md`): the React Compiler removes
150+
the need for `useCallback`/`useMemo` for regular values and JSX-passed
151+
callbacks. `useEffectEvent` complements this for the effect-internal case
152+
where the compiler cannot express "ignore this dep for re-synchronization".
153+
- `component-props-extension.md`: wrapper component prop conventions.
154+
- `antd-v6-props.md`: Ant Design v6 prop name conventions.

0 commit comments

Comments
 (0)