fix(web,ci): issue #43 batch P1 — security + correctness (7 items) - #60
Merged
Conversation
7 findings from the CodeRabbit review on PR #41 / issue #43. Each is a one-file fix; bundled because they share the same scope (security / correctness) and the issue itself asked to batch P1 together. ## Changes - `.github/workflows/ci-web.yml` — `actions/checkout` step grows `persist-credentials: false`. Drops the GITHUB_TOKEN from `.git/config` after checkout — nothing in the job pushes or makes authenticated git calls, so the token would be pure blast radius if a downstream step gets compromised. - `.github/workflows/label-pr.yml` — drop workflow-level `pull-requests: write`; grant it per job (scope / type / size) so only the steps that actually mutate labels carry the privilege. Add a top-level `concurrency` block keyed on PR number — five fast rebases collapse to one labeler run instead of five overlapping races. - `web/src/server-fns/session.ts` — wrap `auth.api.getSession` in try/catch. Better Auth throws `APIError('UNAUTHORIZED')` for expired / missing cookies, which is a normal "not logged in" state the `beforeLoad` consumer treats as `null`. Rethrow everything else so DB / network failures still surface. - `web/src/routes/_authed.profiles.$profileId.libraries.$libraryId.artists.tsx` and `…playlists.tsx` — loaders no longer echo `err.message` back to the UI. Log the raw error server-side via `console.error`, return a stable generic message. Same shape on both loaders; the fix is mechanically identical. - `web/src/lib/db.ts` — tighten `BETTER_AUTH_DB_MAX` parse to `/^[1-9]\d*$/` so `'10foo'` / `'1e3'` / `' 5 '` fail loud at boot instead of silently dropping to `10` / `1` / `5`. - `web/src/components/ThemeToggle.tsx` — wrap both `localStorage.getItem` and `setItem` in try/catch. Safari's private browsing + some embedded WebViews throw quota / disabled-storage errors that would otherwise crash the toggle's render path. Falling back to `'auto'` keeps the UI responsive in those contexts. - `web/.gitignore` — `*.env*` with `!.env.example` negation. Catches `.env.production` / `.env.staging` / any other variant the prior `.env` / `.env.local` / `.env.*.local` trio missed. ## Validation - `bun run typecheck` clean. - `bun run lint` clean. - `bun run test` — 233 pass across 26 files. - `bun run build` builds the SSR + Nitro bundle clean. - `prettier --check` on the 5 modified web files — clean (the pre-existing 104-file format drift on `main` is out of scope). ## Refs Issue #43 — P1 batch (7/7). P2 (UX/a11y) and P3+P4 (tooling/tests/labeler) follow in separate PRs per the issue's "≤ 200 LOC" guidance. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
|
Warning Review limit reached
More reviews will be available in 1 hour, 18 minutes, and 53 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughCe PR applique un ensemble de corrections de sécurité et de robustesse : suppression des credentials git après checkout CI, permissions minimales dans les workflows, validation stricte de la variable DB via regex, gestion explicite de ModificationsSécurité CI, configuration et robustesse applicative
🎯 Estimation de l'effort de revue🎯 2 (Simple) | ⏱️ ~10 minutes Issues potentiellement liées
Labels suggérés
Poème
🚥 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)
Comment |
github-actions
Bot
added
type: fix
This was referenced Jun 15, 2026
Owner
Author
|
@coderabbitai full review please. |
23 tasks
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== PR `#60` diff ==="
gh pr view 60 --repo InstaZDLL/waveflow-server --json files,additions,deletions,title,body
echo ""
echo "=== Full diff ==="
gh pr diff 60 --repo InstaZDLL/waveflow-serverLength of output: 12571 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== Check for .envrc usage ==="
fd -H '.envrc' 2>/dev/null || echo "No .envrc found"
echo ""
echo "=== better-auth APIError: check status type ==="
fd -e ts -e d.ts . node_modules/better-auth 2>/dev/null | xargs grep -l 'APIError' 2>/dev/null | head -5 || echo "node_modules not present in sandbox"
echo ""
echo "=== Check existing .env files tracked by git ==="
git ls-files | grep -i 'env' || echo "None"
echo ""
echo "=== Full session.ts current state ==="
cat web/src/server-fns/session.ts
echo ""
echo "=== Full db.ts current state ==="
cat web/src/lib/db.ts
echo ""
echo "=== ThemeToggle.tsx full ==="
cat web/src/components/ThemeToggle.tsx
echo ""
echo "=== Check web/.gitignore for context ==="
cat web/.gitignore
echo ""
echo "=== Check if .env.example exists ==="
fd -H '.env.example' . || echo "No .env.example found"
echo ""
echo "=== label-pr.yml full ==="
cat .github/workflows/label-pr.yml
echo ""
echo "=== Check better-auth version ==="
cat web/package.json | python3 -c "import sys,json; p=json.load(sys.stdin); deps={**p.get('dependencies',{}), **p.get('devDependencies',{})}; print('better-auth:', deps.get('better-auth','not found'))"Length of output: 13431
Review of PR
|
| File | Finding | Severity |
|---|---|---|
web/.gitignore |
*.env* would silently ignore .envrc if added; consider .env + .env.* instead |
Low |
| All others | Correct | ✅ |
The one actionable item is the .gitignore pattern. Everything else is solid — good batch.
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
github-actions
Bot
added
scope: web
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Follow-up to #60 review. The original `*.env*` pattern caught every `.env*` variant we wanted, but as a side effect it also silently ignored `.envrc` — a standard direnv file that a future contributor might reasonably add to the repo. No `.envrc` exists today (verified via git ls-files + filesystem check), so this is prophylactic rather than a current bug. `.env` + `.env.*` is the tighter pair: it matches `.env`, `.env.local`, `.env.production`, `.env.staging`, etc., while leaving `.envrc` (and any other non-`.` suffix) unaffected. The `!.env.example` negation stays so the bootstrap template still commits. Validation: `git check-ignore -v .envrc` returns nothing (not ignored); `.env.production` / `.env.staging` are still ignored. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
Owner
Author
|
@coderabbitai fix appliqué sur commit a640fa1. Switch |
|
Tip For best results, initiate chat on the files or code changes.
(ˆ⌣ˆ) |
InstaZDLL
added a commit
that referenced
this pull request
Jun 15, 2026
P2 batch from CodeRabbit on PR #41 / issue #43. 4 of the 5 P2 items applied; the 5th (styles.css OKLCH unification) is deferred — see § below. ## Changes - `web/src/components/PlayerBar.tsx` — `onPointerCancel` now commits the pending `seekScrub` instead of dropping it. The user already expressed intent by dragging; a system-initiated cancel (notification focus steal, gesture-to-scroll promotion) shouldn't lose that input. - `web/src/lib/use-focus-trap.ts` — when `items.length === 0`, stamp a transient `tabindex="-1"` on the container before `.focus()` so `HTMLElement.focus()` actually runs (it silently no-ops on a non-tabbable element, which would let Tab fall through to the page underneath). Restore the prior attribute state after focus so the DOM contract stays intact for subsequent renders. - `web/src/routes/_authed.profiles.tsx` — pre-format `last_used_at` in the loader via `Intl.DateTimeFormat('en-US', { timeZone: 'UTC' })`. Drops the SSR ↔ client divergence that `new Date(...).toLocaleDateString()` produced (Node defaults to system locale + TZ, browser uses the user's — React then logs a hydration mismatch and briefly flickers the wrong format). The loader also now logs raw err + surfaces a generic message, matching the artists / playlists loader fix from the P1 batch. - `web/src/components/Footer.tsx` — display string was "AGPL-3.0", the licence file is "AGPL-3.0-only". Mirror the SPDX identifier. ## Deferred — styles.css OKLCH unification The CodeRabbit finding asks the `@theme` block + `:root` fallback in `styles.css` to consume the OKLCH tokens from `@waveflow/design-tokens` instead of redefining `--surface`, `--surface-strong`, `--line`, etc. in `rgba(...)` locally. In practice this is a real refactor, not a one-liner: - `design-tokens` currently emits the four base surface HEX values (`--color-surface-{dark,light}{,-elevated}`). The translucent overlay variables (`--surface`, `--line`, `--chip-bg`, …) are NOT in the design-tokens public surface today. - Pulling them in needs one of: - Extend `design-tokens` to emit translucent overlays per preset (Sprint-level work; touches every preset definition). - Or rewrite `styles.css` to derive them via `oklch(from var(--color-surface-light) l c h / 0.72)` — works in Chromium 125+, Firefox 128+, Safari 17.2+; OK against the WaveFlow web's target matrix but worth a deliberate design decision. Logging this for a follow-up PR rather than slipping a half-finished refactor through the P2 batch. The other 4 P2 items ship now. ## Validation - `bun run typecheck` clean. - `bun run lint` clean. - `bun run test` — 233 pass across 26 files. - `prettier --check` on the 4 modified files clean. ## Refs Closes 4/24 items of #43 (11/24 cumulative with P1 batch in #60). P3+P4 ride in a separate PR. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
InstaZDLL
added a commit
that referenced
this pull request
Jun 15, 2026
7 findings from CodeRabbit on PR #41 / issue #43 — the P3 tooling batch (6 items) + the single P4 labeler item, bundled because they all touch dev / CI infrastructure rather than runtime code. ## Changes - `web/scripts/db-migrate.ts` — resolve `MIGRATIONS_DIR` relative to the script via `fileURLToPath(import.meta.url)` instead of `process.cwd()`. After the monorepo merge the script can be invoked from the repo root (`bun --cwd=web run db:migrate`); the previous CWD anchor broke silently because it pointed at `<repo>/db/migrations`, which doesn't exist. - `web/vitest.config.ts` — `passWithNoTests: false`. The flag was a bootstrap-era convenience while the suite was empty; now that 230+ tests are landing, a typo in the `include` glob (or an accidental file move that breaks discovery) would silently pass CI under `true`. Failing loud is the right signal. - `web/src/routes/sign-up.test.tsx` — three new cases: - empty / whitespace-only display name → blocked - password at MAX_PASSWORD + 1 (129 chars) → blocked - surrounding whitespace on name + email → trimmed before `signUp.email` call - Six route-adjacent test files (`sign-up.test.tsx`, `_authed.profiles.$profileId.libraries.$libraryId.albums*`, `…artists*`, `…playlists.test.tsx`, `…playlists.$playlistId.test.tsx`) — switch `React.PropsWithChildren` (which referenced the global `React` namespace without an `import * as React`) to a typed `PropsWithChildren` imported via `import type { PropsWithChildren } from 'react'`. The tests passed before only because the JSX transform happens to expose `React` ambient; the explicit import locks the contract and is what every other test file in the suite already does. - `web/src/routes/sign-in.test.ts` → `-sign-in.test.ts` — TanStack Router's `-`-prefix convention excludes the file from the auto-generated route tree. Without the rename the router plugin had been incidentally treating `sign-in.test.ts` as a candidate route module on every dev reload. Kept `.ts` (not `.tsx`) because the test doesn't render JSX (`Link: () => null` mock). - `.github/pull_request_template.md` — first heading `## Summary` → `# Summary`. MD041 (first-line-heading) wants H1; the template was the only file in the repo tripping it. - `.github/labeler.yml` — `web/src/routes/sign-in/**` / `…sign-up/**` only matched DIRECTORIES; the actual routes are `sign-in.tsx` / `sign-up.tsx` files at the root of `routes/`. Switch to `sign-in{,/**}` / `sign-up{,/**}` so a PR touching either form lights up the auth label. ## Validation - `bun run typecheck` clean. - `bun run lint` clean. - `bun run test` — 236 pass across 26 files (was 233, +3 new sign-up boundary cases). - `prettier --check` on the modified files clean. ## Refs Closes 7/24 items of #43. Cumulative: 18/24 across the three batched PRs (#60 P1 + #61 P2 + this). The 4 P0 items + 1 deferred P2 (styles.css OKLCH unification) remain. P0 was tagged in the issue as "ship with the archive PR" — verifying status as a follow-up. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
InstaZDLL
added a commit
that referenced
this pull request
Jun 15, 2026
7 findings from CodeRabbit on PR #41 / issue #43 — the P3 tooling batch (6 items) + the single P4 labeler item, bundled because they all touch dev / CI infrastructure rather than runtime code. ## Changes - `web/scripts/db-migrate.ts` — resolve `MIGRATIONS_DIR` relative to the script via `fileURLToPath(import.meta.url)` instead of `process.cwd()`. After the monorepo merge the script can be invoked from the repo root (`bun --cwd=web run db:migrate`); the previous CWD anchor broke silently because it pointed at `<repo>/db/migrations`, which doesn't exist. - `web/vitest.config.ts` — `passWithNoTests: false`. The flag was a bootstrap-era convenience while the suite was empty; now that 230+ tests are landing, a typo in the `include` glob (or an accidental file move that breaks discovery) would silently pass CI under `true`. Failing loud is the right signal. - `web/src/routes/sign-up.test.tsx` — three new cases: - empty / whitespace-only display name → blocked - password at MAX_PASSWORD + 1 (129 chars) → blocked - surrounding whitespace on name + email → trimmed before `signUp.email` call - Six route-adjacent test files (`sign-up.test.tsx`, `_authed.profiles.$profileId.libraries.$libraryId.albums*`, `…artists*`, `…playlists.test.tsx`, `…playlists.$playlistId.test.tsx`) — switch `React.PropsWithChildren` (which referenced the global `React` namespace without an `import * as React`) to a typed `PropsWithChildren` imported via `import type { PropsWithChildren } from 'react'`. The tests passed before only because the JSX transform happens to expose `React` ambient; the explicit import locks the contract and is what every other test file in the suite already does. - `web/src/routes/sign-in.test.ts` → `-sign-in.test.ts` — TanStack Router's `-`-prefix convention excludes the file from the auto-generated route tree. Without the rename the router plugin had been incidentally treating `sign-in.test.ts` as a candidate route module on every dev reload. Kept `.ts` (not `.tsx`) because the test doesn't render JSX (`Link: () => null` mock). - `.github/pull_request_template.md` — first heading `## Summary` → `# Summary`. MD041 (first-line-heading) wants H1; the template was the only file in the repo tripping it. - `.github/labeler.yml` — `web/src/routes/sign-in/**` / `…sign-up/**` only matched DIRECTORIES; the actual routes are `sign-in.tsx` / `sign-up.tsx` files at the root of `routes/`. Switch to `sign-in{,/**}` / `sign-up{,/**}` so a PR touching either form lights up the auth label. ## Validation - `bun run typecheck` clean. - `bun run lint` clean. - `bun run test` — 236 pass across 26 files (was 233, +3 new sign-up boundary cases). - `prettier --check` on the modified files clean. ## Refs Closes 7/24 items of #43. Cumulative: 18/24 across the three batched PRs (#60 P1 + #61 P2 + this). The 4 P0 items + 1 deferred P2 (styles.css OKLCH unification) remain. P0 was tagged in the issue as "ship with the archive PR" — verifying status as a follow-up. Signed-off-by: InstaZDLL <github.105mh@8shield.net>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
7 P1 findings from CodeRabbit on PR #41 / issue #43, bundled per
the issue's "batch P1 together" guidance.
Changes
.github/workflows/ci-web.ymlactions/checkout:persist-credentials: false.github/workflows/label-pr.ymlpull-requests: write(grant per-job), addconcurrencyblock keyed on PR numberweb/src/server-fns/session.tsauth.api.getSession, returnnullonAPIError('UNAUTHORIZED'), rethrow othersweb/src/routes/_authed.profiles.$profileId.libraries.$libraryId.artists.tsxweb/src/routes/_authed.profiles.$profileId.playlists.tsxweb/src/lib/db.tsBETTER_AUTH_DB_MAXparse to/^[1-9]\d*$/so'10foo'fails loudweb/src/components/ThemeToggle.tsxlocalStorageget/set — Safari private + WebView edge casesweb/.gitignore*.env*+!.env.exampleso.env.production/.env.stagingcan't slip throughTest plan
bun run typecheckcleanbun run lintcleanbun run test— 233 pass across 26 filesbun run build— SSR + Nitro bundle builds cleanprettier --checkon the 5 modified web files cleanRefs
Closes 7/24 items of #43. P2 (UX/a11y) and P3+P4 (tooling/tests/
labeler) ride in separate PRs to keep each review ≤200 LOC per
the issue's batching guidance.
Summary by CodeRabbit
Bug Fixes
Chores