feat: Create Svelte app-launchpad by goulinkh · Pull Request #246 · canonical/pragma

goulinkh · 2025-06-05T13:10:47Z

Done

feat(storybook): Enhance configuration for Svelte support
feat(svelte): Introduce Svelte app launchpad

Fixes LP-2618

QA

Checkout the branch locally
Run bun install
Go to cd packages/svelte/ds-app-launchpad
Run the storybook server: bun storybook
- Make sure that the component AwesomeButton stories are showing and working like React components
Run the unit tests: bun run test
- Make sure that the tests are running as expected
- It is ok if the first run is slow, because vitest is running in browser mode

TODO

In a follow up PR I will add the CI tasks to publish to NPM and run chromatic in PRs.
We are still looking into the linter options (biome vs eslint+prettier), so that will be done in a separate PR

PR readiness check

PR should have one of the following labels:
- Feature 🎁, Breaking Change 💣, Bug 🐛, Documentation 📝, Maintenance 🔨.
PR title follows the Conventional Commits format.
All packages define the required scripts in package.json:
- All packages: check, check:fix, and test.
- Packages with a build step: build.

advl

Starting to look good.

This review is not final (still missing package.json and vite config)

General comments :

Formatting/linting is not passing
Wondering what would be a better way to contextualize cursor
I like your usage of the lib folder. So far we've used src/ui for components, but I wonder if we wouldn't benefit to harmonize every ds package to src/lib instead ? Wdyt @jmuzina ?

packages/svelte/ds-app-launchpad/.gitignore

configs/storybook/src/createConfig.ts

packages/svelte/ds-app-launchpad/src/app.html

advl · 2025-06-05T15:02:51Z

packages/svelte/.cursor/rules/svelte5.mdc

I wonder if those should live on the monorepo - if it's the case we could end up with ide specific rules for many editors. Would there be a better way to consume those ?
Maybe have them on a special package cursor-rules ?

In my opinion these are useful, unfortunately there is no standard LLM rules across IDEs (I hope eventually there will be?)

Maybe have them on a special package cursor-rules ?

The .cursor folder works similarly to rc files where it needs to have this name and be a parent or grandparent of the folder where we want the rules to be used, so it won't be possible to: change the name of the folder, move it to a far place where it wouldn't be a grandparent of the directory svelte.

I think, the place of this folder is good, it avoids having all cursor rules in the root and instead separate rules by context.
We should also consider adding the contribution guideline as a cursor rule as well.

But again, this is specific to cursor and it is a shame that there are no standards..
So happy to add Zed, Copilot or other IDE specific rule if someone need them.

packages/svelte/ds-app-launchpad/.storybook/main.ts

packages/svelte/ds-app-launchpad/src/lib/AwesomeButton/AwesomeButton.svelte

packages/svelte/ds-app-launchpad/src/lib/AwesomeButton/styles.css

packages/svelte/ds-app-launchpad/src/lib/AwesomeButton/types.ts

goulinkh · 2025-06-06T14:17:01Z

Hi @advl thank you for taking the time to look into this and write these valuable comments. I've applied the requested changes and left comments on some of your comments.

advl · 2025-06-10T11:18:16Z

I wonder why the tests are failing on core ?
cc @jmuzina

jmuzina · 2025-06-10T12:06:51Z

