Fix: usage shifts #2345
Fix: usage shifts #2345
Conversation
ConsoleProject ID: Sites (2)
Note Appwrite has a Discord community with over 16 000 members. |
WalkthroughAdds UI and data changes across the project overview area. New public props: Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (11)
src/lib/charts/line.svelte (1)
17-29: Avoid mutating incoming series; return new objects to prevent side-effects.The current map mutates each series object. Prefer immutable mapping to keep props pure and avoid subtle bugs when the same series array is reused elsewhere.
Apply:
- series={series.map((s) => { - s.type = 'line'; - s.stack = 'total'; - s.lineStyle = { - shadowBlur: applyStyles ? 38 : undefined, - shadowColor: applyStyles ? Colors.Primary : undefined, - shadowOffsetY: applyStyles ? 15 : undefined, - shadowOffsetX: 0, - width: applyStyles ? undefined : 2 - }; - s.showSymbol = false; - - return s; - })} + series={series.map((s) => ({ + ...s, + type: 'line', + stack: s.stack ?? 'total', + lineStyle: { + ...s.lineStyle, + shadowBlur: applyStyles ? 38 : undefined, + shadowColor: applyStyles ? Colors.Primary : undefined, + shadowOffsetY: applyStyles ? 15 : undefined, + shadowOffsetX: 0, + width: applyStyles ? undefined : 2 + }, + showSymbol: false + }))}src/lib/components/card.svelte (1)
49-73: Ensure the parent container can actually stretch to 100% height.Setting
height="100%"onLayout.Stackonly works if the Card variant (Link/Button/Base) also participates in a height-constrained layout. If you see no effect, add a fallback inline style to the stack.Example tweak:
- <Layout.Stack gap="xl" height={fullHeightChild ? '100%' : undefined}> + <Layout.Stack + gap="xl" + height={fullHeightChild ? '100%' : undefined} + style={fullHeightChild ? 'min-height:100%' : undefined}>(Repeat for the other two branches.)
src/routes/(console)/project-[region]-[project]/overview/(components)/skeletons/extended.svelte (2)
30-47: Minor a11y: mark container busy and announce updates politely.- <div class="overlay-container"> + <div class="overlay-container" aria-busy={loading} aria-live="polite">
54-59: Provide a safe fallback forlhunit to reduce cross-browser layout jitter.- .overlay-container { - display: grid; - min-height: 1lh; + .overlay-container { + display: grid; + min-height: 1em; /* fallback */ + min-height: 1lh;src/routes/(console)/project-[region]-[project]/overview/(components)/skeletons/simple.svelte (2)
19-39: Minor a11y: mark busy and announce politely.- <div class="overlay-container"> + <div class="overlay-container" aria-busy={loading} aria-live="polite">
46-51: Addlhfallback to avoid layout differences across browsers.- .overlay-container { - display: grid; - min-height: 1lh; + .overlay-container { + display: grid; + min-height: 1em; /* fallback */ + min-height: 1lh;src/routes/(console)/project-[region]-[project]/overview/+layout.svelte (2)
37-39: Avoid double-invokinghandleon initial load.
onMount(handle)+afterNavigate(handle)can both fire on first render. Consider keeping onlyafterNavigate, or gate the first call.Example:
let first = true; afterNavigate(() => { if (first) { first = false; handle(); return; } handle(); });
98-98: Remove debug log.Stray console log in production.
- $: console.log(`$loadingProjectUsage: ${$loadingProjectUsage}`); + // Intentionally left blanksrc/routes/(console)/project-[region]-[project]/overview/store.ts (1)
54-57: Minor: avoid store.get for static columns
get(devKeyColumns)freezes the current value at module init. Since columns are static, just spread the literal to keep things simple.Apply this diff:
-export const keyColumns = readable<Column[]>([ - ...get(devKeyColumns), - { id: 'scopes', title: 'Scopes', type: 'string', width: { min: 120 } } -]); +const baseDevKeyColumns: Column[] = [ + { id: 'name', title: 'Name', type: 'string', width: { min: 120 } }, + { id: 'last_accessed', title: 'Last accessed', type: 'datetime', width: { min: 120 } }, + { id: 'expiration', title: 'Expiration date', type: 'datetime', width: { min: 120 } } +]; +export const devKeyColumns = readable<Column[]>(baseDevKeyColumns); +export const keyColumns = readable<Column[]>([ + ...baseDevKeyColumns, + { id: 'scopes', title: 'Scopes', type: 'string', width: { min: 120 } } +]);src/routes/(console)/project-[region]-[project]/overview/bandwidth.svelte (2)
46-47: Guard chart data when not loadingIf
networkis undefined,chartDatabecomesundefined. Safer to default to[]to avoid surprises in downstream consumers.Apply this diff:
- let chartData = $derived(loading ? fakeBarChartData : network?.map((e) => [e.date, e.value])); + let chartData = $derived( + loading ? fakeBarChartData : (network?.map((e) => [e.date, e.value]) ?? []) + );
57-71: Micro: avoid double compute in axis label formatter
humanFileSize(+value)is called twice. Cache once.Apply this diff:
- formatter: (value: number) => { - return loading - ? '-- MB' - : !value - ? '0' - : `${humanFileSize(+value).value} ${humanFileSize(+value).unit}`; - } + formatter: (value: number) => { + if (loading) return '-- MB'; + if (!value) return '0'; + const hf = humanFileSize(+value); + return `${hf.value} ${hf.unit}`; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (11)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlsrc/routes/(console)/project-[region]-[project]/overview/intro-dark.pngis excluded by!**/*.pngsrc/routes/(console)/project-[region]-[project]/overview/intro-light.pngis excluded by!**/*.pngsrc/routes/(console)/project-[region]-[project]/overview/onboard-1-dark-desktop.svgis excluded by!**/*.svgsrc/routes/(console)/project-[region]-[project]/overview/onboard-1-dark-mobile.svgis excluded by!**/*.svgsrc/routes/(console)/project-[region]-[project]/overview/onboard-1-light-desktop.svgis excluded by!**/*.svgsrc/routes/(console)/project-[region]-[project]/overview/onboard-1-light-mobile.svgis excluded by!**/*.svgsrc/routes/(console)/project-[region]-[project]/overview/onboard-2-dark-desktop.svgis excluded by!**/*.svgsrc/routes/(console)/project-[region]-[project]/overview/onboard-2-dark-mobile.svgis excluded by!**/*.svgsrc/routes/(console)/project-[region]-[project]/overview/onboard-2-light-desktop.svgis excluded by!**/*.svgsrc/routes/(console)/project-[region]-[project]/overview/onboard-2-light-mobile.svgis excluded by!**/*.svg
📒 Files selected for processing (10)
package.json(1 hunks)src/lib/charts/line.svelte(2 hunks)src/lib/components/card.svelte(3 hunks)src/lib/helpers/faker.ts(1 hunks)src/routes/(console)/project-[region]-[project]/overview/(components)/skeletons/extended.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/overview/(components)/skeletons/simple.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/overview/+layout.svelte(3 hunks)src/routes/(console)/project-[region]-[project]/overview/bandwidth.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/overview/requests.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/overview/store.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/routes/(console)/project-[region]-[project]/overview/store.ts (2)
src/lib/helpers/string.ts (1)
hash(77-84)src/lib/stores/sdk.ts (1)
sdk(142-165)
🔇 Additional comments (4)
package.json (1)
27-29: Lockfile consistency verified
Both@appwrite.io/pink-svelteand@appwrite.io/pink-icons-sveltein pnpm-lock.yaml resolve to commit 1489b16 with no lingering duplicate versions.src/lib/charts/line.svelte (1)
20-21: Confirm that stacking is intended for all line series.
stack = 'total'changes chart semantics; verify this is desired for every caller of .src/lib/components/card.svelte (1)
20-21: Prop addition LGTM; name reads well.
fullHeightChildis clear and defaulting to false avoids behavior changes.src/routes/(console)/project-[region]-[project]/overview/+layout.svelte (1)
111-121: Nice: passingloadingthrough to child charts aligns with the skeleton flow.This is consistent and reduces perceived layout shift during fetch.
src/routes/(console)/project-[region]-[project]/overview/+layout.svelte
Outdated
Show resolved
Hide resolved
src/routes/(console)/project-[region]-[project]/overview/bandwidth.svelte
Show resolved
Hide resolved
src/routes/(console)/project-[region]-[project]/overview/requests.svelte
Show resolved
Hide resolved
src/routes/(console)/project-[region]-[project]/overview/requests.svelte
Show resolved
Hide resolved
src/routes/(console)/project-[region]-[project]/overview/requests.svelte
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/routes/(console)/project-[region]-[project]/overview/store.ts (1)
19-60: Critical: Race conditions and stuck loading state remain unaddressed.The past review comment on lines 20-42 identified two critical issues that are still present:
Race condition: Concurrent loads can result in older responses overwriting newer data. Without tracking the in-flight request,
lastParamsHashcan be set out of order.Stuck loading on errors: If the API call throws,
loadingProjectUsageremainstrueindefinitely. Combined with theminTimedelay, this creates a poor UX where the UI is stuck in a loading state.Apply the diff from the previous review comment to add try/finally blocks and in-flight request tracking:
export const usage = cachedStore< Models.UsageProject, { load: (start: string, end: string, period: ProjectUsageRange) => Promise<void>; } >('projectUsage', function ({ set }) { const minTime = 1250; let lastParamsHash: string | null = null; + let inflightParamsHash: string | null = null; return { load: async (start, end, period) => { const currentData = get(usage); const currentParamsHash = hash([ page.params.project, page.params.region, start, end, period.toString() ]); // don't hit the API call if we have the data! if (lastParamsHash === currentParamsHash && currentData && !isDev) { loadingProjectUsage.set(false); return; } const initTime = Date.now(); loadingProjectUsage.set(true); + inflightParamsHash = currentParamsHash; - const usages = await sdk - .forProject(page.params.region, page.params.project) - .project.getUsage({ - startDate: start, - endDate: end, - period - }); - - const elapsed = Date.now() - initTime; - const remainingTime = minTime - elapsed; - - if (remainingTime >= 0) { - await sleep(remainingTime); - } - - set(usages); - lastParamsHash = currentParamsHash; - loadingProjectUsage.set(false); + try { + const usages = await sdk + .forProject(page.params.region, page.params.project) + .project.getUsage({ + startDate: start, + endDate: end, + period + }); + + // Ignore stale responses + if (inflightParamsHash !== currentParamsHash) return; + + const elapsed = Date.now() - initTime; + const remainingTime = minTime - elapsed; + + if (remainingTime >= 0) { + await sleep(remainingTime); + } + + set(usages); + lastParamsHash = currentParamsHash; + } catch (err) { + console.error('Failed to load project usage', err); + } finally { + // Only clear loading if this request is still the latest + if (inflightParamsHash === currentParamsHash) { + loadingProjectUsage.set(false); + inflightParamsHash = null; + } + } } }; });src/routes/(console)/project-[region]-[project]/overview/requests.svelte (1)
38-47: Runtime risk:requestscan be undefined.The derived
requestsvalue uses a type assertion but doesn't guarantee the value exists. At line 46,requests.map()will throw ifrequestsis undefined. While the author previously stated "not when not loading", there's no guarantee that$usage?.requestsis populated whenloadingbecomesfalse.Apply this diff to add defensive checks:
let requests = $derived( - $usage?.requests as unknown as Array<{ + ($usage?.requests ?? []) as Array<{ date: number; value: number; }> ); let chartData = $derived( - loading ? fakeLineChartData : [...requests.map((e) => [e.date, e.value])] + loading ? fakeLineChartData : requests.map((e) => [e.date, e.value]) );
🧹 Nitpick comments (1)
src/routes/(console)/project-[region]-[project]/overview/bandwidth.svelte (1)
38-47: Consider improving type safety and null handling.While line 47 uses optional chaining (
network?.map()), the type assertion pattern at lines 38-43 is similar to requests.svelte and could be more defensive. Consider explicitly defaulting to an empty array:let network = $derived( - $usage?.network as unknown as Array<{ + ($usage?.network ?? []) as Array<{ date: number; value: number; }> ); let bandwidth = $derived(humanFileSize(totalMetrics($usage?.network))); -let chartData = $derived(loading ? fakeBarChartData : network?.map((e) => [e.date, e.value])); +let chartData = $derived(loading ? fakeBarChartData : network.map((e) => [e.date, e.value]));This ensures
networkis always an array and eliminates the need for optional chaining.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/lib/helpers/faker.ts(1 hunks)src/routes/(console)/project-[region]-[project]/overview/bandwidth.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/overview/requests.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/overview/store.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/helpers/faker.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/routes/(console)/project-[region]-[project]/overview/store.ts (3)
src/lib/helpers/string.ts (1)
hash(77-84)src/lib/system.ts (1)
isDev(28-28)src/lib/stores/sdk.ts (1)
sdk(147-170)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: e2e
🔇 Additional comments (2)
src/routes/(console)/project-[region]-[project]/overview/requests.svelte (1)
85-100: LGTM! Dispatch issue resolved.The sequential
toggle(event); dispatch(...)pattern correctly ensures both functions execute. This addresses the previous review comment about the short-circuit issue.src/routes/(console)/project-[region]-[project]/overview/bandwidth.svelte (1)
84-106: LGTM! Dispatch issue resolved.The sequential
toggle(event); dispatch(...)pattern correctly addresses the previous short-circuit issue, ensuring both the menu toggle and period change dispatch execute as expected.
| $effect(() => { | ||
| console.log(totalMetrics($usage?.requests)); | ||
| }); |
There was a problem hiding this comment.
Remove debug logging from production code.
The console.log statement should be removed before merging to production.
Apply this diff:
- $effect(() => {
- console.log(totalMetrics($usage?.requests));
- });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $effect(() => { | |
| console.log(totalMetrics($usage?.requests)); | |
| }); |
🤖 Prompt for AI Agents
In src/routes/(console)/project-[region]-[project]/overview/requests.svelte
around lines 67 to 69, remove the debug console.log call inside the $effect
block; replace it with nothing or, if needed for production-safe telemetry, send
the value to a proper logger/analytics function instead of console.log, ensuring
no debug logs remain in production code.
What does this PR do?
Fixes the usage layout shift.
Test Plan
Manual.
Before -
Screen.Recording.2025-09-10.at.1.02.05.PM.mov
After -
shifts-fixed.mov
Related PRs and Issues
N/A.
Have you read the Contributing Guidelines on issues?
Yes.
Summary by CodeRabbit
New Features
Performance
UX Improvements