Skip to content

Commit 7d46648

Browse files
authored
fix(hooks): stop calling translation hooks inside try/catch (#2879) (#2881)
Eleven call sites wrapped a React hook in try/catch to make it "provider-safe". `useObjectTranslation` and `useObjectLabel` already are and never throw, so the catch bought nothing and cost correctness: a throw AFTER the hook ran desyncs hook order on the next render. #2595/#2596 fixed this in the canonical `createSafeTranslation`; nine plugin-local copies kept the bug, and two more were found by the new lint rule (`ObjectView` had suppressed `react-hooks/rules-of-hooks` inline to keep its version). - Six re-implementations plus `data-table` now delegate to `createSafeTranslation`, which additionally returns `language`. - `plugin-gantt` and `ImportWizard` keep their local hooks — they fall back per key, which a single-probe factory cannot express — losing only the try/catch. - `ObjectTimeline` / `ObjectView` call the hook directly and probe the result. Adds `object-ui/no-try-catch-around-hook` (error). It matches only `use*` names, accepts member calls solely on `React`, and resets try-depth inside nested functions, so `vi.useRealTimers()` and `renderHook(() => useThing())` are not flagged — both were real code here and are pinned in its tests. `eslint-rules/**/*.test.js` matched no vitest project glob, so the local plugin's specs had never run in CI. Now included; all three pass. `ObjectTimeline`'s mock of `@object-ui/react` omitted `useObjectLabel` — the removed try/catch had been absorbing that gap. Mock completed.
1 parent 6e8fd3c commit 7d46648

21 files changed

Lines changed: 371 additions & 283 deletions

File tree

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
---
2+
"@object-ui/i18n": patch
3+
"@object-ui/plugin-detail": patch
4+
"@object-ui/plugin-timeline": patch
5+
"@object-ui/plugin-list": patch
6+
"@object-ui/plugin-calendar": patch
7+
"@object-ui/plugin-grid": patch
8+
"@object-ui/plugin-designer": patch
9+
"@object-ui/plugin-gantt": patch
10+
"@object-ui/plugin-view": patch
11+
"@object-ui/components": patch
12+
---
13+
14+
fix(hooks): stop calling translation hooks inside try/catch (objectui#2879)
15+
16+
Eleven call sites wrapped a React hook in `try`/`catch` to make it
17+
"provider-safe". `useObjectTranslation` and `useObjectLabel` already are — they
18+
read context optionally and fall back to react-i18next's global instance, and
19+
never throw. The `catch` bought nothing and cost correctness: a throw *after*
20+
the hook ran desyncs hook order on the next render, because React matches hooks
21+
positionally. objectui#2595/#2596 fixed exactly this in `@object-ui/i18n`'s
22+
`createSafeTranslation`; nine plugin-local re-implementations kept their own
23+
copy of the bug, and two more (`ObjectTimeline`, `ObjectView`) were found by the
24+
new lint rule below — `ObjectView` had even suppressed
25+
`react-hooks/rules-of-hooks` inline to keep it.
26+
27+
- Six exact re-implementations now delegate to `createSafeTranslation`:
28+
`plugin-detail`, `plugin-timeline`, `plugin-list`, `plugin-calendar`,
29+
`plugin-grid`'s `ObjectGrid`, `plugin-designer`.
30+
- `components`' `data-table` also delegates; `createSafeTranslation` now
31+
returns `language` alongside `t` so consumers that localize dates don't need
32+
a second hook call. Purely additive.
33+
- `plugin-gantt` and `plugin-grid`'s `ImportWizard` keep their local hooks —
34+
they fall back *per key*, which a single-probe factory cannot express and
35+
which their comments justify (a host dictionary that covers common keys but
36+
lags on newer ones). Only the `try`/`catch` is removed.
37+
- `ObjectTimeline` and `ObjectView` call the hook directly and probe the
38+
returned value, mirroring `useSafeFieldLabel`.
39+
40+
Adds `object-ui/no-try-catch-around-hook` (error) so a twelfth copy fails CI.
41+
It only matches `use*` names, accepts member calls solely on `React` (so
42+
`vi.useRealTimers()` is not a hook), and resets its try-depth inside nested
43+
functions (so `renderHook(() => useThing())` inside a `try` is fine) — both
44+
false positives were real code in this repo and are pinned in the rule's tests.
45+
46+
`eslint-rules/**/*.test.js` matched no vitest project glob, so the local
47+
plugin's specs had never run in CI. They are now included; all three pass.
48+
49+
`ObjectTimeline`'s test mock of `@object-ui/react` omitted `useObjectLabel`
50+
the removed `try`/`catch` had been silently absorbing that gap. The mock is now
51+
complete.

eslint-rules/index.js

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,12 @@
33
*/
44
import noSyntheticEventTrigger from './no-synthetic-event-trigger.js';
55
import noInlineSpecConfig from './no-inline-spec-config.js';
6+
import noTryCatchAroundHook from './no-try-catch-around-hook.js';
67

78
export default {
89
rules: {
910
'no-synthetic-event-trigger': noSyntheticEventTrigger,
1011
'no-inline-spec-config': noInlineSpecConfig,
12+
'no-try-catch-around-hook': noTryCatchAroundHook,
1113
},
1214
};
Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
/**
2+
* ObjectUI ESLint rule: no-try-catch-around-hook
3+
*
4+
* Bans wrapping a React hook call in `try`/`catch`.
5+
*
6+
* A `catch` that swallows a throw from a hook desyncs hook order on the next
7+
* render: the hooks that already ran before the throw were registered, the
8+
* ones after it were not, and React matches hooks positionally. The result is
9+
* the classic "Rendered fewer hooks than expected" crash, or silently reading
10+
* another hook's state.
11+
*
12+
* This exists because the pattern kept getting copy-pasted. objectui#2595 /
13+
* #2596 fixed it in `@object-ui/i18n`'s `createSafeTranslation`, but nine
14+
* plugin-local re-implementations kept their own `try { useObjectTranslation() }
15+
* catch {}` (objectui#2879) — every one of them written to make a hook
16+
* "provider-safe", which the hook already is.
17+
*
18+
* The correct pattern for "no provider mounted" is a VALUE probe after the
19+
* hook has run (e.g. `t(testKey) === testKey` → use English defaults), not a
20+
* `catch`. See `packages/i18n/src/useSafeTranslation.ts`.
21+
*
22+
* Scope, deliberately narrow to stay false-positive free:
23+
* - Only `use*` names, per React's own convention.
24+
* - Bare identifiers and `React.useX` only. `vi.useRealTimers()` and
25+
* `jest.useFakeTimers()` are test utilities, not hooks.
26+
* - The try-depth RESETS inside a nested function. A hook called in a
27+
* callback that merely happens to be written inside a try —
28+
* `renderHook(() => useCondition(...))` — runs during React's render of
29+
* that component, not in the try's synchronous flow, and is fine.
30+
*
31+
* @type {import('eslint').Rule.RuleModule}
32+
*/
33+
34+
/** React's own convention: a hook is `use` followed by a non-lowercase char. */
35+
function isHookName(name) {
36+
return typeof name === 'string' && /^use[A-Z0-9]/.test(name);
37+
}
38+
39+
/**
40+
* Resolve the called name, but only for shapes that can be a React hook.
41+
* `vi.useRealTimers()` / `jest.useFakeTimers()` are namespaced test helpers
42+
* that match the `use*` spelling by coincidence, so member calls are accepted
43+
* only on `React`.
44+
*/
45+
function hookCalleeName(callee) {
46+
if (callee.type === 'Identifier') return callee.name;
47+
if (
48+
callee.type === 'MemberExpression' &&
49+
callee.object.type === 'Identifier' &&
50+
callee.object.name === 'React' &&
51+
callee.property.type === 'Identifier'
52+
) {
53+
return callee.property.name;
54+
}
55+
return undefined;
56+
}
57+
58+
export default {
59+
meta: {
60+
type: 'problem',
61+
docs: {
62+
description:
63+
'Disallow calling a React hook inside try/catch — a caught throw desyncs hook order on the next render (objectui#2595/#2596/#2879).',
64+
recommended: true,
65+
},
66+
schema: [],
67+
messages: {
68+
banned:
69+
"Do not call the hook '{{name}}' inside try/catch — a caught throw desyncs hook order on the next render (objectui#2879). Call the hook unconditionally and probe its RETURN VALUE for the degraded case; see createSafeTranslation in packages/i18n/src/useSafeTranslation.ts.",
70+
},
71+
},
72+
create(context) {
73+
// Stack of try-depths, one frame per enclosing function. Entering a
74+
// function pushes a fresh 0 so a callback written inside a try starts
75+
// clean; leaving pops back to the outer count.
76+
const frames = [0];
77+
const bump = (n) => { frames[frames.length - 1] += n; };
78+
79+
const enterFn = () => { frames.push(0); };
80+
const exitFn = () => { frames.pop(); };
81+
82+
return {
83+
':function': enterFn,
84+
':function:exit': exitFn,
85+
86+
'TryStatement > BlockStatement': () => bump(1),
87+
'TryStatement > BlockStatement:exit': () => bump(-1),
88+
'CatchClause > BlockStatement': () => bump(1),
89+
'CatchClause > BlockStatement:exit': () => bump(-1),
90+
91+
CallExpression(node) {
92+
if (frames[frames.length - 1] === 0) return;
93+
const name = hookCalleeName(node.callee);
94+
if (!isHookName(name)) return;
95+
context.report({ node, messageId: 'banned', data: { name } });
96+
},
97+
};
98+
},
99+
};
Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
/**
2+
* Pins `no-try-catch-around-hook` against the two false positives that the
3+
* first draft of the rule produced when it was run across the repo:
4+
*
5+
* - `vi.useRealTimers()` / `jest.useFakeTimers()` — test helpers that match
6+
* the `use*` spelling by coincidence and are not React hooks.
7+
* - `renderHook(() => useThing())` inside a try — the hook runs during
8+
* React's render of the harness component, not in the try's synchronous
9+
* flow, so hook order is unaffected.
10+
*
11+
* Both were real code in this repo (packages/auth/.../createAuthClient.test.ts
12+
* and packages/react/.../useExpression.test.ts), which is why they are pinned
13+
* rather than left to judgement.
14+
*/
15+
import { describe, it, afterAll } from 'vitest';
16+
import { RuleTester } from 'eslint';
17+
import rule from './no-try-catch-around-hook.js';
18+
19+
RuleTester.afterAll = afterAll;
20+
RuleTester.it = it;
21+
RuleTester.describe = describe;
22+
23+
const ruleTester = new RuleTester();
24+
25+
ruleTester.run('no-try-catch-around-hook', rule, {
26+
valid: [
27+
// The correct shape: hook first, probe its return value.
28+
`function useThing() {
29+
const r = useObjectTranslation();
30+
return r.t('k') === 'k' ? FALLBACK : r.t;
31+
}`,
32+
// Namespaced test helpers are not hooks.
33+
`function t() { try { vi.useRealTimers(); } finally { done(); } }`,
34+
`function t() { try { jest.useFakeTimers(); } catch { /* noop */ } }`,
35+
// Hook inside a nested callback: runs under React's render, not the try.
36+
`function t() {
37+
try { renderHook(() => useCondition(SRC)); } finally { cleanup(); }
38+
}`,
39+
// try/catch with no hook at all.
40+
`function t() { try { JSON.parse(s); } catch { return null; } }`,
41+
// Lowercase `user*` must not be mistaken for a hook.
42+
`function t() { try { username(); } catch { return null; } }`,
43+
],
44+
invalid: [
45+
{
46+
code: `function useBad() {
47+
try { const r = useObjectTranslation(); return { t: r.t }; }
48+
catch { return { t: (k) => k }; }
49+
}`,
50+
errors: [{ messageId: 'banned' }],
51+
},
52+
{
53+
code: `function useBad() { try { return useObjectLabel(); } catch { return null; } }`,
54+
errors: [{ messageId: 'banned' }],
55+
},
56+
// `React.useX` member form is still a hook.
57+
{
58+
code: `function useBad() { try { return React.useMemo(f, []); } catch { return null; } }`,
59+
errors: [{ messageId: 'banned' }],
60+
},
61+
// A hook in the catch body is equally order-desyncing.
62+
{
63+
code: `function useBad() { try { f(); } catch { return useObjectTranslation(); } }`,
64+
errors: [{ messageId: 'banned' }],
65+
},
66+
],
67+
});

eslint.config.js

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,12 @@ export default tseslint.config({ ignores: ['**/dist', '**/.next', '**/node_modul
4343
// new violation fails CI; the existing surfaces were converted to direct
4444
// idempotent commands first, so this lints clean today.
4545
'object-ui/no-synthetic-event-trigger': 'error',
46+
// objectui#2879 ratchet — a hook called inside try/catch desyncs hook order
47+
// when the catch swallows a throw. #2595/#2596 fixed this in the canonical
48+
// createSafeTranslation; nine plugin-local copies kept it until #2879.
49+
// Error so a tenth copy fails CI; all known sites were converted first, so
50+
// this lints clean today.
51+
'object-ui/no-try-catch-around-hook': 'error',
4652
},
4753
}, {
4854
// Type-discipline ratchet, scoped to the canonical view-schema file: a

packages/components/src/renderers/complex/data-table.tsx

Lines changed: 7 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,8 @@ import { resolveIcon } from '../action/resolve-icon';
1313
import { useGridFieldAuthoring } from '../../context/gridFieldAuthoring';
1414
import { ComponentRegistry } from '@object-ui/core';
1515
import type { DataTableSchema } from '@object-ui/types';
16-
import { useObjectTranslation, useRowPredicate } from '@object-ui/react';
16+
import { useRowPredicate } from '@object-ui/react';
17+
import { createSafeTranslation } from '@object-ui/i18n';
1718
import {
1819
Table,
1920
TableHeader,
@@ -155,41 +156,12 @@ const TABLE_DEFAULT_TRANSLATIONS: Record<string, string> = {
155156
/**
156157
* Safe wrapper for useObjectTranslation that falls back to English defaults
157158
* when I18nProvider is not available (e.g., standalone usage).
159+
*
160+
* Delegates to `@object-ui/i18n`'s `createSafeTranslation` (which also
161+
* surfaces `language` for the date/number formatting below); the local copy
162+
* this replaced wrapped the hook in try/catch (rules-of-hooks, objectui#2879).
158163
*/
159-
function useTableTranslation() {
160-
try {
161-
const result = useObjectTranslation();
162-
const testValue = result.t('table.rowsPerPage');
163-
if (testValue === 'table.rowsPerPage') {
164-
return {
165-
t: (key: string, options?: Record<string, unknown>) => {
166-
let value = TABLE_DEFAULT_TRANSLATIONS[key] || key;
167-
if (options) {
168-
for (const [k, v] of Object.entries(options)) {
169-
value = value.replace(`{{${k}}}`, String(v));
170-
}
171-
}
172-
return value;
173-
},
174-
language: result.language || 'en',
175-
};
176-
}
177-
return { t: result.t, language: result.language || 'en' };
178-
} catch {
179-
return {
180-
t: (key: string, options?: Record<string, unknown>) => {
181-
let value = TABLE_DEFAULT_TRANSLATIONS[key] || key;
182-
if (options) {
183-
for (const [k, v] of Object.entries(options)) {
184-
value = value.replace(`{{${k}}}`, String(v));
185-
}
186-
}
187-
return value;
188-
},
189-
language: 'en',
190-
};
191-
}
192-
}
164+
const useTableTranslation = createSafeTranslation(TABLE_DEFAULT_TRANSLATIONS, 'table.rowsPerPage');
193165

194166
/**
195167
* Pull the most useful human-readable message out of whatever the save path

packages/i18n/src/useSafeTranslation.ts

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,10 +37,15 @@ export function createSafeTranslation(
3737
// "translations not configured" fallback.
3838
const result = useObjectTranslation();
3939
const testValue = result.t(testKey);
40+
// `language` is surfaced so consumers that localize dates/numbers
41+
// alongside their copy (data-table, gantt) don't have to call
42+
// `useObjectTranslation` a second time just to read it. With no provider
43+
// it is whatever react-i18next reports, which callers may treat as
44+
// "follow the runtime default".
4045
if (testValue === testKey) {
41-
return { t: fallbackT };
46+
return { t: fallbackT, language: result.language };
4247
}
43-
return { t: result.t };
48+
return { t: result.t, language: result.language };
4449
};
4550
}
4651

packages/plugin-calendar/src/CalendarView.tsx

Lines changed: 5 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ import {
2323
PopoverContent,
2424
PopoverTrigger
2525
} from "@object-ui/components"
26-
import { useObjectTranslation } from "@object-ui/i18n"
26+
import { createSafeTranslation } from "@object-ui/i18n"
2727

2828
const DEFAULT_EVENT_COLOR = "bg-blue-100 text-blue-900 border border-blue-200"
2929
const STABLE_DEFAULT_DATE = new Date()
@@ -87,43 +87,11 @@ const DEFAULT_TRANSLATIONS: Record<string, string> = {
8787
/**
8888
* Safe wrapper for useObjectTranslation that falls back to English defaults
8989
* when I18nProvider is not available (e.g., standalone usage outside console).
90+
*
91+
* Delegates to `@object-ui/i18n`'s `createSafeTranslation`; the local copy this
92+
* replaced wrapped the hook in try/catch (rules-of-hooks, objectui#2879).
9093
*/
91-
function useCalendarTranslation() {
92-
try {
93-
const result = useObjectTranslation()
94-
// Check if i18n is properly initialized by testing a known key
95-
const testValue = result.t('calendar.today')
96-
if (testValue === 'calendar.today') {
97-
// i18n returned the key itself — not initialized
98-
return {
99-
t: (key: string, options?: Record<string, unknown>) => {
100-
let value = DEFAULT_TRANSLATIONS[key] || key
101-
if (options) {
102-
for (const [k, v] of Object.entries(options)) {
103-
value = value.replace(`{{${k}}}`, String(v))
104-
}
105-
}
106-
return value
107-
},
108-
language: 'en',
109-
}
110-
}
111-
return { t: result.t, language: result.language }
112-
} catch {
113-
return {
114-
t: (key: string, options?: Record<string, unknown>) => {
115-
let value = DEFAULT_TRANSLATIONS[key] || key
116-
if (options) {
117-
for (const [k, v] of Object.entries(options)) {
118-
value = value.replace(`{{${k}}}`, String(v))
119-
}
120-
}
121-
return value
122-
},
123-
language: 'en',
124-
}
125-
}
126-
}
94+
const useCalendarTranslation = createSafeTranslation(DEFAULT_TRANSLATIONS, 'calendar.today')
12795

12896
export interface CalendarEvent {
12997
id: string | number

0 commit comments

Comments
 (0)