I wonder why the tests are failing on core ? cc @jmuzina

   ReferenceError: window is not defined
   ❯ src/ui/hooks/useWindowDimensions/useWindowDimensions.ts:42:22
       40|     if (isServer) return;
       41|     const handleResize = debounce(() => {
       42|       setWindowWidth(window.innerWidth);
         |                      ^
       43|       setWindowHeight(window.innerHeight);
       44|       if (onResize) onResize(result);
   ❯ Timeout._onTimeout ../../utils/src/debounce.ts:36:19
   ❯ listOnTimeout node:internal/timers:581:17
   ❯ processTimers node:internal/timers:519:7
  
  This error originated in "src/ui/Tooltip/common/TooltipArea/TooltipArea.tests.tsx" test file. It doesn't mean the error was thrown inside the file itself, but while it was running.
  This error was caught after test environment was torn down. Make sure to cancel any running tasks before test finishes:
  - cancel timeouts using clearTimeout and clearInterval
  - wait for promises to resolve using the await keyword

@advl I encountered these in #247 and found that there is an oversight in some of the tooltip-related hooks that can cause asynchronous browser API calls to continue running after the test environment has been torn down, and the browser APIs are no longer available. I added some cleanup code for debounce and useDelayedToggle that seems to have fixed the issue for me.

What's stranger is that the issue is happening in two separate PRs that have not been merged together, and is not happening on main. My guess is our loose version numbering (using ^ to accept minor and patch updates) has caused some version in the lockfile to be updated and cause our tests to fail in these PRs.

If we merge #247 , or at least the fixes introduced by the two commits I linked above, these should stop. In general, though, I think this is an indication we should be pinning dependency versions to specific versions, rather than using the ^ that bun will generate by default.

A simpler fix would be to add some manual wait calls / settimeouts to the end of the failing tests to give some time for remaining async calls to resolve before allowing the test to close.

jmuzina · 2025-06-10T13:05:44Z

Going to draft a simpler patch fix for the failing CI and send it today, to unblock

advl · 2025-06-11T14:01:19Z

@goulinkh

The promised fix is coming later today as will the review and approval
I wanted to ask you about your usage of the src/lib folder. In the react storybooks, we use the src/ui folder to store components. Is there a reason why you went for a different pattern ? @jmuzina and I thought of keeping the src/ui pattern for now, even if we are open to making it evolve to lib if we have a clear justification for it.

goulinkh · 2025-06-11T14:20:34Z

Svelte libraries have a naming convention which mostly aligns with what we have for React regarding component folder naming and the component filename,..
However the components are usually inside the folder src/lib not src/ui.

advl · 2025-06-11T15:34:52Z

@goulinkh could we possibly have src/ui for now to have a predictable equivalence with the react packages ?
I am open to changing it for all but I feel it is a discussion that lives outside of the scope of this pr

jmuzina · 2025-06-11T17:10:58Z

On first install, I ran into an infinitely waiting postinstall step for the new package:

bun install v1.2.9 (9a329c04)
  ⚙️  @canonical/svelte-d  ⚙️  @canonical/svelte-ds-app-launchpad [4/5] warn: @canonical/svelte-ds-app-launchpad's postinstall cost you 30s
warn: @canonical/svelte-ds-app-launchpad's postinstall cost you 1m1s
warn: @canonical/svelte-ds-app-launchpad's postinstall cost you 1m31.9s

Upon running the package's postinstall script playwright install --with-deps chromium myself, rather than within Lerna, I found that a sudo password prompt was being suppressed by Lerna, so I needed to manually run this myself before I could build and test the repo.

jmuzina@jmuzina-ubuntu-desktop:~/source/work/canonical/repos/ds25$ playwright install --with-deps chromium
Installing dependencies...
Switching to root user to install dependencies...
[sudo] password for jmuzina:

This also dramatically increases the time taken for CI for the entire project. See this CI run:

warn: @canonical/svelte-ds-app-launchpad's postinstall cost you 6m33.9s

I think we should avoid packages having postinstall/prepare scripts that require prompts like this, as it presents the issue of requiring us to document package-specific install instructions at the root level. Some options could be:

Adding a postinstall script for installing chromium to the root level of the monorepo - maybe this wouldn't be suppressed by Lerna but we would have to try it out.
Adding a note to the root README explaining to install Chromium with playwright beforehand
Separately installing chromium with playwright in the CI

jmuzina · 2025-06-11T17:15:47Z

I wonder why the tests are failing on core ? cc @jmuzina

@advl I believe this has been fixed

jmuzina · 2025-06-11T18:32:25Z

I believe we could speed up the CI by using a cached playwright browser, I found these articles that detail it:

Then, we could remove the postinstall script (as Playwright/chromium should already be installed in CI environment).

Then, maybe we can define some root-level dependency installation script, like install-prereqs, which is called by the root-level prepare script. That script could then define installation scripts like the chromium/playwright install, and hopefully run outside the scope of Lerna, causing CLI prompts like sudo passwords to appear to the user. This way, we can install pre-reqs like this, without requiring another setup script other than bun i.

Update: talked to @advl , we think maybe the playwright integration here should be extracted from the PR and discussed separately, to avoid blocking this PR any longer

advl

While this PR is undoubtedly an essential step in diversifying our practice in different frameworks, it does not support as well as it could the ambition of the monorepo to become a codebase for different users and teams. Your code has taken a few liberties from the standards set in other packages - in testing, in the typescript config, in the package.json commands, in the folder structure. These liberties are not bad code - but create a cognitive load and complexity that should be better introduced considering the rest of the monorepo. In other words, it would be more helpful to contribute a monorepo package you rather than a package in the monorepo - this way we would avoid readers/users to wonder why things are done in a certain way in react and differently in svelte. This would significantly support the shared purpose of this codebase.

It is important to align on the idea that this type of novel and category-defining PRs need to take into account that they are contributing to a common, shared practice, rather than a single projects. In that sense, it is essential to see an intentional control of complexity and limited divergences as a main goal more than nice to haves!

Additionally, there's a lot we can learn from this - starting by the need to document better on our side the monorepo architecture and goals, to support better all kinds of contributions, that shouldn't be as hard as the one you are doing atm ;) Thank you again for this !

advl · 2025-06-12T18:03:38Z

configs/storybook/src/createConfig.ts

+  sveltekit: {
+    storyPatterns: ["../src/**/*.stories.@(js|jsx|mjs|ts|tsx|svelte)"],
+    additionalAddons: [
+      getAbsolutePath("@storybook/addon-svelte-csf"),


What are the added features of this addon in relation with the default storybook story format ?

Could we possibly document the motivation behind this framework specific addon in the README.md of the package ?

advl · 2025-06-12T18:05:03Z

configs/storybook/src/createConfig.ts

 }

-type CreateConfigOptions = {
+const FRAMEWORK_DATA = {


I am not fully happy with this object, since the shape of framework specific options is a partial match with the storybook configuration object. It would feel more comfortable to match this more thoroughly with the StorybookConfig type

advl · 2025-06-12T18:08:12Z

packages/svelte/ds-app-launchpad/.svelte-kit/ambient.d.ts

This file should likely not be there or not in this format. It is leaking information about the setup of the person that created it.

packages/svelte/ds-app-launchpad/.svelte-kit/non-ambient.d.ts

packages/svelte/ds-app-launchpad/.svelte-kit/tsconfig.json

advl · 2025-06-12T18:17:37Z

packages/svelte/ds-app-launchpad/package.json

+    "@sveltejs/vite-plugin-svelte": "^5.0.0",
+    "@types/node": "^22.10.1",
+    "@vitest/browser": "^3.2.1",
+    "playwright": "^1.52.0",


if playwright is not needed for now, it can safely be removed (we'll add it in the relevant PR later)

advl · 2025-06-12T18:20:20Z

packages/svelte/ds-app-launchpad/tsconfig.json

@@ -0,0 +1,12 @@
+{
+  "extends": "./.svelte-kit/tsconfig.json",


This should extend one of our configs instead.

Is is important that this file is kept as lean as possible, in particular non default options. The options chosen here should be able to be applied to all the packages in the monorepo with a minimal cognitive overhead (why is this option enabled and not this one ?) - or at least the svelte packages only.

advl · 2025-06-12T18:20:38Z

packages/svelte/ds-app-launchpad/vite.config.ts

+						enabled: true,
+						provider: 'playwright',
+						instances: [{ browser: 'chromium' }]


let's disable playwright for now and follow up together on how to enable it for the whole monorepo ;)

advl · 2025-06-12T18:21:09Z

packages/svelte/ds-app-launchpad/vite.config.ts

+				test: {
+					name: 'ssr',
+					environment: 'node',
+					include: ['src/**/*.ssr.{test,spec}.{js,ts}'],
+				},
+			},


disable ssr tests for now - we'll implement a holistic approach for both react and svelte in a follow-up

advl · 2025-06-12T18:22:59Z

packages/svelte/ds-app-launchpad/vite.config.ts

+				extends: './vite.config.ts',
+				test: {
+					name: 'server',
+					environment: 'node',
+					include: ['src/**/*.{test,spec}.{js,ts}'],
+					exclude: [
+						'src/**/*.svelte.{test,spec}.{js,ts}',
+						'src/**/*.ssr.{test,spec}.{js,ts}'
+					],


Same thing here. While these features are a clear plus and worth having - I would like us to be intentional in adopting this approach in testing in all of the monorepo. So let's follow-up with this in a structured way.

advl · 2025-07-16T14:10:39Z

Closed for the time being. Shall be superseded by a vite+svelte package instead of a sveltekit packages - which diverges too much from the monorepo setup.

Referenced in #290

goulinkh added the Maintenance 🔨 label Jun 5, 2025

goulinkh requested review from advl and jmuzina June 5, 2025 13:11

goulinkh changed the title ~~Lp svelte~~ Create Svelte app-launchpad Jun 5, 2025

advl reviewed Jun 5, 2025

View reviewed changes

steciuk mentioned this pull request Jun 6, 2025

feat(svelte-generator): Generate unit test files and stories #248

Merged

5 tasks

goulinkh changed the title ~~Create Svelte app-launchpad~~ feat: Create Svelte app-launchpad Jun 6, 2025

goulinkh requested a review from advl June 6, 2025 14:15

jmuzina mentioned this pull request Jun 10, 2025

fix: fix TooltipArea tests failing after teardown #252

Merged

5 tasks

advl closed this in #252 Jun 11, 2025

advl reopened this Jun 11, 2025

advl and others added 7 commits June 12, 2025 19:27

Rebased the sb config

043742e

Bun lock file

326c279

chore: remove .gitignore

d0f0a21

refactor(app): Change body wrapper from div to main for semantic HTML

250ea2f

remove cursor rules

e4e4a38

feat(storybook): add static dirs to svelte

45e026e

chore: disable tests and rename lib folder to ui

86d74a6

advl force-pushed the lp-svelte branch from 156ed25 to 010d670 Compare June 12, 2025 17:32

-

010d670

advl added 3 commits June 12, 2025 19:39

Runs

73fa80b

Lint

3a324ef

Disabled the theming addon for now

dda5562

advl requested changes Jun 12, 2025

View reviewed changes

Removed svelte-kit files

8c4ff8a

advl mentioned this pull request Jul 16, 2025

Add packages/svelte/ds-core #290

Open

advl closed this Jul 16, 2025

PawanKolhe mentioned this pull request Oct 26, 2025

Standardize LLM rules across code editors #367

Open

Conversation

goulinkh commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Done

QA

TODO

PR readiness check

Uh oh!

advl left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

goulinkh commented Jun 6, 2025

Uh oh!

advl commented Jun 10, 2025

Uh oh!

jmuzina commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmuzina commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

advl commented Jun 11, 2025

Uh oh!

goulinkh commented Jun 11, 2025

Uh oh!

advl commented Jun 11, 2025

Uh oh!

jmuzina commented Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmuzina commented Jun 11, 2025

Uh oh!

jmuzina commented Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

advl left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

advl Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

advl commented Jul 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

goulinkh commented Jun 5, 2025 •

edited

Loading

advl left a comment •

edited

Loading

jmuzina commented Jun 10, 2025 •

edited

Loading

jmuzina commented Jun 10, 2025 •

edited

Loading

jmuzina commented Jun 11, 2025 •

edited

Loading

jmuzina commented Jun 11, 2025 •

edited

Loading

advl left a comment •

edited

Loading

advl Jun 12, 2025 •

edited

Loading