Uh oh!
There was an error while loading. Please reload this page.
fix(hooks): stop calling translation hooks inside try/catch (#2879) - #2881
Merged
Conversation
Eleven call sites wrapped a React hook in try/catch to make it "provider-safe". `useObjectTranslation` and `useObjectLabel` already are — they read context optionally and fall back to react-i18next's global instance, and never throw. The catch bought nothing and cost correctness: a throw AFTER the hook ran desyncs hook order on the next render, since React matches hooks positionally. #2595/#2596 fixed exactly this in `@object-ui/i18n`'s `createSafeTranslation`; nine plugin-local re-implementations kept their own copy of the bug. Two more (`ObjectTimeline`, `ObjectView`) were found by the new lint rule and had been missed by a grep — `ObjectView` had suppressed `react-hooks/rules-of-hooks` inline to keep its version. - Six exact re-implementations now delegate to `createSafeTranslation`. - `data-table` also delegates; `createSafeTranslation` now returns `language` alongside `t` (purely additive) so date-formatting consumers need only one hook call. - `plugin-gantt` and `ImportWizard` KEEP their local hooks: they fall back per key, which a single-probe factory cannot express and which their comments justify. Only the try/catch is removed. - All imports go to `@object-ui/i18n` directly, matching the existing convention. Routing them through `@object-ui/react` tripped a module-init cycle — `data-table` blew up at import time in the combined suite while passing alone. Adds `object-ui/no-try-catch-around-hook` (error) so a twelfth copy fails CI. The first draft was too broad; it now matches only `use*` names, accepts member calls solely on `React` (so `vi.useRealTimers()` is not a hook), and resets its try-depth inside nested functions (so `renderHook(() => useThing())` in a try is fine). Both false positives 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, which is precisely the kind of real breakage a swallowed throw hides. Mock completed. Full suite 7604 passed; type-check clean except a pre-existing maplibre-gl default-export error in plugin-map, identical on baseline. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TubWYdWquVkS9dj733sDmC
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
os-zhuang
marked this pull request as ready for review
July 28, 2026 01:32
Uh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes#2879.
The bug
Eleven call sites wrapped a React hook in
try/catchto make it "provider-safe".useObjectTranslationanduseObjectLabelalready are — they read context optionally (context?.language || i18n.language || 'en') and fall back to react-i18next's global instance. They never throw. CompareuseI18nContextimmediately below, which does throw.So the
catchbought nothing and cost correctness: a throw after the hook ran desyncs hook order on the next render, because React matches hooks positionally.This is not a new discovery — #2595/#2596 fixed exactly this in
@object-ui/i18n'screateSafeTranslation, and its test file says so explicitly:Nine plugin-local re-implementations kept their own copy of the bug.
plugin-detail's was even nameduseSafeTranslation— the canonical name, minus the fix.What changed
createSafeTranslationplugin-detail,plugin-timeline,plugin-list,plugin-calendar,plugin-grid/ObjectGrid,plugin-designer,components/data-tabletry/catchplugin-gantt,plugin-grid/ImportWizardplugin-timeline/ObjectTimeline,plugin-view/ObjectViewGantt and ImportWizard are deliberately not consolidated. They fall back per key, not on a single probe — so a host dictionary that translates the common keys but lags on newer ones still resolves what it has. A single-probe factory can't express that, and both files document the reasoning. Consolidating them would have been a silent behaviour regression.
createSafeTranslationnow returnslanguagealongsidet(purely additive) sodata-table'sIntl.DateTimeFormatconsumer doesn't need a second hook call.Two things this surfaced that a grep could not
1. Two more violations I'd missed. My initial grep found 9; the lint rule found 11.
ObjectViewhad an inlineeslint-disable-next-line react-hooks/rules-of-hookswith the rationale "always the first call; catch only guards a missing provider" — which is the exact misconception the canonical comment refutes.2. The
try/catchwas masking a broken test mock. With it removed,ObjectTimeline.test.tsxfailed: itsvi.mock('@object-ui/react')omitsuseObjectLabel. The catch had been absorbing that — which is precisely the class of real breakage a swallowed throw hides. Mock completed.The lint rule
object-ui/no-try-catch-around-hook, error-level, so a twelfth copy fails CI.My first draft was too broad and I only found out by running it repo-wide — it flagged
vi.useRealTimers()(a vitest helper matchinguse*by coincidence) andrenderHook(() => useCondition(…))inside atry(the hook runs during React's render of the harness, not in the try's flow). Both were real code in this repo. The rule now matches onlyuse*names, accepts member calls solely onReact, and resets its try-depth inside nested functions. Both false positives are pinned asvalidcases in its tests.eslint-rules/**/*.test.jsmatched no vitest project glob, so the local plugin's specs — including the pre-existingno-synthetic-event-triggerandno-inline-spec-configones — had never run in CI. Now included; all three files pass (25 tests).Verification
turbo type-check: clean except a pre-existingmaplibre-gldefault-export error inplugin-map, verified identical on a stashed baselineOne nuance worth flagging: imports go to
@object-ui/i18ndirectly, not via@object-ui/react. I initially routed them through thereactbarrel, which tripped a module-init cycle —data-tablethrew at import time in the combined suite while passing in isolation. Importing directly matches what the three existingcomponentsconsumers already do.🤖 Generated with Claude Code
https://claude.ai/code/session_01TubWYdWquVkS9dj733sDmC
Generated by Claude Code