fix: Hide navigation bar during onboarding animation #2370
Conversation
ConsoleProject ID: Sites (2)
Note Appwrite has a Discord community with over 16 000 members. |
WalkthroughThe shell subscription for Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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: 0
🧹 Nitpick comments (2)
src/lib/layout/shell.svelte (1)
126-131: Replace imperative header toggle with a derived flag to avoid prop overrides and unsubscribe leaksThe current subscription mutates the
showHeaderprop and never unsubscribes. Prefer a derived reactive flag that respects the parent prop and hides the header only while the wizard is open.Apply this diff here:
- let initialShowHeader = showHeader; - isNewWizardStatusOpen.subscribe((value) => { - if (initialShowHeader) { - showHeader = !value; - } - }); + // Honor parent-controlled visibility; additionally hide while wizard is open + $: headerVisible = showHeader && !$isNewWizardStatusOpen;Then update template usages (outside this hunk) to use
headerVisible:- class:no-header={!showHeader} + class:no-header={!headerVisible} ... - {#if showHeader} + {#if headerVisible}src/routes/(console)/+layout.svelte (1)
342-343: Make the onboarding route check precise and DRYMinor: compute a single
isOnboardingRouteand reuse it for header/footer (and optionally side nav). Using a path-anchored regex avoids accidental matches (e.g.,/console/onboarding-legacy).Suggested changes:
In the
<script>block:const isOnboardingRoute = /^\/console\/onboarding(?:\/|$)/.test(page.url.pathname);In the
Shellprops:- showHeader={!page.url.pathname.includes('/console/onboarding')} - showFooter={!page.url.pathname.includes('/console/onboarding')} + showHeader={!isOnboardingRoute} + showFooter={!isOnboardingRoute}If you intentionally want to catch suffixed variants like
/console/onboarding-legacy, keep the currentincludescheck.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lib/layout/shell.svelte(1 hunks)src/routes/(console)/+layout.svelte(1 hunks)
⏰ 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: e2e
- GitHub Check: build
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/layout/shell.svelte(1 hunks)
⏰ 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: e2e
- GitHub Check: build
| isNewWizardStatusOpen.subscribe((value) => { | ||
| if (showHeader) { | ||
| showHeader = !value; | ||
| } | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Guarded inversion can leave the header hidden after the wizard closes; derive visibility instead of mutating the prop.
When the wizard opens, showHeader is set to false, but when it closes (value === false) this block does nothing if showHeader is already false. On non-onboarding routes (where the parent doesn’t re-drive showHeader), the header can remain hidden until a route change. This also introduces an avoidable manual subscription that isn’t unsubscribed.
Apply this diff to remove the manual subscription:
- isNewWizardStatusOpen.subscribe((value) => {
- if (showHeader) {
- showHeader = !value;
- }
- });
+ // Removed: use a derived reactive flag instead (see below).Then, introduce a derived flag and use it in markup (outside this hunk):
@@
// reactive blocks
@@
+ // Drive header visibility from parent intent AND wizard state; no local prop mutation.
+ $: effectiveShowHeader = showHeader && !$isNewWizardStatusOpen;
@@
- class:no-header={!showHeader}
+ class:no-header={!effectiveShowHeader}
@@
- {#if showHeader}
+ {#if effectiveShowHeader}Why: avoids sticky state, prevents flicker on onboarding routes (parent still passes showHeader=false), and removes the untracked subscription.
Verification (quick manual):
- Navigate to any non-onboarding page, open and close the new wizard: header should reappear immediately after close.
- Navigate within
/console/onboarding/**: header should remain hidden throughout, regardless of wizard open/close.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/lib/layout/shell.svelte"
echo "PWD: $(pwd)"
if [ -f "$FILE" ]; then
echo "== File exists: $FILE =="
echo "== Grep for relevant symbols (context 3) =="
rg -n -C3 'isNewWizardStatusOpen|\$isNewWizardStatusOpen|showHeader|class:no-header' "$FILE" || true
echo "== Showing file (first 400 lines) =="
sed -n '1,400p' "$FILE" || true
else
echo "File not found: $FILE"
echo "== Searching repo for symbols =="
rg -n -C3 'isNewWizardStatusOpen|\$isNewWizardStatusOpen|showHeader|class:no-header' || true
fiLength of output: 11750
Derive header visibility instead of mutating the exported prop
Manual subscription at src/lib/layout/shell.svelte (lines ~126-130) mutates the exported showHeader and is never unsubscribed — this can leave the header hidden after the wizard closes. Remove the subscription and drive visibility from the parent prop + wizard store.
Apply these changes:
@@
- isNewWizardStatusOpen.subscribe((value) => {
- if (showHeader) {
- showHeader = !value;
- }
- });
+ // Removed manual subscription; header visibility is derived instead.Add a derived reactive flag and use it in markup:
@@
// reactive blocks
@@
+ // Drive header visibility from parent intent AND wizard state; no local prop mutation.
+ $: effectiveShowHeader = showHeader && !$isNewWizardStatusOpen;
@@
- class:no-header={!showHeader}
+ class:no-header={!effectiveShowHeader}
@@
- {#if showHeader}
+ {#if effectiveShowHeader}src/lib/layout/shell.svelte: fix required at the subscription (≈lines 126-130) and the header markup (class/no-header + {#if showHeader}).
📝 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.
| isNewWizardStatusOpen.subscribe((value) => { | |
| if (showHeader) { | |
| showHeader = !value; | |
| } | |
| }); | |
| // Removed manual subscription; header visibility is derived instead. | |
| // Drive header visibility from parent intent AND wizard state; no local prop mutation. | |
| $: effectiveShowHeader = showHeader && !$isNewWizardStatusOpen; | |
| class:no-header={!effectiveShowHeader} | |
| {#if effectiveShowHeader} |
| showHeader={!page.url.pathname.includes('/console/onboarding')} | ||
| showFooter={!page.url.pathname.includes('/console/onboarding')} |
There was a problem hiding this comment.
can be exported to a var since its the same check.
What does this PR do?
(Provide a description of what this PR does.)
Test Plan
Related PRs and Issues
(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)
Have you read the Contributing Guidelines on issues?
yes
Summary by CodeRabbit
New Features
Bug Fixes