You can not select more than 25 topics
Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.
666 lines
26 KiB
666 lines
26 KiB
# Comprehensive Codebase Audit Report (v2) |
|
|
|
**Project:** root-org (SvelteKit + Supabase + Tailwind v4) |
|
**Date:** 2026-02-06 (updated) |
|
**Auditor:** Cascade |
|
|
|
> **Changes since v1:** Dead stores (auth, organizations, documents, kanban, theme) deleted. `OrgWithRole` moved to `$lib/api/organizations.ts`. `FileTree` removed. Documents pages refactored into shared `FileBrowser` component. Document locking added (`document-locks` API + migration). Calendar `$derived` bugs fixed. `buildDocumentTree`/`DocumentWithChildren` removed. Editor CSS typo fixed. Invite page routes corrected. KanbanBoard button label fixed. |
|
|
|
--- |
|
|
|
## 1. Security |
|
|
|
### S-1 · **CRITICAL** — Real credentials committed to `.env` in git |
|
|
|
**File:** `.env:1-4` |
|
|
|
``` |
|
PUBLIC_SUPABASE_URL=https://zlworzrghsrokdkuckez.supabase.co |
|
PUBLIC_SUPABASE_ANON_KEY=sb_publishable_UDoCgcmpUeE5d-jocBSdVw_TWzDxK3x |
|
GOOGLE_API_KEY=AIzaSyAn2LnXkgwyLcTHQPt3nbFhBwnYWosmMT0 |
|
``` |
|
|
|
**Problem:** The `.env` file contains real Supabase and Google API keys and is tracked by git. Anyone with repo access (or if the repo is public) has full access to your Supabase project and Google API quota. |
|
|
|
**Fix:** |
|
1. Add `.env` to `.gitignore` (it's already there — but the file was committed before the gitignore rule was added). |
|
2. Run `git rm --cached .env` to untrack it. |
|
3. **Rotate all three keys immediately** — they are compromised the moment they entered git history. |
|
4. Use `git filter-branch` or `git-filter-repo` to purge `.env` from history. |
|
|
|
--- |
|
|
|
### S-2 · **CRITICAL** — Google Calendar API route has no authentication check |
|
|
|
**File:** `src/routes/api/google-calendar/events/+server.ts:7-9` |
|
|
|
```ts |
|
export const GET: RequestHandler = async ({ url, locals }) => { |
|
const orgId = url.searchParams.get('org_id'); |
|
``` |
|
|
|
**Problem:** The endpoint accepts any `org_id` parameter and fetches calendar data without verifying the requesting user is a member of that org. An unauthenticated user can enumerate org IDs and read all connected Google Calendar events. |
|
|
|
**Fix:** Add session + membership check: |
|
```ts |
|
const { session, user } = await locals.safeGetSession(); |
|
if (!session || !user) return json({ error: 'Unauthorized' }, { status: 401 }); |
|
|
|
const { data: membership } = await locals.supabase |
|
.from('org_members') |
|
.select('id') |
|
.eq('org_id', orgId) |
|
.eq('user_id', user.id) |
|
.single(); |
|
|
|
if (!membership) return json({ error: 'Forbidden' }, { status: 403 }); |
|
``` |
|
|
|
--- |
|
|
|
### S-3 · **HIGH** — Auth callback open redirect vulnerability |
|
|
|
**File:** `src/routes/auth/callback/+server.ts:6` |
|
|
|
```ts |
|
const next = url.searchParams.get('next') ?? url.searchParams.get('redirect') ?? '/'; |
|
``` |
|
|
|
**Problem:** The `next`/`redirect` parameter is used directly in `redirect(303, next)` without validating it's a relative URL. An attacker can craft `?next=https://evil.com` to redirect users after login. |
|
|
|
**Fix:** Validate the redirect target is a relative path: |
|
```ts |
|
function safeRedirect(target: string): string { |
|
if (target.startsWith('/') && !target.startsWith('//')) return target; |
|
return '/'; |
|
} |
|
const next = safeRedirect(url.searchParams.get('next') ?? url.searchParams.get('redirect') ?? '/'); |
|
``` |
|
|
|
--- |
|
|
|
### S-4 · **HIGH** — Settings page performs destructive operations without server-side authorization |
|
|
|
**File:** `src/routes/[orgSlug]/settings/+page.svelte:186-463` |
|
|
|
**Problem:** All settings mutations (update org, delete org, invite/remove members, manage roles, connect calendar) are done via direct Supabase client calls from the browser. The only server-side check is the page load guard (`owner`/`admin`). If RLS policies are not perfectly configured, any authenticated user could call these mutations directly. |
|
|
|
**Fix:** Move destructive operations to SvelteKit form actions or API routes with explicit server-side authorization checks, rather than relying solely on Supabase RLS. |
|
|
|
--- |
|
|
|
### S-5 · **MEDIUM** — `.env.example` uses wrong variable prefix |
|
|
|
**File:** `.env.example:1-2` |
|
|
|
``` |
|
VITE_SUPABASE_URL=your_supabase_url |
|
VITE_SUPABASE_ANON_KEY=your_supabase_anon_key |
|
``` |
|
|
|
**Problem:** The example uses `VITE_` prefix but the actual app uses `PUBLIC_` prefix (SvelteKit convention). A developer copying `.env.example` would have a broken setup. |
|
|
|
**Fix:** Update `.env.example`: |
|
``` |
|
PUBLIC_SUPABASE_URL=your_supabase_url |
|
PUBLIC_SUPABASE_ANON_KEY=your_supabase_anon_key |
|
GOOGLE_API_KEY=your_google_api_key |
|
``` |
|
|
|
--- |
|
|
|
### S-6 · **MEDIUM** — Document lock RLS allows any user to delete expired locks |
|
|
|
**File:** `supabase/migrations/016_document_locks.sql:40-41` |
|
|
|
```sql |
|
CREATE POLICY "Anyone can delete expired locks" ON document_locks FOR DELETE |
|
USING (last_heartbeat < now() - interval '60 seconds'); |
|
``` |
|
|
|
**Problem:** Any authenticated user can delete any expired lock. While the intent is correct (cleanup), this could be exploited to race-condition a lock takeover. The `acquireLock` function in `document-locks.ts:75-79` also deletes expired locks client-side, creating two competing cleanup paths. |
|
|
|
**Fix:** Remove the client-side expired lock cleanup from `acquireLock()` and rely solely on the RLS policy, or vice versa. Consider using a server-side cron/function for lock cleanup instead. |
|
|
|
--- |
|
|
|
## 2. Dead & Unused Code |
|
|
|
### ~~D-1~~ RESOLVED — Dead stores deleted |
|
### ~~D-4~~ RESOLVED — `FileTree` removed |
|
### ~~D-5~~ RESOLVED — `$lib/stores/index.ts` and `auth.svelte.ts` deleted |
|
|
|
### D-2 · **HIGH** — `$lib/utils/api-helpers.ts` is never imported |
|
|
|
**File:** `src/lib/utils/api-helpers.ts` (96 lines) |
|
|
|
**Problem:** `unwrap()` and `safeCall()` are well-designed helpers but are never used anywhere. All API modules use manual `if (error) throw error` patterns instead. |
|
|
|
**Fix:** Either delete the file, or migrate API modules to use `unwrap()` (recommended — see finding A-2). |
|
|
|
--- |
|
|
|
### D-3 · **HIGH** — Entire `layout/` component directory is never imported |
|
|
|
**Files:** |
|
- `src/lib/components/layout/PageContainer.svelte` |
|
- `src/lib/components/layout/PageHeader.svelte` |
|
- `src/lib/components/layout/ResponsiveGrid.svelte` |
|
- `src/lib/components/layout/SplitPane.svelte` |
|
- `src/lib/components/layout/index.ts` |
|
|
|
**Problem:** Zero imports of any layout component anywhere in the codebase. |
|
|
|
**Fix:** Delete the entire `src/lib/components/layout/` directory. |
|
|
|
--- |
|
|
|
### D-6 · **MEDIUM** — Unused variables in kanban page |
|
|
|
**File:** `src/routes/[orgSlug]/kanban/+page.svelte:48-49,107` |
|
|
|
```ts |
|
let newBoardVisibility = $state<"team" | "personal">("team"); |
|
let editBoardVisibility = $state<"team" | "personal">("team"); |
|
let sidebarCollapsed = $state(false); |
|
``` |
|
|
|
**Problem:** These three reactive variables are declared but never read or written to anywhere in the template or logic. |
|
|
|
**Fix:** Remove all three declarations. |
|
|
|
--- |
|
|
|
### D-7 · **MEDIUM** — `$lib/index.ts` is an empty placeholder |
|
|
|
**File:** `src/lib/index.ts` |
|
|
|
**Problem:** Empty file with only a comment. Serves no purpose. |
|
|
|
**Fix:** Delete the file. |
|
|
|
--- |
|
|
|
### D-8 · **MEDIUM** — `$lib/supabase/server.ts` is never imported |
|
|
|
**File:** `src/lib/supabase/server.ts` (20 lines) |
|
|
|
**Problem:** Re-exported as `createServerClient` from `$lib/supabase/index.ts`, but never imported anywhere. All server routes use `locals.supabase` from `hooks.server.ts`. |
|
|
|
**Fix:** Delete `src/lib/supabase/server.ts` and remove the re-export from `src/lib/supabase/index.ts`. |
|
|
|
--- |
|
|
|
### D-9 · **LOW** — Demo test and scaffold test are placeholders |
|
|
|
**Files:** |
|
- `src/demo.spec.ts` — `it('adds 1 + 2 to equal 3')` |
|
- `src/routes/page.svelte.spec.ts` |
|
|
|
**Fix:** Delete or replace with real smoke tests. |
|
|
|
--- |
|
|
|
## 3. Type Safety & Correctness |
|
|
|
### T-1 · **HIGH** — Supabase types are stale, causing 66 `as any` casts |
|
|
|
**Files:** 15 files with `as any` casts, concentrated in: |
|
- `src/lib/components/kanban/CardDetailModal.svelte` (9) |
|
- `src/lib/api/document-locks.ts` (6) |
|
- `src/lib/components/documents/FileBrowser.svelte` (6) |
|
- `src/routes/[orgSlug]/documents/[id]/+page.server.ts` (6) |
|
- `src/routes/[orgSlug]/documents/file/[id]/+page.server.ts` (6) |
|
- `src/routes/[orgSlug]/documents/folder/[id]/+page.server.ts` (6) |
|
- `src/routes/[orgSlug]/documents/file/[id]/+page.svelte` (5) |
|
- `src/lib/components/kanban/CardComments.svelte` (4) |
|
- `src/lib/components/kanban/KanbanBoard.svelte` (4) |
|
- `src/routes/api/google-calendar/events/+server.ts` (4) |
|
|
|
**Problem:** The `Database` type in `src/lib/supabase/types.ts` is out of sync with the actual database schema. Missing tables (`document_locks`, `org_google_calendars`, `org_invites`, `org_roles`, `activity_log`), missing columns on existing tables, and incorrect optionality force `as any` everywhere. |
|
|
|
**Fix:** Regenerate types with `supabase gen types typescript --project-id <id> > src/lib/supabase/types.ts`. This single fix will eliminate most of the 66 `as any` casts. |
|
|
|
--- |
|
|
|
### T-2 · **MEDIUM** — `user: any` in root page Props |
|
|
|
**File:** `src/routes/+page.svelte:19` |
|
|
|
```ts |
|
user: any; // ← should be typed |
|
``` |
|
|
|
**Fix:** Use `import type { User } from '@supabase/supabase-js'` and type as `user: User | null`. |
|
|
|
--- |
|
|
|
### T-3 · **MEDIUM** — Local `OrgWithRole` interface duplicates the API type |
|
|
|
**File:** `src/routes/+page.svelte:9-14` |
|
|
|
```ts |
|
interface OrgWithRole { |
|
id: string; name: string; slug: string; role: string; |
|
} |
|
``` |
|
|
|
**Problem:** This is a simplified duplicate of `$lib/api/organizations.OrgWithRole` (which now extends `Organization`). The server load returns full org fields, but this interface drops `avatar_url`, `created_at`, `updated_at`. |
|
|
|
**Fix:** Import and use the canonical type: `import type { OrgWithRole } from '$lib/api/organizations'`. |
|
|
|
--- |
|
|
|
### T-4 · **MEDIUM** — `org` from parent layout is untyped, causing `(org as any).id` everywhere |
|
|
|
**Files:** All `+page.server.ts` files under `[orgSlug]/documents/`, `[orgSlug]/settings/` |
|
|
|
**Problem:** The parent layout returns `org` as the raw Supabase row, but child loads access it as `(org as any).id`, `(org as any).slug` because TypeScript can't infer the parent return type. |
|
|
|
**Fix:** Create a shared type for the org layout data: |
|
```ts |
|
// $lib/types/layout.ts |
|
export interface OrgLayoutData { |
|
org: Organization; |
|
role: string; |
|
userRole: string; |
|
user: User; |
|
members: any[]; |
|
recentActivity: any[]; |
|
} |
|
``` |
|
Then use it in child loads: `const { org } = await parent() as OrgLayoutData;` |
|
|
|
--- |
|
|
|
### T-5 · **MEDIUM** — `folder: any` in folder page Props |
|
|
|
**File:** `src/routes/[orgSlug]/documents/folder/[id]/+page.svelte:8,17,21` |
|
|
|
```ts |
|
folder: any; |
|
const currentFolderId = $derived((data.folder as any).id as string); |
|
<title>{(data.folder as any).name} |
|
``` |
|
|
|
**Problem:** The folder prop is typed as `any`, requiring double casts. This would be resolved by T-1 (regenerating Supabase types) combined with T-4 (typing parent data). |
|
|
|
--- |
|
|
|
## 4. Code Duplication & Redundancy |
|
|
|
### ~~R-1~~ RESOLVED — `FileBrowser` component extracted, both pages are now thin wrappers (~30 lines each) |
|
|
|
### R-2 · **MEDIUM** — Server loads for documents still repeat the same `as any` pattern |
|
|
|
**Files:** |
|
- `src/routes/[orgSlug]/documents/+page.server.ts` |
|
- `src/routes/[orgSlug]/documents/folder/[id]/+page.server.ts` |
|
- `src/routes/[orgSlug]/documents/file/[id]/+page.server.ts` |
|
- `src/routes/[orgSlug]/documents/[id]/+page.server.ts` |
|
|
|
**Problem:** All four files repeat: get parent data, cast `org as any`, query documents table, handle errors. The `as any` casts are identical across all four. |
|
|
|
**Fix:** Create a shared `getOrgFromParent(parent)` utility that types the parent data once. Resolving T-1 + T-4 would also eliminate this. |
|
|
|
--- |
|
|
|
### R-3 · **MEDIUM** — `$lib/supabase/server.ts` duplicates `hooks.server.ts` |
|
|
|
**Problem:** `server.ts` creates a server Supabase client from cookies, but `hooks.server.ts` already does this and puts it on `locals.supabase`. The `server.ts` export is never imported. |
|
|
|
**Fix:** Delete `src/lib/supabase/server.ts` and remove the `createServerClient` re-export from `src/lib/supabase/index.ts`. |
|
|
|
--- |
|
|
|
### R-4 · **LOW** — `role` and `userRole` are the same value returned from org layout |
|
|
|
**File:** `src/routes/[orgSlug]/+layout.server.ts:74-75` |
|
|
|
```ts |
|
role: membership.role, |
|
userRole: membership.role, // kept for backwards compat — same as role |
|
``` |
|
|
|
**Problem:** Both contain `membership.role`. The layout uses `data.role`, child pages use `data.userRole`. The comment acknowledges the duplication. |
|
|
|
**Fix:** Migrate all consumers to `userRole` and remove `role`, or vice versa. |
|
|
|
--- |
|
|
|
## 5. Error Handling & Resilience |
|
|
|
### E-1 · **HIGH** — Settings page uses `alert()` and swallows errors |
|
|
|
**File:** `src/routes/[orgSlug]/settings/+page.svelte` |
|
|
|
- Line 234: `alert("Failed to send invite: " + error.message)` |
|
- Line 447: `alert("Owners cannot leave...")` |
|
- Lines 240, 274, 357, 426, 440: No error handling on `await supabase.from(...).delete()` — errors are silently ignored. |
|
|
|
**Fix:** Use the existing `toasts` store for user feedback. Add error handling to all mutations: |
|
```ts |
|
const { error } = await supabase.from('org_members').delete().eq('id', memberId); |
|
if (error) { toasts.error('Failed to remove member'); return; } |
|
``` |
|
|
|
--- |
|
|
|
### E-2 · **HIGH** — `console.error` used instead of structured logger in 6 files |
|
|
|
**Files (11 instances):** |
|
- `src/routes/api/google-calendar/events/+server.ts` — 4 instances (`console.error`, `console.log`) |
|
- `src/routes/[orgSlug]/calendar/+page.svelte` — 3 instances |
|
- `src/routes/invite/[token]/+page.svelte` — 2 instances |
|
- `src/lib/api/google-calendar.ts` — 1 instance |
|
- `src/routes/[orgSlug]/kanban/+page.svelte` — 1 instance |
|
|
|
**Problem:** The project has a well-designed `createLogger()` system but these modules bypass it with raw `console.*` calls, losing structured context, timestamps, and the ring buffer for error reports. |
|
|
|
**Fix:** Replace all `console.error/log/warn` with `createLogger()` calls. |
|
|
|
--- |
|
|
|
### E-3 · **MEDIUM** — Calendar page `handleDateClick` is a no-op |
|
|
|
**File:** `src/routes/[orgSlug]/calendar/+page.svelte:41-43` |
|
|
|
```ts |
|
function handleDateClick(_date: Date) { |
|
// Event creation disabled |
|
} |
|
``` |
|
|
|
**Fix:** Either implement event creation or remove the click handler from the Calendar component to avoid misleading UX. |
|
|
|
--- |
|
|
|
### E-4 · **MEDIUM** — `data` captured by value causes stale state in multiple pages |
|
|
|
**Files:** |
|
- `src/routes/[orgSlug]/calendar/+page.svelte:26` — `let events = $state(data.events)` |
|
- `src/routes/[orgSlug]/documents/+page.svelte:15` — `let documents = $state(data.documents)` |
|
- `src/routes/[orgSlug]/documents/folder/[id]/+page.svelte:16` — `let documents = $state(data.documents)` |
|
- `src/routes/[orgSlug]/kanban/+page.svelte:40` — `let boards = $state(data.boards)` |
|
- `src/routes/+page.svelte:27` — `let organizations = $state(data.organizations)` |
|
|
|
**Problem:** Svelte warns: "This reference only captures the initial value of `data`." If `data` changes (e.g., via `invalidateAll()`), the local state won't update. |
|
|
|
**Fix:** Add `$effect` blocks to sync: |
|
```ts |
|
let events = $state(data.events); |
|
$effect(() => { events = data.events; }); |
|
``` |
|
|
|
--- |
|
|
|
### E-5 · **MEDIUM** — `releaseLock` called in `onDestroy` may not complete |
|
|
|
**File:** `src/routes/[orgSlug]/documents/file/[id]/+page.svelte:222-225` |
|
|
|
```ts |
|
onDestroy(() => { |
|
// ... |
|
releaseLock(supabase, data.document.id, data.user.id); |
|
}); |
|
``` |
|
|
|
**Problem:** `releaseLock` is async but not awaited. In `onDestroy`, the component is being torn down and the async call may not complete before the page unloads (especially on navigation). The lock would then rely on heartbeat expiry (60s) to be cleaned up. |
|
|
|
**Fix:** Use `beforeunload` event for page unloads and `navigator.sendBeacon` for reliable cleanup: |
|
```ts |
|
window.addEventListener('beforeunload', () => { |
|
navigator.sendBeacon(`/api/release-lock`, JSON.stringify({ documentId, userId })); |
|
}); |
|
``` |
|
|
|
--- |
|
|
|
## 6. Architecture & Structure |
|
|
|
### A-1 · **HIGH** — Settings page is a 1205-line god component |
|
|
|
**File:** `src/routes/[orgSlug]/settings/+page.svelte` (1205 lines) |
|
|
|
**Problem:** This single file handles 5 tabs (General, Members, Roles, Integrations, Appearance), each with their own state, modals, and API calls. It contains ~15 async functions, ~25 state variables, and 6 modals. |
|
|
|
**Fix:** Extract each tab into its own component: |
|
- `SettingsGeneral.svelte` |
|
- `SettingsMembers.svelte` |
|
- `SettingsRoles.svelte` |
|
- `SettingsIntegrations.svelte` |
|
- `SettingsAppearance.svelte` |
|
|
|
--- |
|
|
|
### A-2 · **MEDIUM** — Inconsistent data fetching patterns |
|
|
|
**Problem:** The codebase uses three different patterns for Supabase calls: |
|
1. **API modules** (`$lib/api/*.ts`) — used by kanban page, org overview, document locks |
|
2. **Direct `supabase.from()` in components** — used by settings, FileBrowser, card detail modal, comments |
|
3. **Server loads** — used by all `+page.server.ts` files |
|
|
|
Pattern 2 is problematic because it bypasses the API layer's logging and error handling. The new `FileBrowser` component (872 lines) does all mutations via direct Supabase calls despite `$lib/api/documents.ts` having `createDocument`, `updateDocument`, `moveDocument`, `deleteDocument` functions. |
|
|
|
**Fix:** Migrate `FileBrowser` to use the documents API module. Add missing `kanban` type support to `createDocument`. |
|
|
|
--- |
|
|
|
### A-3 · **MEDIUM** — `createDocument` API doesn't support `kanban` type |
|
|
|
**File:** `src/lib/api/documents.ts:30` |
|
|
|
```ts |
|
type: 'folder' | 'document', // missing 'kanban' |
|
``` |
|
|
|
**Problem:** The function signature only accepts `'folder' | 'document'` but the database and UI support `'kanban'` as a document type. `FileBrowser.handleCreate()` calls Supabase directly to create kanban documents. |
|
|
|
**Fix:** Update the type to `'folder' | 'document' | 'kanban'` and add kanban board creation logic. |
|
|
|
--- |
|
|
|
## 7. Performance |
|
|
|
### P-1 · **HIGH** — Folder page loads ALL org documents on every visit |
|
|
|
**File:** `src/routes/[orgSlug]/documents/folder/[id]/+page.server.ts` |
|
|
|
**Problem:** Every folder page load fetches every document in the entire org (including content blobs via `select('*')`). For orgs with many documents, this is wasteful — content is fetched but only names/types are displayed in the file browser. |
|
|
|
**Fix:** |
|
1. Select only needed columns: `.select('id, name, type, parent_id, created_at, updated_at')` |
|
2. Consider fetching only the current folder's children + ancestors for breadcrumbs. |
|
|
|
--- |
|
|
|
### P-2 · **HIGH** — `fetchBoardWithColumns` makes 3 sequential queries |
|
|
|
**File:** `src/lib/api/kanban.ts:33-86` |
|
|
|
**Problem:** Fetches board, then columns, then cards in 3 separate queries. The columns and cards queries could run in parallel. |
|
|
|
**Fix:** After fetching the board, run columns and cards queries in parallel with `Promise.all`, or use a single query with joins. |
|
|
|
--- |
|
|
|
### P-3 · **MEDIUM** — `moveCard` in API makes N individual UPDATE queries |
|
|
|
**File:** `src/lib/api/kanban.ts:260-271` |
|
|
|
**Problem:** Moving a card fires one UPDATE query per card in the target column. A column with 20 cards = 20 queries. |
|
|
|
**Fix:** Use a Supabase RPC function for batch position updates, or only update cards whose position actually changed. |
|
|
|
--- |
|
|
|
### P-4 · **MEDIUM** — Realtime subscription reloads entire board on any change |
|
|
|
**Files:** |
|
- `src/routes/[orgSlug]/kanban/+page.svelte:64-74` |
|
- `src/routes/[orgSlug]/documents/file/[id]/+page.svelte:199-215` |
|
|
|
**Problem:** Any card or column change (even by the current user) triggers a full board reload (3 queries). Combined with optimistic updates, this can cause UI flicker and wasted bandwidth. |
|
|
|
**Fix:** Use the realtime payload to apply incremental updates instead of full reloads. |
|
|
|
--- |
|
|
|
## 8. Dependency Health |
|
|
|
### DEP-1 · **MEDIUM** — `lucide-svelte` is installed but never imported |
|
|
|
**File:** `package.json:40` |
|
|
|
```json |
|
"lucide-svelte": "^0.563.0" |
|
``` |
|
|
|
**Problem:** Zero imports of `lucide-svelte` anywhere in the codebase. The app uses Google Material Symbols for all icons. |
|
|
|
**Fix:** `npm uninstall lucide-svelte` — saves bundle size. |
|
|
|
--- |
|
|
|
### DEP-2 · **LOW** — `@tailwindcss/forms` may be unused |
|
|
|
**Problem:** The plugin is listed as a devDependency and imported in `layout.css`, but all form elements are custom-styled via the `Input`, `Select`, `Textarea` components. The forms plugin may be adding base styles that are immediately overridden. |
|
|
|
**Fix:** Test removing `@tailwindcss/forms` to see if anything breaks. If not, remove it. |
|
|
|
--- |
|
|
|
## 9. Maintainability & Readability |
|
|
|
### M-1 · **MEDIUM** — Magic numbers for invite expiry |
|
|
|
**File:** `src/routes/[orgSlug]/settings/+page.svelte:220-222` |
|
|
|
```ts |
|
expires_at: new Date(Date.now() + 7 * 24 * 60 * 60 * 1000).toISOString(), |
|
``` |
|
|
|
**Fix:** Extract to a constant: `const INVITE_EXPIRY_MS = 7 * 24 * 60 * 60 * 1000; // 7 days` |
|
|
|
--- |
|
|
|
### M-2 · **MEDIUM** — Inconsistent error feedback patterns |
|
|
|
**Problem:** The codebase uses 4 different patterns for user-facing errors: |
|
1. `alert()` — settings page |
|
2. `toasts.error()` — documents, file viewer |
|
3. `console.error()` only (silent to user) — kanban, calendar, invite pages |
|
4. Inline `error` state variable — login page, invite page |
|
|
|
**Fix:** Standardize on `toasts` for all user-facing error feedback. |
|
|
|
--- |
|
|
|
### M-3 · **LOW** — Inline SVG icons in multiple places |
|
|
|
**Files:** |
|
- `src/routes/+page.svelte:88-98,105-116` — inline SVG for plus icon, people icon |
|
- `src/routes/[orgSlug]/settings/+page.svelte` — inline SVGs for invite, Google, Discord, Slack icons |
|
- `src/routes/invite/[token]/+page.svelte:103-136` — inline SVGs for error/invite icons |
|
|
|
**Fix:** Use `<Icon name="..." />` for Material icons. For brand logos, create a `BrandIcon` component or use static SVG imports. |
|
|
|
--- |
|
|
|
## 10. Future-Proofing |
|
|
|
### F-1 · **MEDIUM** — No permission enforcement beyond role checks |
|
|
|
**Problem:** The settings page defines a granular permission system (`documents.view`, `kanban.create`, `members.manage`, etc.) and roles can have custom permissions. But no code anywhere actually checks these permissions — only `role === 'owner' || role === 'admin'` checks exist. |
|
|
|
**Fix:** Create a `hasPermission(userRole, roles, permission)` utility and use it to gate actions throughout the app. |
|
|
|
--- |
|
|
|
### F-2 · **MEDIUM** — Kanban board subscription listens to ALL card changes |
|
|
|
**File:** `src/lib/api/kanban.ts:291` |
|
|
|
```ts |
|
.on('postgres_changes', { event: '*', schema: 'public', table: 'kanban_cards' }, onCardChange) |
|
``` |
|
|
|
**Problem:** No filter on `column_id` or board. This subscription fires for every card change across all boards in the entire database. |
|
|
|
**Fix:** Add a filter or use a Supabase function to scope the subscription to the current board's column IDs. |
|
|
|
--- |
|
|
|
### F-3 · **LOW** — `@tailwindcss/typography` prose styles may conflict with editor |
|
|
|
**Problem:** The TipTap editor content is wrapped in `.prose` which applies Tailwind Typography styles. These may conflict with the editor's own styling. |
|
|
|
**Fix:** Audit the `.prose` overrides in `layout.css` and consider scoping them more tightly. |
|
|
|
--- |
|
|
|
--- |
|
|
|
## Summary |
|
|
|
### Issues by Severity |
|
|
|
| Severity | Count | |
|
|----------|-------| |
|
| **Critical** | 2 | |
|
| **High** | 7 | |
|
| **Medium** | 17 | |
|
| **Low** | 4 | |
|
| **Total** | **30** | |
|
|
|
### Resolved Since v1 |
|
|
|
| ID | Issue | Resolution | |
|
|----|-------|------------| |
|
| D-1 | 3 dead stores (660 lines) | Deleted (auth, organizations, documents, kanban, theme) | |
|
| D-4 | Unused `FileTree` component | Removed from index and codebase | |
|
| D-5 | Dead `$lib/stores/index.ts` + `auth.svelte.ts` | Deleted | |
|
| R-1 | Documents page duplication (~1900 lines) | Extracted `FileBrowser` component; both pages now ~30 lines | |
|
| R-4 | Kanban store vs inline optimistic updates | Store deleted; inline implementation kept | |
|
| R-6 | `Promise.resolve(null)` placeholder | Layout load refactored | |
|
| — | Calendar `$derived` vs `$derived.by` bug | Fixed (`weeks`, `headerTitle`) | |
|
| — | `buildDocumentTree`/`DocumentWithChildren` dead code | Removed | |
|
| — | Editor CSS typo (`py-1.5bg-background`) | Fixed to `py-1.5 bg-background` | |
|
| — | Invite page wrong routes (`/signup`, `/logout`) | Fixed to `/login?tab=signup`, `/auth/logout` | |
|
| — | KanbanBoard "Add column" mislabel | Fixed to "Add card" | |
|
|
|
### Resolved Since v2 |
|
|
|
| ID | Issue | Resolution | |
|
|----|-------|------------| |
|
| S-2 | Google Calendar API no auth | Added session + org membership check; returns 401/403 | |
|
| S-3 | Auth callback open redirect | Added `safeRedirect()` validator — only allows relative paths | |
|
| S-5 | `.env.example` wrong prefix | Fixed `VITE_` → `PUBLIC_` | |
|
| D-2 | Unused `api-helpers.ts` (96 lines) | Deleted | |
|
| D-3 | Unused `layout/` directory (5 files) | Deleted entire directory | |
|
| D-6 | Unused kanban vars (`newBoardVisibility`, `editBoardVisibility`, `sidebarCollapsed`) | Removed | |
|
| D-7 | Empty `$lib/index.ts` | Deleted | |
|
| D-8 | Unused `$lib/supabase/server.ts` | Deleted; removed re-export from index | |
|
| D-9 | Placeholder tests (`demo.spec.ts`, `page.svelte.spec.ts`) | Deleted | |
|
| DEP-1 | Unused `lucide-svelte` dependency | Uninstalled | |
|
| E-1 | Settings page `alert()` calls | Replaced with `toasts` store | |
|
| E-2 | `console.*` in Google Calendar API route | Replaced with `createLogger()` | |
|
| E-4 | Stale `$state(data.*)` in 5 pages | Added `$effect` sync blocks | |
|
| A-1 | Settings page god component (partial) | Extracted `SettingsGeneral` component; Figma-style pill tab nav | |
|
| R-3 | `server.ts` duplicates `hooks.server.ts` | Deleted (same as D-8) | |
|
|
|
### Top 3 Highest-Impact Remaining Improvements |
|
|
|
1. **Rotate credentials & secure `.env`** (S-1) — Immediate security risk. Takes 15 minutes but prevents catastrophic data breach. **Must be done manually.** |
|
|
|
2. **Regenerate Supabase types** (T-1) — Single command that eliminates 66 `as any` casts, all `never` type errors, and makes the entire codebase type-safe. Also resolves T-4, T-5, and most of R-2. |
|
|
|
3. **Continue splitting settings page** (A-1) — Members, Roles, and Integrations tabs still inline. Extract each into its own component. |
|
|
|
### Suggested Order of Operations (updated) |
|
|
|
1. **Immediate (security):** S-1 (rotate keys — manual), S-4 (server-side auth for settings mutations) |
|
2. **Type safety (1 hour):** T-1 (regenerate Supabase types), T-2→T-5 (fix remaining type issues) |
|
3. **Architecture (1-2 days):** A-1 (finish splitting settings tabs), A-2 (migrate FileBrowser to use API modules), A-3 (add kanban type to createDocument) |
|
4. **Performance (1 day):** P-1 (select only needed columns), P-2 (parallelize kanban queries), P-4 (incremental realtime updates) |
|
5. **Polish:** E-5 (reliable lock release), M-1→M-3 (constants, consistent patterns), F-1 (permission enforcement), F-2 (scoped subscriptions)
|
|
|