Aligns all the database and nested routes #2330
Conversation
ConsoleProject ID: Sites (2)
Note Cursor pagination performs better than offset pagination when loading further pages. |
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/layout/emptySheet.svelte (1)
60-76: Clamp overlay height to avoid negatives; compute collapsed offset in JS.If the header’s bottom exceeds viewport height,
viewportHeight - headerRect.bottomcan go negative, yielding invalid heights. Also, computing the 89px offset in JS avoids the extra CSScalc().- if (headerElement) { - const headerRect = headerElement.getBoundingClientRect(); - const viewportHeight = window.innerHeight; - const dynamicHeight = viewportHeight - headerRect.bottom; - - dynamicOverlayHeight = `${dynamicHeight}px`; - if (!$expandTabs) { - dynamicOverlayHeight = `calc(${dynamicHeight}px - 89px)`; - } - } + if (headerElement) { + const headerRect = headerElement.getBoundingClientRect(); + const viewportHeight = window.innerHeight; + const collapsedOffset = $expandTabs ? 0 : 89; + const dynamicHeight = Math.max(0, viewportHeight - headerRect.bottom - collapsedOffset); + dynamicOverlayHeight = `${dynamicHeight}px`; + }src/routes/(console)/project-[region]-[project]/databases/database-[database]/backups/+page.svelte (1)
141-147: Potential XSS: isHtml with user-controlled label
isHtml: trueand embeddingtotalPolicies[0].labelrisks injection. Prefer plain text or sanitize.Suggested fix:
- // TODO: html isn't yet supported on Toast. - addNotification({ - isHtml: true, - type: 'success', - message - }); + addNotification({ + type: 'success', + message: totalPolicies.length > 1 + ? 'Backup policies have been created' + : `${totalPolicies[0].label} policy has been created` + });
🧹 Nitpick comments (23)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/layout/emptySheet.svelte (5)
352-358: Avoid overlapping breakpoints at 1024px.
max-width: 1024pxandmin-width: 1024pxboth match at exactly 1024px. Prefer a 1023/1024 split to prevent double application.Please verify at exactly 1024px that the intended rule wins.
- @media (max-width: 1024px) { + @media (max-width: 1023px) { height: var(--dynamic-overlay-height, 63.35vh); } @media (min-width: 1024px) { height: var(--dynamic-overlay-height, 70.35vh); }
173-174: Debounce window resize handler to reduce layout thrash.You already debounce ResizeObserver callbacks; mirror that for window resizes.
-<svelte:window on:resize={updateOverlayHeight} /> +<svelte:window on:resize={debouncedUpdateOverlayHeight} />
93-97: Optionally cancel pending debounce on destroy.If your
debouncehelper exposescancel, call it to avoid a late post-destroy callback.onDestroy(() => { if (resizeObserver) { resizeObserver.disconnect(); } + // If supported by the debounce helper + debouncedUpdateOverlayHeight.cancel?.(); });
193-205: A11y: add keyboard activation for the header “button”.Provide Enter/Space support to complement click. Then remove the Svelte a11y ignore.
- <!-- svelte-ignore a11y_click_events_have_key_events --> <div role="button" tabindex="0" style:cursor={columnActionsById ? 'pointer' : null} - onclick={() => { + onkeydown={(e) => { + if (columnActionsById && mode === 'rows' && (e.key === 'Enter' || e.key === ' ')) { + e.preventDefault(); + $showCreateColumnSheet.show = true; + $showCreateColumnSheet.title = 'Create column'; + $showCreateColumnSheet.columns = $tableColumns; + $showCreateColumnSheet.columnsOrder = $columnsOrder; + } + }} + onclick={() => { if (columnActionsById && mode === 'rows') { $showCreateColumnSheet.show = true; $showCreateColumnSheet.title = 'Create column'; $showCreateColumnSheet.columns = $tableColumns; $showCreateColumnSheet.columnsOrder = $columnsOrder; } }}>
380-402: Responsive offsets rely on magic percentages; consider transform-based centering.The series of
left: 47.5%/50%/45%tweaks is brittle across container changes. Preferleft: 50%; transform: translateX(-50%);with breakpoint-specific bottom only, or use container queries to anchor relative to the grid.Confirm these offsets align with the new container/cover layout at widths: 1024, 1280, 1440, 1728.
src/lib/layout/footer.svelte (1)
193-195: Scope the spreadsheet footer offset to desktop and drop!importantto avoid collisionsUnconditionally shifting the footer by 2rem (and with
!important) can misalign it on small viewports and fight the sub-navigation rule. Align this with shell’s 1024px breakpoint and the console-container offset.-:global(main:has(.databases-spreadsheet)) footer { - margin-inline-start: 2rem !important; -} +@media (min-width: 1024px) { + :global(main:has(.databases-spreadsheet)) footer { + margin-inline-start: 45px; /* keep in sync with shell.svelte console-container padding */ + } +}Please sanity-check alignment on:
- Database root, table list, table settings (≥1024px and <1024px).
src/lib/components/sidebar.svelte (1)
382-388: Small-screen nav offset respects banners — good additionThe
--banner-spacingadjustments prevent overlap under alerts. Consider mirroring a similar guard for the sub-navigation’s internalnavif it ever renders one on small screens.src/lib/layout/cover.svelte (1)
83-87: Avoid hard-coded 190px; use a CSS var so sidebar width changes don’t ripple.This width is repeated across the app. Make it tokenized.
Proposed minimal change with safe fallbacks:
.top-cover-console { - margin-left: -190px; - padding-left: 190px; + margin-left: calc(0px - var(--console-sidebar-width, 190px)); + padding-left: var(--console-sidebar-width, 190px); @@ - &.adjustForSpreadsheet { + &.adjustForSpreadsheet { @media (min-width: 1024px) { - padding-left: calc(190px + 3rem); + padding-left: calc(var(--console-sidebar-width, 190px) + 3rem); } }Also applies to: 61-68
src/routes/(console)/project-[region]-[project]/databases/database-[database]/header.svelte (1)
44-47: Inline -2.5rem offset may no longer be necessary (or may misalign).Given the new Cover defaults, consider dropping or scoping this negative margin to only the spreadsheet-adjusted contexts.
- <CoverTitle - style="margin-inline-start: -2.5rem;" + <CoverTitle + style="" href={`${base}/project-${page.params.region}-${projectId}/databases`}>src/lib/layout/animatedTitle.svelte (2)
30-39: Back button: hide semantics for a11y.When hidden, also mark the wrapper
aria-hiddento keep it out of the a11y tree;visibility: hiddenhelps, but this is clearer.- {#if href} - <span style:position="relative"> + {#if href} + <span style:position="relative" aria-hidden={$isTabletViewport}> <Button.Anchor {href} icon variant="text" size={buttonSize} aria-label="page back" disabled={$isTabletViewport} style={$isTabletViewport ? 'visibility: hidden' : ''}>
57-61: Remove unused transitions.
line-heightandletter-spacingare no longer dynamic; trimming them reduces needless style work.- transition: - font-size 300ms cubic-bezier(0.4, 0, 0.2, 1), - line-height 300ms cubic-bezier(0.4, 0, 0.2, 1), - letter-spacing 300ms cubic-bezier(0.4, 0, 0.2, 1); + transition: + font-size 300ms cubic-bezier(0.4, 0, 0.2, 1);Also applies to: 66-66
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/header.svelte (4)
77-83: Gate spreadsheet adjustments to spreadsheet routes; also avoid hard-coded heightsAlways enabling adjustForSpreadsheet risks misalignment on non-sheet pages. Bind it to the computed route type and prefer tokens/CSS vars for blocksize.
Apply:
-<Cover - adjustForSpreadsheet +<Cover + adjustForSpreadsheet={!nonSheetPages} expanded animate collapsed={!$expandTabs} - blocksize={$expandTabs ? '152px' : '90px'}> + blocksize={$expandTabs ? 'var(--db-header-expanded, 152px)' : 'var(--db-header-collapsed, 90px)'}>
84-86: Remove inline negative margin on titleInline styles are harder to theme/override. Use a class.
- <AnimatedTitle href={link} collapsed={!$expandTabs} style="margin-inline-start: -2.5rem;"> + <AnimatedTitle href={link} collapsed={!$expandTabs} class="title-shift"> {$table?.name} </AnimatedTitle>Add CSS (see style block):
.title-shift { margin-inline-start: -2.5rem; }
88-92: Render Id only when available to avoid undefined propsPrevents passing undefined as value and avoids unnecessary remounts when the table is not yet loaded.
-{#key $table?.$id} - <Id value={$table?.$id} tooltipPlacement={$expandTabs ? undefined : 'right'} - >{$table?.$id}</Id> -{/key} +{#if $table?.$id} + {#key $table.$id} + <Id value={$table.$id} tooltipPlacement={$expandTabs ? undefined : 'right'}> + {$table.$id} + </Id> + {/key} +{/if}
94-105: Key the tab list and improve a11y when collapsedKeyed each reduces DOM churn; aria-hidden prevents hidden tabs from being announced when collapsed.
- <div class="tabs-container" class:collapsed={!$expandTabs}> + <div class="tabs-container" class:collapsed={!$expandTabs} aria-hidden={!$expandTabs}> <Tabs> - {#each tabs as tab} + {#each tabs as tab (tab.href)} <Tab href={tab.href} selected={isTabSelected(tab, page.url.pathname, path, tabs)} event={tab.event}> {tab.title} </Tab> {/each} </Tabs> </div>src/lib/layout/container.svelte (1)
93-101: databasesMainScreen rules: OK; consider future-proofing constantsSpecificity beats the later generic breakpoints, so behavior is stable. If possible, move these rules after the generic ones or extract 1rem/1000px to design tokens to reduce drift.
Please sanity-check widths at 1024/1440 breakpoints against design specs.
src/routes/(console)/project-[region]-[project]/databases/database-[database]/+page.svelte (1)
21-94: Container no longer marked as main screenConfirm this page doesn’t rely on the main-screen gutters used elsewhere (e.g., spreadsheet views). If visual diffs show narrower content or misaligned search bar, add the flag back.
Optional revert:
-<Container> +<Container databasesMainScreen>src/routes/(console)/project-[region]-[project]/databases/database-[database]/settings/+page.svelte (1)
64-132: Settings container: verify consistency with other database pagesIf this screen should visually align with database “main” pages, you may still want databasesMainScreen for consistent gutters.
Optional:
- <Container> + <Container databasesMainScreen>src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/usage/[[period]]/+page.svelte (2)
9-11: Confirmdata.rowstype forcountIf
data.rowsis an array, pass its length; if it’s a number, current code is fine.If it’s an array:
-$: count = data.rows; +$: count = data.rows?.length ?? 0;
13-24: Import consistency (optional)Other pages import
Containerfrom$lib/layout. Consider standardizing to reduce import surface churn.Example:
-import Container from '$lib/layout/container.svelte'; +import { Container } from '$lib/layout';src/lib/layout/activity.svelte (3)
14-16: Public prop rename + new expanded: verify call-sites and consider a compat shimThis is a breaking API change. Please confirm all consumers switched from
databasesScreentodatabasesMainScreen, and thatexpandedis passed intentionally only where needed. For a soft rollout, accept the old prop and coerce it.Apply within this range:
-export let databasesMainScreen = false; -export let expanded = false; +export let databasesMainScreen: boolean = false; +// Back-compat: remove after downstreams migrate +export let databasesScreen?: boolean; +export let expanded: boolean = false;Outside this range (top-level script), add:
// reactive effective flag $: effectiveDatabasesMainScreen = databasesMainScreen || !!databasesScreen;And update Container usage (see Line 27 comment).
27-27: Container prop pass: align with new API and explicit expanded semanticsLooks good. Please ensure
Containeractually exposesdatabasesMainScreenandexpanded(post-API cleanup), and that suppressing expansion inside side sheet is desired UX.Apply:
-<Container {insideSideSheet} {databasesMainScreen} expanded={expanded && !insideSideSheet}> +<Container {insideSideSheet} databasesMainScreen={effectiveDatabasesMainScreen} expanded={expanded && !insideSideSheet}>(Optional) Precompute for readability:
$: isExpanded = expanded && !insideSideSheet;then:
<Container {insideSideSheet} databasesMainScreen={effectiveDatabasesMainScreen} {isExpanded} />
18-24: Make columns reactive to insideSideSheet changesIf
insideSideSheetcan toggle after mount,columnswon’t recompute. Make it reactive.-const columns: PinkColumn[] = [ +let columns: PinkColumn[]; +$: columns = [ { id: 'user', ...(insideSideSheet ? { width: 140 } : {}) }, { id: 'event', ...(insideSideSheet ? { width: 125 } : {}) }, { id: 'location', ...(insideSideSheet ? { width: 100 } : {}) }, { id: 'ip', ...(insideSideSheet ? { width: { min: 150 } } : {}) }, { id: 'date', ...(insideSideSheet ? { width: { min: 200 } } : {}) } -]; +];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (18)
src/lib/components/sidebar.svelte(3 hunks)src/lib/layout/activity.svelte(2 hunks)src/lib/layout/animatedTitle.svelte(4 hunks)src/lib/layout/container.svelte(2 hunks)src/lib/layout/cover.svelte(5 hunks)src/lib/layout/footer.svelte(1 hunks)src/lib/layout/shell.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/+page.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/backups/+page.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/header.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/settings/+page.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/subNavigation.svelte(2 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/activity/+page.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/header.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/layout/emptySheet.svelte(8 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/settings/+page.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/usage/[[period]]/+page.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/usage/[[period]]/+page.svelte(1 hunks)
🔇 Additional comments (17)
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/layout/emptySheet.svelte (1)
56-57: Ready-gating and resize observers look good.Nice pattern: compute on mount, observe container resizes, gate transition with a rAF-toggled
data-ready.Also applies to: 248-251, 82-91, 99-103
src/routes/(console)/project-[region]-[project]/databases/database-[database]/subNavigation.svelte (1)
177-177: Navbar now shows only on “table” routes — confirm this UX is intended for nested table subpagesWith
includes('table-[table]'), Navbar will render on base table and nested routes (e.g., columns, indexes, settings). If this should be limited to the base table screen, switch toendsWith('table-[table]').src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/settings/+page.svelte (1)
12-13: Container API migration looks good; verify layout hooks are wiredUsing the
.databases-spreadsheetwrapper plus<Container databasesMainScreen>aligns with the new layout scheme. Please verify header/footer alignment given the new global rules keyed off.databases-spreadsheet.src/lib/layout/shell.svelte (1)
308-311: Global.console-containerusage verifiedThe
.console-containerclass is introduced incontainer.svelteand is present in the template (+layout.svelte), auth (+layout.svelte), and console pages (overview/onboard, activity), so the left-padding offset will apply as intended.src/lib/components/sidebar.svelte (2)
106-107: Desktop-only transition suppression is correctGating
no-transitionson!$isTabletViewportavoids janky mobile behavior. Looks good.
370-370: Apply the same desktop-only transition suppression to sub-navigationConsistent with the main sidebar; good.
src/lib/layout/cover.svelte (3)
10-10: New prop and class wiring look correct.The
adjustForSpreadsheetprop is exported and correctly bound to both wrappers. No concerns on reactivity or class toggling.Also applies to: 37-38, 48-49
55-57: Slot rendering: good change.Dropping the extra wrapper around the default slot simplifies DOM and styles.
130-134: KeepdatabasesMainScreen– it’s exported and used in cover.svelte, container.svelte, and activity.svelte, and passed by multiple database routes for styling.src/routes/(console)/project-[region]-[project]/databases/database-[database]/header.svelte (1)
41-41: Switch to plain looks fine but re-check wide layouts.Removing
databasesMainScreenchanges container max-width at ≥1024px. Please sanity-check header alignment vs. content on 1024–1440px.src/lib/layout/animatedTitle.svelte (1)
2-2: Viewport switch and optional children: LGTM.Importing
isTabletViewportand makingchildrenoptional are correct and type-safe.Also applies to: 16-17
src/lib/layout/container.svelte (1)
39-43: Static .console-container class looks goodThis stabilizes selector targeting and still composes with restProps.class via Svelte’s merge semantics.
src/routes/(console)/project-[region]-[project]/databases/database-[database]/usage/[[period]]/+page.svelte (1)
13-13: Verify ContainerdatabasesMainScreenprop removal
File: src/routes/(console)/project-[region]-[project]/databases/database-[database]/usage/[[period]]/+page.svelte (around line 13)
Dropping thedatabasesMainScreenprop changes the Container’s width on this usage page versus other database screens. Confirm this is intentional and aligns with the new “main screen” variant.src/routes/(console)/project-[region]-[project]/databases/database-[database]/backups/+page.svelte (1)
180-241: Dropping databasesMainScreen: verify spacing/alignment on smaller viewportsWithout the databasesMainScreen variant, gutters and top spacing may differ from other database pages. Please sanity-check header alignment and horizontal scroll behavior at 360–768px.
If you find regressions, consider:
-<Container size="xxl"> +<Container size="xxl" databasesMainScreen>src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/usage/[[period]]/+page.svelte (1)
13-24: Good move to databasesMainScreen and new wrapperThis aligns with the updated layout API and footer selector targeting
.databases-spreadsheet. LGTM.src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/activity/+page.svelte (2)
8-10: Prop rename and wrapper look correctSwitching to
databasesMainScreenand removing the wide-screen wrapper matches the new layout model. Order{...data} databasesMainScreenensures the explicit flag isn’t overridden by spread props.
8-10: No staledatabasesScreenreferences found
Ran a global search fordatabasesScreenand legacy container imports; no occurrences remain.
| <h1 class="animated-title" style:font-size={currentFontSize}> | ||
| {@render children?.()} | ||
| </h1> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid empty
when no children provided.
Render the heading only if children exists, or provide a title prop fallback.
- <h1 class="animated-title" style:font-size={currentFontSize}>
- {@render children?.()}
- </h1>
+ {#if children}
+ <h1 class="animated-title" style:font-size={currentFontSize}>
+ {@render children()}
+ </h1>
+ {/if}🤖 Prompt for AI Agents
In src/lib/layout/animatedTitle.svelte around lines 44-46, the <h1> may render
empty when children is missing; update the component to only render the <h1>
when children exists or to render a fallback title prop: add an optional "title"
prop, and change the template to conditionally render the heading — if children
is provided call/render it inside the <h1>, otherwise render the <h1> with the
title fallback only when title is non-empty (avoid emitting an empty <h1>).
src/lib/layout/cover.svelte
Outdated
| &.adjustForSpreadsheet { | ||
| padding-left: 2.5px; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Likely unit bug: 2.5px should be 2.5rem.
padding-left: 2.5px is almost certainly meant to align with the -2.5rem title offset and the extra 3rem top padding. Using px causes visible misalignment.
Apply:
- &.adjustForSpreadsheet {
- padding-left: 2.5px;
- }
+ &.adjustForSpreadsheet {
+ padding-left: 2.5rem;
+ }📝 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.
| &.adjustForSpreadsheet { | |
| padding-left: 2.5px; | |
| } | |
| &.adjustForSpreadsheet { | |
| padding-left: 2.5rem; | |
| } |
🤖 Prompt for AI Agents
In src/lib/layout/cover.svelte around lines 126 to 128, the CSS rule uses
padding-left: 2.5px which is likely a unit bug causing misalignment with the
-2.5rem title offset and 3rem top padding; change the unit from px to rem
(padding-left: 2.5rem) so the spacing aligns with the other rem-based
measurements and preserves responsive scaling.
| const isTablesScreen = $derived(page.route.id.endsWith('table-[table]')); | ||
|
|
||
| const isMainDatabaseScreen = $derived(page.route.id.endsWith('database-[database]')); | ||
| const isTablesScreen = $derived(page.route.id.includes('table-[table]')); |
There was a problem hiding this comment.
Guard page.route.id before calling .includes to avoid runtime crashes
page.route.id can be undefined during initial render; .includes would throw.
-const isTablesScreen = $derived(page.route.id.includes('table-[table]'));
+const isTablesScreen = $derived.by(() => !!page.route.id?.includes('table-[table]'));Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
src/routes/(console)/project-[region]-[project]/databases/database-[database]/subNavigation.svelte
around line 52, the expression calls page.route.id.includes(...) which can throw
if page.route.id is undefined; update the derived store to first guard that
page.route and page.route.id are defined (and a string) before calling .includes
(e.g. use an explicit conditional or optional chaining) and return a safe
default (false) when id is missing so the component won't crash on initial
render.
| @@ -328,7 +341,7 @@ | |||
| z-index: 20; | |||
| display: flex; | |||
| justify-content: center; | |||
| transition: height 300ms cubic-bezier(0.4, 0, 0.2, 1); | |||
| transition: none !important; | |||
|
|
|||
There was a problem hiding this comment.
Ready-gated height transition is disabled by !important.
transition: none !important; on the base rule prevents the later [data-ready='true'] rule from taking effect. The height will never animate.
Apply this diff to fix:
- transition: none !important;
+ transition: none;Option B (if you must keep higher specificity on the base rule): also mark the ready rule !important.
- &[data-ready='true'] {
- transition: height 300ms cubic-bezier(0.4, 0, 0.2, 1);
- }
+ &[data-ready='true'] {
+ transition: height 300ms cubic-bezier(0.4, 0, 0.2, 1) !important;
+ }Also applies to: 360-362
🤖 Prompt for AI Agents
In
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/layout/emptySheet.svelte
around lines 345 and also affecting 360-362, the base CSS rule sets "transition:
none !important" which prevents the later "[data-ready='true']" height
transition from running; remove the "!important" from the base rule so the
ready-state rule can override it, or if you must keep the higher specificity on
the base rule, add "!important" to the "[data-ready='true']" transition
declaration so the height animation takes effect.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/layout/container.svelte (1)
49-114: Ensure .console-container styling remains available to all consumers
We scoped.console-containerlocally insrc/lib/layout/container.svelte, but the class is still used (and styled) elsewhere. Update or reintroduce global styles for these locations, or convert them to use the<Container>component:
- src/lib/layout/shell.svelte:308
- src/routes/(console)/project-[region]-[project]/databases/.../rows/activity.svelte:59
- src/routes/(console)/project-[region]-[project]/overview/onboard.svelte:79
- src/routes/(public)/auth/+layout.svelte:11
- src/routes/(public)/template-[template]/+layout.svelte:8
♻️ Duplicate comments (3)
src/lib/layout/cover.svelte (1)
126-128: Likely unit bug: 2px should be 2.5rem for consistent spacing
padding-left: 2pxis almost certainly meant to be2.5rem(consistent with other rem-based offsets). Usingpxmakes the effect nearly invisible and breaks scale.- &.adjustForSpreadsheet { - padding-left: 2px; - } + &.adjustForSpreadsheet { + padding-left: 2.5rem; + }src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/layout/emptySheet.svelte (1)
339-354: Ready-gated transition never fires due to!importantand missing data-ready attribute
transition: none !important;blocks the later[data-ready='true']rule.- The element never sets
data-ready, so the rule won’t match anyway.Fix either by removing
!important(preferred) and setting the attribute, or by adding!importantto the ready rule as well.Apply:
- transition: none !important; + transition: none;And ensure the element sets the attribute when rendered (it’s already gated by
!$spreadsheetLoading):- <div + <div class="spreadsheet-fade-bottom" data-collapsed-tabs={!$expandTabs} + data-ready="true" style:--dynamic-overlay-height={dynamicOverlayHeight}>Alternative (if you must keep
!importantabove):- &[data-ready='true'] { - transition: height 300ms cubic-bezier(0.4, 0, 0.2, 1); - } + &[data-ready='true'] { + transition: height 300ms cubic-bezier(0.4, 0, 0.2, 1) !important; + }src/lib/layout/animatedTitle.svelte (1)
53-55: Avoid rendering an emptywhen no children are provided
Render the heading only when children exists (same ask from prior review).
- <h1 class="animated-title" style:font-size={currentFontSize}> - {@render children?.()} - </h1> + {#if children} + <h1 class="animated-title" style:font-size={currentFontSize}> + {@render children()} + </h1> + {/if}
🧹 Nitpick comments (10)
src/lib/layout/shell.svelte (1)
308-319: Scope.console-containerpadding to.main-contentto avoid compounded offsets
The onlypadding-inline-starton.console-containerlives inshell.svelteunder:global(main:has(.databases-spreadsheet)), and you’re already addingpadding-left: 210pxto.main-contentin that same block. Restricting the rule to.main-content :global(.console-container)will prevent unintended double‐padding without affecting any other contexts.Apply:
- :global(.console-container) { + .main-content :global(.console-container) { @media (min-width: 1024px) { padding-inline-start: var(--base-32, 32px); } /* … */ }src/lib/layout/cover.svelte (2)
83-87: Use logical property for RTL supportPrefer
padding-inline-startoverpadding-leftto keep spreadsheet alignment RTL-safe.- &.adjustForSpreadsheet { - @media (min-width: 1024px) { - padding-left: calc(190px + 3rem); - } - } + &.adjustForSpreadsheet { + @media (min-width: 1024px) { + padding-inline-start: calc(190px + 3rem); + } + }
130-134: Remove unuseddatabasesMainScreenprop and related styles
No external references todatabasesMainScreenwere found—its defaultfalsemeans theclass:databasesMainScreenbinding (src/lib/layout/cover.svelte:47) and the CSS block (lines 130–134) never apply. Drop theexport let databasesMainScreendeclaration (line 11), the class directive, and the.databasesMainScreenrule.src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/layout/emptySheet.svelte (2)
169-170: Debounce window resize handler to reduce layout thrashUse the existing debounced function for
resizeto avoid rapid reflows during viewport chrome changes (esp. mobile Safari).-<svelte:window on:resize={updateOverlayHeight} /> +<svelte:window on:resize={debouncedUpdateOverlayHeight} />
371-397: Simplify centering logic; use transform instead of breakpoint-specific left offsetsMultiple breakpoint-specific
left: 45–50%tweaks are brittle. Center reliably withleft: 50%+transform: translateX(-50%), and adjust onlybottomper breakpoint..empty-actions { - left: 50%; - bottom: 35%; - position: fixed; + left: 50%; + bottom: 35%; + position: fixed; + transform: translateX(-50%); - @media (max-width: 768px) { - bottom: 35%; - } + @media (max-width: 768px) { + bottom: 35%; + } - @media (max-width: 1023px) { - left: unset; - } + @media (max-width: 1023px) { + /* keep centered; remove left overrides */ + } - @media (min-width: 1024px) { - left: 47.5%; - } + @media (min-width: 1024px) { + bottom: 35%; + } @media (min-width: 1280px) { - left: 50%; bottom: 37.5%; } @media (min-width: 1440px) { - left: 47.5%; bottom: 40%; } @media (min-width: 1728px) { - left: 45%; - bottom: 50%; + bottom: 50%; } }src/lib/layout/animatedTitle.svelte (1)
66-70: Nit: drop unused transitionsline-height and letter-spacing no longer change; keep the transition scoped to font-size to reduce repaints.
- transition: - font-size 300ms cubic-bezier(0.4, 0, 0.2, 1), - line-height 300ms cubic-bezier(0.4, 0, 0.2, 1), - letter-spacing 300ms cubic-bezier(0.4, 0, 0.2, 1); + transition: font-size 300ms cubic-bezier(0.4, 0, 0.2, 1);Also applies to: 75-76
src/lib/layout/coverTitle.svelte (2)
20-28: Nit: prefer class toggle over inline style for the offsetImproves readability and avoids string styles.
- style={backOnlyDesktop && $isTabletViewport ? 'margin-inline-start: -2.5rem;' : ''} - {...restProps}> + class:back-offset={backOnlyDesktop && $isTabletViewport} + {...restProps}>Add (in this file’s style block or a shared stylesheet):
.back-offset { margin-inline-start: -2.5rem; }
43-49: Avoid rendering an empty title when no children are providedMirror animatedTitle.svelte to prevent empty headings.
- <Typography.Title - truncate - color="--fgcolor-neutral-primary" - size={$isSmallViewport ? 'm' : 'l'}> - {@render children?.()} - </Typography.Title> + {#if children} + <Typography.Title + truncate + color="--fgcolor-neutral-primary" + size={$isSmallViewport ? 'm' : 'l'}> + {@render children()} + </Typography.Title> + {/if}src/lib/layout/container.svelte (2)
73-89: Rules depending on :first-child can be fragile.If a sibling ever precedes the inner container, these expanded offsets won’t apply. Consider a more robust hook (e.g., wrap selector in a data attribute or target a known wrapper class) rather than positional pseudo-classes.
116-120: :has() usage and global selector: confirm baseline or add a fallback.:has() is broadly supported in evergreen browsers, but if older versions are in scope, add a non-:has fallback (e.g., toggling a .has-sub-navigation class on
).Proposed CSS fallback (requires adding .has-sub-navigation on
when the sub-nav is present):@media (min-width: 1024px) { - :global(main:has(.sub-navigation) .console-container) { - padding-inline-end: var(--base-32, 32px); - } + :global(main:has(.sub-navigation) .console-container), + :global(main.has-sub-navigation .console-container) { + padding-inline-end: var(--base-32, 32px); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/lib/layout/activity.svelte(2 hunks)src/lib/layout/animatedTitle.svelte(3 hunks)src/lib/layout/container.svelte(2 hunks)src/lib/layout/cover.svelte(5 hunks)src/lib/layout/coverTitle.svelte(3 hunks)src/lib/layout/shell.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/header.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/activity/+page.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/header.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/layout/emptySheet.svelte(5 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/settings/+page.svelte(1 hunks)src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/usage/[[period]]/+page.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/activity/+page.svelte
- src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/settings/+page.svelte
- src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/usage/[[period]]/+page.svelte
- src/routes/(console)/project-[region]-[project]/databases/database-[database]/header.svelte
🔇 Additional comments (8)
src/lib/layout/cover.svelte (1)
10-10: New prop looks good
adjustForSpreadsheetexport is clear and aligns with usage in DB headers.src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/layout/emptySheet.svelte (2)
80-82: Eager height computation on mountCalling
updateOverlayHeight()on mount prevents initial layout jump. Looks good.
95-99: Reactive recalc on tab expansionThe
$effecthook cleanly ties overlay height to$expandTabs. LGTM.src/lib/layout/coverTitle.svelte (1)
26-38: Back button gating on tablet looks correctUsing backOnlyDesktop to control disabled/visibility on tablets is consistent with the intended behavior.
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/header.svelte (1)
77-87: LGTM: Cover-based header compositionSwitching to Cover with adjustForSpreadsheet + collapsed/expanded states simplifies the layout and aligns with the new route structure.
src/lib/layout/container.svelte (1)
36-36: Always-on base class is good.Setting class="console-container" ensures consistent styling hooks and simplifies selector specificity. Looks good.
src/lib/layout/activity.svelte (2)
14-14: New expanded prop: good decoupling.Exposing expanded (default false) keeps this component independent of route-specific flags.
26-26: DerivecontainerExpandedand verifyexpandedis passed
- No
<Activity>call site passes theexpandedprop—confirm this is intended or addexpandedto relevant pages if needed.- Optional: extract
expanded && !insideSideSheetinto a constant for clarity:-<Container {insideSideSheet} expanded={expanded && !insideSideSheet}> +{#const containerExpanded = expanded && !insideSideSheet} +<Container {insideSideSheet} expanded={containerExpanded}>
| <Button.Anchor | ||
| {href} | ||
| icon | ||
| variant="text" | ||
| size={buttonSize} | ||
| aria-label="page back" | ||
| disabled={$isTabletViewport} | ||
| style={$isTabletViewport ? 'visibility: hidden' : ''}> | ||
| <Icon icon={IconChevronLeft} /> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix: respect backOnlyDesktop when hiding/disabling back button on tablet
Currently the back button is disabled/hidden on all tablets, ignoring backOnlyDesktop. This breaks navigation in contexts where the back button should remain visible on tablet. Align with coverTitle.svelte behavior.
- <Button.Anchor
- {href}
- icon
- variant="text"
- size={buttonSize}
- aria-label="page back"
- disabled={$isTabletViewport}
- style={$isTabletViewport ? 'visibility: hidden' : ''}>
+ <Button.Anchor
+ {href}
+ icon
+ variant="text"
+ size={buttonSize}
+ aria-label="page back"
+ disabled={backOnlyDesktop && $isTabletViewport}
+ style={backOnlyDesktop && $isTabletViewport ? 'visibility: hidden' : ''}>Run to confirm there are no other unconditional tablet hides:
🏁 Script executed:
#!/bin/bash
rg -n -C2 -g 'src/**' -e 'disabled=\{\$isTabletViewport\}' -e 'visibility:\s*hidden.*\$isTabletViewport'Length of output: 431
Respect backOnlyDesktop when hiding/disabling back button on tablet
In src/lib/layout/animatedTitle.svelte (lines 47–48), wrap both disabled and style in backOnlyDesktop && $isTabletViewport so the back button remains active unless explicitly restricted:
- disabled={$isTabletViewport}
- style={$isTabletViewport ? 'visibility: hidden' : ''}>
+ disabled={backOnlyDesktop && $isTabletViewport}
+ style={backOnlyDesktop && $isTabletViewport ? 'visibility: hidden' : ''}>📝 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.
| <Button.Anchor | |
| {href} | |
| icon | |
| variant="text" | |
| size={buttonSize} | |
| aria-label="page back" | |
| disabled={$isTabletViewport} | |
| style={$isTabletViewport ? 'visibility: hidden' : ''}> | |
| <Icon icon={IconChevronLeft} /> | |
| <Button.Anchor | |
| {href} | |
| icon | |
| variant="text" | |
| size={buttonSize} | |
| aria-label="page back" | |
| disabled={backOnlyDesktop && $isTabletViewport} | |
| style={backOnlyDesktop && $isTabletViewport ? 'visibility: hidden' : ''}> | |
| <Icon icon={IconChevronLeft} /> |
🤖 Prompt for AI Agents
In src/lib/layout/animatedTitle.svelte around lines 41 to 49, the back button
currently uses $isTabletViewport to set disabled and style; update both the
disabled prop and the style expression to only apply when backOnlyDesktop &&
$isTabletViewport so the button stays active by default unless backOnlyDesktop
is true and we are on tablet. Replace disabled={$isTabletViewport} with
disabled={backOnlyDesktop && $isTabletViewport} and change
style={$isTabletViewport ? 'visibility: hidden' : ''} to style={backOnlyDesktop
&& $isTabletViewport ? 'visibility: hidden' : ''}.
| <div class="tabs-container" class:collapsed={!$expandTabs}> | ||
| <Tabs> | ||
| {#each tabs as tab} | ||
| <Tab | ||
| href={tab.href} | ||
| selected={isTabSelected(tab, page.url.pathname, path, tabs)} | ||
| event={tab.event}> | ||
| {tab.title} | ||
| </Tab> | ||
| {/each} | ||
| </Tabs> | ||
| </div> |
There was a problem hiding this comment.
Fix a11y: hidden tabs remain keyboard-focusable when collapsed
Opacity/pointer-events hide the tabs visually but do not remove them from the accessibility tree or keyboard focus. Add aria-hidden and inert when collapsed.
- <div class="tabs-container" class:collapsed={!$expandTabs}>
+ <div
+ class="tabs-container"
+ class:collapsed={!$expandTabs}
+ aria-hidden={!$expandTabs}
+ inert={!$expandTabs}>📝 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.
| <div class="tabs-container" class:collapsed={!$expandTabs}> | |
| <Tabs> | |
| {#each tabs as tab} | |
| <Tab | |
| href={tab.href} | |
| selected={isTabSelected(tab, page.url.pathname, path, tabs)} | |
| event={tab.event}> | |
| {tab.title} | |
| </Tab> | |
| {/each} | |
| </Tabs> | |
| </div> | |
| <div | |
| class="tabs-container" | |
| class:collapsed={!$expandTabs} | |
| aria-hidden={!$expandTabs} | |
| inert={!$expandTabs}> | |
| <Tabs> | |
| {#each tabs as tab} | |
| <Tab | |
| href={tab.href} | |
| selected={isTabSelected(tab, page.url.pathname, path, tabs)} | |
| event={tab.event}> | |
| {tab.title} | |
| </Tab> | |
| {/each} | |
| </Tabs> | |
| </div> |
🤖 Prompt for AI Agents
In
src/routes/(console)/project-[region]-[project]/databases/database-[database]/table-[table]/header.svelte
around lines 94 to 105, the tabs container is visually hidden when collapsed but
remains keyboard-focusable; update the container element to set
aria-hidden="true" and add the inert attribute when collapsed (i.e., when
!$expandTabs) and ensure aria-hidden="false" and remove inert when expanded so
the tabs are removed from the accessibility tree and cannot receive focus when
collapsed.
What does this PR do?
Aligns the cover and container.
Test Plan
Manual
Related PRs and Issues
N/A.
Have you read the Contributing Guidelines on issues?
Yes.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor