Fix: usage shifts #2345
Fix: usage shifts #2345
Conversation
ConsoleProject ID: Sites (2)
Note Cursor pagination performs better than offset pagination when loading further pages. |
WalkthroughThis PR updates pinned commits for @appwrite.io/pink-svelte and @appwrite.io/pink-icons-svelte. It adds applyStyles to Line chart, fullHeightChild to Card, and new faker helpers for bar/line chart data. Two new skeleton components (simple and extended) are introduced. The Overview layout adopts a loading state with skeleton rendering and uses a new loadingProjectUsage store. Bandwidth and Requests components switch to $props-based period/loading, derived stores, loading-aware chart options, and fake data during loading. The Overview store adds hashing to skip redundant fetches, a loading flag, a modal state, and column definitions for keys. Possibly related PRs
Suggested reviewers
Pre-merge checks (1 passed, 1 warning, 1 inconclusive)❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
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.
| export function generateFakeBarChartData(seed = 1) { | ||
| const fakeData: [number, number][] = []; | ||
| const now = Date.now(); | ||
| for (let i = 23; i >= 0; i--) { | ||
| const val = seededRandom(seed + i) * 1_000_000; | ||
| fakeData.push([now - i * 60 * 60 * 1000, Math.round(val)]); | ||
| } | ||
| return fakeData; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Stabilize timestamps to prevent loading-state chart reflows.
Using Date.now() on each render can shift x-axis and trigger re-layout. Accept a baseTimeMs parameter (defaulted once) so repeated calls during the same loading session produce consistent timestamps.
-export function generateFakeBarChartData(seed = 1) {
+export function generateFakeBarChartData(seed = 1, baseTimeMs = Date.now()) {
const fakeData: [number, number][] = [];
- const now = Date.now();
+ const now = baseTimeMs;
for (let i = 23; i >= 0; i--) {
const val = seededRandom(seed + i) * 1_000_000;
fakeData.push([now - i * 60 * 60 * 1000, Math.round(val)]);
}
return fakeData;
}📝 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.
| export function generateFakeBarChartData(seed = 1) { | |
| const fakeData: [number, number][] = []; | |
| const now = Date.now(); | |
| for (let i = 23; i >= 0; i--) { | |
| const val = seededRandom(seed + i) * 1_000_000; | |
| fakeData.push([now - i * 60 * 60 * 1000, Math.round(val)]); | |
| } | |
| return fakeData; | |
| } | |
| export function generateFakeBarChartData(seed = 1, baseTimeMs = Date.now()) { | |
| const fakeData: [number, number][] = []; | |
| const now = baseTimeMs; | |
| for (let i = 23; i >= 0; i--) { | |
| const val = seededRandom(seed + i) * 1_000_000; | |
| fakeData.push([now - i * 60 * 60 * 1000, Math.round(val)]); | |
| } | |
| return fakeData; | |
| } |
🤖 Prompt for AI Agents
In src/lib/helpers/faker.ts around lines 206 to 214, the function uses
Date.now() each call which causes shifting x-axis; change the function to accept
an optional baseTimeMs parameter and use a module-level constant
initialBaseTimeMs = Date.now() as the default so the default is evaluated once
at module load; update the signature to accept baseTimeMs?: number, use
baseTimeMs ?? initialBaseTimeMs instead of Date.now() inside the loop, and leave
the rest of the logic unchanged so repeated calls during the same loading
session produce stable timestamps.
| export function generateFakeLineChartData(seed = 2) { | ||
| const fakeData: [number, number][] = []; | ||
| const now = Date.now(); | ||
| let value = seededRandom(seed) * 5000 + 5000; | ||
| for (let i = 23; i >= 0; i--) { | ||
| value += (seededRandom(seed + i) - 0.5) * 1000; | ||
| value = Math.max(0, value); | ||
| fakeData.push([now - i * 60 * 60 * 1000, Math.round(value)]); | ||
| } | ||
| return fakeData; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same stabilization for line data.
-export function generateFakeLineChartData(seed = 2) {
+export function generateFakeLineChartData(seed = 2, baseTimeMs = Date.now()) {
const fakeData: [number, number][] = [];
- const now = Date.now();
+ const now = baseTimeMs;
let value = seededRandom(seed) * 5000 + 5000;
for (let i = 23; i >= 0; i--) {
value += (seededRandom(seed + i) - 0.5) * 1000;
value = Math.max(0, value);
fakeData.push([now - i * 60 * 60 * 1000, Math.round(value)]);
}
return fakeData;
}Follow-up: Pass a single baseTimeMs from the parent (e.g., layout/store) to all loading charts so they share the same axis during a loading cycle.
📝 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.
| export function generateFakeLineChartData(seed = 2) { | |
| const fakeData: [number, number][] = []; | |
| const now = Date.now(); | |
| let value = seededRandom(seed) * 5000 + 5000; | |
| for (let i = 23; i >= 0; i--) { | |
| value += (seededRandom(seed + i) - 0.5) * 1000; | |
| value = Math.max(0, value); | |
| fakeData.push([now - i * 60 * 60 * 1000, Math.round(value)]); | |
| } | |
| return fakeData; | |
| } | |
| export function generateFakeLineChartData(seed = 2, baseTimeMs = Date.now()) { | |
| const fakeData: [number, number][] = []; | |
| const now = baseTimeMs; | |
| let value = seededRandom(seed) * 5000 + 5000; | |
| for (let i = 23; i >= 0; i--) { | |
| value += (seededRandom(seed + i) - 0.5) * 1000; | |
| value = Math.max(0, value); | |
| fakeData.push([now - i * 60 * 60 * 1000, Math.round(value)]); | |
| } | |
| return fakeData; | |
| } |
🤖 Prompt for AI Agents
In src/lib/helpers/faker.ts around lines 216 to 226, generateFakeLineChartData
uses Date.now() internally which prevents multiple charts from sharing a single
synchronized axis; change the function signature to accept an optional
baseTimeMs (defaulting to Date.now()) and replace the local now = Date.now()
with now = baseTimeMs so callers can pass the same baseTimeMs to all charts
during a loading cycle; update all call sites (layout/store and any tests) to
pass the shared baseTimeMs from the parent so charts render on the same time
axis, and keep the seededRandom behavior unchanged.
| $loadingProjectUsage = true; | ||
| const promise = changePeriod(period); | ||
|
|
||
| if ($usage) { | ||
| await promise; | ||
| await promise.finally(() => { | ||
| $loadingProjectUsage = false; | ||
| }); | ||
| } |
There was a problem hiding this comment.
Loading flag may never reset; remove the $usage guard and always clear in finally.
As written, if $usage is falsy, $loadingProjectUsage stays true, causing a stuck loading UI.
- async function handle() {
- $loadingProjectUsage = true;
- const promise = changePeriod(period);
-
- if ($usage) {
- await promise.finally(() => {
- $loadingProjectUsage = false;
- });
- }
- }
+ async function handle() {
+ $loadingProjectUsage = true;
+ try {
+ await changePeriod(period);
+ } finally {
+ $loadingProjectUsage = false;
+ }
+ }📝 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.
| $loadingProjectUsage = true; | |
| const promise = changePeriod(period); | |
| if ($usage) { | |
| await promise; | |
| await promise.finally(() => { | |
| $loadingProjectUsage = false; | |
| }); | |
| } | |
| async function handle() { | |
| $loadingProjectUsage = true; | |
| try { | |
| await changePeriod(period); | |
| } finally { | |
| $loadingProjectUsage = false; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/routes/(console)/project-[region]-[project]/overview/+layout.svelte
around lines 41–48, the loading flag $loadingProjectUsage is only cleared inside
a conditional that checks $usage, which can leave it true indefinitely; remove
the if ($usage) guard and always await the promise.finally callback so
$loadingProjectUsage is set to false regardless of $usage (i.e., call await
promise.finally(() => { $loadingProjectUsage = false; }) unconditionally).
| <ActionMenu.Root width="fit-content" slot="tooltip" let:toggle> | ||
| <ActionMenu.Item.Button | ||
| on:click={(event) => toggle(event) && dispatch('change', '24h')}> | ||
| 24h | ||
| </ActionMenu.Item.Button> | ||
| <ActionMenu.Item.Button | ||
| on:click={(event) => toggle(event) && dispatch('change', '30d')}> | ||
| 30d | ||
| </ActionMenu.Item.Button> | ||
| <ActionMenu.Item.Button | ||
| on:click={(event) => toggle(event) && dispatch('change', '90d')}> | ||
| 90d | ||
| </ActionMenu.Item.Button> | ||
| </ActionMenu.Root> |
There was a problem hiding this comment.
Dispatch never fires due to short-circuit with void return
toggle(event) && dispatch(...) won’t call dispatch because toggle returns void (falsy). Call them sequentially.
Apply this diff:
- <ActionMenu.Root width="fit-content" slot="tooltip" let:toggle>
- <ActionMenu.Item.Button
- on:click={(event) => toggle(event) && dispatch('change', '24h')}>
+ <ActionMenu.Root width="fit-content" slot="tooltip" let:toggle>
+ <ActionMenu.Item.Button
+ on:click={(event) => { toggle(event); dispatch('change', '24h'); }}>
24h
</ActionMenu.Item.Button>
- <ActionMenu.Item.Button
- on:click={(event) => toggle(event) && dispatch('change', '30d')}>
+ <ActionMenu.Item.Button
+ on:click={(event) => { toggle(event); dispatch('change', '30d'); }}>
30d
</ActionMenu.Item.Button>
- <ActionMenu.Item.Button
- on:click={(event) => toggle(event) && dispatch('change', '90d')}>
+ <ActionMenu.Item.Button
+ on:click={(event) => { toggle(event); dispatch('change', '90d'); }}>
90d
</ActionMenu.Item.Button>
</ActionMenu.Root>🤖 Prompt for AI Agents
In src/routes/(console)/project-[region]-[project]/overview/bandwidth.svelte
around lines 84 to 97, the click handlers use short-circuiting like
toggle(event) && dispatch(...), but toggle returns void so dispatch never runs;
replace each handler with two sequential calls: first call toggle(event); then
call dispatch('change', '<period>') (ensuring you keep the event argument for
toggle and the same period string for dispatch).
| let { | ||
| period, | ||
| loading | ||
| }: { | ||
| period: UsagePeriods; | ||
| loading: boolean; | ||
| } = $props(); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
API change: component now reads period/loading via $props()
Ensure all call-sites provide loading and still pass period. Validate no consumers rely on the old export let period signature.
Run to check usages and the toggle short-circuit pattern across the repo:
Also applies to: 69-73, 94-113
🏁 Script executed:
#!/bin/bash
# Verify call sites pass `loading` to requests/bandwidth and no stale signatures remain.
rg -nP --type=svelte -C2 '<Requests\b'
rg -nP --type=svelte -C2 '<Bandwidth\b'
# Find any remaining short-circuit patterns that block dispatch.
rg -nP --type=svelte -n "toggle\\(event\\)\\s*&&\\s*dispatch\\("Length of output: 92
🏁 Script executed:
#!/bin/bash
# Search for <Requests and <Bandwidth usages in .svelte files
rg -nP -C2 '<Requests\b' -g '*.svelte'
rg -nP -C2 '<Bandwidth\b' -g '*.svelte'
# Check for any remaining export signatures
rg -n 'export let period' -g '*.svelte'
rg -n 'export let loading' -g '*.svelte'
# Find any short-circuit toggle patterns blocking dispatch
rg -nP -C2 "toggle\(event\)\s*&&\s*dispatch" -g '*.svelte'Length of output: 5929
Add loading prop to <Requests> and <Bandwidth> call-sites
In src/routes/(console)/project-[region]-[project]/overview/+layout.svelte (around lines 111 & 117), both <Requests> and <Bandwidth> now read loading via $props() but are only passed {period}. Update them to also pass {loading} alongside {period}. No remaining export let period signatures were found.
🤖 Prompt for AI Agents
In src/routes/(console)/project-[region]-[project]/overview/requests.svelte
around lines 26 to 33, the component destructures a loading prop from $props()
but the parent layout (+layout.svelte around lines 111 & 117) only passes
{period}; update the two call-sites for <Requests> and <Bandwidth> to pass
{loading} as well (i.e., change the props object to include loading alongside
period) so the components receive the loading prop they read from $props().
| let chartData = $derived( | ||
| loading ? fakeLineChartData : [...requests.map((e) => [e.date, e.value])] | ||
| ); | ||
|
|
There was a problem hiding this comment.
Runtime risk: requests can be undefined
requests.map(...) will throw when $usage?.requests is undefined. Default to [].
Apply this diff:
- let chartData = $derived(
- loading ? fakeLineChartData : [...requests.map((e) => [e.date, e.value])]
- );
+ let chartData = $derived(
+ loading ? fakeLineChartData : Array.isArray(requests) ? requests.map((e) => [e.date, e.value]) : []
+ );📝 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.
| let chartData = $derived( | |
| loading ? fakeLineChartData : [...requests.map((e) => [e.date, e.value])] | |
| ); | |
| let chartData = $derived( | |
| loading | |
| ? fakeLineChartData | |
| : Array.isArray(requests) | |
| ? requests.map((e) => [e.date, e.value]) | |
| : [] | |
| ); |
🤖 Prompt for AI Agents
In src/routes/(console)/project-[region]-[project]/overview/requests.svelte
around lines 44 to 47, the expression uses requests.map(...) which will throw if
requests is undefined; change the source to default to an empty array before
mapping (e.g. use ($usage?.requests ?? []) or (requests ?? []) ) so the map
operates on an array and the derived chartData uses loading ? fakeLineChartData
: [...(requestsFallback).map(e => [e.date, e.value])]; ensure you only replace
the requests reference with the fallback exactly where map is called.
| <ActionMenu.Item.Button | ||
| on:click={(event) => toggle(event) && dispatch('change', '24h')} | ||
| >24h</ActionMenu.Item.Button> | ||
| <ActionMenu.Item.Button on:click={() => dispatch('change', '30d')} | ||
| <ActionMenu.Item.Button | ||
| on:click={(event) => toggle(event) && dispatch('change', '30d')} | ||
| >30d</ActionMenu.Item.Button> | ||
| <ActionMenu.Item.Button on:click={() => dispatch('change', '90d')} | ||
| <ActionMenu.Item.Button | ||
| on:click={(event) => toggle(event) && dispatch('change', '90d')} | ||
| >90d</ActionMenu.Item.Button> | ||
| </ActionMenu.Root> |
There was a problem hiding this comment.
Dispatch never fires due to short-circuit with void return
Same issue as in bandwidth: toggle(event) && dispatch(...) prevents dispatch.
Apply this diff:
- <ActionMenu.Root width="fit-content" slot="tooltip" let:toggle>
- <ActionMenu.Item.Button
- on:click={(event) => toggle(event) && dispatch('change', '24h')}
- >24h</ActionMenu.Item.Button>
- <ActionMenu.Item.Button
- on:click={(event) => toggle(event) && dispatch('change', '30d')}
- >30d</ActionMenu.Item.Button>
- <ActionMenu.Item.Button
- on:click={(event) => toggle(event) && dispatch('change', '90d')}
- >90d</ActionMenu.Item.Button>
+ <ActionMenu.Root width="fit-content" slot="tooltip" let:toggle>
+ <ActionMenu.Item.Button
+ on:click={(event) => { toggle(event); dispatch('change', '24h'); }}
+ >24h</ActionMenu.Item.Button>
+ <ActionMenu.Item.Button
+ on:click={(event) => { toggle(event); dispatch('change', '30d'); }}
+ >30d</ActionMenu.Item.Button>
+ <ActionMenu.Item.Button
+ on:click={(event) => { toggle(event); dispatch('change', '90d'); }}
+ >90d</ActionMenu.Item.Button>📝 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.
| <ActionMenu.Item.Button | |
| on:click={(event) => toggle(event) && dispatch('change', '24h')} | |
| >24h</ActionMenu.Item.Button> | |
| <ActionMenu.Item.Button on:click={() => dispatch('change', '30d')} | |
| <ActionMenu.Item.Button | |
| on:click={(event) => toggle(event) && dispatch('change', '30d')} | |
| >30d</ActionMenu.Item.Button> | |
| <ActionMenu.Item.Button on:click={() => dispatch('change', '90d')} | |
| <ActionMenu.Item.Button | |
| on:click={(event) => toggle(event) && dispatch('change', '90d')} | |
| >90d</ActionMenu.Item.Button> | |
| </ActionMenu.Root> | |
| <ActionMenu.Root width="fit-content" slot="tooltip" let:toggle> | |
| <ActionMenu.Item.Button | |
| on:click={(event) => { toggle(event); dispatch('change', '24h'); }} | |
| >24h</ActionMenu.Item.Button> | |
| <ActionMenu.Item.Button | |
| on:click={(event) => { toggle(event); dispatch('change', '30d'); }} | |
| >30d</ActionMenu.Item.Button> | |
| <ActionMenu.Item.Button | |
| on:click={(event) => { toggle(event); dispatch('change', '90d'); }} | |
| >90d</ActionMenu.Item.Button> | |
| </ActionMenu.Root> |
🤖 Prompt for AI Agents
In src/routes/(console)/project-[region]-[project]/overview/requests.svelte
around lines 81 to 90 the on:click handlers use short-circuiting (toggle(event)
&& dispatch(...)) which prevents dispatch from running because toggle returns
void; change each handler to call toggle(event) first then call dispatch
independently (e.g. call toggle(event); dispatch('change', '24h')) so dispatch
always fires after toggle.
|
|
||
| export const usage = cachedStore< | ||
| Models.UsageProject, | ||
| { | ||
| load: (start: string, end: string, period: ProjectUsageRange) => Promise<void>; | ||
| } | ||
| >('projectUsage', function ({ set }) { | ||
| let lastParamsHash: string | null = null; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent stale overwrites and stuck loading on errors
Concurrent loads can race and older responses can overwrite newer results; also, an exception leaves loadingProjectUsage stuck at true. Guard by tracking the in-flight hash and use try/finally.
Apply this diff:
- let lastParamsHash: string | null = null;
+ let lastParamsHash: string | null = null;
+ let inflightParamsHash: string | null = null;
@@
- loadingProjectUsage.set(true);
- const usages = await sdk
- .forProject(page.params.region, page.params.project)
- .project.getUsage({
- startDate: start,
- endDate: end,
- period
- });
-
- set(usages);
- lastParamsHash = currentParamsHash;
- loadingProjectUsage.set(false);
+ loadingProjectUsage.set(true);
+ inflightParamsHash = currentParamsHash;
+ 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;
+ 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;
+ }
+ }Also applies to: 30-42
🤖 Prompt for AI Agents
In src/routes/(console)/project-[region]-[project]/overview/store.ts around line
17 (and similarly for lines 30-42), the code uses lastParamsHash to detect stale
responses but does not guard against race conditions or ensure
loadingProjectUsage is reset on errors; update by introducing a local in-flight
hash variable set to the current params hash before starting the async load,
check that the in-flight hash still matches before applying results to avoid
older responses overwriting newer state, and wrap the async work in try/finally
to set loadingProjectUsage = false in the finally block so errors don't leave
loading stuck; also ensure lastParamsHash is updated only when the response is
applied.
| const currentData = get(usage); | ||
| const currentParamsHash = hash([page.params.project, start, end, period.toString()]); | ||
|
|
||
| // don't hit the API call if we have the data! | ||
| if (lastParamsHash === currentParamsHash && currentData) { | ||
| loadingProjectUsage.set(false); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Bug: cache key misses region — can return stale data when switching regions
currentParamsHash omits page.params.region, so identical project/start/end/period across different regions will hit the skip-path and reuse wrong data.
Apply this diff:
- const currentParamsHash = hash([page.params.project, start, end, period.toString()]);
+ const currentParamsHash = hash([
+ page.params.region,
+ page.params.project,
+ start,
+ end,
+ period.toString()
+ ]);📝 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.
| const currentData = get(usage); | |
| const currentParamsHash = hash([page.params.project, start, end, period.toString()]); | |
| // don't hit the API call if we have the data! | |
| if (lastParamsHash === currentParamsHash && currentData) { | |
| loadingProjectUsage.set(false); | |
| return; | |
| } | |
| const currentData = get(usage); | |
| const currentParamsHash = hash([ | |
| page.params.region, | |
| page.params.project, | |
| start, | |
| end, | |
| period.toString() | |
| ]); | |
| // don't hit the API call if we have the data! | |
| if (lastParamsHash === currentParamsHash && currentData) { | |
| loadingProjectUsage.set(false); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In src/routes/(console)/project-[region]-[project]/overview/store.ts around
lines 21 to 28, the cache key omits page.params.region which causes stale data
to be reused when switching regions; update the currentParamsHash computation to
include page.params.region (e.g., hash([page.params.region, page.params.project,
start, end, period.toString()])), ensure the same region-aware key is used
wherever lastParamsHash is set/compared so the skip-path only triggers for
identical region+project+time+period combinations, and keep the existing
loadingProjectUsage.set(false) behavior when skipping.
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