chore(examples,e2e): replace nextjs app with vite showcase - #513
Conversation
Code Metrics Report
Details | | main (1e3f9e5) | #513 (081fdac) | +/- |
|---------------------|----------------|----------------|------|
| Coverage | 87.2% | 87.2% | 0.0% |
| Files | 198 | 198 | 0 |
| Lines | 5840 | 5840 | 0 |
| Covered | 5093 | 5093 | 0 |
+ | Test Execution Time | 2m13s | 2m4s | -9s |Code coverage of files in pull request scope (100.0% → 100.0%)
Reported by octocov |
|
@copilot resolve the merge conflicts in this pull request |
|
@copilot fix CI errors |
|
@copilot resolve the merge conflicts in this pull request |
|
@copilot why CI has failed due to the diff in snapshots? |
|
|
@copilot resolve the merge conflicts in this pull request |
|
@copilot resolve the merge conflicts in this pull request |
| "@types/react-dom": ^19 | ||
| "@vitejs/plugin-react": ^6.1.0 | ||
| lucide-react: ^1.8.0 | ||
| lucide-react: 1.8.0 |
There was a problem hiding this comment.
oxlint is pinned so the lint baseline does not widen unexpectedly. A newer release introduced additional rules and caused lint failures; adopting those rule changes is being handled separately in #525.
lucide-react is pinned so third-party icon SVG updates do not create unrelated snapshot diffs in packages/core.
I'm unsure where I sit on this. The reasoning to pin these seems like avoiding the sort of routine maintenance that comes with using a package. If there is a bug in a lucide-react component that gets fixed (which would affect snapshots), we miss it, for the "benefit" of not having to update snapshots.
PR #525 seems to contradict the purpose of pinning — it updates oxlint as well as the code that is required to satisfy it (at least as I understand it). If we do not want to be updating code, then we lock a version, if we want to update code to align with oxlint's advised best practices, then should we leave the dependency unpinned and address the warnings when they arise?
#525 and pinning seem to have different points of view
We have inconsistency between these two packages and other dependencies on whether we are pinning exact or whether we automatically take minor version bumps or not
There was a problem hiding this comment.
The pins are intentional: they keep the lint and snapshot baselines stable, so unrelated installs do not introduce new rules or third-party SVG diffs. Renovate is configured to update pinned dependencies as default, so updates still arrive as explicit PRs.
PR #525 follows that model: it deliberately bumps the pinned oxlint version and includes the required rule-adoption changes. Leaving ranges would allow these changes to land incidentally during unrelated work, which is what we want to avoid.
There was a problem hiding this comment.
Will we just update oxlint when there are renovate requests to do so?
On a side note, It's not great that that their minor bumps made changes to rules!
There was a problem hiding this comment.
Yes. We will update the pinned version through Renovate PRs, reviewing any resulting rule changes deliberately rather than receiving them during unrelated installs. If an update needs rule adoption or other non-routine work, we will close the Renovate PR and handle the version bump and required changes together in a dedicated PR.
| AlertTokensPage.appShellPageProps = { | ||
| meta: { | ||
| title: "Alert tokens", | ||
| icon: <MegaphoneIcon size={16} />, |
There was a problem hiding this comment.
Not sure why we are losing the icons
There was a problem hiding this comment.
Restored the page icon in a820353. It had been removed while making sidebar item presentation consistent, but it is appropriate for this page.
There was a problem hiding this comment.
@IzumiSy sorry there are a bunch across different existing pages. I only commented on one of the ones that were removed
There was a problem hiding this comment.
Restored the remaining existing Vite page icons in abd4dbb: DataTable Lab, DataTable + Filters, DatePicker, and Document Progress. Thank you for calling out the wider scope.
interacsean
left a comment
interacsean
left a comment
There was a problem hiding this comment.
I wonder if its worth moving most of the existing pages to within 'showcase', I think they serve the same purpose
Agreed, so implemented in de6eac4b. The component demo pages now live under |
Co-authored-by: IzumiSy <982850+IzumiSy@users.noreply.github.com>
de6eac4 to
c66a35b
Compare
PR #536 supersedes this branch's guidance for the bundled skill, so its version wins on every file the two touch: catalogue/src/fundamental/{components,design-system}.md catalogue/src/pattern/detail/hero-with-actions/{PATTERN.md,*.tsx} catalogue/src/pattern/form/composer/PATTERN.md packages/core/src/components/layout/types.ts Theirs is the better line: rather than teaching a specificity escape hatch, it removes the need for one — table-in-card renders Table.Root directly after the card header instead of overriding Card.Content's padding, and Layout.gap stays a supported prop rather than being deprecated toward a className override. It also ships the rule as lint (@tailor-platform/eslint-plugin-app-shell, no-astw-prefix). examples/nextjs-app was deleted upstream (#513, #532) in favour of the Vite showcase, so its four conflicting files are dropped. Git matched the rest by rename, carrying this branch's conversions into examples/vite-app/src/{pages/showcase/{colors,grid,metric-card},showcase/form-demo}.tsx — including the items-stretch! fix from the adversarial-review round. Retained from this branch: the #1652 prefix-order fix in four components, its regression guard, docs/**, and the ActionPanel JSDoc. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Motivation
examples/nextjs-apphad grown into two different things at once:That made the Next example much heavier than necessary for the actual compatibility contract we want to keep. The goal of this change is to keep Next.js support covered at the smallest useful level, while consolidating showcase-oriented content into the Vite example that already serves as the main interactive demo app.
Design Decision
Split showcase coverage from framework coverage
The Vite example is now the single showcase app. Routes migrated from the old Next.js example now live under
/showcase/*, and a small landing page links both to those migrated pages and to existing Vite pages that already cover the same components.This keeps the broad demo surface in one place instead of maintaining it twice through two different routing models.
Keep Next.js coverage as a minimal smoke fixture
Instead of a full
examples/nextjs-app, this PR addse2e/tests/nextjs-smoke/app, a minimal Next.js App Router fixture that still usesdefineModule()anddefineResource().The smoke suite covers the integration points that matter for Next.js support:
That keeps the Next-specific contract covered without carrying a full showcase app.
Pin drifting dependencies that affect repo baselines
This PR also pins two dependencies that were previously allowed to float within a range:
oxlintis pinned so the lint baseline does not widen unexpectedly. A newer release introduced additional rules and caused lint failures; adopting those rule changes is being handled separately in refactor(core): align state ownership with React Compiler #525.lucide-reactis pinned so third-party icon SVG updates do not create unrelated snapshot diffs inpackages/core.Summary
examples/nextjs-appexamples/vite-app, grouped under/showcase/*nextjs-smokeE2E fixture and Playwright project for Next.js App Router compatibilityoxlintandlucide-reactto keep the current lint and snapshot baselines stable