feat(web): /snippets page — pipeline-ab Arm A (BASELINE)#9
Conversation
|
🚅 Deployed to the testing-pr-9 environment in testing
|
Add shiki (dependency) for build-time syntax highlighting and vitest + jsdom (devDependencies) for the unit-test floor. Wire up test/test:watch scripts and a dedicated vitest.config.ts so pure-TS units run without the SvelteKit plugin. Station-Trailer-Version: 1
Add the pure snippet data model (typescript/svelte/shell samples) and a browser-free highlightToHtml() that uses Shiki's css-variables theme so every token color resolves through --shiki-* vars. Define those vars under :root and .dark in app.css so syntax colors flip with the theme -- no hardcoded hex in the rendered HTML. Unit tests assert the lang mapping and the hex-absence guarantee. Station-Trailer-Version: 1
Add a dependency-free toast store with full timer ownership (every setTimeout has a matching clearTimeout path; _clearAllTimers for viewport unmount) plus its Toaster viewport, and an SSR-guarded copyToClipboard that no-ops when navigator is unavailable. Tests cover auto-dismiss via fake timers, the cleanup path, and the clipboard success/SSR/reject branches. Station-Trailer-Version: 1
Add the SnippetCard (title, language badge, highlighted code, copy button with a 1.2s check-icon state and unmount timer cleanup) and the /snippets route. The load() precomputes highlighted HTML on the server so the page prerenders to static, already-highlighted output and ships zero highlighting JS to the client. The page mounts the Toaster once near its root. Station-Trailer-Version: 1
Add the Snippets link to the site-header nav array so it appears alongside Home and Components and highlights as active on the route. Station-Trailer-Version: 1
|
Warning Review limit reached
More reviews will be available in 47 minutes and 54 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR introduces a complete code snippet gallery at ChangesSnippets Gallery Feature
Sequence DiagramsequenceDiagram
participant Browser
participant PageLoad as +page.ts Load
participant Highlighter as highlightToHtml
participant PageRender as +page.svelte
participant SnippetCard as SnippetCard
participant Clipboard as copyToClipboard
Browser->>PageLoad: GET /snippets
PageLoad->>Highlighter: for each snippet
Highlighter-->>PageLoad: html (pre-rendered)
PageLoad-->>PageRender: data.snippets with html
PageRender->>SnippetCard: render snippet card keyed
SnippetCard->>Browser: display code + copy button
Browser->>SnippetCard: click copy button
SnippetCard->>Clipboard: copyToClipboard(code)
Clipboard-->>SnippetCard: true/false
SnippetCard->>Browser: toast notification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
…of the client bundle A universal +page.ts load pulls its import graph ($lib/highlight -> shiki) into the CLIENT bundle. The built snippets node chunk was 232KB and shipped the Shiki runtime (createHighlighter/codeToHtml) plus the oniguruma WASM engine to the browser, contradicting the design intent of build-time-only highlighting. Renaming to +page.server.ts makes load server-only (still runs at prerender), so the highlighter never enters the client graph. The route chunk drops to 8KB and the browser receives only the precomputed static HTML. Verified: npm run check / test (16) / build all green; shiki runtime absent from .svelte-kit/output/client, present only in the server build. Station-Trailer-Version: 1
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/src/lib/components/toaster.svelte`:
- Around line 8-10: The effect cleanup currently only cancels timers via
toast._clearAllTimers(), leaving toast entries in the store; update the cleanup
returned from $effect to both cancel timers and clear toast state (e.g., call
toast._clearAllTimers(); then toast.clear() or the library's clearAll/clear
method) so toast entries are removed on unmount; modify the cleanup inside the
$effect that references toast._clearAllTimers() to also invoke the toast store's
clear method (toast.clear() / toast.clearAll()).
In `@web/src/lib/highlight.ts`:
- Around line 31-37: The cached promise _hp currently gets permanently set to a
rejected promise by getHighlighter(); change getHighlighter() so when assigning
(_hp = createHighlighter(...)) you attach a .catch handler that resets _hp to
null and rethrows the error (so transient init failures don't poison the cache);
ensure the handler references the same _hp variable and that highlightToHtml
callers still receive the original rejection when appropriate.
In `@web/src/routes/snippets/`+page.ts:
- Around line 1-15: The universal load() in +page.ts is using highlightToHtml
(which imports shiki at module scope) causing Shiki to be bundled to the client;
move the server-only highlighting to a server load: create +page.server.ts with
an exported load() that imports highlightToHtml (or move $lib/highlight.ts →
$lib/server/highlight.server.ts) and performs the Promise.all mapping (function
name: load, helper: highlightToHtml, data: snippets), then have +page.ts export
only a client-safe load or no load so the Shiki module is never imported in the
universal bundle.
In `@web/vitest.config.ts`:
- Line 14: Replace the Windows-unsafe new URL(...).pathname usage in the Vitest
alias by importing fileURLToPath from 'url' and using fileURLToPath(new
URL('./src/lib/', import.meta.url)) for the $lib alias; update the alias object
(alias: { $lib: ... }) to call fileURLToPath and add the fileURLToPath import at
the top of web/vitest.config.ts so path resolution is cross-platform-safe.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5a1617ee-08d1-4e98-84fa-d73828d22ce5
⛔ Files ignored due to path filters (1)
web/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
web/package.jsonweb/src/app.cssweb/src/lib/clipboard.test.tsweb/src/lib/clipboard.tsweb/src/lib/components/site-header.svelteweb/src/lib/components/snippet-card.svelteweb/src/lib/components/toaster.svelteweb/src/lib/highlight.test.tsweb/src/lib/highlight.tsweb/src/lib/snippets.test.tsweb/src/lib/snippets.tsweb/src/lib/toast.test.tsweb/src/lib/toast.tsweb/src/routes/snippets/+page.svelteweb/src/routes/snippets/+page.tsweb/vitest.config.ts
| $effect(() => { | ||
| return () => toast._clearAllTimers(); | ||
| }); |
There was a problem hiding this comment.
Clear toast state on unmount; timer-only cleanup can leave stale toasts.
Line 9 cancels timers but does not remove toast entries from the store. Because <Toaster /> is page-scoped (web/src/routes/snippets/+page.svelte), navigating away before TTL can leave old toasts persisted and re-rendered on remount.
💡 Suggested fix
<script lang="ts">
import { toast } from '$lib/toast';
+ import { get } from 'svelte/store';
import { fly, fade } from 'svelte/transition';
@@
$effect(() => {
- return () => toast._clearAllTimers();
+ return () => {
+ for (const t of get({ subscribe: toast.subscribe })) {
+ toast.dismiss(t.id);
+ }
+ };
});
</script>📝 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.
| $effect(() => { | |
| return () => toast._clearAllTimers(); | |
| }); | |
| $effect(() => { | |
| return () => { | |
| for (const t of get({ subscribe: toast.subscribe })) { | |
| toast.dismiss(t.id); | |
| } | |
| }; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/lib/components/toaster.svelte` around lines 8 - 10, The effect
cleanup currently only cancels timers via toast._clearAllTimers(), leaving toast
entries in the store; update the cleanup returned from $effect to both cancel
timers and clear toast state (e.g., call toast._clearAllTimers(); then
toast.clear() or the library's clearAll/clear method) so toast entries are
removed on unmount; modify the cleanup inside the $effect that references
toast._clearAllTimers() to also invoke the toast store's clear method
(toast.clear() / toast.clearAll()).
| let _hp: Promise<Highlighter> | null = null; | ||
|
|
||
| function getHighlighter(): Promise<Highlighter> { | ||
| return (_hp ??= createHighlighter({ | ||
| themes: [cssVariablesTheme], | ||
| langs: ['typescript', 'svelte', 'shellscript'], | ||
| })); |
There was a problem hiding this comment.
Reset cached highlighter promise on initialization failure.
At Line 34, a single rejected createHighlighter(...) call permanently poisons _hp, so all future highlightToHtml calls fail even after transient startup errors.
Suggested fix
let _hp: Promise<Highlighter> | null = null;
function getHighlighter(): Promise<Highlighter> {
- return (_hp ??= createHighlighter({
- themes: [cssVariablesTheme],
- langs: ['typescript', 'svelte', 'shellscript'],
- }));
+ if (!_hp) {
+ _hp = createHighlighter({
+ themes: [cssVariablesTheme],
+ langs: ['typescript', 'svelte', 'shellscript'],
+ }).catch((err) => {
+ _hp = null;
+ throw err;
+ });
+ }
+ return _hp;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/lib/highlight.ts` around lines 31 - 37, The cached promise _hp
currently gets permanently set to a rejected promise by getHighlighter(); change
getHighlighter() so when assigning (_hp = createHighlighter(...)) you attach a
.catch handler that resets _hp to null and rethrows the error (so transient init
failures don't poison the cache); ensure the handler references the same _hp
variable and that highlightToHtml callers still receive the original rejection
when appropriate.
| resolve: { | ||
| // Resolve `$lib/...` imports without pulling in the SvelteKit plugin, so pure-TS | ||
| // tests stay fast and avoid `$app/*` resolution issues. | ||
| alias: { $lib: new URL('./src/lib/', import.meta.url).pathname }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify current alias-path pattern and whether Windows CI is in scope.
set -euo pipefail
echo "1) Find pathname-based URL alias conversions:"
rg -n --type ts "new URL\\(.*import\\.meta\\.url\\)\\.pathname"
echo
echo "2) Check for Windows runners in GitHub Actions workflows (if present):"
if [ -d ".github/workflows" ]; then
rg -n --iglob '*.yml' --iglob '*.yaml' 'windows|win32|matrix' .github/workflows || true
else
echo "No .github/workflows directory found."
fiRepository: startino/testing
Length of output: 308
Fix $lib alias path conversion to be Windows-safe
web/vitest.config.ts:14 uses new URL(...).pathname for a filesystem path; this isn’t cross-platform safe on Windows. Switch to fileURLToPath() for reliable path resolution in Vitest.
Proposed fix
import { defineConfig } from 'vitest/config';
+import { fileURLToPath } from 'node:url';
export default defineConfig({
test: {
@@
resolve: {
@@
- alias: { $lib: new URL('./src/lib/', import.meta.url).pathname },
+ alias: { $lib: fileURLToPath(new URL('./src/lib/', import.meta.url)) },
},
});📝 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.
| alias: { $lib: new URL('./src/lib/', import.meta.url).pathname }, | |
| import { defineConfig } from 'vitest/config'; | |
| import { fileURLToPath } from 'node:url'; | |
| export default defineConfig({ | |
| test: { | |
| environment: 'node', | |
| include: 'src/**/*.{test,spec}.ts', | |
| }, | |
| resolve: { | |
| alias: { $lib: fileURLToPath(new URL('./src/lib/', import.meta.url)) }, | |
| }, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/vitest.config.ts` at line 14, Replace the Windows-unsafe new
URL(...).pathname usage in the Vitest alias by importing fileURLToPath from
'url' and using fileURLToPath(new URL('./src/lib/', import.meta.url)) for the
$lib alias; update the alias object (alias: { $lib: ... }) to call fileURLToPath
and add the fileURLToPath import at the top of web/vitest.config.ts so path
resolution is cross-platform-safe.
Final review (advisory -- not a merge gate)Arm A (BASELINE) -- review report: /snippetsAdvisory red-team over every commit since origin/alpha. Reviewed the complete VerdictStrong, faithful implementation of the plan. One real quality finding Lens 1 -- Spec gapsNo material gaps. The change satisfies the plan and the original ask end to end:
Note (not a gap): the plan's §3a/§3b text prescribed a universal Lens 2 -- Standards violationsNone blocking.
Minor (judgment calls, left):
Lens 3 -- Quality problemsFINDING (acted-on): Shiki leaked into the CLIENT bundle.Root cause: the route loaded via a UNIVERSAL
Fix applied: renamed Post-fix verification (rebuilt):
Other quality checks -- clean
Gate after fixPASS -- check (0/0), test (16/16), build all green; prettier clean. Commit
|
Arm A (BASELINE) -- review dispositionAdvisory only -- never blocks the ship. findings-count: 4 ACTED-ON (cheap, clearly-correct, behavior-preserving)
LEFT (judgment calls -- one-line reason each)
PASS-2 (report-only, post-fix)Re-read the renamed |
Known issues from QA (George -- baseline arm exploratory pass)Surfaced by exploratory QA, triaged unblock-with-note (kept for the operator to eyeball; not auto-fixed so the two experiment arms stay comparable):
These are the baseline arm's QA yield -- part of the experiment artifact, intentionally left visible. |
1 participant
EXPERIMENT PR — DO NOT MERGE. pipeline-ab A/B experiment artifact (Arm A (BASELINE)).
Baseline
featurelayout: Opus hands + full mid-process prime<->linus review loop + George QA + review-reconcile.Both arms build the IDENTICAL
/snippetsfeature from ONE shared plan. This PR stays OPEN for the operator to evaluate by eye against its live Railway preview. Implementation commits land here next.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by CodeRabbit