InfoTab: Add caching for namespace details#503
Conversation
skoeva
force-pushed
the
infotab
branch
2 times, most recently
from
510e14b to
fdc2582
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
plugins/aks-desktop/src/components/InfoTab/hooks/useInfoTab.ts:235
- The
namespaceDetails-> form pre-population effect runs on everynamespaceDetailschange and unconditionally callssetFormData/setBaselineFormData. With the new background revalidation behavior, a late-arriving refresh can overwrite in-progress user edits (and resethasChanges) while the user is typing. Guard this so background revalidation only updates the form when there are no unsaved changes (or only populate on the initial load), otherwise keep the user’s local edits and optionally signal that newer server data is available.
// Pre-populate form when namespace details are fetched
useEffect(() => {
if (!namespaceDetails) return;
const quota = namespaceDetails.properties?.defaultResourceQuota;
const policy = namespaceDetails.properties?.defaultNetworkPolicy;
const populated: FormData = {
...DEFAULT_FORM_DATA,
ingress: normalizePolicy(policy?.ingress ?? 'AllowSameNamespace'),
egress: normalizePolicy(policy?.egress ?? 'AllowAll'),
cpuRequest: parseMillicores(quota?.cpuRequest ?? '0m'),
cpuLimit: parseMillicores(quota?.cpuLimit ?? '0m'),
memoryRequest: parseMiB(quota?.memoryRequest ?? '0Mi'),
memoryLimit: parseMiB(quota?.memoryLimit ?? '0Mi'),
};
setFormData(populated);
setBaselineFormData(populated);
}, [namespaceDetails]);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tejhan
left a comment
tejhan
left a comment
There was a problem hiding this comment.
Changes LGTM, tests pass & was able to replicated new behavior.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Skip the initial mount — state is already initialised correctly from the cache. | ||
| useEffect(() => { | ||
| if (isFirstRenderRef.current) { | ||
| isFirstRenderRef.current = false; | ||
| return; | ||
| } | ||
| setNamespaceDetails(null); | ||
| setFormData(DEFAULT_FORM_DATA); | ||
| setBaselineFormData(null); | ||
| hasUnsavedChangesRef.current = false; | ||
| setError(null); | ||
| setLoading(!!clusterName); | ||
| setRevalidating(false); | ||
| }, [clusterName, projectId]); |
There was a problem hiding this comment.
The “project identity reset” effect only keys off [clusterName, projectId], but the cache key includes subscription and resourceGroup. If those labels change without clusterName/projectId changing, the hook can temporarily keep showing the previous project’s namespaceDetails while revalidating the new cacheKey. Consider basing the reset on cacheKey (or including subscription/resourceGroup) and, on reset, seeding namespaceDetails/loading from detailsCache.get(nextCacheKey) so cached data can be shown immediately on project switches (consistent with the SWR intent).
| // Skip the initial mount — state is already initialised correctly from the cache. | |
| useEffect(() => { | |
| if (isFirstRenderRef.current) { | |
| isFirstRenderRef.current = false; | |
| return; | |
| } | |
| setNamespaceDetails(null); | |
| setFormData(DEFAULT_FORM_DATA); | |
| setBaselineFormData(null); | |
| hasUnsavedChangesRef.current = false; | |
| setError(null); | |
| setLoading(!!clusterName); | |
| setRevalidating(false); | |
| }, [clusterName, projectId]); | |
| // Use the same identity as the cache/fetch path, and hydrate from cache | |
| // immediately when available so project switches keep SWR behaviour. | |
| // Skip the initial mount — state is already initialised correctly from the cache. | |
| useEffect(() => { | |
| if (isFirstRenderRef.current) { | |
| isFirstRenderRef.current = false; | |
| return; | |
| } | |
| const cachedDetails = cacheKey ? detailsCache.get(cacheKey) ?? null : null; | |
| setNamespaceDetails(cachedDetails); | |
| setFormData(DEFAULT_FORM_DATA); | |
| setBaselineFormData(null); | |
| hasUnsavedChangesRef.current = false; | |
| setError(null); | |
| setLoading(!!cacheKey && !cachedDetails); | |
| setRevalidating(!!cacheKey && !!cachedDetails); | |
| }, [cacheKey, clusterName, projectId]); |
| <Tooltip title={t('Refreshing data in background')}> | ||
| <CircularProgress size={16} /> | ||
| </Tooltip> | ||
| )} | ||
| <Button | ||
| variant="outlined" | ||
| onClick={handleRefresh} | ||
| disabled={revalidating || updating} |
There was a problem hiding this comment.
The background revalidation spinner is rendered as a standalone CircularProgress with only a hover Tooltip. This isn’t reliably announced to screen readers and doesn’t convey why the Refresh button becomes disabled. Consider marking the spinner aria-hidden and adding an accessible busy/status signal (e.g., aria-busy={revalidating} on the Refresh button and/or a visually-hidden role="status" live region with the same text).
| <Tooltip title={t('Refreshing data in background')}> | |
| <CircularProgress size={16} /> | |
| </Tooltip> | |
| )} | |
| <Button | |
| variant="outlined" | |
| onClick={handleRefresh} | |
| disabled={revalidating || updating} | |
| <> | |
| <Box | |
| id="info-tab-refresh-status" | |
| role="status" | |
| aria-live="polite" | |
| sx={{ | |
| position: 'absolute', | |
| width: 1, | |
| height: 1, | |
| p: 0, | |
| m: -1, | |
| overflow: 'hidden', | |
| clip: 'rect(0 0 0 0)', | |
| whiteSpace: 'nowrap', | |
| border: 0, | |
| }} | |
| > | |
| {t('Refreshing data in background')} | |
| </Box> | |
| <Tooltip title={t('Refreshing data in background')}> | |
| <CircularProgress size={16} aria-hidden="true" /> | |
| </Tooltip> | |
| </> | |
| )} | |
| <Button | |
| variant="outlined" | |
| onClick={handleRefresh} | |
| disabled={revalidating || updating} | |
| aria-busy={revalidating} | |
| aria-describedby={revalidating ? 'info-tab-refresh-status' : undefined} |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
Localize/locales/en/plugin-translation.json:175
"Cores"was removed from the localization source, but it is still referenced in the plugin UI (e.g.plugins/aks-desktop/src/components/Metrics/components/MetricsChartsGrid.tsxusest('Cores')). Please restore this key (or move it to the correct localization source) to avoid missing translations.
"Copilot agent": "Copilot agent",
"Copilot agent reviewed your repo and generated deployment PR": "Copilot agent reviewed your repo and generated deployment PR",
"Copy": "Copy",
"Copy to clipboard": "Copy to clipboard",
"CPU": "CPU",
"CPU limit": "CPU limit",
"CPU Limits": "CPU Limits",
"CPU request": "CPU request",
"CPU Requests": "CPU Requests",
"CPU Resources": "CPU Resources",
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Evangelos Skopelitis <eskopelitis@microsoft.com>
illume
left a comment
illume
left a comment
There was a problem hiding this comment.
There's some git conflicts now. Can you please have a look?
(Also some review comment(s) left open)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| > | ||
| {revalidating && ( | ||
| <Tooltip title={t('Refreshing data in background')}> | ||
| <CircularProgress size={16} /> |
There was a problem hiding this comment.
The background refresh indicator is a bare CircularProgress wrapped in a Tooltip. Tooltips on non-focusable elements aren’t keyboard-accessible, and CircularProgress may be exposed to screen readers as an unlabeled progressbar. Consider either marking it aria-hidden (if decorative) and adding separate accessible text, or give it an aria-label/aria-describedby so assistive tech can understand what’s happening.
| <CircularProgress size={16} /> | |
| <CircularProgress | |
| size={16} | |
| aria-label={t('Refreshing data in background')} | |
| /> |
|
|
||
| await waitFor(() => expect(result.current.revalidating).toBe(false)); | ||
| }); | ||
|
|
There was a problem hiding this comment.
There’s good coverage for cache-hit and revalidation, but no test covers the case where Namespace.useGet initially returns no labels (so cacheKey is null) and then later provides subscription/resourceGroup. That’s the scenario where state hydration from the module cache can be missed. Consider adding a test that starts with missing labels + a pre-seeded cache, then updates the mock to return labels and asserts cached data is shown immediately (no loading spinner) and the fetch runs as revalidating.
| test('hydrates from cache when labels become available after initial render and revalidates in background', async () => { | |
| const cached = createNamespaceDetails({ ingress: 'AllowAll' }); | |
| setDetailsCacheForTests('sub-123/rg-prod/my-cluster/my-project', cached); | |
| let resolveFetch: (value: ReturnType<typeof createNamespaceDetails>) => void; | |
| const fetchPromise = new Promise<ReturnType<typeof createNamespaceDetails>>(resolve => { | |
| resolveFetch = resolve; | |
| }); | |
| mockGetManagedNamespaceDetails.mockReturnValue(fetchPromise); | |
| mockUseGet | |
| .mockReturnValueOnce({ | |
| jsonData: { | |
| metadata: { | |
| labels: {}, | |
| }, | |
| }, | |
| }) | |
| .mockReturnValue({ | |
| jsonData: { | |
| metadata: { | |
| labels: { | |
| subscription: 'sub-123', | |
| resourceGroup: 'rg-prod', | |
| }, | |
| }, | |
| }, | |
| }); | |
| const { result, rerender } = renderHook(() => useInfoTab(defaultProject)); | |
| expect(result.current.namespaceDetails).toBeNull(); | |
| expect(result.current.loading).toBe(false); | |
| expect(result.current.revalidating).toBe(false); | |
| act(() => { | |
| rerender(); | |
| }); | |
| expect(result.current.namespaceDetails).toEqual(cached); | |
| expect(result.current.loading).toBe(false); | |
| expect(result.current.revalidating).toBe(true); | |
| resolveFetch!(cached); | |
| await waitFor(() => expect(result.current.revalidating).toBe(false)); | |
| }); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -262,6 +318,7 @@ export const useInfoTab = (project: { | |||
| clusterName, | |||
| resourceGroup, | |||
| namespaceName: projectId, | |||
| subscriptionId: subscription, | |||
| ingressPolicy: formData.ingress, | |||
There was a problem hiding this comment.
handleSave does not guard against a missing subscription, but passes subscriptionId: subscription to updateManagedNamespace. If subscription is undefined (e.g., labels still resolving), this can trigger an invalid CLI call. Consider including subscription (or cacheKey) in the early-return guard so the update cannot run without a fully-qualified target.
| const cacheKey = | ||
| clusterName && projectId && resourceGroup && subscription | ||
| ? `${subscription}/${resourceGroup}/${clusterName}/${projectId}` | ||
| : null; | ||
| const cached = cacheKey ? detailsCache.get(cacheKey) ?? null : null; | ||
|
|
||
| const [loading, setLoading] = useState(cacheKey !== null && cached === null); | ||
| const [revalidating, setRevalidating] = useState(false); | ||
| const [error, setError] = useState<string | null>(null); | ||
| const [namespaceDetails, setNamespaceDetails] = useState<NamespaceDetails | null>(null); | ||
| const [namespaceDetails, setNamespaceDetails] = useState<NamespaceDetails | null>(cached); |
There was a problem hiding this comment.
The initial state only hydrates from cache on the first render. If subscription/resourceGroup arrive asynchronously (via namespaceInstance), cacheKey is initially null, so cached details for that project won’t be shown immediately once labels resolve; the hook will instead do a cold fetch and show loading. To preserve the intended “instant subsequent opens”, consider adding a small effect that, when cacheKey becomes available and namespaceDetails is still null, re-checks detailsCache and hydrates state before starting a blocking load.
| isMounted = false; | ||
| }; | ||
| }, [clusterName, projectId, subscription, resourceGroup]); | ||
| }, [clusterName, projectId, subscription, resourceGroup, refreshTick]); |
There was a problem hiding this comment.
This effect uses t(...) from useTranslation() but t is not included in the dependency array. This can cause stale translations if the locale changes while the component is mounted and may violate react-hooks/exhaustive-deps if enabled. Consider adding t (and optionally cacheKey for readability) to the dependency list.
| }, [clusterName, projectId, subscription, resourceGroup, refreshTick]); | |
| }, [cacheKey, clusterName, projectId, resourceGroup, refreshTick, subscription, t]); |
| "Failed to fetch managed namespaces": "未能提取托管命名空间", | ||
| "Failed to fetch managed namespace details": "未能提取托管命名空间详细信息", | ||
| "Failed to update managed namespace": "未能更新托管命名空间", | ||
| "Refreshing data in background": "", |
There was a problem hiding this comment.
Several locales add new keys with empty-string values (e.g., this one). i18n frameworks commonly treat an empty string as a valid translation, which results in blank UI (here: an empty tooltip). Prefer omitting the key to allow fallback to en, or provide an actual translation value.
| "Refreshing data in background": "", |
| "Failed to fetch metrics from Prometheus": "Failed to fetch metrics from Prometheus", | ||
| "Metrics": "Metrics", | ||
| "Loading metrics from Prometheus": "Loading metrics from Prometheus", | ||
| "Metrics refreshed every 30 seconds": "Metrics refreshed every 30 seconds", |
There was a problem hiding this comment.
The PR description is focused on InfoTab caching/refresh, but this PR also adds/reshuffles multiple metrics/Prometheus translation keys across many locales. If these strings are unrelated to the InfoTab change, consider splitting them into a separate PR to keep scope tight and reduce review/merge conflicts in locale files.
The InfoTab was making two sequential Azure CLI calls on every open, causing ~1 minute load times. This change replaces that with a single fetch backed by a module-level cache so subsequent opens are instant.
Fixes: #280
Summary
detailsCache(stale-while-revalidate): cached data is shown immediately, a background fetch keeps it fresh. Cache key includessubscription/resourceGroup/clusterName/projectIdto prevent cross-subscription collisionshandleRefreshtriggers a background revalidation without clearing the cache, keeping the form visiblerevalidatingstate for background fetch indicationInfoTab.tsxto show a background spinner and a Refresh buttongetManagedNamespacesmock/cases; added coverage for cache hit, background revalidation, error suppression with existing data, cache invalidation on save, and post-save refresh running as revalidation; replaced directdetailsCacheaccess with exported test helpersTesting
cd plugins/aks-desktop && npm testand ensure the tests pass