mirror of
https://github.com/arsvendg/Stirling-PDF.git
synced 2026-07-14 10:34:06 +02:00
## Summary
The frontend was rerendering excessively across many interactions —
typing, clicking tools, opening modals, toggling the sidebar — because
**multiple compounding ref-instability cascades defeated `memo()` checks
in hot paths**. This PR fixes the cascades structurally.
Seven focused commits, low-to-high blast radius:
1. `perf(contexts): memoize BannerContext provider value`
2. `perf(contexts): memoize CommentAuthor and ActiveDocument provider
values`
3. `perf(contexts): memoize AppConfigContext provider value`
4. `perf(useToolManagement): stop spreading tool entries to keep refs
stable` — **root-cause fix**
5. `refactor(ToolPicker): hoist module-scope styles and helpers`
6. `feat(ToolWorkflowContext): add ref-stable Actions and Data subset
contexts` — additive
7. `perf(tools): migrate hot consumers to slim contexts and wrap in
memo()`
(Plus `style: apply prettier formatting` for CI.)
## What was wrong
Whenever something high-up in the tree caused a render, a chain of
unstable references propagated downward and forced every `ToolButton` to
re-execute its full body (hooks, derived computations, hook
subscriptions to other contexts). The chain:
- **4 unstable Context providers** (`Banner`, `CommentAuthor`,
`ActiveDocument`, `AppConfig`) were passing fresh `value={{ … }}`
objects on every render. Every consumer rerendered on every ancestor
render.
- **`useToolManagement.toolRegistry`** spread `{...baseTool, name,
description}` — a no-op spread that manufactured a new tool object
identity on every memo recompute.
- **The big `ToolWorkflowContext`** (25+ fields including
`state.searchQuery`) rebuilt its entire value on every
keystroke/click/toggle, forcing every `useToolWorkflow()` consumer (~36
files) to rerender.
- **`useToolNavigation`** transitively subscribed every `ToolButton` to
the full workflow context.
- **`ToolButton` & `ToolPicker`** weren't `memo()`-wrapped, so nothing
checked.
- **`ToolPanel`** passed inline `onSelect={(id) =>
handleToolSelect(...)}` — fresh ref every render, defeats child
memoization.
- **`ToolPicker`** allocated inline styles / `[]` / `toTitleCase` inside
the function body — churned `useToolSections`'s internal memo.
## Interaction matrix — what improves
The PR fixes the underlying ref-stability problem; the same fix benefits
*every* interaction that previously triggered the cascade:
| Interaction | Before | After |
|---|---|---|
| **Typing in tool search** | All visible buttons rerender per keystroke
| Only buttons whose matched-text changes rerender |
| **Clicking a tool** | All 36 `useToolWorkflow()` consumers rerender |
Only previously-selected and newly-selected buttons rerender (via
`isSelected` prop) |
| **Toggling sidebar / panel mode / reader mode** | Every tool button
rerenders | Tool components stay still (slim context doesn't see UI
state) |
| **Switching workbench / navigation** | `handleToolSelect` identity
changes → cascades through `onSelect` props | Ref-stabilized in Actions
context. Identity stable. Children's memo bails |
| **Modal/dialog open/close** | AppConfig churns → every `useAppConfig`
consumer rerenders (ToolButton reads `premiumEnabled`) | AppConfig
memoized; consumers rerender only when config changes |
| **Banner show/hide** | BannerProvider value churns → every consumer
rerenders on any ancestor render | Memoized; AppLayout rerenders only
when banner content changes |
| **Any state update high in the tree** | Compounding cascade defeats
memo everywhere | Stable subscriptions; memo bails out |
## Evidence
Per-keystroke prop instability on `ToolButton` (cleanest measurable
signal, captured via custom memo comparators logging which prop refs
differ):
| | `tool` ref diffs | `onSelect` ref diffs | `matchedSynonym` value
diffs | Total |
|---|---|---|---|---|
| Before | 18 | 18 | 6 | **42** |
| After | 0 | 0 | 6 | **6 (all legitimate)** |
→ **86% reduction** in spurious per-keystroke prop instability. The 6
remaining matched-synonym diffs are correct (different substring
highlighted per keystroke).
Context value rebuild counts during a keystroke (verified with
instrumented `useMemo` factories): `useToolWorkflowData=0`,
`useToolWorkflowActions=0`, `AppConfigContext=0`.
The same stabilization applies to click/toggle/modal interactions — they
were all driven by the same cascading invalidations.
## Honest caveat on render-count metrics
`React.Profiler` counts and function-body execution counts in **dev
mode** came back identical before vs after (StrictMode + concurrent
rendering + Mantine internal commits dominate the numbers). The PR's
value is measured against the **prop-stability signal** above, not
Profiler counts. Production builds — where StrictMode doesn't
double-render and Mantine internals aren't constantly committing — will
show memo bail out properly.
## Risk × benefit
| # | Commit | Risk | Benefit |
|---|--------|------|---------|
| 1 | BannerContext memo | ⬛ Trivial | 🟦 Small |
| 2 | CommentAuthor + ActiveDocument memo | ⬛ Trivial | 🟦 Small |
| 3 | AppConfig memo | ⬛ Trivial | 🟦 Moderate (wide consumer base) |
| 4 | useToolManagement spread removal | ⬛ Trivial | 🟥 **High (root
cause)** |
| 5 | ToolPicker hoist | ⬛ Trivial | 🟦 Small |
| 6 | ToolWorkflowContext split | 🟧 Low-Med | 🟥 **High (foundation)** |
| 7 | Hot consumer migration + memo | 🟧 Low-Med | 🟥 **High
(actualization)** |
Commit 6 introduces an invariant: ref-stabilized callbacks in the
Actions context must only be invoked from event handlers (post-commit),
never during render. All current call sites comply.
## Test plan
- [x] `npx playwright test --project=stubbed` — 145 / 6 skipped / 0
failed before and after.
- [x] Targeted regression: `main-dashboard`, `tool-search`, `navigation`
— 11/11 passing.
- [x] CI passing on commits (one infrastructure flake on
`docker-compose-tests` — "No space left on device" — unrelated;
rerunning).
- [ ] Manual sanity check in a dev build after merge.
## What this enables
The same Actions + Data subset-context pattern can be applied to
`FileContext`, `NavigationContext`, and other big contexts. The
foundation is in place.
253 lines
7.5 KiB
TypeScript
253 lines
7.5 KiB
TypeScript
import React, {
|
|
createContext,
|
|
useContext,
|
|
useMemo,
|
|
useState,
|
|
useEffect,
|
|
ReactNode,
|
|
useCallback,
|
|
} from "react";
|
|
import apiClient from "@app/services/apiClient";
|
|
import { getSimulatedAppConfig } from "@app/testing/serverExperienceSimulations";
|
|
import type { AppConfig, AppConfigBootstrapMode } from "@app/types/appConfig";
|
|
import { useJwtConfigSync } from "@app/hooks/useJwtConfigSync";
|
|
|
|
/**
|
|
* Sleep utility for delays
|
|
*/
|
|
function sleep(ms: number): Promise<void> {
|
|
return new Promise((resolve) => setTimeout(resolve, ms));
|
|
}
|
|
|
|
export interface AppConfigRetryOptions {
|
|
maxRetries?: number;
|
|
initialDelay?: number;
|
|
}
|
|
|
|
export type { AppConfig, AppConfigBootstrapMode };
|
|
|
|
interface AppConfigContextValue {
|
|
config: AppConfig | null;
|
|
loading: boolean;
|
|
error: string | null;
|
|
refetch: () => Promise<void>;
|
|
}
|
|
|
|
const AppConfigContext = createContext<AppConfigContextValue | undefined>({
|
|
config: null,
|
|
loading: true,
|
|
error: null,
|
|
refetch: async () => {},
|
|
});
|
|
|
|
/**
|
|
* Provider component that fetches and provides app configuration
|
|
* Should be placed at the top level of the app, before any components that need config
|
|
*/
|
|
export interface AppConfigProviderProps {
|
|
children: ReactNode;
|
|
retryOptions?: AppConfigRetryOptions;
|
|
initialConfig?: AppConfig | null;
|
|
bootstrapMode?: AppConfigBootstrapMode;
|
|
autoFetch?: boolean;
|
|
onConfigLoaded?: (config: AppConfig) => void;
|
|
}
|
|
|
|
export const AppConfigProvider: React.FC<AppConfigProviderProps> = ({
|
|
children,
|
|
retryOptions,
|
|
initialConfig = null,
|
|
bootstrapMode = "blocking",
|
|
autoFetch = true,
|
|
onConfigLoaded,
|
|
}) => {
|
|
const isBlockingMode = bootstrapMode === "blocking";
|
|
const [config, setConfig] = useState<AppConfig | null>(initialConfig);
|
|
const [error, setError] = useState<string | null>(null);
|
|
// Track how many times we've attempted to fetch. useRef avoids re-renders that can trigger loops.
|
|
const fetchCountRef = React.useRef(0);
|
|
const [hasResolvedConfig, setHasResolvedConfig] = useState(
|
|
Boolean(initialConfig) && !isBlockingMode,
|
|
);
|
|
const [loading, setLoading] = useState(!hasResolvedConfig);
|
|
|
|
const onConfigLoadedRef = React.useRef(onConfigLoaded);
|
|
onConfigLoadedRef.current = onConfigLoaded;
|
|
|
|
const maxRetries = retryOptions?.maxRetries ?? 0;
|
|
const initialDelay = retryOptions?.initialDelay ?? 1000;
|
|
|
|
const fetchConfig = useCallback(
|
|
async (force = false) => {
|
|
// Prevent duplicate fetches unless forced
|
|
if (!force && fetchCountRef.current > 0) {
|
|
console.debug("[AppConfig] Already fetched, skipping");
|
|
return;
|
|
}
|
|
|
|
// Mark that we've attempted a fetch to prevent repeated auto-fetch loops
|
|
fetchCountRef.current += 1;
|
|
|
|
const shouldBlockUI = !hasResolvedConfig || isBlockingMode;
|
|
if (shouldBlockUI) {
|
|
setLoading(true);
|
|
}
|
|
setError(null);
|
|
|
|
const startTime = performance.now();
|
|
for (let attempt = 0; attempt <= maxRetries; attempt++) {
|
|
try {
|
|
const testConfig = getSimulatedAppConfig();
|
|
if (testConfig) {
|
|
setConfig(testConfig);
|
|
setHasResolvedConfig(true);
|
|
setLoading(false);
|
|
return;
|
|
}
|
|
|
|
if (attempt > 0) {
|
|
const delay = initialDelay * Math.pow(2, attempt - 1);
|
|
console.log(
|
|
`[AppConfig] Retry attempt ${attempt}/${maxRetries} after ${delay}ms delay...`,
|
|
);
|
|
await sleep(delay);
|
|
} else {
|
|
console.log("[AppConfig] Fetching app config...");
|
|
}
|
|
|
|
// apiClient automatically adds JWT header if available via interceptors
|
|
// Always suppress error toast - we handle 401 errors locally
|
|
console.debug("[AppConfig] Fetching app config", {
|
|
attempt,
|
|
force,
|
|
path: window.location.pathname,
|
|
});
|
|
const response = await apiClient.get<AppConfig>(
|
|
"/api/v1/config/app-config",
|
|
{
|
|
suppressErrorToast: true,
|
|
skipAuthRedirect: true,
|
|
} as any,
|
|
);
|
|
const data = response.data;
|
|
|
|
console.debug("[AppConfig] Config fetched successfully:", data);
|
|
console.debug(
|
|
"[AppConfig] Fetch duration ms:",
|
|
(performance.now() - startTime).toFixed(2),
|
|
);
|
|
setConfig(data);
|
|
setHasResolvedConfig(true);
|
|
setLoading(false);
|
|
onConfigLoadedRef.current?.(data);
|
|
return; // Success - exit function
|
|
} catch (err: any) {
|
|
const status = err?.response?.status;
|
|
|
|
// On 401 (not authenticated), use default config with login enabled
|
|
// This allows the app to work even without authentication
|
|
if (status === 401) {
|
|
console.debug(
|
|
"[AppConfig] 401 error - using default config (login enabled)",
|
|
);
|
|
console.debug(
|
|
"[AppConfig] Fetch duration ms:",
|
|
(performance.now() - startTime).toFixed(2),
|
|
);
|
|
setConfig({ enableLogin: true });
|
|
setHasResolvedConfig(true);
|
|
setLoading(false);
|
|
return;
|
|
}
|
|
|
|
// Check if we should retry (network errors or 5xx errors)
|
|
const shouldRetry =
|
|
(!status || status >= 500) && attempt < maxRetries;
|
|
|
|
if (shouldRetry) {
|
|
console.warn(
|
|
`[AppConfig] Attempt ${attempt + 1} failed (status ${status || "network error"}):`,
|
|
err.message,
|
|
"- will retry...",
|
|
);
|
|
continue;
|
|
}
|
|
|
|
// Final attempt failed or non-retryable error (4xx)
|
|
const errorMessage =
|
|
err?.response?.data?.message ||
|
|
err?.message ||
|
|
"Unknown error occurred";
|
|
setError(errorMessage);
|
|
console.error(
|
|
`[AppConfig] Failed to fetch app config after ${attempt + 1} attempts:`,
|
|
err,
|
|
);
|
|
console.debug(
|
|
"[AppConfig] Fetch duration ms:",
|
|
(performance.now() - startTime).toFixed(2),
|
|
);
|
|
// Preserve existing config (initial default or previous fetch). If nothing is set, assume login enabled.
|
|
setConfig((current) => current ?? { enableLogin: true });
|
|
setHasResolvedConfig(true);
|
|
break;
|
|
}
|
|
}
|
|
|
|
setLoading(false);
|
|
},
|
|
[hasResolvedConfig, isBlockingMode, maxRetries, initialDelay],
|
|
);
|
|
|
|
const { isAuthPage } = useJwtConfigSync(fetchConfig);
|
|
|
|
useEffect(() => {
|
|
if (isAuthPage) {
|
|
console.debug(
|
|
"[AppConfig] On auth page - using default config, skipping fetch",
|
|
{ path: window.location.pathname },
|
|
);
|
|
setConfig({ enableLogin: true });
|
|
setHasResolvedConfig(true);
|
|
setLoading(false);
|
|
return;
|
|
}
|
|
|
|
if (autoFetch) {
|
|
fetchConfig();
|
|
}
|
|
}, [autoFetch, fetchConfig, isAuthPage]);
|
|
|
|
const refetch = useCallback(() => fetchConfig(true), [fetchConfig]);
|
|
|
|
const value = useMemo<AppConfigContextValue>(
|
|
() => ({
|
|
config,
|
|
loading,
|
|
error,
|
|
refetch,
|
|
}),
|
|
[config, loading, error, refetch],
|
|
);
|
|
|
|
return (
|
|
<AppConfigContext.Provider value={value}>
|
|
{children}
|
|
</AppConfigContext.Provider>
|
|
);
|
|
};
|
|
|
|
/**
|
|
* Hook to access application configuration
|
|
* Must be used within AppConfigProvider
|
|
*/
|
|
export function useAppConfig(): AppConfigContextValue {
|
|
const context = useContext(AppConfigContext);
|
|
|
|
if (context === undefined) {
|
|
throw new Error("useAppConfig must be used within AppConfigProvider");
|
|
}
|
|
|
|
return context;
|
|
}
|