-
Notifications
You must be signed in to change notification settings - Fork 206
New promo #2701
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New promo #2701
Conversation
Console (appwrite/console)Project ID: Tip Global CDN and DDoS protection come free with every Sites deployment |
WalkthroughThis pull request upgrades Node.js version support from 20 to 22 across GitHub Actions workflows and package.json. It introduces Three.js and Threlte dependencies for 3D rendering capabilities and adds two new Svelte components: an "Imagine" promo featuring a 3D shader-based canvas, and a post-processing shader component using procedural noise and radial masking. The bottom modal alert system is extended with theme-aware color resolution, optional background components, and enhanced mobile layout configuration. A feature-flagged Imagine promo replaces the previous cloud/AI suggestions promo in the console route. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 (1)
src/lib/components/bottomModalAlert.svelte (1)
216-229: src is now optional but still dereferenced unguarded when nobackgroundComponentis providedWith
BottomModalAlertItem.srcmade optional andbackgroundComponentadded, the template still assumes that ifbackgroundComponentis absent thensrcexists:{:else if $app.themeInUse === 'dark'} <img src={currentModalAlert.src.dark} ... /> {:else} <img src={currentModalAlert.src.light} ... /> {/if}This is safe for existing alerts (they all had
srcbefore), and the Imagine promo path usesbackgroundComponent, so it’s fine there as well. But the updated type now allows callers to define alerts that have neithersrcnorbackgroundComponent, which would cause a runtime error here whensrcisundefined.To make the new API harder to misuse, you could either:
- Add a defensive check in the template, e.g. only dereference
srcif it exists and otherwise skip the image entirely, or- Tighten the type to a union that enforces “exactly one of
srcorbackgroundComponent” so consumers can’t create an invalid combination.Example of the first option:
-{:else if $app.themeInUse === 'dark'} - <img src={currentModalAlert.src.dark} ... /> -{:else} - <img src={currentModalAlert.src.light} ... /> -{/if} +{:else if currentModalAlert.src && $app.themeInUse === 'dark'} + <img src={currentModalAlert.src.dark} ... /> +{:else if currentModalAlert.src} + <img src={currentModalAlert.src.light} ... /> +{/if}Consider applying the same guard in both desktop and mobile branches.
Also applies to: 339-353
🧹 Nitpick comments (7)
.github/workflows/tests.yml (1)
19-22: Consider upgradingactions/setup-nodeversion for consistency.This workflow uses
actions/setup-node@v3whilecopilot-setup-steps.ymluses@v6. Consider upgrading to at least@v4for consistency across workflows and to benefit from newer features and bug fixes.- name: Use Node.js - uses: actions/setup-node@v3 + uses: actions/setup-node@v4 with: node-version: 22.github/workflows/e2e.yml (1)
16-19: Consider upgradingactions/setup-nodeversion.Same as
tests.yml, this workflow uses@v3whilecopilot-setup-steps.ymluses@v6. Consider upgrading to@v4for consistency.- name: Use Node.js - uses: actions/setup-node@v3 + uses: actions/setup-node@v4 with: node-version: 22src/lib/components/promos/imagine.svelte (1)
1-6: File naming convention: consider PascalCase.As per coding guidelines, component files should use PascalCase (e.g.,
Imagine.svelte). The current lowercase naming (imagine.svelte) is inconsistent with the convention.src/lib/components/promos/shader.svelte (2)
4-4: Use$libalias for imports per coding guidelines.The import for
noise.glsluses a relative path. Per the coding guidelines, prefer the$libalias for module imports from thesrc/libdirectory.- import noise from './threlte/shaders/noise.glsl?raw'; + import noise from '$lib/components/promos/threlte/shaders/noise.glsl?raw';
1-6: File naming convention: consider PascalCase.Similar to
imagine.svelte, this component file uses lowercase naming. Per coding guidelines, component files should use PascalCase (e.g.,Shader.svelte).src/routes/(console)/bottomAlerts.ts (1)
3-8: Imagine promo wiring and mobile layout config look consistent; consider future-proofing the list handlingThe Imagine promo definition and gating (
isCloud && SHOW_IMAGINE_PROMO) plus the call intoshowBottomModalAlertandsetMobileSingleAlertLayoutare coherent and match the updated store types. BecauselistOfPromotionsis populated once at module load andshowBottomModalAlertdedupes byid, there’s no risk of duplicate alerts.Two small forward-looking points:
setMobileSingleAlertLayoutcurrently always derives its config fromlistOfPromotions[0]. If you ever add more promotions to this list, you may want to explicitly pick the promo that should control the single-mobile layout (e.g. via a flag onBottomModalAlertItem) rather than assuming “first in list”.SHOW_IMAGINE_PROMOas a hard-coded boolean means enabling/disabling requires a deploy. If you expect to toggle this more dynamically later, consider threading it through an env/feature-flag system instead.Overall, behavior for the current single-promo Imagine case looks correct.
Also applies to: 10-45, 47-55
src/lib/stores/bottom-alerts.ts (1)
2-12: New styling and layout fields are well-scoped; consider enforcing the src/backgroundComponent invariant in the typeThe extensions to the store types look good:
- Optional
color/background/backgroundHoveronBottomModalAlertActiongive enough flexibility for theming without impacting existing alerts.- Adding
backgroundComponent?: ComponentandsameContentOnMobileLayout?: booleantoBottomModalAlertItemmatches how the UI now renders promos and derives the mobile layout.One thing to watch: the comment
// use either of these!forsrcandbackgroundComponentisn’t enforced by the type. Right now, callers can legally provide both or neither, and the rendering code assumes at least one is present. To better encode the intended contract, you could introduce a union type such as:type WithSrc = { src: Record<'dark' | 'light', string>; backgroundComponent?: never }; type WithBackgroundComponent = { src?: never; backgroundComponent: Component }; export type BottomModalAlertItem = BaseAlertItem & (WithSrc | WithBackgroundComponent);(where
BaseAlertItemholds the rest of the existing fields).Not required for this PR to function, but it would make the new API harder to misuse.
Also applies to: 39-42, 54-59
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlsrc/lib/components/promos/imagine.svgis excluded by!**/*.svgsrc/lib/components/promos/threlte/shaders/noise.glslis excluded by!**/*.glsl
📒 Files selected for processing (9)
.github/workflows/copilot-setup-steps.yml(1 hunks).github/workflows/e2e.yml(1 hunks).github/workflows/tests.yml(1 hunks)package.json(4 hunks)src/lib/components/bottomModalAlert.svelte(10 hunks)src/lib/components/promos/imagine.svelte(1 hunks)src/lib/components/promos/shader.svelte(1 hunks)src/lib/stores/bottom-alerts.ts(3 hunks)src/routes/(console)/bottomAlerts.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx,js,jsx,svelte}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx,svelte}: Import reusable modules from the src/lib directory using the $lib alias
Use minimal comments in code; reserve comments for TODOs or complex logic explanations
Use $lib, $routes, and $themes aliases instead of relative paths for module imports
Files:
src/lib/components/bottomModalAlert.sveltesrc/lib/components/promos/shader.sveltesrc/lib/components/promos/imagine.sveltesrc/lib/stores/bottom-alerts.tssrc/routes/(console)/bottomAlerts.ts
src/lib/components/**/*.svelte
📄 CodeRabbit inference engine (AGENTS.md)
Use PascalCase for component file names and place them in src/lib/components/[feature]/ directory structure
Files:
src/lib/components/bottomModalAlert.sveltesrc/lib/components/promos/shader.sveltesrc/lib/components/promos/imagine.svelte
**/*.{ts,tsx,js,jsx,svelte,json}
📄 CodeRabbit inference engine (AGENTS.md)
Use 4 spaces for indentation, single quotes, 100 character line width, and no trailing commas per Prettier configuration
Files:
src/lib/components/bottomModalAlert.sveltepackage.jsonsrc/lib/components/promos/shader.sveltesrc/lib/components/promos/imagine.sveltesrc/lib/stores/bottom-alerts.tssrc/routes/(console)/bottomAlerts.ts
**/*.svelte
📄 CodeRabbit inference engine (AGENTS.md)
Use Svelte 5 + SvelteKit 2 syntax with TypeScript for component development
Files:
src/lib/components/bottomModalAlert.sveltesrc/lib/components/promos/shader.sveltesrc/lib/components/promos/imagine.svelte
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ts: Define types inline or in .d.ts files, avoid creating separate .types.ts files
Use TypeScript in non-strict mode; any type is tolerated in this project
Files:
src/lib/stores/bottom-alerts.tssrc/routes/(console)/bottomAlerts.ts
src/routes/**
📄 CodeRabbit inference engine (AGENTS.md)
Configure dynamic routes using SvelteKit convention with [param] syntax in route directory names
Files:
src/routes/(console)/bottomAlerts.ts
🧠 Learnings (2)
📚 Learning: 2025-11-25T03:15:27.539Z
Learnt from: CR
Repo: appwrite/console PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T03:15:27.539Z
Learning: Run the mandatory pre-completion checklist before finishing work: pnpm run format → pnpm run check → pnpm run lint → pnpm run test → pnpm run build, ensuring all commands pass with zero errors
Applied to files:
.github/workflows/copilot-setup-steps.yml
📚 Learning: 2025-11-25T03:15:27.539Z
Learnt from: CR
Repo: appwrite/console PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T03:15:27.539Z
Learning: Applies to src/lib/components/**/*.svelte : Use PascalCase for component file names and place them in src/lib/components/[feature]/ directory structure
Applied to files:
src/lib/components/promos/imagine.svelte
🧬 Code graph analysis (1)
src/routes/(console)/bottomAlerts.ts (2)
src/lib/stores/bottom-alerts.ts (3)
BottomModalAlertItem(34-59)showBottomModalAlert(89-104)setMobileSingleAlertLayout(75-80)src/lib/system.ts (1)
isCloud(26-26)
⏰ 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
🔇 Additional comments (4)
.github/workflows/copilot-setup-steps.yml (1)
21-25: LGTM!Node.js version upgrade to 22 is consistent with
package.jsonengine requirements (>=22.0.0) and aligns with the other workflow files in this PR. Usingactions/setup-node@v6is appropriate.src/lib/components/bottomModalAlert.svelte (3)
22-28: The theming helper and CTA styling integration look sound
resolveThemeColor’s handling ofstring | {light,dark} | undefinedis straightforward and works well with the newcolor/background/backgroundHoverfields onBottomModalAlertAction. Passing its result into CSS custom properties and inlinestyle:coloron the CTA text means:
- Existing alerts without these fields keep their previous appearance (CSS vars simply don’t get set).
- The Imagine promo (and future promos) can provide explicit themed colors without affecting the rest.
No correctness issues stand out here.
Also applies to: 275-292, 403-427
111-124: Mobile layout override behavior matches the new config and flag semanticsThe
getMobileWindowConfigchanges and the use ofmobileConfig.titlein the collapsed mobile card align with the new mobile layout behavior:
sameContentOnMobileLayoutnow updates the fallback title/message to mirrorcurrentModalAlertwhen no explicit layout config is provided, matching the comment on the new field.- When a
MobileSingleAlertLayoutConfigis set (as in the Imagine promo flow), itstitle/messagecorrectly override the fallback and are used for the collapsed mobile banner viamobileConfig.title/mobileConfig.message.This gives you a clear separation between “per-alert, opt-in sync with desktop content” and “explicit, layout-level overrides” and looks correct for both existing alerts and the new promo.
Also applies to: 388-400, 451-483, 470-471
490-496: UI polish changes (shadow and z-index) are localized and low-riskThe added box-shadow on
.cardand the explicitz-indexon.icon-inline-tagare small, self-contained visual tweaks and don’t affect logic. They should help keep the close button above background content (including the new backgroundComponent) and visually separate the card from the page.No changes needed from a correctness perspective.
Also applies to: 513-516

What does this PR do?
(Provide a description of what this PR does.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work.)
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?
(Write your answer here.)
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.